From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 05/14] ALSA: pci: Remove superfluous snd_pcm_suspend*() calls Date: Thu, 17 Jan 2019 09:39:18 +0100 Message-ID: References: <20190115162155.6308-1-tiwai@suse.de> <20190115162155.6308-6-tiwai@suse.de> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 16067266A7C for ; Thu, 17 Jan 2019 09:39:18 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Pierre-Louis Bossart Cc: Libin Yang , alsa-devel@alsa-project.org, Mengdong Lin , Keyon Jie , liam.r.girdwood@linux.intel.com, broonie@kernel.org List-Id: alsa-devel@alsa-project.org 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