* [PATCH v4 0/2] Add support for TI TPS65219 PMIC GPIO interface.
@ 2023-05-30 7:59 Jerome Neanne
2023-05-30 7:59 ` [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC Jerome Neanne
2023-05-30 8:00 ` [PATCH v4 2/2] mfd: tps65219: Add gpio cell instance Jerome Neanne
0 siblings, 2 replies; 7+ messages in thread
From: Jerome Neanne @ 2023-05-30 7:59 UTC (permalink / raw
To: Linus Walleij, Bartosz Golaszewski, Tony Lindgren, Lee Jones
Cc: khilman, msp, francesco, linux-kernel, linux-gpio, linux-omap,
Jonathan Cormier, Jerome Neanne
Changes in v4:
- andy.shevchenko review:
- Nits:
- move blank line before link in this cover.
- Kconfig indentation.
- Formatting: Comments; indentation; line length 100.
- Do not split string literals.
- Add var *dev = gpio->tps->dev and replace to get shorter blocks.
- Evaluate GPIO0 MULTI_DEVICE_ENABLE only once.
- Explicit include of bits.h
Change to CONFIG_DEBUG_GPIO: debug options from Kconfig for GPIO only.
- Link to v3: https://lore.kernel.org/r/20230511-tps65219-add-gpio-support-v3-0-19837a34d820@baylibre.com
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 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
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
- Link to 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 | 15 ++++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-tps65219.c | 181 +++++++++++++++++++++++++++++++++++++++++++
drivers/mfd/tps65219.c | 2 +-
5 files changed, 199 insertions(+), 1 deletion(-)
---
base-commit: 8ded96e4d9a6da88bdbad61350cc6147b7c0c00c
change-id: 20230511-tps65219-add-gpio-support-322bdb4e0297
Best regards,
--
Jerome Neanne <jneanne@baylibre.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC
2023-05-30 7:59 [PATCH v4 0/2] Add support for TI TPS65219 PMIC GPIO interface Jerome Neanne
@ 2023-05-30 7:59 ` Jerome Neanne
2023-05-30 11:07 ` andy.shevchenko
2023-05-30 11:36 ` Linus Walleij
2023-05-30 8:00 ` [PATCH v4 2/2] mfd: tps65219: Add gpio cell instance Jerome Neanne
1 sibling, 2 replies; 7+ messages in thread
From: Jerome Neanne @ 2023-05-30 7:59 UTC (permalink / raw
To: Linus Walleij, Bartosz Golaszewski, Tony Lindgren, Lee Jones
Cc: khilman, msp, francesco, linux-kernel, linux-gpio, linux-omap,
Jonathan Cormier, 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 | 15 ++++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-tps65219.c | 181 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 198 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..337803b3a022 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1440,6 +1440,21 @@ 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..0c6ba7021a26
--- /dev/null
+++ b/drivers/gpio/gpio-tps65219.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GPIO driver for TI TPS65219 PMICs
+ *
+ * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#include <linux/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>
+
+#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);
+ struct device *dev = gpio->tps->dev;
+ int ret, val;
+
+ if (offset != TPS65219_GPIO0_IDX) {
+ dev_err(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;
+
+ ret = !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK));
+ dev_warn(dev, "GPIO%d = %d, MULTI_DEVICE_ENABLE, not a standard GPIO\n", offset, ret);
+
+ /* 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 ret;
+}
+
+static void tps65219_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+ struct tps65219_gpio *gpio = gpiochip_get_data(gc);
+ struct device *dev = gpio->tps->dev;
+ 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(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);
+ struct device *dev = gpio->tps->dev;
+
+ /*
+ * 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(CONFIG_DEBUG_GPIO)) {
+ int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG,
+ TPS65219_GPIO0_DIR_MASK, direction);
+ if (ret) {
+ dev_err(dev,
+ "GPIO DEBUG enabled: Fail to change direction to %u for GPIO%d.\n",
+ direction, offset);
+ return ret;
+ }
+ }
+
+ dev_err(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);
+ struct device *dev = gpio->tps->dev;
+
+ if (offset != TPS65219_GPIO0_IDX) {
+ dev_err(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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4 2/2] mfd: tps65219: Add gpio cell instance
2023-05-30 7:59 [PATCH v4 0/2] Add support for TI TPS65219 PMIC GPIO interface Jerome Neanne
2023-05-30 7:59 ` [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC Jerome Neanne
@ 2023-05-30 8:00 ` Jerome Neanne
1 sibling, 0 replies; 7+ messages in thread
From: Jerome Neanne @ 2023-05-30 8:00 UTC (permalink / raw
To: Linus Walleij, Bartosz Golaszewski, Tony Lindgren, Lee Jones
Cc: khilman, msp, francesco, linux-kernel, linux-gpio, linux-omap,
Jonathan Cormier, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC
2023-05-30 7:59 ` [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC Jerome Neanne
@ 2023-05-30 11:07 ` andy.shevchenko
2023-06-06 12:45 ` jerome Neanne
2023-05-30 11:36 ` Linus Walleij
1 sibling, 1 reply; 7+ messages in thread
From: andy.shevchenko @ 2023-05-30 11:07 UTC (permalink / raw
To: Jerome Neanne
Cc: Linus Walleij, Bartosz Golaszewski, Tony Lindgren, Lee Jones,
khilman, msp, francesco, linux-kernel, linux-gpio, linux-omap,
Jonathan Cormier
Tue, May 30, 2023 at 09:59:59AM +0200, Jerome Neanne kirjoitti:
First of all, I have a bit of déjà vu that I have given already some comments
that left neither answered nor addressed.
> 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
Can you convert this to be a Datasheet tag? Currently even Link is *not* a tag
because there must be no blank lines in the tag block.
> 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 driver can also be built as a module.
> + If so, the module will be called gpio_tps65219.
Random indentation. Can you use as much room as available on each line, please?
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPIO driver for TI TPS65219 PMICs
> + *
> + * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +#include <linux/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);
> + struct device *dev = gpio->tps->dev;
> + int ret, val;
> +
> + if (offset != TPS65219_GPIO0_IDX) {
> + dev_err(dev, "GPIO%d is output only, cannot get\n", offset);
> + return -EOPNOTSUPP;
This seems blind following the checkpatch false warning. The checkpatch does
not know about subsystem details, i.e. GPIOLIB uses ENOTSUPP in the callbacks.
The userspace won't see that as GPIOLIB takes care of translating it when
needed.
> + }
> +
> + ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_CTRL, &val);
> + if (ret)
> + return ret;
> +
> + ret = !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK));
> + dev_warn(dev, "GPIO%d = %d, MULTI_DEVICE_ENABLE, not a standard GPIO\n", offset, ret);
> +
> + /* depends on NVM config return error if dir output else the GPIO0 status bit */
/*
* Depending on NVM config, return an error if direction is output,
* otherwise the GPIO0 status bit.
*/
> + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT)
> + return -EOPNOTSUPP;
Fix the error code.
> + return ret;
> +}
...
> +static int tps65219_gpio_change_direction(struct gpio_chip *gc, unsigned int offset,
> + unsigned int direction)
> +{
> + struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> + struct device *dev = gpio->tps->dev;
> +
> + /*
> + * 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(CONFIG_DEBUG_GPIO)) {
> + int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG,
> + TPS65219_GPIO0_DIR_MASK, direction);
> + if (ret) {
> + dev_err(dev,
> + "GPIO DEBUG enabled: Fail to change direction to %u for GPIO%d.\n",
> + direction, offset);
> + return ret;
> + }
> + }
> +
> + dev_err(dev,
> + "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
> + offset, direction);
> +
> + return -EOPNOTSUPP;
Ditto.
> +}
...
> +static int tps65219_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> + struct device *dev = gpio->tps->dev;
> +
> + if (offset != TPS65219_GPIO0_IDX) {
> + dev_err(dev, "GPIO%d is output only, cannot change to input\n", offset);
> + return -EOPNOTSUPP;
Ditto.
> + }
> +
> + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_IN)
> + return 0;
> +
> + return tps65219_gpio_change_direction(gc, offset, TPS65219_GPIO_DIR_IN);
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC
2023-05-30 7:59 ` [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC Jerome Neanne
2023-05-30 11:07 ` andy.shevchenko
@ 2023-05-30 11:36 ` Linus Walleij
1 sibling, 0 replies; 7+ messages in thread
From: Linus Walleij @ 2023-05-30 11:36 UTC (permalink / raw
To: Jerome Neanne
Cc: Bartosz Golaszewski, Tony Lindgren, Lee Jones, khilman, msp,
francesco, linux-kernel, linux-gpio, linux-omap, Jonathan Cormier
On Tue, May 30, 2023 at 10:00 AM 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>
I agree with Andy's review comments, so fix these.
Once those are fixed you can add my:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC
2023-05-30 11:07 ` andy.shevchenko
@ 2023-06-06 12:45 ` jerome Neanne
2023-06-06 14:36 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: jerome Neanne @ 2023-06-06 12:45 UTC (permalink / raw
To: andy.shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, Tony Lindgren, Lee Jones,
khilman, msp, francesco, linux-kernel, linux-gpio, linux-omap,
Jonathan Cormier
On 30/05/2023 13:07, andy.shevchenko@gmail.com wrote:
> Tue, May 30, 2023 at 09:59:59AM +0200, Jerome Neanne kirjoitti:
>
> First of all, I have a bit of déjà vu that I have given already some comments
> that left neither answered nor addressed.
Sorry for that. I did not realized that some comments on the cover
letter also apply to commit message.
>
>> 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
>
> Can you convert this to be a Datasheet tag? Currently even Link is *not* a tag
> because there must be no blank lines in the tag block.
>
>> Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
>> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
>> Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
>
I misinterpreted this comment. I looked at wrong examples but I think I
understand now that the right usage is to have all the tags grouped
together into one block which is delimited by blank lines before and
after the whole block.
I'll then do this and put all the Datasheet/Link into the tag block.
Stop putting Links inside the commit message right after I refer to it.
https://www.kernel.org/doc/html/latest/process/5.Posting.html#patch-formatting-and-changelogs
> ...
>
>> + 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.
>
> Random indentation. Can you use as much room as available on each line, please?
Sure for next iteration, I choosed 80 columns here to stay consistent
with other configs. I kept a carriage return after the first sentence
like it is done for other descriptions.
This driver can also be built as a module... is separated with a blank
line as it is done in all other configs.
For all the other lines, I now keep the same line until last word
strictly exceed column 80.
>
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * GPIO driver for TI TPS65219 PMICs
>> + *
>> + * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/
>> + */
>> +
>> +#include <linux/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);
>> + struct device *dev = gpio->tps->dev;
>> + int ret, val;
>> +
>> + if (offset != TPS65219_GPIO0_IDX) {
>> + dev_err(dev, "GPIO%d is output only, cannot get\n", offset);
>
>> + return -EOPNOTSUPP;
>
> This seems blind following the checkpatch false warning. The checkpatch does
> not know about subsystem details, i.e. GPIOLIB uses ENOTSUPP in the callbacks.
> The userspace won't see that as GPIOLIB takes care of translating it when
> needed.
>
Thanks for explaining, I'm often in trouble for choosing the error code.
I'll replace here and all other places where it's used with EOPNOTSUPP.
Regards,
Jerome
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC
2023-06-06 12:45 ` jerome Neanne
@ 2023-06-06 14:36 ` Andy Shevchenko
0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2023-06-06 14:36 UTC (permalink / raw
To: jerome Neanne
Cc: Linus Walleij, Bartosz Golaszewski, Tony Lindgren, Lee Jones,
khilman, msp, francesco, linux-kernel, linux-gpio, linux-omap,
Jonathan Cormier
On Tue, Jun 6, 2023 at 3:45 PM jerome Neanne <jneanne@baylibre.com> wrote:
> On 30/05/2023 13:07, andy.shevchenko@gmail.com wrote:
> > Tue, May 30, 2023 at 09:59:59AM +0200, Jerome Neanne kirjoitti:
...
> >> Datasheet describes specific usage for non standard GPIO.
> >> Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf
> >
> > Can you convert this to be a Datasheet tag? Currently even Link is *not* a tag
> > because there must be no blank lines in the tag block.
> >
> >> Co-developed-by: Jonathan Cormier <jcormier@criticallink.com>
> >> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> >> Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
> >
> I misinterpreted this comment. I looked at wrong examples but I think I
> understand now that the right usage is to have all the tags grouped
> together into one block which is delimited by blank lines before and
> after the whole block.
(Note, there is no "after", tag block is the last in the commit message)
> I'll then do this and put all the Datasheet/Link into the tag block.
> Stop putting Links inside the commit message right after I refer to it.
> https://www.kernel.org/doc/html/latest/process/5.Posting.html#patch-formatting-and-changelogs
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-06 14:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-30 7:59 [PATCH v4 0/2] Add support for TI TPS65219 PMIC GPIO interface Jerome Neanne
2023-05-30 7:59 ` [PATCH v4 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC Jerome Neanne
2023-05-30 11:07 ` andy.shevchenko
2023-06-06 12:45 ` jerome Neanne
2023-06-06 14:36 ` Andy Shevchenko
2023-05-30 11:36 ` Linus Walleij
2023-05-30 8:00 ` [PATCH v4 2/2] mfd: tps65219: Add gpio cell instance Jerome Neanne
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).