| Message ID | 20201014140507.v3.1.I4567b5e7e17bbb15ef063d447cb83fd43746cb18@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] clk: qcom: lpasscc-sc7810: Use devm in probe | expand |
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?
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
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.
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;
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
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
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 = {
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(-)