[PATCH v2 0/2] Add support for TI TPS65219 PMIC GPIO interface.

[PATCH v2 0/2] Add support for TI TPS65219 PMIC GPIO interface.

[Jerome Neanne]

GPIO interface consist in 3 pins:
Two GPIOS are output only: GPO1, GPO2.

GPIO0 is used for multi device support:
- The input-functionality is only used in multi-PMIC configuration
- In single-PMIC, it can be used as an output

The configuration is static and flashed in NVM in factory.
Description tps65219.pdf chapter 7.3.13

Linux must not change MULTI_DEVICE_ENABLE bit at run time.

This was done for test purpose only to check input/output
correct behavior on EVM board (no access to different NVM config).

Tested on k3-am62x-lp-sk board. This board MULTI_DEVICE_ENABLE=0

Despite the register bits are out of order,
driver is remapping in natural order:
GPIO0 is gpiochip line 0
GPO1/2 are gpiochip line 1/2

Initial version by Jon Cormier on TI Mainline.
Ported upstream by Jerome Neanne

PMIC datasheet:
Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf

Changes in v2:
andy.shevchenko review
- Typo and indentation in commit message.
- Clarify Co-developer role.
- Specify name for module.
- Code simplification for tps65219_gpio_set
- Put test code into #if 0 ... #endif to make it easier to re-use
- Formatting for .driver
- remove dupplicated error management => dead code

Previous version:
v1 - https://lore.kernel.org/all/20230224113837.874264-1-jneanne@baylibre.com/

Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
Signed-off-by: Jerome Neanne <jneanne@baylibre.com>

Jerome Neanne (2):
  gpio: tps65219: add GPIO support for TPS65219 PMIC
  mfd: tps65219: Add gpio cell instance

 MAINTAINERS                  |   1 +
 drivers/gpio/Kconfig         |  13 +++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-tps65219.c | 167 +++++++++++++++++++++++++++++++++++
 drivers/mfd/tps65219.c       |   7 +-
 5 files changed, 188 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpio/gpio-tps65219.c

--
2.34.1

---
Jerome Neanne (2):
      gpio: tps65219: add GPIO support for TPS65219 PMIC
      mfd: tps65219: Add gpio cell instance

 MAINTAINERS                  |   1 +
 drivers/gpio/Kconfig         |  17 +++++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-tps65219.c | 173 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/tps65219.c       |   2 +-
 5 files changed, 193 insertions(+), 1 deletion(-)
---
base-commit: 1a5304fecee523060f26e2778d9d8e33c0562df3
change-id: 20230511-tps65219-add-gpio-support-322bdb4e0297

Best regards,
-- 
Jerome Neanne <jneanne@baylibre.com>

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

[Jerome Neanne]

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@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
---
 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>
+
+#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;
+
+	regmap_update_bits(gpio->tps->regmap,
+			   TPS65219_REG_GENERAL_CONFIG,
+			   mask, v);
+}
+
+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
+	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_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;
+	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");

-- 
2.34.1

[PATCH v2 2/2] mfd: tps65219: Add gpio cell instance

[Jerome Neanne]

tps65219 PMIC GPIOs are exposed in a standard way:
gpiodetect
gpiochip0 [tps65219-gpio] (3 lines)

Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
---
 drivers/mfd/tps65219.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c
index 0e402fda206b..e1d090ba4258 100644
--- a/drivers/mfd/tps65219.c
+++ b/drivers/mfd/tps65219.c
@@ -106,7 +106,7 @@ static const struct mfd_cell tps65219_cells[] = {
 		.resources = tps65219_regulator_resources,
 		.num_resources = ARRAY_SIZE(tps65219_regulator_resources),
 	},
-	{ .name = "tps65219-gpios", },
+	{ .name = "tps65219-gpio", },
 };
 
 static const struct mfd_cell tps65219_pwrbutton_cell = {

-- 
2.34.1

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

[Linus Walleij]

On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@baylibre.com> 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@criticallink.com>
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
(...)

This overall looks fine.

> +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
> +       dev_err(gpio->tps->dev,
> +               "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
> +                offset, direction);
> +       return -EOPNOTSUPP;
> +}

Normally people would complain about #if 0 code.

But this is a special case!

I definitely want the code to be in there somehow.

What about:

if (IS_ENABLED(DEBUG))?

If someone enables debug with an explicit -DDEBUG to the compiler
this could be allowed.

Yours,
Linus Walleij

Yours,
Linus Walleij

Re: [PATCH v2 2/2] mfd: tps65219: Add gpio cell instance

[Linus Walleij]

On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@baylibre.com> wrote:

> tps65219 PMIC GPIOs are exposed in a standard way:
> gpiodetect
> gpiochip0 [tps65219-gpio] (3 lines)

Write something in the commit message about that the cell
has the wrong name, because all you do is change it from
plural to singular.

> Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> Signed-off-by: Jerome Neanne <jneanne@baylibre.com>

With that:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[jerome Neanne]

On 11/05/2023 22:57, Linus Walleij wrote:
>> +       /* 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
>> +       dev_err(gpio->tps->dev,
>> +               "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
>> +                offset, direction);
>> +       return -EOPNOTSUPP;
>> +}
> 
> Normally people would complain about #if 0 code.
> 
> But this is a special case!
> 
> I definitely want the code to be in there somehow.
> 
> What about:
> 
> if (IS_ENABLED(DEBUG))?
> 
> If someone enables debug with an explicit -DDEBUG to the compiler
> this could be allowed.
I'm fine with your proposal. Will wait few days just in case anyone 
wants to add any comment then go for this.

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

[Bartosz Golaszewski]

On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@baylibre.com> 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@criticallink.com>
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
> ---
>  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@criticallink.com>");
> +MODULE_DESCRIPTION("TPS65219 GPIO driver");
> +MODULE_LICENSE("GPL");
>

Otherwise looks good, just a couple nits.

Bart

> --
> 2.34.1
>

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

[jerome Neanne]

On 15/05/2023 17:36, Bartosz Golaszewski wrote:
>> +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?
I tried a build with W=1 to check for warning I might have missed but 
can't catch any here.
It's done in the exact same way in many other upstream drivers.
Anyway I can remove the const here if you think that could create 
trouble at some point.

Just let me know your recommendation.

Regards,
Jerome

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

[Tony Lindgren]

* jerome Neanne <jneanne@baylibre.com> [230512 07:13]:
> 
> 
> On 11/05/2023 22:57, Linus Walleij wrote:
> > > +       /* 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
> > > +       dev_err(gpio->tps->dev,
> > > +               "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
> > > +                offset, direction);
> > > +       return -EOPNOTSUPP;
> > > +}
> > 
> > Normally people would complain about #if 0 code.
> > 
> > But this is a special case!
> > 
> > I definitely want the code to be in there somehow.
> > 
> > What about:
> > 
> > if (IS_ENABLED(DEBUG))?
> > 
> > If someone enables debug with an explicit -DDEBUG to the compiler
> > this could be allowed.
> I'm fine with your proposal. Will wait few days just in case anyone wants to
> add any comment then go for this.

Just wondering.. Would it help for the driver probe to set gpio0 as a gpio
hog after reading the configured value?

Maybe the multi device enable just means the pin may be shared with no
specific hardware reason to not change it during the runtime?

Regards,

Tony

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

[andy.shevchenko]

Mon, May 15, 2023 at 05:36:46PM +0200, Bartosz Golaszewski kirjoitti:
> On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@baylibre.com> wrote:

...

> > +       gpio->gpio_chip = tps65219_gpio_chip;
> 
> Aren't you getting any warnings here about dropping the 'const' from
> the global structure?

But this is a copy of the contents and not the simple pointer.

-- 
With Best Regards,
Andy Shevchenko

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

[jerome Neanne]

On 20/05/2023 11:44, andy.shevchenko@gmail.com wrote:
> Mon, May 15, 2023 at 05:36:46PM +0200, Bartosz Golaszewski kirjoitti:
>> On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@baylibre.com> wrote:
> 
> ...
> 
>>> +       gpio->gpio_chip = tps65219_gpio_chip;
>>
>> Aren't you getting any warnings here about dropping the 'const' from
>> the global structure?
> 
> But this is a copy of the contents and not the simple pointer.
> 
In many other places where this is done, the struct is declared like:

static const struct gpio_chip template_chip = {

After internal review, I changed this to:

static const struct gpio_chip tps65219_gpio_chip = {

This is because I didn't want to have this "template" that sounds to me 
like "dummy". Maybe I misunderstood and this "template" was used on 
purpose because this const struct is just copied once to initialize
tps65219_gpio->gpio_chip during probe.

Introducing tps65219_gpio_chip name is maybe confusing with 
tps65219_gpio struct.

I think the const should not be a problem here but the naming I used 
might be misleading. If you have a suggestion of what is a good practice 
to make this piece of code clearer. I'll follow your suggestion (use 
template? more_explicit name like ???).

Thanks for your time.

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

[Andy Shevchenko]

On Mon, May 22, 2023 at 10:47 AM jerome Neanne <jneanne@baylibre.com> wrote:
> On 20/05/2023 11:44, andy.shevchenko@gmail.com wrote:
> > Mon, May 15, 2023 at 05:36:46PM +0200, Bartosz Golaszewski kirjoitti:
> >> On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@baylibre.com> wrote:

...

> >>> +       gpio->gpio_chip = tps65219_gpio_chip;
> >>
> >> Aren't you getting any warnings here about dropping the 'const' from
> >> the global structure?
> >
> > But this is a copy of the contents and not the simple pointer.

I commented on Bart's question.

> In many other places where this is done, the struct is declared like:
>
> static const struct gpio_chip template_chip = {
>
> After internal review, I changed this to:
>
> static const struct gpio_chip tps65219_gpio_chip = {
>
> This is because I didn't want to have this "template" that sounds to me
> like "dummy". Maybe I misunderstood and this "template" was used on
> purpose because this const struct is just copied once to initialize
> tps65219_gpio->gpio_chip during probe.
>
> Introducing tps65219_gpio_chip name is maybe confusing with
> tps65219_gpio struct.
>
> I think the const should not be a problem here but the naming I used
> might be misleading. If you have a suggestion of what is a good practice
> to make this piece of code clearer. I'll follow your suggestion (use
> template? more_explicit name like ???).

It's up to Bart.

-- 
With Best Regards,
Andy Shevchenko

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

[jerome Neanne]

On 17/05/2023 08:33, Tony Lindgren wrote:
> * jerome Neanne <jneanne@baylibre.com> [230512 07:13]:
>>
>>
>> On 11/05/2023 22:57, Linus Walleij wrote:
>>>> +       /* 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
>>>> +       dev_err(gpio->tps->dev,
>>>> +               "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
>>>> +                offset, direction);
>>>> +       return -EOPNOTSUPP;
>>>> +}
>>>
>>> Normally people would complain about #if 0 code.
>>>
>>> But this is a special case!
>>>
>>> I definitely want the code to be in there somehow.
>>>
>>> What about:
>>>
>>> if (IS_ENABLED(DEBUG))?
>>>
>>> If someone enables debug with an explicit -DDEBUG to the compiler
>>> this could be allowed.
>> I'm fine with your proposal. Will wait few days just in case anyone wants to
>> add any comment then go for this.
> 
> Just wondering.. Would it help for the driver probe to set gpio0 as a gpio
> hog after reading the configured value?
> 
> Maybe the multi device enable just means the pin may be shared with no
> specific hardware reason to not change it during the runtime?

Your point looks valid. But I think we can't simply add a regular 
"gpio-hog" as a property in the device tree because we don't have a one 
to one mapping for GPIO consumer (theoretically we can have more than 2 
PMICs).

I think your suggestion is to read the multi-device value through regmap 
then "kind of" hog gpio with devm_gpio_request_one
So that gpio0 is preserved from being requested by other user.
Is this correct understanding?

Practically the board I'm using for test currently only have one PMIC.
I'm reluctant to implement additional logic that does not look so 
"conventional" that I can't test.

If maintainers agree, I'll postpone the implementation of your proposal 
until a platform compatible with this implementation is available for 
testing. So that we can check what is most accurate in real life.

Side Note: wo this implementation use of the driver is less optimal in 
multi-device configuration but still usable.

Regards,
Jerome.

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

[jerome Neanne]

On 22/05/2023 13:18, Andy Shevchenko wrote:
> On Mon, May 22, 2023 at 10:47 AM jerome Neanne <jneanne@baylibre.com> wrote:
>> On 20/05/2023 11:44, andy.shevchenko@gmail.com wrote:
>>> Mon, May 15, 2023 at 05:36:46PM +0200, Bartosz Golaszewski kirjoitti:
>>>> On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@baylibre.com> wrote:
> 
> ...
> 
>>>>> +       gpio->gpio_chip = tps65219_gpio_chip;
>>>>
>>>> Aren't you getting any warnings here about dropping the 'const' from
>>>> the global structure?
>>>
>>> But this is a copy of the contents and not the simple pointer.
> 
> I commented on Bart's question.
> 
>> In many other places where this is done, the struct is declared like:
>>
>> static const struct gpio_chip template_chip = {
>>
>> After internal review, I changed this to:
>>
>> static const struct gpio_chip tps65219_gpio_chip = {
>>
>> This is because I didn't want to have this "template" that sounds to me
>> like "dummy". Maybe I misunderstood and this "template" was used on
>> purpose because this const struct is just copied once to initialize
>> tps65219_gpio->gpio_chip during probe.
>>
>> Introducing tps65219_gpio_chip name is maybe confusing with
>> tps65219_gpio struct.
>>
>> I think the const should not be a problem here but the naming I used
>> might be misleading. If you have a suggestion of what is a good practice
>> to make this piece of code clearer. I'll follow your suggestion (use
>> template? more_explicit name like ???).
> 
> It's up to Bart.
> 
Bart, should I keep the code like this or do you suggest a name change 
so that's it's more appealing?

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

[Bartosz Golaszewski]

On Tue, May 23, 2023 at 11:09 AM jerome Neanne <jneanne@baylibre.com> wrote:
>
>
>
> On 22/05/2023 13:18, Andy Shevchenko wrote:
> > On Mon, May 22, 2023 at 10:47 AM jerome Neanne <jneanne@baylibre.com> wrote:
> >> On 20/05/2023 11:44, andy.shevchenko@gmail.com wrote:
> >>> Mon, May 15, 2023 at 05:36:46PM +0200, Bartosz Golaszewski kirjoitti:
> >>>> On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@baylibre.com> wrote:
> >
> > ...
> >
> >>>>> +       gpio->gpio_chip = tps65219_gpio_chip;
> >>>>
> >>>> Aren't you getting any warnings here about dropping the 'const' from
> >>>> the global structure?
> >>>
> >>> But this is a copy of the contents and not the simple pointer.
> >
> > I commented on Bart's question.
> >
> >> In many other places where this is done, the struct is declared like:
> >>
> >> static const struct gpio_chip template_chip = {
> >>
> >> After internal review, I changed this to:
> >>
> >> static const struct gpio_chip tps65219_gpio_chip = {
> >>
> >> This is because I didn't want to have this "template" that sounds to me
> >> like "dummy". Maybe I misunderstood and this "template" was used on
> >> purpose because this const struct is just copied once to initialize
> >> tps65219_gpio->gpio_chip during probe.
> >>
> >> Introducing tps65219_gpio_chip name is maybe confusing with
> >> tps65219_gpio struct.
> >>
> >> I think the const should not be a problem here but the naming I used
> >> might be misleading. If you have a suggestion of what is a good practice
> >> to make this piece of code clearer. I'll follow your suggestion (use
> >> template? more_explicit name like ???).
> >
> > It's up to Bart.
> >
> Bart, should I keep the code like this or do you suggest a name change
> so that's it's more appealing?

Yes, I prefer it to be named something something template for clarity.

tps65219_template_chip would be great.

Bart