[PATCH v2 0/9] gpio: sysfs: add a per-chip export/unexport attribute pair

[PATCH v2 0/9] 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 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 (9):
      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: don't use driver data in sysfs callbacks for line attributes
      gpio: sysfs: rename the data variable in gpiod_(un)export()
      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          | 502 +++++++++++++++++++++++++---------
 4 files changed, 388 insertions(+), 147 deletions(-)
---
base-commit: cb908f3699fb137e28017a8fdf506c35762b3eb6
change-id: 20250402-gpio-sysfs-chip-export-84ac424b61c5

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

[PATCH v2 1/9] 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          | 191 +++++++++++++++++++++++++---------
 2 files changed, 149 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 c4c21e25c682b939e4a0517393308343feb6585a..fbe93cda4ca16038a1cffe766f7e5ead55ace5e6 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -46,6 +46,7 @@ struct gpiod_data {
 struct gpiodev_data {
 	struct gpio_device *gdev;
 	struct device *cdev_base; /* Class device by GPIO base */
+	struct device *cdev_id; /* Class device by GPIO device ID */
 };
 
 /*
@@ -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,16 @@ 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);
+		kfree(data);
+		return PTR_ERR(data->cdev_id);
+	}
+
 	return 0;
 }
 
@@ -802,6 +896,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 v2 2/9] 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.

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 fbe93cda4ca16038a1cffe766f7e5ead55ace5e6..c812c58e171448501f3d67e6287d32fcac00797d 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

[PATCH v2 3/9] gpio: sysfs: pass gpiod_data directly to internal GPIO sysfs functions

[Bartosz Golaszewski]

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

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

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 c812c58e171448501f3d67e6287d32fcac00797d..0e2c6b2d0940b1d4e4ad0a90aa172e7d01908969 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 v2 4/9] 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.

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 0e2c6b2d0940b1d4e4ad0a90aa172e7d01908969..2f1df2ceb7360200c718ea95089720ebfa5a513a 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
@@ -664,6 +687,7 @@ gdev_get_data(struct gpio_device *gdev) __must_hold(&sysfs_lock)
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
 	struct gpio_device *gdev;
+	struct attribute **attrs;
 	struct gpiod_data *data;
 	struct device *dev;
 	int status;
@@ -709,8 +733,26 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	else
 		data->direction_can_change = false;
 
+	gpiod_attr_init(&data->dir_attr, "direction",
+			direction_show, direction_store);
+	gpiod_attr_init(&data->val_attr, "value", value_show, value_store);
+	gpiod_attr_init(&data->edge_attr, "edge", edge_show, edge_store);
+	gpiod_attr_init(&data->active_low_attr, "active_low",
+			active_low_show, active_low_store);
+
+	attrs = data->attrs;
+	data->attr_group.is_visible = gpio_is_visible;
+	attrs[GPIO_SYSFS_LINE_ATTR_DIRECTION] = &data->dir_attr.attr;
+	attrs[GPIO_SYSFS_LINE_ATTR_VALUE] = &data->val_attr.attr;
+	attrs[GPIO_SYSFS_LINE_ATTR_EDGE] = &data->edge_attr.attr;
+	attrs[GPIO_SYSFS_LINE_ATTR_ACTIVE_LOW] =
+						&data->active_low_attr.attr;
+
+	data->attr_group.attrs = data->attrs;
+	data->attr_groups[0] = &data->attr_group;
+
 	dev = device_create_with_groups(&gpio_class, &gdev->dev,
-					MKDEV(0, 0), data, gpio_groups,
+					MKDEV(0, 0), data, data->attr_groups,
 					"gpio%u", desc_to_gpio(desc));
 	if (IS_ERR(dev)) {
 		status = PTR_ERR(dev);

-- 
2.48.1

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

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

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 2f1df2ceb7360200c718ea95089720ebfa5a513a..515fd0d307cf820b036b1ea966b300715992359f 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -686,9 +686,9 @@ 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 gpiod_data *desc_data;
 	struct gpio_device *gdev;
 	struct attribute **attrs;
-	struct gpiod_data *data;
 	struct device *dev;
 	int status;
 
@@ -720,47 +720,48 @@ 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;
 
-	gpiod_attr_init(&data->dir_attr, "direction",
+	gpiod_attr_init(&desc_data->dir_attr, "direction",
 			direction_show, direction_store);
-	gpiod_attr_init(&data->val_attr, "value", value_show, value_store);
-	gpiod_attr_init(&data->edge_attr, "edge", edge_show, edge_store);
-	gpiod_attr_init(&data->active_low_attr, "active_low",
-			active_low_show, active_low_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 = data->attrs;
-	data->attr_group.is_visible = gpio_is_visible;
-	attrs[GPIO_SYSFS_LINE_ATTR_DIRECTION] = &data->dir_attr.attr;
-	attrs[GPIO_SYSFS_LINE_ATTR_VALUE] = &data->val_attr.attr;
-	attrs[GPIO_SYSFS_LINE_ATTR_EDGE] = &data->edge_attr.attr;
+	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] =
-						&data->active_low_attr.attr;
+				&desc_data->active_low_attr.attr;
 
-	data->attr_group.attrs = data->attrs;
-	data->attr_groups[0] = &data->attr_group;
+	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), data, data->attr_groups,
+					MKDEV(0, 0), desc_data,
+					desc_data->attr_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;
 	}
@@ -770,7 +771,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);
@@ -827,7 +828,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) {
@@ -843,22 +844,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 v2 6/9] 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.

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 515fd0d307cf820b036b1ea966b300715992359f..adf030f74eb163f5d8b1092d00418b84354f923f 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -43,6 +43,7 @@ enum {
 
 struct gpiod_data {
 	struct gpio_desc *desc;
+	struct device *dev;
 
 	struct mutex mutex;
 	struct kernfs_node *value_class_node;
@@ -58,12 +59,15 @@ struct gpiod_data {
 	struct attribute *attrs[GPIO_SYSFS_LINE_ATTR_SIZE];
 	struct attribute_group attr_group;
 	const struct attribute_group *attr_groups[2];
+
+	struct list_head list;
 };
 
 struct gpiodev_data {
 	struct gpio_device *gdev;
 	struct device *cdev_base; /* Class device by GPIO base */
 	struct device *cdev_id; /* Class device by GPIO device ID */
+	struct list_head exported_lines;
 };
 
 /*
@@ -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 ... */
@@ -751,25 +755,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:
@@ -828,8 +847,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__);
@@ -840,14 +860,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 (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
@@ -857,7 +885,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);
 }
@@ -899,6 +926,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 v2 7/9] 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 adf030f74eb163f5d8b1092d00418b84354f923f..37d58009a51333f7d6a8d600dbeaeb333df27ac3 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -47,11 +47,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;
 }
@@ -780,13 +783,46 @@ 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;
+
+	char *path __free(kfree) = 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:
@@ -876,6 +912,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

[PATCH v2 8/9] 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.

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 37d58009a51333f7d6a8d600dbeaeb333df27ac3..0e7605fee3bd9edbe4e90d8e466742df11a8d579 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -46,7 +46,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;
@@ -67,7 +69,9 @@ struct gpiod_data {
 
 struct gpiodev_data {
 	struct gpio_device *gdev;
+#if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
 	struct device *cdev_base; /* Class device by GPIO base */
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 	struct device *cdev_id; /* Class device by GPIO device ID */
 	struct list_head exported_lines;
 };
@@ -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) {
@@ -822,10 +838,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);
@@ -834,12 +852,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 data->desc == desc;
 }
+#endif /* CONFIG_GPIO_SYSFS_LEGACY */
 
 /**
  * gpiod_export_link - create a sysfs link to an exported GPIO node
@@ -856,6 +876,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;
 
@@ -872,6 +893,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);
 
@@ -910,8 +934,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);
 
@@ -968,6 +994,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,
@@ -979,13 +1006,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 */
 		kfree(data);
 		return PTR_ERR(data->cdev_id);
 	}
@@ -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 v2 9/9] 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 v2 7/9] gpio: sysfs: export the GPIO directory locally in the gpiochip<id> directory

[kernel test robot]

Hi Bartosz,

kernel test robot noticed the following build errors:

[auto build test ERROR on cb908f3699fb137e28017a8fdf506c35762b3eb6]

url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-sysfs-add-a-parallel-class-device-for-each-GPIO-chip-using-device-IDs/20250623-170412
base:   cb908f3699fb137e28017a8fdf506c35762b3eb6
patch link:    https://lore.kernel.org/r/20250623-gpio-sysfs-chip-export-v2-7-d592793f8964%40linaro.org
patch subject: [PATCH v2 7/9] gpio: sysfs: export the GPIO directory locally in the gpiochip<id> directory
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20250624/202506240548.l2wS2XW6-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250624/202506240548.l2wS2XW6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506240548.l2wS2XW6-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpio/gpiolib-sysfs.c:802:3: error: cannot jump from this goto statement to its label
     802 |                 goto err_free_name;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:804:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     804 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:795:3: error: cannot jump from this goto statement to its label
     795 |                 goto err_put_dirent;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:804:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     804 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:786:3: error: cannot jump from this goto statement to its label
     786 |                 goto err_put_dirent;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:804:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     804 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:780:3: error: cannot jump from this goto statement to its label
     780 |                 goto err_unregister_device;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:804:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     804 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:773:3: error: cannot jump from this goto statement to its label
     773 |                 goto err_free_data;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:804:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     804 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:733:3: error: cannot jump from this goto statement to its label
     733 |                 goto err_clear_bit;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:804:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     804 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:727:3: error: cannot jump from this goto statement to its label
     727 |                 goto err_clear_bit;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:804:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     804 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   7 errors generated.


vim +802 drivers/gpio/gpiolib-sysfs.c

   677	
   678	/**
   679	 * gpiod_export - export a GPIO through sysfs
   680	 * @desc: GPIO to make available, already requested
   681	 * @direction_may_change: true if userspace may change GPIO direction
   682	 * Context: arch_initcall or later
   683	 *
   684	 * When drivers want to make a GPIO accessible to userspace after they
   685	 * have requested it -- perhaps while debugging, or as part of their
   686	 * public interface -- they may use this routine.  If the GPIO can
   687	 * change direction (some can't) and the caller allows it, userspace
   688	 * will see "direction" sysfs attribute which may be used to change
   689	 * the gpio's direction.  A "value" attribute will always be provided.
   690	 *
   691	 * Returns:
   692	 * 0 on success, or negative errno on failure.
   693	 */
   694	int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
   695	{
   696		struct gpiodev_data *gdev_data;
   697		struct gpiod_data *desc_data;
   698		struct gpio_device *gdev;
   699		struct attribute **attrs;
   700		int status;
   701	
   702		/* can't export until sysfs is available ... */
   703		if (!class_is_registered(&gpio_class)) {
   704			pr_debug("%s: called too early!\n", __func__);
   705			return -ENOENT;
   706		}
   707	
   708		if (!desc) {
   709			pr_debug("%s: invalid gpio descriptor\n", __func__);
   710			return -EINVAL;
   711		}
   712	
   713		CLASS(gpio_chip_guard, guard)(desc);
   714		if (!guard.gc)
   715			return -ENODEV;
   716	
   717		if (test_and_set_bit(FLAG_EXPORT, &desc->flags))
   718			return -EPERM;
   719	
   720		gdev = desc->gdev;
   721	
   722		guard(mutex)(&sysfs_lock);
   723	
   724		if (!test_bit(FLAG_REQUESTED, &desc->flags)) {
   725			gpiod_dbg(desc, "%s: unavailable (not requested)\n", __func__);
   726			status = -EPERM;
   727			goto err_clear_bit;
   728		}
   729	
   730		desc_data = kzalloc(sizeof(*desc_data), GFP_KERNEL);
   731		if (!desc_data) {
   732			status = -ENOMEM;
   733			goto err_clear_bit;
   734		}
   735	
   736		desc_data->desc = desc;
   737		mutex_init(&desc_data->mutex);
   738		if (guard.gc->direction_input && guard.gc->direction_output)
   739			desc_data->direction_can_change = direction_may_change;
   740		else
   741			desc_data->direction_can_change = false;
   742	
   743		gpiod_attr_init(&desc_data->dir_attr, "direction",
   744				direction_show, direction_store);
   745		gpiod_attr_init(&desc_data->val_attr, "value", value_show, value_store);
   746		gpiod_attr_init(&desc_data->edge_attr, "edge", edge_show, edge_store);
   747		gpiod_attr_init(&desc_data->active_low_attr, "active_low",
   748					active_low_show, active_low_store);
   749	
   750		attrs = desc_data->attrs;
   751		desc_data->attr_group.is_visible = gpio_is_visible;
   752		attrs[GPIO_SYSFS_LINE_ATTR_DIRECTION] = &desc_data->dir_attr.attr;
   753		attrs[GPIO_SYSFS_LINE_ATTR_VALUE] = &desc_data->val_attr.attr;
   754		attrs[GPIO_SYSFS_LINE_ATTR_EDGE] = &desc_data->edge_attr.attr;
   755		attrs[GPIO_SYSFS_LINE_ATTR_ACTIVE_LOW] =
   756					&desc_data->active_low_attr.attr;
   757	
   758		desc_data->attr_group.attrs = desc_data->attrs;
   759		desc_data->attr_groups[0] = &desc_data->attr_group;
   760	
   761		/*
   762		 * Note: we need to continue passing desc_data here as there's still
   763		 * at least one known user of gpiod_export_link() in the tree. This
   764		 * function still uses class_find_device() internally.
   765		 */
   766		desc_data->dev = device_create_with_groups(&gpio_class, &gdev->dev,
   767							   MKDEV(0, 0), desc_data,
   768							   desc_data->attr_groups,
   769							   "gpio%u",
   770							   desc_to_gpio(desc));
   771		if (IS_ERR(desc_data->dev)) {
   772			status = PTR_ERR(desc_data->dev);
   773			goto err_free_data;
   774		}
   775	
   776		desc_data->value_class_node = sysfs_get_dirent(desc_data->dev->kobj.sd,
   777							       "value");
   778		if (!desc_data->value_class_node) {
   779			status = -ENODEV;
   780			goto err_unregister_device;
   781		}
   782	
   783		gdev_data = gdev_get_data(gdev);
   784		if (!gdev_data) {
   785			status = -ENODEV;
   786			goto err_put_dirent;
   787		}
   788	
   789		list_add(&desc_data->list, &gdev_data->exported_lines);
   790	
   791		desc_data->attr_group.name = kasprintf(GFP_KERNEL, "gpio%u",
   792						       gpio_chip_hwgpio(desc));
   793		if (!desc_data->attr_group.name) {
   794			status = -ENOMEM;
   795			goto err_put_dirent;
   796		}
   797	
   798		desc_data->parent = &gdev_data->cdev_id->kobj;
   799		status = sysfs_create_groups(desc_data->parent,
   800					     desc_data->attr_groups);
   801		if (status)
 > 802			goto err_free_name;
   803	
   804		char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
   805						     gpio_chip_hwgpio(desc));
   806		if (!path) {
   807			status = -ENOMEM;
   808			goto err_remove_groups;
   809		}
   810	
   811		desc_data->value_chip_node = kernfs_walk_and_get(desc_data->parent->sd,
   812								 path);
   813		if (!desc_data->value_chip_node) {
   814			status = -ENODEV;
   815			goto err_remove_groups;
   816		}
   817	
   818		return 0;
   819	
   820	err_remove_groups:
   821		sysfs_remove_groups(desc_data->parent, desc_data->attr_groups);
   822	err_free_name:
   823		kfree(desc_data->attr_group.name);
   824	err_put_dirent:
   825		sysfs_put(desc_data->value_class_node);
   826	err_unregister_device:
   827		device_unregister(desc_data->dev);
   828	err_free_data:
   829		kfree(desc_data);
   830	err_clear_bit:
   831		clear_bit(FLAG_EXPORT, &desc->flags);
   832		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
   833		return status;
   834	}
   835	EXPORT_SYMBOL_GPL(gpiod_export);
   836	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 8/9] gpio: sysfs: allow disabling the legacy parts of the GPIO sysfs interface

[Geert Uytterhoeven]

Hi Bartosz,

On Mon, 23 Jun 2025 at 11:01, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 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.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Thanks for your patch!

> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c

> @@ -968,6 +994,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,
> @@ -979,13 +1006,16 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
>                 kfree(data);
>                 return err;
>         }
> +#endif /* CONFIG_GPIO_SYSFS_LEGACY */

"err" is always declared, but only used in this block.
Hence if CONFIG_GPIO_SYSFS_LEGACY=n:

    drivers/gpio/gpiolib-sysfs.c: In function ‘gpiochip_sysfs_register’:
    drivers/gpio/gpiolib-sysfs.c:962:13: error: unused variable ‘err’
[-Werror=unused-variable]
      962 |         int err;
          |             ^~~

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v2 3/9] gpio: sysfs: pass gpiod_data directly to internal GPIO sysfs functions

[Linus Walleij]

On Mon, Jun 23, 2025 at 11:00 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> To make the transition to not using dev_get_drvdata() across line
> callbacks for sysfs attributes, pass gpiod_data directly to
> gpio_sysfs_request_irq(), gpio_sysfs_free_irq() and
> gpio_sysfs_set_active_low() instead of having it wrapped in struct
> device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

Re: [PATCH v2 4/9] gpio: sysfs: don't use driver data in sysfs callbacks for line attributes

[Linus Walleij]

On Mon, Jun 23, 2025 at 11:00 AM Bartosz Golaszewski <brgl@bgdev.pl> 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.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

Re: [PATCH v2 5/9] gpio: sysfs: rename the data variable in gpiod_(un)export()

[Linus Walleij]

On Mon, Jun 23, 2025 at 11:00 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> 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.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

Re: [PATCH v2 6/9] gpio: sysfs: don't look up exported lines as class devices

[Linus Walleij]

On Mon, Jun 23, 2025 at 11:00 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> 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.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

Re: [PATCH v2 8/9] gpio: sysfs: allow disabling the legacy parts of the GPIO sysfs interface

[Linus Walleij]

On Mon, Jun 23, 2025 at 11:00 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> 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.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

Re: [PATCH v2 8/9] gpio: sysfs: allow disabling the legacy parts of the GPIO sysfs interface

[kernel test robot]

Hi Bartosz,

kernel test robot noticed the following build warnings:

[auto build test WARNING on cb908f3699fb137e28017a8fdf506c35762b3eb6]

url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-sysfs-add-a-parallel-class-device-for-each-GPIO-chip-using-device-IDs/20250623-170412
base:   cb908f3699fb137e28017a8fdf506c35762b3eb6
patch link:    https://lore.kernel.org/r/20250623-gpio-sysfs-chip-export-v2-8-d592793f8964%40linaro.org
patch subject: [PATCH v2 8/9] gpio: sysfs: allow disabling the legacy parts of the GPIO sysfs interface
config: s390-randconfig-002-20250627 (https://download.01.org/0day-ci/archive/20250627/202506271940.T2QaCrba-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project e04c938cc08a90ae60440ce22d072ebc69d67ee8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250627/202506271940.T2QaCrba-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506271940.T2QaCrba-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpio/gpiolib-sysfs.c:818:3: error: cannot jump from this goto statement to its label
     818 |                 goto err_free_name;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:820:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     820 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:811:3: error: cannot jump from this goto statement to its label
     811 |                 goto err_put_dirent;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:820:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     820 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:802:3: error: cannot jump from this goto statement to its label
     802 |                 goto err_put_dirent;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:820:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     820 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:747:3: error: cannot jump from this goto statement to its label
     747 |                 goto err_clear_bit;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:820:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     820 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
   drivers/gpio/gpiolib-sysfs.c:741:3: error: cannot jump from this goto statement to its label
     741 |                 goto err_clear_bit;
         |                 ^
   drivers/gpio/gpiolib-sysfs.c:820:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
     820 |         char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
         |               ^
>> drivers/gpio/gpiolib-sysfs.c:962:6: warning: unused variable 'err' [-Wunused-variable]
     962 |         int err;
         |             ^~~
   1 warning and 5 errors generated.


vim +/err +962 drivers/gpio/gpiolib-sysfs.c

65fbd24108991d Bartosz Golaszewski 2025-06-23  798  
574194e59098d9 Bartosz Golaszewski 2025-06-23  799  	gdev_data = gdev_get_data(gdev);
574194e59098d9 Bartosz Golaszewski 2025-06-23  800  	if (!gdev_data) {
574194e59098d9 Bartosz Golaszewski 2025-06-23  801  		status = -ENODEV;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  802  		goto err_put_dirent;
574194e59098d9 Bartosz Golaszewski 2025-06-23  803  	}
574194e59098d9 Bartosz Golaszewski 2025-06-23  804  
574194e59098d9 Bartosz Golaszewski 2025-06-23  805  	list_add(&desc_data->list, &gdev_data->exported_lines);
574194e59098d9 Bartosz Golaszewski 2025-06-23  806  
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  807  	desc_data->attr_group.name = kasprintf(GFP_KERNEL, "gpio%u",
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  808  					       gpio_chip_hwgpio(desc));
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  809  	if (!desc_data->attr_group.name) {
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  810  		status = -ENOMEM;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  811  		goto err_put_dirent;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  812  	}
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  813  
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  814  	desc_data->parent = &gdev_data->cdev_id->kobj;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  815  	status = sysfs_create_groups(desc_data->parent,
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  816  				     desc_data->attr_groups);
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  817  	if (status)
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  818  		goto err_free_name;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  819  
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23 @820  	char *path __free(kfree) = kasprintf(GFP_KERNEL, "gpio%u/value",
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  821  					     gpio_chip_hwgpio(desc));
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  822  	if (!path) {
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  823  		status = -ENOMEM;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  824  		goto err_remove_groups;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  825  	}
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  826  
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  827  	desc_data->value_chip_node = kernfs_walk_and_get(desc_data->parent->sd,
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  828  							 path);
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  829  	if (!desc_data->value_chip_node) {
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  830  		status = -ENODEV;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  831  		goto err_remove_groups;
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  832  	}
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  833  
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  834  	return 0;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  835  
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  836  err_remove_groups:
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  837  	sysfs_remove_groups(desc_data->parent, desc_data->attr_groups);
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  838  err_free_name:
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  839  	kfree(desc_data->attr_group.name);
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  840  err_put_dirent:
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  841  #if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  842  	sysfs_put(desc_data->value_class_node);
65fbd24108991d Bartosz Golaszewski 2025-06-23  843  err_unregister_device:
574194e59098d9 Bartosz Golaszewski 2025-06-23  844  	device_unregister(desc_data->dev);
c43960fbcc514b Johan Hovold        2015-05-04  845  err_free_data:
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  846  #endif /* CONFIG_GPIO_SYSFS_LEGACY */
d54627a4c4f9a8 Bartosz Golaszewski 2025-06-23  847  	kfree(desc_data);
f4af1671c28854 Bartosz Golaszewski 2024-10-31  848  err_clear_bit:
35b545332b809a Bartosz Golaszewski 2024-01-12  849  	clear_bit(FLAG_EXPORT, &desc->flags);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  850  	gpiod_dbg(desc, "%s: status %d\n", __func__, status);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  851  	return status;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  852  }
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  853  EXPORT_SYMBOL_GPL(gpiod_export);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  854  
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  855  #if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
c43960fbcc514b Johan Hovold        2015-05-04  856  static int match_export(struct device *dev, const void *desc)
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  857  {
c43960fbcc514b Johan Hovold        2015-05-04  858  	struct gpiod_data *data = dev_get_drvdata(dev);
c43960fbcc514b Johan Hovold        2015-05-04  859  
c43960fbcc514b Johan Hovold        2015-05-04  860  	return data->desc == desc;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  861  }
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  862  #endif /* CONFIG_GPIO_SYSFS_LEGACY */
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  863  
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  864  /**
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  865   * gpiod_export_link - create a sysfs link to an exported GPIO node
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  866   * @dev: device under which to create symlink
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  867   * @name: name of the symlink
2d9d05192e7d1a Thierry Reding      2017-07-24  868   * @desc: GPIO to create symlink to, already exported
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  869   *
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  870   * Set up a symlink from /sys/.../dev/name to /sys/class/gpio/gpioN
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  871   * node. Caller is responsible for unlinking.
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  872   *
94bd9ce16063a2 Andy Shevchenko     2024-08-28  873   * Returns:
94bd9ce16063a2 Andy Shevchenko     2024-08-28  874   * 0 on success, or negative errno on failure.
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  875   */
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  876  int gpiod_export_link(struct device *dev, const char *name,
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  877  		      struct gpio_desc *desc)
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  878  {
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  879  #if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
56d30ec14c1330 Johan Hovold        2015-05-04  880  	struct device *cdev;
56d30ec14c1330 Johan Hovold        2015-05-04  881  	int ret;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  882  
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  883  	if (!desc) {
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  884  		pr_warn("%s: invalid GPIO\n", __func__);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  885  		return -EINVAL;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  886  	}
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  887  
56d30ec14c1330 Johan Hovold        2015-05-04  888  	cdev = class_find_device(&gpio_class, NULL, desc, match_export);
56d30ec14c1330 Johan Hovold        2015-05-04  889  	if (!cdev)
56d30ec14c1330 Johan Hovold        2015-05-04  890  		return -ENODEV;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  891  
56d30ec14c1330 Johan Hovold        2015-05-04  892  	ret = sysfs_create_link(&dev->kobj, &cdev->kobj, name);
56d30ec14c1330 Johan Hovold        2015-05-04  893  	put_device(cdev);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  894  
56d30ec14c1330 Johan Hovold        2015-05-04  895  	return ret;
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  896  #else
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  897  	return -EOPNOTSUPP;
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  898  #endif /* CONFIG_GPIO_SYSFS_LEGACY */
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  899  }
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  900  EXPORT_SYMBOL_GPL(gpiod_export_link);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  901  
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  902  /**
31963eb039b7d5 Amitesh Singh       2016-09-08  903   * gpiod_unexport - reverse effect of gpiod_export()
2d9d05192e7d1a Thierry Reding      2017-07-24  904   * @desc: GPIO to make unavailable
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  905   *
31963eb039b7d5 Amitesh Singh       2016-09-08  906   * This is implicit on gpiod_free().
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  907   */
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  908  void gpiod_unexport(struct gpio_desc *desc)
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  909  {
574194e59098d9 Bartosz Golaszewski 2025-06-23  910  	struct gpiod_data *desc_data = NULL;
574194e59098d9 Bartosz Golaszewski 2025-06-23  911  	struct gpiodev_data *gdev_data;
574194e59098d9 Bartosz Golaszewski 2025-06-23  912  	struct gpio_device *gdev;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  913  
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  914  	if (!desc) {
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  915  		pr_warn("%s: invalid GPIO\n", __func__);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  916  		return;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  917  	}
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  918  
f4af1671c28854 Bartosz Golaszewski 2024-10-31  919  	scoped_guard(mutex, &sysfs_lock) {
72eba6f66a0017 Johan Hovold        2015-05-04  920  		if (!test_bit(FLAG_EXPORT, &desc->flags))
f4af1671c28854 Bartosz Golaszewski 2024-10-31  921  			return;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  922  
574194e59098d9 Bartosz Golaszewski 2025-06-23  923  		gdev = gpiod_to_gpio_device(desc);
574194e59098d9 Bartosz Golaszewski 2025-06-23  924  		gdev_data = gdev_get_data(gdev);
574194e59098d9 Bartosz Golaszewski 2025-06-23  925  		if (!gdev_data)
574194e59098d9 Bartosz Golaszewski 2025-06-23  926  			return;
574194e59098d9 Bartosz Golaszewski 2025-06-23  927  
574194e59098d9 Bartosz Golaszewski 2025-06-23  928  		list_for_each_entry(desc_data, &gdev_data->exported_lines, list)
574194e59098d9 Bartosz Golaszewski 2025-06-23  929  			if (desc == desc_data->desc)
574194e59098d9 Bartosz Golaszewski 2025-06-23  930  				break;
574194e59098d9 Bartosz Golaszewski 2025-06-23  931  
574194e59098d9 Bartosz Golaszewski 2025-06-23  932  		if (!desc_data)
f4af1671c28854 Bartosz Golaszewski 2024-10-31  933  			return;
72eba6f66a0017 Johan Hovold        2015-05-04  934  
574194e59098d9 Bartosz Golaszewski 2025-06-23  935  		list_del(&desc_data->list);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  936  		clear_bit(FLAG_EXPORT, &desc->flags);
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  937  #if IS_ENABLED(CONFIG_GPIO_SYSFS_LEGACY)
d54627a4c4f9a8 Bartosz Golaszewski 2025-06-23  938  		sysfs_put(desc_data->value_class_node);
574194e59098d9 Bartosz Golaszewski 2025-06-23  939  		device_unregister(desc_data->dev);
d6fe296d76af3f Bartosz Golaszewski 2025-06-23  940  #endif /* CONFIG_GPIO_SYSFS_LEGACY */
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  941  		sysfs_remove_groups(desc_data->parent, desc_data->attr_groups);
d65a6b43cccca4 Bartosz Golaszewski 2025-06-23  942  		kernfs_put(desc_data->value_chip_node);
72eba6f66a0017 Johan Hovold        2015-05-04  943  
54d9acd7540995 Johan Hovold        2015-05-04  944  		/*
f4af1671c28854 Bartosz Golaszewski 2024-10-31  945  		 * Release irq after deregistration to prevent race with
f4af1671c28854 Bartosz Golaszewski 2024-10-31  946  		 * edge_store.
54d9acd7540995 Johan Hovold        2015-05-04  947  		 */
d54627a4c4f9a8 Bartosz Golaszewski 2025-06-23  948  		if (desc_data->irq_flags)
d54627a4c4f9a8 Bartosz Golaszewski 2025-06-23  949  			gpio_sysfs_free_irq(desc_data);
f4af1671c28854 Bartosz Golaszewski 2024-10-31  950  	}
72eba6f66a0017 Johan Hovold        2015-05-04  951  
d54627a4c4f9a8 Bartosz Golaszewski 2025-06-23  952  	mutex_destroy(&desc_data->mutex);
d54627a4c4f9a8 Bartosz Golaszewski 2025-06-23  953  	kfree(desc_data);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  954  }
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  955  EXPORT_SYMBOL_GPL(gpiod_unexport);
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  956  
afbc4f312b5e6e Linus Walleij       2016-02-09  957  int gpiochip_sysfs_register(struct gpio_device *gdev)
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  958  {
fd19792851db77 Bartosz Golaszewski 2025-06-10  959  	struct gpiodev_data *data;
d83cee3d2bb131 Bartosz Golaszewski 2024-01-23  960  	struct gpio_chip *chip;
513246a34b8dc5 Bartosz Golaszewski 2023-12-21  961  	struct device *parent;
e6bb78570f7d53 Antonio Quartulli   2025-06-23 @962  	int err;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  963  
426577bd8846d6 Johan Hovold        2015-05-04  964  	/*
426577bd8846d6 Johan Hovold        2015-05-04  965  	 * Many systems add gpio chips for SOC support very early,
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  966  	 * before driver model support is available.  In those cases we
426577bd8846d6 Johan Hovold        2015-05-04  967  	 * register later, in gpiolib_sysfs_init() ... here we just
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  968  	 * verify that _some_ field of gpio_class got initialized.
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  969  	 */
6f14c02220c791 Greg Kroah-Hartman  2023-03-31  970  	if (!class_is_registered(&gpio_class))
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  971  		return 0;
0eb4c6c2671ca0 Alexandre Courbot   2014-07-01  972  
d83cee3d2bb131 Bartosz Golaszewski 2024-01-23  973  	guard(srcu)(&gdev->srcu);
d83cee3d2bb131 Bartosz Golaszewski 2024-01-23  974  
d82b9e0887e69d Bartosz Golaszewski 2024-02-14  975  	chip = srcu_dereference(gdev->chip, &gdev->srcu);
d83cee3d2bb131 Bartosz Golaszewski 2024-01-23  976  	if (!chip)
d83cee3d2bb131 Bartosz Golaszewski 2024-01-23  977  		return -ENODEV;
d83cee3d2bb131 Bartosz Golaszewski 2024-01-23  978  
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  979  	/*
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  980  	 * For sysfs backward compatibility we need to preserve this
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  981  	 * preferred parenting to the gpio_chip parent field, if set.
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  982  	 */
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  983  	if (chip->parent)
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  984  		parent = chip->parent;
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  985  	else
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  986  		parent = &gdev->dev;
d27c17285eb7eb Bamvor Jian Zhang   2016-02-24  987  
fd19792851db77 Bartosz Golaszewski 2025-06-10  988  	data = kmalloc(sizeof(*data), GFP_KERNEL);
fd19792851db77 Bartosz Golaszewski 2025-06-10  989  	if (!data)
fd19792851db77 Bartosz Golaszewski 2025-06-10  990  		return -ENOMEM;
fd19792851db77 Bartosz Golaszewski 2025-06-10  991  
fd19792851db77 Bartosz Golaszewski 2025-06-10  992  	data->gdev = gdev;
574194e59098d9 Bartosz Golaszewski 2025-06-23  993  	INIT_LIST_HEAD(&data->exported_lines);
3ff74be5c1a72d Johan Hovold        2015-05-04  994  
f4af1671c28854 Bartosz Golaszewski 2024-10-31  995  	guard(mutex)(&sysfs_lock);
fd19792851db77 Bartosz Golaszewski 2025-06-10  996  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/9] gpio: sysfs: add a parallel class device for each GPIO chip using device IDs

[Andy Shevchenko]

On Mon, Jun 23, 2025 at 10:59:49AM +0200, Bartosz Golaszewski wrote:
> 
> 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.

...

>  struct gpiodev_data {
>  	struct gpio_device *gdev;
>  	struct device *cdev_base; /* Class device by GPIO base */
> +	struct device *cdev_id; /* Class device by GPIO device ID */

I would add it in the middle in a way of the possible drop or conditional
compiling of the legacy access in the future.

>  };

...

> +static int export_gpio_desc(struct gpio_desc *desc)
> +{
> +	int offset, ret;

Why offset is signed?

> +	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));

Can we use gdev here? (IIRC we can't due to some legacy corner cases)

> +		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 struct device_attribute dev_attr_export = __ATTR(export, 0200, NULL,
> +							chip_export_store);

__ATTR_WO()

...

> +static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
> +							  NULL,
> +							  chip_unexport_store);

Ditto.

...

> +static struct attribute *gpiochip_ext_attrs[] = {
> +	&dev_attr_label.attr,
> +	&dev_attr_ngpio.attr,
> +	&dev_attr_export.attr,
> +	&dev_attr_unexport.attr,
> +	NULL,

No comma for the terminator, please.

> +};

...

> +	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);
> +		kfree(data);

UAF

> +		return PTR_ERR(data->cdev_id);
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] gpio: sysfs: only get the dirent reference for the value attr once

[Andy Shevchenko]

On Mon, Jun 23, 2025 at 10:59:50AM +0200, Bartosz Golaszewski wrote:
> 
> 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.

...

> -	struct kernfs_node *value_kn;
> +	struct kernfs_node *value_class_node;

This change is not mentioned in the commit message. Why?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/9] gpio: sysfs: pass gpiod_data directly to internal GPIO sysfs functions

[Andy Shevchenko]

On Mon, Jun 23, 2025 at 10:59:51AM +0200, Bartosz Golaszewski wrote:
> 
> To make the transition to not using dev_get_drvdata() across line
> callbacks for sysfs attributes, pass gpiod_data directly to
> gpio_sysfs_request_irq(), gpio_sysfs_free_irq() and
> gpio_sysfs_set_active_low() instead of having it wrapped in struct
> device.

I would improve the commit message by mentioning why using data over dev is
better. Something like "since we do not use anything from dev, pass the data
object pointer ...".


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/9] gpio: sysfs: don't use driver data in sysfs callbacks for line attributes

[Andy Shevchenko]

On Mon, Jun 23, 2025 at 10:59:52AM +0200, Bartosz Golaszewski wrote:
> 
> 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);

Defining once something like

#define to_gpiod_data() ...

we may leave this and others as one-liners.

...

> +	attrs[GPIO_SYSFS_LINE_ATTR_ACTIVE_LOW] =
> +						&data->active_low_attr.attr;

What's the point of two lines here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/9] gpio: sysfs: rename the data variable in gpiod_(un)export()

[Andy Shevchenko]

On Mon, Jun 23, 2025 at 10:59:53AM +0200, Bartosz Golaszewski wrote:
> 
> 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.

...

> +	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] =
> -						&data->active_low_attr.attr;
> +				&desc_data->active_low_attr.attr;

These were added in the previous patch and immediately got rewritten?!
Sounds like a wrong patch order.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/9] gpio: sysfs: don't look up exported lines as class devices

[Andy Shevchenko]

On Mon, Jun 23, 2025 at 10:59:54AM +0200, Bartosz Golaszewski wrote:
> 
> 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.

...

> +	struct list_head list;
>  };

If you make this to be the first member, you may make the container_of() (from
list_entry APIs) to be no-op. Have you checked with bloat-o-meter the difference?

...

> +	struct list_head exported_lines;
>  };

Ditto.


...

> -	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");

In such cases I find the following style to be slightly better to read.

	desc_data->value_class_node =
			sysfs_get_dirent(desc_data->dev->kobj.sd, "value");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/9] gpio: sysfs: add a parallel class device for each GPIO chip using device IDs

[Bartosz Golaszewski]

On Fri, Jun 27, 2025 at 5:21 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jun 23, 2025 at 10:59:49AM +0200, Bartosz Golaszewski wrote:
> >
> > 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.
>
> ...
>
> >  struct gpiodev_data {
> >       struct gpio_device *gdev;
> >       struct device *cdev_base; /* Class device by GPIO base */
> > +     struct device *cdev_id; /* Class device by GPIO device ID */
>
> I would add it in the middle in a way of the possible drop or conditional
> compiling of the legacy access in the future.
>

I'm not sure what difference it makes?

> >  };
>
> ...
>
> > +static int export_gpio_desc(struct gpio_desc *desc)
> > +{
> > +     int offset, ret;
>
> Why offset is signed?
>

Because gpio_chip_hwgpio() returns a signed int.

> > +     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));
>
> Can we use gdev here? (IIRC we can't due to some legacy corner cases)
>

Yeah, I think there was some revert here? In any case: it's material
for a different series, I'm just moving the code here.

> > +             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 struct device_attribute dev_attr_export = __ATTR(export, 0200, NULL,
> > +                                                     chip_export_store);
>
> __ATTR_WO()
>

No can do, the attribute would have to be called "chip_export". A
function called export_store() already exists in this file.

> ...
>
> > +static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
> > +                                                       NULL,
> > +                                                       chip_unexport_store);
>
> Ditto.
>
> ...
>
> > +static struct attribute *gpiochip_ext_attrs[] = {
> > +     &dev_attr_label.attr,
> > +     &dev_attr_ngpio.attr,
> > +     &dev_attr_export.attr,
> > +     &dev_attr_unexport.attr,
> > +     NULL,
>
> No comma for the terminator, please.
>

Ok.

> > +};
>
> ...
>
> > +     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);
> > +             kfree(data);
>
> UAF
>

Ok.

> > +             return PTR_ERR(data->cdev_id);
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thanks,
Bart

Re: [PATCH v2 2/9] gpio: sysfs: only get the dirent reference for the value attr once

[Bartosz Golaszewski]

On Fri, Jun 27, 2025 at 5:35 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jun 23, 2025 at 10:59:50AM +0200, Bartosz Golaszewski wrote:
> >
> > 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.
>
> ...
>
> > -     struct kernfs_node *value_kn;
> > +     struct kernfs_node *value_class_node;
>
> This change is not mentioned in the commit message. Why?
>

Yeah, I'll mention it.

Bart

Re: [PATCH v2 4/9] gpio: sysfs: don't use driver data in sysfs callbacks for line attributes

[Bartosz Golaszewski]

On Fri, Jun 27, 2025 at 5:41 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jun 23, 2025 at 10:59:52AM +0200, Bartosz Golaszewski wrote:
> >
> > 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);
>
> Defining once something like
>
> #define to_gpiod_data() ...
>
> we may leave this and others as one-liners.

We'd need one per every attribute. Look closer, we do get a different
attr address in every pair of callbacks.

>
> ...
>
> > +     attrs[GPIO_SYSFS_LINE_ATTR_ACTIVE_LOW] =
> > +                                             &data->active_low_attr.attr;
>
> What's the point of two lines here?
>

I tend to stick with the 80 chars limit even though it was lifted.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2 5/9] gpio: sysfs: rename the data variable in gpiod_(un)export()

[Bartosz Golaszewski]

On Fri, Jun 27, 2025 at 5:43 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jun 23, 2025 at 10:59:53AM +0200, Bartosz Golaszewski wrote:
> >
> > 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.
>
> ...
>
> > +     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] =
> > -                                             &data->active_low_attr.attr;
> > +                             &desc_data->active_low_attr.attr;
>
> These were added in the previous patch and immediately got rewritten?!
> Sounds like a wrong patch order.
>

Yeah, bad rebase. Thanks for catching it.

Bart

Re: [PATCH v2 5/9] gpio: sysfs: rename the data variable in gpiod_(un)export()

[Bartosz Golaszewski]

On Mon, Jun 30, 2025 at 10:57 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Jun 27, 2025 at 5:43 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Mon, Jun 23, 2025 at 10:59:53AM +0200, Bartosz Golaszewski wrote:
> > >
> > > 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.
> >
> > ...
> >
> > > +     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] =
> > > -                                             &data->active_low_attr.attr;
> > > +                             &desc_data->active_low_attr.attr;
> >
> > These were added in the previous patch and immediately got rewritten?!
> > Sounds like a wrong patch order.
> >
>
> Yeah, bad rebase. Thanks for catching it.
>
> Bart

Ah, no actually I got misled by the indentation difference. It was
supposed to be like this but maybe it is better to change places of
those two patches.

Bart

Re: [PATCH v2 1/9] gpio: sysfs: add a parallel class device for each GPIO chip using device IDs

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 10:34:52AM +0200, Bartosz Golaszewski wrote:
> On Fri, Jun 27, 2025 at 5:21 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Jun 23, 2025 at 10:59:49AM +0200, Bartosz Golaszewski wrote:

...

> > >  struct gpiodev_data {
> > >       struct gpio_device *gdev;
> > >       struct device *cdev_base; /* Class device by GPIO base */
> > > +     struct device *cdev_id; /* Class device by GPIO device ID */
> >
> > I would add it in the middle in a way of the possible drop or conditional
> > compiling of the legacy access in the future.
> 
> I'm not sure what difference it makes?

It collects optional (which you do with ifdeffery later on) at the end of the
structure. Maybe there is no effect now, but it might be in the future.

> > >  };

...

> > > +static struct device_attribute dev_attr_export = __ATTR(export, 0200, NULL,
> > > +                                                     chip_export_store);
> >
> > __ATTR_WO()
> >
> 
> No can do, the attribute would have to be called "chip_export". A
> function called export_store() already exists in this file.

I didn't get, we have two "export" nodes implemented in the same file?

...

> > > +static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
> > > +                                                       NULL,
> > > +                                                       chip_unexport_store);
> >
> > Ditto.

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/9] gpio: sysfs: don't use driver data in sysfs callbacks for line attributes

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 10:57:06AM +0200, Bartosz Golaszewski wrote:
> On Fri, Jun 27, 2025 at 5:41 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Jun 23, 2025 at 10:59:52AM +0200, Bartosz Golaszewski wrote:

...

> > Defining once something like
> >
> > #define to_gpiod_data() ...
> >
> > we may leave this and others as one-liners.
> 
> We'd need one per every attribute. Look closer, we do get a different
> attr address in every pair of callbacks.

I see, thanks for pointing that out.

...

> > > +     attrs[GPIO_SYSFS_LINE_ATTR_ACTIVE_LOW] =
> > > +                                             &data->active_low_attr.attr;
> >
> > What's the point of two lines here?
> >
> 
> I tend to stick with the 80 chars limit even though it was lifted.

The above looks just ugly. Saving one character on the line here doesn't
justify readability loss.


-- 
With Best Regards,
Andy Shevchenko