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

[Andrew Lunn <andrew@lunn.ch>]

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