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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>>> @@ -233,6 +234,12 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
>>>    		ret = PTR_ERR(adata->pdev);
>>>    		goto unregister_devs;
>>>    	}
>>> +	pm_runtime_set_autosuspend_delay(&pci->dev,
>> ACP_SUSPEND_DELAY_MS);
>>> +	pm_runtime_use_autosuspend(&pci->dev);
>>> +	pm_runtime_set_active(&pci->dev);
>>
>> is the set_active() needed? I haven't seen this in the other PCI audio
>> drivers? >
> We have similar implementation in our Raven ACP PCI driver as well
> which got up streamed.
> I will give a try by modifying this sequence.
> Could you please point me , what's exactly wrong with this code?

you would use pm_runtime_set_active() if the device was suspended. I 
don't think this can possibly happen since there is a _get done by the 
PCI core, which you compensate for in the line below.

Also look at drivers/pci/pci.c, the core already does this set_active() 
and _enable().

void pci_pm_init(struct pci_dev *dev)
{
...

	pm_runtime_forbid(&dev->dev);
	pm_runtime_set_active(&dev->dev);
	pm_runtime_enable(&dev->dev);

>>> +	pm_runtime_put_noidle(&pci->dev);
>>> +	pm_runtime_enable(&pci->dev);
>>
>> same, is the _enable() needed()?
> 
> We have similar implementation in Raven ACP PCI driver as well.

It's quite common unfortunately that extended pm_runtime sequences are 
used without checking what's necessary - it took Intel some time to 
clearly define what we needed and what was redundant/noop.

>>> +	pm_runtime_allow(&pci->dev);
>>>    	return 0;
>>>
>>>    unregister_devs:
>>> @@ -250,6 +257,42 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
>>>    	return ret;
>>>    }
>>>
>