mbox series

[v3,0/3] PM: add two devres helpers and use them in qcom cc

Message ID 20210731195034.979084-1-dmitry.baryshkov@linaro.org
Headers show
Series PM: add two devres helpers and use them in qcom cc | expand

Message

Dmitry Baryshkov July 31, 2021, 7:50 p.m. UTC
Most of the drivers using using pm_runtime_enable() or pm_clk_create()
follow the same pattern: call the function in the probe() path and call
correspondingly pm_runtime_disable() or pm_clk_destroy() from the
probe()'s error path and from the remove() function. This common code
pattern has several drawbacks. I.e. driver authors have to ensure that
the disable/destroy call in the error path really corresponds to the
proper error clause. Or that the disable/destroy call is not missed in
the remove() callback.

Add two devres helpers replacing these code patterns with relevant devm
function call, removing the need to call corresponding disable/destroy
functions. As an example modify Qualcomm clock controller code to use
new helpers. In this case we are able to drop error path and remove
functions completely, simplifying the drivers in question.

Changes since v2:
 - Expand commit messages
 - Drop extra clock controller changes not strictly relevant to these
   two helpers

Changes since v1:
 - Add a patch making Qualcomm clock controller drivers actually execute
   these helpers, thus demonstrating their usage and the necessity

----------------------------------------------------------------
Dmitry Baryshkov (3):
      PM: runtime: add devm_pm_runtime_enable helper
      PM: runtime: add devm_pm_clk_create helper
      clk: qcom: use devm_pm_runtime_enable and devm_pm_clk_create

 drivers/base/power/clock_ops.c        | 17 +++++++++++++++++
 drivers/base/power/runtime.c          | 17 +++++++++++++++++
 drivers/clk/qcom/camcc-sc7180.c       | 25 ++++++++++---------------
 drivers/clk/qcom/lpass-gfm-sm8250.c   | 21 +++++++++------------
 drivers/clk/qcom/lpasscorecc-sc7180.c | 18 ++----------------
 drivers/clk/qcom/mss-sc7180.c         | 30 ++++++++----------------------
 drivers/clk/qcom/q6sstop-qcs404.c     | 32 +++++++++-----------------------
 drivers/clk/qcom/turingcc-qcs404.c    | 30 ++++++++----------------------
 include/linux/pm_clock.h              |  5 +++++
 include/linux/pm_runtime.h            |  4 ++++
 10 files changed, 89 insertions(+), 110 deletions(-)

Comments

Rafael J. Wysocki Aug. 4, 2021, 6:06 p.m. UTC | #1
On Sat, Jul 31, 2021 at 9:50 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>

> A typical code pattern for pm_runtime_enable() call is to call it in the

> _probe function and to call pm_runtime_disable() both from _probe error

> path and from _remove function. For some drivers the whole remove

> function would consist of the call to pm_remove_disable().

>

> Add helper function to replace this bolierplate piece of code. Calling

> devm_pm_runtime_enable() removes the need for calling

> pm_runtime_disable() both in the probe()'s error path and in the

> remove() function.

>

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---

>  drivers/base/power/runtime.c | 17 +++++++++++++++++

>  include/linux/pm_runtime.h   |  4 ++++

>  2 files changed, 21 insertions(+)

>

> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c

> index 8a66eaf731e4..ec94049442b9 100644

> --- a/drivers/base/power/runtime.c

> +++ b/drivers/base/power/runtime.c

> @@ -1447,6 +1447,23 @@ void pm_runtime_enable(struct device *dev)

>  }

>  EXPORT_SYMBOL_GPL(pm_runtime_enable);

>

> +static void pm_runtime_disable_action(void *data)

> +{

> +       pm_runtime_disable(data);

> +}

> +

> +/**

> + * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.

> + * @dev: Device to handle.

> + */

> +int devm_pm_runtime_enable(struct device *dev)

> +{

> +       pm_runtime_enable(dev);

> +

> +       return devm_add_action_or_reset(dev, pm_runtime_disable_action, dev);


When exactly is pm_runtime_disable_action() going to run by this rule?
 When the device goes away or when the driver is unbound from it?

> +}

> +EXPORT_SYMBOL_GPL(devm_pm_runtime_enable);

> +

>  /**

>   * pm_runtime_forbid - Block runtime PM of a device.

>   * @dev: Device to handle.

> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h

> index aab8b35e9f8a..222da43b7096 100644

> --- a/include/linux/pm_runtime.h

> +++ b/include/linux/pm_runtime.h

> @@ -59,6 +59,8 @@ extern void pm_runtime_put_suppliers(struct device *dev);

>  extern void pm_runtime_new_link(struct device *dev);

>  extern void pm_runtime_drop_link(struct device_link *link);

>

> +extern int devm_pm_runtime_enable(struct device *dev);

> +

>  /**

>   * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.

>   * @dev: Target device.

> @@ -253,6 +255,8 @@ static inline void __pm_runtime_disable(struct device *dev, bool c) {}

>  static inline void pm_runtime_allow(struct device *dev) {}

>  static inline void pm_runtime_forbid(struct device *dev) {}

>

> +static inline int devm_pm_runtime_enable(struct device *dev) { return 0; }

> +

>  static inline void pm_suspend_ignore_children(struct device *dev, bool enable) {}

>  static inline void pm_runtime_get_noresume(struct device *dev) {}

>  static inline void pm_runtime_put_noidle(struct device *dev) {}

> --

> 2.30.2

>
Rafael J. Wysocki Aug. 4, 2021, 6:12 p.m. UTC | #2
On Sat, Jul 31, 2021 at 9:50 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>

> Use two new helpers instead of pm_runtime_enable() and pm_clk_create(),

> removing the need for calling pm_runtime_disable and pm_clk_destroy().

>

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


This needs some ACKs if you want me to apply it.

> ---

>  drivers/clk/qcom/camcc-sc7180.c       | 25 +++++++++------------

>  drivers/clk/qcom/lpass-gfm-sm8250.c   | 21 ++++++++----------

>  drivers/clk/qcom/lpasscorecc-sc7180.c | 18 ++-------------

>  drivers/clk/qcom/mss-sc7180.c         | 30 +++++++------------------

>  drivers/clk/qcom/q6sstop-qcs404.c     | 32 ++++++++-------------------

>  drivers/clk/qcom/turingcc-qcs404.c    | 30 +++++++------------------

>  6 files changed, 46 insertions(+), 110 deletions(-)

>

> diff --git a/drivers/clk/qcom/camcc-sc7180.c b/drivers/clk/qcom/camcc-sc7180.c

> index 9bcf2f8ed4de..ce73ee9037cb 100644

> --- a/drivers/clk/qcom/camcc-sc7180.c

> +++ b/drivers/clk/qcom/camcc-sc7180.c

> @@ -1652,32 +1652,35 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)

>         struct regmap *regmap;

>         int ret;

>

> -       pm_runtime_enable(&pdev->dev);

> -       ret = pm_clk_create(&pdev->dev);

> +       ret = devm_pm_runtime_enable(&pdev->dev);

> +       if (ret < 0)

> +               return ret;

> +

> +       ret = devm_pm_clk_create(&pdev->dev);

>         if (ret < 0)

>                 return ret;

>

>         ret = pm_clk_add(&pdev->dev, "xo");

>         if (ret < 0) {

>                 dev_err(&pdev->dev, "Failed to acquire XO clock\n");

> -               goto disable_pm_runtime;

> +               return ret;

>         }

>

>         ret = pm_clk_add(&pdev->dev, "iface");

>         if (ret < 0) {

>                 dev_err(&pdev->dev, "Failed to acquire iface clock\n");

> -               goto disable_pm_runtime;

> +               return ret;

>         }

>

>         ret = pm_runtime_get(&pdev->dev);

>         if (ret)

> -               goto destroy_pm_clk;

> +               return ret;

>

>         regmap = qcom_cc_map(pdev, &cam_cc_sc7180_desc);

>         if (IS_ERR(regmap)) {

>                 ret = PTR_ERR(regmap);

>                 pm_runtime_put(&pdev->dev);

> -               goto destroy_pm_clk;

> +               return ret;

>         }

>

>         clk_fabia_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);

> @@ -1689,18 +1692,10 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)

>         pm_runtime_put(&pdev->dev);

>         if (ret < 0) {

>                 dev_err(&pdev->dev, "Failed to register CAM CC clocks\n");

> -               goto destroy_pm_clk;

> +               return ret;

>         }

>

>         return 0;

> -

> -destroy_pm_clk:

> -       pm_clk_destroy(&pdev->dev);

> -

> -disable_pm_runtime:

> -       pm_runtime_disable(&pdev->dev);

> -

> -       return ret;

>  }

>

>  static const struct dev_pm_ops cam_cc_pm_ops = {

> diff --git a/drivers/clk/qcom/lpass-gfm-sm8250.c b/drivers/clk/qcom/lpass-gfm-sm8250.c

> index f5e31e692b9b..96f476f24eb2 100644

> --- a/drivers/clk/qcom/lpass-gfm-sm8250.c

> +++ b/drivers/clk/qcom/lpass-gfm-sm8250.c

> @@ -251,15 +251,18 @@ static int lpass_gfm_clk_driver_probe(struct platform_device *pdev)

>         if (IS_ERR(cc->base))

>                 return PTR_ERR(cc->base);

>

> -       pm_runtime_enable(dev);

> -       err = pm_clk_create(dev);

> +       err = devm_pm_runtime_enable(dev);

>         if (err)

> -               goto pm_clk_err;

> +               return err;

> +

> +       err = devm_pm_clk_create(dev);

> +       if (err)

> +               return err;

>

>         err = of_pm_clk_add_clks(dev);

>         if (err < 0) {

>                 dev_dbg(dev, "Failed to get lpass core voting clocks\n");

> -               goto clk_reg_err;

> +               return err;

>         }

>

>         for (i = 0; i < data->onecell_data->num; i++) {

> @@ -273,22 +276,16 @@ static int lpass_gfm_clk_driver_probe(struct platform_device *pdev)

>

>                 err = devm_clk_hw_register(dev, &data->gfm_clks[i]->hw);

>                 if (err)

> -                       goto clk_reg_err;

> +                       return err;

>

>         }

>

>         err = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,

>                                           data->onecell_data);

>         if (err)

> -               goto clk_reg_err;

> +               return err;

>

>         return 0;

> -

> -clk_reg_err:

> -       pm_clk_destroy(dev);

> -pm_clk_err:

> -       pm_runtime_disable(dev);

> -       return err;

>  }

>

>  static const struct of_device_id lpass_gfm_clk_match_table[] = {

> diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c

> index 2e0ecc38efdd..ac09b7b840ab 100644

> --- a/drivers/clk/qcom/lpasscorecc-sc7180.c

> +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c

> @@ -356,32 +356,18 @@ static const struct qcom_cc_desc lpass_audio_hm_sc7180_desc = {

>         .num_gdscs = ARRAY_SIZE(lpass_audio_hm_sc7180_gdscs),

>  };

>

> -static void lpass_pm_runtime_disable(void *data)

> -{

> -       pm_runtime_disable(data);

> -}

> -

> -static void lpass_pm_clk_destroy(void *data)

> -{

> -       pm_clk_destroy(data);

> -}

> -

>  static int lpass_create_pm_clks(struct platform_device *pdev)

>  {

>         int ret;

>

>         pm_runtime_use_autosuspend(&pdev->dev);

>         pm_runtime_set_autosuspend_delay(&pdev->dev, 500);

> -       pm_runtime_enable(&pdev->dev);

>

> -       ret = devm_add_action_or_reset(&pdev->dev, lpass_pm_runtime_disable, &pdev->dev);

> +       ret = devm_pm_runtime_enable(&pdev->dev);

>         if (ret)

>                 return ret;

>

> -       ret = pm_clk_create(&pdev->dev);

> -       if (ret)

> -               return ret;

> -       ret = devm_add_action_or_reset(&pdev->dev, lpass_pm_clk_destroy, &pdev->dev);

> +       ret = devm_pm_clk_create(&pdev->dev);

>         if (ret)

>                 return ret;

>

> diff --git a/drivers/clk/qcom/mss-sc7180.c b/drivers/clk/qcom/mss-sc7180.c

> index 673fa1a4f734..5a1407440662 100644

> --- a/drivers/clk/qcom/mss-sc7180.c

> +++ b/drivers/clk/qcom/mss-sc7180.c

> @@ -73,36 +73,23 @@ static int mss_sc7180_probe(struct platform_device *pdev)

>  {

>         int ret;

>

> -       pm_runtime_enable(&pdev->dev);

> -       ret = pm_clk_create(&pdev->dev);

> +       ret = devm_pm_runtime_enable(&pdev->dev);

>         if (ret)

> -               goto disable_pm_runtime;

> +               return ret;

> +

> +       ret = devm_pm_clk_create(&pdev->dev);

> +       if (ret)

> +               return ret;

>

>         ret = pm_clk_add(&pdev->dev, "cfg_ahb");

>         if (ret < 0) {

>                 dev_err(&pdev->dev, "failed to acquire iface clock\n");

> -               goto destroy_pm_clk;

> +               return ret;

>         }

>

>         ret = qcom_cc_probe(pdev, &mss_sc7180_desc);

>         if (ret < 0)

> -               goto destroy_pm_clk;

> -

> -       return 0;

> -

> -destroy_pm_clk:

> -       pm_clk_destroy(&pdev->dev);

> -

> -disable_pm_runtime:

> -       pm_runtime_disable(&pdev->dev);

> -

> -       return ret;

> -}

> -

> -static int mss_sc7180_remove(struct platform_device *pdev)

> -{

> -       pm_clk_destroy(&pdev->dev);

> -       pm_runtime_disable(&pdev->dev);

> +               return ret;

>

>         return 0;

>  }

> @@ -119,7 +106,6 @@ MODULE_DEVICE_TABLE(of, mss_sc7180_match_table);

>

>  static struct platform_driver mss_sc7180_driver = {

>         .probe          = mss_sc7180_probe,

> -       .remove         = mss_sc7180_remove,

>         .driver         = {

>                 .name           = "sc7180-mss",

>                 .of_match_table = mss_sc7180_match_table,

> diff --git a/drivers/clk/qcom/q6sstop-qcs404.c b/drivers/clk/qcom/q6sstop-qcs404.c

> index 723f932fbf7d..507386bee07d 100644

> --- a/drivers/clk/qcom/q6sstop-qcs404.c

> +++ b/drivers/clk/qcom/q6sstop-qcs404.c

> @@ -159,15 +159,18 @@ static int q6sstopcc_qcs404_probe(struct platform_device *pdev)

>         const struct qcom_cc_desc *desc;

>         int ret;

>

> -       pm_runtime_enable(&pdev->dev);

> -       ret = pm_clk_create(&pdev->dev);

> +       ret = devm_pm_runtime_enable(&pdev->dev);

>         if (ret)

> -               goto disable_pm_runtime;

> +               return ret;

> +

> +       ret = devm_pm_clk_create(&pdev->dev);

> +       if (ret)

> +               return ret;

>

>         ret = pm_clk_add(&pdev->dev, NULL);

>         if (ret < 0) {

>                 dev_err(&pdev->dev, "failed to acquire iface clock\n");

> -               goto destroy_pm_clk;

> +               return ret;

>         }

>

>         q6sstop_regmap_config.name = "q6sstop_tcsr";

> @@ -175,30 +178,14 @@ static int q6sstopcc_qcs404_probe(struct platform_device *pdev)

>

>         ret = qcom_cc_probe_by_index(pdev, 1, desc);

>         if (ret)

> -               goto destroy_pm_clk;

> +               return ret;

>

>         q6sstop_regmap_config.name = "q6sstop_cc";

>         desc = &q6sstop_qcs404_desc;

>

>         ret = qcom_cc_probe_by_index(pdev, 0, desc);

>         if (ret)

> -               goto destroy_pm_clk;

> -

> -       return 0;

> -

> -destroy_pm_clk:

> -       pm_clk_destroy(&pdev->dev);

> -

> -disable_pm_runtime:

> -       pm_runtime_disable(&pdev->dev);

> -

> -       return ret;

> -}

> -

> -static int q6sstopcc_qcs404_remove(struct platform_device *pdev)

> -{

> -       pm_clk_destroy(&pdev->dev);

> -       pm_runtime_disable(&pdev->dev);

> +               return ret;

>

>         return 0;

>  }

> @@ -209,7 +196,6 @@ static const struct dev_pm_ops q6sstopcc_pm_ops = {

>

>  static struct platform_driver q6sstopcc_qcs404_driver = {

>         .probe          = q6sstopcc_qcs404_probe,

> -       .remove         = q6sstopcc_qcs404_remove,

>         .driver         = {

>                 .name   = "qcs404-q6sstopcc",

>                 .of_match_table = q6sstopcc_qcs404_match_table,

> diff --git a/drivers/clk/qcom/turingcc-qcs404.c b/drivers/clk/qcom/turingcc-qcs404.c

> index 4cfbbf5bf4d9..4543bda793f4 100644

> --- a/drivers/clk/qcom/turingcc-qcs404.c

> +++ b/drivers/clk/qcom/turingcc-qcs404.c

> @@ -110,36 +110,23 @@ static int turingcc_probe(struct platform_device *pdev)

>  {

>         int ret;

>

> -       pm_runtime_enable(&pdev->dev);

> -       ret = pm_clk_create(&pdev->dev);

> +       ret = devm_pm_runtime_enable(&pdev->dev);

>         if (ret)

> -               goto disable_pm_runtime;

> +               return ret;

> +

> +       ret = devm_pm_clk_create(&pdev->dev);

> +       if (ret)

> +               return ret;

>

>         ret = pm_clk_add(&pdev->dev, NULL);

>         if (ret < 0) {

>                 dev_err(&pdev->dev, "failed to acquire iface clock\n");

> -               goto destroy_pm_clk;

> +               return ret;

>         }

>

>         ret = qcom_cc_probe(pdev, &turingcc_desc);

>         if (ret < 0)

> -               goto destroy_pm_clk;

> -

> -       return 0;

> -

> -destroy_pm_clk:

> -       pm_clk_destroy(&pdev->dev);

> -

> -disable_pm_runtime:

> -       pm_runtime_disable(&pdev->dev);

> -

> -       return ret;

> -}

> -

> -static int turingcc_remove(struct platform_device *pdev)

> -{

> -       pm_clk_destroy(&pdev->dev);

> -       pm_runtime_disable(&pdev->dev);

> +               return ret;

>

>         return 0;

>  }

> @@ -156,7 +143,6 @@ MODULE_DEVICE_TABLE(of, turingcc_match_table);

>

>  static struct platform_driver turingcc_driver = {

>         .probe          = turingcc_probe,

> -       .remove         = turingcc_remove,

>         .driver         = {

>                 .name   = "qcs404-turingcc",

>                 .of_match_table = turingcc_match_table,

> --

> 2.30.2

>
Dmitry Baryshkov Aug. 4, 2021, 9:02 p.m. UTC | #3
On Wed, 4 Aug 2021 at 21:07, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sat, Jul 31, 2021 at 9:50 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > A typical code pattern for pm_runtime_enable() call is to call it in the
> > _probe function and to call pm_runtime_disable() both from _probe error
> > path and from _remove function. For some drivers the whole remove
> > function would consist of the call to pm_remove_disable().
> >
> > Add helper function to replace this bolierplate piece of code. Calling
> > devm_pm_runtime_enable() removes the need for calling
> > pm_runtime_disable() both in the probe()'s error path and in the
> > remove() function.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/base/power/runtime.c | 17 +++++++++++++++++
> >  include/linux/pm_runtime.h   |  4 ++++
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 8a66eaf731e4..ec94049442b9 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1447,6 +1447,23 @@ void pm_runtime_enable(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(pm_runtime_enable);
> >
> > +static void pm_runtime_disable_action(void *data)
> > +{
> > +       pm_runtime_disable(data);
> > +}
> > +
> > +/**
> > + * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.
> > + * @dev: Device to handle.
> > + */
> > +int devm_pm_runtime_enable(struct device *dev)
> > +{
> > +       pm_runtime_enable(dev);
> > +
> > +       return devm_add_action_or_reset(dev, pm_runtime_disable_action, dev);
>
> When exactly is pm_runtime_disable_action() going to run by this rule?
>  When the device goes away or when the driver is unbound from it?

When the driver is unbound (either because probe() returns an error or
because __device_release_driver() is being called).
This corresponds to a typical call to pm_runtime_disable() from the
probe()'s error path or in the remove() callback.

> > +}
> > +EXPORT_SYMBOL_GPL(devm_pm_runtime_enable);
> > +
> >  /**
> >   * pm_runtime_forbid - Block runtime PM of a device.
> >   * @dev: Device to handle.
> > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > index aab8b35e9f8a..222da43b7096 100644
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -59,6 +59,8 @@ extern void pm_runtime_put_suppliers(struct device *dev);
> >  extern void pm_runtime_new_link(struct device *dev);
> >  extern void pm_runtime_drop_link(struct device_link *link);
> >
> > +extern int devm_pm_runtime_enable(struct device *dev);
> > +
> >  /**
> >   * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> >   * @dev: Target device.
> > @@ -253,6 +255,8 @@ static inline void __pm_runtime_disable(struct device *dev, bool c) {}
> >  static inline void pm_runtime_allow(struct device *dev) {}
> >  static inline void pm_runtime_forbid(struct device *dev) {}
> >
> > +static inline int devm_pm_runtime_enable(struct device *dev) { return 0; }
> > +
> >  static inline void pm_suspend_ignore_children(struct device *dev, bool enable) {}
> >  static inline void pm_runtime_get_noresume(struct device *dev) {}
> >  static inline void pm_runtime_put_noidle(struct device *dev) {}
> > --
> > 2.30.2
> >
Dmitry Baryshkov Aug. 6, 2021, 7:08 a.m. UTC | #4
On Fri, 6 Aug 2021 at 02:53, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2021-07-31 12:50:31)
> > Most of the drivers using using pm_runtime_enable() or pm_clk_create()
> > follow the same pattern: call the function in the probe() path and call
> > correspondingly pm_runtime_disable() or pm_clk_destroy() from the
> > probe()'s error path and from the remove() function. This common code
> > pattern has several drawbacks. I.e. driver authors have to ensure that
> > the disable/destroy call in the error path really corresponds to the
> > proper error clause. Or that the disable/destroy call is not missed in
> > the remove() callback.
> >
> > Add two devres helpers replacing these code patterns with relevant devm
> > function call, removing the need to call corresponding disable/destroy
> > functions. As an example modify Qualcomm clock controller code to use
> > new helpers. In this case we are able to drop error path and remove
> > functions completely, simplifying the drivers in question.
>
> There are lots of folks on the To: line so I'm not sure who is supposed
> to apply this. Please indicate which maintainer tree you're planning to
> land a series through if it touches different areas of the tree.

I'd prefer for them to go through the clock tree.
Rafael J. Wysocki Aug. 6, 2021, 1:27 p.m. UTC | #5
On Wed, Aug 4, 2021 at 11:03 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>

> On Wed, 4 Aug 2021 at 21:07, Rafael J. Wysocki <rafael@kernel.org> wrote:

> >

> > On Sat, Jul 31, 2021 at 9:50 PM Dmitry Baryshkov

> > <dmitry.baryshkov@linaro.org> wrote:

> > >

> > > A typical code pattern for pm_runtime_enable() call is to call it in the

> > > _probe function and to call pm_runtime_disable() both from _probe error

> > > path and from _remove function. For some drivers the whole remove

> > > function would consist of the call to pm_remove_disable().

> > >

> > > Add helper function to replace this bolierplate piece of code. Calling

> > > devm_pm_runtime_enable() removes the need for calling

> > > pm_runtime_disable() both in the probe()'s error path and in the

> > > remove() function.

> > >

> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> > > ---

> > >  drivers/base/power/runtime.c | 17 +++++++++++++++++

> > >  include/linux/pm_runtime.h   |  4 ++++

> > >  2 files changed, 21 insertions(+)

> > >

> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c

> > > index 8a66eaf731e4..ec94049442b9 100644

> > > --- a/drivers/base/power/runtime.c

> > > +++ b/drivers/base/power/runtime.c

> > > @@ -1447,6 +1447,23 @@ void pm_runtime_enable(struct device *dev)

> > >  }

> > >  EXPORT_SYMBOL_GPL(pm_runtime_enable);

> > >

> > > +static void pm_runtime_disable_action(void *data)

> > > +{

> > > +       pm_runtime_disable(data);

> > > +}

> > > +

> > > +/**

> > > + * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.

> > > + * @dev: Device to handle.

> > > + */

> > > +int devm_pm_runtime_enable(struct device *dev)

> > > +{

> > > +       pm_runtime_enable(dev);

> > > +

> > > +       return devm_add_action_or_reset(dev, pm_runtime_disable_action, dev);

> >

> > When exactly is pm_runtime_disable_action() going to run by this rule?

> >  When the device goes away or when the driver is unbound from it?

>

> When the driver is unbound (either because probe() returns an error or

> because __device_release_driver() is being called).

> This corresponds to a typical call to pm_runtime_disable() from the

> probe()'s error path or in the remove() callback.


OK, so

Acked-by: Rafael J. Wysocki <rafael@kernel.org>


for the PM-runtime framework changes in this series (patches [1-2/3])
and please feel free to route them in through whatever tree is most
suitable (or let me know if you want me to pick them up).
Dmitry Baryshkov Aug. 16, 2021, 5:08 p.m. UTC | #6
On 06/08/2021 10:08, Dmitry Baryshkov wrote:
> On Fri, 6 Aug 2021 at 02:53, Stephen Boyd <sboyd@kernel.org> wrote:

>>

>> Quoting Dmitry Baryshkov (2021-07-31 12:50:31)

>>> Most of the drivers using using pm_runtime_enable() or pm_clk_create()

>>> follow the same pattern: call the function in the probe() path and call

>>> correspondingly pm_runtime_disable() or pm_clk_destroy() from the

>>> probe()'s error path and from the remove() function. This common code

>>> pattern has several drawbacks. I.e. driver authors have to ensure that

>>> the disable/destroy call in the error path really corresponds to the

>>> proper error clause. Or that the disable/destroy call is not missed in

>>> the remove() callback.

>>>

>>> Add two devres helpers replacing these code patterns with relevant devm

>>> function call, removing the need to call corresponding disable/destroy

>>> functions. As an example modify Qualcomm clock controller code to use

>>> new helpers. In this case we are able to drop error path and remove

>>> functions completely, simplifying the drivers in question.

>>

>> There are lots of folks on the To: line so I'm not sure who is supposed

>> to apply this. Please indicate which maintainer tree you're planning to

>> land a series through if it touches different areas of the tree.

> 

> I'd prefer for them to go through the clock tree.


Stephen, since Rafael has acked the patched, could you please take a 
look and hopefully merge them?


-- 
With best wishes
Dmitry