Hello,
On Sat, Aug 31, 2024 at 10:13:27AM +0800, Li Zetao wrote:
> Use devm_clk_get_enabled() instead of devm_clk_get() +
> clk_prepare_enable(), which can make the clk consistent with the device
> life cycle and reduce the risk of unreleased clk resources. Since the
> device framework has automatically released the clk resource, there is
> no need to execute clk_disable_unprepare(clk) on the error path, drop
> the cleanup_clk label, and the original error process can return directly.
>
> Signed-off-by: Li Zetao <lizetao1@xxxxxxxxxx>
> ---
> drivers/net/ethernet/faraday/ftgmac100.c | 27 ++++++------------------
> 1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 4c546c3aef0f..eb57c822c5ac 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1752,13 +1752,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
> @@ -1767,21 +1764,17 @@ 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");
> + if (IS_ERR(priv->rclk))
> + return PTR_ERR(priv->rclk);
>
> -cleanup_clk:
> - clk_disable_unprepare(priv->clk);
> -
> - return rc;
> + return 0;
You're changing semantics here. Before your patch ftgmac100_setup_clk()
was left with priv->clk disabled; now you keep it enabled.
Further note that there is a bug here, because in ftgmac100_probe()
(i.e. the caller of ftgmac100_setup_clk())
clk_disable_unprepare(priv->clk) is called in the error path.
(I only looked quickly, so I might have missed a detail.)
So while your patch is an improvement for clock enable/disable
balancing, it might regress on power consumption.
> }
>
> static bool ftgmac100_has_child_node(struct device_node *np, const char *name)
> @@ -1996,16 +1989,13 @@ static int ftgmac100_probe(struct platform_device *pdev)
> err = register_netdev(netdev);
> if (err) {
> dev_err(&pdev->dev, "Failed to register netdev\n");
> - goto err_register_netdev;
> + goto err_phy_connect;
> }
>
> netdev_info(netdev, "irq %d, mapped at %p\n", netdev->irq, priv->base);
>
> return 0;
>
> -err_register_netdev:
> - clk_disable_unprepare(priv->rclk);
> - clk_disable_unprepare(priv->clk);
> err_phy_connect:
> ftgmac100_phy_disconnect(netdev);
> err_ncsi_dev:
> @@ -2034,9 +2024,6 @@ static void ftgmac100_remove(struct platform_device *pdev)
> ncsi_unregister_dev(priv->ndev);
> unregister_netdev(netdev);
>
> - clk_disable_unprepare(priv->rclk);
> - clk_disable_unprepare(priv->clk);
> -
> /* There's a small chance the reset task will have been re-queued,
> * during stop, make sure it's gone before we free the structure.
> */
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
