[PATCH v2 0/3] Add TI TPS65215 PMIC GPIO Support

[PATCH v2 0/3] Add TI TPS65215 PMIC GPIO Support

[Shree Ramamoorthy]

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.

This follow-up series is dependent on:
Commit 25c86c81b0ad ("regulator: dt-bindings: Add TI TPS65215 PMIC bindings")
Commit c3cc37e8d23d ("mfd: tps65215: Add support for TI TPS65215 PMIC")
Commit 5f0f36835b90 ("mfd: tps65215: Remove regmap_read check")

TPS65219 Cleanup Series:
GPIO: https://lore.kernel.org/all/20241217204755.1011731-1-s-ramamoorthy@ti.com/
MFD: https://lore.kernel.org/all/20241217204935.1012106-1-s-ramamoorthy@ti.com/
Reg: https://lore.kernel.org/all/20241217204526.1010989-1-s-ramamoorthy@ti.com/

- 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:
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
---

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

 drivers/gpio/gpio-tps65219.c | 54 +++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] gpio: tps65215: Add TPS65215 to platform_device_id table

[Shree Ramamoorthy]

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..7e03be0c7c92 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 },
+	{ }
+};
+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.34.1

[PATCH v2 2/3] gpio: tps65215: Update GPIO0_IDX macro prefix

[Shree Ramamoorthy]

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>
---
 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 7e03be0c7c92..70a4410c473a 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.34.1

[PATCH v2 3/3] gpio tps65215: Add support for varying gpio/offset values

[Shree Ramamoorthy]

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 | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
index 70a4410c473a..14286dd5fdfb 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.ngpio - 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;
+
+	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.34.1

Re: [PATCH v2 1/3] gpio: tps65215: Add TPS65215 to platform_device_id table

[Roger Quadros]

On 04/01/2025 00:54, 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..7e03be0c7c92 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 },
> +	{ }
> +};
> +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");

Why do you drop the MODULE_ALIAS?
You can add multiple MODULE_ALIASES if needed.

>  MODULE_AUTHOR("Jonathan Cormier <jcormier@criticallink.com>");
> -MODULE_DESCRIPTION("TPS65219 GPIO driver");
> +MODULE_DESCRIPTION("TPS65215/TPS65219 GPIO driver");

"TPS6521x GPIO driver"?

I also see a product named TPS65216.
By any chance can that be also supported by this driver?

>  MODULE_LICENSE("GPL");

-- 
cheers,
-roger

Re: [PATCH v2 2/3] gpio: tps65215: Update GPIO0_IDX macro prefix

[Roger Quadros]

On 04/01/2025 00:54, Shree Ramamoorthy wrote:
> 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>

Re: [PATCH v2 3/3] gpio tps65215: Add support for varying gpio/offset values

[Roger Quadros]

On 04/01/2025 00:54, 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 | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
> index 70a4410c473a..14286dd5fdfb 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.ngpio - 1) : offset - 1;

shouldn't this be
	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;
> +
> +	enum pmic_id chip = platform_get_device_id(pdev)->driver_data;
> +

unnecessary newline?

> +	pmic = &chip_info_table[chip];
>

here too?
  
>  	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);

-- 
cheers,
-roger

Re: [PATCH v2 1/3] gpio: tps65215: Add TPS65215 to platform_device_id table

[Andrew Davis]

On 1/4/25 12:21 PM, Roger Quadros wrote:
> 
> 
> On 04/01/2025 00:54, 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..7e03be0c7c92 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 },
>> +	{ }
>> +};
>> +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");
> 
> Why do you drop the MODULE_ALIAS?
> You can add multiple MODULE_ALIASES if needed.
> 

The new MODULE_DEVICE_TABLE() above causes all the needed
module aliases to be made for us automatically.

>>   MODULE_AUTHOR("Jonathan Cormier <jcormier@criticallink.com>");
>> -MODULE_DESCRIPTION("TPS65219 GPIO driver");
>> +MODULE_DESCRIPTION("TPS65215/TPS65219 GPIO driver");
> 
> "TPS6521x GPIO driver"?
> 
> I also see a product named TPS65216.
> By any chance can that be also supported by this driver?
> 

That is kinda the issue with "x" in the name, TPS65216 might
need a different driver, in which case the x here would mislead
folks into thinking this driver covers the whole family.

Andrew

>>   MODULE_LICENSE("GPL");
> 

Re: [PATCH v2 3/3] gpio tps65215: Add support for varying gpio/offset values

[Shree Ramamoorthy]

Hi,

On 1/4/2025 12:27 PM, Roger Quadros wrote:
>
> On 04/01/2025 00:54, 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 | 27 ++++++++++++++++++++++++---
>>  1 file changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
>> index 70a4410c473a..14286dd5fdfb 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.ngpio - 1) : offset - 1;
> shouldn't this be
> 	bit = (offset == TPS6521X_GPIO0_IDX) ? (gpio->gpio_chip.offset - 1) : offset - 1;

Thank you for catching this! I have applied to the change for the next version sent out.

>>  
>>  	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;
>> +
>> +	enum pmic_id chip = platform_get_device_id(pdev)->driver_data;
>> +
> unnecessary newline?
>
>> +	pmic = &chip_info_table[chip];
>>
> here too?

I have reorganized the above section to have the variable declarations separate
from the chunk of code assigning values. Thanks for reviewing!

>>  	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);

Re: [PATCH v2 1/3] gpio: tps65215: Add TPS65215 to platform_device_id table

[Roger Quadros]

On 06/01/2025 19:30, Andrew Davis wrote:
> On 1/4/25 12:21 PM, Roger Quadros wrote:
>>
>>
>> On 04/01/2025 00:54, 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..7e03be0c7c92 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 },
>>> +    { }
>>> +};
>>> +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");
>>
>> Why do you drop the MODULE_ALIAS?
>> You can add multiple MODULE_ALIASES if needed.
>>
> 
> The new MODULE_DEVICE_TABLE() above causes all the needed
> module aliases to be made for us automatically.

Thanks!

> 
>>>   MODULE_AUTHOR("Jonathan Cormier <jcormier@criticallink.com>");
>>> -MODULE_DESCRIPTION("TPS65219 GPIO driver");
>>> +MODULE_DESCRIPTION("TPS65215/TPS65219 GPIO driver");
>>
>> "TPS6521x GPIO driver"?
>>
>> I also see a product named TPS65216.
>> By any chance can that be also supported by this driver?
>>
> 
> That is kinda the issue with "x" in the name, TPS65216 might
> need a different driver, in which case the x here would mislead
> folks into thinking this driver covers the whole family.

Agreed.

> 
> Andrew
> 
>>>   MODULE_LICENSE("GPL");
>>

-- 
cheers,
-roger