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

[Simon Horman <horms@kernel.org>]

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.

> 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