Re: [PATCH v1] ASoC: ops: fix snd_soc_get_volsw for sx controls

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

On 16/12/2025 15:49, Stefan Binding wrote:
> SX controls are currently broken, since the clamp introduced in
> commit a0ce874cfaaa ("ASoC: ops: improve snd_soc_get_volsw") does not
> handle SX controls, for example where the min value in the clamp is
> greater than the max value in the clamp.
> 
> Add clamp parameter to prevent clamping in SX controls.
> The nature of SX controls mean that it wraps around 0, with a variable
> number of bits, therefore clamping the value becomes complicated and
> prone to error.
> 
> Fixes 35 kunit tests for soc_ops_test_access.
> 
> Fixes: a0ce874cfaaa ("ASoC: ops: improve snd_soc_get_volsw")
> 
> Co-developed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>

Tested-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

> ---
>  sound/soc/soc-ops.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
> index ce86978c158d..624e9269fc25 100644
> --- a/sound/soc/soc-ops.c
> +++ b/sound/soc/soc-ops.c
> @@ -111,7 +111,8 @@ int snd_soc_put_enum_double(struct snd_kcontrol *kcontrol,
>  EXPORT_SYMBOL_GPL(snd_soc_put_enum_double);
>  
>  static int sdca_soc_q78_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_val,
> -				unsigned int mask, unsigned int shift, int max)
> +				   unsigned int mask, unsigned int shift, int max,
> +				   bool sx)
>  {
>  	int val = reg_val;
>  
> @@ -141,20 +142,26 @@ static unsigned int sdca_soc_q78_ctl_to_reg(struct soc_mixer_control *mc, int va
>  }
>  
>  static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_val,
> -				unsigned int mask, unsigned int shift, int max)
> +				unsigned int mask, unsigned int shift, int max,
> +				bool sx)
>  {
>  	int val = (reg_val >> shift) & mask;
>  
>  	if (mc->sign_bit)
>  		val = sign_extend32(val, mc->sign_bit);
>  
> -	val = clamp(val, mc->min, mc->max);
> -	val -= mc->min;
> +	if (sx) {
> +		val -= mc->min; // SX controls intentionally can overflow here
> +		val = min_t(unsigned int, val & mask, max);
> +	} else {
> +		val = clamp(val, mc->min, mc->max);
> +		val -= mc->min;
> +	}
>  
>  	if (mc->invert)
>  		val = max - val;
>  
> -	return val & mask;
> +	return val;
>  }
>  
>  static unsigned int soc_mixer_ctl_to_reg(struct soc_mixer_control *mc, int val,
> @@ -280,9 +287,10 @@ static int soc_put_volsw(struct snd_kcontrol *kcontrol,
>  
>  static int soc_get_volsw(struct snd_kcontrol *kcontrol,
>  			 struct snd_ctl_elem_value *ucontrol,
> -			 struct soc_mixer_control *mc, int mask, int max)
> +			 struct soc_mixer_control *mc, int mask, int max, bool sx)
>  {
> -	int (*reg_to_ctl)(struct soc_mixer_control *, unsigned int, unsigned int, unsigned int, int);
> +	int (*reg_to_ctl)(struct soc_mixer_control *, unsigned int, unsigned int,
> +			  unsigned int, int, bool);
>  	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>  	unsigned int reg_val;
>  	int val;
> @@ -293,16 +301,16 @@ static int soc_get_volsw(struct snd_kcontrol *kcontrol,
>  		reg_to_ctl = soc_mixer_reg_to_ctl;
>  
>  	reg_val = snd_soc_component_read(component, mc->reg);
> -	val = reg_to_ctl(mc, reg_val, mask, mc->shift, max);
> +	val = reg_to_ctl(mc, reg_val, mask, mc->shift, max, sx);
>  
>  	ucontrol->value.integer.value[0] = val;
>  
>  	if (snd_soc_volsw_is_stereo(mc)) {
>  		if (mc->reg == mc->rreg) {
> -			val = reg_to_ctl(mc, reg_val, mask, mc->rshift, max);
> +			val = reg_to_ctl(mc, reg_val, mask, mc->rshift, max, sx);
>  		} else {
>  			reg_val = snd_soc_component_read(component, mc->rreg);
> -			val = reg_to_ctl(mc, reg_val, mask, mc->shift, max);
> +			val = reg_to_ctl(mc, reg_val, mask, mc->shift, max, sx);
>  		}
>  
>  		ucontrol->value.integer.value[1] = val;
> @@ -371,7 +379,7 @@ int snd_soc_get_volsw(struct snd_kcontrol *kcontrol,
>  		(struct soc_mixer_control *)kcontrol->private_value;
>  	unsigned int mask = soc_mixer_mask(mc);
>  
> -	return soc_get_volsw(kcontrol, ucontrol, mc, mask, mc->max - mc->min);
> +	return soc_get_volsw(kcontrol, ucontrol, mc, mask, mc->max - mc->min, false);
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_get_volsw);
>  
> @@ -413,7 +421,7 @@ int snd_soc_get_volsw_sx(struct snd_kcontrol *kcontrol,
>  		(struct soc_mixer_control *)kcontrol->private_value;
>  	unsigned int mask = soc_mixer_sx_mask(mc);
>  
> -	return soc_get_volsw(kcontrol, ucontrol, mc, mask, mc->max);
> +	return soc_get_volsw(kcontrol, ucontrol, mc, mask, mc->max, true);
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_get_volsw_sx);
>  

-- 
Péter