Re: [alsa-devel] [RESEND PATCH v5 6/6] ASoC: amd: Added ACP3x system resume and runtime pm

diff --git a/sound/soc/amd/raven/acp3x.h b/sound/soc/amd/raven/acp3x.h index 01b283a..c40f960 100644 --- a/sound/soc/amd/raven/acp3x.h +++ b/sound/soc/amd/raven/acp3x.h @@ -7,6 +7,7 @@

#include "chip_offset_byte.h"

+#define DELAY 600

you probably want a prefix here to avoid conflicts

+static int acp3x_power_on(void __iomem *acp3x_base) +{

• u32 val;
• u32 timeout = 0;
• int ret = 0;
• val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS);
• if (val == 0)

• return val;
  

• if (!((val & ACP_PGFSM_STATUS_MASK) ==

• 		ACP_POWER_ON_IN_PROGRESS))
  

• rv_writel(ACP_PGFSM_CNTL_POWER_ON_MASK,
  

• 	acp3x_base + mmACP_PGFSM_CONTROL);
  

• while (++timeout < DELAY) {

so here timeout can reach 600.

• val  = rv_readl(acp3x_base + mmACP_PGFSM_STATUS);
  

• if (!val)
  

• 	break;
  

• udelay(1);
  

• if (timeout > 500) {
  

but here you abort at 500. Looks like the first test is not needed?

• 	pr_err("ACP is Not Powered ON\n");
  

• 	return -ETIMEDOUT;
  

• }
  

• }

+} +static int acp3x_power_off(void __iomem *acp3x_base) +{

• u32 val;
• u32 timeout = 0;
• rv_writel(ACP_PGFSM_CNTL_POWER_OFF_MASK,

• 	acp3x_base + mmACP_PGFSM_CONTROL);
  

• while (++timeout < DELAY) {

same here

• val  = rv_readl(acp3x_base + mmACP_PGFSM_STATUS);
  

• if ((val & ACP_PGFSM_STATUS_MASK) == ACP_POWERED_OFF)
  

• 	return 0;
  

• udelay(1);
  

• if (timeout > 500) {
  

• 	pr_err("ACP is Not Powered OFF\n");
  

• 	return -ETIMEDOUT;
  

• }
  

• }

+} +static int acp3x_reset(void __iomem *acp3x_base) +{

• u32 val, timeout;
• rv_writel(1, acp3x_base + mmACP_SOFT_RESET);
• timeout = 0;
• while (++timeout < DELAY) {

• val = rv_readl(acp3x_base + mmACP_SOFT_RESET);
  

• if ((val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) ||
  

• 					timeout > 100) {
  

and here

• 	if (val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK)
  

• 		break;
  

• 	return -ENODEV;
  

• }
  

• cpu_relax();
  

• }
• rv_writel(0, acp3x_base + mmACP_SOFT_RESET);
• timeout = 0;
• while (++timeout < DELAY) {

• val = rv_readl(acp3x_base + mmACP_SOFT_RESET);
  

• if (!val)
  

• 	break;
  

• if (timeout > 100)
  

and here.

@@ -133,9 +248,19 @@ static int snd_acp3x_probe(struct pci_dev *pci, ret = -ENODEV; goto unmap_mmio; }

• pm_runtime_set_autosuspend_delay(&pci->dev, 10000);

I think it's the largest value I've ever seen for autosuspend. It is necessary? If yes you should document why.

• pm_runtime_use_autosuspend(&pci->dev);

• pm_runtime_set_active(&pci->dev);

• pm_runtime_put_noidle(&pci->dev);

• pm_runtime_enable(&pci->dev); return 0;

  unmap_mmio:

• ret = acp3x_deinit(adata->acp3x_base);

• if (ret)

• dev_err(&pci->dev, "ACP de-init failed\n");
  

• else

• dev_info(&pci->dev, "ACP de-initialized\n");
  

dev_dbg?

pci_disable_msi(pci); for (i = 0 ; i < ACP3x_DEVS ; i++) platform_device_unregister(adata->pdev[i]); @@ -148,23 +273,57 @@ static int snd_acp3x_probe(struct pci_dev *pci,

return ret; } +static int snd_acp3x_suspend(struct device *dev) +{

• int status;
• struct acp3x_dev_data *adata = dev_get_drvdata(dev);
• status = acp3x_deinit(adata->acp3x_base);
• if (status)

• dev_err(dev, "ACP de-init failed\n");
  

• else

• dev_info(dev, "ACP de-initialized\n");
  

dev_dbg?

static void snd_acp3x_remove(struct pci_dev *pci) {

• int i;

• int i, ret; struct acp3x_dev_data *adata = pci_get_drvdata(pci);

  if (adata->acp3x_audio_mode == ACP3x_I2S_MODE) { for (i = 0 ; i < ACP3x_DEVS ; i++) platform_device_unregister(adata->pdev[i]); }

• ret = acp3x_deinit(adata->acp3x_base);

• if (ret)

• dev_err(&pci->dev, "ACP de-init failed\n");
  

• else

• dev_info(&pci->dev, "ACP de-initialized\n");
  

dev_dbg?