archive mirror
 help / color / mirror / Atom feed
From: Maarten Brock <>
To: Daniel Mack <>
Subject: Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ
Date: Mon, 18 May 2020 13:14:49 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 2020-05-17 22:44, Daniel Mack wrote:
> 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 <>
>>> ---
>>>  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.

I'm sorry. I remembered this problem and didn't look well enough. It has
been fixed already for 20 months by the keep_polling flag.

> 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 switch to threaded makes it possible to use IRQF_TRIGGER_LOW instead
of IRQF_TRIGGER_FALLING. The current implementation would keep on 
with IRQF_TRIGGER_LOW and the worker thread might not even get executed.

I don't have a clue what a pdata probed device is.

>> 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.

Right, already fixed for a single device.

Different problem then: what if the interrupt is shared with another
device, say another sc16is7xx?

>> 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. But still, I would
expect the interrupt controller driver to handle that problem, not the
device driver for the device hanging off the interrupt controller. Or
at least the interrupt controller driver should throw an error.

> Thanks,
> Daniel

- 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.

- 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.

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

This makes me wonder if it would be better to let the device tree 
the interrupt configuration. Which leads back to my earlier question: 
does one specify in the device tree that an interrupt is shared?

Kind regards,

  reply	other threads:[~2020-05-18 11:15 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-08 14:37 [PATCH 0/4] sc16is7xx: Add IrDA mode and threaded IRQ Daniel Mack
2020-05-08 14:37 ` [PATCH 1/4] dt-bindings: sc16is7xx: Add flag to activate IrDA mode Daniel Mack
2020-05-18 18:08   ` Rob Herring
2020-05-18 18:41     ` Daniel Mack
2020-05-19 12:01       ` Maarten Brock
2020-05-08 14:37 ` [PATCH 2/4] " Daniel Mack
2020-05-08 14:37 ` [PATCH 3/4] sc16is7xx: Always use falling edge IRQ Daniel Mack
2020-05-09 12:41   ` Maarten Brock
2020-05-08 14:37 ` [PATCH 4/4] sc16is7xx: Use threaded IRQ Daniel Mack
2020-05-09 12:55   ` Maarten Brock
2020-05-17 20:44     ` Daniel Mack
2020-05-18 11:14       ` Maarten Brock [this message]
2020-05-18 16:57         ` Daniel Mack
2020-05-19 16:32           ` Maarten Brock
2020-05-19 17:37             ` Daniel Mack

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).