From mboxrd@z Thu Jan 1 00:00:00 1970 From: Koro Chen Subject: Re: ASoC updates for v4.2 Date: Tue, 23 Jun 2015 15:45:02 +0800 Message-ID: <1435045502.14283.5.camel@mtksdaap41> References: <20150622092616.GN14071@sirena.org.uk> <20150622103034.GQ14071@sirena.org.uk> <20150622135729.GR14071@sirena.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mailgw02.mediatek.com (unknown [210.61.82.184]) by alsa0.perex.cz (Postfix) with ESMTP id 55D302604ED for ; Tue, 23 Jun 2015 09:45:07 +0200 (CEST) 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: Takashi Iwai Cc: alsa-devel@alsa-project.org, Mark Brown , Liam Girdwood List-Id: alsa-devel@alsa-project.org On Mon, 2015-06-22 at 16:10 +0200, Takashi Iwai wrote: > At Mon, 22 Jun 2015 14:57:29 +0100, > Mark Brown wrote: > > > > On Mon, Jun 22, 2015 at 01:24:02PM +0200, Takashi Iwai wrote: > > > At Mon, 22 Jun 2015 11:30:34 +0100, > > > Mark Brown wrote: > > > > On Mon, Jun 22, 2015 at 11:58:24AM +0200, Takashi Iwai wrote: > > > > > > > And, looking at the code, it seems calling runtime suspend in the > > > > > following way at probe: > > > > > > > pm_runtime_enable(&pdev->dev); > > > > > if (!pm_runtime_enabled(&pdev->dev)) { > > > > > ret = mtk_afe_runtime_resume(&pdev->dev); > > > > > if (ret) > > > > > goto err_pm_disable; > > > > > } > > > > > > I'm confused, where's the call to runtime suspend? > > > > > It's in > > > static const struct dev_pm_ops mtk_afe_pm_ops = { > > > SET_RUNTIME_PM_OPS(mtk_afe_runtime_suspend, mtk_afe_runtime_resume, > > > NULL) > > > }; > > > > Sorry, I'm still confused about what you're seeing in the probe - I know > > where the callbacks for runtime PM are registered but I'm not seeing a > > call to suspend (or something that I'd expect to trigger one) in the > > above? > > There is no place calling runtime suspend manually, that's why the > compiler catches and warns. > > > > But my concern above isn't about the warning itself. I just stumbled > > > on the code invoking runtime resume while looking at this warning, and > > > wondered the behavior with CONFIG_PM=n. > > > > > Usually this kind of warning could be simply fixed by adding a proper > > > ifdef. But, this driver calls runtime resume in the probe manually. > > > > Sure, that's a fairly common pattern though? > > Depends. The more common pattern seems to call pm_runtime_resume(). > And this will skip the call of runtime PM when CONFIG_PM=n. > > > > > > I'm not sure whether this really behaves correctly, especially when a > > > > > kernel is built without CONFIG_PM. > > > > > > Could you be more specific about the problem you're seeing? If runtime > > > > PM is disabled pm_runtime_enabled() will return false and we'll run > > > > through the resume path during probe() instead, otherwise we'll runtime > > > > resume whenever we need to use the hardware. > > > > > The runtime suspend is never called when CONFIG_PM=n. OTOH, we call > > > runtime resume *always* at probe when CONFIG_PM=n. This looks > > > inconsistent to me. > > > > Yeah, it should really resuspend the hardware in the remove path. > > Right, then it'll be balanced properly. So I think maybe it should be modified like this? static int mtk_afe_pcm_dev_remove(struct platform_device *pdev) { pm_runtime_disable(&pdev->dev); + if (!pm_runtime_status_suspended(&pdev->dev)) + mtk_afe_runtime_suspend(&pdev->dev); snd_soc_unregister_component(&pdev->dev); snd_soc_unregister_platform(&pdev->dev); return 0; It can fix both build warning and unbalanced calls of suspend/resume. Should I send a patch for this? > > > > If it's a part of the mandatory initialization, it should be named > > > explicitly so, and make the runtime resume callback just calls it > > > instead. > > > > I disagree, I think either way is fine - if the clear intent and > > expectation is that the driver is used with runtime PM it seems fine to > > structure things to to say say "this is the special case path for !PM". > > I'd actually like to see this pattern better supported by the core so > > that drivers can enable runtime PM with calls that have !PM paths like > > in this driver, it'd make the whole !PM case a lot simpler. > > I don't mind much in either way. But the current code looks somehow > inconsistent. > > > Takashi > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel