[PATCH] ASoC: ops: Revert addition of min whilst checking platform max

[PATCH] ASoC: ops: Revert addition of min whilst checking platform max

[Charles Keepax]

This reverts commit 9bdd10d57a88 ("ASoC: ops: Shift tested values in
snd_soc_put_volsw() by +min").

There are two ways the platform_max could be interpreted; the maximum
register value, or the maximum value the control can be set to. The
patch moved from treating the value as a control value to a register
one. When the patch was applied it was technically correct as
snd_soc_limit_volume() also used the register interpretation. However,
even then all the other usages treated platform_max as a control
value, and snd_soc_limit_volume() has since been updated to also
do so in commit fb9ad24485087 ("ASoC: ops: add correct range
check for limiting volume"). That patch however, missed updating
snd_soc_put_volsw() back to the control interpretation. The control
interpretation makes more sense as limiting is typically done from
the machine driver, so it is appropriate to use the customer facing
representation rather than the internal codec representation.

Finally, also add some comments to the soc_mixer_control struct to
hopefully avoid further patches switching between the two approaches.

Fixes: fb9ad24485087 ("ASoC: ops: add correct range check for limiting volume")
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/soc.h | 5 ++++-
 sound/soc/soc-ops.c | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6bb2fca044c5e..bd7a0514ad7fa 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1251,7 +1251,10 @@ void snd_soc_close_delayed_work(struct snd_soc_pcm_runtime *rtd);
 
 /* mixer control */
 struct soc_mixer_control {
-	int min, max, platform_max;
+	/* Minimum and maximum specified as written to the hardware */
+	int min, max;
+	/* Limited maximum value specified as presented through the control */
+	int platform_max;
 	int reg, rreg;
 	unsigned int shift, rshift;
 	u32 num_channels;
diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 60b33e22ac08c..28babe42ed40c 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -325,7 +325,7 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 	if (ucontrol->value.integer.value[0] < 0)
 		return -EINVAL;
 	val = ucontrol->value.integer.value[0];
-	if (mc->platform_max && ((int)val + min) > mc->platform_max)
+	if (mc->platform_max && val > mc->platform_max)
 		return -EINVAL;
 	if (val > max - min)
 		return -EINVAL;
@@ -338,7 +338,7 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 		if (ucontrol->value.integer.value[1] < 0)
 			return -EINVAL;
 		val2 = ucontrol->value.integer.value[1];
-		if (mc->platform_max && ((int)val2 + min) > mc->platform_max)
+		if (mc->platform_max && val2 > mc->platform_max)
 			return -EINVAL;
 		if (val2 > max - min)
 			return -EINVAL;
-- 
2.39.5

Re: [PATCH] ASoC: ops: Revert addition of min whilst checking platform max

[Charles Keepax]

On Thu, Feb 27, 2025 at 02:35:13PM +0000, Charles Keepax wrote:
> This reverts commit 9bdd10d57a88 ("ASoC: ops: Shift tested values in
> snd_soc_put_volsw() by +min").
> 
> There are two ways the platform_max could be interpreted; the maximum
> register value, or the maximum value the control can be set to. The
> patch moved from treating the value as a control value to a register
> one. When the patch was applied it was technically correct as
> snd_soc_limit_volume() also used the register interpretation. However,
> even then all the other usages treated platform_max as a control
> value, and snd_soc_limit_volume() has since been updated to also
> do so in commit fb9ad24485087 ("ASoC: ops: add correct range
> check for limiting volume"). That patch however, missed updating
> snd_soc_put_volsw() back to the control interpretation. The control
> interpretation makes more sense as limiting is typically done from
> the machine driver, so it is appropriate to use the customer facing
> representation rather than the internal codec representation.
> 

Seems there is also one other case in snd_soc_info_volsw_range
than needs fixed will do a v2 and resend.

Thanks,
Charles