Messages in this thread Patch in this message | | | Subject | Re: [RESEND PATCH v9 6/6] ASoC: amd: Added ACP3x system resume and runtime pm | From | vishnu <> | Date | Tue, 19 Nov 2019 18:26:17 +0530 |
| |
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 >
| |