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