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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f92085bd-e35e-422a-8aa3-66e624c44586@criticallink.com>
Date: Mon, 28 Apr 2025 12:41:07 -0400
From: Jonathan Cormier <jcormier@...ticallink.com>
To: Shree Ramamoorthy <s-ramamoorthy@...com>, aaro.koskinen@....fi,
 andreas@...nade.info, khilman@...libre.com, rogerq@...nel.org,
 tony@...mide.com, linus.walleij@...aro.org, brgl@...ev.pl,
 linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org, Jerome Neanne <jneanne@...libre.com>
Cc: m-leonard@...com, praneeth@...com, jsava@...ticallink.com,
 Praneeth Bajjuri <praneeth@...com>
Subject: Re: [PATCH v4 3/3] gpio: tps65219: Add support for varying
 gpio/offset values

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@...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.