Re: [PATCH RFC can-next 3/3] can: tcan4x5x: add handle_dev_interrupts callback to ops

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

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

On 14.05.2021 13:19:46, Torin Cooper-Bennun wrote:
> Though the TCAN4550's device-specific interrupts are cleared in
> tcan4x5x_clear_interrupts(), they are ignored, which may cause the m_can
> driver to stop working due to the ISR becoming disabled (the famous
> "nobody cared" message).
> 
> Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 4147cecfbbd6..cee7dfff381f 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -39,6 +39,7 @@
>  #define TCAN4X5X_CANBUS_ERR_INT_EN BIT(5)
>  #define TCAN4X5X_BUS_FAULT BIT(4)
>  #define TCAN4X5X_MCAN_INT BIT(1)
> +#define TCAN4X5X_VTWD_INT BIT(0)
>  #define TCAN4X5X_ENABLE_TCAN_INT \
>  	(TCAN4X5X_MCAN_INT | TCAN4X5X_BUS_FAULT | \
>  	 TCAN4X5X_CANBUS_ERR_INT_EN | TCAN4X5X_CANINT_INT_EN)
> @@ -190,6 +191,16 @@ static int tcan4x5x_power_enable(struct regulator *reg, int enable)
>  		return regulator_disable(reg);
>  }
>  
> +static u32 tcan4x5x_read_tcan_reg(struct m_can_classdev *cdev, int reg)
> +{
> +	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
> +	u32 val;
> +
> +	regmap_read(priv->regmap, reg, &val);
> +
> +	return val;
> +}
> +
>  static int tcan4x5x_write_tcan_reg(struct m_can_classdev *cdev,
>  				   int reg, int val)
>  {
> @@ -221,6 +232,19 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
>  				       TCAN4X5X_CLEAR_ALL_INT);
>  }
>  
> +static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev)
> +{
> +	irqreturn_t ret = IRQ_NONE;
> +	int ir;
> +
> +	ir = tcan4x5x_read_tcan_reg(cdev, TCAN4X5X_INT_FLAGS);

For new code, please don't wrap the regmap_*() functions so that the
error values are ignored. I know, it's a bit annoying to always do the
"if (err) return err" dance.

As this is the interrupt handler there's not much we can do in case of
an error. In the mcp251xfd driver I print an error message and shut down
the interface. You should at least print an error message at the end of
the handle_interrupts() function.

I think it's best to have a single handle_interrupts() callback that
combines the existing clear_interrupts and this code. If you want to
save an SPI transaction and only read the TCAN4X5X_INT_FLAGS if the
M_CAN_IR is unset, pass is as 2nd parameter from the generic interrupt
handler.

> +
> +	if (ir & (TCAN4X5X_CANDOM_INT_EN | TCAN4X5X_VTWD_INT))
> +		ret = IRQ_HANDLED;
> +
> +	return ret;
> +}
> +
>  static int tcan4x5x_init(struct m_can_classdev *cdev)
>  {
>  	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
> @@ -305,6 +329,7 @@ static struct m_can_ops tcan4x5x_ops = {
>  	.write_fifo = tcan4x5x_write_fifo,
>  	.read_fifo = tcan4x5x_read_fifo,
>  	.clear_interrupts = tcan4x5x_clear_interrupts,
> +	.handle_dev_interrupts = tcan4x5x_handle_dev_interrupts,
>  };
>  
>  static int tcan4x5x_can_probe(struct spi_device *spi)

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 --]