[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4dae5296-9984-4c3b-803a-f6024edd0dd9@sirena.org.uk>
Date: Wed, 13 Dec 2023 19:20:09 +0000
From: Mark Brown <broonie@...nel.org>
To: Neil Armstrong <neil.armstrong@...aro.org>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Banajit Goswami <bgoswami@...cinc.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Liam Girdwood <lgirdwood@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>, linux-arm-msm@...r.kernel.org,
alsa-devel@...a-project.org, linux-sound@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/5] ASoC: codecs: Add WCD939x Codec driver
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.
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists