ASoC: SDCA: support Q7.8 volume format by shumingfan · Pull Request #5563 · thesofproject/linux

shumingfan · 2025-10-21T10:17:17Z

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.

changed log
v2: create sdca_soc_get_volsw/sdca_soc_put_volsw in the SDCA code. For SDCA volume control only.
v3: use a function pointer for ctl_to_reg/reg_to_ctl for Q78 format

charleskeepax · 2025-10-22T13:37:57Z

include/sound/soc.h

 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
+


Probably shouldn't be changing whitespace in unrelated places.

charleskeepax · 2025-10-22T13:39:37Z

include/sound/soc.h

 	unsigned int sign_bit;
 	unsigned int invert:1;
 	unsigned int autodisable:1;
+	unsigned int sdca_vol_q7p8:1;


I am a little nervous about pulling this into the ASoC core, have you looked at adding wrappers for snd_soc_get_volsw/snd_soc_put_volsw in the SDCA code, if it doesn't end up looking too messy I feel like that might be preferrable.

Understood. I will update the patch.

charleskeepax · 2025-10-22T13:42:45Z

sound/soc/sdca/sdca_asoc.c

+	mc->min = ((int)tlv[2] / step);
+	mc->max = ((int)tlv[3] / step);
+	mc->shift = step;
+	mc->rshift = step;


Are you sure you can just use step as the shift here? I don't think this works, step is the gap between valid values, but shift is an actual bit shift.

The shift will be used in the soc_mixer_reg_to_ctl/soc_mixer_ctl_to_reg function.
Do you think I should create another field to store the step value?

charleskeepax · 2025-10-23T09:36:31Z

include/sound/soc.h

 	/* Limited maximum value specified as presented through the control */
 	int platform_max;
 	int reg, rreg;
+	int step;


If we are duplicating the handlers we can probably reuse the shift member for step, rather than adding a new one. I was just unsure before as we were using shift as a shift sometimes and as a step other times.

charleskeepax · 2025-10-23T09:37:52Z

sound/soc/sdca/sdca_asoc.c

+	val -= mc->min;
+
+	if (mc->invert)
+		val = max - val;


Can probably drop the invert we dont use it for volume controls in SDCA.

charleskeepax · 2025-10-23T09:38:41Z

sound/soc/sdca/sdca_asoc.c

+		return -EINVAL;
+
+	if (mc->sign_bit)
+		val = sign_extend32(val, mc->sign_bit);


q7.8 will always have a sign bit so can probably drop the if here.

charleskeepax · 2025-10-23T09:39:06Z

sound/soc/sdca/sdca_asoc.c

 }
 EXPORT_SYMBOL_NS(sdca_asoc_populate_dapm, "SND_SOC_SDCA");

+static int sdca_soc_vol_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_val,


Maybe update the name to include q78 or something incase we need to as most custom handlers in the future.

charleskeepax · 2025-10-23T09:41:37Z

Do we definitely need to duplicate the helpers? In my mind I thought it might be easier to wrap them such that we just do a little pre-processing on the values then call the normal handler. Fine if that ends up being too complex but just a thought to consider.

shumingfan · 2025-10-27T11:25:27Z

Do we definitely need to duplicate the helpers? In my mind I thought it might be easier to wrap them such that we just do a little pre-processing on the values then call the normal handler. Fine if that ends up being too complex but just a thought to consider.

@charleskeepax I updated v3 patch. That patch only added reg_to_ctl/ctl_to_reg function for Q78 format in the soc-ops.c.
Could you take a look? Thanks.

shumingfan · 2025-11-04T02:39:30Z

@charleskeepax I updated v3 patch. That patch only added reg_to_ctl/ctl_to_reg function for Q78 format in the soc-ops.c. Could you take a look? Thanks.

@charleskeepax Would you mind reviewing the v3 patch? Thanks.

charleskeepax · 2025-11-04T16:20:33Z

sound/soc/soc-ops.c

-			val1 |= soc_mixer_ctl_to_reg(mc,
+			val1 |= ctl_to_reg(mc,
 						     ucontrol->value.integer.value[1],
 						     mask, mc->rshift, max);


We should sort the indentation on these.

charleskeepax · 2025-11-04T16:21:58Z

sound/soc/soc-ops.c

+		return -EINVAL;
+
+	reg_val = val + mc->min;
+	ret_val = (((int)(((int)reg_val * (int)mc->shift) << 8)) / 100);


Is all the casting necessary?

will reduce that

charleskeepax · 2025-11-04T16:24:01Z

I think this looks like it should work, still a little nervous about pushing changes into soc-ops for SDCA I am not sure it has the re-usability. I will have a poke see what it would look like if we pulled that in, but perhaps we should do the next bit of the review upstream see if Mark or anyone has any thoughts on that.

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>

shumingfan · 2025-11-05T07:28:46Z

I think this looks like it should work, still a little nervous about pushing changes into soc-ops for SDCA I am not sure it has the re-usability. I will have a poke see what it would look like if we pulled that in, but perhaps we should do the next bit of the review upstream see if Mark or anyone has any thoughts on that.

@charleskeepax Thanks for the review. Will send the patch and see if Mark or anyone has any suggestions.

bardliao · 2025-11-10T03:44:01Z

The PR is upstreamed

shumingfan requested a review from charleskeepax October 21, 2025 10:17

shumingfan requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart, ranj063 and ujfalusi as code owners October 21, 2025 10:17

charleskeepax reviewed Oct 22, 2025

View reviewed changes

shumingfan force-pushed the support-sdca-q7p8 branch from 4897262 to 27731dd Compare October 23, 2025 09:10

charleskeepax reviewed Oct 23, 2025

View reviewed changes

shumingfan force-pushed the support-sdca-q7p8 branch from 27731dd to 05dee72 Compare October 27, 2025 11:21

charleskeepax reviewed Nov 4, 2025

View reviewed changes

ASoC: SDCA: support Q7.8 volume format

c65c386

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>

shumingfan force-pushed the support-sdca-q7p8 branch from 05dee72 to c65c386 Compare November 5, 2025 07:24

bardliao closed this Nov 10, 2025

Conversation

shumingfan commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charleskeepax commented Oct 23, 2025

Uh oh!

shumingfan commented Oct 27, 2025

Uh oh!

shumingfan commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charleskeepax commented Nov 4, 2025

Uh oh!

shumingfan commented Nov 5, 2025

Uh oh!

bardliao commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shumingfan commented Oct 21, 2025 •

edited

Loading