Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller

[Horatiu Vultur]

The 10/13/2022 09:30, Michael Walle wrote:

Hi Michael,

> > We lose the interrupt here, as the HW will not generate another one
> > but at later point we read again the line status. And if the line is
> > active then we kick again the interrupt handler again.
> 
> Ahh, thanks for explaining. That also explains the read below.
> 
> Will you send a proper patch?

No worries. Yes, I will do that.

> 
> -michael
> 
> > 
> > > 
> > > > +
> > > >       /* Enable the interrupt now */
> > > >       gpiochip_enable_irq(chip, gpio);
> > > >       regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
> > > >                          bit, bit);
> > > >
> > > >       /*
> > > > -      * In case the interrupt line is still active and the interrupt
> > > > -      * controller has not seen any changes in the interrupt line, then it
> > > > -      * means that there happen another interrupt while the line was
> > > > active.
> > > > +      * In case the interrupt line is still active then it means that
> > > > +      * there happen another interrupt while the line was active.
> > > >        * So we missed that one, so we need to kick the interrupt again
> > > >        * handler.
> > > >        */
> > > > -     if (active && !ack) {
> > > > +     regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
> > > > +     if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
> > > > +           (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
> > > > +             active = true;
> > > 
> > > Why do you read the line state twice? What happens if the line state
> > > changes right after you've read it?
> > 
> > Here we need to read again the status because we might have clear the
> > ack of interrupt.
> > If the line becomes active right after this read, then the HW will
> > generate another interrupt as the interrupt is enabled and ack is
> > cleared.
> > 
> > > 
> > > > +
> > > > +     if (active) {
> > > >               struct ocelot_irq_work *work;
> > > >
> > > >               work = kmalloc(sizeof(*work), GFP_ATOMIC);
> > > 
> > > So yes, maybe the trade-off that there will be two interrupts are
> > > better than this additional patch. But it should be documented
> > > somewhere, even if it's just a comment in this driver.
> > > 
> > > -michael

-- 
/Horatiu