On Wed, 30 Jan 2019 10:35:35 +0100,We might need entire functionality of hda_tegra_runtime_suspend() replicated here,
Jon Hunter wrote:
Only from my personal taste, I find the v2 patch is better.
On 28/01/2019 06:06, Sameer Pujar wrote:
On 1/25/2019 7:34 PM, Jon Hunter wrote:I am not going to block this and ultimately it is Iwai-san call.
On 25/01/2019 13:58, Takashi Iwai wrote:Do we have a consensus here? Request others to provide opinions to help
On Fri, 25 Jan 2019 14:26:27 +0100,Yes agree.
Jon Hunter wrote:
On 25/01/2019 12:40, Takashi Iwai wrote:Although there are some magical pm_runtime_*() in some places, most of
On Fri, 25 Jan 2019 12:36:00 +0100,Yes and you can make the same argument for every driver that calls
Jon Hunter wrote:
On 24/01/2019 19:08, Takashi Iwai wrote:The point is that we're not "resuming" anything there. It's in the
On Thu, 24 Jan 2019 18:36:43 +0100,Why? IMO it is better to have a single handler for resuming the
Sameer Pujar wrote:
If CONFIG_PM is disabled or runtime PM calls are forbidden, the(snip)
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@xxxxxxxxxx>
Reviewed-by: Ravindra Lokhande <rlokhande@xxxxxxxxxx>
Reviewed-by: Jon Hunter <jonathanh@xxxxxxxxxx>
@@ -555,6 +553,13 @@ static int hda_tegra_probe(structCalling runtime_resume here is really confusing...
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);
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 ...
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.
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.
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.
I don't its less readable. However, I do think it is less error prone :-)Yes at the end of the day it is the same and given that we have doneThe code becomes less readable, and that's a good reason against it :)
this elsewhere I think it is good to be consistent if/where we can.
close on this.
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.
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