[alsa-devel] [PATCH 05/14] ALSA: pci: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

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);
> > >   	state = hda_call_codec_suspend(codec);
> > >   	if (codec->link_down_at_suspend ||
> > >   	    (codec_has_clkstop(codec) && codec_has_epss(codec) &&
> > 
> > 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,