[RFC] some patches updated during SDCA library testing by shumingfan · Pull Request #5469 · thesofproject/linux

shumingfan · 2025-06-30T07:37:56Z

This PR includes some patches I modified during the SDCA library testing
Any comments are welcome.

One initial setting is 5 bytes, so num_init_writes should divide by 5. Signed-off-by: Shuming Fan <shumingf@realtek.com>

…e proper position The reading mipi-sdca-control-cn-list property should be done before reading "mipi-sdca-control-dc-value" property. Signed-off-by: Shuming Fan <shumingf@realtek.com>

This patch checks the entity label to display the warning messages. Signed-off-by: Shuming Fan <shumingf@realtek.com>

This patch adds the put/get callback for Q7.8 volume format. Signed-off-by: Shuming Fan <shumingf@realtek.com>

The control number should not be equal to the number of input pins. Therefore, this patch removed the judgment. Signed-off-by: Shuming Fan <shumingf@realtek.com>

charleskeepax · 2025-07-07T13:19:27Z

Apologies for the delay in looking at these I have been on holiday.

charleskeepax · 2025-07-07T13:21:05Z

sound/soc/sdca/sdca_functions.c

-		control->has_fixed = true;
+			control->value = tmp;
+			control->has_fixed = true;
+		}


I do not believe this is correct, I think it is perfect legal in SDCA to use a dc-value on a control with more than a single control number, it just means all the control numbers share the same value. Is there a particular part of the spec you are thinking of here?

Oh, it is a Disco table issue on my side. Will remove this patch.

Oh, it is a Disco table issue on my side. Will remove this patch.

I checked again. One situation is that the control descriptor uses mipi-sdca-control-cn-< n>-dc-value property to define the DC value of control number.
This case should check mipi-sdca-control-cn-list property first, and then read mipi-sdca-control-cn-< n>-dc-value property, rather than mipi-sdca-control-dc-value.
Is it reasonable?

The problem here is that it is expected to use this property with controls that have multiple control numbers. To quote the description for mipi-sdca-control-dc-value in the DisCo spec "if all Control Numbers for this Control have the same DC value, then this Property shall be present.". We should probably at some point add support for mipi-sdca-control-cn-dc-value but it is only used for controls which have different values for each number.

Okay, we still need to support mipi-sdca-control-cn-< n>-dc-value property, right?
In Realtek codec, one control descriptor doesn't include mipi-sdca-control-dc-value, but using mipi-sdca-control-cn-< n>-dc-value to define the DC value for each control number.

Ok yeah then we will need to add support for sdca-control-cn--dc-value, but comment on the change here is just that we can't limit sdca-control-dc-value to only controls with a single cn. That shouldn't stop us adding support for cn-dc-value.

Could we do the modification like below? In my case, there is no mipi-sdca-control-dc-value property, and we don't support mipi-sdca-control-cn-< n>-dc-value property yet.

@@ -851,7 +851,7 @@ static int find_sdca_entity_control(struct device *dev, struct sdca_entity *enti if (ret) { dev_err(dev, "%s: control %#x: dc value missing: %d\n", entity->label, control->sel, ret); - return ret; + break;

I would rather not go that way as then you end up with controls that don't have defined values, try this patch I threw together:
charleskeepax@ec8f6f6
We don't have any of those -cn- values so that part isn't tested but reasonably sure I got most of it right.

I tried that patch, it looks good to me and works. Thanks.

Super I will add it to our upstreaming backlog.

charleskeepax · 2025-07-07T13:23:29Z

sound/soc/sdca/sdca_functions.c

+		    strstr(entity->label, "MFPU") || strstr(entity->label, "SMPU") ||
+		    strstr(entity->label, "SAPU") || strstr(entity->label, "PPU") ||
+		    strstr(entity->label, "SPE"))
+			dev_warn(dev, "%s: missing pin list\n", entity->label);


I actually just upstreamed a patch this morning to just remove this warning it is probably a bit overly cautious and it seems overly complex to carefully mask the entities like this.

Apologies and a link: https://lore.kernel.org/linux-sound/20250707124155.2596744-3-ckeepax@opensource.cirrus.com/T/#u

Okay, thanks for the fix.

charleskeepax · 2025-07-07T13:55:59Z

sound/soc/sdca/sdca_asoc.c

 		kctl[i].info = snd_soc_info_volsw;
 		kctl[i].get = snd_soc_dapm_get_volsw;
 		kctl[i].put = snd_soc_dapm_put_volsw;
-		i++;


I don't object to this change, being more resilient to the ACPI is probably good and there is actually no reason we need the control numbers and sources to match here.

Although as far as I read the spec there should be a control number for each connection in the mixer unit (6.3.4.3 and table 178 both seem to imply so). Perhaps a slight update to the commit message to be more clear this is adding resilience rather than changing a spec compliance issue, unless I have missed something in the spec where these are to differ?

charleskeepax · 2025-07-07T14:00:34Z

sound/soc/sdca/sdca_asoc.c

 	(*kctl)->info = snd_soc_info_volsw;
-	(*kctl)->get = snd_soc_get_volsw;
-	(*kctl)->put = snd_soc_put_volsw;
+	if (strstr(control_name, "Volume")) {


This should be done based of control->type instead of looking for strings in the name.

Thanks. Will fix.

charleskeepax · 2025-07-07T14:02:03Z

sound/soc/sdca/sdca_asoc.c

-	(*kctl)->put = snd_soc_put_volsw;
+	if (strstr(control_name, "Volume")) {
+		(*kctl)->get = sdca_get_volsw;
+		(*kctl)->put = sdca_put_volsw;


Something like q78_get/put_volsw would be better, in the future we will almost certainly add helpers for the other control types.

charleskeepax · 2025-07-07T14:14:26Z

sound/soc/sdca/sdca_asoc.c

 		return -ENOMEM;

-	mc = devm_kmalloc(dev, sizeof(*mc), GFP_KERNEL);
+	mc = devm_kzalloc(dev, sizeof(*mc), GFP_KERNEL);


Hmm... this might we worth a separate fix touching more of the allocations, looking through I think a few of the memory allocations should probably zalloc'ed.

charleskeepax · 2025-07-07T14:18:18Z

sound/soc/sdca/sdca_asoc.c

-	mc->rshift = shift;
-	mc->sign_bit = 15 - shift;
+	mc->min = 0;
+	mc->max = (max - min) / step;


I will need to spend a little more time to work through the maths on these ones. But my initial concern would be with rounding errors, are you relatively sure rounding works out sensible here? I guess ultimately user-space tends to just treat the control as a min to max control anyway so maybe it doesn't matter?

Ok... I think mostly this is fine since the number of steps is calculated after the calculation of the step size, so kinda compensates for the error in the step size. However, there is one corner case we probably need to think about which is the case where SDCA gives a step size below 1/100th of a dB this would currently result in a 0dB step size. So we still need to do some sort of capping on the step size. I would be a little tempted to keep the capping to 1/4dB steps but I could be persuaded otherwise if you really wanted a smaller cap.

Sorry, I don't understand.
Many volume controls use 0.75dB/step in Realtek codecs. How do I use 1/4dB steps?

Apologies for not being clear. I more thinking a minimum step size rather than a fixed step size, so it would be fine for you guys to have 0.75dB steps. Although it is only a suggestion and I am also open to other ways of tackling the problem. I think this problem might actually go away completely if we use the min/max version of the TLV as suggested in my other comment, because then the step size is implicit.

charleskeepax · 2025-07-08T13:21:19Z

sound/soc/sdca/sdca_asoc.c

+	if (value > 127996)
+		value = 127996;
+	else if (value < -128000)
+		value = -128000;


Use clamp_val() here.

charleskeepax · 2025-07-08T13:22:07Z

sound/soc/sdca/sdca_asoc.c

+
+static short tlv_value_to_q78(int value)
+{
+	short value16;


Be nicer to use int16_t rather than short.

charleskeepax · 2025-07-08T13:26:13Z

sound/soc/sdca/sdca_asoc.c

-	shift = max(ffs(step) - 1, 6);
+	min = min * 100 / SCALE_FACTOR;
+	max = max * 100 / SCALE_FACTOR;
+	step = step * 100 / SCALE_FACTOR;


I would be tempted to bracket the multiplies even though it isn't strictly necessary but it does make it more clear.

Also seems a bit odd to have a define for the SDCA side but not one for the ALSA side. Maybe add a define for the 100 as well.

Sure, will add a definition for it.

charleskeepax · 2025-07-08T13:32:38Z

sound/soc/sdca/sdca_asoc.c

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


Does this invert work here? The values are still q78 at this point.

Okay, will remove it.

charleskeepax · 2025-07-08T14:08:48Z

sound/soc/sdca/sdca_asoc.c

-	tlv[2] = (min * 100) >> 8;
-	tlv[3] = ((1 << shift) * 100) >> 8;
+	tlv[2] = min;
+	tlv[3] = step;


Just wondering as well if it might make more sense to use SNDRV_CTL_TLVT_DB_MINMAX here, I think that would let us keep a number of levels that matches the hardware and avoid a lot of the rounding problems. On the base code before your changes the switch would look like:

diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c index a9a6611ea2b5..aa38fbd33295 100644 --- a/sound/soc/sdca/sdca_asoc.c +++ b/sound/soc/sdca/sdca_asoc.c @@ -837,10 +837,10 @@ static int control_limit_kctl(struct device *dev, 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;

I tried this on my side. The volume mixer control is shown below.

numid=2,iface=MIXER,name='rt722 FU 42 Channel Volume' ; type=INTEGER,access=rw---R--,values=2,min=0,max=-1989565568,step=0 : values=0,0 | dBminmax-min=-65.25dB,max=0.00dB

The max value is weird. Do you have the same situation?

After applying the patch 8e5a534, the volume mixer control is shown.

numid=2,iface=MIXER,name='rt722 FU 42 Channel Volume' ; type=INTEGER,access=rw---R--,values=2,min=0,max=87,step=0 : values=0,0 | dBscale-min=-65.25dB,step=0.75dB,mute=0

Hmm... no I haven't seen the max end up weird in my testing. But to be clear I am applying this change before any of your changes. Care will need to be taken to ensure that mc->min and mc->max are still set appropriately.

I got the normal max value after the modification as below.
Could you create a patch to change devm_kmalloc to devm_kzalloc for some memory allocations?

@@ -915,7 +926,7 @@ static int populate_control(struct device *dev, if (!control_name) return -ENOMEM; - mc = devm_kmalloc(dev, sizeof(*mc), GFP_KERNEL); + mc = devm_kzalloc(dev, sizeof(*mc), GFP_KERNEL); if (!mc)

The steps always are 0.25dB, right?

Yeah I can look at doing a patch for that should be able to get that done tomorrow morning.

The step size is then basically up to userspace to figure out with the min/max solution, but it would match the hardware one.

Here is a patch updating the memory allocations: charleskeepax@6f007fc

Yeah should be fine to remove that check once we move to minmax I think.

Okay, I can run the test again after switching to minmax. Thanks.
Now, this PR leaves two patches to go forward.
b4b9a84
0857185
Could we upstream these two patches?

Both patches look good for upstreaming although the commit message on the "add route" one could use a little tweak as per my comment: #5469 (comment)

And we should keep working through the q78 changes, that is good work just needs a few small tweaks.

Both patches have been sent already. Waiting for review.
https://patchwork.kernel.org/project/alsa-devel/list/?submitter=179771

@charleskeepax Both patches are upstreamed. Thanks for reviewing.
If there are q78 changes, we could discuss on a new PR. This PR will close now.

shumingfan requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart, ranj063 and ujfalusi as code owners June 30, 2025 07:37

bardliao requested a review from charleskeepax June 30, 2025 07:46

shumingfan force-pushed the rfc-sdca-fix branch from b99d34d to c33fd6b Compare June 30, 2025 10:14

shumingfan added 5 commits July 2, 2025 14:49

ASoC: SDCA: correct the calculation of the maximum init table size

b4b9a84

One initial setting is 5 bytes, so num_init_writes should divide by 5. Signed-off-by: Shuming Fan <shumingf@realtek.com>

ASoC: SDCA: move the reading mipi-sdca-control-cn-list property to th…

5e24db0

…e proper position The reading mipi-sdca-control-cn-list property should be done before reading "mipi-sdca-control-dc-value" property. Signed-off-by: Shuming Fan <shumingf@realtek.com>

ASoC: SDCA: certain entities have mipi-sdca-input-pin-list property

84bf2e8

This patch checks the entity label to display the warning messages. Signed-off-by: Shuming Fan <shumingf@realtek.com>

ASoC: SDCA: add put/get callback for Q7.8 volume format

8e5a534

This patch adds the put/get callback for Q7.8 volume format. Signed-off-by: Shuming Fan <shumingf@realtek.com>

ASoC: SDCA: add route by the number of input pins in MU entity

0857185

The control number should not be equal to the number of input pins. Therefore, this patch removed the judgment. Signed-off-by: Shuming Fan <shumingf@realtek.com>

shumingfan force-pushed the rfc-sdca-fix branch from c33fd6b to 0857185 Compare July 2, 2025 06:56

charleskeepax reviewed Jul 7, 2025

View reviewed changes

charleskeepax reviewed Jul 8, 2025

View reviewed changes

shumingfan closed this Jul 24, 2025

Conversation

shumingfan commented Jun 30, 2025

Uh oh!

charleskeepax commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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!

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

shumingfan Jul 8, 2025 •

edited

Loading