| Message ID | 20240606033102.2271916-1-mark.tomlinson@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: pca953x: Improve interrupt support | expand |
Thu, Jun 06, 2024 at 03:31:02PM +1200, Mark Tomlinson kirjoitti: > The GPIO drivers with latch interrupt support (typically types starting > with PCAL) have interrupt status registers to determine which particular > inputs have caused an interrupt. Unfortunately there is no atomic > operation to read these registers and clear the interrupt. Clearing the > interrupt is done by reading the input registers. What you are describing sounds to me like the case without latch enabled. Can you elaborate a bit more? > The code was reading the interrupt status registers, and then reading > the input registers. If an input changed between these two events it was > lost. I don't see how. If there is a short pulse or a series of pulses between interrupt latching and input reading, the second+ will be lost in any case. This is HW limitation as far as I can see. > The solution in this patch is to revert to the non-latch version of > code, i.e. remembering the previous input status, and looking for the > changes. This system results in no more I2C transfers, so is no slower. > The latch property of the device still means interrupts will still be > noticed if the input changes back to its initial state. Again, can you elaborate? Is it a real use case? If so, can you provide the chart of the pin sginalling against the time line and depict where the problem is?
On Sat, 2024-06-08 at 12:44 +0300, Andy Shevchenko wrote: > Thu, Jun 06, 2024 at 03:31:02PM +1200, Mark Tomlinson kirjoitti: > > The GPIO drivers with latch interrupt support (typically types starting > > with PCAL) have interrupt status registers to determine which > > particular > > inputs have caused an interrupt. Unfortunately there is no atomic > > operation to read these registers and clear the interrupt. Clearing the > > interrupt is done by reading the input registers. > > What you are describing sounds to me like the case without latch enabled. > Can you elaborate a bit more? The latch is useful when an input changes state, but changes back again before the input is read. Using the latch causes the input register to show what caused the interrupt, rather than the current state of the pin. The problem I have is not related to the latch as the inputs are not changing back to their original state. I have two inputs which change state at almost the same time. When the first input changes state, an interrupt occurs. Prior to my patch, the interrupt status register was read, and only this one interrupt is shown as pending. The second input changes state between reading the interrupt status and reading the input (which clears both interrupt sources). So I only get the one interrupt and not both. > > The code was reading the interrupt status registers, and then reading > > the input registers. If an input changed between these two events it > > was > > lost. > > I don't see how. If there is a short pulse or a series of pulses between > interrupt latching and input reading, the second+ will be lost in any > case. > This is HW limitation as far as I can see. I feel you're thinking of the single input pin case. There is no issue with a single pin pulsing as the latch will keep the value which caused the interrupt until it is read. The interrupt status register will have the correct value too. > > > The solution in this patch is to revert to the non-latch version of > > code, i.e. remembering the previous input status, and looking for the > > changes. This system results in no more I2C transfers, so is no slower. > > The latch property of the device still means interrupts will still be > > noticed if the input changes back to its initial state. > > Again, can you elaborate? Is it a real use case? If so, can you provide > the > chart of the pin sginalling against the time line and depict where the > problem > is? Yes, this is real. Hopefully the above description explains what we're seeing, but as a picture is worth 1000 words, here's a timeline: --------+ Input 1 | +--------------------------------------------- ----------------------------------+ Input 2 | +------------------- --------+ +------- IRQ | | +-------------------------------------+ Interrupt status * Register Read Input Register * Read Note that the interrupt status read only sees one event, but both are cleared later. As these two reads are I2C bus transfers, they are more than 100µs apart, so this event occurs quite frequently in our system. Thanks for reviewing this. Best Regards,
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index 77a2812f2974..14b80cb00274 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -839,25 +839,6 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, unsigned long *pendin DECLARE_BITMAP(trigger, MAX_LINE); 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) - return false; - - /* 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); - - return !bitmap_empty(pending, gc->ngpio); - } - ret = pca953x_read_regs(chip, chip->regs->input, cur_stat); if (ret) return false;
The GPIO drivers with latch interrupt support (typically types starting with PCAL) have interrupt status registers to determine which particular inputs have caused an interrupt. Unfortunately there is no atomic operation to read these registers and clear the interrupt. Clearing the interrupt is done by reading the input registers. The code was reading the interrupt status registers, and then reading the input registers. If an input changed between these two events it was lost. The solution in this patch is to revert to the non-latch version of code, i.e. remembering the previous input status, and looking for the changes. This system results in no more I2C transfers, so is no slower. The latch property of the device still means interrupts will still be noticed if the input changes back to its initial state. Fixes: 44896beae605 ("gpio: pca953x: add PCAL9535 interrupt support for Galileo Gen2") Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz> --- drivers/gpio/gpio-pca953x.c | 19 ------------------- 1 file changed, 19 deletions(-)