Re: [PATCH 7/7] memory: pl353-smc: simplify with devm_clk_get_enabled()

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

On Fri, 23 Aug 2024 12:16:02 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
> make code simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
LGTM.
Follow up suggestion inline.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/memory/pl353-smc.c | 36 +++---------------------------------
>  1 file changed, 3 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/memory/pl353-smc.c b/drivers/memory/pl353-smc.c
> index c75b99e49970..994c7a792e34 100644
> --- a/drivers/memory/pl353-smc.c
> +++ b/drivers/memory/pl353-smc.c
> @@ -75,34 +75,21 @@ static int pl353_smc_probe(struct amba_device *adev, const struct amba_id *id)
>  	const struct of_device_id *match = NULL;
>  	struct pl353_smc_data *pl353_smc;
>  	struct device_node *child;
> -	int err;
>  
>  	pl353_smc = devm_kzalloc(&adev->dev, sizeof(*pl353_smc), GFP_KERNEL);
>  	if (!pl353_smc)
>  		return -ENOMEM;
>  
> -	pl353_smc->aclk = devm_clk_get(&adev->dev, "apb_pclk");
> +	pl353_smc->aclk = devm_clk_get_enabled(&adev->dev, "apb_pclk");
>  	if (IS_ERR(pl353_smc->aclk))
>  		return dev_err_probe(&adev->dev, PTR_ERR(pl353_smc->aclk),
>  				     "aclk clock not found.\n");
>  
> -	pl353_smc->memclk = devm_clk_get(&adev->dev, "memclk");
> +	pl353_smc->memclk = devm_clk_get_enabled(&adev->dev, "memclk");
>  	if (IS_ERR(pl353_smc->memclk))
>  		return dev_err_probe(&adev->dev, PTR_ERR(pl353_smc->memclk),
>  				     "memclk clock not found.\n");
>  
> -	err = clk_prepare_enable(pl353_smc->aclk);
> -	if (err) {
> -		dev_err(&adev->dev, "Unable to enable AXI clock.\n");
> -		return err;
> -	}
> -
> -	err = clk_prepare_enable(pl353_smc->memclk);
> -	if (err) {
> -		dev_err(&adev->dev, "Unable to enable memory clock.\n");
> -		goto disable_axi_clk;
> -	}
> -
>  	amba_set_drvdata(adev, pl353_smc);
>  
>  	/* Find compatible children. Only a single child is supported */
> @@ -115,30 +102,14 @@ static int pl353_smc_probe(struct amba_device *adev, const struct amba_id *id)
>  		break;
>  	}
>  	if (!match) {
With change below this becomes unconditional as we'll have already
returned in the loop for the good path.

Might as well use dev_err_probe() here as well to save a few lines.

> -		err = -ENODEV;
>  		dev_err(&adev->dev, "no matching children\n");
> -		goto disable_mem_clk;
> +		return -ENODEV;
>  	}
>  
>  	of_platform_device_create(child, NULL, &adev->dev);
>  	of_node_put(child);

An additional cleanup looks sensible here.

Push this last bit into the loop and use 

for_each_available_child_of_node_scoped()

Assuming you don't already have a patch doing that :)

>  
>  	return 0;
> -
> -disable_mem_clk:
> -	clk_disable_unprepare(pl353_smc->memclk);
> -disable_axi_clk:
> -	clk_disable_unprepare(pl353_smc->aclk);
> -
> -	return err;
> -}
> -
> -static void pl353_smc_remove(struct amba_device *adev)
> -{
> -	struct pl353_smc_data *pl353_smc = amba_get_drvdata(adev);
> -
> -	clk_disable_unprepare(pl353_smc->memclk);
> -	clk_disable_unprepare(pl353_smc->aclk);
>  }
>  
>  static const struct amba_id pl353_ids[] = {
> @@ -157,7 +128,6 @@ static struct amba_driver pl353_smc_driver = {
>  	},
>  	.id_table = pl353_ids,
>  	.probe = pl353_smc_probe,
> -	.remove = pl353_smc_remove,
>  };
>  
>  module_amba_driver(pl353_smc_driver);
>