Re: [PATCH] gpio: pca953x: enable latch only on edge-triggered inputs

[Ian Ray <ian.ray@gehealthcare.com>]

On Thu, Oct 09, 2025 at 11:24:28AM +0100, Martyn Welch wrote:
> CAUTION: This email originated from outside of GE HealthCare. Only open links or attachments if you trust the sender. Report suspicious emails using Outlook’s “Report” button.
> 
> On 09/10/2025 08:44, Francesco Lavra wrote:
> > On Thu, 2025-10-09 at 08:17 +0100, Martyn Welch wrote:
> >> On 09/10/2025 07:03, Linus Walleij wrote:
> >>> Hi Francesco,
> >>>
> >>> thanks for your patch!
> >>>
> >>> On Wed, Oct 8, 2025 at 12:43 PM Francesco Lavra <flavra@baylibre.com>
> >>> wrote:
> >>>
> >>>
> >>>> The latched input feature of the pca953x GPIO controller is useful
> >>>> when an input is configured to trigger interrupts on rising or
> >>>> falling edges, because it allows retrieving which edge type caused
> >>>> a given interrupt even if the pin state changes again before the
> >>>> interrupt handler has a chance to run. But for level-triggered
> >>>> interrupts, reading the latched input state can cause an active
> >>>> interrupt condition to be missed, e.g. if an active-low signal (for
> >>>> which an IRQ_TYPE_LEVEL_LOW interrupt has been configured) triggers
> >>>> an interrupt when switching to the inactive state, but then becomes
> >>>> active again before the interrupt handler has a chance to run: in
> >>>> this case, if the interrupt handler reads the latched input state,
> >>>> it will wrongly assume that the interrupt is not pending.
> >>>> Fix the above issue by enabling the latch only on edge-triggered
> >>>> inputs, instead of all interrupt-enabled inputs.
> >>>>
> >>>> Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> >>>> ---
> >>>>    drivers/gpio/gpio-pca953x.c | 7 +++++--
> >>>>    1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-
> >>>> pca953x.c
> >>>> index e80a96f39788..e87ef2c3ff82 100644
> >>>> --- a/drivers/gpio/gpio-pca953x.c
> >>>> +++ b/drivers/gpio/gpio-pca953x.c
> >>>> @@ -761,10 +761,13 @@ static void pca953x_irq_bus_sync_unlock(struct
> >>>> irq_data *d)
> >>>>           int level;
> >>>>
> >>>>           if (chip->driver_data & PCA_PCAL) {
> >>>> +               DECLARE_BITMAP(latched_inputs, MAX_LINE);
> >>>>                   guard(mutex)(&chip->i2c_lock);
> >>>>
> >>>> -               /* Enable latch on interrupt-enabled inputs */
> >>>> -               pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip-
> >>>>> irq_mask);
> >>>> +               /* Enable latch on edge-triggered interrupt-enabled
> >>>> inputs */
> >>>> +               bitmap_or(latched_inputs, chip->irq_trig_fall, chip-
> >>>>> irq_trig_raise, gc->ngpio);
> >>>> +               bitmap_and(latched_inputs, latched_inputs, chip-
> >>>>> irq_mask, gc->ngpio);
> >>>> +               pca953x_write_regs(chip, PCAL953X_IN_LATCH,
> >>>> latched_inputs);
> >>>
> >>> This driver is used by a *lot* of systems and people.
> >>>
> >>> It is maybe the most used GPIO driver in the kernel.
> >>>
> >>> So I added a lot of affected developers to the To: line of
> >>> the mail so we can get a wider review and testing.
> >>>
> >>
> >> I don't have access to the relevant hardware to test this anymore and
> >> it's been a while since I thought much about edge vs. level triggered
> >> interrupts. But if the state of the interrupt is unilaterally returning
> >> to an inactive state, it sounds like that should be configured as an
> >> edge triggered interrupt, not a level triggered one...
> >
> > I will try to better describe the problematic scenario:
> > - a device has an IRQ line that becomes active when the device needs to be
> > serviced, and becomes inactive when the device has been serviced (e.g. by
> > reading a status register); this is the classic use case for level-
> > triggered interrupts
> > - the IRQ line of this device is connected to a pca953x input, and this
> > input is configured as a level-triggered interrupt
> > - the device IRQ line becomes active, this triggers an interrupt in the
> > pca953x, the pca953x interrupt handler is invoked, it reads the input
> > state, then calls the nested interrupt handler
> > - the nested interrupt handler services the device, which causes the IRQ
> > line to become inactive: this triggers a second interrupt in the pca953x
> > - before the pca953x interrupt handler is invoked for the second time, the
> > device IRQ line becomes active again
> > - the pca953x interrupt handler is invoked, it reads the input state, which
> > shows the line as inactive (because that is the state that triggered the
> > second interrupt), and as a result the nested interrupt handler is not
> > invoked, and the device will stay forever with the interrupt line asserted
> >
> > With my proposed change, in the last step above the pca953x interrupt
> > handler will read the current input state instead of the state that caused
> > the second interrupt, and thus will correctly invoke the nested interrupt
> > handler for the second time.
> 
> Thanks for the detail Francesco.
> 
> To me it seems that the latching is more or less required in the edge
> triggered case because, as the hardware manual for the PCAL6408A at
> least states:
> 
>      When an input latch register bit is 0, the corresponding input pin
>      state is not latched...    ...If the input goes back to its initial
>      logic state before the Input port register is read, then the
>      interrupt is cleared.
> 
> So the hardware doesn't retain which bits triggered the interrupt
> without the latching.
> 
> For level based interrupts I think you're right and it doesn't make
> sense to latch the value.
> 
> This change seems sensible to me.
> 
> Reviewed-by: Martyn Welch <martyn.welch@collabora.com>

Makes sense to me.

(Untested as the hardware I have access to does not use the IRQ
line.)

Reviewed-by: Ian Ray <ian.ray@gehealthcare.com>