[PATCH v2 0/4] gpio: aggregator: Incorporate gpio-delay functionality

[PATCH v2 0/4] gpio: aggregator: Incorporate gpio-delay functionality

[Andy Shevchenko]

The newly appeared gpio-delay module enables external signal delay lines
that may be connected to the GPIOs. But at the same time it copies the
GPIO forwarder functionality. Besides that the approach does not scale.
If we would have another external component, we would need yet another
driver. That's why I think, and seems others support me, better to
enable such a functionality inside GPIO aggregator driver.

Patch 1 is a cleanup that may be applied independently on the decision
about the rest.

Please, test and comment!

In v2:
- split as a series
- covered CONFIG_OF_GPIO=n case
- removed the gpio-delay
- moved gpio-delay Kconfig help to the comment in the code

Not in v2:
- updated DT description to note about long timeouts in atomic mode
- sysfs hiding for gpio-delay functionality
- used msleep(): the documentation is _against_ this

Andy Shevchenko (4):
  gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections
  gpio: aggregator: Support delay for setting up individual GPIOs
  gpio: aggregator: Set up a parser of delay line parameters
  gpio: delay: Remove duplicative functionality

 drivers/gpio/Kconfig           |   9 --
 drivers/gpio/Makefile          |   1 -
 drivers/gpio/gpio-aggregator.c | 109 ++++++++++++++++++++--
 drivers/gpio/gpio-delay.c      | 164 ---------------------------------
 4 files changed, 103 insertions(+), 180 deletions(-)
 delete mode 100644 drivers/gpio/gpio-delay.c

-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 1/4] gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections

[Andy Shevchenko]

They stop the driver being used with ACPI PRP0001 and are something
I want to avoid being cut and paste into new drivers. Also include
mod_devicetable.h as we struct of_device_id is defined in there.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 20a686f12df7..1aa7455672d3 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -12,6 +12,7 @@
 #include <linux/ctype.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/overflow.h>
@@ -500,23 +501,21 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id gpio_aggregator_dt_ids[] = {
 	/*
 	 * Add GPIO-operated devices controlled from userspace below,
-	 * or use "driver_override" in sysfs
+	 * or use "driver_override" in sysfs.
 	 */
 	{}
 };
 MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
-#endif
 
 static struct platform_driver gpio_aggregator_driver = {
 	.probe = gpio_aggregator_probe,
 	.driver = {
 		.name = DRV_NAME,
 		.groups = gpio_aggregator_groups,
-		.of_match_table = of_match_ptr(gpio_aggregator_dt_ids),
+		.of_match_table = gpio_aggregator_dt_ids,
 	},
 };
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 2/4] gpio: aggregator: Support delay for setting up individual GPIOs

[Andy Shevchenko]

In some cases the GPIO may require an additional delay after setting
its value. Add support for that into the GPIO forwarder code.

This will be fully enabled for use in the following changes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 1aa7455672d3..a74a8d86caf3 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -10,6 +10,7 @@
 #include <linux/bitmap.h>
 #include <linux/bitops.h>
 #include <linux/ctype.h>
+#include <linux/delay.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
@@ -240,6 +241,11 @@ static void __exit gpio_aggregator_remove_all(void)
  *  GPIO Forwarder
  */
 
+struct gpiochip_fwd_timing {
+	u32 ramp_up_us;
+	u32 ramp_down_us;
+};
+
 struct gpiochip_fwd {
 	struct gpio_chip chip;
 	struct gpio_desc **descs;
@@ -247,6 +253,7 @@ struct gpiochip_fwd {
 		struct mutex mlock;	/* protects tmp[] if can_sleep */
 		spinlock_t slock;	/* protects tmp[] if !can_sleep */
 	};
+	struct gpiochip_fwd_timing *delay_timings;
 	unsigned long tmp[];		/* values and descs for multiple ops */
 };
 
@@ -331,6 +338,28 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
 	return error;
 }
 
+static void gpio_fwd_delay(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	const struct gpiochip_fwd_timing *delay_timings;
+	struct gpio_desc *desc = fwd->descs[offset];
+	bool is_active_low = gpiod_is_active_low(desc);
+	u32 delay_us;
+
+	delay_timings = &fwd->delay_timings[offset];
+	if ((!is_active_low && value) || (is_active_low && !value))
+		delay_us = delay_timings->ramp_up_us;
+	else
+		delay_us = delay_timings->ramp_down_us;
+	if (!delay_us)
+		return;
+
+	if (chip->can_sleep)
+		fsleep(delay_us);
+	else
+		udelay(delay_us);
+}
+
 static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
@@ -339,6 +368,9 @@ static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
 		gpiod_set_value_cansleep(fwd->descs[offset], value);
 	else
 		gpiod_set_value(fwd->descs[offset], value);
+
+	if (fwd->delay_timings)
+		gpio_fwd_delay(chip, offset, value);
 }
 
 static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 3/4] gpio: aggregator: Set up a parser of delay line parameters

[Andy Shevchenko]

The aggregator mode can also handle properties of the platform,
that do not belong to the GPIO controller itself. One of such
a property is a signal delay line. Set up a parser to support it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 70 +++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index a74a8d86caf3..ed11aa56bc51 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -18,6 +18,7 @@
 #include <linux/mutex.h>
 #include <linux/overflow.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
@@ -423,6 +424,51 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
 	return gpiod_to_irq(fwd->descs[offset]);
 }
 
+#ifdef CONFIG_OF_GPIO
+static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip,
+				       const struct of_phandle_args *gpiospec,
+				       u32 *flags)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	struct gpiochip_fwd_timing *timings;
+	u32 line;
+
+	if (gpiospec->args_count != chip->of_gpio_n_cells)
+		return -EINVAL;
+
+	line = gpiospec->args[0];
+	if (line >= chip->ngpio)
+		return -EINVAL;
+
+	timings = &fwd->delay_timings[line];
+	timings->ramp_up_us = gpiospec->args[1];
+	timings->ramp_down_us = gpiospec->args[2];
+
+	return line;
+}
+
+static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *chip,
+					 struct gpiochip_fwd *fwd)
+{
+	fwd->delay_timings = devm_kcalloc(dev, chip->ngpio,
+					  sizeof(*fwd->delay_timings),
+					  GFP_KERNEL);
+	if (!fwd->delay_timings)
+		return -ENOMEM;
+
+	chip->of_xlate = gpiochip_fwd_delay_of_xlate;
+	chip->of_gpio_n_cells = 3;
+
+	return 0;
+}
+#else
+static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *chip,
+					 struct gpiochip_fwd *fwd)
+{
+	return 0;
+}
+#endif	/* !CONFIG_OF_GPIO */
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -430,6 +476,7 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
  * @descs: Array containing the GPIO descriptors to forward to.
  *         This array must contain @ngpios entries, and must not be deallocated
  *         before the forwarder has been destroyed again.
+ * @delay_line: True if the pins have an external delay line.
  *
  * This function creates a new gpiochip, which forwards all GPIO operations to
  * the passed GPIO descriptors.
@@ -439,7 +486,8 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
  */
 static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 						unsigned int ngpios,
-						struct gpio_desc *descs[])
+						struct gpio_desc *descs[],
+						bool delay_line)
 {
 	const char *label = dev_name(dev);
 	struct gpiochip_fwd *fwd;
@@ -492,6 +540,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	else
 		spin_lock_init(&fwd->slock);
 
+	if (delay_line) {
+		error = gpiochip_fwd_setup_delay_line(dev, chip, fwd);
+		if (error)
+			return ERR_PTR(error);
+	}
+
 	error = devm_gpiochip_add_data(dev, chip, fwd);
 	if (error)
 		return ERR_PTR(error);
@@ -509,6 +563,7 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct gpio_desc **descs;
 	struct gpiochip_fwd *fwd;
+	bool delay_line;
 	int i, n;
 
 	n = gpiod_count(dev, NULL);
@@ -525,7 +580,9 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 			return PTR_ERR(descs[i]);
 	}
 
-	fwd = gpiochip_fwd_create(dev, n, descs);
+	delay_line = fwnode_device_is_compatible(dev_fwnode(dev), "gpio-delay");
+
+	fwd = gpiochip_fwd_create(dev, n, descs, delay_line);
 	if (IS_ERR(fwd))
 		return PTR_ERR(fwd);
 
@@ -534,6 +591,15 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id gpio_aggregator_dt_ids[] = {
+	/*
+	 * The GPIO delay provides a way to configure platform specific delays
+	 * for GPIO ramp-up or ramp-down delays. This can serve the following
+	 * purposes:
+	 * - Open-drain output using an RC filter
+	 */
+	{
+		.compatible = "gpio-delay",
+	},
 	/*
 	 * Add GPIO-operated devices controlled from userspace below,
 	 * or use "driver_override" in sysfs.
-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 4/4] gpio: delay: Remove duplicative functionality

[Andy Shevchenko]

Now that GPIO aggregator supports a delay line, drop the duplicative
functionality, i.e. the entire gpio-delay driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/Kconfig      |   9 ---
 drivers/gpio/Makefile     |   1 -
 drivers/gpio/gpio-delay.c | 164 --------------------------------------
 3 files changed, 174 deletions(-)
 delete mode 100644 drivers/gpio/gpio-delay.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 209738ef1446..abaae68c88a4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1748,15 +1748,6 @@ config GPIO_AGGREGATOR
 	      industrial control context, to be operated from userspace using
 	      the GPIO chardev interface.
 
-config GPIO_DELAY
-	tristate "GPIO delay"
-	depends on OF_GPIO
-	help
-	  Say yes here to enable the GPIO delay, which provides a way to
-	  configure platform specific delays for GPIO ramp-up or ramp-down
-	  delays. This can serve the following purposes:
-	    - Open-drain output using an RC filter
-
 config GPIO_LATCH
 	tristate "GPIO latch driver"
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 947c9cf9aba8..7843b16f5d59 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -52,7 +52,6 @@ obj-$(CONFIG_GPIO_DA9052)		+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)		+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)		+= gpio-davinci.o
 obj-$(CONFIG_GPIO_DLN2)			+= gpio-dln2.o
-obj-$(CONFIG_GPIO_DELAY)		+= gpio-delay.o
 obj-$(CONFIG_GPIO_DWAPB)		+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EIC_SPRD)		+= gpio-eic-sprd.o
 obj-$(CONFIG_GPIO_ELKHARTLAKE)		+= gpio-elkhartlake.o
diff --git a/drivers/gpio/gpio-delay.c b/drivers/gpio/gpio-delay.c
deleted file mode 100644
index b489b561b225..000000000000
--- a/drivers/gpio/gpio-delay.c
+++ /dev/null
@@ -1,164 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright 2022 TQ-Systems GmbH
- * Author: Alexander Stein <linux@ew.tq-group.com>
- */
-
-#include <linux/err.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/driver.h>
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
-#include <linux/platform_device.h>
-#include <linux/delay.h>
-
-#include "gpiolib.h"
-
-struct gpio_delay_timing {
-	unsigned long ramp_up_delay_us;
-	unsigned long ramp_down_delay_us;
-};
-
-struct gpio_delay_priv {
-	struct gpio_chip gc;
-	struct gpio_descs *input_gpio;
-	struct gpio_delay_timing *delay_timings;
-};
-
-static int gpio_delay_get_direction(struct gpio_chip *gc, unsigned int offset)
-{
-	return GPIO_LINE_DIRECTION_OUT;
-}
-
-static void gpio_delay_set(struct gpio_chip *gc, unsigned int offset, int val)
-{
-	struct gpio_delay_priv *priv = gpiochip_get_data(gc);
-	struct gpio_desc *gpio_desc = priv->input_gpio->desc[offset];
-	const struct gpio_delay_timing *delay_timings;
-	bool ramp_up;
-
-	gpiod_set_value(gpio_desc, val);
-
-	delay_timings = &priv->delay_timings[offset];
-	ramp_up = (!gpiod_is_active_low(gpio_desc) && val) ||
-		  (gpiod_is_active_low(gpio_desc) && !val);
-
-	if (ramp_up && delay_timings->ramp_up_delay_us)
-		udelay(delay_timings->ramp_up_delay_us);
-	if (!ramp_up && delay_timings->ramp_down_delay_us)
-		udelay(delay_timings->ramp_down_delay_us);
-}
-
-static void gpio_delay_set_can_sleep(struct gpio_chip *gc, unsigned int offset, int val)
-{
-	struct gpio_delay_priv *priv = gpiochip_get_data(gc);
-	struct gpio_desc *gpio_desc = priv->input_gpio->desc[offset];
-	const struct gpio_delay_timing *delay_timings;
-	bool ramp_up;
-
-	gpiod_set_value_cansleep(gpio_desc, val);
-
-	delay_timings = &priv->delay_timings[offset];
-	ramp_up = (!gpiod_is_active_low(gpio_desc) && val) ||
-		  (gpiod_is_active_low(gpio_desc) && !val);
-
-	if (ramp_up && delay_timings->ramp_up_delay_us)
-		fsleep(delay_timings->ramp_up_delay_us);
-	if (!ramp_up && delay_timings->ramp_down_delay_us)
-		fsleep(delay_timings->ramp_down_delay_us);
-}
-
-static int gpio_delay_of_xlate(struct gpio_chip *gc,
-			       const struct of_phandle_args *gpiospec,
-			       u32 *flags)
-{
-	struct gpio_delay_priv *priv = gpiochip_get_data(gc);
-	struct gpio_delay_timing *timings;
-	u32 line;
-
-	if (gpiospec->args_count != gc->of_gpio_n_cells)
-		return -EINVAL;
-
-	line = gpiospec->args[0];
-	if (line >= gc->ngpio)
-		return -EINVAL;
-
-	timings = &priv->delay_timings[line];
-	timings->ramp_up_delay_us = gpiospec->args[1];
-	timings->ramp_down_delay_us = gpiospec->args[2];
-
-	return line;
-}
-
-static bool gpio_delay_can_sleep(const struct gpio_delay_priv *priv)
-{
-	int i;
-
-	for (i = 0; i < priv->input_gpio->ndescs; i++)
-		if (gpiod_cansleep(priv->input_gpio->desc[i]))
-			return true;
-
-	return false;
-}
-
-static int gpio_delay_probe(struct platform_device *pdev)
-{
-	struct gpio_delay_priv *priv;
-
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	priv->input_gpio = devm_gpiod_get_array(&pdev->dev, NULL, GPIOD_OUT_LOW);
-	if (IS_ERR(priv->input_gpio))
-		return dev_err_probe(&pdev->dev, PTR_ERR(priv->input_gpio),
-				     "Failed to get input-gpios");
-
-	priv->delay_timings = devm_kcalloc(&pdev->dev,
-					   priv->input_gpio->ndescs,
-					   sizeof(*priv->delay_timings),
-					   GFP_KERNEL);
-	if (!priv->delay_timings)
-		return -ENOMEM;
-
-	if (gpio_delay_can_sleep(priv)) {
-		priv->gc.can_sleep = true;
-		priv->gc.set = gpio_delay_set_can_sleep;
-	} else {
-		priv->gc.can_sleep = false;
-		priv->gc.set = gpio_delay_set;
-	}
-
-	priv->gc.get_direction = gpio_delay_get_direction;
-	priv->gc.of_xlate = gpio_delay_of_xlate;
-	priv->gc.of_gpio_n_cells = 3;
-	priv->gc.ngpio = priv->input_gpio->ndescs;
-	priv->gc.owner = THIS_MODULE;
-	priv->gc.base = -1;
-	priv->gc.parent = &pdev->dev;
-
-	platform_set_drvdata(pdev, priv);
-
-	return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
-}
-
-static const struct of_device_id gpio_delay_ids[] = {
-	{
-		.compatible = "gpio-delay",
-	},
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, gpio_delay_ids);
-
-static struct platform_driver gpio_delay_driver = {
-	.driver	= {
-		.name		= "gpio-delay",
-		.of_match_table	= gpio_delay_ids,
-	},
-	.probe	= gpio_delay_probe,
-};
-module_platform_driver(gpio_delay_driver);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Alexander Stein <alexander.stein@ew.tq-group.com>");
-MODULE_DESCRIPTION("GPIO delay driver");
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 1/4] gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections

[Linus Walleij]

On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> They stop the driver being used with ACPI PRP0001 and are something
> I want to avoid being cut and paste into new drivers. Also include
> mod_devicetable.h as we struct of_device_id is defined in there.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

Yours,
Linus Walleij

Re: [PATCH v2 2/4] gpio: aggregator: Support delay for setting up individual GPIOs

[Linus Walleij]

On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> In some cases the GPIO may require an additional delay after setting
> its value. Add support for that into the GPIO forwarder code.
>
> This will be fully enabled for use in the following changes.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

Yours,
Linus Walleij

Re: [PATCH v2 3/4] gpio: aggregator: Set up a parser of delay line parameters

[Linus Walleij]

On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The aggregator mode can also handle properties of the platform,
> that do not belong to the GPIO controller itself. One of such
> a property is a signal delay line. Set up a parser to support it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I guess this is one of those instances where we actually need some OF-specific
code for parsing the special case, and other HW descriptions will need
other special code. It's fine for non-trivial stuff I think.

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

Yours,
Linus Walleij

Re: [PATCH v2 4/4] gpio: delay: Remove duplicative functionality

[Linus Walleij]

On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Now that GPIO aggregator supports a delay line, drop the duplicative
> functionality, i.e. the entire gpio-delay driver.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This is the most pleasing technical solution for sure.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 1/4] gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections

[Geert Uytterhoeven]

On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> They stop the driver being used with ACPI PRP0001 and are something
> I want to avoid being cut and paste into new drivers. Also include
> mod_devicetable.h as we struct of_device_id is defined in there.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/4] gpio: aggregator: Support delay for setting up individual GPIOs

[Geert Uytterhoeven]

On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> In some cases the GPIO may require an additional delay after setting
> its value. Add support for that into the GPIO forwarder code.
>
> This will be fully enabled for use in the following changes.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] gpio: aggregator: Set up a parser of delay line parameters

[Geert Uytterhoeven]

Hi Andy,

On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> The aggregator mode can also handle properties of the platform,
> that do not belong to the GPIO controller itself. One of such
> a property is a signal delay line. Set up a parser to support it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

One suggestion for improvement below...

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -525,7 +580,9 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
>                         return PTR_ERR(descs[i]);
>         }
>
> -       fwd = gpiochip_fwd_create(dev, n, descs);
> +       delay_line = fwnode_device_is_compatible(dev_fwnode(dev), "gpio-delay");

Please do not use explicit checks for compatible values in .probe()
methods.  Instead, use device_get_match_data() to get the feature
flag(s).  This will also make it easier to scale to other external
components later.

> +
> +       fwd = gpiochip_fwd_create(dev, n, descs, delay_line);
>         if (IS_ERR(fwd))
>                 return PTR_ERR(fwd);
>
> @@ -534,6 +591,15 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
>  }
>
>  static const struct of_device_id gpio_aggregator_dt_ids[] = {
> +       /*
> +        * The GPIO delay provides a way to configure platform specific delays
> +        * for GPIO ramp-up or ramp-down delays. This can serve the following
> +        * purposes:
> +        * - Open-drain output using an RC filter
> +        */
> +       {
> +               .compatible = "gpio-delay",

.data = (void *)FWD_FEATURE_DELAY,

> +       },
>         /*
>          * Add GPIO-operated devices controlled from userspace below,
>          * or use "driver_override" in sysfs.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/4] gpio: delay: Remove duplicative functionality

[Geert Uytterhoeven]

On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Now that GPIO aggregator supports a delay line, drop the duplicative
> functionality, i.e. the entire gpio-delay driver.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] gpio: aggregator: Set up a parser of delay line parameters

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 9:37 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>
> > The aggregator mode can also handle properties of the platform,
> > that do not belong to the GPIO controller itself. One of such
> > a property is a signal delay line. Set up a parser to support it.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> I guess this is one of those instances where we actually need some OF-specific
> code for parsing the special case, and other HW descriptions will need
> other special code. It's fine for non-trivial stuff I think.

Actually if you look into IIO, the xlate there is fwnode specific
nowadays and theoretically the specifics of OF can be parsed in ACPI
as well. I would like to get rid of OF_GPIO completely, but let's
see...

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

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] gpio: aggregator: Set up a parser of delay line parameters

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 10:48 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > The aggregator mode can also handle properties of the platform,
> > that do not belong to the GPIO controller itself. One of such
> > a property is a signal delay line. Set up a parser to support it.

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you!

> One suggestion for improvement below...

Sure, for v3!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] gpio: aggregator: Incorporate gpio-delay functionality

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 2:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The newly appeared gpio-delay module enables external signal delay lines
> that may be connected to the GPIOs. But at the same time it copies the
> GPIO forwarder functionality. Besides that the approach does not scale.
> If we would have another external component, we would need yet another
> driver. That's why I think, and seems others support me, better to
> enable such a functionality inside GPIO aggregator driver.
>
> Patch 1 is a cleanup that may be applied independently on the decision
> about the rest.
>
> Please, test and comment!

Alexander, I have slightly changed the code, can you test this and
give your formal Tested-by tag?

-- 
With Best Regards,
Andy Shevchenko