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

--z46ei6zew2ialjou
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PATCH v3] can: tcan4x5x: put tcan into sleep when removing
 driver
MIME-Version: 1.0

On 03.08.2026 14:09:16, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issu=
e(s) to consider:
> - [Medium] The transceiver is not put into sleep mode if a regulator is c=
onfigured, defeating the power-saving goal for shared or always-on regulato=
rs.
> --
>
> --- Patch [1]: [PATCH v3] can: tcan4x5x: put tcan into sleep when removin=
g driver ---
> commit 153dfa0940acefa28cd89e367eedf818096ade5b
> Author: Sean Nyekjaer <[email protected]>
>
> 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_class=
dev *cdev,
> >  	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_off=
set, val, val_count);
> >  }
> >
> > -static int tcan4x5x_power_enable(struct regulator *reg, int enable)
> > +static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enabl=
e)
> >  {
> > -	if (IS_ERR_OR_NULL(reg))
> > +	struct regulator *reg =3D 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. Inste=
ad,
> 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 co=
unt
> 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_gp=
io
> is available, before disabling the regulator, to ensure power is saved re=
gardless
> of the physical regulator state?

Sean, sounds like a valid point to me, what do you think? The additional
SPI message, even if the device is powered off, should not hurt.

regards,
Marc

--=20
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung N=C3=BCrnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

--z46ei6zew2ialjou
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQSl+MghEFFAdY3pYJLMOmT6rpmt0gUCanChogAKCRDMOmT6rpmt
0os0AP45TRVs4w1o1MkOvgMcXb1RloTfSq0NZEwNyawc4A+4zAD/SHTuZJDUtYbC
brLN8578QZ0/p8n6WdYCmEzb/dqJNg4=
=5zJn
-----END PGP SIGNATURE-----

--z46ei6zew2ialjou--