| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4654f21b-bf61-4b41-b073-407fab4bff6a@linaro.org>
Date: Tue, 25 Mar 2025 18:04:24 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: peda@...ntia.se, broonie@...nel.org, andersson@...nel.org,
krzk+dt@...nel.org, ivprusov@...utedevices.com, luca.ceresoli@...tlin.com,
zhoubinbin@...ngson.cn, paulha@...nsource.cirrus.com, lgirdwood@...il.com,
robh@...nel.org, conor+dt@...nel.org, konradybcio@...nel.org,
perex@...ex.cz, tiwai@...e.com, linux-sound@...r.kernel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, johan+linaro@...nel.org,
Christopher Obbard <christopher.obbard@...aro.org>
Subject: Re: [PATCH v5 5/6] ASoC: codecs: wcd938x: add mux control support for
hp audio mux
On 25/03/2025 13:36, Dmitry Baryshkov wrote:
> On Tue, Mar 25, 2025 at 11:40:57AM +0000, srinivas.kandagatla@...aro.org wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>>
>> On some platforms to minimise pop and click during switching between
>> CTIA and OMTP headset an additional HiFi mux is used. Most common
>> case is that this switch is switched on by default, but on some
>> platforms this needs a regulator enable.
>>
>> move to using mux control to enable both regulator and handle gpios,
>> deprecate the usage of gpio.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>> Tested-by: Christopher Obbard <christopher.obbard@...aro.org>
>> ---
>> sound/soc/codecs/Kconfig | 1 +
>> sound/soc/codecs/wcd938x.c | 50 +++++++++++++++++++++++++++++---------
>> 2 files changed, 39 insertions(+), 12 deletions(-)
>>
>> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
>> index ee35f3aa5521..a2829d76e108 100644
>> --- a/sound/soc/codecs/Kconfig
>> +++ b/sound/soc/codecs/Kconfig
>> @@ -2226,6 +2226,7 @@ config SND_SOC_WCD938X
>> tristate
>> depends on SOUNDWIRE || !SOUNDWIRE
>> select SND_SOC_WCD_CLASSH
>> + select MULTIPLEXER
>>
>> config SND_SOC_WCD938X_SDW
>> tristate "WCD9380/WCD9385 Codec - SDW"
>> diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
>> index dfaa3de31164..209d0b64c8be 100644
>> --- a/sound/soc/codecs/wcd938x.c
>> +++ b/sound/soc/codecs/wcd938x.c
>> @@ -19,6 +19,7 @@
>> #include <linux/regmap.h>
>> #include <sound/soc.h>
>> #include <sound/soc-dapm.h>
>> +#include <linux/mux/consumer.h>
>> #include <linux/regulator/consumer.h>
>>
>> #include "wcd-clsh-v2.h"
>> @@ -178,6 +179,8 @@ struct wcd938x_priv {
>> int variant;
>> int reset_gpio;
>> struct gpio_desc *us_euro_gpio;
>> + struct mux_control *us_euro_mux;
>> + unsigned int mux_state;
>> u32 micb1_mv;
>> u32 micb2_mv;
>> u32 micb3_mv;
>> @@ -3237,15 +3240,22 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri
>>
>> static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component)
>> {
>> - int value;
>> -
>> - struct wcd938x_priv *wcd938x;
>> -
>> - wcd938x = snd_soc_component_get_drvdata(component);
>> + struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component);
>> + struct device *dev = component->dev;
>> + int ret;
>>
>> - value = gpiod_get_value(wcd938x->us_euro_gpio);
>> + if (wcd938x->us_euro_mux) {
>> + mux_control_deselect(wcd938x->us_euro_mux);
>> + ret = mux_control_try_select(wcd938x->us_euro_mux, !wcd938x->mux_state);
>> + if (ret) {
>> + dev_err(dev, "Error (%d) Unable to select us/euro mux state\n", ret);
>> + return false;
>
>
> I really don't see any improvement here. If mux_control_try_select()
> fails, then on the next toggle mux_control_deselect() would still try to
> deselect the mux, although the driver no longer owns it.
TBH, if the first select fails, there is no guarantee that next select
will pass,
Something like this to conditionally check before deselecting should
address some of your concerns:
---------------------------->cut<-----------------------------------
if (wcd938x->us_euro_mux) {
if (wcd938x->mux_setup_done)
mux_control_deselect(wcd938x->us_euro_mux);
ret = mux_control_try_select(wcd938x->us_euro_mux, !wcd938x->mux_state);
if (ret) {
dev_err(dev, "Error (%d) Unable to select us/euro mux state\n", ret);
wcd938x->mux_setup_done = false;
return false;
}
} else {
gpiod_set_value(wcd938x->us_euro_gpio, !wcd938x->mux_state);
}
wcd938x->mux_state = !wcd938x->mux_state;
wcd938x->mux_setup_done = true;
---------------------------->cut<-----------------------------------
I wish we could be taken care in mux-core or even in the deselect api
--srini
Likewise in the
> remove path the mux_control_deselect() is called unconditionally. I
> understand that this driver is the only user of the MUX, so currently
> there seems to be no need for any special handling. However if the
> hardware design gets more complicated, we can easily face the situation
> when selecting the MUX state errors out.
>
>> + }
>> + } else {
>> + gpiod_set_value(wcd938x->us_euro_gpio, !wcd938x->mux_state);
>> + }
>>
>> - gpiod_set_value(wcd938x->us_euro_gpio, !value);
>> + wcd938x->mux_state = !wcd938x->mux_state;
>>
>> return true;
>> }
>
> [...]
>
>> @@ -3581,6 +3604,9 @@ static void wcd938x_remove(struct platform_device *pdev)
>> pm_runtime_set_suspended(dev);
>> pm_runtime_dont_use_autosuspend(dev);
>>
>> + if (wcd938x->us_euro_mux)
>> + mux_control_deselect(wcd938x->us_euro_mux);
>> +
>> regulator_bulk_disable(WCD938X_MAX_SUPPLY, wcd938x->supplies);
>> regulator_bulk_free(WCD938X_MAX_SUPPLY, wcd938x->supplies);
>> }
>> --
>> 2.39.5
>>
>
Powered by blists - more mailing lists