diff mbox series

[v1,1/2] gpiolib: acpi: Add fwnode name to the GPIO interrupt label

Message ID 20240417103829.2324960-2-andriy.shevchenko@linux.intel.com
State Accepted
Commit 716b532814ffd94792f9033c3ece79145d92d701
Headers show
Series gpiolib: acpi: Improve IRQ labeling | expand

Commit Message

Andy Shevchenko April 17, 2024, 10:37 a.m. UTC
It's ambiguous to have a device-related index in the GPIO interrupt
label as most of the devices will have it the same or very similar.
Extend label with fwnode name for better granularity. It significantly
reduces the scope of searching among devices.

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

Comments

Mika Westerberg April 18, 2024, 4:49 a.m. UTC | #1
On Wed, Apr 17, 2024 at 01:37:27PM +0300, Andy Shevchenko wrote:
> It's ambiguous to have a device-related index in the GPIO interrupt
> label as most of the devices will have it the same or very similar.
> Extend label with fwnode name for better granularity. It significantly
> reduces the scope of searching among devices.

Can you add an example here how it looks like before and after the
patch?

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 909113312a1b..0b0c8729fc6e 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1035,6 +1035,7 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
>  int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id, int index,
>  				  bool *wake_capable)
>  {
> +	struct fwnode_handle *fwnode = acpi_fwnode_handle(adev);
>  	int idx, i;
>  	unsigned int irq_flags;
>  	int ret;
> @@ -1044,7 +1045,7 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
>  		struct gpio_desc *desc;
>  
>  		/* Ignore -EPROBE_DEFER, it only matters if idx matches */
> -		desc = __acpi_find_gpio(acpi_fwnode_handle(adev), con_id, i, true, &info);
> +		desc = __acpi_find_gpio(fwnode, con_id, i, true, &info);
>  		if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER)
>  			return PTR_ERR(desc);
>  
> @@ -1064,7 +1065,7 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
>  			acpi_gpio_update_gpiod_flags(&dflags, &info);
>  			acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
>  
> -			snprintf(label, sizeof(label), "GpioInt() %d", index);
> +			snprintf(label, sizeof(label), "%pfwP GpioInt(%d)", fwnode, index);
>  			ret = gpiod_configure_flags(desc, label, lflags, dflags);
>  			if (ret < 0)
>  				return ret;
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac
Andy Shevchenko April 18, 2024, 9:23 a.m. UTC | #2
On Thu, Apr 18, 2024 at 07:49:07AM +0300, Mika Westerberg wrote:
> On Wed, Apr 17, 2024 at 01:37:27PM +0300, Andy Shevchenko wrote:
> > It's ambiguous to have a device-related index in the GPIO interrupt
> > label as most of the devices will have it the same or very similar.
> > Extend label with fwnode name for better granularity. It significantly
> > reduces the scope of searching among devices.
> 
> Can you add an example here how it looks like before and after the
> patch?

Sure:

Before:

  GpioInt() 0
  GpioInt() 0

After:

  NIO1 GpioInt(0)
  URT0 GpioInt(0)

Assuming I update this when applying, can you give your tag?
Mika Westerberg April 18, 2024, 9:33 a.m. UTC | #3
On Thu, Apr 18, 2024 at 12:23:45PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 18, 2024 at 07:49:07AM +0300, Mika Westerberg wrote:
> > On Wed, Apr 17, 2024 at 01:37:27PM +0300, Andy Shevchenko wrote:
> > > It's ambiguous to have a device-related index in the GPIO interrupt
> > > label as most of the devices will have it the same or very similar.
> > > Extend label with fwnode name for better granularity. It significantly
> > > reduces the scope of searching among devices.
> > 
> > Can you add an example here how it looks like before and after the
> > patch?
> 
> Sure:
> 
> Before:
> 
>   GpioInt() 0
>   GpioInt() 0
> 
> After:
> 
>   NIO1 GpioInt(0)
>   URT0 GpioInt(0)
> 
> Assuming I update this when applying, can you give your tag?

Sure. For both,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Andy Shevchenko April 18, 2024, 10 a.m. UTC | #4
On Thu, Apr 18, 2024 at 12:33:59PM +0300, Mika Westerberg wrote:
> On Thu, Apr 18, 2024 at 12:23:45PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 18, 2024 at 07:49:07AM +0300, Mika Westerberg wrote:
> > > On Wed, Apr 17, 2024 at 01:37:27PM +0300, Andy Shevchenko wrote:
> > > > It's ambiguous to have a device-related index in the GPIO interrupt
> > > > label as most of the devices will have it the same or very similar.
> > > > Extend label with fwnode name for better granularity. It significantly
> > > > reduces the scope of searching among devices.
> > > 
> > > Can you add an example here how it looks like before and after the
> > > patch?
> > 
> > Sure:
> > 
> > Before:
> > 
> >   GpioInt() 0
> >   GpioInt() 0
> > 
> > After:
> > 
> >   NIO1 GpioInt(0)
> >   URT0 GpioInt(0)
> > 
> > Assuming I update this when applying, can you give your tag?
> 
> Sure. For both,
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review and testing queue, thanks!
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 909113312a1b..0b0c8729fc6e 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1035,6 +1035,7 @@  struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id, int index,
 				  bool *wake_capable)
 {
+	struct fwnode_handle *fwnode = acpi_fwnode_handle(adev);
 	int idx, i;
 	unsigned int irq_flags;
 	int ret;
@@ -1044,7 +1045,7 @@  int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
 		struct gpio_desc *desc;
 
 		/* Ignore -EPROBE_DEFER, it only matters if idx matches */
-		desc = __acpi_find_gpio(acpi_fwnode_handle(adev), con_id, i, true, &info);
+		desc = __acpi_find_gpio(fwnode, con_id, i, true, &info);
 		if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER)
 			return PTR_ERR(desc);
 
@@ -1064,7 +1065,7 @@  int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
 			acpi_gpio_update_gpiod_flags(&dflags, &info);
 			acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
 
-			snprintf(label, sizeof(label), "GpioInt() %d", index);
+			snprintf(label, sizeof(label), "%pfwP GpioInt(%d)", fwnode, index);
 			ret = gpiod_configure_flags(desc, label, lflags, dflags);
 			if (ret < 0)
 				return ret;