Re: [alsa-devel] [PATCH 6/7] ASoC: amd: ACP powergating should be done by controller

On Sat, Oct 19, 2019 at 02:35:44AM +0530, Ravulapati Vishnu vardhan rao wrote:

diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c index 7f435b3..b74ecf6 100644 --- a/sound/soc/amd/raven/pci-acp3x.c +++ b/sound/soc/amd/raven/pci-acp3x.c @@ -19,11 +19,140 @@ struct acp3x_dev_data { struct platform_device *pdev[ACP3x_DEVS]; };

+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) {

Just flip this around.

if (val == 0) return 0;

• 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 (true) {
  

while (++timeout < 500) {

• 	val  = rv_readl(acp3x_base + mmACP_PGFSM_STATUS);
  

• 	if (!val)
  

• 		break;
  

return 0;

• 	udelay(1);
  

• 	if (timeout > 500) {
  

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

• 		ret = -ETIMEDOUT;
  

• 		break;
  

• 	}
  

• 	timeout++;
  

• }
  

• if (ret) {
  

• 	pr_err("ACP is not powered on status:%d\n", ret);
  

Just one error message is enough.

pr_err("ACP is Not Powered ON\n"); return -ETIMEDOUT;

• 	return ret;
  

• }
  

• }
• return ret;

+}

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

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

val is not used. We want to turn on set but not used warnings eventually.

• rv_writel(ACP_PGFSM_CNTL_POWER_OFF_MASK,

• 	acp3x_base + mmACP_PGFSM_CONTROL);
  

• while (true) {

• val  = rv_readl(acp3x_base + mmACP_PGFSM_STATUS);
  

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

• 	break;
  

• udelay(1);
  

• if (timeout > 500) {
  

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

• 	ret = -ETIMEDOUT;
  

• 	break;
  

• }
  

• timeout++;
  

• }
• if (ret)

• pr_err("ACP is not powered off status:%d\n", ret);
  

• return ret;

Same as above.

+}

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

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

while (++timeout < 100) {

• val = rv_readl(acp3x_base + mmACP_SOFT_RESET);
  

• if ((val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) ||
  

• 					timeout > 100) {
  

• 	if (val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK)
  

• 		break;
  

Duplicated needlessly.

• 	return -ENODEV;
  

• }
  

• timeout++;
  

• cpu_relax();
  

• }

if (timeout == 100) return -ENODEV;

• rv_writel(0, acp3x_base + mmACP_SOFT_RESET);
• timeout = 0;
• while (true) {

• val = rv_readl(acp3x_base + mmACP_SOFT_RESET);
  

Split the "if (!val) break;" into it's own condition instead of part of the ||.

• if (!val || timeout > 100) {
  

• 	if (!val)
  

• 		break;
  

• 	return -ENODEV;
  

• }
  

• timeout++;
  

• cpu_relax();
  

• }
• return 0;

+}

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

• int ret;
• /* power on */
• ret = acp3x_power_on(acp3x_base);
• if (ret) {

• pr_err("ACP3x power on failed\n");
  

• return ret;
  

• }
• /* Reset */
• ret = acp3x_reset(acp3x_base);
• if (ret) {

• pr_err("ACP3x reset failed\n");
  

• return ret;
  

• }
• return 0;

+}

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

• int ret;
• /* Reset */
• ret = acp3x_reset(acp3x_base);
• if (ret) {

• pr_err("ACP3x reset failed\n");
  

• return ret;
  

• }
• /* power off */
• ret = acp3x_power_off(acp3x_base);
• if (ret) {

• pr_err("ACP3x power off failed\n");
  

• return ret;
  

• }
• return 0;

+}

static int snd_acp3x_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) { int ret;

• u32 addr, val, i;

• u32 addr, val, i, status; struct acp3x_dev_data *adata; struct platform_device_info pdevinfo[ACP3x_DEVS]; unsigned int irqflags;

@@ -63,6 +192,10 @@ static int snd_acp3x_probe(struct pci_dev *pci, } pci_set_master(pci); pci_set_drvdata(pci, adata);

• status = acp3x_init(adata->acp3x_base);
• if (status)

• return -ENODEV;
  

Why do we need both "status" and "ret". Preserve the error code?

• val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG); switch (val) {

@@ -132,6 +265,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, return 0;

unmap_mmio:

• status = acp3x_deinit(adata->acp3x_base);
• if (status)

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

• else

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

  for (i = 0 ; i < ACP3x_DEVS ; i++) platform_device_unregister(adata->pdev[i]); kfree(adata->res);

@@ -153,6 +291,11 @@ static void snd_acp3x_remove(struct pci_dev *pci) for (i = 0 ; i < ACP3x_DEVS ; i++) platform_device_unregister(adata->pdev[i]); }

• i = acp3x_deinit(adata->acp3x_base);

Please don't re-use "i" like this. Declare "ret" or "status" or something.

• if (i)

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

• else

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

  iounmap(adata->acp3x_base);

  pci_disable_msi(pci);

regards, dan carpenter