public inbox for linux-gpio@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] Add TI TPS65215 PMIC GPIO Support
@ 2025-04-25 20:33 Shree Ramamoorthy
  2025-04-25 20:33 ` [PATCH v4 1/3] gpio: tps65219: Add TPS65215 to platform_device_id table Shree Ramamoorthy
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Shree Ramamoorthy @ 2025-04-25 20:33 UTC (permalink / raw)
  To: aaro.koskinen, andreas, khilman, rogerq, tony, linus.walleij,
	brgl, linux-omap, linux-kernel, linux-gpio
  Cc: m-leonard, praneeth

Rebase patch series for 6.16 cycle. The related MFD series was integrated 
in mainline during 6.15 cycle [0].

TPS65215 is a Power Management Integrated Circuit (PMIC) that has
significant register map overlap with TPS65219. The series introduces
TPS65215 and restructures the existing driver to support multiple devices.

- Both TPS65215 and TPS65219 have 3 Buck regulators.
- TPS65215 has 2 LDOs, whereas TPS65219 has 4 LDOs.
- TPS65215 and TPS65219's LDO1 are the same.
- TPS65215's LDO2 maps to TPS65219's LDO3.
- TPS65215 has 1 GPO, whereas TPS65219 has 2 GPOs.
- The remaining features are the same.

TPS65215 TRM: https://www.ti.com/lit/pdf/slvucw5/

AM62L + TPS65215 Test Logs:
https://gist.github.com/ramamoorthyhs/7560eca6110fafc77b51894fa2c0fd22

---
Change Log:
v3 -> v4:
- Update cover letter
- Rebase for 6.16 cycle
v2 -> v3:
- Correct gpio_chip.ngpio line to use .offset field
- Remove unnecessary newlines
v1 -> v2:
- have any PMIC lists be in alpha-numeric order: TPS65215, then TPS65219
- remove comma after terminator
- Add driver prefix to chip_data struct
---
[0]: https://lore.kernel.org/all/173928615760.2233464.12306998726512431222.b4-ty@kernel.org/

Shree Ramamoorthy (3):
  gpio: tps65219: Add TPS65215 to platform_device_id table
  gpio: tps65219: Update GPIO0_IDX macro prefix
  gpio: tps65219: Add support for varying gpio/offset values

 drivers/gpio/gpio-tps65219.c | 56 +++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 14 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 1/3] gpio: tps65219: Add TPS65215 to platform_device_id table
  2025-04-25 20:33 [PATCH v4 0/3] Add TI TPS65215 PMIC GPIO Support Shree Ramamoorthy
@ 2025-04-25 20:33 ` Shree Ramamoorthy
  2025-04-25 22:47   ` Andrew Davis
  2025-04-25 20:33 ` [PATCH v4 2/3] gpio: tps65219: Update GPIO0_IDX macro prefix Shree Ramamoorthy
  2025-04-25 20:33 ` [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values Shree Ramamoorthy
  2 siblings, 1 reply; 9+ messages in thread
From: Shree Ramamoorthy @ 2025-04-25 20:33 UTC (permalink / raw)
  To: aaro.koskinen, andreas, khilman, rogerq, tony, linus.walleij,
	brgl, linux-omap, linux-kernel, linux-gpio
  Cc: m-leonard, praneeth

Add platform_device_id struct and use the platform_get_device_id() output
to match which PMIC device is in use. With new name options, the gpio_chip
.label field is now assigned to the platform_device name match.

Remove MODULE_ALIAS since it is now generated by MODULE_DEVICE_TABLE.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/gpio/gpio-tps65219.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
index 526640c39a11..996f8deaf03d 100644
--- a/drivers/gpio/gpio-tps65219.c
+++ b/drivers/gpio/gpio-tps65219.c
@@ -1,8 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * GPIO driver for TI TPS65219 PMICs
+ * GPIO driver for TI TPS65215/TPS65219 PMICs
  *
- * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/
+ * Copyright (C) 2024 Texas Instruments Incorporated - http://www.ti.com/
  */
 
 #include <linux/bits.h>
@@ -141,7 +141,6 @@ static int tps65219_gpio_direction_output(struct gpio_chip *gc, unsigned int off
 }
 
 static const struct gpio_chip tps65219_template_chip = {
-	.label			= "tps65219-gpio",
 	.owner			= THIS_MODULE,
 	.get_direction		= tps65219_gpio_get_direction,
 	.direction_input	= tps65219_gpio_direction_input,
@@ -164,20 +163,28 @@ static int tps65219_gpio_probe(struct platform_device *pdev)
 
 	gpio->tps = tps;
 	gpio->gpio_chip = tps65219_template_chip;
+	gpio->gpio_chip.label = dev_name(&pdev->dev);
 	gpio->gpio_chip.parent = tps->dev;
 
 	return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio_chip, gpio);
 }
 
+static const struct platform_device_id tps6521x_gpio_id_table[] = {
+	{ "tps65215-gpio", TPS65215 },
+	{ "tps65219-gpio", TPS65219 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, tps6521x_gpio_id_table);
+
 static struct platform_driver tps65219_gpio_driver = {
 	.driver = {
 		.name = "tps65219-gpio",
 	},
 	.probe = tps65219_gpio_probe,
+	.id_table = tps6521x_gpio_id_table,
 };
 module_platform_driver(tps65219_gpio_driver);
 
-MODULE_ALIAS("platform:tps65219-gpio");
 MODULE_AUTHOR("Jonathan Cormier <jcormier@criticallink.com>");
-MODULE_DESCRIPTION("TPS65219 GPIO driver");
+MODULE_DESCRIPTION("TPS65215/TPS65219 GPIO driver");
 MODULE_LICENSE("GPL");
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 2/3] gpio: tps65219: Update GPIO0_IDX macro prefix
  2025-04-25 20:33 [PATCH v4 0/3] Add TI TPS65215 PMIC GPIO Support Shree Ramamoorthy
  2025-04-25 20:33 ` [PATCH v4 1/3] gpio: tps65219: Add TPS65215 to platform_device_id table Shree Ramamoorthy
@ 2025-04-25 20:33 ` Shree Ramamoorthy
  2025-04-25 20:33 ` [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values Shree Ramamoorthy
  2 siblings, 0 replies; 9+ messages in thread
From: Shree Ramamoorthy @ 2025-04-25 20:33 UTC (permalink / raw)
  To: aaro.koskinen, andreas, khilman, rogerq, tony, linus.walleij,
	brgl, linux-omap, linux-kernel, linux-gpio
  Cc: m-leonard, praneeth

Updating the macro name to TPS6521X_GPIO0_IDX is meant to indicate this
macro applies to both PMIC devices.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-tps65219.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
index 996f8deaf03d..a5a9dfdb214c 100644
--- a/drivers/gpio/gpio-tps65219.c
+++ b/drivers/gpio/gpio-tps65219.c
@@ -14,7 +14,7 @@
 
 #define TPS65219_GPIO0_DIR_MASK		BIT(3)
 #define TPS65219_GPIO0_OFFSET		2
-#define TPS65219_GPIO0_IDX		0
+#define TPS6521X_GPIO0_IDX			0
 
 struct tps65219_gpio {
 	struct gpio_chip gpio_chip;
@@ -26,7 +26,7 @@ static int tps65219_gpio_get_direction(struct gpio_chip *gc, unsigned int offset
 	struct tps65219_gpio *gpio = gpiochip_get_data(gc);
 	int ret, val;
 
-	if (offset != TPS65219_GPIO0_IDX)
+	if (offset != TPS6521X_GPIO0_IDX)
 		return GPIO_LINE_DIRECTION_OUT;
 
 	ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, &val);
@@ -42,7 +42,7 @@ static int tps65219_gpio_get(struct gpio_chip *gc, unsigned int offset)
 	struct device *dev = gpio->tps->dev;
 	int ret, val;
 
-	if (offset != TPS65219_GPIO0_IDX) {
+	if (offset != TPS6521X_GPIO0_IDX) {
 		dev_err(dev, "GPIO%d is output only, cannot get\n", offset);
 		return -ENOTSUPP;
 	}
@@ -71,7 +71,7 @@ static void tps65219_gpio_set(struct gpio_chip *gc, unsigned int offset, int val
 	struct device *dev = gpio->tps->dev;
 	int v, mask, bit;
 
-	bit = (offset == TPS65219_GPIO0_IDX) ? TPS65219_GPIO0_OFFSET : offset - 1;
+	bit = (offset == TPS6521X_GPIO0_IDX) ? TPS65219_GPIO0_OFFSET : offset - 1;
 
 	mask = BIT(bit);
 	v = value ? mask : 0;
@@ -117,7 +117,7 @@ static int tps65219_gpio_direction_input(struct gpio_chip *gc, unsigned int offs
 	struct tps65219_gpio *gpio = gpiochip_get_data(gc);
 	struct device *dev = gpio->tps->dev;
 
-	if (offset != TPS65219_GPIO0_IDX) {
+	if (offset != TPS6521X_GPIO0_IDX) {
 		dev_err(dev, "GPIO%d is output only, cannot change to input\n", offset);
 		return -ENOTSUPP;
 	}
@@ -131,7 +131,7 @@ static int tps65219_gpio_direction_input(struct gpio_chip *gc, unsigned int offs
 static int tps65219_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
 {
 	tps65219_gpio_set(gc, offset, value);
-	if (offset != TPS65219_GPIO0_IDX)
+	if (offset != TPS6521X_GPIO0_IDX)
 		return 0;
 
 	if (tps65219_gpio_get_direction(gc, offset) == GPIO_LINE_DIRECTION_OUT)
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values
  2025-04-25 20:33 [PATCH v4 0/3] Add TI TPS65215 PMIC GPIO Support Shree Ramamoorthy
  2025-04-25 20:33 ` [PATCH v4 1/3] gpio: tps65219: Add TPS65215 to platform_device_id table Shree Ramamoorthy
  2025-04-25 20:33 ` [PATCH v4 2/3] gpio: tps65219: Update GPIO0_IDX macro prefix Shree Ramamoorthy
@ 2025-04-25 20:33 ` Shree Ramamoorthy
  2025-04-28 16:41   ` Jonathan Cormier
  2 siblings, 1 reply; 9+ messages in thread
From: Shree Ramamoorthy @ 2025-04-25 20:33 UTC (permalink / raw)
  To: aaro.koskinen, andreas, khilman, rogerq, tony, linus.walleij,
	brgl, linux-omap, linux-kernel, linux-gpio
  Cc: m-leonard, praneeth

Add device-specific structs to select the different PMIC .npgio and .offset
values. With the chip_data struct values selected based on the match data,
having a separate GPIO0_OFFSET macro is no longer needed.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/gpio/gpio-tps65219.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
index a5a9dfdb214c..c971deac8619 100644
--- a/drivers/gpio/gpio-tps65219.c
+++ b/drivers/gpio/gpio-tps65219.c
@@ -13,7 +13,6 @@
 #include <linux/regmap.h>
 
 #define TPS65219_GPIO0_DIR_MASK		BIT(3)
-#define TPS65219_GPIO0_OFFSET		2
 #define TPS6521X_GPIO0_IDX			0
 
 struct tps65219_gpio {
@@ -21,6 +20,11 @@ struct tps65219_gpio {
 	struct tps65219 *tps;
 };
 
+struct tps65219_chip_data {
+	int ngpio;
+	int offset;
+};
+
 static int tps65219_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
 {
 	struct tps65219_gpio *gpio = gpiochip_get_data(gc);
@@ -71,7 +75,7 @@ static void tps65219_gpio_set(struct gpio_chip *gc, unsigned int offset, int val
 	struct device *dev = gpio->tps->dev;
 	int v, mask, bit;
 
-	bit = (offset == TPS6521X_GPIO0_IDX) ? TPS65219_GPIO0_OFFSET : offset - 1;
+	bit = (offset == TPS6521X_GPIO0_IDX) ? (gpio->gpio_chip.offset - 1) : offset - 1;
 
 	mask = BIT(bit);
 	v = value ? mask : 0;
@@ -148,14 +152,29 @@ static const struct gpio_chip tps65219_template_chip = {
 	.get			= tps65219_gpio_get,
 	.set			= tps65219_gpio_set,
 	.base			= -1,
-	.ngpio			= 3,
 	.can_sleep		= true,
 };
 
+static const struct tps65219_chip_data chip_info_table[] = {
+	[TPS65215] = {
+		.ngpio = 2,
+		.offset = 1,
+	},
+	[TPS65219] = {
+		.ngpio = 3,
+		.offset = 2,
+	},
+};
+
 static int tps65219_gpio_probe(struct platform_device *pdev)
 {
-	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
 	struct tps65219_gpio *gpio;
+	const struct tps65219_chip_data *pmic;
+
+	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
+	enum pmic_id chip = platform_get_device_id(pdev)->driver_data;
+
+	pmic = &chip_info_table[chip];
 
 	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
 	if (!gpio)
@@ -164,6 +183,8 @@ static int tps65219_gpio_probe(struct platform_device *pdev)
 	gpio->tps = tps;
 	gpio->gpio_chip = tps65219_template_chip;
 	gpio->gpio_chip.label = dev_name(&pdev->dev);
+	gpio->gpio_chip.ngpio =  pmic->ngpio;
+	gpio->gpio_chip.offset = pmic->offset;
 	gpio->gpio_chip.parent = tps->dev;
 
 	return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio_chip, gpio);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/3] gpio: tps65219: Add TPS65215 to platform_device_id table
  2025-04-25 20:33 ` [PATCH v4 1/3] gpio: tps65219: Add TPS65215 to platform_device_id table Shree Ramamoorthy
@ 2025-04-25 22:47   ` Andrew Davis
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Davis @ 2025-04-25 22:47 UTC (permalink / raw)
  To: Shree Ramamoorthy, aaro.koskinen, andreas, khilman, rogerq, tony,
	linus.walleij, brgl, linux-omap, linux-kernel, linux-gpio
  Cc: m-leonard, praneeth

On 4/25/25 3:33 PM, Shree Ramamoorthy wrote:
> Add platform_device_id struct and use the platform_get_device_id() output
> to match which PMIC device is in use. With new name options, the gpio_chip
> .label field is now assigned to the platform_device name match.
> 
> Remove MODULE_ALIAS since it is now generated by MODULE_DEVICE_TABLE.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>   drivers/gpio/gpio-tps65219.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
> index 526640c39a11..996f8deaf03d 100644
> --- a/drivers/gpio/gpio-tps65219.c
> +++ b/drivers/gpio/gpio-tps65219.c
> @@ -1,8 +1,8 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /*
> - * GPIO driver for TI TPS65219 PMICs
> + * GPIO driver for TI TPS65215/TPS65219 PMICs
>    *
> - * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/
> + * Copyright (C) 2024 Texas Instruments Incorporated - http://www.ti.com/
>    */
>   
>   #include <linux/bits.h>
> @@ -141,7 +141,6 @@ static int tps65219_gpio_direction_output(struct gpio_chip *gc, unsigned int off
>   }
>   
>   static const struct gpio_chip tps65219_template_chip = {
> -	.label			= "tps65219-gpio",
>   	.owner			= THIS_MODULE,
>   	.get_direction		= tps65219_gpio_get_direction,
>   	.direction_input	= tps65219_gpio_direction_input,
> @@ -164,20 +163,28 @@ static int tps65219_gpio_probe(struct platform_device *pdev)
>   
>   	gpio->tps = tps;
>   	gpio->gpio_chip = tps65219_template_chip;
> +	gpio->gpio_chip.label = dev_name(&pdev->dev);
>   	gpio->gpio_chip.parent = tps->dev;
>   
>   	return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio_chip, gpio);
>   }
>   
> +static const struct platform_device_id tps6521x_gpio_id_table[] = {
> +	{ "tps65215-gpio", TPS65215 },

I would have added this TPS65215 item to this table in patch [3/3]
where it is first used, but that's nitpicking,

Reviewed-by: Andrew Davis <afd@ti.com>

> +	{ "tps65219-gpio", TPS65219 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, tps6521x_gpio_id_table);
> +
>   static struct platform_driver tps65219_gpio_driver = {
>   	.driver = {
>   		.name = "tps65219-gpio",
>   	},
>   	.probe = tps65219_gpio_probe,
> +	.id_table = tps6521x_gpio_id_table,
>   };
>   module_platform_driver(tps65219_gpio_driver);
>   
> -MODULE_ALIAS("platform:tps65219-gpio");
>   MODULE_AUTHOR("Jonathan Cormier <jcormier@criticallink.com>");
> -MODULE_DESCRIPTION("TPS65219 GPIO driver");
> +MODULE_DESCRIPTION("TPS65215/TPS65219 GPIO driver");
>   MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values
  2025-04-25 20:33 ` [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values Shree Ramamoorthy
@ 2025-04-28 16:41   ` Jonathan Cormier
  2025-04-29 13:17     ` Jon Cormier
  2025-04-29 16:42     ` Shree Ramamoorthy
  0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Cormier @ 2025-04-28 16:41 UTC (permalink / raw)
  To: Shree Ramamoorthy, aaro.koskinen, andreas, khilman, rogerq, tony,
	linus.walleij, brgl, linux-omap, linux-kernel, linux-gpio,
	Jerome Neanne
  Cc: m-leonard, praneeth, jsava, Praneeth Bajjuri

On 4/25/25 4:33 PM, Shree Ramamoorthy wrote:
> Add device-specific structs to select the different PMIC .npgio and .offset
> values. With the chip_data struct values selected based on the match data,
> having a separate GPIO0_OFFSET macro is no longer needed.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>   drivers/gpio/gpio-tps65219.c | 29 +++++++++++++++++++++++++----
>   1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
> index a5a9dfdb214c..c971deac8619 100644
> --- a/drivers/gpio/gpio-tps65219.c
> +++ b/drivers/gpio/gpio-tps65219.c
> @@ -13,7 +13,6 @@
>   #include <linux/regmap.h>
>   
>   #define TPS65219_GPIO0_DIR_MASK		BIT(3)
> -#define TPS65219_GPIO0_OFFSET		2
>   #define TPS6521X_GPIO0_IDX			0
>   
>   struct tps65219_gpio {
> @@ -21,6 +20,11 @@ struct tps65219_gpio {
>   	struct tps65219 *tps;
>   };
>   
> +struct tps65219_chip_data {
> +	int ngpio;
> +	int offset;
> +};
> +
>   static int tps65219_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
>   {
>   	struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> @@ -71,7 +75,7 @@ static void tps65219_gpio_set(struct gpio_chip *gc, unsigned int offset, int val
>   	struct device *dev = gpio->tps->dev;
>   	int v, mask, bit;
>   
> -	bit = (offset == TPS6521X_GPIO0_IDX) ? TPS65219_GPIO0_OFFSET : offset - 1;
> +	bit = (offset == TPS6521X_GPIO0_IDX) ? (gpio->gpio_chip.offset - 1) : offset - 1;
(gpio->gpio_chip.offset - 1) is incorrect.  TPS65219_GPIO0_OFFSET used 
to be 2 but now ends up being calculated as 1.  This causes our board to 
power cycle when we try to blink our LED connected to GPIO (Pin 16) - 
"gpio 0".

The whole reason for this strange offset math was that on the TPS65219:
GPO0 -> Register bit 0
GPO1 -> Register bit 1
GPIO -> Register bit 2

However Jerome wanted GPIO to map to linux "GPIO 0".  Is this still the 
case for TPS65215?
>   
>   	mask = BIT(bit);
>   	v = value ? mask : 0;
> @@ -148,14 +152,29 @@ static const struct gpio_chip tps65219_template_chip = {
>   	.get			= tps65219_gpio_get,
>   	.set			= tps65219_gpio_set,
>   	.base			= -1,
> -	.ngpio			= 3,
>   	.can_sleep		= true,
>   };
>   
> +static const struct tps65219_chip_data chip_info_table[] = {
> +	[TPS65215] = {
> +		.ngpio = 2,
> +		.offset = 1,
I cannot validate this.

The linked TRM for the TPS65215 mentions register and field names but 
doesn't provide any register addresses or field offsets. So I cannot 
validate if the GPIO0 math is correct or how it compares to the TPS65219.

Figure 2-2 shows the PMIC has 3 "GPIO" (1 GPIO and 2 GPO) similar to the 
TPS65219 but Shree has stated there is only 2 (1 GPIO and 1 GPO) so a 
little confusing.
> TPS65215 TRM: https://www.ti.com/lit/pdf/slvucw5/

> +	},
> +	[TPS65219] = {
> +		.ngpio = 3,
> +		.offset = 2,
Offset 2 ends up becoming 1
> +	},
> +};

Note for TI, this needs to be fixed in the SDK11 6.12 release for the 
AM62x as well.

Note: Its unclear to me, why Jerome Neanne and I weren't included in 
this patch set, considering we were the ones who authored and submitted 
this driver.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values
  2025-04-28 16:41   ` Jonathan Cormier
@ 2025-04-29 13:17     ` Jon Cormier
  2025-04-29 16:42     ` Shree Ramamoorthy
  1 sibling, 0 replies; 9+ messages in thread
From: Jon Cormier @ 2025-04-29 13:17 UTC (permalink / raw)
  To: Shree Ramamoorthy, aaro.koskinen, andreas, khilman, rogerq, tony,
	linus.walleij, brgl, linux-omap, linux-kernel, linux-gpio
  Cc: m-leonard, jsava, Praneeth Bajjuri

On Mon, Apr 28, 2025 at 12:41 PM Jonathan Cormier
<jcormier@criticallink.com> wrote:
>

>
> Note: Its unclear to me, why Jerome Neanne and I weren't included in
> this patch set, considering we were the ones who authored and submitted
> this driver.

I see Jerome's email is dead. So that's one mystery solved.



-- 
Jonathan Cormier
Senior Software Engineer

Voice:  315.425.4045 x222

http://www.CriticalLink.com
6712 Brooklawn Parkway, Syracuse, NY 13211

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values
  2025-04-28 16:41   ` Jonathan Cormier
  2025-04-29 13:17     ` Jon Cormier
@ 2025-04-29 16:42     ` Shree Ramamoorthy
  2025-04-29 19:25       ` Jon Cormier
  1 sibling, 1 reply; 9+ messages in thread
From: Shree Ramamoorthy @ 2025-04-29 16:42 UTC (permalink / raw)
  To: Jonathan Cormier, aaro.koskinen, andreas, khilman, rogerq, tony,
	linus.walleij, brgl, linux-omap, linux-kernel, linux-gpio,
	Jerome Neanne
  Cc: m-leonard, praneeth, jsava

Hi,

On 4/28/2025 11:41 AM, Jonathan Cormier wrote:
> On 4/25/25 4:33 PM, Shree Ramamoorthy wrote:
>> Add device-specific structs to select the different PMIC .npgio and
>> .offset
>> values. With the chip_data struct values selected based on the match
>> data,
>> having a separate GPIO0_OFFSET macro is no longer needed.
>>
>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>> ---
>>   drivers/gpio/gpio-tps65219.c | 29 +++++++++++++++++++++++++----
>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
>> index a5a9dfdb214c..c971deac8619 100644
>> --- a/drivers/gpio/gpio-tps65219.c
>> +++ b/drivers/gpio/gpio-tps65219.c
>> @@ -13,7 +13,6 @@
>>   #include <linux/regmap.h>
>>     #define TPS65219_GPIO0_DIR_MASK        BIT(3)
>> -#define TPS65219_GPIO0_OFFSET        2
>>   #define TPS6521X_GPIO0_IDX            0
>>     struct tps65219_gpio {
>> @@ -21,6 +20,11 @@ struct tps65219_gpio {
>>       struct tps65219 *tps;
>>   };
>>   +struct tps65219_chip_data {
>> +    int ngpio;
>> +    int offset;
>> +};
>> +
>>   static int tps65219_gpio_get_direction(struct gpio_chip *gc,
>> unsigned int offset)
>>   {
>>       struct tps65219_gpio *gpio = gpiochip_get_data(gc);
>> @@ -71,7 +75,7 @@ static void tps65219_gpio_set(struct gpio_chip *gc,
>> unsigned int offset, int val
>>       struct device *dev = gpio->tps->dev;
>>       int v, mask, bit;
>>   -    bit = (offset == TPS6521X_GPIO0_IDX) ? TPS65219_GPIO0_OFFSET :
>> offset - 1;
>> +    bit = (offset == TPS6521X_GPIO0_IDX) ? (gpio->gpio_chip.offset -
>> 1) : offset - 1;
> (gpio->gpio_chip.offset - 1) is incorrect.  TPS65219_GPIO0_OFFSET used
> to be 2 but now ends up being calculated as 1.  This causes our board
> to power cycle when we try to blink our LED connected to GPIO (Pin 16)
> - "gpio 0".
>
> The whole reason for this strange offset math was that on the TPS65219:
> GPO0 -> Register bit 0
> GPO1 -> Register bit 1
> GPIO -> Register bit 2
>
> However Jerome wanted GPIO to map to linux "GPIO 0".  Is this still
> the case for TPS65215?

In my attempt to combine TPS65214 (which originally had 1 GPO and 1 GPIO
when I wrote the patch, but systems informed me they just switched it to
2 GPOs and 1 GPIO) & TPS65215 (2 GPOs and 1 GPIO), I made a mistake in
combining the 2 series during rebase & with how similar the PMICs are.
Thanks for reviewing this as I wrote it a cycle ago. I'll made the
necessary changes & re-test. I will double check that GPIO matches to
linux "GPIO 0" now that I have more context about the offset math (super
helpful explanation!).

>>         mask = BIT(bit);
>>       v = value ? mask : 0;
>> @@ -148,14 +152,29 @@ static const struct gpio_chip
>> tps65219_template_chip = {
>>       .get            = tps65219_gpio_get,
>>       .set            = tps65219_gpio_set,
>>       .base            = -1,
>> -    .ngpio            = 3,
>>       .can_sleep        = true,
>>   };
>>   +static const struct tps65219_chip_data chip_info_table[] = {
>> +    [TPS65215] = {
>> +        .ngpio = 2,
>> +        .offset = 1,
> I cannot validate this.
>
> The linked TRM for the TPS65215 mentions register and field names but
> doesn't provide any register addresses or field offsets. So I cannot
> validate if the GPIO0 math is correct or how it compares to the TPS65219.
>
> Figure 2-2 shows the PMIC has 3 "GPIO" (1 GPIO and 2 GPO) similar to
> the TPS65219 but Shree has stated there is only 2 (1 GPIO and 1 GPO)
> so a little confusing.
This was a mistake while rebasing to combine patches for TPS65214 and
TPS65215 :( I will fix this immediately. Sorry for the incorrect patch,
but thank you for taking the time to review!
>> TPS65215 TRM: https://www.ti.com/lit/pdf/slvucw5/
>
>> +    },
>> +    [TPS65219] = {
>> +        .ngpio = 3,
>> +        .offset = 2,
> Offset 2 ends up becoming 1
>> +    },
>> +};
>
> Note for TI, this needs to be fixed in the SDK11 6.12 release for the
> AM62x as well.
>
> Note: Its unclear to me, why Jerome Neanne and I weren't included in
> this patch set, considering we were the ones who authored and
> submitted this driver.
Jerome's email came back as invalid & my habit of using suppress cc's
while sending these for review didn't add your email. I'll be sure to
add you to the cc list for future series! Sorry again for the mistakes
and dropped cc, will fix these accordingly.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values
  2025-04-29 16:42     ` Shree Ramamoorthy
@ 2025-04-29 19:25       ` Jon Cormier
  0 siblings, 0 replies; 9+ messages in thread
From: Jon Cormier @ 2025-04-29 19:25 UTC (permalink / raw)
  To: Shree Ramamoorthy
  Cc: aaro.koskinen, andreas, khilman, rogerq, tony, linus.walleij,
	brgl, linux-omap, linux-kernel, linux-gpio, Jerome Neanne,
	m-leonard, praneeth, jsava

On Tue, Apr 29, 2025 at 12:42 PM Shree Ramamoorthy <s-ramamoorthy@ti.com> wrote:
>
> Hi,
>
> On 4/28/2025 11:41 AM, Jonathan Cormier wrote:
> > On 4/25/25 4:33 PM, Shree Ramamoorthy wrote:

> >
> > However Jerome wanted GPIO to map to linux "GPIO 0".  Is this still
> > the case for TPS65215?
>
> In my attempt to combine TPS65214 (which originally had 1 GPO and 1 GPIO
> when I wrote the patch, but systems informed me they just switched it to
> 2 GPOs and 1 GPIO) & TPS65215 (2 GPOs and 1 GPIO), I made a mistake in
> combining the 2 series during rebase & with how similar the PMICs are.
> Thanks for reviewing this as I wrote it a cycle ago. I'll made the
> necessary changes & re-test. I will double check that GPIO matches to
> linux "GPIO 0" now that I have more context about the offset math (super
> helpful explanation!).


Thanks. Considering this confusion, could you add a comment for the
pin mappings? Something like:
// TPS65219 GPIO mapping
// Linux gpio 0 -> GPIO (pin16) -> offset 2
// Linux gpio 1 -> GPO1 (pin8 ) -> offset 0
// Linux gpio 2 -> GPO2 (pin17) -> offset 1




-- 
Jonathan Cormier
Senior Software Engineer

Voice:  315.425.4045 x222

http://www.CriticalLink.com
6712 Brooklawn Parkway, Syracuse, NY 13211

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-04-29 19:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-25 20:33 [PATCH v4 0/3] Add TI TPS65215 PMIC GPIO Support Shree Ramamoorthy
2025-04-25 20:33 ` [PATCH v4 1/3] gpio: tps65219: Add TPS65215 to platform_device_id table Shree Ramamoorthy
2025-04-25 22:47   ` Andrew Davis
2025-04-25 20:33 ` [PATCH v4 2/3] gpio: tps65219: Update GPIO0_IDX macro prefix Shree Ramamoorthy
2025-04-25 20:33 ` [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values Shree Ramamoorthy
2025-04-28 16:41   ` Jonathan Cormier
2025-04-29 13:17     ` Jon Cormier
2025-04-29 16:42     ` Shree Ramamoorthy
2025-04-29 19:25       ` Jon Cormier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox