Re: [PATCH can-next 5/5] can: tcan4x5x: implement handling of device interrupts

[Torin Cooper-Bennun <torin@maxiluxsystems.com>]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

On Wed, May 26, 2021 at 05:15:59PM +0200, Marc Kleine-Budde wrote:
> On 26.05.2021 13:47:47, Torin Cooper-Bennun wrote:
> > Handle power, transceiver and SPI failures by printing a useful error
> > message (multiple simultaneous failures are not logged) and disabling
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Is this a limitation of your code or the tcan core?

My code doesn't print an error message for every handled interrupt, only
the first, because it's very rare to see more than one. Perhaps it's
prudent to print a line for each handled interrupt just in case.

> >  static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev,
> >  						  bool clear_only)
> >  {
> > -	tcan4x5x_clear_interrupts(cdev);
> > +	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
> > +	int err = 0;
> > +	irqreturn_t handled = IRQ_NONE;
> 
> nitpick: please make "int err" the last.

ACK

> 
> > +
> > +	if (!clear_only) {
> > +		u32 ir = 0;
> > +		const char *fail_str = "";
> 
> nitpick: please make the u32 the last.

ACK

> > +		else if (ir & TCAN4X5X_CANDOM_INT_EN)
> > +			fail_str = "CAN stuck dominant (CANDOM)";
> 
> The error message suggests, that this error can be triggered by messing
> around with the CAN high/low wires. I'm not sure if it's a good idea to
> shutdown the driver in this case.

ACK, but I need to test whether the device stays functional without CPU
intervention after CANDOM is asserted.

> > +		if (handled == IRQ_HANDLED) {
> > +			netdev_err(cdev->net, "%s: device is disabled.\n",
> 
> Better change the error message and say that the driver is disabling the
> device due to the error.

ACK

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]