diff mbox series

ASoC: codecs: tlv320dac33: switch to gpiod api

Message ID 20250831030855.957707-1-alex.t.tran@gmail.com
State New
Headers show
Series ASoC: codecs: tlv320dac33: switch to gpiod api | expand

Commit Message

Alex Tran Aug. 31, 2025, 3:08 a.m. UTC 
Changelog:
- Changed reset GPIO setup that uses 'gpio_request' and
  'gpio_direction_output' to use 'devm_gpio_request_one' instead 
  for legacy support.
- Convert to gpio descriptor for use.
- Better error handling with 'gpiod_set_value'.
- Removed cleanup of reset gpio as gpiod api is now used.

Signed-off-by: Alex Tran <alex.t.tran@gmail.com>
---
 sound/soc/codecs/tlv320dac33.c | 56 +++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 18 deletions(-)

Comments

Bartosz Golaszewski Aug. 31, 2025, 12:55 p.m. UTC | #1 
On Sun, Aug 31, 2025 at 5:09 AM Alex Tran <alex.t.tran@gmail.com> wrote:
>
> Changelog:
> - Changed reset GPIO setup that uses 'gpio_request' and
>   'gpio_direction_output' to use 'devm_gpio_request_one' instead
>   for legacy support.
> - Convert to gpio descriptor for use.
> - Better error handling with 'gpiod_set_value'.
> - Removed cleanup of reset gpio as gpiod api is now used.
>
> Signed-off-by: Alex Tran <alex.t.tran@gmail.com>
> ---

The fact that the call to gpio_request() is still there made me think
immediately that the conversion is incomplete. I looked into why you
didn't change that and noticed that the global GPIO number comes
through platform data, specifically: struct tlv320dac33_platform_data.
That platform data struct however is not used in the kernel -and even
the header that defines it - sound/tlv320dac33-plat.h - is never
included outside of the driver. Seems to me like the main obstacle to
completing the conversion is not even used in mainline and can be
dropped?

Bart
Alex Tran Sept. 1, 2025, 2:55 a.m. UTC | #2 
On Sun, Aug 31, 2025 at 5:55 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> The fact that the call to gpio_request() is still there made me think
> immediately that the conversion is incomplete. I looked into why you
> didn't change that and noticed that the global GPIO number comes
> through platform data, specifically: struct tlv320dac33_platform_data.
> That platform data struct however is not used in the kernel -and even
> the header that defines it - sound/tlv320dac33-plat.h - is never
> included outside of the driver. Seems to me like the main obstacle to
> completing the conversion is not even used in mainline and can be
> dropped?
>
> Bart

Since struct tlv320dac33_platform_data isn't used in the kernel and the
corresponding header file it's in isn't used outside of the driver,
I'll go ahead
and drop them and send in a v2. Thanks for the review.
diff mbox series

Patch

diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c
index 423b9264a..b8e833efb 100644
--- a/sound/soc/codecs/tlv320dac33.c
+++ b/sound/soc/codecs/tlv320dac33.c
@@ -15,6 +15,7 @@ 
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <sound/core.h>
@@ -109,6 +110,8 @@  struct tlv320dac33_priv {
 
 	enum dac33_state state;
 	struct i2c_client *i2c;
+
+	struct gpio_desc *power_gpiod;
 };
 
 static const u8 dac33_reg[DAC33_CACHEREGNUM] = {
@@ -383,14 +386,28 @@  static int dac33_hard_power(struct snd_soc_component *component, int power)
 			goto exit;
 		}
 
-		if (dac33->power_gpio >= 0)
-			gpio_set_value(dac33->power_gpio, 1);
+		if (dac33->power_gpio >= 0) {
+			ret = gpiod_set_value(dac33->power_gpiod, 1);
+			if (ret < 0) {
+				dev_err(&dac33->i2c->dev,
+					"Failed to set reset GPIO %d: %d\n",
+					dac33->power_gpio, ret);
+				goto exit;
+			}
+		}
 
 		dac33->chip_power = 1;
 	} else {
 		dac33_soft_power(component, 0);
-		if (dac33->power_gpio >= 0)
-			gpio_set_value(dac33->power_gpio, 0);
+		if (dac33->power_gpio >= 0) {
+			ret = gpiod_set_value(dac33->power_gpiod, 0);
+			if (ret < 0) {
+				dev_err(&dac33->i2c->dev,
+					"Failed to set reset GPIO %d: %d\n",
+					dac33->power_gpio, ret);
+				goto exit;
+			}
+		}
 
 		ret = regulator_bulk_disable(ARRAY_SIZE(dac33->supplies),
 					     dac33->supplies);
@@ -1500,14 +1517,22 @@  static int dac33_i2c_probe(struct i2c_client *client)
 
 	/* Check if the reset GPIO number is valid and request it */
 	if (dac33->power_gpio >= 0) {
-		ret = gpio_request(dac33->power_gpio, "tlv320dac33 reset");
+		ret = devm_gpio_request_one(&client->dev, dac33->power_gpio,
+					    GPIOF_OUT_INIT_LOW,
+					    "tlv320dac33 reset");
 		if (ret < 0) {
 			dev_err(&client->dev,
-				"Failed to request reset GPIO (%d)\n",
-				dac33->power_gpio);
-			goto err_gpio;
+				"Failed to request reset GPIO %d: %d\n",
+				dac33->power_gpio, ret);
+			goto err;
+		}
+
+		dac33->power_gpiod = gpio_to_desc(dac33->power_gpio);
+		if (!dac33->power_gpiod) {
+			dev_err(&client->dev,
+				"Failed to get reset GPIO descriptor\n");
+			return -EINVAL;
 		}
-		gpio_direction_output(dac33->power_gpio, 0);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(dac33->supplies); i++)
@@ -1518,19 +1543,17 @@  static int dac33_i2c_probe(struct i2c_client *client)
 
 	if (ret != 0) {
 		dev_err(&client->dev, "Failed to request supplies: %d\n", ret);
-		goto err_get;
+		goto err;
 	}
 
 	ret = devm_snd_soc_register_component(&client->dev,
 			&soc_component_dev_tlv320dac33, &dac33_dai, 1);
 	if (ret < 0)
-		goto err_get;
+		goto err;
 
 	return ret;
-err_get:
-	if (dac33->power_gpio >= 0)
-		gpio_free(dac33->power_gpio);
-err_gpio:
+
+err:
 	return ret;
 }
 
@@ -1540,9 +1563,6 @@  static void dac33_i2c_remove(struct i2c_client *client)
 
 	if (unlikely(dac33->chip_power))
 		dac33_hard_power(dac33->component, 0);
-
-	if (dac33->power_gpio >= 0)
-		gpio_free(dac33->power_gpio);
 }
 
 static const struct i2c_device_id tlv320dac33_i2c_id[] = {