[PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Bartosz Golaszewski]

Following our discussion[1], here's a proposal for extending the sysfs
interface with attributes not referring to GPIO lines by their global
numbers in a backward compatible way.

Long story short: there is now a new class device for each GPIO chip.
It's called chipX where X is the ID of the device as per the driver
model and it lives next to the old gpiochipABC where ABC is the GPIO
base. Each new chip class device has a pair of export/unexport
attributes which work similarly to the global ones under /sys/class/gpio
but take hardware offsets within the chip as input, instead of the
global numbers. Finally, each exported line appears at the same time as
the global /sys/class/gpio/gpioABC as well as per-chip
/sys/class/gpio/chipX/gpioY sysfs group.

The series contains the implementation of a parallel GPIO chip entry not
containing the base GPIO number in the name and the corresponding sysfs
attribute group for each exported line that lives under the new chip
class device as well as a way to allow to compile out the legacy parts
leaving only the new elements of the sysfs ABI.

This series passes the compatibility tests I wrote while working on the
user-space compatibility layer for sysfs[2].

[1] https://lore.kernel.org/all/CAMRc=McUCeZcU6co1aN54rTudo+JfPjjForu4iKQ5npwXk6GXA@mail.gmail.com/
[2] https://github.com/brgl/gpio-sysfs-compat-tests

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Changes in v3:
- wrap a local variable in #ifdef's as it's only used if legacy sysfs
  ABI is enabled (Geert)
- fix an issue with jumping over a variable annotated with __free()
- use gpiod_is_equal() where applicable
- fix a use-after-free issue
- remove trailing commas from NULL array terminators
- improve commit messages
- change patch order for smaller diffstats and better readability
- put struct list_head at the beginning of structures for better
  performance
- reshuffle the contents of struct gpiodev_data: put all legacy fields
  at the end
- don't break lines too eagerly
- Link to v2: https://lore.kernel.org/r/20250623-gpio-sysfs-chip-export-v2-0-d592793f8964@linaro.org

Changes in v2:
- add missing call to sysfs_remove_groups() in gpiod_unexport()
- pick up review tags
- drop patches that were already applied
- add missing documentation for the chip-local line attributes
- correct the statement about chip's label uniqueness in docs
- Link to v1: https://lore.kernel.org/r/20250610-gpio-sysfs-chip-export-v1-0-a8c7aa4478b1@linaro.org

---
Bartosz Golaszewski (10):
      gpio: sysfs: use gpiod_is_equal() to compare GPIO descriptors
      gpio: sysfs: add a parallel class device for each GPIO chip using device IDs
      gpio: sysfs: only get the dirent reference for the value attr once
      gpio: sysfs: pass gpiod_data directly to internal GPIO sysfs functions
      gpio: sysfs: rename the data variable in gpiod_(un)export()
      gpio: sysfs: don't use driver data in sysfs callbacks for line attributes
      gpio: sysfs: don't look up exported lines as class devices
      gpio: sysfs: export the GPIO directory locally in the gpiochip<id> directory
      gpio: sysfs: allow disabling the legacy parts of the GPIO sysfs interface
      gpio: TODO: remove the task for the sysfs rework

 Documentation/ABI/obsolete/sysfs-gpio |  12 +-
 drivers/gpio/Kconfig                  |   8 +
 drivers/gpio/TODO                     |  13 -
 drivers/gpio/gpiolib-sysfs.c          | 504 +++++++++++++++++++++++++---------
 4 files changed, 389 insertions(+), 148 deletions(-)
---
base-commit: 26981e8906bb5c902e2d34874f64ecfa975d28c8
change-id: 20250402-gpio-sysfs-chip-export-84ac424b61c5

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH v3 01/10] gpio: sysfs: use gpiod_is_equal() to compare GPIO descriptors

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We have a dedicated comparator for GPIO descriptors that performs
additional checks and hides the implementation detail of whether the
same GPIO can be associated with two separate struct gpio_desc objects.
Use it in sysfs code

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c4c21e25c682b939e4a0517393308343feb6585a..c4e85f2827697d0239ff6296caf49d243cf10fe8 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -657,7 +657,7 @@ static int match_export(struct device *dev, const void *desc)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
 
-	return data->desc == desc;
+	return gpiod_is_equal(data->desc, desc);
 }
 
 /**

-- 
2.48.1

Re: [PATCH v3 01/10] gpio: sysfs: use gpiod_is_equal() to compare GPIO descriptors

[Linus Walleij]

On Mon, Jun 30, 2025 at 2:37 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We have a dedicated comparator for GPIO descriptors that performs
> additional checks and hides the implementation detail of whether the
> same GPIO can be associated with two separate struct gpio_desc objects.
> Use it in sysfs code
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

[PATCH v3 02/10] gpio: sysfs: add a parallel class device for each GPIO chip using device IDs

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to enable moving away from the global GPIO numberspace-based
exporting of lines over sysfs: add a parallel, per-chip entry under
/sys/class/gpio/ for every registered GPIO chip, denoted by device ID
in the file name and not its base GPIO number.

Compared to the existing chip group: it does not contain the "base"
attribute as the goal of this change is to not refer to GPIOs by their
global number from user-space anymore. It also contains its own,
per-chip export/unexport attribute pair which allow to export lines by
their hardware offset within the chip.

Caveat #1: the new device cannot be a link to (or be linked to by) the
existing "gpiochip<BASE>" entry as we cannot create links in
/sys/class/xyz/.

Caveat #2: the new entry cannot be named "gpiochipX" as it could
conflict with devices whose base is statically defined to a low number.
Let's go with "chipX" instead.

While at it: the chip label is unique so update the untrue statement
when extending the docs.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 Documentation/ABI/obsolete/sysfs-gpio |   7 +-
 drivers/gpio/gpiolib-sysfs.c          | 192 +++++++++++++++++++++++++---------
 2 files changed, 150 insertions(+), 49 deletions(-)

diff --git a/Documentation/ABI/obsolete/sysfs-gpio b/Documentation/ABI/obsolete/sysfs-gpio
index 480316fee6d80fb7a0ed61706559838591ec0932..ff694708a3bef787afa42dedf94faf209c44dbf0 100644
--- a/Documentation/ABI/obsolete/sysfs-gpio
+++ b/Documentation/ABI/obsolete/sysfs-gpio
@@ -25,8 +25,13 @@ Description:
 	    /active_low ... r/w as: 0, 1
 	/gpiochipN ... for each gpiochip; #N is its first GPIO
 	    /base ... (r/o) same as N
-	    /label ... (r/o) descriptive, not necessarily unique
+	    /label ... (r/o) descriptive chip name
 	    /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1)
+	/chipX ... for each gpiochip; #X is the gpio device ID
+	    /export ... asks the kernel to export a GPIO at HW offset X to userspace
+	    /unexport ... to return a GPIO at HW offset X to the kernel
+	    /label ... (r/o) descriptive chip name
+	    /ngpio ... (r/o) number of GPIOs exposed by the chip
 
   This ABI is obsoleted by Documentation/ABI/testing/gpio-cdev and will be
   removed after 2020.
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c4e85f2827697d0239ff6296caf49d243cf10fe8..990db0405cc86c42bad61295dc823f970199534e 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -45,6 +45,7 @@ struct gpiod_data {
 
 struct gpiodev_data {
 	struct gpio_device *gdev;
+	struct device *cdev_id; /* Class device by GPIO device ID */
 	struct device *cdev_base; /* Class device by GPIO base */
 };
 
@@ -399,6 +400,14 @@ static const struct attribute_group *gpio_groups[] = {
  *   /base ... matching gpio_chip.base (N)
  *   /label ... matching gpio_chip.label
  *   /ngpio ... matching gpio_chip.ngpio
+ *
+ * AND
+ *
+ * /sys/class/gpio/chipX/
+ *   /export ... export GPIO at given offset
+ *   /unexport ... unexport GPIO at given offset
+ *   /label ... matching gpio_chip.label
+ *   /ngpio ... matching gpio_chip.ngpio
  */
 
 static ssize_t base_show(struct device *dev, struct device_attribute *attr,
@@ -428,6 +437,111 @@ static ssize_t ngpio_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(ngpio);
 
+static int export_gpio_desc(struct gpio_desc *desc)
+{
+	int offset, ret;
+
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
+	offset = gpio_chip_hwgpio(desc);
+	if (!gpiochip_line_is_valid(guard.gc, offset)) {
+		pr_debug_ratelimited("%s: GPIO %d masked\n", __func__,
+				     gpio_chip_hwgpio(desc));
+		return -EINVAL;
+	}
+
+	/*
+	 * No extra locking here; FLAG_SYSFS just signifies that the
+	 * request and export were done by on behalf of userspace, so
+	 * they may be undone on its behalf too.
+	 */
+
+	ret = gpiod_request_user(desc, "sysfs");
+	if (ret)
+		return ret;
+
+	ret = gpiod_set_transitory(desc, false);
+	if (ret) {
+		gpiod_free(desc);
+		return ret;
+	}
+
+	ret = gpiod_export(desc, true);
+	if (ret < 0) {
+		gpiod_free(desc);
+	} else {
+		set_bit(FLAG_SYSFS, &desc->flags);
+		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
+	}
+
+	return ret;
+}
+
+static int unexport_gpio_desc(struct gpio_desc *desc)
+{
+	/*
+	 * No extra locking here; FLAG_SYSFS just signifies that the
+	 * request and export were done by on behalf of userspace, so
+	 * they may be undone on its behalf too.
+	 */
+	if (!test_and_clear_bit(FLAG_SYSFS, &desc->flags))
+		return -EINVAL;
+
+	gpiod_unexport(desc);
+	gpiod_free(desc);
+
+	return 0;
+}
+
+static ssize_t do_chip_export_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, ssize_t size,
+				    int (*handler)(struct gpio_desc *desc))
+{
+	struct gpiodev_data *data = dev_get_drvdata(dev);
+	struct gpio_device *gdev = data->gdev;
+	struct gpio_desc *desc;
+	unsigned int gpio;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &gpio);
+	if (ret)
+		return ret;
+
+	desc = gpio_device_get_desc(gdev, gpio);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	ret = handler(desc);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t chip_export_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t size)
+{
+	return do_chip_export_store(dev, attr, buf, size, export_gpio_desc);
+}
+
+static struct device_attribute dev_attr_export = __ATTR(export, 0200, NULL,
+							chip_export_store);
+
+static ssize_t chip_unexport_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t size)
+{
+	return do_chip_export_store(dev, attr, buf, size, unexport_gpio_desc);
+}
+
+static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
+							  NULL,
+							  chip_unexport_store);
+
 static struct attribute *gpiochip_attrs[] = {
 	&dev_attr_base.attr,
 	&dev_attr_label.attr,
@@ -436,6 +550,15 @@ static struct attribute *gpiochip_attrs[] = {
 };
 ATTRIBUTE_GROUPS(gpiochip);
 
+static struct attribute *gpiochip_ext_attrs[] = {
+	&dev_attr_label.attr,
+	&dev_attr_ngpio.attr,
+	&dev_attr_export.attr,
+	&dev_attr_unexport.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(gpiochip_ext);
+
 /*
  * /sys/class/gpio/export ... write-only
  *	integer N ... number of GPIO to export (full access)
@@ -447,7 +570,7 @@ static ssize_t export_store(const struct class *class,
 			    const char *buf, size_t len)
 {
 	struct gpio_desc *desc;
-	int status, offset;
+	int status;
 	long gpio;
 
 	status = kstrtol(buf, 0, &gpio);
@@ -461,40 +584,7 @@ static ssize_t export_store(const struct class *class,
 		return -EINVAL;
 	}
 
-	CLASS(gpio_chip_guard, guard)(desc);
-	if (!guard.gc)
-		return -ENODEV;
-
-	offset = gpio_chip_hwgpio(desc);
-	if (!gpiochip_line_is_valid(guard.gc, offset)) {
-		pr_debug_ratelimited("%s: GPIO %ld masked\n", __func__, gpio);
-		return -EINVAL;
-	}
-
-	/* No extra locking here; FLAG_SYSFS just signifies that the
-	 * request and export were done by on behalf of userspace, so
-	 * they may be undone on its behalf too.
-	 */
-
-	status = gpiod_request_user(desc, "sysfs");
-	if (status)
-		goto done;
-
-	status = gpiod_set_transitory(desc, false);
-	if (status) {
-		gpiod_free(desc);
-		goto done;
-	}
-
-	status = gpiod_export(desc, true);
-	if (status < 0) {
-		gpiod_free(desc);
-	} else {
-		set_bit(FLAG_SYSFS, &desc->flags);
-		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-	}
-
-done:
+	status = export_gpio_desc(desc);
 	if (status)
 		pr_debug("%s: status %d\n", __func__, status);
 	return status ? : len;
@@ -511,7 +601,7 @@ static ssize_t unexport_store(const struct class *class,
 
 	status = kstrtol(buf, 0, &gpio);
 	if (status < 0)
-		goto done;
+		return status;
 
 	desc = gpio_to_desc(gpio);
 	/* reject bogus commands (gpiod_unexport() ignores them) */
@@ -520,18 +610,7 @@ static ssize_t unexport_store(const struct class *class,
 		return -EINVAL;
 	}
 
-	status = -EINVAL;
-
-	/* No extra locking here; FLAG_SYSFS just signifies that the
-	 * request and export were done by on behalf of userspace, so
-	 * they may be undone on its behalf too.
-	 */
-	if (test_and_clear_bit(FLAG_SYSFS, &desc->flags)) {
-		gpiod_unexport(desc);
-		gpiod_free(desc);
-		status = 0;
-	}
-done:
+	status = unexport_gpio_desc(desc);
 	if (status)
 		pr_debug("%s: status %d\n", __func__, status);
 	return status ? : len;
@@ -561,6 +640,11 @@ static int match_gdev(struct device *dev, const void *desc)
 static struct gpiodev_data *
 gdev_get_data(struct gpio_device *gdev) __must_hold(&sysfs_lock)
 {
+	/*
+	 * Find the first device in GPIO class that matches. Whether that's
+	 * the one indexed by GPIO base or device ID doesn't matter, it has
+	 * the same address set as driver data.
+	 */
 	struct device *cdev __free(put_device) = class_find_device(&gpio_class,
 								   NULL, gdev,
 								   match_gdev);
@@ -787,6 +871,17 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 		return err;
 	}
 
+	data->cdev_id = device_create_with_groups(&gpio_class, parent,
+						  MKDEV(0, 0), data,
+						  gpiochip_ext_groups,
+						  "chip%d", gdev->id);
+	if (IS_ERR(data->cdev_id)) {
+		device_unregister(data->cdev_base);
+		err = PTR_ERR(data->cdev_id);
+		kfree(data);
+		return err;
+	}
+
 	return 0;
 }
 
@@ -802,6 +897,7 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 			return;
 
 		device_unregister(data->cdev_base);
+		device_unregister(data->cdev_id);
 		kfree(data);
 	}
 

-- 
2.48.1

[PATCH v3 03/10] gpio: sysfs: only get the dirent reference for the value attr once

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There's no reason to retrieve the reference to the sysfs dirent every
time we request an interrupt, we can as well only do it once when
exporting the GPIO. While at it: change the name of the kernfs node
field to value_class_node ahead of adding a second one for the
chip-local GPIO line value attribute.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 990db0405cc86c42bad61295dc823f970199534e..80bb58c96b9b45ce4e1a4def8085d71baa0ce45d 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -36,7 +36,7 @@ struct gpiod_data {
 	struct gpio_desc *desc;
 
 	struct mutex mutex;
-	struct kernfs_node *value_kn;
+	struct kernfs_node *value_class_node;
 	int irq;
 	unsigned char irq_flags;
 
@@ -156,7 +156,7 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
 	struct gpiod_data *data = priv;
 
-	sysfs_notify_dirent(data->value_kn);
+	sysfs_notify_dirent(data->value_class_node);
 
 	return IRQ_HANDLED;
 }
@@ -177,10 +177,6 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	if (data->irq < 0)
 		return -EIO;
 
-	data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
-	if (!data->value_kn)
-		return -ENODEV;
-
 	irq_flags = IRQF_SHARED;
 	if (flags & GPIO_IRQF_TRIGGER_FALLING) {
 		irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
@@ -203,7 +199,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	 */
 	ret = gpiochip_lock_as_irq(guard.gc, gpio_chip_hwgpio(desc));
 	if (ret < 0)
-		goto err_put_kn;
+		goto err_clr_bits;
 
 	ret = request_any_context_irq(data->irq, gpio_sysfs_irq, irq_flags,
 				"gpiolib", data);
@@ -216,10 +212,9 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 
 err_unlock:
 	gpiochip_unlock_as_irq(guard.gc, gpio_chip_hwgpio(desc));
-err_put_kn:
+err_clr_bits:
 	clear_bit(FLAG_EDGE_RISING, &desc->flags);
 	clear_bit(FLAG_EDGE_FALLING, &desc->flags);
-	sysfs_put(data->value_kn);
 
 	return ret;
 }
@@ -242,7 +237,6 @@ static void gpio_sysfs_free_irq(struct device *dev)
 	gpiochip_unlock_as_irq(guard.gc, gpio_chip_hwgpio(desc));
 	clear_bit(FLAG_EDGE_RISING, &desc->flags);
 	clear_bit(FLAG_EDGE_FALLING, &desc->flags);
-	sysfs_put(data->value_kn);
 }
 
 static const char *const trigger_names[] = {
@@ -726,8 +720,16 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		goto err_free_data;
 	}
 
+	data->value_class_node = sysfs_get_dirent(dev->kobj.sd, "value");
+	if (!data->value_class_node) {
+		status = -ENODEV;
+		goto err_unregister_device;
+	}
+
 	return 0;
 
+err_unregister_device:
+	device_unregister(dev);
 err_free_data:
 	kfree(data);
 err_clear_bit:
@@ -804,6 +806,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 
 		data = dev_get_drvdata(dev);
 		clear_bit(FLAG_EXPORT, &desc->flags);
+		sysfs_put(data->value_class_node);
 		device_unregister(dev);
 
 		/*

-- 
2.48.1

Re: [PATCH v3 03/10] gpio: sysfs: only get the dirent reference for the value attr once

[Linus Walleij]

On Mon, Jun 30, 2025 at 2:37 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> There's no reason to retrieve the reference to the sysfs dirent every
> time we request an interrupt, we can as well only do it once when
> exporting the GPIO. While at it: change the name of the kernfs node
> field to value_class_node ahead of adding a second one for the
> chip-local GPIO line value attribute.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

[PATCH v3 04/10] gpio: sysfs: pass gpiod_data directly to internal GPIO sysfs functions

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We don't use any fields from struct device in gpio_sysfs_request_irq(),
gpio_sysfs_free_irq() and gpio_sysfs_set_active_low(). We only use the
dev argument to get the associated struct gpiod_data pointer with
dev_get_drvdata().

To make the transition to not using dev_get_drvdata() across line
callbacks for sysfs attributes easier, pass gpiod_data directly to
these functions instead of having it wrapped in struct device.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 80bb58c96b9b45ce4e1a4def8085d71baa0ce45d..59b66ee222a84b36a05458c8cd6cfa1d568e6783 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -162,9 +162,8 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 }
 
 /* Caller holds gpiod-data mutex. */
-static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
+static int gpio_sysfs_request_irq(struct gpiod_data *data, unsigned char flags)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
 	struct gpio_desc *desc = data->desc;
 	unsigned long irq_flags;
 	int ret;
@@ -223,9 +222,8 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
  * Caller holds gpiod-data mutex (unless called after class-device
  * deregistration).
  */
-static void gpio_sysfs_free_irq(struct device *dev)
+static void gpio_sysfs_free_irq(struct gpiod_data *data)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
 	struct gpio_desc *desc = data->desc;
 
 	CLASS(gpio_chip_guard, guard)(desc);
@@ -278,12 +276,12 @@ static ssize_t edge_store(struct device *dev, struct device_attribute *attr,
 		return size;
 
 	if (data->irq_flags)
-		gpio_sysfs_free_irq(dev);
+		gpio_sysfs_free_irq(data);
 
 	if (!flags)
 		return size;
 
-	status = gpio_sysfs_request_irq(dev, flags);
+	status = gpio_sysfs_request_irq(data, flags);
 	if (status)
 		return status;
 
@@ -294,9 +292,8 @@ static ssize_t edge_store(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR_RW(edge);
 
 /* Caller holds gpiod-data mutex. */
-static int gpio_sysfs_set_active_low(struct device *dev, int value)
+static int gpio_sysfs_set_active_low(struct gpiod_data *data, int value)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
 	unsigned int flags = data->irq_flags;
 	struct gpio_desc *desc = data->desc;
 	int status = 0;
@@ -309,8 +306,8 @@ static int gpio_sysfs_set_active_low(struct device *dev, int value)
 	/* reconfigure poll(2) support if enabled on one edge only */
 	if (flags == GPIO_IRQF_TRIGGER_FALLING ||
 	    flags == GPIO_IRQF_TRIGGER_RISING) {
-		gpio_sysfs_free_irq(dev);
-		status = gpio_sysfs_request_irq(dev, flags);
+		gpio_sysfs_free_irq(data);
+		status = gpio_sysfs_request_irq(data, flags);
 	}
 
 	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
@@ -345,7 +342,7 @@ static ssize_t active_low_store(struct device *dev,
 
 	guard(mutex)(&data->mutex);
 
-	return gpio_sysfs_set_active_low(dev, value) ?: size;
+	return gpio_sysfs_set_active_low(data, value) ?: size;
 }
 static DEVICE_ATTR_RW(active_low);
 
@@ -814,7 +811,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 		 * edge_store.
 		 */
 		if (data->irq_flags)
-			gpio_sysfs_free_irq(dev);
+			gpio_sysfs_free_irq(data);
 	}
 
 	put_device(dev);

-- 
2.48.1

[PATCH v3 05/10] gpio: sysfs: rename the data variable in gpiod_(un)export()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In preparation for future commits which will make use of descriptor AND
GPIO-device data in the same functions rename the former from data to
desc_data separately which will make future changes smaller and easier
to read.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 59b66ee222a84b36a05458c8cd6cfa1d568e6783..dba9dceb5e79db16f466ee40255c8f646b6b6d15 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -663,8 +663,8 @@ gdev_get_data(struct gpio_device *gdev) __must_hold(&sysfs_lock)
  */
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
+	struct gpiod_data *desc_data;
 	struct gpio_device *gdev;
-	struct gpiod_data *data;
 	struct device *dev;
 	int status;
 
@@ -696,29 +696,29 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		goto err_clear_bit;
 	}
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data) {
+	desc_data = kzalloc(sizeof(*desc_data), GFP_KERNEL);
+	if (!desc_data) {
 		status = -ENOMEM;
 		goto err_clear_bit;
 	}
 
-	data->desc = desc;
-	mutex_init(&data->mutex);
+	desc_data->desc = desc;
+	mutex_init(&desc_data->mutex);
 	if (guard.gc->direction_input && guard.gc->direction_output)
-		data->direction_can_change = direction_may_change;
+		desc_data->direction_can_change = direction_may_change;
 	else
-		data->direction_can_change = false;
+		desc_data->direction_can_change = false;
 
 	dev = device_create_with_groups(&gpio_class, &gdev->dev,
-					MKDEV(0, 0), data, gpio_groups,
+					MKDEV(0, 0), desc_data, gpio_groups,
 					"gpio%u", desc_to_gpio(desc));
 	if (IS_ERR(dev)) {
 		status = PTR_ERR(dev);
 		goto err_free_data;
 	}
 
-	data->value_class_node = sysfs_get_dirent(dev->kobj.sd, "value");
-	if (!data->value_class_node) {
+	desc_data->value_class_node = sysfs_get_dirent(dev->kobj.sd, "value");
+	if (!desc_data->value_class_node) {
 		status = -ENODEV;
 		goto err_unregister_device;
 	}
@@ -728,7 +728,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 err_unregister_device:
 	device_unregister(dev);
 err_free_data:
-	kfree(data);
+	kfree(desc_data);
 err_clear_bit:
 	clear_bit(FLAG_EXPORT, &desc->flags);
 	gpiod_dbg(desc, "%s: status %d\n", __func__, status);
@@ -785,7 +785,7 @@ EXPORT_SYMBOL_GPL(gpiod_export_link);
  */
 void gpiod_unexport(struct gpio_desc *desc)
 {
-	struct gpiod_data *data;
+	struct gpiod_data *desc_data;
 	struct device *dev;
 
 	if (!desc) {
@@ -801,22 +801,22 @@ void gpiod_unexport(struct gpio_desc *desc)
 		if (!dev)
 			return;
 
-		data = dev_get_drvdata(dev);
+		desc_data = dev_get_drvdata(dev);
 		clear_bit(FLAG_EXPORT, &desc->flags);
-		sysfs_put(data->value_class_node);
+		sysfs_put(desc_data->value_class_node);
 		device_unregister(dev);
 
 		/*
 		 * Release irq after deregistration to prevent race with
 		 * edge_store.
 		 */
-		if (data->irq_flags)
-			gpio_sysfs_free_irq(data);
+		if (desc_data->irq_flags)
+			gpio_sysfs_free_irq(desc_data);
 	}
 
 	put_device(dev);
-	mutex_destroy(&data->mutex);
-	kfree(data);
+	mutex_destroy(&desc_data->mutex);
+	kfree(desc_data);
 }
 EXPORT_SYMBOL_GPL(gpiod_unexport);
 

-- 
2.48.1

[PATCH v3 06/10] gpio: sysfs: don't use driver data in sysfs callbacks for line attributes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Currently each exported GPIO is represented in sysfs as a separate class
device. This allows us to simply use dev_get_drvdata() to retrieve the
pointer passed to device_create_with_groups() from sysfs ops callbacks.

However, we're preparing to add a parallel set of per-line sysfs
attributes that will live inside the associated gpiochip group. They are
not registered as class devices and so have the parent device passed as
argument to their callbacks (the GPIO chip class device).

Put the attribute structs inside the GPIO descriptor data and
dereference the relevant ones using container_of() in the callbacks.
This way, we'll be able to reuse the same code for both the legacy and
new GPIO attributes.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 122 +++++++++++++++++++++++++++++--------------
 1 file changed, 82 insertions(+), 40 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index dba9dceb5e79db16f466ee40255c8f646b6b6d15..c1de8846cc4c1fc9e871dfa4a7d7a8f287043c7d 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -32,6 +32,15 @@ struct kernfs_node;
 #define GPIO_IRQF_TRIGGER_BOTH		(GPIO_IRQF_TRIGGER_FALLING | \
 					 GPIO_IRQF_TRIGGER_RISING)
 
+enum {
+	GPIO_SYSFS_LINE_ATTR_DIRECTION = 0,
+	GPIO_SYSFS_LINE_ATTR_VALUE,
+	GPIO_SYSFS_LINE_ATTR_EDGE,
+	GPIO_SYSFS_LINE_ATTR_ACTIVE_LOW,
+	GPIO_SYSFS_LINE_ATTR_SENTINEL,
+	GPIO_SYSFS_LINE_ATTR_SIZE,
+};
+
 struct gpiod_data {
 	struct gpio_desc *desc;
 
@@ -41,6 +50,14 @@ struct gpiod_data {
 	unsigned char irq_flags;
 
 	bool direction_can_change;
+
+	struct device_attribute dir_attr;
+	struct device_attribute val_attr;
+	struct device_attribute edge_attr;
+	struct device_attribute active_low_attr;
+	struct attribute *attrs[GPIO_SYSFS_LINE_ATTR_SIZE];
+	struct attribute_group attr_group;
+	const struct attribute_group *attr_groups[2];
 };
 
 struct gpiodev_data {
@@ -79,7 +96,8 @@ static DEFINE_MUTEX(sysfs_lock);
 static ssize_t direction_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpiod_data *data = container_of(attr, struct gpiod_data,
+					       dir_attr);
 	struct gpio_desc *desc = data->desc;
 	int value;
 
@@ -95,7 +113,8 @@ static ssize_t direction_store(struct device *dev,
 			       struct device_attribute *attr, const char *buf,
 			       size_t size)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpiod_data *data = container_of(attr, struct gpiod_data,
+					       dir_attr);
 	struct gpio_desc *desc = data->desc;
 	ssize_t status;
 
@@ -112,12 +131,12 @@ static ssize_t direction_store(struct device *dev,
 
 	return status ? : size;
 }
-static DEVICE_ATTR_RW(direction);
 
 static ssize_t value_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpiod_data *data = container_of(attr, struct gpiod_data,
+					       val_attr);
 	struct gpio_desc *desc = data->desc;
 	ssize_t status;
 
@@ -133,7 +152,8 @@ static ssize_t value_show(struct device *dev, struct device_attribute *attr,
 static ssize_t value_store(struct device *dev, struct device_attribute *attr,
 			   const char *buf, size_t size)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpiod_data *data = container_of(attr, struct gpiod_data,
+					       val_attr);
 	struct gpio_desc *desc = data->desc;
 	ssize_t status;
 	long value;
@@ -150,7 +170,6 @@ static ssize_t value_store(struct device *dev, struct device_attribute *attr,
 
 	return size;
 }
-static DEVICE_ATTR_PREALLOC(value, S_IWUSR | S_IRUGO, value_show, value_store);
 
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
@@ -247,7 +266,8 @@ static const char *const trigger_names[] = {
 static ssize_t edge_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpiod_data *data = container_of(attr, struct gpiod_data,
+					       edge_attr);
 	int flags;
 
 	scoped_guard(mutex, &data->mutex)
@@ -262,7 +282,8 @@ static ssize_t edge_show(struct device *dev, struct device_attribute *attr,
 static ssize_t edge_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t size)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpiod_data *data = container_of(attr, struct gpiod_data,
+					       edge_attr);
 	ssize_t status = size;
 	int flags;
 
@@ -289,7 +310,6 @@ static ssize_t edge_store(struct device *dev, struct device_attribute *attr,
 
 	return size;
 }
-static DEVICE_ATTR_RW(edge);
 
 /* Caller holds gpiod-data mutex. */
 static int gpio_sysfs_set_active_low(struct gpiod_data *data, int value)
@@ -318,7 +338,8 @@ static int gpio_sysfs_set_active_low(struct gpiod_data *data, int value)
 static ssize_t active_low_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpiod_data *data = container_of(attr, struct gpiod_data,
+					       active_low_attr);
 	struct gpio_desc *desc = data->desc;
 	int value;
 
@@ -332,7 +353,8 @@ static ssize_t active_low_store(struct device *dev,
 				struct device_attribute *attr,
 				const char *buf, size_t size)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpiod_data *data = container_of(attr, struct gpiod_data,
+					       active_low_attr);
 	ssize_t status;
 	long value;
 
@@ -344,48 +366,34 @@ static ssize_t active_low_store(struct device *dev,
 
 	return gpio_sysfs_set_active_low(data, value) ?: size;
 }
-static DEVICE_ATTR_RW(active_low);
 
 static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
 			       int n)
 {
-	struct device *dev = kobj_to_dev(kobj);
-	struct gpiod_data *data = dev_get_drvdata(dev);
-	struct gpio_desc *desc = data->desc;
+	struct device_attribute *dev_attr = container_of(attr,
+						struct device_attribute, attr);
 	umode_t mode = attr->mode;
-	bool show_direction = data->direction_can_change;
+	struct gpiod_data *data;
 
-	if (attr == &dev_attr_direction.attr) {
-		if (!show_direction)
+	if (strcmp(attr->name, "direction") == 0) {
+		data = container_of(dev_attr, struct gpiod_data, dir_attr);
+
+		if (!data->direction_can_change)
 			mode = 0;
-	} else if (attr == &dev_attr_edge.attr) {
-		if (gpiod_to_irq(desc) < 0)
+	} else if (strcmp(attr->name, "edge") == 0) {
+		data = container_of(dev_attr, struct gpiod_data, edge_attr);
+
+		if (gpiod_to_irq(data->desc) < 0)
 			mode = 0;
-		if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
+
+		if (!data->direction_can_change &&
+		    test_bit(FLAG_IS_OUT, &data->desc->flags))
 			mode = 0;
 	}
 
 	return mode;
 }
 
-static struct attribute *gpio_attrs[] = {
-	&dev_attr_direction.attr,
-	&dev_attr_edge.attr,
-	&dev_attr_value.attr,
-	&dev_attr_active_low.attr,
-	NULL,
-};
-
-static const struct attribute_group gpio_group = {
-	.attrs = gpio_attrs,
-	.is_visible = gpio_is_visible,
-};
-
-static const struct attribute_group *gpio_groups[] = {
-	&gpio_group,
-	NULL
-};
-
 /*
  * /sys/class/gpio/gpiochipN/
  *   /base ... matching gpio_chip.base (N)
@@ -645,6 +653,21 @@ gdev_get_data(struct gpio_device *gdev) __must_hold(&sysfs_lock)
 	return dev_get_drvdata(cdev);
 };
 
+static void gpiod_attr_init(struct device_attribute *dev_attr, const char *name,
+			    ssize_t (*show)(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf),
+			    ssize_t (*store)(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count))
+{
+	sysfs_attr_init(&dev_attr->attr);
+	dev_attr->attr.name = name;
+	dev_attr->attr.mode = 0644;
+	dev_attr->show = show;
+	dev_attr->store = store;
+}
+
 /**
  * gpiod_export - export a GPIO through sysfs
  * @desc: GPIO to make available, already requested
@@ -665,6 +688,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
 	struct gpiod_data *desc_data;
 	struct gpio_device *gdev;
+	struct attribute **attrs;
 	struct device *dev;
 	int status;
 
@@ -709,8 +733,26 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	else
 		desc_data->direction_can_change = false;
 
+	gpiod_attr_init(&desc_data->dir_attr, "direction",
+			direction_show, direction_store);
+	gpiod_attr_init(&desc_data->val_attr, "value", value_show, value_store);
+	gpiod_attr_init(&desc_data->edge_attr, "edge", edge_show, edge_store);
+	gpiod_attr_init(&desc_data->active_low_attr, "active_low",
+			active_low_show, active_low_store);
+
+	attrs = desc_data->attrs;
+	desc_data->attr_group.is_visible = gpio_is_visible;
+	attrs[GPIO_SYSFS_LINE_ATTR_DIRECTION] = &desc_data->dir_attr.attr;
+	attrs[GPIO_SYSFS_LINE_ATTR_VALUE] = &desc_data->val_attr.attr;
+	attrs[GPIO_SYSFS_LINE_ATTR_EDGE] = &desc_data->edge_attr.attr;
+	attrs[GPIO_SYSFS_LINE_ATTR_ACTIVE_LOW] = &desc_data->active_low_attr.attr;
+
+	desc_data->attr_group.attrs = desc_data->attrs;
+	desc_data->attr_groups[0] = &desc_data->attr_group;
+
 	dev = device_create_with_groups(&gpio_class, &gdev->dev,
-					MKDEV(0, 0), desc_data, gpio_groups,
+					MKDEV(0, 0), desc_data,
+					desc_data->attr_groups,
 					"gpio%u", desc_to_gpio(desc));
 	if (IS_ERR(dev)) {
 		status = PTR_ERR(dev);

-- 
2.48.1

Re: [PATCH v3 06/10] gpio: sysfs: don't use driver data in sysfs callbacks for line attributes

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 02:37:13PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Currently each exported GPIO is represented in sysfs as a separate class
> device. This allows us to simply use dev_get_drvdata() to retrieve the
> pointer passed to device_create_with_groups() from sysfs ops callbacks.
> 
> However, we're preparing to add a parallel set of per-line sysfs
> attributes that will live inside the associated gpiochip group. They are
> not registered as class devices and so have the parent device passed as
> argument to their callbacks (the GPIO chip class device).
> 
> Put the attribute structs inside the GPIO descriptor data and
> dereference the relevant ones using container_of() in the callbacks.
> This way, we'll be able to reuse the same code for both the legacy and
> new GPIO attributes.

...

> -	struct gpiod_data *data = dev_get_drvdata(dev);
> +	struct gpiod_data *data = container_of(attr, struct gpiod_data,
> +					       dir_attr);

> -	struct gpiod_data *data = dev_get_drvdata(dev);
> +	struct gpiod_data *data = container_of(attr, struct gpiod_data,
> +					       dir_attr);

I still think that bunch of definitions to wrap contaner_of():s will help.
Note, two of them you are using even in the code below, besides the attribute
show()/store() cases.

...

> +	if (strcmp(attr->name, "direction") == 0) {
> +		data = container_of(dev_attr, struct gpiod_data, dir_attr);
> +
> +		if (!data->direction_can_change)
>  			mode = 0;

		data = dir_attr_to_gpiod_data(dev_attr);
		if (!data->direction_can_change)
			mode = 0;

> +	} else if (strcmp(attr->name, "edge") == 0) {

> +		data = container_of(dev_attr, struct gpiod_data, edge_attr);
> +
> +		if (gpiod_to_irq(data->desc) < 0)
>  			mode = 0;

In the similar way:

		data = edge_attr_to_gpiod_data(dev_attr);
		if (gpiod_to_irq(data->desc) < 0)
			mode = 0;

> +		if (!data->direction_can_change &&
> +		    test_bit(FLAG_IS_OUT, &data->desc->flags))
>  			mode = 0;
>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 06/10] gpio: sysfs: don't use driver data in sysfs callbacks for line attributes

[Bartosz Golaszewski]

On Mon, Jun 30, 2025 at 3:28 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jun 30, 2025 at 02:37:13PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Currently each exported GPIO is represented in sysfs as a separate class
> > device. This allows us to simply use dev_get_drvdata() to retrieve the
> > pointer passed to device_create_with_groups() from sysfs ops callbacks.
> >
> > However, we're preparing to add a parallel set of per-line sysfs
> > attributes that will live inside the associated gpiochip group. They are
> > not registered as class devices and so have the parent device passed as
> > argument to their callbacks (the GPIO chip class device).
> >
> > Put the attribute structs inside the GPIO descriptor data and
> > dereference the relevant ones using container_of() in the callbacks.
> > This way, we'll be able to reuse the same code for both the legacy and
> > new GPIO attributes.
>
> ...
>
> > -     struct gpiod_data *data = dev_get_drvdata(dev);
> > +     struct gpiod_data *data = container_of(attr, struct gpiod_data,
> > +                                            dir_attr);
>
> > -     struct gpiod_data *data = dev_get_drvdata(dev);
> > +     struct gpiod_data *data = container_of(attr, struct gpiod_data,
> > +                                            dir_attr);
>
> I still think that bunch of definitions to wrap contaner_of():s will help.
> Note, two of them you are using even in the code below, besides the attribute
> show()/store() cases.
>

I think this is overkill, the container_of() macro is pretty explicit
and having 4 new macros will result in more LOC in the end. I'll allow
myself to keep this as is.

Bart

> ...
>
> > +     if (strcmp(attr->name, "direction") == 0) {
> > +             data = container_of(dev_attr, struct gpiod_data, dir_attr);
> > +
> > +             if (!data->direction_can_change)
> >                       mode = 0;
>
>                 data = dir_attr_to_gpiod_data(dev_attr);
>                 if (!data->direction_can_change)
>                         mode = 0;
>
> > +     } else if (strcmp(attr->name, "edge") == 0) {
>
> > +             data = container_of(dev_attr, struct gpiod_data, edge_attr);
> > +
> > +             if (gpiod_to_irq(data->desc) < 0)
> >                       mode = 0;
>
> In the similar way:
>
>                 data = edge_attr_to_gpiod_data(dev_attr);
>                 if (gpiod_to_irq(data->desc) < 0)
>                         mode = 0;
>
> > +             if (!data->direction_can_change &&
> > +                 test_bit(FLAG_IS_OUT, &data->desc->flags))
> >                       mode = 0;
> >       }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3 06/10] gpio: sysfs: don't use driver data in sysfs callbacks for line attributes

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 03:39:20PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jun 30, 2025 at 3:28 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Jun 30, 2025 at 02:37:13PM +0200, Bartosz Golaszewski wrote:

...

> > > -     struct gpiod_data *data = dev_get_drvdata(dev);
> > > +     struct gpiod_data *data = container_of(attr, struct gpiod_data,
> > > +                                            dir_attr);
> >
> > > -     struct gpiod_data *data = dev_get_drvdata(dev);
> > > +     struct gpiod_data *data = container_of(attr, struct gpiod_data,
> > > +                                            dir_attr);
> >
> > I still think that bunch of definitions to wrap contaner_of():s will help.
> > Note, two of them you are using even in the code below, besides the attribute
> > show()/store() cases.
> 
> I think this is overkill, the container_of() macro is pretty explicit
> and having 4 new macros will result in more LOC in the end.

I don't know how you counted. I counted as first one takes the same +4/-4 LoCs,
second one will gain already 1 LoC if we stack them as

#define foo_attr_to_gpiod_data(a)	container_of(...)
#define bar_attr_to_gpiod_data(a)	container_of(...)

...and so on...

But yes, some people are still fans of 80, and this might distort
the calculations I made.

> I'll allow myself to keep this as is.

Fair enough.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 07/10] gpio: sysfs: don't look up exported lines as class devices

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In preparation for adding a parallel, per-chip attribute group for
exported GPIO lines, stop using class device APIs to refer to it in the
code. When unregistering the chip, don't call class_find_device() but
instead store exported lines in a linked list inside the GPIO chip data
object and look it up there.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 60 ++++++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c1de8846cc4c1fc9e871dfa4a7d7a8f287043c7d..bd84489ac3cf7a25c4c876af0668a14858d7ac34 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -42,7 +42,10 @@ enum {
 };
 
 struct gpiod_data {
+	struct list_head list;
+
 	struct gpio_desc *desc;
+	struct device *dev;
 
 	struct mutex mutex;
 	struct kernfs_node *value_class_node;
@@ -61,6 +64,7 @@ struct gpiod_data {
 };
 
 struct gpiodev_data {
+	struct list_head exported_lines;
 	struct gpio_device *gdev;
 	struct device *cdev_id; /* Class device by GPIO device ID */
 	struct device *cdev_base; /* Class device by GPIO base */
@@ -686,10 +690,10 @@ static void gpiod_attr_init(struct device_attribute *dev_attr, const char *name,
  */
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
+	struct gpiodev_data *gdev_data;
 	struct gpiod_data *desc_data;
 	struct gpio_device *gdev;
 	struct attribute **attrs;
-	struct device *dev;
 	int status;
 
 	/* can't export until sysfs is available ... */
@@ -750,25 +754,40 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	desc_data->attr_group.attrs = desc_data->attrs;
 	desc_data->attr_groups[0] = &desc_data->attr_group;
 
-	dev = device_create_with_groups(&gpio_class, &gdev->dev,
-					MKDEV(0, 0), desc_data,
-					desc_data->attr_groups,
-					"gpio%u", desc_to_gpio(desc));
-	if (IS_ERR(dev)) {
-		status = PTR_ERR(dev);
+	/*
+	 * Note: we need to continue passing desc_data here as there's still
+	 * at least one known user of gpiod_export_link() in the tree. This
+	 * function still uses class_find_device() internally.
+	 */
+	desc_data->dev = device_create_with_groups(&gpio_class, &gdev->dev,
+						   MKDEV(0, 0), desc_data,
+						   desc_data->attr_groups,
+						   "gpio%u",
+						   desc_to_gpio(desc));
+	if (IS_ERR(desc_data->dev)) {
+		status = PTR_ERR(desc_data->dev);
 		goto err_free_data;
 	}
 
-	desc_data->value_class_node = sysfs_get_dirent(dev->kobj.sd, "value");
+	desc_data->value_class_node = sysfs_get_dirent(desc_data->dev->kobj.sd,
+						       "value");
 	if (!desc_data->value_class_node) {
 		status = -ENODEV;
 		goto err_unregister_device;
 	}
 
+	gdev_data = gdev_get_data(gdev);
+	if (!gdev_data) {
+		status = -ENODEV;
+		goto err_unregister_device;
+	}
+
+	list_add(&desc_data->list, &gdev_data->exported_lines);
+
 	return 0;
 
 err_unregister_device:
-	device_unregister(dev);
+	device_unregister(desc_data->dev);
 err_free_data:
 	kfree(desc_data);
 err_clear_bit:
@@ -827,8 +846,9 @@ EXPORT_SYMBOL_GPL(gpiod_export_link);
  */
 void gpiod_unexport(struct gpio_desc *desc)
 {
-	struct gpiod_data *desc_data;
-	struct device *dev;
+	struct gpiod_data *desc_data = NULL;
+	struct gpiodev_data *gdev_data;
+	struct gpio_device *gdev;
 
 	if (!desc) {
 		pr_warn("%s: invalid GPIO\n", __func__);
@@ -839,14 +859,22 @@ void gpiod_unexport(struct gpio_desc *desc)
 		if (!test_bit(FLAG_EXPORT, &desc->flags))
 			return;
 
-		dev = class_find_device(&gpio_class, NULL, desc, match_export);
-		if (!dev)
+		gdev = gpiod_to_gpio_device(desc);
+		gdev_data = gdev_get_data(gdev);
+		if (!gdev_data)
 			return;
 
-		desc_data = dev_get_drvdata(dev);
+		list_for_each_entry(desc_data, &gdev_data->exported_lines, list)
+			if (gpiod_is_equal(desc, desc_data->desc))
+				break;
+
+		if (!desc_data)
+			return;
+
+		list_del(&desc_data->list);
 		clear_bit(FLAG_EXPORT, &desc->flags);
 		sysfs_put(desc_data->value_class_node);
-		device_unregister(dev);
+		device_unregister(desc_data->dev);
 
 		/*
 		 * Release irq after deregistration to prevent race with
@@ -856,7 +884,6 @@ void gpiod_unexport(struct gpio_desc *desc)
 			gpio_sysfs_free_irq(desc_data);
 	}
 
-	put_device(dev);
 	mutex_destroy(&desc_data->mutex);
 	kfree(desc_data);
 }
@@ -898,6 +925,7 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 		return -ENOMEM;
 
 	data->gdev = gdev;
+	INIT_LIST_HEAD(&data->exported_lines);
 
 	guard(mutex)(&sysfs_lock);
 

-- 
2.48.1

[PATCH v3 08/10] gpio: sysfs: export the GPIO directory locally in the gpiochip<id> directory

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As a way to allow the user-space to stop referring to GPIOs by their
global numbers, introduce a parallel group of line attributes for
exported GPIO that live inside the GPIO chip class device and are
referred to by their HW offset within their parent chip.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 Documentation/ABI/obsolete/sysfs-gpio |  5 +++++
 drivers/gpio/gpiolib-sysfs.c          | 40 ++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/obsolete/sysfs-gpio b/Documentation/ABI/obsolete/sysfs-gpio
index ff694708a3bef787afa42dedf94faf209c44dbf0..c0bb51412a912cefe032c4e84288f99754acb1b5 100644
--- a/Documentation/ABI/obsolete/sysfs-gpio
+++ b/Documentation/ABI/obsolete/sysfs-gpio
@@ -27,6 +27,11 @@ Description:
 	    /base ... (r/o) same as N
 	    /label ... (r/o) descriptive chip name
 	    /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1)
+	    /gpio<OFFSET>
+	        /value ... always readable, writes fail for input GPIOs
+	        /direction ... r/w as: in, out (default low); write: high, low
+	        /edge ... r/w as: none, falling, rising, both
+	        /active-low ... r/w as: 0, 1
 	/chipX ... for each gpiochip; #X is the gpio device ID
 	    /export ... asks the kernel to export a GPIO at HW offset X to userspace
 	    /unexport ... to return a GPIO at HW offset X to the kernel
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index bd84489ac3cf7a25c4c876af0668a14858d7ac34..5b8ea601a3ddf0c8442589db8b1111ecb26e1221 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -49,11 +49,13 @@ struct gpiod_data {
 
 	struct mutex mutex;
 	struct kernfs_node *value_class_node;
+	struct kernfs_node *value_chip_node;
 	int irq;
 	unsigned char irq_flags;
 
 	bool direction_can_change;
 
+	struct kobject *parent;
 	struct device_attribute dir_attr;
 	struct device_attribute val_attr;
 	struct device_attribute edge_attr;
@@ -180,6 +182,7 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 	struct gpiod_data *data = priv;
 
 	sysfs_notify_dirent(data->value_class_node);
+	kernfs_notify(data->value_chip_node);
 
 	return IRQ_HANDLED;
 }
@@ -690,6 +693,7 @@ static void gpiod_attr_init(struct device_attribute *dev_attr, const char *name,
  */
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
+	char *path __free(kfree) = NULL;
 	struct gpiodev_data *gdev_data;
 	struct gpiod_data *desc_data;
 	struct gpio_device *gdev;
@@ -779,13 +783,45 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	gdev_data = gdev_get_data(gdev);
 	if (!gdev_data) {
 		status = -ENODEV;
-		goto err_unregister_device;
+		goto err_put_dirent;
 	}
 
 	list_add(&desc_data->list, &gdev_data->exported_lines);
 
+	desc_data->attr_group.name = kasprintf(GFP_KERNEL, "gpio%u",
+					       gpio_chip_hwgpio(desc));
+	if (!desc_data->attr_group.name) {
+		status = -ENOMEM;
+		goto err_put_dirent;
+	}
+
+	desc_data->parent = &gdev_data->cdev_id->kobj;
+	status = sysfs_create_groups(desc_data->parent,
+				     desc_data->attr_groups);
+	if (status)
+		goto err_free_name;
+
+	path = kasprintf(GFP_KERNEL, "gpio%u/value", gpio_chip_hwgpio(desc));
+	if (!path) {
+		status = -ENOMEM;
+		goto err_remove_groups;
+	}
+
+	desc_data->value_chip_node = kernfs_walk_and_get(desc_data->parent->sd,
+							 path);
+	if (!desc_data->value_chip_node) {
+		status = -ENODEV;
+		goto err_remove_groups;
+	}
+
 	return 0;
 
+err_remove_groups:
+	sysfs_remove_groups(desc_data->parent, desc_data->attr_groups);
+err_free_name:
+	kfree(desc_data->attr_group.name);
+err_put_dirent:
+	sysfs_put(desc_data->value_class_node);
 err_unregister_device:
 	device_unregister(desc_data->dev);
 err_free_data:
@@ -875,6 +911,8 @@ void gpiod_unexport(struct gpio_desc *desc)
 		clear_bit(FLAG_EXPORT, &desc->flags);
 		sysfs_put(desc_data->value_class_node);
 		device_unregister(desc_data->dev);
+		sysfs_remove_groups(desc_data->parent, desc_data->attr_groups);
+		kernfs_put(desc_data->value_chip_node);
 
 		/*
 		 * Release irq after deregistration to prevent race with

-- 
2.48.1

Re: [PATCH v3 08/10] gpio: sysfs: export the GPIO directory locally in the gpiochip<id> directory

[Linus Walleij]

On Mon, Jun 30, 2025 at 2:37 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> As a way to allow the user-space to stop referring to GPIOs by their
> global numbers, introduce a parallel group of line attributes for
> exported GPIO that live inside the GPIO chip class device and are
> referred to by their HW offset within their parent chip.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

[PATCH v3 09/10] gpio: sysfs: allow disabling the legacy parts of the GPIO sysfs interface

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a Kconfig switch allowing to disable the legacy parts of the GPIO
sysfs interface. This means that even though we keep the
/sys/class/gpio/ directory, it no longer contains the global
export/unexport attribute pair (instead, the user should use the
per-chip export/unpexport) nor the gpiochip$BASE entries. This option
default to y if GPIO sysfs is enabled but we'll default it to n at some
point in the future.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/Kconfig         |  8 ++++++++
 drivers/gpio/gpiolib-sysfs.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 44f922e10db2f8dcbdacf79ccd27b0fd9cd93564..d040fdd95ee4b19851057fbedbb023f277149c9c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -69,6 +69,14 @@ config GPIO_SYSFS
 	  use the character device /dev/gpiochipN with the appropriate
 	  ioctl() operations instead.
 
+config GPIO_SYSFS_LEGACY
+	bool "Enable legacy functionalities of the sysfs interface"
+	depends on GPIO_SYSFS
+	default y if GPIO_SYSFS
+	help
+	  Say Y here if you want to enable the legacy, global GPIO
+	  numberspace-based functionalities of the sysfs interface.
+
 config GPIO_CDEV
 	bool "Character device (/dev/gpiochipN) support" if EXPERT
 	default y
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 5b8ea601a3ddf0c8442589db8b1111ecb26e1221..d33fd7eeb82b2dc69861c4fc89613380df7efeea 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -48,7 +48,9 @@ struct gpiod_data {
 	struct device *dev;
 
 	struct mutex mutex;
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	struct kernfs_node *value_class_node;
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 	struct kernfs_node *value_chip_node;
 	int irq;
 	unsigned char irq_flags;
@@ -69,7 +71,9 @@ struct gpiodev_data {
 	struct list_head exported_lines;
 	struct gpio_device *gdev;
 	struct device *cdev_id; /* Class device by GPIO device ID */
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	struct device *cdev_base; /* Class device by GPIO base */
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 };
 
 /*
@@ -181,7 +185,9 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
 	struct gpiod_data *data = priv;
 
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	sysfs_notify_dirent(data->value_class_node);
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 	kernfs_notify(data->value_chip_node);
 
 	return IRQ_HANDLED;
@@ -416,6 +422,7 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
  *   /ngpio ... matching gpio_chip.ngpio
  */
 
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 static ssize_t base_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
@@ -424,6 +431,7 @@ static ssize_t base_show(struct device *dev, struct device_attribute *attr,
 	return sysfs_emit(buf, "%u\n", data->gdev->base);
 }
 static DEVICE_ATTR_RO(base);
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 
 static ssize_t label_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
@@ -548,6 +556,7 @@ static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
 							  NULL,
 							  chip_unexport_store);
 
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 static struct attribute *gpiochip_attrs[] = {
 	&dev_attr_base.attr,
 	&dev_attr_label.attr,
@@ -555,6 +564,7 @@ static struct attribute *gpiochip_attrs[] = {
 	NULL,
 };
 ATTRIBUTE_GROUPS(gpiochip);
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 
 static struct attribute *gpiochip_ext_attrs[] = {
 	&dev_attr_label.attr,
@@ -565,6 +575,7 @@ static struct attribute *gpiochip_ext_attrs[] = {
 };
 ATTRIBUTE_GROUPS(gpiochip_ext);
 
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 /*
  * /sys/class/gpio/export ... write-only
  *	integer N ... number of GPIO to export (full access)
@@ -629,10 +640,13 @@ static struct attribute *gpio_class_attrs[] = {
 	NULL,
 };
 ATTRIBUTE_GROUPS(gpio_class);
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 
 static const struct class gpio_class = {
 	.name =		"gpio",
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	.class_groups =	gpio_class_groups,
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 };
 
 static int match_gdev(struct device *dev, const void *desc)
@@ -763,6 +777,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	 * at least one known user of gpiod_export_link() in the tree. This
 	 * function still uses class_find_device() internally.
 	 */
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	desc_data->dev = device_create_with_groups(&gpio_class, &gdev->dev,
 						   MKDEV(0, 0), desc_data,
 						   desc_data->attr_groups,
@@ -779,6 +794,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		status = -ENODEV;
 		goto err_unregister_device;
 	}
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 
 	gdev_data = gdev_get_data(gdev);
 	if (!gdev_data) {
@@ -821,10 +837,12 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 err_free_name:
 	kfree(desc_data->attr_group.name);
 err_put_dirent:
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	sysfs_put(desc_data->value_class_node);
 err_unregister_device:
 	device_unregister(desc_data->dev);
 err_free_data:
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 	kfree(desc_data);
 err_clear_bit:
 	clear_bit(FLAG_EXPORT, &desc->flags);
@@ -833,12 +851,14 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 }
 EXPORT_SYMBOL_GPL(gpiod_export);
 
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 static int match_export(struct device *dev, const void *desc)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
 
 	return gpiod_is_equal(data->desc, desc);
 }
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 
 /**
  * gpiod_export_link - create a sysfs link to an exported GPIO node
@@ -855,6 +875,7 @@ static int match_export(struct device *dev, const void *desc)
 int gpiod_export_link(struct device *dev, const char *name,
 		      struct gpio_desc *desc)
 {
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	struct device *cdev;
 	int ret;
 
@@ -871,6 +892,9 @@ int gpiod_export_link(struct device *dev, const char *name,
 	put_device(cdev);
 
 	return ret;
+#else
+	return -EOPNOTSUPP;
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 }
 EXPORT_SYMBOL_GPL(gpiod_export_link);
 
@@ -909,8 +933,10 @@ void gpiod_unexport(struct gpio_desc *desc)
 
 		list_del(&desc_data->list);
 		clear_bit(FLAG_EXPORT, &desc->flags);
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 		sysfs_put(desc_data->value_class_node);
 		device_unregister(desc_data->dev);
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 		sysfs_remove_groups(desc_data->parent, desc_data->attr_groups);
 		kernfs_put(desc_data->value_chip_node);
 
@@ -967,6 +993,7 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 
 	guard(mutex)(&sysfs_lock);
 
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	/* use chip->base for the ID; it's already known to be unique */
 	data->cdev_base = device_create_with_groups(&gpio_class, parent,
 						    MKDEV(0, 0), data,
@@ -978,13 +1005,16 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 		kfree(data);
 		return err;
 	}
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 
 	data->cdev_id = device_create_with_groups(&gpio_class, parent,
 						  MKDEV(0, 0), data,
 						  gpiochip_ext_groups,
 						  "chip%d", gdev->id);
 	if (IS_ERR(data->cdev_id)) {
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 		device_unregister(data->cdev_base);
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 		err = PTR_ERR(data->cdev_id);
 		kfree(data);
 		return err;
@@ -1004,7 +1034,9 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 		if (!data)
 			return;
 
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 		device_unregister(data->cdev_base);
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 		device_unregister(data->cdev_id);
 		kfree(data);
 	}

-- 
2.48.1

[PATCH v3 10/10] gpio: TODO: remove the task for the sysfs rework

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Remove the completed task tracking the rework of the sysfs interface.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/TODO | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpio/TODO b/drivers/gpio/TODO
index ef53892cb44d7c01d100f10f1b805c0aca561b46..a6380e51699cc5704aebefbda906762fa60cfef3 100644
--- a/drivers/gpio/TODO
+++ b/drivers/gpio/TODO
@@ -188,19 +188,6 @@ remove the old ones and finally rename the new ones back to the old names.
 
 -------------------------------------------------------------------------------
 
-Extend the sysfs ABI to allow exporting lines by their HW offsets
-
-The need to support the sysfs GPIO class is one of the main obstacles to
-removing the global GPIO numberspace from the kernel. In order to wean users
-off using global numbers from user-space, extend the existing interface with
-new per-gpiochip export/unexport attributes that allow to refer to GPIOs using
-their hardware offsets within the chip.
-
-Encourage users to switch to using them and eventually remove the existing
-global export/unexport attribues.
-
--------------------------------------------------------------------------------
-
 Remove GPIOD_FLAGS_BIT_NONEXCLUSIVE
 
 GPIOs in the linux kernel are meant to be an exclusive resource. This means

-- 
2.48.1

Re: [PATCH v3 10/10] gpio: TODO: remove the task for the sysfs rework

[Linus Walleij]

On Mon, Jun 30, 2025 at 2:37 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Remove the completed task tracking the rework of the sysfs interface.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Maye add a task to delete
default y if GPIO_SYSFS
for the legacy sysfs in a few kernel releases from now, or whatever is
required to
shift people toward the new ABI?

... and then a task to just delete the old sysfs some years after that.

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

Yours,
Linus Walleij

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 02:37:07PM +0200, Bartosz Golaszewski wrote:
> Following our discussion[1], here's a proposal for extending the sysfs
> interface with attributes not referring to GPIO lines by their global
> numbers in a backward compatible way.
> 
> Long story short: there is now a new class device for each GPIO chip.
> It's called chipX where X is the ID of the device as per the driver
> model and it lives next to the old gpiochipABC where ABC is the GPIO
> base. Each new chip class device has a pair of export/unexport
> attributes which work similarly to the global ones under /sys/class/gpio
> but take hardware offsets within the chip as input, instead of the
> global numbers. Finally, each exported line appears at the same time as
> the global /sys/class/gpio/gpioABC as well as per-chip
> /sys/class/gpio/chipX/gpioY sysfs group.
> 
> The series contains the implementation of a parallel GPIO chip entry not
> containing the base GPIO number in the name and the corresponding sysfs
> attribute group for each exported line that lives under the new chip
> class device as well as a way to allow to compile out the legacy parts
> leaving only the new elements of the sysfs ABI.
> 
> This series passes the compatibility tests I wrote while working on the
> user-space compatibility layer for sysfs[2].

It seems I never expressed my overall opinion about this. I think the poking
sysfs and making it working with a new schema won't solve the issues that
character device was developed to target. If so, doing this just brings yet
another broken interface. I would be happy to be mistaken!

If I am mistaken, I would like to see a summary here that explains that clearly
that the new sysfs approach does not inherit design flaws of the original
implementation.

> [1] https://lore.kernel.org/all/CAMRc=McUCeZcU6co1aN54rTudo+JfPjjForu4iKQ5npwXk6GXA@mail.gmail.com/
> [2] https://github.com/brgl/gpio-sysfs-compat-tests

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Kent Gibson]

On Tue, Jul 01, 2025 at 05:05:10PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 02:37:07PM +0200, Bartosz Golaszewski wrote:
> > Following our discussion[1], here's a proposal for extending the sysfs
> > interface with attributes not referring to GPIO lines by their global
> > numbers in a backward compatible way.
> >
> > Long story short: there is now a new class device for each GPIO chip.
> > It's called chipX where X is the ID of the device as per the driver
> > model and it lives next to the old gpiochipABC where ABC is the GPIO
> > base. Each new chip class device has a pair of export/unexport
> > attributes which work similarly to the global ones under /sys/class/gpio
> > but take hardware offsets within the chip as input, instead of the
> > global numbers. Finally, each exported line appears at the same time as
> > the global /sys/class/gpio/gpioABC as well as per-chip
> > /sys/class/gpio/chipX/gpioY sysfs group.
> >
> > The series contains the implementation of a parallel GPIO chip entry not
> > containing the base GPIO number in the name and the corresponding sysfs
> > attribute group for each exported line that lives under the new chip
> > class device as well as a way to allow to compile out the legacy parts
> > leaving only the new elements of the sysfs ABI.
> >
> > This series passes the compatibility tests I wrote while working on the
> > user-space compatibility layer for sysfs[2].
>
> It seems I never expressed my overall opinion about this. I think the poking
> sysfs and making it working with a new schema won't solve the issues that
> character device was developed to target. If so, doing this just brings yet
> another broken interface. I would be happy to be mistaken!
>
> If I am mistaken, I would like to see a summary here that explains that clearly
> that the new sysfs approach does not inherit design flaws of the original
> implementation.
>

Indeed.  I've already expressed my reservations about supporting the whole
of the existing sysfs capabilties, but I've otherwise tried to remain out
of the discussion.

To reiterate my position:
While I am all for maintaining sysfs in some form to cater for those
rare cases where cdev is too heavyweight, IMHO it is a mistake to
support the existing sysfs capabilities in toto.  Take the opportunity to
remove the parts of the sysfs interface that don't work well.
The new sysfs should only provide the features required by those rare use
cases, which IIUC would be basic sets and gets, and exclude those features
not required, particularly warts like edges.

If you need more advanced features then use cdev.
If all you need is basic sets and gets then sysfs is probably fine.

If that isn't the case then there should be some explanation as to why those
sysfs features are being maintained.  Treat this as a new interface.

Cheers,
Kent.

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Bartosz Golaszewski]

On Wed, Jul 2, 2025 at 5:54 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Jul 01, 2025 at 05:05:10PM +0300, Andy Shevchenko wrote:
> >
> > It seems I never expressed my overall opinion about this. I think the poking
> > sysfs and making it working with a new schema won't solve the issues that
> > character device was developed to target. If so, doing this just brings yet
> > another broken interface. I would be happy to be mistaken!
> >
> > If I am mistaken, I would like to see a summary here that explains that clearly
> > that the new sysfs approach does not inherit design flaws of the original
> > implementation.
> >

You cut out the link to the discussion that preceded this series where
a good summary is in the very first email. Anyway: the gist is: people
need to do some basic GPIO fiddling early on from initramfs that may
not have any tools other than basic shell utils from busybox. This
series is not about improving or extending the sysfs interface - it's
about removing its reliance on global GPIO numbers. And that's about
it. We don't add any new features really, just move the GPIO line
groups into their respective chip directories and make exporting use
the hardware offsets, not global numbers.

>
> Indeed.  I've already expressed my reservations about supporting the whole
> of the existing sysfs capabilties, but I've otherwise tried to remain out
> of the discussion.
>
> To reiterate my position:
> While I am all for maintaining sysfs in some form to cater for those
> rare cases where cdev is too heavyweight, IMHO it is a mistake to
> support the existing sysfs capabilities in toto.  Take the opportunity to
> remove the parts of the sysfs interface that don't work well.

Doesn't the last patch do it? We cannot remove it without giving
user-space some time to switch. This series does everything in a
backward compatible way and then isolates the old bits under ifdefs so
that when the time comes it's just a matter of removing everything
guarded by them.

> The new sysfs should only provide the features required by those rare use
> cases, which IIUC would be basic sets and gets, and exclude those features
> not required, particularly warts like edges.
>
> If you need more advanced features then use cdev.
> If all you need is basic sets and gets then sysfs is probably fine.
>
> If that isn't the case then there should be some explanation as to why those
> sysfs features are being maintained.  Treat this as a new interface.
>

I tend to not interpret it as adding new features. We really just
*move* what exists under a slightly different path when you think
about it.

So what are you suggesting, remove the `edge` attribute and polling
features from the new `value` attribute?

Bart

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Kent Gibson]

On Wed, Jul 02, 2025 at 11:45:02AM +0200, Bartosz Golaszewski wrote:
> On Wed, Jul 2, 2025 at 5:54 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, Jul 01, 2025 at 05:05:10PM +0300, Andy Shevchenko wrote:
> > >
> > > It seems I never expressed my overall opinion about this. I think the poking
> > > sysfs and making it working with a new schema won't solve the issues that
> > > character device was developed to target. If so, doing this just brings yet
> > > another broken interface. I would be happy to be mistaken!
> > >
> > > If I am mistaken, I would like to see a summary here that explains that clearly
> > > that the new sysfs approach does not inherit design flaws of the original
> > > implementation.
> > >
>
> You cut out the link to the discussion that preceded this series where
> a good summary is in the very first email. Anyway: the gist is: people
> need to do some basic GPIO fiddling early on from initramfs that may
> not have any tools other than basic shell utils from busybox. This
> series is not about improving or extending the sysfs interface - it's
> about removing its reliance on global GPIO numbers. And that's about
> it. We don't add any new features really, just move the GPIO line
> groups into their respective chip directories and make exporting use
> the hardware offsets, not global numbers.
>

And that is the problem I have with it - it is just removing the global
numbering, while keeping all the other sysfs baggage.
Instead I think it should be thought of as adding a new minimal sysfs
alternative to cdev, based on the old sysfs.

> >
> > Indeed.  I've already expressed my reservations about supporting the whole
> > of the existing sysfs capabilties, but I've otherwise tried to remain out
> > of the discussion.
> >
> > To reiterate my position:
> > While I am all for maintaining sysfs in some form to cater for those
> > rare cases where cdev is too heavyweight, IMHO it is a mistake to
> > support the existing sysfs capabilities in toto.  Take the opportunity to
> > remove the parts of the sysfs interface that don't work well.
>
> Doesn't the last patch do it? We cannot remove it without giving
> user-space some time to switch. This series does everything in a
> backward compatible way and then isolates the old bits under ifdefs so
> that when the time comes it's just a matter of removing everything
> guarded by them.
>

Not suggesting any changes to the existing sysfs here, only your new.

> > The new sysfs should only provide the features required by those rare use
> > cases, which IIUC would be basic sets and gets, and exclude those features
> > not required, particularly warts like edges.
> >
> > If you need more advanced features then use cdev.
> > If all you need is basic sets and gets then sysfs is probably fine.
> >
> > If that isn't the case then there should be some explanation as to why those
> > sysfs features are being maintained.  Treat this as a new interface.
> >
>
> I tend to not interpret it as adding new features. We really just
> *move* what exists under a slightly different path when you think
> about it.
>
> So what are you suggesting, remove the `edge` attribute and polling
> features from the new `value` attribute?
>

Exactly. I'm not suggesting ANY changes to the old sysfs, only your new
non-global numbering version.  The idea being don't port everything over
from the old sysfs - just the core feature set that non-cdev users need.

Cheers,
Kent.

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Bartosz Golaszewski]

On Wed, Jul 2, 2025 at 12:12 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> >
> > I tend to not interpret it as adding new features. We really just
> > *move* what exists under a slightly different path when you think
> > about it.
> >
> > So what are you suggesting, remove the `edge` attribute and polling
> > features from the new `value` attribute?
> >
>
> Exactly. I'm not suggesting ANY changes to the old sysfs, only your new
> non-global numbering version.  The idea being don't port everything over
> from the old sysfs - just the core feature set that non-cdev users need.
>

I mean, if someone shows up saying they need this or that from the old
sysfs and without they won't switch, we can always add it back I
guess... Much easier than removing something that's carved in stone.

Anything else should go away? `active_low`?

Bart

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Kent Gibson]

On Wed, Jul 02, 2025 at 12:28:01PM +0200, Bartosz Golaszewski wrote:
> On Wed, Jul 2, 2025 at 12:12 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > >
> > > I tend to not interpret it as adding new features. We really just
> > > *move* what exists under a slightly different path when you think
> > > about it.
> > >
> > > So what are you suggesting, remove the `edge` attribute and polling
> > > features from the new `value` attribute?
> > >
> >
> > Exactly. I'm not suggesting ANY changes to the old sysfs, only your new
> > non-global numbering version.  The idea being don't port everything over
> > from the old sysfs - just the core feature set that non-cdev users need.
> >
>
> I mean, if someone shows up saying they need this or that from the old
> sysfs and without they won't switch, we can always add it back I
> guess... Much easier than removing something that's carved in stone.
>

Exactly - expect to be supporting whatever goes in now forever.

> Anything else should go away? `active_low`?
>

I don't personally see any value in 'active_low' in the sysfs API if you
drop edges. It is easy enough to flip values as necessary in userspace.
(From time to time I think it should've been dropped from cdev in v2 but, as
above, it is carved in stone now so oh well...)

Cheers,
Kent.

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 07:01:27PM +0800, Kent Gibson wrote:
> On Wed, Jul 02, 2025 at 12:28:01PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Jul 2, 2025 at 12:12 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > > I tend to not interpret it as adding new features. We really just
> > > > *move* what exists under a slightly different path when you think
> > > > about it.
> > > >
> > > > So what are you suggesting, remove the `edge` attribute and polling
> > > > features from the new `value` attribute?
> > >
> > > Exactly. I'm not suggesting ANY changes to the old sysfs, only your new
> > > non-global numbering version.  The idea being don't port everything over
> > > from the old sysfs - just the core feature set that non-cdev users need.
> >
> > I mean, if someone shows up saying they need this or that from the old
> > sysfs and without they won't switch, we can always add it back I
> > guess... Much easier than removing something that's carved in stone.
> 
> Exactly - expect to be supporting whatever goes in now forever.

+1, this is my biggest worries about the interfaces proposed by this series.

> > Anything else should go away? `active_low`?
> 
> I don't personally see any value in 'active_low' in the sysfs API if you
> drop edges. It is easy enough to flip values as necessary in userspace.
> (From time to time I think it should've been dropped from cdev in v2 but, as
> above, it is carved in stone now so oh well...)

But in cdev case this is different. Active-low state is needed to be
HW independent. For sysfs I agree as it's _already_ HW *dependent*
(due to global number space in use at bare minumum).


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Kent Gibson]

On Wed, Jul 02, 2025 at 02:59:43PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 07:01:27PM +0800, Kent Gibson wrote:
> > On Wed, Jul 02, 2025 at 12:28:01PM +0200, Bartosz Golaszewski wrote:
> > > On Wed, Jul 2, 2025 at 12:12 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > > I tend to not interpret it as adding new features. We really just
> > > > > *move* what exists under a slightly different path when you think
> > > > > about it.
> > > > >
> > > > > So what are you suggesting, remove the `edge` attribute and polling
> > > > > features from the new `value` attribute?
> > > >
> > > > Exactly. I'm not suggesting ANY changes to the old sysfs, only your new
> > > > non-global numbering version.  The idea being don't port everything over
> > > > from the old sysfs - just the core feature set that non-cdev users need.
> > >
> > > I mean, if someone shows up saying they need this or that from the old
> > > sysfs and without they won't switch, we can always add it back I
> > > guess... Much easier than removing something that's carved in stone.
> >
> > Exactly - expect to be supporting whatever goes in now forever.
>
> +1, this is my biggest worries about the interfaces proposed by this series.
>
> > > Anything else should go away? `active_low`?
> >
> > I don't personally see any value in 'active_low' in the sysfs API if you
> > drop edges. It is easy enough to flip values as necessary in userspace.
> > (From time to time I think it should've been dropped from cdev in v2 but, as
> > above, it is carved in stone now so oh well...)
>
> But in cdev case this is different. Active-low state is needed to be
> HW independent. For sysfs I agree as it's _already_ HW *dependent*
> (due to global number space in use at bare minumum).
>

As Geert pointed out, userspace needs to set the 'active_low' itself, so
it isn't really providing hardware independence.

The issue isn't whether the logical/physical mapping should exist, but
whether there is any benefit having it baked into the API.
It could've been added in userspace instead and made the uAPI a tad simpler
with one less source of confusion.

Cheers,
Kent.

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Geert Uytterhoeven]

On Wed, 2 Jul 2025 at 13:01, Kent Gibson <warthog618@gmail.com> wrote:
> On Wed, Jul 02, 2025 at 12:28:01PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Jul 2, 2025 at 12:12 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > >
> > > > I tend to not interpret it as adding new features. We really just
> > > > *move* what exists under a slightly different path when you think
> > > > about it.
> > > >
> > > > So what are you suggesting, remove the `edge` attribute and polling
> > > > features from the new `value` attribute?
> > > >
> > >
> > > Exactly. I'm not suggesting ANY changes to the old sysfs, only your new
> > > non-global numbering version.  The idea being don't port everything over
> > > from the old sysfs - just the core feature set that non-cdev users need.
> > >
> >
> > I mean, if someone shows up saying they need this or that from the old
> > sysfs and without they won't switch, we can always add it back I
> > guess... Much easier than removing something that's carved in stone.
>
> Exactly - expect to be supporting whatever goes in now forever.
>
> > Anything else should go away? `active_low`?
> >
>
> I don't personally see any value in 'active_low' in the sysfs API if you
> drop edges. It is easy enough to flip values as necessary in userspace.
> (From time to time I think it should've been dropped from cdev in v2 but, as
> above, it is carved in stone now so oh well...)

IMHO active_low is only really useful if you have some hardware
description that provides it, at which point you may be better off
having a real Linux
driver for the thing connected to the GPIO...

People who mess with GPIO /sysfs better know their hardware,
so they should be aware of the polarity.

From my toolbox:
  - For pcf857x (which is pseudo-bi-directional), I usually just set
    direction to "in" (pulled high) or "out" (driven low),
  - For everything else, switching direction to "out" is ill-defined,
    so I do not write to ".../value", but set direction and value
    together by writing "high" or "low" to ".../direction",
  - For reading, I do use ".../value", of course.

My 0.02€...

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 11:45:02AM +0200, Bartosz Golaszewski wrote:
> On Wed, Jul 2, 2025 at 5:54 AM Kent Gibson <warthog618@gmail.com> wrote:
> > On Tue, Jul 01, 2025 at 05:05:10PM +0300, Andy Shevchenko wrote:
> > > It seems I never expressed my overall opinion about this. I think the poking
> > > sysfs and making it working with a new schema won't solve the issues that
> > > character device was developed to target. If so, doing this just brings yet
> > > another broken interface. I would be happy to be mistaken!
> > >
> > > If I am mistaken, I would like to see a summary here that explains that clearly
> > > that the new sysfs approach does not inherit design flaws of the original
> > > implementation.
> 
> You cut out the link to the discussion that preceded this series where
> a good summary is in the very first email. Anyway: the gist is: people
> need to do some basic GPIO fiddling early on from initramfs that may
> not have any tools other than basic shell utils from busybox. This
> series is not about improving or extending the sysfs interface - it's
> about removing its reliance on global GPIO numbers. And that's about
> it. We don't add any new features really, just move the GPIO line
> groups into their respective chip directories and make exporting use
> the hardware offsets, not global numbers.

I see it differently. This adds the second variant of how sysfs can be handled
and it needs to be rotten in the same way as the original sysfs. I really don't
see a point to prolonging the life of the broken interface in such a way. If somebody
wants to check the GPIO without accessing character device interface, they probably
are simply lazy to think of how to do that on early stages properly. The desire
sounds like a workaround against proper thinking.

> > Indeed.  I've already expressed my reservations about supporting the whole
> > of the existing sysfs capabilties, but I've otherwise tried to remain out
> > of the discussion.
> >
> > To reiterate my position:
> > While I am all for maintaining sysfs in some form to cater for those
> > rare cases where cdev is too heavyweight, IMHO it is a mistake to
> > support the existing sysfs capabilities in toto.  Take the opportunity to
> > remove the parts of the sysfs interface that don't work well.
> 
> Doesn't the last patch do it? We cannot remove it without giving
> user-space some time to switch.

Famous Last Words. How many years the sysfs is being rotten?! This just makes
it a Frankenstein.

> This series does everything in a
> backward compatible way and then isolates the old bits under ifdefs so
> that when the time comes it's just a matter of removing everything
> guarded by them.
> 
> > The new sysfs should only provide the features required by those rare use
> > cases, which IIUC would be basic sets and gets, and exclude those features
> > not required, particularly warts like edges.
> >
> > If you need more advanced features then use cdev.
> > If all you need is basic sets and gets then sysfs is probably fine.
> >
> > If that isn't the case then there should be some explanation as to why those
> > sysfs features are being maintained.  Treat this as a new interface.
> 
> I tend to not interpret it as adding new features. We really just
> *move* what exists under a slightly different path when you think
> about it.
> 
> So what are you suggesting, remove the `edge` attribute and polling
> features from the new `value` attribute?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Bartosz Golaszewski]

On Wed, Jul 2, 2025 at 1:55 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Jul 02, 2025 at 11:45:02AM +0200, Bartosz Golaszewski wrote:
> > On Wed, Jul 2, 2025 at 5:54 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > On Tue, Jul 01, 2025 at 05:05:10PM +0300, Andy Shevchenko wrote:
> > > > It seems I never expressed my overall opinion about this. I think the poking
> > > > sysfs and making it working with a new schema won't solve the issues that
> > > > character device was developed to target. If so, doing this just brings yet
> > > > another broken interface. I would be happy to be mistaken!
> > > >
> > > > If I am mistaken, I would like to see a summary here that explains that clearly
> > > > that the new sysfs approach does not inherit design flaws of the original
> > > > implementation.
> >
> > You cut out the link to the discussion that preceded this series where
> > a good summary is in the very first email. Anyway: the gist is: people
> > need to do some basic GPIO fiddling early on from initramfs that may
> > not have any tools other than basic shell utils from busybox. This
> > series is not about improving or extending the sysfs interface - it's
> > about removing its reliance on global GPIO numbers. And that's about
> > it. We don't add any new features really, just move the GPIO line
> > groups into their respective chip directories and make exporting use
> > the hardware offsets, not global numbers.
>
> I see it differently. This adds the second variant of how sysfs can be handled
> and it needs to be rotten in the same way as the original sysfs. I really don't
> see a point to prolonging the life of the broken interface in such a way. If somebody
> wants to check the GPIO without accessing character device interface, they probably
> are simply lazy to think of how to do that on early stages properly. The desire
> sounds like a workaround against proper thinking.
>

Whatever your opinion on this is - if user-space wants to keep the
interface, then we need to support it. We can only propose
alternatives and hope the users will switch. Please read the
discussion, it explains why people want to keep using the simple sysfs
ABI and why those specific users will most likely never switch to the
character device. At this point a bigger concern to me is the global
GPIO numberspace, not the existence of the sysfs class as such.

We have three alternatives:

1. Do nothing. Keep the sysfs as is and hope we'll drop it eventually
but this will most likely never happen.
2. Add an entirely new "simple" interface next to the existing sysfs
AND character device. It seems to be what you're proposing but this is
the worst of two worlds. I don't want to be in charge of maintaining
three separate interfaces. Four if you count cdev v1.
3. Modify the existing sysfs in a backward compatible way but make it
possible to export lines by their HW offset within their parent chip,
not by global numbers. This is what this series does and what people
having interest in using sysfs confirmed would work for them.
Eventually we could drop the bits that use the global numberspace
which seems more palatable for user-space than entirely removing the
sysfs GPIO class.

> > > Indeed.  I've already expressed my reservations about supporting the whole
> > > of the existing sysfs capabilties, but I've otherwise tried to remain out
> > > of the discussion.
> > >
> > > To reiterate my position:
> > > While I am all for maintaining sysfs in some form to cater for those
> > > rare cases where cdev is too heavyweight, IMHO it is a mistake to
> > > support the existing sysfs capabilities in toto.  Take the opportunity to
> > > remove the parts of the sysfs interface that don't work well.
> >
> > Doesn't the last patch do it? We cannot remove it without giving
> > user-space some time to switch.
>
> Famous Last Words. How many years the sysfs is being rotten?! This just makes
> it a Frankenstein.
>

I'm doing it BECAUSE sysfs hasn't gone anywhere despite several years
of efforts. I cannot force user-space to stop using it, I can only
propose alternatives and it seems that cdev has not been enough.

Bart

Re: [PATCH v3 00/10] gpio: sysfs: add a per-chip export/unexport attribute pair

[Linus Walleij]

On Wed, Jul 2, 2025 at 3:19 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Whatever your opinion on this is - if user-space wants to keep the
> interface, then we need to support it. We can only propose
> alternatives and hope the users will switch. Please read the
> discussion, it explains why people want to keep using the simple sysfs
> ABI and why those specific users will most likely never switch to the
> character device. At this point a bigger concern to me is the global
> GPIO numberspace, not the existence of the sysfs class as such.

I agree with Bart here, the global GPIO numberspace is the big
problem we need to get rid of, if we move users over to the new
sysfs ABI and disable and finally delete the old one, we can get
rid of the global GPIO numberspace.

This will be needed to conclude the work inside the kernel do
move everyone and their dog over to using GPIO descriptors
instead of numbers.

We would maybe be able to delete <linux/gpio.h> but without
also getting rid of the global numberspace it will feel very
incomplete.

Yours,
Linus Walleij