Re: [PATCH v4 3/3] gpio: tps65219: Add support for varying gpio/offset values

[Jonathan Cormier <jcormier@criticallink.com>]

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.