Re: [PATCH 3/3] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Miquel,

On Fri, Apr 29, 2022 at 12:52 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> This NAND controller is part of a well defined power domain handled by
> the runtime PM core. Let's keep the harmony with the other RZ/N1 drivers
> and exclusively use the runtime PM API to enable/disable the clocks.
>
> We still need to retrieve the external clock rate in order to derive the
> NAND timings, but that is not a big deal, we can still do that in the
> probe and just save this value to reuse it later.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- a/drivers/mtd/nand/raw/renesas-nand-controller.c
> +++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
> @@ -1319,6 +1319,7 @@ static int rnandc_chips_init(struct rnandc *rnandc)
>  static int rnandc_probe(struct platform_device *pdev)
>  {
>         struct rnandc *rnandc;
> +       struct clk *eclk;
>         int irq, ret;
>
>         rnandc = devm_kzalloc(&pdev->dev, sizeof(*rnandc), GFP_KERNEL);
> @@ -1335,29 +1336,10 @@ static int rnandc_probe(struct platform_device *pdev)
>         if (IS_ERR(rnandc->regs))
>                 return PTR_ERR(rnandc->regs);
>
> -       /* APB clock */
> -       rnandc->hclk = devm_clk_get(&pdev->dev, "hclk");
> -       if (IS_ERR(rnandc->hclk))
> -               return PTR_ERR(rnandc->hclk);
> -
> -       /* External NAND bus clock */
> -       rnandc->eclk = devm_clk_get(&pdev->dev, "eclk");
> -       if (IS_ERR(rnandc->eclk))
> -               return PTR_ERR(rnandc->eclk);
> -
> -       ret = clk_prepare_enable(rnandc->hclk);
> -       if (ret)
> -               return ret;
> -
> -       ret = clk_prepare_enable(rnandc->eclk);
> -       if (ret)
> -               goto disable_hclk;
> -
>         rnandc_dis_interrupts(rnandc);
>         irq = platform_get_irq_optional(pdev, 0);
>         if (irq == -EPROBE_DEFER) {
> -               ret = irq;
> -               goto disable_eclk;
> +               return irq;
>         } else if (irq < 0) {
>                 dev_info(&pdev->dev, "No IRQ found, fallback to polling\n");
>                 rnandc->use_polling = true;
> @@ -1365,12 +1347,25 @@ static int rnandc_probe(struct platform_device *pdev)
>                 ret = devm_request_irq(&pdev->dev, irq, rnandc_irq_handler, 0,
>                                        "renesas-nand-controller", rnandc);
>                 if (ret < 0)
> -                       goto disable_eclk;
> +                       return ret;
>         }
>
>         ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>         if (ret)
> -               goto disable_eclk;
> +               return ret;
> +
> +       pm_runtime_enable(&pdev->dev);
> +       pm_runtime_get_sync(&pdev->dev);

ret = pm_runtime_resume_and_get)...);
if (ret < 0) ...

> +
> +       /* The external NAND bus clock rate is needed for computing timings */
> +       eclk = clk_get(&pdev->dev, "eclk");
> +       if (IS_ERR(eclk)) {
> +               ret = PTR_ERR(eclk);
> +               goto dis_runtime_pm;
> +       }
> +
> +       rnandc->ext_clk_rate = clk_get_rate(eclk);
> +       clk_put(eclk);
>
>         rnandc_clear_fifo(rnandc);
>
> @@ -1378,14 +1373,13 @@ static int rnandc_probe(struct platform_device *pdev)
>
>         ret = rnandc_chips_init(rnandc);
>         if (ret)
> -               goto disable_eclk;
> +               goto dis_runtime_pm;
>
>         return 0;
>
> -disable_eclk:
> -       clk_disable_unprepare(rnandc->eclk);
> -disable_hclk:
> -       clk_disable_unprepare(rnandc->hclk);
> +dis_runtime_pm:
> +       pm_runtime_put_sync(&pdev->dev);

pm_runtime_put()

> +       pm_runtime_disable(&pdev->dev);
>
>         return ret;
>  }
> @@ -1396,8 +1390,8 @@ static int rnandc_remove(struct platform_device *pdev)
>
>         rnandc_chips_cleanup(rnandc);
>
> -       clk_disable_unprepare(rnandc->eclk);
> -       clk_disable_unprepare(rnandc->hclk);
> +       pm_runtime_put_sync(&pdev->dev);

pm_runtime_put()

> +       pm_runtime_disable(&pdev->dev);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds