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

["Benoît Thébaudeau" <benoit.thebaudeau@advansee.com>]

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