Re: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

["Westerberg, Mika" <mika.westerberg@intel.com>]

On Mon, Mar 14, 2016 at 02:54:36PM +0200, Westerberg, Mika wrote:
> On Mon, Mar 14, 2016 at 01:40:39PM +0100, Linus Walleij wrote:
> > On Mon, Mar 14, 2016 at 9:44 AM, Westerberg, Mika
> > <mika.westerberg@intel.com> wrote:
> > > On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote:
> > 
> > >> > +   intel_gpio_irq_ack(d);
> > >>
> > >> > If the pin toggles right here, we still have the same issue, no?
> > >>
> > >> Yes. But it is very short time from here to the place IRQ got enabled.
> > >
> > > That is still a race.
> > >
> > >> To me, it is the only available platform dependent interface to do
> > > the "ACK" in the flow of request_irq. Maybe you have other better
> > > option here?
> > (...)
> > > Drivers need to deal with the fact that they might get spurious
> > > interrupts from time to time. You need to check in the interrupt handler
> > > if the interrupt was from the device you are driving or not.
> > >
> > > request_irq() enables the interrupt line and if the pin is already in
> > > a state that triggers an interrupt, the driver interrupt handler will
> > > be called.
> > 
> > This looks like something is wrong in the irqchip.
> 
> If request_irq() is called so that the interrupt is level triggered,
> let's say active low, and the pin is already in that state I would
> expect interrupt to trigger immediately when enabled, no?
> 
> If I understand correctly this is precisely what Zheng is describing
> they are trying to solve.

Scratch that. I just re-read the patch changelog and they are
configuring the pin as edge triggered.

> > Your set_type() is supporting edges but have all IRQs
> > handled by handle_simple_irq() rather than handle_edge_irq()
> > for the edges, which gives a more robust control flow
> > from IRQ to ACK to calling the handler.
> > 
> > Zheng/Mika: please look at how the level/edge
> > IRQs are handled in drivers/gpio/gpio-pl061.c
> > where I *tried* to do things right, switching handler
> > in .set_type() using irq_set_handler_locked(). I think
> > you may need to use handle_edge_irq() for the edge IRQs
> > and handle_level_irq() for the level IRQs just like I do
> > in the PL061 driver.
> 
> The driver is already doing that as far as I can tell (see
> intel_gpio_irq_type()).

I will check again if we are still missing something there.