On 01.06.2021 09:23:38, Torin Cooper-Bennun wrote:
> On Wed, May 26, 2021 at 05:18:33PM +0200, Marc Kleine-Budde wrote:
> > On 26.05.2021 17:07:05, Marc Kleine-Budde wrote:
> > > On 26.05.2021 13:47:44, Torin Cooper-Bennun wrote:
> > > > + if (!ir) {
> > > > + /* Handle device-specific interrupts */
> > > > + if (cdev->ops->handle_dev_interrupts)
> > > > + irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);
> > > > + return irq_ret;
> > > > + }
> > > > +
> > > > + /* ACK M_CAN interrupts */
> > > > + m_can_write(cdev, M_CAN_IR, ir);
> > > >
> > > > + /* ACK device-specific interrupts */
> > > > if (cdev->ops->handle_dev_interrupts)
> > > > cdev->ops->handle_dev_interrupts(cdev, true);
> > >
> > > Why do you call a 2nd time the handle_dev_interrupts() callback?
> >
> > I see, clear and no clear. Why are these two separate operations?
>
> As discussed here,
>
> https://lore.kernel.org/linux-can/20210514141012.3ehw4tosog3lreq4@xxxxxxxxxxxxxx/
>
> you previously recommended adding a 2nd parameter the callback to
> indicate that we only want to clear the device IR, not read it, in order
> to save an SPI transaction when M_CAN core interrupts were already
> handled.
ACK.
I had in mind a single call to handle_dev_interrupts().
In tcan4x5x_clear_interrupts() the original code does 4 individual reg
writes. With:
| tcan4x5x: tcan4x5x_clear_interrupts(): rm unnecessary reg clears
two of them are removed \o/. In Linux we want to avoid individual SPI
transfers at all cost, as they are quite expensive. So the SPI accesses
should be optimized for the no error use case.
If the tcan4x5x interrupts only need clearing/handling in the error
case, it might be an option to only call handle_dev_interrupts() if the
M_CAN_IR register shows no interrupts.
If both m-can and tcan interrupts are pending, in the first run only the
m-can IRQs are handled. If the IRQ line is still asserted the Linux IRQ
code should call the ISR again, no m-can IRQ, then the tcan IRQs will be
handled.
> However, quoting from your previous mail,
>
> > On 26.05.2021 13:47:44, Torin Cooper-Bennun wrote:
> > > Device-specific interrupts are handled, if no M_CAN core interrupts were
> > > handled in the ISR call.
> >
> > In case there are both core and device specific interrupts the kernel
> > IRQ handler will call the ISR a 2nd time - should be OK.
>
> this implies I should leave device IR totally alone unless no M_CAN
> interrupts are asserted, and do the following only:
ACK
> | ir = m_can_read(cdev, M_CAN_IR);
> |
> | if (!ir) {
> | /* Handle device-specific interrupts */
> | if (cdev->ops->handle_dev_interrupts)
> | irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);
ACK - and the handle_dev_interrupts handler should do all necessary
things :)
> | return irq_ret;
> | }
> |
> | /* ACK M_CAN interrupts */
> | m_can_write(cdev, M_CAN_IR, ir);
>
> I can probably also kill off the clear_only parameter.
If the tcan only needs handling/clearing in the error case, then there's
no need for a 2nd parameter.
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 |
Attachment:
signature.asc
Description: PGP signature
