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

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(-)

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

> 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

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.