[alsa-devel] [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 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

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


Takashi