From: Sameer Pujar <spujar@nvidia.com> To: Takashi Iwai <tiwai@suse.de>, Jon Hunter <jonathanh@nvidia.com> Cc: pierre-louis.bossart@linux.intel.com, perex@perex.cz, alsa-devel@alsa-project.org, thierry.reding@gmail.com, rlokhande@nvidia.com, linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe Date: Wed, 30 Jan 2019 16:26:31 +0530 [thread overview] Message-ID: <4aee601a-f72c-7c94-60cb-ec2546900b8c@nvidia.com> (raw) In-Reply-To: <s5h8sz2e8ev.wl-tiwai@suse.de> 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
WARNING: multiple messages have this Message-ID (diff)
From: Sameer Pujar <spujar@nvidia.com> To: Takashi Iwai <tiwai@suse.de>, Jon Hunter <jonathanh@nvidia.com> Cc: <pierre-louis.bossart@linux.intel.com>, <perex@perex.cz>, <alsa-devel@alsa-project.org>, <thierry.reding@gmail.com>, <rlokhande@nvidia.com>, <linux-kernel@vger.kernel.org>, <linux-tegra@vger.kernel.org> Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe Date: Wed, 30 Jan 2019 16:26:31 +0530 [thread overview] Message-ID: <4aee601a-f72c-7c94-60cb-ec2546900b8c@nvidia.com> (raw) In-Reply-To: <s5h8sz2e8ev.wl-tiwai@suse.de> 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
next prev parent reply other threads:[~2019-01-30 10:56 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-24 17:36 [PATCH] ALSA: hda/tegra: enable clock during probe Sameer Pujar 2019-01-24 17:36 ` Sameer Pujar 2019-01-24 19:08 ` Takashi Iwai 2019-01-24 19:08 ` Takashi Iwai 2019-01-25 7:08 ` Sameer Pujar 2019-01-25 7:08 ` Sameer Pujar 2019-01-25 11:36 ` Jon Hunter 2019-01-25 11:36 ` Jon Hunter 2019-01-25 12:40 ` Takashi Iwai 2019-01-25 12:40 ` Takashi Iwai 2019-01-25 13:26 ` Jon Hunter 2019-01-25 13:26 ` Jon Hunter 2019-01-25 13:58 ` Takashi Iwai 2019-01-25 13:58 ` Takashi Iwai 2019-01-25 14:04 ` Jon Hunter 2019-01-25 14:04 ` Jon Hunter 2019-01-28 6:06 ` Sameer Pujar 2019-01-28 6:06 ` Sameer Pujar 2019-01-30 9:35 ` Jon Hunter 2019-01-30 9:35 ` Jon Hunter 2019-01-30 10:39 ` Takashi Iwai 2019-01-30 10:39 ` Takashi Iwai 2019-01-30 10:56 ` Sameer Pujar [this message] 2019-01-30 10:56 ` Sameer Pujar 2019-01-30 12:24 ` Jon Hunter 2019-01-30 12:24 ` Jon Hunter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4aee601a-f72c-7c94-60cb-ec2546900b8c@nvidia.com \ --to=spujar@nvidia.com \ --cc=alsa-devel@alsa-project.org \ --cc=jonathanh@nvidia.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=perex@perex.cz \ --cc=pierre-louis.bossart@linux.intel.com \ --cc=rlokhande@nvidia.com \ --cc=thierry.reding@gmail.com \ --cc=tiwai@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.