[alsa-devel] [bug report] ASoC: codecs: ad193x: Fix frame polarity for DSP_A format

[Codrin.Ciubotariu at microchip.com]

On 19.06.2019 17:34, Dan Carpenter wrote:
> External E-Mail
> 
> 
> Hi Codrin,
> 
> Linus recently corrected me on one of my patches and I said I would
> look through the kernel and fix similar bugs as well.  The problem I
> realized is that I'm really stupid and I have forgot how to do math...
> 
> The patch 90f6e6803139: "ASoC: codecs: ad193x: Fix frame polarity for
> DSP_A format" from Feb 18, 2019, leads to the following static
> checker warning:
> 
> 	sound/soc/codecs/ad193x.c:244 ad193x_set_dai_fmt()
> 	warn: passing casted pointer '&dac_fmt' to 'change_bit()' 32 vs 64.
> 
> sound/soc/codecs/ad193x.c
>     194  static int ad193x_set_dai_fmt(struct snd_soc_dai *codec_dai,
>     195                  unsigned int fmt)
>     196  {
>     197          struct ad193x_priv *ad193x = snd_soc_component_get_drvdata(codec_dai->component);
>     198          unsigned int adc_serfmt = 0;
>     199          unsigned int dac_serfmt = 0;
>     200          unsigned int adc_fmt = 0;
>     201          unsigned int dac_fmt = 0;
>                  ^^^^^^^^^^^^^^^^^^^^^^^^
> This is a u32.
> 
>     202
>     203          /* At present, the driver only support AUX ADC mode(SND_SOC_DAIFMT_I2S
>     204           * with TDM), ADC&DAC TDM mode(SND_SOC_DAIFMT_DSP_A) and DAC I2S mode
>     205           * (SND_SOC_DAIFMT_I2S)
>     206           */
>     207          switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>     208          case SND_SOC_DAIFMT_I2S:
>     209                  adc_serfmt |= AD193X_ADC_SERFMT_TDM;
>     210                  dac_serfmt |= AD193X_DAC_SERFMT_STEREO;
>     211                  break;
>     212          case SND_SOC_DAIFMT_DSP_A:
>     213                  adc_serfmt |= AD193X_ADC_SERFMT_AUX;
>     214                  dac_serfmt |= AD193X_DAC_SERFMT_TDM;
>     215                  break;
>     216          default:
>     217                  if (ad193x_has_adc(ad193x))
>     218                          return -EINVAL;
>     219          }
>     220
>     221          switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
>     222          case SND_SOC_DAIFMT_NB_NF: /* normal bit clock + frame */
>     223                  break;
>     224          case SND_SOC_DAIFMT_NB_IF: /* normal bclk + invert frm */
>     225                  adc_fmt |= AD193X_ADC_LEFT_HIGH;
>     226                  dac_fmt |= AD193X_DAC_LEFT_HIGH;
>     227                  break;
>     228          case SND_SOC_DAIFMT_IB_NF: /* invert bclk + normal frm */
>     229                  adc_fmt |= AD193X_ADC_BCLK_INV;
>     230                  dac_fmt |= AD193X_DAC_BCLK_INV;
>     231                  break;
>     232          case SND_SOC_DAIFMT_IB_IF: /* invert bclk + frm */
>     233                  adc_fmt |= AD193X_ADC_LEFT_HIGH;
>     234                  adc_fmt |= AD193X_ADC_BCLK_INV;
>     235                  dac_fmt |= AD193X_DAC_LEFT_HIGH;
>     236                  dac_fmt |= AD193X_DAC_BCLK_INV;
>     237                  break;
>     238          default:
>     239                  return -EINVAL;
>     240          }
>     241
>     242          /* For DSP_*, LRCLK's polarity must be inverted */
>     243          if (fmt & SND_SOC_DAIFMT_DSP_A) {
>     244                  change_bit(ffs(AD193X_DAC_LEFT_HIGH) - 1,
>     245                             (unsigned long *)&dac_fmt);
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This change_bit() will work on little endian systems but on 64 bit big
> endian systems it will corrupt memory.  I *think* the correct code looks
> like this, but again, I'm feeling dumb so I might be wrong:
> 
> 			dac_fmt ^= 1 << (ffs(AD193X_DAC_LEFT_HIGH) - 1);

or just:
dac_fmt ^= AD193X_DAC_LEFT_HIGH;

Thank you for reporting this. I will send a patch to fix it.

Best regards,
Codrin