[alsa-devel] [PATCH] ALSA: fm801: PCI core handles power state for us
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.
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, }, };
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...
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...?
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.
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
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.
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).
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.
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
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
On Wed, Jan 7, 2015 at 12:41 PM, Takashi Iwai tiwai@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@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?
I suspect that Documentation/power/pci.txt may explain all of this.
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.
Okay, let's remove the structure manipulation in v2.
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
At Wed, 7 Jan 2015 13:07:41 +0200, Andy Shevchenko wrote:
On Wed, Jan 7, 2015 at 12:41 PM, Takashi Iwai tiwai@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@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
participants (2)
-
Andy Shevchenko -
Takashi Iwai