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

Miquel Raynal
2023-08-17T08:34:54Z

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