Message-ID: <aca89e7d-421f-490f-8c3a-c2d9567dbc26@collabora.com>
Date: Thu, 9 Oct 2025 11:24:28 +0100
From: Martyn Welch <martyn.welch@...labora.com>
To: Francesco Lavra <flavra@...libre.com>,
 Linus Walleij <linus.walleij@...aro.org>,
 Hugo Villeneuve <hvilleneuve@...onoff.com>,
 Maria Garcia <mariagarcia7293@...il.com>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Emanuele Ghidoli <emanuele.ghidoli@...adex.com>,
 Potin Lai <potin.lai.pt@...il.com>,
 Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>,
 Fabio Estevam <festevam@...x.de>, Ian Ray <ian.ray@...ealthcare.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpio: pca953x: enable latch only on edge-triggered inputs



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@...libre.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@...libre.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@...labora.com>

Powered by blists - more mailing lists