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

[Manivannan Sadhasivam]

On Tue, Jun 30, 2026 at 02:28:46PM +0200, Thomas Richard (TI) wrote:
> Add suspend operation for tcan1043. It switches the PHY in Sleep mode, the
> lowest power mode of the device. If a bus wake-up pattern or a local
> wake-up event occurs, the PHY transitions to Standby mode, set its internal
> WAKERQ flag and set the INH output high. In Sleep mode INH is floating.
> 
> The WAKERQ flag prevents transition to Go-to-Sleep mode. The only way to
> clear it is to switch to Normal mode. So to reach Sleep mode, we firstly
> switch to Normal mode, then to Go-to-Sleep mode.
> 
> Suspend sequence (PHY is off):
> 
>     Standby -> Normal -> Go-to-Sleep -> Sleep
> 
> Suspend sequence (PHY is on):
> 
>     Normal -> Go-to-Sleep -> Sleep
> 
> Signed-off-by: Thomas Richard (TI) <thomas.richard@xxxxxxxxxxx>
> ---
>  drivers/phy/phy-can-transceiver.c | 56 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 49 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 75dc49e75ca0..2bca1a173fcc 100644
> --- a/drivers/phy/phy-can-transceiver.c
> +++ b/drivers/phy/phy-can-transceiver.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com
>   *
>   */
> +#include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> @@ -12,25 +13,28 @@
>  #include <linux/module.h>
>  #include <linux/mux/consumer.h>
>  
> +struct can_transceiver_phy {
> +	struct phy *generic_phy;
> +	struct gpio_desc *silent_gpio;
> +	struct gpio_desc *standby_gpio;
> +	struct gpio_desc *enable_gpio;
> +	struct can_transceiver_priv *priv;
> +};
> +
>  struct can_transceiver_data {
>  	u32 flags;
>  #define CAN_TRANSCEIVER_STB_PRESENT	BIT(0)
>  #define CAN_TRANSCEIVER_EN_PRESENT	BIT(1)
>  #define CAN_TRANSCEIVER_DUAL_CH		BIT(2)
>  #define CAN_TRANSCEIVER_SILENT_PRESENT	BIT(3)
> +	int	(*suspend)(struct can_transceiver_phy *phy);
>  };
>  
> -struct can_transceiver_phy {
> -	struct phy *generic_phy;
> -	struct gpio_desc *silent_gpio;
> -	struct gpio_desc *standby_gpio;
> -	struct gpio_desc *enable_gpio;
> -	struct can_transceiver_priv *priv;
> -};
>  
>  struct can_transceiver_priv {
>  	struct mux_state *mux_state;
>  	int num_ch;
> +	const struct can_transceiver_data *data;
>  	struct can_transceiver_phy can_transceiver_phy[] __counted_by(num_ch);
>  };
>  
> @@ -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);
> +
> +	/* Wait transition to Sleep mode */
> +	fsleep(5);
> +
> +	return 0;
> +}
> +
>  static const struct can_transceiver_data tcan1042_drvdata = {
>  	.flags = CAN_TRANSCEIVER_STB_PRESENT,
>  };
>  
>  static const struct can_transceiver_data tcan1043_drvdata = {
>  	.flags = CAN_TRANSCEIVER_STB_PRESENT | CAN_TRANSCEIVER_EN_PRESENT,
> +	.suspend = tcan1043_suspend,
>  };
>  
>  static const struct can_transceiver_data tja1048_drvdata = {
> @@ -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);

I believe you didn't add resume() callback here based on the assumption that the
CAN transceiver driver will call phy_power_off() during suspend and
phy_power_on() during resume. But that implicit assumption is wrong since the
PHY driver should act on its own and not rely on the consumer's behavior. In
that sense, resume() callback should be present and it should undo suspend().

- Mani

-- 
மணிவண்ணன் சதாசிவம்