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

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

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

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.

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.

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