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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The transceiver is not put into sleep mode if a regulator is configured, defeating the power-saving goal for shared or always-on regulators.
--

--- Patch [1]: [PATCH v3] can: tcan4x5x: put tcan into sleep when removing driver ---
commit 153dfa0940acefa28cd89e367eedf818096ade5b
Author: Sean Nyekjaer <sean@geanix.com>

can: tcan4x5x: put tcan into sleep when removing driver

This commit attempts to put the tcan4x5x transceiver into sleep mode when
the driver is removed to reduce power consumption. It updates
tcan4x5x_power_enable to send the sleep command via SPI if the RST pin is
available and no regulator is present.

> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 31cc9d0abd453..a318f1874b351 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -211,10 +211,26 @@ static int tcan4x5x_write_fifo(struct m_can_classdev *cdev,
>  	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
>  }
>  
> -static int tcan4x5x_power_enable(struct regulator *reg, int enable)
> +static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enable)
>  {
> -	if (IS_ERR_OR_NULL(reg))
> +	struct regulator *reg = priv->power;
> +
> +	/*
> +	 * Check if the power regulator is configured. If it is, just power
> +	 * the device on/off. If not, put the device into sleep mode here 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.
> +	 * Datasheet: TCAN4550, section "8.4.3 Sleep Mode"
> +	 * https://www.ti.com/lit/gpn/tcan4550
> +	 */
> +	if (IS_ERR_OR_NULL(reg)) {
> +		if (priv->reset_gpio && !enable)
> +			return regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
> +						  TCAN4X5X_MODE_SEL_MASK,
> +						  TCAN4X5X_MODE_SLEEP);
> +
>  		return 0;
> +	}

[Severity: Medium]
Will this miss the power savings if the regulator is shared or marked as
always-on?

Because the sleep command is nested inside the IS_ERR_OR_NULL(reg) check,
the device is not put into sleep mode if a regulator is configured. Instead,
the function continues to the else block and calls regulator_disable().

If the regulator is shared with other active devices or is marked always-on in
the device tree, regulator_disable() will only decrement the reference count
without physically turning off the power. The transceiver will stay fully
powered in its active operating mode.

Should the driver issue the sleep command via SPI whenever priv->reset_gpio
is available, before disabling the regulator, to ensure power is saved regardless
of the physical regulator state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803135011.230006-1-sean@geanix.com?part=1