| Message ID | 20250408-asoc-gpio-v1-2-c0db9d3fd6e9@nxp.com (mailing list archive) |
|---|---|
| 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); > } > > >
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); }