[alsa-devel] [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