| Message ID | 20210728142445.774158-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | PM: add two devres helpers and use them in qcom cc | expand |
On Wed 28 Jul 09:24 CDT 2021, Dmitry Baryshkov 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> It's quite reasonable to expect that Rafael wants to take the two first patches through his tree, but I expect that these clock patches will conflict with your other changes resulting in a less smooth path into mainline. Grepping the tree for candidates I find many better examples, that we don't foresee any conflicting changes.. So may I suggest that you fix drivers/hwspinlock/omap_hwspinlock.c and drivers/hwspinlock/stm32_hwspinlock.c, then I (as maintainer) can Ack those changes and Rafael can take the series in his tree for v5.15. Then as v5.15-rc1 lands, we have the API and we can follow up changing these more complex drivers - and those patches can go through the individual maintainer trees. Regards, Bjorn > --- > 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 28 Jul 07:24 PDT 2021, Dmitry Baryshkov wrote: > Add devm_pm_clk_create helper, devres-enabled version of the > pm_clk_create(), which will call pm_clk_destroy at the correct time. > As with path 1, please describe why this is a good thing. (I definitely think it is, but I've been part of the discussion leading up to this patch) > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/base/power/clock_ops.c | 17 +++++++++++++++++ > include/linux/pm_clock.h | 5 +++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c > index 0251f3e6e61d..4110c19c08dc 100644 > --- a/drivers/base/power/clock_ops.c > +++ b/drivers/base/power/clock_ops.c > @@ -519,6 +519,23 @@ void pm_clk_destroy(struct device *dev) > } > EXPORT_SYMBOL_GPL(pm_clk_destroy); > > +static void pm_clk_destroy_action(void *data) > +{ > + pm_clk_destroy(data); > +} > + As this is an addition to the API, it deserves some kerneldoc. Regards, Bjorn > +int devm_pm_clk_create(struct device *dev) > +{ > + int ret; > + > + ret = pm_clk_create(dev); > + if (ret) > + return ret; > + > + return devm_add_action_or_reset(dev, pm_clk_destroy_action, dev); > +} > +EXPORT_SYMBOL_GPL(devm_pm_clk_create); > + > /** > * pm_clk_suspend - Disable clocks in a device's PM clock list. > * @dev: Device to disable the clocks for. > diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h > index 8ddc7860e131..ada3a0ab10bf 100644 > --- a/include/linux/pm_clock.h > +++ b/include/linux/pm_clock.h > @@ -47,6 +47,7 @@ extern void pm_clk_remove(struct device *dev, const char *con_id); > extern void pm_clk_remove_clk(struct device *dev, struct clk *clk); > extern int pm_clk_suspend(struct device *dev); > extern int pm_clk_resume(struct device *dev); > +extern int devm_pm_clk_create(struct device *dev); > #else > static inline bool pm_clk_no_clocks(struct device *dev) > { > @@ -83,6 +84,10 @@ static inline void pm_clk_remove(struct device *dev, const char *con_id) > static inline void pm_clk_remove_clk(struct device *dev, struct clk *clk) > { > } > +static inline int devm_pm_clk_create(struct device *dev) > +{ > + return -EINVAL; > +} > #endif > > #ifdef CONFIG_HAVE_CLK > -- > 2.30.2 >
Qualcomm clock controller code (and most probably other drivers) would benefit from having devres helpers for pm_runtime_enable() and pm_clk_create(). Add those two helpers. Modify Qualcomm clock controller code to use new helpers and separate common pm code into common.c. ---------------------------------------------------------------- Dmitry Baryshkov (5): PM: runtime: add devm_pm_runtime_enable helper PM: clk: add devm_pm_clk_create helper clk: qcom: use devm_pm_runtime_enable and devm_pm_clk_create clk: qcom: use common code for qcom_cc_probe_by_index clk: qcom: move pm_clk functionality into common code drivers/base/power/clock_ops.c | 17 +++++++++++ drivers/base/power/runtime.c | 17 +++++++++++ drivers/clk/qcom/camcc-sc7180.c | 39 +++++++++---------------- drivers/clk/qcom/common.c | 55 ++++++++++++++++++++++++++++------- drivers/clk/qcom/common.h | 17 +++++++++++ drivers/clk/qcom/lpass-gfm-sm8250.c | 21 ++++++------- drivers/clk/qcom/lpasscorecc-sc7180.c | 55 +++++++++-------------------------- drivers/clk/qcom/mss-sc7180.c | 45 ++++++---------------------- drivers/clk/qcom/q6sstop-qcs404.c | 40 +++++++------------------ drivers/clk/qcom/turingcc-qcs404.c | 45 ++++++---------------------- include/linux/pm_clock.h | 5 ++++ include/linux/pm_runtime.h | 4 +++ 12 files changed, 169 insertions(+), 191 deletions(-)