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

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

[Peng Fan (OSS)]

of_gpio.h is deprecated, update the driver to use GPIO descriptors.

Mostly about dropping legacy platform data usage, and switching
to use devm_gpiod_get_optional to get GPIO descriptors and use
gpiod_set_value to configure output.

Because of lacking of DTS users, I checked datasheet especially
for CS42Lxx and update code accordingly.

I not have devices to test, so just my best effort to do this work.
For cs42lxx codecs, there is no in-tree users for quite long time,
I was thinking to remove the drivers. But in case people have concern,
so I still do the convertion.
For those that have in-tree uers, I have added Cc in each patch and
appreciate if there is T-b from users.

With this patchset post out for ASoC, the left one under ASoC is
sound/arm/pxa2xx-ac97-lib.c which I have not looked into.

For others, below patches are alreay in maillist for reviewing.
ASoC: codec: sma1307: Remove including of_gpio.h
ASoC: codec: wcd9335: Convert to GPIO descriptors
ASoC: codec: wcd938x: Convert to GPIO descriptors
ASoC: codec: wcd939x: Convert to GPIO descriptors
ASoC: codec: ak5386: Convert to GPIO descriptors

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (7):
      ASoC: codec: tlv320aic32x4: Drop aic32x4_pdata usage
      ASoC: codec: tlv320aic32x4: Convert to GPIO descriptors
      ASoC: codec: twl4030: Convert to GPIO descriptors
      ASoC: codec: cs42l56: Convert to GPIO descriptors
      ASoC: codec: cs42l73: Convert to GPIO descriptors
      ASoC: codec: cs42l52: Convert to GPIO descriptors
      ASoC: codec: tpa6130a2: Convert to GPIO descriptors

 MAINTAINERS                      |   1 -
 include/sound/cs42l52.h          |  29 -----------
 include/sound/cs42l56.h          |  45 ----------------
 include/sound/cs42l73.h          |  19 -------
 include/sound/tlv320aic32x4.h    |   9 ----
 include/sound/tpa6130a2-plat.h   |  17 ------
 sound/soc/codecs/cs42l52.c       | 108 ++++++++++++++++++++-------------------
 sound/soc/codecs/cs42l56.c       |  91 +++++++++++++++++++++------------
 sound/soc/codecs/cs42l73.c       |  81 ++++++++++++++---------------
 sound/soc/codecs/tlv320aic32x4.c |  53 +++++++++----------
 sound/soc/codecs/tpa6130a2.c     |  54 ++++++--------------
 sound/soc/codecs/twl4030.c       |  76 +++++++++++----------------
 12 files changed, 220 insertions(+), 363 deletions(-)
---
base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c
change-id: 20250408-asoc-gpio-8862a7ae9090

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

[PATCH 1/7] ASoC: codec: tlv320aic32x4: Drop aic32x4_pdata usage

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

There is no machine is using aic32x4_pdata as platform_data, so
remove the dead code.

Cc: Markus Niebel <Markus.Niebel@ew.tq-group.com>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 include/sound/tlv320aic32x4.h    | 9 ---------
 sound/soc/codecs/tlv320aic32x4.c | 9 +--------
 2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/include/sound/tlv320aic32x4.h b/include/sound/tlv320aic32x4.h
index 0abf74d7edbd69484c45ad6a1c39b3f67d61bd63..b779d671a99576deadc6e647edff9b1b3a5d33c2 100644
--- a/include/sound/tlv320aic32x4.h
+++ b/include/sound/tlv320aic32x4.h
@@ -40,13 +40,4 @@
 struct aic32x4_setup_data {
 	unsigned int gpio_func[5];
 };
-
-struct aic32x4_pdata {
-	struct aic32x4_setup_data *setup;
-	u32 power_cfg;
-	u32 micpga_routing;
-	bool swapdacs;
-	int rstn_gpio;
-};
-
 #endif
diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index 54ea4bc58c276d9ab39a15d312287dfb300dbab9..7dbcf7f7130b04a27f58f20beb83eb3676c79c3d 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -1346,7 +1346,6 @@ int aic32x4_probe(struct device *dev, struct regmap *regmap,
 		  enum aic32x4_type type)
 {
 	struct aic32x4_priv *aic32x4;
-	struct aic32x4_pdata *pdata = dev->platform_data;
 	struct device_node *np = dev->of_node;
 	int ret;
 
@@ -1363,13 +1362,7 @@ int aic32x4_probe(struct device *dev, struct regmap *regmap,
 
 	dev_set_drvdata(dev, aic32x4);
 
-	if (pdata) {
-		aic32x4->power_cfg = pdata->power_cfg;
-		aic32x4->swapdacs = pdata->swapdacs;
-		aic32x4->micpga_routing = pdata->micpga_routing;
-		aic32x4->rstn_gpio = pdata->rstn_gpio;
-		aic32x4->mclk_name = "mclk";
-	} else if (np) {
+	if (np) {
 		ret = aic32x4_parse_dt(aic32x4, np);
 		if (ret) {
 			dev_err(dev, "Failed to parse DT node\n");

-- 
2.37.1

[PATCH 2/7] ASoC: codec: tlv320aic32x4: 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 devm_gpiod_get_optional to get GPIO descriptor, and set consumer
   name.
 - Use gpiod_set_value to configure output value.

reset pin is active low, so when request the gpio, set GPIOD_OUT_HIGH to
assert reset and later deassert reset with value set to 0.

While at here, reorder the included headers.

Checking the DTS that use the device, all are using GPIOD_ACTIVE_LOW
polarity for reset-gpios, so all should work as expected with this patch.

Cc: Markus Niebel <Markus.Niebel@ew.tq-group.com>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 sound/soc/codecs/tlv320aic32x4.c | 44 ++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index 7dbcf7f7130b04a27f58f20beb83eb3676c79c3d..1423186f5a6c181a20dd2dd552679d33174edaee 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -9,27 +9,26 @@
  * Based on sound/soc/codecs/wm8974 and TI driver for kernel 2.6.27.
  */
 
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/init.h>
-#include <linux/delay.h>
-#include <linux/pm.h>
-#include <linux/gpio.h>
-#include <linux/of_gpio.h>
 #include <linux/cdev.h>
-#include <linux/slab.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/of_clk.h>
+#include <linux/pm.h>
 #include <linux/regulator/consumer.h>
+#include <linux/slab.h>
 
-#include <sound/tlv320aic32x4.h>
 #include <sound/core.h>
+#include <sound/initval.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/soc-dapm.h>
-#include <sound/initval.h>
 #include <sound/tlv.h>
+#include <sound/tlv320aic32x4.h>
 
 #include "tlv320aic32x4.h"
 
@@ -38,7 +37,7 @@ struct aic32x4_priv {
 	u32 power_cfg;
 	u32 micpga_routing;
 	bool swapdacs;
-	int rstn_gpio;
+	struct gpio_desc *rstn_gpio;
 	const char *mclk_name;
 
 	struct regulator *supply_ldo;
@@ -1236,7 +1235,14 @@ static int aic32x4_parse_dt(struct aic32x4_priv *aic32x4,
 
 	aic32x4->swapdacs = false;
 	aic32x4->micpga_routing = 0;
-	aic32x4->rstn_gpio = of_get_named_gpio(np, "reset-gpios", 0);
+	/* Assert reset using GPIOD_OUT_HIGH, because reset is GPIO_ACTIVE_LOW */
+	aic32x4->rstn_gpio = devm_gpiod_get_optional(aic32x4->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(aic32x4->rstn_gpio)) {
+		return dev_err_probe(aic32x4->dev, PTR_ERR(aic32x4->rstn_gpio),
+				     "Failed to get reset gpio\n");
+	} else {
+		gpiod_set_consumer_name(aic32x4->rstn_gpio, "tlv320aic32x4_rstn");
+	}
 
 	if (of_property_read_u32_array(np, "aic32x4-gpio-func",
 				aic32x4_setup->gpio_func, 5) >= 0)
@@ -1372,26 +1378,20 @@ int aic32x4_probe(struct device *dev, struct regmap *regmap,
 		aic32x4->power_cfg = 0;
 		aic32x4->swapdacs = false;
 		aic32x4->micpga_routing = 0;
-		aic32x4->rstn_gpio = -1;
+		aic32x4->rstn_gpio = ERR_PTR(-ENOENT);
 		aic32x4->mclk_name = "mclk";
 	}
 
-	if (gpio_is_valid(aic32x4->rstn_gpio)) {
-		ret = devm_gpio_request_one(dev, aic32x4->rstn_gpio,
-				GPIOF_OUT_INIT_LOW, "tlv320aic32x4 rstn");
-		if (ret != 0)
-			return ret;
-	}
-
 	ret = aic32x4_setup_regulators(dev, aic32x4);
 	if (ret) {
 		dev_err(dev, "Failed to setup regulators\n");
 		return ret;
 	}
 
-	if (gpio_is_valid(aic32x4->rstn_gpio)) {
+	if (!IS_ERR(aic32x4->rstn_gpio)) {
 		ndelay(10);
-		gpio_set_value_cansleep(aic32x4->rstn_gpio, 1);
+		/* deassert reset */
+		gpiod_set_value_cansleep(aic32x4->rstn_gpio, 0);
 		mdelay(1);
 	}
 

-- 
2.37.1

Re: [PATCH 2/7] ASoC: codec: tlv320aic32x4: Convert to GPIO descriptors

[Linus Walleij]

On Tue, Apr 8, 2025 at 3:41 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 devm_gpiod_get_optional to get GPIO descriptor, and set consumer
>    name.
>  - Use gpiod_set_value to configure output value.
>
> reset pin is active low, so when request the gpio, set GPIOD_OUT_HIGH to
> assert reset and later deassert reset with value set to 0.
>
> While at here, reorder the included headers.
>
> Checking the DTS that use the device, all are using GPIOD_ACTIVE_LOW
> polarity for reset-gpios, so all should work as expected with this patch.
>
> Cc: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/7] ASoC: codec: tlv320aic32x4: Convert to GPIO descriptors

[Alexander Stein]

Hi,

Am Dienstag, 8. April 2025, 03:39:58 CEST schrieb Peng Fan (OSS):
> From: Peng Fan <peng.fan@nxp.com>
> 
> of_gpio.h is deprecated, update the driver to use GPIO descriptors.
>  - Use devm_gpiod_get_optional to get GPIO descriptor, and set consumer
>    name.
>  - Use gpiod_set_value to configure output value.
> 
> reset pin is active low, so when request the gpio, set GPIOD_OUT_HIGH to
> assert reset and later deassert reset with value set to 0.

IMHO it shouldn't matter if that pin is active-low or not. You want to
activate that reset, so GPIOD_OUT_HIGH is correct. If the GPIO is active-low
then nice, everything will be taken careof.

> While at here, reorder the included headers.
> 
> Checking the DTS that use the device, all are using GPIOD_ACTIVE_LOW
> polarity for reset-gpios, so all should work as expected with this patch.
> 
> Cc: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  sound/soc/codecs/tlv320aic32x4.c | 44 ++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
> index 7dbcf7f7130b04a27f58f20beb83eb3676c79c3d..1423186f5a6c181a20dd2dd552679d33174edaee 100644
> --- a/sound/soc/codecs/tlv320aic32x4.c
> +++ b/sound/soc/codecs/tlv320aic32x4.c
> @@ -9,27 +9,26 @@
>   * Based on sound/soc/codecs/wm8974 and TI driver for kernel 2.6.27.
>   */
>  
> -#include <linux/module.h>
> -#include <linux/moduleparam.h>
> -#include <linux/init.h>
> -#include <linux/delay.h>
> -#include <linux/pm.h>
> -#include <linux/gpio.h>
> -#include <linux/of_gpio.h>
>  #include <linux/cdev.h>
> -#include <linux/slab.h>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
>  #include <linux/of_clk.h>
> +#include <linux/pm.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
>  
> -#include <sound/tlv320aic32x4.h>
>  #include <sound/core.h>
> +#include <sound/initval.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
>  #include <sound/soc-dapm.h>
> -#include <sound/initval.h>
>  #include <sound/tlv.h>
> +#include <sound/tlv320aic32x4.h>
>  
>  #include "tlv320aic32x4.h"
>  
> @@ -38,7 +37,7 @@ struct aic32x4_priv {
>  	u32 power_cfg;
>  	u32 micpga_routing;
>  	bool swapdacs;
> -	int rstn_gpio;
> +	struct gpio_desc *rstn_gpio;
>  	const char *mclk_name;
>  
>  	struct regulator *supply_ldo;
> @@ -1236,7 +1235,14 @@ static int aic32x4_parse_dt(struct aic32x4_priv *aic32x4,
>  
>  	aic32x4->swapdacs = false;
>  	aic32x4->micpga_routing = 0;
> -	aic32x4->rstn_gpio = of_get_named_gpio(np, "reset-gpios", 0);
> +	/* Assert reset using GPIOD_OUT_HIGH, because reset is GPIO_ACTIVE_LOW */

As stated above this comment shouldn't be necessary, it might be even
confusing if there is some external inverter to the GPIO.

> +	aic32x4->rstn_gpio = devm_gpiod_get_optional(aic32x4->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(aic32x4->rstn_gpio)) {
> +		return dev_err_probe(aic32x4->dev, PTR_ERR(aic32x4->rstn_gpio),
> +				     "Failed to get reset gpio\n");
> +	} else {
> +		gpiod_set_consumer_name(aic32x4->rstn_gpio, "tlv320aic32x4_rstn");

Any reason to not set the consumer name to "tlv320aic32x4_reset"? I assume
the 'n' implies active-low. But this is something for the devicetree, not the driver.

Best regards,
Alexander

> +	}
>  
>  	if (of_property_read_u32_array(np, "aic32x4-gpio-func",
>  				aic32x4_setup->gpio_func, 5) >= 0)
> @@ -1372,26 +1378,20 @@ int aic32x4_probe(struct device *dev, struct regmap *regmap,
>  		aic32x4->power_cfg = 0;
>  		aic32x4->swapdacs = false;
>  		aic32x4->micpga_routing = 0;
> -		aic32x4->rstn_gpio = -1;
> +		aic32x4->rstn_gpio = ERR_PTR(-ENOENT);
>  		aic32x4->mclk_name = "mclk";
>  	}
>  
> -	if (gpio_is_valid(aic32x4->rstn_gpio)) {
> -		ret = devm_gpio_request_one(dev, aic32x4->rstn_gpio,
> -				GPIOF_OUT_INIT_LOW, "tlv320aic32x4 rstn");
> -		if (ret != 0)
> -			return ret;
> -	}
> -
>  	ret = aic32x4_setup_regulators(dev, aic32x4);
>  	if (ret) {
>  		dev_err(dev, "Failed to setup regulators\n");
>  		return ret;
>  	}
>  
> -	if (gpio_is_valid(aic32x4->rstn_gpio)) {
> +	if (!IS_ERR(aic32x4->rstn_gpio)) {
>  		ndelay(10);
> -		gpio_set_value_cansleep(aic32x4->rstn_gpio, 1);
> +		/* deassert reset */
> +		gpiod_set_value_cansleep(aic32x4->rstn_gpio, 0);
>  		mdelay(1);
>  	}
>  
> 
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

RE: [PATCH 2/7] ASoC: codec: tlv320aic32x4: Convert to GPIO descriptors

[Peng Fan]

> Subject: Re: [PATCH 2/7] ASoC: codec: tlv320aic32x4: Convert to GPIO
> descriptors
> 
> Hi,
> 
> Am Dienstag, 8. April 2025, 03:39:58 CEST schrieb Peng Fan (OSS):
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > of_gpio.h is deprecated, update the driver to use GPIO descriptors.
> >  - Use devm_gpiod_get_optional to get GPIO descriptor, and set
> consumer
> >    name.
> >  - Use gpiod_set_value to configure output value.
> >
> > reset pin is active low, so when request the gpio, set
> GPIOD_OUT_HIGH
> > to assert reset and later deassert reset with value set to 0.
> 
> IMHO it shouldn't matter if that pin is active-low or not. You want to
> activate that reset, so GPIOD_OUT_HIGH is correct. If the GPIO is
> active-low then nice, everything will be taken careof.
> 
> > While at here, reorder the included headers.
> >
> > Checking the DTS that use the device, all are using
> GPIOD_ACTIVE_LOW
> > polarity for reset-gpios, so all should work as expected with this patch.
> >
> > Cc: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  sound/soc/codecs/tlv320aic32x4.c | 44
> > ++++++++++++++++++++--------------------
> >  1 file changed, 22 insertions(+), 22 deletions(-)
> >
> > diff --git a/sound/soc/codecs/tlv320aic32x4.c
> > b/sound/soc/codecs/tlv320aic32x4.c
> > index
> >
> 7dbcf7f7130b04a27f58f20beb83eb3676c79c3d..1423186f5a6c181a2
> 0dd2dd55267
> > 9d33174edaee 100644
> > --- a/sound/soc/codecs/tlv320aic32x4.c
> > +++ b/sound/soc/codecs/tlv320aic32x4.c
> > @@ -9,27 +9,26 @@
> >   * Based on sound/soc/codecs/wm8974 and TI driver for kernel
> 2.6.27.
> >   */
> >
> > -#include <linux/module.h>
> > -#include <linux/moduleparam.h>
> > -#include <linux/init.h>
> > -#include <linux/delay.h>
> > -#include <linux/pm.h>
> > -#include <linux/gpio.h>
> > -#include <linux/of_gpio.h>
> >  #include <linux/cdev.h>
> > -#include <linux/slab.h>
> >  #include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> >  #include <linux/of_clk.h>
> > +#include <linux/pm.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> >
> > -#include <sound/tlv320aic32x4.h>
> >  #include <sound/core.h>
> > +#include <sound/initval.h>
> >  #include <sound/pcm.h>
> >  #include <sound/pcm_params.h>
> >  #include <sound/soc.h>
> >  #include <sound/soc-dapm.h>
> > -#include <sound/initval.h>
> >  #include <sound/tlv.h>
> > +#include <sound/tlv320aic32x4.h>
> >
> >  #include "tlv320aic32x4.h"
> >
> > @@ -38,7 +37,7 @@ struct aic32x4_priv {
> >  	u32 power_cfg;
> >  	u32 micpga_routing;
> >  	bool swapdacs;
> > -	int rstn_gpio;
> > +	struct gpio_desc *rstn_gpio;
> >  	const char *mclk_name;
> >
> >  	struct regulator *supply_ldo;
> > @@ -1236,7 +1235,14 @@ static int aic32x4_parse_dt(struct
> aic32x4_priv
> > *aic32x4,
> >
> >  	aic32x4->swapdacs = false;
> >  	aic32x4->micpga_routing = 0;
> > -	aic32x4->rstn_gpio = of_get_named_gpio(np, "reset-gpios", 0);
> > +	/* Assert reset using GPIOD_OUT_HIGH, because reset is
> > +GPIO_ACTIVE_LOW */
> 
> As stated above this comment shouldn't be necessary, it might be even
> confusing if there is some external inverter to the GPIO.

I just write down why use GPIOD_OUT_HIGH here.
of gpio API not take polarity into consideration, but gpiod API takes
polarity.

By adding external inverter, the reset for the codec is still low active,
but the gpio output from SoC should be high for the codec to stay
in reset state.

Then need a driver or whatever to describe the converter.

Here the reset-gpios is for codec.

> 
> > +	aic32x4->rstn_gpio = devm_gpiod_get_optional(aic32x4->dev,
> "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(aic32x4->rstn_gpio)) {
> > +		return dev_err_probe(aic32x4->dev, PTR_ERR(aic32x4-
> >rstn_gpio),
> > +				     "Failed to get reset gpio\n");
> > +	} else {
> > +		gpiod_set_consumer_name(aic32x4->rstn_gpio,
> "tlv320aic32x4_rstn");
> 
> Any reason to not set the consumer name to "tlv320aic32x4_reset"? I
> assume the 'n' implies active-low. But this is something for the
> devicetree, not the driver.

I not change the name, it is same as original driver
-		ret = devm_gpio_request_one(dev, aic32x4->rstn_gpio,
-				GPIOF_OUT_INIT_LOW, "tlv320aic32x4 rstn");

Regards,
Peng.

> 
> Best regards,
> Alexander
> 
> > +	}
> >
> >  	if (of_property_read_u32_array(np, "aic32x4-gpio-func",
> >  				aic32x4_setup->gpio_func, 5) >= 0)
> @@ -1372,26 +1378,20 @@ int
> > aic32x4_probe(struct device *dev, struct regmap *regmap,
> >  		aic32x4->power_cfg = 0;
> >  		aic32x4->swapdacs = false;
> >  		aic32x4->micpga_routing = 0;
> > -		aic32x4->rstn_gpio = -1;
> > +		aic32x4->rstn_gpio = ERR_PTR(-ENOENT);
> >  		aic32x4->mclk_name = "mclk";
> >  	}
> >
> > -	if (gpio_is_valid(aic32x4->rstn_gpio)) {
> > -		ret = devm_gpio_request_one(dev, aic32x4->rstn_gpio,
> > -				GPIOF_OUT_INIT_LOW,
> "tlv320aic32x4 rstn");
> > -		if (ret != 0)
> > -			return ret;
> > -	}
> > -
> >  	ret = aic32x4_setup_regulators(dev, aic32x4);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to setup regulators\n");
> >  		return ret;
> >  	}
> >
> > -	if (gpio_is_valid(aic32x4->rstn_gpio)) {
> > +	if (!IS_ERR(aic32x4->rstn_gpio)) {
> >  		ndelay(10);
> > -		gpio_set_value_cansleep(aic32x4->rstn_gpio, 1);
> > +		/* deassert reset */
> > +		gpiod_set_value_cansleep(aic32x4->rstn_gpio, 0);
> >  		mdelay(1);
> >  	}
> >
> >
> >
> 
> 
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld,
> Germany Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.tq-
> group.com%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7C85c2dca
> 8a42049d66eb808dd7c24eac6%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C638803220194936731%7CUnknown%7CTWFpbGZ
> sb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW
> 4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=5
> MgiAxA1ccqvepsjo1%2BPD%2Fv2dMiMNe%2F%2Fmy37xvSk5Wc%3D
> &reserved=0
> 

Re: [PATCH 2/7] ASoC: codec: tlv320aic32x4: Convert to GPIO descriptors

[Linus Walleij]

On Tue, Apr 15, 2025 at 3:53 PM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> > +     /* Assert reset using GPIOD_OUT_HIGH, because reset is GPIO_ACTIVE_LOW */
>
> As stated above this comment shouldn't be necessary, it might be even
> confusing if there is some external inverter to the GPIO.

I have added comments like this to many patches due to spurious comments
from developers who were not aware that gpiolib handles polarity inversion
and are confused when we set the value to "1" to reset an active low reset
line.

At some point I wanted to define something like

#define GPIO_ASSERTED 1
#define GPIO_UNASSERTED 0

And use these defines to make it absolutely clear what is going on...
But I didn't get around to.

Yours,
Linus Walleij

[PATCH 3/7] ASoC: codec: twl4030: 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 of_property_present to check "ti,hs_extmute_gpio" to set hs_extmute
 - if returned value is true.
 - Use devm_gpiod_get_optional to get GPIO descriptor, set consumer name.
 - Use gpiod_set_value to configure output value.

While at here
 - reorder the included headers.
 - drop remove hook after switching to use devm_gpiod_get_optional
 - Add return value for twl4030_init_chip to propagate value to parent
   in case defer probe happens

Checking the only user logicpd-som-lv.dtsi that uses polarity
GPIO_ACTIVE_HIGH, so all should work as expected.

Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 sound/soc/codecs/twl4030.c | 76 ++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 46 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 609886461805f85f826a002942bd07c9105f2038..2879b44eba41daf9a1877bc604b7bbfdbf476c47 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -5,18 +5,18 @@
  * Author:      Steve Sakoman, <steve@sakoman.com>
  */
 
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/init.h>
 #include <linux/delay.h>
-#include <linux/pm.h>
+#include <linux/init.h>
 #include <linux/i2c.h>
-#include <linux/platform_device.h>
-#include <linux/of.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/pm.h>
 #include <linux/mfd/twl.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -39,7 +39,7 @@ struct twl4030_board_params {
 	unsigned int ramp_delay_value;
 	unsigned int offset_cncl_path;
 	unsigned int hs_extmute:1;
-	int hs_extmute_gpio;
+	struct gpio_desc *hs_extmute_gpio;
 };
 
 /* codec private data */
@@ -213,8 +213,7 @@ twl4030_get_board_param_values(struct twl4030_board_params *board_params,
 	if (!of_property_read_u32(node, "ti,hs_extmute", &value))
 		board_params->hs_extmute = value;
 
-	board_params->hs_extmute_gpio = of_get_named_gpio(node, "ti,hs_extmute_gpio", 0);
-	if (gpio_is_valid(board_params->hs_extmute_gpio))
+	if (of_property_present(node, "ti,hs_extmute_gpio"))
 		board_params->hs_extmute = 1;
 }
 
@@ -242,7 +241,7 @@ twl4030_get_board_params(struct snd_soc_component *component)
 	return board_params;
 }
 
-static void twl4030_init_chip(struct snd_soc_component *component)
+static int twl4030_init_chip(struct snd_soc_component *component)
 {
 	struct twl4030_board_params *board_params;
 	struct twl4030_priv *twl4030 = snd_soc_component_get_drvdata(component);
@@ -252,24 +251,20 @@ static void twl4030_init_chip(struct snd_soc_component *component)
 	board_params = twl4030_get_board_params(component);
 
 	if (board_params && board_params->hs_extmute) {
-		if (gpio_is_valid(board_params->hs_extmute_gpio)) {
-			int ret;
-
-			if (!board_params->hs_extmute_gpio)
-				dev_warn(component->dev,
-					"Extmute GPIO is 0 is this correct?\n");
-
-			ret = gpio_request_one(board_params->hs_extmute_gpio,
-					       GPIOF_OUT_INIT_LOW,
-					       "hs_extmute");
-			if (ret) {
-				dev_err(component->dev,
-					"Failed to get hs_extmute GPIO\n");
-				board_params->hs_extmute_gpio = -1;
-			}
+		board_params->hs_extmute_gpio = devm_gpiod_get_optional(component->dev,
+									"ti,hs_extmute",
+									GPIOD_OUT_LOW);
+		if (IS_ERR(board_params->hs_extmute_gpio))
+			return dev_err_probe(component->dev, PTR_ERR(board_params->hs_extmute_gpio),
+					     "Failed to get hs_extmute GPIO\n");
+
+		if (board_params->hs_extmute_gpio) {
+			gpiod_set_consumer_name(board_params->hs_extmute_gpio, "hs_extmute");
 		} else {
 			u8 pin_mux;
 
+			dev_info(component->dev, "use TWL4030 GPIO6\n");
+
 			/* Set TWL4030 GPIO6 as EXTMUTE signal */
 			twl_i2c_read_u8(TWL4030_MODULE_INTBR, &pin_mux,
 					TWL4030_PMBR1_REG);
@@ -297,7 +292,7 @@ static void twl4030_init_chip(struct snd_soc_component *component)
 
 	/* Machine dependent setup */
 	if (!board_params)
-		return;
+		return 0;
 
 	twl4030->board_params = board_params;
 
@@ -332,6 +327,8 @@ static void twl4030_init_chip(struct snd_soc_component *component)
 		  TWL4030_CNCL_OFFSET_START));
 
 	twl4030_codec_enable(component, 0);
+
+	return 0;
 }
 
 static void twl4030_apll_enable(struct snd_soc_component *component, int enable)
@@ -714,8 +711,8 @@ static void headset_ramp(struct snd_soc_component *component, int ramp)
 	/* Enable external mute control, this dramatically reduces
 	 * the pop-noise */
 	if (board_params && board_params->hs_extmute) {
-		if (gpio_is_valid(board_params->hs_extmute_gpio)) {
-			gpio_set_value(board_params->hs_extmute_gpio, 1);
+		if (board_params->hs_extmute_gpio) {
+			gpiod_set_value(board_params->hs_extmute_gpio, 1);
 		} else {
 			hs_pop |= TWL4030_EXTMUTE;
 			twl4030_write(component, TWL4030_REG_HS_POPN_SET, hs_pop);
@@ -750,8 +747,8 @@ static void headset_ramp(struct snd_soc_component *component, int ramp)
 
 	/* Disable external mute */
 	if (board_params && board_params->hs_extmute) {
-		if (gpio_is_valid(board_params->hs_extmute_gpio)) {
-			gpio_set_value(board_params->hs_extmute_gpio, 0);
+		if (board_params->hs_extmute_gpio) {
+			gpiod_set_value(board_params->hs_extmute_gpio, 0);
 		} else {
 			hs_pop &= ~TWL4030_EXTMUTE;
 			twl4030_write(component, TWL4030_REG_HS_POPN_SET, hs_pop);
@@ -2168,24 +2165,11 @@ static int twl4030_soc_probe(struct snd_soc_component *component)
 	/* Set the defaults, and power up the codec */
 	twl4030->sysclk = twl4030_audio_get_mclk() / 1000;
 
-	twl4030_init_chip(component);
-
-	return 0;
-}
-
-static void twl4030_soc_remove(struct snd_soc_component *component)
-{
-	struct twl4030_priv *twl4030 = snd_soc_component_get_drvdata(component);
-	struct twl4030_board_params *board_params = twl4030->board_params;
-
-	if (board_params && board_params->hs_extmute &&
-	    gpio_is_valid(board_params->hs_extmute_gpio))
-		gpio_free(board_params->hs_extmute_gpio);
+	return twl4030_init_chip(component);
 }
 
 static const struct snd_soc_component_driver soc_component_dev_twl4030 = {
 	.probe			= twl4030_soc_probe,
-	.remove			= twl4030_soc_remove,
 	.read			= twl4030_read,
 	.write			= twl4030_write,
 	.set_bias_level		= twl4030_set_bias_level,

-- 
2.37.1

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

[Linus Walleij]

On Tue, Apr 8, 2025 at 3:41 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 of_property_present to check "ti,hs_extmute_gpio" to set hs_extmute
>  - if returned value is true.
>  - Use devm_gpiod_get_optional to get GPIO descriptor, set consumer name.
>  - Use gpiod_set_value to configure output value.
>
> While at here
>  - reorder the included headers.
>  - drop remove hook after switching to use devm_gpiod_get_optional
>  - Add return value for twl4030_init_chip to propagate value to parent
>    in case defer probe happens
>
> Checking the only user logicpd-som-lv.dtsi that uses polarity
> GPIO_ACTIVE_HIGH, so all should work as expected.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

RE: [PATCH 3/7] ASoC: codec: twl4030: Convert to GPIO descriptors

[Peng Fan]

Hi Linus,

> Subject: Re: [PATCH 3/7] ASoC: codec: twl4030: Convert to GPIO
> descriptors
> 
> On Tue, Apr 8, 2025 at 3:41 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 of_property_present to check "ti,hs_extmute_gpio" to set
> > hs_extmute
> >  - if returned value is true.
> >  - Use devm_gpiod_get_optional to get GPIO descriptor, set
> consumer name.
> >  - Use gpiod_set_value to configure output value.
> >
> > While at here
> >  - reorder the included headers.
> >  - drop remove hook after switching to use devm_gpiod_get_optional
> >  - Add return value for twl4030_init_chip to propagate value to
> parent
> >    in case defer probe happens
> >
> > Checking the only user logicpd-som-lv.dtsi that uses polarity
> > GPIO_ACTIVE_HIGH, so all should work as expected.
> >
> > Cc: Tony Lindgren <tony@atomide.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for reviewing.

Mark raised a comment that he would like to see
Separate the changes(e.g below) to a driver into
separate patches.
-Reorder headers
-Cleanup drivers
-Convert to GPIOD descriptors

So I would redo the patches, and please not
waste reviewing efforts on this version.

Thanks,
Peng.

> 
> Yours,
> Linus Walleij

[PATCH 4/7] ASoC: codec: cs42l56: 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 devm_gpiod_get_optional to get GPIO descriptor with default
   polarity GPIOD_OUT_LOW, set consumer name.
 - Use gpiod_set_value_cansleep to configure output value.

While at here
 - reorder the included headers.
 - Move cs42l56_platform_data from sound/cs42l56.h to driver code
 - Drop sound/cs42l56.h because no user is creating the device using
   platform data

Checking the current driver using legacy GPIO API, the
nreset value is first output HIGH, then LOW, then HIGH.

Checking the datasheet, nreset is should be held low after power
on, when nreset is high, it starts to work.

Since the driver has been here for quite long time and no complain on
the nreset flow, still follow original flow when using GPIOD
descriptors.

Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding
example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is
no in-tree DTS has the device, so all should be fine.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 include/sound/cs42l56.h    | 45 -----------------------
 sound/soc/codecs/cs42l56.c | 91 +++++++++++++++++++++++++++++-----------------
 2 files changed, 58 insertions(+), 78 deletions(-)

diff --git a/include/sound/cs42l56.h b/include/sound/cs42l56.h
deleted file mode 100644
index 62e9f7a3b414f6d1bcb651b22f7f8bd1f29b0eb3..0000000000000000000000000000000000000000
--- a/include/sound/cs42l56.h
+++ /dev/null
@@ -1,45 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * linux/sound/cs42l56.h -- Platform data for CS42L56
- *
- * Copyright (c) 2014 Cirrus Logic Inc.
- */
-
-#ifndef __CS42L56_H
-#define __CS42L56_H
-
-struct cs42l56_platform_data {
-
-	/* GPIO for Reset */
-	unsigned int gpio_nreset;
-
-	/* MICBIAS Level. Check datasheet Pg48 */
-	unsigned int micbias_lvl;
-
-	/* Analog Input 1A Reference 0=Single 1=Pseudo-Differential */
-	unsigned int ain1a_ref_cfg;
-
-	/* Analog Input 2A Reference 0=Single 1=Pseudo-Differential */
-	unsigned int ain2a_ref_cfg;
-
-	/* Analog Input 1B Reference 0=Single 1=Pseudo-Differential */
-	unsigned int ain1b_ref_cfg;
-
-	/* Analog Input 2B Reference 0=Single 1=Pseudo-Differential */
-	unsigned int ain2b_ref_cfg;
-
-	/* Charge Pump Freq. Check datasheet Pg62 */
-	unsigned int chgfreq;
-
-	/* HighPass Filter Right Channel Corner Frequency */
-	unsigned int hpfb_freq;
-
-	/* HighPass Filter Left Channel Corner Frequency */
-	unsigned int hpfa_freq;
-
-	/* Adaptive Power Control for LO/HP */
-	unsigned int adaptive_pwr;
-
-};
-
-#endif /* __CS42L56_H */
diff --git a/sound/soc/codecs/cs42l56.c b/sound/soc/codecs/cs42l56.c
index aaf90c8b7339dc7d9fa469048a56f38dca1797cd..8223e22dd1fea68e746151e637b611100f4e1a6e 100644
--- a/sound/soc/codecs/cs42l56.c
+++ b/sound/soc/codecs/cs42l56.c
@@ -7,32 +7,66 @@
  * Author: Brian Austin <brian.austin@cirrus.com>
  */
 
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/pm.h>
-#include <linux/i2c.h>
-#include <linux/input.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
-#include <linux/platform_device.h>
-#include <linux/regulator/consumer.h>
-#include <linux/of.h>
-#include <linux/of_gpio.h>
 #include <sound/core.h>
+#include <sound/initval.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/soc-dapm.h>
-#include <sound/initval.h>
 #include <sound/tlv.h>
-#include <sound/cs42l56.h>
 #include "cs42l56.h"
 
 #define CS42L56_NUM_SUPPLIES 3
+
+struct cs42l56_platform_data {
+
+	/* GPIO for Reset */
+	struct gpio_desc *gpio_nreset;
+
+	/* MICBIAS Level. Check datasheet Pg48 */
+	unsigned int micbias_lvl;
+
+	/* Analog Input 1A Reference 0=Single 1=Pseudo-Differential */
+	unsigned int ain1a_ref_cfg;
+
+	/* Analog Input 2A Reference 0=Single 1=Pseudo-Differential */
+	unsigned int ain2a_ref_cfg;
+
+	/* Analog Input 1B Reference 0=Single 1=Pseudo-Differential */
+	unsigned int ain1b_ref_cfg;
+
+	/* Analog Input 2B Reference 0=Single 1=Pseudo-Differential */
+	unsigned int ain2b_ref_cfg;
+
+	/* Charge Pump Freq. Check datasheet Pg62 */
+	unsigned int chgfreq;
+
+	/* HighPass Filter Right Channel Corner Frequency */
+	unsigned int hpfb_freq;
+
+	/* HighPass Filter Left Channel Corner Frequency */
+	unsigned int hpfa_freq;
+
+	/* Adaptive Power Control for LO/HP */
+	unsigned int adaptive_pwr;
+
+};
+
 static const char *const cs42l56_supply_names[CS42L56_NUM_SUPPLIES] = {
 	"VA",
 	"VCP",
@@ -1161,7 +1195,13 @@ static int cs42l56_handle_of_data(struct i2c_client *i2c_client,
 	if (of_property_read_u32(np, "cirrus,hpf-left-freq", &val32) >= 0)
 		pdata->hpfb_freq = val32;
 
-	pdata->gpio_nreset = of_get_named_gpio(np, "cirrus,gpio-nreset", 0);
+	pdata->gpio_nreset = devm_gpiod_get_optional(&i2c_client->dev, "cirrus,gpio-nreset",
+						     GPIOD_OUT_LOW);
+
+	if (IS_ERR(pdata->gpio_nreset))
+		return PTR_ERR(pdata->gpio_nreset);
+
+	gpiod_set_consumer_name(pdata->gpio_nreset, "CS42L56 /RST");
 
 	return 0;
 }
@@ -1169,8 +1209,6 @@ static int cs42l56_handle_of_data(struct i2c_client *i2c_client,
 static int cs42l56_i2c_probe(struct i2c_client *i2c_client)
 {
 	struct cs42l56_private *cs42l56;
-	struct cs42l56_platform_data *pdata =
-		dev_get_platdata(&i2c_client->dev);
 	int ret, i;
 	unsigned int devid;
 	unsigned int alpha_rev, metal_rev;
@@ -1188,28 +1226,15 @@ static int cs42l56_i2c_probe(struct i2c_client *i2c_client)
 		return ret;
 	}
 
-	if (pdata) {
-		cs42l56->pdata = *pdata;
-	} else {
-		if (i2c_client->dev.of_node) {
-			ret = cs42l56_handle_of_data(i2c_client,
-						     &cs42l56->pdata);
-			if (ret != 0)
-				return ret;
-		}
+	if (i2c_client->dev.of_node) {
+		ret = cs42l56_handle_of_data(i2c_client, &cs42l56->pdata);
+		if (ret != 0)
+			return ret;
 	}
 
 	if (cs42l56->pdata.gpio_nreset) {
-		ret = gpio_request_one(cs42l56->pdata.gpio_nreset,
-				       GPIOF_OUT_INIT_HIGH, "CS42L56 /RST");
-		if (ret < 0) {
-			dev_err(&i2c_client->dev,
-				"Failed to request /RST %d: %d\n",
-				cs42l56->pdata.gpio_nreset, ret);
-			return ret;
-		}
-		gpio_set_value_cansleep(cs42l56->pdata.gpio_nreset, 0);
-		gpio_set_value_cansleep(cs42l56->pdata.gpio_nreset, 1);
+		gpiod_set_value_cansleep(cs42l56->pdata.gpio_nreset, 1);
+		gpiod_set_value_cansleep(cs42l56->pdata.gpio_nreset, 0);
 	}
 
 

-- 
2.37.1

Re: [PATCH 4/7] ASoC: codec: cs42l56: Convert to GPIO descriptors

[Charles Keepax]

On Tue, Apr 08, 2025 at 09:40:00AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> Checking the current driver using legacy GPIO API, the
> nreset value is first output HIGH, then LOW, then HIGH.
> 
> Checking the datasheet, nreset is should be held low after power
> on, when nreset is high, it starts to work.
> 

Does feel like it would have made more sense to request it in
reset at the start certainly, but as you say reasonable to leave
well enough alone.

> Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding
> example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is
> no in-tree DTS has the device, so all should be fine.

Yeah it is technically wrong, discussed more below.

> -	pdata->gpio_nreset = of_get_named_gpio(np, "cirrus,gpio-nreset", 0);
> +	pdata->gpio_nreset = devm_gpiod_get_optional(&i2c_client->dev, "cirrus,gpio-nreset",
> +						     GPIOD_OUT_LOW);

Would be nice to call out that this part is already included in
the quirks array in of_find_gpio_rename:

944004eb56dc ("gpiolib: of: add a quirk for reset line for Cirrus CS42L56")

Took me a while to realise this would request the right property.

> -		gpio_set_value_cansleep(cs42l56->pdata.gpio_nreset, 0);
> -		gpio_set_value_cansleep(cs42l56->pdata.gpio_nreset, 1);
> +		gpiod_set_value_cansleep(cs42l56->pdata.gpio_nreset, 1);
> +		gpiod_set_value_cansleep(cs42l56->pdata.gpio_nreset, 0);

I can't say I super love this change as it will mean any users
with a DT that worked with the driver before this change will see
things break. As far as I know the parts you are updating in
this series do not have a lot of users, (and none in tree as you
note) so I guess if everyone else is happy, I don't really object.

Thanks,
Charles

Re: [PATCH 4/7] ASoC: codec: cs42l56: Convert to GPIO descriptors

[Peng Fan]

On Tue, Apr 08, 2025 at 01:53:15PM +0100, Charles Keepax wrote:
>On Tue, Apr 08, 2025 at 09:40:00AM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> Checking the current driver using legacy GPIO API, the
>> nreset value is first output HIGH, then LOW, then HIGH.
>> 
>> Checking the datasheet, nreset is should be held low after power
>> on, when nreset is high, it starts to work.
>> 
>
>Does feel like it would have made more sense to request it in
>reset at the start certainly, but as you say reasonable to leave
>well enough alone.

yeah. request it in reset state and set HIGH later is better.

I could update to use this new flow.

>
>> Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding
>> example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is
>> no in-tree DTS has the device, so all should be fine.
>
>Yeah it is technically wrong, discussed more below.
>
>> -	pdata->gpio_nreset = of_get_named_gpio(np, "cirrus,gpio-nreset", 0);
>> +	pdata->gpio_nreset = devm_gpiod_get_optional(&i2c_client->dev, "cirrus,gpio-nreset",
>> +						     GPIOD_OUT_LOW);
>
>Would be nice to call out that this part is already included in
>the quirks array in of_find_gpio_rename:
>
>944004eb56dc ("gpiolib: of: add a quirk for reset line for Cirrus CS42L56")

I will update commit log to include this.

>
>Took me a while to realise this would request the right property.

My bad.

>
>> -		gpio_set_value_cansleep(cs42l56->pdata.gpio_nreset, 0);
>> -		gpio_set_value_cansleep(cs42l56->pdata.gpio_nreset, 1);
>> +		gpiod_set_value_cansleep(cs42l56->pdata.gpio_nreset, 1);
>> +		gpiod_set_value_cansleep(cs42l56->pdata.gpio_nreset, 0);
>
>I can't say I super love this change as it will mean any users
>with a DT that worked with the driver before this change will see
>things break. As far as I know the parts you are updating in
>this series do not have a lot of users, (and none in tree as you
>note) so I guess if everyone else is happy, I don't really object.

A polarity quirk could be added to gpiolib-of, if this is preferred.
Before adding quirk, I would like to see whether Linus and Bartosz agree
on this.

BTW, [1] shows the chip is discontinued. Since there is no in-tree user
for quite some time, and new users would not use end-of-life chips,
should we totally delete this driver?

[1] https://www.cirrus.com/products/cs42l56/
  

Thanks,
Peng

>
>Thanks,
>Charles
>

Re: [PATCH 4/7] ASoC: codec: cs42l56: Convert to GPIO descriptors

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On Tue, Apr 08, 2025 at 09:40:00AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>

> of_gpio.h is deprecated, update the driver to use GPIO descriptors.
>  - Use devm_gpiod_get_optional to get GPIO descriptor with default
>    polarity GPIOD_OUT_LOW, set consumer name.
>  - Use gpiod_set_value_cansleep to configure output value.

> While at here
>  - reorder the included headers.
>  - Move cs42l56_platform_data from sound/cs42l56.h to driver code
>  - Drop sound/cs42l56.h because no user is creating the device using
>    platform data

This is a good sign that things should be split into multiple patches.
The series would probably be a little more manageable in general if it
were done per driver.

> Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding
> example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is
> no in-tree DTS has the device, so all should be fine.

This is the whole thing where gpiolib introducing inversion causing
confusion.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/7] ASoC: codec: cs42l56: Convert to GPIO descriptors

[Peng Fan]

On Tue, Apr 08, 2025 at 03:24:35PM +0100, Mark Brown wrote:
>On Tue, Apr 08, 2025 at 09:40:00AM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>
>> of_gpio.h is deprecated, update the driver to use GPIO descriptors.
>>  - Use devm_gpiod_get_optional to get GPIO descriptor with default
>>    polarity GPIOD_OUT_LOW, set consumer name.
>>  - Use gpiod_set_value_cansleep to configure output value.
>
>> While at here
>>  - reorder the included headers.
>>  - Move cs42l56_platform_data from sound/cs42l56.h to driver code
>>  - Drop sound/cs42l56.h because no user is creating the device using
>>    platform data
>
>This is a good sign that things should be split into multiple patches.
>The series would probably be a little more manageable in general if it
>were done per driver.

Sure. Just drop this patchset, I will do the patches per driver and
split the changes to a single driver into multiple small patches.

>
>> Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding
>> example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is
>> no in-tree DTS has the device, so all should be fine.
>
>This is the whole thing where gpiolib introducing inversion causing
>confusion.

Let's see whether Linus and Bartosz prefer a polarity quirk for this.

But honestly, I am thinking if might be better to remove the drivers
that for end-of-life chips and no users for quite long time(years?).

Thanks,
Peng

Re: [PATCH 4/7] ASoC: codec: cs42l56: Convert to GPIO descriptors

[Charles Keepax]

On Wed, Apr 09, 2025 at 12:04:08AM +0800, Peng Fan wrote:
> On Tue, Apr 08, 2025 at 03:24:35PM +0100, Mark Brown wrote:
> >On Tue, Apr 08, 2025 at 09:40:00AM +0800, Peng Fan (OSS) wrote:
> >> From: Peng Fan <peng.fan@nxp.com>
> >> Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding
> >> example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is
> >> no in-tree DTS has the device, so all should be fine.
> >
> >This is the whole thing where gpiolib introducing inversion causing
> >confusion.

Indeed it comes up pretty often, even worse with ACPI, at least
in DT we can generally make new things use the flag.

> Let's see whether Linus and Bartosz prefer a polarity quirk for this.

Yeah happy to wait see what Linus and Bartosz think, as noted I
am ok either way really, just mostly wanted to make sure we were
making a concious choice.

> But honestly, I am thinking if might be better to remove the drivers
> that for end-of-life chips and no users for quite long time(years?).

As for removing the drivers, I don't have a great objection to
that they are all end of life products with no in tree users. But
on the flip side the cost of keeping them seems fairly low here.
Personally, I would be inclined to keep the driver and just leave
the polarity stuff as is. That should mean any out of tree users
remain in what ever state they are currently in and if a user
turns up wanting it one way or the other then an in tree user
probably wins.

Thanks,
Charles

[PATCH 5/7] ASoC: codec: cs42l73: 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 devm_gpiod_get_optional to get GPIO descriptor with default
   polarity GPIOD_OUT_LOW, set consumer name.
 - Use gpiod_set_value_cansleep to configure output value.

While at here
 - reorder the included headers.
 - Move cs42l73_platform_data from sound/cs42l56.h to driver code
 - Drop sound/cs42l73.h because no user is creating the device using
   platform data

Checking the current driver using legacy GPIO API, the
reset value is first output HIGH, then LOW, then HIGH.

Checking the datasheet, Hold RESET LOW (active) until all the power
supply rails have risen to greater than or equal to the minimum
recommended operating voltages.

Since the driver has been here for quite long time and no complain on
the reset flow, still follow original flow when using GPIOD
descriptors.

Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding
example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is
no in-tree DTS has the device, so all should be fine.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 include/sound/cs42l73.h    | 19 -----------
 sound/soc/codecs/cs42l73.c | 81 +++++++++++++++++++++-------------------------
 2 files changed, 37 insertions(+), 63 deletions(-)

diff --git a/include/sound/cs42l73.h b/include/sound/cs42l73.h
deleted file mode 100644
index 5a93393b6124f746bfb7bf5076e4bd1f458019d2..0000000000000000000000000000000000000000
--- a/include/sound/cs42l73.h
+++ /dev/null
@@ -1,19 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * linux/sound/cs42l73.h -- Platform data for CS42L73
- *
- * Copyright (c) 2012 Cirrus Logic Inc.
- */
-
-#ifndef __CS42L73_H
-#define __CS42L73_H
-
-struct cs42l73_platform_data {
-	/* RST GPIO */
-	unsigned int reset_gpio;
-	unsigned int chgfreq;
-	int jack_detection;
-	unsigned int mclk_freq;
-};
-
-#endif /* __CS42L73_H */
diff --git a/sound/soc/codecs/cs42l73.c b/sound/soc/codecs/cs42l73.c
index ddf36001100eef29f74f4d99420511f620f1948d..73a980ed2cefce2eaacdce0b758be433e632019c 100644
--- a/sound/soc/codecs/cs42l73.c
+++ b/sound/soc/codecs/cs42l73.c
@@ -8,24 +8,23 @@
  *	    Brian Austin, Cirrus Logic Inc, <brian.austin@cirrus.com>
  */
 
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/delay.h>
-#include <linux/of_gpio.h>
 #include <linux/pm.h>
-#include <linux/i2c.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <sound/core.h>
+#include <sound/initval.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/soc-dapm.h>
-#include <sound/initval.h>
 #include <sound/tlv.h>
-#include <sound/cs42l73.h>
 #include "cs42l73.h"
 #include "cirrus_legacy.h"
 
@@ -33,6 +32,15 @@ struct sp_config {
 	u8 spc, mmcc, spfs;
 	u32 srate;
 };
+
+struct cs42l73_platform_data {
+	/* RST GPIO */
+	struct gpio_desc *reset_gpio;
+	unsigned int chgfreq;
+	int jack_detection;
+	unsigned int mclk_freq;
+};
+
 struct  cs42l73_private {
 	struct cs42l73_platform_data pdata;
 	struct sp_config config[3];
@@ -1276,7 +1284,7 @@ static const struct regmap_config cs42l73_regmap = {
 static int cs42l73_i2c_probe(struct i2c_client *i2c_client)
 {
 	struct cs42l73_private *cs42l73;
-	struct cs42l73_platform_data *pdata = dev_get_platdata(&i2c_client->dev);
+	struct cs42l73_platform_data *pdata;
 	int ret, devid;
 	unsigned int reg;
 	u32 val32;
@@ -1292,38 +1300,31 @@ static int cs42l73_i2c_probe(struct i2c_client *i2c_client)
 		return ret;
 	}
 
-	if (pdata) {
-		cs42l73->pdata = *pdata;
-	} else {
-		pdata = devm_kzalloc(&i2c_client->dev, sizeof(*pdata),
-				     GFP_KERNEL);
-		if (!pdata)
-			return -ENOMEM;
-
-		if (i2c_client->dev.of_node) {
-			if (of_property_read_u32(i2c_client->dev.of_node,
-				"chgfreq", &val32) >= 0)
-				pdata->chgfreq = val32;
-		}
-		pdata->reset_gpio = of_get_named_gpio(i2c_client->dev.of_node,
-						"reset-gpio", 0);
-		cs42l73->pdata = *pdata;
+	pdata = devm_kzalloc(&i2c_client->dev, sizeof(*pdata),
+			     GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	if (i2c_client->dev.of_node) {
+		if (of_property_read_u32(i2c_client->dev.of_node,
+			"chgfreq", &val32) >= 0)
+			pdata->chgfreq = val32;
 	}
 
+	pdata->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev, "reset", GPIOD_OUT_LOW);
+
+	if (IS_ERR(pdata->reset_gpio))
+		return PTR_ERR(pdata->reset_gpio);
+
+	gpiod_set_consumer_name(pdata->reset_gpio, "CS42L73 /RST");
+
+	cs42l73->pdata = *pdata;
+
 	i2c_set_clientdata(i2c_client, cs42l73);
 
 	if (cs42l73->pdata.reset_gpio) {
-		ret = devm_gpio_request_one(&i2c_client->dev,
-					    cs42l73->pdata.reset_gpio,
-					    GPIOF_OUT_INIT_HIGH,
-					    "CS42L73 /RST");
-		if (ret < 0) {
-			dev_err(&i2c_client->dev, "Failed to request /RST %d: %d\n",
-				cs42l73->pdata.reset_gpio, ret);
-			return ret;
-		}
-		gpio_set_value_cansleep(cs42l73->pdata.reset_gpio, 0);
-		gpio_set_value_cansleep(cs42l73->pdata.reset_gpio, 1);
+		gpiod_set_value_cansleep(cs42l73->pdata.reset_gpio, 1);
+		gpiod_set_value_cansleep(cs42l73->pdata.reset_gpio, 0);
 	}
 
 	/* initialize codec */
@@ -1360,7 +1361,7 @@ static int cs42l73_i2c_probe(struct i2c_client *i2c_client)
 	return 0;
 
 err_reset:
-	gpio_set_value_cansleep(cs42l73->pdata.reset_gpio, 0);
+	gpiod_set_value_cansleep(cs42l73->pdata.reset_gpio, 1);
 
 	return ret;
 }
@@ -1371,19 +1372,11 @@ static const struct of_device_id cs42l73_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, cs42l73_of_match);
 
-static const struct i2c_device_id cs42l73_id[] = {
-	{"cs42l73"},
-	{}
-};
-
-MODULE_DEVICE_TABLE(i2c, cs42l73_id);
-
 static struct i2c_driver cs42l73_i2c_driver = {
 	.driver = {
 		   .name = "cs42l73",
 		   .of_match_table = cs42l73_of_match,
 		   },
-	.id_table = cs42l73_id,
 	.probe = cs42l73_i2c_probe,
 
 };

-- 
2.37.1

[PATCH 6/7] ASoC: codec: cs42l52: 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 devm_gpiod_get_optional to get GPIO descriptor with default
   polarity GPIOD_OUT_LOW, set consumer name.
 - Use gpiod_set_value_cansleep to configure output value.

While at here
 - reorder the included headers.
 - Move cs42l52_platform_data from sound/cs42l52.h to driver code
 - Drop sound/cs42l52.h because no user is creating the device using
   platform data

Checking the current driver using legacy GPIO API, the
reset value is first output HIGH, then LOW, then HIGH.

Checking the datasheet, the device remains in Power-down state until
RESET pin is brought high.

Since the driver has been here for quite long time and no complain on
the reset flow, still follow original flow when using GPIOD
descriptors.

Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding
example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong.
And the binding use reset-gpio as example, not same as driver using
"cirrus,reset-gpio", and there is no in-tree DTS has the device,
so all should be fine with this patch.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 include/sound/cs42l52.h    |  29 ------------
 sound/soc/codecs/cs42l52.c | 108 +++++++++++++++++++++++----------------------
 2 files changed, 56 insertions(+), 81 deletions(-)

diff --git a/include/sound/cs42l52.h b/include/sound/cs42l52.h
deleted file mode 100644
index c20649666abe5dcbbf628f6c2d1692d3e7190eeb..0000000000000000000000000000000000000000
--- a/include/sound/cs42l52.h
+++ /dev/null
@@ -1,29 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * linux/sound/cs42l52.h -- Platform data for CS42L52
- *
- * Copyright (c) 2012 Cirrus Logic Inc.
- */
-
-#ifndef __CS42L52_H
-#define __CS42L52_H
-
-struct cs42l52_platform_data {
-
-	/* MICBIAS Level. Check datasheet Pg48 */
-	unsigned int micbias_lvl;
-
-	/* MICA mode selection Differential or Single-ended */
-	bool mica_diff_cfg;
-
-	/* MICB mode selection Differential or Single-ended */
-	bool micb_diff_cfg;
-
-	/* Charge Pump Freq. Check datasheet Pg73 */
-	unsigned int chgfreq;
-
-	/* Reset GPIO */
-	unsigned int reset_gpio;
-};
-
-#endif /* __CS42L52_H */
diff --git a/sound/soc/codecs/cs42l52.c b/sound/soc/codecs/cs42l52.c
index cd3f92c19045ad32f1f0f0f1764390640d68eb10..e56faf6e45c2bd4303350ed088fb28f2b9d9c1e2 100644
--- a/sound/soc/codecs/cs42l52.c
+++ b/sound/soc/codecs/cs42l52.c
@@ -8,27 +8,26 @@
  * Author: Brian Austin <brian.austin@cirrus.com>
  */
 
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/kernel.h>
-#include <linux/init.h>
 #include <linux/delay.h>
-#include <linux/of_gpio.h>
-#include <linux/pm.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
+#include <linux/init.h>
 #include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
-#include <linux/platform_device.h>
 #include <sound/core.h>
+#include <sound/initval.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/soc-dapm.h>
-#include <sound/initval.h>
 #include <sound/tlv.h>
-#include <sound/cs42l52.h>
 #include "cs42l52.h"
 
 struct sp_config {
@@ -36,6 +35,24 @@ struct sp_config {
 	u32 srate;
 };
 
+struct cs42l52_platform_data {
+
+	/* MICBIAS Level. Check datasheet Pg48 */
+	unsigned int micbias_lvl;
+
+	/* MICA mode selection Differential or Single-ended */
+	bool mica_diff_cfg;
+
+	/* MICB mode selection Differential or Single-ended */
+	bool micb_diff_cfg;
+
+	/* Charge Pump Freq. Check datasheet Pg73 */
+	unsigned int chgfreq;
+
+	/* Reset GPIO */
+	struct gpio_desc *reset_gpio;
+};
+
 struct  cs42l52_private {
 	struct regmap *regmap;
 	struct snd_soc_component *component;
@@ -1090,7 +1107,7 @@ static const struct regmap_config cs42l52_regmap = {
 static int cs42l52_i2c_probe(struct i2c_client *i2c_client)
 {
 	struct cs42l52_private *cs42l52;
-	struct cs42l52_platform_data *pdata = dev_get_platdata(&i2c_client->dev);
+	struct cs42l52_platform_data *pdata;
 	int ret;
 	unsigned int devid;
 	unsigned int reg;
@@ -1107,50 +1124,44 @@ static int cs42l52_i2c_probe(struct i2c_client *i2c_client)
 		dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret);
 		return ret;
 	}
-	if (pdata) {
-		cs42l52->pdata = *pdata;
-	} else {
-		pdata = devm_kzalloc(&i2c_client->dev, sizeof(*pdata),
+
+	pdata = devm_kzalloc(&i2c_client->dev, sizeof(*pdata),
 				     GFP_KERNEL);
-		if (!pdata)
-			return -ENOMEM;
+	if (!pdata)
+		return -ENOMEM;
 
-		if (i2c_client->dev.of_node) {
-			if (of_property_read_bool(i2c_client->dev.of_node,
-				"cirrus,mica-differential-cfg"))
-				pdata->mica_diff_cfg = true;
+	if (i2c_client->dev.of_node) {
+		if (of_property_read_bool(i2c_client->dev.of_node,
+			"cirrus,mica-differential-cfg"))
+			pdata->mica_diff_cfg = true;
 
-			if (of_property_read_bool(i2c_client->dev.of_node,
-				"cirrus,micb-differential-cfg"))
-				pdata->micb_diff_cfg = true;
+		if (of_property_read_bool(i2c_client->dev.of_node,
+			"cirrus,micb-differential-cfg"))
+			pdata->micb_diff_cfg = true;
 
-			if (of_property_read_u32(i2c_client->dev.of_node,
-				"cirrus,micbias-lvl", &val32) >= 0)
-				pdata->micbias_lvl = val32;
+		if (of_property_read_u32(i2c_client->dev.of_node,
+			"cirrus,micbias-lvl", &val32) >= 0)
+			pdata->micbias_lvl = val32;
 
-			if (of_property_read_u32(i2c_client->dev.of_node,
-				"cirrus,chgfreq-divisor", &val32) >= 0)
-				pdata->chgfreq = val32;
+		if (of_property_read_u32(i2c_client->dev.of_node,
+			"cirrus,chgfreq-divisor", &val32) >= 0)
+			pdata->chgfreq = val32;
+
+		pdata->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev,
+							    "cirrus,reset",
+							    GPIOD_OUT_LOW);
+
+		if (IS_ERR(pdata->reset_gpio))
+			return PTR_ERR(pdata->reset_gpio);
+
+		gpiod_set_consumer_name(pdata->reset_gpio, "CS42L52 /RST");
 
-			pdata->reset_gpio =
-				of_get_named_gpio(i2c_client->dev.of_node,
-						"cirrus,reset-gpio", 0);
-		}
-		cs42l52->pdata = *pdata;
 	}
+		cs42l52->pdata = *pdata;
 
 	if (cs42l52->pdata.reset_gpio) {
-		ret = devm_gpio_request_one(&i2c_client->dev,
-					    cs42l52->pdata.reset_gpio,
-					    GPIOF_OUT_INIT_HIGH,
-					    "CS42L52 /RST");
-		if (ret < 0) {
-			dev_err(&i2c_client->dev, "Failed to request /RST %d: %d\n",
-				cs42l52->pdata.reset_gpio, ret);
-			return ret;
-		}
-		gpio_set_value_cansleep(cs42l52->pdata.reset_gpio, 0);
-		gpio_set_value_cansleep(cs42l52->pdata.reset_gpio, 1);
+		gpiod_set_value_cansleep(cs42l52->pdata.reset_gpio, 1);
+		gpiod_set_value_cansleep(cs42l52->pdata.reset_gpio, 0);
 	}
 
 	i2c_set_clientdata(i2c_client, cs42l52);
@@ -1214,18 +1225,11 @@ static const struct of_device_id cs42l52_of_match[] = {
 MODULE_DEVICE_TABLE(of, cs42l52_of_match);
 
 
-static const struct i2c_device_id cs42l52_id[] = {
-	{ "cs42l52" },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, cs42l52_id);
-
 static struct i2c_driver cs42l52_i2c_driver = {
 	.driver = {
 		.name = "cs42l52",
 		.of_match_table = cs42l52_of_match,
 	},
-	.id_table = cs42l52_id,
 	.probe = cs42l52_i2c_probe,
 };
 

-- 
2.37.1

[PATCH 7/7] ASoC: codec: tpa6130a2: 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 devm_gpiod_get_optional to get GPIO descriptor with default
   polarity GPIOD_OUT_LOW, set consumer name.
 - Use gpiod_set_value to configure output value.

While at here
 - reorder the included headers.
 - Drop sound/tpa6130a2-plat.h because no user is creating the device using
   platform data

Checking the DTS polarity, all users are using GPIOD_ACTIVE_HIGH.
so all should work as expected with this patch.

Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 MAINTAINERS                    |  1 -
 include/sound/tpa6130a2-plat.h | 17 -------------
 sound/soc/codecs/tpa6130a2.c   | 54 +++++++++++++-----------------------------
 3 files changed, 16 insertions(+), 56 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index de65f37966ef0accc3497e5f75eaf94399944a90..2a1a91e4707740edb59ce6712b66ed7196050720 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23881,7 +23881,6 @@ F:	Documentation/devicetree/bindings/sound/ti,tlv320*.yaml
 F:	Documentation/devicetree/bindings/sound/ti,tlv320adcx140.yaml
 F:	include/sound/tas2*.h
 F:	include/sound/tlv320*.h
-F:	include/sound/tpa6130a2-plat.h
 F:	sound/pci/hda/tas2781_hda_i2c.c
 F:	sound/soc/codecs/pcm1681.c
 F:	sound/soc/codecs/pcm1789*.*
diff --git a/include/sound/tpa6130a2-plat.h b/include/sound/tpa6130a2-plat.h
deleted file mode 100644
index a60930e36e93958c674e8e1f3ff0b39cd0be7677..0000000000000000000000000000000000000000
--- a/include/sound/tpa6130a2-plat.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * TPA6130A2 driver platform header
- *
- * Copyright (C) Nokia Corporation
- *
- * Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
- */
-
-#ifndef TPA6130A2_PLAT_H
-#define TPA6130A2_PLAT_H
-
-struct tpa6130a2_platform_data {
-	int power_gpio;
-};
-
-#endif
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c
index b5472fa1bddab3d69b88c040ed561e8b5d9d1d0d..62c4cc7941114623fb33bd7d6727495b3d64a3ae 100644
--- a/sound/soc/codecs/tpa6130a2.c
+++ b/sound/soc/codecs/tpa6130a2.c
@@ -7,19 +7,17 @@
  * Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
  */
 
-#include <linux/module.h>
-#include <linux/errno.h>
 #include <linux/device.h>
+#include <linux/errno.h>
 #include <linux/i2c.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
-#include <sound/tpa6130a2-plat.h>
 #include <sound/soc.h>
 #include <sound/tlv.h>
-#include <linux/of.h>
-#include <linux/of_gpio.h>
-#include <linux/regmap.h>
 
 #include "tpa6130a2.h"
 
@@ -33,7 +31,7 @@ struct tpa6130a2_data {
 	struct device *dev;
 	struct regmap *regmap;
 	struct regulator *supply;
-	int power_gpio;
+	struct gpio_desc *power_gpio;
 	enum tpa_model id;
 };
 
@@ -49,8 +47,7 @@ static int tpa6130a2_power(struct tpa6130a2_data *data, bool enable)
 			return ret;
 		}
 		/* Power on */
-		if (data->power_gpio >= 0)
-			gpio_set_value(data->power_gpio, 1);
+		gpiod_set_value(data->power_gpio, 1);
 
 		/* Sync registers */
 		regcache_cache_only(data->regmap, false);
@@ -59,8 +56,7 @@ static int tpa6130a2_power(struct tpa6130a2_data *data, bool enable)
 			dev_err(data->dev,
 				"Failed to sync registers: %d\n", ret);
 			regcache_cache_only(data->regmap, true);
-			if (data->power_gpio >= 0)
-				gpio_set_value(data->power_gpio, 0);
+			gpiod_set_value(data->power_gpio, 0);
 			ret2 = regulator_disable(data->supply);
 			if (ret2 != 0)
 				dev_err(data->dev,
@@ -76,8 +72,7 @@ static int tpa6130a2_power(struct tpa6130a2_data *data, bool enable)
 		regcache_cache_only(data->regmap, true);
 
 		/* Power off */
-		if (data->power_gpio >= 0)
-			gpio_set_value(data->power_gpio, 0);
+		gpiod_set_value(data->power_gpio, 0);
 
 		ret = regulator_disable(data->supply);
 		if (ret != 0) {
@@ -209,18 +204,10 @@ static const struct regmap_config tpa6130a2_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
-static const struct i2c_device_id tpa6130a2_id[] = {
-	{ "tpa6130a2", TPA6130A2 },
-	{ "tpa6140a2", TPA6140A2 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, tpa6130a2_id);
-
 static int tpa6130a2_probe(struct i2c_client *client)
 {
 	struct device *dev;
 	struct tpa6130a2_data *data;
-	struct tpa6130a2_platform_data *pdata = client->dev.platform_data;
 	struct device_node *np = client->dev.of_node;
 	const char *regulator;
 	unsigned int version;
@@ -238,10 +225,13 @@ static int tpa6130a2_probe(struct i2c_client *client)
 	if (IS_ERR(data->regmap))
 		return PTR_ERR(data->regmap);
 
-	if (pdata) {
-		data->power_gpio = pdata->power_gpio;
-	} else if (np) {
-		data->power_gpio = of_get_named_gpio(np, "power-gpio", 0);
+	if (np) {
+		data->power_gpio = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
+		if (IS_ERR(data->power_gpio)) {
+			return dev_err_probe(dev, PTR_ERR(data->power_gpio),
+					     "Failed to request power GPIO\n");
+		}
+		gpiod_set_consumer_name(data->power_gpio, "tpa6130a2 enable");
 	} else {
 		dev_err(dev, "Platform data not set\n");
 		dump_stack();
@@ -252,17 +242,6 @@ static int tpa6130a2_probe(struct i2c_client *client)
 
 	data->id = (uintptr_t)i2c_get_match_data(client);
 
-	if (data->power_gpio >= 0) {
-		ret = devm_gpio_request(dev, data->power_gpio,
-					"tpa6130a2 enable");
-		if (ret < 0) {
-			dev_err(dev, "Failed to request power GPIO (%d)\n",
-				data->power_gpio);
-			return ret;
-		}
-		gpio_direction_output(data->power_gpio, 0);
-	}
-
 	switch (data->id) {
 	default:
 		dev_warn(dev, "Unknown TPA model (%d). Assuming 6130A2\n",
@@ -318,7 +297,6 @@ static struct i2c_driver tpa6130a2_i2c_driver = {
 		.of_match_table = of_match_ptr(tpa6130a2_of_match),
 	},
 	.probe = tpa6130a2_probe,
-	.id_table = tpa6130a2_id,
 };
 
 module_i2c_driver(tpa6130a2_i2c_driver);

-- 
2.37.1

Re: [PATCH 7/7] ASoC: codec: tpa6130a2: Convert to GPIO descriptors

[Linus Walleij]

On Tue, Apr 8, 2025 at 3:41 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 devm_gpiod_get_optional to get GPIO descriptor with default
>    polarity GPIOD_OUT_LOW, set consumer name.
>  - Use gpiod_set_value to configure output value.
>
> While at here
>  - reorder the included headers.
>  - Drop sound/tpa6130a2-plat.h because no user is creating the device using
>    platform data
>
> Checking the DTS polarity, all users are using GPIOD_ACTIVE_HIGH.
> so all should work as expected with this patch.
>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

(...)
> -static const struct i2c_device_id tpa6130a2_id[] = {
> -       { "tpa6130a2", TPA6130A2 },
> -       { "tpa6140a2", TPA6140A2 },
> -       { }
> -};
> -MODULE_DEVICE_TABLE(i2c, tpa6130a2_id);
(...)
>         },
>         .probe = tpa6130a2_probe,
> -       .id_table = tpa6130a2_id,

IIRC the DT-only I2C drivers still require this ID table,
unintuitive, but I think that's how it is.

(Others may correct me!)

Yours,
Linus Walleij