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