Messages in this thread | | | Date | Fri, 15 Dec 2023 14:11:58 +0100 | From | Neil Armstrong <> | Subject | Re: [PATCH v3 5/5] ASoC: codecs: Add WCD939x Codec driver |
| |
On 13/12/2023 20:20, Mark Brown wrote: > 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.
Ack, I'll fix this, I wasn't happy anyway with the design
Neil
| |