From: Jon Hunter <jonathanh@nvidia.com> To: Takashi Iwai <tiwai@suse.de>, Sameer Pujar <spujar@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: Fri, 25 Jan 2019 11:36:00 +0000 [thread overview] Message-ID: <06c00ce1-32ed-8aa9-0340-d00202a8fa62@nvidia.com> (raw) In-Reply-To: <s5h5zudc1qw.wl-tiwai@suse.de> 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 ... sound/soc/tegra/tegra30_i2s.c > >> @@ -571,7 +576,14 @@ static void hda_tegra_probe_work(struct work_struct *work) >> struct platform_device *pdev = to_platform_device(hda->dev); >> int err; >> >> - pm_runtime_get_sync(hda->dev); >> + err = pm_runtime_get_sync(hda->dev); >> + if (err < 0) { >> + dev_err(hda->dev, >> + "failed in pm_runtime_get_syc with err = %d\n", >> + err); >> + return; >> + } > > This pm_runtime_get_sync() is needed just because you enabled runtime > PM before probe_work. Why not deferring the runtime PM enablement > after probing done? That would be fine with me. > That is what we need is the hda_tegra_enable_clocks() call at probe > unconditionally before enabling runtime PM. I think that calling hda_tegra_runtime_resume as above is sufficient. The nice thing about calling the runtime_resume function is that if for a future device there is something else in addition to clocks, say a reset, that also needs to be handled, we just added to the runtime_resume/suspend handlers and we are done. Cheers Jon -- nvpublic
WARNING: multiple messages have this Message-ID (diff)
From: Jon Hunter <jonathanh@nvidia.com> To: Takashi Iwai <tiwai@suse.de>, Sameer Pujar <spujar@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: Fri, 25 Jan 2019 11:36:00 +0000 [thread overview] Message-ID: <06c00ce1-32ed-8aa9-0340-d00202a8fa62@nvidia.com> (raw) In-Reply-To: <s5h5zudc1qw.wl-tiwai@suse.de> 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 ... sound/soc/tegra/tegra30_i2s.c > >> @@ -571,7 +576,14 @@ static void hda_tegra_probe_work(struct work_struct *work) >> struct platform_device *pdev = to_platform_device(hda->dev); >> int err; >> >> - pm_runtime_get_sync(hda->dev); >> + err = pm_runtime_get_sync(hda->dev); >> + if (err < 0) { >> + dev_err(hda->dev, >> + "failed in pm_runtime_get_syc with err = %d\n", >> + err); >> + return; >> + } > > This pm_runtime_get_sync() is needed just because you enabled runtime > PM before probe_work. Why not deferring the runtime PM enablement > after probing done? That would be fine with me. > That is what we need is the hda_tegra_enable_clocks() call at probe > unconditionally before enabling runtime PM. I think that calling hda_tegra_runtime_resume as above is sufficient. The nice thing about calling the runtime_resume function is that if for a future device there is something else in addition to clocks, say a reset, that also needs to be handled, we just added to the runtime_resume/suspend handlers and we are done. Cheers Jon -- nvpublic
next prev parent reply other threads:[~2019-01-25 11:36 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 [this message] 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 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=06c00ce1-32ed-8aa9-0340-d00202a8fa62@nvidia.com \ --to=jonathanh@nvidia.com \ --cc=alsa-devel@alsa-project.org \ --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=spujar@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.