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

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