Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

From: Takashi Iwai <tiwai@suse.de>
To: Mark Brown <broonie@kernel.org>
Cc: Marek Vasut <marex@denx.de>,
	alsa-devel@alsa-project.org, stable@vger.kernel.org
Subject: Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min
Date: Wed, 23 Feb 2022 17:44:45 +0100	[thread overview]
Message-ID: <s5hfso9y1ky.wl-tiwai@suse.de> (raw)
In-Reply-To: <YhZhkz6gQYsK3Fwd@sirena.org.uk>

On Wed, 23 Feb 2022 17:32:19 +0100,
Mark Brown wrote:
> 
> On Wed, Feb 23, 2022 at 03:55:54PM +0100, Takashi Iwai wrote:
> > 
> > But, more reading the code, I suspect whether the function does work
> > correctly at all...  How is the mask calculation done in that way?
> >   unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
> > What's the difference of this function with snd_soc_put_volsw()?
> 
> Yeah, I'm not clear either - Marek mentioned _SX when he was doing the
> patch but I didn't get the bandwidth to figure out what it's doing
> properly yet.  At this point I'm not clear what _SX is supposed to do,
> I'm hoping it works well for the devices that use it but I don't have
> any of them.

OK, let's hope that...

> > Furthermore, the mask calculation and usage in snd_soc_put_volsw()
> > isn't right, either, I'm afraid; if the range is [-10, 0], max=0, then
> > mask will 0, which will omit all values...
> 
> Indeed, if anyone did that.  Fortunately I don't *think* that's an
> issue.  The whole way that code handles signed bitfields by remapping
> them into unsigned user visible controls is a landmine, it's not even
> obvious that they handle signed bitfields in the first place.

Thanks, then it seems OK as is for now.  I guess the signed bit should
be detected by the helper instead of hard-coding, but it's no urgent
issue.

Takashi