| Message ID | 20221109065546.24912-5-mranostay@ti.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | mfd: add tps6594x support for Jacinto platforms | expand |
On 09/11/2022 07:55, Matt Ranostay wrote: > Add support for TPS6594X PMICs GPIO interface that has 11 that can be > configured as input or outputs. > > Tested-by: Keerthy <j-keerthy@ti.com> > Signed-off-by: Matt Ranostay <mranostay@ti.com> > --- > drivers/gpio/Kconfig | 7 ++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-tps6594x.c | 142 +++++++++++++++++++++++++++++++++++ > include/linux/mfd/tps6594x.h | 6 ++ > 4 files changed, 156 insertions(+) > create mode 100644 drivers/gpio/gpio-tps6594x.c > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 8c756cb29214..0225e6bddf0a 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -1405,6 +1405,13 @@ config GPIO_TPS65912 > help > This driver supports TPS65912 GPIO chip. > > +config GPIO_TPS6594X > + tristate "TI TPS6594X GPIO driver" > + depends on MFD_TPS6594X Maybe || COMPILE_TEST? > + help > + Select this option to enable GPIO driver for the TPS6954X > + PMIC chip family. There are 11 GPIOs that can be configured. > + (...) > + > +static int tps6594x_gpio_probe(struct platform_device *pdev) > +{ > + struct tps6594x *tps = dev_get_drvdata(pdev->dev.parent); > + struct tps6594x_gpio *gpio; > + > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); and here you did it correctly... I don't get why your two patches differ. > + if (!gpio) > + return -ENOMEM; > + > + gpio->tps = dev_get_drvdata(pdev->dev.parent); > + gpio->gpio_chip = template_chip; > + gpio->gpio_chip.parent = tps->dev; > + > + return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio_chip, gpio); > +} > + > +static const struct of_device_id of_tps6594x_gpio_match[] = { > + { .compatible = "ti,tps6594x-gpio", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_tps6594x_gpio_match); > + > +static struct platform_driver tps6594x_gpio_driver = { > + .driver = { > + .name = "tps6594x-gpio", > + .of_match_table = of_match_ptr(of_tps6594x_gpio_match), Drop of_match_ptr(). It comes with maybe_unused, which you do not have here. Best regards, Krzysztof
On Wed, Nov 9, 2022 at 7:56 AM Matt Ranostay <mranostay@ti.com> wrote: > Add support for TPS6594X PMICs GPIO interface that has 11 that can be > configured as input or outputs. > > Tested-by: Keerthy <j-keerthy@ti.com> > Signed-off-by: Matt Ranostay <mranostay@ti.com> (...) > +config GPIO_TPS6594X > + tristate "TI TPS6594X GPIO driver" > + depends on MFD_TPS6594X > + help > + Select this option to enable GPIO driver for the TPS6954X > + PMIC chip family. There are 11 GPIOs that can be configured. select GPIO_REGMAP This driver is an archetypical example of a driver that can make great use of GPIO_REGMAP helpers, so rewrite it to use them. Look in drivers/gpio/gpio-sl28cpld.c for an example. Yours, Linus Walleij
On Wed, Nov 09, 2022 at 10:59:08AM +0100, Linus Walleij wrote: > On Wed, Nov 9, 2022 at 7:56 AM Matt Ranostay <mranostay@ti.com> wrote: > > > Add support for TPS6594X PMICs GPIO interface that has 11 that can be > > configured as input or outputs. > > > > Tested-by: Keerthy <j-keerthy@ti.com> > > Signed-off-by: Matt Ranostay <mranostay@ti.com> > > (...) > > +config GPIO_TPS6594X > > + tristate "TI TPS6594X GPIO driver" > > + depends on MFD_TPS6594X > > + help > > + Select this option to enable GPIO driver for the TPS6954X > > + PMIC chip family. There are 11 GPIOs that can be configured. > > select GPIO_REGMAP > > This driver is an archetypical example of a driver that can make great > use of GPIO_REGMAP helpers, so rewrite it to use them. > Look in drivers/gpio/gpio-sl28cpld.c for an example. Linus, Those helpers look great for this usecase on the surface but however I think there could be some issues. For GPIO direction it isn't configured by a bitmap on a register(s) but by a bit on a range of registers (with a register for each GPIOx). For set/get values the gpio helper would work though. - Matt > > Yours, > Linus Walleij
On Thu, Nov 10, 2022 at 11:12 AM Matt Ranostay <mranostay@ti.com> wrote: > On Wed, Nov 09, 2022 at 10:59:08AM +0100, Linus Walleij wrote: > > On Wed, Nov 9, 2022 at 7:56 AM Matt Ranostay <mranostay@ti.com> wrote: > > > > > Add support for TPS6594X PMICs GPIO interface that has 11 that can be > > > configured as input or outputs. > > > > > > Tested-by: Keerthy <j-keerthy@ti.com> > > > Signed-off-by: Matt Ranostay <mranostay@ti.com> > > > > (...) > > > +config GPIO_TPS6594X > > > + tristate "TI TPS6594X GPIO driver" > > > + depends on MFD_TPS6594X > > > + help > > > + Select this option to enable GPIO driver for the TPS6954X > > > + PMIC chip family. There are 11 GPIOs that can be configured. > > > > select GPIO_REGMAP > > > > This driver is an archetypical example of a driver that can make great > > use of GPIO_REGMAP helpers, so rewrite it to use them. > > Look in drivers/gpio/gpio-sl28cpld.c for an example. > > Linus, > > Those helpers look great for this usecase on the surface but however I think there could be some issues. > For GPIO direction it isn't configured by a bitmap on a register(s) but by a bit on a range of > registers (with a register for each GPIOx). > > For set/get values the gpio helper would work though. Isn't is possible to just use parts of the GPIO_REGMAP helpers? I thought it's designed like such. Michael Walle will know what to do with your usecase, and whether to use it or not, let's page him! Yours, Linus Walleij
On Thu, Nov 10, 2022 at 11:15:22AM +0100, Linus Walleij wrote: > On Thu, Nov 10, 2022 at 11:12 AM Matt Ranostay <mranostay@ti.com> wrote: > > On Wed, Nov 09, 2022 at 10:59:08AM +0100, Linus Walleij wrote: > > > On Wed, Nov 9, 2022 at 7:56 AM Matt Ranostay <mranostay@ti.com> wrote: > > > > > > > Add support for TPS6594X PMICs GPIO interface that has 11 that can be > > > > configured as input or outputs. > > > > > > > > Tested-by: Keerthy <j-keerthy@ti.com> > > > > Signed-off-by: Matt Ranostay <mranostay@ti.com> > > > > > > (...) > > > > +config GPIO_TPS6594X > > > > + tristate "TI TPS6594X GPIO driver" > > > > + depends on MFD_TPS6594X > > > > + help > > > > + Select this option to enable GPIO driver for the TPS6954X > > > > + PMIC chip family. There are 11 GPIOs that can be configured. > > > > > > select GPIO_REGMAP > > > > > > This driver is an archetypical example of a driver that can make great > > > use of GPIO_REGMAP helpers, so rewrite it to use them. > > > Look in drivers/gpio/gpio-sl28cpld.c for an example. > > > > Linus, > > > > Those helpers look great for this usecase on the surface but however I think there could be some issues. > > For GPIO direction it isn't configured by a bitmap on a register(s) but by a bit on a range of > > registers (with a register for each GPIOx). > > > > For set/get values the gpio helper would work though. > > Isn't is possible to just use parts of the GPIO_REGMAP > helpers? I thought it's designed like such. > > Michael Walle will know what to do with your usecase, and > whether to use it or not, let's page him! > So after looking around a bit and digging into the helper code I found this drivers/pinctrl/bcm/pinctrl-bcm63xx.c which has a example on how to override the reg_mask_xlate function which could be used for changing the stride, and mask based on the base address. Currently have coded up using the gpio regmap helper. Will run through some testing first and then submit for review. - Matt > Yours, > Linus Walleij
Hi, Am 2022-11-11 09:58, schrieb Matt Ranostay: > On Thu, Nov 10, 2022 at 11:15:22AM +0100, Linus Walleij wrote: >> On Thu, Nov 10, 2022 at 11:12 AM Matt Ranostay <mranostay@ti.com> >> wrote: >> > On Wed, Nov 09, 2022 at 10:59:08AM +0100, Linus Walleij wrote: >> > > On Wed, Nov 9, 2022 at 7:56 AM Matt Ranostay <mranostay@ti.com> wrote: >> > > >> > > > Add support for TPS6594X PMICs GPIO interface that has 11 that can be >> > > > configured as input or outputs. >> > > > >> > > > Tested-by: Keerthy <j-keerthy@ti.com> >> > > > Signed-off-by: Matt Ranostay <mranostay@ti.com> >> > > >> > > (...) >> > > > +config GPIO_TPS6594X >> > > > + tristate "TI TPS6594X GPIO driver" >> > > > + depends on MFD_TPS6594X >> > > > + help >> > > > + Select this option to enable GPIO driver for the TPS6954X >> > > > + PMIC chip family. There are 11 GPIOs that can be configured. >> > > >> > > select GPIO_REGMAP >> > > >> > > This driver is an archetypical example of a driver that can make great >> > > use of GPIO_REGMAP helpers, so rewrite it to use them. >> > > Look in drivers/gpio/gpio-sl28cpld.c for an example. >> > >> > Linus, >> > >> > Those helpers look great for this usecase on the surface but however I think there could be some issues. >> > For GPIO direction it isn't configured by a bitmap on a register(s) but by a bit on a range of >> > registers (with a register for each GPIOx). As long as there is only one register to be changed per pin/action, .reg_mask_xlate should work, as you've already found out. >> > For set/get values the gpio helper would work though. >> >> Isn't is possible to just use parts of the GPIO_REGMAP >> helpers? I thought it's designed like such. No, you can't use them as they are kept private, along with the needed struct gpio_regmap. Which was intentional back then as it should be easier to change the implementation or add features without its use being spread all over the gpio drivers. We could change that though - or just add the needed feature to gpio-regmap if it looks generic enough. >> >> Michael Walle will know what to do with your usecase, and >> whether to use it or not, let's page him! >> > > So after looking around a bit and digging into the helper code I found > this > drivers/pinctrl/bcm/pinctrl-bcm63xx.c which has a example on how to > override > the reg_mask_xlate function which could be used for changing the > stride, and mask > based on the base address. > > Currently have coded up using the gpio regmap helper. Will run through > some testing > first and then submit for review. Sounds promising. -michael
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 8c756cb29214..0225e6bddf0a 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1405,6 +1405,13 @@ config GPIO_TPS65912 help This driver supports TPS65912 GPIO chip. +config GPIO_TPS6594X + tristate "TI TPS6594X GPIO driver" + depends on MFD_TPS6594X + help + Select this option to enable GPIO driver for the TPS6954X + PMIC chip family. There are 11 GPIOs that can be configured. + config GPIO_TPS68470 tristate "TPS68470 GPIO" depends on INTEL_SKL_INT3472 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 8629e9eaf79e..e67a9b787b09 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -158,6 +158,7 @@ obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.o obj-$(CONFIG_GPIO_TPS6586X) += gpio-tps6586x.o obj-$(CONFIG_GPIO_TPS65910) += gpio-tps65910.o obj-$(CONFIG_GPIO_TPS65912) += gpio-tps65912.o +obj-$(CONFIG_GPIO_TPS6594X) += gpio-tps6594x.o obj-$(CONFIG_GPIO_TPS68470) += gpio-tps68470.o obj-$(CONFIG_GPIO_TQMX86) += gpio-tqmx86.o obj-$(CONFIG_GPIO_TS4800) += gpio-ts4800.o diff --git a/drivers/gpio/gpio-tps6594x.c b/drivers/gpio/gpio-tps6594x.c new file mode 100644 index 000000000000..f530ac17f73f --- /dev/null +++ b/drivers/gpio/gpio-tps6594x.c @@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * GPIO driver for TI TPS6594x PMICs + * + * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/ + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include <linux/gpio/driver.h> +#include <linux/mfd/tps6594x.h> + +#define GPIO_CFG_MASK BIT(0) +#define NGPIOS_PER_REG 8 + +struct tps6594x_gpio { + struct gpio_chip gpio_chip; + struct tps6594x *tps; +}; + +static int tps6594x_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + struct tps6594x_gpio *gpio = gpiochip_get_data(gc); + int ret, val; + + ret = regmap_read(gpio->tps->regmap, TPS6594X_GPIO1_CONF + offset, &val); + if (ret) + return ret; + + if (val & GPIO_CFG_MASK) + return GPIO_LINE_DIRECTION_OUT; + + return GPIO_LINE_DIRECTION_IN; +} + +static int tps6594x_gpio_direction_input(struct gpio_chip *gc, unsigned int offset) +{ + struct tps6594x_gpio *gpio = gpiochip_get_data(gc); + + return regmap_update_bits(gpio->tps->regmap, TPS6594X_GPIO1_CONF + offset, + GPIO_CFG_MASK, 0); +} + +static int tps6594x_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value) +{ + struct tps6594x_gpio *gpio = gpiochip_get_data(gc); + unsigned int reg = TPS6594X_GPIO_OUT_1, shift = offset; + + if (shift >= NGPIOS_PER_REG) { + reg = TPS6594X_GPIO_OUT_2; + shift -= NGPIOS_PER_REG; + } + + regmap_update_bits(gpio->tps->regmap, reg, BIT(shift), value ? BIT(shift) : 0); + + return regmap_update_bits(gpio->tps->regmap, TPS6594X_GPIO1_CONF + offset, + GPIO_CFG_MASK, GPIO_CFG_MASK); +} + +static int tps6594x_gpio_get(struct gpio_chip *gc, unsigned int offset) +{ + struct tps6594x_gpio *gpio = gpiochip_get_data(gc); + unsigned int reg = TPS6594X_GPIO_IN_1; + int ret, val; + + if (offset >= NGPIOS_PER_REG) { + reg = TPS6594X_GPIO_IN_2; + offset -= NGPIOS_PER_REG; + } + + ret = regmap_read(gpio->tps->regmap, reg, &val); + if (ret) + return ret; + + return !!(val & BIT(offset)); +} + +static void tps6594x_gpio_set(struct gpio_chip *gc, unsigned int offset, + int value) +{ + struct tps6594x_gpio *gpio = gpiochip_get_data(gc); + unsigned int reg = TPS6594X_GPIO_OUT_1; + + if (offset >= NGPIOS_PER_REG) { + reg = TPS6594X_GPIO_OUT_2; + offset -= NGPIOS_PER_REG; + } + + regmap_update_bits(gpio->tps->regmap, reg, BIT(offset), value ? BIT(offset) : 0); +} + +static const struct gpio_chip template_chip = { + .label = "tps6594x-gpio", + .owner = THIS_MODULE, + .get_direction = tps6594x_gpio_get_direction, + .direction_input = tps6594x_gpio_direction_input, + .direction_output = tps6594x_gpio_direction_output, + .get = tps6594x_gpio_get, + .set = tps6594x_gpio_set, + .base = -1, + .ngpio = 11, + .can_sleep = true, +}; + +static int tps6594x_gpio_probe(struct platform_device *pdev) +{ + struct tps6594x *tps = dev_get_drvdata(pdev->dev.parent); + struct tps6594x_gpio *gpio; + + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); + if (!gpio) + return -ENOMEM; + + gpio->tps = dev_get_drvdata(pdev->dev.parent); + gpio->gpio_chip = template_chip; + gpio->gpio_chip.parent = tps->dev; + + return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio_chip, gpio); +} + +static const struct of_device_id of_tps6594x_gpio_match[] = { + { .compatible = "ti,tps6594x-gpio", }, + {}, +}; +MODULE_DEVICE_TABLE(of, of_tps6594x_gpio_match); + +static struct platform_driver tps6594x_gpio_driver = { + .driver = { + .name = "tps6594x-gpio", + .of_match_table = of_match_ptr(of_tps6594x_gpio_match), + }, + .probe = tps6594x_gpio_probe, +}; +module_platform_driver(tps6594x_gpio_driver); + +MODULE_ALIAS("platform:tps6594x-gpio"); +MODULE_AUTHOR("Matt Ranostay <mranostay@ti.com>"); +MODULE_DESCRIPTION("TPS6594X GPIO driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/tps6594x.h b/include/linux/mfd/tps6594x.h index 5a6af0da9223..c2440f5f43d3 100644 --- a/include/linux/mfd/tps6594x.h +++ b/include/linux/mfd/tps6594x.h @@ -21,6 +21,12 @@ #define TPS6594X_FSM_I2C_TRIGGERS 0x85 #define TPS6594X_FSM_NSLEEP_TRIGGERS 0x86 +#define TPS6594X_GPIO1_CONF 0x31 +#define TPS6594X_GPIO_OUT_1 0x3d +#define TPS6594X_GPIO_OUT_2 0x3e +#define TPS6594X_GPIO_IN_1 0x3f +#define TPS6594X_GPIO_IN_2 0x40 + #define TPS6594X_RTC_SECONDS 0xb5 #define TPS6594X_RTC_MINUTES 0xb6 #define TPS6594X_RTC_HOURS 0xb7