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.1On 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);
> }
>
>
>
--
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/
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
© 2016 - 2025 Red Hat, Inc.