Messages in this thread | | | Subject | Re: [PATCH] ALSA: hda/tegra: enable clock during probe | From | Sameer Pujar <> | Date | Wed, 30 Jan 2019 16:26:31 +0530 |
| |
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
| |