| Message ID | 20250425203315.71497-4-s-ramamoorthy@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add TI TPS65215 PMIC GPIO Support | expand |
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.
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.
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.
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
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);
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(-)