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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2020-05-18 18:57, Daniel Mack wrote:
Hi Maarten,

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

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.

It has always been like that for this driver. But that should be no
reason why the driver might not be improved. I wonder how the 8250
handles this. PC's have always shared interrupts for COM1/2/3/4 AFAIK.

- 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 :)

That depends on the IRQ coming out of the interrupt controller. If that is
a level interrupt itself, then it is easy to see if all interrupts are
handled. Further polling zooms in on the devices that require attention.

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

If you would require multiple sc16is7xx devices on I2C would you like to
connect multiple interrupt lines? Or just SCL,SDA and *one* IRQ?

OTOH for SPI you would require multiple CS already.

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?

That sounds like a decent plan.


Thanks,
Daniel

Kind regards,
Maarten




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux