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

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