linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc
@ 2021-07-31 19:50 Dmitry Baryshkov
  2021-07-31 19:50 ` [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper Dmitry Baryshkov
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Dmitry Baryshkov @ 2021-07-31 19:50 UTC (permalink / raw)
  To: Rafael J. Wysocki, Pavel Machek, Greg Kroah-Hartman,
	Stephen Boyd, Taniya Das, Michael Turquette, Andy Gross,
	Bjorn Andersson
  Cc: linux-pm, linux-kernel, linux-clk, linux-arm-msm

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(-)



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper
  2021-07-31 19:50 [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc Dmitry Baryshkov
@ 2021-07-31 19:50 ` Dmitry Baryshkov
  2021-08-04 18:06   ` Rafael J. Wysocki
  2021-07-31 19:50 ` [PATCH v3 2/3] PM: runtime: add devm_pm_clk_create helper Dmitry Baryshkov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Dmitry Baryshkov @ 2021-07-31 19:50 UTC (permalink / raw)
  To: Rafael J. Wysocki, Pavel Machek, Greg Kroah-Hartman,
	Stephen Boyd, Taniya Das, Michael Turquette, Andy Gross,
	Bjorn Andersson
  Cc: linux-pm, linux-kernel, linux-clk, linux-arm-msm

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);
+}
+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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 2/3] PM: runtime: add devm_pm_clk_create helper
  2021-07-31 19:50 [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc Dmitry Baryshkov
  2021-07-31 19:50 ` [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper Dmitry Baryshkov
@ 2021-07-31 19:50 ` Dmitry Baryshkov
  2021-08-31 12:15   ` Geert Uytterhoeven
  2021-07-31 19:50 ` [PATCH v3 3/3] clk: qcom: use devm_pm_runtime_enable and devm_pm_clk_create Dmitry Baryshkov
       [not found] ` <162820760640.19113.2386978922035728014@swboyd.mtv.corp.google.com>
  3 siblings, 1 reply; 11+ messages in thread
From: Dmitry Baryshkov @ 2021-07-31 19:50 UTC (permalink / raw)
  To: Rafael J. Wysocki, Pavel Machek, Greg Kroah-Hartman,
	Stephen Boyd, Taniya Das, Michael Turquette, Andy Gross,
	Bjorn Andersson
  Cc: linux-pm, linux-kernel, linux-clk, linux-arm-msm

A typical code pattern for pm_clk_create() call is to call it in the
_probe function and to call pm_clk_destroy() 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_clk_create() removes the need for calling pm_clk_destroy() both
in the probe()'s error path and in the remove() function.

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);
+}
+
+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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 3/3] clk: qcom: use devm_pm_runtime_enable and devm_pm_clk_create
  2021-07-31 19:50 [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc Dmitry Baryshkov
  2021-07-31 19:50 ` [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper Dmitry Baryshkov
  2021-07-31 19:50 ` [PATCH v3 2/3] PM: runtime: add devm_pm_clk_create helper Dmitry Baryshkov
@ 2021-07-31 19:50 ` Dmitry Baryshkov
  2021-08-04 18:12   ` Rafael J. Wysocki
       [not found] ` <162820760640.19113.2386978922035728014@swboyd.mtv.corp.google.com>
  3 siblings, 1 reply; 11+ messages in thread
From: Dmitry Baryshkov @ 2021-07-31 19:50 UTC (permalink / raw)
  To: Rafael J. Wysocki, Pavel Machek, Greg Kroah-Hartman,
	Stephen Boyd, Taniya Das, Michael Turquette, Andy Gross,
	Bjorn Andersson
  Cc: linux-pm, linux-kernel, linux-clk, linux-arm-msm

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>
---
 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper
  2021-07-31 19:50 ` [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper Dmitry Baryshkov
@ 2021-08-04 18:06   ` Rafael J. Wysocki
  2021-08-04 21:02     ` Dmitry Baryshkov
  0 siblings, 1 reply; 11+ messages in thread
From: Rafael J. Wysocki @ 2021-08-04 18:06 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Rafael J. Wysocki, Pavel Machek, Greg Kroah-Hartman,
	Stephen Boyd, Taniya Das, Michael Turquette, Andy Gross,
	Bjorn Andersson, Linux PM, Linux Kernel Mailing List, linux-clk,
	linux-arm-msm

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/3] clk: qcom: use devm_pm_runtime_enable and devm_pm_clk_create
  2021-07-31 19:50 ` [PATCH v3 3/3] clk: qcom: use devm_pm_runtime_enable and devm_pm_clk_create Dmitry Baryshkov
@ 2021-08-04 18:12   ` Rafael J. Wysocki
  0 siblings, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2021-08-04 18:12 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Rafael J. Wysocki, Pavel Machek, Greg Kroah-Hartman,
	Stephen Boyd, Taniya Das, Michael Turquette, Andy Gross,
	Bjorn Andersson, Linux PM, Linux Kernel Mailing List, linux-clk,
	linux-arm-msm

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper
  2021-08-04 18:06   ` Rafael J. Wysocki
@ 2021-08-04 21:02     ` Dmitry Baryshkov
  2021-08-06 13:27       ` Rafael J. Wysocki
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Baryshkov @ 2021-08-04 21:02 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Pavel Machek, Greg Kroah-Hartman,
	Stephen Boyd, Taniya Das, Michael Turquette, Andy Gross,
	Bjorn Andersson, Linux PM, Linux Kernel Mailing List, linux-clk,
	linux-arm-msm

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
> >



-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc
       [not found] ` <162820760640.19113.2386978922035728014@swboyd.mtv.corp.google.com>
@ 2021-08-06  7:08   ` Dmitry Baryshkov
  2021-08-16 17:08     ` Dmitry Baryshkov
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Baryshkov @ 2021-08-06  7:08 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman,
	Michael Turquette, Pavel Machek, Rafael J. Wysocki, Taniya Das,
	Linux PM, open list, open list:COMMON CLK FRAMEWORK,
	open list:DRM DRIVER FOR MSM ADRENO GPU

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.

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper
  2021-08-04 21:02     ` Dmitry Baryshkov
@ 2021-08-06 13:27       ` Rafael J. Wysocki
  0 siblings, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2021-08-06 13:27 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Pavel Machek,
	Greg Kroah-Hartman, Stephen Boyd, Taniya Das, Michael Turquette,
	Andy Gross, Bjorn Andersson, Linux PM, Linux Kernel Mailing List,
	linux-clk, linux-arm-msm

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).

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc
  2021-08-06  7:08   ` [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc Dmitry Baryshkov
@ 2021-08-16 17:08     ` Dmitry Baryshkov
  0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Baryshkov @ 2021-08-16 17:08 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman,
	Michael Turquette, Pavel Machek, Rafael J. Wysocki, Taniya Das,
	Linux PM, open list, open list:COMMON CLK FRAMEWORK,
	open list:DRM DRIVER FOR MSM ADRENO GPU

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] PM: runtime: add devm_pm_clk_create helper
  2021-07-31 19:50 ` [PATCH v3 2/3] PM: runtime: add devm_pm_clk_create helper Dmitry Baryshkov
@ 2021-08-31 12:15   ` Geert Uytterhoeven
  0 siblings, 0 replies; 11+ messages in thread
From: Geert Uytterhoeven @ 2021-08-31 12:15 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Rafael J. Wysocki, Pavel Machek, Greg Kroah-Hartman,
	Stephen Boyd, Taniya Das, Michael Turquette, Andy Gross,
	Bjorn Andersson, Linux PM list, Linux Kernel Mailing List,
	linux-clk, linux-arm-msm

Hi Dmitry,

Thanks for your patch!

On Sat, Jul 31, 2021 at 9:51 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
> A typical code pattern for pm_clk_create() call is to call it in the
> _probe function and to call pm_clk_destroy() 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().

Is it? I could find only one (drivers/clk/mmp/clk-audio.c).
All other users call it from their PM Domain .attach_dev() callback,
which cannot become managed.

> Add helper function to replace this bolierplate piece of code. Calling
> devm_pm_clk_create() removes the need for calling pm_clk_destroy() both
> in the probe()'s error path and in the remove() function.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-08-31 12:15 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-31 19:50 [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc Dmitry Baryshkov
2021-07-31 19:50 ` [PATCH v3 1/3] PM: runtime: add devm_pm_runtime_enable helper Dmitry Baryshkov
2021-08-04 18:06   ` Rafael J. Wysocki
2021-08-04 21:02     ` Dmitry Baryshkov
2021-08-06 13:27       ` Rafael J. Wysocki
2021-07-31 19:50 ` [PATCH v3 2/3] PM: runtime: add devm_pm_clk_create helper Dmitry Baryshkov
2021-08-31 12:15   ` Geert Uytterhoeven
2021-07-31 19:50 ` [PATCH v3 3/3] clk: qcom: use devm_pm_runtime_enable and devm_pm_clk_create Dmitry Baryshkov
2021-08-04 18:12   ` Rafael J. Wysocki
     [not found] ` <162820760640.19113.2386978922035728014@swboyd.mtv.corp.google.com>
2021-08-06  7:08   ` [PATCH v3 0/3] PM: add two devres helpers and use them in qcom cc Dmitry Baryshkov
2021-08-16 17:08     ` Dmitry Baryshkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).