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

[Andy Shevchenko]

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?

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 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
>> >>
>>
>>
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>



-- 
With Best Regards,
Andy Shevchenko