| Message ID | 20230511-tps65219-add-gpio-support-v3-1-19837a34d820@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for TI TPS65219 PMIC GPIO interface. | expand |
Fri, May 26, 2023 at 05:06:03PM +0200, Jerome Neanne kirjoitti: > Add support for TPS65219 PMICs GPIO interface. > > 3 GPIO pins: > - GPIO0 only is IO but input mode reserved for MULTI_DEVICE_ENABLE usage > - GPIO1 and GPIO2 are Output only and referred as GPO1 and GPO2 in spec > > GPIO0 is statically configured as input or output prior to Linux boot. > it is used for MULTI_DEVICE_ENABLE function. > This setting is statically configured by NVM. > GPIO0 can't be used as a generic GPIO (specification Table 8-34). > It's either a GPO when MULTI_DEVICE_EN=0, > or a GPI when MULTI_DEVICE_EN=1. > Datasheet describes specific usage for non standard GPIO. > Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf > This blank line makes Link: above not to be a tag. Tag block mustn't have blank lines. OTOH, the other text must be delimited by a blank line before the tag block. That said, move this blank line to before Link: line. > Co-developed-by: Jonathan Cormier <jcormier@criticallink.com> > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > Signed-off-by: Jerome Neanne <jneanne@baylibre.com> ... > + help > + Select this option to enable GPIO driver for the TPS65219 > + chip family. > + GPIO0 is statically configured as input or output prior to Linux boot. > + it is used for MULTI_DEVICE_ENABLE function. > + This setting is statically configured by NVM. > + GPIO0 can't be used as a generic GPIO. > + It's either a GPO when MULTI_DEVICE_EN=0, > + or a GPI when MULTI_DEVICE_EN=1. This is strange indentation, we have longer lines, why not using all room available? Btw, seems the commit message itself has the same issue. > + This driver can also be built as a module. If so, the > + module will be called gpio_tps65219. ... Missing bits.h > +#include <linux/gpio/driver.h> > +#include <linux/mfd/tps65219.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> ... > +static int tps65219_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct tps65219_gpio *gpio = gpiochip_get_data(gc); With struct device *dev = gpio->tps->dev; you may make some code blocks shorter. > + int ret, val; > + > + if (offset != TPS65219_GPIO0_IDX) { > + dev_err(gpio->tps->dev, > + "GPIO%d is output only, cannot get\n", > + offset); Like here. > + return -EOPNOTSUPP; > + } > + > + ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_CTRL, &val); > + if (ret) > + return ret; > + > + dev_warn(gpio->tps->dev, > + "GPIO%d = %d, used for MULTI_DEVICE_ENABLE, not as standard GPIO\n", > + offset, !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK))); Isn't it the same... > + /* depends on NVM config return error if dir output else the GPIO0 status bit */ > + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT) > + return -EOPNOTSUPP; > + > + return !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK)); ...as this one? What the point to evaluate it twice? > +} ... > + dev_err(gpio->tps->dev, "GPIO%d, set to value %d failed.\n", offset, value); Yeah, there is an inconsistency between line lengths in different functions. Define for yourself the style 80 or 100 and use it everywhere. ... > + /* Documentation is stating that GPIO0 direction must not be changed in Linux: > + * Table 8-34. MFP_1_CONFIG(3): MULTI_DEVICE_ENABLE, > + * Should only be changed in INITIALIZE state (prior to ON Request). > + * Set statically by NVM, changing direction in application can cause a hang. > + * Below can be used for test purpose only: > + */ > + /* * The style of multi-line comment * is incorrect. See this example. * Besides that, remove unneeded * blank line above. */ > + if (IS_ENABLED(DEBUG)) { > + int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, > + TPS65219_GPIO0_DIR_MASK, direction); > + if (ret) { > + dev_err(gpio->tps->dev, > + "DEBUG enabled: Fail to change direction to %u for GPIO%d. \ > + For test only\n", Do not split string literals on non-\n characters. > + direction, offset); > + return ret; Wrong indentation. > + } > + }
On 26/05/2023 20:15, andy.shevchenko@gmail.com wrote: > ... > > Missing bits.h > >> +#include <linux/gpio/driver.h> >> +#include <linux/mfd/tps65219.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> > Thanks for your review.Just to be sure on this particular point: Your recommendation here it to include explicitly bits.h. I can see BIT_MASK(n) defined in linux/bits.h BIT(n) is defined in vdso/bits.h From what I can see, BIT(n) is broadly used across kernel but BIT_MASK(n) sounds to be the Linux strict way... In current version I'm using BIT(n) macro not BIT_MASK(n). Do you recommend to replace every BIT(n) currently used with BIT_MASK(n)? Sorry for asking dumb questions. Just trying to make sure I correctly/fully understand your feedback... And do it all right for the next iteration. Regards, Jerome.
On Mon, May 29, 2023 at 6:21 PM jerome Neanne <jneanne@baylibre.com> wrote: > On 26/05/2023 20:15, andy.shevchenko@gmail.com wrote: ... > > Missing bits.h > > > > Thanks for your review.Just to be sure on this particular point: > Your recommendation here it to include explicitly bits.h. > > I can see BIT_MASK(n) defined in linux/bits.h > BIT(n) is defined in vdso/bits.h > From what I can see, BIT(n) is broadly used across kernel but > BIT_MASK(n) sounds to be the Linux strict way... > > In current version I'm using BIT(n) macro not BIT_MASK(n). > Do you recommend to replace every BIT(n) currently used with BIT_MASK(n)? The semantics (if you look into implementations of those two) are different. BIT() is for a single word (your case), while BIT_MASK() is for an array of words. * word in case of Linux kernel means element of unsigned long type. > Sorry for asking dumb questions. Just trying to make sure I > correctly/fully understand your feedback... And do it all right for the > next iteration. No problem.
diff --git a/MAINTAINERS b/MAINTAINERS index c0cde28c62c6..d912b7465e84 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15398,6 +15398,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git F: arch/arm/configs/omap2plus_defconfig F: arch/arm/mach-omap2/ F: drivers/bus/ti-sysc.c +F: drivers/gpio/gpio-tps65219.c F: drivers/i2c/busses/i2c-omap.c F: drivers/irqchip/irq-omap-intc.c F: drivers/mfd/*omap*.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 5521f060d58e..f4e37881d01a 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1440,6 +1440,23 @@ config GPIO_TPS65218 Select this option to enable GPIO driver for the TPS65218 chip family. +config GPIO_TPS65219 + tristate "TPS65219 GPIO" + depends on MFD_TPS65219 + default MFD_TPS65219 + help + Select this option to enable GPIO driver for the TPS65219 + chip family. + GPIO0 is statically configured as input or output prior to Linux boot. + it is used for MULTI_DEVICE_ENABLE function. + This setting is statically configured by NVM. + GPIO0 can't be used as a generic GPIO. + It's either a GPO when MULTI_DEVICE_EN=0, + or a GPI when MULTI_DEVICE_EN=1. + + This driver can also be built as a module. If so, the + module will be called gpio_tps65219. + config GPIO_TPS6586X bool "TPS6586X GPIO" depends on MFD_TPS6586X diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 20036af3acb1..7843b16f5d59 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -160,6 +160,7 @@ obj-$(CONFIG_GPIO_TN48M_CPLD) += gpio-tn48m.o obj-$(CONFIG_GPIO_TPIC2810) += gpio-tpic2810.o obj-$(CONFIG_GPIO_TPS65086) += gpio-tps65086.o obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.o +obj-$(CONFIG_GPIO_TPS65219) += gpio-tps65219.o obj-$(CONFIG_GPIO_TPS6586X) += gpio-tps6586x.o obj-$(CONFIG_GPIO_TPS65910) += gpio-tps65910.o obj-$(CONFIG_GPIO_TPS65912) += gpio-tps65912.o diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c new file mode 100644 index 000000000000..34759d3cd476 --- /dev/null +++ b/drivers/gpio/gpio-tps65219.c @@ -0,0 +1,183 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * GPIO driver for TI TPS65219 PMICs + * + * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/ + */ + +#include <linux/gpio/driver.h> +#include <linux/mfd/tps65219.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#define TPS65219_GPIO0_DIR_MASK BIT(3) +#define TPS65219_GPIO0_OFFSET 2 +#define TPS65219_GPIO0_IDX 0 +#define TPS65219_GPIO_DIR_IN 1 +#define TPS65219_GPIO_DIR_OUT 0 + +struct tps65219_gpio { + struct gpio_chip gpio_chip; + struct tps65219 *tps; +}; + +static int tps65219_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + struct tps65219_gpio *gpio = gpiochip_get_data(gc); + int ret, val; + + if (offset != TPS65219_GPIO0_IDX) + return GPIO_LINE_DIRECTION_OUT; + + ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, &val); + if (ret) + return ret; + + return !!(val & TPS65219_GPIO0_DIR_MASK); +} + +static int tps65219_gpio_get(struct gpio_chip *gc, unsigned int offset) +{ + struct tps65219_gpio *gpio = gpiochip_get_data(gc); + int ret, val; + + if (offset != TPS65219_GPIO0_IDX) { + dev_err(gpio->tps->dev, + "GPIO%d is output only, cannot get\n", + offset); + return -EOPNOTSUPP; + } + + ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_CTRL, &val); + if (ret) + return ret; + + dev_warn(gpio->tps->dev, + "GPIO%d = %d, used for MULTI_DEVICE_ENABLE, not as standard GPIO\n", + offset, !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK))); + + /* depends on NVM config return error if dir output else the GPIO0 status bit */ + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT) + return -EOPNOTSUPP; + + return !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK)); +} + +static void tps65219_gpio_set(struct gpio_chip *gc, unsigned int offset, + int value) +{ + struct tps65219_gpio *gpio = gpiochip_get_data(gc); + int v, mask, bit; + + bit = (offset == TPS65219_GPIO0_IDX) ? TPS65219_GPIO0_OFFSET : offset - 1; + + mask = BIT(bit); + v = value ? mask : 0; + + if (regmap_update_bits(gpio->tps->regmap, TPS65219_REG_GENERAL_CONFIG, mask, v)) + dev_err(gpio->tps->dev, "GPIO%d, set to value %d failed.\n", offset, value); +} + +static int tps65219_gpio_change_direction(struct gpio_chip *gc, unsigned int offset, + unsigned int direction) +{ + struct tps65219_gpio *gpio = gpiochip_get_data(gc); + + /* Documentation is stating that GPIO0 direction must not be changed in Linux: + * Table 8-34. MFP_1_CONFIG(3): MULTI_DEVICE_ENABLE, + * Should only be changed in INITIALIZE state (prior to ON Request). + * Set statically by NVM, changing direction in application can cause a hang. + * Below can be used for test purpose only: + */ + + if (IS_ENABLED(DEBUG)) { + int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, + TPS65219_GPIO0_DIR_MASK, direction); + if (ret) { + dev_err(gpio->tps->dev, + "DEBUG enabled: Fail to change direction to %u for GPIO%d. \ + For test only\n", + direction, offset); + return ret; + } + } + + dev_err(gpio->tps->dev, + "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n", + offset, direction); + + return -EOPNOTSUPP; +} + +static int tps65219_gpio_direction_input(struct gpio_chip *gc, unsigned int offset) +{ + struct tps65219_gpio *gpio = gpiochip_get_data(gc); + + if (offset != TPS65219_GPIO0_IDX) { + dev_err(gpio->tps->dev, + "GPIO%d is output only, cannot change to input\n", + offset); + return -EOPNOTSUPP; + } + + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_IN) + return 0; + + return tps65219_gpio_change_direction(gc, offset, TPS65219_GPIO_DIR_IN); +} + +static int tps65219_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, + int value) +{ + tps65219_gpio_set(gc, offset, value); + if (offset != TPS65219_GPIO0_IDX) + return 0; + + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT) + return 0; + + return tps65219_gpio_change_direction(gc, offset, TPS65219_GPIO_DIR_OUT); +} + +static const struct gpio_chip tps65219_template_chip = { + .label = "tps65219-gpio", + .owner = THIS_MODULE, + .get_direction = tps65219_gpio_get_direction, + .direction_input = tps65219_gpio_direction_input, + .direction_output = tps65219_gpio_direction_output, + .get = tps65219_gpio_get, + .set = tps65219_gpio_set, + .base = -1, + .ngpio = 3, + .can_sleep = true, +}; + +static int tps65219_gpio_probe(struct platform_device *pdev) +{ + struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent); + struct tps65219_gpio *gpio; + + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); + if (!gpio) + return -ENOMEM; + + gpio->tps = tps; + gpio->gpio_chip = tps65219_template_chip; + gpio->gpio_chip.parent = tps->dev; + + return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio_chip, gpio); +} + +static struct platform_driver tps65219_gpio_driver = { + .driver = { + .name = "tps65219-gpio", + }, + .probe = tps65219_gpio_probe, +}; +module_platform_driver(tps65219_gpio_driver); + +MODULE_ALIAS("platform:tps65219-gpio"); +MODULE_AUTHOR("Jonathan Cormier <jcormier@criticallink.com>"); +MODULE_DESCRIPTION("TPS65219 GPIO driver"); +MODULE_LICENSE("GPL");