At Wed, 7 Jan 2015 12:35:10 +0200, Andy Shevchenko wrote:
On Wed, Jan 7, 2015 at 8:54 AM, Takashi Iwai tiwai@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.
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?
In anyway, there are lots of similar codes in sound/pci/*, and I prefer patching all them at once. Of course the patches can be split if it's better.
Better to do this driver-by-driver. But I don't care about other sound drivers right now.
And I do care :) If applying this change, I'm going to put into a series. So please slim down to only the necessary part.
thanks,
Takashi
thanks,
Takashi
Signed-off-by: Andy Shevchenko andy.shevchenko@gmail.com
sound/pci/fm801.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c index 9a2122f..dcda3c1 100644 --- a/sound/pci/fm801.c +++ b/sound/pci/fm801.c @@ -1384,7 +1384,6 @@ static unsigned char saved_regs[] = {
static int snd_fm801_suspend(struct device *dev) {
struct pci_dev *pci = to_pci_dev(dev); struct snd_card *card = dev_get_drvdata(dev); struct fm801 *chip = card->private_data; int i;@@ -1396,29 +1395,15 @@ static int snd_fm801_suspend(struct device *dev) for (i = 0; i < ARRAY_SIZE(saved_regs); i++) chip->saved_regs[i] = inw(chip->port + saved_regs[i]); /* FIXME: tea575x suspend */
pci_disable_device(pci);pci_save_state(pci);pci_set_power_state(pci, PCI_D3hot); return 0;}
static int snd_fm801_resume(struct device *dev) {
struct pci_dev *pci = to_pci_dev(dev); struct snd_card *card = dev_get_drvdata(dev); struct fm801 *chip = card->private_data; int i;pci_set_power_state(pci, PCI_D0);pci_restore_state(pci);if (pci_enable_device(pci) < 0) {dev_err(dev, "pci_enable_device failed, disabling device\n");snd_card_disconnect(card);return -EIO;}pci_set_master(pci);snd_fm801_chip_init(chip, 1); snd_ac97_resume(chip->ac97); snd_ac97_resume(chip->ac97_sec);@@ -1428,12 +1413,9 @@ static int snd_fm801_resume(struct device *dev) snd_power_change_state(card, SNDRV_CTL_POWER_D0); return 0; } +#endif /* CONFIG_PM_SLEEP */
static SIMPLE_DEV_PM_OPS(snd_fm801_pm, snd_fm801_suspend, snd_fm801_resume); -#define SND_FM801_PM_OPS &snd_fm801_pm -#else -#define SND_FM801_PM_OPS NULL -#endif /* CONFIG_PM_SLEEP */
static struct pci_driver fm801_driver = { .name = KBUILD_MODNAME, @@ -1441,7 +1423,7 @@ static struct pci_driver fm801_driver = { .probe = snd_card_fm801_probe, .remove = snd_card_fm801_remove, .driver = {
.pm = SND_FM801_PM_OPS,
.pm = &snd_fm801_pm, },};
-- 1.8.3.101.g727a46b
-- With Best Regards, Andy Shevchenko