[PATCH 00/21] gpio: convert users to gpio_device_find() and remove gpiochip_find()

[PATCH 00/21] gpio: convert users to gpio_device_find() and remove gpiochip_find()

[Bartosz Golaszewski]

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

The GPIO subsystem does not handle hot-unplug events very well. We have
recently patched the user-space part of it so that at least a rouge user
cannot crash the kernel but in-kernel users are still affected by a lot of
issues: from incorrect locking or lack thereof to using structures that are
private to GPIO drivers. Since almost all GPIO controllers can be unbound,
not to mention that we have USB devices registering GPIO expanders as well as
I2C-on-USB HID devices on which I2C GPIO expanders can live, various media
gadgets etc., we really need to make GPIO hotplug/unplug friendly.

Before we can even get to fixing the locking, we need to address a serious
abuse of the GPIO driver API - accessing struct gpio_chip by anyone who isn't
the driver owning this object. This structure is owned by the GPIO provider
and its lifetime is tied to that of that provider. It is destroyed when the
device is unregistered and this may happen at any moment. struct gpio_device
is the opaque, reference counted interface to struct gpio_chip (which is the
low-level implementation) and all access should pass through it.

The end-goal is to make all gpio_device manipulators check the existence of
gdev->chip and then lock it for the duration of any of the calls using SRCU.
Before we can get there, we need to first provide a set of functions that will
replace any gpio_chip functions and convert all in-kernel users.

This series starts the process by replacing gpiochip_find() with
gpio_device_find(). This is in line with other device_find type interfaces and
returns a reference to the GPIO device that is guaranteed to remain valid
until it is put.

Note that this does not make everything correct just yet. Especially the
GPIOLIB internal users release the reference returned by the lookup function
after getting the descriptor of interest but before requesting it. This will
eventually be addressed. This is not a regression either.

First we add a bunch of new APIs that are needed to start replacing calls
to gpiochip_find. We then use them first in external users and then locally in
GPIOLIB core. Finally we remove gpiochip_find().

Bartosz Golaszewski (21):
  gpiolib: make gpio_device_get() and gpio_device_put() public
  gpiolib: provide gpio_device_find()
  gpiolib: provide gpio_device_find_by_label()
  gpiolib: provide gpio_device_get_desc()
  gpiolib: add support for scope-based management to gpio_device
  gpiolib: provide gpiod_to_device()
  gpiolib: provide gpio_device_get_base()
  gpio: acpi: provide acpi_gpio_device_free_interrupts()
  gpiolib: reluctantly provide gpio_device_get_chip()
  gpiolib: replace find_chip_by_name() with gpio_device_find_by_label()
  platform: x86: android-tablets: don't access GPIOLIB private members
  hte: allow building modules with COMPILE_TEST enabled
  hte: tegra194: improve the GPIO-related comment
  hte: tegra194: don't access struct gpio_chip
  arm: omap1: ams-delta: stop using gpiochip_find()
  gpio: of: correct notifier return codes
  gpio: of: replace gpiochip_find_* with gpio_device_find_*
  gpio: acpi: replace gpiochip_find() with gpio_device_find()
  gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()
  gpio: sysfs: drop the mention of gpiochip_find() from sysfs code
  gpiolib: remove gpiochip_find()

 arch/arm/mach-omap1/board-ams-delta.c         |  36 ++--
 drivers/gpio/gpiolib-acpi.c                   |  37 +++-
 drivers/gpio/gpiolib-of.c                     |  48 ++---
 drivers/gpio/gpiolib-swnode.c                 |  29 ++-
 drivers/gpio/gpiolib-sysfs.c                  |   2 +-
 drivers/gpio/gpiolib.c                        | 203 +++++++++++++-----
 drivers/gpio/gpiolib.h                        |  10 -
 drivers/hte/Kconfig                           |   4 +-
 drivers/hte/hte-tegra194.c                    |  49 +++--
 .../platform/x86/x86-android-tablets/core.c   |  38 ++--
 include/linux/gpio/driver.h                   |  30 ++-
 11 files changed, 316 insertions(+), 170 deletions(-)

-- 
2.39.2

[PATCH 01/21] gpiolib: make gpio_device_get() and gpio_device_put() public

[Bartosz Golaszewski]

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

In order to start migrating away from accessing struct gpio_chip by
users other than their owners, let's first make the reference management
functions for the opaque struct gpio_device public in the driver.h
header.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 24 ++++++++++++++++++++++++
 drivers/gpio/gpiolib.h      | 10 ----------
 include/linux/gpio/driver.h |  3 +++
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index edffa0d2acaa..f84ad54d8dbd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1058,6 +1058,30 @@ static struct gpio_chip *find_chip_by_name(const char *name)
 	return gpiochip_find((void *)name, gpiochip_match_name);
 }
 
+/**
+ * gpio_device_get() - Increase the reference count of this GPIO device
+ * @gdev: GPIO device to increase the refcount for
+ *
+ * Returns:
+ * Pointer to @gdev.
+ */
+struct gpio_device *gpio_device_get(struct gpio_device *gdev)
+{
+	return to_gpio_device(get_device(&gdev->dev));
+}
+EXPORT_SYMBOL_GPL(gpio_device_get);
+
+/**
+ * gpio_device_put() - Decrease the reference count of this GPIO device and
+ *                     possibly free all resources associated with it.
+ * @gdev: GPIO device to decrease the reference count for
+ */
+void gpio_device_put(struct gpio_device *gdev)
+{
+	put_device(&gdev->dev);
+}
+EXPORT_SYMBOL_GPL(gpio_device_put);
+
 #ifdef CONFIG_GPIOLIB_IRQCHIP
 
 /*
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 9bff5c2cf720..3ccacf3c1288 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -86,16 +86,6 @@ static inline struct gpio_device *to_gpio_device(struct device *dev)
 	return container_of(dev, struct gpio_device, dev);
 }
 
-static inline struct gpio_device *gpio_device_get(struct gpio_device *gdev)
-{
-	return to_gpio_device(get_device(&gdev->dev));
-}
-
-static inline void gpio_device_put(struct gpio_device *gdev)
-{
-	put_device(&gdev->dev);
-}
-
 /* gpio suffixes used for ACPI and device tree lookup */
 static __maybe_unused const char * const gpio_suffixes[] = { "gpios", "gpio" };
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 8f0859ba7065..a2060dc3344b 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -606,6 +606,9 @@ int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc,
 struct gpio_chip *gpiochip_find(void *data,
 				int (*match)(struct gpio_chip *gc, void *data));
 
+struct gpio_device *gpio_device_get(struct gpio_device *gdev);
+void gpio_device_put(struct gpio_device *gdev);
+
 bool gpiochip_line_is_irq(struct gpio_chip *gc, unsigned int offset);
 int gpiochip_reqres_irq(struct gpio_chip *gc, unsigned int offset);
 void gpiochip_relres_irq(struct gpio_chip *gc, unsigned int offset);
-- 
2.39.2

Re: [PATCH 01/21] gpiolib: make gpio_device_get() and gpio_device_put() public

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> In order to start migrating away from accessing struct gpio_chip by
> users other than their owners, let's first make the reference management
> functions for the opaque struct gpio_device public in the driver.h
> header.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

[PATCH 02/21] gpiolib: provide gpio_device_find()

[Bartosz Golaszewski]

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

gpiochip_find() is wrong and its kernel doc is misleading as the
function doesn't return a reference to the gpio_chip but just a raw
pointer. The chip itself is not guaranteed to stay alive, in fact it can
be deleted at any point. Also: other than GPIO drivers themselves,
nobody else has any business accessing gpio_chip structs.

Provide a new gpio_device_find() function that returns a real reference
to the opaque gpio_device structure that is guaranteed to stay alive for
as long as there are active users of it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 64 ++++++++++++++++++++++++++-----------
 include/linux/gpio/driver.h |  4 +++
 2 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f84ad54d8dbd..9e083ecb8df0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1014,16 +1014,10 @@ void gpiochip_remove(struct gpio_chip *gc)
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
 
-/**
- * gpiochip_find() - iterator for locating a specific gpio_chip
- * @data: data to pass to match function
- * @match: Callback function to check gpio_chip
+/*
+ * FIXME: This will be removed soon.
  *
- * Similar to bus_find_device.  It returns a reference to a gpio_chip as
- * determined by a user supplied @match callback.  The callback should return
- * 0 if the device doesn't match and non-zero if it does.  If the callback is
- * non-zero, this function will return to the caller and not iterate over any
- * more gpio_chips.
+ * This function is depracated, don't use.
  */
 struct gpio_chip *gpiochip_find(void *data,
 				int (*match)(struct gpio_chip *gc,
@@ -1031,21 +1025,55 @@ struct gpio_chip *gpiochip_find(void *data,
 {
 	struct gpio_device *gdev;
 	struct gpio_chip *gc = NULL;
-	unsigned long flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
-	list_for_each_entry(gdev, &gpio_devices, list)
-		if (gdev->chip && match(gdev->chip, data)) {
-			gc = gdev->chip;
-			break;
-		}
-
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	gdev = gpio_device_find(data, match);
+	if (gdev) {
+		gc = gdev->chip;
+		gpio_device_put(gdev);
+	}
 
 	return gc;
 }
 EXPORT_SYMBOL_GPL(gpiochip_find);
 
+/**
+ * gpio_device_find() - find a specific GPIO device
+ * @data: data to pass to match function
+ * @match: Callback function to check gpio_chip
+ *
+ * Returns:
+ * New reference to struct gpio_device.
+ *
+ * Similar to bus_find_device(). It returns a reference to a gpio_device as
+ * determined by a user supplied @match callback. The callback should return
+ * 0 if the device doesn't match and non-zero if it does. If the callback
+ * returns non-zero, this function will return to the caller and not iterate
+ * over any more gpio_devices.
+ *
+ * The callback takes the GPIO chip structure as argument. During the execution
+ * of the callback function the chip is protected from being freed. TODO: This
+ * actually has yet to be implemented.
+ *
+ * If the function returns non-NULL, the returned reference must be freed by
+ * the caller using gpio_device_put().
+ */
+struct gpio_device *gpio_device_find(void *data,
+				     int (*match)(struct gpio_chip *gc,
+						  void *data))
+{
+	struct gpio_device *gdev;
+
+	guard(spinlock_irqsave)(&gpio_lock);
+
+	list_for_each_entry(gdev, &gpio_devices, list) {
+		if (gdev->chip && match(gdev->chip, data))
+			return gpio_device_get(gdev);
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(gpio_device_find);
+
 static int gpiochip_match_name(struct gpio_chip *gc, void *data)
 {
 	const char *name = data;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a2060dc3344b..5c5029cec226 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -606,6 +606,10 @@ int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc,
 struct gpio_chip *gpiochip_find(void *data,
 				int (*match)(struct gpio_chip *gc, void *data));
 
+struct gpio_device *gpio_device_find(void *data,
+				     int (*match)(struct gpio_chip *gc,
+						  void *data));
+
 struct gpio_device *gpio_device_get(struct gpio_device *gdev);
 void gpio_device_put(struct gpio_device *gdev);
 
-- 
2.39.2

Re: [PATCH 02/21] gpiolib: provide gpio_device_find()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:52:50PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> gpiochip_find() is wrong and its kernel doc is misleading as the
> function doesn't return a reference to the gpio_chip but just a raw
> pointer. The chip itself is not guaranteed to stay alive, in fact it can
> be deleted at any point. Also: other than GPIO drivers themselves,
> nobody else has any business accessing gpio_chip structs.
> 
> Provide a new gpio_device_find() function that returns a real reference
> to the opaque gpio_device structure that is guaranteed to stay alive for
> as long as there are active users of it.

...

> +/**
> + * gpio_device_find() - find a specific GPIO device
> + * @data: data to pass to match function
> + * @match: Callback function to check gpio_chip

> + * Returns:
> + * New reference to struct gpio_device.

I believe this is wrong location of the Return section.
AFAIU how kernel doc uses section markers, this entire description becomes
a Return(s) section. Have you tried to render man/html/pdf and see this?

> + * Similar to bus_find_device(). It returns a reference to a gpio_device as
> + * determined by a user supplied @match callback. The callback should return
> + * 0 if the device doesn't match and non-zero if it does. If the callback
> + * returns non-zero, this function will return to the caller and not iterate
> + * over any more gpio_devices.
> + *
> + * The callback takes the GPIO chip structure as argument. During the execution
> + * of the callback function the chip is protected from being freed. TODO: This
> + * actually has yet to be implemented.
> + *
> + * If the function returns non-NULL, the returned reference must be freed by
> + * the caller using gpio_device_put().
> + */
> +struct gpio_device *gpio_device_find(void *data,

> +				     int (*match)(struct gpio_chip *gc,
> +						  void *data))

One line?
Or maybe a type for it? (gpio_match_fn, for example)

> +{
> +	struct gpio_device *gdev;
> +
> +	guard(spinlock_irqsave)(&gpio_lock);
> +
> +	list_for_each_entry(gdev, &gpio_devices, list) {
> +		if (gdev->chip && match(gdev->chip, data))
> +			return gpio_device_get(gdev);
> +	}
> +
> +	return NULL;
> +}

...

> +struct gpio_device *gpio_device_find(void *data,
> +				     int (*match)(struct gpio_chip *gc,
> +						  void *data));

Ditto.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/21] gpiolib: provide gpio_device_find()

[Bartosz Golaszewski]

On Wed, Sep 6, 2023 at 4:10 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 05, 2023 at 08:52:50PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > gpiochip_find() is wrong and its kernel doc is misleading as the
> > function doesn't return a reference to the gpio_chip but just a raw
> > pointer. The chip itself is not guaranteed to stay alive, in fact it can
> > be deleted at any point. Also: other than GPIO drivers themselves,
> > nobody else has any business accessing gpio_chip structs.
> >
> > Provide a new gpio_device_find() function that returns a real reference
> > to the opaque gpio_device structure that is guaranteed to stay alive for
> > as long as there are active users of it.
>
> ...
>
> > +/**
> > + * gpio_device_find() - find a specific GPIO device
> > + * @data: data to pass to match function
> > + * @match: Callback function to check gpio_chip
>
> > + * Returns:
> > + * New reference to struct gpio_device.
>
> I believe this is wrong location of the Return section.
> AFAIU how kernel doc uses section markers, this entire description becomes
> a Return(s) section. Have you tried to render man/html/pdf and see this?
>

Yes, it works just fine. Try for yourself: scripts/kernel-doc -rst
drivers/gpio/gpiolib.c

Bart

> > + * Similar to bus_find_device(). It returns a reference to a gpio_device as
> > + * determined by a user supplied @match callback. The callback should return
> > + * 0 if the device doesn't match and non-zero if it does. If the callback
> > + * returns non-zero, this function will return to the caller and not iterate
> > + * over any more gpio_devices.
> > + *
> > + * The callback takes the GPIO chip structure as argument. During the execution
> > + * of the callback function the chip is protected from being freed. TODO: This
> > + * actually has yet to be implemented.
> > + *
> > + * If the function returns non-NULL, the returned reference must be freed by
> > + * the caller using gpio_device_put().
> > + */
> > +struct gpio_device *gpio_device_find(void *data,
>
> > +                                  int (*match)(struct gpio_chip *gc,
> > +                                               void *data))
>
> One line?
> Or maybe a type for it? (gpio_match_fn, for example)
>
> > +{
> > +     struct gpio_device *gdev;
> > +
> > +     guard(spinlock_irqsave)(&gpio_lock);
> > +
> > +     list_for_each_entry(gdev, &gpio_devices, list) {
> > +             if (gdev->chip && match(gdev->chip, data))
> > +                     return gpio_device_get(gdev);
> > +     }
> > +
> > +     return NULL;
> > +}
>
> ...
>
> > +struct gpio_device *gpio_device_find(void *data,
> > +                                  int (*match)(struct gpio_chip *gc,
> > +                                               void *data));
>
> Ditto.
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 02/21] gpiolib: provide gpio_device_find()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> gpiochip_find() is wrong and its kernel doc is misleading as the
> function doesn't return a reference to the gpio_chip but just a raw
> pointer. The chip itself is not guaranteed to stay alive, in fact it can
> be deleted at any point. Also: other than GPIO drivers themselves,
> nobody else has any business accessing gpio_chip structs.
>
> Provide a new gpio_device_find() function that returns a real reference
> to the opaque gpio_device structure that is guaranteed to stay alive for
> as long as there are active users of it.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


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

Yours,
Linus Walleij

[PATCH 03/21] gpiolib: provide gpio_device_find_by_label()

[Bartosz Golaszewski]

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

By far the most common way of looking up GPIO devices is using their
label. Provide a helpers for that to avoid every user implementing their
own matching function.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 21 +++++++++++++++++++++
 include/linux/gpio/driver.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9e083ecb8df0..74b837671d30 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -20,6 +20,7 @@
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/string.h>
 
 #include <linux/gpio.h>
 #include <linux/gpio/driver.h>
@@ -1074,6 +1075,26 @@ struct gpio_device *gpio_device_find(void *data,
 }
 EXPORT_SYMBOL_GPL(gpio_device_find);
 
+static int gpio_chip_match_by_label(struct gpio_chip *gc, void *label)
+{
+	return gc->label && !strcmp(gc->label, label);
+}
+
+/**
+ * gpio_device_find_by_label() - wrapper around gpio_device_find() finding the
+ *                               GPIO device by its backing chip's label
+ * @label: Label to lookup
+ *
+ * Returns:
+ * Reference to the GPIO device or NULL. Reference must be released with
+ * ``gpio_device_put``.
+ */
+struct gpio_device *gpio_device_find_by_label(const char *label)
+{
+	return gpio_device_find((void *)label, gpio_chip_match_by_label);
+}
+EXPORT_SYMBOL_GPL(gpio_device_find_by_label);
+
 static int gpiochip_match_name(struct gpio_chip *gc, void *data)
 {
 	const char *name = data;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 5c5029cec226..92f7143bad3e 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -609,6 +609,7 @@ struct gpio_chip *gpiochip_find(void *data,
 struct gpio_device *gpio_device_find(void *data,
 				     int (*match)(struct gpio_chip *gc,
 						  void *data));
+struct gpio_device *gpio_device_find_by_label(const char *label);
 
 struct gpio_device *gpio_device_get(struct gpio_device *gdev);
 void gpio_device_put(struct gpio_device *gdev);
-- 
2.39.2

Re: [PATCH 03/21] gpiolib: provide gpio_device_find_by_label()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:52:51PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> By far the most common way of looking up GPIO devices is using their
> label. Provide a helpers for that to avoid every user implementing their
> own matching function.

...

> +/**
> + * gpio_device_find_by_label() - wrapper around gpio_device_find() finding the
> + *                               GPIO device by its backing chip's label
> + * @label: Label to lookup
> + *
> + * Returns:
> + * Reference to the GPIO device or NULL. Reference must be released with

> + * ``gpio_device_put``.

Out of a sudden different format to refer the function.
Should be gpio_device_put(), so kernel-doc makes a hyperlink.

> + */

So, please, render all your kernel docs and look at the end result.

...

> +	return gpio_device_find((void *)label, gpio_chip_match_by_label);

This casting is a bit awkward... But I have no good proposal for a change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/21] gpiolib: provide gpio_device_find_by_label()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> By far the most common way of looking up GPIO devices is using their
> label. Provide a helpers for that to avoid every user implementing their
> own matching function.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Yeah this is what everyone and their dog reimplements.
Good to centralize this.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 04/21] gpiolib: provide gpio_device_get_desc()

[Bartosz Golaszewski]

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

Getting the GPIO descriptor directly from the gpio_chip struct is
dangerous as we don't take the reference to the underlying GPIO device.
In order to start working towards removing gpiochip_get_desc(), let's
provide a safer variant that works with an existing reference to struct
gpio_device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 45 +++++++++++++++++++++++++++----------
 include/linux/gpio/driver.h |  2 ++
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 74b837671d30..4a9af6bfc6d4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -147,27 +147,48 @@ struct gpio_desc *gpio_to_desc(unsigned gpio)
 }
 EXPORT_SYMBOL_GPL(gpio_to_desc);
 
-/**
- * gpiochip_get_desc - get the GPIO descriptor corresponding to the given
- *                     hardware number for this chip
- * @gc: GPIO chip
- * @hwnum: hardware number of the GPIO for this chip
- *
- * Returns:
- * A pointer to the GPIO descriptor or ``ERR_PTR(-EINVAL)`` if no GPIO exists
- * in the given chip for the specified hardware number.
- */
+/* This function is deprecated and will be removed soon, don't use. */
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *gc,
 				    unsigned int hwnum)
 {
-	struct gpio_device *gdev = gc->gpiodev;
+	return gpio_device_get_desc(gc->gpiodev, hwnum);
+}
+EXPORT_SYMBOL_GPL(gpiochip_get_desc);
+
+/**
+ * gpio_device_get_desc() - get the GPIO descriptor corresponding to the given
+ *                          hardware number for this GPIO device
+ * @gdev: GPIO device to get the descriptor from
+ * @hwnum: hardware number of the GPIO for this chip
+ *
+ * Returns:
+ * A pointer to the GPIO descriptor or ``ERR_PTR(-EINVAL)`` if no GPIO exists
+ * in the given chip for the specified hardware number or ``ERR_PTR(-ENODEV)``
+ * if the underlying chip already vanished.
+ *
+ * The reference count of struct gpio_device is *NOT* increased like when the
+ * GPIO is being requested for exclusive usage. It's up to the caller to make
+ * sure the GPIO device will stay alive together with the descriptor returned
+ * by this function.
+ */
+struct gpio_desc *
+gpio_device_get_desc(struct gpio_device *gdev, unsigned int hwnum)
+{
+	struct gpio_chip *gc = gdev->chip;
+
+	/*
+	 * FIXME: This will be locked once we protect gdev->chip everywhere
+	 * with SRCU.
+	 */
+	if (!gc)
+		return ERR_PTR(-ENODEV);
 
 	if (hwnum >= gdev->ngpio)
 		return ERR_PTR(-EINVAL);
 
 	return &gdev->descs[hwnum];
 }
-EXPORT_SYMBOL_GPL(gpiochip_get_desc);
+EXPORT_SYMBOL_GPL(gpio_device_get_desc);
 
 /**
  * desc_to_gpio - convert a GPIO descriptor to the integer namespace
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 92f7143bad3e..a769baf3d731 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -766,6 +766,8 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 void gpiochip_free_own_desc(struct gpio_desc *desc);
 
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *gc, unsigned int hwnum);
+struct gpio_desc *
+gpio_device_get_desc(struct gpio_device *gdev, unsigned int hwnum);
 
 #ifdef CONFIG_GPIOLIB
 
-- 
2.39.2

Re: [PATCH 04/21] gpiolib: provide gpio_device_get_desc()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:52:52PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Getting the GPIO descriptor directly from the gpio_chip struct is
> dangerous as we don't take the reference to the underlying GPIO device.
> In order to start working towards removing gpiochip_get_desc(), let's
> provide a safer variant that works with an existing reference to struct
> gpio_device.

...

> +/**
> + * gpio_device_get_desc() - get the GPIO descriptor corresponding to the given
> + *                          hardware number for this GPIO device
> + * @gdev: GPIO device to get the descriptor from
> + * @hwnum: hardware number of the GPIO for this chip
> + *
> + * Returns:
> + * A pointer to the GPIO descriptor or ``ERR_PTR(-EINVAL)`` if no GPIO exists

The known constants can be referenced as %EINVAL.

> + * in the given chip for the specified hardware number or ``ERR_PTR(-ENODEV)``

Ditto.

> + * if the underlying chip already vanished.
> + *
> + * The reference count of struct gpio_device is *NOT* increased like when the
> + * GPIO is being requested for exclusive usage. It's up to the caller to make
> + * sure the GPIO device will stay alive together with the descriptor returned
> + * by this function.
> + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 04/21] gpiolib: provide gpio_device_get_desc()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Getting the GPIO descriptor directly from the gpio_chip struct is
> dangerous as we don't take the reference to the underlying GPIO device.
> In order to start working towards removing gpiochip_get_desc(), let's
> provide a safer variant that works with an existing reference to struct
> gpio_device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Andy had some good doc comments, with these addressed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 05/21] gpiolib: add support for scope-based management to gpio_device

[Bartosz Golaszewski]

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

As the few users that need to get the reference to the GPIO device often
release it right after inspecting its properties, let's add support for
the automatic reference release to struct gpio_device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/gpio/driver.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a769baf3d731..b2572b26c8e3 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -614,6 +614,8 @@ struct gpio_device *gpio_device_find_by_label(const char *label);
 struct gpio_device *gpio_device_get(struct gpio_device *gdev);
 void gpio_device_put(struct gpio_device *gdev);
 
+DEFINE_FREE(gpio_device_put, struct gpio_device *, if (_T) gpio_device_put(_T));
+
 bool gpiochip_line_is_irq(struct gpio_chip *gc, unsigned int offset);
 int gpiochip_reqres_irq(struct gpio_chip *gc, unsigned int offset);
 void gpiochip_relres_irq(struct gpio_chip *gc, unsigned int offset);
-- 
2.39.2

Re: [PATCH 05/21] gpiolib: add support for scope-based management to gpio_device

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> As the few users that need to get the reference to the GPIO device often
> release it right after inspecting its properties, let's add support for
> the automatic reference release to struct gpio_device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Needless to say, I'm a strong advocate for scoped resource management.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 06/21] gpiolib: provide gpiod_to_device()

[Bartosz Golaszewski]

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

struct gpio_desc is opaque so provide a way for users to retrieve the
underlying GPIO device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 14 ++++++++++++++
 include/linux/gpio/driver.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4a9af6bfc6d4..9637a79a9a60 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -219,6 +219,20 @@ struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_to_chip);
 
+/**
+ * gpiod_to_device() - Return the GPIO device owning this descriptor
+ * @desc: GPIO descriptor
+ *
+ * Returns:
+ * Pointer to the opaque struct gpio_device object. This function does not
+ * increase the reference count of gpio_device.
+ */
+struct gpio_device *gpiod_to_device(struct gpio_desc *desc)
+{
+	return gpio_device_get(desc->gdev);
+}
+EXPORT_SYMBOL_GPL(gpiod_to_device);
+
 /* dynamic allocation of GPIOs, e.g. on a hotplugged device */
 static int gpiochip_find_base(int ngpio)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index b2572b26c8e3..e3747e730ed1 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -778,6 +778,7 @@ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset);
 void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset);
 
 struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc);
+struct gpio_device *gpiod_to_device(struct gpio_desc *desc);
 
 #else /* CONFIG_GPIOLIB */
 
-- 
2.39.2

Re: [PATCH 06/21] gpiolib: provide gpiod_to_device()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:52:54PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> struct gpio_desc is opaque so provide a way for users to retrieve the
> underlying GPIO device.

...

> +/**
> + * gpiod_to_device() - Return the GPIO device owning this descriptor
> + * @desc: GPIO descriptor
> + *
> + * Returns:
> + * Pointer to the opaque struct gpio_device object. This function does not
> + * increase the reference count of gpio_device.

GPIO device.

> + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/21] gpiolib: provide gpiod_to_device()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> struct gpio_desc is opaque so provide a way for users to retrieve the
> underlying GPIO device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

[PATCH 07/21] gpiolib: provide gpio_device_get_base()

[Bartosz Golaszewski]

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

Let's start adding getters for the opaque struct gpio_device. Start with
a function allowing to retrieve the base GPIO number.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 13 +++++++++++++
 include/linux/gpio/driver.h |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9637a79a9a60..9715bbc698e9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -233,6 +233,19 @@ struct gpio_device *gpiod_to_device(struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_to_device);
 
+/**
+ * gpio_device_get_base() - Get the base GPIO number allocated by this device
+ * @gdev: GPIO device
+ *
+ * Returns:
+ * First GPIO number in the global GPIO numberspace for this device.
+ */
+int gpio_device_get_base(struct gpio_device *gdev)
+{
+	return gdev->base;
+}
+EXPORT_SYMBOL_GPL(gpio_device_get_base);
+
 /* dynamic allocation of GPIOs, e.g. on a hotplugged device */
 static int gpiochip_find_base(int ngpio)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index e3747e730ed1..b68b3493b29d 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -780,6 +780,9 @@ void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset);
 struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc);
 struct gpio_device *gpiod_to_device(struct gpio_desc *desc);
 
+/* struct gpio_device getters */
+int gpio_device_get_base(struct gpio_device *gdev);
+
 #else /* CONFIG_GPIOLIB */
 
 #include <linux/err.h>
-- 
2.39.2

Re: [PATCH 07/21] gpiolib: provide gpio_device_get_base()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Let's start adding getters for the opaque struct gpio_device. Start with
> a function allowing to retrieve the base GPIO number.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

I guess you have a solid usecase for drivers needing to do this
crazy thing, because I suppose you feel as much as me that
this should rather be gpiolib-internal and confined to
drivers/gpio/gpiolib.h?

If you add a valid reason for making this globally visible outside
of drivers/[gpio|pinctrl] to the commit message I guess I can live
with it because we need to think of the bigger picture:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

It brings to mind the now confusing "base" inside of
struct gpio_chip. We all know it should go away, but since it
is never used during the lifetime of the gpio_chip - or SHOULD
never be used - it should rather be an argument to
[devm_]gpiochip_add_data( .... int base);...

Maybe something we should add to our TODO file.

Yours,
Linus Walleij

Re: [PATCH 07/21] gpiolib: provide gpio_device_get_base()

[Bartosz Golaszewski]

On Thu, Sep 7, 2023 at 9:17 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Let's start adding getters for the opaque struct gpio_device. Start with
> > a function allowing to retrieve the base GPIO number.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> I guess you have a solid usecase for drivers needing to do this
> crazy thing, because I suppose you feel as much as me that
> this should rather be gpiolib-internal and confined to
> drivers/gpio/gpiolib.h?
>
> If you add a valid reason for making this globally visible outside
> of drivers/[gpio|pinctrl] to the commit message I guess I can live
> with it because we need to think of the bigger picture:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> It brings to mind the now confusing "base" inside of
> struct gpio_chip. We all know it should go away, but since it
> is never used during the lifetime of the gpio_chip - or SHOULD
> never be used - it should rather be an argument to
> [devm_]gpiochip_add_data( .... int base);...
>
> Maybe something we should add to our TODO file.
>
> Yours,
> Linus Walleij

For this series it's the HTE driver that uses it and I don't have a
good idea about how to change it. Dipen?

I would also love to make pinctrl not use the internal GPIOLIB header
so it'll be another user, unless you can figure out a way to not use
gc->base? :)

I think we're stuck with it for now.

Bart

Re: [PATCH 07/21] gpiolib: provide gpio_device_get_base()

[Dipen Patel]

On 9/7/23 12:57 AM, Bartosz Golaszewski wrote:
> On Thu, Sep 7, 2023 at 9:17 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> Let's start adding getters for the opaque struct gpio_device. Start with
>>> a function allowing to retrieve the base GPIO number.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> I guess you have a solid usecase for drivers needing to do this
>> crazy thing, because I suppose you feel as much as me that
>> this should rather be gpiolib-internal and confined to
>> drivers/gpio/gpiolib.h?
>>
>> If you add a valid reason for making this globally visible outside
>> of drivers/[gpio|pinctrl] to the commit message I guess I can live
>> with it because we need to think of the bigger picture:
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> It brings to mind the now confusing "base" inside of
>> struct gpio_chip. We all know it should go away, but since it
>> is never used during the lifetime of the gpio_chip - or SHOULD
>> never be used - it should rather be an argument to
>> [devm_]gpiochip_add_data( .... int base);...
>>
>> Maybe something we should add to our TODO file.
>>
>> Yours,
>> Linus Walleij
> 
> For this series it's the HTE driver that uses it and I don't have a
> good idea about how to change it. Dipen?

Missed responding to this, apologies. I believe we are having similar
discussion in the hte only patch. We can continue discussing there.

> 
> I would also love to make pinctrl not use the internal GPIOLIB header
> so it'll be another user, unless you can figure out a way to not use
> gc->base? :)
> 
> I think we're stuck with it for now.
> 
> Bart

[PATCH 08/21] gpio: acpi: provide acpi_gpio_device_free_interrupts()

[Bartosz Golaszewski]

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

We're moving away from public functions that take struct gpio_chip as
argument as the chip - unlike struct gpio_device - is owned by its
provider and tied to its lifetime.

Provide an alternative to acpi_gpiochip_free_interrupts().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-acpi.c | 29 +++++++++++++++++++++++------
 include/linux/gpio/driver.h | 12 ++++++++++++
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index fbda452fb4d6..5633e39396bc 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -558,12 +558,9 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 }
 EXPORT_SYMBOL_GPL(acpi_gpiochip_request_interrupts);
 
-/**
- * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
- * @chip:      GPIO chip
- *
- * Free interrupts associated with GPIO ACPI event method for the given
- * GPIO chip.
+/*
+ * This function is deprecated, use acpi_gpio_device_free_interrupts()
+ * instead.
  */
 void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 {
@@ -604,6 +601,26 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 }
 EXPORT_SYMBOL_GPL(acpi_gpiochip_free_interrupts);
 
+/**
+ * acpi_gpio_device_free_interrupts() - Free GPIO ACPI event interrupts.
+ * @chip	GPIO device
+ *
+ * Free interrupts associated with GPIO ACPI event method for the given
+ * GPIO device.
+ */
+void acpi_gpio_device_free_interrupts(struct gpio_device *gdev)
+{
+	struct gpio_chip *gc;
+
+	/* TODO: protect gdev->chip once SRCU is in place in GPIOLIB. */
+	gc = gdev->chip;
+	if (!gc)
+		return;
+
+	acpi_gpiochip_free_interrupts(gc);
+}
+EXPORT_SYMBOL_GPL(acpi_gpio_device_free_interrupts);
+
 int acpi_dev_add_driver_gpios(struct acpi_device *adev,
 			      const struct acpi_gpio_mapping *gpios)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index b68b3493b29d..47906bc56b3d 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -835,4 +835,16 @@ static inline struct fwnode_handle *gpiochip_node_get_first(struct device *dev)
 	return NULL;
 }
 
+/*
+ * FIXME: Remove this once the only driver that uses it - android tablets -
+ * becomes a good citizen and stops abusing GPIOLIB.
+ */
+#ifdef CONFIG_ACPI
+void acpi_gpio_device_free_interrupts(struct gpio_device *gdev);
+#else
+static inline void acpi_gpio_device_free_interrupts(struct gpio_device *gdev)
+{
+}
+#endif
+
 #endif /* __LINUX_GPIO_DRIVER_H */
-- 
2.39.2

Re: [PATCH 08/21] gpio: acpi: provide acpi_gpio_device_free_interrupts()

[Mika Westerberg]

Hi,

On Tue, Sep 05, 2023 at 08:52:56PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're moving away from public functions that take struct gpio_chip as
> argument as the chip - unlike struct gpio_device - is owned by its
> provider and tied to its lifetime.
> 
> Provide an alternative to acpi_gpiochip_free_interrupts().

Looks good to me, few minor comments below.

> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> ---
>  drivers/gpio/gpiolib-acpi.c | 29 +++++++++++++++++++++++------
>  include/linux/gpio/driver.h | 12 ++++++++++++
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index fbda452fb4d6..5633e39396bc 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -558,12 +558,9 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
>  }
>  EXPORT_SYMBOL_GPL(acpi_gpiochip_request_interrupts);
>  
> -/**
> - * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
> - * @chip:      GPIO chip
> - *
> - * Free interrupts associated with GPIO ACPI event method for the given
> - * GPIO chip.
> +/*
> + * This function is deprecated, use acpi_gpio_device_free_interrupts()
> + * instead.
>   */
>  void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
>  {
> @@ -604,6 +601,26 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
>  }
>  EXPORT_SYMBOL_GPL(acpi_gpiochip_free_interrupts);
>  
> +/**
> + * acpi_gpio_device_free_interrupts() - Free GPIO ACPI event interrupts.
> + * @chip	GPIO device

Should be:

@chip: GPIO device

> + *
> + * Free interrupts associated with GPIO ACPI event method for the given
> + * GPIO device.
> + */
> +void acpi_gpio_device_free_interrupts(struct gpio_device *gdev)
> +{
> +	struct gpio_chip *gc;
> +
> +	/* TODO: protect gdev->chip once SRCU is in place in GPIOLIB. */
> +	gc = gdev->chip;
> +	if (!gc)
> +		return;
> +
> +	acpi_gpiochip_free_interrupts(gc);
> +}
> +EXPORT_SYMBOL_GPL(acpi_gpio_device_free_interrupts);
> +
>  int acpi_dev_add_driver_gpios(struct acpi_device *adev,
>  			      const struct acpi_gpio_mapping *gpios)
>  {
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index b68b3493b29d..47906bc56b3d 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -835,4 +835,16 @@ static inline struct fwnode_handle *gpiochip_node_get_first(struct device *dev)
>  	return NULL;
>  }
>  
> +/*
> + * FIXME: Remove this once the only driver that uses it - android tablets -
> + * becomes a good citizen and stops abusing GPIOLIB.

There are a acouple of more when grepping for acpi_gpiochip_free_interrupts().

I'm not entirely sure why these functions are exposed to the drivers in
the first place. IMHO GPIOLIB should deal with these but perhaps there
is some good reason these drivers do it...

> + */
> +#ifdef CONFIG_ACPI
> +void acpi_gpio_device_free_interrupts(struct gpio_device *gdev);
> +#else
> +static inline void acpi_gpio_device_free_interrupts(struct gpio_device *gdev)
> +{
> +}

I would put these {} to the same line:

static inline void acpi_gpio_device_free_interrupts(struct gpio_device *gdev) { }

> +#endif
> +
>  #endif /* __LINUX_GPIO_DRIVER_H */
> -- 
> 2.39.2

[PATCH 09/21] gpiolib: reluctantly provide gpio_device_get_chip()

[Bartosz Golaszewski]

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

The process of converting all unauthorized users of struct gpio_chip to
using dedicated struct gpio_device function will be long so in the
meantime we must provide a way of retrieving the pointer to struct
gpio_chip from a GPIO device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 18 ++++++++++++++++++
 include/linux/gpio/driver.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9715bbc698e9..408f8a7753f9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -246,6 +246,24 @@ int gpio_device_get_base(struct gpio_device *gdev)
 }
 EXPORT_SYMBOL_GPL(gpio_device_get_base);
 
+/**
+ * gpio_device_get_chip() - Get the gpio_chip implementation of this GPIO device
+ * @gdev: GPIO device
+ *
+ * Until we can get rid of all non-driver users struct gpio_chip, we must
+ * provide a way of retrieving the pointer to it from struct gpio_device. This
+ * is *NOT* safe as the GPIO API is considered to be hot-unpluggable and the
+ * chip can dissapear at any moment (unlike reference-counted struct
+ * gpio_device).
+ *
+ * Use at your own risk.
+ */
+struct gpio_chip *gpio_device_get_chip(struct gpio_device *gdev)
+{
+	return gdev->chip;
+}
+EXPORT_SYMBOL_GPL(gpio_device_get_chip);
+
 /* dynamic allocation of GPIOs, e.g. on a hotplugged device */
 static int gpiochip_find_base(int ngpio)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 47906bc56b3d..bb9ec741bfda 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -782,6 +782,7 @@ struct gpio_device *gpiod_to_device(struct gpio_desc *desc);
 
 /* struct gpio_device getters */
 int gpio_device_get_base(struct gpio_device *gdev);
+struct gpio_chip *gpio_device_get_chip(struct gpio_device *gdev);
 
 #else /* CONFIG_GPIOLIB */
 
-- 
2.39.2

Re: [PATCH 09/21] gpiolib: reluctantly provide gpio_device_get_chip()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> The process of converting all unauthorized users of struct gpio_chip to
> using dedicated struct gpio_device function will be long so in the
> meantime we must provide a way of retrieving the pointer to struct
> gpio_chip from a GPIO device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Yeah, sigh. Maybe add a notice to drivers/gpio/TODO that this needs
to happen long time.

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

Yours,
Linus Walleij

[PATCH 10/21] gpiolib: replace find_chip_by_name() with gpio_device_find_by_label()

[Bartosz Golaszewski]

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

Remove all remaining uses of find_chip_by_name() (and subsequently:
gpiochip_find()) from gpiolib.c and use the new
gpio_device_find_by_label() instead.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 408f8a7753f9..90e8c3d8b6f6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1161,18 +1161,6 @@ struct gpio_device *gpio_device_find_by_label(const char *label)
 }
 EXPORT_SYMBOL_GPL(gpio_device_find_by_label);
 
-static int gpiochip_match_name(struct gpio_chip *gc, void *data)
-{
-	const char *name = data;
-
-	return !strcmp(gc->label, name);
-}
-
-static struct gpio_chip *find_chip_by_name(const char *name)
-{
-	return gpiochip_find((void *)name, gpiochip_match_name);
-}
-
 /**
  * gpio_device_get() - Increase the reference count of this GPIO device
  * @gdev: GPIO device to increase the refcount for
@@ -3924,21 +3912,22 @@ EXPORT_SYMBOL_GPL(gpiod_remove_lookup_table);
  */
 void gpiod_add_hogs(struct gpiod_hog *hogs)
 {
-	struct gpio_chip *gc;
 	struct gpiod_hog *hog;
 
 	mutex_lock(&gpio_machine_hogs_mutex);
 
 	for (hog = &hogs[0]; hog->chip_label; hog++) {
+		struct gpio_device *gdev __free(gpio_device_put) = NULL;
+
 		list_add_tail(&hog->list, &gpio_machine_hogs);
 
 		/*
 		 * The chip may have been registered earlier, so check if it
 		 * exists and, if so, try to hog the line now.
 		 */
-		gc = find_chip_by_name(hog->chip_label);
-		if (gc)
-			gpiochip_machine_hog(gc, hog);
+		gdev = gpio_device_find_by_label(hog->chip_label);
+		if (gdev)
+			gpiochip_machine_hog(gdev->chip, hog);
 	}
 
 	mutex_unlock(&gpio_machine_hogs_mutex);
@@ -3999,7 +3988,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 		return desc;
 
 	for (p = &table->table[0]; p->key; p++) {
-		struct gpio_chip *gc;
+		struct gpio_device *gdev __free(gpio_device_put) = NULL;
 
 		/* idx must always match exactly */
 		if (p->idx != idx)
@@ -4021,9 +4010,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 			return ERR_PTR(-EPROBE_DEFER);
 		}
 
-		gc = find_chip_by_name(p->key);
-
-		if (!gc) {
+		gdev = gpio_device_find_by_label(p->key);
+		if (!gdev) {
 			/*
 			 * As the lookup table indicates a chip with
 			 * p->key should exist, assume it may
@@ -4036,15 +4024,15 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 			return ERR_PTR(-EPROBE_DEFER);
 		}
 
-		if (gc->ngpio <= p->chip_hwnum) {
+		if (gdev->chip->ngpio <= p->chip_hwnum) {
 			dev_err(dev,
 				"requested GPIO %u (%u) is out of range [0..%u] for chip %s\n",
-				idx, p->chip_hwnum, gc->ngpio - 1,
-				gc->label);
+				idx, p->chip_hwnum, gdev->chip->ngpio - 1,
+				gdev->chip->label);
 			return ERR_PTR(-EINVAL);
 		}
 
-		desc = gpiochip_get_desc(gc, p->chip_hwnum);
+		desc = gpiochip_get_desc(gdev->chip, p->chip_hwnum);
 		*flags = p->flags;
 
 		return desc;
-- 
2.39.2

Re: [PATCH 10/21] gpiolib: replace find_chip_by_name() with gpio_device_find_by_label()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:52:58PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Remove all remaining uses of find_chip_by_name() (and subsequently:
> gpiochip_find()) from gpiolib.c and use the new
> gpio_device_find_by_label() instead.

...

> -		desc = gpiochip_get_desc(gc, p->chip_hwnum);
> +		desc = gpiochip_get_desc(gdev->chip, p->chip_hwnum);

Why not gpio_device_get_desc()?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 10/21] gpiolib: replace find_chip_by_name() with gpio_device_find_by_label()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Remove all remaining uses of find_chip_by_name() (and subsequently:
> gpiochip_find()) from gpiolib.c and use the new
> gpio_device_find_by_label() instead.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With Andy's comment addressed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFT PATCH 11/21] platform: x86: android-tablets: don't access GPIOLIB private members

[Bartosz Golaszewski]

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

We're slowly removing cases of abuse of the GPIOLIB public API. One of
the biggest issues is looking up and accessing struct gpio_chip whose
life-time is tied to the provider and which can disappear from under any
user at any given moment. We have provided new interfaces that use the
opaque struct gpio_device which is reference counted and will soon be
thorougly protected with appropriate locking.

Stop using old interfaces in this driver and switch to safer
alternatives.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../platform/x86/x86-android-tablets/core.c   | 38 ++++++++++---------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
index 2fd6060a31bb..687f84cd193c 100644
--- a/drivers/platform/x86/x86-android-tablets/core.c
+++ b/drivers/platform/x86/x86-android-tablets/core.c
@@ -12,6 +12,7 @@
 
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
 #include <linux/irq.h>
@@ -21,27 +22,28 @@
 #include <linux/string.h>
 
 #include "x86-android-tablets.h"
-/* For gpiochip_get_desc() which is EXPORT_SYMBOL_GPL() */
-#include "../../../gpio/gpiolib.h"
-#include "../../../gpio/gpiolib-acpi.h"
-
-static int gpiochip_find_match_label(struct gpio_chip *gc, void *data)
-{
-	return gc->label && !strcmp(gc->label, data);
-}
 
 int x86_android_tablet_get_gpiod(const char *label, int pin, struct gpio_desc **desc)
 {
+	struct gpio_device *gdev;
 	struct gpio_desc *gpiod;
-	struct gpio_chip *chip;
 
-	chip = gpiochip_find((void *)label, gpiochip_find_match_label);
-	if (!chip) {
-		pr_err("error cannot find GPIO chip %s\n", label);
+	/*
+	 * FIXME: handle GPIOs correctly! This driver should really use struct
+	 * device and GPIO lookup tables.
+	 *
+	 * WONTDO: We do leak this reference, but the whole approach to getting
+	 * GPIOs in this driver is such an abuse of the GPIOLIB API that it
+	 * doesn't make it much worse and it's the only way to keep the
+	 * interrupt requested later functional...
+	 */
+	gdev = gpio_device_find_by_label(label);
+	if (!gdev) {
+		pr_err("error cannot find GPIO device %s\n", label);
 		return -ENODEV;
 	}
 
-	gpiod = gpiochip_get_desc(chip, pin);
+	gpiod = gpio_device_get_desc(gdev, pin);
 	if (IS_ERR(gpiod)) {
 		pr_err("error %ld getting GPIO %s %d\n", PTR_ERR(gpiod), label, pin);
 		return PTR_ERR(gpiod);
@@ -257,9 +259,9 @@ static void x86_android_tablet_cleanup(void)
 
 static __init int x86_android_tablet_init(void)
 {
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
 	const struct x86_dev_info *dev_info;
 	const struct dmi_system_id *id;
-	struct gpio_chip *chip;
 	int i, ret = 0;
 
 	id = dmi_first_match(x86_android_tablet_ids);
@@ -273,13 +275,13 @@ static __init int x86_android_tablet_init(void)
 	 * _AEI (ACPI Event Interrupt) handlers, disable these.
 	 */
 	if (dev_info->invalid_aei_gpiochip) {
-		chip = gpiochip_find(dev_info->invalid_aei_gpiochip,
-				     gpiochip_find_match_label);
-		if (!chip) {
+		gdev = gpio_device_find_by_label(
+				dev_info->invalid_aei_gpiochip);
+		if (!gdev) {
 			pr_err("error cannot find GPIO chip %s\n", dev_info->invalid_aei_gpiochip);
 			return -ENODEV;
 		}
-		acpi_gpiochip_free_interrupts(chip);
+		acpi_gpio_device_free_interrupts(gdev);
 	}
 
 	/*
-- 
2.39.2

Re: [RFT PATCH 11/21] platform: x86: android-tablets: don't access GPIOLIB private members

[Hans de Goede]

Hi Bartosz,

On 9/5/23 20:52, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're slowly removing cases of abuse of the GPIOLIB public API. One of
> the biggest issues is looking up and accessing struct gpio_chip whose
> life-time is tied to the provider and which can disappear from under any
> user at any given moment. We have provided new interfaces that use the
> opaque struct gpio_device which is reference counted and will soon be
> thorougly protected with appropriate locking.
> 
> Stop using old interfaces in this driver and switch to safer
> alternatives.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

First of all sorry for the issues this hack-ish kernel module
is causing for cleaning up gpiolib APIs.

I don't know how close a look you took at the code, so first of
all let me try to briefly explain what this hackish kernel module
is for:

There are some x86_64/ACPI tablets which shipped with Android as
factory OS. On these tablets the device-specific (BSP style)
kernel has things like the touchscreen driver simply having
a hardcoded I2C bus-number + I2C client address. Combined
with also hardcoded GPIO numbers (using the old number base APIs)
for any GPIOs it needs.

So the original Android kernels do not need the devices
to be properly described in ACPI and the ACPI tables are
just one big copy and paste job from some BSP which do
not accurately describe the hardware at all.

x86-android-tablets.ko identifies affected models by their
DMI strings and then manually instantiates things like
i2c-clients for the touchscreen, accelerometer and also
other stuff. Yes this is ugly but it allows mainline kernels
to run pretty well on these devices since other then
the messed up ACPI tables these are pretty standard x86/ACPI
tablets.

I hope this explains the hacks, now on to the problems
these are causing:

> ---
>  .../platform/x86/x86-android-tablets/core.c   | 38 ++++++++++---------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
> index 2fd6060a31bb..687f84cd193c 100644
> --- a/drivers/platform/x86/x86-android-tablets/core.c
> +++ b/drivers/platform/x86/x86-android-tablets/core.c
> @@ -12,6 +12,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/irq.h>
> @@ -21,27 +22,28 @@
>  #include <linux/string.h>
>  
>  #include "x86-android-tablets.h"
> -/* For gpiochip_get_desc() which is EXPORT_SYMBOL_GPL() */
> -#include "../../../gpio/gpiolib.h"
> -#include "../../../gpio/gpiolib-acpi.h"
> -
> -static int gpiochip_find_match_label(struct gpio_chip *gc, void *data)
> -{
> -	return gc->label && !strcmp(gc->label, data);
> -}
>  
>  int x86_android_tablet_get_gpiod(const char *label, int pin, struct gpio_desc **desc)
>  {
> +	struct gpio_device *gdev;
>  	struct gpio_desc *gpiod;
> -	struct gpio_chip *chip;
>  
> -	chip = gpiochip_find((void *)label, gpiochip_find_match_label);
> -	if (!chip) {
> -		pr_err("error cannot find GPIO chip %s\n", label);
> +	/*
> +	 * FIXME: handle GPIOs correctly! This driver should really use struct
> +	 * device and GPIO lookup tables.
> +	 *
> +	 * WONTDO: We do leak this reference, but the whole approach to getting
> +	 * GPIOs in this driver is such an abuse of the GPIOLIB API that it
> +	 * doesn't make it much worse and it's the only way to keep the
> +	 * interrupt requested later functional...
> +	 */
> +	gdev = gpio_device_find_by_label(label);
> +	if (!gdev) {
> +		pr_err("error cannot find GPIO device %s\n", label);
>  		return -ENODEV;
>  	}
>  
> -	gpiod = gpiochip_get_desc(chip, pin);
> +	gpiod = gpio_device_get_desc(gdev, pin);
>  	if (IS_ERR(gpiod)) {
>  		pr_err("error %ld getting GPIO %s %d\n", PTR_ERR(gpiod), label, pin);
>  		return PTR_ERR(gpiod);


So rather then the above I think what needs to happen here
(and I can hopefully make some time for that this weekend) is:

1. Have the x86-android-tablets code instantiate a
   "x86-android-tablets" platform-dev
2. Have the code generate a gpiod_lookup_table for all GPIOs
   for which it currently uses x86_android_tablet_get_gpiod()
   with the .dev_id set to "x86-android-tablets"
3. Use regular gpiod_get() on the "x86-android-tablets" pdev
   to get the desc.

I think this should solve all the issues with 
x86_android_tablet_get_gpiod() poking inside
gpiolib external since now it is only using
public gpiolib APIs, right ?

One question about 2. there are 2 ways to do this:

i. Have the module_init() function loop over all
x86_dev_info members which will result in calling
x86_android_tablet_get_gpiod() and have it generate
one big gpiod_lookup_table for all GPIOs needed
in one go. At which point x86_android_tablet_get_gpiod()
goes away and can be directly replaced with gpiod_get()
calls on the pdev.

ii. Keep x86_android_tablet_get_gpiod() and have it
generate a gpiod_lookup_table with just 1 entry for
the GPIO which its caller wants. Register the lookup
table, do the gpiod_get() and then immediately
unregister the lookup table again.

ii. Would be easier for me to implement, especially
since there is also some custom (board specific)
init code calling x86_android_tablet_get_gpiod()
which would require some special handling for i.

OTOH I guess some people will consider ii. somewhat
ugly, although AFAICT it is perfectly ok to use
the gpiolib lookup APIs this way.

Can you please let me known if you are ok with ii,
or if you would prefer me going with solution i. ?

That way when I can make some time to start working
on this I can pick the preferred solution right away.



> @@ -257,9 +259,9 @@ static void x86_android_tablet_cleanup(void)
>  
>  static __init int x86_android_tablet_init(void)
>  {
> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;
>  	const struct x86_dev_info *dev_info;
>  	const struct dmi_system_id *id;
> -	struct gpio_chip *chip;
>  	int i, ret = 0;
>  
>  	id = dmi_first_match(x86_android_tablet_ids);
> @@ -273,13 +275,13 @@ static __init int x86_android_tablet_init(void)
>  	 * _AEI (ACPI Event Interrupt) handlers, disable these.
>  	 */
>  	if (dev_info->invalid_aei_gpiochip) {
> -		chip = gpiochip_find(dev_info->invalid_aei_gpiochip,
> -				     gpiochip_find_match_label);
> -		if (!chip) {
> +		gdev = gpio_device_find_by_label(
> +				dev_info->invalid_aei_gpiochip);
> +		if (!gdev) {
>  			pr_err("error cannot find GPIO chip %s\n", dev_info->invalid_aei_gpiochip);
>  			return -ENODEV;
>  		}
> -		acpi_gpiochip_free_interrupts(chip);
> +		acpi_gpio_device_free_interrupts(gdev);
>  	}
>  
>  	/*

After some recent improvements there is only 1 board left which sets
dev_info->invalid_aei_gpiochip and that can easily be replaced with
with adding 1 extra entry to gpiolib_acpi_quirks[] inside
drivers/gpio/gpiolib-acpi.c .

So I believe the right solution here is to just remove
dev_info->invalid_aei_gpiochip support for x86-android-tablets
all together and then at least x86-android-tablets will no
longer be making any hackish acpi_gpiochip_free_interrupts() calls.

I don't want to make any promises wrt the timing, but I should
be able to prepare a set of patches which simply removes all
the private gpiolib API use from x86-android-tablets, so that
you don't need to workaround that in this patch series.

With some luck I can have an immutable branch with 6.6-rc1 +
such a patch-series ready for you soon after 6.6-rc1 is
released.

Regards,

Hans

Re: [RFT PATCH 11/21] platform: x86: android-tablets: don't access GPIOLIB private members

[Bartosz Golaszewski]

On Wed, Sep 6, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Bartosz,
>
> On 9/5/23 20:52, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We're slowly removing cases of abuse of the GPIOLIB public API. One of
> > the biggest issues is looking up and accessing struct gpio_chip whose
> > life-time is tied to the provider and which can disappear from under any
> > user at any given moment. We have provided new interfaces that use the
> > opaque struct gpio_device which is reference counted and will soon be
> > thorougly protected with appropriate locking.
> >
> > Stop using old interfaces in this driver and switch to safer
> > alternatives.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> First of all sorry for the issues this hack-ish kernel module
> is causing for cleaning up gpiolib APIs.
>
> I don't know how close a look you took at the code, so first of
> all let me try to briefly explain what this hackish kernel module
> is for:
>
> There are some x86_64/ACPI tablets which shipped with Android as
> factory OS. On these tablets the device-specific (BSP style)
> kernel has things like the touchscreen driver simply having
> a hardcoded I2C bus-number + I2C client address. Combined
> with also hardcoded GPIO numbers (using the old number base APIs)
> for any GPIOs it needs.
>
> So the original Android kernels do not need the devices
> to be properly described in ACPI and the ACPI tables are
> just one big copy and paste job from some BSP which do
> not accurately describe the hardware at all.
>
> x86-android-tablets.ko identifies affected models by their
> DMI strings and then manually instantiates things like
> i2c-clients for the touchscreen, accelerometer and also
> other stuff. Yes this is ugly but it allows mainline kernels
> to run pretty well on these devices since other then
> the messed up ACPI tables these are pretty standard x86/ACPI
> tablets.
>
> I hope this explains the hacks, now on to the problems
> these are causing:

This makes sense! Maybe we'd need a good-old board file setting up all
non-described devices using the driver model?

>
> > ---
> >  .../platform/x86/x86-android-tablets/core.c   | 38 ++++++++++---------
> >  1 file changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
> > index 2fd6060a31bb..687f84cd193c 100644
> > --- a/drivers/platform/x86/x86-android-tablets/core.c
> > +++ b/drivers/platform/x86/x86-android-tablets/core.c
> > @@ -12,6 +12,7 @@
> >
> >  #include <linux/acpi.h>
> >  #include <linux/dmi.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/gpio/driver.h>
> >  #include <linux/gpio/machine.h>
> >  #include <linux/irq.h>
> > @@ -21,27 +22,28 @@
> >  #include <linux/string.h>
> >
> >  #include "x86-android-tablets.h"
> > -/* For gpiochip_get_desc() which is EXPORT_SYMBOL_GPL() */
> > -#include "../../../gpio/gpiolib.h"
> > -#include "../../../gpio/gpiolib-acpi.h"
> > -
> > -static int gpiochip_find_match_label(struct gpio_chip *gc, void *data)
> > -{
> > -     return gc->label && !strcmp(gc->label, data);
> > -}
> >
> >  int x86_android_tablet_get_gpiod(const char *label, int pin, struct gpio_desc **desc)
> >  {
> > +     struct gpio_device *gdev;
> >       struct gpio_desc *gpiod;
> > -     struct gpio_chip *chip;
> >
> > -     chip = gpiochip_find((void *)label, gpiochip_find_match_label);
> > -     if (!chip) {
> > -             pr_err("error cannot find GPIO chip %s\n", label);
> > +     /*
> > +      * FIXME: handle GPIOs correctly! This driver should really use struct
> > +      * device and GPIO lookup tables.
> > +      *
> > +      * WONTDO: We do leak this reference, but the whole approach to getting
> > +      * GPIOs in this driver is such an abuse of the GPIOLIB API that it
> > +      * doesn't make it much worse and it's the only way to keep the
> > +      * interrupt requested later functional...
> > +      */
> > +     gdev = gpio_device_find_by_label(label);
> > +     if (!gdev) {
> > +             pr_err("error cannot find GPIO device %s\n", label);
> >               return -ENODEV;
> >       }
> >
> > -     gpiod = gpiochip_get_desc(chip, pin);
> > +     gpiod = gpio_device_get_desc(gdev, pin);
> >       if (IS_ERR(gpiod)) {
> >               pr_err("error %ld getting GPIO %s %d\n", PTR_ERR(gpiod), label, pin);
> >               return PTR_ERR(gpiod);
>
>
> So rather then the above I think what needs to happen here
> (and I can hopefully make some time for that this weekend) is:
>
> 1. Have the x86-android-tablets code instantiate a
>    "x86-android-tablets" platform-dev
> 2. Have the code generate a gpiod_lookup_table for all GPIOs
>    for which it currently uses x86_android_tablet_get_gpiod()
>    with the .dev_id set to "x86-android-tablets"
> 3. Use regular gpiod_get() on the "x86-android-tablets" pdev
>    to get the desc.
>
> I think this should solve all the issues with
> x86_android_tablet_get_gpiod() poking inside
> gpiolib external since now it is only using
> public gpiolib APIs, right ?
>
> One question about 2. there are 2 ways to do this:
>
> i. Have the module_init() function loop over all
> x86_dev_info members which will result in calling
> x86_android_tablet_get_gpiod() and have it generate
> one big gpiod_lookup_table for all GPIOs needed
> in one go. At which point x86_android_tablet_get_gpiod()
> goes away and can be directly replaced with gpiod_get()
> calls on the pdev.
>
> ii. Keep x86_android_tablet_get_gpiod() and have it
> generate a gpiod_lookup_table with just 1 entry for
> the GPIO which its caller wants. Register the lookup
> table, do the gpiod_get() and then immediately
> unregister the lookup table again.
>
> ii. Would be easier for me to implement, especially
> since there is also some custom (board specific)
> init code calling x86_android_tablet_get_gpiod()
> which would require some special handling for i.
>
> OTOH I guess some people will consider ii. somewhat
> ugly, although AFAICT it is perfectly ok to use
> the gpiolib lookup APIs this way.
>
> Can you please let me known if you are ok with ii,
> or if you would prefer me going with solution i. ?
>

I am fine with ii. I have recently sent a patch that does exactly that
in one of the SPI drivers. It's ugly but it's better than what we have
now.

> That way when I can make some time to start working
> on this I can pick the preferred solution right away.
>
>
>
> > @@ -257,9 +259,9 @@ static void x86_android_tablet_cleanup(void)
> >
> >  static __init int x86_android_tablet_init(void)
> >  {
> > +     struct gpio_device *gdev __free(gpio_device_put) = NULL;
> >       const struct x86_dev_info *dev_info;
> >       const struct dmi_system_id *id;
> > -     struct gpio_chip *chip;
> >       int i, ret = 0;
> >
> >       id = dmi_first_match(x86_android_tablet_ids);
> > @@ -273,13 +275,13 @@ static __init int x86_android_tablet_init(void)
> >        * _AEI (ACPI Event Interrupt) handlers, disable these.
> >        */
> >       if (dev_info->invalid_aei_gpiochip) {
> > -             chip = gpiochip_find(dev_info->invalid_aei_gpiochip,
> > -                                  gpiochip_find_match_label);
> > -             if (!chip) {
> > +             gdev = gpio_device_find_by_label(
> > +                             dev_info->invalid_aei_gpiochip);
> > +             if (!gdev) {
> >                       pr_err("error cannot find GPIO chip %s\n", dev_info->invalid_aei_gpiochip);
> >                       return -ENODEV;
> >               }
> > -             acpi_gpiochip_free_interrupts(chip);
> > +             acpi_gpio_device_free_interrupts(gdev);
> >       }
> >
> >       /*
>
> After some recent improvements there is only 1 board left which sets
> dev_info->invalid_aei_gpiochip and that can easily be replaced with
> with adding 1 extra entry to gpiolib_acpi_quirks[] inside
> drivers/gpio/gpiolib-acpi.c .
>
> So I believe the right solution here is to just remove
> dev_info->invalid_aei_gpiochip support for x86-android-tablets
> all together and then at least x86-android-tablets will no
> longer be making any hackish acpi_gpiochip_free_interrupts() calls.
>
> I don't want to make any promises wrt the timing, but I should
> be able to prepare a set of patches which simply removes all
> the private gpiolib API use from x86-android-tablets, so that
> you don't need to workaround that in this patch series.
>
> With some luck I can have an immutable branch with 6.6-rc1 +
> such a patch-series ready for you soon after 6.6-rc1 is
> released.
>

That would be awesome, thanks a lot!

> Regards,
>
> Hans
>
>
>

Bart

[1] https://lore.kernel.org/lkml/d57a99ce-77eb-409f-8371-95f2658fa0c0@sirena.org.uk/T/

Re: [RFT PATCH 11/21] platform: x86: android-tablets: don't access GPIOLIB private members

[Hans de Goede]

Hi Bart,

On 9/6/23 16:27, Bartosz Golaszewski wrote:
> On Wed, Sep 6, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Bartosz,
>>
>> On 9/5/23 20:52, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> We're slowly removing cases of abuse of the GPIOLIB public API. One of
>>> the biggest issues is looking up and accessing struct gpio_chip whose
>>> life-time is tied to the provider and which can disappear from under any
>>> user at any given moment. We have provided new interfaces that use the
>>> opaque struct gpio_device which is reference counted and will soon be
>>> thorougly protected with appropriate locking.
>>>
>>> Stop using old interfaces in this driver and switch to safer
>>> alternatives.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> First of all sorry for the issues this hack-ish kernel module
>> is causing for cleaning up gpiolib APIs.
>>
>> I don't know how close a look you took at the code, so first of
>> all let me try to briefly explain what this hackish kernel module
>> is for:
>>
>> There are some x86_64/ACPI tablets which shipped with Android as
>> factory OS. On these tablets the device-specific (BSP style)
>> kernel has things like the touchscreen driver simply having
>> a hardcoded I2C bus-number + I2C client address. Combined
>> with also hardcoded GPIO numbers (using the old number base APIs)
>> for any GPIOs it needs.
>>
>> So the original Android kernels do not need the devices
>> to be properly described in ACPI and the ACPI tables are
>> just one big copy and paste job from some BSP which do
>> not accurately describe the hardware at all.
>>
>> x86-android-tablets.ko identifies affected models by their
>> DMI strings and then manually instantiates things like
>> i2c-clients for the touchscreen, accelerometer and also
>> other stuff. Yes this is ugly but it allows mainline kernels
>> to run pretty well on these devices since other then
>> the messed up ACPI tables these are pretty standard x86/ACPI
>> tablets.
>>
>> I hope this explains the hacks, now on to the problems
>> these are causing:
> 
> This makes sense! Maybe we'd need a good-old board file setting up all
> non-described devices using the driver model?

Right, this is pretty much exactly what the x86-android-tablets
code does. Except that it does it for a bunch of boards in a single
.ko / driver. There is a lot of commonality between these boards,
so this allows sharing most of the code.

The driver uses DMI matching, with the match's driver_data pointing
to a description of which devices to instantiate and then the shared
code takes care of instantiating those.

About 90% of the data / code is __init or __initdata so both
the code to instantiate the devices as well as the per board
data is free-ed after module_init() has run.

<snip>

>> So rather then the above I think what needs to happen here
>> (and I can hopefully make some time for that this weekend) is:
>>
>> 1. Have the x86-android-tablets code instantiate a
>>    "x86-android-tablets" platform-dev
>> 2. Have the code generate a gpiod_lookup_table for all GPIOs
>>    for which it currently uses x86_android_tablet_get_gpiod()
>>    with the .dev_id set to "x86-android-tablets"
>> 3. Use regular gpiod_get() on the "x86-android-tablets" pdev
>>    to get the desc.
>>
>> I think this should solve all the issues with
>> x86_android_tablet_get_gpiod() poking inside
>> gpiolib external since now it is only using
>> public gpiolib APIs, right ?
>>
>> One question about 2. there are 2 ways to do this:
>>
>> i. Have the module_init() function loop over all
>> x86_dev_info members which will result in calling
>> x86_android_tablet_get_gpiod() and have it generate
>> one big gpiod_lookup_table for all GPIOs needed
>> in one go. At which point x86_android_tablet_get_gpiod()
>> goes away and can be directly replaced with gpiod_get()
>> calls on the pdev.
>>
>> ii. Keep x86_android_tablet_get_gpiod() and have it
>> generate a gpiod_lookup_table with just 1 entry for
>> the GPIO which its caller wants. Register the lookup
>> table, do the gpiod_get() and then immediately
>> unregister the lookup table again.
>>
>> ii. Would be easier for me to implement, especially
>> since there is also some custom (board specific)
>> init code calling x86_android_tablet_get_gpiod()
>> which would require some special handling for i.
>>
>> OTOH I guess some people will consider ii. somewhat
>> ugly, although AFAICT it is perfectly ok to use
>> the gpiolib lookup APIs this way.
>>
>> Can you please let me known if you are ok with ii,
>> or if you would prefer me going with solution i. ?
>>
> 
> I am fine with ii. I have recently sent a patch that does exactly that
> in one of the SPI drivers. It's ugly but it's better than what we have
> now.

Ok, I have just finished implementing this using the ii. method.

I'll post a patch-series for this for review right after this email.

After that series x86-android-tablets should no longer be a problem
wrt using any private gpiolib APIs.

Regards,

Hans

Re: [RFT PATCH 11/21] platform: x86: android-tablets: don't access GPIOLIB private members

[Andy Shevchenko]

On Sat, Sep 09, 2023 at 04:17:53PM +0200, Hans de Goede wrote:
> On 9/6/23 16:27, Bartosz Golaszewski wrote:
> > On Wed, Sep 6, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > This makes sense! Maybe we'd need a good-old board file setting up all
> > non-described devices using the driver model?
> 
> Right, this is pretty much exactly what the x86-android-tablets
> code does. Except that it does it for a bunch of boards in a single
> .ko / driver. There is a lot of commonality between these boards,
> so this allows sharing most of the code.
> 
> The driver uses DMI matching, with the match's driver_data pointing
> to a description of which devices to instantiate and then the shared
> code takes care of instantiating those.
> 
> About 90% of the data / code is __init or __initdata so both
> the code to instantiate the devices as well as the per board
> data is free-ed after module_init() has run.

...which is nicely looked and isolated hack (or quirk if you prefer)
that I like! Thanks, Hans, for maintaining that!

-- 
With Best Regards,
Andy Shevchenko

[PATCH 12/21] hte: allow building modules with COMPILE_TEST enabled

[Bartosz Golaszewski]

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

Allow building all HTE modules with COMPILE_TEST Kconfig option enabled.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/hte/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hte/Kconfig b/drivers/hte/Kconfig
index cf29e0218bae..083e67492bf2 100644
--- a/drivers/hte/Kconfig
+++ b/drivers/hte/Kconfig
@@ -16,7 +16,7 @@ if HTE
 
 config HTE_TEGRA194
 	tristate "NVIDIA Tegra194 HTE Support"
-	depends on ARCH_TEGRA_194_SOC
+	depends on (ARCH_TEGRA_194_SOC || COMPILE_TEST)
 	help
 	  Enable this option for integrated hardware timestamping engine also
 	  known as generic timestamping engine (GTE) support on NVIDIA Tegra194
@@ -25,7 +25,7 @@ config HTE_TEGRA194
 
 config HTE_TEGRA194_TEST
         tristate "NVIDIA Tegra194 HTE Test"
-        depends on HTE_TEGRA194
+        depends on (HTE_TEGRA194 || COMPILE_TEST)
         help
 	  The NVIDIA Tegra194 GTE test driver demonstrates how to use HTE
 	  framework to timestamp GPIO and LIC IRQ lines.
-- 
2.39.2

Re: [PATCH 12/21] hte: allow building modules with COMPILE_TEST enabled

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Allow building all HTE modules with COMPILE_TEST Kconfig option enabled.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This should be a separate patch should it not?
Just send it separately to Dipen so he can merge it.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 12/21] hte: allow building modules with COMPILE_TEST enabled

[Bartosz Golaszewski]

On Thu, Sep 7, 2023 at 9:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Allow building all HTE modules with COMPILE_TEST Kconfig option enabled.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This should be a separate patch should it not?
> Just send it separately to Dipen so he can merge it.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij

Dipen,

Can you just pick this up and the other patch addressing a comment in
a HTE driver separately? Would spare a resend to the list and I'd drop
it from the series.

Bart

[PATCH 13/21] hte: tegra194: improve the GPIO-related comment

[Bartosz Golaszewski]

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

Using any of the GPIO interfaces using the global numberspace is
deprecated. Make it clear in the comment.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/hte/hte-tegra194.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/hte/hte-tegra194.c b/drivers/hte/hte-tegra194.c
index 6fe6897047ac..9fd3c00ff695 100644
--- a/drivers/hte/hte-tegra194.c
+++ b/drivers/hte/hte-tegra194.c
@@ -407,12 +407,15 @@ static int tegra_hte_line_xlate(struct hte_chip *gc,
 		return -EINVAL;
 
 	/*
+	 * GPIO consumers can access GPIOs in two ways:
 	 *
-	 * There are two paths GPIO consumers can take as follows:
-	 * 1) The consumer (gpiolib-cdev for example) which uses GPIO global
-	 * number which gets assigned run time.
-	 * 2) The consumer passing GPIO from the DT which is assigned
-	 * statically for example by using TEGRA194_AON_GPIO gpio DT binding.
+	 * 1) Using the global GPIO numberspace.
+	 *
+	 * This is the old, now DEPRECATED method and should not be used in
+	 * new code. TODO: Check if tegra is even concerned by this.
+	 *
+	 * 2) Using GPIO descriptors that can be assigned to consumer devices
+	 * using device-tree, ACPI or lookup tables.
 	 *
 	 * The code below addresses both the consumer use cases and maps into
 	 * HTE/GTE namespace.
-- 
2.39.2

Re: [PATCH 13/21] hte: tegra194: improve the GPIO-related comment

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Using any of the GPIO interfaces using the global numberspace is
> deprecated. Make it clear in the comment.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Split this off and send separately to Dipen.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Bartosz Golaszewski]

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

Using struct gpio_chip is not safe as it will disappear if the
underlying driver is unbound for any reason. Switch to using reference
counted struct gpio_device and its dedicated accessors.

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

diff --git a/drivers/hte/hte-tegra194.c b/drivers/hte/hte-tegra194.c
index 9fd3c00ff695..d83ef30c9588 100644
--- a/drivers/hte/hte-tegra194.c
+++ b/drivers/hte/hte-tegra194.c
@@ -132,7 +132,7 @@ struct tegra_hte_soc {
 	const struct tegra_hte_data *prov_data;
 	struct tegra_hte_line_data *line_data;
 	struct hte_chip *chip;
-	struct gpio_chip *c;
+	struct gpio_device *gdev;
 	void __iomem *regs;
 };
 
@@ -421,7 +421,7 @@ static int tegra_hte_line_xlate(struct hte_chip *gc,
 	 * HTE/GTE namespace.
 	 */
 	if (gs->prov_data->type == HTE_TEGRA_TYPE_GPIO && !args) {
-		line_id = desc->attr.line_id - gs->c->base;
+		line_id = desc->attr.line_id - gpio_device_get_base(gs->gdev);
 		map = gs->prov_data->map;
 		map_sz = gs->prov_data->map_sz;
 	} else if (gs->prov_data->type == HTE_TEGRA_TYPE_GPIO && args) {
@@ -643,12 +643,15 @@ static irqreturn_t tegra_hte_isr(int irq, void *dev_id)
 static bool tegra_hte_match_from_linedata(const struct hte_chip *chip,
 					  const struct hte_ts_desc *hdesc)
 {
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
 	struct tegra_hte_soc *hte_dev = chip->data;
 
 	if (!hte_dev || (hte_dev->prov_data->type != HTE_TEGRA_TYPE_GPIO))
 		return false;
 
-	return hte_dev->c == gpiod_to_chip(hdesc->attr.line_data);
+	gdev = gpiod_to_device(hdesc->attr.line_data);
+
+	return hte_dev->gdev == gdev;
 }
 
 static const struct of_device_id tegra_hte_of_match[] = {
@@ -676,16 +679,18 @@ static void tegra_gte_disable(void *data)
 	tegra_hte_writel(gs, HTE_TECTRL, 0);
 }
 
-static int tegra_get_gpiochip_from_name(struct gpio_chip *chip, void *data)
-{
-	return !strcmp(chip->label, data);
-}
-
 static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
 {
 	return chip->fwnode == of_node_to_fwnode(data);
 }
 
+static void tegra_hte_put_gpio_device(void *data)
+{
+	struct gpio_device *gdev = data;
+
+	gpio_device_put(gdev);
+}
+
 static int tegra_hte_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -763,8 +768,8 @@ static int tegra_hte_probe(struct platform_device *pdev)
 
 		if (of_device_is_compatible(dev->of_node,
 					    "nvidia,tegra194-gte-aon")) {
-			hte_dev->c = gpiochip_find("tegra194-gpio-aon",
-						tegra_get_gpiochip_from_name);
+			hte_dev->gdev =
+				gpio_device_find_by_label("tegra194-gpio-aon");
 		} else {
 			gpio_ctrl = of_parse_phandle(dev->of_node,
 						     "nvidia,gpio-controller",
@@ -775,14 +780,19 @@ static int tegra_hte_probe(struct platform_device *pdev)
 				return -ENODEV;
 			}
 
-			hte_dev->c = gpiochip_find(gpio_ctrl,
-						   tegra_gpiochip_match);
+			hte_dev->gdev = gpio_device_find(gpio_ctrl,
+							 tegra_gpiochip_match);
 			of_node_put(gpio_ctrl);
 		}
 
-		if (!hte_dev->c)
+		if (!hte_dev->gdev)
 			return dev_err_probe(dev, -EPROBE_DEFER,
 					     "wait for gpio controller\n");
+
+		ret = devm_add_action_or_reset(dev, tegra_hte_put_gpio_device,
+					       hte_dev->gdev);
+		if (ret)
+			return ret;
 	}
 
 	hte_dev->chip = gc;
-- 
2.39.2

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:53:02PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Using struct gpio_chip is not safe as it will disappear if the
> underlying driver is unbound for any reason. Switch to using reference
> counted struct gpio_device and its dedicated accessors.

...

> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;

Using this requires cleanup.h to be included.
Does any of the included GPIO headers guarantee that inclusion implicitly?
Even though, it's a good practice to include headers of what we are using
independently if other (library) headers include them. I.o.w. we can rely
only on our headers (here HTE framework related) to guarantee any inclusions
implicitly.

This also applies to other users of the same construct.

...

>  static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>  {
>  	return chip->fwnode == of_node_to_fwnode(data);
>  }

Not sure how many users of this kind of match, but it might be useful to have
it by GPIO library

	gpio_device_find_by_fwnode()

-- 
With Best Regards,
Andy Shevchenko

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Using struct gpio_chip is not safe as it will disappear if the
> underlying driver is unbound for any reason. Switch to using reference
> counted struct gpio_device and its dedicated accessors.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As Andy points out add <linux/cleanup.h>, with that fixed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I think this can be merged into the gpio tree after leaving some
slack for the HTE maintainer to look at it, things look so much
better after this.

Yours,
Linus Walleij

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Bartosz Golaszewski]

On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Using struct gpio_chip is not safe as it will disappear if the
> > underlying driver is unbound for any reason. Switch to using reference
> > counted struct gpio_device and its dedicated accessors.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> As Andy points out add <linux/cleanup.h>, with that fixed:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> I think this can be merged into the gpio tree after leaving some
> slack for the HTE maintainer to look at it, things look so much
> better after this.
>
> Yours,
> Linus Walleij

Dipen,

if you could give this patch a test and possibly ack it for me to take
it through the GPIO tree (or go the immutable tag from HTE route) then
it would be great. This is the last user of gpiochip_find() treewide,
so with it we could remove it entirely for v6.7.

Bart

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Dipen Patel]

On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> Using struct gpio_chip is not safe as it will disappear if the
>>> underlying driver is unbound for any reason. Switch to using reference
>>> counted struct gpio_device and its dedicated accessors.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> As Andy points out add <linux/cleanup.h>, with that fixed:
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> I think this can be merged into the gpio tree after leaving some
>> slack for the HTE maintainer to look at it, things look so much
>> better after this.
>>
>> Yours,
>> Linus Walleij
> 
> Dipen,
> 
> if you could give this patch a test and possibly ack it for me to take
> it through the GPIO tree (or go the immutable tag from HTE route) then
> it would be great. This is the last user of gpiochip_find() treewide,
> so with it we could remove it entirely for v6.7.

Progress so far for the RFT...

I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
some patches I needed to manually apply and correct. With all this, it failed
compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
compile. I thought I should let you know this part.

Now, I tried to test the hte and it seems to fail finding the gpio device,
roughly around this place [1]. I thought it would be your patch series so
tried to just use 6.6rc1 without your patches and it still failed at the
same place. I have to trace back now from which kernel version it broke.

> 
> Bart

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Dipen Patel]

On 10/4/23 1:30 PM, Dipen Patel wrote:
> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>
>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>
>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>
>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>> underlying driver is unbound for any reason. Switch to using reference
>>>> counted struct gpio_device and its dedicated accessors.
>>>>
>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> I think this can be merged into the gpio tree after leaving some
>>> slack for the HTE maintainer to look at it, things look so much
>>> better after this.
>>>
>>> Yours,
>>> Linus Walleij
>>
>> Dipen,
>>
>> if you could give this patch a test and possibly ack it for me to take
>> it through the GPIO tree (or go the immutable tag from HTE route) then
>> it would be great. This is the last user of gpiochip_find() treewide,
>> so with it we could remove it entirely for v6.7.
> 
> Progress so far for the RFT...
> 
> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> some patches I needed to manually apply and correct. With all this, it failed
> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> compile. I thought I should let you know this part.
> 
> Now, I tried to test the hte and it seems to fail finding the gpio device,
> roughly around this place [1]. I thought it would be your patch series so
> tried to just use 6.6rc1 without your patches and it still failed at the
> same place. I have to trace back now from which kernel version it broke.

[1].
https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781

of course with your patches it would fail for the gdev instead of the chip.
> 
>>
>> Bart
> 

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Dipen Patel]

On 10/4/23 1:33 PM, Dipen Patel wrote:
> On 10/4/23 1:30 PM, Dipen Patel wrote:
>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>
>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>
>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>
>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>
>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>
>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>
>>>> I think this can be merged into the gpio tree after leaving some
>>>> slack for the HTE maintainer to look at it, things look so much
>>>> better after this.
>>>>
>>>> Yours,
>>>> Linus Walleij
>>>
>>> Dipen,
>>>
>>> if you could give this patch a test and possibly ack it for me to take
>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>> it would be great. This is the last user of gpiochip_find() treewide,
>>> so with it we could remove it entirely for v6.7.
>>
>> Progress so far for the RFT...
>>
>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>> some patches I needed to manually apply and correct. With all this, it failed
>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>> compile. I thought I should let you know this part.
>>
>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>> roughly around this place [1]. I thought it would be your patch series so
>> tried to just use 6.6rc1 without your patches and it still failed at the
>> same place. I have to trace back now from which kernel version it broke.
> 
> [1].
> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> 
> of course with your patches it would fail for the gdev instead of the chip.

Small update:

I put some debugging prints in the gpio match function in the hte-tegra194.c as
below:

static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
 {
+       struct device_node *node = data;
+       struct fwnode_handle *fw = of_node_to_fwnode(data);
+       if (!fw || !chip->fwnode)
+               pr_err("dipen patel: fw is null\n");

-       pr_err("%s:%d\n", __func__, __LINE__);
+       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
__func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
fw), fw->dev->init_name);
        return chip->fwnode == of_node_to_fwnode(data);
 }

The output of the printfs looks like below:
[    3.955194] dipen patel: fw is null -----> this message started appearing
when I added !chip->fwnode test in the if condition line.

[    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
gpio@c2f0000, match?:0, fwnode name:(null)

I conclude that chip->fwnode is empty. Any idea in which conditions that node
would be empty?

>>
>>>
>>> Bart
>>
> 

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Dipen Patel]

On 10/4/23 3:54 PM, Dipen Patel wrote:
> On 10/4/23 1:33 PM, Dipen Patel wrote:
>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>
>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>
>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>
>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>
>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>
>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>
>>>>> I think this can be merged into the gpio tree after leaving some
>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>> better after this.
>>>>>
>>>>> Yours,
>>>>> Linus Walleij
>>>>
>>>> Dipen,
>>>>
>>>> if you could give this patch a test and possibly ack it for me to take
>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>> so with it we could remove it entirely for v6.7.
>>>
>>> Progress so far for the RFT...
>>>
>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>> some patches I needed to manually apply and correct. With all this, it failed
>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>> compile. I thought I should let you know this part.
>>>
>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>> roughly around this place [1]. I thought it would be your patch series so
>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>> same place. I have to trace back now from which kernel version it broke.
>>
>> [1].
>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>
>> of course with your patches it would fail for the gdev instead of the chip.
> 
> Small update:
> 
> I put some debugging prints in the gpio match function in the hte-tegra194.c as
> below:
> 
> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>  {
> +       struct device_node *node = data;
> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> +       if (!fw || !chip->fwnode)
> +               pr_err("dipen patel: fw is null\n");
> 
> -       pr_err("%s:%d\n", __func__, __LINE__);
> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> fw), fw->dev->init_name);
>         return chip->fwnode == of_node_to_fwnode(data);
>  }
> 
> The output of the printfs looks like below:
> [    3.955194] dipen patel: fw is null -----> this message started appearing
> when I added !chip->fwnode test in the if condition line.
> 
> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> gpio@c2f0000, match?:0, fwnode name:(null)
> 
> I conclude that chip->fwnode is empty. Any idea in which conditions that node
> would be empty?

sorry for spamming, one last message before I sign off for the day....

Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
was able to verify your patch series.

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index d87dd06db40d..a56c159d7136 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
                offset += port->pins;
        }

+       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
+
        return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
 }

Now, few follow up questions:
1) is this the correct way of setting the chip fwnode in the gpio driver?
2) Or should I use something else in hte matching function instead of fwnode so
to avoid adding above line in the gpio driver?

> 
>>>
>>>>
>>>> Bart
>>>
>>
> 

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Bartosz Golaszewski]

On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>
> On 10/4/23 3:54 PM, Dipen Patel wrote:
> > On 10/4/23 1:33 PM, Dipen Patel wrote:
> >> On 10/4/23 1:30 PM, Dipen Patel wrote:
> >>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> >>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>
> >>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>>>
> >>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>
> >>>>>> Using struct gpio_chip is not safe as it will disappear if the
> >>>>>> underlying driver is unbound for any reason. Switch to using reference
> >>>>>> counted struct gpio_device and its dedicated accessors.
> >>>>>>
> >>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>
> >>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
> >>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>
> >>>>> I think this can be merged into the gpio tree after leaving some
> >>>>> slack for the HTE maintainer to look at it, things look so much
> >>>>> better after this.
> >>>>>
> >>>>> Yours,
> >>>>> Linus Walleij
> >>>>
> >>>> Dipen,
> >>>>
> >>>> if you could give this patch a test and possibly ack it for me to take
> >>>> it through the GPIO tree (or go the immutable tag from HTE route) then
> >>>> it would be great. This is the last user of gpiochip_find() treewide,
> >>>> so with it we could remove it entirely for v6.7.
> >>>
> >>> Progress so far for the RFT...
> >>>
> >>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> >>> some patches I needed to manually apply and correct. With all this, it failed
> >>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> >>> compile. I thought I should let you know this part.
> >>>
> >>> Now, I tried to test the hte and it seems to fail finding the gpio device,
> >>> roughly around this place [1]. I thought it would be your patch series so
> >>> tried to just use 6.6rc1 without your patches and it still failed at the
> >>> same place. I have to trace back now from which kernel version it broke.
> >>
> >> [1].
> >> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> >>
> >> of course with your patches it would fail for the gdev instead of the chip.
> >
> > Small update:
> >
> > I put some debugging prints in the gpio match function in the hte-tegra194.c as
> > below:
> >
> > static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
> >  {
> > +       struct device_node *node = data;
> > +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> > +       if (!fw || !chip->fwnode)
> > +               pr_err("dipen patel: fw is null\n");
> >
> > -       pr_err("%s:%d\n", __func__, __LINE__);
> > +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> > __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> > fw), fw->dev->init_name);
> >         return chip->fwnode == of_node_to_fwnode(data);
> >  }
> >
> > The output of the printfs looks like below:
> > [    3.955194] dipen patel: fw is null -----> this message started appearing
> > when I added !chip->fwnode test in the if condition line.
> >
> > [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> > gpio@c2f0000, match?:0, fwnode name:(null)
> >
> > I conclude that chip->fwnode is empty. Any idea in which conditions that node
> > would be empty?
>
> sorry for spamming, one last message before I sign off for the day....
>
> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
> was able to verify your patch series.
>
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index d87dd06db40d..a56c159d7136 100644
> --- a/drivers/gpio/gpio-tegra186.c
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>                 offset += port->pins;
>         }
>
> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +
>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>  }
>
> Now, few follow up questions:
> 1) is this the correct way of setting the chip fwnode in the gpio driver?

You shouldn't need this. This driver already does:

    gpio->gpio.parent = &pdev->dev;

so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
check why this doesn't happen?

Bart

> 2) Or should I use something else in hte matching function instead of fwnode so
> to avoid adding above line in the gpio driver?
>
> >
> >>>
> >>>>
> >>>> Bart
> >>>
> >>
> >
>

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Dipen Patel]

On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>>
>> On 10/4/23 3:54 PM, Dipen Patel wrote:
>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>
>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>>>
>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>
>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>>>
>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>
>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>
>>>>>>> I think this can be merged into the gpio tree after leaving some
>>>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>>>> better after this.
>>>>>>>
>>>>>>> Yours,
>>>>>>> Linus Walleij
>>>>>>
>>>>>> Dipen,
>>>>>>
>>>>>> if you could give this patch a test and possibly ack it for me to take
>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>>>> so with it we could remove it entirely for v6.7.
>>>>>
>>>>> Progress so far for the RFT...
>>>>>
>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>>>> some patches I needed to manually apply and correct. With all this, it failed
>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>>>> compile. I thought I should let you know this part.
>>>>>
>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>>>> roughly around this place [1]. I thought it would be your patch series so
>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>>>> same place. I have to trace back now from which kernel version it broke.
>>>>
>>>> [1].
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>>>
>>>> of course with your patches it would fail for the gdev instead of the chip.
>>>
>>> Small update:
>>>
>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
>>> below:
>>>
>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>>>  {
>>> +       struct device_node *node = data;
>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
>>> +       if (!fw || !chip->fwnode)
>>> +               pr_err("dipen patel: fw is null\n");
>>>
>>> -       pr_err("%s:%d\n", __func__, __LINE__);
>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
>>> fw), fw->dev->init_name);
>>>         return chip->fwnode == of_node_to_fwnode(data);
>>>  }
>>>
>>> The output of the printfs looks like below:
>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
>>> when I added !chip->fwnode test in the if condition line.
>>>
>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
>>> gpio@c2f0000, match?:0, fwnode name:(null)
>>>
>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
>>> would be empty?
>>
>> sorry for spamming, one last message before I sign off for the day....
>>
>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
>> was able to verify your patch series.
>>
>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
>> index d87dd06db40d..a56c159d7136 100644
>> --- a/drivers/gpio/gpio-tegra186.c
>> +++ b/drivers/gpio/gpio-tegra186.c
>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>>                 offset += port->pins;
>>         }
>>
>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
>> +
>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>>  }
>>
>> Now, few follow up questions:
>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
> 
> You shouldn't need this. This driver already does:
> 
>     gpio->gpio.parent = &pdev->dev;
> 
> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
> check why this doesn't happen?

I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
The only reference I see is here [1]. Does it mean I need to change my match
function from:

chip->fwnode == of_node_to_fwnode(data)

to:
dev_fwnode(chip->parent) == of_node_to_fwnode(data)?

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767

> 
> Bart
> 
>> 2) Or should I use something else in hte matching function instead of fwnode so
>> to avoid adding above line in the gpio driver?
>>
>>>
>>>>>
>>>>>>
>>>>>> Bart
>>>>>
>>>>
>>>
>>

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Bartosz Golaszewski]

On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
>
> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
> > On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
> >>
> >> On 10/4/23 3:54 PM, Dipen Patel wrote:
> >>> On 10/4/23 1:33 PM, Dipen Patel wrote:
> >>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
> >>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> >>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>>>
> >>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>>>>>
> >>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>
> >>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
> >>>>>>>> underlying driver is unbound for any reason. Switch to using reference
> >>>>>>>> counted struct gpio_device and its dedicated accessors.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>
> >>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
> >>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>>>
> >>>>>>> I think this can be merged into the gpio tree after leaving some
> >>>>>>> slack for the HTE maintainer to look at it, things look so much
> >>>>>>> better after this.
> >>>>>>>
> >>>>>>> Yours,
> >>>>>>> Linus Walleij
> >>>>>>
> >>>>>> Dipen,
> >>>>>>
> >>>>>> if you could give this patch a test and possibly ack it for me to take
> >>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
> >>>>>> it would be great. This is the last user of gpiochip_find() treewide,
> >>>>>> so with it we could remove it entirely for v6.7.
> >>>>>
> >>>>> Progress so far for the RFT...
> >>>>>
> >>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> >>>>> some patches I needed to manually apply and correct. With all this, it failed
> >>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> >>>>> compile. I thought I should let you know this part.
> >>>>>
> >>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
> >>>>> roughly around this place [1]. I thought it would be your patch series so
> >>>>> tried to just use 6.6rc1 without your patches and it still failed at the
> >>>>> same place. I have to trace back now from which kernel version it broke.
> >>>>
> >>>> [1].
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> >>>>
> >>>> of course with your patches it would fail for the gdev instead of the chip.
> >>>
> >>> Small update:
> >>>
> >>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
> >>> below:
> >>>
> >>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
> >>>  {
> >>> +       struct device_node *node = data;
> >>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> >>> +       if (!fw || !chip->fwnode)
> >>> +               pr_err("dipen patel: fw is null\n");
> >>>
> >>> -       pr_err("%s:%d\n", __func__, __LINE__);
> >>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> >>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> >>> fw), fw->dev->init_name);
> >>>         return chip->fwnode == of_node_to_fwnode(data);
> >>>  }
> >>>
> >>> The output of the printfs looks like below:
> >>> [    3.955194] dipen patel: fw is null -----> this message started appearing
> >>> when I added !chip->fwnode test in the if condition line.
> >>>
> >>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> >>> gpio@c2f0000, match?:0, fwnode name:(null)
> >>>
> >>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
> >>> would be empty?
> >>
> >> sorry for spamming, one last message before I sign off for the day....
> >>
> >> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
> >> was able to verify your patch series.
> >>
> >> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> >> index d87dd06db40d..a56c159d7136 100644
> >> --- a/drivers/gpio/gpio-tegra186.c
> >> +++ b/drivers/gpio/gpio-tegra186.c
> >> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
> >>                 offset += port->pins;
> >>         }
> >>
> >> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
> >> +
> >>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
> >>  }
> >>
> >> Now, few follow up questions:
> >> 1) is this the correct way of setting the chip fwnode in the gpio driver?
> >
> > You shouldn't need this. This driver already does:
> >
> >     gpio->gpio.parent = &pdev->dev;
> >
> > so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
> > check why this doesn't happen?
>
> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
> The only reference I see is here [1]. Does it mean I need to change my match
> function from:
>
> chip->fwnode == of_node_to_fwnode(data)
>
> to:
> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?

No! chip->fwnode is only used to let GPIOLIB know which fwnode to
assign to the GPIO device (struct gpio_device).

Bart

>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
>
> >
> > Bart
> >
> >> 2) Or should I use something else in hte matching function instead of fwnode so
> >> to avoid adding above line in the gpio driver?
> >>
> >>>
> >>>>>
> >>>>>>
> >>>>>> Bart
> >>>>>
> >>>>
> >>>
> >>
>

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Dipen Patel]

On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
> On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>
>> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
>>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>
>>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
>>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
>>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>>>>>
>>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>
>>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>
>>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>>>
>>>>>>>>> I think this can be merged into the gpio tree after leaving some
>>>>>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>>>>>> better after this.
>>>>>>>>>
>>>>>>>>> Yours,
>>>>>>>>> Linus Walleij
>>>>>>>>
>>>>>>>> Dipen,
>>>>>>>>
>>>>>>>> if you could give this patch a test and possibly ack it for me to take
>>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>>>>>> so with it we could remove it entirely for v6.7.
>>>>>>>
>>>>>>> Progress so far for the RFT...
>>>>>>>
>>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>>>>>> some patches I needed to manually apply and correct. With all this, it failed
>>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>>>>>> compile. I thought I should let you know this part.
>>>>>>>
>>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>>>>>> roughly around this place [1]. I thought it would be your patch series so
>>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>>>>>> same place. I have to trace back now from which kernel version it broke.
>>>>>>
>>>>>> [1].
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>>>>>
>>>>>> of course with your patches it would fail for the gdev instead of the chip.
>>>>>
>>>>> Small update:
>>>>>
>>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
>>>>> below:
>>>>>
>>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>>>>>  {
>>>>> +       struct device_node *node = data;
>>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
>>>>> +       if (!fw || !chip->fwnode)
>>>>> +               pr_err("dipen patel: fw is null\n");
>>>>>
>>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
>>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
>>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
>>>>> fw), fw->dev->init_name);
>>>>>         return chip->fwnode == of_node_to_fwnode(data);
>>>>>  }
>>>>>
>>>>> The output of the printfs looks like below:
>>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
>>>>> when I added !chip->fwnode test in the if condition line.
>>>>>
>>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
>>>>> gpio@c2f0000, match?:0, fwnode name:(null)
>>>>>
>>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
>>>>> would be empty?
>>>>
>>>> sorry for spamming, one last message before I sign off for the day....
>>>>
>>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
>>>> was able to verify your patch series.
>>>>
>>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
>>>> index d87dd06db40d..a56c159d7136 100644
>>>> --- a/drivers/gpio/gpio-tegra186.c
>>>> +++ b/drivers/gpio/gpio-tegra186.c
>>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>>>>                 offset += port->pins;
>>>>         }
>>>>
>>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
>>>> +
>>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>>>>  }
>>>>
>>>> Now, few follow up questions:
>>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
>>>
>>> You shouldn't need this. This driver already does:
>>>
>>>     gpio->gpio.parent = &pdev->dev;
>>>
>>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
>>> check why this doesn't happen?
>>
>> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
>> The only reference I see is here [1]. Does it mean I need to change my match
>> function from:
>>
>> chip->fwnode == of_node_to_fwnode(data)
>>
>> to:
>> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
> 
> No! chip->fwnode is only used to let GPIOLIB know which fwnode to
> assign to the GPIO device (struct gpio_device).
What do you suggest I should use for the match as I do not see chip->fwnode
being set?

> 
> Bart
> 
>>
>> [1]:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
>>
>>>
>>> Bart
>>>
>>>> 2) Or should I use something else in hte matching function instead of fwnode so
>>>> to avoid adding above line in the gpio driver?
>>>>
>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Bart
>>>>>>>
>>>>>>
>>>>>
>>>>
>>

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Bartosz Golaszewski]

On Thu, Oct 5, 2023 at 9:43 PM Dipen Patel <dipenp@nvidia.com> wrote:
>
> On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
> > On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
> >>
> >> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
> >>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
> >>>>
> >>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
> >>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
> >>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
> >>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> >>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>>>>>
> >>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>>>>>>>
> >>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>>>
> >>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
> >>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
> >>>>>>>>>> counted struct gpio_device and its dedicated accessors.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>>
> >>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
> >>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>>>>>
> >>>>>>>>> I think this can be merged into the gpio tree after leaving some
> >>>>>>>>> slack for the HTE maintainer to look at it, things look so much
> >>>>>>>>> better after this.
> >>>>>>>>>
> >>>>>>>>> Yours,
> >>>>>>>>> Linus Walleij
> >>>>>>>>
> >>>>>>>> Dipen,
> >>>>>>>>
> >>>>>>>> if you could give this patch a test and possibly ack it for me to take
> >>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
> >>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
> >>>>>>>> so with it we could remove it entirely for v6.7.
> >>>>>>>
> >>>>>>> Progress so far for the RFT...
> >>>>>>>
> >>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> >>>>>>> some patches I needed to manually apply and correct. With all this, it failed
> >>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> >>>>>>> compile. I thought I should let you know this part.
> >>>>>>>
> >>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
> >>>>>>> roughly around this place [1]. I thought it would be your patch series so
> >>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
> >>>>>>> same place. I have to trace back now from which kernel version it broke.
> >>>>>>
> >>>>>> [1].
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> >>>>>>
> >>>>>> of course with your patches it would fail for the gdev instead of the chip.
> >>>>>
> >>>>> Small update:
> >>>>>
> >>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
> >>>>> below:
> >>>>>
> >>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
> >>>>>  {
> >>>>> +       struct device_node *node = data;
> >>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> >>>>> +       if (!fw || !chip->fwnode)
> >>>>> +               pr_err("dipen patel: fw is null\n");
> >>>>>
> >>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
> >>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> >>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> >>>>> fw), fw->dev->init_name);
> >>>>>         return chip->fwnode == of_node_to_fwnode(data);
> >>>>>  }
> >>>>>
> >>>>> The output of the printfs looks like below:
> >>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
> >>>>> when I added !chip->fwnode test in the if condition line.
> >>>>>
> >>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> >>>>> gpio@c2f0000, match?:0, fwnode name:(null)
> >>>>>
> >>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
> >>>>> would be empty?
> >>>>
> >>>> sorry for spamming, one last message before I sign off for the day....
> >>>>
> >>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
> >>>> was able to verify your patch series.
> >>>>
> >>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> >>>> index d87dd06db40d..a56c159d7136 100644
> >>>> --- a/drivers/gpio/gpio-tegra186.c
> >>>> +++ b/drivers/gpio/gpio-tegra186.c
> >>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
> >>>>                 offset += port->pins;
> >>>>         }
> >>>>
> >>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
> >>>> +
> >>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
> >>>>  }
> >>>>
> >>>> Now, few follow up questions:
> >>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
> >>>
> >>> You shouldn't need this. This driver already does:
> >>>
> >>>     gpio->gpio.parent = &pdev->dev;
> >>>
> >>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
> >>> check why this doesn't happen?
> >>
> >> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
> >> The only reference I see is here [1]. Does it mean I need to change my match
> >> function from:
> >>
> >> chip->fwnode == of_node_to_fwnode(data)
> >>
> >> to:
> >> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
> >
> > No! chip->fwnode is only used to let GPIOLIB know which fwnode to
> > assign to the GPIO device (struct gpio_device).
> What do you suggest I should use for the match as I do not see chip->fwnode
> being set?
>

Andy, Linus,

Do you think it makes sense to make gpiochip_add_data_with_key()
assign the chip's fwnode if it's not set by the caller (and instead
taken from the parent device) for this particular use-case?

I think it's fine but wanted to run it by you.

Bart

> >
> > Bart
> >
> >>
> >> [1]:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
> >>
> >>>
> >>> Bart
> >>>
> >>>> 2) Or should I use something else in hte matching function instead of fwnode so
> >>>> to avoid adding above line in the gpio driver?
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Bart
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
>

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Bartosz Golaszewski]

On Thu, Oct 5, 2023 at 9:43 PM Dipen Patel <dipenp@nvidia.com> wrote:
>
> On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
> > On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
> >>
> >> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
> >>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
> >>>>
> >>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
> >>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
> >>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
> >>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> >>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>>>>>
> >>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>>>>>>>
> >>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>>>
> >>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
> >>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
> >>>>>>>>>> counted struct gpio_device and its dedicated accessors.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>>
> >>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
> >>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>>>>>
> >>>>>>>>> I think this can be merged into the gpio tree after leaving some
> >>>>>>>>> slack for the HTE maintainer to look at it, things look so much
> >>>>>>>>> better after this.
> >>>>>>>>>
> >>>>>>>>> Yours,
> >>>>>>>>> Linus Walleij
> >>>>>>>>
> >>>>>>>> Dipen,
> >>>>>>>>
> >>>>>>>> if you could give this patch a test and possibly ack it for me to take
> >>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
> >>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
> >>>>>>>> so with it we could remove it entirely for v6.7.
> >>>>>>>
> >>>>>>> Progress so far for the RFT...
> >>>>>>>
> >>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> >>>>>>> some patches I needed to manually apply and correct. With all this, it failed
> >>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> >>>>>>> compile. I thought I should let you know this part.
> >>>>>>>
> >>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
> >>>>>>> roughly around this place [1]. I thought it would be your patch series so
> >>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
> >>>>>>> same place. I have to trace back now from which kernel version it broke.
> >>>>>>
> >>>>>> [1].
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> >>>>>>
> >>>>>> of course with your patches it would fail for the gdev instead of the chip.
> >>>>>
> >>>>> Small update:
> >>>>>
> >>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
> >>>>> below:
> >>>>>
> >>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
> >>>>>  {
> >>>>> +       struct device_node *node = data;
> >>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> >>>>> +       if (!fw || !chip->fwnode)
> >>>>> +               pr_err("dipen patel: fw is null\n");
> >>>>>
> >>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
> >>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> >>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> >>>>> fw), fw->dev->init_name);
> >>>>>         return chip->fwnode == of_node_to_fwnode(data);
> >>>>>  }
> >>>>>
> >>>>> The output of the printfs looks like below:
> >>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
> >>>>> when I added !chip->fwnode test in the if condition line.
> >>>>>
> >>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> >>>>> gpio@c2f0000, match?:0, fwnode name:(null)
> >>>>>
> >>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
> >>>>> would be empty?
> >>>>
> >>>> sorry for spamming, one last message before I sign off for the day....
> >>>>
> >>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
> >>>> was able to verify your patch series.
> >>>>
> >>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> >>>> index d87dd06db40d..a56c159d7136 100644
> >>>> --- a/drivers/gpio/gpio-tegra186.c
> >>>> +++ b/drivers/gpio/gpio-tegra186.c
> >>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
> >>>>                 offset += port->pins;
> >>>>         }
> >>>>
> >>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
> >>>> +
> >>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
> >>>>  }
> >>>>
> >>>> Now, few follow up questions:
> >>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
> >>>
> >>> You shouldn't need this. This driver already does:
> >>>
> >>>     gpio->gpio.parent = &pdev->dev;
> >>>
> >>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
> >>> check why this doesn't happen?
> >>
> >> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
> >> The only reference I see is here [1]. Does it mean I need to change my match
> >> function from:
> >>
> >> chip->fwnode == of_node_to_fwnode(data)
> >>
> >> to:
> >> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
> >
> > No! chip->fwnode is only used to let GPIOLIB know which fwnode to
> > assign to the GPIO device (struct gpio_device).
> What do you suggest I should use for the match as I do not see chip->fwnode
> being set?
>

This is most likely going to be a longer discussion. I suggest that in
the meantime you just assign the gc->fwnode pointer explicitly from
the platform device in the tegra GPIO driver and use it in the lookup
function. Note that this is NOT wrong or a hack. It's just that most
devices don't need to be looked up using gpio_device_find().

Bart

> >
> > Bart
> >
> >>
> >> [1]:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
> >>
> >>>
> >>> Bart
> >>>
> >>>> 2) Or should I use something else in hte matching function instead of fwnode so
> >>>> to avoid adding above line in the gpio driver?
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Bart
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
>

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Dipen Patel]

On 10/8/23 11:48 PM, Bartosz Golaszewski wrote:
> On Thu, Oct 5, 2023 at 9:43 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>
>> On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
>>> On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>
>>>> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
>>>>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>>>
>>>>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
>>>>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
>>>>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>>>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>>>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>>
>>>>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>>>>>
>>>>>>>>>>> I think this can be merged into the gpio tree after leaving some
>>>>>>>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>>>>>>>> better after this.
>>>>>>>>>>>
>>>>>>>>>>> Yours,
>>>>>>>>>>> Linus Walleij
>>>>>>>>>>
>>>>>>>>>> Dipen,
>>>>>>>>>>
>>>>>>>>>> if you could give this patch a test and possibly ack it for me to take
>>>>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>>>>>>>> so with it we could remove it entirely for v6.7.
>>>>>>>>>
>>>>>>>>> Progress so far for the RFT...
>>>>>>>>>
>>>>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>>>>>>>> some patches I needed to manually apply and correct. With all this, it failed
>>>>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>>>>>>>> compile. I thought I should let you know this part.
>>>>>>>>>
>>>>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>>>>>>>> roughly around this place [1]. I thought it would be your patch series so
>>>>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>>>>>>>> same place. I have to trace back now from which kernel version it broke.
>>>>>>>>
>>>>>>>> [1].
>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>>>>>>>
>>>>>>>> of course with your patches it would fail for the gdev instead of the chip.
>>>>>>>
>>>>>>> Small update:
>>>>>>>
>>>>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
>>>>>>> below:
>>>>>>>
>>>>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>>>>>>>  {
>>>>>>> +       struct device_node *node = data;
>>>>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
>>>>>>> +       if (!fw || !chip->fwnode)
>>>>>>> +               pr_err("dipen patel: fw is null\n");
>>>>>>>
>>>>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
>>>>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
>>>>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
>>>>>>> fw), fw->dev->init_name);
>>>>>>>         return chip->fwnode == of_node_to_fwnode(data);
>>>>>>>  }
>>>>>>>
>>>>>>> The output of the printfs looks like below:
>>>>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
>>>>>>> when I added !chip->fwnode test in the if condition line.
>>>>>>>
>>>>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
>>>>>>> gpio@c2f0000, match?:0, fwnode name:(null)
>>>>>>>
>>>>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
>>>>>>> would be empty?
>>>>>>
>>>>>> sorry for spamming, one last message before I sign off for the day....
>>>>>>
>>>>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
>>>>>> was able to verify your patch series.
>>>>>>
>>>>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
>>>>>> index d87dd06db40d..a56c159d7136 100644
>>>>>> --- a/drivers/gpio/gpio-tegra186.c
>>>>>> +++ b/drivers/gpio/gpio-tegra186.c
>>>>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>>>>>>                 offset += port->pins;
>>>>>>         }
>>>>>>
>>>>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
>>>>>> +
>>>>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>>>>>>  }
>>>>>>
>>>>>> Now, few follow up questions:
>>>>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
>>>>>
>>>>> You shouldn't need this. This driver already does:
>>>>>
>>>>>     gpio->gpio.parent = &pdev->dev;
>>>>>
>>>>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
>>>>> check why this doesn't happen?
>>>>
>>>> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
>>>> The only reference I see is here [1]. Does it mean I need to change my match
>>>> function from:
>>>>
>>>> chip->fwnode == of_node_to_fwnode(data)
>>>>
>>>> to:
>>>> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
>>>
>>> No! chip->fwnode is only used to let GPIOLIB know which fwnode to
>>> assign to the GPIO device (struct gpio_device).
>> What do you suggest I should use for the match as I do not see chip->fwnode
>> being set?
>>
> 
> This is most likely going to be a longer discussion. I suggest that in
> the meantime you just assign the gc->fwnode pointer explicitly from
> the platform device in the tegra GPIO driver and use it in the lookup
> function. Note that this is NOT wrong or a hack. It's just that most
> devices don't need to be looked up using gpio_device_find().

Sure, at the same time, I am also find to use any other method/s.
> 
> Bart
> 
>>>
>>> Bart
>>>
>>>>
>>>> [1]:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
>>>>
>>>>>
>>>>> Bart
>>>>>
>>>>>> 2) Or should I use something else in hte matching function instead of fwnode so
>>>>>> to avoid adding above line in the gpio driver?
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Bart
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

Re: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip

[Dipen Patel]

On 10/9/23 9:34 AM, Dipen Patel wrote:
> On 10/8/23 11:48 PM, Bartosz Golaszewski wrote:
>> On Thu, Oct 5, 2023 at 9:43 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>>
>>> On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
>>>> On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>>
>>>>> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
>>>>>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>>>>
>>>>>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
>>>>>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
>>>>>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>>>>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>>>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>>>>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>> I think this can be merged into the gpio tree after leaving some
>>>>>>>>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>>>>>>>>> better after this.
>>>>>>>>>>>>
>>>>>>>>>>>> Yours,
>>>>>>>>>>>> Linus Walleij
>>>>>>>>>>>
>>>>>>>>>>> Dipen,
>>>>>>>>>>>
>>>>>>>>>>> if you could give this patch a test and possibly ack it for me to take
>>>>>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>>>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>>>>>>>>> so with it we could remove it entirely for v6.7.
>>>>>>>>>>
>>>>>>>>>> Progress so far for the RFT...
>>>>>>>>>>
>>>>>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>>>>>>>>> some patches I needed to manually apply and correct. With all this, it failed
>>>>>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>>>>>>>>> compile. I thought I should let you know this part.
>>>>>>>>>>
>>>>>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>>>>>>>>> roughly around this place [1]. I thought it would be your patch series so
>>>>>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>>>>>>>>> same place. I have to trace back now from which kernel version it broke.
>>>>>>>>>
>>>>>>>>> [1].
>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>>>>>>>>
>>>>>>>>> of course with your patches it would fail for the gdev instead of the chip.
>>>>>>>>
>>>>>>>> Small update:
>>>>>>>>
>>>>>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
>>>>>>>> below:
>>>>>>>>
>>>>>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>>>>>>>>  {
>>>>>>>> +       struct device_node *node = data;
>>>>>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
>>>>>>>> +       if (!fw || !chip->fwnode)
>>>>>>>> +               pr_err("dipen patel: fw is null\n");
>>>>>>>>
>>>>>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
>>>>>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
>>>>>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
>>>>>>>> fw), fw->dev->init_name);
>>>>>>>>         return chip->fwnode == of_node_to_fwnode(data);
>>>>>>>>  }
>>>>>>>>
>>>>>>>> The output of the printfs looks like below:
>>>>>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
>>>>>>>> when I added !chip->fwnode test in the if condition line.
>>>>>>>>
>>>>>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
>>>>>>>> gpio@c2f0000, match?:0, fwnode name:(null)
>>>>>>>>
>>>>>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
>>>>>>>> would be empty?
>>>>>>>
>>>>>>> sorry for spamming, one last message before I sign off for the day....
>>>>>>>
>>>>>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
>>>>>>> was able to verify your patch series.
>>>>>>>
>>>>>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
>>>>>>> index d87dd06db40d..a56c159d7136 100644
>>>>>>> --- a/drivers/gpio/gpio-tegra186.c
>>>>>>> +++ b/drivers/gpio/gpio-tegra186.c
>>>>>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>>>>>>>                 offset += port->pins;
>>>>>>>         }
>>>>>>>
>>>>>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
>>>>>>> +
>>>>>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>>>>>>>  }
>>>>>>>
>>>>>>> Now, few follow up questions:
>>>>>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
>>>>>>
>>>>>> You shouldn't need this. This driver already does:
>>>>>>
>>>>>>     gpio->gpio.parent = &pdev->dev;
>>>>>>
>>>>>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
>>>>>> check why this doesn't happen?
>>>>>
>>>>> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
>>>>> The only reference I see is here [1]. Does it mean I need to change my match
>>>>> function from:
>>>>>
>>>>> chip->fwnode == of_node_to_fwnode(data)
>>>>>
>>>>> to:
>>>>> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
>>>>
>>>> No! chip->fwnode is only used to let GPIOLIB know which fwnode to
>>>> assign to the GPIO device (struct gpio_device).
>>> What do you suggest I should use for the match as I do not see chip->fwnode
>>> being set?
>>>
>>
>> This is most likely going to be a longer discussion. I suggest that in
>> the meantime you just assign the gc->fwnode pointer explicitly from
>> the platform device in the tegra GPIO driver and use it in the lookup
>> function. Note that this is NOT wrong or a hack. It's just that most
>> devices don't need to be looked up using gpio_device_find().
> 
> Sure, at the same time, I am also find to use any other method/s.

(Correction) I am also fine*

With patch
https://patchwork.ozlabs.org/project/linux-gpio/patch/20231009173858.723686-1-dipenp@nvidia.com/

Tested-by: Dipen Patel <dipenp@nvidia.com>

>>
>> Bart
>>
>>>>
>>>> Bart
>>>>
>>>>>
>>>>> [1]:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
>>>>>
>>>>>>
>>>>>> Bart
>>>>>>
>>>>>>> 2) Or should I use something else in hte matching function instead of fwnode so
>>>>>>> to avoid adding above line in the gpio driver?
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Bart
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
> 

[RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Bartosz Golaszewski]

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

gpiochip_find() is going away as it's not hot-unplug safe. This platform
is not affected by any of the related problems as this GPIO controller
cannot really go away but in order to finally remove this function, we
need to convert it to using gpio_device_find() as well.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm/mach-omap1/board-ams-delta.c | 36 +++++++++++++--------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
index 9808cd27e2cf..a28ea6ac1eba 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -560,22 +560,6 @@ static struct gpiod_lookup_table *ams_delta_gpio_tables[] __initdata = {
 	&ams_delta_nand_gpio_table,
 };
 
-/*
- * Some drivers may not use GPIO lookup tables but need to be provided
- * with GPIO numbers.  The same applies to GPIO based IRQ lines - some
- * drivers may even not use GPIO layer but expect just IRQ numbers.
- * We could either define GPIO lookup tables then use them on behalf
- * of those devices, or we can use GPIO driver level methods for
- * identification of GPIO and IRQ numbers. For the purpose of the latter,
- * defina a helper function which identifies GPIO chips by their labels.
- */
-static int gpiochip_match_by_label(struct gpio_chip *chip, void *data)
-{
-	char *label = data;
-
-	return !strcmp(label, chip->label);
-}
-
 static struct gpiod_hog ams_delta_gpio_hogs[] = {
 	GPIO_HOG(LATCH2_LABEL, LATCH2_PIN_KEYBRD_DATAOUT, "keybrd_dataout",
 		 GPIO_ACTIVE_HIGH, GPIOD_OUT_LOW),
@@ -615,14 +599,28 @@ static void __init modem_assign_irq(struct gpio_chip *chip)
  */
 static void __init omap_gpio_deps_init(void)
 {
+	struct gpio_device *gdev;
 	struct gpio_chip *chip;
 
-	chip = gpiochip_find(OMAP_GPIO_LABEL, gpiochip_match_by_label);
-	if (!chip) {
-		pr_err("%s: OMAP GPIO chip not found\n", __func__);
+	/*
+	 * Some drivers may not use GPIO lookup tables but need to be provided
+	 * with GPIO numbers. The same applies to GPIO based IRQ lines - some
+	 * drivers may even not use GPIO layer but expect just IRQ numbers.
+	 * We could either define GPIO lookup tables then use them on behalf
+	 * of those devices, or we can use GPIO driver level methods for
+	 * identification of GPIO and IRQ numbers.
+	 *
+	 * This reference will be leaked but that's alright as this device
+	 * never goes down.
+	 */
+	gdev = gpio_device_find_by_label(OMAP_GPIO_LABEL);
+	if (!gdev) {
+		pr_err("%s: OMAP GPIO device not found\n", __func__);
 		return;
 	}
 
+	chip = gpio_device_get_chip(gdev);
+
 	/*
 	 * Start with FIQ initialization as it may have to request
 	 * and release successfully each OMAP GPIO pin in turn.
-- 
2.39.2

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:53:03PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> gpiochip_find() is going away as it's not hot-unplug safe. This platform
> is not affected by any of the related problems as this GPIO controller
> cannot really go away but in order to finally remove this function, we
> need to convert it to using gpio_device_find() as well.

Side question, have you used --patience when preparing this series?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Bartosz Golaszewski]

On Wed, Sep 6, 2023 at 4:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 05, 2023 at 08:53:03PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > is not affected by any of the related problems as this GPIO controller
> > cannot really go away but in order to finally remove this function, we
> > need to convert it to using gpio_device_find() as well.
>
> Side question, have you used --patience when preparing this series?
>

Yes! Thanks for bringing it to my attention.

Bart

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> gpiochip_find() is going away as it's not hot-unplug safe. This platform
> is not affected by any of the related problems as this GPIO controller
> cannot really go away but in order to finally remove this function, we
> need to convert it to using gpio_device_find() as well.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

I was cleaning this one just some merge cycle ago, now it
looks even better!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Janusz Krzysztofik]

Dnia czwartek, 7 września 2023 09:31:01 CEST Linus Walleij pisze:
> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > is not affected by any of the related problems as this GPIO controller
> > cannot really go away but in order to finally remove this function, we
> > need to convert it to using gpio_device_find() as well.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> I was cleaning this one just some merge cycle ago, now it
> looks even better!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Thanks,
Janusz

> 
> Yours,
> Linus Walleij
> 

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Bartosz Golaszewski]

On Fri, Sep 8, 2023 at 8:07 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
>
> Dnia czwartek, 7 września 2023 09:31:01 CEST Linus Walleij pisze:
> > On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > > is not affected by any of the related problems as this GPIO controller
> > > cannot really go away but in order to finally remove this function, we
> > > need to convert it to using gpio_device_find() as well.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > I was cleaning this one just some merge cycle ago, now it
> > looks even better!
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Acked-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>

Janusz,

Is it fine if I take it through the GPIO tree?

Bartosz

> Thanks,
> Janusz
>
> >
> > Yours,
> > Linus Walleij
> >
>
>
>
>

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Tony Lindgren]

* Bartosz Golaszewski <brgl@bgdev.pl> [230911 11:10]:
> On Fri, Sep 8, 2023 at 8:07 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> >
> > Dnia czwartek, 7 września 2023 09:31:01 CEST Linus Walleij pisze:
> > > On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > > > is not affected by any of the related problems as this GPIO controller
> > > > cannot really go away but in order to finally remove this function, we
> > > > need to convert it to using gpio_device_find() as well.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > I was cleaning this one just some merge cycle ago, now it
> > > looks even better!
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >
> > Acked-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> >
> 
> Janusz,
> 
> Is it fine if I take it through the GPIO tree?

Works for me at least:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Janusz Krzysztofik]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Hi Bartosz,

Dnia poniedziałek, 11 września 2023 13:09:56 CEST Bartosz Golaszewski pisze:
> On Fri, Sep 8, 2023 at 8:07 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> >
> > Dnia czwartek, 7 września 2023 09:31:01 CEST Linus Walleij pisze:
> > > On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > > > is not affected by any of the related problems as this GPIO controller
> > > > cannot really go away but in order to finally remove this function, we
> > > > need to convert it to using gpio_device_find() as well.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > I was cleaning this one just some merge cycle ago, now it
> > > looks even better!
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >
> > Acked-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> >
> 
> Janusz,
> 
> Is it fine if I take it through the GPIO tree?

Yes, should be fine, I believe.  Tony, Aaro, any doubts?

Thanks,
Janusz

> 
> Bartosz
> 
> > Thanks,
> > Janusz
> >
> > >
> > > Yours,
> > > Linus Walleij
> > >
> >
> >
> >
> >
> 


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Linus Walleij]

Oops one more note:

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> gpiochip_find() is going away as it's not hot-unplug safe. This platform
> is not affected by any of the related problems as this GPIO controller
> cannot really go away but in order to finally remove this function, we
> need to convert it to using gpio_device_find() as well.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
(...)
> +       struct gpio_device *gdev;
(...)
> +       gdev = gpio_device_find_by_label(OMAP_GPIO_LABEL);

This leaves a reference to the gdev right? No scoped guard?

If you leave a dangling reference intentionally I think it warrants
a comment ("leaving a ref here so the device will never be
free:ed").

Yours,
Linus Walleij

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Bartosz Golaszewski]

On Thu, Sep 7, 2023 at 9:35 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Oops one more note:
>
> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > is not affected by any of the related problems as this GPIO controller
> > cannot really go away but in order to finally remove this function, we
> > need to convert it to using gpio_device_find() as well.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> (...)
> > +       struct gpio_device *gdev;
> (...)
> > +       gdev = gpio_device_find_by_label(OMAP_GPIO_LABEL);
>
> This leaves a reference to the gdev right? No scoped guard?
>
> If you leave a dangling reference intentionally I think it warrants
> a comment ("leaving a ref here so the device will never be
> free:ed").
>

It's right there in the comment. :)

Bart

Re: [RFT PATCH 15/21] arm: omap1: ams-delta: stop using gpiochip_find()

[Bartosz Golaszewski]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> gpiochip_find() is going away as it's not hot-unplug safe. This platform
> is not affected by any of the related problems as this GPIO controller
> cannot really go away but in order to finally remove this function, we
> need to convert it to using gpio_device_find() as well.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm/mach-omap1/board-ams-delta.c | 36 +++++++++++++--------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
> index 9808cd27e2cf..a28ea6ac1eba 100644
> --- a/arch/arm/mach-omap1/board-ams-delta.c
> +++ b/arch/arm/mach-omap1/board-ams-delta.c
> @@ -560,22 +560,6 @@ static struct gpiod_lookup_table *ams_delta_gpio_tables[] __initdata = {
>         &ams_delta_nand_gpio_table,
>  };
>
> -/*
> - * Some drivers may not use GPIO lookup tables but need to be provided
> - * with GPIO numbers.  The same applies to GPIO based IRQ lines - some
> - * drivers may even not use GPIO layer but expect just IRQ numbers.
> - * We could either define GPIO lookup tables then use them on behalf
> - * of those devices, or we can use GPIO driver level methods for
> - * identification of GPIO and IRQ numbers. For the purpose of the latter,
> - * defina a helper function which identifies GPIO chips by their labels.
> - */
> -static int gpiochip_match_by_label(struct gpio_chip *chip, void *data)
> -{
> -       char *label = data;
> -
> -       return !strcmp(label, chip->label);
> -}
> -
>  static struct gpiod_hog ams_delta_gpio_hogs[] = {
>         GPIO_HOG(LATCH2_LABEL, LATCH2_PIN_KEYBRD_DATAOUT, "keybrd_dataout",
>                  GPIO_ACTIVE_HIGH, GPIOD_OUT_LOW),
> @@ -615,14 +599,28 @@ static void __init modem_assign_irq(struct gpio_chip *chip)
>   */
>  static void __init omap_gpio_deps_init(void)
>  {
> +       struct gpio_device *gdev;
>         struct gpio_chip *chip;
>
> -       chip = gpiochip_find(OMAP_GPIO_LABEL, gpiochip_match_by_label);
> -       if (!chip) {
> -               pr_err("%s: OMAP GPIO chip not found\n", __func__);
> +       /*
> +        * Some drivers may not use GPIO lookup tables but need to be provided
> +        * with GPIO numbers. The same applies to GPIO based IRQ lines - some
> +        * drivers may even not use GPIO layer but expect just IRQ numbers.
> +        * We could either define GPIO lookup tables then use them on behalf
> +        * of those devices, or we can use GPIO driver level methods for
> +        * identification of GPIO and IRQ numbers.
> +        *
> +        * This reference will be leaked but that's alright as this device
> +        * never goes down.
> +        */
> +       gdev = gpio_device_find_by_label(OMAP_GPIO_LABEL);
> +       if (!gdev) {
> +               pr_err("%s: OMAP GPIO device not found\n", __func__);
>                 return;
>         }
>
> +       chip = gpio_device_get_chip(gdev);
> +
>         /*
>          * Start with FIQ initialization as it may have to request
>          * and release successfully each OMAP GPIO pin in turn.
> --
> 2.39.2
>

Patch applied, thanks!

Bart

[PATCH 16/21] gpio: of: correct notifier return codes

[Bartosz Golaszewski]

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

According to the comments in linux/notifier.h, the code to return when a
notifications is "not for us" is NOTIFY_DONE, not NOTIFY_OK.

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

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 531faabead0f..5515f32cf19b 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -834,14 +834,14 @@ static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
 	switch (of_reconfig_get_state_change(action, arg)) {
 	case OF_RECONFIG_CHANGE_ADD:
 		if (!of_property_read_bool(rd->dn, "gpio-hog"))
-			return NOTIFY_OK;	/* not for us */
+			return NOTIFY_DONE;	/* not for us */
 
 		if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
-			return NOTIFY_OK;
+			return NOTIFY_DONE;
 
 		chip = of_find_gpiochip_by_node(rd->dn->parent);
 		if (chip == NULL)
-			return NOTIFY_OK;	/* not for us */
+			return NOTIFY_DONE;	/* not for us */
 
 		ret = of_gpiochip_add_hog(chip, rd->dn);
 		if (ret < 0) {
@@ -850,22 +850,22 @@ static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
 			of_node_clear_flag(rd->dn, OF_POPULATED);
 			return notifier_from_errno(ret);
 		}
-		break;
+		return NOTIFY_OK;
 
 	case OF_RECONFIG_CHANGE_REMOVE:
 		if (!of_node_check_flag(rd->dn, OF_POPULATED))
-			return NOTIFY_OK;	/* already depopulated */
+			return NOTIFY_DONE;	/* already depopulated */
 
 		chip = of_find_gpiochip_by_node(rd->dn->parent);
 		if (chip == NULL)
-			return NOTIFY_OK;	/* not for us */
+			return NOTIFY_DONE;	/* not for us */
 
 		of_gpiochip_remove_hog(chip, rd->dn);
 		of_node_clear_flag(rd->dn, OF_POPULATED);
-		break;
+		return NOTIFY_OK;
 	}
 
-	return NOTIFY_OK;
+	return NOTIFY_DONE;
 }
 
 struct notifier_block gpio_of_notifier = {
-- 
2.39.2

Re: [PATCH 16/21] gpio: of: correct notifier return codes

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> According to the comments in linux/notifier.h, the code to return when a
> notifications is "not for us" is NOTIFY_DONE, not NOTIFY_OK.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

This should just be applied, right? If the notifiers already
went upstream (sorry for my ignorance) then it should be
a Fixes: even.

Yours,
Linus Walleij

[PATCH 17/21] gpio: of: replace gpiochip_find_* with gpio_device_find_*

[Bartosz Golaszewski]

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

We're porting all users of gpiochip_find() to using gpio_device_find().
Update the OF GPIO code.

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

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 5515f32cf19b..9b087f93e049 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -127,10 +127,10 @@ static int of_gpiochip_match_node_and_xlate(struct gpio_chip *chip, void *data)
 				chip->of_xlate(chip, gpiospec, NULL) >= 0;
 }
 
-static struct gpio_chip *of_find_gpiochip_by_xlate(
-					struct of_phandle_args *gpiospec)
+static struct gpio_device *
+of_find_gpio_device_by_xlate(struct of_phandle_args *gpiospec)
 {
-	return gpiochip_find(gpiospec, of_gpiochip_match_node_and_xlate);
+	return gpio_device_find(gpiospec, of_gpiochip_match_node_and_xlate);
 }
 
 static struct gpio_desc *of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
@@ -362,8 +362,8 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 static struct gpio_desc *of_get_named_gpiod_flags(const struct device_node *np,
 		     const char *propname, int index, enum of_gpio_flags *flags)
 {
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
 	struct of_phandle_args gpiospec;
-	struct gpio_chip *chip;
 	struct gpio_desc *desc;
 	int ret;
 
@@ -375,13 +375,13 @@ static struct gpio_desc *of_get_named_gpiod_flags(const struct device_node *np,
 		return ERR_PTR(ret);
 	}
 
-	chip = of_find_gpiochip_by_xlate(&gpiospec);
-	if (!chip) {
+	gdev = of_find_gpio_device_by_xlate(&gpiospec);
+	if (!gdev) {
 		desc = ERR_PTR(-EPROBE_DEFER);
 		goto out;
 	}
 
-	desc = of_xlate_and_get_gpiod_flags(chip, &gpiospec, flags);
+	desc = of_xlate_and_get_gpiod_flags(gdev->chip, &gpiospec, flags);
 	if (IS_ERR(desc))
 		goto out;
 
@@ -813,16 +813,16 @@ static int of_gpiochip_match_node(struct gpio_chip *chip, void *data)
 	return device_match_of_node(&chip->gpiodev->dev, data);
 }
 
-static struct gpio_chip *of_find_gpiochip_by_node(struct device_node *np)
+static struct gpio_device *of_find_gpio_device_by_node(struct device_node *np)
 {
-	return gpiochip_find(np, of_gpiochip_match_node);
+	return gpio_device_find(np, of_gpiochip_match_node);
 }
 
 static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
 			  void *arg)
 {
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
 	struct of_reconfig_data *rd = arg;
-	struct gpio_chip *chip;
 	int ret;
 
 	/*
@@ -839,11 +839,11 @@ static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
 		if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
 			return NOTIFY_DONE;
 
-		chip = of_find_gpiochip_by_node(rd->dn->parent);
-		if (chip == NULL)
+		gdev = of_find_gpio_device_by_node(rd->dn->parent);
+		if (!gdev)
 			return NOTIFY_DONE;	/* not for us */
 
-		ret = of_gpiochip_add_hog(chip, rd->dn);
+		ret = of_gpiochip_add_hog(gdev->chip, rd->dn);
 		if (ret < 0) {
 			pr_err("%s: failed to add hogs for %pOF\n", __func__,
 			       rd->dn);
@@ -856,11 +856,11 @@ static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
 		if (!of_node_check_flag(rd->dn, OF_POPULATED))
 			return NOTIFY_DONE;	/* already depopulated */
 
-		chip = of_find_gpiochip_by_node(rd->dn->parent);
-		if (chip == NULL)
+		gdev = of_find_gpio_device_by_node(rd->dn->parent);
+		if (!gdev)
 			return NOTIFY_DONE;	/* not for us */
 
-		of_gpiochip_remove_hog(chip, rd->dn);
+		of_gpiochip_remove_hog(gdev->chip, rd->dn);
 		of_node_clear_flag(rd->dn, OF_POPULATED);
 		return NOTIFY_OK;
 	}
-- 
2.39.2

Re: [PATCH 17/21] gpio: of: replace gpiochip_find_* with gpio_device_find_*

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We're porting all users of gpiochip_find() to using gpio_device_find().
> Update the OF GPIO code.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 17/21] gpio: of: replace gpiochip_find_* with gpio_device_find_*

[Linus Walleij]

(Sorry for fatfinger reply)

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We're porting all users of gpiochip_find() to using gpio_device_find().
> Update the OF GPIO code.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Scoped guards, nice!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 18/21] gpio: acpi: replace gpiochip_find() with gpio_device_find()

[Bartosz Golaszewski]

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

We're porting all users of gpiochip_find() to using gpio_device_find().
Update the ACPI GPIO code.

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

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 5633e39396bc..15d3350123d9 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -143,7 +143,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
  */
 static struct gpio_desc *acpi_get_gpiod(char *path, unsigned int pin)
 {
-	struct gpio_chip *chip;
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
 	acpi_handle handle;
 	acpi_status status;
 
@@ -151,11 +151,11 @@ static struct gpio_desc *acpi_get_gpiod(char *path, unsigned int pin)
 	if (ACPI_FAILURE(status))
 		return ERR_PTR(-ENODEV);
 
-	chip = gpiochip_find(handle, acpi_gpiochip_find);
-	if (!chip)
+	gdev = gpio_device_find(handle, acpi_gpiochip_find);
+	if (!gdev)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	return gpiochip_get_desc(chip, pin);
+	return gpiochip_get_desc(gdev->chip, pin);
 }
 
 /**
-- 
2.39.2

Re: [PATCH 18/21] gpio: acpi: replace gpiochip_find() with gpio_device_find()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:53:06PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're porting all users of gpiochip_find() to using gpio_device_find().
> Update the ACPI GPIO code.

...

> -	return gpiochip_get_desc(chip, pin);
> +	return gpiochip_get_desc(gdev->chip, pin);

gpio_device_get_desc() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 18/21] gpio: acpi: replace gpiochip_find() with gpio_device_find()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We're porting all users of gpiochip_find() to using gpio_device_find().
> Update the ACPI GPIO code.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With Andy's comment addressed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

[Bartosz Golaszewski]

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

We're porting all users of gpiochip_find() to using gpio_device_find().
Update the swnode GPIO code.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index b5a6eaf3729b..56c8519be538 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -31,31 +31,26 @@ static void swnode_format_propname(const char *con_id, char *propname,
 		strscpy(propname, "gpios", max_size);
 }
 
-static int swnode_gpiochip_match_name(struct gpio_chip *chip, void *data)
+static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
 {
-	return !strcmp(chip->label, data);
-}
+	const struct software_node *gdev_node;
+	struct gpio_device *gdev;
 
-static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
-{
-	const struct software_node *chip_node;
-	struct gpio_chip *chip;
-
-	chip_node = to_software_node(fwnode);
-	if (!chip_node || !chip_node->name)
+	gdev_node = to_software_node(fwnode);
+	if (!gdev_node || !gdev_node->name)
 		return ERR_PTR(-EINVAL);
 
-	chip = gpiochip_find((void *)chip_node->name, swnode_gpiochip_match_name);
-	return chip ?: ERR_PTR(-EPROBE_DEFER);
+	gdev = gpio_device_find_by_label((void *)gdev_node->name);
+	return gdev ?: ERR_PTR(-EPROBE_DEFER);
 }
 
 struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
 				   const char *con_id, unsigned int idx,
 				   unsigned long *flags)
 {
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
 	const struct software_node *swnode;
 	struct fwnode_reference_args args;
-	struct gpio_chip *chip;
 	struct gpio_desc *desc;
 	char propname[32]; /* 32 is max size of property name */
 	int error;
@@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
 		return ERR_PTR(error);
 	}
 
-	chip = swnode_get_chip(args.fwnode);
+	gdev = swnode_get_gpio_device(args.fwnode);
 	fwnode_handle_put(args.fwnode);
-	if (IS_ERR(chip))
-		return ERR_CAST(chip);
+	if (IS_ERR(gdev))
+		return ERR_CAST(gdev);
 
-	desc = gpiochip_get_desc(chip, args.args[0]);
+	desc = gpiochip_get_desc(gdev->chip, args.args[0]);
 	*flags = args.args[1]; /* We expect native GPIO flags */
 
 	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
-- 
2.39.2

Re: [PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:53:07PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're porting all users of gpiochip_find() to using gpio_device_find().
> Update the swnode GPIO code.

...

> -static int swnode_gpiochip_match_name(struct gpio_chip *chip, void *data)
> +static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
>  {
> -	return !strcmp(chip->label, data);
> -}
> +	const struct software_node *gdev_node;
> +	struct gpio_device *gdev;
>  
> -static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
> -{

Not sure if you used --patience. Patches might look better.

> -	const struct software_node *chip_node;
> -	struct gpio_chip *chip;
> -
> -	chip_node = to_software_node(fwnode);
> -	if (!chip_node || !chip_node->name)
> +	gdev_node = to_software_node(fwnode);
> +	if (!gdev_node || !gdev_node->name)
>  		return ERR_PTR(-EINVAL);
>  
> -	chip = gpiochip_find((void *)chip_node->name, swnode_gpiochip_match_name);
> -	return chip ?: ERR_PTR(-EPROBE_DEFER);
> +	gdev = gpio_device_find_by_label((void *)gdev_node->name);
> +	return gdev ?: ERR_PTR(-EPROBE_DEFER);
>  }

...

> -	desc = gpiochip_get_desc(chip, args.args[0]);
> +	desc = gpiochip_get_desc(gdev->chip, args.args[0]);

gpio_device_get_desc() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We're porting all users of gpiochip_find() to using gpio_device_find().
> Update the swnode GPIO code.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With Andy's comment addressed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

[Paul Cercueil]

Hi Bartosz,

Le mardi 05 septembre 2023 à 20:53 +0200, Bartosz Golaszewski a écrit :
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're porting all users of gpiochip_find() to using
> gpio_device_find().
> Update the swnode GPIO code.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-
> swnode.c
> index b5a6eaf3729b..56c8519be538 100644
> --- a/drivers/gpio/gpiolib-swnode.c
> +++ b/drivers/gpio/gpiolib-swnode.c
> @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> *con_id, char *propname,
>  		strscpy(propname, "gpios", max_size);
>  }
>  
> -static int swnode_gpiochip_match_name(struct gpio_chip *chip, void
> *data)
> +static struct gpio_device *swnode_get_gpio_device(struct
> fwnode_handle *fwnode)
>  {
> -	return !strcmp(chip->label, data);
> -}
> +	const struct software_node *gdev_node;
> +	struct gpio_device *gdev;
>  
> -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> *fwnode)
> -{
> -	const struct software_node *chip_node;
> -	struct gpio_chip *chip;
> -
> -	chip_node = to_software_node(fwnode);
> -	if (!chip_node || !chip_node->name)
> +	gdev_node = to_software_node(fwnode);
> +	if (!gdev_node || !gdev_node->name)
>  		return ERR_PTR(-EINVAL);
>  
> -	chip = gpiochip_find((void *)chip_node->name,
> swnode_gpiochip_match_name);
> -	return chip ?: ERR_PTR(-EPROBE_DEFER);
> +	gdev = gpio_device_find_by_label((void *)gdev_node->name);
> +	return gdev ?: ERR_PTR(-EPROBE_DEFER);
>  }
>  
>  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
>  				   const char *con_id, unsigned int
> idx,
>  				   unsigned long *flags)
>  {
> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;
>  	const struct software_node *swnode;
>  	struct fwnode_reference_args args;
> -	struct gpio_chip *chip;
>  	struct gpio_desc *desc;
>  	char propname[32]; /* 32 is max size of property name */
>  	int error;
> @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> fwnode_handle *fwnode,
>  		return ERR_PTR(error);
>  	}
>  
> -	chip = swnode_get_chip(args.fwnode);
> +	gdev = swnode_get_gpio_device(args.fwnode);
>  	fwnode_handle_put(args.fwnode);
> -	if (IS_ERR(chip))
> -		return ERR_CAST(chip);
> +	if (IS_ERR(gdev))
> +		return ERR_CAST(gdev);

I'm a bit late to the party, sorry.

I'm looking at how __free() should be used to use it in my own
patchset, and I was wondering if this code actually works.

What happens if swnode_get_gpio_device() returns an error pointer?
Won't that cause a call to gpio_device_put() with the invalid pointer?

Cheers,
-Paul

>  
> -	desc = gpiochip_get_desc(chip, args.args[0]);
> +	desc = gpiochip_get_desc(gdev->chip, args.args[0]);
>  	*flags = args.args[1]; /* We expect native GPIO flags */
>  
>  	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> status (%d)\n",

Re: [PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

[Bartosz Golaszewski]

On Wed, Jan 24, 2024 at 3:59 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi Bartosz,
>
> Le mardi 05 septembre 2023 à 20:53 +0200, Bartosz Golaszewski a écrit :
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We're porting all users of gpiochip_find() to using
> > gpio_device_find().
> > Update the swnode GPIO code.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
> >  1 file changed, 12 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-
> > swnode.c
> > index b5a6eaf3729b..56c8519be538 100644
> > --- a/drivers/gpio/gpiolib-swnode.c
> > +++ b/drivers/gpio/gpiolib-swnode.c
> > @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> > *con_id, char *propname,
> >               strscpy(propname, "gpios", max_size);
> >  }
> >
> > -static int swnode_gpiochip_match_name(struct gpio_chip *chip, void
> > *data)
> > +static struct gpio_device *swnode_get_gpio_device(struct
> > fwnode_handle *fwnode)
> >  {
> > -     return !strcmp(chip->label, data);
> > -}
> > +     const struct software_node *gdev_node;
> > +     struct gpio_device *gdev;
> >
> > -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> > *fwnode)
> > -{
> > -     const struct software_node *chip_node;
> > -     struct gpio_chip *chip;
> > -
> > -     chip_node = to_software_node(fwnode);
> > -     if (!chip_node || !chip_node->name)
> > +     gdev_node = to_software_node(fwnode);
> > +     if (!gdev_node || !gdev_node->name)
> >               return ERR_PTR(-EINVAL);
> >
> > -     chip = gpiochip_find((void *)chip_node->name,
> > swnode_gpiochip_match_name);
> > -     return chip ?: ERR_PTR(-EPROBE_DEFER);
> > +     gdev = gpio_device_find_by_label((void *)gdev_node->name);
> > +     return gdev ?: ERR_PTR(-EPROBE_DEFER);
> >  }
> >
> >  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> >                                  const char *con_id, unsigned int
> > idx,
> >                                  unsigned long *flags)
> >  {
> > +     struct gpio_device *gdev __free(gpio_device_put) = NULL;
> >       const struct software_node *swnode;
> >       struct fwnode_reference_args args;
> > -     struct gpio_chip *chip;
> >       struct gpio_desc *desc;
> >       char propname[32]; /* 32 is max size of property name */
> >       int error;
> > @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> > fwnode_handle *fwnode,
> >               return ERR_PTR(error);
> >       }
> >
> > -     chip = swnode_get_chip(args.fwnode);
> > +     gdev = swnode_get_gpio_device(args.fwnode);
> >       fwnode_handle_put(args.fwnode);
> > -     if (IS_ERR(chip))
> > -             return ERR_CAST(chip);
> > +     if (IS_ERR(gdev))
> > +             return ERR_CAST(gdev);
>
> I'm a bit late to the party, sorry.
>
> I'm looking at how __free() should be used to use it in my own
> patchset, and I was wondering if this code actually works.
>
> What happens if swnode_get_gpio_device() returns an error pointer?
> Won't that cause a call to gpio_device_put() with the invalid pointer?
>
> Cheers,
> -Paul
>

No. because the __free() callback is defined as:

DEFINE_FREE(gpio_device_put, struct gpio_device *,
    if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))

Bart

> >
> > -     desc = gpiochip_get_desc(chip, args.args[0]);
> > +     desc = gpiochip_get_desc(gdev->chip, args.args[0]);
> >       *flags = args.args[1]; /* We expect native GPIO flags */
> >
> >       pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> > status (%d)\n",
>

Re: [PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

[Paul Cercueil]

Le mercredi 24 janvier 2024 à 16:04 +0100, Bartosz Golaszewski a
écrit :
> On Wed, Jan 24, 2024 at 3:59 PM Paul Cercueil <paul@crapouillou.net>
> wrote:
> > 
> > Hi Bartosz,
> > 
> > Le mardi 05 septembre 2023 à 20:53 +0200, Bartosz Golaszewski a
> > écrit :
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > 
> > > We're porting all users of gpiochip_find() to using
> > > gpio_device_find().
> > > Update the swnode GPIO code.
> > > 
> > > Signed-off-by: Bartosz Golaszewski
> > > <bartosz.golaszewski@linaro.org>
> > > ---
> > >  drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
> > >  1 file changed, 12 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib-swnode.c
> > > b/drivers/gpio/gpiolib-
> > > swnode.c
> > > index b5a6eaf3729b..56c8519be538 100644
> > > --- a/drivers/gpio/gpiolib-swnode.c
> > > +++ b/drivers/gpio/gpiolib-swnode.c
> > > @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> > > *con_id, char *propname,
> > >               strscpy(propname, "gpios", max_size);
> > >  }
> > > 
> > > -static int swnode_gpiochip_match_name(struct gpio_chip *chip,
> > > void
> > > *data)
> > > +static struct gpio_device *swnode_get_gpio_device(struct
> > > fwnode_handle *fwnode)
> > >  {
> > > -     return !strcmp(chip->label, data);
> > > -}
> > > +     const struct software_node *gdev_node;
> > > +     struct gpio_device *gdev;
> > > 
> > > -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> > > *fwnode)
> > > -{
> > > -     const struct software_node *chip_node;
> > > -     struct gpio_chip *chip;
> > > -
> > > -     chip_node = to_software_node(fwnode);
> > > -     if (!chip_node || !chip_node->name)
> > > +     gdev_node = to_software_node(fwnode);
> > > +     if (!gdev_node || !gdev_node->name)
> > >               return ERR_PTR(-EINVAL);
> > > 
> > > -     chip = gpiochip_find((void *)chip_node->name,
> > > swnode_gpiochip_match_name);
> > > -     return chip ?: ERR_PTR(-EPROBE_DEFER);
> > > +     gdev = gpio_device_find_by_label((void *)gdev_node->name);
> > > +     return gdev ?: ERR_PTR(-EPROBE_DEFER);
> > >  }
> > > 
> > >  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> > >                                  const char *con_id, unsigned int
> > > idx,
> > >                                  unsigned long *flags)
> > >  {
> > > +     struct gpio_device *gdev __free(gpio_device_put) = NULL;
> > >       const struct software_node *swnode;
> > >       struct fwnode_reference_args args;
> > > -     struct gpio_chip *chip;
> > >       struct gpio_desc *desc;
> > >       char propname[32]; /* 32 is max size of property name */
> > >       int error;
> > > @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> > > fwnode_handle *fwnode,
> > >               return ERR_PTR(error);
> > >       }
> > > 
> > > -     chip = swnode_get_chip(args.fwnode);
> > > +     gdev = swnode_get_gpio_device(args.fwnode);
> > >       fwnode_handle_put(args.fwnode);
> > > -     if (IS_ERR(chip))
> > > -             return ERR_CAST(chip);
> > > +     if (IS_ERR(gdev))
> > > +             return ERR_CAST(gdev);
> > 
> > I'm a bit late to the party, sorry.
> > 
> > I'm looking at how __free() should be used to use it in my own
> > patchset, and I was wondering if this code actually works.
> > 
> > What happens if swnode_get_gpio_device() returns an error pointer?
> > Won't that cause a call to gpio_device_put() with the invalid
> > pointer?
> > 
> > Cheers,
> > -Paul
> > 
> 
> No. because the __free() callback is defined as:
> 
> DEFINE_FREE(gpio_device_put, struct gpio_device *,
>     if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))

Ok. I missed this.

I would argue that it's still not right though - it should probably use
IS_ERR() instead. gpio_device_put() only happens to accept NULL
pointers because the "dev" field is at the very beginning of the
"gpio_device" struct. I'm not sure this works with e.g.
CONFIG_RANDSTRUCT_FULL.

> Bart

Cheers,
-Paul

> > > 
> > > -     desc = gpiochip_get_desc(chip, args.args[0]);
> > > +     desc = gpiochip_get_desc(gdev->chip, args.args[0]);
> > >       *flags = args.args[1]; /* We expect native GPIO flags */
> > > 
> > >       pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> > > status (%d)\n",
> > 

Re: [PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

[Paul Cercueil]

Le mercredi 24 janvier 2024 à 16:04 +0100, Bartosz Golaszewski a
écrit :
> On Wed, Jan 24, 2024 at 3:59 PM Paul Cercueil <paul@crapouillou.net>
> wrote:
> > 
> > Hi Bartosz,
> > 
> > Le mardi 05 septembre 2023 à 20:53 +0200, Bartosz Golaszewski a
> > écrit :
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > 
> > > We're porting all users of gpiochip_find() to using
> > > gpio_device_find().
> > > Update the swnode GPIO code.
> > > 
> > > Signed-off-by: Bartosz Golaszewski
> > > <bartosz.golaszewski@linaro.org>
> > > ---
> > >  drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
> > >  1 file changed, 12 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib-swnode.c
> > > b/drivers/gpio/gpiolib-
> > > swnode.c
> > > index b5a6eaf3729b..56c8519be538 100644
> > > --- a/drivers/gpio/gpiolib-swnode.c
> > > +++ b/drivers/gpio/gpiolib-swnode.c
> > > @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> > > *con_id, char *propname,
> > >               strscpy(propname, "gpios", max_size);
> > >  }
> > > 
> > > -static int swnode_gpiochip_match_name(struct gpio_chip *chip,
> > > void
> > > *data)
> > > +static struct gpio_device *swnode_get_gpio_device(struct
> > > fwnode_handle *fwnode)
> > >  {
> > > -     return !strcmp(chip->label, data);
> > > -}
> > > +     const struct software_node *gdev_node;
> > > +     struct gpio_device *gdev;
> > > 
> > > -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> > > *fwnode)
> > > -{
> > > -     const struct software_node *chip_node;
> > > -     struct gpio_chip *chip;
> > > -
> > > -     chip_node = to_software_node(fwnode);
> > > -     if (!chip_node || !chip_node->name)
> > > +     gdev_node = to_software_node(fwnode);
> > > +     if (!gdev_node || !gdev_node->name)
> > >               return ERR_PTR(-EINVAL);
> > > 
> > > -     chip = gpiochip_find((void *)chip_node->name,
> > > swnode_gpiochip_match_name);
> > > -     return chip ?: ERR_PTR(-EPROBE_DEFER);
> > > +     gdev = gpio_device_find_by_label((void *)gdev_node->name);
> > > +     return gdev ?: ERR_PTR(-EPROBE_DEFER);
> > >  }
> > > 
> > >  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> > >                                  const char *con_id, unsigned int
> > > idx,
> > >                                  unsigned long *flags)
> > >  {
> > > +     struct gpio_device *gdev __free(gpio_device_put) = NULL;
> > >       const struct software_node *swnode;
> > >       struct fwnode_reference_args args;
> > > -     struct gpio_chip *chip;
> > >       struct gpio_desc *desc;
> > >       char propname[32]; /* 32 is max size of property name */
> > >       int error;
> > > @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> > > fwnode_handle *fwnode,
> > >               return ERR_PTR(error);
> > >       }
> > > 
> > > -     chip = swnode_get_chip(args.fwnode);
> > > +     gdev = swnode_get_gpio_device(args.fwnode);
> > >       fwnode_handle_put(args.fwnode);
> > > -     if (IS_ERR(chip))
> > > -             return ERR_CAST(chip);
> > > +     if (IS_ERR(gdev))
> > > +             return ERR_CAST(gdev);
> > 
> > I'm a bit late to the party, sorry.
> > 
> > I'm looking at how __free() should be used to use it in my own
> > patchset, and I was wondering if this code actually works.
> > 
> > What happens if swnode_get_gpio_device() returns an error pointer?
> > Won't that cause a call to gpio_device_put() with the invalid
> > pointer?
> > 
> > Cheers,
> > -Paul
> > 
> 
> No. because the __free() callback is defined as:
> 
> DEFINE_FREE(gpio_device_put, struct gpio_device *,
>     if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))

Disregard my previous email, I'm stupid. This actually checks that the
pointer is non-null.

Cheers,
-Paul

> 
> Bart
> 
> > > 
> > > -     desc = gpiochip_get_desc(chip, args.args[0]);
> > > +     desc = gpiochip_get_desc(gdev->chip, args.args[0]);
> > >       *flags = args.args[1]; /* We expect native GPIO flags */
> > > 
> > >       pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> > > status (%d)\n",
> > 

[PATCH 20/21] gpio: sysfs: drop the mention of gpiochip_find() from sysfs code

[Bartosz Golaszewski]

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

We have removed all callers of gpiochip_find() so don't mention it in
gpiolib-sysfs.c.

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

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 50503a4525eb..6f309a3b2d9a 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -814,7 +814,7 @@ static int __init gpiolib_sysfs_init(void)
 		 * gpiochip_sysfs_register() acquires a mutex. This is unsafe
 		 * and needs to be fixed.
 		 *
-		 * Also it would be nice to use gpiochip_find() here so we
+		 * Also it would be nice to use gpio_device_find() here so we
 		 * can keep gpio_chips local to gpiolib.c, but the yield of
 		 * gpio_lock prevents us from doing this.
 		 */
-- 
2.39.2

Re: [PATCH 20/21] gpio: sysfs: drop the mention of gpiochip_find() from sysfs code

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We have removed all callers of gpiochip_find() so don't mention it in
> gpiolib-sysfs.c.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Yours,
Linus Walleij

[PATCH 21/21] gpiolib: remove gpiochip_find()

[Bartosz Golaszewski]

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

With all users of gpiochip_find() converted to using gpio_device_find(),
we can now remove this function from the kernel.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 22 ----------------------
 include/linux/gpio/driver.h |  3 ---
 2 files changed, 25 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 90e8c3d8b6f6..bd700fb4871e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1081,28 +1081,6 @@ void gpiochip_remove(struct gpio_chip *gc)
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
 
-/*
- * FIXME: This will be removed soon.
- *
- * This function is depracated, don't use.
- */
-struct gpio_chip *gpiochip_find(void *data,
-				int (*match)(struct gpio_chip *gc,
-					     void *data))
-{
-	struct gpio_device *gdev;
-	struct gpio_chip *gc = NULL;
-
-	gdev = gpio_device_find(data, match);
-	if (gdev) {
-		gc = gdev->chip;
-		gpio_device_put(gdev);
-	}
-
-	return gc;
-}
-EXPORT_SYMBOL_GPL(gpiochip_find);
-
 /**
  * gpio_device_find() - find a specific GPIO device
  * @data: data to pass to match function
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index bb9ec741bfda..7d2bf464478a 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -603,9 +603,6 @@ int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc,
 				    void *data, struct lock_class_key *lock_key,
 				    struct lock_class_key *request_key);
 
-struct gpio_chip *gpiochip_find(void *data,
-				int (*match)(struct gpio_chip *gc, void *data));
-
 struct gpio_device *gpio_device_find(void *data,
 				     int (*match)(struct gpio_chip *gc,
 						  void *data));
-- 
2.39.2

Re: [PATCH 21/21] gpiolib: remove gpiochip_find()

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 08:53:09PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> With all users of gpiochip_find() converted to using gpio_device_find(),
> we can now remove this function from the kernel.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 21/21] gpiolib: remove gpiochip_find()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> With all users of gpiochip_find() converted to using gpio_device_find(),
> we can now remove this function from the kernel.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This is so much cleaner, and introducing some scoped guards
along the way it's a clear win.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 00/21] gpio: convert users to gpio_device_find() and remove gpiochip_find()

[Linus Walleij]

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> The GPIO subsystem does not handle hot-unplug events very well.

Yeah :/ it was never designed for this, and I've seen the discussions.

The person who made the biggest effort to make this sort-of work
was actually Johan Hovold so I added him to the mail so you can
include him in upcoming iterations. I think he was working with
GPIO on greybus at the time. Maybe he want to take a look!

> Before we can even get to fixing the locking, we need to address a serious
> abuse of the GPIO driver API - accessing struct gpio_chip by anyone who isn't
> the driver owning this object. This structure is owned by the GPIO provider
> and its lifetime is tied to that of that provider. It is destroyed when the
> device is unregistered and this may happen at any moment. struct gpio_device
> is the opaque, reference counted interface to struct gpio_chip (which is the
> low-level implementation) and all access should pass through it.

Thanks for looking into this. As I remember I have tried to bring down
this abuse over the years and IIRC it used to be even worse, it came
from the fact that all GPIO drivers used to be under some arch/*
tree and often loosely using the kernel GPIO API but in addition
providing a custom API...

> The end-goal is to make all gpio_device manipulators check the existence of
> gdev->chip and then lock it for the duration of any of the calls using SRCU.

Excellent!

> This series starts the process by replacing gpiochip_find() with
> gpio_device_find(). This is in line with other device_find type interfaces and
> returns a reference to the GPIO device that is guaranteed to remain valid
> until it is put.

I agree with the direction and I see no major problem with the
patches other than some testing and cosmetics. The kernel sure
as hell looks better *after* this than *before* so once you have rough
confidence in the patches I think they should be merged and any
issuse fixed up in-tree so we get wider audience and can continue
the planned refactorings. So:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

I'll try to provide some detailed reviews if something stands out.

Yours,
Linus Walleij