Re: [PATCH] phy: phy-can-transceiver: Add suspend operation for tcan1043

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `.resume` callback in PM operations leaves the CAN PHY permanently in Sleep mode after system resume.
- [Medium] Missing delay when transitioning through Normal mode may result in the transition being ignored by the transceiver hardware.
--

commit 2d2e1a772ce8aac2faea27b0135abc2a7ed70072
Author: Thomas Richard (TI) <thomas.richard@bootlin.com>

phy: phy-can-transceiver: Add suspend operation for tcan1043

This commit adds a suspend operation for the tcan1043 PHY, switching it
to Sleep mode during system suspend. It transitions the PHY through Normal
mode to clear the WAKERQ flag before entering Go-to-Sleep and finally
Sleep mode.

> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 75dc49e75ca0e..2bca1a173fcc3 100644
> --- a/drivers/phy/phy-can-transceiver.c
> +++ b/drivers/phy/phy-can-transceiver.c
[ ... ]
> @@ -76,12 +80,28 @@ static const struct phy_ops can_transceiver_phy_ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +static int tcan1043_suspend(struct can_transceiver_phy *phy)
> +{
> +	/* Switch to Normal mode, it clears WAKERQ */
> +	gpiod_set_value_cansleep(phy->standby_gpio, 0);
> +	gpiod_set_value_cansleep(phy->enable_gpio, 1);
> +
> +	/* Switch to Go-to-Sleep mode */
> +	gpiod_set_value_cansleep(phy->standby_gpio, 1);

[Severity: Medium]
Will this back-to-back GPIO toggle execute too quickly on platforms with fast
memory-mapped GPIOs?

Without an intermediate delay, the sub-microsecond state change might not
give the transceiver's internal state machine enough time to register the
Normal mode transition. Can this cause the transceiver to ignore the
transition, fail to clear the WAKERQ flag, and thus fail to enter Sleep
mode?

> +
> +	/* Wait transition to Sleep mode */
> +	fsleep(5);
> +
> +	return 0;
> +}
[ ... ]
> @@ -115,6 +135,26 @@ static struct phy *can_transceiver_phy_xlate(struct device *dev,
>  	return priv->can_transceiver_phy[idx].generic_phy;
>  }
>  
> +static int can_transceiver_phy_suspend(struct device *dev)
> +{
> +	struct can_transceiver_priv *priv = dev_get_drvdata(dev);
> +	const struct can_transceiver_data *data = priv->data;
> +	int ret, i;
> +
> +	for (i = 0; i < priv->num_ch; i++) {
> +		if (data->suspend) {
> +			ret = data->suspend(&priv->can_transceiver_phy[i]);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(can_transceiver_phy_pm_ops,
> +				can_transceiver_phy_suspend, NULL);

[Severity: High]
Does leaving the resume callback as NULL permanently leave the CAN PHY in
Sleep mode after a system resume?

Since the parent CAN MAC driver typically relies on the PHY framework for
power management and may not manually wake the PHY during its own resume
flow, could this prevent the CAN interface from becoming responsive again
until it is manually brought down and back up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630-phy-can-transceiver-suspend-resume-support-v1-1-6842c2d697e4@bootlin.com?part=1