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