| Message ID | 20240417103829.2324960-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: acpi: Improve IRQ labeling | expand |
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
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?
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>
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 --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;
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(-)