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

[Daniel Mack <daniel@zonque.org>]

Hi Maarten,

On 5/18/20 1:14 PM, Maarten Brock wrote:
> On 2020-05-17 22:44, Daniel Mack wrote:

>>> 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.
> 
> That sounds like a lousy interrupt controller to me. 

While that is true, there are many such controllers around.

> Summerizing:
> - After switching to a threaded IRQ, the trigger could be switched to
> IRQF_TRIGGER_LOW and with that interrupt sharing can be enabled for
> this device with IRQF_SHARED.

Yes, but we don't need that. As discussed, the UART driver can cope with
edge IRQs just fine.

> - Some (your) interrupt controllers do not support IRQF_TRIGGER_LOW.
> For those only IRQF_TRIGGER_FALLING can be used for this device and
> thus IRQF_SHARED cannot be used.

True. Interrupts cannot be shared for this device then. That's a fair
limitation, and it has always been like that.

> - The driver for your interrupt controller should be improved to support
> level IRQs.

It's a controller that sits behind another hardware bus itself, so
polling is expensive. If the controller would need to check for level
IRQs it would need to poll, and then we could as well just poll the UART
directly, that's just as good :)

But again - the UART driver works perfectly fine with edge IRQs as long
as the interrupt is not shared.

> This makes me wonder if it would be better to let the device tree specify
> the interrupt configuration.

There can be flags in the 2nd cell of the node, but their meaning is
specific to the controller. Hence the SPI/I2C layers don't pass that
information up.

What many drivers do is try with one setting, and if that fails because
the interrupt controller returns an error, they fall back to something
else. We could do the same here of course, but it'd be another patch on
top, as it's unrelated to the concrete change the patch we're commenting
on is bringing in.

So what I can add is logic that first tries with IRQF_LOW|IRQF_SHARED,
and if that fails, we fall back to IRQF_FALLING and retry. WDYT?



Thanks,
Daniel