Re: [PATCH] mmc: sdhci-pxav3: use managed clock enablement

[Rosen Penev]

On Wed, Sep 9, 2026 at 12:51 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>
> On 01/09/2026 03:42, Rosen Penev wrote:
> > Replace devm_clk_get() plus clk_prepare_enable() with the managed
> > variants devm_clk_get_enabled() and devm_clk_get_optional_enabled().
> > The core clock becomes optional, which removes the IS_ERR() guards
> > in runtime suspend/resume and lets devms unwind the clocks, so the
> > manual clk_disable_unprepare() calls in the probe error paths and
> > remove are dropped.
> >
> > clk_prepare_enable() and friends are NULL safe, so no need to check if
> > the optional clock is present.
> >
> > Also handle failures of clk_prepare_enable() in runtime resume instead
> > of ignoring them: a failed enable would leave controller register
> > accesses hitting unclocked hardware. Bail out before resuming the host
> > and unwind clk_io when enabling clk_core fails. This also drops the
> > now unused pltfm_host variable from remove().
> >
> > Assisted-by: opencode:big-pickle
> > Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> > ---
> >  drivers/mmc/host/sdhci-pxav3.c | 42 +++++++++++++++-------------------
> >  1 file changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> > index d082c4e21aa9..db9dfa92cb57 100644
> > --- a/drivers/mmc/host/sdhci-pxav3.c
> > +++ b/drivers/mmc/host/sdhci-pxav3.c
> > @@ -414,19 +414,18 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
> >       pltfm_host = sdhci_priv(host);
> >       pxa = sdhci_pltfm_priv(pltfm_host);
> >
> > -     pxa->clk_io = devm_clk_get(dev, "io");
> > +     pxa->clk_io = devm_clk_get_enabled(dev, "io");
> >       if (IS_ERR(pxa->clk_io))
> > -             pxa->clk_io = devm_clk_get(dev, NULL);
> > +             pxa->clk_io = devm_clk_get_enabled(dev, NULL);
>
> Sashiko has a point about getting the wrong clock:
>
>         https://sashiko.dev/#/patchset/20260901004243.50052-1-rosenp%40gmail.com?part=1
It's technically correct but meaningless. All dts files specify the
clock properly. Both paths attempt to enable the same clock.
>
> >       if (IS_ERR(pxa->clk_io)) {
> >               dev_err(dev, "failed to get io clock\n");
> >               return PTR_ERR(pxa->clk_io);
> >       }
> >       pltfm_host->clk = pxa->clk_io;
> > -     clk_prepare_enable(pxa->clk_io);
> >
> > -     pxa->clk_core = devm_clk_get(dev, "core");
> > -     if (!IS_ERR(pxa->clk_core))
> > -             clk_prepare_enable(pxa->clk_core);
> > +     pxa->clk_core = devm_clk_get_optional_enabled(dev, "core");
> > +     if (IS_ERR(pxa->clk_core))
> > +             return PTR_ERR(pxa->clk_core);
> >
> >       host->mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
> >       /* enable 1/8V DDR capable */
> > @@ -435,17 +434,17 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
> >       if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) {
> >               ret = armada_38x_quirks(pdev, host);
> >               if (ret < 0)
> > -                     goto err_mbus_win;
> > +                     return ret;
> >               ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
> >               if (ret < 0)
> > -                     goto err_mbus_win;
> > +                     return ret;
> >       }
> >
> >       match = of_match_device(of_match_ptr(sdhci_pxav3_of_match), &pdev->dev);
> >       if (match) {
> >               ret = mmc_of_parse(host->mmc);
> >               if (ret)
> > -                     goto err_of_parse;
> > +                     return ret;
> >               sdhci_get_of_property(pdev);
> >               pdata = pxav3_get_mmc_pdata(dev);
> >               pdev->dev.platform_data = pdata;
> > @@ -500,27 +499,18 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
> >  err_add_host:
> >       pm_runtime_disable(&pdev->dev);
> >       pm_runtime_put_noidle(&pdev->dev);
> > -err_of_parse:
> > -err_mbus_win:
> > -     clk_disable_unprepare(pxa->clk_io);
> > -     clk_disable_unprepare(pxa->clk_core);
> >       return ret;
> >  }
> >
> >  static void sdhci_pxav3_remove(struct platform_device *pdev)
> >  {
> >       struct sdhci_host *host = platform_get_drvdata(pdev);
> > -     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > -     struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
> >
> >       pm_runtime_get_sync(&pdev->dev);
> >       pm_runtime_disable(&pdev->dev);
> >       pm_runtime_put_noidle(&pdev->dev);
> >
> >       sdhci_remove_host(host, 1);
> > -
> > -     clk_disable_unprepare(pxa->clk_io);
> > -     clk_disable_unprepare(pxa->clk_core);
> >  }
> >
> >  static int sdhci_pxav3_suspend(struct device *dev)
> > @@ -561,8 +551,7 @@ static int sdhci_pxav3_runtime_suspend(struct device *dev)
> >               mmc_retune_needed(host->mmc);
> >
> >       clk_disable_unprepare(pxa->clk_io);
> > -     if (!IS_ERR(pxa->clk_core))
> > -             clk_disable_unprepare(pxa->clk_core);
> > +     clk_disable_unprepare(pxa->clk_core);
> >
> >       return 0;
> >  }
> > @@ -572,10 +561,17 @@ static int sdhci_pxav3_runtime_resume(struct device *dev)
> >       struct sdhci_host *host = dev_get_drvdata(dev);
> >       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >       struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
> > +     int ret;
> >
> > -     clk_prepare_enable(pxa->clk_io);
> > -     if (!IS_ERR(pxa->clk_core))
> > -             clk_prepare_enable(pxa->clk_core);
> > +     ret = clk_prepare_enable(pxa->clk_io);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_prepare_enable(pxa->clk_core);
> > +     if (ret) {
> > +             clk_disable_unprepare(pxa->clk_io);
> > +             return ret;
> > +     }
> >
> >       sdhci_runtime_resume_host(host, 0);
> >       return 0;
>