[PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Peter Ujfalusi]

In 'normal' controls the mc->min is the minimum value the register can
have, the mc->max is the maximum (the steps between are max - min).

SX types are defined differently: mc->min is the minimum value and the
mc->max is the steps.

The max parameter of soc_mixer_reg_to_ctl() is the number of steps in
either type.

To have correct register value range in clamp the maximum value that needs
to be used is mc->min + max, which will be equal to mc->max for 'normal'
controls and mc->min + mc->max for SX ones.

The original clamp broke SX controls and rendered some of them impossible
to even set, like the cs42l43's Headphone Digital Volume, where the
min is smaller than the max (min=283, max=229 - 229 steps starting from
val 283).

The soc_mixer_ctl_to_reg() correctly uses the max parameter instead of
mc->max, so storing the value was correct.

Fixes: a0ce874cfaaa ("ASoC: ops: improve snd_soc_get_volsw")
Cc: stable@vger.kernel.org
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/soc-ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index ce86978c158d..6a18c56a9746 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -148,7 +148,7 @@ static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_v
 	if (mc->sign_bit)
 		val = sign_extend32(val, mc->sign_bit);
 
-	val = clamp(val, mc->min, mc->max);
+	val = clamp(val, mc->min, mc->min + max);
 	val -= mc->min;
 
 	if (mc->invert)
-- 
2.52.0

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
> In 'normal' controls the mc->min is the minimum value the register can
> have, the mc->max is the maximum (the steps between are max - min).

Have you seen:

  https://lore.kernel.org/r/20251216134938.788625-1-sbinding@opensource.cirrus.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 14:16, Mark Brown wrote:
> On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
>> In 'normal' controls the mc->min is the minimum value the register can
>> have, the mc->max is the maximum (the steps between are max - min).
> 
> Have you seen:
> 
>   https://lore.kernel.org/r/20251216134938.788625-1-sbinding@opensource.cirrus.com

No, I tried to look for possible fixes for this, but have not found it.

I think my one liner is a bit simpler with the same result, but I'll let
people decide which is better (and test on Cirrus side)

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Richard Fitzgerald]

On 17/12/2025 12:20 pm, Péter Ujfalusi wrote:
> 
> 
> On 17/12/2025 14:16, Mark Brown wrote:
>> On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
>>> In 'normal' controls the mc->min is the minimum value the register can
>>> have, the mc->max is the maximum (the steps between are max - min).
>>
>> Have you seen:
>>
>>    https://lore.kernel.org/r/20251216134938.788625-1-sbinding@opensource.cirrus.com
> 
> No, I tried to look for possible fixes for this, but have not found it.
> 
> I think my one liner is a bit simpler with the same result, but I'll let
> people decide which is better (and test on Cirrus side)
> 
Does it pass the kunit tests for SX controls?
The ASoC kunit tests have specific tests for SX controls.
The original patch failed those tests, but it was merged anyway.

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 14:36, Richard Fitzgerald wrote:
> On 17/12/2025 12:20 pm, Péter Ujfalusi wrote:
>>
>>
>> On 17/12/2025 14:16, Mark Brown wrote:
>>> On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
>>>> In 'normal' controls the mc->min is the minimum value the register can
>>>> have, the mc->max is the maximum (the steps between are max - min).
>>>
>>> Have you seen:
>>>
>>>    https://lore.kernel.org/r/20251216134938.788625-1-
>>> sbinding@opensource.cirrus.com
>>
>> No, I tried to look for possible fixes for this, but have not found it.
>>
>> I think my one liner is a bit simpler with the same result, but I'll let
>> people decide which is better (and test on Cirrus side)
>>
> Does it pass the kunit tests for SX controls?
> The ASoC kunit tests have specific tests for SX controls.
> The original patch failed those tests, but it was merged anyway.

It passes my manual tests on cs42l43, not sure how to run the unit-test
for SX, can you by chance test this?

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Richard Fitzgerald]

On 17/12/2025 12:38 pm, Péter Ujfalusi wrote:
> 
> 
> On 17/12/2025 14:36, Richard Fitzgerald wrote:
>> On 17/12/2025 12:20 pm, Péter Ujfalusi wrote:
>>>
>>>
>>> On 17/12/2025 14:16, Mark Brown wrote:
>>>> On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
>>>>> In 'normal' controls the mc->min is the minimum value the register can
>>>>> have, the mc->max is the maximum (the steps between are max - min).
>>>>
>>>> Have you seen:
>>>>
>>>>     https://lore.kernel.org/r/20251216134938.788625-1-
>>>> sbinding@opensource.cirrus.com
>>>
>>> No, I tried to look for possible fixes for this, but have not found it.
>>>
>>> I think my one liner is a bit simpler with the same result, but I'll let
>>> people decide which is better (and test on Cirrus side)
>>>
>> Does it pass the kunit tests for SX controls?
>> The ASoC kunit tests have specific tests for SX controls.
>> The original patch failed those tests, but it was merged anyway.
> 
> It passes my manual tests on cs42l43, not sure how to run the unit-test
> for SX, can you by chance test this?
> 
The easiest option is to run them from your kernel build tree like this:

make mrproper
./tools/testing/kunit/kunit.py run 
--kunitconfig=tools/testing/kunit/configs/all_tests.config

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]

On Wed, Dec 17, 2025 at 12:40:39PM +0000, Richard Fitzgerald wrote:

> make mrproper
> ./tools/testing/kunit/kunit.py run
> --kunitconfig=tools/testing/kunit/configs/all_tests.config

There's the --alltests option for this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 14:40, Richard Fitzgerald wrote:
>>
>> It passes my manual tests on cs42l43, not sure how to run the unit-test
>> for SX, can you by chance test this?
>>
> The easiest option is to run them from your kernel build tree like this:

It is in my to-do list to get kunit working on my setups (Artix Linux on
dev and DUTs), so it is really something that will take rest of the year
easily ;)

> 
> make mrproper
> ./tools/testing/kunit/kunit.py run --kunitconfig=tools/testing/kunit/
> configs/all_tests.config

> 

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Richard Fitzgerald]

On 17/12/2025 1:01 pm, Péter Ujfalusi wrote:
> 
> 
> On 17/12/2025 14:40, Richard Fitzgerald wrote:
>>>
>>> It passes my manual tests on cs42l43, not sure how to run the unit-test
>>> for SX, can you by chance test this?
>>>
>> The easiest option is to run them from your kernel build tree like this:
> 
> It is in my to-do list to get kunit working on my setups (Artix Linux on
> dev and DUTs), so it is really something that will take rest of the year
> easily ;)
> 
>>
>> make mrproper
>> ./tools/testing/kunit/kunit.py run --kunitconfig=tools/testing/kunit/
>> configs/all_tests.config
> 
>>
> 
The kunit tests can run on the computer you use to build the kernel. It
takes a couple of minutes to run.

On my desktop box with vanilla 6.19-rc1:

rf@debianbox:~/work/kernel/linux$ make mrproper
rf@debianbox:~/work/kernel/linux$ time ./tools/testing/kunit/kunit.py 
run --alltests

<SNIP>

[13:12:31] Testing complete. Ran 7154 tests: passed: 7082, failed: 36, 
skipped: 36
[13:12:31] Elapsed time: 110.250s total, 3.072s configuring, 55.003s 
building, 52.119s running

real	1m50.314s
user	15m52.878s
sys	1m18.186s

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 15:16, Richard Fitzgerald wrote:
> On 17/12/2025 1:01 pm, Péter Ujfalusi wrote:
>>
>>
>> On 17/12/2025 14:40, Richard Fitzgerald wrote:
>>>>
>>>> It passes my manual tests on cs42l43, not sure how to run the unit-test
>>>> for SX, can you by chance test this?
>>>>
>>> The easiest option is to run them from your kernel build tree like this:
>>
>> It is in my to-do list to get kunit working on my setups (Artix Linux on
>> dev and DUTs), so it is really something that will take rest of the year
>> easily ;)
>>
>>>
>>> make mrproper
>>> ./tools/testing/kunit/kunit.py run --kunitconfig=tools/testing/kunit/
>>> configs/all_tests.config
>>
>>>
>>
> The kunit tests can run on the computer you use to build the kernel. It
> takes a couple of minutes to run.
> 
> On my desktop box with vanilla 6.19-rc1:
> 
> rf@debianbox:~/work/kernel/linux$ make mrproper
> rf@debianbox:~/work/kernel/linux$ time ./tools/testing/kunit/kunit.py
> run --alltests

well, I don't have kunit test setup, so it fails, but I can build the
kernel and install on DUT and load the module:
[  159.843361]         ok 97 single volsw_sx: 0,0 -> range: 15->4(0), sign: 0, inv: 0 -> 0xf,0x0
[  159.843499]     # soc_ops_test_access: EXPECTATION FAILED at sound/soc/soc-ops-test.c:520
                   Expected result->value.integer.value[0] == param->lctl, but
                       result->value.integer.value[0] == 0 (0x0)
                       param->lctl == 1 (0x1)
[  159.843641]         not ok 98 single volsw_sx: 1,1 -> range: 15->4(0), sign: 0, inv: 0 -> 0x0,0x0

I guess, this implies that this patch is indeed does not work.

Tested the other patch and that works, sent my tested-by for it.

> 
> <SNIP>
> 
> [13:12:31] Testing complete. Ran 7154 tests: passed: 7082, failed: 36,
> skipped: 36
> [13:12:31] Elapsed time: 110.250s total, 3.072s configuring, 55.003s
> building, 52.119s running
> 
> real    1m50.314s
> user    15m52.878s
> sys    1m18.186s
> 

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Wed, Dec 17, 2025 at 03:54:43PM +0200, Péter Ujfalusi wrote:

> > rf@debianbox:~/work/kernel/linux$ time ./tools/testing/kunit/kunit.py
> > run --alltests

> well, I don't have kunit test setup, so it fails, but I can build the

What do you mean by "kunit test setup" - that's just a self contained
Python script that drives everything, it has minimal Python
requirements.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 15:56, Mark Brown wrote:
> On Wed, Dec 17, 2025 at 03:54:43PM +0200, Péter Ujfalusi wrote:
> 
>>> rf@debianbox:~/work/kernel/linux$ time ./tools/testing/kunit/kunit.py
>>> run --alltests
> 
>> well, I don't have kunit test setup, so it fails, but I can build the
> 
> What do you mean by "kunit test setup" - that's just a self contained
> Python script that drives everything, it has minimal Python
> requirements.

I'm holding it wrong I'm sure:

[15:35:21] Configuring KUnit Kernel ...
[15:35:21] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json scripts_gdb ARCH=um O=.kunit --jobs=14
[15:35:30] Starting KUnit Kernel (1/1)...
[15:35:30] ============================================================
Running tests with:
$ .kunit/linux kunit.enable=1 mem=1G console=tty kunit_shutdown=halt
[15:35:30] [ERROR] Test: <missing>: Could not find any KTAP output. Did any KUnit tests run?
[15:35:30] ============================================================
[15:35:30] Testing complete. Ran 0 tests: errors: 1
[15:35:30] Elapsed time: 9.328s total, 0.001s configuring, 9.321s building, 0.006s running

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Wed, Dec 17, 2025 at 03:59:02PM +0200, Péter Ujfalusi wrote:
> On 17/12/2025 15:56, Mark Brown wrote:

> > What do you mean by "kunit test setup" - that's just a self contained
> > Python script that drives everything, it has minimal Python
> > requirements.

> I'm holding it wrong I'm sure:

> [15:35:21] Configuring KUnit Kernel ...
> [15:35:21] Building KUnit Kernel ...
> Populating config with:
> $ make ARCH=um O=.kunit olddefconfig

What did you actually run there?  Did you somehow have an existing
.config that it was picking up with everything turned off?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 16:00, Mark Brown wrote:
> On Wed, Dec 17, 2025 at 03:59:02PM +0200, Péter Ujfalusi wrote:
>> On 17/12/2025 15:56, Mark Brown wrote:
> 
>>> What do you mean by "kunit test setup" - that's just a self contained
>>> Python script that drives everything, it has minimal Python
>>> requirements.
> 
>> I'm holding it wrong I'm sure:
> 
>> [15:35:21] Configuring KUnit Kernel ...
>> [15:35:21] Building KUnit Kernel ...
>> Populating config with:
>> $ make ARCH=um O=.kunit olddefconfig
> 
> What did you actually run there?  Did you somehow have an existing
> .config that it was picking up with everything turned off?
git clean -xdf
make mrproper

./tools/testing/kunit/kunit.py run --alltests
[16:09:25] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[16:09:30] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json scripts_gdb ARCH=um O=.kunit --jobs=14
[16:12:52] Starting KUnit Kernel (1/1)...
[16:12:52] ============================================================
Running tests with:
$ .kunit/linux kunit.enable=1 mem=1G console=tty kunit_shutdown=halt
[16:12:52] [ERROR] Test: <missing>: Could not find any KTAP output. Did any KUnit tests run?
[16:12:52] ============================================================
[16:12:52] Testing complete. Ran 0 tests: errors: 1
[16:12:52] Elapsed time: 206.260s total, 4.835s configuring, 201.418s building, 0.007s running

well, this is for the cold and dark winter days.. ;)

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

On Wed, Dec 17, 2025 at 04:19:31PM +0200, Péter Ujfalusi wrote:
> On 17/12/2025 16:00, Mark Brown wrote:

> > What did you actually run there?  Did you somehow have an existing
> > .config that it was picking up with everything turned off?
> git clean -xdf
> make mrproper

> ./tools/testing/kunit/kunit.py run --alltests

Interesting, that works happily for me.  You might try running in qemu
by specifying --arch I guess, but generally I'd report that to the kunit
people.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 409 bytes --]

On Wed, Dec 17, 2025 at 12:36:39PM +0000, Richard Fitzgerald wrote:

> The ASoC kunit tests have specific tests for SX controls.
> The original patch failed those tests, but it was merged anyway.

The issue there is that these tests use weird names in the form

   single volsw_sx: 0,0 -> range: 15->4(0), sign: 0, inv: 0 -> 0xf,0x0

which was causing the tests to just not get matched when checking
results.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 14:06, Peter Ujfalusi wrote:
> In 'normal' controls the mc->min is the minimum value the register can
> have, the mc->max is the maximum (the steps between are max - min).
> 
> SX types are defined differently: mc->min is the minimum value and the
> mc->max is the steps.
> 
> The max parameter of soc_mixer_reg_to_ctl() is the number of steps in
> either type.
> 
> To have correct register value range in clamp the maximum value that needs
> to be used is mc->min + max, which will be equal to mc->max for 'normal'
> controls and mc->min + mc->max for SX ones.
> 
> The original clamp broke SX controls and rendered some of them impossible
> to even set, like the cs42l43's Headphone Digital Volume, where the
> min is smaller than the max (min=283, max=229 - 229 steps starting from

when the min is bigger than the max, sorry, will resend with corrected
message

> val 283).
> 
> The soc_mixer_ctl_to_reg() correctly uses the max parameter instead of
> mc->max, so storing the value was correct.
> 
> Fixes: a0ce874cfaaa ("ASoC: ops: improve snd_soc_get_volsw")
> Cc: stable@vger.kernel.org
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
>  sound/soc/soc-ops.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
> index ce86978c158d..6a18c56a9746 100644
> --- a/sound/soc/soc-ops.c
> +++ b/sound/soc/soc-ops.c
> @@ -148,7 +148,7 @@ static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_v
>  	if (mc->sign_bit)
>  		val = sign_extend32(val, mc->sign_bit);
>  
> -	val = clamp(val, mc->min, mc->max);
> +	val = clamp(val, mc->min, mc->min + max);
>  	val -= mc->min;
>  
>  	if (mc->invert)

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Charles Keepax]

On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
>  sound/soc/soc-ops.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
> index ce86978c158d..6a18c56a9746 100644
> --- a/sound/soc/soc-ops.c
> +++ b/sound/soc/soc-ops.c
> @@ -148,7 +148,7 @@ static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_v
>  	if (mc->sign_bit)
>  		val = sign_extend32(val, mc->sign_bit);
>  
> -	val = clamp(val, mc->min, mc->max);
> +	val = clamp(val, mc->min, mc->min + max);

This won't work, for an SX control it is perfectly valid for
the value read from the register to be smaller than the minimum
value specified in the control.

The minimum value gives the register value that equates to the
smallest possible control value. From there the values increase
but the register field can overflow and end up lower than the
min. I often think of it in terms of a 2's compliement number
with an implicit sign bit.

Thanks,
Charles

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 14:47, Charles Keepax wrote:
> On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
>>  sound/soc/soc-ops.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
>> index ce86978c158d..6a18c56a9746 100644
>> --- a/sound/soc/soc-ops.c
>> +++ b/sound/soc/soc-ops.c
>> @@ -148,7 +148,7 @@ static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_v
>>  	if (mc->sign_bit)
>>  		val = sign_extend32(val, mc->sign_bit);
>>  
>> -	val = clamp(val, mc->min, mc->max);
>> +	val = clamp(val, mc->min, mc->min + max);
> 
> This won't work, for an SX control it is perfectly valid for
> the value read from the register to be smaller than the minimum
> value specified in the control.

Hrm, so an SX control returns sort of rand() and the value have no
correlation to min or max?

The info() gives this explanation:
SX TLV controls have a range that represents both positive and negative
values either side of zero but without a sign bit. min is the minimum
register value, max is the number of steps.

which kind of makes some sense, but really fuzzy.
> The minimum value gives the register value that equates to the
> smallest possible control value.

right, so min is min, OK

> From there the values increase
> but the register field can overflow and end up lower than the
> min.

smaller value than min is then bigger than min, right?
The value can wrap at any random value to 0 and continue from 0 up to
some value, which is the max?
max (which is the number of steps) = vrap_point-min + some_value (where
the value is likely less than min, but not specified)

How this is in practice for the cs42l43' Headphone Digital Volume?
SOC_DOUBLE_SX_TLV("Headphone Digital Volume", CS42L43_HPPATHVOL,
	  CS42L43_AMP3_PATH_VOL_SHIFT, CS42L43_AMP4_PATH_VOL_SHIFT,
	  0x11B, 229, cs42l43_headphone_tlv),

min=283
max=229
shifts: 0 and 16
masks are 0x1ff

if you step 229 from 283 then you reach 0x1ff, this is the max the mask
can cover.

> I often think of it in terms of a 2's compliement number
> with an implicit sign bit.

I see, but why???

> 
> Thanks,
> Charles

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Charles Keepax]

On Wed, Dec 17, 2025 at 03:13:45PM +0200, Péter Ujfalusi wrote:
> On 17/12/2025 14:47, Charles Keepax wrote:
> > On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
> >>  sound/soc/soc-ops.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
> >> index ce86978c158d..6a18c56a9746 100644
> >> --- a/sound/soc/soc-ops.c
> >> +++ b/sound/soc/soc-ops.c
> >> @@ -148,7 +148,7 @@ static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_v
> >>  	if (mc->sign_bit)
> >>  		val = sign_extend32(val, mc->sign_bit);
> >>  
> >> -	val = clamp(val, mc->min, mc->max);
> >> +	val = clamp(val, mc->min, mc->min + max);
> > 
> > This won't work, for an SX control it is perfectly valid for
> > the value read from the register to be smaller than the minimum
> > value specified in the control.
> 
> Hrm, so an SX control returns sort of rand() and the value have no
> correlation to min or max?

lol, yes exactly :-) arn't they great

> The value can wrap at any random value to 0 and continue from 0 up to
> some value, which is the max?

Mostly correct, not any random value it wraps at the mask.

> How this is in practice for the cs42l43' Headphone Digital Volume?
> SOC_DOUBLE_SX_TLV("Headphone Digital Volume", CS42L43_HPPATHVOL,
> 	  CS42L43_AMP3_PATH_VOL_SHIFT, CS42L43_AMP4_PATH_VOL_SHIFT,
> 	  0x11B, 229, cs42l43_headphone_tlv),
> 
> min=283
> max=229
> shifts: 0 and 16
> masks are 0x1ff
> 
> if you step 229 from 283 then you reach 0x1ff, this is the max the mask
> can cover.

Not quite your maths is off by one, 229 + 283 = 512 = 0x200,
which is then &ed with the mask to get 0x0. Which on the cs42l43
headphones a value of 0x0->0dB. Stepping 1 back from that would
give you 0x1FF->-0.5dB.

> > I often think of it in terms of a 2's compliement number
> > with an implicit sign bit.
> 
> I see, but why???

Mostly because hardware people love to wind me up, I assume. But
more seriously, imagine an 4-bit signed number volume control
with 5 values:

0xE -> -2 -> -2dB
0xF -> -1 -> -1dB
0x0 ->  0 -> 0dB
0x1 ->  1 -> 1dB
0x2 ->  2 -> 2dB

Super, a very sensible control, but wait being a good hardware
engineer you realise you don't need 4 bits to represent 5 values
you can get away with 3 bits for that and save like 2 gates
resulting in an ice cream and a plaque from your manager. So
you drop the sign bit giving you:

0x6 -> -2dB
0x7 -> -1dB
0x0 -> 0dB
0x1 -> 1dB
0x2 -> 2dB

This then results in an SX control with a minimum of 0x6 and a
mask of 0x7.

Thanks,
Charles

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Péter Ujfalusi]

On 17/12/2025 15:42, Charles Keepax wrote:
> On Wed, Dec 17, 2025 at 03:13:45PM +0200, Péter Ujfalusi wrote:
>> On 17/12/2025 14:47, Charles Keepax wrote:
>>> On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
>>>>  sound/soc/soc-ops.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
>>>> index ce86978c158d..6a18c56a9746 100644
>>>> --- a/sound/soc/soc-ops.c
>>>> +++ b/sound/soc/soc-ops.c
>>>> @@ -148,7 +148,7 @@ static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_v
>>>>  	if (mc->sign_bit)
>>>>  		val = sign_extend32(val, mc->sign_bit);
>>>>  
>>>> -	val = clamp(val, mc->min, mc->max);
>>>> +	val = clamp(val, mc->min, mc->min + max);
>>>
>>> This won't work, for an SX control it is perfectly valid for
>>> the value read from the register to be smaller than the minimum
>>> value specified in the control.
>>
>> Hrm, so an SX control returns sort of rand() and the value have no
>> correlation to min or max?
> 
> lol, yes exactly :-) arn't they great
> 
>> The value can wrap at any random value to 0 and continue from 0 up to
>> some value, which is the max?
> 
> Mostly correct, not any random value it wraps at the mask.
> 
>> How this is in practice for the cs42l43' Headphone Digital Volume?
>> SOC_DOUBLE_SX_TLV("Headphone Digital Volume", CS42L43_HPPATHVOL,
>> 	  CS42L43_AMP3_PATH_VOL_SHIFT, CS42L43_AMP4_PATH_VOL_SHIFT,
>> 	  0x11B, 229, cs42l43_headphone_tlv),
>>
>> min=283
>> max=229
>> shifts: 0 and 16
>> masks are 0x1ff
>>
>> if you step 229 from 283 then you reach 0x1ff, this is the max the mask
>> can cover.
> 
> Not quite your maths is off by one, 229 + 283 = 512 = 0x200,
> which is then &ed with the mask to get 0x0. Which on the cs42l43
> headphones a value of 0x0->0dB. Stepping 1 back from that would
> give you 0x1FF->-0.5dB.
> 
>>> I often think of it in terms of a 2's compliement number
>>> with an implicit sign bit.
>>
>> I see, but why???
> 
> Mostly because hardware people love to wind me up, I assume. But
> more seriously, imagine an 4-bit signed number volume control
> with 5 values:
> 
> 0xE -> -2 -> -2dB
> 0xF -> -1 -> -1dB
> 0x0 ->  0 -> 0dB
> 0x1 ->  1 -> 1dB
> 0x2 ->  2 -> 2dB
> 
> Super, a very sensible control, but wait being a good hardware
> engineer you realise you don't need 4 bits to represent 5 values
> you can get away with 3 bits for that and save like 2 gates
> resulting in an ice cream and a plaque from your manager. So
> you drop the sign bit giving you:
> 
> 0x6 -> -2dB
> 0x7 -> -1dB
> 0x0 -> 0dB
> 0x1 -> 1dB
> 0x2 -> 2dB

I must say, wow.
Being a SW guy I would probably done this differently:
0x0 -> -2dB
0x1 -> -1dB
0x2 -> 0dB
0x3 -> 1dB
0x4 -> 2dB

> This then results in an SX control with a minimum of 0x6 and a
> mask of 0x7.

then the comment at info() is hard to match still.

static const DECLARE_TLV_DB_RANGE(sx_thing,
	6, 7, TLV_DB_SCALE_ITEM(-2000, -1000, 0),
	0, 2, TLV_DB_SCALE_ITEM(0, 1000, 0)
};

is sort of the same, no?

Thanks for the explanation, fascinating!

-- 
Péter

Re: [PATCH] ASoC: soc-ops: Correct the max value for clamp in soc_mixer_reg_to_ctl()

[Charles Keepax]

On Wed, Dec 17, 2025 at 04:31:37PM +0200, Péter Ujfalusi wrote:
> On 17/12/2025 15:42, Charles Keepax wrote:
> > On Wed, Dec 17, 2025 at 03:13:45PM +0200, Péter Ujfalusi wrote:
> >> On 17/12/2025 14:47, Charles Keepax wrote:
> >>> On Wed, Dec 17, 2025 at 02:06:23PM +0200, Peter Ujfalusi wrote:
> > you drop the sign bit giving you:
> > 
> > 0x6 -> -2dB
> > 0x7 -> -1dB
> > 0x0 -> 0dB
> > 0x1 -> 1dB
> > 0x2 -> 2dB
> 
> I must say, wow.
> Being a SW guy I would probably done this differently:
> 0x0 -> -2dB
> 0x1 -> -1dB
> 0x2 -> 0dB
> 0x3 -> 1dB
> 0x4 -> 2dB

Yes that is exactly what I would have done too :-) but then
that is probably why we arn't hardware guys, would make life too
easy for the software guys.

> > This then results in an SX control with a minimum of 0x6 and a
> > mask of 0x7.
> 
> then the comment at info() is hard to match still.

Yeah I think the wording "min is the minimum register value" is
perhaps slightly misleading. It is the register value that equates to the
lowest volume, but that isn't necessarily the minimum value that
can be written into the register.

If I can find a spare minute I will ping up a patch to tweak
that.

> static const DECLARE_TLV_DB_RANGE(sx_thing,
> 	6, 7, TLV_DB_SCALE_ITEM(-2000, -1000, 0),
> 	0, 2, TLV_DB_SCALE_ITEM(0, 1000, 0)
> };
> 
> is sort of the same, no?

Similar but some issues, I think it would let you write values
that arn't valid. The control would be fairly confusing to a
manual user, as the values are not strictly increasing. And I
would imagine it blows up most user-space stuff, which likely
also assumes all values are valid and strictly increasing.

> Thanks for the explanation, fascinating!

No problem, I would be a rich man if I had a pound for each
minute I have spent chasing SX control gremlins.

Thanks,
Charles