Re: [RESEND PATCH v9 6/6] ASoC: amd: Added ACP3x system resume and runtime pm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 19/11/19 6:26 PM, vishnu wrote:


On 19/11/19 6:05 PM, Dan Carpenter wrote:
I can't apply this because I'm not CC'd on patches 2-5.

On Tue, Nov 19, 2019 at 05:41:16PM +0530, Ravulapati Vishnu vardhan rao wrote:
+static int acp3x_power_on(void __iomem *acp3x_base)
+{
+    u32 val;
+    u32 timeout;
+
+    timeout = 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) {

while (++timeout < 500)


If I check with timeout<500 and in next condition i have
if(timeout >500) this never happens.

Our intention is to wait for time out and exit.

+        val  = rv_readl(acp3x_base + mmACP_PGFSM_STATUS);
                    ^^
Extra space character.


+        if (!val)
+            break;

return 0;

+        udelay(1);
+        if (timeout > 500) {
+            pr_err("ACP is Not Powered ON\n");
+            return -ETIMEDOUT;
+        }
+    }
+    return 0;

Since we combined the ++timeout and the < 500 this becomes
"return -ETIMEOUT;" here.

+}
+
+static int acp3x_power_off(void __iomem *acp3x_base)
+{
+    u32 val;
+    u32 timeout, ret;

Both ret and timeout should just be int.  Please update this throughout.

+
+    timeout = 0;

Move the timeout = 0 next to the loop or put it in the initializer.

+    rv_writel(ACP_PGFSM_CNTL_POWER_OFF_MASK,
+            acp3x_base + mmACP_PGFSM_CONTROL);
+    while (++timeout) {

while (++timeout < 500) {

+        val  = rv_readl(acp3x_base + mmACP_PGFSM_STATUS);

Extra space char.

+        if ((val & ACP_PGFSM_STATUS_MASK) == ACP_POWERED_OFF) {
+            ret = 0;
+            break;

return 0;

+        }
+        udelay(1);
+        if (timeout > 500) {
+            pr_err("ACP is Not Powered OFF\n");
+            ret = -ETIMEDOUT;
+            break;
+        }
+    }
+    return ret;
+}
+
+static int acp3x_reset(void __iomem *acp3x_base)
+{
+    u32 val, timeout;
+
+    rv_writel(1, acp3x_base + mmACP_SOFT_RESET);
+    timeout = 0;
+    while (++timeout) {
+        val = rv_readl(acp3x_base + mmACP_SOFT_RESET);
+        if ((val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) ||
+                            timeout > 100) {

This timeout > 100 limit was difficult to spot.  Like finding Waldo.

+            if (val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK)
+                break;

This is a duplicate condition.

+            return -ENODEV;
+        }
+        cpu_relax();
+    }
+    rv_writel(0, acp3x_base + mmACP_SOFT_RESET);
+    timeout = 0;
+    while (++timeout) {
+        val = rv_readl(acp3x_base + mmACP_SOFT_RESET);
+        if (!val)
+            break;
+        if (timeout > 100)
+            return -ENODEV;
+        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)
  {
@@ -64,6 +186,9 @@ static int snd_acp3x_probe(struct pci_dev *pci,
      }
      pci_set_master(pci);
      pci_set_drvdata(pci, adata);
+    ret = acp3x_init(adata->acp3x_base);
+    if (ret)
+        goto disable_msi;
      val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG);
      switch (val) {
@@ -73,7 +198,7 @@ static int snd_acp3x_probe(struct pci_dev *pci,
                        GFP_KERNEL);
          if (!adata->res) {
              ret = -ENOMEM;
-            goto disable_msi;
+            goto de_init;
          }
          adata->res[0].name = "acp3x_i2s_iomem";
@@ -134,12 +259,23 @@ static int snd_acp3x_probe(struct pci_dev *pci,
          ret = -ENODEV;
          goto disable_msi;
      }
+    pm_runtime_set_autosuspend_delay(&pci->dev, 5000);
+    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;
  unregister_devs:
      if (val == I2S_MODE)
          for (i = 0 ; i < ACP3x_DEVS ; i++)
              platform_device_unregister(adata->pdev[i]);
+de_init:
+    ret = acp3x_deinit(adata->acp3x_base);
+    if (ret)
+        dev_err(&pci->dev, "ACP de-init failed\n");
+    else
+        dev_dbg(&pci->dev, "ACP de-initialized\n");


We can't overwrite ret (probe failed even if deinit() succeeded).  I
dont' know that the debug printk is useful.

de_init:
    if (acp3x_deinit(adata->acp3x_base))
        dev_err(&pci->dev, "ACP de-init failed in probe error handling\n");


  disable_msi:
      pci_disable_msi(pci);
  release_regions:
@@ -150,15 +286,58 @@ static int snd_acp3x_probe(struct pci_dev *pci,
      return ret;
  }
+static int  snd_acp3x_suspend(struct device *dev)
              ^^
Extra space char

+{
+    int status;

int ret;

+    struct acp3x_dev_data *adata;
+
+    adata = dev_get_drvdata(dev);
+    status = acp3x_deinit(adata->acp3x_base);
+    if (status)
+        dev_err(dev, "ACP de-init failed\n");
+    else
+        dev_dbg(dev, "ACP de-initialized\n");
+
+    return 0;
+}
+
+static int  snd_acp3x_resume(struct device *dev)
              ^^
Extra space

+{
+    int status;
+    struct acp3x_dev_data *adata;
+
+    adata = dev_get_drvdata(dev);
+    status = acp3x_init(adata->acp3x_base);
+    if (status) {
+        dev_err(dev, "ACP init failed\n");
+        return status;
+    }
+    return 0;
+}
+
+static const struct dev_pm_ops acp3x_pm = {
+    .runtime_suspend = snd_acp3x_suspend,
+    .runtime_resume =  snd_acp3x_resume,
+    .resume =       snd_acp3x_resume,

Fix whitespace.

+};
+
  static void snd_acp3x_remove(struct pci_dev *pci)
  {
-    struct acp3x_dev_data *adata = pci_get_drvdata(pci);

This was fine.  Leave it as-is.


Actually I was reported by kbuild robot tool about ISO mixed forbids of initialization so I did this.

-    int i;
+    struct acp3x_dev_data *adata;
+    int i, ret;
+    adata = pci_get_drvdata(pci);
      if (adata->acp3x_audio_mode == ACP3x_I2S_MODE) {
          for (i = 0 ; i <  ACP3x_DEVS ; i++)
                                 ^^
There is an extra space char here as well.  I guess I missed it when I
reviewed patch 1.

              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_dbg(&pci->dev, "ACP de-initialized\n");

Put the printk in acp3x_deinit() itself and remove it from all the
callers.

regards,
dan carpenter

_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux