diff mbox series

ASoC: ops: Don't modify the driver's plaform_max when reading state

Message ID 20220603112508.3856519-1-broonie@kernel.org (mailing list archive)
State Accepted
Commit 30ac49841386f933339817771ec315a34a4c0edd
Headers show
Series ASoC: ops: Don't modify the driver's plaform_max when reading state | expand

Commit Message

Mark Brown June 3, 2022, 11:25 a.m. UTC
Currently snd_soc_info_volsw() will set a platform_max based on the limit
the control has if one is not already set. This isn't really great, we
shouldn't be modifying the passed in driver data especially in a path like
this which may not ever be executed or where we may execute other callbacks
before this one. Instead make this function leave the data unchanged, and
clarify things a bit by referring to max rather than platform_max within
the function. platform_max is now applied as a limit after working out the
natural maximum value for the control.

This means that platform_max is no longer treated as a direct register
value for controls were min is non-zero. The put() callbacks already
validate on this basis, and there do not appear to be any in tree users
that would be affected.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-ops.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Mark Brown June 14, 2022, 2:02 p.m. UTC | #1
On Fri, 3 Jun 2022 13:25:08 +0200, Mark Brown wrote:
> Currently snd_soc_info_volsw() will set a platform_max based on the limit
> the control has if one is not already set. This isn't really great, we
> shouldn't be modifying the passed in driver data especially in a path like
> this which may not ever be executed or where we may execute other callbacks
> before this one. Instead make this function leave the data unchanged, and
> clarify things a bit by referring to max rather than platform_max within
> the function. platform_max is now applied as a limit after working out the
> natural maximum value for the control.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: ops: Don't modify the driver's plaform_max when reading state
      commit: 30ac49841386f933339817771ec315a34a4c0edd

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
Martin Povišer Aug. 19, 2022, 4:17 p.m. UTC | #2
> On 3. 6. 2022, at 13:25, Mark Brown <broonie@kernel.org> wrote:
> 
> Currently snd_soc_info_volsw() will set a platform_max based on the limit
> the control has if one is not already set. This isn't really great, we
> shouldn't be modifying the passed in driver data especially in a path like
> this which may not ever be executed or where we may execute other callbacks
> before this one. Instead make this function leave the data unchanged, and
> clarify things a bit by referring to max rather than platform_max within
> the function. platform_max is now applied as a limit after working out the
> natural maximum value for the control.
> 
> This means that platform_max is no longer treated as a direct register
> value for controls were min is non-zero. The put() callbacks already
> validate on this basis, and there do not appear to be any in tree users
> that would be affected.

At least ‘put_volsw' seem to validate on the other conflicting interpretation
of platform_max [as was introduced in commit 9bdd10d57a88 (“ASoC: ops:
Shift tested values in snd_soc_put_volsw() by +min”)].

Also, the soc.h definitions of SOC_SINGLE_*/SOC_DOUBLE_* set platform_max
to the register maximum, again interpreting platform_max the other way.

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

This commit breaks controls with non-zero minimum.

Best,
Martin
Martin Povišer Aug. 19, 2022, 4:33 p.m. UTC | #3
> On 19. 8. 2022, at 18:17, Martin Povišer <povik+lin@cutebit.org> wrote:
> 
>> 
>> On 3. 6. 2022, at 13:25, Mark Brown <broonie@kernel.org> wrote:


>> This means that platform_max is no longer treated as a direct register
>> value for controls were min is non-zero. The put() callbacks already
>> validate on this basis, and there do not appear to be any in tree users
>> that would be affected.
> 
> At least ‘put_volsw' seem to validate on the other conflicting interpretation
> of platform_max [as was introduced in commit 9bdd10d57a88 (“ASoC: ops:
> Shift tested values in snd_soc_put_volsw() by +min”)].
> 
> Also, the soc.h definitions of SOC_SINGLE_*/SOC_DOUBLE_* set platform_max
> to the register maximum, again interpreting platform_max the other way.

Another instance: snd_soc_limit_volume in checking the supplied platform
maximum against mc->max
Mark Brown Aug. 19, 2022, 10:38 p.m. UTC | #4
On Fri, Aug 19, 2022 at 06:17:25PM +0200, Martin Povišer wrote:
> > On 3. 6. 2022, at 13:25, Mark Brown <broonie@kernel.org> wrote:

> > This means that platform_max is no longer treated as a direct register
> > value for controls were min is non-zero. The put() callbacks already
> > validate on this basis, and there do not appear to be any in tree users
> > that would be affected.

> At least ‘put_volsw' seem to validate on the other conflicting interpretation
> of platform_max [as was introduced in commit 9bdd10d57a88 (“ASoC: ops:
> Shift tested values in snd_soc_put_volsw() by +min”)].

Ugh, so it does.  The patchwork of reuse and differing interpretations
of these controls is causing all sorts of confusion :/

> Also, the soc.h definitions of SOC_SINGLE_*/SOC_DOUBLE_* set platform_max
> to the register maximum, again interpreting platform_max the other way.

That use of platform_max has been removed since it was just obviously
not sensible anyway, the whole purpose of platform_max is to override
what was set in the control so having both max and platform_max set is
just redundant and causing confusion.

> > Signed-off-by: Mark Brown <broonie@kernel.org>

> This commit breaks controls with non-zero minimum.

Could you specify more exactly how it does that, and indeed where we
have non-zero minimums - all the info callbacks report 0 as a minimum as
far as I can see?  Life would be a lot simpler if the controls had all
been defined to just be the register values, I've never been able to
figure out why they're anything else since the ABI for controls supports
negative values just fine.
Martin Povišer Aug. 20, 2022, 1:10 a.m. UTC | #5
> On 20. 8. 2022, at 0:38, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Aug 19, 2022 at 06:17:25PM +0200, Martin Povišer wrote:
>>> On 3. 6. 2022, at 13:25, Mark Brown <broonie@kernel.org> wrote:
> 
>>> This means that platform_max is no longer treated as a direct register
>>> value for controls were min is non-zero. The put() callbacks already
>>> validate on this basis, and there do not appear to be any in tree users
>>> that would be affected.
> 
>> At least ‘put_volsw' seem to validate on the other conflicting interpretation
>> of platform_max [as was introduced in commit 9bdd10d57a88 (“ASoC: ops:
>> Shift tested values in snd_soc_put_volsw() by +min”)].
> 
> Ugh, so it does.  The patchwork of reuse and differing interpretations
> of these controls is causing all sorts of confusion :/
> 
>> Also, the soc.h definitions of SOC_SINGLE_*/SOC_DOUBLE_* set platform_max
>> to the register maximum, again interpreting platform_max the other way.
> 
> That use of platform_max has been removed since it was just obviously
> not sensible anyway, the whole purpose of platform_max is to override
> what was set in the control so having both max and platform_max set is
> just redundant and causing confusion.

Right, I was about to submit removal of it myself to address the issues
that surfaced with this patch, then I saw there’s more confusion with
platform_max. Anyway I see the removal’s been done in 26bdcc4b.

>>> Signed-off-by: Mark Brown <broonie@kernel.org>
> 
>> This commit breaks controls with non-zero minimum.
> 
> Could you specify more exactly how it does that, and indeed where we
> have non-zero minimums - all the info callbacks report 0 as a minimum as
> far as I can see?  Life would be a lot simpler if the controls had all
> been defined to just be the register values, I've never been able to
> figure out why they're anything else since the ABI for controls supports
> negative values just fine.

Sure. What I meant are non-zero register value minimums, especially
negative ones, and the breaking was in interaction with the default
platform_max values from SOC_SINGLE_*/SOC_DOUBLE_*.

Taking for example

    SOC_SINGLE_S8_TLV("ADC Volume", CS42L42_ADC_VOLUME, -97, 12, adc_tlv),

of codecs/cs42l42.c, platform_max was set to 12 and the register value
was then clipped to -97..-85.

So this should be fixed by the removal of the default platform_max,
leaving us with few discrepancies that only manifest if platform_max
is being actively used (and in that only on controls with non-zero
register minimum).

Martin
Mark Brown Aug. 22, 2022, 4:47 p.m. UTC | #6
On Sat, Aug 20, 2022 at 03:10:51AM +0200, Martin Povišer wrote:
> > On 20. 8. 2022, at 0:38, Mark Brown <broonie@kernel.org> wrote:

> >> This commit breaks controls with non-zero minimum.

> > Could you specify more exactly how it does that, and indeed where we
> > have non-zero minimums - all the info callbacks report 0 as a minimum as
> > far as I can see?  Life would be a lot simpler if the controls had all
> > been defined to just be the register values, I've never been able to
> > figure out why they're anything else since the ABI for controls supports
> > negative values just fine.

> Sure. What I meant are non-zero register value minimums, especially
> negative ones, and the breaking was in interaction with the default
> platform_max values from SOC_SINGLE_*/SOC_DOUBLE_*.

Ah, you mean the register field side - like I say the actual controls
themselves are always zero referenced.

> Taking for example

>     SOC_SINGLE_S8_TLV("ADC Volume", CS42L42_ADC_VOLUME, -97, 12, adc_tlv),

> of codecs/cs42l42.c, platform_max was set to 12 and the register value
> was then clipped to -97..-85.

> So this should be fixed by the removal of the default platform_max,
> leaving us with few discrepancies that only manifest if platform_max
> is being actively used (and in that only on controls with non-zero
> register minimum).

But only for controls using snd_soc_info_volsw(), not for controls in
general.  We need to figure out if we want platform_max to be a the
register or user facing value since that's the confusion here and bring
things into line.  Looking at the info callbacks _info_volsw() is
currently handling platform_max relative to the user visible control,
_info_volsw_sx() is too in that it doesn't support a non-zero register
minimum and _xr_sx() doesn't do platform_max at all which is fun.  

We also still have snd_soc_info_volsw_range() modifying the control's
platform_max which ought to be fixed, it doesn't support non-zero
register minimums either.

I'm in two minds here, user facing feels cleaner but we had a long time
where the code was mostly doing register values and I think some out of
tree Qualcomm stuff that assumed that (not just machine drivers but core
changes) were using that.  The usuability does feel like a bit of a toss
up.
diff mbox series

Patch

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 2d39370ddeca..93e72a016b4d 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -176,20 +176,21 @@  int snd_soc_info_volsw(struct snd_kcontrol *kcontrol,
 {
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	int platform_max;
+	int max;
 
-	if (!mc->platform_max)
-		mc->platform_max = mc->max;
-	platform_max = mc->platform_max;
+	max = uinfo->value.integer.max = mc->max - mc->min;
+	if (mc->platform_max && mc->platform_max < max)
+		max = mc->platform_max;
 
-	if (platform_max == 1 && !strstr(kcontrol->id.name, " Volume"))
+	if (max == 1 && !strstr(kcontrol->id.name, " Volume"))
 		uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
 	else
 		uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
 
 	uinfo->count = snd_soc_volsw_is_stereo(mc) ? 2 : 1;
 	uinfo->value.integer.min = 0;
-	uinfo->value.integer.max = platform_max - mc->min;
+	uinfo->value.integer.max = max;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_info_volsw);