From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe Date: Wed, 30 Jan 2019 11:39:36 +0100 Message-ID: References: <1548351403-1875-1-git-send-email-spujar@nvidia.com> <06c00ce1-32ed-8aa9-0340-d00202a8fa62@nvidia.com> <1f4c5185-e518-5674-4a8c-4e7db64aa0d3@nvidia.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Jon Hunter Cc: Sameer Pujar , pierre-louis.bossart@linux.intel.com, perex@perex.cz, alsa-devel@alsa-project.org, thierry.reding@gmail.com, rlokhande@nvidia.com, linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org List-Id: linux-tegra@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