On Mon, May 11, 2020 at 6:37 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
@@ -233,6 +234,11 @@ 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_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.
Alex
pci_disable_msi(pci); pci_release_regions(pci); pci_disable_device(pci);@@ -278,6 +323,9 @@ static struct pci_driver rn_acp_driver = { .id_table = snd_rn_acp_ids, .probe = snd_rn_acp_probe, .remove = snd_rn_acp_remove,
.driver = {.pm = &rn_acp_pm,}};
module_pci_driver(rn_acp_driver);
diff --git a/sound/soc/amd/renoir/rn_acp3x.h b/sound/soc/amd/renoir/rn_acp3x.h index a4f654cf2df0..6e1888167fb3 100644 --- a/sound/soc/amd/renoir/rn_acp3x.h +++ b/sound/soc/amd/renoir/rn_acp3x.h @@ -40,6 +40,8 @@ #define TWO_CH 0x02 #define DELAY_US 5 #define ACP_COUNTER 20000 +/* time in ms for runtime suspend delay */ +#define ACP_SUSPEND_DELAY_MS 2000
#define ACP_SRAM_PTE_OFFSET 0x02050000 #define PAGE_SIZE_4K_ENABLE 0x2