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;
Date:   Mon, 04 Feb 2019 11:13:49 +0100
Message-ID: <s5ho97rrhcy.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Jon Hunter <jonathanh@nvidia.com>
Cc:     Sameer Pujar <spujar@nvidia.com>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Thierry Reding <thierry.reding@gmail.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Jaroslav Kysela <perex@perex.cz>,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@alsa-project.org>, <mkumard@nvidia.com>,
        <rlokhande@nvidia.com>, <sharadg@nvidia.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        <linux-tegra@vger.kernel.org>, Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v2] ALSA: hda/tegra: enable clock during probe
In-Reply-To: <80a45aa3-8a22-959c-0c73-03ad2d6b1f8c@nvidia.com>
References: <1548414418-5785-1-git-send-email-spujar@nvidia.com>
        <CAJZ5v0hezV7et3VgD7yHa8MLiP0z3YtJ9stwSQL54-tyz_VXkQ@mail.gmail.com>
        <20190131143024.GO23438@ulmo>
        <2034694.JE9CgBysmF@aspire.rjw.lan>
        <e7597220-2be7-ca29-50b7-c515c06cbed7@nvidia.com>
        <80a45aa3-8a22-959c-0c73-03ad2d6b1f8c@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

On Mon, 04 Feb 2019 11:04:50 +0100,
Jon Hunter wrote:
> 
> 
> On 04/02/2019 08:16, Sameer Pujar wrote:
> 
> ...
> 
> > Objective is to have things working with or without CONFIG_PM enabled.
> > From previous comments and discussions it appears that there is mixed
> > response
> > for calling hda_tegra_runtime_resume() or runtime PM APIs in probe()
> > call. Need
> > to have consensus regarding the best practice to be followed, which
> > would eventually
> > can be used in other drivers too.
> > 
> > Rafael is suggesting to use CONFIG_PM check to do manual setup or
> > runtime PM setup in probe,
> > which would bring back the earlier above mentioned concern.
> > 
> > if (IS_ENABLED(CONFIG_PM)) {
> > do setup based on pm-runtime
> > } else {
> >     do manual setup
> > }
> > Both if/else might end up doing the same here.
> > Do we really need CONFIG_PM check here?
> > 
> > Instead does below proposal appear fine?
> > 
> > probe() {
> >     hda_tegra_enable_clock();
> > }
> > 
> > probe_work() {
> >     /* hda setup */
> >     . . .
> >     pm_runtime_set_active(); /* initial state as active */
> >     pm_runtime_enable();
> >     return;
> > }
> 
> I believe that this still does not work, because if there is a
> power-domain that needs to be turned on, this does not guarantee this.
> So I think that you need to call pm_runtime_get ...
> 
>  probe() {
>  	if (!IS_ENABLED(CONFIG_PM))
>  		hda_tegra_enable_clock();
>  }
> 
> 
>  probe_work() {
>     	/* hda setup */
>     	. . .
> 	pm_runtime_enable();
> 	pm_runtime_get_sync();
> 	return;
>  }
> 
> The alternative here could just be ...
> 
>  probe() {
> 	pm_runtime_enable();
>  	if (!pm_runtime_enabled())
>  		ret = hda_tegra_enable_clock();
> 	else
> 		ret = pm_runtime_get_sync();
> 
> 	if (ret < 0) {
> 		...
> 	}
>  }
> 
> Very similar to what I was saying to begin with but not call the
> pm_runtime_resume handler directly. Which I believe was Iwai-san's dislike.

Yes, exactly, what bothered me was really a nuance: calling
hda_tegra_runtime_resume() there makes the code misleading (or
confusing) as if the runtime PM were mandatory.

I hoped there could be some standard idiom for this expression, but
apparently there isn't any, so far...

Obviously the easiest option is to enforce the dependency on
CONFIG_PM.  Would there be any platform that needs to run without PM,
practically seen...?

But, now after lengthy discussions and the clarification of the
current situation, I have no strong opinion on this any longer.
So I leave the decision to you guys, and I'll apply it as-is.


thanks,

Takashi