Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 1/3] clk: qcom: lpasscc-sc7810: Use devm in probe
@ 2020-10-14 21:05 Douglas Anderson
  2020-10-14 21:05 ` [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices Douglas Anderson
  2020-10-14 21:05 ` [PATCH v3 3/3] clk: qcom: lpasscc-sc7180: Re-configure the PLL in case lost Douglas Anderson
  0 siblings, 2 replies; 11+ messages in thread
From: Douglas Anderson @ 2020-10-14 21:05 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Taniya Das, linux-soc, David Brown, Rajendra Nayak,
	Douglas Anderson, Andy Gross, Bjorn Andersson, Michael Turquette,
	linux-arm-msm, linux-clk, linux-kernel

Let's convert the lpass clock control driver to use devm.  This is a
few more lines of code, but it will be useful in a later patch which
disentangles the two devices handled by this driver.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- ("clk: qcom: lpasscc-sc7810: Use devm in probe") new for v3.

 drivers/clk/qcom/lpasscorecc-sc7180.c | 38 +++++++++++++++------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
index 228d08f5d26f..abcf36006926 100644
--- a/drivers/clk/qcom/lpasscorecc-sc7180.c
+++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
@@ -412,40 +412,44 @@ static const struct of_device_id lpass_core_cc_sc7180_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, lpass_core_cc_sc7180_match_table);
 
+static void lpass_pm_runtime_disable(void *data)
+{
+	pm_runtime_disable(data);
+}
+
+static void lapss_pm_clk_destroy(void *data)
+{
+	pm_clk_destroy(data);
+}
+
 static int lpass_core_sc7180_probe(struct platform_device *pdev)
 {
 	int (*clk_probe)(struct platform_device *p);
 	int ret;
 
 	pm_runtime_enable(&pdev->dev);
+	ret = devm_add_action_or_reset(&pdev->dev, lpass_pm_runtime_disable, &pdev->dev);
+	if (ret)
+		return ret;
+
 	ret = pm_clk_create(&pdev->dev);
 	if (ret)
-		goto disable_pm_runtime;
+		return ret;
+	ret = devm_add_action_or_reset(&pdev->dev, lapss_pm_clk_destroy, &pdev->dev);
+	if (ret)
+		return ret;
 
 	ret = pm_clk_add(&pdev->dev, "iface");
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to acquire iface clock\n");
-		goto destroy_pm_clk;
+		return ret;
 	}
 
-	ret = -EINVAL;
 	clk_probe = of_device_get_match_data(&pdev->dev);
 	if (!clk_probe)
-		goto destroy_pm_clk;
-
-	ret = clk_probe(pdev);
-	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 -EINVAL;
 
-	return ret;
+	return clk_probe(pdev);
 }
 
 static const struct dev_pm_ops lpass_core_cc_pm_ops = {
-- 
2.28.0.1011.ga647a8990f-goog


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

* [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices
  2020-10-14 21:05 [PATCH v3 1/3] clk: qcom: lpasscc-sc7810: Use devm in probe Douglas Anderson
@ 2020-10-14 21:05 ` Douglas Anderson
  2020-10-14 22:10   ` Stephen Boyd
  2020-10-14 21:05 ` [PATCH v3 3/3] clk: qcom: lpasscc-sc7180: Re-configure the PLL in case lost Douglas Anderson
  1 sibling, 1 reply; 11+ messages in thread
From: Douglas Anderson @ 2020-10-14 21:05 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Taniya Das, linux-soc, David Brown, Rajendra Nayak,
	Douglas Anderson, Andy Gross, Bjorn Andersson, Michael Turquette,
	linux-arm-msm, linux-clk, linux-kernel

The sc7180 lpass clock driver manages two different devices.  These
two devices were tangled together, using one probe and a lookup to
figure out the real probe.  I think it's cleaner to really separate
the probe for these two devices since they're really different things,
just both managed by the same driver.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- ("clk: qcom: lpass-sc7180: Disentangle the two clock devices") new for v3.

 drivers/clk/qcom/lpasscorecc-sc7180.c | 121 +++++++++++++++-----------
 1 file changed, 72 insertions(+), 49 deletions(-)

diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
index abcf36006926..48d370e2108e 100644
--- a/drivers/clk/qcom/lpasscorecc-sc7180.c
+++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
@@ -356,12 +356,48 @@ 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 lapss_pm_clk_destroy(void *data)
+{
+	pm_clk_destroy(data);
+}
+
+static int lpass_create_pm_clks(struct platform_device *pdev)
+{
+	int ret;
+
+	pm_runtime_enable(&pdev->dev);
+	ret = devm_add_action_or_reset(&pdev->dev, lpass_pm_runtime_disable, &pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_clk_create(&pdev->dev);
+	if (ret)
+		return ret;
+	ret = devm_add_action_or_reset(&pdev->dev, lapss_pm_clk_destroy, &pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_clk_add(&pdev->dev, "iface");
+	if (ret < 0)
+		dev_err(&pdev->dev, "failed to acquire iface clock\n");
+	return ret;
+}
+
 static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
 {
 	const struct qcom_cc_desc *desc;
 	struct regmap *regmap;
 	int ret;
 
+	ret = lpass_create_pm_clks(pdev);
+	if (ret)
+		return ret;
+
 	lpass_core_cc_sc7180_regmap_config.name = "lpass_audio_cc";
 	desc = &lpass_audio_hm_sc7180_desc;
 	ret = qcom_cc_probe_by_index(pdev, 1, desc);
@@ -392,6 +428,11 @@ static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
 static int lpass_hm_core_probe(struct platform_device *pdev)
 {
 	const struct qcom_cc_desc *desc;
+	int ret;
+
+	ret = lpass_create_pm_clks(pdev);
+	if (ret)
+		return ret;
 
 	lpass_core_cc_sc7180_regmap_config.name = "lpass_hm_core";
 	desc = &lpass_core_hm_sc7180_desc;
@@ -399,65 +440,28 @@ static int lpass_hm_core_probe(struct platform_device *pdev)
 	return qcom_cc_probe_by_index(pdev, 0, desc);
 }
 
-static const struct of_device_id lpass_core_cc_sc7180_match_table[] = {
+static const struct of_device_id lpass_hm_sc7180_match_table[] = {
 	{
 		.compatible = "qcom,sc7180-lpasshm",
-		.data = lpass_hm_core_probe,
 	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lpass_hm_sc7180_match_table);
+
+static const struct of_device_id lpass_core_cc_sc7180_match_table[] = {
 	{
 		.compatible = "qcom,sc7180-lpasscorecc",
-		.data = lpass_core_cc_sc7180_probe,
 	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, lpass_core_cc_sc7180_match_table);
 
-static void lpass_pm_runtime_disable(void *data)
-{
-	pm_runtime_disable(data);
-}
-
-static void lapss_pm_clk_destroy(void *data)
-{
-	pm_clk_destroy(data);
-}
-
-static int lpass_core_sc7180_probe(struct platform_device *pdev)
-{
-	int (*clk_probe)(struct platform_device *p);
-	int ret;
-
-	pm_runtime_enable(&pdev->dev);
-	ret = devm_add_action_or_reset(&pdev->dev, lpass_pm_runtime_disable, &pdev->dev);
-	if (ret)
-		return ret;
-
-	ret = pm_clk_create(&pdev->dev);
-	if (ret)
-		return ret;
-	ret = devm_add_action_or_reset(&pdev->dev, lapss_pm_clk_destroy, &pdev->dev);
-	if (ret)
-		return ret;
-
-	ret = pm_clk_add(&pdev->dev, "iface");
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to acquire iface clock\n");
-		return ret;
-	}
-
-	clk_probe = of_device_get_match_data(&pdev->dev);
-	if (!clk_probe)
-		return -EINVAL;
-
-	return clk_probe(pdev);
-}
-
 static const struct dev_pm_ops lpass_core_cc_pm_ops = {
 	SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
 };
 
 static struct platform_driver lpass_core_cc_sc7180_driver = {
-	.probe = lpass_core_sc7180_probe,
+	.probe = lpass_core_cc_sc7180_probe,
 	.driver = {
 		.name = "lpass_core_cc-sc7180",
 		.of_match_table = lpass_core_cc_sc7180_match_table,
@@ -465,17 +469,36 @@ static struct platform_driver lpass_core_cc_sc7180_driver = {
 	},
 };
 
-static int __init lpass_core_cc_sc7180_init(void)
+static const struct dev_pm_ops lpass_hm_pm_ops = {
+	SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
+};
+
+static struct platform_driver lpass_hm_sc7180_driver = {
+	.probe = lpass_hm_core_probe,
+	.driver = {
+		.name = "lpass_hm-sc7180",
+		.of_match_table = lpass_hm_sc7180_match_table,
+		.pm = &lpass_hm_pm_ops,
+	},
+};
+
+static int __init lpass_sc7180_init(void)
 {
-	return platform_driver_register(&lpass_core_cc_sc7180_driver);
+	int ret;
+
+	ret = platform_driver_register(&lpass_core_cc_sc7180_driver);
+	if (ret)
+		return ret;
+	return platform_driver_register(&lpass_hm_sc7180_driver);
 }
-subsys_initcall(lpass_core_cc_sc7180_init);
+subsys_initcall(lpass_sc7180_init);
 
-static void __exit lpass_core_cc_sc7180_exit(void)
+static void __exit lpass_sc7180_exit(void)
 {
+	platform_driver_unregister(&lpass_hm_sc7180_driver);
 	platform_driver_unregister(&lpass_core_cc_sc7180_driver);
 }
-module_exit(lpass_core_cc_sc7180_exit);
+module_exit(lpass_sc7180_exit);
 
 MODULE_DESCRIPTION("QTI LPASS_CORE_CC SC7180 Driver");
 MODULE_LICENSE("GPL v2");
-- 
2.28.0.1011.ga647a8990f-goog


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

* [PATCH v3 3/3] clk: qcom: lpasscc-sc7180: Re-configure the PLL in case lost
  2020-10-14 21:05 [PATCH v3 1/3] clk: qcom: lpasscc-sc7810: Use devm in probe Douglas Anderson
  2020-10-14 21:05 ` [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices Douglas Anderson
@ 2020-10-14 21:05 ` Douglas Anderson
  2020-10-14 23:39   ` Stephen Boyd
  1 sibling, 1 reply; 11+ messages in thread
From: Douglas Anderson @ 2020-10-14 21:05 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Taniya Das, linux-soc, David Brown, Rajendra Nayak,
	Douglas Anderson, Andy Gross, Bjorn Andersson, Michael Turquette,
	linux-arm-msm, linux-clk, linux-kernel

From: Taniya Das <tdas@codeaurora.org>

In the case where the PLL configuration is lost, then the pm runtime
resume will reconfigure before usage.

Fixes: edab812d802d ("clk: qcom: lpass: Add support for LPASS clock controller for SC7180")
Signed-off-by: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Now based on a series which disentangles the two clock devices.
- Use dev_get_regmap().
- Better comment about reading PLL_L_VAL.

Changes in v2:
- Don't needlessly have a 2nd copy of dev_pm_ops and jam it in.
- Check the return value of pm_clk_resume()
- l_val should be unsigned int.

 drivers/clk/qcom/lpasscorecc-sc7180.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
index 48d370e2108e..e12d4c2b1b70 100644
--- a/drivers/clk/qcom/lpasscorecc-sc7180.c
+++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
@@ -388,6 +388,25 @@ static int lpass_create_pm_clks(struct platform_device *pdev)
 	return ret;
 }
 
+static int lpass_core_cc_pm_clk_resume(struct device *dev)
+{
+	struct regmap *regmap = dev_get_regmap(dev, "lpass_core_cc");
+	unsigned int l_val;
+	int ret;
+
+	ret = pm_clk_resume(dev);
+	if (ret)
+		return ret;
+
+	/* If PLL_L_VAL was cleared then we should re-init the whole PLL */
+	regmap_read(regmap, 0x1004, &l_val);
+	if (!l_val)
+		clk_fabia_pll_configure(&lpass_lpaaudio_dig_pll, regmap,
+				&lpass_lpaaudio_dig_pll_config);
+
+	return 0;
+}
+
 static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
 {
 	const struct qcom_cc_desc *desc;
@@ -457,7 +476,7 @@ static const struct of_device_id lpass_core_cc_sc7180_match_table[] = {
 MODULE_DEVICE_TABLE(of, lpass_core_cc_sc7180_match_table);
 
 static const struct dev_pm_ops lpass_core_cc_pm_ops = {
-	SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
+	SET_RUNTIME_PM_OPS(pm_clk_suspend, lpass_core_cc_pm_clk_resume, NULL)
 };
 
 static struct platform_driver lpass_core_cc_sc7180_driver = {
-- 
2.28.0.1011.ga647a8990f-goog


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

* Re: [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices
  2020-10-14 21:05 ` [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices Douglas Anderson
@ 2020-10-14 22:10   ` Stephen Boyd
  2020-10-14 22:28     ` Doug Anderson
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2020-10-14 22:10 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Taniya Das, linux-soc, David Brown, Rajendra Nayak,
	Douglas Anderson, Andy Gross, Bjorn Andersson, Michael Turquette,
	linux-arm-msm, linux-clk, linux-kernel

Quoting Douglas Anderson (2020-10-14 14:05:22)
> diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
> index abcf36006926..48d370e2108e 100644
> --- a/drivers/clk/qcom/lpasscorecc-sc7180.c
> +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
> @@ -356,12 +356,48 @@ 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 lapss_pm_clk_destroy(void *data)
> +{
> +       pm_clk_destroy(data);
> +}

Why are these helpers added again? And do we even need them? Can't we
just pass pm_runtime_disable or pm_clk_destroy to the
devm_add_action_or_reset() second parameter?

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

* Re: [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices
  2020-10-14 22:10   ` Stephen Boyd
@ 2020-10-14 22:28     ` Doug Anderson
  2020-10-14 23:00       ` Stephen Boyd
  0 siblings, 1 reply; 11+ messages in thread
From: Doug Anderson @ 2020-10-14 22:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Taniya Das, open list:ARM/QUALCOMM SUPPORT, David Brown,
	Rajendra Nayak, Andy Gross, Bjorn Andersson, Michael Turquette,
	linux-arm-msm, linux-clk, LKML

Hi,

On Wed, Oct 14, 2020 at 3:10 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Douglas Anderson (2020-10-14 14:05:22)
> > diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > index abcf36006926..48d370e2108e 100644
> > --- a/drivers/clk/qcom/lpasscorecc-sc7180.c
> > +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > @@ -356,12 +356,48 @@ 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 lapss_pm_clk_destroy(void *data)
> > +{
> > +       pm_clk_destroy(data);
> > +}
>
> Why are these helpers added again? And do we even need them? Can't we
> just pass pm_runtime_disable or pm_clk_destroy to the
> devm_add_action_or_reset() second parameter?

Unfortunately, we can't due to the C specification.  Take a look at
all the other users of devm_add_action_or_reset() and they all have
pretty much the same stupid thing.  This is a pretty great grep, for
instance:

git grep devm_add_action_or_reset.*clk_disable_unprepare

I did a quick Google search and the top hit was a stack overflow that
explained it:

https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type

The net-net is that the answer there says:

> Hence, since a void* is not compatible with a struct my_struct*, a
> function pointer of type void (*)(void*) is not compatible with a
> function pointer of type void (*)(struct my_struct*), so this
> casting of function pointers is technically undefined behavior.

I suppose I could try to add devm variants of these functions
somewhere more general if you think it's a good idea, though there it
seems like there's not a huge need since these two greps are zero:

git grep devm_add_action_or_reset.*runtime_disable
git grep devm_add_action_or_reset.*pm_clk_destroy

...actually, do we even need the runtime_disable in the error path?
When the dev goes away does it matter if you left pm_runtime enabled
on it?

-Doug

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

* Re: [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices
  2020-10-14 22:28     ` Doug Anderson
@ 2020-10-14 23:00       ` Stephen Boyd
  2020-10-14 23:07         ` Doug Anderson
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2020-10-14 23:00 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Taniya Das, ARM/QUALCOMM SUPPORT, David Brown, Rajendra Nayak,
	Andy Gross, Bjorn Andersson, Michael Turquette, linux-arm-msm,
	linux-clk, LKML,

Quoting Doug Anderson (2020-10-14 15:28:58)
> Hi,
> 
> On Wed, Oct 14, 2020 at 3:10 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Douglas Anderson (2020-10-14 14:05:22)
> > > diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > index abcf36006926..48d370e2108e 100644
> > > --- a/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > @@ -356,12 +356,48 @@ 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 lapss_pm_clk_destroy(void *data)
> > > +{
> > > +       pm_clk_destroy(data);
> > > +}
> >
> > Why are these helpers added again? And do we even need them? Can't we
> > just pass pm_runtime_disable or pm_clk_destroy to the
> > devm_add_action_or_reset() second parameter?
> 
> Unfortunately, we can't due to the C specification.  Take a look at
> all the other users of devm_add_action_or_reset() and they all have
> pretty much the same stupid thing. 

Ok, but we don't need two of the same functions, right?

> ...actually, do we even need the runtime_disable in the error path?
> When the dev goes away does it matter if you left pm_runtime enabled
> on it?
> 

I don't know. The device isn't destroyed but maybe when the driver is
unbound it resets the runtime PM counters?

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

* Re: [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices
  2020-10-14 23:00       ` Stephen Boyd
@ 2020-10-14 23:07         ` Doug Anderson
  2020-10-14 23:33           ` Stephen Boyd
  0 siblings, 1 reply; 11+ messages in thread
From: Doug Anderson @ 2020-10-14 23:07 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Taniya Das, ARM/QUALCOMM SUPPORT, David Brown, Rajendra Nayak,
	Andy Gross, Bjorn Andersson, Michael Turquette, linux-arm-msm,
	linux-clk, LKML

Hi,

On Wed, Oct 14, 2020 at 4:00 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Doug Anderson (2020-10-14 15:28:58)
> > Hi,
> >
> > On Wed, Oct 14, 2020 at 3:10 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Douglas Anderson (2020-10-14 14:05:22)
> > > > diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > index abcf36006926..48d370e2108e 100644
> > > > --- a/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > @@ -356,12 +356,48 @@ 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 lapss_pm_clk_destroy(void *data)
> > > > +{
> > > > +       pm_clk_destroy(data);
> > > > +}
> > >
> > > Why are these helpers added again? And do we even need them? Can't we
> > > just pass pm_runtime_disable or pm_clk_destroy to the
> > > devm_add_action_or_reset() second parameter?
> >
> > Unfortunately, we can't due to the C specification.  Take a look at
> > all the other users of devm_add_action_or_reset() and they all have
> > pretty much the same stupid thing.
>
> Ok, but we don't need two of the same functions, right?

How would you write it more cleanly?  I suppose I could allocate an
extra structure somewhere and put in a tuple of (function_pointer,
dev_pointer) there and pass that as the data.  Then I could do:

  struct fp_dp_tuple {
    void (*fn)(void *);
    struct device *dev;
  };

  struct fp_dp_tuple *tuple = data;
  tuple->fn(tuple->dev);

...but now I've got to create that tuple and stash it somewhere,
right?  ...or am I missing some super easy/obvious solution for how I
can know whether to call pm_runtime_disable() or pm_clk_destroy()?


> > ...actually, do we even need the runtime_disable in the error path?
> > When the dev goes away does it matter if you left pm_runtime enabled
> > on it?
> >
>
> I don't know. The device isn't destroyed but maybe when the driver is
> unbound it resets the runtime PM counters?

Certainly it seems safest just to do it...

-Doug

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

* Re: [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices
  2020-10-14 23:07         ` Doug Anderson
@ 2020-10-14 23:33           ` Stephen Boyd
  2020-10-15  0:10             ` Doug Anderson
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2020-10-14 23:33 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Taniya Das, ARM/QUALCOMM SUPPORT, David Brown, Rajendra Nayak,
	Andy Gross, Bjorn Andersson, Michael Turquette, linux-arm-msm,
	linux-clk, LKML

Quoting Doug Anderson (2020-10-14 16:07:52)
> Hi,
> 
> On Wed, Oct 14, 2020 at 4:00 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Doug Anderson (2020-10-14 15:28:58)
> > > Hi,
> > >
> > > On Wed, Oct 14, 2020 at 3:10 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Douglas Anderson (2020-10-14 14:05:22)
> > > > > diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > > index abcf36006926..48d370e2108e 100644
> > > > > --- a/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > > +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > > @@ -356,12 +356,48 @@ 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 lapss_pm_clk_destroy(void *data)
> > > > > +{
> > > > > +       pm_clk_destroy(data);
> > > > > +}
> > > >
> > > > Why are these helpers added again? And do we even need them? Can't we
> > > > just pass pm_runtime_disable or pm_clk_destroy to the
> > > > devm_add_action_or_reset() second parameter?
> > >
> > > Unfortunately, we can't due to the C specification.  Take a look at
> > > all the other users of devm_add_action_or_reset() and they all have
> > > pretty much the same stupid thing.
> >
> > Ok, but we don't need two of the same functions, right?
> 
> How would you write it more cleanly? 

Oh I see I'm making it confusing. Patch 1 has two functions for
pm_runtime_disable() and pm_clk_destroy(), called
lpass_pm_runtime_disable() and lapss_pm_clk_destroy() respectively
(please fix the lapss typo regardless).

Then this patch seems to introduce them again, but really the diff is
getting confused and it looks like the functions are introduced again.
Can you move them to this location (or at least near it) in the first
patch so that this doesn't look like they're being introduced again?

> > > ...actually, do we even need the runtime_disable in the error path?
> > > When the dev goes away does it matter if you left pm_runtime enabled
> > > on it?
> > >
> >
> > I don't know. The device isn't destroyed but maybe when the driver is
> > unbound it resets the runtime PM counters?
> 
> Certainly it seems safest just to do it...
> 

Can you confirm? I'd rather not carry extra code.

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

* Re: [PATCH v3 3/3] clk: qcom: lpasscc-sc7180: Re-configure the PLL in case lost
  2020-10-14 21:05 ` [PATCH v3 3/3] clk: qcom: lpasscc-sc7180: Re-configure the PLL in case lost Douglas Anderson
@ 2020-10-14 23:39   ` Stephen Boyd
  2020-10-15  0:10     ` Doug Anderson
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2020-10-14 23:39 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Taniya Das, linux-soc, David Brown, Rajendra Nayak,
	Douglas Anderson, Andy Gross, Bjorn Andersson, Michael Turquette,
	linux-arm-msm, linux-clk, linux-kernel

Quoting Douglas Anderson (2020-10-14 14:05:23)
> diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
> index 48d370e2108e..e12d4c2b1b70 100644
> --- a/drivers/clk/qcom/lpasscorecc-sc7180.c
> +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
> @@ -388,6 +388,25 @@ static int lpass_create_pm_clks(struct platform_device *pdev)
>         return ret;
>  }
>  
> +static int lpass_core_cc_pm_clk_resume(struct device *dev)
> +{
> +       struct regmap *regmap = dev_get_regmap(dev, "lpass_core_cc");

Please make "lpass_core_cc" a static const pointer in this driver so
that it can be used here and when the regmap is made so that we're
certain they match.

> +       unsigned int l_val;
> +       int ret;
> +
> +       ret = pm_clk_resume(dev);
> +       if (ret)
> +               return ret;
> +
> +       /* If PLL_L_VAL was cleared then we should re-init the whole PLL */
> +       regmap_read(regmap, 0x1004, &l_val);
> +       if (!l_val)
> +               clk_fabia_pll_configure(&lpass_lpaaudio_dig_pll, regmap,
> +                               &lpass_lpaaudio_dig_pll_config);
> +
> +       return 0;
> +}
> +
>  static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
>  {
>         const struct qcom_cc_desc *desc;

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

* Re: [PATCH v3 3/3] clk: qcom: lpasscc-sc7180: Re-configure the PLL in case lost
  2020-10-14 23:39   ` Stephen Boyd
@ 2020-10-15  0:10     ` Doug Anderson
  0 siblings, 0 replies; 11+ messages in thread
From: Doug Anderson @ 2020-10-15  0:10 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Taniya Das, open list:ARM/QUALCOMM SUPPORT, David Brown,
	Rajendra Nayak, Andy Gross, Bjorn Andersson, Michael Turquette,
	linux-arm-msm, linux-clk, LKML

Hi,

On Wed, Oct 14, 2020 at 4:39 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Douglas Anderson (2020-10-14 14:05:23)
> > diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > index 48d370e2108e..e12d4c2b1b70 100644
> > --- a/drivers/clk/qcom/lpasscorecc-sc7180.c
> > +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > @@ -388,6 +388,25 @@ static int lpass_create_pm_clks(struct platform_device *pdev)
> >         return ret;
> >  }
> >
> > +static int lpass_core_cc_pm_clk_resume(struct device *dev)
> > +{
> > +       struct regmap *regmap = dev_get_regmap(dev, "lpass_core_cc");
>
> Please make "lpass_core_cc" a static const pointer in this driver so
> that it can be used here and when the regmap is made so that we're
> certain they match.

Sure.  In theory the compiler ought to use string constant pooling so
they would have pointed to the same place, but you're right that it
wouldn't catch typos or other cases where they don't match.  I'll do
both regmap names just for symmetry.  Hopefully that's OK.

-Doug

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

* Re: [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices
  2020-10-14 23:33           ` Stephen Boyd
@ 2020-10-15  0:10             ` Doug Anderson
  0 siblings, 0 replies; 11+ messages in thread
From: Doug Anderson @ 2020-10-15  0:10 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Taniya Das, ARM/QUALCOMM SUPPORT, David Brown, Rajendra Nayak,
	Andy Gross, Bjorn Andersson, Michael Turquette, linux-arm-msm,
	linux-clk, LKML

Hi,

On Wed, Oct 14, 2020 at 4:33 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Doug Anderson (2020-10-14 16:07:52)
> > Hi,
> >
> > On Wed, Oct 14, 2020 at 4:00 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Doug Anderson (2020-10-14 15:28:58)
> > > > Hi,
> > > >
> > > > On Wed, Oct 14, 2020 at 3:10 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > >
> > > > > Quoting Douglas Anderson (2020-10-14 14:05:22)
> > > > > > diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > > > index abcf36006926..48d370e2108e 100644
> > > > > > --- a/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > > > +++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
> > > > > > @@ -356,12 +356,48 @@ 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 lapss_pm_clk_destroy(void *data)
> > > > > > +{
> > > > > > +       pm_clk_destroy(data);
> > > > > > +}
> > > > >
> > > > > Why are these helpers added again? And do we even need them? Can't we
> > > > > just pass pm_runtime_disable or pm_clk_destroy to the
> > > > > devm_add_action_or_reset() second parameter?
> > > >
> > > > Unfortunately, we can't due to the C specification.  Take a look at
> > > > all the other users of devm_add_action_or_reset() and they all have
> > > > pretty much the same stupid thing.
> > >
> > > Ok, but we don't need two of the same functions, right?
> >
> > How would you write it more cleanly?
>
> Oh I see I'm making it confusing. Patch 1 has two functions for
> pm_runtime_disable() and pm_clk_destroy(), called
> lpass_pm_runtime_disable() and lapss_pm_clk_destroy() respectively
> (please fix the lapss typo regardless).

Oops, sorry for the typo.


> Then this patch seems to introduce them again, but really the diff is
> getting confused and it looks like the functions are introduced again.
> Can you move them to this location (or at least near it) in the first
> patch so that this doesn't look like they're being introduced again?

Yeah.  v4 coming up soon then.


> > > > ...actually, do we even need the runtime_disable in the error path?
> > > > When the dev goes away does it matter if you left pm_runtime enabled
> > > > on it?
> > > >
> > >
> > > I don't know. The device isn't destroyed but maybe when the driver is
> > > unbound it resets the runtime PM counters?
> >
> > Certainly it seems safest just to do it...
> >
>
> Can you confirm? I'd rather not carry extra code.

Confirmed that we need it.  Specifically from
"Documentation/power/runtime_pm.rst"

> Drivers in ->remove() callback should undo the runtime PM changes done
> in ->probe(). Usually this means calling pm_runtime_disable(),
> pm_runtime_dont_use_autosuspend() etc.

It's my assertion that having it in devm is as good as having it in
the remove() callback because devres_release_all() follows the
remove() calls in base/dd.c

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

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 21:05 [PATCH v3 1/3] clk: qcom: lpasscc-sc7810: Use devm in probe Douglas Anderson
2020-10-14 21:05 ` [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices Douglas Anderson
2020-10-14 22:10   ` Stephen Boyd
2020-10-14 22:28     ` Doug Anderson
2020-10-14 23:00       ` Stephen Boyd
2020-10-14 23:07         ` Doug Anderson
2020-10-14 23:33           ` Stephen Boyd
2020-10-15  0:10             ` Doug Anderson
2020-10-14 21:05 ` [PATCH v3 3/3] clk: qcom: lpasscc-sc7180: Re-configure the PLL in case lost Douglas Anderson
2020-10-14 23:39   ` Stephen Boyd
2020-10-15  0:10     ` Doug Anderson

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git