[PATCH v3 0/5] gpio: aggregator: Incorporate gpio-delay functionality

[PATCH v3 0/5] 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! Alexander, I would appreciate your tag.

In v3:
- added new patch 3 to prevent device removal from sysfs
- switched to feature in driver data instead of "compatible" (Geert)
- applied tags (Geert, Linus)
- left DT bindings untouched, can be amended later on

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
- left udelay() call untouched as recommended by documentation

Andy Shevchenko (5):
  gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections
  gpio: aggregator: Support delay for setting up individual GPIOs
  gpio: aggregator: Prevent collisions between DT and user device IDs
  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 | 113 +++++++++++++++++++++--
 drivers/gpio/gpio-delay.c      | 164 ---------------------------------
 4 files changed, 106 insertions(+), 181 deletions(-)
 delete mode 100644 drivers/gpio/gpio-delay.c

-- 
2.40.0.1.gaa8946217a0b

[PATCH v3 1/5] 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.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
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 v3 2/5] 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.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 1aa7455672d3..4a470dd8b75d 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,27 @@ 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;
+	bool is_active_low = gpiod_is_active_low(fwd->descs[offset]);
+	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 +367,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 v3 3/5] gpio: aggregator: Prevent collisions between DT and user device IDs

[Andy Shevchenko]

In case we have a device instantiated via DT or other means than
via new_device sysfs node, the collision with the latter is possible.
Prevent such collisions by allocating user instantiated devices with
higher IDs, currently set to 1024.

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 4a470dd8b75d..8892cb37ad79 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -26,6 +26,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
 
+#define AGGREGATOR_MIN_DEVID 1024
 #define AGGREGATOR_MAX_GPIOS 512
 
 /*
@@ -135,7 +136,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
 	}
 
 	mutex_lock(&gpio_aggregator_lock);
-	id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
+	id = idr_alloc(&gpio_aggregator_idr, aggr, AGGREGATOR_MIN_DEVID, 0, GFP_KERNEL);
 	mutex_unlock(&gpio_aggregator_lock);
 
 	if (id < 0) {
-- 
2.40.0.1.gaa8946217a0b

[PATCH v3 4/5] 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.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 72 +++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 8892cb37ad79..b944ce9e030e 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,59 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
 	return gpiod_to_irq(fwd->descs[offset]);
 }
 
+/*
+ * The GPIO delay provides a way to configure platform specific delays
+ * for the GPIO ramp-up or ramp-down delays. This can serve the following
+ * purposes:
+ *   - Open-drain output using an RC filter
+ */
+#define FWD_FEATURE_DELAY		BIT(0)
+
+#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 +484,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.
+ * @features: Bitwise ORed features as defined with FWD_FEATURE_*.
  *
  * This function creates a new gpiochip, which forwards all GPIO operations to
  * the passed GPIO descriptors.
@@ -439,7 +494,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[],
+						unsigned long features)
 {
 	const char *label = dev_name(dev);
 	struct gpiochip_fwd *fwd;
@@ -492,6 +548,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	else
 		spin_lock_init(&fwd->slock);
 
+	if (features & FWD_FEATURE_DELAY) {
+		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 +571,7 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct gpio_desc **descs;
 	struct gpiochip_fwd *fwd;
+	unsigned long features;
 	int i, n;
 
 	n = gpiod_count(dev, NULL);
@@ -525,7 +588,8 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 			return PTR_ERR(descs[i]);
 	}
 
-	fwd = gpiochip_fwd_create(dev, n, descs);
+	features = (uintptr_t)device_get_match_data(dev);
+	fwd = gpiochip_fwd_create(dev, n, descs, features);
 	if (IS_ERR(fwd))
 		return PTR_ERR(fwd);
 
@@ -534,6 +598,10 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id gpio_aggregator_dt_ids[] = {
+	{
+		.compatible = "gpio-delay",
+		.data = (void *)FWD_FEATURE_DELAY,
+	},
 	/*
 	 * Add GPIO-operated devices controlled from userspace below,
 	 * or use "driver_override" in sysfs.
-- 
2.40.0.1.gaa8946217a0b

[PATCH v3 5/5] 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.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
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 v3 3/5] gpio: aggregator: Prevent collisions between DT and user device IDs

[Geert Uytterhoeven]

Hi Andy,

Thanks for your patch!

On Thu, Jun 15, 2023 at 3:51 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> In case we have a device instantiated via DT or other means than
> via new_device sysfs node, the collision with the latter is possible.
> Prevent such collisions by allocating user instantiated devices with
> higher IDs, currently set to 1024.

Can you please elaborate? How exactly is this possible?

Aggregators instantiated through sysfs are named "gpio-aggregator.<n>",
and are IDR-based.
Aggregators instantiated from DT are named "<unit-address>.<node-name>".
How can this conflict? When instantiated from ACPI?
What am I missing?

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -26,6 +26,7 @@
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/machine.h>
>
> +#define AGGREGATOR_MIN_DEVID 1024
>  #define AGGREGATOR_MAX_GPIOS 512
>
>  /*
> @@ -135,7 +136,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
>         }
>
>         mutex_lock(&gpio_aggregator_lock);
> -       id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> +       id = idr_alloc(&gpio_aggregator_idr, aggr, AGGREGATOR_MIN_DEVID, 0, GFP_KERNEL);

Iff this would solve an issue, it would be only temporarily, until someone
instantiates 1024 aggregators through some other means ;-)

>         mutex_unlock(&gpio_aggregator_lock);
>
>         if (id < 0) {

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 v3 3/5] gpio: aggregator: Prevent collisions between DT and user device IDs

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 04:54:14PM +0200, Geert Uytterhoeven wrote:
> On Thu, Jun 15, 2023 at 3:51 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > In case we have a device instantiated via DT or other means than
> > via new_device sysfs node, the collision with the latter is possible.
> > Prevent such collisions by allocating user instantiated devices with
> > higher IDs, currently set to 1024.
> 
> Can you please elaborate? How exactly is this possible?
> 
> Aggregators instantiated through sysfs are named "gpio-aggregator.<n>",
> and are IDR-based.
> Aggregators instantiated from DT are named "<unit-address>.<node-name>".
> How can this conflict? When instantiated from ACPI?
> What am I missing?

Nothing. It's me who misunderstood how OF platform device naming schema works.

So this patch can be discarded as we never will have gpio-delay available for
removal via delete_device sysfs node.

Bart, tell me if you need a new version w/o this patch (but note that b4 can
handle this case with

	b4 -slt -P1,2,4,5 ...

).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/5] gpio: aggregator: Incorporate gpio-delay functionality

[Bartosz Golaszewski]

On Thu, Jun 15, 2023 at 3:51 PM 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 would appreciate your tag.
>
> In v3:
> - added new patch 3 to prevent device removal from sysfs
> - switched to feature in driver data instead of "compatible" (Geert)
> - applied tags (Geert, Linus)
> - left DT bindings untouched, can be amended later on
>
> 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
> - left udelay() call untouched as recommended by documentation
>
> Andy Shevchenko (5):
>   gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections
>   gpio: aggregator: Support delay for setting up individual GPIOs
>   gpio: aggregator: Prevent collisions between DT and user device IDs
>   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 | 113 +++++++++++++++++++++--
>  drivers/gpio/gpio-delay.c      | 164 ---------------------------------
>  4 files changed, 106 insertions(+), 181 deletions(-)
>  delete mode 100644 drivers/gpio/gpio-delay.c
>
> --
> 2.40.0.1.gaa8946217a0b
>

Applied patches 1, 2, 4 and 5. Thanks!

Bart

Re: [PATCH v3 0/5] gpio: aggregator: Incorporate gpio-delay functionality

[Alexander Stein]

Hi Andy,

Am Donnerstag, 15. Juni 2023, 15:20:18 CEST schrieb 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! Alexander, I would appreciate your tag.

This works on my platform:
Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> In v3:
> - added new patch 3 to prevent device removal from sysfs
> - switched to feature in driver data instead of "compatible" (Geert)
> - applied tags (Geert, Linus)
> - left DT bindings untouched, can be amended later on
> 
> 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
> - left udelay() call untouched as recommended by documentation
> 
> Andy Shevchenko (5):
>   gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections
>   gpio: aggregator: Support delay for setting up individual GPIOs
>   gpio: aggregator: Prevent collisions between DT and user device IDs
>   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 | 113 +++++++++++++++++++++--
>  drivers/gpio/gpio-delay.c      | 164 ---------------------------------
>  4 files changed, 106 insertions(+), 181 deletions(-)
>  delete mode 100644 drivers/gpio/gpio-delay.c


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 0/5] gpio: aggregator: Incorporate gpio-delay functionality

[Andy Shevchenko]

On Fri, Jun 16, 2023 at 11:01:17AM +0200, Alexander Stein wrote:
> Am Donnerstag, 15. Juni 2023, 15:20:18 CEST schrieb 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! Alexander, I would appreciate your tag.
> 
> This works on my platform:
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Thank you!

-- 
With Best Regards,
Andy Shevchenko