[PATCH 0/2] ASoC: tlv320aic3x: Fix snd_soc_dapm_put_volsw_aic3x callback

[PATCH 0/2] ASoC: tlv320aic3x: Fix snd_soc_dapm_put_volsw_aic3x callback

[Peter Ujfalusi]

Hi,

Commit "cf7c1de20c576 ASoC: dapm: Move 'value' field from widget to
control" changed the way how the 'value' has been stored for a widget.
This resulted issues with codec drivers needing and using custom dapm put
callbacks to handle the hw underneath, like the tlv320aic3x driver.
Since the driver is not updated following the mentioned commit it is mostly
broken when we try to change Left/Righ PGA mixers:

amixer sset 'Left PGA Mixer Line1L' off
amixer: Invalid command!

The reason for this is that the custom callback was not calling the
dapm_kcontrol_set_value() to store the value which causes inconsistent state
between the HW and dapm.

Regards,
Peter
---
Peter Ujfalusi (2):
  ASoC: dapm: Export dapm_kcontrol_set_value() so it can be used by
    drivers
  ASoC: tlv320aic3x: Fix Left/Rigth PGA mixer widgets/kcontrols

 include/sound/soc-dapm.h       |  3 +++
 sound/soc/codecs/tlv320aic3x.c | 14 +++++++-------
 sound/soc/soc-dapm.c           |  3 ++-
 3 files changed, 12 insertions(+), 8 deletions(-)

-- 
1.9.3

[PATCH 1/2] ASoC: dapm: Export dapm_kcontrol_set_value() so it can be used by drivers

[Peter Ujfalusi]

Drivers needing special put dapm callbacks need to use this function to
update the state of the kcontrol associated with the widget.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/sound/soc-dapm.h | 3 +++
 sound/soc/soc-dapm.c     | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 6b59471cdf44..f31b323fe799 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -440,6 +440,9 @@ void dapm_mark_io_dirty(struct snd_soc_dapm_context *dapm);
 int snd_soc_dapm_dai_get_connected_widgets(struct snd_soc_dai *dai, int stream,
 	struct snd_soc_dapm_widget_list **list);
 
+bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
+	unsigned int value);
+
 struct snd_soc_codec *snd_soc_dapm_kcontrol_codec(struct snd_kcontrol *kcontrol);
 
 /* dapm widget types */
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index a74b9bf23d9f..a3df05d1f22e 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -333,7 +333,7 @@ static unsigned int dapm_kcontrol_get_value(const struct snd_kcontrol *kcontrol)
 	return data->value;
 }
 
-static bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
+bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
 	unsigned int value)
 {
 	struct dapm_kcontrol_data *data = snd_kcontrol_chip(kcontrol);
@@ -348,6 +348,7 @@ static bool dapm_kcontrol_set_value(const struct snd_kcontrol *kcontrol,
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(dapm_kcontrol_set_value);
 
 /**
  * snd_soc_dapm_kcontrol_codec() - Returns the codec associated to a kcontrol
-- 
1.9.3

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

[Peter Ujfalusi]

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.
This can be seen when trying to switch of one of the routes for the first
time (the second try will succeed):
amixer sset 'Left PGA Mixer Line1L' off
amixer: Invalid command!

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/codecs/tlv320aic3x.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index d7349bc89ad3..167c95bdf289 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -156,21 +156,21 @@ static int snd_soc_dapm_put_volsw_aic3x(struct snd_kcontrol *kcontrol,
 	int connect, change;
 
 	val = (ucontrol->value.integer.value[0] & mask);
+	connect = !!val;
+
+	if (invert)
+		val = max - val;
+
+	change = dapm_kcontrol_set_value(kcontrol, val);
 
 	mask = 0xf;
 	if (val)
 		val = mask;
 
-	connect = !!val;
-
-	if (invert)
-		val = mask - val;
-
 	mask <<= shift;
 	val <<= shift;
 
-	change = snd_soc_test_bits(codec, val, mask, reg);
-	if (change) {
+	if (change && snd_soc_test_bits(codec, val, mask, reg)) {
 		update.kcontrol = kcontrol;
 		update.reg = reg;
 		update.mask = mask;
-- 
1.9.3

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

[Lars-Peter Clausen]

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.

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. And that's 
the only place where the value stored for the kcontrol is used. I don't see 
how calling dapm_kcontrol_set_value() or not calling 
dapm_kcontrol_set_value() changes anything.

I might be missing something, but I think the issue is somewhere else.

- Lars

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

[Peter Ujfalusi]

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.

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

> And that's the
> only place where the value stored for the kcontrol is used. I don't see how
> calling dapm_kcontrol_set_value() or not calling dapm_kcontrol_set_value()
> changes anything.

When the widget is not powered the stored value is going to be used. The
custom code in the driver should mimic the code from the core which it is set
to replace/customize.

> 
> I might be missing something, but I think the issue is somewhere else.
> 
> - Lars


-- 
Péter

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

[Lars-Peter Clausen]

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.

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

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

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

[Lars-Peter Clausen]

On 05/27/2014 01: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.
>
>>
>>> 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.
>
> 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.

Picking up on that. What you changed in the patch is that you no longer 
return the return value of snd_soc_test_bits() to the upper layers. amixer 
will print "Invalid command!" if it got a error code. So I'd expect that 
snd_soc_test_bits() returns a error code, can you check that?

- Lars

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

[Peter Ujfalusi]

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

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

[Lars-Peter Clausen]

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

The widget that data->widget points to is not the mixer widget. It is the 
widget that is created for auto-mute handling if the control is auto-muted. 
It will be NULL in your case, which means dapm_kcontrol_is_powered() return 
true.

- Lars

Re: [PATCH 0/2] ASoC: tlv320aic3x: Fix snd_soc_dapm_put_volsw_aic3x callback

[Mark Brown]

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

On Tue, May 27, 2014 at 01:53:04PM +0300, Peter Ujfalusi wrote:
> Hi,
> 
> Commit "cf7c1de20c576 ASoC: dapm: Move 'value' field from widget to
> control" changed the way how the 'value' has been stored for a widget.
> This resulted issues with codec drivers needing and using custom dapm put
> callbacks to handle the hw underneath, like the tlv320aic3x driver.
> Since the driver is not updated following the mentioned commit it is mostly
> broken when we try to change Left/Righ PGA mixers:

Applied both, thanks.

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

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

Re: [PATCH 0/2] ASoC: tlv320aic3x: Fix snd_soc_dapm_put_volsw_aic3x callback

[Peter Ujfalusi]

Mark,

On 05/27/2014 02:02 PM, Mark Brown wrote:
> On Tue, May 27, 2014 at 01:53:04PM +0300, Peter Ujfalusi wrote:
>> Hi,
>>
>> Commit "cf7c1de20c576 ASoC: dapm: Move 'value' field from widget to
>> control" changed the way how the 'value' has been stored for a widget.
>> This resulted issues with codec drivers needing and using custom dapm put
>> callbacks to handle the hw underneath, like the tlv320aic3x driver.
>> Since the driver is not updated following the mentioned commit it is mostly
>> broken when we try to change Left/Righ PGA mixers:
> 
> Applied both, thanks.

After discussing this with Lars over the IRC, would it be possible to drop
these patches? I'll come back with another set to fix this issue tomorrow.

Thanks,
Péter

Re: [PATCH 0/2] ASoC: tlv320aic3x: Fix snd_soc_dapm_put_volsw_aic3x callback

[Lars-Peter Clausen]

On 05/27/2014 03:04 PM, Peter Ujfalusi wrote:
> Mark,
>
> On 05/27/2014 02:02 PM, Mark Brown wrote:
>> On Tue, May 27, 2014 at 01:53:04PM +0300, Peter Ujfalusi wrote:
>>> Hi,
>>>
>>> Commit "cf7c1de20c576 ASoC: dapm: Move 'value' field from widget to
>>> control" changed the way how the 'value' has been stored for a widget.
>>> This resulted issues with codec drivers needing and using custom dapm put
>>> callbacks to handle the hw underneath, like the tlv320aic3x driver.
>>> Since the driver is not updated following the mentioned commit it is mostly
>>> broken when we try to change Left/Righ PGA mixers:
>>
>> Applied both, thanks.
>
> After discussing this with Lars over the IRC, would it be possible to drop
> these patches? I'll come back with another set to fix this issue tomorrow.


After giving some more though to the discussion from yesterday I think it is 
best to just find out why snd_soc_test_bits() returns -EINVAL and fix that 
and leave the rest as it is for now. Moving dapm_kcontrol_set_value() around 
is a bit more complicated than I thought.

- Lars

Re: [PATCH 0/2] ASoC: tlv320aic3x: Fix snd_soc_dapm_put_volsw_aic3x callback

[Peter Ujfalusi]

On 05/28/2014 10:27 AM, Lars-Peter Clausen wrote:
> On 05/27/2014 03:04 PM, Peter Ujfalusi wrote:
>> Mark,
>>
>> On 05/27/2014 02:02 PM, Mark Brown wrote:
>>> On Tue, May 27, 2014 at 01:53:04PM +0300, Peter Ujfalusi wrote:
>>>> Hi,
>>>>
>>>> Commit "cf7c1de20c576 ASoC: dapm: Move 'value' field from widget to
>>>> control" changed the way how the 'value' has been stored for a widget.
>>>> This resulted issues with codec drivers needing and using custom dapm put
>>>> callbacks to handle the hw underneath, like the tlv320aic3x driver.
>>>> Since the driver is not updated following the mentioned commit it is mostly
>>>> broken when we try to change Left/Righ PGA mixers:
>>>
>>> Applied both, thanks.
>>
>> After discussing this with Lars over the IRC, would it be possible to drop
>> these patches? I'll come back with another set to fix this issue tomorrow.
> 
> 
> After giving some more though to the discussion from yesterday I think it is
> best to just find out why snd_soc_test_bits() returns -EINVAL and fix that and
> leave the rest as it is for now. Moving dapm_kcontrol_set_value() around is a
> bit more complicated than I thought.

I was thinking of moving the snd_soc_test_bits() to
snd_soc_dapm_mixer_update_power() and do the checks for the values in the
update. The return from snd_soc_dapm_mixer_update_power() will be true if
there's a change going to be and false when there is no need for the update to
run since the register is already in the same state.

But, yes, I'm looking at the failure, so far I do not see the reason. It
succeed when changing from off to on state and fails first when moving from on
to off, but succeed when trying the on -> off for the second time :o

-- 
Péter