[PATCH] ASoC: SDCA: support Q7.8 volume format

[PATCH] ASoC: SDCA: support Q7.8 volume format

[shumingf]

From: Shuming Fan <shumingf@realtek.com>

The SDCA specification uses Q7.8 volume format.
This patch adds a field to indicate whether it is SDCA volume control
and supports the volume settings.

Signed-off-by: Shuming Fan <shumingf@realtek.com>
---
 include/sound/soc.h        |  1 +
 sound/soc/sdca/sdca_asoc.c | 34 ++++++---------------
 sound/soc/soc-ops.c        | 62 +++++++++++++++++++++++++++++++-------
 3 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 1aebf14fcf80..53b4129ee97a 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1225,6 +1225,7 @@ struct soc_mixer_control {
 	unsigned int sign_bit;
 	unsigned int invert:1;
 	unsigned int autodisable:1;
+	unsigned int sdca_q78:1;
 #ifdef CONFIG_SND_SOC_TOPOLOGY
 	struct snd_soc_dobj dobj;
 #endif
diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
index c493ec530cc5..892b7c028fae 100644
--- a/sound/soc/sdca/sdca_asoc.c
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -795,7 +795,6 @@ static int control_limit_kctl(struct device *dev,
 	struct sdca_control_range *range;
 	int min, max, step;
 	unsigned int *tlv;
-	int shift;
 
 	if (control->type != SDCA_CTL_DATATYPE_Q7P8DB)
 		return 0;
@@ -814,37 +813,22 @@ static int control_limit_kctl(struct device *dev,
 	min = sign_extend32(min, control->nbits - 1);
 	max = sign_extend32(max, control->nbits - 1);
 
-	/*
-	 * FIXME: Only support power of 2 step sizes as this can be supported
-	 * by a simple shift.
-	 */
-	if (hweight32(step) != 1) {
-		dev_err(dev, "%s: %s: currently unsupported step size\n",
-			entity->label, control->label);
-		return -EINVAL;
-	}
-
-	/*
-	 * The SDCA volumes are in steps of 1/256th of a dB, a step down of
-	 * 64 (shift of 6) gives 1/4dB. 1/4dB is the smallest unit that is also
-	 * representable in the ALSA TLVs which are in 1/100ths of a dB.
-	 */
-	shift = max(ffs(step) - 1, 6);
-
 	tlv = devm_kcalloc(dev, 4, sizeof(*tlv), GFP_KERNEL);
 	if (!tlv)
 		return -ENOMEM;
 
-	tlv[0] = SNDRV_CTL_TLVT_DB_SCALE;
+	tlv[0] = SNDRV_CTL_TLVT_DB_MINMAX;
 	tlv[1] = 2 * sizeof(*tlv);
 	tlv[2] = (min * 100) >> 8;
-	tlv[3] = ((1 << shift) * 100) >> 8;
+	tlv[3] = (max * 100) >> 8;
+
+	step = (step * 100) >> 8;
 
-	mc->min = min >> shift;
-	mc->max = max >> shift;
-	mc->shift = shift;
-	mc->rshift = shift;
-	mc->sign_bit = 15 - shift;
+	mc->min = ((int)tlv[2] / step);
+	mc->max = ((int)tlv[3] / step);
+	mc->shift = step;
+	mc->sign_bit = 15;
+	mc->sdca_q78 = 1;
 
 	kctl->tlv.p = tlv;
 	kctl->access |= SNDRV_CTL_ELEM_ACCESS_TLV_READ;
diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index d2b6fb8e0b6c..6dc961253947 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -110,6 +110,36 @@ 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)
+{
+	int val = reg_val;
+
+	if (WARN_ON(!mc->shift))
+		return -EINVAL;
+
+	val = sign_extend32(val, mc->sign_bit);
+	val = (((val * 100) >> 8) / (int)mc->shift);
+	val -= mc->min;
+
+	return val & mask;
+}
+
+static unsigned int sdca_soc_q78_ctl_to_reg(struct soc_mixer_control *mc, int val,
+					 unsigned int mask, unsigned int shift, int max)
+{
+	unsigned int ret_val;
+	int reg_val;
+
+	if (WARN_ON(!mc->shift))
+		return -EINVAL;
+
+	reg_val = val + mc->min;
+	ret_val = (int)((reg_val * mc->shift) << 8) / 100;
+
+	return ret_val & mask;
+}
+
 static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_val,
 				unsigned int mask, unsigned int shift, int max)
 {
@@ -202,14 +232,22 @@ static int soc_put_volsw(struct snd_kcontrol *kcontrol,
 	unsigned int val2 = 0;
 	bool double_r = false;
 	int ret;
+	unsigned int (*ctl_to_reg)(struct soc_mixer_control *, int, unsigned int, unsigned int, int);
+
+	if (mc->sdca_q78) {
+		ctl_to_reg = sdca_soc_q78_ctl_to_reg;
+		val_mask = mask;
+	} else {
+		ctl_to_reg = soc_mixer_ctl_to_reg;
+		val_mask = mask << mc->shift;
+	}
 
 	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0], max);
 	if (ret)
 		return ret;
 
-	val1 = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[0],
+	val1 = ctl_to_reg(mc, ucontrol->value.integer.value[0],
 				    mask, mc->shift, max);
-	val_mask = mask << mc->shift;
 
 	if (snd_soc_volsw_is_stereo(mc)) {
 		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1], max);
@@ -217,14 +255,10 @@ static int soc_put_volsw(struct snd_kcontrol *kcontrol,
 			return ret;
 
 		if (mc->reg == mc->rreg) {
-			val1 |= soc_mixer_ctl_to_reg(mc,
-						     ucontrol->value.integer.value[1],
-						     mask, mc->rshift, max);
+			val1 |= ctl_to_reg(mc, ucontrol->value.integer.value[1], mask, mc->rshift, max);
 			val_mask |= mask << mc->rshift;
 		} else {
-			val2 = soc_mixer_ctl_to_reg(mc,
-						    ucontrol->value.integer.value[1],
-						    mask, mc->shift, max);
+			val2 = ctl_to_reg(mc, ucontrol->value.integer.value[1], mask, mc->shift, max);
 			double_r = true;
 		}
 	}
@@ -251,18 +285,24 @@ static int soc_get_volsw(struct snd_kcontrol *kcontrol,
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	unsigned int reg_val;
 	int val;
+	int (*reg_to_ctl)(struct soc_mixer_control *, unsigned int, unsigned int, unsigned int, int);
+
+	if (mc->sdca_q78)
+		reg_to_ctl = sdca_soc_q78_reg_to_ctl;
+	else
+		reg_to_ctl = soc_mixer_reg_to_ctl;
 
 	reg_val = snd_soc_component_read(component, mc->reg);
-	val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
+	val = reg_to_ctl(mc, reg_val, mask, mc->shift, max);
 
 	ucontrol->value.integer.value[0] = val;
 
 	if (snd_soc_volsw_is_stereo(mc)) {
 		if (mc->reg == mc->rreg) {
-			val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->rshift, max);
+			val = reg_to_ctl(mc, reg_val, mask, mc->rshift, max);
 		} else {
 			reg_val = snd_soc_component_read(component, mc->rreg);
-			val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
+			val = reg_to_ctl(mc, reg_val, mask, mc->shift, max);
 		}
 
 		ucontrol->value.integer.value[1] = val;
-- 
2.51.1

Re: [PATCH] ASoC: SDCA: support Q7.8 volume format

[Charles Keepax]

On Wed, Nov 05, 2025 at 05:11:22PM +0800, shumingf@realtek.com wrote:
> From: Shuming Fan <shumingf@realtek.com>
> 
> The SDCA specification uses Q7.8 volume format.
> This patch adds a field to indicate whether it is SDCA volume control
> and supports the volume settings.
> 
> Signed-off-by: Shuming Fan <shumingf@realtek.com>
> ---
>  include/sound/soc.h        |  1 +
>  sound/soc/sdca/sdca_asoc.c | 34 ++++++---------------
>  sound/soc/soc-ops.c        | 62 +++++++++++++++++++++++++++++++-------

Mark, how do you feel about putting this in soc-ops? I think I
am good with putting it here, the code is definitely a little
smaller than open coding helpers in the SDCA code. However, I
do have a slight doubt that this doesn't have great reusability
outside of SDCA.

> +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)
> +{
> +	int val = reg_val;
> +
> +	if (WARN_ON(!mc->shift))

We should use the function argument for shift in these functions.

>  static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_val,
>  				unsigned int mask, unsigned int shift, int max)
>  {
> @@ -202,14 +232,22 @@ static int soc_put_volsw(struct snd_kcontrol *kcontrol,
>  	unsigned int val2 = 0;
>  	bool double_r = false;
>  	int ret;
> +	unsigned int (*ctl_to_reg)(struct soc_mixer_control *, int, unsigned int, unsigned int, int);

Would be nice to put this at the top of the variable list, given
the length.

Thanks,
Charles

Re: [PATCH] ASoC: SDCA: support Q7.8 volume format

[Mark Brown]

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

On Wed, Nov 05, 2025 at 02:25:27PM +0000, Charles Keepax wrote:
> On Wed, Nov 05, 2025 at 05:11:22PM +0800, shumingf@realtek.com wrote:

> > The SDCA specification uses Q7.8 volume format.
> > This patch adds a field to indicate whether it is SDCA volume control
> > and supports the volume settings.

> Mark, how do you feel about putting this in soc-ops? I think I
> am good with putting it here, the code is definitely a little
> smaller than open coding helpers in the SDCA code. However, I
> do have a slight doubt that this doesn't have great reusability
> outside of SDCA.

Yeah, it's a bit unclear but probably fine.  The functions aren't super
complex so it's not really a problem other than the small extra code
size for non-SDCA systems but there's already a bunch of niche helpers
that aren't used by that many systems so meh.

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

RE: [PATCH] ASoC: SDCA: support Q7.8 volume format

[Shuming [范書銘]]

> > +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) {
> > +     int val = reg_val;
> > +
> > +     if (WARN_ON(!mc->shift))
> 
> We should use the function argument for shift in these functions.

It is intentional to do this. 
The patch only uses the 'shift' field to store the step value and the 'rshift' field remains zero.
Therefore, we could reuse the function 'soc_put_volsw' and generate the correct result for stereo volume.

> >  static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int
> reg_val,
> >                               unsigned int mask, unsigned int shift,
> > int max)  { @@ -202,14 +232,22 @@ static int soc_put_volsw(struct
> > snd_kcontrol *kcontrol,
> >       unsigned int val2 = 0;
> >       bool double_r = false;
> >       int ret;
> > +     unsigned int (*ctl_to_reg)(struct soc_mixer_control *, int,
> > + unsigned int, unsigned int, int);
> 
> Would be nice to put this at the top of the variable list, given the length.

Will fix

Re: [PATCH] ASoC: SDCA: support Q7.8 volume format

[Charles Keepax]

On Thu, Nov 06, 2025 at 02:30:17AM +0000, Shuming [范書銘] wrote:
> > > +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) {
> > > +     int val = reg_val;
> > > +
> > > +     if (WARN_ON(!mc->shift))
> > 
> > We should use the function argument for shift in these functions.
> 
> It is intentional to do this. 
> The patch only uses the 'shift' field to store the step value and the 'rshift' field remains zero.
> Therefore, we could reuse the function 'soc_put_volsw' and generate the correct result for stereo volume.

Ah ok that does make sense, perhaps a comment for this would be
good.

> > >  static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int
> > reg_val,
> > >                               unsigned int mask, unsigned int shift,
> > > int max)  { @@ -202,14 +232,22 @@ static int soc_put_volsw(struct
> > > snd_kcontrol *kcontrol,
> > >       unsigned int val2 = 0;
> > >       bool double_r = false;
> > >       int ret;
> > > +     unsigned int (*ctl_to_reg)(struct soc_mixer_control *, int,
> > > + unsigned int, unsigned int, int);
> > 
> > Would be nice to put this at the top of the variable list, given the length.
> 
> Will fix

Thanks,
Charles