| Message ID | 20250325114058.12083-6-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: wcd938x: enable t14s audio headset | expand |
On Tue, Mar 25, 2025 at 11:40:57AM +0000, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.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@linaro.org> > Tested-by: Christopher Obbard <christopher.obbard@linaro.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. 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 >
On 25/03/2025 13:36, Dmitry Baryshkov wrote: > On Tue, Mar 25, 2025 at 11:40:57AM +0000, srinivas.kandagatla@linaro.org wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.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@linaro.org> >> Tested-by: Christopher Obbard <christopher.obbard@linaro.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 >> >
Hi!
2025-03-25 at 19:04, Srinivas Kandagatla wrote:
> I wish we could be taken care in mux-core or even in the deselect api
It is not easily done. A mux is a shared resource. How can the mux core
know if it is consumer A or consumer B that deselects the mux if both
ignore failures when calling select? Mux select is backed by a semaphore
and there is no guarantee that a consumer selects/deselects from the
same thread or anything like that. The onus is on the consumer to get
this right and only deselect when select is successful.
I believe the documentation is clear on this topic: "do not call
mux_control_deselect() if mux_control_select() fails".
One thing can be done from the mux core, and that is to provide a new
API where consumers get a mux that is exclusive so that the consumer
can call select/deselect without involving a lock in the core. There
need not even be a requirement to call deselect between selects in that
case. Such an API is what many consumers want, I think, but it is of
course not really compatible with the existing API, which is totally
focused on the need to share a mux among multiple consumers.
And, of course, someone has to do it.
Cheers,
Peter
Thanks Peter, On 25/03/2025 20:13, Peter Rosin wrote: > Hi! > > 2025-03-25 at 19:04, Srinivas Kandagatla wrote: >> I wish we could be taken care in mux-core or even in the deselect api > > It is not easily done. A mux is a shared resource. How can the mux core > know if it is consumer A or consumer B that deselects the mux if both > ignore failures when calling select? Mux select is backed by a semaphore > and there is no guarantee that a consumer selects/deselects from the > same thread or anything like that. The onus is on the consumer to get > this right and only deselect when select is successful. Should deselect fail if there was no previous mux selected? > > I believe the documentation is clear on this topic: "do not call > mux_control_deselect() if mux_control_select() fails". True, the documentation is pretty clear about this behavior. > > One thing can be done from the mux core, and that is to provide a new > API where consumers get a mux that is exclusive so that the consumer > can call select/deselect without involving a lock in the core. There > need not even be a requirement to call deselect between selects in that > case. Such an API is what many consumers want, I think, but it is of > course not really compatible with the existing API, which is totally > focused on the need to share a mux among multiple consumers. > exclusive apis would simplify the consumer side of code for sure. > And, of course, someone has to do it. Yes, I can give it a go and see how it will turn out. --srini > > Cheers, > Peter
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; + } + } 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; } @@ -3261,11 +3271,24 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device return dev_err_probe(dev, wcd938x->reset_gpio, "Failed to get reset gpio\n"); - wcd938x->us_euro_gpio = devm_gpiod_get_optional(dev, "us-euro", - GPIOD_OUT_LOW); - if (IS_ERR(wcd938x->us_euro_gpio)) - return dev_err_probe(dev, PTR_ERR(wcd938x->us_euro_gpio), - "us-euro swap Control GPIO not found\n"); + wcd938x->us_euro_mux = devm_mux_control_get(dev, NULL); + if (IS_ERR(wcd938x->us_euro_mux)) { + if (PTR_ERR(wcd938x->us_euro_mux) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* mux is optional and now fallback to using gpio */ + wcd938x->us_euro_mux = NULL; + wcd938x->us_euro_gpio = devm_gpiod_get_optional(dev, "us-euro", GPIOD_OUT_LOW); + if (IS_ERR(wcd938x->us_euro_gpio)) + return dev_err_probe(dev, PTR_ERR(wcd938x->us_euro_gpio), + "us-euro swap Control GPIO not found\n"); + } else { + 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 ret; + } + } cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; @@ -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); }