From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: ASoC updates for v4.2 Date: Mon, 22 Jun 2015 16:40:59 +0200 Message-ID: References: <20150622092616.GN14071@sirena.org.uk> <20150622103034.GQ14071@sirena.org.uk> <20150622135729.GR14071@sirena.org.uk> 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 mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 6CB17265083 for ; Mon, 22 Jun 2015 16:41:00 +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: Mark Brown Cc: alsa-devel@alsa-project.org, Koro Chen , Liam Girdwood List-Id: alsa-devel@alsa-project.org At Mon, 22 Jun 2015 16:10:32 +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. > > > > 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. In anyway, I now pushed out for-next branch after merging your two pull requests. Let's fix this warning later. thanks, Takashi