Skip to content

[RFC] some patches updated during SDCA library testing #5469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shumingfan wants to merge 5 commits into from
Closed

[RFC] some patches updated during SDCA library testing #5469

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4b9a84
ASoC: SDCA: correct the calculation of the maximum init table size
shumingfan Jun 27, 2025
5e24db0
ASoC: SDCA: move the reading mipi-sdca-control-cn-list property to th…
shumingfan Jun 27, 2025
84bf2e8
ASoC: SDCA: certain entities have mipi-sdca-input-pin-list property
shumingfan Jun 27, 2025
8e5a534
ASoC: SDCA: add put/get callback for Q7.8 volume format
shumingfan Jun 30, 2025
0857185
ASoC: SDCA: add route by the number of input pins in MU entity
shumingfan Jul 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 32 additions & 23 deletions sound/soc/sdca/sdca_functions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ static int find_sdca_init_table(struct device *dev,
} else if (num_init_writes % sizeof(*raw) != 0) {
dev_err(dev, "%pfwP: init table size invalid\n", function_node);
return -EINVAL;
} else if (num_init_writes > SDCA_MAX_INIT_COUNT) {
} else if ((num_init_writes / sizeof(*raw)) > SDCA_MAX_INIT_COUNT) {
dev_err(dev, "%pfwP: maximum init table size exceeded\n", function_node);
return -EINVAL;
}
Expand Down Expand Up @@ -843,19 +843,32 @@ static int find_sdca_entity_control(struct device *dev, struct sdca_entity *enti

control->layers = tmp;

ret = fwnode_property_read_u64(control_node, "mipi-sdca-control-cn-list",
&control->cn_list);
if (ret == -EINVAL) {
/* Spec allows not specifying cn-list if only the first number is used */
control->cn_list = 0x1;
} else if (ret || !control->cn_list) {
dev_err(dev, "%s: control %#x: cn list missing: %d\n",
entity->label, control->sel, ret);
return ret;
}

switch (control->mode) {
case SDCA_ACCESS_MODE_DC:
ret = fwnode_property_read_u32(control_node,
"mipi-sdca-control-dc-value",
&tmp);
if (ret) {
dev_err(dev, "%s: control %#x: dc value missing: %d\n",
entity->label, control->sel, ret);
return ret;
}
/* only single control number case */
if (control->cn_list == 0x1) {
ret = fwnode_property_read_u32(control_node,
"mipi-sdca-control-dc-value", &tmp);
if (ret) {
dev_err(dev, "%s: control %#x: dc value missing: %d\n",
entity->label, control->sel, ret);
return ret;
}

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

@charleskeepax charleskeepax Jul 7, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@shumingfan shumingfan Jul 8, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shumingfan shumingfan Jul 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@charleskeepax charleskeepax Jul 8, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shumingfan shumingfan Jul 9, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@charleskeepax charleskeepax Jul 10, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shumingfan shumingfan Jul 10, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

@charleskeepax charleskeepax Jul 10, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shumingfan shumingfan Jul 14, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@charleskeepax charleskeepax Jul 14, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super I will add it to our upstreaming backlog.

break;
case SDCA_ACCESS_MODE_RW:
case SDCA_ACCESS_MODE_DUAL:
Expand Down Expand Up @@ -896,17 +909,6 @@ static int find_sdca_entity_control(struct device *dev, struct sdca_entity *enti
return ret;
}

ret = fwnode_property_read_u64(control_node, "mipi-sdca-control-cn-list",
&control->cn_list);
if (ret == -EINVAL) {
/* Spec allows not specifying cn-list if only the first number is used */
control->cn_list = 0x1;
} else if (ret || !control->cn_list) {
dev_err(dev, "%s: control %#x: cn list missing: %d\n",
entity->label, control->sel, ret);
return ret;
}

ret = fwnode_property_read_u32(control_node,
"mipi-sdca-control-interrupt-position",
&tmp);
Expand Down Expand Up @@ -1632,7 +1634,14 @@ static int find_sdca_entity_connection(struct device *dev,
ret = fwnode_property_read_u64(entity_node, "mipi-sdca-input-pin-list", &pin_list);
if (ret == -EINVAL) {
/* Allow missing pin lists, assume no pins. */
dev_warn(dev, "%s: missing pin list\n", entity->label);
if (strstr(entity->label, "OT") || strstr(entity->label, "MU") ||
strstr(entity->label, "SU") || strstr(entity->label, "FU") ||
strstr(entity->label, "XU") || strstr(entity->label, "CX") ||
strstr(entity->label, "CRU") || strstr(entity->label, "UDMPU") ||
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);

@charleskeepax charleskeepax Jul 7, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@charleskeepax charleskeepax Jul 7, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shumingfan shumingfan Jul 8, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for the fix.

return 0;
} else if (ret) {
dev_err(dev, "%s: failed to read pin list: %d\n", entity->label, ret);
Expand Down