Re: [PATCH v2] ALSA: hda/tegra: enable clock during probe

[Sameer Pujar]

On 1/31/2019 5:40 PM, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@xxxxxxx> wrote:
> > On Thu, 31 Jan 2019 12:46:54 +0100,
> > Rafael J. Wysocki wrote:
> > > On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@xxxxxxx> wrote:
> > > > On Thu, 31 Jan 2019 12:05:30 +0100,
> > > > Thierry Reding wrote:
> > > > > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> > > [cut]
> > >
> > > > > > If I understand correctly the code, the pm domain is already activated
> > > > > > at calling driver's probe callback.
> > > > > As far as I can tell, the domain will also be powered off again after
> > > > > probe finished, unless the device grabs a runtime PM reference. This is
> > > > > what happens via the dev->pm_domain->sync() call after successful probe
> > > > > of a driver.
> > > > Ah, a good point.  This can be a problem with a probe work like this
> > > > case.

Are you suggesting, whether runtime PM is enabled/disabled, after 
successful probe the
domain would be powered off?
For CONFIG_PM enabled case, probe() can call get_sync() and put_sync() 
can be in probe_work.
How this needs to be handled for CONFG_PM disabled case? (just calling 
clock_enable() may
not be sufficient as per previous comments)

> > > > > It seems to me like it's not a very well defined case what to do when a
> > > > > device needs to be powered up but runtime PM is not enabled.
> > > > >
> > > > > Adding Rafael and linux-pm, maybe they can provide some guidance on what
> > > > > to do in these situations.
> > > > >
> > > > > To summarize, what we're debating here is how to handle powering up a
> > > > > device if the pm_runtime infrastructure doesn't take care of it. Jon's
> > > > > proposal here was, and we use this elsewhere, to do something like this:
> > > > >
> > > > >        pm_runtime_enable(dev);
> > > > >        if (!pm_runtime_enabled(dev)) {
> > > > >                err = foo_runtime_resume(dev);
> > > > >                if (err < 0)
> > > > >                        goto fail;
> > > > >        }
> > > > >
> > > > > So basically when runtime PM is not available, we explicitly "resume"
> > > > > the device to power it up.
> > > > >
> > > > > It seems to me like that's a fairly common problem, so I'm wondering if
> > > > > there's something that the runtime PM core could do to help with this.
> > > > > Or perhaps there's already a way to achieve this that we're all
> > > > > overlooking?
> > > > >
> > > > > Rafael, any suggestions?
> > > > If any, a common helper would be appreciated, indeed.
> > > I'm not sure that I understand the problem correctly, so let me
> > > restate it the way I understand it.
> > >
> > > What we're talking about is a driver ->probe() callback.  Runtime PM
> > > is disabled initially and the device is off.  It needs to be powered
> > > up, but the way to do that depends on some configuration of the board
> > > etc., so ideally
> > >
> > > pm_runtime_enable(dev);
> > > ret = pm_runtime_resume(dev);
> > >
> > > should just work, but the question is what to do if runtime PM doesn't
> > > work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
> > > else?
> > Yes, the question is how to write the code for both with and without
> > CONFIG_PM (or CONFIG_PM_RUNTIME).
> This basically is about setup, because after that point all should
> just work in both cases.
>
> Personally, I would do
>
> if (IS_ENABLED(CONFIG_PM)) {
>    do setup based on pm-runtime
> } else {
>    do manual setup
> }

do we really need config check here?
The debate was, whether to call hda_tegra_runtime_resume() or 
hda_tegra_enable_clocks() unconditionally here.
It would take care of both CONFIG_PM enabled/disabled cases. Then enable 
runtime PM.

> > Right now, we have a code like below, pushing the initialization in an
> > async work and let the probe returning quickly.
> >
> >          hda_tegra_probe() {
> >                  ....
> So why don't you do
>
> if (!IS_ENABLED(CONFIG_PM)) {
>    do manual clock setup
> }
>
> here?
>
> >                  pm_runtime_enable();
> >                  schedule_work();
> >                  return;
> >          }
> >
> >          hda_tegra_probe_work() {
> >                  pm_runtime_get_sync();
> >                  ....
> >                  pm_runtime_put_sync();
> >          }
> >
> > Then it truned outhis code lacks of the clock initialization when
> > runtime PM isn't enabled.  Normally it's done via runtime resume
> >
> >          hda_tegra_runtime_resume() {
> >                  hda_tegra_enable_clocks();
> >                  ....
> >          }
> >
> > And now the question is what is the standard idiom in such a case.
> >
> > IMO, calling pm_runtime_resume() inside the probe function looks
> > weird, and my preference was to initialize the clocks explicitly, then
> > enable runtime PM.  But if using pm_runtime_resume() in the proc
> > should be seen as a standard procedure, I'm fine with that.

I think reference here is, whether calling hda_tegra_runtime_resume() in 
probe() is
a standard procedure or not.

> Well, people do pm_runtime_resume() in ->probe() too, but
> pm_runtime_resume() returns 1 for CONFIG_PM unset, so that won't give
> you what you want anyway. :-)
>
> Cheers,
> Rafael