Re: [PATCH v2 3/4] phy: rockchip: inno-usb2: move suspend handling into new function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phy: rockchip: inverted logic in rockchip_usb2phy_set_suspend()
--

commit 559f37e7fcbd18c0d269eb9a711cca723c264b19
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: inno-usb2: move suspend handling into new function

This commit refactors the PHY suspend handling into its own dedicated helper
function. It is intended to produce no functional changes to the driver.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index e4d8abf935c1b..925a03fee6bc5 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -332,6 +332,35 @@ rockchip_usb2phy_clk480m_clkout_ctl(struct clk_hw *hw, struct regmap **base,
>  	}
>  }
>  
> +static int rockchip_usb2phy_set_suspend(struct rockchip_usb2phy *rphy,
> +					struct rockchip_usb2phy_port *rport,
> +					bool do_suspend)
> +{
> +	int ret;
> +
> +	ret = property_enable(rphy->grf, &rport->port_cfg->phy_sus, !do_suspend);

[Severity: High]
Does this helper invert the intended logic?

In the original code, rockchip_usb2phy_power_on() called property_enable()
with false to clear the suspend property, and rockchip_usb2phy_power_off()
called it with true to set the property.

By passing !do_suspend here, when callers want to exit suspend and pass
false, we end up passing true to property_enable(). This seems to reverse 
the hardware state for suspend and resume operations.

[ ... ]

> @@ -603,27 +632,12 @@ static int rockchip_usb2phy_power_on(struct phy *phy)
>  	if (ret)
>  		return ret;
>  
> -	ret = property_enable(rphy->grf, &rport->port_cfg->phy_sus, false);
> +	ret = rockchip_usb2phy_set_suspend(rphy, rport, false);

[Severity: High]
Could this cause the PHY to incorrectly suspend when we're trying to power 
it on?

When rockchip_usb2phy_power_on() calls rockchip_usb2phy_set_suspend() with
false, the new helper will execute property_enable(..., true).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-phy-rockchip-inno-usb2-clock-fix-v2-0-e01bf509359e@collabora.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy