[alsa-devel] [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 at ti.com>
> > Cc: Mark Brown <broonie at opensource.wolfsonmicro.com>
> > Cc: <alsa-devel at alsa-project.org>
> > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau at 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