[PATCH 0/4] ASoC: codecs: Qualcomm codecs fix kcontrol put return values
Hi Mark, Some recent testing found few issues with wcd934x and wsa881x codec drivers that are not handling kcontrol put correctly. This patchset fixes those instances. Along with this there is also a bug fix for the way channel list was updated for wcd934x dais.
--srini
Srinivas Kandagatla (4): ASoC: codecs: wcd934x: handle channel mappping list correctly ASoC: codecs: wcd934x: remove redundant ret variable ASoC: codecs: wcd934x: return correct value from mixer put ASoC: codecs: wsa881x: fix return values from kcontrol put
sound/soc/codecs/wcd934x.c | 132 ++++++++++++++++++++++++++----------- sound/soc/codecs/wsa881x.c | 16 +++-- 2 files changed, 107 insertions(+), 41 deletions(-)
Currently each channel is added as list to dai channel list, however there is danger of adding same channel to multiple dai channel list which endups corrupting the other list where its already added.
This patch ensures that the channel is actually free before adding to the dai channel list and also ensures that the channel is on the list before deleting it.
This check was missing previously, and we did not hit this issue as we were testing very simple usecases with sequence of amixer commands.
Fixes: a70d9245759a ("ASoC: wcd934x: add capture dapm widgets") Fixes: dd9eb19b5673 ("ASoC: wcd934x: add playback dapm widgets") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/codecs/wcd934x.c | 119 +++++++++++++++++++++++++++---------- 1 file changed, 88 insertions(+), 31 deletions(-)
diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index 4f568abd59e2..eb4e2f2a24ae 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -3326,6 +3326,31 @@ static int slim_rx_mux_get(struct snd_kcontrol *kc, return 0; }
+static int slim_rx_mux_to_dai_id(int mux) +{ + int aif_id; + + switch (mux) { + case 1: + aif_id = AIF1_PB; + break; + case 2: + aif_id = AIF2_PB; + break; + case 3: + aif_id = AIF3_PB; + break; + case 4: + aif_id = AIF4_PB; + break; + default: + aif_id = -1; + break; + } + + return aif_id; +} + static int slim_rx_mux_put(struct snd_kcontrol *kc, struct snd_ctl_elem_value *ucontrol) { @@ -3333,43 +3358,59 @@ static int slim_rx_mux_put(struct snd_kcontrol *kc, struct wcd934x_codec *wcd = dev_get_drvdata(w->dapm->dev); struct soc_enum *e = (struct soc_enum *)kc->private_value; struct snd_soc_dapm_update *update = NULL; + struct wcd934x_slim_ch *ch, *c; u32 port_id = w->shift; + bool found = false; + int mux_idx; + int prev_mux_idx = wcd->rx_port_value[port_id]; + int aif_id;
- if (wcd->rx_port_value[port_id] == ucontrol->value.enumerated.item[0]) - return 0; + mux_idx = ucontrol->value.enumerated.item[0];
- wcd->rx_port_value[port_id] = ucontrol->value.enumerated.item[0]; + if (mux_idx == prev_mux_idx) + return 0;
- switch (wcd->rx_port_value[port_id]) { + switch(mux_idx) { case 0: - list_del_init(&wcd->rx_chs[port_id].list); - break; - case 1: - list_add_tail(&wcd->rx_chs[port_id].list, - &wcd->dai[AIF1_PB].slim_ch_list); - break; - case 2: - list_add_tail(&wcd->rx_chs[port_id].list, - &wcd->dai[AIF2_PB].slim_ch_list); - break; - case 3: - list_add_tail(&wcd->rx_chs[port_id].list, - &wcd->dai[AIF3_PB].slim_ch_list); + aif_id = slim_rx_mux_to_dai_id(prev_mux_idx); + if (aif_id < 0) + return 0; + + list_for_each_entry_safe(ch, c, &wcd->dai[aif_id].slim_ch_list, list) { + if (ch->port == port_id + WCD934X_RX_START) { + found = true; + list_del_init(&ch->list); + break; + } + } + if (!found) + return 0; + break; - case 4: - list_add_tail(&wcd->rx_chs[port_id].list, - &wcd->dai[AIF4_PB].slim_ch_list); + case 1 ... 4: + aif_id = slim_rx_mux_to_dai_id(mux_idx); + if (aif_id < 0) + return 0; + + if (list_empty(&wcd->rx_chs[port_id].list)) { + list_add_tail(&wcd->rx_chs[port_id].list, + &wcd->dai[aif_id].slim_ch_list); + } else { + dev_err(wcd->dev ,"SLIM_RX%d PORT is busy\n", port_id); + return 0; + } break; + default: - dev_err(wcd->dev, "Unknown AIF %d\n", - wcd->rx_port_value[port_id]); + dev_err(wcd->dev, "Unknown AIF %d\n", mux_idx); goto err; }
+ wcd->rx_port_value[port_id] = mux_idx; snd_soc_dapm_mux_update_power(w->dapm, kc, wcd->rx_port_value[port_id], e, update);
- return 0; + return 1; err: return -EINVAL; } @@ -3815,6 +3856,7 @@ static int slim_tx_mixer_put(struct snd_kcontrol *kc, struct soc_mixer_control *mixer = (struct soc_mixer_control *)kc->private_value; int enable = ucontrol->value.integer.value[0]; + struct wcd934x_slim_ch *ch, *c; int dai_id = widget->shift; int port_id = mixer->shift;
@@ -3822,17 +3864,32 @@ static int slim_tx_mixer_put(struct snd_kcontrol *kc, if (enable == wcd->tx_port_value[port_id]) return 0;
- wcd->tx_port_value[port_id] = enable; - - if (enable) - list_add_tail(&wcd->tx_chs[port_id].list, - &wcd->dai[dai_id].slim_ch_list); - else - list_del_init(&wcd->tx_chs[port_id].list); + if (enable) { + if (list_empty(&wcd->tx_chs[port_id].list)) { + list_add_tail(&wcd->tx_chs[port_id].list, + &wcd->dai[dai_id].slim_ch_list); + } else { + dev_err(wcd->dev ,"SLIM_TX%d PORT is busy\n", port_id); + return 0; + } + } else { + bool found = false; + + list_for_each_entry_safe(ch, c, &wcd->dai[dai_id].slim_ch_list, list) { + if (ch->port == port_id) { + found = true; + list_del_init(&wcd->tx_chs[port_id].list); + break; + } + } + if (!found) + return 0; + }
+ wcd->tx_port_value[port_id] = enable; snd_soc_dapm_mixer_update_power(widget->dapm, kc, enable, update);
- return 0; + return 1; }
static const struct snd_kcontrol_new aif1_slim_cap_mixer[] = {
return value form snd_soc_dapm_put_enum_double() directly instead of taking this in another redundant variable.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/codecs/wcd934x.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index eb4e2f2a24ae..3294c685d4d8 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -3420,7 +3420,7 @@ static int wcd934x_int_dem_inp_mux_put(struct snd_kcontrol *kc, { struct soc_enum *e = (struct soc_enum *)kc->private_value; struct snd_soc_component *component; - int reg, val, ret; + int reg, val;
component = snd_soc_dapm_kcontrol_component(kc); val = ucontrol->value.enumerated.item[0]; @@ -3443,9 +3443,7 @@ static int wcd934x_int_dem_inp_mux_put(struct snd_kcontrol *kc, WCD934X_RX_DLY_ZN_EN_MASK, WCD934X_RX_DLY_ZN_DISABLE);
- ret = snd_soc_dapm_put_enum_double(kc, ucontrol); - - return ret; + return snd_soc_dapm_put_enum_double(kc, ucontrol); }
static int wcd934x_dec_enum_put(struct snd_kcontrol *kcontrol,
On Tue, Nov 30, 2021 at 04:05:05PM +0000, Srinivas Kandagatla wrote:
return value form snd_soc_dapm_put_enum_double() directly instead of taking this in another redundant variable.
Cleanups like this should come after any fixes in a series.
On 30/11/2021 16:31, Mark Brown wrote:
On Tue, Nov 30, 2021 at 04:05:05PM +0000, Srinivas Kandagatla wrote:
return value form snd_soc_dapm_put_enum_double() directly instead of taking this in another redundant variable.
Cleanups like this should come after any fixes in a series.
Noted, do you want me to resend a v2 fixing the order?
--srini
On Tue, Nov 30, 2021 at 04:33:38PM +0000, Srinivas Kandagatla wrote:
On 30/11/2021 16:31, Mark Brown wrote:
On Tue, Nov 30, 2021 at 04:05:05PM +0000, Srinivas Kandagatla wrote:
return value form snd_soc_dapm_put_enum_double() directly instead of taking this in another redundant variable.
Cleanups like this should come after any fixes in a series.
Noted, do you want me to resend a v2 fixing the order?
It should be fine, I'll let you know if it's needed.
wcd934x_compander_set() currently returns zero eventhough it changes the value. Fix this, so that change notifications are sent correctly.
Fixes: 1cde8b822332 ("ASoC: wcd934x: add basic controls") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/codecs/wcd934x.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index 3294c685d4d8..6c468527fec6 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -3256,6 +3256,9 @@ static int wcd934x_compander_set(struct snd_kcontrol *kc, int value = ucontrol->value.integer.value[0]; int sel;
+ if (wcd->comp_enabled[comp] == value) + return 0; + wcd->comp_enabled[comp] = value; sel = value ? WCD934X_HPH_GAIN_SRC_SEL_COMPANDER : WCD934X_HPH_GAIN_SRC_SEL_REGISTER; @@ -3279,10 +3282,10 @@ static int wcd934x_compander_set(struct snd_kcontrol *kc, case COMPANDER_8: break; default: - break; + return 0; }
- return 0; + return 1; }
static int wcd934x_rx_hph_mode_get(struct snd_kcontrol *kc,
wsa881x_set_port() and wsa881x_put_pa_gain() currently returns zero eventhough it changes the value. Fix this, so that change notifications are sent correctly.
Fixes: a0aab9e1404a ("ASoC: codecs: add wsa881x amplifier support") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/codecs/wsa881x.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/wsa881x.c b/sound/soc/codecs/wsa881x.c index 2da4a5fa7a18..564b78f3cdd0 100644 --- a/sound/soc/codecs/wsa881x.c +++ b/sound/soc/codecs/wsa881x.c @@ -772,7 +772,8 @@ static int wsa881x_put_pa_gain(struct snd_kcontrol *kc,
usleep_range(1000, 1010); } - return 0; + + return 1; }
static int wsa881x_get_port(struct snd_kcontrol *kcontrol, @@ -816,15 +817,22 @@ static int wsa881x_set_port(struct snd_kcontrol *kcontrol, (struct soc_mixer_control *)kcontrol->private_value; int portidx = mixer->reg;
- if (ucontrol->value.integer.value[0]) + if (ucontrol->value.integer.value[0]) { + if (data->port_enable[portidx]) + return 0; + data->port_enable[portidx] = true; - else + } else { + if (!data->port_enable[portidx]) + return 0; + data->port_enable[portidx] = false; + }
if (portidx == WSA881X_PORT_BOOST) /* Boost Switch */ wsa881x_boost_ctrl(comp, data->port_enable[portidx]);
- return 0; + return 1; }
static const char * const smart_boost_lvl_text[] = {
On Tue, 30 Nov 2021 16:05:03 +0000, Srinivas Kandagatla wrote:
Some recent testing found few issues with wcd934x and wsa881x codec drivers that are not handling kcontrol put correctly. This patchset fixes those instances. Along with this there is also a bug fix for the way channel list was updated for wcd934x dais.
--srini
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-linus
Thanks!
[1/4] ASoC: codecs: wcd934x: handle channel mappping list correctly commit: 23ba28616d3063bd4c4953598ed5e439ca891101 [2/4] ASoC: codecs: wcd934x: remove redundant ret variable (no commit info) [3/4] ASoC: codecs: wcd934x: return correct value from mixer put commit: d9be0ff4796d1b6f5ee391c1b7e3653a43cedfab [4/4] ASoC: codecs: wsa881x: fix return values from kcontrol put commit: 3fc27e9a1f619b50700f020e6cd270c1b74755f0
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Mark Brown -
Srinivas Kandagatla