[alsa-devel] ASoC updates for v4.2

[Koro Chen]

On Mon, 2015-06-22 at 16:10 +0200, Takashi Iwai wrote:
> 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.
So I think maybe it should be modified like this?

 static int mtk_afe_pcm_dev_remove(struct platform_device *pdev)
 {
        pm_runtime_disable(&pdev->dev);
+       if (!pm_runtime_status_suspended(&pdev->dev))
+               mtk_afe_runtime_suspend(&pdev->dev);
        snd_soc_unregister_component(&pdev->dev);
        snd_soc_unregister_platform(&pdev->dev);
        return 0;

It can fix both build warning and unbalanced calls of suspend/resume.
Should I send a patch for this?

> 
> > > 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
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel