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