From: Jon Hunter <jonathanh@nvidia.com>
To: Sameer Pujar <spujar@nvidia.com>, Takashi Iwai <tiwai@suse.de>
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 09:35:35 +0000 [thread overview]
Message-ID: <ef05ac7a-8fa8-5776-312a-afecdeae10c6@nvidia.com> (raw)
In-Reply-To: <da021878-5d89-c6d3-e5b2-4ab20f9b573b@nvidia.com>
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.
Cheers
Jon
--
nvpublic
next prev parent reply other threads:[~2019-01-30 9:35 UTC|newest]
Thread overview: 13+ 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 19:08 ` Takashi Iwai
2019-01-25 7:08 ` Sameer Pujar
2019-01-25 11:36 ` Jon Hunter
2019-01-25 12:40 ` Takashi Iwai
2019-01-25 13:26 ` Jon Hunter
2019-01-25 13:58 ` Takashi Iwai
2019-01-25 14:04 ` Jon Hunter
2019-01-28 6:06 ` Sameer Pujar
2019-01-30 9:35 ` Jon Hunter [this message]
2019-01-30 10:39 ` Takashi Iwai
2019-01-30 10:56 ` Sameer Pujar
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=ef05ac7a-8fa8-5776-312a-afecdeae10c6@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).