[alsa-devel] [PATCH] ALSA: fm801: PCI core handles power state for us

[Takashi Iwai]

At Wed, 7 Jan 2015 13:07:41 +0200,
Andy Shevchenko wrote:
> 
> On Wed, Jan 7, 2015 at 12:41 PM, Takashi Iwai <tiwai at suse.de> wrote:
> > At Wed, 7 Jan 2015 12:35:10 +0200,
> > Andy Shevchenko wrote:
> >>
> >> On Wed, Jan 7, 2015 at 8:54 AM, Takashi Iwai <tiwai at suse.de> wrote:
> >> > At Tue,  6 Jan 2015 22:39:15 +0200,
> >> > Andy Shevchenko wrote:
> >> >>
> >> >> There is no need to duplicate the work that is already done in the PCI
> >> >> driver core. The patch removes excerpts from suspend and resume
> >> >> callbacks.
> >> >>
> >> >> While here amend a definition of the snd_fm801_pm. The PM core has the
> >> >> stubs for non-PM_SLEEP cases, thus we can always define the variable.
> >> >
> >> > But this will result in a waste of bytes, no?  It's not a big deal,
> >> > but still...
> >>
> >> We can leave it under #ifdef, the main purpose of the patch to remove
> >> duplicate PM work.
> >
> > Yes.
> 
> Okay for patch v2.
> 
> >
> >> > above all, I couldn't find the code enabling/disabling pci device in
> >> > the default pci pm handlers.  pci_pm_default_resume_early() powers up
> >> > and restore the state.  These can be removed indeed.  But
> >> > pci_pm_restore() calls pci_pm_reenable_device() only as a fallback.
> >> > So it looks to me like the restore callback still needs to enable the
> >> > device explicitly.
> >> >
> >> > Am I missing anything obvious...?
> >>
> >> The suspend_noirq is called at the end of the suspend process. It gets
> >> device to the lower power state. I think this was implemented by
> >> 46939f8b15e44f065d052e89ea4f2adc81fdc740.
> >>
> >> There is a really nice documentation in dev_pm_ops description (linux/pm.h).
> >
> > Well, this does only power up/down and register save/restore.  Where
> > pci_disable_device() and pci_enable_device() (or their variants) are
> > called in the framework?
> 
> pci_pm_suspend_noirq() -> pci_prepare_to_sleep() ->
> pci_set_power_state() [pci_target_state()]
> So, this regarding to power state.
> 
> I doubt we have to do this during suspend/resume cycle. What is the point?

The calls are needed in the past, especially when the interrupt number
varies at resume (and thus re-acquiring the interrupt at resume).

I guess these calls are nowadays superfluous.  But, even if so, you
must describe it clearly.  It's no duplicated calls, after all, and
this has to be clarified in a more exact context.


Takashi