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 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 :)
thanks,
Takashi