Re: [PATCH] ALSA: hda/tegra: enable clock during probe

[Jon Hunter]

On 30/01/2019 10:56, Sameer Pujar wrote:
> 
> 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@xxxxxxxxxx>
>>>>>>>>>>> Reviewed-by: Ravindra Lokhande <rlokhande@xxxxxxxxxx>
>>>>>>>>>>> Reviewed-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>>>>>>>>>> (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.

We should avoid replicating the function.

Jon

-- 
nvpublic