Re: [PATCH v2 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@xxxxxxxxxxxx> wrote:
>
> 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
>
> Co-developed-by: Jonathan Cormier <jcormier@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jonathan Cormier <jcormier@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jerome Neanne <jneanne@xxxxxxxxxxxx>
> ---
>  MAINTAINERS                  |   1 +
>  drivers/gpio/Kconfig         |  17 +++++
>  drivers/gpio/Makefile        |   1 +
>  drivers/gpio/gpio-tps65219.c | 173 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 192 insertions(+)
>
> 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..42bbd142e4b6
> --- /dev/null
> +++ b/drivers/gpio/gpio-tps65219.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPIO driver for TI TPS65219 PMICs
> + *
> + * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/tps65219.h>
> +

Please keep all includes together and ordered alphabetically. I see
you probably wanted to split them thematically but we don't really do
this.

> +#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;

Add newline here.

> +       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;

Add newline here.

> +       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;
> +
> +       regmap_update_bits(gpio->tps->regmap,
> +                          TPS65219_REG_GENERAL_CONFIG,
> +                          mask, v);

This can fail - I suggest emitting an error message as regmap won't do it.

> +}
> +
> +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 0
> +       int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG,
> +                                TPS65219_GPIO0_DIR_MASK, direction);
> +       if (ret)
> +               return ret;
> +#endif

Agree with Linus here on enabling it for DEBUG.

And a newline here.

> +       dev_err(gpio->tps->dev,
> +               "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
> +                offset, direction);

And before return.

> +       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;
> +       }

Newline here.

> +       if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_IN)
> +               return 0;

and here.

> +       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;

and here.

> +       return tps65219_gpio_change_direction(gc, offset, TPS65219_GPIO_DIR_OUT);
> +}
> +
> +static const struct gpio_chip tps65219_gpio_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_gpio_chip;

Aren't you getting any warnings here about dropping the 'const' from
the global structure?

> +       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@xxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("TPS65219 GPIO driver");
> +MODULE_LICENSE("GPL");
>

Otherwise looks good, just a couple nits.

Bart

> --
> 2.34.1
>