Re: [PATCH v3 1/2] clk: qcom: camcc-sm6350: add pm_runtime support

["Luca Weiss" <luca.weiss@fairphone.com>]

On Tue Feb 14, 2023 at 1:32 PM CET, Dmitry Baryshkov wrote:
> On Tue, 14 Feb 2023 at 13:01, Luca Weiss <luca.weiss@fairphone.com> wrote:
> >
> > Make sure that we can enable and disable the power domains used for
> > camcc when the clocks are and aren't used.
> >
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> >  drivers/clk/qcom/camcc-sm6350.c | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/camcc-sm6350.c b/drivers/clk/qcom/camcc-sm6350.c
> > index acba9f99d960..fc5532e2ee5b 100644
> > --- a/drivers/clk/qcom/camcc-sm6350.c
> > +++ b/drivers/clk/qcom/camcc-sm6350.c
> > @@ -7,6 +7,8 @@
> >  #include <linux/clk-provider.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_clock.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> >
> >  #include <dt-bindings/clock/qcom,sm6350-camcc.h>
> > @@ -1869,6 +1871,19 @@ MODULE_DEVICE_TABLE(of, camcc_sm6350_match_table);
> >  static int camcc_sm6350_probe(struct platform_device *pdev)
> >  {
> >         struct regmap *regmap;
> > +       int ret;
> > +
> > +       ret = devm_pm_runtime_enable(&pdev->dev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = devm_pm_clk_create(&pdev->dev);
> > +       if (ret < 0)
> > +               return ret;
>
> This makes me wonder, what is the use for the pm_clk in your case? The
> driver doesn't seem to use of_pm_clk_add_clk(), of_pm_clk_add_clks()
> or pm_clk_add_clk(). So pm_clk_suspend() and pm_clk_resume() do
> nothing.

You're right that we're not using any of these functions in the driver.
However still when camcc is not used, the associated power domain turns
off correctly so that part works as expected.

Honestly these lines have been copied from a different driver and I'm
not familiar enough with the pm_runtime APIs to know what to use here
without using the pm_clk* and pm_clk_suspend.

Basically we need, if any clock is being used in the driver, the
power-domain needs to be enabled as well, and if nothing is used the
power-domain can be disabled again.

Please advise.

Regards
Luca

>
> > +
> > +       ret = pm_runtime_get(&pdev->dev);
> > +       if (ret)
> > +               return ret;
> >
> >         regmap = qcom_cc_map(pdev, &camcc_sm6350_desc);
> >         if (IS_ERR(regmap))
> > @@ -1879,14 +1894,22 @@ static int camcc_sm6350_probe(struct platform_device *pdev)
> >         clk_agera_pll_configure(&camcc_pll2, regmap, &camcc_pll2_config);
> >         clk_fabia_pll_configure(&camcc_pll3, regmap, &camcc_pll3_config);
> >
> > -       return qcom_cc_really_probe(pdev, &camcc_sm6350_desc, regmap);
> > +       ret = qcom_cc_really_probe(pdev, &camcc_sm6350_desc, regmap);
> > +       pm_runtime_put(&pdev->dev);
> > +
> > +       return ret;
> >  }
> >
> > +static const struct dev_pm_ops camcc_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> > +};
> > +
> >  static struct platform_driver camcc_sm6350_driver = {
> >         .probe = camcc_sm6350_probe,
> >         .driver = {
> >                 .name = "sm6350-camcc",
> >                 .of_match_table = camcc_sm6350_match_table,
> > +               .pm = &camcc_pm_ops,
> >         },
> >  };
> >
> >
> > --
> > 2.39.1
> >
>
>
> -- 
> With best wishes
> Dmitry