[alsa-devel] wm8731 - redundant register initialization, stream name vs DAPM widgets, snd_soc_io to regmap_io conversion
Hi,
I have a few questions regarding wm8731. Some might be applicable to other codecs as well.
1. According to the datasheet bit #8 of all the registers below is set to '0' by the hardware reset. Is there a good reason to set them anyway? (Perhaps an errata in previous chip version?) /* Latch the update bits */ snd_soc_update_bits(codec, WM8731_LOUT1V, 0x100, 0); snd_soc_update_bits(codec, WM8731_ROUT1V, 0x100, 0); snd_soc_update_bits(codec, WM8731_LINVOL, 0x100, 0); snd_soc_update_bits(codec, WM8731_RINVOL, 0x100, 0);
2. In wm8731's snd_soc_dai_driver structure the steam names are 'Playback' and 'Capture', while DAPM widgets have 'HiFi' prepended to the names: SND_SOC_DAPM_DAC("DAC", "HiFi Playback", WM8731_PWR, 3, 1), SND_SOC_DAPM_ADC("ADC", "HiFi Capture", WM8731_PWR, 2, 1), Should the name match? DAPM.txt says so, but the DAPM code actually looks for a substring match (DAI name as a substring in widget name).
3. Do I understand correctly the direction is to move from snd_soc_${io} to more generic regmap_${io}? If so, I'll submit a patch.
Thanks
On Fri, Jun 05, 2015 at 12:27:48PM -0700, Sergey Kiselev wrote:
Hi,
I have a few questions regarding wm8731. Some might be applicable to other codecs as well.
- According to the datasheet bit #8 of all the registers below is set to
'0' by the hardware reset. Is there a good reason to set them anyway? (Perhaps an errata in previous chip version?) /* Latch the update bits */ snd_soc_update_bits(codec, WM8731_LOUT1V, 0x100, 0); snd_soc_update_bits(codec, WM8731_ROUT1V, 0x100, 0); snd_soc_update_bits(codec, WM8731_LINVOL, 0x100, 0); snd_soc_update_bits(codec, WM8731_RINVOL, 0x100, 0);
Hmm... this does actually look like it is probably redundant, it looks like initially these set the simultaneous load bits (commit 40e0aa64660b [ALSA] ASoC codecs: WM8731 support), but this caused problems with the Stereo controls so it was changed in (commit 389619f1063e [ALSA] soc/wm8731: Fix stereo mixer controls). I think it was just overlooked in that second commit that these are the defaults and I would expect that those lines could now be removed with no ill effects.
- In wm8731's snd_soc_dai_driver structure the steam names are 'Playback'
and 'Capture', while DAPM widgets have 'HiFi' prepended to the names: SND_SOC_DAPM_DAC("DAC", "HiFi Playback", WM8731_PWR, 3, 1), SND_SOC_DAPM_ADC("ADC", "HiFi Capture", WM8731_PWR, 2, 1), Should the name match? DAPM.txt says so, but the DAPM code actually looks for a substring match (DAI name as a substring in widget name).
The documentation is fairly out of date here I think and even so it decidely implies a substring match looking at the examples it gives. I might defer to Mark or Lars for the definitive answer on this one.
- Do I understand correctly the direction is to move from snd_soc_${io}
to more generic regmap_${io}? If so, I'll submit a patch.
Don't think I really have a strong opinion on this. I do tend to prefer to use the regmap calls, but I doubt the snd_soc helpers are going away anytime soon.
Thanks, Charles
On 06/08/2015 11:51 AM, Charles Keepax wrote:
On Fri, Jun 05, 2015 at 12:27:48PM -0700, Sergey Kiselev wrote:
Hi,
I have a few questions regarding wm8731. Some might be applicable to other codecs as well.
- According to the datasheet bit #8 of all the registers below is set to
'0' by the hardware reset. Is there a good reason to set them anyway? (Perhaps an errata in previous chip version?) /* Latch the update bits */ snd_soc_update_bits(codec, WM8731_LOUT1V, 0x100, 0); snd_soc_update_bits(codec, WM8731_ROUT1V, 0x100, 0); snd_soc_update_bits(codec, WM8731_LINVOL, 0x100, 0); snd_soc_update_bits(codec, WM8731_RINVOL, 0x100, 0);
Hmm... this does actually look like it is probably redundant, it looks like initially these set the simultaneous load bits (commit 40e0aa64660b [ALSA] ASoC codecs: WM8731 support), but this caused problems with the Stereo controls so it was changed in (commit 389619f1063e [ALSA] soc/wm8731: Fix stereo mixer controls). I think it was just overlooked in that second commit that these are the defaults and I would expect that those lines could now be removed with no ill effects.
- In wm8731's snd_soc_dai_driver structure the steam names are 'Playback'
and 'Capture', while DAPM widgets have 'HiFi' prepended to the names: SND_SOC_DAPM_DAC("DAC", "HiFi Playback", WM8731_PWR, 3, 1), SND_SOC_DAPM_ADC("ADC", "HiFi Capture", WM8731_PWR, 2, 1), Should the name match? DAPM.txt says so, but the DAPM code actually looks for a substring match (DAI name as a substring in widget name).
The documentation is fairly out of date here I think and even so it decidely implies a substring match looking at the examples it gives. I might defer to Mark or Lars for the definitive answer on this one.
It looks as if the code itself always used substring matching.
These days the preferred method for connecting the stream is to directly connect the widget that is created for the stream by the core directly in the DAPM route table to the DAC/ADC widget rather than setting the stream name.
- Do I understand correctly the direction is to move from snd_soc_${io}
to more generic regmap_${io}? If so, I'll submit a patch.
Don't think I really have a strong opinion on this. I do tend to prefer to use the regmap calls, but I doubt the snd_soc helpers are going away anytime soon.
They wont be going to go away, but hopefully soon they'll be direct static inline wrappers around the regmap function. But as long as the driver is consistent it doesn't really matter which functions are used.
- Lars
On Tue, Jun 09, 2015 at 08:24:59PM +0200, Lars-Peter Clausen wrote:
On 06/08/2015 11:51 AM, Charles Keepax wrote:
The documentation is fairly out of date here I think and even so it decidely implies a substring match looking at the examples it gives. I might defer to Mark or Lars for the definitive answer on this one.
It looks as if the code itself always used substring matching.
Yes, that's the case - we've always relied on that.
These days the preferred method for connecting the stream is to directly connect the widget that is created for the stream by the core directly in the DAPM route table to the DAC/ADC widget rather than setting the stream name.
Indeed, that's the appropriate fix if there are any problems (and a useful cleanup even if there aren't).
participants (4)
-
Charles Keepax -
Lars-Peter Clausen -
Mark Brown -
Sergey Kiselev