Re: [PATCH] can: mcp251xfd: mcp251xfd_open(): request IRQ as shared

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Tue, Jul 27, 2021 at 09:46:17AM +0200, Marc Kleine-Budde wrote:
> On 27.07.2021 12:47:17, Manivannan Sadhasivam wrote:
> > On Sat, Jul 24, 2021 at 10:52:13PM +0200, Marc Kleine-Budde wrote:
> > > The driver's IRQ handler supports shared IRQs, so request a shared IRQ
> > > handler.
> > 
> > I don't see any issue with the idea but I'd like to understand the
> > requirement for it.
> 
> Hardware designers might come up with strange ideas, so better be
> prepared for this. :)
> 
> But seriously, there's a group of people trying to bring the mcp251xfd
> driver to work on ACPI based systems. Having a shared mcp251xfd IRQ
> handler will (hopefully) help them during debugging.
> 

Oh, that's very interesting! ACPI on a ECU :)

> I've written the IRQ handler to properly only return IRQ_HANDLED if
> there really was an interrupt, this means it should be capable running
> as a shared IRQ handler. I've tested it and it works. So let the driver
> request a shared IRQ handler.
> 

I had one more look at it and looks fine to me. The driver authors tend to
return IRQ_HANDLED even during error cases (especially for devices sitting on
buses like i2c, spi) but I hate it. Why would anyone want to enable the
interrupt for the device if it can't be communicated in the ISR?

But you did return IRQ_NONE for error cases, so fine.

> > Usually the IRQ lines are shared when multiple devices use them
> > physically. For instance, a MFD device using a single GPIO for all of its
> > functions. But I don't see any sort of requirement like that here.
> 
> Indeed there is really no good reason to do so, but it works. For
> testing I've connected 2 mcp2518fd to the same IRQ line and the kernel
> runs both IRQ handlers an interrupt is triggered. If course this brings
> the overhead of an additional SPI transfer per IRQ, but it works.
> 
> > Making the IRQ lines shared will only induce latency IMO.
> 
> ACK - you will not get better performance compared to separate IRQ lines
> :) But if you don't use shared interrupts this change doesn't make the
> driver or system any slower or induce latency.
> 

Ack. Feel free to add,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> regards,
> Marc
> 
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |