Re: [PATCH v3 2/3] clk: qcom: lpass-sc7180: Disentangle the two clock devices

[Stephen Boyd]

Quoting Doug Anderson (2020-10-14 16:07:52)
> Hi,
> 
> On Wed, Oct 14, 2020 at 4:00 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> >
> > Quoting Doug Anderson (2020-10-14 15:28:58)
> > > Hi,
> > >
> > > On Wed, Oct 14, 2020 at 3:10 PM Stephen Boyd <sboyd@xxxxxxxxxx> 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.