diff mbox series

[v2] gpiolib: Pass consumer device through to core in devm_fwnode_gpiod_get_index()

Message ID 20240223065254.3795204-1-swboyd@chromium.org
State New
Headers show
Series [v2] gpiolib: Pass consumer device through to core in devm_fwnode_gpiod_get_index() | expand

Commit Message

Stephen Boyd Feb. 23, 2024, 6:52 a.m. UTC 
This devm API takes a consumer device as an argument to setup the devm
action, but throws it away when calling further into gpiolib. This leads
to odd debug messages like this:

 (NULL device *): using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup

Let's pass the consumer device down, by directly calling what
fwnode_gpiod_get_index() calls but pass the device used for devm. This
changes the message to look like this instead:

 gpio-keys gpio-keys: using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup

Note that callers of fwnode_gpiod_get_index() will still see the NULL
device pointer debug message, but there's not much we can do about that
because the API doesn't take a struct device.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Changes from v1 (https://lore.kernel.org/r/20240221015920.676063-1-swboyd@chromium.org):
 * Rebased onto gpio/for-next

 drivers/gpio/gpiolib-devres.c |  2 +-
 drivers/gpio/gpiolib.c        | 14 +++++++-------
 drivers/gpio/gpiolib.h        |  8 ++++++++
 3 files changed, 16 insertions(+), 8 deletions(-)


base-commit: 36e44186e0badfda499b65d4462c49783bf92314

Comments

Bartosz Golaszewski Feb. 27, 2024, 1:18 p.m. UTC | #1 
On Fri, Feb 23, 2024 at 7:52 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> This devm API takes a consumer device as an argument to setup the devm
> action, but throws it away when calling further into gpiolib. This leads
> to odd debug messages like this:
>
>  (NULL device *): using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
>
> Let's pass the consumer device down, by directly calling what
> fwnode_gpiod_get_index() calls but pass the device used for devm. This
> changes the message to look like this instead:
>
>  gpio-keys gpio-keys: using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
>
> Note that callers of fwnode_gpiod_get_index() will still see the NULL
> device pointer debug message, but there's not much we can do about that
> because the API doesn't take a struct device.
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---

Applied, thanks!

Bart
Andy Shevchenko Feb. 28, 2024, 6:57 p.m. UTC | #2 
On Thu, Feb 22, 2024 at 10:52:53PM -0800, Stephen Boyd wrote:
> This devm API takes a consumer device as an argument to setup the devm
> action, but throws it away when calling further into gpiolib. This leads
> to odd debug messages like this:
> 
>  (NULL device *): using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
> 
> Let's pass the consumer device down, by directly calling what
> fwnode_gpiod_get_index() calls but pass the device used for devm. This
> changes the message to look like this instead:
> 
>  gpio-keys gpio-keys: using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
> 
> Note that callers of fwnode_gpiod_get_index() will still see the NULL
> device pointer debug message, but there's not much we can do about that
> because the API doesn't take a struct device.

Have you seen this?
https://lore.kernel.org/r/20231019173457.2445119-1-andriy.shevchenko@linux.intel.com
Bartosz Golaszewski Feb. 28, 2024, 9:28 p.m. UTC | #3 
On Wed, Feb 28, 2024 at 7:57 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Feb 22, 2024 at 10:52:53PM -0800, Stephen Boyd wrote:
> > This devm API takes a consumer device as an argument to setup the devm
> > action, but throws it away when calling further into gpiolib. This leads
> > to odd debug messages like this:
> >
> >  (NULL device *): using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
> >
> > Let's pass the consumer device down, by directly calling what
> > fwnode_gpiod_get_index() calls but pass the device used for devm. This
> > changes the message to look like this instead:
> >
> >  gpio-keys gpio-keys: using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
> >
> > Note that callers of fwnode_gpiod_get_index() will still see the NULL
> > device pointer debug message, but there's not much we can do about that
> > because the API doesn't take a struct device.
>
> Have you seen this?
> https://lore.kernel.org/r/20231019173457.2445119-1-andriy.shevchenko@linux.intel.com

Clearly yes as I queued the first one in that series. The rest did not
make its way upstream for whatever reason. What is your point? You
want to respin it?

Bart
Andy Shevchenko Feb. 28, 2024, 9:35 p.m. UTC | #4 
On Wed, Feb 28, 2024 at 10:28:07PM +0100, Bartosz Golaszewski wrote:
> On Wed, Feb 28, 2024 at 7:57 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Thu, Feb 22, 2024 at 10:52:53PM -0800, Stephen Boyd wrote:
> > > This devm API takes a consumer device as an argument to setup the devm
> > > action, but throws it away when calling further into gpiolib. This leads
> > > to odd debug messages like this:
> > >
> > >  (NULL device *): using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
> > >
> > > Let's pass the consumer device down, by directly calling what
> > > fwnode_gpiod_get_index() calls but pass the device used for devm. This
> > > changes the message to look like this instead:
> > >
> > >  gpio-keys gpio-keys: using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
> > >
> > > Note that callers of fwnode_gpiod_get_index() will still see the NULL
> > > device pointer debug message, but there's not much we can do about that
> > > because the API doesn't take a struct device.
> >
> > Have you seen this?
> > https://lore.kernel.org/r/20231019173457.2445119-1-andriy.shevchenko@linux.intel.com
> 
> Clearly yes as I queued the first one in that series. The rest did not
> make its way upstream for whatever reason. What is your point? You
> want to respin it?

It was a reply to Stephen. :-)
Bartosz Golaszewski Feb. 28, 2024, 9:37 p.m. UTC | #5 
On Wed, Feb 28, 2024 at 10:35 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Feb 28, 2024 at 10:28:07PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Feb 28, 2024 at 7:57 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Thu, Feb 22, 2024 at 10:52:53PM -0800, Stephen Boyd wrote:
> > > > This devm API takes a consumer device as an argument to setup the devm
> > > > action, but throws it away when calling further into gpiolib. This leads
> > > > to odd debug messages like this:
> > > >
> > > >  (NULL device *): using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
> > > >
> > > > Let's pass the consumer device down, by directly calling what
> > > > fwnode_gpiod_get_index() calls but pass the device used for devm. This
> > > > changes the message to look like this instead:
> > > >
> > > >  gpio-keys gpio-keys: using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup
> > > >
> > > > Note that callers of fwnode_gpiod_get_index() will still see the NULL
> > > > device pointer debug message, but there's not much we can do about that
> > > > because the API doesn't take a struct device.
> > >
> > > Have you seen this?
> > > https://lore.kernel.org/r/20231019173457.2445119-1-andriy.shevchenko@linux.intel.com
> >
> > Clearly yes as I queued the first one in that series. The rest did not
> > make its way upstream for whatever reason. What is your point? You
> > want to respin it?
>
> It was a reply to Stephen. :-)
>

Ah, fair enough.

Bart
Stephen Boyd Feb. 28, 2024, 9:38 p.m. UTC | #6 
Quoting Andy Shevchenko (2024-02-28 13:35:31)
> On Wed, Feb 28, 2024 at 10:28:07PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Feb 28, 2024 at 7:57 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > >
> > > Have you seen this?
> > > https://lore.kernel.org/r/20231019173457.2445119-1-andriy.shevchenko@linux.intel.com
> >
> > Clearly yes as I queued the first one in that series. The rest did not
> > make its way upstream for whatever reason. What is your point? You
> > want to respin it?
>
> It was a reply to Stephen. :-)
>

I saw it but it hadn't gone anywhere for many months so I fixed the
problem I saw. Will you resend it?
Andy Shevchenko Feb. 29, 2024, 10:58 a.m. UTC | #7 
On Wed, Feb 28, 2024 at 01:38:54PM -0800, Stephen Boyd wrote:
> Quoting Andy Shevchenko (2024-02-28 13:35:31)
> > On Wed, Feb 28, 2024 at 10:28:07PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Feb 28, 2024 at 7:57 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:

...

> > > > Have you seen this?
> > > > https://lore.kernel.org/r/20231019173457.2445119-1-andriy.shevchenko@linux.intel.com
> > >
> > > Clearly yes as I queued the first one in that series. The rest did not
> > > make its way upstream for whatever reason. What is your point? You
> > > want to respin it?
> >
> > It was a reply to Stephen. :-)
> 
> I saw it but it hadn't gone anywhere for many months so I fixed the
> problem I saw. Will you resend it?

Yes, this is the plan, too many things are ongoing, so have had no time (yet)
to address comments and resubmit. In any case I'm not against your change, it
makes it better anyway without hacking the code.
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index fe9ce6b19f15..4987e62dcb3d 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -158,7 +158,7 @@  struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev,
 	if (!dr)
 		return ERR_PTR(-ENOMEM);
 
-	desc = fwnode_gpiod_get_index(fwnode, con_id, index, flags, label);
+	desc = gpiod_find_and_request(dev, fwnode, con_id, index, flags, label, false);
 	if (IS_ERR(desc)) {
 		devres_free(dr);
 		return desc;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3c22920bd201..cff4ac2403a5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4187,13 +4187,13 @@  static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
 	return desc;
 }
 
-static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
-						struct fwnode_handle *fwnode,
-						const char *con_id,
-						unsigned int idx,
-						enum gpiod_flags flags,
-						const char *label,
-						bool platform_lookup_allowed)
+struct gpio_desc *gpiod_find_and_request(struct device *consumer,
+					 struct fwnode_handle *fwnode,
+					 const char *con_id,
+					 unsigned int idx,
+					 enum gpiod_flags flags,
+					 const char *label,
+					 bool platform_lookup_allowed)
 {
 	unsigned long lookupflags = GPIO_LOOKUP_FLAGS_DEFAULT;
 	/*
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index ada36aa0f81a..f67d5991ab1c 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -223,6 +223,14 @@  static inline int gpiod_request_user(struct gpio_desc *desc, const char *label)
 	return ret;
 }
 
+struct gpio_desc *gpiod_find_and_request(struct device *consumer,
+					 struct fwnode_handle *fwnode,
+					 const char *con_id,
+					 unsigned int idx,
+					 enum gpiod_flags flags,
+					 const char *label,
+					 bool platform_lookup_allowed);
+
 int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags);
 int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);