Re: [PATCH v4 06/22] ASoC: ops: Introduce 'soc_set_enum_kctl'

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ASoC: ops: return value mismatch with kernel-doc in snd_soc_set_enum_kctl()
- [Low] ASoC: ops: commit subject names static helper instead of snd_soc_set_enum_kctl()
--

commit 2c9f602d361cf2e55c7e8d05183ffda527ff209d
Author: Martin Povišer <povik+lin@cutebit.org>

ASoC: ops: Introduce 'soc_set_enum_kctl'

This commit introduces snd_soc_set_enum_kctl() and its internal helper
soc_set_enum_kctl() so machine drivers can set enumerated ALSA controls to a
desired string value, such as during fixup_controls.

>     ASoC: ops: Introduce 'soc_set_enum_kctl'
>     
>     The new function is to be used to set enumerated controls to desired
>     values. It is something a machine driver may call in fixup_controls.
>     
>     Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
>     Signed-off-by: James Calligeros <jcalligeros99@gmail.com>

[Severity: Low]
Should the commit subject refer to snd_soc_set_enum_kctl instead of
soc_set_enum_kctl?

In sound/soc/soc-ops.c, soc_set_enum_kctl() is a file-local static helper,
whereas snd_soc_set_enum_kctl() is the exported function declared in
include/sound/soc.h for machine drivers to call in fixup_controls.

> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
> index 1d60e290e40e4..1cd7f1caefe4d 100644
> --- a/sound/soc/soc-ops.c
> +++ b/sound/soc/soc-ops.c
> @@ -505,6 +505,73 @@ int snd_soc_ctl_activate(struct snd_soc_card *card,
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_ctl_activate);
>  
> +static int soc_set_enum_kctl(struct snd_kcontrol *kctl, const char *value)
> +{

[ ... ]

> +	for (i = 0; i < info->count; i++)
> +		evalue->value.enumerated.item[i] = sel;
> +
> +	return kctl->put(kctl, evalue);
> +}
> +
> +/**
> + * snd_soc_set_enum_kctl - Set enumerated control matching a name
> + *
> + * @card: where to look for the controls
> + * @name: name
> + * @value: string value to set the controls to
> + *
> + * Return 0 on success, else error.
> + */
> +int snd_soc_set_enum_kctl(struct snd_soc_card *card,
> +			  const char *name, const char *value)
> +{
> +	struct snd_kcontrol *kctl;
> +
> +	kctl = snd_soc_card_get_kcontrol(card, name);
> +	if (!kctl) {
> +		dev_dbg(card->dev, "Tried to set unknown control '%s' to '%s'\n", name, value);
> +		return -EINVAL;
> +	}
> +
> +	return soc_set_enum_kctl(kctl, value);
> +}

[Severity: Medium]
Does snd_soc_set_enum_kctl() return 1 instead of 0 when the control value is
updated, contradicting its kernel-doc comment?

When snd_soc_set_enum_kctl() calls soc_set_enum_kctl() to change an
enumerated control to a value different from its current state,
soc_set_enum_kctl() directly returns the result of kctl->put(kctl, evalue).
In ALSA and ASoC, snd_kcontrol_put_t callbacks such as
snd_soc_put_enum_double() and snd_soc_dapm_put_enum_double() return 1 when
the control value changes, 0 when it is unchanged, and a negative error code
on failure. Any caller checking for a non-zero return value based on the
documented contract would treat a changed control value as an error.

In the same file, snd_soc_clip_to_platform_max() normalizes the kctl->put()
return value to 0:

sound/soc/soc-ops.c:snd_soc_clip_to_platform_max() {
    ...
	ret = kctl->put(kctl, uctl);
	if (ret < 0)
		return ret;

	return 0;
}

whereas snd_soc_ctl_activate() documents the tri-state return value with a
colon after Return as expected by kernel-doc:

 * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.

Should soc_set_enum_kctl() clamp non-negative returns from kctl->put() to 0,
or should the kernel-doc Return: section for snd_soc_set_enum_kctl() document
that it returns 0 if unchanged, 1 if changed, or a negative error code on
failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-macaudio-v4-0-637484efe46d@gmail.com?part=6