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

[Takashi Iwai]

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 at 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
>