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

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

[Jerome Neanne]

Changes in v3:
- Linus Walleij Review:
	- put test code under IS_ENABLED(DEBUG)
- Bartosz Golaszewski Review:
	- nits: alphabetical ordering for includes, newline after
	  return, 
	- Add error message on regmap_update_bits failure.
	- Change variable name in: tps65219_template_chip so that it's
	  clear that gpio_chip get's a copy of this template structure.

- Link to v2: https://lore.kernel.org/r/20230511-tps65219-add-gpio-support-v2-0-60feb64d649a@baylibre.com

Add support for TI TPS65219 PMIC GPIO interface.

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 | 183 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/tps65219.c       |   2 +-
 5 files changed, 203 insertions(+), 1 deletion(-)
---
base-commit: 8ded96e4d9a6da88bdbad61350cc6147b7c0c00c
change-id: 20230511-tps65219-add-gpio-support-322bdb4e0297

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

[PATCH v3 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 | 183 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 202 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..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");

-- 
2.34.1

[PATCH v3 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)

tps65219-gpios is incorrect cell name (plural):
Changed to tps65219-gpio (singular)

Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
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 v3 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC

[andy.shevchenko]

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.

> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko

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

[jerome Neanne]

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.

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko