Re: [PATCH] gpio: pca953x: Improve interrupt support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Jan 27, 2025 at 09:47:17AM +0200, Andy Shevchenko wrote:
> On Tue, Jan 21, 2025 at 12:12 PM lakabd <lakabd.work@xxxxxxxxx> wrote:
> > Le mar. 14 janv. 2025 à 16:44, work work <lakabd.work@xxxxxxxxx> a écrit :
> > > Le mar. 14 janv. 2025 à 10:37, Andy Shevchenko
> > > <andy.shevchenko@xxxxxxxxx> a écrit :
> > > > On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@xxxxxxxxx> wrote:

...

> > > > > +                             /* Store irq_mask for later use when checking pending IRQs */
> > > > > +                             bitmap_or(chip->unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, gc->ngpio);
> > > >
> > > > This solution has a flaw. Where is any code that clears this new
> > > > bitmap? The code starts with 0 (obviously) and step by step it gets
> > > > saturated to all-1s.
> > >
> > > Yes indeed, and actually the new bitmap is not necessary at all
> > > because what we need does already exist which is chip->irq_mask (I
> > > noticed it just now!).
> > > chip->irq_mask is updated whenever a pin is masked or unmasked via
> > > pca953x_irq_mask() and pca953x_irq_unmask().
> > >
> > > The solution should look like this:
> > >
> > > diff --git a/gpio-pca953x.c b/gpio-pca953x.c
> > > index 272febc..29e8c20 100644
> > > --- a/gpio-pca953x.c
> > > +++ b/gpio-pca953x.c
> > > @@ -842,11 +842,6 @@ static bool pca953x_irq_pending(struct
> > > pca953x_chip *chip, unsigned long *pendin
> > >   int ret;
> > >
> > >   if (chip->driver_data & PCA_PCAL) {
> > > - /* Read the current interrupt status from the device */
> > > - ret = pca953x_read_regs(chip, PCAL953X_INT_STAT, trigger);
> > > - if (ret)
> > > - return false;
> > > -
> > >   /* Check latched inputs and clear interrupt status */
> > >   ret = pca953x_read_regs(chip, chip->regs->input, cur_stat);
> > >   if (ret)
> > > @@ -855,7 +850,7 @@ static bool pca953x_irq_pending(struct
> > > pca953x_chip *chip, unsigned long *pendin
> > >   /* Apply filter for rising/falling edge selection */
> > >   bitmap_replace(new_stat, chip->irq_trig_fall, chip->irq_trig_raise,
> > > cur_stat, gc->ngpio);
> > >
> > > - bitmap_and(pending, new_stat, trigger, gc->ngpio);
> > > + bitmap_and(pending, new_stat, chip->irq_mask, gc->ngpio);
> > >
> > >   return !bitmap_empty(pending, gc->ngpio);>
> 
> > >   }
> >
> > Hi Andy, do you have any other suggestions regarding the proposed fix ?
> 
> Currently I'm reading the datasheet to understand how the chip
> actually works. I'll come back to you soon.
> 
> Nevertheless, I would like to hear from Mark if your patch fixes the
> issue. Preliminary I can say that it looks like we need slightly
> different and more complex logic there.
> 
> P.S. Sorry for the delays.

Okay, I have read a lot the datasheet (PCAL9535A), and while Fig.12 shows
an example of what happens in practice, neither the schematic on Fig.6 nor
the description of the interrupt status register doesn't clarify this
behaviour. The bottom line is that the latch helps only to keep the input data
for longer and doesn't anyhow affect the input change on another pin while
servicing the one that asserts the interrupt. Basically what they should have
said is that the interrupt status register snapshot is made on the very first
event and doesn't reflect the actual status anymore in case more input changes
are coming. Hence there is no practical use of the interrupt status register.

Seems to me a good candidate for errata. Does anybody inform NXP about this?

Meanwhile looking into the code I'm wondering why we can't actually use
just input port register data with the logic as for PCAL. Nonetheless this
can be optimized later. I think Mark's patch is good enough as current fix.

-- 
With Best Regards,
Andy Shevchenko