| Message ID | 20250324130057.4855-6-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: wcd938x: enable t14s audio headset | expand |
On Mon, 24 Mar 2025 at 15:01, <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, 40 insertions(+), 11 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..88c758efe40d 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; > + u32 mux_state; > u32 micb1_mv; > u32 micb2_mv; > u32 micb3_mv; > @@ -3235,17 +3238,31 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri > dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); > } > > -static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) > +static int wcd938x_select_mux_state(struct device *dev, struct wcd938x_priv *wcd938x, int state) > { > - int value; > + int ret = mux_control_try_select(wcd938x->us_euro_mux, state); Hmm. Does this really work? You have selected the mux in probe function, now you are trying to select it again. If I'm reading the code correctly, you will get -EBUSY here. > > - struct wcd938x_priv *wcd938x; > + if (ret) { > + dev_err(dev, "Error (%d) Unable to select us/euro mux state\n", ret); > + return ret; > + } > > - wcd938x = snd_soc_component_get_drvdata(component); > + wcd938x->mux_state = state; > + mux_control_deselect(wcd938x->us_euro_mux); > + > + return 0; > +} > > - value = gpiod_get_value(wcd938x->us_euro_gpio); > +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) > +{ > + struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component); > > - gpiod_set_value(wcd938x->us_euro_gpio, !value); > + if (wcd938x->us_euro_mux) { > + if (wcd938x_select_mux_state(component->dev, wcd938x, !wcd938x->mux_state)) > + return false; > + } else { > + gpiod_set_value(wcd938x->us_euro_gpio, !wcd938x->mux_state); > + } > > return true; > } > @@ -3261,11 +3278,22 @@ 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 = wcd938x_select_mux_state(dev, wcd938x, wcd938x->mux_state); > + if (ret) > + return ret; > + } > > cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; > > -- > 2.39.5 >
On 24/03/2025 13:50, Dmitry Baryshkov wrote: > On Mon, 24 Mar 2025 at 15:01, <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, 40 insertions(+), 11 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..88c758efe40d 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; >> + u32 mux_state; >> u32 micb1_mv; >> u32 micb2_mv; >> u32 micb3_mv; >> @@ -3235,17 +3238,31 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri >> dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); >> } >> >> -static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) >> +static int wcd938x_select_mux_state(struct device *dev, struct wcd938x_priv *wcd938x, int state) >> { >> - int value; >> + int ret = mux_control_try_select(wcd938x->us_euro_mux, state); > > Hmm. Does this really work? You have selected the mux in probe > function, now you are trying to select it again. If I'm reading the > code correctly, you will get -EBUSY here. On successful selection of mux state, the mux will be kept available (mux_control_deselect) for any new callers. So we will not get EBUSY for the second caller. --srini > >> >> - struct wcd938x_priv *wcd938x; >> + if (ret) { >> + dev_err(dev, "Error (%d) Unable to select us/euro mux state\n", ret); >> + return ret; >> + } >> >> - wcd938x = snd_soc_component_get_drvdata(component); >> + wcd938x->mux_state = state; >> + mux_control_deselect(wcd938x->us_euro_mux); >> + >> + return 0; >> +} >> >> - value = gpiod_get_value(wcd938x->us_euro_gpio); >> +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) >> +{ >> + struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component); >> >> - gpiod_set_value(wcd938x->us_euro_gpio, !value); >> + if (wcd938x->us_euro_mux) { >> + if (wcd938x_select_mux_state(component->dev, wcd938x, !wcd938x->mux_state)) >> + return false; >> + } else { >> + gpiod_set_value(wcd938x->us_euro_gpio, !wcd938x->mux_state); >> + } >> >> return true; >> } >> @@ -3261,11 +3278,22 @@ 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 = wcd938x_select_mux_state(dev, wcd938x, wcd938x->mux_state); >> + if (ret) >> + return ret; >> + } >> >> cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; >> >> -- >> 2.39.5 >> > >
On Mon, Mar 24, 2025 at 01:58:06PM +0000, Srinivas Kandagatla wrote: > > > On 24/03/2025 13:50, Dmitry Baryshkov wrote: > > On Mon, 24 Mar 2025 at 15:01, <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, 40 insertions(+), 11 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..88c758efe40d 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; > > > + u32 mux_state; > > > u32 micb1_mv; > > > u32 micb2_mv; > > > u32 micb3_mv; > > > @@ -3235,17 +3238,31 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri > > > dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); > > > } > > > > > > -static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) > > > +static int wcd938x_select_mux_state(struct device *dev, struct wcd938x_priv *wcd938x, int state) > > > { > > > - int value; > > > + int ret = mux_control_try_select(wcd938x->us_euro_mux, state); > > > > Hmm. Does this really work? You have selected the mux in probe > > function, now you are trying to select it again. If I'm reading the > > code correctly, you will get -EBUSY here. > > On successful selection of mux state, the mux will be kept available > (mux_control_deselect) for any new callers. > > So we will not get EBUSY for the second caller. No. wcd938x_populate_dt_data() selects the state by calling wcd938x_select_mux_state(). Then you call mux_control_try_select() here. As far as I understand, it will return -EBUSY as the sempahore is already taken. Moreover, this is not how the MUX API is supposed to be used. The driver is supposed to hold a state while it is still in use. > > --srini > > > > > > > > - struct wcd938x_priv *wcd938x; > > > + if (ret) { > > > + dev_err(dev, "Error (%d) Unable to select us/euro mux state\n", ret); > > > + return ret; > > > + } > > > > > > - wcd938x = snd_soc_component_get_drvdata(component); > > > + wcd938x->mux_state = state; > > > + mux_control_deselect(wcd938x->us_euro_mux); > > > + > > > + return 0; > > > +} > > > > > > - value = gpiod_get_value(wcd938x->us_euro_gpio); > > > +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) > > > +{ > > > + struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component); > > > > > > - gpiod_set_value(wcd938x->us_euro_gpio, !value); > > > + if (wcd938x->us_euro_mux) { > > > + if (wcd938x_select_mux_state(component->dev, wcd938x, !wcd938x->mux_state)) > > > + return false; > > > + } else { > > > + gpiod_set_value(wcd938x->us_euro_gpio, !wcd938x->mux_state); > > > + } > > > > > > return true; > > > } > > > @@ -3261,11 +3278,22 @@ 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 = wcd938x_select_mux_state(dev, wcd938x, wcd938x->mux_state); > > > + if (ret) > > > + return ret; > > > + } > > > > > > cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; > > > > > > -- > > > 2.39.5 > > > > > > >
On 24/03/2025 15:18, Dmitry Baryshkov wrote: > On Mon, Mar 24, 2025 at 01:58:06PM +0000, Srinivas Kandagatla wrote: >> >> >> On 24/03/2025 13:50, Dmitry Baryshkov wrote: >>> On Mon, 24 Mar 2025 at 15:01, <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, 40 insertions(+), 11 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..88c758efe40d 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; >>>> + u32 mux_state; >>>> u32 micb1_mv; >>>> u32 micb2_mv; >>>> u32 micb3_mv; >>>> @@ -3235,17 +3238,31 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri >>>> dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); >>>> } >>>> >>>> -static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) >>>> +static int wcd938x_select_mux_state(struct device *dev, struct wcd938x_priv *wcd938x, int state) >>>> { >>>> - int value; >>>> + int ret = mux_control_try_select(wcd938x->us_euro_mux, state); >>> >>> Hmm. Does this really work? You have selected the mux in probe >>> function, now you are trying to select it again. If I'm reading the >>> code correctly, you will get -EBUSY here. >> >> On successful selection of mux state, the mux will be kept available >> (mux_control_deselect) for any new callers. >> >> So we will not get EBUSY for the second caller. > > No. wcd938x_populate_dt_data() selects the state by calling > wcd938x_select_mux_state(). At this point we also released it (both in success and error case). This will hold on to the previous state unless we have defined a fallback idle-state. Then you call mux_control_try_select() here. > As far as I understand, it will return -EBUSY as the sempahore is > already taken. Moreover, this is not how the MUX API is supposed to be > used. The driver is supposed to hold a state while it is still in use. > >> >> --srini >>> >>>> >>>> - struct wcd938x_priv *wcd938x; >>>> + if (ret) { >>>> + dev_err(dev, "Error (%d) Unable to select us/euro mux state\n", ret); >>>> + return ret; >>>> + } >>>> >>>> - wcd938x = snd_soc_component_get_drvdata(component); >>>> + wcd938x->mux_state = state; >>>> + mux_control_deselect(wcd938x->us_euro_mux); >>>> + >>>> + return 0; >>>> +} >>>> >>>> - value = gpiod_get_value(wcd938x->us_euro_gpio); >>>> +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) >>>> +{ >>>> + struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component); >>>> >>>> - gpiod_set_value(wcd938x->us_euro_gpio, !value); >>>> + if (wcd938x->us_euro_mux) { >>>> + if (wcd938x_select_mux_state(component->dev, wcd938x, !wcd938x->mux_state)) >>>> + return false; >>>> + } else { >>>> + gpiod_set_value(wcd938x->us_euro_gpio, !wcd938x->mux_state); >>>> + } >>>> >>>> return true; >>>> } >>>> @@ -3261,11 +3278,22 @@ 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 = wcd938x_select_mux_state(dev, wcd938x, wcd938x->mux_state); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> >>>> cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; >>>> >>>> -- >>>> 2.39.5 >>>> >>> >>> >
On Mon, Mar 24, 2025 at 03:58:32PM +0000, Srinivas Kandagatla wrote: > > > On 24/03/2025 15:18, Dmitry Baryshkov wrote: > > On Mon, Mar 24, 2025 at 01:58:06PM +0000, Srinivas Kandagatla wrote: > > > > > > > > > On 24/03/2025 13:50, Dmitry Baryshkov wrote: > > > > On Mon, 24 Mar 2025 at 15:01, <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, 40 insertions(+), 11 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..88c758efe40d 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; > > > > > + u32 mux_state; > > > > > u32 micb1_mv; > > > > > u32 micb2_mv; > > > > > u32 micb3_mv; > > > > > @@ -3235,17 +3238,31 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri > > > > > dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); > > > > > } > > > > > > > > > > -static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) > > > > > +static int wcd938x_select_mux_state(struct device *dev, struct wcd938x_priv *wcd938x, int state) > > > > > { > > > > > - int value; > > > > > + int ret = mux_control_try_select(wcd938x->us_euro_mux, state); > > > > > > > > Hmm. Does this really work? You have selected the mux in probe > > > > function, now you are trying to select it again. If I'm reading the > > > > code correctly, you will get -EBUSY here. > > > > > > On successful selection of mux state, the mux will be kept available > > > (mux_control_deselect) for any new callers. > > > > > > So we will not get EBUSY for the second caller. > > > > No. wcd938x_populate_dt_data() selects the state by calling > > wcd938x_select_mux_state(). > > At this point we also released it (both in success and error case). By which code path? You call this function directly from wcd938x_swap_gnd_mic() and the control is already selected from the probe path. > This will hold on to the previous state unless we have defined a fallback > idle-state. As I wrote, this doesn't seem to be a proper use of MUX API. I think it's much easier to add a boolean which states that MUX has been selected and use it to deselect the state both in this function and in shutdown path. Another option is really to expand the MUX API to add 'switch state' call which will work if the state is already selected. > > > Then you call mux_control_try_select() here. > > As far as I understand, it will return -EBUSY as the sempahore is > already taken. Moreover, this is not how the MUX API is supposed to be > > used. The driver is supposed to hold a state while it is still in use. > > > > > > > > --srini > > > > > > > > > > > > > > - struct wcd938x_priv *wcd938x; > > > > > + if (ret) { > > > > > + dev_err(dev, "Error (%d) Unable to select us/euro mux state\n", ret); > > > > > + return ret; > > > > > + } > > > > > > > > > > - wcd938x = snd_soc_component_get_drvdata(component); > > > > > + wcd938x->mux_state = state; > > > > > + mux_control_deselect(wcd938x->us_euro_mux); > > > > > + > > > > > + return 0; > > > > > +} > > > > > > > > > > - value = gpiod_get_value(wcd938x->us_euro_gpio); > > > > > +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) > > > > > +{ > > > > > + struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component); > > > > > > > > > > - gpiod_set_value(wcd938x->us_euro_gpio, !value); > > > > > + if (wcd938x->us_euro_mux) { > > > > > + if (wcd938x_select_mux_state(component->dev, wcd938x, !wcd938x->mux_state)) > > > > > + return false; > > > > > + } else { > > > > > + gpiod_set_value(wcd938x->us_euro_gpio, !wcd938x->mux_state); Which codepath will update mux_state in case of the GPIO? > > > > > + } > > > > > > > > > > return true; > > > > > } > > > > > @@ -3261,11 +3278,22 @@ 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 = wcd938x_select_mux_state(dev, wcd938x, wcd938x->mux_state); > > > > > + if (ret) > > > > > + return ret; > > > > > + } > > > > > > > > > > cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; > > > > > > > > > > -- > > > > > 2.39.5 > > > > > > > > > > > > > > >
Hi! 2025-03-24 at 16:58, Srinivas Kandagatla wrote: > > > On 24/03/2025 15:18, Dmitry Baryshkov wrote: >> On Mon, Mar 24, 2025 at 01:58:06PM +0000, Srinivas Kandagatla wrote: >>> >>> >>> On 24/03/2025 13:50, Dmitry Baryshkov wrote: >>>> On Mon, 24 Mar 2025 at 15:01, <srinivas.kandagatla@linaro.org> wrote: >>>>> >>>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> *snip* >>>>> + int ret = mux_control_try_select(wcd938x->us_euro_mux, state); >>>> >>>> Hmm. Does this really work? You have selected the mux in probe >>>> function, now you are trying to select it again. If I'm reading the >>>> code correctly, you will get -EBUSY here. >>> >>> On successful selection of mux state, the mux will be kept available >>> (mux_control_deselect) for any new callers. >>> >>> So we will not get EBUSY for the second caller. >> >> No. wcd938x_populate_dt_data() selects the state by calling >> wcd938x_select_mux_state(). > > At this point we also released it (both in success and error case). > > This will hold on to the previous state unless we have defined a fallback idle-state. > > > Then you call mux_control_try_select() here. >> As far as I understand, it will return -EBUSY as the sempahore is > already taken. Moreover, this is not how the MUX API is supposed to be >> used. The driver is supposed to hold a state while it is still in use. Dmitry is correct. A mux consumer is supposed to keep the mux selected while it needs the mux to remain in a certain state. Relying on details such as idle as-is and that no other consumer butts in and clobbers the state is fragile. Mux access is not exclusive, at least not until a mux state is selected. Cheers, Peter
On 24/03/2025 16:33, Peter Rosin wrote: > Hi! > > 2025-03-24 at 16:58, Srinivas Kandagatla wrote: >> >> >> On 24/03/2025 15:18, Dmitry Baryshkov wrote: >>> On Mon, Mar 24, 2025 at 01:58:06PM +0000, Srinivas Kandagatla wrote: >>>> >>>> >>>> On 24/03/2025 13:50, Dmitry Baryshkov wrote: >>>>> On Mon, 24 Mar 2025 at 15:01, <srinivas.kandagatla@linaro.org> wrote: >>>>>> >>>>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > *snip* > >>>>>> + int ret = mux_control_try_select(wcd938x->us_euro_mux, state); >>>>> >>>>> Hmm. Does this really work? You have selected the mux in probe >>>>> function, now you are trying to select it again. If I'm reading the >>>>> code correctly, you will get -EBUSY here. >>>> >>>> On successful selection of mux state, the mux will be kept available >>>> (mux_control_deselect) for any new callers. >>>> >>>> So we will not get EBUSY for the second caller. >>> >>> No. wcd938x_populate_dt_data() selects the state by calling >>> wcd938x_select_mux_state(). >> >> At this point we also released it (both in success and error case). >> >> This will hold on to the previous state unless we have defined a fallback idle-state. >> >> >> Then you call mux_control_try_select() here. >>> As far as I understand, it will return -EBUSY as the sempahore is > already taken. Moreover, this is not how the MUX API is supposed to be >>> used. The driver is supposed to hold a state while it is still in use. > > Dmitry is correct. A mux consumer is supposed to keep the mux selected > while it needs the mux to remain in a certain state. Relying on details > such as idle as-is and that no other consumer butts in and clobbers the > state is fragile. Mux access is not exclusive, at least not until a > mux state is selected. Thanks Peter, I agree that its fragile to depend on idle as-is flags. Will update accordingly. --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..88c758efe40d 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; + u32 mux_state; u32 micb1_mv; u32 micb2_mv; u32 micb3_mv; @@ -3235,17 +3238,31 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); } -static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) +static int wcd938x_select_mux_state(struct device *dev, struct wcd938x_priv *wcd938x, int state) { - int value; + int ret = mux_control_try_select(wcd938x->us_euro_mux, state); - struct wcd938x_priv *wcd938x; + if (ret) { + dev_err(dev, "Error (%d) Unable to select us/euro mux state\n", ret); + return ret; + } - wcd938x = snd_soc_component_get_drvdata(component); + wcd938x->mux_state = state; + mux_control_deselect(wcd938x->us_euro_mux); + + return 0; +} - value = gpiod_get_value(wcd938x->us_euro_gpio); +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component) +{ + struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component); - gpiod_set_value(wcd938x->us_euro_gpio, !value); + if (wcd938x->us_euro_mux) { + if (wcd938x_select_mux_state(component->dev, wcd938x, !wcd938x->mux_state)) + return false; + } else { + gpiod_set_value(wcd938x->us_euro_gpio, !wcd938x->mux_state); + } return true; } @@ -3261,11 +3278,22 @@ 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 = wcd938x_select_mux_state(dev, wcd938x, wcd938x->mux_state); + if (ret) + return ret; + } cfg->swap_gnd_mic = wcd938x_swap_gnd_mic;