| Message ID | 20250408-asoc-gpio-v1-2-c0db9d3fd6e9@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: codec: Convert to GPIO descriptors | expand |
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
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); > } > > >
> 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 >
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
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); }