On Tue, 10 Nov 2015 15:36:21 +0100, Maarten Brock wrote:
> > 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 *think* by other part of the kernel. But
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
says only egde/level can be configured that way which makes my
suggestion to use it for IRQF_SHARED quite nonsensical.
> 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.
We want the edge. If I understand this code correctly (note: it's
copied from max310x.c on which this driver was originally based so it's
quite old code) the else branch handles non-device tree configurations.
Kernel will read this flag from the device tree normally, but if the
device is probed using some other mechanism, it will default to the
setting of *_FALLING.
We don't hardwire to *_FALLING always, though, since people may use
inverters etc.
> 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?
We create our own kworker for handling async configuration anyway (since
RT-prio control on workqueues is a mess) so we just use that for irq as
well. It saves some resources and simplifies the code a bit.
> 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.
The driver should work fine - but if you can - definitely try to avoid
sharing IRQs.
--
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
