From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46019C282D7 for ; Wed, 30 Jan 2019 10:39:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 20E3A218AF for ; Wed, 30 Jan 2019 10:39:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730724AbfA3Kjk (ORCPT ); Wed, 30 Jan 2019 05:39:40 -0500 Received: from mx2.suse.de ([195.135.220.15]:56798 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726548AbfA3Kjk (ORCPT ); Wed, 30 Jan 2019 05:39:40 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id B5D07B147; Wed, 30 Jan 2019 10:39:37 +0000 (UTC) Date: Wed, 30 Jan 2019 11:39:36 +0100 Message-ID: From: Takashi Iwai To: Jon Hunter Cc: Sameer Pujar , , , , , , , Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe In-Reply-To: References: <1548351403-1875-1-git-send-email-spujar@nvidia.com> <06c00ce1-32ed-8aa9-0340-d00202a8fa62@nvidia.com> <1f4c5185-e518-5674-4a8c-4e7db64aa0d3@nvidia.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 30 Jan 2019 10:35:35 +0100, Jon Hunter wrote: > > > On 28/01/2019 06:06, Sameer Pujar wrote: > > > > On 1/25/2019 7:34 PM, Jon Hunter wrote: > >> On 25/01/2019 13:58, Takashi Iwai wrote: > >>> On Fri, 25 Jan 2019 14:26:27 +0100, > >>> Jon Hunter wrote: > >>>> > >>>> On 25/01/2019 12:40, Takashi Iwai wrote: > >>>>> On Fri, 25 Jan 2019 12:36:00 +0100, > >>>>> Jon Hunter wrote: > >>>>>> > >>>>>> On 24/01/2019 19:08, Takashi Iwai wrote: > >>>>>>> On Thu, 24 Jan 2019 18:36:43 +0100, > >>>>>>> Sameer Pujar wrote: > >>>>>>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the > >>>>>>>> clocks > >>>>>>>> will not be ON. This could cause issue during probe, where hda init > >>>>>>>> setup is done. This patch checks whether runtime PM is enabled > >>>>>>>> or not. > >>>>>>>> If disabled, clocks are enabled in probe() and disabled in remove() > >>>>>>>> > >>>>>>>> This patch does following minor changes as cleanup, > >>>>>>>>    * return code check for pm_runtime_get_sync() to take care of > >>>>>>>> failure > >>>>>>>>      and exit gracefully. > >>>>>>>>    * In remove path runtime PM is disabled before calling > >>>>>>>> snd_card_free(). > >>>>>>>>    * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP > >>>>>>>> check. > >>>>>>>>    * runtime PM callbacks moved out of CONFIG_PM check > >>>>>>>> > >>>>>>>> Signed-off-by: Sameer Pujar > >>>>>>>> Reviewed-by: Ravindra Lokhande > >>>>>>>> Reviewed-by: Jon Hunter > >>>>>>> (snip) > >>>>>>>> @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct > >>>>>>>> platform_device *pdev) > >>>>>>>>       if (!azx_has_pm_runtime(chip)) > >>>>>>>>           pm_runtime_forbid(hda->dev); > >>>>>>>>   +    /* explicit resume if runtime PM is disabled */ > >>>>>>>> +    if (!pm_runtime_enabled(hda->dev)) { > >>>>>>>> +        err = hda_tegra_runtime_resume(hda->dev); > >>>>>>>> +        if (err) > >>>>>>>> +            goto out_free; > >>>>>>>> +    } > >>>>>>>> + > >>>>>>>>       schedule_work(&hda->probe_work); > >>>>>>> Calling runtime_resume here is really confusing... > >>>>>> Why? IMO it is better to have a single handler for resuming the > >>>>>> device > >>>>>> and so if RPM is not enabled we call the handler directly. This is > >>>>>> what > >>>>>> we have been advised to do in the past and do in other drivers. > >>>>>> See ... > >>>>> The point is that we're not "resuming" anything there.  It's in the > >>>>> early probe stage, and the device state is uninitialized, not really > >>>>> suspended.  It'd end up with just calling the same helper > >>>>> (hda_tegra_enable_clocks()), though. > >>>> Yes and you can make the same argument for every driver that calls > >>>> pm_runtime_get_sync() during probe to turn on clocks, handle resets, > >>>> etc, because at the end of the day the very first call to > >>>> pm_runtime_get_sync() invokes the runtime_resume callback, when we have > >>>> never been suspended. > >>> Although there are some magical pm_runtime_*() in some places, most of > >>> such pm_runtime_get_sync() is for the actual runtime PM management (to > >>> prevent the runtime suspend), while the code above is for explicitly > >>> setting up something for non-PM cases. > >>> > >>> And if pm_runtime_get_sync() is obviously superfluous, we should > >>> remove such calls.  Really. > >> Yes agree. > >> > >>>> Yes at the end of the day it is the same and given that we have done > >>>> this elsewhere I think it is good to be consistent if/where we can. > >>> The code becomes less readable, and that's a good reason against it :) > >> I don't its less readable. However, I do think it is less error prone :-) > > > > Do we have a consensus here? Request others to provide opinions to help > > close on this. > > I am not going to block this and ultimately it is Iwai-san call. > > However, I wonder if it would be appropriate to move the whole ... > > if (pm_runtime_enabled()) > ret = pm_runtime_get_sync(); > else > ret = hda_tegra_runtime_resume(); > > ... into the probe_work function? In other words, we are just resuming > when we really need to. Unless I am still misunderstanding Iwai-san > comment. Otherwise if Iwai-san is happy with V2 then go with that. Only from my personal taste, I find the v2 patch is better. It like simpler, after all. That is, the code in v1 patch probe() { .... pm_runtime_enable(); .... if (!pm_runtime_enabled()) hda_tegra_runtime_resume(); schedule_work(); } work() { pm_runtime_get_sync(); .... pm_runtime_put(); } becomes shorter in v2: probe() { .... hda_tegra_enable_clocks(); schedule_work(); } work() { .... pm_runtime_enable(); } However, the point about hda_tegra_remove() you raised in the v2 patch is still valid. (BTW, I guess the discussion followed in that thread was somehow misunderstood; your argument was about hda_tegra_remove() while Sameer discussed about the probe.) It can be with hda_tegra_disable_clocks() if we want more consistency. Though, I don't mind too much about that as long as the proper comment is given. thanks, Takashi