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

Show file tree
Hide file tree
Changes from all 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
184 changes: 149 additions & 35 deletions sound/soc/sdca/sdca_asoc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,6 @@ static int entity_parse_mu(struct device *dev,
{
struct sdca_control *control;
struct snd_kcontrol_new *kctl;
int cn;
int i;

if (!entity->num_sources) {
Expand All @@ -650,18 +649,11 @@ static int entity_parse_mu(struct device *dev,
dev_warn(dev, "%s: unexpected access layer: %x\n",
entity->label, control->layers);

if (entity->num_sources != hweight64(control->cn_list)) {
dev_err(dev, "%s: mismatched control and sources\n", entity->label);
return -EINVAL;
}

kctl = devm_kcalloc(dev, entity->num_sources, sizeof(*kctl), GFP_KERNEL);
if (!kctl)
return -ENOMEM;

i = 0;
for_each_set_bit(cn, (unsigned long *)&control->cn_list,
BITS_PER_TYPE(control->cn_list)) {
for (i = 0; i < entity->num_sources; i++) {
const char *control_name;
struct soc_mixer_control *mc;

Expand All @@ -686,7 +678,6 @@ static int entity_parse_mu(struct device *dev,
kctl[i].info = snd_soc_info_volsw;
kctl[i].get = snd_soc_dapm_get_volsw;
kctl[i].put = snd_soc_dapm_put_volsw;
i++;
Copy link

@charleskeepax charleskeepax Jul 7, 2025

Choose a reason for hiding this comment

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

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?

}

(*widget)->id = snd_soc_dapm_mixer;
Expand Down Expand Up @@ -834,6 +825,8 @@ int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *funct
}
EXPORT_SYMBOL_NS(sdca_asoc_populate_dapm, "SND_SOC_SDCA");

#define SCALE_FACTOR BIT(8)

static int control_limit_kctl(struct device *dev,
struct sdca_entity *entity,
struct sdca_control *control,
Expand All @@ -843,7 +836,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;
Expand All @@ -863,43 +855,160 @@ static int control_limit_kctl(struct device *dev,
max = sign_extend32(max, control->nbits - 1);

/*
* FIXME: Only support power of 2 step sizes as this can be supported
* by a simple shift.
* The SDCA volumes are in steps of 1/256th of a dB, so we need to
* scale them to 1/100ths of a dB for the ALSA TLV.
* The min and max values are in Q7.8 format, so we need to scale
* them to 1/100ths of a dB.
*/
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);
min = min * 100 / SCALE_FACTOR;
max = max * 100 / SCALE_FACTOR;
step = step * 100 / SCALE_FACTOR;
Copy link

@charleskeepax charleskeepax Jul 8, 2025

Choose a reason for hiding this comment

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

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

Copy link

@charleskeepax charleskeepax Jul 8, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@shumingfan shumingfan Jul 9, 2025

Choose a reason for hiding this comment

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

Sure, will add a definition for it.


tlv = devm_kcalloc(dev, 4, sizeof(*tlv), GFP_KERNEL);
if (!tlv)
return -ENOMEM;

tlv[0] = SNDRV_CTL_TLVT_DB_SCALE;
tlv[1] = 2 * sizeof(*tlv);
tlv[2] = (min * 100) >> 8;
tlv[3] = ((1 << shift) * 100) >> 8;
tlv[2] = min;
tlv[3] = step;
Copy link

@charleskeepax charleskeepax Jul 8, 2025

Choose a reason for hiding this comment

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

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;

Copy link
Author

@shumingfan shumingfan Jul 10, 2025

Choose a reason for hiding this comment

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

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

Copy link

@charleskeepax charleskeepax Jul 10, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@shumingfan shumingfan Jul 14, 2025

Choose a reason for hiding this comment

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

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?

Copy link

@charleskeepax charleskeepax Jul 14, 2025

Choose a reason for hiding this comment

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

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.

Copy link

@charleskeepax charleskeepax Jul 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@shumingfan shumingfan Jul 16, 2025

Choose a reason for hiding this comment

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

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?

Copy link

@charleskeepax charleskeepax Jul 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@shumingfan shumingfan Jul 21, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

@shumingfan shumingfan Jul 24, 2025

Choose a reason for hiding this comment

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

@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.


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

@charleskeepax charleskeepax Jul 7, 2025

Choose a reason for hiding this comment

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

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?

Copy link

@charleskeepax charleskeepax Jul 8, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@shumingfan shumingfan Jul 9, 2025

Choose a reason for hiding this comment

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

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

Copy link

@charleskeepax charleskeepax Jul 10, 2025

Choose a reason for hiding this comment

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

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.


kctl->tlv.p = tlv;
kctl->access |= SNDRV_CTL_ELEM_ACCESS_TLV_READ;

return 0;
}

// Convert Q7.8 to tlv value
static int q78_to_tlv_val(int16_t q78)
{
return (int)((q78 * 100) / SCALE_FACTOR);
}

static short tlv_value_to_q78(int value)
{
short value16;
Copy link

@charleskeepax charleskeepax Jul 8, 2025

Choose a reason for hiding this comment

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

Be nicer to use int16_t rather than short.

Copy link
Author

@shumingfan shumingfan Jul 9, 2025

Choose a reason for hiding this comment

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

Sure

int tmp;
// Clamp value to representable range
if (value > 127996)
value = 127996;
else if (value < -128000)
value = -128000;
Copy link

@charleskeepax charleskeepax Jul 8, 2025

Choose a reason for hiding this comment

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

Use clamp_val() here.

Copy link
Author

@shumingfan shumingfan Jul 9, 2025

Choose a reason for hiding this comment

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

will fix


tmp = value * SCALE_FACTOR;
value16 = (short)(tmp / 1000);

return value16;
}

static unsigned int soc_mixer_q78_to_ctl(struct soc_mixer_control *mc,
struct snd_kcontrol *kcontrol, int val, int max)
{
int reg_val = 0;
const unsigned int *tlv;
int min, step;

if (mc->invert)
val = max - val;
Copy link

@charleskeepax charleskeepax Jul 8, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

@shumingfan shumingfan Jul 9, 2025

Choose a reason for hiding this comment

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

Okay, will remove it.


tlv = kcontrol->tlv.p;
min = tlv[2];
step = tlv[3];

reg_val = q78_to_tlv_val(val);
reg_val = reg_val - min;
reg_val = reg_val / step;

return reg_val;
}

static unsigned int soc_mixer_ctl_to_q78(struct soc_mixer_control *mc,
struct snd_kcontrol *kcontrol, int val, int max)
{
unsigned int reg_val = 0;
const unsigned int *tlv;
int min, step, dB;

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

tlv = kcontrol->tlv.p;
min = tlv[2];
step = tlv[3];

dB = min + (step * val);
reg_val = tlv_value_to_q78(dB * 10);

return reg_val;
}

static int sdca_get_volsw(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct soc_mixer_control *mc =
(struct soc_mixer_control *)kcontrol->private_value;
int max = mc->max - mc->min;
struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
unsigned int reg_val;
int val;

reg_val = snd_soc_component_read(component, mc->reg);
val = soc_mixer_q78_to_ctl(mc, kcontrol, reg_val, max);

ucontrol->value.integer.value[0] = val;

if (mc->reg == mc->rreg) {
val = soc_mixer_q78_to_ctl(mc, kcontrol, reg_val, max);
} else {
reg_val = snd_soc_component_read(component, mc->rreg);
val = soc_mixer_q78_to_ctl(mc, kcontrol, reg_val, max);
}

ucontrol->value.integer.value[1] = val;
return 0;
}

static int sdca_put_volsw(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct soc_mixer_control *mc =
(struct soc_mixer_control *)kcontrol->private_value;
int max = mc->max - mc->min;
struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
unsigned int val1, val_mask = 0xffff;
unsigned int val2 = 0;
bool double_r = false;
int ret;

val1 = soc_mixer_ctl_to_q78(mc, kcontrol, ucontrol->value.integer.value[0], max);

if (mc->reg == mc->rreg) {
val1 |= soc_mixer_ctl_to_q78(mc, kcontrol,
ucontrol->value.integer.value[1], max);
} else {
val2 = soc_mixer_ctl_to_q78(mc, kcontrol,
ucontrol->value.integer.value[1], max);
double_r = true;
}

ret = snd_soc_component_update_bits(component, mc->reg, val_mask, val1);
if (ret < 0)
return ret;

if (double_r) {
int err = snd_soc_component_update_bits(component, mc->rreg,
val_mask, val2);
/* Don't drop change flag */
if (err)
return err;
}

return ret;
}

static int populate_control(struct device *dev,
struct sdca_function_data *function,
struct sdca_entity *entity,
Expand All @@ -924,7 +1033,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);
Copy link

@charleskeepax charleskeepax Jul 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@shumingfan shumingfan Jul 8, 2025

Choose a reason for hiding this comment

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

agreed.

if (!mc)
return -ENOMEM;

Expand Down Expand Up @@ -954,8 +1063,13 @@ static int populate_control(struct device *dev,
(*kctl)->private_value = (unsigned long)mc;
(*kctl)->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
(*kctl)->info = snd_soc_info_volsw;
(*kctl)->get = snd_soc_get_volsw;
(*kctl)->put = snd_soc_put_volsw;
if (strstr(control_name, "Volume")) {
Copy link

@charleskeepax charleskeepax Jul 7, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

@shumingfan shumingfan Jul 8, 2025

Choose a reason for hiding this comment

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

Thanks. Will fix.

(*kctl)->get = sdca_get_volsw;
(*kctl)->put = sdca_put_volsw;
Copy link

@charleskeepax charleskeepax Jul 7, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

@shumingfan shumingfan Jul 9, 2025

Choose a reason for hiding this comment

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

will fix

} else {
(*kctl)->get = snd_soc_get_volsw;
(*kctl)->put = snd_soc_put_volsw;
}

if (readonly_control(control))
(*kctl)->access = SNDRV_CTL_ELEM_ACCESS_READ;
Expand Down
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;
}
Copy link

@charleskeepax charleskeepax Jul 7, 2025

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?

Copy link
Author

@shumingfan shumingfan Jul 8, 2025

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.

Copy link
Author

@shumingfan shumingfan Jul 8, 2025 •
edited
Loading

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?

Copy link

@charleskeepax charleskeepax Jul 8, 2025

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.

Copy link
Author

@shumingfan shumingfan Jul 9, 2025

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.

Copy link

@charleskeepax charleskeepax Jul 10, 2025

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.

Copy link
Author

@shumingfan shumingfan Jul 10, 2025

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;

Copy link

@charleskeepax charleskeepax Jul 10, 2025

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.

Copy link
Author

@shumingfan shumingfan Jul 14, 2025

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.

Copy link

@charleskeepax charleskeepax Jul 14, 2025

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);
Copy link

@charleskeepax charleskeepax Jul 7, 2025

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.

Copy link

@charleskeepax charleskeepax Jul 7, 2025

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

Copy link
Author

@shumingfan shumingfan Jul 8, 2025

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
Loading