[PATCH v3] net: stmmac: avoid repeated devm_gpiod_get_optional in mdio

[PATCH v3] net: stmmac: avoid repeated devm_gpiod_get_optional in mdio

[Liu Xiang]

During system resume, stmmac_resume() calls stmmac_mdio_reset().
This results in repeated calls to devm_gpiod_get_optional() for the same
GPIO. The second invocation returns an error since the GPIO descriptor has
already been obtained.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  2 +
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 43 +++++++++++--------
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 33667a267..34ebc6f3a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -372,6 +372,8 @@ struct stmmac_priv {
 	struct bpf_prog *xdp_prog;
 
 	struct devlink *devlink;
+
+	struct gpio_desc *reset_gpio;
 };
 
 enum stmmac_state {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index a7c2496b3..c0b35b8e2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -386,29 +386,26 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 
 #ifdef CONFIG_OF
 	if (priv->device->of_node) {
-		struct gpio_desc *reset_gpio;
-		u32 delays[3] = { 0, 0, 0 };
+		if (priv->reset_gpio) {
+			u32 delays[3] = { 0, 0, 0 };
 
-		reset_gpio = devm_gpiod_get_optional(priv->device,
-						     "snps,reset",
-						     GPIOD_OUT_LOW);
-		if (IS_ERR(reset_gpio))
-			return PTR_ERR(reset_gpio);
+			device_property_read_u32_array(priv->device,
+						       "snps,reset-delays-us",
+						       delays,
+						       ARRAY_SIZE(delays));
 
-		device_property_read_u32_array(priv->device,
-					       "snps,reset-delays-us",
-					       delays, ARRAY_SIZE(delays));
+			gpiod_set_value_cansleep(priv->reset_gpio, 0);
+			if (delays[0])
+				msleep(DIV_ROUND_UP(delays[0], 1000));
 
-		if (delays[0])
-			msleep(DIV_ROUND_UP(delays[0], 1000));
+			gpiod_set_value_cansleep(priv->reset_gpio, 1);
+			if (delays[1])
+				msleep(DIV_ROUND_UP(delays[1], 1000));
 
-		gpiod_set_value_cansleep(reset_gpio, 1);
-		if (delays[1])
-			msleep(DIV_ROUND_UP(delays[1], 1000));
-
-		gpiod_set_value_cansleep(reset_gpio, 0);
-		if (delays[2])
-			msleep(DIV_ROUND_UP(delays[2], 1000));
+			gpiod_set_value_cansleep(priv->reset_gpio, 0);
+			if (delays[2])
+				msleep(DIV_ROUND_UP(delays[2], 1000));
+		}
 	}
 #endif
 
@@ -648,6 +645,14 @@ int stmmac_mdio_register(struct net_device *ndev)
 	if (priv->plat->core_type == DWMAC_CORE_XGMAC)
 		stmmac_xgmac2_mdio_read_c45(new_bus, 0, 0, 0);
 
+	priv->reset_gpio = devm_gpiod_get_optional(priv->device,
+						   "snps,reset",
+						    GPIOD_ASIS);
+	if (IS_ERR(priv->reset_gpio)) {
+		priv->reset_gpio = NULL;
+		dev_warn(dev, "No reset GPIO found\n");
+	}
+
 	/* If fixed-link is set, skip PHY scanning */
 	fwnode = priv->plat->port_node;
 	if (!fwnode)
-- 
2.34.1

Re: [PATCH v3] net: stmmac: avoid repeated devm_gpiod_get_optional in mdio

[Andrew Lunn]

On Mon, Apr 27, 2026 at 03:50:32PM +0800, Liu Xiang wrote:
> During system resume, stmmac_resume() calls stmmac_mdio_reset().
> This results in repeated calls to devm_gpiod_get_optional() for the same
> GPIO. The second invocation returns an error since the GPIO descriptor has
> already been obtained.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>

Should this have a Fixes: tag?


>  #ifdef CONFIG_OF
>  	if (priv->device->of_node) {
> -		struct gpio_desc *reset_gpio;
> -		u32 delays[3] = { 0, 0, 0 };
> +		if (priv->reset_gpio) {
> +			u32 delays[3] = { 0, 0, 0 };
>  
> -		reset_gpio = devm_gpiod_get_optional(priv->device,
> -						     "snps,reset",
> -						     GPIOD_OUT_LOW);
> -		if (IS_ERR(reset_gpio))
> -			return PTR_ERR(reset_gpio);
> +			device_property_read_u32_array(priv->device,
> +						       "snps,reset-delays-us",
> +						       delays,
> +						       ARRAY_SIZE(delays));
>  
> -		device_property_read_u32_array(priv->device,
> -					       "snps,reset-delays-us",
> -					       delays, ARRAY_SIZE(delays));
> +			gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +			if (delays[0])
> +				msleep(DIV_ROUND_UP(delays[0], 1000));
>  
> -		if (delays[0])
> -			msleep(DIV_ROUND_UP(delays[0], 1000));
> +			gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +			if (delays[1])
> +				msleep(DIV_ROUND_UP(delays[1], 1000));
>  
> -		gpiod_set_value_cansleep(reset_gpio, 1);
> -		if (delays[1])
> -			msleep(DIV_ROUND_UP(delays[1], 1000));
> -
> -		gpiod_set_value_cansleep(reset_gpio, 0);
> -		if (delays[2])
> -			msleep(DIV_ROUND_UP(delays[2], 1000));
> +			gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +			if (delays[2])
> +				msleep(DIV_ROUND_UP(delays[2], 1000));
> +		}
>  	}

This is not obviously correct. Sometimes it is better to change less,
so the diff is smaller, and so you can look at the diff and say, Yes,
this is obviously correct.

What is actually changing here? The devm_gpiod_get_optional() should
disappear, and reset_gpio is replaced with priv->reset_gpio. Does the
indentation need to change? Does u32 delays[3] need to move from
outside an if to inside an if?

So if i stare at this long enough it looks correct, but please try
again, and see if you can make if obviously correct.

    Andrew

---
pw-bot: cr