Message-ID: <4a607da0-616b-378d-b62a-598277f0254c@nvidia.com>
Date:   Wed, 30 Jan 2019 12:24:17 +0000
From:   Jon Hunter <jonathanh@...dia.com>
To:     Sameer Pujar <spujar@...dia.com>, Takashi Iwai <tiwai@...e.de>
CC:     <pierre-louis.bossart@...ux.intel.com>, <perex@...ex.cz>,
        <alsa-devel@...a-project.org>, <thierry.reding@...il.com>,
        <rlokhande@...dia.com>, <linux-kernel@...r.kernel.org>,
        <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe


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@...dia.com>
>>>>>>>>>>> Reviewed-by: Ravindra Lokhande <rlokhande@...dia.com>
>>>>>>>>>>> Reviewed-by: Jon Hunter <jonathanh@...dia.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.

We should avoid replicating the function.

Jon

-- 
nvpublic

Powered by blists - more mailing lists