[Linux-stm32] [PATCH net-next] net: dwmac-rk: No need to check the return value of the phy_power_on()

[lizhe]

Hi, 
  Is gmac_clk_enable() also redundant?

    it never gets executed because phy_power_on() always returns false Therefore, the following approach might be a fix :  
    diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 51ea0caf16c1..085c0d669eca 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -1461,7 +1461,7 @@ static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
        return 0;
 }


-static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
+static int rk_phy_power_set(struct rk_priv_data *bsp_priv, bool enable)
 {
        struct regulator *ldo = bsp_priv->regulator;
        struct device *dev = bsp_priv->dev;
@@ -1477,7 +1477,7 @@ static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
                        dev_err(dev, "fail to disable phy-supply\n");
        }


- return 0;
+ return ret;
 }


 static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
@@ -1655,7 +1655,7 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
                dev_err(dev, "NO interface defined!\n");
        }


- ret = phy_power_on(bsp_priv, true);
+ ret = rk_phy_power_set(bsp_priv, true);
        if (ret) {
                gmac_clk_enable(bsp_priv, false);
                return ret;
@@ -1676,7 +1676,7 @@ static void rk_gmac_powerdown(struct rk_priv_data *gmac)


        pm_runtime_put_sync(gmac->dev);


- phy_power_on(gmac, false);
+ rk_phy_power_set(gmac, false);
        gmac_clk_enable(gmac, false);
 }


--
2.17.1












At 2025-10-15 14:17:29, "Chaoyi Chen" <chaoyi.chen at rock-chips.com> wrote:
>On 10/15/2025 12:08 PM, Lizhe wrote:
>
>> 'phy_power_on' is a local scope one within the driver, since the return
>> value of the phy_power_on() function is always 0, checking its return
>> value is redundant.
>>
>> the function name 'phy_power_on()' conflicts with the existing
>> phy_power_on() function in the PHY subsystem. a suitable alternative
>> name would be rk_phy_power_set(), particularly since when the second
>> argument is false, this function actually powers off the PHY
>>
>> Signed-off-by: Lizhe <sensor1010 at 163.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 17 ++++-------------
>>   1 file changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> index 51ea0caf16c1..9d296bfab013 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> @@ -1461,23 +1461,18 @@ static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>>   	return 0;
>>   }
>>   
>> -static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>> +static void rk_phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>>   {
>>   	struct regulator *ldo = bsp_priv->regulator;
>>   	struct device *dev = bsp_priv->dev;
>> -	int ret;
>>   
>>   	if (enable) {
>> -		ret = regulator_enable(ldo);
>> -		if (ret)
>> +		if (regulator_enable(ldo))
>>   			dev_err(dev, "fail to enable phy-supply\n");
>>   	} else {
>> -		ret = regulator_disable(ldo);
>> -		if (ret)
>> +		if (regulator_disable(ldo))
>>   			dev_err(dev, "fail to disable phy-supply\n");
>>   	}
>> -
>> -	return 0;
>>   }
>>   
>>   static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
>> @@ -1655,11 +1650,7 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>>   		dev_err(dev, "NO interface defined!\n");
>>   	}
>>   
>> -	ret = phy_power_on(bsp_priv, true);
>> -	if (ret) {
>> -		gmac_clk_enable(bsp_priv, false);
>
>Is gmac_clk_enable() also redundant?
>
>
>> -		return ret;
>> -	}
>> +	rk_phy_power_on(bsp_priv, true);
>>   
>>   	pm_runtime_get_sync(dev);
>>   
>
>-- 
>Best,
>Chaoyi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://st-md-mailman.stormreply.com/pipermail/linux-stm32/attachments/20251015/63860a98/attachment.html>