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 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 spujar@nvidia.com >>>> Reviewed-by: Ravindra Lokhande rlokhande@nvidia.com >>>> Reviewed-by: Jon Hunter jonathanh@nvidia.com >>> (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.
We might need entire functionality of hda_tegra_runtime_suspend() replicated here, if hda_tegra_disable_clocks() were to be used. Right now it takes care of both the cases where runtime PM is enabled/disabled. If you all agree, we can move the discussion to v2 patch.
Thanks, Sameer.
thanks,
Takashi