[PATCH v1 0/2] gpiolib: acpi: Improve IRQ labeling

[PATCH v1 0/2] gpiolib: acpi: Improve IRQ labeling

[Andy Shevchenko]

The IRQ labeling now is quite ambiguous, improve it here.

Andy Shevchenko (2):
  gpiolib: acpi: Add fwnode name to the GPIO interrupt label
  gpiolib: acpi: Set label for IRQ only lines

 drivers/gpio/gpiolib-acpi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

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

[Andy Shevchenko]

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

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

[PATCH v1 2/2] gpiolib: acpi: Set label for IRQ only lines

[Andy Shevchenko]

When line locked as IRQ it has no label assigned. Assign
the meaningful value to it.

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

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 0b0c8729fc6e..553a5f94c00a 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1066,6 +1066,10 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
 			acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
 
 			snprintf(label, sizeof(label), "%pfwP GpioInt(%d)", fwnode, index);
+			ret = gpiod_set_consumer_name(desc, con_id ?: label);
+			if (ret)
+				return ret;
+
 			ret = gpiod_configure_flags(desc, label, lflags, dflags);
 			if (ret < 0)
 				return ret;
-- 
2.43.0.rc1.1336.g36b5255a03ac

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

[Mika Westerberg]

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

Re: [PATCH v1 2/2] gpiolib: acpi: Set label for IRQ only lines

[Mika Westerberg]

On Wed, Apr 17, 2024 at 01:37:28PM +0300, Andy Shevchenko wrote:
> When line locked as IRQ it has no label assigned. Assign
> the meaningful value to it.

Same here.

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 0b0c8729fc6e..553a5f94c00a 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1066,6 +1066,10 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
>  			acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
>  
>  			snprintf(label, sizeof(label), "%pfwP GpioInt(%d)", fwnode, index);
> +			ret = gpiod_set_consumer_name(desc, con_id ?: label);
> +			if (ret)
> +				return ret;
> +
>  			ret = gpiod_configure_flags(desc, label, lflags, dflags);
>  			if (ret < 0)
>  				return ret;
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] gpiolib: acpi: Set label for IRQ only lines

[Andy Shevchenko]

On Thu, Apr 18, 2024 at 07:49:50AM +0300, Mika Westerberg wrote:
> On Wed, Apr 17, 2024 at 01:37:28PM +0300, Andy Shevchenko wrote:
> > When line locked as IRQ it has no label assigned. Assign
> > the meaningful value to it.
> 
> Same here.

Same answer as in the first reply.

-- 
With Best Regards,
Andy Shevchenko

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

[Mika Westerberg]

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>

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

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko