[PATCH v2 0/2] gpiolib: Show IRQ line in debugfs

[PATCH v2 0/2] gpiolib: Show IRQ line in debugfs

[Andy Shevchenko]

Couple of patches to show IRQ only (when it's not requested as GPIO)
lines in the debugfs.

In v2:
- joined two separate patches
- rebased on top of latest GPIO for-next (Bart)

Andy Shevchenko (2):
  gpiolib: Return label, if set, for IRQ only line
  gpiolib: Show more info for interrupt only lines in debugfs

 drivers/gpio/gpiolib.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/2] gpiolib: Return label, if set, for IRQ only line

[Andy Shevchenko]

If line has been locked as IRQ without requesting,
still check its label and return it, if not NULL.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 73c9f99d141e..a6032b84ba98 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -107,16 +107,16 @@ const char *gpiod_get_label(struct gpio_desc *desc)
 	unsigned long flags;
 
 	flags = READ_ONCE(desc->flags);
-	if (test_bit(FLAG_USED_AS_IRQ, &flags) &&
-	    !test_bit(FLAG_REQUESTED, &flags))
-		return "interrupt";
-
-	if (!test_bit(FLAG_REQUESTED, &flags))
-		return NULL;
 
 	label = srcu_dereference_check(desc->label, &desc->gdev->desc_srcu,
 				srcu_read_lock_held(&desc->gdev->desc_srcu));
 
+	if (test_bit(FLAG_USED_AS_IRQ, &flags))
+		return label->str ?: "interrupt";
+
+	if (!test_bit(FLAG_REQUESTED, &flags))
+		return NULL;
+
 	return label->str;
 }
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 1/2] gpiolib: Return label, if set, for IRQ only line

[Linus Walleij]

On Thu, May 30, 2024 at 9:14 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> If line has been locked as IRQ without requesting,
> still check its label and return it, if not NULL.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

I'm OK with it personally, the perfect solution would be to catenate
"interrupt" to the line name so we don't miss the information that the
line is used for interrupts in case of proper labels, but it's not a big
problem.

Yours,
Linus Walleij

[PATCH v2 2/2] gpiolib: Show more info for interrupt only lines in debugfs

[Andy Shevchenko]

Show more info for interrupt only lines in debugfs. It's useful
to monitor the lines that have been never requested as GPIOs,
but IRQs.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a6032b84ba98..f3b2f5c4781d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4888,11 +4888,11 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 
 	for_each_gpio_desc(gc, desc) {
 		guard(srcu)(&desc->gdev->desc_srcu);
-		if (test_bit(FLAG_REQUESTED, &desc->flags)) {
+		is_irq = test_bit(FLAG_USED_AS_IRQ, &desc->flags);
+		if (is_irq || test_bit(FLAG_REQUESTED, &desc->flags)) {
 			gpiod_get_direction(desc);
 			is_out = test_bit(FLAG_IS_OUT, &desc->flags);
 			value = gpio_chip_get_value(gc, desc);
-			is_irq = test_bit(FLAG_USED_AS_IRQ, &desc->flags);
 			active_low = test_bit(FLAG_ACTIVE_LOW, &desc->flags);
 			seq_printf(s, " gpio-%-3u (%-20.20s|%-20.20s) %s %s %s%s\n",
 				   gpio, desc->name ?: "", gpiod_get_label(desc),
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 2/2] gpiolib: Show more info for interrupt only lines in debugfs

[Alexander Stein]

Hi,

Am Donnerstag, 30. Mai 2024, 21:12:30 CEST schrieb Andy Shevchenko:
> Show more info for interrupt only lines in debugfs. It's useful
> to monitor the lines that have been never requested as GPIOs,
> but IRQs.

I was trying to test this on TQMa8MPQL (i.MX8MP) using gpio-mxc.c.
But apparently this series only has an effect when gpiochip_lock_as_irq()
is called eventually. I'm wondering what needs to be done so IRQ only
GPIOs are listed in debugfs. Using irq_request_resources/irq_release_resources
similar to what pinctrl-at91.c is doing?

Best regards,
Alexander

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a6032b84ba98..f3b2f5c4781d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4888,11 +4888,11 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
>  
>  	for_each_gpio_desc(gc, desc) {
>  		guard(srcu)(&desc->gdev->desc_srcu);
> -		if (test_bit(FLAG_REQUESTED, &desc->flags)) {
> +		is_irq = test_bit(FLAG_USED_AS_IRQ, &desc->flags);
> +		if (is_irq || test_bit(FLAG_REQUESTED, &desc->flags)) {
>  			gpiod_get_direction(desc);
>  			is_out = test_bit(FLAG_IS_OUT, &desc->flags);
>  			value = gpio_chip_get_value(gc, desc);
> -			is_irq = test_bit(FLAG_USED_AS_IRQ, &desc->flags);
>  			active_low = test_bit(FLAG_ACTIVE_LOW, &desc->flags);
>  			seq_printf(s, " gpio-%-3u (%-20.20s|%-20.20s) %s %s %s%s\n",
>  				   gpio, desc->name ?: "", gpiod_get_label(desc),
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 2/2] gpiolib: Show more info for interrupt only lines in debugfs

[Andy Shevchenko]

On Fri, May 31, 2024 at 01:19:56PM +0200, Alexander Stein wrote:
> Am Donnerstag, 30. Mai 2024, 21:12:30 CEST schrieb Andy Shevchenko:
> > Show more info for interrupt only lines in debugfs. It's useful
> > to monitor the lines that have been never requested as GPIOs,
> > but IRQs.
> 
> I was trying to test this on TQMa8MPQL (i.MX8MP) using gpio-mxc.c.

Thank you for trying!

> But apparently this series only has an effect when gpiochip_lock_as_irq()
> is called eventually. I'm wondering what needs to be done so IRQ only
> GPIOs are listed in debugfs. Using irq_request_resources/irq_release_resources
> similar to what pinctrl-at91.c is doing?

I haven't looked deeply into this and I don't know if it's relevant, but...

The idea is that GPIO driver has an IRQ chip that announces handle_bad_irq()
as a handler and IRQ_TYPE_NONE as default type at probe stage. It also needs
to implement ->set_irq_type() callback where actual handler is going to be
locked.

That's what I do not see implemented in the driver. Moreover, I do see it
implements its own ->to_irq() callback which shouldn't be there.

Taking all above into consideration _I think_ the drivers need a bit of
refreshments.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] gpiolib: Show more info for interrupt only lines in debugfs

[Alexander Stein]

Hi,

Am Freitag, 31. Mai 2024, 15:30:58 CEST schrieb Andy Shevchenko:
> On Fri, May 31, 2024 at 01:19:56PM +0200, Alexander Stein wrote:
> > Am Donnerstag, 30. Mai 2024, 21:12:30 CEST schrieb Andy Shevchenko:
> > > Show more info for interrupt only lines in debugfs. It's useful
> > > to monitor the lines that have been never requested as GPIOs,
> > > but IRQs.
> > 
> > I was trying to test this on TQMa8MPQL (i.MX8MP) using gpio-mxc.c.
> 
> Thank you for trying!
> 
> > But apparently this series only has an effect when gpiochip_lock_as_irq()
> > is called eventually. I'm wondering what needs to be done so IRQ only
> > GPIOs are listed in debugfs. Using irq_request_resources/irq_release_resources
> > similar to what pinctrl-at91.c is doing?
> 
> I haven't looked deeply into this and I don't know if it's relevant, but...
> 
> The idea is that GPIO driver has an IRQ chip that announces handle_bad_irq()
> as a handler and IRQ_TYPE_NONE as default type at probe stage. It also needs
> to implement ->set_irq_type() callback where actual handler is going to be
> locked.
> 
> That's what I do not see implemented in the driver. Moreover, I do see it
> implements its own ->to_irq() callback which shouldn't be there.
> 
> Taking all above into consideration _I think_ the drivers need a bit of
> refreshments.

I noticed this driver is using irq_chip_generic and a dedicated irq domain.
I'm not sure if this is superseded meanwhile using the integrated IRQ chip
inside that GPIO chip.
Thanks for looking into this.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 2/2] gpiolib: Show more info for interrupt only lines in debugfs

[Andy Shevchenko]

On Fri, May 31, 2024 at 01:19:56PM +0200, Alexander Stein wrote:
> Hi,
> 
> Am Donnerstag, 30. Mai 2024, 21:12:30 CEST schrieb Andy Shevchenko:
> > Show more info for interrupt only lines in debugfs. It's useful
> > to monitor the lines that have been never requested as GPIOs,
> > but IRQs.
> 
> I was trying to test this on TQMa8MPQL (i.MX8MP) using gpio-mxc.c.
> But apparently this series only has an effect when gpiochip_lock_as_irq()
> is called eventually. I'm wondering what needs to be done so IRQ only
> GPIOs are listed in debugfs. Using irq_request_resources/irq_release_resources
> similar to what pinctrl-at91.c is doing?

For the record on Intel Galileo Gen2 I see this difference

Before:

gpiochip4: GPIOs 8-15, parent: platform/gpio-dwapb.1, gpio-dwapb.1:
gpio-10  (                    |spi169 CS0          ) out hi

After:

gpiochip4: GPIOs 8-15, parent: platform/gpio-dwapb.1, gpio-dwapb.1:
gpio-9   (                    |irq                 ) in  hi IRQ ACTIVE LOW
gpio-10  (                    |spi169 CS0          ) out hi

but it's handled via gpiolib-acpi.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] gpiolib: Show IRQ line in debugfs

[Bartosz Golaszewski]

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


On Thu, 30 May 2024 22:12:28 +0300, Andy Shevchenko wrote:
> Couple of patches to show IRQ only (when it's not requested as GPIO)
> lines in the debugfs.
> 
> In v2:
> - joined two separate patches
> - rebased on top of latest GPIO for-next (Bart)
> 
> [...]

Applied, thanks!

[1/2] gpiolib: Return label, if set, for IRQ only line
      commit: 5a646e03e956f45e74c0a606f51e1ebc13880309
[2/2] gpiolib: Show more info for interrupt only lines in debugfs
      commit: 54a687cd49e3625819f24e17655346c85f498cbc

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>