Skip to contentSkip to conversation

gpiolib: Avoid modifying GPIO chip fwnode

PATCH Applied Andy Shevchenko 4 participants 8 messages Jul 3, 2023 – Jul 10, 2023

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v2 cover gpiolib: Avoid modifying GPIO chip fwnode

Ideally the GPIO chip data structure has to be constant. In real life it's not true, but we can make it closer to that. Hence the series.

Bart, the idea is that this series has to land immediately after v6.5-rc1 is out so we will have longer time to fix downsides and regressions found, if any.

Benjamin, thank you for testing!

Changelog v2:
- replaced open coded device_match_acpi_handle()
- corrected commit message due to above change
- added tag (Benjamin)

Andy Shevchenko (3):
gpiolib: of: Don't use GPIO chip fwnode in of_gpiochip_*()
gpiolib: acpi: Don't use GPIO chip fwnode in acpi_gpiochip_find()
gpiolib: Do not alter GPIO chip fwnode member

 drivers/gpio/gpiolib-acpi.c |  2 +-
 drivers/gpio/gpiolib-of.c   |  6 +++---
 drivers/gpio/gpiolib.c      | 16 ++++++++--------
 3 files changed, 12 insertions(+), 12 deletions(-)
Signature · 1 line
2.40.0.1.gaa8946217a0b
#1 lore raw

PATCH v2 1/3 gpiolib: of: Don't use GPIO chip fwnode in of_gpiochip_*()

GPIO library should rely only on the GPIO device's fwnode. Hence, replace GPIO chip fwnode usage by respective OF node of the GPIO device.

JFYI, this is partial revert of 8afe82550240 ("gpiolib: of:
Prepare of_gpiochip_add() / of_gpiochip_remove() for fwnode").

---
 drivers/gpio/gpiolib-of.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpio/gpiolib-of.c +3 -3
index 1436cdb5fa26..5fde5a3f5118 100644
@@ -1078,16 +1078,16 @@ int of_gpiochip_add(struct gpio_chip *chip)
 	if (ret)
 		return ret;
 
-	fwnode_handle_get(chip->fwnode);
+	of_node_get(np);
 
 	ret = of_gpiochip_scan_gpios(chip);
 	if (ret)
-		fwnode_handle_put(chip->fwnode);
+		of_node_put(np);
 
 	return ret;
 }
 
 void of_gpiochip_remove(struct gpio_chip *chip)
 {
-	fwnode_handle_put(chip->fwnode);
+	of_node_put(dev_of_node(&chip->gpiodev->dev));
 }
Signature · 1 line
2.40.0.1.gaa8946217a0b
#2 lore raw

PATCH v2 2/3 gpiolib: acpi: Don't use GPIO chip fwnode in acpi_gpiochip_find()

In reply to Andy Shevchenko

GPIO library should rely only on the GPIO device's fwnode. Hence, replace GPIO chip fwnode usage by respective handle of the GPIO device.

---
 drivers/gpio/gpiolib-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib-acpi.c +1 -1
index 97496c0f9133..fbda452fb4d6 100644
@@ -128,7 +128,7 @@ static bool acpi_gpio_deferred_req_irqs_done;
 
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
-	return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
+	return device_match_acpi_handle(&gc->gpiodev->dev, data);
 }
 
 /**
Signature · 1 line
2.40.0.1.gaa8946217a0b
#3 lore raw

Re: [PATCH v2 2/3] gpiolib: acpi: Don't use GPIO chip fwnode in acpi_gpiochip_find()

Quoted 16 lines · Andy Shevchenko

GPIO library should rely only on the GPIO device's fwnode. Hence, replace GPIO chip fwnode usage by respective handle of the GPIO device.

---
 drivers/gpio/gpiolib-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib-acpi.c +0 -0
index 97496c0f9133..fbda452fb4d6 100644

drivers/gpio/gpiolib-acpi.c static bool acpi_gpio_deferred_req_irqs_done; · line 131

Quoted 5 lines
drivers/gpio/gpiolib-acpi.c +1 -1
@@ -128,7 +128,7 @@ static bool acpi_gpio_deferred_req_irqs_done;
 
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
-       return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
+       return device_match_acpi_handle(&gc->gpiodev->dev, data);

Quick confirmation that using "device_match_acpi_handle" also works in v2 (was easy enough to test given that everything was already set up locally).

Cheers,
Benjamin

Quoted 6 lines
drivers/gpio/gpiolib-acpi.c +0 -1
 }
 
 /**
--

2.40.0.1.gaa8946217a0b

#4 lore raw

Re: [PATCH v2 2/3] gpiolib: acpi: Don't use GPIO chip fwnode in acpi_gpiochip_find()

Quoted 9 lines · Benjamin Tissoires

drivers/gpio/gpiolib-acpi.c static bool acpi_gpio_deferred_req_irqs_done; · line 131

Quoted 2 lines · Andy Shevchenko
drivers/gpio/gpiolib-acpi.c +1 -1
-       return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
+       return device_match_acpi_handle(&gc->gpiodev->dev, data);

Quick confirmation that using "device_match_acpi_handle" also works in v2 (was easy enough to test given that everything was already set up locally).

Thank you! I would be very surprised if not :-)

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#5 lore raw

PATCH v2 3/3 gpiolib: Do not alter GPIO chip fwnode member

In reply to Andy Shevchenko

Ideally we should not touch data in the given GPIO chip structure. Let's become closer to it by avoiding altering fwnode member.

The GPIO library must use fwnode from GPIO device and the drivers might use one from GPIO chip in case they initialized it.

---
 drivers/gpio/gpiolib.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/gpio/gpiolib.c +8 -8
index bc8b9d6afe0e..8b7032300039 100644
@@ -708,13 +708,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	int base = 0;
 	int ret = 0;
 
-	/*
-	 * If the calling driver did not initialize firmware node, do it here
-	 * using the parent device, if any.
-	 */
-	if (!gc->fwnode && gc->parent)
-		gc->fwnode = dev_fwnode(gc->parent);
-
 	/*
 	 * First: allocate and populate the internal stat container, and
 	 * set up the struct device.
@@ -729,7 +722,14 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	gc->gpiodev = gdev;
 	gpiochip_set_data(gc, data);
 
-	device_set_node(&gdev->dev, gc->fwnode);
+	/*
+	 * If the calling driver did not initialize firmware node,
+	 * do it here using the parent device, if any.
+	 */
+	if (gc->fwnode)
+		device_set_node(&gdev->dev, gc->fwnode);
+	else if (gc->parent)
+		device_set_node(&gdev->dev, dev_fwnode(gc->parent));
 
 	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
 	if (gdev->id < 0) {
Signature · 1 line
2.40.0.1.gaa8946217a0b
#7 lore raw
In reply to Andy Shevchenko
Quoted 21 lines · Andy Shevchenko

Ideally the GPIO chip data structure has to be constant. In real life it's not true, but we can make it closer to that. Hence the series.

Bart, the idea is that this series has to land immediately after v6.5-rc1 is out so we will have longer time to fix downsides and regressions found, if any.

Benjamin, thank you for testing!

Changelog v2:
- replaced open coded device_match_acpi_handle()
- corrected commit message due to above change
- added tag (Benjamin)

Andy Shevchenko (3):
gpiolib: of: Don't use GPIO chip fwnode in of_gpiochip_*()
gpiolib: acpi: Don't use GPIO chip fwnode in acpi_gpiochip_find()
gpiolib: Do not alter GPIO chip fwnode member

Quoted 8 lines
 drivers/gpio/gpiolib-acpi.c |  2 +-
 drivers/gpio/gpiolib-of.c   |  6 +++---
 drivers/gpio/gpiolib.c      | 16 ++++++++--------
 3 files changed, 12 insertions(+), 12 deletions(-)

--
2.40.0.1.gaa8946217a0b

Applied, thanks!

Bart