Re: [PATCH 2/2] ASoC: tlv320aic3x: Fix Left/Rigth PGA mixer widgets/kcontrols

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 05/27/2014 02:54 PM, Lars-Peter Clausen wrote:
> On 05/27/2014 01:41 PM, Peter Ujfalusi wrote:
>> On 05/27/2014 02:18 PM, Lars-Peter Clausen wrote:
>>> On 05/27/2014 12:53 PM, Peter Ujfalusi wrote:
>>>> Commit "cf7c1de20c576 ASoC: dapm: Move 'value' field from widget to
>>>> control" changed the way how the 'value' has been stored for a widget.
>>>> Since the driver is not updated following the mentioned commit it is mostly
>>>> broken when we try to change mixers for Left/Right PGA since the value is
>>>> not going to be updated correctly.
>>>
>>> I don't think the mentioned commit is the cause. The driver didn't set
>>> widget->value either, so there is no change in behavior here. The commit only
>>> changed where the value is stored not how it is handled.
>>
>> Yes, exactly. The driver has not been updated after the place where the value
>> is stored, so it is not going to store it at all, which causes issues along
>> the way.
> 
> Before the patch the value was stored in widget->value, after the patch the
> value is stored in the kcontrol. The tlv320aic3x driver did not store anything
> in widget->value before the patch and neither does it store anything in the
> kcontrol after the patch.

Sure it is not doing itself. When this custom function has been added it was
essentially a copy of the snd_soc_dapm_put_volsw() - stripped down a bit copy.
But it has not been updated for a long time to follow what is happening the
the core underneath.
I believe not many people uses these paths with the codec and this is why it
has gone unnoticed.
I might attempt to bisect this, but not really sure from which point this is
broken (3.14, 3.12?).

>>> This driver uses a custom put handler and the generic get handler.
>>> dapm_kcontrol_get_value() is used in the generic get handler, but only if reg
>>> equal to SND_SOC_NOPM. Which is not the case for this driver.
>>
>> The relevant code is this:
>>     if (dapm_kcontrol_is_powered(kcontrol) && reg != SND_SOC_NOPM)
>>         val = (snd_soc_read(codec, reg) >> shift) & mask;
>>     else
>>         val = dapm_kcontrol_get_value(kcontrol);
>>
>> and when the widget is not powered the code will try to get the value from the
>> dapm_kcontrol_data, which is not even touched by the custom code in the driver.
> 
> dapm_kcontrol_is_powered() always returns true if the control is not a
> auto-muted control. And there are no auto-muted controls in the tlv320aix3c
> driver.

dapm_kcontrol_is_powered() returns the data->widget->power, which is not the
automute state. Widgets do turn off when they are not in use, like in my system:

# cat tlv320aic3x-codec.0-001b/dapm/Left\ PGA\ Mixer
Left PGA Mixer: Off  in 0 out 0
 out "static" "Left ADC"

So the widget's power is off.

> 
> There are two cases where dapm_kcontrol_get_value() can be called
> a) The control is a virtual control
> b) The control is a auto-muted control
> 
> Neither is the case here, which is why I expect that the real issue is
> something else and it is just shadowed by this patch.
> 
> - Lars


-- 
Péter