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