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

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

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?

> the TCAN4550 by setting it to standby mode.
> 
> Additionally, print an error message if any regmap access fails in the
> tcan4x5x_handle_dev_interrupts() call.
> 
> Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 50 ++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index a300a14dc5de..2016a4b54a44 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -38,6 +38,7 @@
>  #define TCAN4X5X_CANDOM_INT_EN BIT(8)
>  #define TCAN4X5X_CANBUS_ERR_INT_EN BIT(5)
>  #define TCAN4X5X_BUS_FAULT BIT(4)
> +#define TCAN4X5X_SPIERR_INT BIT(3)
>  #define TCAN4X5X_MCAN_INT BIT(1)
>  #define TCAN4X5X_ENABLE_TCAN_INT \
>  	(TCAN4X5X_UVSUP_INT_EN | TCAN4X5X_UVIO_INT_EN | TCAN4X5X_TSD_INT_EN | \
> @@ -214,7 +215,54 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
>  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.

> +
> +	if (!clear_only) {
> +		u32 ir = 0;
> +		const char *fail_str = "";

nitpick: please make the u32 the last.

> +
> +		err = regmap_read(priv->regmap, TCAN4X5X_INT_FLAGS, &ir);
> +		if (err)
> +			goto exit_regmap_failure;
> +
> +		handled = IRQ_HANDLED;
> +
> +		if (ir & TCAN4X5X_UVSUP_INT_EN)
> +			fail_str = "supply under-voltage (UVSUP)";
> +		else if (ir & TCAN4X5X_UVIO_INT_EN)
> +			fail_str = "I/O under-voltage (UVIO)";
> +		else if (ir & TCAN4X5X_TSD_INT_EN)
> +			fail_str = "thermal shutdown (TSD)";
> +		else if (ir & TCAN4X5X_ECCERR_INT_EN)
> +			fail_str = "uncorrectable ECC error (ECCERR)";
> +		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.

> +		else if (ir & TCAN4X5X_SPIERR_INT)
> +			fail_str = "SPI error (SPIERR)";
> +		else
> +			handled = IRQ_NONE;
> +
> +		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.

> +				   fail_str);
> +			err = regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
> +						 TCAN4X5X_MODE_SEL_MASK,
> +						 TCAN4X5X_MODE_STANDBY);
> +			if (err)
> +				goto exit_regmap_failure;
> +		}
> +	}
> +
> +	err = tcan4x5x_clear_interrupts(cdev);
> +	if (err)
> +		goto exit_regmap_failure;
> +
> +	return handled;
> +
> +exit_regmap_failure:
> +	netdev_err(cdev->net, "regmap access failed in IRQ handler.\n");
>  	return IRQ_NONE;
>  }
>  
> -- 
> 2.30.2
> 
>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]