Re: [RESEND PATCH v9 6/6] ASoC: amd: Added ACP3x system resume and runtime pm
From: vishnu
Date: Tue Nov 19 2019 - 09:04:34 EST
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