| Message ID | 20250324-wcd-gpiod-v1-2-27afa472e331@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: codec: wcd93xx: Convert to GPIO descriptors | expand |
Hi Peng Fan, On Sun, Mar 23, 2025 at 9:28 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > of_gpio.h is deprecated, update the driver to use GPIO descriptors. > - Use dev_gpiod_get to get GPIO descriptor. > - Use gpiod_set_value to configure output value. > > With legacy of_gpio API, the driver set gpio value 0 to assert reset, > and 1 to deassert reset. And the reset-gpios use GPIO_ACTIVE_LOW flag in > DTS, so set GPIOD_ASIS when get GPIO descriptors, and set value 1 means > output low, set value 0 means output high with gpiod API. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > sound/soc/codecs/wcd938x.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c > index 1ae498c323912ed799dcc033e7777936d90c9284..c70da29406f36883e4926eca40ab5ba5df02c383 100644 > --- a/sound/soc/codecs/wcd938x.c > +++ b/sound/soc/codecs/wcd938x.c > @@ -11,7 +11,6 @@ > #include <linux/pm_runtime.h> > #include <linux/component.h> > #include <sound/tlv.h> > -#include <linux/of_gpio.h> > #include <linux/of.h> > #include <sound/jack.h> > #include <sound/pcm.h> > @@ -171,7 +170,7 @@ struct wcd938x_priv { > int flyback_cur_det_disable; > int ear_rx_path; > int variant; > - int reset_gpio; > + struct gpio_desc *reset_gpio; > struct gpio_desc *us_euro_gpio; > u32 micb1_mv; > u32 micb2_mv; > @@ -3251,9 +3250,9 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device > struct wcd_mbhc_config *cfg = &wcd938x->mbhc_cfg; > int ret; > > - wcd938x->reset_gpio = of_get_named_gpio(dev->of_node, "reset-gpios", 0); > - if (wcd938x->reset_gpio < 0) > - return dev_err_probe(dev, wcd938x->reset_gpio, > + wcd938x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS); > + if (IS_ERR(wcd938x->reset_gpio)) > + return dev_err_probe(dev, PTR_ERR(wcd938x->reset_gpio), > "Failed to get reset gpio\n"); > > wcd938x->us_euro_gpio = devm_gpiod_get_optional(dev, "us-euro", > @@ -3297,10 +3296,10 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device > > static int wcd938x_reset(struct wcd938x_priv *wcd938x) > { > - gpio_direction_output(wcd938x->reset_gpio, 0); > + gpiod_set_value(wcd938x->reset_gpio, 1); > /* 20us sleep required after pulling the reset gpio to LOW */ > usleep_range(20, 30); > - gpio_set_value(wcd938x->reset_gpio, 1); > + gpiod_set_value(wcd938x->reset_gpio, 0); > /* 20us sleep required after pulling the reset gpio to HIGH */ > usleep_range(20, 30); > > > -- > 2.37.1 > > With this patchset applied, the wcd938x codec used in the Thinkpad X13s stops working: wcd938x_codec audio-codec: soundwire device init timeout wcd938x_codec audio-codec: ASoC: error at snd_soc_component_probe on audio-codec: -110 snd-sc8280xp sound: ASoC: failed to instantiate card -110 snd-sc8280xp sound: probe with driver snd-sc8280xp failed with error -110 -- steev
On Mon, Mar 24, 2025 at 3:28 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > From: Peng Fan <peng.fan@nxp.com> > > of_gpio.h is deprecated, update the driver to use GPIO descriptors. > - Use dev_gpiod_get to get GPIO descriptor. > - Use gpiod_set_value to configure output value. > > With legacy of_gpio API, the driver set gpio value 0 to assert reset, > and 1 to deassert reset. And the reset-gpios use GPIO_ACTIVE_LOW flag in > DTS, so set GPIOD_ASIS when get GPIO descriptors, and set value 1 means > output low, set value 0 means output high with gpiod API. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Steev, > Subject: Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO > descriptors > > Hi Peng Fan, > > On Sun, Mar 23, 2025 at 9:28 PM Peng Fan (OSS) > <peng.fan@oss.nxp.com> wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > of_gpio.h is deprecated, update the driver to use GPIO descriptors. > > - Use dev_gpiod_get to get GPIO descriptor. > > - Use gpiod_set_value to configure output value. > > > > With legacy of_gpio API, the driver set gpio value 0 to assert reset, > > and 1 to deassert reset. And the reset-gpios use GPIO_ACTIVE_LOW > flag > > in DTS, so set GPIOD_ASIS when get GPIO descriptors, and set value 1 > > means output low, set value 0 means output high with gpiod API. > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > sound/soc/codecs/wcd938x.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/sound/soc/codecs/wcd938x.c > b/sound/soc/codecs/wcd938x.c > > index > > > 1ae498c323912ed799dcc033e7777936d90c9284..c70da29406f36883 > e4926eca40ab > > 5ba5df02c383 100644 > > --- a/sound/soc/codecs/wcd938x.c > > +++ b/sound/soc/codecs/wcd938x.c > > @@ -11,7 +11,6 @@ > > #include <linux/pm_runtime.h> > > #include <linux/component.h> > > #include <sound/tlv.h> > > -#include <linux/of_gpio.h> > > #include <linux/of.h> > > #include <sound/jack.h> > > #include <sound/pcm.h> > > @@ -171,7 +170,7 @@ struct wcd938x_priv { > > int flyback_cur_det_disable; > > int ear_rx_path; > > int variant; > > - int reset_gpio; > > + struct gpio_desc *reset_gpio; > > struct gpio_desc *us_euro_gpio; > > u32 micb1_mv; > > u32 micb2_mv; > > @@ -3251,9 +3250,9 @@ static int > wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct > device > > struct wcd_mbhc_config *cfg = &wcd938x->mbhc_cfg; > > int ret; > > > > - wcd938x->reset_gpio = of_get_named_gpio(dev->of_node, > "reset-gpios", 0); > > - if (wcd938x->reset_gpio < 0) > > - return dev_err_probe(dev, wcd938x->reset_gpio, > > + wcd938x->reset_gpio = devm_gpiod_get(dev, "reset", > GPIOD_ASIS); > > + if (IS_ERR(wcd938x->reset_gpio)) > > + return dev_err_probe(dev, > > + PTR_ERR(wcd938x->reset_gpio), > > "Failed to get reset gpio\n"); > > > > wcd938x->us_euro_gpio = devm_gpiod_get_optional(dev, > > "us-euro", @@ -3297,10 +3296,10 @@ static int > > wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct > device > > > > static int wcd938x_reset(struct wcd938x_priv *wcd938x) { > > - gpio_direction_output(wcd938x->reset_gpio, 0); > > + gpiod_set_value(wcd938x->reset_gpio, 1); > > /* 20us sleep required after pulling the reset gpio to LOW */ > > usleep_range(20, 30); > > - gpio_set_value(wcd938x->reset_gpio, 1); > > + gpiod_set_value(wcd938x->reset_gpio, 0); > > /* 20us sleep required after pulling the reset gpio to HIGH */ > > usleep_range(20, 30); > > > > > > -- > > 2.37.1 > > > > > > With this patchset applied, the wcd938x codec used in the Thinkpad > X13s stops working: > > wcd938x_codec audio-codec: soundwire device init timeout > wcd938x_codec audio-codec: ASoC: error at > snd_soc_component_probe on > audio-codec: -110 > snd-sc8280xp sound: ASoC: failed to instantiate card -110 snd- > sc8280xp sound: probe with driver snd-sc8280xp failed with error -110 Thanks for help testing. But per current in-tree DTS, the reset is using GPIO_ACTIVE_LOW, so it should work. I am not sure whether you are using firmware published DTS, if yes, could you please help check the codec node to dump the reset-gpios property under /sys/firmware/devicetree/xx ? Thanks, Peng. > > -- steev
On Mon, Mar 24, 2025 at 8:33 AM Peng Fan <peng.fan@nxp.com> wrote: > > With this patchset applied, the wcd938x codec used in the Thinkpad > > X13s stops working: > > > > wcd938x_codec audio-codec: soundwire device init timeout > > wcd938x_codec audio-codec: ASoC: error at > > snd_soc_component_probe on > > audio-codec: -110 > > snd-sc8280xp sound: ASoC: failed to instantiate card -110 snd- > > sc8280xp sound: probe with driver snd-sc8280xp failed with error -110 > > Thanks for help testing. But per current in-tree DTS, the reset is using > GPIO_ACTIVE_LOW, so it should work. > > I am not sure whether you are using firmware published DTS, > if yes, could you please help check the codec node to dump > the reset-gpios property under /sys/firmware/devicetree/xx ? I'm also a bit puzzled. I think maybe this device has some DTB that comes from the vendor with the wrong polarity :/ If this is the case we need to add a quirk to gpiolib to force this GPIO into active low, something like this: From dfe3d2a12a63135e917abacd0d3a29ce347a6cf9 Mon Sep 17 00:00:00 2001 From: Linus Walleij <linus.walleij@linaro.org> Date: Mon, 24 Mar 2025 08:44:45 +0100 Subject: [PATCH] Fix WCD938x polarity Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/gpiolib-of.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 2e537ee979f3..3baaddedb7b6 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -220,6 +220,15 @@ static void of_gpio_try_fixup_polarity(const struct device_node *np, * treats it as "active low". */ { "ti,tsc2005", "reset-gpios", false }, +#endif +#if IS_ENABLED(SND_SOC_WCD938X) + /* + * This codec is used in laptops with deployed devicetrees + * that fail to specify the correct active low property for + * the reset line. + */ + { "qcom,wcd9380-codec", "reset-gpios", false }, + { "qcom,wcd9385-codec", "reset-gpios", false }, #endif }; unsigned int i;
Hi Linus, > Subject: Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO > descriptors > > On Mon, Mar 24, 2025 at 8:33 AM Peng Fan <peng.fan@nxp.com> > wrote: > > > > With this patchset applied, the wcd938x codec used in the > Thinkpad > > > X13s stops working: > > > > > > wcd938x_codec audio-codec: soundwire device init timeout > > > wcd938x_codec audio-codec: ASoC: error at > snd_soc_component_probe on > > > audio-codec: -110 > > > snd-sc8280xp sound: ASoC: failed to instantiate card -110 snd- > > > sc8280xp sound: probe with driver snd-sc8280xp failed with error > > > -110 > > > > Thanks for help testing. But per current in-tree DTS, the reset is > > using GPIO_ACTIVE_LOW, so it should work. > > > > I am not sure whether you are using firmware published DTS, if yes, > > could you please help check the codec node to dump the reset-gpios > > property under /sys/firmware/devicetree/xx ? > > I'm also a bit puzzled. > > I think maybe this device has some DTB that comes from the vendor > with the wrong polarity :/ > > If this is the case we need to add a quirk to gpiolib to force this GPIO > into active low, something like this: > > From dfe3d2a12a63135e917abacd0d3a29ce347a6cf9 Mon Sep 17 > 00:00:00 2001 > From: Linus Walleij <linus.walleij@linaro.org> > Date: Mon, 24 Mar 2025 08:44:45 +0100 > Subject: [PATCH] Fix WCD938x polarity > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/gpiolib-of.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index > 2e537ee979f3..3baaddedb7b6 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -220,6 +220,15 @@ static void of_gpio_try_fixup_polarity(const > struct device_node *np, > * treats it as "active low". > */ > { "ti,tsc2005", "reset-gpios", false }, > +#endif > +#if IS_ENABLED(SND_SOC_WCD938X) > + /* > + * This codec is used in laptops with deployed devicetrees > + * that fail to specify the correct active low property for > + * the reset line. > + */ > + { "qcom,wcd9380-codec", "reset-gpios", false }, > + { "qcom,wcd9385-codec", "reset-gpios", false }, > #endif > }; > unsigned int i; > -- > 2.48.1 > > Maybe you can fold this into your patch if it helps. And if there are > more of the codecs with this problem, we need a similar patch in each > one of them. Thanks for your quick reply. I am thinking whether we need to force the polarity of remaining of_gpio users. Some old devices may not able to get tested, and some may use firmware to publish DTS during runtime(such as Fedora, openSUSE using firmware based DT when doing System-Ready IR), so we are not sure whether using gpiod api break the platforms or not. Thanks, Peng. > > Yours, > Linus Walleij
On Mon, Mar 24, 2025 at 8:53 AM Peng Fan <peng.fan@nxp.com> wrote: > > Maybe you can fold this into your patch if it helps. And if there are > > more of the codecs with this problem, we need a similar patch in each > > one of them. > > Thanks for your quick reply. I am thinking whether we need > to force the polarity of remaining of_gpio users. > Some old devices may not able to get tested, and some > may use firmware to publish DTS during runtime(such as Fedora, > openSUSE using firmware based DT when doing System-Ready IR), > so we are not sure whether using gpiod api break the platforms > or not. If we have an indication that this problem is common (let's wait for Steev to confirm) we might have to add a small patch hunk like my quirk to each patch to make sure we hammer down the polarity of these codec reset lines any time they are used. It's fairly straight forward, as you can see, and they are only compiled in if strictly needed anyway. Yours, Linus Walleij
On Mon, Mar 24, 2025 at 08:46:07AM +0100, Linus Walleij wrote: > On Mon, Mar 24, 2025 at 8:33 AM Peng Fan <peng.fan@nxp.com> wrote: > > > > With this patchset applied, the wcd938x codec used in the Thinkpad > > > X13s stops working: > > > > > > wcd938x_codec audio-codec: soundwire device init timeout > > > wcd938x_codec audio-codec: ASoC: error at > > > snd_soc_component_probe on > > > audio-codec: -110 > > > snd-sc8280xp sound: ASoC: failed to instantiate card -110 snd- > > > sc8280xp sound: probe with driver snd-sc8280xp failed with error -110 > > > > Thanks for help testing. But per current in-tree DTS, the reset is using > > GPIO_ACTIVE_LOW, so it should work. > > > > I am not sure whether you are using firmware published DTS, > > if yes, could you please help check the codec node to dump > > the reset-gpios property under /sys/firmware/devicetree/xx ? > > I'm also a bit puzzled. > > I think maybe this device has some DTB that comes from the vendor > with the wrong polarity :/ > > If this is the case we need to add a quirk to gpiolib to force this > GPIO into active low, something like this: I'm quite sure Steev is using the mainline devicetree with correct polarity so that should not be the issue here. Johan
> Subject: Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO > descriptors > > On Mon, Mar 24, 2025 at 08:46:07AM +0100, Linus Walleij wrote: > > On Mon, Mar 24, 2025 at 8:33 AM Peng Fan <peng.fan@nxp.com> > wrote: > > > > > > With this patchset applied, the wcd938x codec used in the > Thinkpad > > > > X13s stops working: > > > > > > > > wcd938x_codec audio-codec: soundwire device init timeout > > > > wcd938x_codec audio-codec: ASoC: error at > snd_soc_component_probe > > > > on > > > > audio-codec: -110 > > > > snd-sc8280xp sound: ASoC: failed to instantiate card -110 snd- > > > > sc8280xp sound: probe with driver snd-sc8280xp failed with error > > > > -110 > > > > > > Thanks for help testing. But per current in-tree DTS, the reset is > > > using GPIO_ACTIVE_LOW, so it should work. > > > > > > I am not sure whether you are using firmware published DTS, if yes, > > > could you please help check the codec node to dump the reset- > gpios > > > property under /sys/firmware/devicetree/xx ? > > > > I'm also a bit puzzled. > > > > I think maybe this device has some DTB that comes from the vendor > with > > the wrong polarity :/ > > > > If this is the case we need to add a quirk to gpiolib to force this > > GPIO into active low, something like this: > > I'm quite sure Steev is using the mainline devicetree with correct > polarity so that should not be the issue here. ok, then the only suspecting point is wcd938x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS); I may need to use GPIOD_OUT_LOW to configure it to output as set raw set value as 1. Thanks, Peng. > > Johan
On Sun, Mar 23, 2025 at 10:40:51PM -0500, Steev Klimaszewski wrote: > On Sun, Mar 23, 2025 at 9:28 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > of_gpio.h is deprecated, update the driver to use GPIO descriptors. > > - Use dev_gpiod_get to get GPIO descriptor. > > - Use gpiod_set_value to configure output value. > > > > With legacy of_gpio API, the driver set gpio value 0 to assert reset, > > and 1 to deassert reset. And the reset-gpios use GPIO_ACTIVE_LOW flag in > > DTS, so set GPIOD_ASIS when get GPIO descriptors, and set value 1 means > > output low, set value 0 means output high with gpiod API. > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > @@ -3251,9 +3250,9 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device > > struct wcd_mbhc_config *cfg = &wcd938x->mbhc_cfg; > > int ret; > > > > - wcd938x->reset_gpio = of_get_named_gpio(dev->of_node, "reset-gpios", 0); > > - if (wcd938x->reset_gpio < 0) > > - return dev_err_probe(dev, wcd938x->reset_gpio, > > + wcd938x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS); > > + if (IS_ERR(wcd938x->reset_gpio)) > > + return dev_err_probe(dev, PTR_ERR(wcd938x->reset_gpio), > > "Failed to get reset gpio\n"); > > > > wcd938x->us_euro_gpio = devm_gpiod_get_optional(dev, "us-euro", > > @@ -3297,10 +3296,10 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device > > > > static int wcd938x_reset(struct wcd938x_priv *wcd938x) > > { > > - gpio_direction_output(wcd938x->reset_gpio, 0); > > + gpiod_set_value(wcd938x->reset_gpio, 1); This may be what is causing the regression; the driver no longer configures the reset line as an output. From the docs: * GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must be set later with one of the dedicated functions. > > /* 20us sleep required after pulling the reset gpio to LOW */ > > usleep_range(20, 30); > > - gpio_set_value(wcd938x->reset_gpio, 1); > > + gpiod_set_value(wcd938x->reset_gpio, 0); > > /* 20us sleep required after pulling the reset gpio to HIGH */ > > usleep_range(20, 30); > With this patchset applied, the wcd938x codec used in the Thinkpad > X13s stops working: > > wcd938x_codec audio-codec: soundwire device init timeout > wcd938x_codec audio-codec: ASoC: error at snd_soc_component_probe on > audio-codec: -110 > snd-sc8280xp sound: ASoC: failed to instantiate card -110 > snd-sc8280xp sound: probe with driver snd-sc8280xp failed with error -110 Johan
On Mon, Mar 24, 2025 at 9:09 AM Peng Fan <peng.fan@nxp.com> wrote: > ok, then the only suspecting point is > wcd938x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS); > > I may need to use GPIOD_OUT_LOW to configure it > to output as set raw set value as 1. I think there may be a bug in gpiod_configure_flags() in gpiolib.c: /* Process flags */ if (dflags & GPIOD_FLAGS_BIT_DIR_OUT) ret = gpiod_direction_output_nonotify(desc, !!(dflags & GPIOD_FLAGS_BIT_DIR_VAL)); else ret = gpiod_direction_input_nonotify(desc); Shouldn't this be: if (dflags & GPIOD_FLAGS_BIT_DIR_OUT) ret = gpiod_direction_output_nonotify(desc, !!(dflags & GPIOD_FLAGS_BIT_DIR_VAL)); else if (dflags & GPIOD_FLAGS_BIT_DIR_SET) ret = gpiod_direction_input_nonotify(desc); ? As it looks, the line will be set into input mode unless explicitly requested as output... However this means the patch also has another bug: you need to either: 1. Specify GPIO_OUT_LOW when requesting it or 2. Explicitly call gpiod_direction_out() before setting the value. Yours, Linus Walleij
> Subject: Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO > descriptors > > On Mon, Mar 24, 2025 at 9:09 AM Peng Fan <peng.fan@nxp.com> > wrote: > > > ok, then the only suspecting point is > > wcd938x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS); > > > > I may need to use GPIOD_OUT_LOW to configure it to output as set > raw > > set value as 1. > > I think there may be a bug in gpiod_configure_flags() in gpiolib.c: > > /* Process flags */ > if (dflags & GPIOD_FLAGS_BIT_DIR_OUT) > ret = gpiod_direction_output_nonotify(desc, > !!(dflags & GPIOD_FLAGS_BIT_DIR_VAL)); > else > ret = gpiod_direction_input_nonotify(desc); > > Shouldn't this be: > > if (dflags & GPIOD_FLAGS_BIT_DIR_OUT) > ret = gpiod_direction_output_nonotify(desc, > !!(dflags & GPIOD_FLAGS_BIT_DIR_VAL)); > else if (dflags & GPIOD_FLAGS_BIT_DIR_SET) > ret = gpiod_direction_input_nonotify(desc); Using GPIO_ASIS should not change direction. This change makes sense. > > ? > > As it looks, the line will be set into input mode unless explicitly > requested as output... > > However this means the patch also has another bug: you need to either: > > 1. Specify GPIO_OUT_LOW when requesting it or 2. Explicitly call > gpiod_direction_out() before setting the value. 1 is better before the gpiolib bug got fixed. Then no need change direction twice. 2 is better aligned with current driver. There is no rush, so I could use 2 if you'd fix the gpiolib. BTW, could I still keep your R-b in my V2? Thanks, Peng. > > Yours, > Linus Walleij
On Mon, Mar 24, 2025 at 9:34 AM Peng Fan <peng.fan@nxp.com> wrote: > > Subject: Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO > > descriptors > > > > On Mon, Mar 24, 2025 at 9:09 AM Peng Fan <peng.fan@nxp.com> > > wrote: > > > > > ok, then the only suspecting point is > > > wcd938x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS); > > > > > > I may need to use GPIOD_OUT_LOW to configure it to output as set > > raw > > > set value as 1. > > > > I think there may be a bug in gpiod_configure_flags() in gpiolib.c: > > > > /* Process flags */ > > if (dflags & GPIOD_FLAGS_BIT_DIR_OUT) > > ret = gpiod_direction_output_nonotify(desc, > > !!(dflags & GPIOD_FLAGS_BIT_DIR_VAL)); > > else > > ret = gpiod_direction_input_nonotify(desc); > > > > Shouldn't this be: > > > > if (dflags & GPIOD_FLAGS_BIT_DIR_OUT) > > ret = gpiod_direction_output_nonotify(desc, > > !!(dflags & GPIOD_FLAGS_BIT_DIR_VAL)); > > else if (dflags & GPIOD_FLAGS_BIT_DIR_SET) > > ret = gpiod_direction_input_nonotify(desc); > > Using GPIO_ASIS should not change direction. > This change makes sense. Actually when looking closer at it I was wrong. From <linux/gpio/consumer.h>: enum gpiod_flags { GPIOD_ASIS = 0, GPIOD_IN = GPIOD_FLAGS_BIT_DIR_SET, GPIOD_OUT_LOW = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT, GPIOD_OUT_HIGH = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT | GPIOD_FLAGS_BIT_DIR_VAL, GPIOD_OUT_LOW_OPEN_DRAIN = GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_OPEN_DRAIN, GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_OPEN_DRAIN, }; So GPIOD_ASIS does not set GPIOD_FLAGS_BIT_DIR_SET. Then gpiod_configure_flags() has: /* No particular flag request, return here... */ if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) { gpiod_dbg(desc, "no flags found for GPIO %s\n", name); return 0; } /* Process flags */ if (dflags & GPIOD_FLAGS_BIT_DIR_OUT) ret = gpiod_direction_output_nonotify(desc, !!(dflags & GPIOD_FLAGS_BIT_DIR_VAL)); else ret = gpiod_direction_input_nonotify(desc); So the function bails out right above (you could check the debug print there if in doubt.) So the behaviour is correct. So now I have no idea what is going on again :/ But I would suggest trying to set GPIOD_OUT_LOW when requesting the line and just check if that solves Steev:s problem (trial-and-error). Yours, Linus Walleij
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c index 1ae498c323912ed799dcc033e7777936d90c9284..c70da29406f36883e4926eca40ab5ba5df02c383 100644 --- a/sound/soc/codecs/wcd938x.c +++ b/sound/soc/codecs/wcd938x.c @@ -11,7 +11,6 @@ #include <linux/pm_runtime.h> #include <linux/component.h> #include <sound/tlv.h> -#include <linux/of_gpio.h> #include <linux/of.h> #include <sound/jack.h> #include <sound/pcm.h> @@ -171,7 +170,7 @@ struct wcd938x_priv { int flyback_cur_det_disable; int ear_rx_path; int variant; - int reset_gpio; + struct gpio_desc *reset_gpio; struct gpio_desc *us_euro_gpio; u32 micb1_mv; u32 micb2_mv; @@ -3251,9 +3250,9 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device struct wcd_mbhc_config *cfg = &wcd938x->mbhc_cfg; int ret; - wcd938x->reset_gpio = of_get_named_gpio(dev->of_node, "reset-gpios", 0); - if (wcd938x->reset_gpio < 0) - return dev_err_probe(dev, wcd938x->reset_gpio, + wcd938x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS); + if (IS_ERR(wcd938x->reset_gpio)) + return dev_err_probe(dev, PTR_ERR(wcd938x->reset_gpio), "Failed to get reset gpio\n"); wcd938x->us_euro_gpio = devm_gpiod_get_optional(dev, "us-euro", @@ -3297,10 +3296,10 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device static int wcd938x_reset(struct wcd938x_priv *wcd938x) { - gpio_direction_output(wcd938x->reset_gpio, 0); + gpiod_set_value(wcd938x->reset_gpio, 1); /* 20us sleep required after pulling the reset gpio to LOW */ usleep_range(20, 30); - gpio_set_value(wcd938x->reset_gpio, 1); + gpiod_set_value(wcd938x->reset_gpio, 0); /* 20us sleep required after pulling the reset gpio to HIGH */ usleep_range(20, 30);