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: <683a1c36-0b5a-461f-bc89-3a418f542b57@ti.com>
Date: Tue, 29 Apr 2025 11:42:29 -0500
From: Shree Ramamoorthy <s-ramamoorthy@...com>
To: Jonathan Cormier <jcormier@...ticallink.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>
Subject: Re: [PATCH v4 3/3] gpio: tps65219: Add support for varying
 gpio/offset values

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