Re: [net-next,v2,06/15] net: ftgmac100: Use devm_clk_get_enabled
From: Simon Horman <horms@kernel.org>
Date: 2026-01-20 12:19:55
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 hunk ↗ jump to hunk
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) 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) 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?
[ ... ]
--
pw-bot: changes-requested