On Thu, Dec 07, 2023 at 11:28:08AM +0100, Neil Armstrong wrote:
> +static int wcd939x_rx_hph_mode_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> + struct wcd939x_priv *wcd939x = snd_soc_component_get_drvdata(component);
> + u32 mode_val;
> +
> + mode_val = ucontrol->value.enumerated.item[0];
> +
> + if (wcd939x->variant == WCD9390) {
> + if (mode_val == CLS_H_HIFI || mode_val == CLS_AB_HIFI) {
> + dev_dbg(component->dev, "%s: Invalid HPH Mode\n", __func__);
> + return -EINVAL;
> + }
> + }
> + if (mode_val == CLS_H_NORMAL) {
> + dev_dbg(component->dev, "%s: Unsupported HPH Mode\n", __func__);
> + return -EINVAL;
> + }
> +
> + wcd939x->hph_mode = mode_val;
This seems strange - the code will accept any value other than a small
number of specifically enumerated ones? I would have expected us to
check a defined list of modes and reject anything that isn't in that
list. This also means that the get() function can return out of bounds
values which is buggy. Please use the mixer-test selftest on a card
with this driver running, it should identify at least that issue.
> +
> + return 1;
> +}
This will also unconditionally report that the value of the mux changed,
the function should return 0 if the value written is the control value
hasn't changed.
Attachment:
signature.asc
Description: PGP signature
