Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?

[Andy Shevchenko]

On Tue, Jun 29, 2021 at 1:52 AM Vincent Pelletier <plr.vincent@xxxxxxxxx> wrote:
>
> On Mon, Jun 28, 2021 at 10:42 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > And one important note: do NOT use sysfs GPIO interface. Use a GPIO
> > character device instead.
>
> I am indeed aware of this. My IRQ issue is unrelated to the gpios
> being claimed by anything, and I was doing it while trying to get
> more information about the current state of the gpio driver.
>
> For more background, the context of my IRQ issue is:
>   PMIC (da9063) /irq -> GPIO pin 1 -> PLIC irq 24
> The PMIC has several internal interrupt sources, like the power
> button being pressed or the ADC conversion completion signal.

This is the usual case with PMIC. We have similar on x86 machines
(PMIC is represented by an MFD driver with regmap IRQ being involved).

> The first time after a boot that I press the power button, I do
> get an interrupt and the da9063-onkey driver produces a keypress
> input event.
> But any further button press does not produce an IRQ. So something
> is going wrong in the IRQ acknowledgement.
> AFAIK the PLIC (platform-level interrupt controller) works: it is
> used for PCIe interrupts, and those work.
> The PMIC driver exists since 2013, so I assume any bug would have
> been identified long ago.
> But I believe the GPIO level has not handled any interrupt until I
> enabled the power button event source, and this one is a lot more
> recent: gpio-sifive.c from late 2019. So this is where I turned my
> attention. Discovering that the pin is somehow only half-claimed
> made me wonder if there was some important initialisation step
> missing, which could maybe be related to these IRQ issues.
>
> While on this topic, there is a bullet point in
> Documentation/driver-api/gpio/driver.rst which I fail to understand:
>
> | - Nominally set all handlers to handle_bad_irq() in the setup call and pass
> |   handle_bad_irq() as flow handler parameter in
> gpiochip_irqchip_add() if it is
> |   expected for GPIO driver that irqchip .set_type() callback will be called
> |   before using/enabling each GPIO IRQ. Then set the handler to
> |   handle_level_irq() and/or handle_edge_irq() in the irqchip .set_type()
> |   callback depending on what your controller supports and what is requested
> |   by the consumer.
>
> - why the plural in "set all handlers to handle_bad_irq()" ? Isn't
>   there only a single handler in struct gpio_irq_chip ?

Each GPIO line may have its own handler (usually level or edge). I
guess it's written from the GPIO point of view.

> - I do not find a function named gpiochip_irqchip_add(), only
>   gpiochip_irqchip_add_domain()

Missed during update I suppose.
https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=gpio/for-next&id=f1f37abbe6fc2b1242f78157db76e48dbf9518ee
Feel free to submit a patch!

> - "Then set the handler to [...] in the irqchip .set_type() callback"
>   Isn't set_type per-pin, and isn't the interrupt handler chip-level ?

The idea behind that initially the chip-level IRQ handler is set to
BAD. It means any (spurious) IRQ will be served by it. Now, when one
requests IRQ the framework will call ->irq_set_type() of corresponding
IRQ chip and change the handler for the certain pin (pin-level). So,
the main handler is basically for spurious interrupts only.

-- 
With Best Regards,
Andy Shevchenko