Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp5747858imu; Wed, 30 Jan 2019 02:58:17 -0800 (PST) X-Google-Smtp-Source: ALg8bN5EVTDIBgGDJOrfI2JeQoJHHULRg4A6Nc98aJ6PNNMRNrNLYcCPSqLllwuvBVJnRnvJg3Yr X-Received: by 2002:a63:bf0b:: with SMTP id v11mr27702622pgf.302.1548845897135; Wed, 30 Jan 2019 02:58:17 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1548845897; cv=none; d=google.com; s=arc-20160816; b=j8RDoKPiYo4/rJDSliZ1t4yMhBHXEgZsVWrM21JbFjMZP4EsNcnQ7bb5uQeZ4wC5MW kLG4+OuTmeTjFWpw9PSxjAOfgQ8nEt0aoKIYqCTrCH+ya3/PERrqioPAl2X1trmxCRoJ v7/3+EWI5jxZ6Hewy/j6Md8DC7HRCB0QoCmveA8WK5vq+D9/+iSnJMZ/Q+mrYEivkXjc Ii/vFy3f/bnZtINuJ2ZToRdcEZ+RN/yX/vrmx4hmxCR+3rUXtNsO7ccsO3OoZaEl5ftp 3Hq0vEHbZu2DJNLAxOLtXUTofeJTmpurwLTLC7jspgQrh2HrWpMhKHMva6Rcr5cKeKR0 jZOw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:dkim-signature:content-language :content-transfer-encoding:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=2+1zAyodgP2UzOJoxbEAtjNNYvFD/WJsP4T5J2h6yJc=; b=JnyNC4yjzbatAiIaPZZXPE5zyINL030sMRjgXa1hlxT5an464QX7Gdw1vSHg7CzmEJ YmyAOXzcmAjQ38+UPxcbGtqER5KXawNvODt3HzKv5UBW0crVqRHdgxIfbl1r/B3mtFw9 jBPDqRhmvjwqdr54YfIK6MBSQ20swSDZluy7/ndDhWkB3gs4Ny9lEuA29yQGRIAJ/H9p GPnhpvkGxktuz8Rh1e1kiu1ILG+R+ZXwCiwja1M3X99ENpwakZXRIlnCxkXMoDGjYpla 78jN1xNmRTH13YJ916ItEwBVmTJfwrLNPXAYmdnOdENbX8YqS4R3dRB3SsqDSFqhssdu B/7Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@nvidia.com header.s=n1 header.b=BCPgb+Ti; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n3si1184464pfn.285.2019.01.30.02.58.01; Wed, 30 Jan 2019 02:58:16 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@nvidia.com header.s=n1 header.b=BCPgb+Ti; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730302AbfA3K4j (ORCPT + 99 others); Wed, 30 Jan 2019 05:56:39 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:12720 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726548AbfA3K4j (ORCPT ); Wed, 30 Jan 2019 05:56:39 -0500 Received: from hqpgpgate102.nvidia.com (Not Verified[216.228.121.13]) by hqemgate14.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Wed, 30 Jan 2019 02:56:39 -0800 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate102.nvidia.com (PGP Universal service); Wed, 30 Jan 2019 02:56:37 -0800 X-PGP-Universal: processed; by hqpgpgate102.nvidia.com on Wed, 30 Jan 2019 02:56:37 -0800 Received: from [10.24.44.212] (10.124.1.5) by HQMAIL101.nvidia.com (172.20.187.10) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 30 Jan 2019 10:56:34 +0000 Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe To: Takashi Iwai , Jon Hunter CC: , , , , , , References: <1548351403-1875-1-git-send-email-spujar@nvidia.com> <06c00ce1-32ed-8aa9-0340-d00202a8fa62@nvidia.com>

<1f4c5185-e518-5674-4a8c-4e7db64aa0d3@nvidia.com>

From: Sameer Pujar Message-ID: <4aee601a-f72c-7c94-60cb-ec2546900b8c@nvidia.com> Date: Wed, 30 Jan 2019 16:26:31 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.124.1.5] X-ClientProxiedBy: HQMAIL104.nvidia.com (172.18.146.11) To HQMAIL101.nvidia.com (172.20.187.10) Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: quoted-printable Content-Language: en-GB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1548845799; bh=2+1zAyodgP2UzOJoxbEAtjNNYvFD/WJsP4T5J2h6yJc=; h=X-PGP-Universal:Subject:To:CC:References:From:Message-ID:Date: User-Agent:MIME-Version:In-Reply-To:X-Originating-IP: X-ClientProxiedBy:Content-Type:Content-Transfer-Encoding: Content-Language; b=BCPgb+TivNkOEl/jnFzVOL2VPiun3heqfua/gDuyzMwigxE60czpnr0Qj3zYuFUCz l0sxU5MSDUsxNi5ugdi8qsNMqQ/hUw642AUIuvt7wyB/fJRes/y/IEU7NisQRKHbKK k6QtOMt9gAUR9uy+CZXWdqYKu1Df/jKB8aUe78mMEaffmPJvYXuvtQ6mWAngVb7ntU 1fFz2SmdDOXHN2F/+W+jLYLFO6u4szzQYSGFioipeZmF5PzQTnzNwlEGejO+Jj07vu r9WvtdB9F9YDzXZ3u/NH+LeUQTCvlVPjnBnpWKxI29z9+BWCKuB2CU0mGRgrVfhMP1 qmInxQLGsAAuA== Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 1/30/2019 4:09 PM, Takashi Iwai wrote: > 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 i= nit >>>>>>>>>> setup is done. This patch checks whether runtime PM is enabled >>>>>>>>>> or not. >>>>>>>>>> If disabled, clocks are enabled in probe() and disabled in remov= e() >>>>>>>>>> >>>>>>>>>> This patch does following minor changes as cleanup, >>>>>>>>>> =C2=A0=C2=A0 * return code check for pm_runtime_get_sync() to t= ake care of >>>>>>>>>> failure >>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 and exit gracefully. >>>>>>>>>> =C2=A0=C2=A0 * In remove path runtime PM is disabled before cal= ling >>>>>>>>>> snd_card_free(). >>>>>>>>>> =C2=A0=C2=A0 * hda_tegra_disable_clocks() is moved out of CONFI= G_PM_SLEEP >>>>>>>>>> check. >>>>>>>>>> =C2=A0=C2=A0 * runtime PM callbacks moved out of CONFIG_PM chec= k >>>>>>>>>> >>>>>>>>>> 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) >>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!azx_has_pm_runtime(chip)) >>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pm_runti= me_forbid(hda->dev); >>>>>>>>>> =C2=A0 +=C2=A0=C2=A0=C2=A0 /* explicit resume if runtime PM is = disabled */ >>>>>>>>>> +=C2=A0=C2=A0=C2=A0 if (!pm_runtime_enabled(hda->dev)) { >>>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 err =3D hda_tegra_ru= ntime_resume(hda->dev); >>>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (err) >>>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 goto out_free; >>>>>>>>>> +=C2=A0=C2=A0=C2=A0 } >>>>>>>>>> + >>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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.=C2=A0 It's i= n the >>>>>>> early probe stage, and the device state is uninitialized, not reall= y >>>>>>> suspended.=C2=A0 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 h= ave >>>>>> never been suspended. >>>>> Although there are some magical pm_runtime_*() in some places, most o= f >>>>> such pm_runtime_get_sync() is for the actual runtime PM management (t= o >>>>> 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.=C2=A0 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 =3D pm_runtime_get_sync(); >> else >> ret =3D 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. We might need entire functionality of hda_tegra_runtime_suspend()=20 replicated here, if hda_tegra_disable_clocks() were to be used. Right now it takes care=20 of both the cases where runtime PM is enabled/disabled. If you all agree, we can=20 move the discussion to v2 patch. Thanks, Sameer. > > thanks, > > Takashi