On Tue, 15 Jan 2019 18:01:51 +0100, Takashi Iwai wrote:
On Tue, 15 Jan 2019 17:49:56 +0100, Pierre-Louis Bossart wrote:
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 9f8d59e7e89f..ff6dbed4d3cd 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2927,8 +2927,6 @@ static int hda_codec_runtime_suspend(struct device *dev) unsigned int state; cancel_delayed_work_sync(&codec->jackpoll_work);
- list_for_each_entry(pcm, &codec->pcm_list_head, list)
snd_pcm_suspend_all(pcm->pcm);question: since we now use HDAudio codecs in an ASoC-based implementation (both with the Skylake and SOF drivers), is this creating a case where suspend no longer works? I see no suspend support in sound/soc/codec/hdac_hda.c
As mentioned in patch#1, ASoC calls snd_pcm_suspend_all() in snd_soc_suspend(), which is the canonical place.
But, the suspend / resume for hdac-hda need revisited as well as hdac-hdmi, I suppose. They shouldn't use the device PM ops but just use the ASoC component codec / suspend callbacks. Some more plumbing might be required.
After further consideration, this seems solvable in a different way.
Basically, we can move some preamble code into the PM prepare callback so that it's executed before the suspend callback. For example, the patch below moves a few things into the prepare callback that was formerly done at the beginning of snd_soc_suspend(). This assures snd_pcm_suspend*() call processed beforehand.
And, another question is whether the snd_pcm_suspend*() call really has to be always after the digital_mute call in ASoC suspend procedure. If this can be done beforehand, we may leave snd_pcm_suspend*() call in the PCM device PM ops like others, while doing the digital_mute & else preamble in the prepare callback. The PCM device PM is called before the machine driver's device PM due to the dependency (the PCM device is always created after the card device). This will make things easier again, we can get rid of the ugly flag in the patch set.
BTW, while checking these things, I noticed that there are three exceptional drivers: sound/soc/intel/atom/sst-mfld-platform-pcm.c, sound/soc/intel/haswell/sst-haswell-pcm.c, and sound/soc/samsung/tm2_wm5110.c. (You can simply grep the external snd_soc_suspend call, and only these match.)
The former two drivers look really weird: they do handle the PM only with PM prepare and complete callbacks, while snd_soc_suspend() and co are called internally from there. The prepare and complete callbacks aren't designed for the complete suspend/resume tasks, so I'd say it's a quite abuse.
The last one has prepare and complete callbacks in addition to the other standard PM calls. And tm2_pm_preapre() stops sysclk and complete() starts sysclk. I don't understand why these are needed in prepare and resume. Can anyone explain?
thanks,
Takashi
---
diff --git a/include/sound/soc.h b/include/sound/soc.h --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -432,9 +432,15 @@ int snd_soc_register_card(struct snd_soc_card *card); int snd_soc_unregister_card(struct snd_soc_card *card); int devm_snd_soc_register_card(struct device *dev, struct snd_soc_card *card); #ifdef CONFIG_PM_SLEEP +int snd_soc_pm_prepare(struct device *dev); int snd_soc_suspend(struct device *dev); int snd_soc_resume(struct device *dev); #else +static inline int snd_soc_pm_prepare(struct device *dev) +{ + return 0; +} + static inline int snd_soc_suspend(struct device *dev) { return 0; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -436,13 +436,11 @@ static void codec2codec_close_delayed_work(struct work_struct *work) }
#ifdef CONFIG_PM_SLEEP -/* powers down audio subsystem for suspend */ -int snd_soc_suspend(struct device *dev) +/* prepare for suspend */ +int snd_soc_pm_prepare(struct device *dev) { struct snd_soc_card *card = dev_get_drvdata(dev); - struct snd_soc_component *component; struct snd_soc_pcm_runtime *rtd; - int i;
/* If the card is not initialized yet there is nothing to do */ if (!card->instantiated) @@ -480,6 +478,22 @@ int snd_soc_suspend(struct device *dev) snd_pcm_suspend_all(rtd->pcm); }
+ return 0; +} +EXPORT_SYMBOL_GPL(snd_soc_pm_prepare); + +/* powers down audio subsystem for suspend */ +int snd_soc_suspend(struct device *dev) +{ + struct snd_soc_card *card = dev_get_drvdata(dev); + struct snd_soc_component *component; + struct snd_soc_pcm_runtime *rtd; + int i; + + /* If the card is not initialized yet there is nothing to do */ + if (!card->instantiated) + return 0; + if (card->suspend_pre) card->suspend_pre(card);
@@ -2253,6 +2267,7 @@ int snd_soc_poweroff(struct device *dev) EXPORT_SYMBOL_GPL(snd_soc_poweroff);
const struct dev_pm_ops snd_soc_pm_ops = { + .prepare = snd_soc_pm_prepare, .suspend = snd_soc_suspend, .resume = snd_soc_resume, .freeze = snd_soc_suspend,