Re: [PATCH v4] can: tcan4x5x: put tcan into sleep when removing driver

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Early return on SPI error in `tcan4x5x_power_enable(priv, 0)` bypasses `regulator_disable()`, causing a regulator enable count leak.
--

--- Patch [1]: [PATCH v4] can: tcan4x5x: put tcan into sleep when removing driver ---
commit 277a5e1f34c213275f7c42975ff590a923bc1148
Author: Sean Nyekjaer <[email protected]>

can: tcan4x5x: put tcan into sleep when removing driver

This commit modifies the driver to put the TCAN4x5x transceiver into sleep
mode upon removal. It issues a sleep command via SPI to reduce power
consumption when the driver unbinds. The sleep command is integrated into
tcan4x5x_power_enable().

> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 31cc9d0abd453..d8fd219a27cc5 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c

[ ... ]

> -static int tcan4x5x_power_enable(struct regulator *reg, int enable)
> +static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enable)
>  {
> +	struct regulator *reg = priv->power;
> +	int ret;
> +
> +	/*
> +	 * Put the device into sleep mode if the RST pin is available,
> +	 * since a wake-up event, RST pin toggle, or power cycle are the only
> +	 * ways to exit sleep mode.
> +	 * Redundant if the regulator is exclusive to this device, but that
> +	 * can't be determined here.
> +	 *
> +	 * Datasheet: TCAN4550, section "8.4.3 Sleep Mode"
> +	 * https://www.ti.com/lit/gpn/tcan4550
> +	 */
> +	if (priv->reset_gpio && !enable) {
> +		ret = regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
> +					 TCAN4X5X_MODE_SEL_MASK,
> +					 TCAN4X5X_MODE_SLEEP);
> +		if (ret)
> +			return ret;

[Severity: Medium]
If this regmap_update_bits() fails over SPI during the power-off sequence,
could this early return bypass the regulator_disable() call below?

It looks like this might leak the regulator enable count, which would
prevent the regulator from ever being disabled and defeat the power-saving
purpose of the patch.

> +	}
> +
>  	if (IS_ERR_OR_NULL(reg))
>  		return 0;
>  
>  	if (enable)
>  		return regulator_enable(reg);
>  	else
>  		return regulator_disable(reg);
>  }

[ ... ]

> @@ -543,7 +564,7 @@ static void tcan4x5x_can_remove(struct spi_device *spi)
>  
>  	m_can_class_unregister(&priv->cdev);
>  
> -	tcan4x5x_power_enable(priv->power, 0);
> +	tcan4x5x_power_enable(priv, 0);

[Severity: Medium]
Because tcan4x5x_can_remove() ignores the return value of
tcan4x5x_power_enable(), any error returned from the regmap_update_bits()
failure mentioned above would be silently dropped. 

Would it be safer to ensure the regulator is still disabled even if the
SPI command fails, so the failure doesn't silently keep the power on?

>  
>  	m_can_class_free_dev(priv->cdev.net);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1