[net-next,v2,06/15] net: ftgmac100: Use devm_clk_get_enabled

寄件者: Simon Horman [off-list ref]
寄件日期: Tuesday, January 20, 2026 8:20 PM
收件者: Jacky Chou [off-list ref]
副本: Simon Horman [off-list ref]; davem@davemloft.net;
kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
andrew+netdev@lunn.ch; netdev@vger.kernel.org
主旨: Re: [net-next,v2,06/15] net: ftgmac100: Use devm_clk_get_enabled

This is an AI-generated review of your patch. The human sending this email has
considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: ftgmac100: Use devm_clk_get_enabled

This patch converts the ftgmac100 driver to use devm_clk_get_enabled() and
devm_clk_get_optional_enabled() to simplify clock management and cleanup
paths.

quoted

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c

b/drivers/net/ethernet/faraday/ftgmac100.c
index ec2e7ec23ddf..ffd86655bcc8 100644

--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c

@@ -1801,13 +1801,10 @@ static int ftgmac100_setup_clk(struct

ftgmac100 *priv)

quoted

 	struct clk *clk;
 	int rc;

-	clk = devm_clk_get(priv->dev, NULL /* MACCLK */);
+	clk = devm_clk_get_enabled(priv->dev, NULL /* MACCLK */);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 	priv->clk = clk;
-	rc = clk_prepare_enable(priv->clk);
-	if (rc)
-		return rc;

 	/* Aspeed specifies a 100MHz clock is required for up to
 	 * 1000Mbit link speeds. As NCSI is limited to 100Mbit, 25MHz @@
-1816,21 +1813,15 @@ static int ftgmac100_setup_clk(struct ftgmac100

*priv)

quoted

 	rc = clk_set_rate(priv->clk, priv->use_ncsi ? FTGMAC_25MHZ :
 			  FTGMAC_100MHZ);
 	if (rc)
-		goto cleanup_clk;
+		return rc;

 	/* RCLK is for RMII, typically used for NCSI. Optional because it's not
 	 * necessary if it's the AST2400 MAC, or the MAC is configured for
 	 * RGMII, or the controller is not an ASPEED-based controller.
 	 */
-	priv->rclk = devm_clk_get_optional(priv->dev, "RCLK");
-	rc = clk_prepare_enable(priv->rclk);
-	if (!rc)
-		return 0;
+	priv->rclk = devm_clk_get_optional_enabled(priv->dev, "RCLK");

-cleanup_clk:
-	clk_disable_unprepare(priv->clk);
-
-	return rc;
+	return 0;
 }

Should the return value of devm_clk_get_optional_enabled() be checked here?
The old code would fail probe if RCLK existed but
clk_prepare_enable() failed. The new code unconditionally returns 0, ignoring
any potential error from devm_clk_get_optional_enabled().

Looking at other drivers using this API, most check for IS_ERR(). For example,
cctrng_probe() in drivers/char/hw_random/cctrng.c does:

    drvdata->clk = devm_clk_get_optional_enabled(dev, NULL);
    if (IS_ERR(drvdata->clk))
        return dev_err_probe(...);

While the clock is optional (may not exist), if it does exist but fails to enable,
this seems like a condition that should cause probe to fail rather than succeed
silently?

Agreed for AI review.
We missed checking the return value from devm_clk_get_optional_enabled().
I will fix this in the next version.

Thanks,
Jacky