wm8731 - redundant register initialization, stream name vs DAPM widgets, snd_soc_io to regmap_io conversion

wm8731 - redundant register initialization, stream name vs DAPM widgets, snd_soc_io to regmap_io conversion

[Sergey Kiselev]

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
-- 
Sergey

Re: wm8731 - redundant register initialization, stream name vs DAPM widgets, snd_soc_io to regmap_io conversion

[Charles Keepax]

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.
> 
> 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);

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.

> 
> 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).

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.

> 
> 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.

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

Re: wm8731 - redundant register initialization, stream name vs DAPM widgets, snd_soc_io to regmap_io conversion

[Lars-Peter Clausen]

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.
>>
>> 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);
>
> 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.
>
>>
>> 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).
>
> 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.

>
>>
>> 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.
>
> 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

Re: wm8731 - redundant register initialization, stream name vs DAPM widgets, snd_soc_io to regmap_io conversion

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 797 bytes --]

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).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]