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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Am Montag, 7. Juli 2025, 07:40:58 CEST schrieb Peng Fan:
> ********************
> Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter.
> Attention external email: Open attachments and links only if you know that they are from a secure source and are safe. In doubt forward the email to the IT-Helpdesk to check it.
> ********************
> 
> > Subject: Re: [PATCH 2/2] ASoC: codec: tlv320aic32x4: Convert to GPIO
> > descriptors
> > 
> > Hi,
> > 
> > Am Sonntag, 6. Juli 2025, 03:04:24 CEST schrieb Peng Fan:
> > > 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.
> > >
> > > 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>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > 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>
> > 
> > Mh, maybe create a single commit sorting these headers.
> 
> ok. Let me do a v2 for this.
> 
> > 
> > >
> > >  #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);
> > 
> > Shouldn't this be NULL similar to when devm_gpiod_get_optional()
> > doesn't find any reset GPIO?
> 
> There is a check in driver, so NULL not work here.
> 
>         if (!IS_ERR(aic32x4->rstn_gpio)) {                                                          

I don't like the fact that both paths have different values for rstn_gpio
for the information there is no rstn GPIO. How about setting NULL in the without
DT node case and checking if (aic32x4->rstn_gpio) here?

Best regards
Alexander

>                 ndelay(10);                                                                         
>                 /* deassert reset */                                                                
>                 gpiod_set_value_cansleep(aic32x4->rstn_gpio, 0);                                    
>                 mdelay(1);                                                                          
>         }
> 
> > 
> > Despite that, looks good and works as intended:
> > Tested-By: Alexander Stein <alexander.stein@ew.tq-group.com>
> 
> Appreciate!
> 
> Thanks,
> Peng.
> 
> 


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