[alsa-devel] ASoC updates for v4.2

[Takashi Iwai]

At Mon, 22 Jun 2015 14:57:29 +0100,
Mark Brown wrote:
> 
> On Mon, Jun 22, 2015 at 01:24:02PM +0200, Takashi Iwai wrote:
> > At Mon, 22 Jun 2015 11:30:34 +0100,
> > Mark Brown wrote:
> > > On Mon, Jun 22, 2015 at 11:58:24AM +0200, Takashi Iwai wrote:
> 
> > > > And, looking at the code, it seems calling runtime suspend in the
> > > > following way at probe:
> 
> > > >         pm_runtime_enable(&pdev->dev);
> > > >         if (!pm_runtime_enabled(&pdev->dev)) {
> > > >                 ret = mtk_afe_runtime_resume(&pdev->dev);
> > > >                 if (ret)
> > > >                         goto err_pm_disable;
> > > >         }
> 
> > > I'm confused, where's the call to runtime suspend?
> 
> > It's in
> > static const struct dev_pm_ops mtk_afe_pm_ops = {
> > 	SET_RUNTIME_PM_OPS(mtk_afe_runtime_suspend, mtk_afe_runtime_resume,
> > 			   NULL)
> > };
> 
> Sorry, I'm still confused about what you're seeing in the probe - I know
> where the callbacks for runtime PM are registered but I'm not seeing a
> call to suspend (or something that I'd expect to trigger one) in the
> above?
 
There is no place calling runtime suspend manually, that's why the
compiler catches and warns.

> > But my concern above isn't about the warning itself.  I just stumbled
> > on the code invoking runtime resume while looking at this warning, and
> > wondered the behavior with CONFIG_PM=n.
> 
> > Usually this kind of warning could be simply fixed by adding a proper
> > ifdef.  But, this driver calls runtime resume in the probe manually.
> 
> Sure, that's a fairly common pattern though?

Depends.  The more common pattern seems to call pm_runtime_resume().
And this will skip the call of runtime PM when CONFIG_PM=n.

> > > > I'm not sure whether this really behaves correctly, especially when a
> > > > kernel is built without CONFIG_PM.
> 
> > > Could you be more specific about the problem you're seeing?  If runtime
> > > PM is disabled pm_runtime_enabled() will return false and we'll run
> > > through the resume path during probe() instead, otherwise we'll runtime
> > > resume whenever we need to use the hardware.
> 
> > The runtime suspend is never called when CONFIG_PM=n.  OTOH, we call
> > runtime resume *always* at probe when CONFIG_PM=n.  This looks
> > inconsistent to me.
> 
> Yeah, it should really resuspend the hardware in the remove path.

Right, then it'll be balanced properly.

> > If it's a part of the mandatory initialization, it should be named
> > explicitly so, and make the runtime resume callback just calls it
> > instead.
> 
> I disagree, I think either way is fine - if the clear intent and
> expectation is that the driver is used with runtime PM it seems fine to
> structure things to to say say "this is the special case path for !PM".
> I'd actually like to see this pattern better supported by the core so
> that drivers can enable runtime PM with calls that have !PM paths like
> in this driver, it'd make the whole !PM case a lot simpler.

I don't mind much in either way.  But the current code looks somehow
inconsistent.


Takashi