[alsa-devel] [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect
Takashi Iwai
tiwai at suse.de
Fri Jun 29 18:11:32 CEST 2012
At Fri, 29 Jun 2012 18:05:44 +0200 (CEST),
Benoît Thébaudeau wrote:
>
> 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 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.
>
> 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.
Hm, that's bad.
> What is the point of having customizable get/put callbacks if
> dapm_set_path_status() ignores them and duplicates their core behavior?
It's a design flaw :)
> 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?
One option is to create a new hook in struct soc_mixer_control or
whatever, e.g.
int (*connect_update)(struct soc_mixer_control *mc);
that returns the connect value.
Then give the own callback when you set the customized get/put
things that are incompatible with the default one.
> > > 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.
Looks good to me.
Reivewed-by: Takashi Iwai <tiwai at suse.de>
Takashi
>
> 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 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 | 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: {
>
More information about the Alsa-devel
mailing list