[PATCH 0/3] ASoC: codec: wcd93xx: Convert to GPIO descriptors

[PATCH 0/3] ASoC: codec: wcd93xx: Convert to GPIO descriptors

[Peng Fan (OSS)]

of_gpio.h is deprecated, so update driver to use gpiod API.

The current driver use value 0 to assert reset and 1 to deassert reset.
The DTSes in tree that use the codec are using GPIO_ACTIVE_LOW.
So it is safe to use devm_gpiod_get to get GPIO descriptors and
use gpiod_set_value to configure output with value 1 means raw value
0, value 0 means raw value 1.

Note:
I not have devices to test, just my best pratice to do the convertion.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (3):
      ASoC: codec: wcd939x: Convert to GPIO descriptors
      ASoC: codec: wcd938x: Convert to GPIO descriptors
      ASoC: codec: wcd9335: Convert to GPIO descriptors

 sound/soc/codecs/wcd9335.c | 15 +++++++--------
 sound/soc/codecs/wcd938x.c | 13 ++++++-------
 sound/soc/codecs/wcd939x.c | 16 ++++++++--------
 3 files changed, 21 insertions(+), 23 deletions(-)
---
base-commit: 9388ec571cb1adba59d1cded2300eeb11827679c
change-id: 20250324-wcd-gpiod-782c4a0d8688

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH 1/3] ASoC: codec: wcd939x: Convert to GPIO descriptors

[Peng Fan (OSS)]

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/wcd939x.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/wcd939x.c b/sound/soc/codecs/wcd939x.c
index 0a87a79772da6c0ed3c7dd7d098e949b9cead2a4..f8d7d03f84fcbca57826dd43e9281aff5b225525 100644
--- a/sound/soc/codecs/wcd939x.c
+++ b/sound/soc/codecs/wcd939x.c
@@ -15,7 +15,6 @@
 #include <linux/pm_runtime.h>
 #include <linux/component.h>
 #include <sound/tlv.h>
-#include <linux/of_gpio.h>
 #include <linux/of_graph.h>
 #include <linux/of.h>
 #include <sound/jack.h>
@@ -201,7 +200,7 @@ struct wcd939x_priv {
 	u32 hph_mode;
 	u32 tx_mode[TX_ADC_MAX];
 	int variant;
-	int reset_gpio;
+	struct gpio_desc *reset_gpio;
 	u32 micb1_mv;
 	u32 micb2_mv;
 	u32 micb3_mv;
@@ -3239,10 +3238,11 @@ static int wcd939x_populate_dt_data(struct wcd939x_priv *wcd939x, struct device
 #endif /* CONFIG_TYPEC */
 	int ret;
 
-	wcd939x->reset_gpio = of_get_named_gpio(dev->of_node, "reset-gpios", 0);
-	if (wcd939x->reset_gpio < 0)
-		return dev_err_probe(dev, wcd939x->reset_gpio,
-				     "Failed to get reset gpio\n");
+	wcd939x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
+	if (IS_ERR(wcd939x->reset_gpio)) {
+		ret = PTR_ERR(wcd939x->reset_gpio);
+		return dev_err_probe(dev, ret, "Failed to get reset gpio\n");
+	}
 
 	wcd939x->supplies[0].supply = "vdd-rxtx";
 	wcd939x->supplies[1].supply = "vdd-io";
@@ -3290,10 +3290,10 @@ static int wcd939x_populate_dt_data(struct wcd939x_priv *wcd939x, struct device
 
 static int wcd939x_reset(struct wcd939x_priv *wcd939x)
 {
-	gpio_direction_output(wcd939x->reset_gpio, 0);
+	gpiod_set_value(wcd939x->reset_gpio, 1);
 	/* 20us sleep required after pulling the reset gpio to LOW */
 	usleep_range(20, 30);
-	gpio_set_value(wcd939x->reset_gpio, 1);
+	gpiod_set_value(wcd939x->reset_gpio, 0);
 	/* 20us sleep required after pulling the reset gpio to HIGH */
 	usleep_range(20, 30);
 

-- 
2.37.1

Re: [PATCH 1/3] ASoC: codec: wcd939x: Convert to GPIO descriptors

[Linus Walleij]

Hi Peng,

thanks for your patch, and thanks a *LOT* for working on the gpio descriptor
task!!

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>

The patch as such is perfect:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

However we need to watch out for users:
$ git grep qcom,wcd939
arch/arm64/boot/dts/qcom/sm8650-hdk.dts:                compatible =
"qcom,wcd9395-codec", "qcom,wcd9390-codec";
arch/arm64/boot/dts/qcom/sm8650-hdk.dts:                compatible =
"qcom,wcd9395-usbss", "qcom,wcd9390-usbss";

reset-gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;

arch/arm64/boot/dts/qcom/sm8650-qrd.dts:                compatible =
"qcom,wcd9395-codec", "qcom,wcd9390-codec";
arch/arm64/boot/dts/qcom/sm8650-qrd.dts:                compatible =
"qcom,wcd9395-usbss", "qcom,wcd9390-usbss";

reset-gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;

Mention in the commit message that the in-tree DTS files have the right
polarity set up already so we can expect this to "just work".

I would also mention that the current code in the driver is quite ugly:
it doesn't even request the GPIO before starting to use it :/

Yours,
Linus Walleij

[PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Peng Fan (OSS)]

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

Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Steev Klimaszewski]

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

RE: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Peng Fan]

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

Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Linus Walleij]

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.

Yours,
Linus Walleij

RE: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Peng Fan]

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

Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[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

Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Johan Hovold]

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

RE: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Peng Fan]

> 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

Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Linus Walleij]

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

RE: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Peng Fan]

> 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

Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[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

Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Johan Hovold]

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

Re: [PATCH 2/3] ASoC: codec: wcd938x: Convert to GPIO descriptors

[Linus Walleij]

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

[PATCH 3/3] ASoC: codec: wcd9335: Convert to GPIO descriptors

[Peng Fan (OSS)]

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/wcd9335.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c
index 7cef43bb2a8800971604b0e1ac55aac26c5919bd..90cf4f79cbde5f704549d41771f085574045ff46 100644
--- a/sound/soc/codecs/wcd9335.c
+++ b/sound/soc/codecs/wcd9335.c
@@ -17,7 +17,7 @@
 #include <sound/soc.h>
 #include <sound/pcm_params.h>
 #include <sound/soc-dapm.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <sound/tlv.h>
@@ -331,7 +331,7 @@ struct wcd9335_codec {
 	int comp_enabled[COMPANDER_MAX];
 
 	int intr1;
-	int reset_gpio;
+	struct gpio_desc *reset_gpio;
 	struct regulator_bulk_data supplies[WCD9335_MAX_SUPPLY];
 
 	unsigned int rx_port_value[WCD9335_RX_MAX];
@@ -4975,12 +4975,11 @@ static const struct regmap_irq_chip wcd9335_regmap_irq1_chip = {
 static int wcd9335_parse_dt(struct wcd9335_codec *wcd)
 {
 	struct device *dev = wcd->dev;
-	struct device_node *np = dev->of_node;
 	int ret;
 
-	wcd->reset_gpio = of_get_named_gpio(np,	"reset-gpios", 0);
-	if (wcd->reset_gpio < 0)
-		return dev_err_probe(dev, wcd->reset_gpio, "Reset GPIO missing from DT\n");
+	wcd->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
+	if (IS_ERR(wcd->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(wcd->reset_gpio), "Reset GPIO missing from DT\n");
 
 	wcd->mclk = devm_clk_get(dev, "mclk");
 	if (IS_ERR(wcd->mclk))
@@ -5023,9 +5022,9 @@ static int wcd9335_power_on_reset(struct wcd9335_codec *wcd)
 	 */
 	usleep_range(600, 650);
 
-	gpio_direction_output(wcd->reset_gpio, 0);
+	gpiod_set_value(wcd->reset_gpio, 1);
 	msleep(20);
-	gpio_set_value(wcd->reset_gpio, 1);
+	gpiod_set_value(wcd->reset_gpio, 0);
 	msleep(20);
 
 	return 0;

-- 
2.37.1

Re: [PATCH 3/3] ASoC: codec: wcd9335: Convert to GPIO descriptors

[Linus Walleij]

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