Re: [net-next v3 4/9] net: mdio: hisi-femac: Convert to devm_clk_get_enabled()

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 27 Aug 2024 03:57:07 -0600
Yangtao Li <frank.li@vivo.com> wrote:

> Convert devm_clk_get(), clk_prepare_enable() to a single
> call to devm_clk_get_enabled(), as this is exactly
> what this function does.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>

Mixing an matching devm and otherwise can lead to subtle bugs
and generally makes the code harder to review

I'd also use devm_mdiobus_alloc_size() and devm_mdiobus_register()
and drop the remove() callback entirely.

I would not take this patch on its own as it causes a reordering
of cleanup we probably don't want.

As a general rule, single action devm cleanup series (so only using
one new type) fall into this trap. Much better to look at all the
improvements in each driver that are enabled by new infrastructure
rather than doing a single type change series like this one.

Thanks,

Jonathan


> ---
>  drivers/net/mdio/mdio-hisi-femac.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-hisi-femac.c b/drivers/net/mdio/mdio-hisi-femac.c
> index 6703f626ee83..f6fcb9e11624 100644
> --- a/drivers/net/mdio/mdio-hisi-femac.c
> +++ b/drivers/net/mdio/mdio-hisi-femac.c
> @@ -21,7 +21,6 @@
>  #define BIT_WR_DATA_OFFSET	16
>  
>  struct hisi_femac_mdio_data {
> -	struct clk *clk;
>  	void __iomem *membase;
>  };
>  
> @@ -74,6 +73,7 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	struct mii_bus *bus;
>  	struct hisi_femac_mdio_data *data;
> +	struct clk *clk;
>  	int ret;
>  
>  	bus = mdiobus_alloc_size(sizeof(*data));
> @@ -93,26 +93,20 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
>  		goto err_out_free_mdiobus;
>  	}
>  
> -	data->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(data->clk)) {
> -		ret = PTR_ERR(data->clk);
> +	clk = devm_clk_get_prepared(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
>  		goto err_out_free_mdiobus;
>  	}
>  
> -	ret = clk_prepare_enable(data->clk);
> -	if (ret)
> -		goto err_out_free_mdiobus;
> -
>  	ret = of_mdiobus_register(bus, np);
>  	if (ret)
> -		goto err_out_disable_clk;
> +		goto err_out_free_mdiobus;
>  
>  	platform_set_drvdata(pdev, bus);
>  
>  	return 0;
>  
> -err_out_disable_clk:
> -	clk_disable_unprepare(data->clk);
>  err_out_free_mdiobus:
>  	mdiobus_free(bus);
>  	return ret;
> @@ -121,10 +115,8 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
>  static void hisi_femac_mdio_remove(struct platform_device *pdev)
>  {
>  	struct mii_bus *bus = platform_get_drvdata(pdev);
> -	struct hisi_femac_mdio_data *data = bus->priv;
>  
>  	mdiobus_unregister(bus);
> -	clk_disable_unprepare(data->clk);
>  	mdiobus_free(bus);
>  }
>