Re: [PATCH v4 5/6] ASoC: codecs: wcd938x: add mux control support for hp audio mux

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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
>>>>
>>>
>>>
>