[PATCH v2 00/10] Introduce configfs-based interface for gpio-aggregator

[PATCH v2 00/10] Introduce configfs-based interface for gpio-aggregator

[Koichiro Den]

This patch series introduces a configfs-based interface to gpio-aggregator
to address limitations in the existing 'new_device' interface.

The existing 'new_device' interface has several limitations:

  #1. No way to determine when GPIO aggregator creation is complete.
  #2. No way to retrieve errors when creating a GPIO aggregator.
  #3. No way to trace a GPIO line of an aggregator back to its
      corresponding physical device.
  #4. The 'new_device' echo does not indicate which virtual gpiochip<N>
      was created.
  #5. No way to assign names to GPIO lines exported through an aggregator.

Although Issue#1 to #3 could technically be resolved easily without
configfs, using configfs offers a streamlined, modern, and extensible
approach, especially since gpio-sim and gpio-virtuser already utilize
configfs.

This v2 patch series includes 10 patches:

  Patch#1:   Preparation for Patch#2.
  Patch#2:   Introduce minimal configfs interface.
             Addresses Issue#1 to Issue#3.
  Patch#3:   Addresses Issue#4.
  Patch#4:   Preparation for Patch#5.
  Patch#5:   Addresses Issue#5.
  Patch#6-7: Prepare for Patch#8.
  Patch#8:   Expose devices create with sysfs to configfs.
  Patch#9:   Suppress deferred probe for purely configfs-based aggregators.
  Patch#10:  Documentation.

Changes for v2:
  - Addressed feedback from Bartosz:
    * Expose devices created with sysfs to configfs.
    * Drop 'num_lines' attribute.
    * Fix bugs and crashes.
    * Organize internal symbol prefixes more cleanly.
  - Split diffs for improved reviewability.
  - Update kernel doc to reflect the changes.

RFC (v1): https://lore.kernel.org/linux-gpio/20250129155525.663780-1-koichiro.den@canonical.com/T/#u

Koichiro Den (10):
  gpio: aggregator: reorder functions to prepare for configfs
    introduction
  gpio: aggregator: introduce basic configfs interface
  gpio: aggregator: add read-only 'dev_name' configfs attribute
  gpio: aggregator: add read-write 'name' attribute
  gpio: aggregator: expose custom line names to forwarder gpio_chip
  gpio: aggregator: rename 'name' to 'key' in aggr_parse()
  gpio: aggregator: clean up gpio_aggregator_free()
  gpio: aggregator: expoose aggregator created via legacy sysfs to
    configfs
  gpio: aggregator: cancel deferred probe for devices created via
    configfs
  Documentation: gpio: document configfs interface for gpio-aggregator

 .../admin-guide/gpio/gpio-aggregator.rst      |   93 ++
 drivers/gpio/gpio-aggregator.c                | 1194 ++++++++++++++---
 2 files changed, 1103 insertions(+), 184 deletions(-)

-- 
2.45.2

[PATCH v2 01/10] gpio: aggregator: reorder functions to prepare for configfs introduction

[Koichiro Den]

Reorder functions in drivers/gpio/gpio-aggregator.c to prepare for the
configfs-based interface additions in subsequent commits. Arrange the
code so that the configfs implementations will appear above the existing
sysfs-specific code, since the latter will partly depend on the configfs
interface implementations when it starts to expose the settings to
configfs.

The order in drivers/gpio/gpio-aggregator.c will be as follows:

* Basic gpio_aggregator/gpio_aggregator_line representations
* Common utility functions
* GPIO Forwarder implementations
* Configfs interface implementations
* Sysfs interface implementations
* Platform device implementations
* Module init/exit implementations

This separate commit ensures a clean diff for the subsequent commits.

No functional change.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 352 +++++++++++++++++----------------
 1 file changed, 178 insertions(+), 174 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 65f41cc3eafc..570cd1ff8cc2 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -61,180 +61,6 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
 	return 0;
 }
 
-static int aggr_parse(struct gpio_aggregator *aggr)
-{
-	char *args = skip_spaces(aggr->args);
-	char *name, *offsets, *p;
-	unsigned int i, n = 0;
-	int error = 0;
-
-	unsigned long *bitmap __free(bitmap) =
-			bitmap_alloc(AGGREGATOR_MAX_GPIOS, GFP_KERNEL);
-	if (!bitmap)
-		return -ENOMEM;
-
-	args = next_arg(args, &name, &p);
-	while (*args) {
-		args = next_arg(args, &offsets, &p);
-
-		p = get_options(offsets, 0, &error);
-		if (error == 0 || *p) {
-			/* Named GPIO line */
-			error = aggr_add_gpio(aggr, name, U16_MAX, &n);
-			if (error)
-				return error;
-
-			name = offsets;
-			continue;
-		}
-
-		/* GPIO chip + offset(s) */
-		error = bitmap_parselist(offsets, bitmap, AGGREGATOR_MAX_GPIOS);
-		if (error) {
-			pr_err("Cannot parse %s: %d\n", offsets, error);
-			return error;
-		}
-
-		for_each_set_bit(i, bitmap, AGGREGATOR_MAX_GPIOS) {
-			error = aggr_add_gpio(aggr, name, i, &n);
-			if (error)
-				return error;
-		}
-
-		args = next_arg(args, &name, &p);
-	}
-
-	if (!n) {
-		pr_err("No GPIOs specified\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static ssize_t new_device_store(struct device_driver *driver, const char *buf,
-				size_t count)
-{
-	struct gpio_aggregator *aggr;
-	struct platform_device *pdev;
-	int res, id;
-
-	/* kernfs guarantees string termination, so count + 1 is safe */
-	aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
-	if (!aggr)
-		return -ENOMEM;
-
-	memcpy(aggr->args, buf, count + 1);
-
-	aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
-				GFP_KERNEL);
-	if (!aggr->lookups) {
-		res = -ENOMEM;
-		goto free_ga;
-	}
-
-	mutex_lock(&gpio_aggregator_lock);
-	id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
-	mutex_unlock(&gpio_aggregator_lock);
-
-	if (id < 0) {
-		res = id;
-		goto free_table;
-	}
-
-	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
-	if (!aggr->lookups->dev_id) {
-		res = -ENOMEM;
-		goto remove_idr;
-	}
-
-	res = aggr_parse(aggr);
-	if (res)
-		goto free_dev_id;
-
-	gpiod_add_lookup_table(aggr->lookups);
-
-	pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
-	if (IS_ERR(pdev)) {
-		res = PTR_ERR(pdev);
-		goto remove_table;
-	}
-
-	aggr->pdev = pdev;
-	return count;
-
-remove_table:
-	gpiod_remove_lookup_table(aggr->lookups);
-free_dev_id:
-	kfree(aggr->lookups->dev_id);
-remove_idr:
-	mutex_lock(&gpio_aggregator_lock);
-	idr_remove(&gpio_aggregator_idr, id);
-	mutex_unlock(&gpio_aggregator_lock);
-free_table:
-	kfree(aggr->lookups);
-free_ga:
-	kfree(aggr);
-	return res;
-}
-
-static DRIVER_ATTR_WO(new_device);
-
-static void gpio_aggregator_free(struct gpio_aggregator *aggr)
-{
-	platform_device_unregister(aggr->pdev);
-	gpiod_remove_lookup_table(aggr->lookups);
-	kfree(aggr->lookups->dev_id);
-	kfree(aggr->lookups);
-	kfree(aggr);
-}
-
-static ssize_t delete_device_store(struct device_driver *driver,
-				   const char *buf, size_t count)
-{
-	struct gpio_aggregator *aggr;
-	unsigned int id;
-	int error;
-
-	if (!str_has_prefix(buf, DRV_NAME "."))
-		return -EINVAL;
-
-	error = kstrtouint(buf + strlen(DRV_NAME "."), 10, &id);
-	if (error)
-		return error;
-
-	mutex_lock(&gpio_aggregator_lock);
-	aggr = idr_remove(&gpio_aggregator_idr, id);
-	mutex_unlock(&gpio_aggregator_lock);
-	if (!aggr)
-		return -ENOENT;
-
-	gpio_aggregator_free(aggr);
-	return count;
-}
-static DRIVER_ATTR_WO(delete_device);
-
-static struct attribute *gpio_aggregator_attrs[] = {
-	&driver_attr_new_device.attr,
-	&driver_attr_delete_device.attr,
-	NULL
-};
-ATTRIBUTE_GROUPS(gpio_aggregator);
-
-static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
-{
-	gpio_aggregator_free(p);
-	return 0;
-}
-
-static void __exit gpio_aggregator_remove_all(void)
-{
-	mutex_lock(&gpio_aggregator_lock);
-	idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
-	idr_destroy(&gpio_aggregator_idr);
-	mutex_unlock(&gpio_aggregator_lock);
-}
-
 
 /*
  *  GPIO Forwarder
@@ -559,6 +385,170 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 }
 
 
+/*
+ * Sysfs interface
+ */
+static int aggr_parse(struct gpio_aggregator *aggr)
+{
+	char *args = skip_spaces(aggr->args);
+	char *name, *offsets, *p;
+	unsigned int i, n = 0;
+	int error = 0;
+
+	unsigned long *bitmap __free(bitmap) =
+			bitmap_alloc(AGGREGATOR_MAX_GPIOS, GFP_KERNEL);
+	if (!bitmap)
+		return -ENOMEM;
+
+	args = next_arg(args, &name, &p);
+	while (*args) {
+		args = next_arg(args, &offsets, &p);
+
+		p = get_options(offsets, 0, &error);
+		if (error == 0 || *p) {
+			/* Named GPIO line */
+			error = aggr_add_gpio(aggr, name, U16_MAX, &n);
+			if (error)
+				return error;
+
+			name = offsets;
+			continue;
+		}
+
+		/* GPIO chip + offset(s) */
+		error = bitmap_parselist(offsets, bitmap, AGGREGATOR_MAX_GPIOS);
+		if (error) {
+			pr_err("Cannot parse %s: %d\n", offsets, error);
+			return error;
+		}
+
+		for_each_set_bit(i, bitmap, AGGREGATOR_MAX_GPIOS) {
+			error = aggr_add_gpio(aggr, name, i, &n);
+			if (error)
+				return error;
+		}
+
+		args = next_arg(args, &name, &p);
+	}
+
+	if (!n) {
+		pr_err("No GPIOs specified\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t new_device_store(struct device_driver *driver, const char *buf,
+				size_t count)
+{
+	struct gpio_aggregator *aggr;
+	struct platform_device *pdev;
+	int res, id;
+
+	/* kernfs guarantees string termination, so count + 1 is safe */
+	aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
+	if (!aggr)
+		return -ENOMEM;
+
+	memcpy(aggr->args, buf, count + 1);
+
+	aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
+				GFP_KERNEL);
+	if (!aggr->lookups) {
+		res = -ENOMEM;
+		goto free_ga;
+	}
+
+	mutex_lock(&gpio_aggregator_lock);
+	id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
+	mutex_unlock(&gpio_aggregator_lock);
+
+	if (id < 0) {
+		res = id;
+		goto free_table;
+	}
+
+	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
+	if (!aggr->lookups->dev_id) {
+		res = -ENOMEM;
+		goto remove_idr;
+	}
+
+	res = aggr_parse(aggr);
+	if (res)
+		goto free_dev_id;
+
+	gpiod_add_lookup_table(aggr->lookups);
+
+	pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
+	if (IS_ERR(pdev)) {
+		res = PTR_ERR(pdev);
+		goto remove_table;
+	}
+
+	aggr->pdev = pdev;
+	return count;
+
+remove_table:
+	gpiod_remove_lookup_table(aggr->lookups);
+free_dev_id:
+	kfree(aggr->lookups->dev_id);
+remove_idr:
+	mutex_lock(&gpio_aggregator_lock);
+	idr_remove(&gpio_aggregator_idr, id);
+	mutex_unlock(&gpio_aggregator_lock);
+free_table:
+	kfree(aggr->lookups);
+free_ga:
+	kfree(aggr);
+	return res;
+}
+
+static DRIVER_ATTR_WO(new_device);
+
+static void gpio_aggregator_free(struct gpio_aggregator *aggr)
+{
+	platform_device_unregister(aggr->pdev);
+	gpiod_remove_lookup_table(aggr->lookups);
+	kfree(aggr->lookups->dev_id);
+	kfree(aggr->lookups);
+	kfree(aggr);
+}
+
+static ssize_t delete_device_store(struct device_driver *driver,
+				   const char *buf, size_t count)
+{
+	struct gpio_aggregator *aggr;
+	unsigned int id;
+	int error;
+
+	if (!str_has_prefix(buf, DRV_NAME "."))
+		return -EINVAL;
+
+	error = kstrtouint(buf + strlen(DRV_NAME "."), 10, &id);
+	if (error)
+		return error;
+
+	mutex_lock(&gpio_aggregator_lock);
+	aggr = idr_remove(&gpio_aggregator_idr, id);
+	mutex_unlock(&gpio_aggregator_lock);
+	if (!aggr)
+		return -ENOENT;
+
+	gpio_aggregator_free(aggr);
+	return count;
+}
+static DRIVER_ATTR_WO(delete_device);
+
+static struct attribute *gpio_aggregator_attrs[] = {
+	&driver_attr_new_device.attr,
+	&driver_attr_delete_device.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(gpio_aggregator);
+
+
 /*
  *  GPIO Aggregator platform device
  */
@@ -616,6 +606,20 @@ static struct platform_driver gpio_aggregator_driver = {
 	},
 };
 
+static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
+{
+	gpio_aggregator_free(p);
+	return 0;
+}
+
+static void __exit gpio_aggregator_remove_all(void)
+{
+	mutex_lock(&gpio_aggregator_lock);
+	idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
+	idr_destroy(&gpio_aggregator_idr);
+	mutex_unlock(&gpio_aggregator_lock);
+}
+
 static int __init gpio_aggregator_init(void)
 {
 	return platform_driver_register(&gpio_aggregator_driver);
-- 
2.45.2

[PATCH v2 02/10] gpio: aggregator: introduce basic configfs interface

[Koichiro Den]

The existing sysfs 'new_device' interface has several limitations:
* No way to determine when GPIO aggregator creation is complete.
* No way to retrieve errors when creating a GPIO aggregator.
* No way to trace a GPIO line of an aggregator back to its
  corresponding physical device.
* The 'new_device' echo does not indicate which virtual gpiochip<N>
  was created.
* No way to assign names to GPIO lines exported through an aggregator.

Introduce the new configfs interface for gpio-aggregator to address
these limitations. It provides a more streamlined, modern, and
extensible configuration method. For backward compatibility, the
'new_device' interface and its behaviour is retained for now.

This commit implements minimal functionalities:

  /config/gpio-aggregator/<name-of-your-choice>/
  /config/gpio-aggregator/<name-of-your-choice>/live
  /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
  /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
  /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset

Basic setup flow is:
1. Create a directory for a GPIO aggregator.
2. Create subdirectories for each line you want to instantiate.
3. In each line directory, configure the key and offset.
   The key/offset semantics are as follows:
   * If offset is >= 0:
     - key specifies the name of the chip this GPIO belongs to
     - offset specifies the line offset within that chip.
   * If offset is <0:
     - key needs to specify the GPIO line name.
4. Return to the aggregator's root directory and write '1' to the live
   attribute.

For example, the command in the existing kernel doc:

  echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device

is equivalent to:

  mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
  # Change <custom-name> to name of your choice (e.g. "aggr0")
  cd /sys/kernel/config/gpio-aggregator/<custom-name>
  mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
  echo e6052000.gpio > line0/key
  echo 19            > line0/offset
  echo e6050000.gpio > line1/key
  echo 20            > line1/offset
  echo e6050000.gpio > line2/key
  echo 21            > line2/offset
  echo 1             > live

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 549 ++++++++++++++++++++++++++++++++-
 1 file changed, 548 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 570cd1ff8cc2..c63cf3067ce7 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -9,10 +9,14 @@
 
 #include <linux/bitmap.h>
 #include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/configfs.h>
 #include <linux/ctype.h>
 #include <linux/delay.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/lockdep.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -34,11 +38,39 @@
  */
 
 struct gpio_aggregator {
+	struct config_group group;
 	struct gpiod_lookup_table *lookups;
 	struct platform_device *pdev;
+	struct mutex lock;
+	int id;
+
+	/* Synchronize with probe */
+	struct notifier_block bus_notifier;
+	struct completion probe_completion;
+	bool driver_bound;
+
+	/* List of gpio_aggregator_line. Always added in order */
+	struct list_head list_head;
+
 	char args[];
 };
 
+struct gpio_aggregator_line {
+	struct config_group group;
+	struct gpio_aggregator *parent;
+	struct list_head entry;
+
+	/* Line index within the aggregator device */
+	int idx;
+
+	/* GPIO chip label or line name */
+	char *key;
+	/* Can be negative to indicate lookup by line name */
+	int offset;
+
+	enum gpio_lookup_flags flags;
+};
+
 static DEFINE_MUTEX(gpio_aggregator_lock);	/* protects idr */
 static DEFINE_IDR(gpio_aggregator_idr);
 
@@ -61,6 +93,97 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
 	return 0;
 }
 
+static int aggr_notifier_call(struct notifier_block *nb, unsigned long action,
+			      void *data)
+{
+	struct gpio_aggregator *aggr;
+	struct device *dev = data;
+
+	aggr = container_of(nb, struct gpio_aggregator, bus_notifier);
+	if (!device_match_name(dev, aggr->lookups->dev_id))
+		return NOTIFY_DONE;
+
+	switch (action) {
+	case BUS_NOTIFY_BOUND_DRIVER:
+		aggr->driver_bound = true;
+		break;
+	case BUS_NOTIFY_DRIVER_NOT_BOUND:
+		aggr->driver_bound = false;
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	complete(&aggr->probe_completion);
+	return NOTIFY_OK;
+}
+
+static bool aggr_is_active(struct gpio_aggregator *aggr)
+{
+	lockdep_assert_held(&aggr->lock);
+
+	return !!aggr->pdev && platform_get_drvdata(aggr->pdev);
+}
+
+static size_t aggr_count_lines(struct gpio_aggregator *aggr)
+{
+	lockdep_assert_held(&aggr->lock);
+
+	return list_count_nodes(&aggr->list_head);
+}
+
+static struct gpio_aggregator_line *aggr_line_alloc(
+		struct gpio_aggregator *parent, int idx, char *key, int offset)
+{
+	struct gpio_aggregator_line *line;
+
+	line = kzalloc(sizeof(*line), GFP_KERNEL);
+	if (!line)
+		return ERR_PTR(-ENOMEM);
+
+	if (key) {
+		line->key = kstrdup(key, GFP_KERNEL);
+		if (!line->key) {
+			kfree(line);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+
+	line->flags = GPIO_LOOKUP_FLAGS_DEFAULT;
+	line->parent = parent;
+	line->idx = idx;
+	line->offset = offset;
+	INIT_LIST_HEAD(&line->entry);
+
+	return line;
+}
+
+static void aggr_line_add(struct gpio_aggregator *aggr,
+			  struct gpio_aggregator_line *line)
+{
+	struct gpio_aggregator_line *tmp;
+
+	lockdep_assert_held(&aggr->lock);
+
+	list_for_each_entry(tmp, &aggr->list_head, entry) {
+		if (WARN_ON(tmp->idx == line->idx))
+			return;
+		if (tmp->idx > line->idx) {
+			list_add_tail(&line->entry, &tmp->entry);
+			return;
+		}
+	}
+	list_add_tail(&line->entry, &aggr->list_head);
+}
+
+static void aggr_line_del(struct gpio_aggregator *aggr,
+			  struct gpio_aggregator_line *line)
+{
+	lockdep_assert_held(&aggr->lock);
+
+	list_del(&line->entry);
+}
+
 
 /*
  *  GPIO Forwarder
@@ -385,6 +508,411 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 }
 
 
+/*
+ * Configfs interface
+ */
+
+static struct gpio_aggregator *
+to_gpio_aggregator(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+
+	return container_of(group, struct gpio_aggregator, group);
+}
+
+static struct gpio_aggregator_line *
+to_gpio_aggregator_line(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+
+	return container_of(group, struct gpio_aggregator_line, group);
+}
+
+static int aggr_activate(struct gpio_aggregator *aggr)
+{
+	struct platform_device_info pdevinfo;
+	struct gpio_aggregator_line *line;
+	struct platform_device *pdev;
+	unsigned int n = 0;
+	int ret = 0;
+
+	if (aggr_count_lines(aggr) == 0)
+		return -EINVAL;
+
+	aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
+				GFP_KERNEL);
+	if (!aggr->lookups)
+		return -ENOMEM;
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+	pdevinfo.name = DRV_NAME;
+	pdevinfo.id = aggr->id;
+
+	/* The list is always sorted as new elements are inserted in order. */
+	list_for_each_entry(line, &aggr->list_head, entry) {
+		/*
+		 * - Either GPIO chip label or line name must be configured
+		 *   (i.e. line->key must be non-NULL)
+		 * - Line directories must be named with sequential numeric
+		 *   suffixes starting from 0. (i.e. ./line0, ./line1, ...)
+		 */
+		if (!line->key || line->idx != n) {
+			ret = -EINVAL;
+			goto err_remove_lookups;
+		}
+
+		if (line->offset < 0)
+			ret = aggr_add_gpio(aggr, line->key, U16_MAX, &n);
+		else
+			ret = aggr_add_gpio(aggr, line->key, line->offset, &n);
+		if (ret)
+			goto err_remove_lookups;
+	}
+
+	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, aggr->id);
+	if (!aggr->lookups->dev_id) {
+		ret = -ENOMEM;
+		goto err_remove_lookups;
+	}
+
+	gpiod_add_lookup_table(aggr->lookups);
+
+	reinit_completion(&aggr->probe_completion);
+	aggr->driver_bound = false;
+	aggr->bus_notifier.notifier_call = aggr_notifier_call;
+	bus_register_notifier(&platform_bus_type, &aggr->bus_notifier);
+
+	pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(pdev)) {
+		ret = PTR_ERR(pdev);
+		bus_unregister_notifier(&platform_bus_type, &aggr->bus_notifier);
+		goto err_remove_lookup_table;
+	}
+
+	wait_for_completion(&aggr->probe_completion);
+	bus_unregister_notifier(&platform_bus_type, &aggr->bus_notifier);
+
+	if (!aggr->driver_bound) {
+		ret = -ENXIO;
+		goto err_unregister_pdev;
+	}
+
+	aggr->pdev = pdev;
+	return 0;
+
+err_unregister_pdev:
+	platform_device_unregister(pdev);
+err_remove_lookup_table:
+	kfree(aggr->lookups->dev_id);
+	gpiod_remove_lookup_table(aggr->lookups);
+err_remove_lookups:
+	kfree(aggr->lookups);
+
+	return ret;
+}
+
+static void aggr_deactivate(struct gpio_aggregator *aggr)
+{
+	platform_device_unregister(aggr->pdev);
+	gpiod_remove_lookup_table(aggr->lookups);
+	kfree(aggr->lookups->dev_id);
+	kfree(aggr->lookups);
+	aggr->pdev = NULL;
+}
+
+static void aggr_lockup_configfs(struct gpio_aggregator *aggr, bool lock)
+{
+	struct configfs_subsystem *subsys = aggr->group.cg_subsys;
+	struct gpio_aggregator_line *line;
+
+	/*
+	 * The device only needs to depend on leaf lines. This is
+	 * sufficient to lock up all the configfs entries that the
+	 * instantiated, alive device depends on.
+	 */
+	list_for_each_entry(line, &aggr->list_head, entry) {
+		if (lock)
+			WARN_ON(configfs_depend_item_unlocked(
+					subsys, &line->group.cg_item));
+		else
+			configfs_undepend_item_unlocked(
+					&line->group.cg_item);
+	}
+}
+
+static ssize_t
+gpio_aggr_line_key_show(struct config_item *item, char *page)
+{
+	struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+	struct gpio_aggregator *aggr = line->parent;
+
+	guard(mutex)(&aggr->lock);
+
+	return sprintf(page, "%s\n", line->key ?: "");
+}
+
+static ssize_t
+gpio_aggr_line_key_store(struct config_item *item, const char *page,
+			 size_t count)
+{
+	struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+	struct gpio_aggregator *aggr = line->parent;
+
+	char *key __free(kfree) = kstrndup(skip_spaces(page), count,
+					   GFP_KERNEL);
+	if (!key)
+		return -ENOMEM;
+
+	strim(key);
+
+	guard(mutex)(&aggr->lock);
+
+	if (aggr_is_active(aggr))
+		return -EBUSY;
+
+	kfree(line->key);
+	line->key = no_free_ptr(key);
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_aggr_line_, key);
+
+static ssize_t
+gpio_aggr_line_offset_show(struct config_item *item, char *page)
+{
+	struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+	struct gpio_aggregator *aggr = line->parent;
+	unsigned int offset;
+
+	scoped_guard(mutex, &aggr->lock)
+		offset = line->offset;
+
+	return sprintf(page, "%d\n", offset);
+}
+
+static ssize_t
+gpio_aggr_line_offset_store(struct config_item *item, const char *page,
+			    size_t count)
+{
+	struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+	struct gpio_aggregator *aggr = line->parent;
+	int offset, ret;
+
+	ret = kstrtoint(page, 0, &offset);
+	if (ret)
+		return ret;
+
+	/*
+	 * Negative number here means: 'key' represents a line name to lookup.
+	 * Non-negative means: 'key' represents the label of the chip with
+	 * the 'offset' value representing the line within that chip.
+	 *
+	 * GPIOLIB uses the U16_MAX value to indicate lookup by line name so
+	 * the greatest offset we can accept is (U16_MAX - 1).
+	 */
+	if (offset > (U16_MAX - 1))
+		return -EINVAL;
+
+	guard(mutex)(&aggr->lock);
+
+	if (aggr_is_active(aggr))
+		return -EBUSY;
+
+	line->offset = offset;
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_aggr_line_, offset);
+
+static struct configfs_attribute *gpio_aggr_line_attrs[] = {
+	&gpio_aggr_line_attr_key,
+	&gpio_aggr_line_attr_offset,
+	NULL
+};
+
+static ssize_t
+gpio_aggr_device_live_show(struct config_item *item, char *page)
+{
+	struct gpio_aggregator *aggr = to_gpio_aggregator(item);
+	bool active;
+
+	scoped_guard(mutex, &aggr->lock)
+		active = aggr_is_active(aggr);
+
+	return sprintf(page, "%c\n", active ? '1' : '0');
+}
+
+static ssize_t
+gpio_aggr_device_live_store(struct config_item *item, const char *page,
+			    size_t count)
+{
+	struct gpio_aggregator *aggr = to_gpio_aggregator(item);
+	int ret = 0;
+	bool live;
+
+	ret = kstrtobool(page, &live);
+	if (ret)
+		return ret;
+
+	if (live)
+		aggr_lockup_configfs(aggr, true);
+
+	scoped_guard(mutex, &aggr->lock) {
+		if (live == aggr_is_active(aggr))
+			ret = -EPERM;
+		else if (live)
+			ret = aggr_activate(aggr);
+		else
+			aggr_deactivate(aggr);
+	}
+
+	/*
+	 * Undepend is required only if device disablement (live == 0)
+	 * succeeds or if device enablement (live == 1) fails.
+	 */
+	if (live == !!ret)
+		aggr_lockup_configfs(aggr, false);
+
+	return ret ?: count;
+}
+
+CONFIGFS_ATTR(gpio_aggr_device_, live);
+
+static struct configfs_attribute *gpio_aggr_device_attrs[] = {
+	&gpio_aggr_device_attr_live,
+	NULL
+};
+
+static void
+gpio_aggr_line_release(struct config_item *item)
+{
+	struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+	struct gpio_aggregator *aggr = line->parent;
+
+	guard(mutex)(&aggr->lock);
+
+	aggr_line_del(aggr, line);
+	kfree(line->key);
+	kfree(line);
+}
+
+static struct configfs_item_operations gpio_aggr_line_item_ops = {
+	.release	= gpio_aggr_line_release,
+};
+
+static const struct config_item_type gpio_aggr_line_type = {
+	.ct_item_ops	= &gpio_aggr_line_item_ops,
+	.ct_attrs	= gpio_aggr_line_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static void gpio_aggr_device_release(struct config_item *item)
+{
+	struct gpio_aggregator *aggr = to_gpio_aggregator(item);
+
+	guard(mutex)(&aggr->lock);
+
+	if (aggr_is_active(aggr))
+		aggr_deactivate(aggr);
+
+	mutex_destroy(&aggr->lock);
+	idr_remove(&gpio_aggregator_idr, aggr->id);
+	kfree(aggr);
+}
+
+static struct configfs_item_operations gpio_aggr_device_item_ops = {
+	.release	= gpio_aggr_device_release,
+};
+
+static struct config_group *
+gpio_aggr_device_make_group(struct config_group *group, const char *name)
+{
+	struct gpio_aggregator *aggr = to_gpio_aggregator(&group->cg_item);
+	struct gpio_aggregator_line *line;
+	unsigned int idx;
+	int ret, nchar;
+
+	ret = sscanf(name, "line%u%n", &idx, &nchar);
+	if (ret != 1 || nchar != strlen(name))
+		return ERR_PTR(-EINVAL);
+
+	guard(mutex)(&aggr->lock);
+
+	if (aggr_is_active(aggr))
+		return ERR_PTR(-EBUSY);
+
+	list_for_each_entry(line, &aggr->list_head, entry)
+		if (line->idx == idx)
+			return ERR_PTR(-EINVAL);
+
+	line = aggr_line_alloc(aggr, idx, NULL, -1);
+	if (!line)
+		return ERR_PTR(-ENOMEM);
+
+	config_group_init_type_name(&line->group, name, &gpio_aggr_line_type);
+
+	aggr_line_add(aggr, line);
+
+	return &line->group;
+}
+
+static struct configfs_group_operations gpio_aggr_device_group_ops = {
+	.make_group	= gpio_aggr_device_make_group,
+};
+
+static const struct config_item_type gpio_aggr_device_type = {
+	.ct_group_ops	= &gpio_aggr_device_group_ops,
+	.ct_item_ops	= &gpio_aggr_device_item_ops,
+	.ct_attrs	= gpio_aggr_device_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *
+gpio_aggr_make_group(struct config_group *group, const char *name)
+{
+	/* arg space is unneeded */
+	struct gpio_aggregator *aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
+	if (!aggr)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&gpio_aggregator_lock);
+	aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
+	mutex_unlock(&gpio_aggregator_lock);
+
+	if (aggr->id < 0) {
+		kfree(aggr);
+		return ERR_PTR(aggr->id);
+	}
+
+	INIT_LIST_HEAD(&aggr->list_head);
+	mutex_init(&aggr->lock);
+	config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
+	init_completion(&aggr->probe_completion);
+
+	return &aggr->group;
+}
+
+static struct configfs_group_operations gpio_aggr_group_ops = {
+	.make_group	= gpio_aggr_make_group,
+};
+
+static const struct config_item_type gpio_aggr_type = {
+	.ct_group_ops	= &gpio_aggr_group_ops,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem gpio_aggr_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf	= DRV_NAME,
+			.ci_type	= &gpio_aggr_type,
+		},
+	},
+};
+
+
 /*
  * Sysfs interface
  */
@@ -622,12 +1150,31 @@ static void __exit gpio_aggregator_remove_all(void)
 
 static int __init gpio_aggregator_init(void)
 {
-	return platform_driver_register(&gpio_aggregator_driver);
+	int ret = 0;
+
+	ret = platform_driver_register(&gpio_aggregator_driver);
+	if (ret) {
+		pr_err("Failed to register the platform driver: %d\n", ret);
+		return ret;
+	}
+
+	config_group_init(&gpio_aggr_subsys.su_group);
+	mutex_init(&gpio_aggr_subsys.su_mutex);
+	ret = configfs_register_subsystem(&gpio_aggr_subsys);
+	if (ret) {
+		pr_err("Failed to register the '%s' configfs subsystem: %d\n",
+		       gpio_aggr_subsys.su_group.cg_item.ci_namebuf, ret);
+		mutex_destroy(&gpio_aggr_subsys.su_mutex);
+		platform_driver_unregister(&gpio_aggregator_driver);
+	}
+
+	return ret;
 }
 module_init(gpio_aggregator_init);
 
 static void __exit gpio_aggregator_exit(void)
 {
+	configfs_unregister_subsystem(&gpio_aggr_subsys);
 	gpio_aggregator_remove_all();
 	platform_driver_unregister(&gpio_aggregator_driver);
 }
-- 
2.45.2

[PATCH v2 03/10] gpio: aggregator: add read-only 'dev_name' configfs attribute

[Koichiro Den]

Add a read-only 'dev_name' attribute to configfs interface, which
exposes the platform bus device name. Users can easily identify which
gpiochip<N> has been created as follows:

$ cat /sys/kernel/config/gpio-aggregator/<aggregator-name>/dev_name
  gpio-aggregator.0
$ ls -d /sys/devices/platform/gpio-aggregator.0/gpiochip*
  gpiochip3

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index c63cf3067ce7..76d3a8677308 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -732,6 +732,23 @@ static struct configfs_attribute *gpio_aggr_line_attrs[] = {
 	NULL
 };
 
+static ssize_t
+gpio_aggr_device_dev_name_show(struct config_item *item, char *page)
+{
+	struct gpio_aggregator *aggr = to_gpio_aggregator(item);
+	struct platform_device *pdev;
+
+	guard(mutex)(&aggr->lock);
+
+	pdev = aggr->pdev;
+	if (pdev)
+		return sprintf(page, "%s\n", dev_name(&pdev->dev));
+
+	return sprintf(page, "%s.%d\n", DRV_NAME, aggr->id);
+}
+
+CONFIGFS_ATTR_RO(gpio_aggr_device_, dev_name);
+
 static ssize_t
 gpio_aggr_device_live_show(struct config_item *item, char *page)
 {
@@ -781,6 +798,7 @@ gpio_aggr_device_live_store(struct config_item *item, const char *page,
 CONFIGFS_ATTR(gpio_aggr_device_, live);
 
 static struct configfs_attribute *gpio_aggr_device_attrs[] = {
+	&gpio_aggr_device_attr_dev_name,
 	&gpio_aggr_device_attr_live,
 	NULL
 };
-- 
2.45.2

[PATCH v2 04/10] gpio: aggregator: add read-write 'name' attribute

[Koichiro Den]

Previously, there is no way to assign names to GPIO lines exported
through an aggregator.

Allow users to set custom line names via a 'name' attribute.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 42 ++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 76d3a8677308..3263d99bfe69 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -63,6 +63,8 @@ struct gpio_aggregator_line {
 	/* Line index within the aggregator device */
 	int idx;
 
+	/* Custom name for the virtual line */
+	char *name;
 	/* GPIO chip label or line name */
 	char *key;
 	/* Can be negative to indicate lookup by line name */
@@ -678,6 +680,44 @@ gpio_aggr_line_key_store(struct config_item *item, const char *page,
 
 CONFIGFS_ATTR(gpio_aggr_line_, key);
 
+static ssize_t
+gpio_aggr_line_name_show(struct config_item *item, char *page)
+{
+	struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+	struct gpio_aggregator *aggr = line->parent;
+
+	guard(mutex)(&aggr->lock);
+
+	return sprintf(page, "%s\n", line->name ?: "");
+}
+
+static ssize_t
+gpio_aggr_line_name_store(struct config_item *item, const char *page,
+			  size_t count)
+{
+	struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+	struct gpio_aggregator *aggr = line->parent;
+
+	char *name __free(kfree) = kstrndup(skip_spaces(page), count,
+					    GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+
+	strim(name);
+
+	guard(mutex)(&aggr->lock);
+
+	if (aggr_is_active(aggr))
+		return -EBUSY;
+
+	kfree(line->name);
+	line->name = no_free_ptr(name);
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_aggr_line_, name);
+
 static ssize_t
 gpio_aggr_line_offset_show(struct config_item *item, char *page)
 {
@@ -728,6 +768,7 @@ CONFIGFS_ATTR(gpio_aggr_line_, offset);
 
 static struct configfs_attribute *gpio_aggr_line_attrs[] = {
 	&gpio_aggr_line_attr_key,
+	&gpio_aggr_line_attr_name,
 	&gpio_aggr_line_attr_offset,
 	NULL
 };
@@ -813,6 +854,7 @@ gpio_aggr_line_release(struct config_item *item)
 
 	aggr_line_del(aggr, line);
 	kfree(line->key);
+	kfree(line->name);
 	kfree(line);
 }
 
-- 
2.45.2

[PATCH v2 05/10] gpio: aggregator: expose custom line names to forwarder gpio_chip

[Koichiro Den]

Previously, GPIO lines in the aggregator had empty names. Now that the
configfs interface supports custom names, update the GPIO forwarder to
use them.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 71 ++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 3263d99bfe69..268b9b580ada 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -425,6 +425,20 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
 }
 #endif	/* !CONFIG_OF_GPIO */
 
+static int gpiochip_fwd_line_names(struct device *dev, const char **names, int len)
+{
+	int num = device_property_string_array_count(dev, "gpio-line-names");
+	if (!num)
+		return 0;
+	if (num > len) {
+		pr_warn("gpio-line-names contains %d lines while %d expected",
+			num, len);
+		num = len;
+	}
+	return device_property_read_string_array(dev, "gpio-line-names",
+						 names, num);
+}
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -447,6 +461,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 {
 	const char *label = dev_name(dev);
 	struct gpiochip_fwd *fwd;
+	const char **line_names;
 	struct gpio_chip *chip;
 	unsigned int i;
 	int error;
@@ -458,6 +473,16 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 
 	chip = &fwd->chip;
 
+	if (!dev_of_node(dev)) {
+		line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);
+		if (!line_names)
+			return ERR_PTR(-ENOMEM);
+
+		error = gpiochip_fwd_line_names(dev, line_names, ngpios);
+		if (error < 0)
+			return ERR_PTR(-ENOMEM);
+	}
+
 	/*
 	 * If any of the GPIO lines are sleeping, then the entire forwarder
 	 * will be sleeping.
@@ -491,6 +516,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	chip->ngpio = ngpios;
 	fwd->descs = descs;
 
+	if (!dev_of_node(dev))
+		chip->names = line_names;
+
 	if (chip->can_sleep)
 		mutex_init(&fwd->mlock);
 	else
@@ -530,10 +558,40 @@ to_gpio_aggregator_line(struct config_item *item)
 	return container_of(group, struct gpio_aggregator_line, group);
 }
 
+static struct fwnode_handle *aggr_make_device_swnode(struct gpio_aggregator *aggr)
+{
+	char **line_names __free(kfree) = NULL;
+	struct property_entry properties[2];
+	struct gpio_aggregator_line *line;
+	size_t num_lines;
+	int n = 0;
+
+	memset(properties, 0, sizeof(properties));
+
+	num_lines = aggr_count_lines(aggr);
+	if (num_lines == 0)
+		return NULL;
+
+	line_names = kcalloc(num_lines, sizeof(*line_names), GFP_KERNEL);
+	if (!line_names)
+		return ERR_PTR(-ENOMEM);
+
+	/* The list is always sorted as new elements are inserted in order. */
+	list_for_each_entry(line, &aggr->list_head, entry)
+		line_names[n++] = line->name ?: "";
+
+	properties[0] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+					"gpio-line-names",
+					line_names, num_lines);
+
+	return fwnode_create_software_node(properties, NULL);
+}
+
 static int aggr_activate(struct gpio_aggregator *aggr)
 {
 	struct platform_device_info pdevinfo;
 	struct gpio_aggregator_line *line;
+	struct fwnode_handle *swnode;
 	struct platform_device *pdev;
 	unsigned int n = 0;
 	int ret = 0;
@@ -546,9 +604,14 @@ static int aggr_activate(struct gpio_aggregator *aggr)
 	if (!aggr->lookups)
 		return -ENOMEM;
 
+	swnode = aggr_make_device_swnode(aggr);
+	if (IS_ERR(swnode))
+		goto err_remove_lookups;
+
 	memset(&pdevinfo, 0, sizeof(pdevinfo));
 	pdevinfo.name = DRV_NAME;
 	pdevinfo.id = aggr->id;
+	pdevinfo.fwnode = swnode;
 
 	/* The list is always sorted as new elements are inserted in order. */
 	list_for_each_entry(line, &aggr->list_head, entry) {
@@ -560,7 +623,7 @@ static int aggr_activate(struct gpio_aggregator *aggr)
 		 */
 		if (!line->key || line->idx != n) {
 			ret = -EINVAL;
-			goto err_remove_lookups;
+			goto err_remove_swnode;
 		}
 
 		if (line->offset < 0)
@@ -568,13 +631,13 @@ static int aggr_activate(struct gpio_aggregator *aggr)
 		else
 			ret = aggr_add_gpio(aggr, line->key, line->offset, &n);
 		if (ret)
-			goto err_remove_lookups;
+			goto err_remove_swnode;
 	}
 
 	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, aggr->id);
 	if (!aggr->lookups->dev_id) {
 		ret = -ENOMEM;
-		goto err_remove_lookups;
+		goto err_remove_swnode;
 	}
 
 	gpiod_add_lookup_table(aggr->lookups);
@@ -607,6 +670,8 @@ static int aggr_activate(struct gpio_aggregator *aggr)
 err_remove_lookup_table:
 	kfree(aggr->lookups->dev_id);
 	gpiod_remove_lookup_table(aggr->lookups);
+err_remove_swnode:
+	fwnode_remove_software_node(swnode);
 err_remove_lookups:
 	kfree(aggr->lookups);
 
-- 
2.45.2

[PATCH v2 06/10] gpio: aggregator: rename 'name' to 'key' in aggr_parse()

[Koichiro Den]

Rename the local variable 'name' in aggr_parse() to 'key' because struct
gpio_aggregator_line now uses the 'name' field for the custom line name
and the local variable actually represents a 'key'. This change prepares
for the next but one commit.

No functional change.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 268b9b580ada..123906c821b1 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -1044,7 +1044,7 @@ static struct configfs_subsystem gpio_aggr_subsys = {
 static int aggr_parse(struct gpio_aggregator *aggr)
 {
 	char *args = skip_spaces(aggr->args);
-	char *name, *offsets, *p;
+	char *key, *offsets, *p;
 	unsigned int i, n = 0;
 	int error = 0;
 
@@ -1053,18 +1053,18 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 	if (!bitmap)
 		return -ENOMEM;
 
-	args = next_arg(args, &name, &p);
+	args = next_arg(args, &key, &p);
 	while (*args) {
 		args = next_arg(args, &offsets, &p);
 
 		p = get_options(offsets, 0, &error);
 		if (error == 0 || *p) {
 			/* Named GPIO line */
-			error = aggr_add_gpio(aggr, name, U16_MAX, &n);
+			error = aggr_add_gpio(aggr, key, U16_MAX, &n);
 			if (error)
 				return error;
 
-			name = offsets;
+			key = offsets;
 			continue;
 		}
 
@@ -1076,12 +1076,12 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 		}
 
 		for_each_set_bit(i, bitmap, AGGREGATOR_MAX_GPIOS) {
-			error = aggr_add_gpio(aggr, name, i, &n);
+			error = aggr_add_gpio(aggr, key, i, &n);
 			if (error)
 				return error;
 		}
 
-		args = next_arg(args, &name, &p);
+		args = next_arg(args, &key, &p);
 	}
 
 	if (!n) {
-- 
2.45.2

[PATCH v2 07/10] gpio: aggregator: clean up gpio_aggregator_free()

[Koichiro Den]

- Rename gpio_aggregator_free() to use the "aggr_" prefix for
  consistency with other functions that modify struct gpio_aggregator
  internals.
- Replace four lines within the function to invoke aggr_deactivate()
- Move it to a more natural location.

This is a preparatory change for the next commit.

No functional change.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 123906c821b1..d5fd9fe58164 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -1092,6 +1092,12 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 	return 0;
 }
 
+static void aggr_free(struct gpio_aggregator *aggr)
+{
+	aggr_deactivate(aggr);
+	kfree(aggr);
+}
+
 static ssize_t new_device_store(struct device_driver *driver, const char *buf,
 				size_t count)
 {
@@ -1160,15 +1166,6 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
 
 static DRIVER_ATTR_WO(new_device);
 
-static void gpio_aggregator_free(struct gpio_aggregator *aggr)
-{
-	platform_device_unregister(aggr->pdev);
-	gpiod_remove_lookup_table(aggr->lookups);
-	kfree(aggr->lookups->dev_id);
-	kfree(aggr->lookups);
-	kfree(aggr);
-}
-
 static ssize_t delete_device_store(struct device_driver *driver,
 				   const char *buf, size_t count)
 {
@@ -1189,7 +1186,7 @@ static ssize_t delete_device_store(struct device_driver *driver,
 	if (!aggr)
 		return -ENOENT;
 
-	gpio_aggregator_free(aggr);
+	aggr_free(aggr);
 	return count;
 }
 static DRIVER_ATTR_WO(delete_device);
@@ -1261,7 +1258,7 @@ static struct platform_driver gpio_aggregator_driver = {
 
 static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
 {
-	gpio_aggregator_free(p);
+	aggr_free(p);
 	return 0;
 }
 
-- 
2.45.2

[PATCH v2 08/10] gpio: aggregator: expoose aggregator created via legacy sysfs to configfs

[Koichiro Den]

Expose settings for aggregators created using the sysfs 'new_device'
interface to configfs. Once written to 'new_device', an "_auto.<N>" path
appears in the configfs regardless of whether the probe succeeds.
Consequently, users can no longer use that prefix for custom GPIO
aggregator names. The 'live' attribute changes to 1 when the probe
succeeds and the GPIO forwarder is instantiated.

Note that the aggregator device created via sysfs is asynchrnous, i.e.
writing into 'new_device' returns without waiting for probe completion,
and the probe may succeed, fail, or eventually succeed via deferred
probe. Thus, the 'live' attribute may change from 0 to 1 asynchronously
without notice. So, editting key/offset/name while it's waiting for
deferred probe is prohibited.

The configfs auto-generation relies on create_default_group(), which
inherently prohibits rmdir(2). To align with the limitation, this commit
also prohibits mkdir(2) for them. When users want to change the number
of lines for an aggregator initialized via 'new_device', they need to
tear down the device using 'delete_device' and reconfigure it from
scratch. This does not break previous behaviour; users of legacy sysfs
interface simply gain additional almost read-only configfs exposure.

Still, users can write into 'live' attribute to toggle the device unless
it's waiting for deferred probe. So once probe succeeds, they can
deactivate it in the same manner as the devices initialized via
configfs.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 174 +++++++++++++++++++++++++++++----
 1 file changed, 157 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index d5fd9fe58164..e101b78ad524 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -32,6 +32,7 @@
 #include <linux/gpio/machine.h>
 
 #define AGGREGATOR_MAX_GPIOS 512
+#define AGGREGATOR_LEGACY_PREFIX "_auto"
 
 /*
  * GPIO Aggregator sysfs interface
@@ -52,6 +53,8 @@ struct gpio_aggregator {
 	/* List of gpio_aggregator_line. Always added in order */
 	struct list_head list_head;
 
+	/* used by legacy sysfs interface only */
+	bool init_via_sysfs;
 	char args[];
 };
 
@@ -73,6 +76,10 @@ struct gpio_aggregator_line {
 	enum gpio_lookup_flags flags;
 };
 
+struct gpio_aggregator_pdev_meta {
+	bool init_via_sysfs;
+};
+
 static DEFINE_MUTEX(gpio_aggregator_lock);	/* protects idr */
 static DEFINE_IDR(gpio_aggregator_idr);
 
@@ -127,6 +134,14 @@ static bool aggr_is_active(struct gpio_aggregator *aggr)
 	return !!aggr->pdev && platform_get_drvdata(aggr->pdev);
 }
 
+/* Only aggregators created via legacy sysfs can be "activating". */
+static bool aggr_is_activating(struct gpio_aggregator *aggr)
+{
+	lockdep_assert_held(&aggr->lock);
+
+	return !!aggr->pdev && !platform_get_drvdata(aggr->pdev);
+}
+
 static size_t aggr_count_lines(struct gpio_aggregator *aggr)
 {
 	lockdep_assert_held(&aggr->lock);
@@ -186,6 +201,25 @@ static void aggr_line_del(struct gpio_aggregator *aggr,
 	list_del(&line->entry);
 }
 
+static void aggr_unregister_lines(struct gpio_aggregator *aggr)
+{
+	struct gpio_aggregator_line *line;
+
+	list_for_each_entry(line, &aggr->list_head, entry)
+		configfs_unregister_group(&line->group);
+}
+
+static void aggr_free_lines(struct gpio_aggregator *aggr)
+{
+	struct gpio_aggregator_line *line, *tmp;
+
+	list_for_each_entry_safe(line, tmp, &aggr->list_head, entry) {
+		aggr_line_del(aggr, line);
+		kfree(line->key);
+		kfree(line);
+	}
+}
+
 
 /*
  *  GPIO Forwarder
@@ -447,6 +481,7 @@ static int gpiochip_fwd_line_names(struct device *dev, const char **names, int l
  *         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_*.
+ * @init_via_sysfs: True if the creation is done via legacy sysfs interface.
  *
  * This function creates a new gpiochip, which forwards all GPIO operations to
  * the passed GPIO descriptors.
@@ -457,7 +492,8 @@ static int gpiochip_fwd_line_names(struct device *dev, const char **names, int l
 static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 						unsigned int ngpios,
 						struct gpio_desc *descs[],
-						unsigned long features)
+						unsigned long features,
+						bool init_via_sysfs)
 {
 	const char *label = dev_name(dev);
 	struct gpiochip_fwd *fwd;
@@ -473,7 +509,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 
 	chip = &fwd->chip;
 
-	if (!dev_of_node(dev)) {
+	if (!dev_of_node(dev) && !init_via_sysfs) {
 		line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);
 		if (!line_names)
 			return ERR_PTR(-ENOMEM);
@@ -516,7 +552,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	chip->ngpio = ngpios;
 	fwd->descs = descs;
 
-	if (!dev_of_node(dev))
+	if (!dev_of_node(dev) && !init_via_sysfs)
 		chip->names = line_names;
 
 	if (chip->can_sleep)
@@ -734,7 +770,7 @@ gpio_aggr_line_key_store(struct config_item *item, const char *page,
 
 	guard(mutex)(&aggr->lock);
 
-	if (aggr_is_active(aggr))
+	if (aggr_is_activating(aggr) || aggr_is_active(aggr))
 		return -EBUSY;
 
 	kfree(line->key);
@@ -772,7 +808,7 @@ gpio_aggr_line_name_store(struct config_item *item, const char *page,
 
 	guard(mutex)(&aggr->lock);
 
-	if (aggr_is_active(aggr))
+	if (aggr_is_activating(aggr) || aggr_is_active(aggr))
 		return -EBUSY;
 
 	kfree(line->name);
@@ -821,7 +857,7 @@ gpio_aggr_line_offset_store(struct config_item *item, const char *page,
 
 	guard(mutex)(&aggr->lock);
 
-	if (aggr_is_active(aggr))
+	if (aggr_is_activating(aggr) || aggr_is_active(aggr))
 		return -EBUSY;
 
 	line->offset = offset;
@@ -879,11 +915,12 @@ gpio_aggr_device_live_store(struct config_item *item, const char *page,
 	if (ret)
 		return ret;
 
-	if (live)
+	if (live && !aggr->init_via_sysfs)
 		aggr_lockup_configfs(aggr, true);
 
 	scoped_guard(mutex, &aggr->lock) {
-		if (live == aggr_is_active(aggr))
+		if (aggr_is_activating(aggr) ||
+		    (live == aggr_is_active(aggr)))
 			ret = -EPERM;
 		else if (live)
 			ret = aggr_activate(aggr);
@@ -895,7 +932,7 @@ gpio_aggr_device_live_store(struct config_item *item, const char *page,
 	 * Undepend is required only if device disablement (live == 0)
 	 * succeeds or if device enablement (live == 1) fails.
 	 */
-	if (live == !!ret)
+	if (live == !!ret && !aggr->init_via_sysfs)
 		aggr_lockup_configfs(aggr, false);
 
 	return ret ?: count;
@@ -963,6 +1000,15 @@ gpio_aggr_device_make_group(struct config_group *group, const char *name)
 	if (ret != 1 || nchar != strlen(name))
 		return ERR_PTR(-EINVAL);
 
+	if (aggr->init_via_sysfs)
+		/*
+		 * Aggregators created via legacy sysfs interface are exposed as
+		 * default groups, which means rmdir(2) is prohibited for them.
+		 * For simplicity, and to avoid confusion, we also prohibit
+		 * mkdir(2).
+		 */
+		return ERR_PTR(-EPERM);
+
 	guard(mutex)(&aggr->lock);
 
 	if (aggr_is_active(aggr))
@@ -1002,6 +1048,14 @@ gpio_aggr_make_group(struct config_group *group, const char *name)
 	if (!aggr)
 		return ERR_PTR(-ENOMEM);
 
+	/*
+	 * "_auto" prefix is reserved for auto-generated config group
+	 * for devices create via legacy sysfs interface.
+	 */
+	if (strncmp(name, AGGREGATOR_LEGACY_PREFIX,
+		    sizeof(AGGREGATOR_LEGACY_PREFIX)) == 0)
+		return ERR_PTR(-EINVAL);
+
 	mutex_lock(&gpio_aggregator_lock);
 	aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
 	mutex_unlock(&gpio_aggregator_lock);
@@ -1044,6 +1098,8 @@ static struct configfs_subsystem gpio_aggr_subsys = {
 static int aggr_parse(struct gpio_aggregator *aggr)
 {
 	char *args = skip_spaces(aggr->args);
+	struct gpio_aggregator_line *line;
+	char name[CONFIGFS_ITEM_NAME_LEN];
 	char *key, *offsets, *p;
 	unsigned int i, n = 0;
 	int error = 0;
@@ -1055,14 +1111,29 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 
 	args = next_arg(args, &key, &p);
 	while (*args) {
+		scnprintf(name, CONFIGFS_ITEM_NAME_LEN, "line%u", n);
+
 		args = next_arg(args, &offsets, &p);
 
 		p = get_options(offsets, 0, &error);
 		if (error == 0 || *p) {
 			/* Named GPIO line */
+			line = aggr_line_alloc(aggr, n, key, -1);
+			if (!line) {
+				error = -ENOMEM;
+				goto err;
+			}
+			config_group_init_type_name(&line->group, name,
+						    &gpio_aggr_line_type);
+			error = configfs_register_group(&aggr->group,
+							&line->group);
+			if (error)
+				goto err;
+			aggr_line_add(aggr, line);
+
 			error = aggr_add_gpio(aggr, key, U16_MAX, &n);
 			if (error)
-				return error;
+				goto err;
 
 			key = offsets;
 			continue;
@@ -1076,9 +1147,22 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 		}
 
 		for_each_set_bit(i, bitmap, AGGREGATOR_MAX_GPIOS) {
+			line = aggr_line_alloc(aggr, n, key, i);
+			if (!line) {
+				error = -ENOMEM;
+				goto err;
+			}
+			config_group_init_type_name(&line->group, name,
+						    &gpio_aggr_line_type);
+			error = configfs_register_group(&aggr->group,
+							&line->group);
+			if (error)
+				goto err;
+			aggr_line_add(aggr, line);
+
 			error = aggr_add_gpio(aggr, key, i, &n);
 			if (error)
-				return error;
+				goto err;
 		}
 
 		args = next_arg(args, &key, &p);
@@ -1086,21 +1170,35 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 
 	if (!n) {
 		pr_err("No GPIOs specified\n");
-		return -EINVAL;
+		goto err;
 	}
 
 	return 0;
+
+err:
+	aggr_unregister_lines(aggr);
+	aggr_free_lines(aggr);
+	return error;
 }
 
 static void aggr_free(struct gpio_aggregator *aggr)
 {
-	aggr_deactivate(aggr);
+	if (aggr_is_active(aggr))
+		aggr_deactivate(aggr);
+
+	aggr_unregister_lines(aggr);
+	aggr_free_lines(aggr);
+	configfs_unregister_group(&aggr->group);
+	mutex_destroy(&aggr->lock);
+	idr_remove(&gpio_aggregator_idr, aggr->id);
 	kfree(aggr);
 }
 
 static ssize_t new_device_store(struct device_driver *driver, const char *buf,
 				size_t count)
 {
+	struct gpio_aggregator_pdev_meta meta;
+	char name[CONFIGFS_ITEM_NAME_LEN];
 	struct gpio_aggregator *aggr;
 	struct platform_device *pdev;
 	int res, id;
@@ -1112,6 +1210,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
 
 	memcpy(aggr->args, buf, count + 1);
 
+	aggr->init_via_sysfs = true;
 	aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
 				GFP_KERNEL);
 	if (!aggr->lookups) {
@@ -1128,10 +1227,22 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
 		goto free_table;
 	}
 
+	scnprintf(name, CONFIGFS_ITEM_NAME_LEN,
+		  "%s.%d", AGGREGATOR_LEGACY_PREFIX, id);
+	INIT_LIST_HEAD(&aggr->list_head);
+	mutex_init(&aggr->lock);
+	config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
+	init_completion(&aggr->probe_completion);
+
+	/* Expose to configfs */
+	res = configfs_register_group(&gpio_aggr_subsys.su_group, &aggr->group);
+	if (res)
+		goto remove_idr;
+
 	aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
 	if (!aggr->lookups->dev_id) {
 		res = -ENOMEM;
-		goto remove_idr;
+		goto unregister_group;
 	}
 
 	res = aggr_parse(aggr);
@@ -1140,7 +1251,9 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
 
 	gpiod_add_lookup_table(aggr->lookups);
 
-	pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
+	meta.init_via_sysfs = true;
+
+	pdev = platform_device_register_data(NULL, DRV_NAME, id, &meta, sizeof(meta));
 	if (IS_ERR(pdev)) {
 		res = PTR_ERR(pdev);
 		goto remove_table;
@@ -1153,6 +1266,8 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
 	gpiod_remove_lookup_table(aggr->lookups);
 free_dev_id:
 	kfree(aggr->lookups->dev_id);
+unregister_group:
+	configfs_unregister_group(&aggr->group);
 remove_idr:
 	mutex_lock(&gpio_aggregator_lock);
 	idr_remove(&gpio_aggregator_idr, id);
@@ -1205,7 +1320,9 @@ ATTRIBUTE_GROUPS(gpio_aggregator);
 
 static int gpio_aggregator_probe(struct platform_device *pdev)
 {
+	struct gpio_aggregator_pdev_meta *meta;
 	struct device *dev = &pdev->dev;
+	bool init_via_sysfs = false;
 	struct gpio_desc **descs;
 	struct gpiochip_fwd *fwd;
 	unsigned long features;
@@ -1219,6 +1336,10 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 	if (!descs)
 		return -ENOMEM;
 
+	meta = dev_get_platdata(&pdev->dev);
+	if (meta && meta->init_via_sysfs)
+		init_via_sysfs = true;
+
 	for (i = 0; i < n; i++) {
 		descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
 		if (IS_ERR(descs[i]))
@@ -1226,7 +1347,7 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 	}
 
 	features = (uintptr_t)device_get_match_data(dev);
-	fwd = gpiochip_fwd_create(dev, n, descs, features);
+	fwd = gpiochip_fwd_create(dev, n, descs, features, init_via_sysfs);
 	if (IS_ERR(fwd))
 		return PTR_ERR(fwd);
 
@@ -1258,7 +1379,26 @@ static struct platform_driver gpio_aggregator_driver = {
 
 static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
 {
-	aggr_free(p);
+	/*
+	 * There should be no aggregator created via configfs, as their
+	 * presence would prevent module unloading.
+	 */
+	struct gpio_aggregator *aggr = (struct gpio_aggregator *)p;
+
+	/*
+	 * For aggregators created via legacy sysfs, some may have been
+	 * deactivated via configfs or may be pending a deferred probe.
+	 * In either case, they need to be deactivated.
+	 */
+	if (aggr_is_activating(aggr) || aggr_is_active(aggr))
+		aggr_deactivate(aggr);
+
+	/*
+	 * No need to call aggr_unregister_lines() since
+	 * configfs_unregister_subsystem() has already been invoked.
+	 */
+	aggr_free_lines(aggr);
+	kfree(aggr);
 	return 0;
 }
 
-- 
2.45.2

[PATCH v2 09/10] gpio: aggregator: cancel deferred probe for devices created via configfs

[Koichiro Den]

For aggregators initialized via configfs, write 1 to 'live' waits for
probe completion and returns an error if the probe fails, unlike the
legacy sysfs interface, which is asynchronous.

Since users control the liveness of the aggregator device and might be
editting configurations while 'live' is 0, deferred probing is both
unnatural and unsafe.

Cancel deferred probe for purely configfs-based aggregators when probe
fails.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-aggregator.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index e101b78ad524..174078e02287 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -1313,7 +1313,6 @@ static struct attribute *gpio_aggregator_attrs[] = {
 };
 ATTRIBUTE_GROUPS(gpio_aggregator);
 
-
 /*
  *  GPIO Aggregator platform device
  */
@@ -1342,8 +1341,22 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 
 	for (i = 0; i < n; i++) {
 		descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
-		if (IS_ERR(descs[i]))
+		if (IS_ERR(descs[i])) {
+			/*
+			 * Deferred probing is not suitable when the aggregator
+			 * is created by userspace. They should just retry later
+			 * whenever they like. For device creation via sysfs,
+			 * error is propagated without overriding for backward
+			 * compatibility. .prevent_deferred_probe is kept unset
+			 * for other cases.
+			 */
+			if (!init_via_sysfs && !dev_of_node(dev) &&
+			    descs[i] == ERR_PTR(-EPROBE_DEFER)) {
+				pr_warn("Deferred probe canceled for creation by userspace.\n");
+				return -ENODEV;
+			}
 			return PTR_ERR(descs[i]);
+		}
 	}
 
 	features = (uintptr_t)device_get_match_data(dev);
-- 
2.45.2

[PATCH v2 10/10] Documentation: gpio: document configfs interface for gpio-aggregator

[Koichiro Den]

Add documentation for the newly added configfs-based interface for GPIO
aggregator.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 .../admin-guide/gpio/gpio-aggregator.rst      | 93 +++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst b/Documentation/admin-guide/gpio/gpio-aggregator.rst
index 5cd1e7221756..e753f3dc4ae6 100644
--- a/Documentation/admin-guide/gpio/gpio-aggregator.rst
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -69,6 +69,99 @@ write-only attribute files in sysfs.
 		    $ echo gpio-aggregator.0 > delete_device
 
 
+Aggregating GPIOs using Configfs
+--------------------------------
+
+**Group:** ``/config/gpio-aggregator``
+
+    This is the root directory of the gpio-aggregator configfs tree.
+
+**Group:** ``/config/gpio-aggregator/<example-name>``
+
+    This directory represents a GPIO aggregator device. You can assign any
+    name to ``<example-name>`` (e.g., ``agg0``), except names starting with
+    ``_auto`` prefix, which are reserved for auto-generated configfs
+    entries corresponding to devices created via Sysfs.
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/live``
+
+    The ``live`` attribute allows to trigger the actual creation of the device
+    once it's fully configured. The accepted values are: ``1`` to enable the
+    virtual device and ``0`` to disable and tear it down.
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/dev_name``
+
+    The read-only ``dev_name`` attribute exposes the name of the device as it
+    will appear in the system on the platform bus (e.g. ``gpio-aggregator.0``).
+    This is useful for identifying a character device for the newly created
+    aggregator. If it's ``gpio-aggregator.0``,
+    ``/sys/devices/platform/gpio-aggregator.0/gpiochipX`` path tells you that the
+    GPIO device id is ``X``.
+
+You must create subdirectories for each virtual line you want to
+instantiate, named exactly as ``line0``, ``line1``, ..., ``lineY``, when
+you want to instantiate ``Y+1`` (Y >= 0) lines.  Configure all lines before
+activating the device by setting ``live`` to 1.
+
+**Group:** ``/config/gpio-aggregator/<example-name>/<lineY>/``
+
+    This directory represents a GPIO line to include in the aggregator.
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/key``
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/offset``
+
+    If ``offset`` is >= 0:
+        * ``key`` specifies the name of the chip this GPIO belongs to
+        * ``offset`` specifies the line offset within that chip.
+    If ``offset`` is <0:
+        * ``key`` needs to specify the GPIO line name.
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/name``
+
+    The ``name`` attribute sets a custom name for lineY. If left unset, the
+    line will remain unnamed.
+
+Once the configuration is done, the ``'live'`` attribute must be set to 1
+in order to instantiate the aggregator device. It can be set back to 0 to
+destroy the virtual device. The module will synchronously wait for the new
+aggregator device to be successfully probed and if this doesn't happen, writing
+to ``'live'`` will result in an error. This is a different behaviour from the
+case when you create it using sysfs ``new_device`` interface.
+
+.. note::
+
+   For aggregators created via Sysfs, the configfs entries are
+   auto-generated and appear as ``/config/gpio-aggregator/_auto.<N>/``. You
+   cannot add or remove line directories with mkdir(2)/rmdir(2). To modify
+   lines, you must use the "delete_device" interface to tear down the
+   existing device and reconfigure it from scratch. However, you can still
+   toggle the aggregator with the ``live`` attribute and adjust the
+   ``key``, ``offset``, and ``name`` attributes for each line when ``live``
+   is set to 0 by hand (i.e. it's not waiting for deferred probe).
+
+Sample configuration commands
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. code-block:: sh
+
+    # Create a directory for an aggregator device
+    $ mkdir /sys/kernel/config/gpio-aggregator/agg0
+
+    # Configure each line
+    $ mkdir /sys/kernel/config/gpio-aggregator/agg0/line0
+    $ echo gpiochip0 > /sys/kernel/config/gpio-aggregator/agg0/line0/key
+    $ echo 6         > /sys/kernel/config/gpio-aggregator/agg0/line0/offset
+    $ echo test0     > /sys/kernel/config/gpio-aggregator/agg0/line0/name
+    $ mkdir /sys/kernel/config/gpio-aggregator/agg0/line1
+    $ echo gpiochip0 > /sys/kernel/config/gpio-aggregator/agg0/line1/key
+    $ echo 7         > /sys/kernel/config/gpio-aggregator/agg0/line1/offset
+    $ echo test1     > /sys/kernel/config/gpio-aggregator/agg0/line1/name
+
+    # Activate the aggregator device
+    $ echo 1         > /sys/kernel/config/gpio-aggregator/agg0/live
+
+
 Generic GPIO Driver
 -------------------
 
-- 
2.45.2

Re: [PATCH v2 02/10] gpio: aggregator: introduce basic configfs interface

[Bartosz Golaszewski]

On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> The existing sysfs 'new_device' interface has several limitations:
> * No way to determine when GPIO aggregator creation is complete.
> * No way to retrieve errors when creating a GPIO aggregator.
> * No way to trace a GPIO line of an aggregator back to its
>   corresponding physical device.
> * The 'new_device' echo does not indicate which virtual gpiochip<N>
>   was created.
> * No way to assign names to GPIO lines exported through an aggregator.
>
> Introduce the new configfs interface for gpio-aggregator to address
> these limitations. It provides a more streamlined, modern, and
> extensible configuration method. For backward compatibility, the
> 'new_device' interface and its behaviour is retained for now.
>
> This commit implements minimal functionalities:
>
>   /config/gpio-aggregator/<name-of-your-choice>/
>   /config/gpio-aggregator/<name-of-your-choice>/live
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
>
> Basic setup flow is:
> 1. Create a directory for a GPIO aggregator.
> 2. Create subdirectories for each line you want to instantiate.
> 3. In each line directory, configure the key and offset.
>    The key/offset semantics are as follows:
>    * If offset is >= 0:
>      - key specifies the name of the chip this GPIO belongs to
>      - offset specifies the line offset within that chip.
>    * If offset is <0:
>      - key needs to specify the GPIO line name.
> 4. Return to the aggregator's root directory and write '1' to the live
>    attribute.
>
> For example, the command in the existing kernel doc:
>
>   echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
>
> is equivalent to:
>
>   mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
>   # Change <custom-name> to name of your choice (e.g. "aggr0")
>   cd /sys/kernel/config/gpio-aggregator/<custom-name>
>   mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
>   echo e6052000.gpio > line0/key
>   echo 19            > line0/offset
>   echo e6050000.gpio > line1/key
>   echo 20            > line1/offset
>   echo e6050000.gpio > line2/key
>   echo 21            > line2/offset
>   echo 1             > live
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
>  drivers/gpio/gpio-aggregator.c | 549 ++++++++++++++++++++++++++++++++-
>  1 file changed, 548 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index 570cd1ff8cc2..c63cf3067ce7 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -9,10 +9,14 @@
>
>  #include <linux/bitmap.h>
>  #include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/configfs.h>
>  #include <linux/ctype.h>
>  #include <linux/delay.h>
>  #include <linux/idr.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/lockdep.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -34,11 +38,39 @@
>   */
>
>  struct gpio_aggregator {
> +       struct config_group group;
>         struct gpiod_lookup_table *lookups;
>         struct platform_device *pdev;
> +       struct mutex lock;
> +       int id;
> +
> +       /* Synchronize with probe */
> +       struct notifier_block bus_notifier;
> +       struct completion probe_completion;
> +       bool driver_bound;
> +
> +       /* List of gpio_aggregator_line. Always added in order */
> +       struct list_head list_head;
> +
>         char args[];
>  };
>
> +struct gpio_aggregator_line {
> +       struct config_group group;
> +       struct gpio_aggregator *parent;
> +       struct list_head entry;
> +
> +       /* Line index within the aggregator device */
> +       int idx;
> +
> +       /* GPIO chip label or line name */
> +       char *key;
> +       /* Can be negative to indicate lookup by line name */
> +       int offset;
> +
> +       enum gpio_lookup_flags flags;
> +};
> +
>  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
>  static DEFINE_IDR(gpio_aggregator_idr);
>
> @@ -61,6 +93,97 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
>         return 0;
>  }
>
> +static int aggr_notifier_call(struct notifier_block *nb, unsigned long action,
> +                             void *data)
> +{
> +       struct gpio_aggregator *aggr;
> +       struct device *dev = data;
> +
> +       aggr = container_of(nb, struct gpio_aggregator, bus_notifier);
> +       if (!device_match_name(dev, aggr->lookups->dev_id))
> +               return NOTIFY_DONE;
> +
> +       switch (action) {
> +       case BUS_NOTIFY_BOUND_DRIVER:
> +               aggr->driver_bound = true;
> +               break;
> +       case BUS_NOTIFY_DRIVER_NOT_BOUND:
> +               aggr->driver_bound = false;
> +               break;
> +       default:
> +               return NOTIFY_DONE;
> +       }
> +
> +       complete(&aggr->probe_completion);
> +       return NOTIFY_OK;
> +}

Suggestion: this is the third time we're seeing this mechanism being
used (after gpio-sim and gpio-virtuser). Maybe it's time to try and
abstract it as much of the code is shared?

The rest looks good to me.

Bart

Re: [PATCH v2 03/10] gpio: aggregator: add read-only 'dev_name' configfs attribute

[Bartosz Golaszewski]

On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> Add a read-only 'dev_name' attribute to configfs interface, which
> exposes the platform bus device name. Users can easily identify which
> gpiochip<N> has been created as follows:
>
> $ cat /sys/kernel/config/gpio-aggregator/<aggregator-name>/dev_name
>   gpio-aggregator.0
> $ ls -d /sys/devices/platform/gpio-aggregator.0/gpiochip*
>   gpiochip3
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
>  drivers/gpio/gpio-aggregator.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index c63cf3067ce7..76d3a8677308 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -732,6 +732,23 @@ static struct configfs_attribute *gpio_aggr_line_attrs[] = {
>         NULL
>  };
>
> +static ssize_t
> +gpio_aggr_device_dev_name_show(struct config_item *item, char *page)
> +{
> +       struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> +       struct platform_device *pdev;
> +
> +       guard(mutex)(&aggr->lock);
> +
> +       pdev = aggr->pdev;
> +       if (pdev)
> +               return sprintf(page, "%s\n", dev_name(&pdev->dev));
> +
> +       return sprintf(page, "%s.%d\n", DRV_NAME, aggr->id);
> +}
> +
> +CONFIGFS_ATTR_RO(gpio_aggr_device_, dev_name);
> +
>  static ssize_t
>  gpio_aggr_device_live_show(struct config_item *item, char *page)
>  {
> @@ -781,6 +798,7 @@ gpio_aggr_device_live_store(struct config_item *item, const char *page,
>  CONFIGFS_ATTR(gpio_aggr_device_, live);
>
>  static struct configfs_attribute *gpio_aggr_device_attrs[] = {
> +       &gpio_aggr_device_attr_dev_name,
>         &gpio_aggr_device_attr_live,
>         NULL
>  };
> --
> 2.45.2
>

I don't understand why this isn't part of the previous patch?

Bart

Re: [PATCH v2 08/10] gpio: aggregator: expoose aggregator created via legacy sysfs to configfs

[Bartosz Golaszewski]

On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> Expose settings for aggregators created using the sysfs 'new_device'
> interface to configfs. Once written to 'new_device', an "_auto.<N>" path

I would prefer this to be called "_sysfs.<N>" as it's not really
"automatic" - the user did create this, just with a different
interface.

> appears in the configfs regardless of whether the probe succeeds.
> Consequently, users can no longer use that prefix for custom GPIO
> aggregator names. The 'live' attribute changes to 1 when the probe
> succeeds and the GPIO forwarder is instantiated.
>
> Note that the aggregator device created via sysfs is asynchrnous, i.e.
> writing into 'new_device' returns without waiting for probe completion,
> and the probe may succeed, fail, or eventually succeed via deferred
> probe. Thus, the 'live' attribute may change from 0 to 1 asynchronously
> without notice. So, editting key/offset/name while it's waiting for
> deferred probe is prohibited.
>
> The configfs auto-generation relies on create_default_group(), which
> inherently prohibits rmdir(2). To align with the limitation, this commit
> also prohibits mkdir(2) for them. When users want to change the number
> of lines for an aggregator initialized via 'new_device', they need to
> tear down the device using 'delete_device' and reconfigure it from
> scratch. This does not break previous behaviour; users of legacy sysfs
> interface simply gain additional almost read-only configfs exposure.
>
> Still, users can write into 'live' attribute to toggle the device unless
> it's waiting for deferred probe. So once probe succeeds, they can
> deactivate it in the same manner as the devices initialized via
> configfs.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---

Bart

Re: [PATCH v2 09/10] gpio: aggregator: cancel deferred probe for devices created via configfs

[Bartosz Golaszewski]

On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> For aggregators initialized via configfs, write 1 to 'live' waits for
> probe completion and returns an error if the probe fails, unlike the
> legacy sysfs interface, which is asynchronous.
>
> Since users control the liveness of the aggregator device and might be
> editting configurations while 'live' is 0, deferred probing is both
> unnatural and unsafe.
>
> Cancel deferred probe for purely configfs-based aggregators when probe
> fails.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
>  drivers/gpio/gpio-aggregator.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index e101b78ad524..174078e02287 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -1313,7 +1313,6 @@ static struct attribute *gpio_aggregator_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(gpio_aggregator);
>
> -
>  /*
>   *  GPIO Aggregator platform device
>   */
> @@ -1342,8 +1341,22 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
>
>         for (i = 0; i < n; i++) {
>                 descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> -               if (IS_ERR(descs[i]))
> +               if (IS_ERR(descs[i])) {
> +                       /*
> +                        * Deferred probing is not suitable when the aggregator
> +                        * is created by userspace. They should just retry later
> +                        * whenever they like. For device creation via sysfs,
> +                        * error is propagated without overriding for backward
> +                        * compatibility. .prevent_deferred_probe is kept unset
> +                        * for other cases.
> +                        */
> +                       if (!init_via_sysfs && !dev_of_node(dev) &&
> +                           descs[i] == ERR_PTR(-EPROBE_DEFER)) {
> +                               pr_warn("Deferred probe canceled for creation by userspace.\n");
> +                               return -ENODEV;
> +                       }
>                         return PTR_ERR(descs[i]);
> +               }
>         }
>
>         features = (uintptr_t)device_get_match_data(dev);
> --
> 2.45.2
>

Geert, what do you think about making the sysfs interface synchronous
instead? I would argue it's actually more logical as the user will
instinctively expect the aggregator to be ready after write() to
new_device returns.

Bart

Re: [PATCH v2 02/10] gpio: aggregator: introduce basic configfs interface

[Koichiro Den]

On Tue, Feb 04, 2025 at 01:48:49PM GMT, Bartosz Golaszewski wrote:
> On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > The existing sysfs 'new_device' interface has several limitations:
> > * No way to determine when GPIO aggregator creation is complete.
> > * No way to retrieve errors when creating a GPIO aggregator.
> > * No way to trace a GPIO line of an aggregator back to its
> >   corresponding physical device.
> > * The 'new_device' echo does not indicate which virtual gpiochip<N>
> >   was created.
> > * No way to assign names to GPIO lines exported through an aggregator.
> >
> > Introduce the new configfs interface for gpio-aggregator to address
> > these limitations. It provides a more streamlined, modern, and
> > extensible configuration method. For backward compatibility, the
> > 'new_device' interface and its behaviour is retained for now.
> >
> > This commit implements minimal functionalities:
> >
> >   /config/gpio-aggregator/<name-of-your-choice>/
> >   /config/gpio-aggregator/<name-of-your-choice>/live
> >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
> >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
> >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
> >
> > Basic setup flow is:
> > 1. Create a directory for a GPIO aggregator.
> > 2. Create subdirectories for each line you want to instantiate.
> > 3. In each line directory, configure the key and offset.
> >    The key/offset semantics are as follows:
> >    * If offset is >= 0:
> >      - key specifies the name of the chip this GPIO belongs to
> >      - offset specifies the line offset within that chip.
> >    * If offset is <0:
> >      - key needs to specify the GPIO line name.
> > 4. Return to the aggregator's root directory and write '1' to the live
> >    attribute.
> >
> > For example, the command in the existing kernel doc:
> >
> >   echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
> >
> > is equivalent to:
> >
> >   mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
> >   # Change <custom-name> to name of your choice (e.g. "aggr0")
> >   cd /sys/kernel/config/gpio-aggregator/<custom-name>
> >   mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
> >   echo e6052000.gpio > line0/key
> >   echo 19            > line0/offset
> >   echo e6050000.gpio > line1/key
> >   echo 20            > line1/offset
> >   echo e6050000.gpio > line2/key
> >   echo 21            > line2/offset
> >   echo 1             > live
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
> >  drivers/gpio/gpio-aggregator.c | 549 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 548 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> > index 570cd1ff8cc2..c63cf3067ce7 100644
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -9,10 +9,14 @@
> >
> >  #include <linux/bitmap.h>
> >  #include <linux/bitops.h>
> > +#include <linux/completion.h>
> > +#include <linux/configfs.h>
> >  #include <linux/ctype.h>
> >  #include <linux/delay.h>
> >  #include <linux/idr.h>
> >  #include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/lockdep.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > @@ -34,11 +38,39 @@
> >   */
> >
> >  struct gpio_aggregator {
> > +       struct config_group group;
> >         struct gpiod_lookup_table *lookups;
> >         struct platform_device *pdev;
> > +       struct mutex lock;
> > +       int id;
> > +
> > +       /* Synchronize with probe */
> > +       struct notifier_block bus_notifier;
> > +       struct completion probe_completion;
> > +       bool driver_bound;
> > +
> > +       /* List of gpio_aggregator_line. Always added in order */
> > +       struct list_head list_head;
> > +
> >         char args[];
> >  };
> >
> > +struct gpio_aggregator_line {
> > +       struct config_group group;
> > +       struct gpio_aggregator *parent;
> > +       struct list_head entry;
> > +
> > +       /* Line index within the aggregator device */
> > +       int idx;
> > +
> > +       /* GPIO chip label or line name */
> > +       char *key;
> > +       /* Can be negative to indicate lookup by line name */
> > +       int offset;
> > +
> > +       enum gpio_lookup_flags flags;
> > +};
> > +
> >  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
> >  static DEFINE_IDR(gpio_aggregator_idr);
> >
> > @@ -61,6 +93,97 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
> >         return 0;
> >  }
> >
> > +static int aggr_notifier_call(struct notifier_block *nb, unsigned long action,
> > +                             void *data)
> > +{
> > +       struct gpio_aggregator *aggr;
> > +       struct device *dev = data;
> > +
> > +       aggr = container_of(nb, struct gpio_aggregator, bus_notifier);
> > +       if (!device_match_name(dev, aggr->lookups->dev_id))
> > +               return NOTIFY_DONE;
> > +
> > +       switch (action) {
> > +       case BUS_NOTIFY_BOUND_DRIVER:
> > +               aggr->driver_bound = true;
> > +               break;
> > +       case BUS_NOTIFY_DRIVER_NOT_BOUND:
> > +               aggr->driver_bound = false;
> > +               break;
> > +       default:
> > +               return NOTIFY_DONE;
> > +       }
> > +
> > +       complete(&aggr->probe_completion);
> > +       return NOTIFY_OK;
> > +}
> 
> Suggestion: this is the third time we're seeing this mechanism being
> used (after gpio-sim and gpio-virtuser). Maybe it's time to try and
> abstract it as much of the code is shared?

That makes sense. I would add gpiolib-vgpio.[ch] and add an opaque
* struct vgpio_common
and two api functions as our first step:
* vgpio_init(get_devname_cb) # bus notifier calls the cb to get devname
* vgpio_register_pdev_sync(pdevinfo) # return -ENXIO when probe fails
What do you think?

Thanks,
Koichiro

> 
> The rest looks good to me.
> 
> Bart

Re: [PATCH v2 03/10] gpio: aggregator: add read-only 'dev_name' configfs attribute

[Koichiro Den]

On Tue, Feb 04, 2025 at 01:49:22PM GMT, Bartosz Golaszewski wrote:
> On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > Add a read-only 'dev_name' attribute to configfs interface, which
> > exposes the platform bus device name. Users can easily identify which
> > gpiochip<N> has been created as follows:
> >
> > $ cat /sys/kernel/config/gpio-aggregator/<aggregator-name>/dev_name
> >   gpio-aggregator.0
> > $ ls -d /sys/devices/platform/gpio-aggregator.0/gpiochip*
> >   gpiochip3
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
> >  drivers/gpio/gpio-aggregator.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> > index c63cf3067ce7..76d3a8677308 100644
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -732,6 +732,23 @@ static struct configfs_attribute *gpio_aggr_line_attrs[] = {
> >         NULL
> >  };
> >
> > +static ssize_t
> > +gpio_aggr_device_dev_name_show(struct config_item *item, char *page)
> > +{
> > +       struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> > +       struct platform_device *pdev;
> > +
> > +       guard(mutex)(&aggr->lock);
> > +
> > +       pdev = aggr->pdev;
> > +       if (pdev)
> > +               return sprintf(page, "%s\n", dev_name(&pdev->dev));
> > +
> > +       return sprintf(page, "%s.%d\n", DRV_NAME, aggr->id);
> > +}
> > +
> > +CONFIGFS_ATTR_RO(gpio_aggr_device_, dev_name);
> > +
> >  static ssize_t
> >  gpio_aggr_device_live_show(struct config_item *item, char *page)
> >  {
> > @@ -781,6 +798,7 @@ gpio_aggr_device_live_store(struct config_item *item, const char *page,
> >  CONFIGFS_ATTR(gpio_aggr_device_, live);
> >
> >  static struct configfs_attribute *gpio_aggr_device_attrs[] = {
> > +       &gpio_aggr_device_attr_dev_name,
> >         &gpio_aggr_device_attr_live,
> >         NULL
> >  };
> > --
> > 2.45.2
> >
> 
> I don't understand why this isn't part of the previous patch?

I just separated it to keep the previous one minimal as possible :)
Ok, I'll squash it.

Koichiro

> 
> Bart

Re: [PATCH v2 08/10] gpio: aggregator: expoose aggregator created via legacy sysfs to configfs

[Koichiro Den]

On Tue, Feb 04, 2025 at 02:12:33PM GMT, Bartosz Golaszewski wrote:
> On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > Expose settings for aggregators created using the sysfs 'new_device'
> > interface to configfs. Once written to 'new_device', an "_auto.<N>" path
> 
> I would prefer this to be called "_sysfs.<N>" as it's not really
> "automatic" - the user did create this, just with a different
> interface.

Makes sense, I'll change it in v3.

Thanks,
Koichiro

> 
> > appears in the configfs regardless of whether the probe succeeds.
> > Consequently, users can no longer use that prefix for custom GPIO
> > aggregator names. The 'live' attribute changes to 1 when the probe
> > succeeds and the GPIO forwarder is instantiated.
> >
> > Note that the aggregator device created via sysfs is asynchrnous, i.e.
> > writing into 'new_device' returns without waiting for probe completion,
> > and the probe may succeed, fail, or eventually succeed via deferred
> > probe. Thus, the 'live' attribute may change from 0 to 1 asynchronously
> > without notice. So, editting key/offset/name while it's waiting for
> > deferred probe is prohibited.
> >
> > The configfs auto-generation relies on create_default_group(), which
> > inherently prohibits rmdir(2). To align with the limitation, this commit
> > also prohibits mkdir(2) for them. When users want to change the number
> > of lines for an aggregator initialized via 'new_device', they need to
> > tear down the device using 'delete_device' and reconfigure it from
> > scratch. This does not break previous behaviour; users of legacy sysfs
> > interface simply gain additional almost read-only configfs exposure.
> >
> > Still, users can write into 'live' attribute to toggle the device unless
> > it's waiting for deferred probe. So once probe succeeds, they can
> > deactivate it in the same manner as the devices initialized via
> > configfs.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
> 
> Bart

Re: [PATCH v2 02/10] gpio: aggregator: introduce basic configfs interface

[Koichiro Den]

On Tue, Feb 04, 2025 at 11:41:16PM GMT, Koichiro Den wrote:
> On Tue, Feb 04, 2025 at 01:48:49PM GMT, Bartosz Golaszewski wrote:
> > On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> > >
> > > The existing sysfs 'new_device' interface has several limitations:
> > > * No way to determine when GPIO aggregator creation is complete.
> > > * No way to retrieve errors when creating a GPIO aggregator.
> > > * No way to trace a GPIO line of an aggregator back to its
> > >   corresponding physical device.
> > > * The 'new_device' echo does not indicate which virtual gpiochip<N>
> > >   was created.
> > > * No way to assign names to GPIO lines exported through an aggregator.
> > >
> > > Introduce the new configfs interface for gpio-aggregator to address
> > > these limitations. It provides a more streamlined, modern, and
> > > extensible configuration method. For backward compatibility, the
> > > 'new_device' interface and its behaviour is retained for now.
> > >
> > > This commit implements minimal functionalities:
> > >
> > >   /config/gpio-aggregator/<name-of-your-choice>/
> > >   /config/gpio-aggregator/<name-of-your-choice>/live
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
> > >
> > > Basic setup flow is:
> > > 1. Create a directory for a GPIO aggregator.
> > > 2. Create subdirectories for each line you want to instantiate.
> > > 3. In each line directory, configure the key and offset.
> > >    The key/offset semantics are as follows:
> > >    * If offset is >= 0:
> > >      - key specifies the name of the chip this GPIO belongs to
> > >      - offset specifies the line offset within that chip.
> > >    * If offset is <0:
> > >      - key needs to specify the GPIO line name.
> > > 4. Return to the aggregator's root directory and write '1' to the live
> > >    attribute.
> > >
> > > For example, the command in the existing kernel doc:
> > >
> > >   echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
> > >
> > > is equivalent to:
> > >
> > >   mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
> > >   # Change <custom-name> to name of your choice (e.g. "aggr0")
> > >   cd /sys/kernel/config/gpio-aggregator/<custom-name>
> > >   mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
> > >   echo e6052000.gpio > line0/key
> > >   echo 19            > line0/offset
> > >   echo e6050000.gpio > line1/key
> > >   echo 20            > line1/offset
> > >   echo e6050000.gpio > line2/key
> > >   echo 21            > line2/offset
> > >   echo 1             > live
> > >
> > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > > ---
> > >  drivers/gpio/gpio-aggregator.c | 549 ++++++++++++++++++++++++++++++++-
> > >  1 file changed, 548 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> > > index 570cd1ff8cc2..c63cf3067ce7 100644
> > > --- a/drivers/gpio/gpio-aggregator.c
> > > +++ b/drivers/gpio/gpio-aggregator.c
> > > @@ -9,10 +9,14 @@
> > >
> > >  #include <linux/bitmap.h>
> > >  #include <linux/bitops.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/configfs.h>
> > >  #include <linux/ctype.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/idr.h>
> > >  #include <linux/kernel.h>
> > > +#include <linux/list.h>
> > > +#include <linux/lockdep.h>
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>
> > > @@ -34,11 +38,39 @@
> > >   */
> > >
> > >  struct gpio_aggregator {
> > > +       struct config_group group;
> > >         struct gpiod_lookup_table *lookups;
> > >         struct platform_device *pdev;
> > > +       struct mutex lock;
> > > +       int id;
> > > +
> > > +       /* Synchronize with probe */
> > > +       struct notifier_block bus_notifier;
> > > +       struct completion probe_completion;
> > > +       bool driver_bound;
> > > +
> > > +       /* List of gpio_aggregator_line. Always added in order */
> > > +       struct list_head list_head;
> > > +
> > >         char args[];
> > >  };
> > >
> > > +struct gpio_aggregator_line {
> > > +       struct config_group group;
> > > +       struct gpio_aggregator *parent;
> > > +       struct list_head entry;
> > > +
> > > +       /* Line index within the aggregator device */
> > > +       int idx;
> > > +
> > > +       /* GPIO chip label or line name */
> > > +       char *key;
> > > +       /* Can be negative to indicate lookup by line name */
> > > +       int offset;
> > > +
> > > +       enum gpio_lookup_flags flags;
> > > +};
> > > +
> > >  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
> > >  static DEFINE_IDR(gpio_aggregator_idr);
> > >
> > > @@ -61,6 +93,97 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
> > >         return 0;
> > >  }
> > >
> > > +static int aggr_notifier_call(struct notifier_block *nb, unsigned long action,
> > > +                             void *data)
> > > +{
> > > +       struct gpio_aggregator *aggr;
> > > +       struct device *dev = data;
> > > +
> > > +       aggr = container_of(nb, struct gpio_aggregator, bus_notifier);
> > > +       if (!device_match_name(dev, aggr->lookups->dev_id))
> > > +               return NOTIFY_DONE;
> > > +
> > > +       switch (action) {
> > > +       case BUS_NOTIFY_BOUND_DRIVER:
> > > +               aggr->driver_bound = true;
> > > +               break;
> > > +       case BUS_NOTIFY_DRIVER_NOT_BOUND:
> > > +               aggr->driver_bound = false;
> > > +               break;
> > > +       default:
> > > +               return NOTIFY_DONE;
> > > +       }
> > > +
> > > +       complete(&aggr->probe_completion);
> > > +       return NOTIFY_OK;
> > > +}
> > 
> > Suggestion: this is the third time we're seeing this mechanism being
> > used (after gpio-sim and gpio-virtuser). Maybe it's time to try and
> > abstract it as much of the code is shared?
> 
> That makes sense. I would add gpiolib-vgpio.[ch] and add an opaque
> * struct vgpio_common
> and two api functions as our first step:
> * vgpio_init(get_devname_cb) # bus notifier calls the cb to get devname
> * vgpio_register_pdev_sync(pdevinfo) # return -ENXIO when probe fails
> What do you think?

The previous comment was a bit too rough, sorry. I feel it's not a good
idea to write down detailed design plan here, so I'm thinking of having it
reviewed in v3. Please disregard the above comment.
I also noticed that this v2 patch series lacks 'select CONFIGFS_FS' for
CONFIG_GPIO_AGGREGATOR. I'll add it in v3.

In any case, I'll wait for further progress in the discussion at:
https://lore.kernel.org/all/CAMRc=Meb633zVgemPSeNtnm8oJmk=njcr2CQQbD5UJd=tBC5Zg@mail.gmail.com/

Thanks for the review.

Koichiro

> 
> Thanks,
> Koichiro
> 
> > 
> > The rest looks good to me.
> > 
> > Bart

Re: [PATCH v2 01/10] gpio: aggregator: reorder functions to prepare for configfs introduction

[Geert Uytterhoeven]

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> Reorder functions in drivers/gpio/gpio-aggregator.c to prepare for the
> configfs-based interface additions in subsequent commits. Arrange the
> code so that the configfs implementations will appear above the existing
> sysfs-specific code, since the latter will partly depend on the configfs
> interface implementations when it starts to expose the settings to
> configfs.
>
> The order in drivers/gpio/gpio-aggregator.c will be as follows:
>
> * Basic gpio_aggregator/gpio_aggregator_line representations
> * Common utility functions
> * GPIO Forwarder implementations
> * Configfs interface implementations
> * Sysfs interface implementations
> * Platform device implementations
> * Module init/exit implementations
>
> This separate commit ensures a clean diff for the subsequent commits.
>
> No functional change.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.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 00/10] Introduce configfs-based interface for gpio-aggregator

[Geert Uytterhoeven]

Hi Den-san,

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> This patch series introduces a configfs-based interface to gpio-aggregator
> to address limitations in the existing 'new_device' interface.
>
> The existing 'new_device' interface has several limitations:
>
>   #1. No way to determine when GPIO aggregator creation is complete.
>   #2. No way to retrieve errors when creating a GPIO aggregator.
>   #3. No way to trace a GPIO line of an aggregator back to its
>       corresponding physical device.
>   #4. The 'new_device' echo does not indicate which virtual gpiochip<N>
>       was created.
>   #5. No way to assign names to GPIO lines exported through an aggregator.
>
> Although Issue#1 to #3 could technically be resolved easily without
> configfs, using configfs offers a streamlined, modern, and extensible
> approach, especially since gpio-sim and gpio-virtuser already utilize
> configfs.

Thanks for your series!

I gave it a try using all three ways of configuration (sysfs, configs,
DT), and it works fine!

Tested-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 02/10] gpio: aggregator: introduce basic configfs interface

[Geert Uytterhoeven]

Hi Den-san,

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> The existing sysfs 'new_device' interface has several limitations:
> * No way to determine when GPIO aggregator creation is complete.
> * No way to retrieve errors when creating a GPIO aggregator.
> * No way to trace a GPIO line of an aggregator back to its
>   corresponding physical device.
> * The 'new_device' echo does not indicate which virtual gpiochip<N>
>   was created.
> * No way to assign names to GPIO lines exported through an aggregator.
>
> Introduce the new configfs interface for gpio-aggregator to address
> these limitations. It provides a more streamlined, modern, and
> extensible configuration method. For backward compatibility, the
> 'new_device' interface and its behaviour is retained for now.
>
> This commit implements minimal functionalities:
>
>   /config/gpio-aggregator/<name-of-your-choice>/
>   /config/gpio-aggregator/<name-of-your-choice>/live
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
>
> Basic setup flow is:
> 1. Create a directory for a GPIO aggregator.
> 2. Create subdirectories for each line you want to instantiate.
> 3. In each line directory, configure the key and offset.
>    The key/offset semantics are as follows:
>    * If offset is >= 0:
>      - key specifies the name of the chip this GPIO belongs to
>      - offset specifies the line offset within that chip.
>    * If offset is <0:
>      - key needs to specify the GPIO line name.
> 4. Return to the aggregator's root directory and write '1' to the live
>    attribute.
>
> For example, the command in the existing kernel doc:
>
>   echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
>
> is equivalent to:
>
>   mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
>   # Change <custom-name> to name of your choice (e.g. "aggr0")
>   cd /sys/kernel/config/gpio-aggregator/<custom-name>
>   mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
>   echo e6052000.gpio > line0/key
>   echo 19            > line0/offset
>   echo e6050000.gpio > line1/key
>   echo 20            > line1/offset
>   echo e6050000.gpio > line2/key
>   echo 21            > line2/offset
>   echo 1             > live
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -9,10 +9,14 @@
>
>  #include <linux/bitmap.h>
>  #include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/configfs.h>

Using configfs requires CONFIG_CONFIGFS_FS.
So either GPIO_AGGREGATOR should select CONFIGFS_FS, or
all configfs-related code should be protected by checks for
CONFIG_CONFIGFS_FS.  Since we want to encourage people to use the new
API, I think the former is preferred.

>  #include <linux/ctype.h>
>  #include <linux/delay.h>
>  #include <linux/idr.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/lockdep.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -34,11 +38,39 @@
>   */
>
>  struct gpio_aggregator {
> +       struct config_group group;
>         struct gpiod_lookup_table *lookups;
>         struct platform_device *pdev;
> +       struct mutex lock;
> +       int id;
> +
> +       /* Synchronize with probe */
> +       struct notifier_block bus_notifier;
> +       struct completion probe_completion;
> +       bool driver_bound;
> +
> +       /* List of gpio_aggregator_line. Always added in order */
> +       struct list_head list_head;
> +
>         char args[];
>  };
>
> +struct gpio_aggregator_line {
> +       struct config_group group;
> +       struct gpio_aggregator *parent;
> +       struct list_head entry;
> +
> +       /* Line index within the aggregator device */
> +       int idx;

unsigned int

> +
> +       /* GPIO chip label or line name */
> +       char *key;
> +       /* Can be negative to indicate lookup by line name */
> +       int offset;
> +
> +       enum gpio_lookup_flags flags;
> +};
> +
>  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
>  static DEFINE_IDR(gpio_aggregator_idr);

> +static bool aggr_is_active(struct gpio_aggregator *aggr)
> +{
> +       lockdep_assert_held(&aggr->lock);
> +
> +       return !!aggr->pdev && platform_get_drvdata(aggr->pdev);

No need for "!!".

> +}

> +static void aggr_line_add(struct gpio_aggregator *aggr,
> +                         struct gpio_aggregator_line *line)
> +{
> +       struct gpio_aggregator_line *tmp;
> +
> +       lockdep_assert_held(&aggr->lock);
> +
> +       list_for_each_entry(tmp, &aggr->list_head, entry) {
> +               if (WARN_ON(tmp->idx == line->idx))

Can this really happen?

> +                       return;
> +               if (tmp->idx > line->idx) {
> +                       list_add_tail(&line->entry, &tmp->entry);
> +                       return;
> +               }
> +       }
> +       list_add_tail(&line->entry, &aggr->list_head);
> +}

> +static void aggr_lockup_configfs(struct gpio_aggregator *aggr, bool lock)
> +{
> +       struct configfs_subsystem *subsys = aggr->group.cg_subsys;
> +       struct gpio_aggregator_line *line;
> +
> +       /*
> +        * The device only needs to depend on leaf lines. This is
> +        * sufficient to lock up all the configfs entries that the
> +        * instantiated, alive device depends on.
> +        */
> +       list_for_each_entry(line, &aggr->list_head, entry) {
> +               if (lock)
> +                       WARN_ON(configfs_depend_item_unlocked(
> +                                       subsys, &line->group.cg_item));

Can this happen?
I am also worried if this can be triggered by the user, combined
with panic_on_warn.

> +               else
> +                       configfs_undepend_item_unlocked(
> +                                       &line->group.cg_item);
> +       }
> +}
> +
> +static ssize_t
> +gpio_aggr_line_key_show(struct config_item *item, char *page)
> +{
> +       struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
> +       struct gpio_aggregator *aggr = line->parent;
> +
> +       guard(mutex)(&aggr->lock);
> +
> +       return sprintf(page, "%s\n", line->key ?: "");

Please use sysfs_emit() instead (everywhere).


> +}

> +static ssize_t
> +gpio_aggr_device_live_store(struct config_item *item, const char *page,
> +                           size_t count)
> +{
> +       struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> +       int ret = 0;
> +       bool live;
> +
> +       ret = kstrtobool(page, &live);
> +       if (ret)
> +               return ret;
> +
> +       if (live)
> +               aggr_lockup_configfs(aggr, true);
> +
> +       scoped_guard(mutex, &aggr->lock) {
> +               if (live == aggr_is_active(aggr))
> +                       ret = -EPERM;
> +               else if (live)
> +                       ret = aggr_activate(aggr);
> +               else
> +                       aggr_deactivate(aggr);
> +       }
> +
> +       /*
> +        * Undepend is required only if device disablement (live == 0)

s/Undepend/Lock-up/?

> +        * succeeds or if device enablement (live == 1) fails.
> +        */
> +       if (live == !!ret)
> +               aggr_lockup_configfs(aggr, false);
> +
> +       return ret ?: count;
> +}

> +static struct config_group *
> +gpio_aggr_make_group(struct config_group *group, const char *name)
> +{
> +       /* arg space is unneeded */
> +       struct gpio_aggregator *aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
> +       if (!aggr)
> +               return ERR_PTR(-ENOMEM);
> +
> +       mutex_lock(&gpio_aggregator_lock);
> +       aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> +       mutex_unlock(&gpio_aggregator_lock);
> +
> +       if (aggr->id < 0) {
> +               kfree(aggr);
> +               return ERR_PTR(aggr->id);
> +       }

The above more or less duplicates the existing code in
new_device_store().

> +
> +       INIT_LIST_HEAD(&aggr->list_head);
> +       mutex_init(&aggr->lock);
> +       config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
> +       init_completion(&aggr->probe_completion);
> +
> +       return &aggr->group;
> +}

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 03/10] gpio: aggregator: add read-only 'dev_name' configfs attribute

[Geert Uytterhoeven]

Hi Den-san,

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> Add a read-only 'dev_name' attribute to configfs interface, which
> exposes the platform bus device name. Users can easily identify which
> gpiochip<N> has been created as follows:
>
> $ cat /sys/kernel/config/gpio-aggregator/<aggregator-name>/dev_name
>   gpio-aggregator.0
> $ ls -d /sys/devices/platform/gpio-aggregator.0/gpiochip*
>   gpiochip3
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -732,6 +732,23 @@ static struct configfs_attribute *gpio_aggr_line_attrs[] = {
>         NULL
>  };
>
> +static ssize_t
> +gpio_aggr_device_dev_name_show(struct config_item *item, char *page)
> +{
> +       struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> +       struct platform_device *pdev;
> +
> +       guard(mutex)(&aggr->lock);
> +
> +       pdev = aggr->pdev;
> +       if (pdev)
> +               return sprintf(page, "%s\n", dev_name(&pdev->dev));
> +
> +       return sprintf(page, "%s.%d\n", DRV_NAME, aggr->id);

sysfs_emit(), for both branches.

> +}
> +

Please drop this blank line (everywhere).

> +CONFIGFS_ATTR_RO(gpio_aggr_device_, dev_name);
> +
>  static ssize_t
>  gpio_aggr_device_live_show(struct config_item *item, char *page)
>  {

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 04/10] gpio: aggregator: add read-write 'name' attribute

[Geert Uytterhoeven]

Hi Den,

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> Previously, there is no way to assign names to GPIO lines exported
> through an aggregator.
>
> Allow users to set custom line names via a 'name' attribute.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -63,6 +63,8 @@ struct gpio_aggregator_line {
>         /* Line index within the aggregator device */
>         int idx;
>
> +       /* Custom name for the virtual line */
> +       char *name;

This can be const.

>         /* GPIO chip label or line name */
>         char *key;

Actually this can be const, too.

>         /* Can be negative to indicate lookup by line name */
> @@ -678,6 +680,44 @@ gpio_aggr_line_key_store(struct config_item *item, const char *page,
>
>  CONFIGFS_ATTR(gpio_aggr_line_, key);
>
> +static ssize_t
> +gpio_aggr_line_name_show(struct config_item *item, char *page)
> +{
> +       struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
> +       struct gpio_aggregator *aggr = line->parent;
> +
> +       guard(mutex)(&aggr->lock);
> +
> +       return sprintf(page, "%s\n", line->name ?: "");

sysfs_emit()

> +}
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 05/10] gpio: aggregator: expose custom line names to forwarder gpio_chip

[Geert Uytterhoeven]

Hi Den-san,

Thanks for your patch!

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> Previously, GPIO lines in the aggregator had empty names. Now that the

That is only true for aggregators created through the sysfs interface,
right?  When created from DT, gpio-line-names is already supported:
https://elixir.bootlin.com/linux/v6.13.2/source/Documentation/admin-guide/gpio/gpio-aggregator.rst#L72

> configfs interface supports custom names, update the GPIO forwarder to
> use them.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -425,6 +425,20 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
>  }
>  #endif /* !CONFIG_OF_GPIO */
>
> +static int gpiochip_fwd_line_names(struct device *dev, const char **names, int len)
> +{
> +       int num = device_property_string_array_count(dev, "gpio-line-names");
> +       if (!num)
> +               return 0;
> +       if (num > len) {
> +               pr_warn("gpio-line-names contains %d lines while %d expected",
> +                       num, len);
> +               num = len;
> +       }
> +       return device_property_read_string_array(dev, "gpio-line-names",
> +                                                names, num);
> +}
> +
>  /**
>   * gpiochip_fwd_create() - Create a new GPIO forwarder
>   * @dev: Parent device pointer
> @@ -447,6 +461,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>  {
>         const char *label = dev_name(dev);
>         struct gpiochip_fwd *fwd;
> +       const char **line_names;
>         struct gpio_chip *chip;
>         unsigned int i;
>         int error;
> @@ -458,6 +473,16 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>
>         chip = &fwd->chip;
>
> +       if (!dev_of_node(dev)) {
> +               line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);

So this is always allocated, even when no names are specified?

> +               if (!line_names)
> +                       return ERR_PTR(-ENOMEM);
> +
> +               error = gpiochip_fwd_line_names(dev, line_names, ngpios);
> +               if (error < 0)
> +                       return ERR_PTR(-ENOMEM);
> +       }
> +
>         /*
>          * If any of the GPIO lines are sleeping, then the entire forwarder
>          * will be sleeping.
> @@ -491,6 +516,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>         chip->ngpio = ngpios;
>         fwd->descs = descs;
>
> +       if (!dev_of_node(dev))
> +               chip->names = line_names;
> +

Do you actually need to collect the names yourself?
Below, the driver does:

     error = devm_gpiochip_add_data(dev, chip, fwd);

and gpiochip_add_data_with_key() already calls gpiochip_set_names()
to retrieve the names from the gpio-line-names property.
What is missing to make that work?

>         if (chip->can_sleep)
>                 mutex_init(&fwd->mlock);
>         else
> @@ -530,10 +558,40 @@ to_gpio_aggregator_line(struct config_item *item)
>         return container_of(group, struct gpio_aggregator_line, group);
>  }
>
> +static struct fwnode_handle *aggr_make_device_swnode(struct gpio_aggregator *aggr)
> +{
> +       char **line_names __free(kfree) = NULL;

const (needed when gpio_aggregator_line.name becomes const)

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 06/10] gpio: aggregator: rename 'name' to 'key' in aggr_parse()

[Geert Uytterhoeven]

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> Rename the local variable 'name' in aggr_parse() to 'key' because struct
> gpio_aggregator_line now uses the 'name' field for the custom line name
> and the local variable actually represents a 'key'. This change prepares
> for the next but one commit.
>
> No functional change.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.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 07/10] gpio: aggregator: clean up gpio_aggregator_free()

[Geert Uytterhoeven]

On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@canonical.com> wrote:
> - Rename gpio_aggregator_free() to use the "aggr_" prefix for
>   consistency with other functions that modify struct gpio_aggregator
>   internals.
> - Replace four lines within the function to invoke aggr_deactivate()
> - Move it to a more natural location.
>
> This is a preparatory change for the next commit.
>
> No functional change.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.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 08/10] gpio: aggregator: expoose aggregator created via legacy sysfs to configfs

[Geert Uytterhoeven]

Hi Den-san,

Thanks for your patch!

On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@canonical.com> wrote:
> Expose settings for aggregators created using the sysfs 'new_device'
> interface to configfs. Once written to 'new_device', an "_auto.<N>" path
> appears in the configfs regardless of whether the probe succeeds.
> Consequently, users can no longer use that prefix for custom GPIO
> aggregator names. The 'live' attribute changes to 1 when the probe
> succeeds and the GPIO forwarder is instantiated.
>
> Note that the aggregator device created via sysfs is asynchrnous, i.e.

asynchronous

> writing into 'new_device' returns without waiting for probe completion,
> and the probe may succeed, fail, or eventually succeed via deferred
> probe. Thus, the 'live' attribute may change from 0 to 1 asynchronously
> without notice. So, editting key/offset/name while it's waiting for

editing

> deferred probe is prohibited.
>
> The configfs auto-generation relies on create_default_group(), which
> inherently prohibits rmdir(2). To align with the limitation, this commit
> also prohibits mkdir(2) for them. When users want to change the number
> of lines for an aggregator initialized via 'new_device', they need to
> tear down the device using 'delete_device' and reconfigure it from
> scratch. This does not break previous behaviour; users of legacy sysfs
> interface simply gain additional almost read-only configfs exposure.
>
> Still, users can write into 'live' attribute to toggle the device unless

write to the

> it's waiting for deferred probe. So once probe succeeds, they can
> deactivate it in the same manner as the devices initialized via
> configfs.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

> @@ -73,6 +76,10 @@ struct gpio_aggregator_line {
>         enum gpio_lookup_flags flags;
>  };
>
> +struct gpio_aggregator_pdev_meta {
> +       bool init_via_sysfs;
> +};

The use of this structure to indicate the instantiation method looks
a bit hacky to me, but I don't see a better way...

> +
>  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
>  static DEFINE_IDR(gpio_aggregator_idr);
>
> @@ -127,6 +134,14 @@ static bool aggr_is_active(struct gpio_aggregator *aggr)
>         return !!aggr->pdev && platform_get_drvdata(aggr->pdev);
>  }
>
> +/* Only aggregators created via legacy sysfs can be "activating". */
> +static bool aggr_is_activating(struct gpio_aggregator *aggr)
> +{
> +       lockdep_assert_held(&aggr->lock);
> +
> +       return !!aggr->pdev && !platform_get_drvdata(aggr->pdev);

No need for "!!".

> +}
> +
>  static size_t aggr_count_lines(struct gpio_aggregator *aggr)
>  {
>         lockdep_assert_held(&aggr->lock);

> @@ -1002,6 +1048,14 @@ gpio_aggr_make_group(struct config_group *group, const char *name)
>         if (!aggr)
>                 return ERR_PTR(-ENOMEM);
>
> +       /*
> +        * "_auto" prefix is reserved for auto-generated config group
> +        * for devices create via legacy sysfs interface.
> +        */
> +       if (strncmp(name, AGGREGATOR_LEGACY_PREFIX,
> +                   sizeof(AGGREGATOR_LEGACY_PREFIX)) == 0)
> +               return ERR_PTR(-EINVAL);

Missing kfree(aggr) in case of failure.

> +
>         mutex_lock(&gpio_aggregator_lock);
>         aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
>         mutex_unlock(&gpio_aggregator_lock);
> @@ -1044,6 +1098,8 @@ static struct configfs_subsystem gpio_aggr_subsys = {
>  static int aggr_parse(struct gpio_aggregator *aggr)
>  {
>         char *args = skip_spaces(aggr->args);
> +       struct gpio_aggregator_line *line;
> +       char name[CONFIGFS_ITEM_NAME_LEN];
>         char *key, *offsets, *p;
>         unsigned int i, n = 0;
>         int error = 0;
> @@ -1055,14 +1111,29 @@ static int aggr_parse(struct gpio_aggregator *aggr)
>
>         args = next_arg(args, &key, &p);
>         while (*args) {
> +               scnprintf(name, CONFIGFS_ITEM_NAME_LEN, "line%u", n);

sizeof(name), to protect against future changes of name[].

>  static ssize_t new_device_store(struct device_driver *driver, const char *buf,
>                                 size_t count)
>  {
> +       struct gpio_aggregator_pdev_meta meta;

You might as well pre-initialize this:

   = { .init_via_sysfs = true };

> +       char name[CONFIGFS_ITEM_NAME_LEN];
>         struct gpio_aggregator *aggr;
>         struct platform_device *pdev;
>         int res, id;
> @@ -1112,6 +1210,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
>
>         memcpy(aggr->args, buf, count + 1);
>
> +       aggr->init_via_sysfs = true;
>         aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
>                                 GFP_KERNEL);
>         if (!aggr->lookups) {
> @@ -1128,10 +1227,22 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
>                 goto free_table;
>         }
>
> +       scnprintf(name, CONFIGFS_ITEM_NAME_LEN,

sizeof(name)

> +                 "%s.%d", AGGREGATOR_LEGACY_PREFIX, id);
> +       INIT_LIST_HEAD(&aggr->list_head);
> +       mutex_init(&aggr->lock);
> +       config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
> +       init_completion(&aggr->probe_completion);

The code above is now almost identical to gpio_aggr_make_group().

> +
> +       /* Expose to configfs */
> +       res = configfs_register_group(&gpio_aggr_subsys.su_group, &aggr->group);
> +       if (res)
> +               goto remove_idr;
> +
>         aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
>         if (!aggr->lookups->dev_id) {
>                 res = -ENOMEM;
> -               goto remove_idr;
> +               goto unregister_group;
>         }
>
>         res = aggr_parse(aggr);
> @@ -1140,7 +1251,9 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
>
>         gpiod_add_lookup_table(aggr->lookups);
>
> -       pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
> +       meta.init_via_sysfs = true;
> +
> +       pdev = platform_device_register_data(NULL, DRV_NAME, id, &meta, sizeof(meta));
>         if (IS_ERR(pdev)) {
>                 res = PTR_ERR(pdev);
>                 goto remove_table;

> @@ -1258,7 +1379,26 @@ static struct platform_driver gpio_aggregator_driver = {
>
>  static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
>  {
> -       aggr_free(p);
> +       /*
> +        * There should be no aggregator created via configfs, as their
> +        * presence would prevent module unloading.
> +        */
> +       struct gpio_aggregator *aggr = (struct gpio_aggregator *)p;

The cast is not needed.

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 09/10] gpio: aggregator: cancel deferred probe for devices created via configfs

[Geert Uytterhoeven]

Hi Bartosz,

On Tue, 4 Feb 2025 at 14:14, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> > For aggregators initialized via configfs, write 1 to 'live' waits for
> > probe completion and returns an error if the probe fails, unlike the
> > legacy sysfs interface, which is asynchronous.
> >
> > Since users control the liveness of the aggregator device and might be
> > editting configurations while 'live' is 0, deferred probing is both
> > unnatural and unsafe.
> >
> > Cancel deferred probe for purely configfs-based aggregators when probe
> > fails.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -1313,7 +1313,6 @@ static struct attribute *gpio_aggregator_attrs[] = {
> >  };
> >  ATTRIBUTE_GROUPS(gpio_aggregator);
> >
> > -
> >  /*
> >   *  GPIO Aggregator platform device
> >   */
> > @@ -1342,8 +1341,22 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
> >
> >         for (i = 0; i < n; i++) {
> >                 descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> > -               if (IS_ERR(descs[i]))
> > +               if (IS_ERR(descs[i])) {
> > +                       /*
> > +                        * Deferred probing is not suitable when the aggregator
> > +                        * is created by userspace. They should just retry later
> > +                        * whenever they like. For device creation via sysfs,
> > +                        * error is propagated without overriding for backward
> > +                        * compatibility. .prevent_deferred_probe is kept unset
> > +                        * for other cases.
> > +                        */
> > +                       if (!init_via_sysfs && !dev_of_node(dev) &&
> > +                           descs[i] == ERR_PTR(-EPROBE_DEFER)) {
> > +                               pr_warn("Deferred probe canceled for creation by userspace.\n");
> > +                               return -ENODEV;
> > +                       }
> >                         return PTR_ERR(descs[i]);
> > +               }
> >         }
> >
> >         features = (uintptr_t)device_get_match_data(dev);
>
> Geert, what do you think about making the sysfs interface synchronous
> instead? I would argue it's actually more logical as the user will
> instinctively expect the aggregator to be ready after write() to
> new_device returns.

On one hand, I agree that it would make some scenarios simpler, and
let us propagate an error code to the sysfs writer in case of failure.

On the other hand, it would change user behavior. Currently people can
configure a GPIO aggregator, and load the driver module for the parent
gpiochip later, relying on deferred probing to bring up everything
when it is ready.

I2C's new_device is also synchronous.

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 10/10] Documentation: gpio: document configfs interface for gpio-aggregator

[Geert Uytterhoeven]

Hi Den-san.

On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@canonical.com> wrote:
> Add documentation for the newly added configfs-based interface for GPIO
> aggregator.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

Thanks for your patch!

> --- a/Documentation/admin-guide/gpio/gpio-aggregator.rst
> +++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
> @@ -69,6 +69,99 @@ write-only attribute files in sysfs.
>                     $ echo gpio-aggregator.0 > delete_device
>
>
> +Aggregating GPIOs using Configfs
> +--------------------------------
> +
> +**Group:** ``/config/gpio-aggregator``
> +
> +    This is the root directory of the gpio-aggregator configfs tree.
> +
> +**Group:** ``/config/gpio-aggregator/<example-name>``
> +
> +    This directory represents a GPIO aggregator device. You can assign any
> +    name to ``<example-name>`` (e.g., ``agg0``), except names starting with
> +    ``_auto`` prefix, which are reserved for auto-generated configfs
> +    entries corresponding to devices created via Sysfs.
> +
> +**Attribute:** ``/config/gpio-aggregator/<example-name>/live``
> +
> +    The ``live`` attribute allows to trigger the actual creation of the device
> +    once it's fully configured. The accepted values are: ``1`` to enable the
> +    virtual device and ``0`` to disable and tear it down.

As the code uses kstrtobool(), it accepts variants of
yes/true/on/no/false/off, too.

> +
> +**Attribute:** ``/config/gpio-aggregator/<example-name>/dev_name``
> +
> +    The read-only ``dev_name`` attribute exposes the name of the device as it
> +    will appear in the system on the platform bus (e.g. ``gpio-aggregator.0``).
> +    This is useful for identifying a character device for the newly created
> +    aggregator. If it's ``gpio-aggregator.0``,
> +    ``/sys/devices/platform/gpio-aggregator.0/gpiochipX`` path tells you that the
> +    GPIO device id is ``X``.
> +
> +You must create subdirectories for each virtual line you want to
> +instantiate, named exactly as ``line0``, ``line1``, ..., ``lineY``, when
> +you want to instantiate ``Y+1`` (Y >= 0) lines.  Configure all lines before
> +activating the device by setting ``live`` to 1.
> +
> +**Group:** ``/config/gpio-aggregator/<example-name>/<lineY>/``
> +
> +    This directory represents a GPIO line to include in the aggregator.
> +
> +**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/key``
> +
> +**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/offset``
> +
> +    If ``offset`` is >= 0:
> +        * ``key`` specifies the name of the chip this GPIO belongs to
> +        * ``offset`` specifies the line offset within that chip.
> +    If ``offset`` is <0:

Missing space before 0.
Please add "(default)", so the user knows he can skip writing to this
file when specifying a GPIO line name.

> +        * ``key`` needs to specify the GPIO line name.

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 00/10] Introduce configfs-based interface for gpio-aggregator

[Koichiro Den]

On Wed, Feb 12, 2025 at 02:14:23PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san,
> 
> On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> > This patch series introduces a configfs-based interface to gpio-aggregator
> > to address limitations in the existing 'new_device' interface.
> >
> > The existing 'new_device' interface has several limitations:
> >
> >   #1. No way to determine when GPIO aggregator creation is complete.
> >   #2. No way to retrieve errors when creating a GPIO aggregator.
> >   #3. No way to trace a GPIO line of an aggregator back to its
> >       corresponding physical device.
> >   #4. The 'new_device' echo does not indicate which virtual gpiochip<N>
> >       was created.
> >   #5. No way to assign names to GPIO lines exported through an aggregator.
> >
> > Although Issue#1 to #3 could technically be resolved easily without
> > configfs, using configfs offers a streamlined, modern, and extensible
> > approach, especially since gpio-sim and gpio-virtuser already utilize
> > configfs.
> 
> Thanks for your series!
> 
> I gave it a try using all three ways of configuration (sysfs, configs,
> DT), and it works fine!
> 
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Gr{oetje,eeting}s,

Thank you very much for the through review! I'll reply to each of your comment.

Koichiro

> 
>                         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 02/10] gpio: aggregator: introduce basic configfs interface

[Koichiro Den]

On Wed, Feb 12, 2025 at 02:48:12PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san,
> 
> On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> > The existing sysfs 'new_device' interface has several limitations:
> > * No way to determine when GPIO aggregator creation is complete.
> > * No way to retrieve errors when creating a GPIO aggregator.
> > * No way to trace a GPIO line of an aggregator back to its
> >   corresponding physical device.
> > * The 'new_device' echo does not indicate which virtual gpiochip<N>
> >   was created.
> > * No way to assign names to GPIO lines exported through an aggregator.
> >
> > Introduce the new configfs interface for gpio-aggregator to address
> > these limitations. It provides a more streamlined, modern, and
> > extensible configuration method. For backward compatibility, the
> > 'new_device' interface and its behaviour is retained for now.
> >
> > This commit implements minimal functionalities:
> >
> >   /config/gpio-aggregator/<name-of-your-choice>/
> >   /config/gpio-aggregator/<name-of-your-choice>/live
> >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
> >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
> >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
> >
> > Basic setup flow is:
> > 1. Create a directory for a GPIO aggregator.
> > 2. Create subdirectories for each line you want to instantiate.
> > 3. In each line directory, configure the key and offset.
> >    The key/offset semantics are as follows:
> >    * If offset is >= 0:
> >      - key specifies the name of the chip this GPIO belongs to
> >      - offset specifies the line offset within that chip.
> >    * If offset is <0:
> >      - key needs to specify the GPIO line name.
> > 4. Return to the aggregator's root directory and write '1' to the live
> >    attribute.
> >
> > For example, the command in the existing kernel doc:
> >
> >   echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
> >
> > is equivalent to:
> >
> >   mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
> >   # Change <custom-name> to name of your choice (e.g. "aggr0")
> >   cd /sys/kernel/config/gpio-aggregator/<custom-name>
> >   mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
> >   echo e6052000.gpio > line0/key
> >   echo 19            > line0/offset
> >   echo e6050000.gpio > line1/key
> >   echo 20            > line1/offset
> >   echo e6050000.gpio > line2/key
> >   echo 21            > line2/offset
> >   echo 1             > live
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -9,10 +9,14 @@
> >
> >  #include <linux/bitmap.h>
> >  #include <linux/bitops.h>
> > +#include <linux/completion.h>
> > +#include <linux/configfs.h>
> 
> Using configfs requires CONFIG_CONFIGFS_FS.
> So either GPIO_AGGREGATOR should select CONFIGFS_FS, or
> all configfs-related code should be protected by checks for
> CONFIG_CONFIGFS_FS.  Since we want to encourage people to use the new
> API, I think the former is preferred.

Indeed. I had mentioned this in the response to Bartosz here:
https://lore.kernel.org/all/dmy4mvxut3l5kqds2b2fnnes5ukr73spddwgrbkeoqrb5p5wir@hkq6ltr7d6dt/

I agree with the former.

> 
> >  #include <linux/ctype.h>
> >  #include <linux/delay.h>
> >  #include <linux/idr.h>
> >  #include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/lockdep.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > @@ -34,11 +38,39 @@
> >   */
> >
> >  struct gpio_aggregator {
> > +       struct config_group group;
> >         struct gpiod_lookup_table *lookups;
> >         struct platform_device *pdev;
> > +       struct mutex lock;
> > +       int id;
> > +
> > +       /* Synchronize with probe */
> > +       struct notifier_block bus_notifier;
> > +       struct completion probe_completion;
> > +       bool driver_bound;
> > +
> > +       /* List of gpio_aggregator_line. Always added in order */
> > +       struct list_head list_head;
> > +
> >         char args[];
> >  };
> >
> > +struct gpio_aggregator_line {
> > +       struct config_group group;
> > +       struct gpio_aggregator *parent;
> > +       struct list_head entry;
> > +
> > +       /* Line index within the aggregator device */
> > +       int idx;
> 
> unsigned int

Thanks. I'll fix this in v3.

> 
> > +
> > +       /* GPIO chip label or line name */
> > +       char *key;
> > +       /* Can be negative to indicate lookup by line name */
> > +       int offset;
> > +
> > +       enum gpio_lookup_flags flags;
> > +};
> > +
> >  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
> >  static DEFINE_IDR(gpio_aggregator_idr);
> 
> > +static bool aggr_is_active(struct gpio_aggregator *aggr)
> > +{
> > +       lockdep_assert_held(&aggr->lock);
> > +
> > +       return !!aggr->pdev && platform_get_drvdata(aggr->pdev);
> 
> No need for "!!".

I'll fix this in v3.

> 
> > +}
> 
> > +static void aggr_line_add(struct gpio_aggregator *aggr,
> > +                         struct gpio_aggregator_line *line)
> > +{
> > +       struct gpio_aggregator_line *tmp;
> > +
> > +       lockdep_assert_held(&aggr->lock);
> > +
> > +       list_for_each_entry(tmp, &aggr->list_head, entry) {
> > +               if (WARN_ON(tmp->idx == line->idx))
> 
> Can this really happen?

I don't think so. It was just a safeguard for the future to help us notice
when something very bad would happen, caused by changes on codebase. So let
me drop it in v3.

> 
> > +                       return;
> > +               if (tmp->idx > line->idx) {
> > +                       list_add_tail(&line->entry, &tmp->entry);
> > +                       return;
> > +               }
> > +       }
> > +       list_add_tail(&line->entry, &aggr->list_head);
> > +}
> 
> > +static void aggr_lockup_configfs(struct gpio_aggregator *aggr, bool lock)
> > +{
> > +       struct configfs_subsystem *subsys = aggr->group.cg_subsys;
> > +       struct gpio_aggregator_line *line;
> > +
> > +       /*
> > +        * The device only needs to depend on leaf lines. This is
> > +        * sufficient to lock up all the configfs entries that the
> > +        * instantiated, alive device depends on.
> > +        */
> > +       list_for_each_entry(line, &aggr->list_head, entry) {
> > +               if (lock)
> > +                       WARN_ON(configfs_depend_item_unlocked(
> > +                                       subsys, &line->group.cg_item));
> 
> Can this happen?
> I am also worried if this can be triggered by the user, combined
> with panic_on_warn.

I don't think so. This was just a safeguard for the future.

> 
> > +               else
> > +                       configfs_undepend_item_unlocked(
> > +                                       &line->group.cg_item);
> > +       }
> > +}
> > +
> > +static ssize_t
> > +gpio_aggr_line_key_show(struct config_item *item, char *page)
> > +{
> > +       struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
> > +       struct gpio_aggregator *aggr = line->parent;
> > +
> > +       guard(mutex)(&aggr->lock);
> > +
> > +       return sprintf(page, "%s\n", line->key ?: "");
> 
> Please use sysfs_emit() instead (everywhere).
> 

Thanks for pointing it out. I'll fix all of them in v3.

> 
> > +}
> 
> > +static ssize_t
> > +gpio_aggr_device_live_store(struct config_item *item, const char *page,
> > +                           size_t count)
> > +{
> > +       struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> > +       int ret = 0;
> > +       bool live;
> > +
> > +       ret = kstrtobool(page, &live);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (live)
> > +               aggr_lockup_configfs(aggr, true);
> > +
> > +       scoped_guard(mutex, &aggr->lock) {
> > +               if (live == aggr_is_active(aggr))
> > +                       ret = -EPERM;
> > +               else if (live)
> > +                       ret = aggr_activate(aggr);
> > +               else
> > +                       aggr_deactivate(aggr);
> > +       }
> > +
> > +       /*
> > +        * Undepend is required only if device disablement (live == 0)
> 
> s/Undepend/Lock-up/?

I must admit that I couldn't find a best suitable antonym to 'depend'.
IMO Lock-up sounds a bit misleading. How about 'Unlock'?

> 
> > +        * succeeds or if device enablement (live == 1) fails.
> > +        */
> > +       if (live == !!ret)
> > +               aggr_lockup_configfs(aggr, false);
> > +
> > +       return ret ?: count;
> > +}
> 
> > +static struct config_group *
> > +gpio_aggr_make_group(struct config_group *group, const char *name)
> > +{
> > +       /* arg space is unneeded */
> > +       struct gpio_aggregator *aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
> > +       if (!aggr)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       mutex_lock(&gpio_aggregator_lock);
> > +       aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> > +       mutex_unlock(&gpio_aggregator_lock);
> > +
> > +       if (aggr->id < 0) {
> > +               kfree(aggr);
> > +               return ERR_PTR(aggr->id);
> > +       }
> 
> The above more or less duplicates the existing code in
> new_device_store().

I'll factor out the common part and add new funcs gpio_alloc()/gpio_free().
Please let me know if any objections.

> 
> > +
> > +       INIT_LIST_HEAD(&aggr->list_head);
> > +       mutex_init(&aggr->lock);
> > +       config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
> > +       init_completion(&aggr->probe_completion);
> > +
> > +       return &aggr->group;
> > +}
> 
> Gr{oetje,eeting}s,

Thanks for the review.

Koichiro

> 
>                         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 03/10] gpio: aggregator: add read-only 'dev_name' configfs attribute

[Koichiro Den]

On Wed, Feb 12, 2025 at 03:20:06PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san,
> 
> On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> > Add a read-only 'dev_name' attribute to configfs interface, which
> > exposes the platform bus device name. Users can easily identify which
> > gpiochip<N> has been created as follows:
> >
> > $ cat /sys/kernel/config/gpio-aggregator/<aggregator-name>/dev_name
> >   gpio-aggregator.0
> > $ ls -d /sys/devices/platform/gpio-aggregator.0/gpiochip*
> >   gpiochip3
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -732,6 +732,23 @@ static struct configfs_attribute *gpio_aggr_line_attrs[] = {
> >         NULL
> >  };
> >
> > +static ssize_t
> > +gpio_aggr_device_dev_name_show(struct config_item *item, char *page)
> > +{
> > +       struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> > +       struct platform_device *pdev;
> > +
> > +       guard(mutex)(&aggr->lock);
> > +
> > +       pdev = aggr->pdev;
> > +       if (pdev)
> > +               return sprintf(page, "%s\n", dev_name(&pdev->dev));
> > +
> > +       return sprintf(page, "%s.%d\n", DRV_NAME, aggr->id);
> 
> sysfs_emit(), for both branches.

Will fix in v3.

> 
> > +}
> > +
> 
> Please drop this blank line (everywhere).

Will fix in v3.

Thanks!

Koichiro

> 
> > +CONFIGFS_ATTR_RO(gpio_aggr_device_, dev_name);
> > +
> >  static ssize_t
> >  gpio_aggr_device_live_show(struct config_item *item, char *page)
> >  {
> 
> 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 04/10] gpio: aggregator: add read-write 'name' attribute

[Koichiro Den]

On Wed, Feb 12, 2025 at 03:27:50PM GMT, Geert Uytterhoeven wrote:
> Hi Den,
> 
> On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> > Previously, there is no way to assign names to GPIO lines exported
> > through an aggregator.
> >
> > Allow users to set custom line names via a 'name' attribute.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -63,6 +63,8 @@ struct gpio_aggregator_line {
> >         /* Line index within the aggregator device */
> >         int idx;
> >
> > +       /* Custom name for the virtual line */
> > +       char *name;
> 
> This can be const.

Will fix in v3.

> 
> >         /* GPIO chip label or line name */
> >         char *key;
> 
> Actually this can be const, too.

Will fix in v3.

> 
> >         /* Can be negative to indicate lookup by line name */
> > @@ -678,6 +680,44 @@ gpio_aggr_line_key_store(struct config_item *item, const char *page,
> >
> >  CONFIGFS_ATTR(gpio_aggr_line_, key);
> >
> > +static ssize_t
> > +gpio_aggr_line_name_show(struct config_item *item, char *page)
> > +{
> > +       struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
> > +       struct gpio_aggregator *aggr = line->parent;
> > +
> > +       guard(mutex)(&aggr->lock);
> > +
> > +       return sprintf(page, "%s\n", line->name ?: "");
> 
> sysfs_emit()

Will fix in v3.

Thanks!

Koichiro

> 
> > +}
> 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 05/10] gpio: aggregator: expose custom line names to forwarder gpio_chip

[Koichiro Den]

On Wed, Feb 12, 2025 at 03:44:46PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san,
> 
> Thanks for your patch!
> 
> On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> > Previously, GPIO lines in the aggregator had empty names. Now that the
> 
> That is only true for aggregators created through the sysfs interface,
> right?  When created from DT, gpio-line-names is already supported:
> https://elixir.bootlin.com/linux/v6.13.2/source/Documentation/admin-guide/gpio/gpio-aggregator.rst#L72

You're absolutely right, I'll fix this commit message, along with the
needless codes you mentioned below.

> 
> > configfs interface supports custom names, update the GPIO forwarder to
> > use them.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> 
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -425,6 +425,20 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
> >  }
> >  #endif /* !CONFIG_OF_GPIO */
> >
> > +static int gpiochip_fwd_line_names(struct device *dev, const char **names, int len)
> > +{
> > +       int num = device_property_string_array_count(dev, "gpio-line-names");
> > +       if (!num)
> > +               return 0;
> > +       if (num > len) {
> > +               pr_warn("gpio-line-names contains %d lines while %d expected",
> > +                       num, len);
> > +               num = len;
> > +       }
> > +       return device_property_read_string_array(dev, "gpio-line-names",
> > +                                                names, num);
> > +}
> > +
> >  /**
> >   * gpiochip_fwd_create() - Create a new GPIO forwarder
> >   * @dev: Parent device pointer
> > @@ -447,6 +461,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> >  {
> >         const char *label = dev_name(dev);
> >         struct gpiochip_fwd *fwd;
> > +       const char **line_names;
> >         struct gpio_chip *chip;
> >         unsigned int i;
> >         int error;
> > @@ -458,6 +473,16 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> >
> >         chip = &fwd->chip;
> >
> > +       if (!dev_of_node(dev)) {
> > +               line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);
> 
> So this is always allocated, even when no names are specified?

Yes. This is unreasonable. I'll fix this in v3. Thanks.

> 
> > +               if (!line_names)
> > +                       return ERR_PTR(-ENOMEM);
> > +
> > +               error = gpiochip_fwd_line_names(dev, line_names, ngpios);
> > +               if (error < 0)
> > +                       return ERR_PTR(-ENOMEM);
> > +       }
> > +
> >         /*
> >          * If any of the GPIO lines are sleeping, then the entire forwarder
> >          * will be sleeping.
> > @@ -491,6 +516,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> >         chip->ngpio = ngpios;
> >         fwd->descs = descs;
> >
> > +       if (!dev_of_node(dev))
> > +               chip->names = line_names;
> > +
> 
> Do you actually need to collect the names yourself?
> Below, the driver does:
> 
>      error = devm_gpiochip_add_data(dev, chip, fwd);
> 
> and gpiochip_add_data_with_key() already calls gpiochip_set_names()
> to retrieve the names from the gpio-line-names property.
> What is missing to make that work?

Thanks for pointing it out. It was just my oversight. I'll drop the
needless parts in v3.

> 
> >         if (chip->can_sleep)
> >                 mutex_init(&fwd->mlock);
> >         else
> > @@ -530,10 +558,40 @@ to_gpio_aggregator_line(struct config_item *item)
> >         return container_of(group, struct gpio_aggregator_line, group);
> >  }
> >
> > +static struct fwnode_handle *aggr_make_device_swnode(struct gpio_aggregator *aggr)
> > +{
> > +       char **line_names __free(kfree) = NULL;
> 
> const (needed when gpio_aggregator_line.name becomes const)

Will fix in v3.

Thank you!

> 
> 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 08/10] gpio: aggregator: expoose aggregator created via legacy sysfs to configfs

[Koichiro Den]

On Wed, Feb 12, 2025 at 04:36:13PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san,
> 
> Thanks for your patch!
> 
> On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@canonical.com> wrote:
> > Expose settings for aggregators created using the sysfs 'new_device'
> > interface to configfs. Once written to 'new_device', an "_auto.<N>" path
> > appears in the configfs regardless of whether the probe succeeds.
> > Consequently, users can no longer use that prefix for custom GPIO
> > aggregator names. The 'live' attribute changes to 1 when the probe
> > succeeds and the GPIO forwarder is instantiated.
> >
> > Note that the aggregator device created via sysfs is asynchrnous, i.e.
> 
> asynchronous
> 
> > writing into 'new_device' returns without waiting for probe completion,
> > and the probe may succeed, fail, or eventually succeed via deferred
> > probe. Thus, the 'live' attribute may change from 0 to 1 asynchronously
> > without notice. So, editting key/offset/name while it's waiting for
> 
> editing
> 
> > deferred probe is prohibited.
> >
> > The configfs auto-generation relies on create_default_group(), which
> > inherently prohibits rmdir(2). To align with the limitation, this commit
> > also prohibits mkdir(2) for them. When users want to change the number
> > of lines for an aggregator initialized via 'new_device', they need to
> > tear down the device using 'delete_device' and reconfigure it from
> > scratch. This does not break previous behaviour; users of legacy sysfs
> > interface simply gain additional almost read-only configfs exposure.
> >
> > Still, users can write into 'live' attribute to toggle the device unless
> 
> write to the
> 
> > it's waiting for deferred probe. So once probe succeeds, they can
> > deactivate it in the same manner as the devices initialized via
> > configfs.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> 
> > @@ -73,6 +76,10 @@ struct gpio_aggregator_line {
> >         enum gpio_lookup_flags flags;
> >  };
> >
> > +struct gpio_aggregator_pdev_meta {
> > +       bool init_via_sysfs;
> > +};

Thanks for pointing out all of them above. Will fix them all in v3.

> 
> The use of this structure to indicate the instantiation method looks
> a bit hacky to me, but I don't see a better way...

I agree.

> 
> > +
> >  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
> >  static DEFINE_IDR(gpio_aggregator_idr);
> >
> > @@ -127,6 +134,14 @@ static bool aggr_is_active(struct gpio_aggregator *aggr)
> >         return !!aggr->pdev && platform_get_drvdata(aggr->pdev);
> >  }
> >
> > +/* Only aggregators created via legacy sysfs can be "activating". */
> > +static bool aggr_is_activating(struct gpio_aggregator *aggr)
> > +{
> > +       lockdep_assert_held(&aggr->lock);
> > +
> > +       return !!aggr->pdev && !platform_get_drvdata(aggr->pdev);
> 
> No need for "!!".

Will fix in v3.

> 
> > +}
> > +
> >  static size_t aggr_count_lines(struct gpio_aggregator *aggr)
> >  {
> >         lockdep_assert_held(&aggr->lock);
> 
> > @@ -1002,6 +1048,14 @@ gpio_aggr_make_group(struct config_group *group, const char *name)
> >         if (!aggr)
> >                 return ERR_PTR(-ENOMEM);
> >
> > +       /*
> > +        * "_auto" prefix is reserved for auto-generated config group
> > +        * for devices create via legacy sysfs interface.
> > +        */
> > +       if (strncmp(name, AGGREGATOR_LEGACY_PREFIX,
> > +                   sizeof(AGGREGATOR_LEGACY_PREFIX)) == 0)
> > +               return ERR_PTR(-EINVAL);
> 
> Missing kfree(aggr) in case of failure.

My bad. Will fix in v3.

> 
> > +
> >         mutex_lock(&gpio_aggregator_lock);
> >         aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> >         mutex_unlock(&gpio_aggregator_lock);
> > @@ -1044,6 +1098,8 @@ static struct configfs_subsystem gpio_aggr_subsys = {
> >  static int aggr_parse(struct gpio_aggregator *aggr)
> >  {
> >         char *args = skip_spaces(aggr->args);
> > +       struct gpio_aggregator_line *line;
> > +       char name[CONFIGFS_ITEM_NAME_LEN];
> >         char *key, *offsets, *p;
> >         unsigned int i, n = 0;
> >         int error = 0;
> > @@ -1055,14 +1111,29 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> >
> >         args = next_arg(args, &key, &p);
> >         while (*args) {
> > +               scnprintf(name, CONFIGFS_ITEM_NAME_LEN, "line%u", n);
> 
> sizeof(name), to protect against future changes of name[].

Right, will fix it in v3.

> 
> >  static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >                                 size_t count)
> >  {
> > +       struct gpio_aggregator_pdev_meta meta;
> 
> You might as well pre-initialize this:
> 
>    = { .init_via_sysfs = true };
>

Will do so in v3.

> > +       char name[CONFIGFS_ITEM_NAME_LEN];
> >         struct gpio_aggregator *aggr;
> >         struct platform_device *pdev;
> >         int res, id;
> > @@ -1112,6 +1210,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >
> >         memcpy(aggr->args, buf, count + 1);
> >
> > +       aggr->init_via_sysfs = true;
> >         aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
> >                                 GFP_KERNEL);
> >         if (!aggr->lookups) {
> > @@ -1128,10 +1227,22 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >                 goto free_table;
> >         }
> >
> > +       scnprintf(name, CONFIGFS_ITEM_NAME_LEN,
> 
> sizeof(name)

Will fix in v3.

> 
> > +                 "%s.%d", AGGREGATOR_LEGACY_PREFIX, id);
> > +       INIT_LIST_HEAD(&aggr->list_head);
> > +       mutex_init(&aggr->lock);
> > +       config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
> > +       init_completion(&aggr->probe_completion);
> 
> The code above is now almost identical to gpio_aggr_make_group().

(As I replied to your feedback to [PATCH v2 02/10]) I'll factor out the
common part and add new funcs, then use them in each.

> 
> > +
> > +       /* Expose to configfs */
> > +       res = configfs_register_group(&gpio_aggr_subsys.su_group, &aggr->group);
> > +       if (res)
> > +               goto remove_idr;
> > +
> >         aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> >         if (!aggr->lookups->dev_id) {
> >                 res = -ENOMEM;
> > -               goto remove_idr;
> > +               goto unregister_group;
> >         }
> >
> >         res = aggr_parse(aggr);
> > @@ -1140,7 +1251,9 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >
> >         gpiod_add_lookup_table(aggr->lookups);
> >
> > -       pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
> > +       meta.init_via_sysfs = true;
> > +
> > +       pdev = platform_device_register_data(NULL, DRV_NAME, id, &meta, sizeof(meta));
> >         if (IS_ERR(pdev)) {
> >                 res = PTR_ERR(pdev);
> >                 goto remove_table;
> 
> > @@ -1258,7 +1379,26 @@ static struct platform_driver gpio_aggregator_driver = {
> >
> >  static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
> >  {
> > -       aggr_free(p);
> > +       /*
> > +        * There should be no aggregator created via configfs, as their
> > +        * presence would prevent module unloading.
> > +        */
> > +       struct gpio_aggregator *aggr = (struct gpio_aggregator *)p;
> 
> The cast is not needed.

Will fix in v3.

Thanks!

Koichiro

> 
> 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 10/10] Documentation: gpio: document configfs interface for gpio-aggregator

[Koichiro Den]

On Wed, Feb 12, 2025 at 04:55:24PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san.
> 
> On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@canonical.com> wrote:
> > Add documentation for the newly added configfs-based interface for GPIO
> > aggregator.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> 
> Thanks for your patch!
> 
> > --- a/Documentation/admin-guide/gpio/gpio-aggregator.rst
> > +++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
> > @@ -69,6 +69,99 @@ write-only attribute files in sysfs.
> >                     $ echo gpio-aggregator.0 > delete_device
> >
> >
> > +Aggregating GPIOs using Configfs
> > +--------------------------------
> > +
> > +**Group:** ``/config/gpio-aggregator``
> > +
> > +    This is the root directory of the gpio-aggregator configfs tree.
> > +
> > +**Group:** ``/config/gpio-aggregator/<example-name>``
> > +
> > +    This directory represents a GPIO aggregator device. You can assign any
> > +    name to ``<example-name>`` (e.g., ``agg0``), except names starting with
> > +    ``_auto`` prefix, which are reserved for auto-generated configfs
> > +    entries corresponding to devices created via Sysfs.
> > +
> > +**Attribute:** ``/config/gpio-aggregator/<example-name>/live``
> > +
> > +    The ``live`` attribute allows to trigger the actual creation of the device
> > +    once it's fully configured. The accepted values are: ``1`` to enable the
> > +    virtual device and ``0`` to disable and tear it down.
> 
> As the code uses kstrtobool(), it accepts variants of
> yes/true/on/no/false/off, too.

Thanks for pointing that out. I'll modify this part.

> 
> > +
> > +**Attribute:** ``/config/gpio-aggregator/<example-name>/dev_name``
> > +
> > +    The read-only ``dev_name`` attribute exposes the name of the device as it
> > +    will appear in the system on the platform bus (e.g. ``gpio-aggregator.0``).
> > +    This is useful for identifying a character device for the newly created
> > +    aggregator. If it's ``gpio-aggregator.0``,
> > +    ``/sys/devices/platform/gpio-aggregator.0/gpiochipX`` path tells you that the
> > +    GPIO device id is ``X``.
> > +
> > +You must create subdirectories for each virtual line you want to
> > +instantiate, named exactly as ``line0``, ``line1``, ..., ``lineY``, when
> > +you want to instantiate ``Y+1`` (Y >= 0) lines.  Configure all lines before
> > +activating the device by setting ``live`` to 1.
> > +
> > +**Group:** ``/config/gpio-aggregator/<example-name>/<lineY>/``
> > +
> > +    This directory represents a GPIO line to include in the aggregator.
> > +
> > +**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/key``
> > +
> > +**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/offset``
> > +
> > +    If ``offset`` is >= 0:
> > +        * ``key`` specifies the name of the chip this GPIO belongs to
> > +        * ``offset`` specifies the line offset within that chip.
> > +    If ``offset`` is <0:
> 
> Missing space before 0.
> Please add "(default)", so the user knows he can skip writing to this
> file when specifying a GPIO line name.

That makes sense. Thanks for the review!

Koichiro

> 
> > +        * ``key`` needs to specify the GPIO line name.
> 
> 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 02/10] gpio: aggregator: introduce basic configfs interface

[Geert Uytterhoeven]

Hi Den-san,

On Thu, 13 Feb 2025 at 15:13, Koichiro Den <koichiro.den@canonical.com> wrote:
> On Wed, Feb 12, 2025 at 02:48:12PM GMT, Geert Uytterhoeven wrote:
> > On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> > > The existing sysfs 'new_device' interface has several limitations:
> > > * No way to determine when GPIO aggregator creation is complete.
> > > * No way to retrieve errors when creating a GPIO aggregator.
> > > * No way to trace a GPIO line of an aggregator back to its
> > >   corresponding physical device.
> > > * The 'new_device' echo does not indicate which virtual gpiochip<N>
> > >   was created.
> > > * No way to assign names to GPIO lines exported through an aggregator.
> > >
> > > Introduce the new configfs interface for gpio-aggregator to address
> > > these limitations. It provides a more streamlined, modern, and
> > > extensible configuration method. For backward compatibility, the
> > > 'new_device' interface and its behaviour is retained for now.
> > >
> > > This commit implements minimal functionalities:
> > >
> > >   /config/gpio-aggregator/<name-of-your-choice>/
> > >   /config/gpio-aggregator/<name-of-your-choice>/live
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
> > >
> > > Basic setup flow is:
> > > 1. Create a directory for a GPIO aggregator.
> > > 2. Create subdirectories for each line you want to instantiate.
> > > 3. In each line directory, configure the key and offset.
> > >    The key/offset semantics are as follows:
> > >    * If offset is >= 0:
> > >      - key specifies the name of the chip this GPIO belongs to
> > >      - offset specifies the line offset within that chip.
> > >    * If offset is <0:
> > >      - key needs to specify the GPIO line name.
> > > 4. Return to the aggregator's root directory and write '1' to the live
> > >    attribute.
> > >
> > > For example, the command in the existing kernel doc:
> > >
> > >   echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
> > >
> > > is equivalent to:
> > >
> > >   mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
> > >   # Change <custom-name> to name of your choice (e.g. "aggr0")
> > >   cd /sys/kernel/config/gpio-aggregator/<custom-name>
> > >   mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
> > >   echo e6052000.gpio > line0/key
> > >   echo 19            > line0/offset
> > >   echo e6050000.gpio > line1/key
> > >   echo 20            > line1/offset
> > >   echo e6050000.gpio > line2/key
> > >   echo 21            > line2/offset
> > >   echo 1             > live
> > >
> > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

> > > --- a/drivers/gpio/gpio-aggregator.c
> > > +++ b/drivers/gpio/gpio-aggregator.c

> > > +       /*
> > > +        * Undepend is required only if device disablement (live == 0)
> >
> > s/Undepend/Lock-up/?
>
> I must admit that I couldn't find a best suitable antonym to 'depend'.
> IMO Lock-up sounds a bit misleading. How about 'Unlock'?

I wrote "Lock-up" to match the "lockup" in aggr_lockup_configfs() below.
But now I understand why you wrote "Undepend": when passed "false",
aggr_lockup_configfs() calls configfs_undepend_item_unlocked(),
so "Undepend" is fine.

> > > +        * succeeds or if device enablement (live == 1) fails.
> > > +        */
> > > +       if (live == !!ret)
> > > +               aggr_lockup_configfs(aggr, false);
> > > +
> > > +       return ret ?: count;
> > > +}

> > > +static struct config_group *
> > > +gpio_aggr_make_group(struct config_group *group, const char *name)
> > > +{
> > > +       /* arg space is unneeded */
> > > +       struct gpio_aggregator *aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
> > > +       if (!aggr)
> > > +               return ERR_PTR(-ENOMEM);
> > > +
> > > +       mutex_lock(&gpio_aggregator_lock);
> > > +       aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> > > +       mutex_unlock(&gpio_aggregator_lock);
> > > +
> > > +       if (aggr->id < 0) {
> > > +               kfree(aggr);
> > > +               return ERR_PTR(aggr->id);
> > > +       }
> >
> > The above more or less duplicates the existing code in
> > new_device_store().
>
> I'll factor out the common part and add new funcs gpio_alloc()/gpio_free().
> Please let me know if any objections.

Thanks, that would be fine!

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 09/10] gpio: aggregator: cancel deferred probe for devices created via configfs

[Koichiro Den]

On Wed, Feb 12, 2025 at 04:49:07PM GMT, Geert Uytterhoeven wrote:
> Hi Bartosz,
> 
> On Tue, 4 Feb 2025 at 14:14, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> > > For aggregators initialized via configfs, write 1 to 'live' waits for
> > > probe completion and returns an error if the probe fails, unlike the
> > > legacy sysfs interface, which is asynchronous.
> > >
> > > Since users control the liveness of the aggregator device and might be
> > > editting configurations while 'live' is 0, deferred probing is both
> > > unnatural and unsafe.
> > >
> > > Cancel deferred probe for purely configfs-based aggregators when probe
> > > fails.
> > >
> > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> 
> > > --- a/drivers/gpio/gpio-aggregator.c
> > > +++ b/drivers/gpio/gpio-aggregator.c
> > > @@ -1313,7 +1313,6 @@ static struct attribute *gpio_aggregator_attrs[] = {
> > >  };
> > >  ATTRIBUTE_GROUPS(gpio_aggregator);
> > >
> > > -
> > >  /*
> > >   *  GPIO Aggregator platform device
> > >   */
> > > @@ -1342,8 +1341,22 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
> > >
> > >         for (i = 0; i < n; i++) {
> > >                 descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> > > -               if (IS_ERR(descs[i]))
> > > +               if (IS_ERR(descs[i])) {
> > > +                       /*
> > > +                        * Deferred probing is not suitable when the aggregator
> > > +                        * is created by userspace. They should just retry later
> > > +                        * whenever they like. For device creation via sysfs,
> > > +                        * error is propagated without overriding for backward
> > > +                        * compatibility. .prevent_deferred_probe is kept unset
> > > +                        * for other cases.
> > > +                        */
> > > +                       if (!init_via_sysfs && !dev_of_node(dev) &&
> > > +                           descs[i] == ERR_PTR(-EPROBE_DEFER)) {
> > > +                               pr_warn("Deferred probe canceled for creation by userspace.\n");
> > > +                               return -ENODEV;
> > > +                       }
> > >                         return PTR_ERR(descs[i]);
> > > +               }
> > >         }
> > >
> > >         features = (uintptr_t)device_get_match_data(dev);
> >
> > Geert, what do you think about making the sysfs interface synchronous
> > instead? I would argue it's actually more logical as the user will
> > instinctively expect the aggregator to be ready after write() to
> > new_device returns.
> 
> On one hand, I agree that it would make some scenarios simpler, and
> let us propagate an error code to the sysfs writer in case of failure.
> 
> On the other hand, it would change user behavior. Currently people can
> configure a GPIO aggregator, and load the driver module for the parent
> gpiochip later, relying on deferred probing to bring up everything
> when it is ready.

Thank you both for your insights, Bartosz and Geert. I've just sent v3
(https://lore.kernel.org/all/20250216125816.14430-1-koichiro.den@canonical.com/)
which retains the current behavior, to not suprise anyone now.
I'm now considering whether we might eventually deprecate the sysfs
interface in the future. Doing so could simplify the codebase and bring it
in line with gpio-sim and gpio-virtuser.

Thanks,

Koichiro

> 
> I2C's new_device is also synchronous.
> 
> 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 09/10] gpio: aggregator: cancel deferred probe for devices created via configfs

[Bartosz Golaszewski]

On Sun, Feb 16, 2025 at 2:15 PM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> >
> > On one hand, I agree that it would make some scenarios simpler, and
> > let us propagate an error code to the sysfs writer in case of failure.
> >
> > On the other hand, it would change user behavior. Currently people can
> > configure a GPIO aggregator, and load the driver module for the parent
> > gpiochip later, relying on deferred probing to bring up everything
> > when it is ready.
>
> Thank you both for your insights, Bartosz and Geert. I've just sent v3
> (https://lore.kernel.org/all/20250216125816.14430-1-koichiro.den@canonical.com/)
> which retains the current behavior, to not suprise anyone now.
> I'm now considering whether we might eventually deprecate the sysfs
> interface in the future. Doing so could simplify the codebase and bring it
> in line with gpio-sim and gpio-virtuser.
>

Heh, yeah you'd think so. You can watch my talk[1] on how easy it is
to remove sysfs interfaces. :)

Bartosz

[1] https://fosdem.org/2025/schedule/event/fosdem-2025-5288-the-status-of-removing-sys-class-gpio-and-the-global-gpio-numberspace-from-the-kernel/

Re: [PATCH v2 09/10] gpio: aggregator: cancel deferred probe for devices created via configfs

[Koichiro Den]

On Sun, Feb 16, 2025 at 04:58:51PM GMT, Bartosz Golaszewski wrote:
> On Sun, Feb 16, 2025 at 2:15 PM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > >
> > > On one hand, I agree that it would make some scenarios simpler, and
> > > let us propagate an error code to the sysfs writer in case of failure.
> > >
> > > On the other hand, it would change user behavior. Currently people can
> > > configure a GPIO aggregator, and load the driver module for the parent
> > > gpiochip later, relying on deferred probing to bring up everything
> > > when it is ready.
> >
> > Thank you both for your insights, Bartosz and Geert. I've just sent v3
> > (https://lore.kernel.org/all/20250216125816.14430-1-koichiro.den@canonical.com/)
> > which retains the current behavior, to not suprise anyone now.
> > I'm now considering whether we might eventually deprecate the sysfs
> > interface in the future. Doing so could simplify the codebase and bring it
> > in line with gpio-sim and gpio-virtuser.
> >
> 
> Heh, yeah you'd think so. You can watch my talk[1] on how easy it is
> to remove sysfs interfaces. :)

Well, I just meant new_device/delete_device in this context so my
impression was that it would not be that hard. Anyhow, honestly speaking I
haven't looked through it thoroughly yet. Thank you!

Koichiro

> 
> Bartosz
> 
> [1] https://fosdem.org/2025/schedule/event/fosdem-2025-5288-the-status-of-removing-sys-class-gpio-and-the-global-gpio-numberspace-from-the-kernel/