Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking

[Chen Jeffy <jeffy.chen@rock-chips.com>]

Hi Doug,

It's been a long time, hope you're doing well :)

On 8/23 星期二 1:08, Doug Anderson wrote:
> Hi,
> 
> On Sat, Aug 20, 2022 at 3:07 AM Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>>
>> Otherwise the trigger mode might be out-of-sync when a level change
>> occurred in between.
>>
>> Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>>   drivers/gpio/gpio-rockchip.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
>> index a98351cd6821..736b4d90f1ca 100644
>> --- a/drivers/gpio/gpio-rockchip.c
>> +++ b/drivers/gpio/gpio-rockchip.c
>> @@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>>                  irq = __ffs(pend);
>>                  pend &= ~BIT(irq);
>>
>> -               dev_dbg(bank->dev, "handling irq %d\n", irq);
>> +               generic_handle_domain_irq(bank->domain, irq);
>>
>>                  /*
>>                   * Triggering IRQ on both rising and falling edge
>> @@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>>                                                       bank->gpio_regs->ext_port);
>>                          } while ((data & BIT(irq)) != (data_old & BIT(irq)));
>>                  }
>> -
>> -               generic_handle_domain_irq(bank->domain, irq);
> 
> I'm happy to let others say for sure, but from my point of view I'm
> not convinced. It feels like with your new code you could lose edges.
> 
> The abstraction I always assume for edge triggered interrupts is that
> multiple edges are coalesced into one IRQ but that if an edge comes in
> after the first line of the IRQ handler starts executing then the IRQ
> handler will run again. In other words:
> 
> - edge A
> - edge B
> - IRQ handler starts running (once for A/B)
> - IRQ handler finishes running
> - <idle>
> - edge C
> - IRQ handler starts running (for C)
> - edge D
> - edge E
> - IRQ handler finishes running
> - IRQ handler starts running (for D/E)
> - IRQ handler finishes running
> - <idle>

The thing is, we are currently toggling the trigger mode to make sure it 
matches the current GPIO level (e.g. level low -> rising edge mode), 
than ack it in gpio IRQ handler.

So if an edge come in between, that new IRQ status would be 
acked(cleared) in the following GPIO irq handler as well as the old one, 
without triggering another IRQ demux() to toggle the trigger mode.

- rising edge
- toggle to falling edge mode
- GPIO high with falling edge mode <-- correct
- falling edge
- IRQ handler acked that IRQ
- IRQ handler finished
- GPIO low with falling edge mode <-- oops
- rising edge <-- missed

> 
> For your new code I don't think that will necessarily be the case. I
> think this can happen with your new code:
> 
> - rising edge
> - IRQ handler starts running for rising edge
> - IRQ handler finishes running for rising edge
> - falling edge (not latched since we're looking for rising edges)
> - notice that level is low
> - keep it configured for rising edge
> 
> ...in other words an edge happened _after_ the IRQ handler ran but we
> didn't call the IRQ handler again. I don't think this is right.
> 

Right, so guessing we could somehow move the IRQ ack into the toggling 
flow to make sure that it would not clear the new IRQ?

And it looks like there are quite a few drivers having this kind of 
need, would it make sense to handle it in the framework?

> 
> What problem are you trying to solve?
> 
> -Doug
>