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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
