[PATCH v2 09/14] ASoC: amd: add Renoir ACP PCI driver PM ops

[Pierre-Louis Bossart]

>>>>> +     pm_runtime_set_autosuspend_delay(&pci->dev,
>> ACP_SUSPEND_DELAY_MS);
>>>>> +     pm_runtime_use_autosuspend(&pci->dev);
>>>>> +     pm_runtime_allow(&pci->dev);
>>>>> +     pm_runtime_mark_last_busy(&pci->dev);
>>>>> +     pm_runtime_put_autosuspend(&pci->dev);
>>>>
>>>> usually there is a pm_runtime_put_noidle() here?
>>>
>>> I'm not sure.
>>>
>>>>
>>>> [...]
>>>>
>>>>>     static void snd_rn_acp_remove(struct pci_dev *pci)
>>>>>     {
>>>>>         struct acp_dev_data *adata;
>>>>> @@ -260,6 +302,9 @@ static void snd_rn_acp_remove(struct pci_dev
>> *pci)
>>>>>         ret = rn_acp_deinit(adata->acp_base);
>>>>>         if (ret)
>>>>>                 dev_err(&pci->dev, "ACP de-init failed\n");
>>>>> +     pm_runtime_put_noidle(&pci->dev);
>>>>> +     pm_runtime_get_sync(&pci->dev);
>>>>> +     pm_runtime_forbid(&pci->dev);
>>>>
>>>> doing a put_noidle() followed by a get_sync() and immediately forbid()
>>>> is very odd at best.
>>>> Isn't the recommendation to call get_noresume() here?
>>>>
>>>
>>> I'm not sure here either.  Is there some definitive documentation on
>>> what exact sequences are supposed to be used in drivers?  A quick
>>> browse through drivers that implement runtime pm seems to show a lot
>>> of variation.  This sequence works.  I'm not sure if it's optimal or
>>> not.
>>
>> We based our sequence on the comments in drivers/pci/pci-driver.c
>>
>> /*
>>    * Unbound PCI devices are always put in D0, regardless of
>>    * runtime PM status.  During probe, the device is set to
>>    * active and the usage count is incremented.  If the driver
>>    * supports runtime PM, it should call pm_runtime_put_noidle(),
>>    * or any other runtime PM helper function decrementing the usage
>>    * count, in its probe routine and pm_runtime_get_noresume() in
>>    * its remove routine.
>>    */
> 
> If I understood correctly, below should be  the correct sequence rite ?
> 
> acp pci driver probe sequence:
> 
> pm_runtime_set_autosuspend_delay(&pci->dev, ACP_SUSPEND_DELAY_MS);
> pm_runtime_use_autosuspend(&pci->dev);
> pm_runtime_put_noidle(&pci->dev);
> pm_runtime_allow(&pci->dev);	

sounds about right. We added an extra pm_runtime_mark_last_busy() to 
make sure the information is updated, but that's probably on the 
paranoid side.
> 	
> acp pci driver remove sequence:
> 
> pm_runtime_get_noresume(&pci->dev);
> pm_runtime_disable(&pci->dev);
> 
> I have still have a doubt.
> Do we need to call pm_runtime_disable() explicitly  in this case ?

we don't call pm_runtime_disable().