Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ

[Daniel Mack <daniel@zonque.org>]

Hi Maarten,

Thanks for your review!

On 5/9/20 2:55 PM, Maarten Brock wrote:
> On 2020-05-08 16:37, Daniel Mack wrote:
>> Use a threaded IRQ handler to get rid of the irq_work kthread.
>> This also allows for the driver to use interrupts generated by
>> a threaded controller.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>>  drivers/tty/serial/sc16is7xx.c | 18 +++++-------------
>>  1 file changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c
>> b/drivers/tty/serial/sc16is7xx.c

>> @@ -1317,8 +1308,9 @@ static int sc16is7xx_probe(struct device *dev,
>>      }
>>
>>      /* Setup interrupt */
>> -    ret = devm_request_irq(dev, irq, sc16is7xx_irq,
>> -                   IRQF_TRIGGER_FALLING, dev_name(dev), s);
>> +    ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
>> +                    IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +                    dev_name(dev), s);
>>      if (!ret)
>>          return 0;
> 
> Since UART0 is first handled completely in the for loop before UART1 is
> handled, a new interrupt may arise on UART0 while UART1 is being handled.

The code in the interrupt handling function loops forever until there is
no more interrupt bits pending. So if there is a new IRQ happening for
UART0 while UART1 is being served, it will be handled in the same loop.

And just to be sure I understand correctly: this is unrelated to the
switch to threaded IRQs, right? Falling edge triggers were always used
for pdata probed devices.

> The result is a missed interrupt since the IRQ line might not *FALL* again.

It doesn't have to. We only exit the interrupt handler when there is
nothing left to do, at which point the IRQ line ist back high. So it
will fall again in case of new events.

> Therefor I suggest to change IRQF_TRIGGER_FALLING to IRQF_TRIGGER_LOW. This
> way the thread will be retriggered after IRQ_HANDLED is returned.

This doesn't work in my setup unfortunately, as the interrupt controller
is incapable of handling level IRQs.


Thanks,
Daniel