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@xxxxxxxxxxxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
