[PATCH 00/14] ALSA: PCM suspend cleanup

[PATCH 00/14] ALSA: PCM suspend cleanup

[Takashi Iwai]

Hi,

this is a patch set to clean up the PCM suspend calls by introducing
the PCM device PM ops.  This won't change much for ASoC but reduce
lots of snd_pcm_suspend*() calls in other sound drivers.

Note that this patch series itself won't fix anything about the recent
PM issue reported for Intel ASoC, but it was inspired through the
discussion there.


thanks,

Takashi

===

Takashi Iwai (14):
  ALSA: pcm: Suspend streams globally via device type PM ops
  ALSA: atiixp: Move PCM suspend/resume code into trigger callback
  ALSA: isa: Remove superfluous snd_pcm_suspend*() calls
  ALSA: drivers: Remove superfluous snd_pcm_suspend*() calls
  ALSA: pci: Remove superfluous snd_pcm_suspend*() calls
  ALSA: usb: Remove superfluous snd_pcm_suspend*() calls
  ALSA: x86: Remove superfluous snd_pcm_suspend*() calls
  ALSA: ppc: Remove superfluous snd_pcm_suspend*() calls
  ALSA: aoa: Remove superfluous snd_pcm_suspend*() calls
  ALSA: arm: Remove superfluous snd_pcm_suspend*() calls
  ALSA: pcmcia: Remove superfluous snd_pcm_suspend*() calls
  drm: bridge: dw-hdmi: Remove superfluous snd_pcm_suspend*() calls
  ALSA: doc: Update the description about PCM suspend procedure
  ALSA: pcm: Make snd_pcm_suspend() local static

 .../kernel-api/writing-an-alsa-driver.rst     | 25 ++++++------------
 .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c   |  1 -
 include/sound/pcm.h                           |  6 +----
 sound/aoa/soundbus/i2sbus/core.c              |  4 ---
 sound/arm/aaci.c                              |  1 -
 sound/arm/pxa2xx-ac97.c                       |  1 -
 sound/core/pcm.c                              | 26 +++++++++++++++++++
 sound/core/pcm_native.c                       | 11 +++-----
 sound/drivers/aloop.c                         |  4 ---
 sound/drivers/dummy.c                         |  2 --
 sound/drivers/pcsp/pcsp.c                     |  1 -
 sound/drivers/vx/vx_core.c                    |  4 ---
 sound/isa/ad1816a/ad1816a_lib.c               |  1 -
 sound/isa/als100.c                            |  1 -
 sound/isa/cmi8328.c                           |  1 -
 sound/isa/cmi8330.c                           |  1 -
 sound/isa/es1688/es1688.c                     |  2 --
 sound/isa/es18xx.c                            |  2 --
 sound/isa/sb/jazz16.c                         |  1 -
 sound/isa/sb/sb16.c                           |  1 -
 sound/isa/sb/sb8.c                            |  1 -
 sound/isa/wss/wss_lib.c                       |  1 -
 sound/pci/ali5451/ali5451.c                   |  4 +--
 sound/pci/als300.c                            |  1 -
 sound/pci/als4000.c                           |  1 -
 sound/pci/atiixp.c                            | 19 ++++++--------
 sound/pci/atiixp_modem.c                      |  2 --
 sound/pci/azt3328.c                           |  4 ---
 sound/pci/ca0106/ca0106_main.c                |  3 ---
 sound/pci/cmipci.c                            |  4 ---
 sound/pci/cs4281.c                            |  2 --
 sound/pci/cs46xx/cs46xx_lib.c                 |  6 -----
 sound/pci/cs5535audio/cs5535audio_pm.c        |  1 -
 sound/pci/ctxfi/ctatc.c                       |  8 ------
 sound/pci/echoaudio/echoaudio.c               |  3 ---
 sound/pci/emu10k1/emu10k1.c                   |  6 -----
 sound/pci/ens1370.c                           |  3 ---
 sound/pci/es1938.c                            |  1 -
 sound/pci/es1968.c                            |  1 -
 sound/pci/fm801.c                             |  1 -
 sound/pci/hda/hda_codec.c                     |  2 --
 sound/pci/ice1712/ice1712.c                   |  3 ---
 sound/pci/ice1712/ice1724.c                   |  3 ---
 sound/pci/intel8x0.c                          |  2 --
 sound/pci/intel8x0m.c                         |  3 ---
 sound/pci/maestro3.c                          |  1 -
 sound/pci/nm256/nm256.c                       |  1 -
 sound/pci/oxygen/oxygen_lib.c                 |  5 +---
 sound/pci/riptide/riptide.c                   |  1 -
 sound/pci/rme96.c                             |  2 --
 sound/pci/sis7019.c                           |  1 -
 sound/pci/trident/trident_main.c              |  4 ---
 sound/pci/via82xx.c                           |  2 --
 sound/pci/via82xx_modem.c                     |  2 --
 sound/pci/ymfpci/ymfpci_main.c                |  4 ---
 sound/pcmcia/pdaudiocf/pdaudiocf_core.c       |  1 -
 sound/ppc/pmac.c                              |  1 -
 sound/soc/soc-pcm.c                           |  1 +
 sound/usb/card.c                              |  1 -
 sound/usb/line6/driver.c                      |  4 +--
 sound/x86/intel_hdmi_audio.c                  | 12 ---------
 61 files changed, 50 insertions(+), 174 deletions(-)

-- 
2.20.1

[PATCH 01/14] ALSA: pcm: Suspend streams globally via device type PM ops

[Takashi Iwai]

Until now we rely on each driver calling snd_pcm_suspend*() explicitly
at its own PM handling.  However, this can be done far more easily by
setting the PM ops to each actual snd_pcm device object.

This patch adds the device_type object for PCM stream and assigns to
each PCM stream object.  The type contains only the PM ops for system
suspend; we don't need to deal with the resume in general.

The suspend hook simply calls snd_pcm_suspend_all() for the given PCM
streams.  This implies that the PM order is correctly put, i.e. PCM is
suspended before the main (or codec) driver, which should be true in
general.  If a special ordering is needed, you'd need to adjust the
device PM order manually later.

This patch introduces a new flag, snd_pcm.no_device_suspend, too.
With this flag set, the PCM device object won't invoke
snd_pcm_suspend_all() by itself.  This is needed for ASoC who wants to
manage the PM call orders in its serialized way, and the flag is set
in soc_new_pcm() as default.

For the non-ASoC world, we can get rid of the manual snd_pcm_suspend
calls.  This will be done in the later patches.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h |  1 +
 sound/core/pcm.c    | 26 ++++++++++++++++++++++++++
 sound/soc/soc-pcm.c |  1 +
 3 files changed, 28 insertions(+)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index d6bd3caf6878..04e97564949c 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -538,6 +538,7 @@ struct snd_pcm {
 	void (*private_free) (struct snd_pcm *pcm);
 	bool internal; /* pcm is for internal use only */
 	bool nonatomic; /* whole PCM operations are in non-atomic context */
+	bool no_device_suspend; /* don't invoke device PM suspend */
 #if IS_ENABLED(CONFIG_SND_PCM_OSS)
 	struct snd_pcm_oss oss;
 #endif
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 01b9d62eef14..ca1ea3cf9350 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -683,6 +683,31 @@ static inline int snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea
 
 static const struct attribute_group *pcm_dev_attr_groups[];
 
+/*
+ * PM callbacks: we need to deal only with suspend here, as the resume is
+ * triggered either from user-space or the driver's resume callback
+ */
+#ifdef CONFIG_PM_SLEEP
+static int do_pcm_suspend(struct device *dev)
+{
+	struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev);
+
+	if (!pstr->pcm->no_device_suspend)
+		snd_pcm_suspend_all(pstr->pcm);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops pcm_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL)
+};
+
+/* device type for PCM -- basically only for passing PM callbacks */
+static const struct device_type pcm_dev_type = {
+	.name = "pcm",
+	.pm = &pcm_dev_pm_ops,
+};
+
 /**
  * snd_pcm_new_stream - create a new PCM stream
  * @pcm: the pcm instance
@@ -713,6 +738,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
 
 	snd_device_initialize(&pstr->dev, pcm->card);
 	pstr->dev.groups = pcm_dev_attr_groups;
+	pstr->dev.type = &pcm_dev_type;
 	dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device,
 		     stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
 
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 03f36e534050..485eec5be608 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -3155,6 +3155,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	}
 
 	pcm->private_free = soc_pcm_private_free;
+	pcm->no_device_suspend = true;
 out:
 	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
 		 (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name,
-- 
2.20.1

[PATCH 02/14] ALSA: atiixp: Move PCM suspend/resume code into trigger callback

[Takashi Iwai]

ATIIXP driver supports the full PCM resume and saves/restores the
running PCM pointer.  This used to be done in the suspend and resume
callbacks together with snd_pcm_suspend() call.  But since we moved
the snd_pcm_supsend*() call in PCM device PM ops, this should be moved
to a more appropriate place, i.e. the trigger callback.

Along with the movement of the PCM suspend/resume code, remove the
superfluous snd_pcm_suspend_all() call, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/atiixp.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/sound/pci/atiixp.c b/sound/pci/atiixp.c
index 1a41f8c80243..7715d26916ac 100644
--- a/sound/pci/atiixp.c
+++ b/sound/pci/atiixp.c
@@ -733,6 +733,10 @@ static int snd_atiixp_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 	case SNDRV_PCM_TRIGGER_RESUME:
+		if (dma->running && dma->suspended &&
+		    cmd == SNDRV_PCM_TRIGGER_RESUME)
+			writel(dma->saved_curptr, chip->remap_addr +
+			       dma->ops->dt_cur);
 		dma->ops->enable_transfer(chip, 1);
 		dma->running = 1;
 		dma->suspended = 0;
@@ -740,9 +744,12 @@ static int snd_atiixp_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
+		dma->suspended = cmd == SNDRV_PCM_TRIGGER_SUSPEND;
+		if (dma->running && dma->suspended)
+			dma->saved_curptr = readl(chip->remap_addr +
+						  dma->ops->dt_cur);
 		dma->ops->enable_transfer(chip, 0);
 		dma->running = 0;
-		dma->suspended = cmd == SNDRV_PCM_TRIGGER_SUSPEND;
 		break;
 	default:
 		err = -EINVAL;
@@ -1479,14 +1486,6 @@ static int snd_atiixp_suspend(struct device *dev)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	for (i = 0; i < NUM_ATI_PCMDEVS; i++)
-		if (chip->pcmdevs[i]) {
-			struct atiixp_dma *dma = &chip->dmas[i];
-			if (dma->substream && dma->running)
-				dma->saved_curptr = readl(chip->remap_addr +
-							  dma->ops->dt_cur);
-			snd_pcm_suspend_all(chip->pcmdevs[i]);
-		}
 	for (i = 0; i < NUM_ATI_CODECS; i++)
 		snd_ac97_suspend(chip->ac97[i]);
 	snd_atiixp_aclink_down(chip);
@@ -1514,8 +1513,6 @@ static int snd_atiixp_resume(struct device *dev)
 				dma->substream->ops->prepare(dma->substream);
 				writel((u32)dma->desc_buf.addr | ATI_REG_LINKPTR_EN,
 				       chip->remap_addr + dma->ops->llp_offset);
-				writel(dma->saved_curptr, chip->remap_addr +
-				       dma->ops->dt_cur);
 			}
 		}
 
-- 
2.20.1

[PATCH 03/14] ALSA: isa: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/isa/ad1816a/ad1816a_lib.c | 1 -
 sound/isa/als100.c              | 1 -
 sound/isa/cmi8328.c             | 1 -
 sound/isa/cmi8330.c             | 1 -
 sound/isa/es1688/es1688.c       | 2 --
 sound/isa/es18xx.c              | 2 --
 sound/isa/sb/jazz16.c           | 1 -
 sound/isa/sb/sb16.c             | 1 -
 sound/isa/sb/sb8.c              | 1 -
 sound/isa/wss/wss_lib.c         | 1 -
 10 files changed, 12 deletions(-)

diff --git a/sound/isa/ad1816a/ad1816a_lib.c b/sound/isa/ad1816a/ad1816a_lib.c
index fba6d22f7f4b..61e8c7e524db 100644
--- a/sound/isa/ad1816a/ad1816a_lib.c
+++ b/sound/isa/ad1816a/ad1816a_lib.c
@@ -518,7 +518,6 @@ void snd_ad1816a_suspend(struct snd_ad1816a *chip)
 	int reg;
 	unsigned long flags;
 
-	snd_pcm_suspend_all(chip->pcm);
 	spin_lock_irqsave(&chip->lock, flags);
 	for (reg = 0; reg < 48; reg++)
 		chip->image[reg] = snd_ad1816a_read(chip, reg);
diff --git a/sound/isa/als100.c b/sound/isa/als100.c
index f63142ec287e..571108021e9d 100644
--- a/sound/isa/als100.c
+++ b/sound/isa/als100.c
@@ -322,7 +322,6 @@ static int snd_als100_pnp_suspend(struct pnp_card_link *pcard, pm_message_t stat
 	struct snd_sb *chip = acard->chip;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_sbmixer_suspend(chip);
 	return 0;
 }
diff --git a/sound/isa/cmi8328.c b/sound/isa/cmi8328.c
index de6ef1b1cf0e..617977516201 100644
--- a/sound/isa/cmi8328.c
+++ b/sound/isa/cmi8328.c
@@ -434,7 +434,6 @@ static int snd_cmi8328_suspend(struct device *pdev, unsigned int n,
 	cmi = card->private_data;
 	snd_cmi8328_cfg_save(cmi->port, cmi->cfg);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(cmi->wss->pcm);
 	cmi->wss->suspend(cmi->wss);
 
 	return 0;
diff --git a/sound/isa/cmi8330.c b/sound/isa/cmi8330.c
index 6b8c46942efb..7e5aa06414c4 100644
--- a/sound/isa/cmi8330.c
+++ b/sound/isa/cmi8330.c
@@ -484,7 +484,6 @@ static int snd_cmi8330_suspend(struct snd_card *card)
 	struct snd_cmi8330 *acard = card->private_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(acard->pcm);
 	acard->wss->suspend(acard->wss);
 	snd_sbmixer_suspend(acard->sb);
 	return 0;
diff --git a/sound/isa/es1688/es1688.c b/sound/isa/es1688/es1688.c
index 3dfe7e592c25..87527627e059 100644
--- a/sound/isa/es1688/es1688.c
+++ b/sound/isa/es1688/es1688.c
@@ -301,10 +301,8 @@ static int snd_es968_pnp_suspend(struct pnp_card_link *pcard,
 				 pm_message_t state)
 {
 	struct snd_card *card = pnp_get_card_drvdata(pcard);
-	struct snd_es1688 *chip = card->private_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	return 0;
 }
 
diff --git a/sound/isa/es18xx.c b/sound/isa/es18xx.c
index 0d103d6f805e..77aa9a27fb3b 100644
--- a/sound/isa/es18xx.c
+++ b/sound/isa/es18xx.c
@@ -1731,8 +1731,6 @@ static int snd_es18xx_suspend(struct snd_card *card, pm_message_t state)
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 
-	snd_pcm_suspend_all(chip->pcm);
-
 	/* power down */
 	chip->pm_reg = (unsigned char)snd_es18xx_read(chip, ES18XX_PM);
 	chip->pm_reg |= (ES18XX_PM_FM | ES18XX_PM_SUS);
diff --git a/sound/isa/sb/jazz16.c b/sound/isa/sb/jazz16.c
index bfa0055e1fd6..7a313ff589c7 100644
--- a/sound/isa/sb/jazz16.c
+++ b/sound/isa/sb/jazz16.c
@@ -356,7 +356,6 @@ static int snd_jazz16_suspend(struct device *pdev, unsigned int n,
 	struct snd_sb *chip = acard->chip;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_sbmixer_suspend(chip);
 	return 0;
 }
diff --git a/sound/isa/sb/sb16.c b/sound/isa/sb/sb16.c
index 8f9ebeb998f6..3844d4c02f49 100644
--- a/sound/isa/sb/sb16.c
+++ b/sound/isa/sb/sb16.c
@@ -471,7 +471,6 @@ static int snd_sb16_suspend(struct snd_card *card, pm_message_t state)
 	struct snd_sb *chip = acard->chip;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_sbmixer_suspend(chip);
 	return 0;
 }
diff --git a/sound/isa/sb/sb8.c b/sound/isa/sb/sb8.c
index d77dcba276b5..aa2a83eb81a9 100644
--- a/sound/isa/sb/sb8.c
+++ b/sound/isa/sb/sb8.c
@@ -218,7 +218,6 @@ static int snd_sb8_suspend(struct device *dev, unsigned int n,
 	struct snd_sb *chip = acard->chip;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_sbmixer_suspend(chip);
 	return 0;
 }
diff --git a/sound/isa/wss/wss_lib.c b/sound/isa/wss/wss_lib.c
index 3a5008837576..b11ef97bce1b 100644
--- a/sound/isa/wss/wss_lib.c
+++ b/sound/isa/wss/wss_lib.c
@@ -1625,7 +1625,6 @@ static void snd_wss_suspend(struct snd_wss *chip)
 	int reg;
 	unsigned long flags;
 
-	snd_pcm_suspend_all(chip->pcm);
 	spin_lock_irqsave(&chip->reg_lock, flags);
 	for (reg = 0; reg < 32; reg++)
 		chip->image[reg] = snd_wss_in(chip, reg);
-- 
2.20.1

[PATCH 04/14] ALSA: drivers: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/drivers/aloop.c      | 4 ----
 sound/drivers/dummy.c      | 2 --
 sound/drivers/pcsp/pcsp.c  | 1 -
 sound/drivers/vx/vx_core.c | 4 ----
 4 files changed, 11 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 1e34e6381baa..65c903b639c2 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -1200,12 +1200,8 @@ static int loopback_remove(struct platform_device *devptr)
 static int loopback_suspend(struct device *pdev)
 {
 	struct snd_card *card = dev_get_drvdata(pdev);
-	struct loopback *loopback = card->private_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-
-	snd_pcm_suspend_all(loopback->pcm[0]);
-	snd_pcm_suspend_all(loopback->pcm[1]);
 	return 0;
 }
 	
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 9af154db530a..c8d31550e9a1 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -1138,10 +1138,8 @@ static int snd_dummy_remove(struct platform_device *devptr)
 static int snd_dummy_suspend(struct device *pdev)
 {
 	struct snd_card *card = dev_get_drvdata(pdev);
-	struct snd_dummy *dummy = card->private_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(dummy->pcm);
 	return 0;
 }
 	
diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c
index 0dd3f46eb03e..d83ad3820f02 100644
--- a/sound/drivers/pcsp/pcsp.c
+++ b/sound/drivers/pcsp/pcsp.c
@@ -197,7 +197,6 @@ static int pcsp_suspend(struct device *dev)
 {
 	struct snd_pcsp *chip = dev_get_drvdata(dev);
 	pcsp_stop_beep(chip);
-	snd_pcm_suspend_all(chip->pcm);
 	return 0;
 }
 
diff --git a/sound/drivers/vx/vx_core.c b/sound/drivers/vx/vx_core.c
index 04368dd59a4c..19496fa486aa 100644
--- a/sound/drivers/vx/vx_core.c
+++ b/sound/drivers/vx/vx_core.c
@@ -732,12 +732,8 @@ EXPORT_SYMBOL(snd_vx_dsp_load);
  */
 int snd_vx_suspend(struct vx_core *chip)
 {
-	unsigned int i;
-
 	snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot);
 	chip->chip_status |= VX_STAT_IN_SUSPEND;
-	for (i = 0; i < chip->hw->num_codecs; i++)
-		snd_pcm_suspend_all(chip->pcm[i]);
 
 	return 0;
 }
-- 
2.20.1

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

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/ali5451/ali5451.c            | 4 +---
 sound/pci/als300.c                     | 1 -
 sound/pci/als4000.c                    | 1 -
 sound/pci/atiixp_modem.c               | 2 --
 sound/pci/azt3328.c                    | 4 ----
 sound/pci/ca0106/ca0106_main.c         | 3 ---
 sound/pci/cmipci.c                     | 4 ----
 sound/pci/cs4281.c                     | 2 --
 sound/pci/cs46xx/cs46xx_lib.c          | 6 ------
 sound/pci/cs5535audio/cs5535audio_pm.c | 1 -
 sound/pci/ctxfi/ctatc.c                | 8 --------
 sound/pci/echoaudio/echoaudio.c        | 3 ---
 sound/pci/emu10k1/emu10k1.c            | 6 ------
 sound/pci/ens1370.c                    | 3 ---
 sound/pci/es1938.c                     | 1 -
 sound/pci/es1968.c                     | 1 -
 sound/pci/fm801.c                      | 1 -
 sound/pci/hda/hda_codec.c              | 2 --
 sound/pci/ice1712/ice1712.c            | 3 ---
 sound/pci/ice1712/ice1724.c            | 3 ---
 sound/pci/intel8x0.c                   | 2 --
 sound/pci/intel8x0m.c                  | 3 ---
 sound/pci/maestro3.c                   | 1 -
 sound/pci/nm256/nm256.c                | 1 -
 sound/pci/oxygen/oxygen_lib.c          | 5 +----
 sound/pci/riptide/riptide.c            | 1 -
 sound/pci/rme96.c                      | 2 --
 sound/pci/sis7019.c                    | 1 -
 sound/pci/trident/trident_main.c       | 4 ----
 sound/pci/via82xx.c                    | 2 --
 sound/pci/via82xx_modem.c              | 2 --
 sound/pci/ymfpci/ymfpci_main.c         | 4 ----
 32 files changed, 2 insertions(+), 85 deletions(-)

diff --git a/sound/pci/ali5451/ali5451.c b/sound/pci/ali5451/ali5451.c
index 9f569379b77e..e781ccca1793 100644
--- a/sound/pci/ali5451/ali5451.c
+++ b/sound/pci/ali5451/ali5451.c
@@ -1882,10 +1882,8 @@ static int ali_suspend(struct device *dev)
 		return 0;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	for (i = 0; i < chip->num_of_codecs; i++) {
-		snd_pcm_suspend_all(chip->pcm[i]);
+	for (i = 0; i < chip->num_of_codecs; i++)
 		snd_ac97_suspend(chip->ac97[i]);
-	}
 
 	spin_lock_irq(&chip->reg_lock);
 	
diff --git a/sound/pci/als300.c b/sound/pci/als300.c
index eaa2d853d922..516b3d9cbfdf 100644
--- a/sound/pci/als300.c
+++ b/sound/pci/als300.c
@@ -731,7 +731,6 @@ static int snd_als300_suspend(struct device *dev)
 	struct snd_als300 *chip = card->private_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_ac97_suspend(chip->ac97);
 	return 0;
 }
diff --git a/sound/pci/als4000.c b/sound/pci/als4000.c
index 26b097edec8c..45fa38382e79 100644
--- a/sound/pci/als4000.c
+++ b/sound/pci/als4000.c
@@ -994,7 +994,6 @@ static int snd_als4000_suspend(struct device *dev)
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 	
-	snd_pcm_suspend_all(chip->pcm);
 	snd_sbmixer_suspend(chip);
 	return 0;
 }
diff --git a/sound/pci/atiixp_modem.c b/sound/pci/atiixp_modem.c
index dc1de860cedf..a357a8e2e73d 100644
--- a/sound/pci/atiixp_modem.c
+++ b/sound/pci/atiixp_modem.c
@@ -1125,8 +1125,6 @@ static int snd_atiixp_suspend(struct device *dev)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	for (i = 0; i < NUM_ATI_PCMDEVS; i++)
-		snd_pcm_suspend_all(chip->pcmdevs[i]);
 	for (i = 0; i < NUM_ATI_CODECS; i++)
 		snd_ac97_suspend(chip->ac97[i]);
 	snd_atiixp_aclink_down(chip);
diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c
index fc18c29a8173..90348817f096 100644
--- a/sound/pci/azt3328.c
+++ b/sound/pci/azt3328.c
@@ -2699,10 +2699,6 @@ snd_azf3328_suspend(struct device *dev)
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 
-	/* same pcm object for playback/capture */
-	snd_pcm_suspend_all(chip->pcm[AZF_CODEC_PLAYBACK]);
-	snd_pcm_suspend_all(chip->pcm[AZF_CODEC_I2S_OUT]);
-
 	snd_azf3328_suspend_ac97(chip);
 
 	snd_azf3328_suspend_regs(chip, chip->ctrl_io,
diff --git a/sound/pci/ca0106/ca0106_main.c b/sound/pci/ca0106/ca0106_main.c
index cd27b5536654..3d1b0bbff33b 100644
--- a/sound/pci/ca0106/ca0106_main.c
+++ b/sound/pci/ca0106/ca0106_main.c
@@ -1910,11 +1910,8 @@ static int snd_ca0106_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct snd_ca0106 *chip = card->private_data;
-	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	for (i = 0; i < 4; i++)
-		snd_pcm_suspend_all(chip->pcm[i]);
 	if (chip->details->ac97)
 		snd_ac97_suspend(chip->ac97);
 	snd_ca0106_mixer_suspend(chip);
diff --git a/sound/pci/cmipci.c b/sound/pci/cmipci.c
index 452cc79b44af..5bbf31c1695c 100644
--- a/sound/pci/cmipci.c
+++ b/sound/pci/cmipci.c
@@ -3351,10 +3351,6 @@ static int snd_cmipci_suspend(struct device *dev)
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 	
-	snd_pcm_suspend_all(cm->pcm);
-	snd_pcm_suspend_all(cm->pcm2);
-	snd_pcm_suspend_all(cm->pcm_spdif);
-
 	/* save registers */
 	for (i = 0; i < ARRAY_SIZE(saved_regs); i++)
 		cm->saved_regs[i] = snd_cmipci_read(cm, saved_regs[i]);
diff --git a/sound/pci/cs4281.c b/sound/pci/cs4281.c
index ec4247638fa1..a9fb819cad1d 100644
--- a/sound/pci/cs4281.c
+++ b/sound/pci/cs4281.c
@@ -2002,8 +2002,6 @@ static int cs4281_suspend(struct device *dev)
 	unsigned int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
-
 	snd_ac97_suspend(chip->ac97);
 	snd_ac97_suspend(chip->ac97_secondary);
 
diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
index 750eec437a79..a77d4cc44028 100644
--- a/sound/pci/cs46xx/cs46xx_lib.c
+++ b/sound/pci/cs46xx/cs46xx_lib.c
@@ -3781,12 +3781,6 @@ static int snd_cs46xx_suspend(struct device *dev)
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 	chip->in_suspend = 1;
-	snd_pcm_suspend_all(chip->pcm);
-#ifdef CONFIG_SND_CS46XX_NEW_DSP
-	snd_pcm_suspend_all(chip->pcm_rear);
-	snd_pcm_suspend_all(chip->pcm_center_lfe);
-	snd_pcm_suspend_all(chip->pcm_iec958);
-#endif
 	// chip->ac97_powerdown = snd_cs46xx_codec_read(chip, AC97_POWER_CONTROL);
 	// chip->ac97_general_purpose = snd_cs46xx_codec_read(chip, BA0_AC97_GENERAL_PURPOSE);
 
diff --git a/sound/pci/cs5535audio/cs5535audio_pm.c b/sound/pci/cs5535audio/cs5535audio_pm.c
index 82bd10b68a77..446ef1f1b45a 100644
--- a/sound/pci/cs5535audio/cs5535audio_pm.c
+++ b/sound/pci/cs5535audio/cs5535audio_pm.c
@@ -62,7 +62,6 @@ static int __maybe_unused snd_cs5535audio_suspend(struct device *dev)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(cs5535au->pcm);
 	snd_ac97_suspend(cs5535au->ac97);
 	for (i = 0; i < NUM_CS5535AUDIO_DMAS; i++) {
 		struct cs5535audio_dma *dma = &cs5535au->dmas[i];
diff --git a/sound/pci/ctxfi/ctatc.c b/sound/pci/ctxfi/ctatc.c
index 2ada8444abd9..e622613ea947 100644
--- a/sound/pci/ctxfi/ctatc.c
+++ b/sound/pci/ctxfi/ctatc.c
@@ -1548,18 +1548,10 @@ static void atc_connect_resources(struct ct_atc *atc)
 #ifdef CONFIG_PM_SLEEP
 static int atc_suspend(struct ct_atc *atc)
 {
-	int i;
 	struct hw *hw = atc->hw;
 
 	snd_power_change_state(atc->card, SNDRV_CTL_POWER_D3hot);
 
-	for (i = FRONT; i < NUM_PCMS; i++) {
-		if (!atc->pcms[i])
-			continue;
-
-		snd_pcm_suspend_all(atc->pcms[i]);
-	}
-
 	atc_release_resources(atc);
 
 	hw->suspend(hw);
diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 907cf1a46712..18d30d479b6b 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -2165,9 +2165,6 @@ static int snd_echo_suspend(struct device *dev)
 {
 	struct echoaudio *chip = dev_get_drvdata(dev);
 
-	snd_pcm_suspend_all(chip->analog_pcm);
-	snd_pcm_suspend_all(chip->digital_pcm);
-
 #ifdef ECHOCARD_HAS_MIDI
 	/* This call can sleep */
 	if (chip->midi_out)
diff --git a/sound/pci/emu10k1/emu10k1.c b/sound/pci/emu10k1/emu10k1.c
index d3203df50a1a..3c41a0edcfb0 100644
--- a/sound/pci/emu10k1/emu10k1.c
+++ b/sound/pci/emu10k1/emu10k1.c
@@ -224,12 +224,6 @@ static int snd_emu10k1_suspend(struct device *dev)
 
 	cancel_delayed_work_sync(&emu->emu1010.firmware_work);
 
-	snd_pcm_suspend_all(emu->pcm);
-	snd_pcm_suspend_all(emu->pcm_mic);
-	snd_pcm_suspend_all(emu->pcm_efx);
-	snd_pcm_suspend_all(emu->pcm_multi);
-	snd_pcm_suspend_all(emu->pcm_p16v);
-
 	snd_ac97_suspend(emu->ac97);
 
 	snd_emu10k1_efx_suspend(emu);
diff --git a/sound/pci/ens1370.c b/sound/pci/ens1370.c
index 727eb3da1fda..1f2960ecc57e 100644
--- a/sound/pci/ens1370.c
+++ b/sound/pci/ens1370.c
@@ -2037,9 +2037,6 @@ static int snd_ensoniq_suspend(struct device *dev)
 	
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 
-	snd_pcm_suspend_all(ensoniq->pcm1);
-	snd_pcm_suspend_all(ensoniq->pcm2);
-	
 #ifdef CHIP1371	
 	snd_ac97_suspend(ensoniq->u.es1371.ac97);
 #else
diff --git a/sound/pci/es1938.c b/sound/pci/es1938.c
index 9d248eb2e26c..84d07bce581c 100644
--- a/sound/pci/es1938.c
+++ b/sound/pci/es1938.c
@@ -1475,7 +1475,6 @@ static int es1938_suspend(struct device *dev)
 	unsigned char *s, *d;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 
 	/* save mixer-related registers */
 	for (s = saved_regs, d = chip->saved_regs; *s; s++, d++)
diff --git a/sound/pci/es1968.c b/sound/pci/es1968.c
index 0b1845ca6005..9dcb698fc8c7 100644
--- a/sound/pci/es1968.c
+++ b/sound/pci/es1968.c
@@ -2392,7 +2392,6 @@ static int es1968_suspend(struct device *dev)
 	chip->in_suspend = 1;
 	cancel_work_sync(&chip->hwvol_work);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_ac97_suspend(chip->ac97);
 	snd_es1968_bob_stop(chip);
 	return 0;
diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c
index e3fb9c61017c..1317f3183eb1 100644
--- a/sound/pci/fm801.c
+++ b/sound/pci/fm801.c
@@ -1408,7 +1408,6 @@ static int snd_fm801_suspend(struct device *dev)
 	if (chip->tea575x_tuner & TUNER_ONLY) {
 		/* FIXME: tea575x suspend */
 	} else {
-		snd_pcm_suspend_all(chip->pcm);
 		snd_ac97_suspend(chip->ac97);
 		snd_ac97_suspend(chip->ac97_sec);
 	}
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) &&
diff --git a/sound/pci/ice1712/ice1712.c b/sound/pci/ice1712/ice1712.c
index f1fe497c2f9d..dda9b26192cb 100644
--- a/sound/pci/ice1712/ice1712.c
+++ b/sound/pci/ice1712/ice1712.c
@@ -2792,9 +2792,6 @@ static int snd_ice1712_suspend(struct device *dev)
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 
-	snd_pcm_suspend_all(ice->pcm);
-	snd_pcm_suspend_all(ice->pcm_pro);
-	snd_pcm_suspend_all(ice->pcm_ds);
 	snd_ac97_suspend(ice->ac97);
 
 	spin_lock_irq(&ice->reg_lock);
diff --git a/sound/pci/ice1712/ice1724.c b/sound/pci/ice1712/ice1724.c
index 057c2f394ea7..42994cf36156 100644
--- a/sound/pci/ice1712/ice1724.c
+++ b/sound/pci/ice1712/ice1724.c
@@ -2804,9 +2804,6 @@ static int snd_vt1724_suspend(struct device *dev)
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 
-	snd_pcm_suspend_all(ice->pcm);
-	snd_pcm_suspend_all(ice->pcm_pro);
-	snd_pcm_suspend_all(ice->pcm_ds);
 	snd_ac97_suspend(ice->ac97);
 
 	spin_lock_irq(&ice->reg_lock);
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index ffddcdfe0c66..885e1d488ed6 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -2614,8 +2614,6 @@ static int intel8x0_suspend(struct device *dev)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	for (i = 0; i < chip->pcm_devs; i++)
-		snd_pcm_suspend_all(chip->pcm[i]);
 	for (i = 0; i < chip->ncodecs; i++)
 		snd_ac97_suspend(chip->ac97[i]);
 	if (chip->device_type == DEVICE_INTEL_ICH4)
diff --git a/sound/pci/intel8x0m.c b/sound/pci/intel8x0m.c
index c84629190cba..44eb9e28a1eb 100644
--- a/sound/pci/intel8x0m.c
+++ b/sound/pci/intel8x0m.c
@@ -1025,11 +1025,8 @@ static int intel8x0m_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct intel8x0m *chip = card->private_data;
-	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	for (i = 0; i < chip->pcm_devs; i++)
-		snd_pcm_suspend_all(chip->pcm[i]);
 	snd_ac97_suspend(chip->ac97);
 	if (chip->irq >= 0) {
 		free_irq(chip->irq, chip);
diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c
index 62962178a9d7..1a9468c14aaf 100644
--- a/sound/pci/maestro3.c
+++ b/sound/pci/maestro3.c
@@ -2422,7 +2422,6 @@ static int m3_suspend(struct device *dev)
 	chip->in_suspend = 1;
 	cancel_work_sync(&chip->hwvol_work);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_ac97_suspend(chip->ac97);
 
 	msleep(10); /* give the assp a chance to idle.. */
diff --git a/sound/pci/nm256/nm256.c b/sound/pci/nm256/nm256.c
index b97f4ea6b56c..85e46ff44ac3 100644
--- a/sound/pci/nm256/nm256.c
+++ b/sound/pci/nm256/nm256.c
@@ -1413,7 +1413,6 @@ static int nm256_suspend(struct device *dev)
 	struct nm256 *chip = card->private_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_ac97_suspend(chip->ac97);
 	chip->coeffs_current = 0;
 	return 0;
diff --git a/sound/pci/oxygen/oxygen_lib.c b/sound/pci/oxygen/oxygen_lib.c
index b4ef5804212d..6dce56c209aa 100644
--- a/sound/pci/oxygen/oxygen_lib.c
+++ b/sound/pci/oxygen/oxygen_lib.c
@@ -744,13 +744,10 @@ static int oxygen_pci_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct oxygen *chip = card->private_data;
-	unsigned int i, saved_interrupt_mask;
+	unsigned int saved_interrupt_mask;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 
-	for (i = 0; i < PCM_COUNT; ++i)
-		snd_pcm_suspend(chip->streams[i]);
-
 	if (chip->model.suspend)
 		chip->model.suspend(chip);
 
diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c
index 23017e3bc76c..1d431c8052d6 100644
--- a/sound/pci/riptide/riptide.c
+++ b/sound/pci/riptide/riptide.c
@@ -1158,7 +1158,6 @@ static int riptide_suspend(struct device *dev)
 
 	chip->in_suspend = 1;
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	snd_ac97_suspend(chip->ac97);
 	return 0;
 }
diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index dcfa4d7a73e2..c56702e6cb60 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -2388,8 +2388,6 @@ static int rme96_suspend(struct device *dev)
 	struct rme96 *rme96 = card->private_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend(rme96->playback_substream);
-	snd_pcm_suspend(rme96->capture_substream);
 
 	/* save capture & playback pointers */
 	rme96->playback_pointer = readl(rme96->iobase + RME96_IO_GET_PLAY_POS)
diff --git a/sound/pci/sis7019.c b/sound/pci/sis7019.c
index 964acf302479..6b27980d77a8 100644
--- a/sound/pci/sis7019.c
+++ b/sound/pci/sis7019.c
@@ -1214,7 +1214,6 @@ static int sis_suspend(struct device *dev)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(sis->pcm);
 	if (sis->codecs_present & SIS_PRIMARY_CODEC_PRESENT)
 		snd_ac97_suspend(sis->ac97[0]);
 	if (sis->codecs_present & SIS_SECONDARY_CODEC_PRESENT)
diff --git a/sound/pci/trident/trident_main.c b/sound/pci/trident/trident_main.c
index 5523e193d556..f271ea436cff 100644
--- a/sound/pci/trident/trident_main.c
+++ b/sound/pci/trident/trident_main.c
@@ -3915,10 +3915,6 @@ static int snd_trident_suspend(struct device *dev)
 
 	trident->in_suspend = 1;
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(trident->pcm);
-	snd_pcm_suspend_all(trident->foldback);
-	snd_pcm_suspend_all(trident->spdif);
-
 	snd_ac97_suspend(trident->ac97);
 	snd_ac97_suspend(trident->ac97_sec);
 	return 0;
diff --git a/sound/pci/via82xx.c b/sound/pci/via82xx.c
index c488c5afa195..736ac79901b3 100644
--- a/sound/pci/via82xx.c
+++ b/sound/pci/via82xx.c
@@ -2278,8 +2278,6 @@ static int snd_via82xx_suspend(struct device *dev)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	for (i = 0; i < 2; i++)
-		snd_pcm_suspend_all(chip->pcms[i]);
 	for (i = 0; i < chip->num_devs; i++)
 		snd_via82xx_channel_reset(chip, &chip->devs[i]);
 	synchronize_irq(chip->irq);
diff --git a/sound/pci/via82xx_modem.c b/sound/pci/via82xx_modem.c
index b13c8688cc8d..3f59e0279058 100644
--- a/sound/pci/via82xx_modem.c
+++ b/sound/pci/via82xx_modem.c
@@ -1038,8 +1038,6 @@ static int snd_via82xx_suspend(struct device *dev)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	for (i = 0; i < 2; i++)
-		snd_pcm_suspend_all(chip->pcms[i]);
 	for (i = 0; i < chip->num_devs; i++)
 		snd_via82xx_channel_reset(chip, &chip->devs[i]);
 	synchronize_irq(chip->irq);
diff --git a/sound/pci/ymfpci/ymfpci_main.c b/sound/pci/ymfpci/ymfpci_main.c
index a4926fb03991..c688b7f481da 100644
--- a/sound/pci/ymfpci/ymfpci_main.c
+++ b/sound/pci/ymfpci/ymfpci_main.c
@@ -2304,10 +2304,6 @@ static int snd_ymfpci_suspend(struct device *dev)
 	unsigned int i;
 	
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
-	snd_pcm_suspend_all(chip->pcm2);
-	snd_pcm_suspend_all(chip->pcm_spdif);
-	snd_pcm_suspend_all(chip->pcm_4ch);
 	snd_ac97_suspend(chip->ac97);
 	for (i = 0; i < YDSXGR_NUM_SAVED_REGS; i++)
 		chip->saved_regs[i] = snd_ymfpci_readl(chip, saved_regs_index[i]);
-- 
2.20.1

[PATCH 06/14] ALSA: usb: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/card.c         | 1 -
 sound/usb/line6/driver.c | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/sound/usb/card.c b/sound/usb/card.c
index a105947eaf55..dfa38b78c494 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -811,7 +811,6 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
 		snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot);
 	if (!chip->num_suspended_intf++) {
 		list_for_each_entry(as, &chip->pcm_list, list) {
-			snd_pcm_suspend_all(as->pcm);
 			snd_usb_pcm_suspend(as);
 			as->substream[0].need_setup_ep =
 				as->substream[1].need_setup_ep = true;
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
index c1376bfdc90b..7afe8fae4939 100644
--- a/sound/usb/line6/driver.c
+++ b/sound/usb/line6/driver.c
@@ -849,10 +849,8 @@ int line6_suspend(struct usb_interface *interface, pm_message_t message)
 	if (line6->properties->capabilities & LINE6_CAP_CONTROL)
 		line6_stop_listen(line6);
 
-	if (line6pcm != NULL) {
-		snd_pcm_suspend_all(line6pcm->pcm);
+	if (line6pcm != NULL)
 		line6pcm->flags = 0;
-	}
 
 	return 0;
 }
-- 
2.20.1

[PATCH 07/14] ALSA: x86: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/x86/intel_hdmi_audio.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
index 00c92eb854ce..16ca91f57e7f 100644
--- a/sound/x86/intel_hdmi_audio.c
+++ b/sound/x86/intel_hdmi_audio.c
@@ -1651,18 +1651,6 @@ static int had_create_jack(struct snd_intelhad *ctx,
 static int __maybe_unused hdmi_lpe_audio_suspend(struct device *dev)
 {
 	struct snd_intelhad_card *card_ctx = dev_get_drvdata(dev);
-	int port;
-
-	for_each_port(card_ctx, port) {
-		struct snd_intelhad *ctx = &card_ctx->pcm_ctx[port];
-		struct snd_pcm_substream *substream;
-
-		substream = had_substream_get(ctx);
-		if (substream) {
-			snd_pcm_suspend(substream);
-			had_substream_put(ctx);
-		}
-	}
 
 	snd_power_change_state(card_ctx->card, SNDRV_CTL_POWER_D3hot);
 
-- 
2.20.1

[PATCH 08/14] ALSA: ppc: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/ppc/pmac.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/ppc/pmac.c b/sound/ppc/pmac.c
index d692e4070167..6d420bd3ae17 100644
--- a/sound/ppc/pmac.c
+++ b/sound/ppc/pmac.c
@@ -1365,7 +1365,6 @@ void snd_pmac_suspend(struct snd_pmac *chip)
 	snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot);
 	if (chip->suspend)
 		chip->suspend(chip);
-	snd_pcm_suspend_all(chip->pcm);
 	spin_lock_irqsave(&chip->reg_lock, flags);
 	snd_pmac_beep_stop(chip);
 	spin_unlock_irqrestore(&chip->reg_lock, flags);
-- 
2.20.1

[PATCH 09/14] ALSA: aoa: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/aoa/soundbus/i2sbus/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/sound/aoa/soundbus/i2sbus/core.c b/sound/aoa/soundbus/i2sbus/core.c
index c3f57a3fb1a5..33e82341c048 100644
--- a/sound/aoa/soundbus/i2sbus/core.c
+++ b/sound/aoa/soundbus/i2sbus/core.c
@@ -380,10 +380,6 @@ static int i2sbus_suspend(struct macio_dev* dev, pm_message_t state)
 	int err, ret = 0;
 
 	list_for_each_entry(i2sdev, &control->list, item) {
-		/* Notify Alsa */
-		/* Suspend PCM streams */
-		snd_pcm_suspend_all(i2sdev->sound.pcm);
-
 		/* Notify codecs */
 		list_for_each_entry(cii, &i2sdev->sound.codec_list, list) {
 			err = 0;
-- 
2.20.1

[PATCH 10/14] ALSA: arm: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/arm/aaci.c        | 1 -
 sound/arm/pxa2xx-ac97.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
index 0114ffed56dd..0c3f073e2600 100644
--- a/sound/arm/aaci.c
+++ b/sound/arm/aaci.c
@@ -757,7 +757,6 @@ static int aaci_do_suspend(struct snd_card *card)
 {
 	struct aaci *aaci = card->private_data;
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3cold);
-	snd_pcm_suspend_all(aaci->pcm);
 	return 0;
 }
 
diff --git a/sound/arm/pxa2xx-ac97.c b/sound/arm/pxa2xx-ac97.c
index 1f72672262d0..68fe5bb11eea 100644
--- a/sound/arm/pxa2xx-ac97.c
+++ b/sound/arm/pxa2xx-ac97.c
@@ -124,7 +124,6 @@ static int pxa2xx_ac97_do_suspend(struct snd_card *card)
 	pxa2xx_audio_ops_t *platform_ops = card->dev->platform_data;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3cold);
-	snd_pcm_suspend_all(pxa2xx_ac97_pcm);
 	snd_ac97_suspend(pxa2xx_ac97_ac97);
 	if (platform_ops && platform_ops->suspend)
 		platform_ops->suspend(platform_ops->priv);
-- 
2.20.1

[PATCH 11/14] ALSA: pcmcia: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pcmcia/pdaudiocf/pdaudiocf_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/pcmcia/pdaudiocf/pdaudiocf_core.c b/sound/pcmcia/pdaudiocf/pdaudiocf_core.c
index d724ab0653cf..eabf29252895 100644
--- a/sound/pcmcia/pdaudiocf/pdaudiocf_core.c
+++ b/sound/pcmcia/pdaudiocf/pdaudiocf_core.c
@@ -265,7 +265,6 @@ int snd_pdacf_suspend(struct snd_pdacf *chip)
 	u16 val;
 	
 	snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot);
-	snd_pcm_suspend_all(chip->pcm);
 	/* disable interrupts, but use direct write to preserve old register value in chip->regmap */
 	val = inw(chip->port + PDAUDIOCF_REG_IER);
 	val &= ~(PDAUDIOCF_IRQOVREN|PDAUDIOCF_IRQAKMEN|PDAUDIOCF_IRQLVLEN0|PDAUDIOCF_IRQLVLEN1);
-- 
2.20.1

[PATCH 12/14] drm: bridge: dw-hdmi: Remove superfluous snd_pcm_suspend*() calls

[Takashi Iwai]

The call of snd_pcm_suspend_all() & co became superfluous since we
call it in the PCM PM ops.  Let's remove them.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index cf3f0caf9c63..ed7af7518b52 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -614,7 +614,6 @@ static int snd_dw_hdmi_suspend(struct device *dev)
 	struct snd_dw_hdmi *dw = dev_get_drvdata(dev);
 
 	snd_power_change_state(dw->card, SNDRV_CTL_POWER_D3cold);
-	snd_pcm_suspend_all(dw->pcm);
 
 	return 0;
 }
-- 
2.20.1

[PATCH 13/14] ALSA: doc: Update the description about PCM suspend procedure

[Takashi Iwai]

The PCM suspend procedure was changed for drivers, so that they don't
have to call snd_pcm_suspend*() in each callback any longer.  Update
the documentation to adapt the changes.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../kernel-api/writing-an-alsa-driver.rst     | 25 ++++++-------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
index b37234afdfa1..7c2f2032d30a 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -3924,15 +3924,12 @@ The scheme of the real suspend job is as follows.
 2. Call :c:func:`snd_power_change_state()` with
    ``SNDRV_CTL_POWER_D3hot`` to change the power status.
 
-3. Call :c:func:`snd_pcm_suspend_all()` to suspend the running
-   PCM streams.
-
-4. If AC97 codecs are used, call :c:func:`snd_ac97_suspend()` for
+3. If AC97 codecs are used, call :c:func:`snd_ac97_suspend()` for
    each codec.
 
-5. Save the register values if necessary.
+4. Save the register values if necessary.
 
-6. Stop the hardware if necessary.
+5. Stop the hardware if necessary.
 
 A typical code would be like:
 
@@ -3946,12 +3943,10 @@ A typical code would be like:
           /* (2) */
           snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
           /* (3) */
-          snd_pcm_suspend_all(chip->pcm);
-          /* (4) */
           snd_ac97_suspend(chip->ac97);
-          /* (5) */
+          /* (4) */
           snd_mychip_save_registers(chip);
-          /* (6) */
+          /* (5) */
           snd_mychip_stop_hardware(chip);
           return 0;
   }
@@ -3994,13 +3989,9 @@ A typical code would be like:
           return 0;
   }
 
-As shown in the above, it's better to save registers after suspending
-the PCM operations via :c:func:`snd_pcm_suspend_all()` or
-:c:func:`snd_pcm_suspend()`. It means that the PCM streams are
-already stopped when the register snapshot is taken. But, remember that
-you don't have to restart the PCM stream in the resume callback. It'll
-be restarted via trigger call with ``SNDRV_PCM_TRIGGER_RESUME`` when
-necessary.
+Note that, at the time this callback gets called, the PCM stream has
+been already suspended via its own PM ops calling
+:c:func:`snd_pcm_suspend_all()` internally.
 
 OK, we have all callbacks now. Let's set them up. In the initialization
 of the card, make sure that you can get the chip data from the card
-- 
2.20.1

[PATCH 14/14] ALSA: pcm: Make snd_pcm_suspend() local static

[Takashi Iwai]

snd_pcm_suspend() is no longer called from outside, so let's make it
local static.  Also drop a superfluous NULL check there.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h     |  5 -----
 sound/core/pcm_native.c | 11 +++--------
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 04e97564949c..2c30c1ad1b0d 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -582,13 +582,8 @@ int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status);
 int snd_pcm_drain_done(struct snd_pcm_substream *substream);
 int snd_pcm_stop_xrun(struct snd_pcm_substream *substream);
 #ifdef CONFIG_PM
-int snd_pcm_suspend(struct snd_pcm_substream *substream);
 int snd_pcm_suspend_all(struct snd_pcm *pcm);
 #else
-static inline int snd_pcm_suspend(struct snd_pcm_substream *substream)
-{
-	return 0;
-}
 static inline int snd_pcm_suspend_all(struct snd_pcm *pcm)
 {
 	return 0;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 818dff1de545..26afb6b0889a 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1460,29 +1460,24 @@ static const struct action_ops snd_pcm_action_suspend = {
 	.post_action = snd_pcm_post_suspend
 };
 
-/**
+/*
  * snd_pcm_suspend - trigger SUSPEND to all linked streams
  * @substream: the PCM substream
  *
  * After this call, all streams are changed to SUSPENDED state.
  *
- * Return: Zero if successful (or @substream is %NULL), or a negative error
- * code.
+ * Return: Zero if successful, or a negative error code.
  */
-int snd_pcm_suspend(struct snd_pcm_substream *substream)
+static int snd_pcm_suspend(struct snd_pcm_substream *substream)
 {
 	int err;
 	unsigned long flags;
 
-	if (! substream)
-		return 0;
-
 	snd_pcm_stream_lock_irqsave(substream, flags);
 	err = snd_pcm_action(&snd_pcm_action_suspend, substream, 0);
 	snd_pcm_stream_unlock_irqrestore(substream, flags);
 	return err;
 }
-EXPORT_SYMBOL(snd_pcm_suspend);
 
 /**
  * snd_pcm_suspend_all - trigger SUSPEND to all substreams in the given pcm
-- 
2.20.1

Re: [PATCH 00/14] ALSA: PCM suspend cleanup

[Jaroslav Kysela]

Dne 15.1.2019 v 17:21 Takashi Iwai napsal(a):
> Hi,
> 
> this is a patch set to clean up the PCM suspend calls by introducing
> the PCM device PM ops.  This won't change much for ASoC but reduce
> lots of snd_pcm_suspend*() calls in other sound drivers.
> 
> Note that this patch series itself won't fix anything about the recent
> PM issue reported for Intel ASoC, but it was inspired through the
> discussion there.
> 
> 
> thanks,
> 
> Takashi
>

This change looks fine.

Reviewed-by: Jaroslav Kysela <perex@perex.cz>

Thanks,
  Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Pierre-Louis Bossart]

> 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

I just added a quick trace in the lines being deleted and those lines 
were definitively executed.

     list_for_each_entry(pcm, &codec->pcm_list_head, list) {
         pr_err("plb: suspending pcm\n");
         snd_pcm_suspend_all(pcm->pcm);
     }

[    2.516349] plb: suspending pcm
[    2.516350] plb: suspending pcm



_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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.


thanks,

Takashi

> I just added a quick trace in the lines being deleted and those lines
> were definitively executed.
> 
>     list_for_each_entry(pcm, &codec->pcm_list_head, list) {
>         pr_err("plb: suspending pcm\n");
>         snd_pcm_suspend_all(pcm->pcm);
>     }
> 
> [    2.516349] plb: suspending pcm
> [    2.516350] plb: suspending pcm

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [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,

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 421 bytes --]

On Tue, Jan 15, 2019 at 09:42:09PM +0100, Takashi Iwai wrote:

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

AFAICT it's just making sure that they're available ASAP so they look
always on to the rest of the system.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

On Wed, 16 Jan 2019 16:50:27 +0100,
Mark Brown wrote:
> 
> On Tue, Jan 15, 2019 at 09:42:09PM +0100, Takashi Iwai wrote:
> 
> > 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?
> 
> AFAICT it's just making sure that they're available ASAP so they look
> always on to the rest of the system.

Well, but PM prepare is called before PM suspend call.  And the whole
ASoC suspend procedure (including PCM suspend, etc) is performed in
the PM suspend callback; i.e. we stop sysclk before doing anything
else...


Takashi

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 576 bytes --]

On Wed, Jan 16, 2019 at 04:52:53PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > AFAICT it's just making sure that they're available ASAP so they look
> > always on to the rest of the system.

> Well, but PM prepare is called before PM suspend call.  And the whole
> ASoC suspend procedure (including PCM suspend, etc) is performed in
> the PM suspend callback; i.e. we stop sysclk before doing anything
> else...

Ah, got it the wrong way round...  in that case I frankly have no idea
and would expect it to break if we suspend with active audio - it's
possibly a bug.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 00/14] ALSA: PCM suspend cleanup

[Yang, Libin]

I have tested these patches on my platform with HD Audio.
Suspend => resume => playback
Suspend with playback => resume
It works!

Regards,
Libin


>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Wednesday, January 16, 2019 12:22 AM
>To: alsa-devel@alsa-project.org
>Cc: Yang, Libin <libin.yang@intel.com>; Keyon Jie <yang.jie@linux.intel.com>;
>liam.r.girdwood@linux.intel.com; Pierre-Louis Bossart <pierre-
>louis.bossart@linux.intel.com>; broonie@kernel.org; Lin, Mengdong
><mengdong.lin@intel.com>
>Subject: [PATCH 00/14] ALSA: PCM suspend cleanup
>
>Hi,
>
>this is a patch set to clean up the PCM suspend calls by introducing the PCM
>device PM ops.  This won't change much for ASoC but reduce lots of
>snd_pcm_suspend*() calls in other sound drivers.
>
>Note that this patch series itself won't fix anything about the recent PM issue
>reported for Intel ASoC, but it was inspired through the discussion there.
>
>
>thanks,
>
>Takashi
>
>===
>
>Takashi Iwai (14):
>  ALSA: pcm: Suspend streams globally via device type PM ops
>  ALSA: atiixp: Move PCM suspend/resume code into trigger callback
>  ALSA: isa: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: drivers: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: pci: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: usb: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: x86: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: ppc: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: aoa: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: arm: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: pcmcia: Remove superfluous snd_pcm_suspend*() calls
>  drm: bridge: dw-hdmi: Remove superfluous snd_pcm_suspend*() calls
>  ALSA: doc: Update the description about PCM suspend procedure
>  ALSA: pcm: Make snd_pcm_suspend() local static
>
> .../kernel-api/writing-an-alsa-driver.rst     | 25 ++++++------------
> .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c   |  1 -
> include/sound/pcm.h                           |  6 +----
> sound/aoa/soundbus/i2sbus/core.c              |  4 ---
> sound/arm/aaci.c                              |  1 -
> sound/arm/pxa2xx-ac97.c                       |  1 -
> sound/core/pcm.c                              | 26 +++++++++++++++++++
> sound/core/pcm_native.c                       | 11 +++-----
> sound/drivers/aloop.c                         |  4 ---
> sound/drivers/dummy.c                         |  2 --
> sound/drivers/pcsp/pcsp.c                     |  1 -
> sound/drivers/vx/vx_core.c                    |  4 ---
> sound/isa/ad1816a/ad1816a_lib.c               |  1 -
> sound/isa/als100.c                            |  1 -
> sound/isa/cmi8328.c                           |  1 -
> sound/isa/cmi8330.c                           |  1 -
> sound/isa/es1688/es1688.c                     |  2 --
> sound/isa/es18xx.c                            |  2 --
> sound/isa/sb/jazz16.c                         |  1 -
> sound/isa/sb/sb16.c                           |  1 -
> sound/isa/sb/sb8.c                            |  1 -
> sound/isa/wss/wss_lib.c                       |  1 -
> sound/pci/ali5451/ali5451.c                   |  4 +--
> sound/pci/als300.c                            |  1 -
> sound/pci/als4000.c                           |  1 -
> sound/pci/atiixp.c                            | 19 ++++++--------
> sound/pci/atiixp_modem.c                      |  2 --
> sound/pci/azt3328.c                           |  4 ---
> sound/pci/ca0106/ca0106_main.c                |  3 ---
> sound/pci/cmipci.c                            |  4 ---
> sound/pci/cs4281.c                            |  2 --
> sound/pci/cs46xx/cs46xx_lib.c                 |  6 -----
> sound/pci/cs5535audio/cs5535audio_pm.c        |  1 -
> sound/pci/ctxfi/ctatc.c                       |  8 ------
> sound/pci/echoaudio/echoaudio.c               |  3 ---
> sound/pci/emu10k1/emu10k1.c                   |  6 -----
> sound/pci/ens1370.c                           |  3 ---
> sound/pci/es1938.c                            |  1 -
> sound/pci/es1968.c                            |  1 -
> sound/pci/fm801.c                             |  1 -
> sound/pci/hda/hda_codec.c                     |  2 --
> sound/pci/ice1712/ice1712.c                   |  3 ---
> sound/pci/ice1712/ice1724.c                   |  3 ---
> sound/pci/intel8x0.c                          |  2 --
> sound/pci/intel8x0m.c                         |  3 ---
> sound/pci/maestro3.c                          |  1 -
> sound/pci/nm256/nm256.c                       |  1 -
> sound/pci/oxygen/oxygen_lib.c                 |  5 +---
> sound/pci/riptide/riptide.c                   |  1 -
> sound/pci/rme96.c                             |  2 --
> sound/pci/sis7019.c                           |  1 -
> sound/pci/trident/trident_main.c              |  4 ---
> sound/pci/via82xx.c                           |  2 --
> sound/pci/via82xx_modem.c                     |  2 --
> sound/pci/ymfpci/ymfpci_main.c                |  4 ---
> sound/pcmcia/pdaudiocf/pdaudiocf_core.c       |  1 -
> sound/ppc/pmac.c                              |  1 -
> sound/soc/soc-pcm.c                           |  1 +
> sound/usb/card.c                              |  1 -
> sound/usb/line6/driver.c                      |  4 +--
> sound/x86/intel_hdmi_audio.c                  | 12 ---------
> 61 files changed, 50 insertions(+), 174 deletions(-)
>
>--
>2.20.1

Re: [PATCH 14/14] ALSA: pcm: Make snd_pcm_suspend() local static

[Yang, Libin]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Wednesday, January 16, 2019 12:22 AM
>To: alsa-devel@alsa-project.org
>Cc: Yang, Libin <libin.yang@intel.com>; Keyon Jie <yang.jie@linux.intel.com>;
>liam.r.girdwood@linux.intel.com; Pierre-Louis Bossart <pierre-
>louis.bossart@linux.intel.com>; broonie@kernel.org; Lin, Mengdong
><mengdong.lin@intel.com>
>Subject: [PATCH 14/14] ALSA: pcm: Make snd_pcm_suspend() local static
>
>snd_pcm_suspend() is no longer called from outside, so let's make it local
>static.  Also drop a superfluous NULL check there.
>
>Signed-off-by: Takashi Iwai <tiwai@suse.de>
>---
> include/sound/pcm.h     |  5 -----
> sound/core/pcm_native.c | 11 +++--------
> 2 files changed, 3 insertions(+), 13 deletions(-)
>
>diff --git a/include/sound/pcm.h b/include/sound/pcm.h index
>04e97564949c..2c30c1ad1b0d 100644
>--- a/include/sound/pcm.h
>+++ b/include/sound/pcm.h
>@@ -582,13 +582,8 @@ int snd_pcm_stop(struct snd_pcm_substream
>*substream, snd_pcm_state_t status);  int snd_pcm_drain_done(struct
>snd_pcm_substream *substream);  int snd_pcm_stop_xrun(struct
>snd_pcm_substream *substream);  #ifdef CONFIG_PM -int
>snd_pcm_suspend(struct snd_pcm_substream *substream);  int
>snd_pcm_suspend_all(struct snd_pcm *pcm);  #else -static inline int
>snd_pcm_suspend(struct snd_pcm_substream *substream) -{
>-	return 0;
>-}
> static inline int snd_pcm_suspend_all(struct snd_pcm *pcm)  {
> 	return 0;
>diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index
>818dff1de545..26afb6b0889a 100644
>--- a/sound/core/pcm_native.c
>+++ b/sound/core/pcm_native.c
>@@ -1460,29 +1460,24 @@ static const struct action_ops
>snd_pcm_action_suspend = {
> 	.post_action = snd_pcm_post_suspend
> };
>
>-/**
>+/*
>  * snd_pcm_suspend - trigger SUSPEND to all linked streams
>  * @substream: the PCM substream
>  *
>  * After this call, all streams are changed to SUSPENDED state.
>  *
>- * Return: Zero if successful (or @substream is %NULL), or a negative error
>- * code.
>+ * Return: Zero if successful, or a negative error code.
>  */
>-int snd_pcm_suspend(struct snd_pcm_substream *substream)
>+static int snd_pcm_suspend(struct snd_pcm_substream *substream)

If some drivers may use snd_pcm_suspend() later for corner cases,
is it OK to make it be external again?

Regards,
Libin

> {
> 	int err;
> 	unsigned long flags;
>
>-	if (! substream)
>-		return 0;
>-
> 	snd_pcm_stream_lock_irqsave(substream, flags);
> 	err = snd_pcm_action(&snd_pcm_action_suspend, substream, 0);
> 	snd_pcm_stream_unlock_irqrestore(substream, flags);
> 	return err;
> }
>-EXPORT_SYMBOL(snd_pcm_suspend);
>
> /**
>  * snd_pcm_suspend_all - trigger SUSPEND to all substreams in the given pcm
>--
>2.20.1

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

[Pierre-Louis Bossart]

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

For the Atom/SST driver, I remember there was a need to set/restore the 
DSP state with a specific command that wasn't handled with regular 
controls - largely a work-around due to the firmware design.

For the Haswell driver, there was also a need to preserve/restore state 
and pause/stop pipelines (a recurring issue with the "Made for Windows" 
firmware).

These drivers are quite old now and it's not clear to me if they are 
broken or if we are talking of an improvement. Could you clarify what 
you view as "abuse"?

a) is this the fact that there are prepare/complete callback for those 
drivers, instead of others such as freeze, thaw, etc.

b) the fact they they call snd_soc_suspend/resume directly?

c) the fact that they suspend the PCM streams?

d) all of the above (which is entirely possible).

Thanks

-Pierre

Re: [PATCH 14/14] ALSA: pcm: Make snd_pcm_suspend() local static

[Takashi Iwai]

On Thu, 17 Jan 2019 02:50:21 +0100,
Yang, Libin wrote:
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Wednesday, January 16, 2019 12:22 AM
> >To: alsa-devel@alsa-project.org
> >Cc: Yang, Libin <libin.yang@intel.com>; Keyon Jie <yang.jie@linux.intel.com>;
> >liam.r.girdwood@linux.intel.com; Pierre-Louis Bossart <pierre-
> >louis.bossart@linux.intel.com>; broonie@kernel.org; Lin, Mengdong
> ><mengdong.lin@intel.com>
> >Subject: [PATCH 14/14] ALSA: pcm: Make snd_pcm_suspend() local static
> >
> >snd_pcm_suspend() is no longer called from outside, so let's make it local
> >static.  Also drop a superfluous NULL check there.
> >
> >Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >---
> > include/sound/pcm.h     |  5 -----
> > sound/core/pcm_native.c | 11 +++--------
> > 2 files changed, 3 insertions(+), 13 deletions(-)
> >
> >diff --git a/include/sound/pcm.h b/include/sound/pcm.h index
> >04e97564949c..2c30c1ad1b0d 100644
> >--- a/include/sound/pcm.h
> >+++ b/include/sound/pcm.h
> >@@ -582,13 +582,8 @@ int snd_pcm_stop(struct snd_pcm_substream
> >*substream, snd_pcm_state_t status);  int snd_pcm_drain_done(struct
> >snd_pcm_substream *substream);  int snd_pcm_stop_xrun(struct
> >snd_pcm_substream *substream);  #ifdef CONFIG_PM -int
> >snd_pcm_suspend(struct snd_pcm_substream *substream);  int
> >snd_pcm_suspend_all(struct snd_pcm *pcm);  #else -static inline int
> >snd_pcm_suspend(struct snd_pcm_substream *substream) -{
> >-	return 0;
> >-}
> > static inline int snd_pcm_suspend_all(struct snd_pcm *pcm)  {
> > 	return 0;
> >diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index
> >818dff1de545..26afb6b0889a 100644
> >--- a/sound/core/pcm_native.c
> >+++ b/sound/core/pcm_native.c
> >@@ -1460,29 +1460,24 @@ static const struct action_ops
> >snd_pcm_action_suspend = {
> > 	.post_action = snd_pcm_post_suspend
> > };
> >
> >-/**
> >+/*
> >  * snd_pcm_suspend - trigger SUSPEND to all linked streams
> >  * @substream: the PCM substream
> >  *
> >  * After this call, all streams are changed to SUSPENDED state.
> >  *
> >- * Return: Zero if successful (or @substream is %NULL), or a negative error
> >- * code.
> >+ * Return: Zero if successful, or a negative error code.
> >  */
> >-int snd_pcm_suspend(struct snd_pcm_substream *substream)
> >+static int snd_pcm_suspend(struct snd_pcm_substream *substream)
> 
> If some drivers may use snd_pcm_suspend() later for corner cases,
> is it OK to make it be external again?

Yes.  But it means that you're doing something special and often
wrong.  We can catch such a case more easily by this action :)


thanks,

Takashi

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

[Takashi Iwai]

On Thu, 17 Jan 2019 03:21:57 +0100,
Pierre-Louis Bossart wrote:
> 
> > 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.
> 
> For the Atom/SST driver, I remember there was a need to set/restore
> the DSP state with a specific command that wasn't handled with regular
> controls - largely a work-around due to the firmware design.
> 
> For the Haswell driver, there was also a need to preserve/restore
> state and pause/stop pipelines (a recurring issue with the "Made for
> Windows" firmware).
> 
> These drivers are quite old now and it's not clear to me if they are
> broken or if we are talking of an improvement. Could you clarify what
> you view as "abuse"?
> 
> a) is this the fact that there are prepare/complete callback for those
> drivers, instead of others such as freeze, thaw, etc.
> 
> b) the fact they they call snd_soc_suspend/resume directly?
> 
> c) the fact that they suspend the PCM streams?
> 
> d) all of the above (which is entirely possible).

The purpose of PM prepare and complete devops aren't for actually do
suspend and resume devices there, while these drivers call
snd_soc_suspend() and snd_soc_resume() to perform the complete suspend
/ resume procedure.  That's not the way these callbacks are supposed
to be used.

The prepare callback is called before the suspend callback of *all*
devices on the system.  Ditto for complete, it's called after the
resume of all devices.

I guess they use prepare/callback to assure some tasks to be performed
always suspend and resume.  But it's still puzzling,
e.g. sst_soc_prepare() has

static int sst_soc_prepare(struct device *dev)
{
	struct sst_data *drv = dev_get_drvdata(dev);
	struct snd_soc_pcm_runtime *rtd;

	if (!drv->soc_card)
		return 0;

	/* suspend all pcms first */
	snd_soc_suspend(drv->soc_card->dev);
	snd_soc_poweroff(drv->soc_card->dev);

	/* set the SSPs to idle */
	for_each_card_rtds(drv->soc_card, rtd) {
		struct snd_soc_dai *dai = rtd->cpu_dai;

		if (dai->active) {
			send_ssp_cmd(dai, dai->name, 0);
			sst_handle_vb_timer(dai, false);
		}
	}

	return 0;
}

... and it calls snd_soc_poweroff() for suspend.  That's odd and
likely superfluous.

And, the last part ("set the SSPs to idle") can be moved to
card->suspend_post hook, and then we can simply call
snd_soc_suspend().  Or it can be moved to PM devops suspend_late.

Similarly for sst_soc_complete(), the task "restart SSPs" can be moved
to card->resume_pre hook or PM devops resume_pre.


The rest is to make sure the device PM ops order, and that's the
hardest part.

Further looking at the code, we can see that several Intel ASoC
drivers have device PM ops.

sound/soc/intel/atom/sst/sst.c
sound/soc/intel/haswell/sst-haswell-pcm.c
sound/soc/intel/skylake/skl.c
sound/soc/intel/atom/sst-mfld-platform-pcm.c

... and there are codecs.  We need to list up and define the suspend /
resume call order.


Takashi

Re: [PATCH 14/14] ALSA: pcm: Make snd_pcm_suspend() local static

[Yang, Libin]

>> >diff --git a/include/sound/pcm.h b/include/sound/pcm.h index
>> >04e97564949c..2c30c1ad1b0d 100644
>> >--- a/include/sound/pcm.h
>> >+++ b/include/sound/pcm.h
>> >@@ -582,13 +582,8 @@ int snd_pcm_stop(struct snd_pcm_substream
>> >*substream, snd_pcm_state_t status);  int snd_pcm_drain_done(struct
>> >snd_pcm_substream *substream);  int snd_pcm_stop_xrun(struct
>> >snd_pcm_substream *substream);  #ifdef CONFIG_PM -int
>> >snd_pcm_suspend(struct snd_pcm_substream *substream);  int
>> >snd_pcm_suspend_all(struct snd_pcm *pcm);  #else -static inline int
>> >snd_pcm_suspend(struct snd_pcm_substream *substream) -{
>> >-	return 0;
>> >-}
>> > static inline int snd_pcm_suspend_all(struct snd_pcm *pcm)  {
>> > 	return 0;
>> >diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index
>> >818dff1de545..26afb6b0889a 100644
>> >--- a/sound/core/pcm_native.c
>> >+++ b/sound/core/pcm_native.c
>> >@@ -1460,29 +1460,24 @@ static const struct action_ops
>> >snd_pcm_action_suspend = {
>> > 	.post_action = snd_pcm_post_suspend  };
>> >
>> >-/**
>> >+/*
>> >  * snd_pcm_suspend - trigger SUSPEND to all linked streams
>> >  * @substream: the PCM substream
>> >  *
>> >  * After this call, all streams are changed to SUSPENDED state.
>> >  *
>> >- * Return: Zero if successful (or @substream is %NULL), or a
>> >negative error
>> >- * code.
>> >+ * Return: Zero if successful, or a negative error code.
>> >  */
>> >-int snd_pcm_suspend(struct snd_pcm_substream *substream)
>> >+static int snd_pcm_suspend(struct snd_pcm_substream *substream)
>>
>> If some drivers may use snd_pcm_suspend() later for corner cases, is
>> it OK to make it be external again?
>
>Yes.  But it means that you're doing something special and often wrong.  We
>can catch such a case more easily by this action :)

You are right. I checked SOF (it use snd_pcm_suspend()) and believe we 
don't have to call snd_pcm_suspend() directly. :-)

Regards,
Libin

>
>
>thanks,
>
>Takashi

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 962 bytes --]

On Wed, Jan 16, 2019 at 04:52:53PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > On Tue, Jan 15, 2019 at 09:42:09PM +0100, Takashi Iwai wrote:

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

> > AFAICT it's just making sure that they're available ASAP so they look
> > always on to the rest of the system.

> Well, but PM prepare is called before PM suspend call.  And the whole
> ASoC suspend procedure (including PCM suspend, etc) is performed in
> the PM suspend callback; i.e. we stop sysclk before doing anything
> else...

Thinking about this some more I'm moderately sure that the calls were
intended to do as I described but someone misunderstood what they did
and swapped them around.  I'm guessing nobody's been testing this.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 01/14] ALSA: pcm: Suspend streams globally via device type PM ops

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 309 bytes --]

On Tue, Jan 15, 2019 at 05:21:42PM +0100, Takashi Iwai wrote:
> Until now we rely on each driver calling snd_pcm_suspend*() explicitly
> at its own PM handling.  However, this can be done far more easily by
> setting the PM ops to each actual snd_pcm device object.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Pierre-Louis Bossart]

>>> 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.
>> For the Atom/SST driver, I remember there was a need to set/restore
>> the DSP state with a specific command that wasn't handled with regular
>> controls - largely a work-around due to the firmware design.
>>
>> For the Haswell driver, there was also a need to preserve/restore
>> state and pause/stop pipelines (a recurring issue with the "Made for
>> Windows" firmware).
>>
>> These drivers are quite old now and it's not clear to me if they are
>> broken or if we are talking of an improvement. Could you clarify what
>> you view as "abuse"?
>>
>> a) is this the fact that there are prepare/complete callback for those
>> drivers, instead of others such as freeze, thaw, etc.
>>
>> b) the fact they they call snd_soc_suspend/resume directly?
>>
>> c) the fact that they suspend the PCM streams?
>>
>> d) all of the above (which is entirely possible).
> The purpose of PM prepare and complete devops aren't for actually do
> suspend and resume devices there, while these drivers call
> snd_soc_suspend() and snd_soc_resume() to perform the complete suspend
> / resume procedure.  That's not the way these callbacks are supposed
> to be used.
>
> The prepare callback is called before the suspend callback of *all*
> devices on the system.  Ditto for complete, it's called after the
> resume of all devices.
>
> I guess they use prepare/callback to assure some tasks to be performed
> always suspend and resume.  But it's still puzzling,
> e.g. sst_soc_prepare() has
>
> static int sst_soc_prepare(struct device *dev)
> {
> 	struct sst_data *drv = dev_get_drvdata(dev);
> 	struct snd_soc_pcm_runtime *rtd;
>
> 	if (!drv->soc_card)
> 		return 0;
>
> 	/* suspend all pcms first */
> 	snd_soc_suspend(drv->soc_card->dev);
> 	snd_soc_poweroff(drv->soc_card->dev);
>
> 	/* set the SSPs to idle */
> 	for_each_card_rtds(drv->soc_card, rtd) {
> 		struct snd_soc_dai *dai = rtd->cpu_dai;
>
> 		if (dai->active) {
> 			send_ssp_cmd(dai, dai->name, 0);
> 			sst_handle_vb_timer(dai, false);
> 		}
> 	}
>
> 	return 0;
> }
>
> ... and it calls snd_soc_poweroff() for suspend.  That's odd and
> likely superfluous.
>
> And, the last part ("set the SSPs to idle") can be moved to
> card->suspend_post hook, and then we can simply call
> snd_soc_suspend().  Or it can be moved to PM devops suspend_late.
>
> Similarly for sst_soc_complete(), the task "restart SSPs" can be moved
> to card->resume_pre hook or PM devops resume_pre.
>
>
> The rest is to make sure the device PM ops order, and that's the
> hardest part.
>
> Further looking at the code, we can see that several Intel ASoC
> drivers have device PM ops.
>
> sound/soc/intel/atom/sst/sst.c
> sound/soc/intel/haswell/sst-haswell-pcm.c
> sound/soc/intel/skylake/skl.c
> sound/soc/intel/atom/sst-mfld-platform-pcm.c
>
> ... and there are codecs.  We need to list up and define the suspend /
> resume call order.

I had an offline discussion with Vinod and here are the key points for 
the Atom/SST driver

- the DSP isn't completely modeled with DPCM, there are some pipeline 
management and commands that need to be send manually. This isn't 
necessarily a perfect design but the one that was defined in 2013

- the choice of the .prepare is intentional. The tasks are split between 
the SST device (ACPI or PCI) and the platform device it creates. The 
ACPI/PCI layer handles DSP boot, config, shutdown, fw load, and the 
platform driver handles PCM/pipelines. The PM starts with the .prepare 
done in the child before the .suspend done at a higher level.

For Haswell I have no idea, and I wonder if there are actually any 
devices using this driver. Even from Broadwell we only know of the Pixel 
2015 Chromebook and the Dell XPS 13 where the I2S mode is activated (and 
the latter is deactivated with an ACPI quirk), most devices use HDaudio. 
Even if we found someone at Intel with bandwidth, changing this part is 
going to be very difficult between lack of devices and initial 
teams/individual contributors who have moved on.

SOF will support all these platforms, it might be a better idea to spend 
time making sure we do the right thing with the newer drivers than try 
to fix things but actually introduce regressions in legacy code.

-Pierre