Re: [PATCH -next 07/11] mtd: rawnand: mtk: Use helper function devm_clk_get_enabled()

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Li,

lizetao1@huawei.com wrote on Thu, 17 Aug 2023 10:45:05 +0800:

> After the commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for
> prepared and enabled clocks"), it can replace the pair of functions,
> devm_clk_get() and clk_prepare_enable() with a single helper function
> devm_clk_get_enabled(). Moreover, the driver will keeps a clock prepared
> (or enabled) during the whole lifetime of the driver, it is unnecessary to
> unprepare and disable clock explicitly when remove driver or in the error
> handling path. The mtk_nfc_enable_clk() is a helper function that enables
> the "clk->nfi_clk" and "clk->pad_clk", it can be combined into
> devm_clk_get_enabled().
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/mtd/nand/raw/mtk_nand.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mtk_nand.c b/drivers/mtd/nand/raw/mtk_nand.c
> index b6eb8cb6b5e9..0d185e650aaf 100644
> --- a/drivers/mtd/nand/raw/mtk_nand.c
> +++ b/drivers/mtd/nand/raw/mtk_nand.c
> @@ -1545,40 +1545,36 @@ static int mtk_nfc_probe(struct platform_device *pdev)
>  		goto release_ecc;
>  	}
>  
> -	nfc->clk.nfi_clk = devm_clk_get(dev, "nfi_clk");
> +	nfc->clk.nfi_clk = devm_clk_get_enabled(dev, "nfi_clk");
>  	if (IS_ERR(nfc->clk.nfi_clk)) {
>  		dev_err(dev, "no clk\n");
>  		ret = PTR_ERR(nfc->clk.nfi_clk);
>  		goto release_ecc;
>  	}
>  
> -	nfc->clk.pad_clk = devm_clk_get(dev, "pad_clk");
> +	nfc->clk.pad_clk = devm_clk_get_enabled(dev, "pad_clk");
>  	if (IS_ERR(nfc->clk.pad_clk)) {
>  		dev_err(dev, "no pad clk\n");
>  		ret = PTR_ERR(nfc->clk.pad_clk);
>  		goto release_ecc;
>  	}
>  
> -	ret = mtk_nfc_enable_clk(dev, &nfc->clk);
> -	if (ret)
> -		goto release_ecc;
> -
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
>  		ret = -EINVAL;
> -		goto clk_disable;
> +		goto release_ecc;
>  	}
>  
>  	ret = devm_request_irq(dev, irq, mtk_nfc_irq, 0x0, "mtk-nand", nfc);
>  	if (ret) {
>  		dev_err(dev, "failed to request nfi irq\n");
> -		goto clk_disable;
> +		goto release_ecc;
>  	}
>  
>  	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
>  	if (ret) {
>  		dev_err(dev, "failed to set dma mask\n");
> -		goto clk_disable;
> +		goto release_ecc;
>  	}
>  
>  	platform_set_drvdata(pdev, nfc);
> @@ -1586,14 +1582,11 @@ static int mtk_nfc_probe(struct platform_device *pdev)
>  	ret = mtk_nfc_nand_chips_init(dev, nfc);
>  	if (ret) {
>  		dev_err(dev, "failed to init nand chips\n");
> -		goto clk_disable;
> +		goto release_ecc;
>  	}
>  
>  	return 0;
>  
> -clk_disable:
> -	mtk_nfc_disable_clk(&nfc->clk);
> -
>  release_ecc:
>  	mtk_ecc_release(nfc->ecc);
>  
> @@ -1618,7 +1611,6 @@ static void mtk_nfc_remove(struct platform_device *pdev)
>  	}
>  
>  	mtk_ecc_release(nfc->ecc);
> -	mtk_nfc_disable_clk(&nfc->clk);

mtk_nfc_disable_clk() now has a single user, which is the suspend
callback. This callback does nothing else than calling
mtk_nfs_disable_clk(). Can you please drop the helper and just move the
two lines in the suspend function?

>  }
>  
>  #ifdef CONFIG_PM_SLEEP


Thanks,
Miquèl