> On Sat, 07 Nov 2015 17:32:55 +0100, Maarten Brock wrote:
> > > On Fri, 6 Nov 2015 16:48:20 +0100, Maarten Brock wrote:
> > > > Enable shared interrupts for the sc16is7xx serial driver.
> > > >
> > > > Signed-off-by: Maarten Brock <m.brock@xxxxxxxxxxxxx>
> > > > ---
> > > > drivers/tty/serial/sc16is7xx.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/serial/sc16is7xx.c
> > > b/drivers/tty/serial/sc16is7xx.c
> > > > index 72ffd0d..13f5f69 100644
> > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > @@ -1230,7 +1230,8 @@ static int sc16is7xx_probe(struct device *dev,
> > > >
> > > > /* Setup interrupt */
> > > > ret = devm_request_irq(dev, irq, sc16is7xx_irq,
> > > > - IRQF_ONESHOT | flags, dev_name(dev), s);
> > > > + IRQF_SHARED | IRQF_ONESHOT | flags,
> > > > + dev_name(dev), s);
> > > > if (!ret)
> > > > return 0;
> > > >
> > >
> > > The reason why shared IRQs were not enabled is that we cannot read
> > > the status register from IRQ handler and therefore cannot decide if
> > > the interrupt was actually ours (we always return IRQ_HANDLED). Would
> > > you consider setting the IRQF_SHARED in your device tree instead of
> > > changing the default?
> >
> > Ok, I think I understand why it's difficult or impossible to read the
> > status register from the IRQ handler. But why does that pose a problem when
> > you always return IRQ_HANDLED? Your last remark seems to indicate it doesn't.
>
> I think it doesn't, you'll just loose the ability to detect spurious
> interrupts. We never mask our IRQ so you should be fine (IIUC ONESHOT
> flag is ignored for non-threaded IRQs).
>
> > Or would you recommend never to use shared interrupts with these devices?
>
> Sure, always try not to share IRQ line. Sharing IRQs wastes CPU cycles
> and increases the interrupt latency. I see nothing wrong with your
> patch per-se if it works for you, though. If you still want it
> included in mainline you should probably repost putting Greg KH into
> To: and Cc: linux-serial@xxxxxxxxxxxxxxx.
>
> > Btw. I tried to set it through the device tree, but that didn't seem to
> > work.
> > sc16is740: sc16is740@0 {
> > compatible = "nxp,sc16is740";
> > <snip>
> > interrupts = <25 0x82>; /* 0x80 for shared, 0x02 for falling */
> > }
>
> Sorry if it doesn't work, just something which came to my mind.
I looked a bit more at the probe functions and there is nothing reading the
device tree interrupt flags. No wonder my IRQF_SHARED was ignored. Or should
this have been done by another piece of the kernel?
I don't understand this part though:
if (spi->dev.of_node) {
const struct of_device_id *of_id =
of_match_device(sc16is7xx_dt_ids, &spi->dev);
devtype = (struct sc16is7xx_devtype *)of_id->data;
} else {
const struct spi_device_id *id_entry = spi_get_device_id(spi);
devtype = (struct sc16is7xx_devtype *)id_entry->driver_data;
flags = IRQF_TRIGGER_FALLING;
}
Why is the IRQF_TRIGGER_FALLING only set in the else clause? This is a hard
property of this chip, isn't it? And taking this further, isn't it really a
level interrupt that should use IRQF_TRIGGER_LOW? It would certainly need the
IRQF_ONESHOT with the actual handling being done in sc16is7xx_port_irq.
And why is the irq registered with devm_request_irq and not with
devm_request_threaded_irq? It looks to me like it's threaded with this
queue_kthread_work in sc16is7xx_irq. Am I totally misunderstanding this stuff?
In the meantime I'm going to patch my hardware not to use shared interrupts,
because I have the feeling this driver is not ready for it. Even though it did
seem to work.
Maarten
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
