[PATCH net-next 05/12] net: ftgmac100: Convert using devm_clk_get_enabled() in ftgmac100_setup_clk()

[Uwe Kleine-König]

Hello,

On Tue, Sep 03, 2024 at 06:46:48PM +0800, Li Zetao wrote:
> 在 2024/9/3 16:09, Uwe Kleine-König 写道:
> > 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 at huawei.com>
> > > ---
> > >   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.
> Before my patch, ftgmac100_setup_clk() was only left with priv->clk disabled
> when error occurs, and was left with priv->clk enabled when no error occurs
> because when enabling priv->rclk successfully, it will return 0 directly,
> and when enabling priv->rclk failed, it will disable priv->clk.
> 
> It turns out that the code logic is a bit counter-intuitive, but the
> readability has been improved after adjustments.

Indeed. This is IMHO worth mentioning in the commit log to prevent the
next reviewer stumble over the same code construct.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-rockchip/attachments/20240903/f459c0f7/attachment.sig>