[PATCH v1 0/3] gpiolib: Avpid modifying GPIO chip fwnode

[PATCH v1 0/3] gpiolib: Avpid modifying GPIO chip fwnode

[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.

Benjamin, would be nice it you can perform regression test for your
case.

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 any downsides
and regressions found, if any.

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(-)

-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

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

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

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

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 1436cdb5fa26..5fde5a3f5118 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -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));
 }
-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

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

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

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 97496c0f9133..cafbf9597ead 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -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 ACPI_HANDLE(&gc->gpiodev->dev) == data;
 }
 
 /**
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 3/3] gpiolib: Do not alter GPIO chip fwnode member

[Andy Shevchenko]

Ideally we should not touch data in the given GPIO chip structure.
Let's become closer to it by avoid 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.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bc8b9d6afe0e..8b7032300039 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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) {
-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

On Mon, Jul 03, 2023 at 03:18:37PM +0300, Andy Shevchenko wrote:

...

>  static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  {
> -	return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
> +	return ACPI_HANDLE(&gc->gpiodev->dev) == data;
>  }

And after all this may be changed to device_match_acpi_handle().
I'll update it locally, but still wait for the Benjamin to test
this before sending a v2.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] gpiolib: Avpid modifying GPIO chip fwnode

[Benjamin Tissoires]

On Mon, Jul 3, 2023 at 2:18 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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.
>
> Benjamin, would be nice it you can perform regression test for your
> case.


FWIW:
Tested-by: Benjamin Tissoires <bentiss@kernel.org>

I've tested Danny's series + my SSDT override, with and without your
series on top of the master of hid.git (v6.4+merge of the hid.git tree
for v6.5-rc1), and in both cases, I can access the I2C-HID node that
sits on top of the CP2112 USB adapter.

Cheers,
Benjamin


>
>
> 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 any downsides
> and regressions found, if any.
>
> 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(-)
>
> --
> 2.40.0.1.gaa8946217a0b
>

Re: [PATCH v1 0/3] gpiolib: Avpid modifying GPIO chip fwnode

[Andy Shevchenko]

On Mon, Jul 03, 2023 at 04:06:38PM +0200, Benjamin Tissoires wrote:
> On Mon, Jul 3, 2023 at 2:18 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > 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.
> >
> > Benjamin, would be nice it you can perform regression test for your
> > case.

> 
> FWIW:
> Tested-by: Benjamin Tissoires <bentiss@kernel.org>
> 
> I've tested Danny's series + my SSDT override, with and without your
> series on top of the master of hid.git (v6.4+merge of the hid.git tree
> for v6.5-rc1), and in both cases, I can access the I2C-HID node that
> sits on top of the CP2112 USB adapter.

Thank you very much!

I will issue a v2 either today or this week. So Bart will have time to review
that and robots to test more before v6.5-rc1 is out.


> > 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 any downsides
> > and regressions found, if any.

-- 
With Best Regards,
Andy Shevchenko