[PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with
invert. In that case, the raw disconnect value should be max, which corresponds
to the userspace value 0.

This use case currently does not appear upstream, but it could break
SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.

Cc: Liam Girdwood <lrg@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: <alsa-devel@alsa-project.org>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../sound/soc/soc-dapm.c                           |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
index 405841c..5ef082f 100644
--- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
+++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
@@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 	int wi;
 
 	val = (ucontrol->value.integer.value[0] & mask);
+	connect = !!val;
 
 	if (invert)
 		val = max - val;
 	mask = mask << shift;
 	val = val << shift;
 
-	if (val)
-		/* new connection */
-		connect = invert ? 0 : 1;
-	else
-		/* old connection must be powered down */
-		connect = invert ? 1 : 0;
-
 	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
 
 	change = snd_soc_test_bits(widget->codec, reg, mask, val);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Mark Brown]

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

On Mon, Jun 18, 2012 at 10:41:28PM +0200, Benoît Thébaudeau wrote:
> snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with
> invert. In that case, the raw disconnect value should be max, which corresponds
> to the userspace value 0.

Applied, thanks.

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

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

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Takashi Iwai]

At Mon, 18 Jun 2012 22:41:28 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with
> invert. In that case, the raw disconnect value should be max, which corresponds
> to the userspace value 0.
> 
> This use case currently does not appear upstream, but it could break
> SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
> 
> Cc: Liam Girdwood <lrg@ti.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: <alsa-devel@alsa-project.org>
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> ---
>  .../sound/soc/soc-dapm.c                           |    8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> index 405841c..5ef082f 100644
> --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
>  	int wi;
>  
>  	val = (ucontrol->value.integer.value[0] & mask);
> +	connect = !!val;
>  
>  	if (invert)
>  		val = max - val;
>  	mask = mask << shift;
>  	val = val << shift;
>  
> -	if (val)
> -		/* new connection */
> -		connect = invert ? 0 : 1;
> -	else
> -		/* old connection must be powered down */
> -		connect = invert ? 1 : 0;
> -

Doesn't this result in the same value of connect?

(given value, invert) --> (raw value, connect)

old code:  (0, 0) --> (0, 0)
           (0, 1) --> (max, 0)
           (max, 0) -> (max, 1)
           (max, 1) -> (0, 1)

new code:  (0, 0) --> (0, 0)
           (0, 1) --> (max, 0)
           (max, 0) --> (max, 1)
           (max, 1) --> (0, 1)

I'd understand if the line "connect = !!val;" were after the invert
conversion...

  	if (invert)
  		val = max - val;
	connect = !!val;


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Mark Brown]

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

On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:

> Doesn't this result in the same value of connect?

If nothing else it's a massive readability improvement...  I've pulled
the change over to 3.6 for now, I need to rush out the door right now
and probably won't be able to do anything else for the rest of today.

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

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

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Takashi Iwai]

At Fri, 29 Jun 2012 08:23:18 +0100,
Mark Brown wrote:
> 
> On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
> 
> > Doesn't this result in the same value of connect?
> 
> If nothing else it's a massive readability improvement...

Yeah, but it's a very secret bonus, not what the changelog says :)

>  I've pulled
> the change over to 3.6 for now, I need to rush out the door right now
> and probably won't be able to do anything else for the rest of today.

OK.


thanks,

Takashi

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
> At Mon, 18 Jun 2012 22:41:28 +0200 (CEST),
> Benoît Thébaudeau wrote:
> > 
> > snd_soc_dapm_put_volsw() sets connect incorrectly in the case max >
> > 1 with
> > invert. In that case, the raw disconnect value should be max, which
> > corresponds
> > to the userspace value 0.
> > 
> > This use case currently does not appear upstream, but it could
> > break
> > SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the
> > future.
> > 
> > Cc: Liam Girdwood <lrg@ti.com>
> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > Cc: <alsa-devel@alsa-project.org>
> > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > ---
> >  .../sound/soc/soc-dapm.c                           |    8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > index 405841c..5ef082f 100644
> > --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct
> > snd_kcontrol *kcontrol,
> >  	int wi;
> >  
> >  	val = (ucontrol->value.integer.value[0] & mask);
> > +	connect = !!val;
> >  
> >  	if (invert)
> >  		val = max - val;
> >  	mask = mask << shift;
> >  	val = val << shift;
> >  
> > -	if (val)
> > -		/* new connection */
> > -		connect = invert ? 0 : 1;
> > -	else
> > -		/* old connection must be powered down */
> > -		connect = invert ? 1 : 0;
> > -
> 
> Doesn't this result in the same value of connect?
> 
> (given value, invert) --> (raw value, connect)
> 
> old code:  (0, 0) --> (0, 0)
>            (0, 1) --> (max, 0)
>            (max, 0) -> (max, 1)
>            (max, 1) -> (0, 1)
> 
> new code:  (0, 0) --> (0, 0)
>            (0, 1) --> (max, 0)
>            (max, 0) --> (max, 1)
>            (max, 1) --> (0, 1)
> 
> I'd understand if the line "connect = !!val;" were after the invert
> conversion...
> 
>   	if (invert)
>   		val = max - val;
> 	connect = !!val;

Take max = 5, invert = 1, user val = 2:
old code: connect = 0
new code: connect = 1

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Takashi Iwai]

At Fri, 29 Jun 2012 13:53:56 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
> > At Mon, 18 Jun 2012 22:41:28 +0200 (CEST),
> > Benoît Thébaudeau wrote:
> > > 
> > > snd_soc_dapm_put_volsw() sets connect incorrectly in the case max >
> > > 1 with
> > > invert. In that case, the raw disconnect value should be max, which
> > > corresponds
> > > to the userspace value 0.
> > > 
> > > This use case currently does not appear upstream, but it could
> > > break
> > > SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the
> > > future.
> > > 
> > > Cc: Liam Girdwood <lrg@ti.com>
> > > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > Cc: <alsa-devel@alsa-project.org>
> > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > > ---
> > >  .../sound/soc/soc-dapm.c                           |    8 +-------
> > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > 
> > > diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > index 405841c..5ef082f 100644
> > > --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct
> > > snd_kcontrol *kcontrol,
> > >  	int wi;
> > >  
> > >  	val = (ucontrol->value.integer.value[0] & mask);
> > > +	connect = !!val;
> > >  
> > >  	if (invert)
> > >  		val = max - val;
> > >  	mask = mask << shift;
> > >  	val = val << shift;
> > >  
> > > -	if (val)
> > > -		/* new connection */
> > > -		connect = invert ? 0 : 1;
> > > -	else
> > > -		/* old connection must be powered down */
> > > -		connect = invert ? 1 : 0;
> > > -
> > 
> > Doesn't this result in the same value of connect?
> > 
> > (given value, invert) --> (raw value, connect)
> > 
> > old code:  (0, 0) --> (0, 0)
> >            (0, 1) --> (max, 0)
> >            (max, 0) -> (max, 1)
> >            (max, 1) -> (0, 1)
> > 
> > new code:  (0, 0) --> (0, 0)
> >            (0, 1) --> (max, 0)
> >            (max, 0) --> (max, 1)
> >            (max, 1) --> (0, 1)
> > 
> > I'd understand if the line "connect = !!val;" were after the invert
> > conversion...
> > 
> >   	if (invert)
> >   		val = max - val;
> > 	connect = !!val;
> 
> Take max = 5, invert = 1, user val = 2:
> old code: connect = 0
> new code: connect = 1

OK, then you need to fix dapm_set_path_status() as well, too.
Otherwise the logic becomes inconsistent.


Takashi

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
> At Fri, 29 Jun 2012 13:53:56 +0200 (CEST),
> Benoît Thébaudeau wrote:
> > 
> > On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
> > > At Mon, 18 Jun 2012 22:41:28 +0200 (CEST),
> > > Benoît Thébaudeau wrote:
> > > > 
> > > > snd_soc_dapm_put_volsw() sets connect incorrectly in the case
> > > > max >
> > > > 1 with
> > > > invert. In that case, the raw disconnect value should be max,
> > > > which
> > > > corresponds
> > > > to the userspace value 0.
> > > > 
> > > > This use case currently does not appear upstream, but it could
> > > > break
> > > > SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the
> > > > future.
> > > > 
> > > > Cc: Liam Girdwood <lrg@ti.com>
> > > > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > > Cc: <alsa-devel@alsa-project.org>
> > > > Signed-off-by: Benoît Thébaudeau
> > > > <benoit.thebaudeau@advansee.com>
> > > > ---
> > > >  .../sound/soc/soc-dapm.c                           |    8
> > > >  +-------
> > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > 
> > > > diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > index 405841c..5ef082f 100644
> > > > --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct
> > > > snd_kcontrol *kcontrol,
> > > >  	int wi;
> > > >  
> > > >  	val = (ucontrol->value.integer.value[0] & mask);
> > > > +	connect = !!val;
> > > >  
> > > >  	if (invert)
> > > >  		val = max - val;
> > > >  	mask = mask << shift;
> > > >  	val = val << shift;
> > > >  
> > > > -	if (val)
> > > > -		/* new connection */
> > > > -		connect = invert ? 0 : 1;
> > > > -	else
> > > > -		/* old connection must be powered down */
> > > > -		connect = invert ? 1 : 0;
> > > > -
> > > 
> > > Doesn't this result in the same value of connect?
> > > 
> > > (given value, invert) --> (raw value, connect)
> > > 
> > > old code:  (0, 0) --> (0, 0)
> > >            (0, 1) --> (max, 0)
> > >            (max, 0) -> (max, 1)
> > >            (max, 1) -> (0, 1)
> > > 
> > > new code:  (0, 0) --> (0, 0)
> > >            (0, 1) --> (max, 0)
> > >            (max, 0) --> (max, 1)
> > >            (max, 1) --> (0, 1)
> > > 
> > > I'd understand if the line "connect = !!val;" were after the
> > > invert
> > > conversion...
> > > 
> > >   	if (invert)
> > >   		val = max - val;
> > > 	connect = !!val;
> > 
> > Take max = 5, invert = 1, user val = 2:
> > old code: connect = 0
> > new code: connect = 1
> 
> OK, then you need to fix dapm_set_path_status() as well, too.
> Otherwise the logic becomes inconsistent.

Indeed, I missed that. But the issue is even worse in this function: It uses the
control register value to determine if connect should be set while only the
userspace value can tell that, and it has no way of deriving the userspace value
apart from calling the get function, while here it assumes that the register
value will be more or less compatible with snd_soc_dapm_get_volsw.

Hence, I think that the fix here should be to call get, then to deduce connect
from the returned value. Do you agree?

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Takashi Iwai]

At Fri, 29 Jun 2012 16:29:22 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
> > At Fri, 29 Jun 2012 13:53:56 +0200 (CEST),
> > Benoît Thébaudeau wrote:
> > > 
> > > On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
> > > > At Mon, 18 Jun 2012 22:41:28 +0200 (CEST),
> > > > Benoît Thébaudeau wrote:
> > > > > 
> > > > > snd_soc_dapm_put_volsw() sets connect incorrectly in the case
> > > > > max >
> > > > > 1 with
> > > > > invert. In that case, the raw disconnect value should be max,
> > > > > which
> > > > > corresponds
> > > > > to the userspace value 0.
> > > > > 
> > > > > This use case currently does not appear upstream, but it could
> > > > > break
> > > > > SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the
> > > > > future.
> > > > > 
> > > > > Cc: Liam Girdwood <lrg@ti.com>
> > > > > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > > > Cc: <alsa-devel@alsa-project.org>
> > > > > Signed-off-by: Benoît Thébaudeau
> > > > > <benoit.thebaudeau@advansee.com>
> > > > > ---
> > > > >  .../sound/soc/soc-dapm.c                           |    8
> > > > >  +-------
> > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > > 
> > > > > diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > > linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > > index 405841c..5ef082f 100644
> > > > > --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > > +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > > @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct
> > > > > snd_kcontrol *kcontrol,
> > > > >  	int wi;
> > > > >  
> > > > >  	val = (ucontrol->value.integer.value[0] & mask);
> > > > > +	connect = !!val;
> > > > >  
> > > > >  	if (invert)
> > > > >  		val = max - val;
> > > > >  	mask = mask << shift;
> > > > >  	val = val << shift;
> > > > >  
> > > > > -	if (val)
> > > > > -		/* new connection */
> > > > > -		connect = invert ? 0 : 1;
> > > > > -	else
> > > > > -		/* old connection must be powered down */
> > > > > -		connect = invert ? 1 : 0;
> > > > > -
> > > > 
> > > > Doesn't this result in the same value of connect?
> > > > 
> > > > (given value, invert) --> (raw value, connect)
> > > > 
> > > > old code:  (0, 0) --> (0, 0)
> > > >            (0, 1) --> (max, 0)
> > > >            (max, 0) -> (max, 1)
> > > >            (max, 1) -> (0, 1)
> > > > 
> > > > new code:  (0, 0) --> (0, 0)
> > > >            (0, 1) --> (max, 0)
> > > >            (max, 0) --> (max, 1)
> > > >            (max, 1) --> (0, 1)
> > > > 
> > > > I'd understand if the line "connect = !!val;" were after the
> > > > invert
> > > > conversion...
> > > > 
> > > >   	if (invert)
> > > >   		val = max - val;
> > > > 	connect = !!val;
> > > 
> > > Take max = 5, invert = 1, user val = 2:
> > > old code: connect = 0
> > > new code: connect = 1
> > 
> > OK, then you need to fix dapm_set_path_status() as well, too.
> > Otherwise the logic becomes inconsistent.
> 
> Indeed, I missed that. But the issue is even worse in this function: It uses the
> control register value to determine if connect should be set while only the
> userspace value can tell that, and it has no way of deriving the userspace value
> apart from calling the get function, while here it assumes that the register
> value will be more or less compatible with snd_soc_dapm_get_volsw.

That's a valid assumption.  Usually get and put callbacks must be
paired well.

> Hence, I think that the fix here should be to call get, then to deduce connect
> from the returned value. Do you agree?

Well, calling a control callback internally is a bit worrisome.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Takashi Iwai]

At Fri, 29 Jun 2012 17:43:09 +0200,
Takashi Iwai wrote:
> 
> At Fri, 29 Jun 2012 16:29:22 +0200 (CEST),
> Benoît Thébaudeau wrote:
> > 
> > On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
> > > At Fri, 29 Jun 2012 13:53:56 +0200 (CEST),
> > > Benoît Thébaudeau wrote:
> > > > 
> > > > On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
> > > > > At Mon, 18 Jun 2012 22:41:28 +0200 (CEST),
> > > > > Benoît Thébaudeau wrote:
> > > > > > 
> > > > > > snd_soc_dapm_put_volsw() sets connect incorrectly in the case
> > > > > > max >
> > > > > > 1 with
> > > > > > invert. In that case, the raw disconnect value should be max,
> > > > > > which
> > > > > > corresponds
> > > > > > to the userspace value 0.
> > > > > > 
> > > > > > This use case currently does not appear upstream, but it could
> > > > > > break
> > > > > > SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the
> > > > > > future.
> > > > > > 
> > > > > > Cc: Liam Girdwood <lrg@ti.com>
> > > > > > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > > > > Cc: <alsa-devel@alsa-project.org>
> > > > > > Signed-off-by: Benoît Thébaudeau
> > > > > > <benoit.thebaudeau@advansee.com>
> > > > > > ---
> > > > > >  .../sound/soc/soc-dapm.c                           |    8
> > > > > >  +-------
> > > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > > > 
> > > > > > diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > > > linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > > > index 405841c..5ef082f 100644
> > > > > > --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > > > +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > > > @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct
> > > > > > snd_kcontrol *kcontrol,
> > > > > >  	int wi;
> > > > > >  
> > > > > >  	val = (ucontrol->value.integer.value[0] & mask);
> > > > > > +	connect = !!val;
> > > > > >  
> > > > > >  	if (invert)
> > > > > >  		val = max - val;
> > > > > >  	mask = mask << shift;
> > > > > >  	val = val << shift;
> > > > > >  
> > > > > > -	if (val)
> > > > > > -		/* new connection */
> > > > > > -		connect = invert ? 0 : 1;
> > > > > > -	else
> > > > > > -		/* old connection must be powered down */
> > > > > > -		connect = invert ? 1 : 0;
> > > > > > -
> > > > > 
> > > > > Doesn't this result in the same value of connect?
> > > > > 
> > > > > (given value, invert) --> (raw value, connect)
> > > > > 
> > > > > old code:  (0, 0) --> (0, 0)
> > > > >            (0, 1) --> (max, 0)
> > > > >            (max, 0) -> (max, 1)
> > > > >            (max, 1) -> (0, 1)
> > > > > 
> > > > > new code:  (0, 0) --> (0, 0)
> > > > >            (0, 1) --> (max, 0)
> > > > >            (max, 0) --> (max, 1)
> > > > >            (max, 1) --> (0, 1)
> > > > > 
> > > > > I'd understand if the line "connect = !!val;" were after the
> > > > > invert
> > > > > conversion...
> > > > > 
> > > > >   	if (invert)
> > > > >   		val = max - val;
> > > > > 	connect = !!val;
> > > > 
> > > > Take max = 5, invert = 1, user val = 2:
> > > > old code: connect = 0
> > > > new code: connect = 1
> > > 
> > > OK, then you need to fix dapm_set_path_status() as well, too.
> > > Otherwise the logic becomes inconsistent.
> > 
> > Indeed, I missed that. But the issue is even worse in this function: It uses the
> > control register value to determine if connect should be set while only the
> > userspace value can tell that, and it has no way of deriving the userspace value
> > apart from calling the get function, while here it assumes that the register
> > value will be more or less compatible with snd_soc_dapm_get_volsw.
> 
> That's a valid assumption.  Usually get and put callbacks must be
> paired well.
> 
> > Hence, I think that the fix here should be to call get, then to deduce connect
> > from the returned value. Do you agree?
> 
> Well, calling a control callback internally is a bit worrisome.

BTW, looking at snd_soc_dapm_vol_getsw(), I found that
snd_soc_dapm_vol_putsw() doesn't handle a stereo case.
This needs a fix, too...


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

On Fri, Jun 29, 2012 at 05:44:41PM +0200, Takashi Iwai wrote:
> BTW, looking at snd_soc_dapm_vol_getsw(), I found that
> snd_soc_dapm_vol_putsw() doesn't handle a stereo case.
> This needs a fix, too...

I saw that too. Is it the stereo case that should be added to put or removed
from get, i.e. is it possible to handle a stereo case with only one connect
flag?

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Takashi Iwai]

At Fri, 29 Jun 2012 18:09:35 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> On Fri, Jun 29, 2012 at 05:44:41PM +0200, Takashi Iwai wrote:
> > BTW, looking at snd_soc_dapm_vol_getsw(), I found that
> > snd_soc_dapm_vol_putsw() doesn't handle a stereo case.
> > This needs a fix, too...
> 
> I saw that too. Is it the stereo case that should be added to put or removed
> from get, i.e. is it possible to handle a stereo case with only one connect
> flag?

I would assume that connect should be up when either left or right is
on.  A stereo widget doesn't mean multiple connections.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

On Fri, Jun 29, 2012 at 06:22:28PM +0200, Takashi Iwai wrote:
> At Fri, 29 Jun 2012 18:09:35 +0200 (CEST),
> Benoît Thébaudeau wrote:
> > 
> > On Fri, Jun 29, 2012 at 05:44:41PM +0200, Takashi Iwai wrote:
> > > BTW, looking at snd_soc_dapm_vol_getsw(), I found that
> > > snd_soc_dapm_vol_putsw() doesn't handle a stereo case.
> > > This needs a fix, too...
> > 
> > I saw that too. Is it the stereo case that should be added to put
> > or removed
> > from get, i.e. is it possible to handle a stereo case with only one
> > connect
> > flag?
> 
> I would assume that connect should be up when either left or right is
> on.  A stereo widget doesn't mean multiple connections.

I have started to look into that:
 - Give a choice between reg and shift stereo for snd_soc_dapm_get_volsw(), like
   in snd_soc_get_volsw().
 - Add stereo to the first case of dapm_set_path_status().
 - Add stereo to dapm_widget_update() and struct snd_soc_dapm_update.
 - Then comes snd_soc_dapm_get_volsw(): What should we do with
   "widget->value = val;"? Add an rvalue to struct snd_soc_dapm_widget? This
   field does not seem to be read anywhere except for enums. There is also
   saved_value and other fields that do not seem to be used anywhere.

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

On Fri, Jun 29, 2012 at 09:09:49PM +0200, Benoît Thébaudeau wrote:
> On Fri, Jun 29, 2012 at 06:22:28PM +0200, Takashi Iwai wrote:
> > At Fri, 29 Jun 2012 18:09:35 +0200 (CEST),
> > Benoît Thébaudeau wrote:
> > > 
> > > On Fri, Jun 29, 2012 at 05:44:41PM +0200, Takashi Iwai wrote:
> > > > BTW, looking at snd_soc_dapm_vol_getsw(), I found that
> > > > snd_soc_dapm_vol_putsw() doesn't handle a stereo case.
> > > > This needs a fix, too...
> > > 
> > > I saw that too. Is it the stereo case that should be added to put
> > > or removed
> > > from get, i.e. is it possible to handle a stereo case with only
> > > one
> > > connect
> > > flag?
> > 
> > I would assume that connect should be up when either left or right
> > is
> > on.  A stereo widget doesn't mean multiple connections.
> 
> I have started to look into that:
>  - Give a choice between reg and shift stereo for
>  snd_soc_dapm_get_volsw(), like
>    in snd_soc_get_volsw().
>  - Add stereo to the first case of dapm_set_path_status().
>  - Add stereo to dapm_widget_update() and struct snd_soc_dapm_update.
>  - Then comes snd_soc_dapm_get_volsw(): What should we do with
>    "widget->value = val;"? Add an rvalue to struct
>    snd_soc_dapm_widget? This
>    field does not seem to be read anywhere except for enums. There is
>    also
>    saved_value and other fields that do not seem to be used anywhere.

I've finished. The following patch should be applied after my patch
"ASoC: dapm: Fix dapm_set_path_status() connect".

Regards,
Benoît


[PATCH] ASoC: dapm: Fix/add support for stereo widgets

This patch:
* adds a choice in snd_soc_dapm_get_volsw_mut() for stereo between 1 and 2
  registers, like in snd_soc_get_volsw().
* fixes the missing stereo in other parts of dapm.
* removes the unused saved_value from struct snd_soc_dapm_widget.

Cc: Liam Girdwood <lrg@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: <alsa-devel@alsa-project.org>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../include/sound/soc-dapm.h                       |   10 +--
 .../sound/soc/soc-dapm.c                           |   92 ++++++++++++++++----
 2 files changed, 82 insertions(+), 20 deletions(-)

diff --git linux-next-HEAD-d1d2d3a.orig/include/sound/soc-dapm.h linux-next-HEAD-d1d2d3a/include/sound/soc-dapm.h
index 05559e5..b5d38b9 100644
--- linux-next-HEAD-d1d2d3a.orig/include/sound/soc-dapm.h
+++ linux-next-HEAD-d1d2d3a/include/sound/soc-dapm.h
@@ -508,8 +508,8 @@ struct snd_soc_dapm_widget {
 	/* dapm control */
 	int reg;				/* negative reg = no direct dapm */
 	unsigned char shift;			/* bits to shift */
-	unsigned int saved_value;		/* widget saved value */
-	unsigned int value;				/* widget current value */
+	unsigned int value;			/* widget current value */
+	unsigned int rvalue;			/* widget current value of right channel */
 	unsigned int mask;			/* non-shifted mask */
 	unsigned int on_val;			/* on state value */
 	unsigned int off_val;			/* off state value */
@@ -552,9 +552,9 @@ struct snd_soc_dapm_widget {
 struct snd_soc_dapm_update {
 	struct snd_soc_dapm_widget *widget;
 	struct snd_kcontrol *kcontrol;
-	int reg;
-	int mask;
-	int val;
+	int reg, rreg;
+	int mask, rmask;
+	int val, rval;
 };
 
 /* DAPM context */
diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
index 7f2a4bb..341dade 100644
--- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c
+++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
@@ -313,11 +313,13 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w,
 	case snd_soc_dapm_switch:
 	case snd_soc_dapm_mixer:
 	case snd_soc_dapm_mixer_named_ctl: {
-		int val;
+		int val, val2 = 0;
 		struct soc_mixer_control *mc = (struct soc_mixer_control *)
 			w->kcontrol_news[i].private_value;
 		unsigned int reg = mc->reg;
+		unsigned int reg2 = mc->rreg;
 		unsigned int shift = mc->shift;
+		unsigned int rshift = mc->rshift;
 		int max = mc->max;
 		unsigned int mask = (1 << fls(max)) - 1;
 		unsigned int invert = mc->invert;
@@ -327,7 +329,19 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w,
 		if (invert)
 			val = max - val;
 
-		p->connect = !!val;
+		if (snd_soc_volsw_is_stereo(mc)) {
+			if (reg == reg2) {
+				val2 = soc_widget_read(w, reg);
+				val2 = (val2 >> rshift) & mask;
+			} else {
+				val2 = soc_widget_read(w, reg2);
+				val2 = (val2 >> shift) & mask;
+			}
+			if (invert)
+				val2 = max - val2;
+		}
+
+		p->connect = val || val2;
 	}
 	break;
 	case snd_soc_dapm_mux: {
@@ -1397,6 +1411,14 @@ static void dapm_widget_update(struct snd_soc_dapm_context *dapm)
 	if (ret < 0)
 		pr_err("%s DAPM update failed: %d\n", w->name, ret);
 
+	if (update->rmask) {
+		ret = soc_widget_update_bits_locked(w, update->rreg,
+					  update->rmask, update->rval);
+		if (ret < 0)
+			pr_err("%s DAPM right channel update failed: %d\n",
+			       w->name, ret);
+	}
+
 	if (w->event &&
 	    (w->event_flags & SND_SOC_DAPM_POST_REG)) {
 		ret = w->event(w, update->kcontrol, SND_SOC_DAPM_POST_REG);
@@ -2462,21 +2484,29 @@ int snd_soc_dapm_get_volsw(struct snd_kcontrol *kcontrol,
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
+	unsigned int reg2 = mc->rreg;
 	unsigned int shift = mc->shift;
 	unsigned int rshift = mc->rshift;
 	int max = mc->max;
-	unsigned int invert = mc->invert;
 	unsigned int mask = (1 << fls(max)) - 1;
+	unsigned int invert = mc->invert;
 
 	ucontrol->value.integer.value[0] =
 		(snd_soc_read(widget->codec, reg) >> shift) & mask;
-	if (shift != rshift)
-		ucontrol->value.integer.value[1] =
-			(snd_soc_read(widget->codec, reg) >> rshift) & mask;
-	if (invert) {
+	if (invert)
 		ucontrol->value.integer.value[0] =
 			max - ucontrol->value.integer.value[0];
-		if (shift != rshift)
+
+	if (snd_soc_volsw_is_stereo(mc)) {
+		if (reg == reg2)
+			ucontrol->value.integer.value[1] =
+				(snd_soc_read(widget->codec, reg) >> rshift) &
+						mask;
+		else
+			ucontrol->value.integer.value[1] =
+				(snd_soc_read(widget->codec, reg2) >> shift) &
+						mask;
+		if (invert)
 			ucontrol->value.integer.value[1] =
 				max - ucontrol->value.integer.value[1];
 	}
@@ -2504,11 +2534,15 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
+	unsigned int reg2 = mc->rreg;
 	unsigned int shift = mc->shift;
+	unsigned int rshift = mc->rshift;
 	int max = mc->max;
 	unsigned int mask = (1 << fls(max)) - 1;
 	unsigned int invert = mc->invert;
-	unsigned int val;
+	bool type_2r = 0;
+	unsigned int val2 = 0;
+	unsigned int val, val_mask;
 	int connect, change;
 	struct snd_soc_dapm_update update;
 	int wi;
@@ -2518,27 +2552,53 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 
 	if (invert)
 		val = max - val;
-	mask = mask << shift;
+	val_mask = mask << shift;
 	val = val << shift;
 
+	if (snd_soc_volsw_is_stereo(mc)) {
+		val2 = (ucontrol->value.integer.value[1] & mask);
+		connect |= !!val2;
+
+		if (invert)
+			val2 = max - val2;
+		if (reg == reg2) {
+			val_mask |= mask << rshift;
+			val |= val2 << rshift;
+		} else {
+			val2 = val2 << shift;
+			type_2r = 1;
+		}
+	}
+
 	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
 
-	change = snd_soc_test_bits(widget->codec, reg, mask, val);
+	change = snd_soc_test_bits(widget->codec, reg, val_mask, val);
+	if (type_2r)
+		change |= snd_soc_test_bits(widget->codec,
+					    reg2, val_mask, val2);
+
 	if (change) {
 		for (wi = 0; wi < wlist->num_widgets; wi++) {
 			widget = wlist->widgets[wi];
 
 			widget->value = val;
-
 			update.kcontrol = kcontrol;
 			update.widget = widget;
 			update.reg = reg;
-			update.mask = mask;
+			update.mask = val_mask;
 			update.val = val;
-			widget->dapm->update = &update;
 
-			soc_dapm_mixer_update_power(widget, kcontrol, connect);
+			if (type_2r) {
+				widget->rvalue = val2;
+				update.rreg = reg2;
+				update.rmask = val_mask;
+				update.rval = val2;
+			} else {
+				update.rmask = 0;
+			}
 
+			widget->dapm->update = &update;
+			soc_dapm_mixer_update_power(widget, kcontrol, connect);
 			widget->dapm->update = NULL;
 		}
 	}
@@ -2627,6 +2687,7 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 			update.reg = e->reg;
 			update.mask = mask;
 			update.val = val;
+			update.rmask = 0;
 			widget->dapm->update = &update;
 
 			soc_dapm_mux_update_power(widget, kcontrol, mux, e);
@@ -2793,6 +2854,7 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 			update.reg = e->reg;
 			update.mask = mask;
 			update.val = val;
+			update.rmask = 0;
 			widget->dapm->update = &update;
 
 			soc_dapm_mux_update_power(widget, kcontrol, mux, e);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Mark Brown]

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

On Fri, Jun 29, 2012 at 10:18:07PM +0200, Benoît Thébaudeau wrote:

> I've finished. The following patch should be applied after my patch
> "ASoC: dapm: Fix dapm_set_path_status() connect".

Don't submit patches in the middle of discussions, but please bear in
mind what I just said about not doing stereo.

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

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

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

On Sat, Jun 30, 2012 at 01:39:50PM +0200, Mark Brown wrote:
> On Fri, Jun 29, 2012 at 10:18:07PM +0200, Benoît Thébaudeau wrote:
> 
> > I've finished. The following patch should be applied after my patch
> > "ASoC: dapm: Fix dapm_set_path_status() connect".
> 
> Don't submit patches in the middle of discussions, but please bear in
> mind what I just said about not doing stereo.

> Actually I'd really expect it to mean exactly that.  DAPM is fairly
> fundamentally dealing with single channel audio streams and I'd expect
> that if we did implement stereo controls they'd allow these streams to
> pass through without getting merged.  There's a couple of things that
> stop us doing that right now but I don't think we should add stereo
> controls that don't do that.

Can you be more specific about what stops us doing that right now?

Should we drop this idea for now and simply remove stereo from
snd_soc_dapm_get_volsw()?

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Mark Brown]

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

On Sat, Jun 30, 2012 at 03:03:09PM +0200, Benoît Thébaudeau wrote:
> On Sat, Jun 30, 2012 at 01:39:50PM +0200, Mark Brown wrote:

> > Actually I'd really expect it to mean exactly that.  DAPM is fairly
> > fundamentally dealing with single channel audio streams and I'd expect
> > that if we did implement stereo controls they'd allow these streams to
> > pass through without getting merged.  There's a couple of things that
> > stop us doing that right now but I don't think we should add stereo
> > controls that don't do that.

> Can you be more specific about what stops us doing that right now?

Nothing if someone wants to implement it.

> Should we drop this idea for now and simply remove stereo from
> snd_soc_dapm_get_volsw()?

It's going to be easier to remove the code if we're going to do anything
at all, though there's no great urgency.

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

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

[RFC/PATCH] ASoC: dapm: Fix/add support for stereo widgets

[Benoît Thébaudeau]

This patch:
* adds a choice in snd_soc_dapm_get_volsw_mut() for stereo between 1 and 2
  registers, like in snd_soc_get_volsw().
* fixes the missing stereo in other parts of dapm.
* removes the unused saved_value from struct snd_soc_dapm_widget.

Cc: Liam Girdwood <lrg@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: <alsa-devel@alsa-project.org>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../include/sound/soc-dapm.h                       |   10 +--
 .../sound/soc/soc-dapm.c                           |   92 ++++++++++++++++----
 2 files changed, 82 insertions(+), 20 deletions(-)

diff --git linux-next-HEAD-d1d2d3a.orig/include/sound/soc-dapm.h linux-next-HEAD-d1d2d3a/include/sound/soc-dapm.h
index 05559e5..b5d38b9 100644
--- linux-next-HEAD-d1d2d3a.orig/include/sound/soc-dapm.h
+++ linux-next-HEAD-d1d2d3a/include/sound/soc-dapm.h
@@ -508,8 +508,8 @@ struct snd_soc_dapm_widget {
 	/* dapm control */
 	int reg;				/* negative reg = no direct dapm */
 	unsigned char shift;			/* bits to shift */
-	unsigned int saved_value;		/* widget saved value */
-	unsigned int value;				/* widget current value */
+	unsigned int value;			/* widget current value */
+	unsigned int rvalue;			/* widget current value of right channel */
 	unsigned int mask;			/* non-shifted mask */
 	unsigned int on_val;			/* on state value */
 	unsigned int off_val;			/* off state value */
@@ -552,9 +552,9 @@ struct snd_soc_dapm_widget {
 struct snd_soc_dapm_update {
 	struct snd_soc_dapm_widget *widget;
 	struct snd_kcontrol *kcontrol;
-	int reg;
-	int mask;
-	int val;
+	int reg, rreg;
+	int mask, rmask;
+	int val, rval;
 };
 
 /* DAPM context */
diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
index 7f2a4bb..341dade 100644
--- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c
+++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
@@ -313,11 +313,13 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w,
 	case snd_soc_dapm_switch:
 	case snd_soc_dapm_mixer:
 	case snd_soc_dapm_mixer_named_ctl: {
-		int val;
+		int val, val2 = 0;
 		struct soc_mixer_control *mc = (struct soc_mixer_control *)
 			w->kcontrol_news[i].private_value;
 		unsigned int reg = mc->reg;
+		unsigned int reg2 = mc->rreg;
 		unsigned int shift = mc->shift;
+		unsigned int rshift = mc->rshift;
 		int max = mc->max;
 		unsigned int mask = (1 << fls(max)) - 1;
 		unsigned int invert = mc->invert;
@@ -327,7 +329,19 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w,
 		if (invert)
 			val = max - val;
 
-		p->connect = !!val;
+		if (snd_soc_volsw_is_stereo(mc)) {
+			if (reg == reg2) {
+				val2 = soc_widget_read(w, reg);
+				val2 = (val2 >> rshift) & mask;
+			} else {
+				val2 = soc_widget_read(w, reg2);
+				val2 = (val2 >> shift) & mask;
+			}
+			if (invert)
+				val2 = max - val2;
+		}
+
+		p->connect = val || val2;
 	}
 	break;
 	case snd_soc_dapm_mux: {
@@ -1397,6 +1411,14 @@ static void dapm_widget_update(struct snd_soc_dapm_context *dapm)
 	if (ret < 0)
 		pr_err("%s DAPM update failed: %d\n", w->name, ret);
 
+	if (update->rmask) {
+		ret = soc_widget_update_bits_locked(w, update->rreg,
+					  update->rmask, update->rval);
+		if (ret < 0)
+			pr_err("%s DAPM right channel update failed: %d\n",
+			       w->name, ret);
+	}
+
 	if (w->event &&
 	    (w->event_flags & SND_SOC_DAPM_POST_REG)) {
 		ret = w->event(w, update->kcontrol, SND_SOC_DAPM_POST_REG);
@@ -2462,21 +2484,29 @@ int snd_soc_dapm_get_volsw(struct snd_kcontrol *kcontrol,
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
+	unsigned int reg2 = mc->rreg;
 	unsigned int shift = mc->shift;
 	unsigned int rshift = mc->rshift;
 	int max = mc->max;
-	unsigned int invert = mc->invert;
 	unsigned int mask = (1 << fls(max)) - 1;
+	unsigned int invert = mc->invert;
 
 	ucontrol->value.integer.value[0] =
 		(snd_soc_read(widget->codec, reg) >> shift) & mask;
-	if (shift != rshift)
-		ucontrol->value.integer.value[1] =
-			(snd_soc_read(widget->codec, reg) >> rshift) & mask;
-	if (invert) {
+	if (invert)
 		ucontrol->value.integer.value[0] =
 			max - ucontrol->value.integer.value[0];
-		if (shift != rshift)
+
+	if (snd_soc_volsw_is_stereo(mc)) {
+		if (reg == reg2)
+			ucontrol->value.integer.value[1] =
+				(snd_soc_read(widget->codec, reg) >> rshift) &
+						mask;
+		else
+			ucontrol->value.integer.value[1] =
+				(snd_soc_read(widget->codec, reg2) >> shift) &
+						mask;
+		if (invert)
 			ucontrol->value.integer.value[1] =
 				max - ucontrol->value.integer.value[1];
 	}
@@ -2504,11 +2534,15 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
+	unsigned int reg2 = mc->rreg;
 	unsigned int shift = mc->shift;
+	unsigned int rshift = mc->rshift;
 	int max = mc->max;
 	unsigned int mask = (1 << fls(max)) - 1;
 	unsigned int invert = mc->invert;
-	unsigned int val;
+	bool type_2r = 0;
+	unsigned int val2 = 0;
+	unsigned int val, val_mask;
 	int connect, change;
 	struct snd_soc_dapm_update update;
 	int wi;
@@ -2518,27 +2552,53 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 
 	if (invert)
 		val = max - val;
-	mask = mask << shift;
+	val_mask = mask << shift;
 	val = val << shift;
 
+	if (snd_soc_volsw_is_stereo(mc)) {
+		val2 = (ucontrol->value.integer.value[1] & mask);
+		connect |= !!val2;
+
+		if (invert)
+			val2 = max - val2;
+		if (reg == reg2) {
+			val_mask |= mask << rshift;
+			val |= val2 << rshift;
+		} else {
+			val2 = val2 << shift;
+			type_2r = 1;
+		}
+	}
+
 	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
 
-	change = snd_soc_test_bits(widget->codec, reg, mask, val);
+	change = snd_soc_test_bits(widget->codec, reg, val_mask, val);
+	if (type_2r)
+		change |= snd_soc_test_bits(widget->codec,
+					    reg2, val_mask, val2);
+
 	if (change) {
 		for (wi = 0; wi < wlist->num_widgets; wi++) {
 			widget = wlist->widgets[wi];
 
 			widget->value = val;
-
 			update.kcontrol = kcontrol;
 			update.widget = widget;
 			update.reg = reg;
-			update.mask = mask;
+			update.mask = val_mask;
 			update.val = val;
-			widget->dapm->update = &update;
 
-			soc_dapm_mixer_update_power(widget, kcontrol, connect);
+			if (type_2r) {
+				widget->rvalue = val2;
+				update.rreg = reg2;
+				update.rmask = val_mask;
+				update.rval = val2;
+			} else {
+				update.rmask = 0;
+			}
 
+			widget->dapm->update = &update;
+			soc_dapm_mixer_update_power(widget, kcontrol, connect);
 			widget->dapm->update = NULL;
 		}
 	}
@@ -2627,6 +2687,7 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 			update.reg = e->reg;
 			update.mask = mask;
 			update.val = val;
+			update.rmask = 0;
 			widget->dapm->update = &update;
 
 			soc_dapm_mux_update_power(widget, kcontrol, mux, e);
@@ -2793,6 +2854,7 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 			update.reg = e->reg;
 			update.mask = mask;
 			update.val = val;
+			update.rmask = 0;
 			widget->dapm->update = &update;
 
 			soc_dapm_mux_update_power(widget, kcontrol, mux, e);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC/PATCH] ASoC: dapm: Fix/add support for stereo widgets

[Mark Brown]

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

On Mon, Jul 02, 2012 at 01:46:42PM +0200, Benoît Thébaudeau wrote:
> This patch:
> * adds a choice in snd_soc_dapm_get_volsw_mut() for stereo between 1 and 2
>   registers, like in snd_soc_get_volsw().
> * fixes the missing stereo in other parts of dapm.
> * removes the unused saved_value from struct snd_soc_dapm_widget.

There's a couple of high level problems here.  

One is that you've got multiple different things in a single commit
which isn't great practice for review, especially with such a vauge
changelog.

The other is that while you say that this "fixes the missing stereo in
other parts of dapm" I can't see any sign of any changes to (for
example) the path setup code which would seem to be essential for
supporting more than one channel in controls.

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

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

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Takashi Iwai]

At Sat, 30 Jun 2012 19:24:47 +0100,
Mark Brown wrote:
> 
> On Sat, Jun 30, 2012 at 03:03:09PM +0200, Benoît Thébaudeau wrote:
> > On Sat, Jun 30, 2012 at 01:39:50PM +0200, Mark Brown wrote:
> 
> > > Actually I'd really expect it to mean exactly that.  DAPM is fairly
> > > fundamentally dealing with single channel audio streams and I'd expect
> > > that if we did implement stereo controls they'd allow these streams to
> > > pass through without getting merged.  There's a couple of things that
> > > stop us doing that right now but I don't think we should add stereo
> > > controls that don't do that.
> 
> > Can you be more specific about what stops us doing that right now?
> 
> Nothing if someone wants to implement it.
> 
> > Should we drop this idea for now and simply remove stereo from
> > snd_soc_dapm_get_volsw()?
> 
> It's going to be easier to remove the code if we're going to do anything
> at all, though there's no great urgency.

Then it'd be also better to put a sanity check to warn if
snd_soc_volsw_is_stereo() returns true.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Mark Brown]

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

On Fri, Jun 29, 2012 at 06:22:28PM +0200, Takashi Iwai wrote:
> Benoît Thébaudeau wrote:

> > I saw that too. Is it the stereo case that should be added to put or removed
> > from get, i.e. is it possible to handle a stereo case with only one connect
> > flag?

> I would assume that connect should be up when either left or right is
> on.  A stereo widget doesn't mean multiple connections.

Actually I'd really expect it to mean exactly that.  DAPM is fairly
fundamentally dealing with single channel audio streams and I'd expect
that if we did implement stereo controls they'd allow these streams to
pass through without getting merged.  There's a couple of things that
stop us doing that right now but I don't think we should add stereo
controls that don't do that.

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

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

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

On Fri, Jun 29, 2012 at 05:43:09PM +0200, Takashi Iwai wrote:
> At Fri, 29 Jun 2012 16:29:22 +0200 (CEST),
> Benoît Thébaudeau wrote:
> > 
> > On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
> > > At Fri, 29 Jun 2012 13:53:56 +0200 (CEST),
> > > Benoît Thébaudeau wrote:
> > > > 
> > > > On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
> > > > > At Mon, 18 Jun 2012 22:41:28 +0200 (CEST),
> > > > > Benoît Thébaudeau wrote:
> > > > > > 
> > > > > > snd_soc_dapm_put_volsw() sets connect incorrectly in the
> > > > > > case
> > > > > > max >
> > > > > > 1 with
> > > > > > invert. In that case, the raw disconnect value should be
> > > > > > max,
> > > > > > which
> > > > > > corresponds
> > > > > > to the userspace value 0.
> > > > > > 
> > > > > > This use case currently does not appear upstream, but it
> > > > > > could
> > > > > > break
> > > > > > SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in
> > > > > > the
> > > > > > future.
> > > > > > 
> > > > > > Cc: Liam Girdwood <lrg@ti.com>
> > > > > > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > > > > Cc: <alsa-devel@alsa-project.org>
> > > > > > Signed-off-by: Benoît Thébaudeau
> > > > > > <benoit.thebaudeau@advansee.com>
> > > > > > ---
> > > > > >  .../sound/soc/soc-dapm.c                           |    8
> > > > > >  +-------
> > > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > > > 
> > > > > > diff --git
> > > > > > linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > > > linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > > > index 405841c..5ef082f 100644
> > > > > > --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > > > +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > > > @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct
> > > > > > snd_kcontrol *kcontrol,
> > > > > >  	int wi;
> > > > > >  
> > > > > >  	val = (ucontrol->value.integer.value[0] & mask);
> > > > > > +	connect = !!val;
> > > > > >  
> > > > > >  	if (invert)
> > > > > >  		val = max - val;
> > > > > >  	mask = mask << shift;
> > > > > >  	val = val << shift;
> > > > > >  
> > > > > > -	if (val)
> > > > > > -		/* new connection */
> > > > > > -		connect = invert ? 0 : 1;
> > > > > > -	else
> > > > > > -		/* old connection must be powered down */
> > > > > > -		connect = invert ? 1 : 0;
> > > > > > -
> > > > > 
> > > > > Doesn't this result in the same value of connect?
> > > > > 
> > > > > (given value, invert) --> (raw value, connect)
> > > > > 
> > > > > old code:  (0, 0) --> (0, 0)
> > > > >            (0, 1) --> (max, 0)
> > > > >            (max, 0) -> (max, 1)
> > > > >            (max, 1) -> (0, 1)
> > > > > 
> > > > > new code:  (0, 0) --> (0, 0)
> > > > >            (0, 1) --> (max, 0)
> > > > >            (max, 0) --> (max, 1)
> > > > >            (max, 1) --> (0, 1)
> > > > > 
> > > > > I'd understand if the line "connect = !!val;" were after the
> > > > > invert
> > > > > conversion...
> > > > > 
> > > > >   	if (invert)
> > > > >   		val = max - val;
> > > > > 	connect = !!val;
> > > > 
> > > > Take max = 5, invert = 1, user val = 2:
> > > > old code: connect = 0
> > > > new code: connect = 1
> > > 
> > > OK, then you need to fix dapm_set_path_status() as well, too.
> > > Otherwise the logic becomes inconsistent.
> > 
> > Indeed, I missed that. But the issue is even worse in this
> > function: It uses the
> > control register value to determine if connect should be set while
> > only the
> > userspace value can tell that, and it has no way of deriving the
> > userspace value
> > apart from calling the get function, while here it assumes that the
> > register
> > value will be more or less compatible with snd_soc_dapm_get_volsw.
> 
> That's a valid assumption.  Usually get and put callbacks must be
> paired well.

Yes, except that some codec drivers customize these callbacks for specific
register encodings (e.g. snd_soc_dapm_put_volsw_aic3x). By chance, only the put
callbacks seem to have been customized so far.

What is the point of having customizable get/put callbacks if
dapm_set_path_status() ignores them and duplicates their core behavior?

In the aic3x example, the bit-field is actually a mixer volume control, and not
only a boolean, so I was planning to post a fix for that. The issue is that the
encoding of register values for this volume has holes that should not be
duplicated in userspace raw values, so custom get and put callbacks have to be
used here that will not be compatible with snd_soc_dapm_get_volsw(), which will
be blocking for dapm_set_path_status(). Do you have a simple solution for that?

> > Hence, I think that the fix here should be to call get, then to
> > deduce connect
> > from the returned value. Do you agree?
> 
> Well, calling a control callback internally is a bit worrisome.

Yes, and there is another issue: kcontrol may not be available for get at this
point.

In the meantime, please find below a quick patch for the consistency issue.

Regards,
Benoît


[PATCH] ASoC: dapm: Fix dapm_set_path_status() connect

dapm_set_path_status() sets connect incorrectly in the case max > 1 with invert.
In that case, the raw disconnect value should be max, which corresponds to the
userspace value 0.

This use case currently does not appear upstream, but it could break
SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.

This patch completes commit 3a9abe8.

Cc: Liam Girdwood <lrg@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: <alsa-devel@alsa-project.org>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../sound/soc/soc-dapm.c                           |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
index 9670668..7f2a4bb 100644
--- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c
+++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
@@ -324,11 +324,10 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w,
 
 		val = soc_widget_read(w, reg);
 		val = (val >> shift) & mask;
+		if (invert)
+			val = max - val;
 
-		if ((invert && !val) || (!invert && val))
-			p->connect = 1;
-		else
-			p->connect = 0;
+		p->connect = !!val;
 	}
 	break;
 	case snd_soc_dapm_mux: {
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Takashi Iwai]

At Fri, 29 Jun 2012 18:05:44 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> On Fri, Jun 29, 2012 at 05:43:09PM +0200, Takashi Iwai wrote:
> > At Fri, 29 Jun 2012 16:29:22 +0200 (CEST),
> > Benoît Thébaudeau wrote:
> > > 
> > > On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
> > > > At Fri, 29 Jun 2012 13:53:56 +0200 (CEST),
> > > > Benoît Thébaudeau wrote:
> > > > > 
> > > > > On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
> > > > > > At Mon, 18 Jun 2012 22:41:28 +0200 (CEST),
> > > > > > Benoît Thébaudeau wrote:
> > > > > > > 
> > > > > > > snd_soc_dapm_put_volsw() sets connect incorrectly in the
> > > > > > > case
> > > > > > > max >
> > > > > > > 1 with
> > > > > > > invert. In that case, the raw disconnect value should be
> > > > > > > max,
> > > > > > > which
> > > > > > > corresponds
> > > > > > > to the userspace value 0.
> > > > > > > 
> > > > > > > This use case currently does not appear upstream, but it
> > > > > > > could
> > > > > > > break
> > > > > > > SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in
> > > > > > > the
> > > > > > > future.
> > > > > > > 
> > > > > > > Cc: Liam Girdwood <lrg@ti.com>
> > > > > > > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > > > > > Cc: <alsa-devel@alsa-project.org>
> > > > > > > Signed-off-by: Benoît Thébaudeau
> > > > > > > <benoit.thebaudeau@advansee.com>
> > > > > > > ---
> > > > > > >  .../sound/soc/soc-dapm.c                           |    8
> > > > > > >  +-------
> > > > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git
> > > > > > > linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > > > > linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > > > > index 405841c..5ef082f 100644
> > > > > > > --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c
> > > > > > > +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c
> > > > > > > @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct
> > > > > > > snd_kcontrol *kcontrol,
> > > > > > >  	int wi;
> > > > > > >  
> > > > > > >  	val = (ucontrol->value.integer.value[0] & mask);
> > > > > > > +	connect = !!val;
> > > > > > >  
> > > > > > >  	if (invert)
> > > > > > >  		val = max - val;
> > > > > > >  	mask = mask << shift;
> > > > > > >  	val = val << shift;
> > > > > > >  
> > > > > > > -	if (val)
> > > > > > > -		/* new connection */
> > > > > > > -		connect = invert ? 0 : 1;
> > > > > > > -	else
> > > > > > > -		/* old connection must be powered down */
> > > > > > > -		connect = invert ? 1 : 0;
> > > > > > > -
> > > > > > 
> > > > > > Doesn't this result in the same value of connect?
> > > > > > 
> > > > > > (given value, invert) --> (raw value, connect)
> > > > > > 
> > > > > > old code:  (0, 0) --> (0, 0)
> > > > > >            (0, 1) --> (max, 0)
> > > > > >            (max, 0) -> (max, 1)
> > > > > >            (max, 1) -> (0, 1)
> > > > > > 
> > > > > > new code:  (0, 0) --> (0, 0)
> > > > > >            (0, 1) --> (max, 0)
> > > > > >            (max, 0) --> (max, 1)
> > > > > >            (max, 1) --> (0, 1)
> > > > > > 
> > > > > > I'd understand if the line "connect = !!val;" were after the
> > > > > > invert
> > > > > > conversion...
> > > > > > 
> > > > > >   	if (invert)
> > > > > >   		val = max - val;
> > > > > > 	connect = !!val;
> > > > > 
> > > > > Take max = 5, invert = 1, user val = 2:
> > > > > old code: connect = 0
> > > > > new code: connect = 1
> > > > 
> > > > OK, then you need to fix dapm_set_path_status() as well, too.
> > > > Otherwise the logic becomes inconsistent.
> > > 
> > > Indeed, I missed that. But the issue is even worse in this
> > > function: It uses the
> > > control register value to determine if connect should be set while
> > > only the
> > > userspace value can tell that, and it has no way of deriving the
> > > userspace value
> > > apart from calling the get function, while here it assumes that the
> > > register
> > > value will be more or less compatible with snd_soc_dapm_get_volsw.
> > 
> > That's a valid assumption.  Usually get and put callbacks must be
> > paired well.
> 
> Yes, except that some codec drivers customize these callbacks for specific
> register encodings (e.g. snd_soc_dapm_put_volsw_aic3x). By chance, only the put
> callbacks seem to have been customized so far.

Hm, that's bad.

> What is the point of having customizable get/put callbacks if
> dapm_set_path_status() ignores them and duplicates their core behavior?

It's a design flaw :)

> In the aic3x example, the bit-field is actually a mixer volume control, and not
> only a boolean, so I was planning to post a fix for that. The issue is that the
> encoding of register values for this volume has holes that should not be
> duplicated in userspace raw values, so custom get and put callbacks have to be
> used here that will not be compatible with snd_soc_dapm_get_volsw(), which will
> be blocking for dapm_set_path_status(). Do you have a simple solution for that?

One option is to create a new hook in struct soc_mixer_control or
whatever, e.g.
	int (*connect_update)(struct soc_mixer_control *mc);
that returns the connect value.

Then give the own callback when you set the customized get/put
things that are incompatible with the default one.

> > > Hence, I think that the fix here should be to call get, then to
> > > deduce connect
> > > from the returned value. Do you agree?
> > 
> > Well, calling a control callback internally is a bit worrisome.
> 
> Yes, and there is another issue: kcontrol may not be available for get at this
> point.
> 
> In the meantime, please find below a quick patch for the consistency issue.

Looks good to me.

Reivewed-by: Takashi Iwai <tiwai@suse.de>


Takashi

> 
> Regards,
> Benoît
> 
> 
> [PATCH] ASoC: dapm: Fix dapm_set_path_status() connect
> 
> dapm_set_path_status() sets connect incorrectly in the case max > 1 with invert.
> In that case, the raw disconnect value should be max, which corresponds to the
> userspace value 0.
> 
> This use case currently does not appear upstream, but it could break
> SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
> 
> This patch completes commit 3a9abe8.
> 
> Cc: Liam Girdwood <lrg@ti.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: <alsa-devel@alsa-project.org>
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> ---
>  .../sound/soc/soc-dapm.c                           |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
> index 9670668..7f2a4bb 100644
> --- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c
> +++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
> @@ -324,11 +324,10 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w,
>  
>  		val = soc_widget_read(w, reg);
>  		val = (val >> shift) & mask;
> +		if (invert)
> +			val = max - val;
>  
> -		if ((invert && !val) || (!invert && val))
> -			p->connect = 1;
> -		else
> -			p->connect = 0;
> +		p->connect = !!val;
>  	}
>  	break;
>  	case snd_soc_dapm_mux: {
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Mark Brown]

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

On Fri, Jun 29, 2012 at 06:05:44PM +0200, Benoît Thébaudeau wrote:
> On Fri, Jun 29, 2012 at 05:43:09PM +0200, Takashi Iwai wrote:

> > That's a valid assumption.  Usually get and put callbacks must be
> > paired well.

> What is the point of having customizable get/put callbacks if
> dapm_set_path_status() ignores them and duplicates their core behavior?

Nobody needed to do that yet.

> In the aic3x example, the bit-field is actually a mixer volume control, and not
> only a boolean, so I was planning to post a fix for that. The issue is that the
> encoding of register values for this volume has holes that should not be
> duplicated in userspace raw values, so custom get and put callbacks have to be
> used here that will not be compatible with snd_soc_dapm_get_volsw(), which will
> be blocking for dapm_set_path_status(). Do you have a simple solution for that?

Clearly the driver is just buggy and should implement get for itself
too.  As far as reading back the path status is concerned so long as
the mapping looks OK as a boolean we're fine, it doesn't matter what the
individual volumes are as all the code is interested in is on/off.

> In the meantime, please find below a quick patch for the consistency issue.

Stop doing this, send patches for review rather than burying them in the
middle of e-mails where they're at best a pain to apply.

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

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

Re: [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect

[Benoît Thébaudeau]

On Sat, Jun 30, 2012 at 01:54:25PM +0200, Mark Brown wrote:
> > In the meantime, please find below a quick patch for the
> > consistency issue.
> 
> Stop doing this, send patches for review rather than burying them in
> the
> middle of e-mails where they're at best a pain to apply.

OK, I'll repost it separately on Monday.

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH] ASoC: dapm: Fix dapm_set_path_status() connect

[Benoît Thébaudeau]

dapm_set_path_status() sets connect incorrectly in the case max > 1 with invert.
In that case, the raw disconnect value should be max, which corresponds to the
userspace value 0.

This use case currently does not appear upstream, but it could break
SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.

This patch completes commit 3a9abe8.

Cc: Liam Girdwood <lrg@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: <alsa-devel@alsa-project.org>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../sound/soc/soc-dapm.c                           |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
index 9670668..7f2a4bb 100644
--- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c
+++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c
@@ -324,11 +324,10 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w,
 
 		val = soc_widget_read(w, reg);
 		val = (val >> shift) & mask;
+		if (invert)
+			val = max - val;
 
-		if ((invert && !val) || (!invert && val))
-			p->connect = 1;
-		else
-			p->connect = 0;
+		p->connect = !!val;
 	}
 	break;
 	case snd_soc_dapm_mux: {
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dapm: Fix dapm_set_path_status() connect

[Mark Brown]

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

On Mon, Jul 02, 2012 at 01:45:21PM +0200, Benoît Thébaudeau wrote:
> dapm_set_path_status() sets connect incorrectly in the case max > 1 with invert.
> In that case, the raw disconnect value should be max, which corresponds to the
> userspace value 0.

Applied, thanks.

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

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