| Message ID | 1574165476-24987-7-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v9,1/6] ASoC: amd:Create multiple I2S platform device endpoint | expand |
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) > + 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
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 >
On Tue, Nov 19, 2019 at 06:26:17PM +0530, 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. I was maybe not clear enough. Please don't write: while (++timeout) { That doesn't make sense as a loop. It looks like you are trying to loop UINT_MAX times. Put the ++ and the limit on the same line. There is only one real bug in my review but there is just a lot of clean up left. Can you have a co-worker review your patch before resending? The patch 1/6 looks pretty good now but I haven't seen patches 2-5 so I'm worried there is a lot of cleanup left to do. regards, dan carpenter
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 >>
On Tue, Nov 19, 2019 at 07:33:08PM +0530, vishnu wrote: > > > > 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. > You misunderstood the warning. It is about putting declarations after the start of code. (Initializers don't count as code in this context). regards, dan carpenter
On Tue, Nov 19, 2019 at 04:34:16PM +0300, Dan Carpenter wrote: > There is only one real bug in my review but there is just a lot of clean > up left. Can you have a co-worker review your patch before resending? > The patch 1/6 looks pretty good now but I haven't seen patches 2-5 so > I'm worried there is a lot of cleanup left to do. Please, yes - there's a *lot* of fairly minor problems that are coming up in review each time before I've even had a chance to glance at it. You might also want to consider looking to make smaller, more incremental changes towards the goal (eg, refactoring the drivers in ways that will allow multiple instances more easily) which will be easier for both you and reviewers to check thoroughly.
diff --git a/sound/soc/amd/raven/acp3x-pcm-dma.c b/sound/soc/amd/raven/acp3x-pcm-dma.c index 819ec3a..cae1a0f 100644 --- a/sound/soc/amd/raven/acp3x-pcm-dma.c +++ b/sound/soc/amd/raven/acp3x-pcm-dma.c @@ -58,106 +58,6 @@ static const struct snd_pcm_hardware acp3x_pcm_hardware_capture = { .periods_max = CAPTURE_MAX_NUM_PERIODS, }; -static int acp3x_power_on(void __iomem *acp3x_base, bool on) -{ - u16 val, mask; - u32 timeout; - - if (on == true) { - val = 1; - mask = ACP3x_POWER_ON; - } else { - val = 0; - mask = ACP3x_POWER_OFF; - } - - rv_writel(val, acp3x_base + mmACP_PGFSM_CONTROL); - timeout = 0; - while (true) { - val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); - if ((val & ACP3x_POWER_OFF_IN_PROGRESS) == mask) - break; - if (timeout > 100) { - pr_err("ACP3x power state change failure\n"); - return -ENODEV; - } - timeout++; - cpu_relax(); - } - return 0; -} - -static int acp3x_reset(void __iomem *acp3x_base) -{ - u32 val, timeout; - - rv_writel(1, acp3x_base + mmACP_SOFT_RESET); - timeout = 0; - while (true) { - val = rv_readl(acp3x_base + mmACP_SOFT_RESET); - if ((val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) || - timeout > 100) { - if (val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) - break; - return -ENODEV; - } - timeout++; - cpu_relax(); - } - - rv_writel(0, acp3x_base + mmACP_SOFT_RESET); - timeout = 0; - while (true) { - val = rv_readl(acp3x_base + mmACP_SOFT_RESET); - if (!val || timeout > 100) { - if (!val) - break; - return -ENODEV; - } - timeout++; - cpu_relax(); - } - return 0; -} - -static int acp3x_init(void __iomem *acp3x_base) -{ - int ret; - - /* power on */ - ret = acp3x_power_on(acp3x_base, true); - 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_on(acp3x_base, false); - if (ret) { - pr_err("ACP3x power off failed\n"); - return ret; - } - return 0; -} - static irqreturn_t i2s_irq_handler(int irq, void *dev_id) { struct i2s_dev_data *rv_i2s_data; @@ -535,53 +435,28 @@ static int acp3x_audio_probe(struct platform_device *pdev) adata->i2s_irq = res->start; dev_set_drvdata(&pdev->dev, adata); - /* Initialize ACP */ - status = acp3x_init(adata->acp3x_base); - if (status) - return -ENODEV; - status = devm_snd_soc_register_component(&pdev->dev, &acp3x_i2s_component, NULL, 0); if (status) { dev_err(&pdev->dev, "Fail to register acp i2s component\n"); - ret = -ENODEV; - goto dev_err; + return -ENODEV; } status = devm_request_irq(&pdev->dev, adata->i2s_irq, i2s_irq_handler, irqflags, "ACP3x_I2S_IRQ", adata); if (status) { dev_err(&pdev->dev, "ACP3x I2S IRQ request failed\n"); - ret = -ENODEV; - goto dev_err; + return -ENODEV; } pm_runtime_set_autosuspend_delay(&pdev->dev, 5000); pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_enable(&pdev->dev); return 0; - -dev_err: - status = acp3x_deinit(adata->acp3x_base); - if (status) - dev_err(&pdev->dev, "ACP de-init failed\n"); - else - dev_dbg(&pdev->dev, "ACP de-initialized\n"); - return ret; } static int acp3x_audio_remove(struct platform_device *pdev) { - struct i2s_dev_data *adata; - int ret; - - adata = dev_get_drvdata(&pdev->dev); - ret = acp3x_deinit(adata->acp3x_base); - if (ret) - dev_err(&pdev->dev, "ACP de-init failed\n"); - else - dev_dbg(&pdev->dev, "ACP de-initialized\n"); - pm_runtime_disable(&pdev->dev); return 0; } @@ -589,13 +464,9 @@ static int acp3x_audio_remove(struct platform_device *pdev) static int acp3x_resume(struct device *dev) { struct i2s_dev_data *adata; - int status; u32 val; adata = dev_get_drvdata(dev); - status = acp3x_init(adata->acp3x_base); - if (status) - return -ENODEV; if (adata->play_stream && adata->play_stream->runtime) { struct i2s_stream_instance *rtd = @@ -642,14 +513,8 @@ static int acp3x_resume(struct device *dev) static int acp3x_pcm_runtime_suspend(struct device *dev) { struct i2s_dev_data *adata; - int status; 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"); rv_writel(0, adata->acp3x_base + mmACP_EXTERNAL_INTR_ENB); @@ -659,12 +524,9 @@ static int acp3x_pcm_runtime_suspend(struct device *dev) static int acp3x_pcm_runtime_resume(struct device *dev) { struct i2s_dev_data *adata; - int status; adata = dev_get_drvdata(dev); - status = acp3x_init(adata->acp3x_base); - if (status) - return -ENODEV; + rv_writel(1, adata->acp3x_base + mmACP_EXTERNAL_INTR_ENB); return 0; } diff --git a/sound/soc/amd/raven/acp3x.h b/sound/soc/amd/raven/acp3x.h index 01b283a..cf16ceb 100644 --- a/sound/soc/amd/raven/acp3x.h +++ b/sound/soc/amd/raven/acp3x.h @@ -65,6 +65,13 @@ #define SLOT_WIDTH_16 0x10 #define SLOT_WIDTH_24 0x18 #define SLOT_WIDTH_32 0x20 +#define ACP_PGFSM_CNTL_POWER_ON_MASK 0x01 +#define ACP_PGFSM_CNTL_POWER_OFF_MASK 0x00 +#define ACP_PGFSM_STATUS_MASK 0x03 +#define ACP_POWERED_ON 0x00 +#define ACP_POWER_ON_IN_PROGRESS 0x01 +#define ACP_POWERED_OFF 0x02 +#define ACP_POWER_OFF_IN_PROGRESS 0x03 struct acp3x_platform_info { u16 play_i2s_instance; diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c index 94f5f21..defbc44 100644 --- a/sound/soc/amd/raven/pci-acp3x.c +++ b/sound/soc/amd/raven/pci-acp3x.c @@ -9,6 +9,9 @@ #include <linux/io.h> #include <linux/platform_device.h> #include <linux/interrupt.h> +#include <linux/pm_runtime.h> +#include <linux/delay.h> +#include <sound/pcm.h> #include "acp3x.h" @@ -19,6 +22,125 @@ struct acp3x_dev_data { struct platform_device *pdev[ACP3x_DEVS]; }; +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) { + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); + if (!val) + break; + udelay(1); + if (timeout > 500) { + pr_err("ACP is Not Powered ON\n"); + return -ETIMEDOUT; + } + } + return 0; +} + +static int acp3x_power_off(void __iomem *acp3x_base) +{ + u32 val; + u32 timeout, ret; + + timeout = 0; + rv_writel(ACP_PGFSM_CNTL_POWER_OFF_MASK, + acp3x_base + mmACP_PGFSM_CONTROL); + while (++timeout) { + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); + if ((val & ACP_PGFSM_STATUS_MASK) == ACP_POWERED_OFF) { + ret = 0; + break; + } + 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) { + if (val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) + break; + 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"); 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) +{ + int status; + 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) +{ + 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, +}; + static void snd_acp3x_remove(struct pci_dev *pci) { - struct acp3x_dev_data *adata = pci_get_drvdata(pci); - 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++) 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"); + pm_runtime_disable(&pci->dev); + pm_runtime_get_noresume(&pci->dev); pci_disable_msi(pci); pci_release_regions(pci); pci_disable_device(pci); @@ -177,6 +356,9 @@ static struct pci_driver acp3x_driver = { .id_table = snd_acp3x_ids, .probe = snd_acp3x_probe, .remove = snd_acp3x_remove, + .driver = { + .pm = &acp3x_pm, + } }; module_pci_driver(acp3x_driver);
When system wide suspend happens, ACP will be powered off and when system resumes,for audio usecase to continue,all the runtime configuration data needs to be programmed again. Added resume pm call back to ACP pm ops and also added runtime PM operations for ACP3x PCM platform device. Device will enter into D3 state when there is no activity on audio I2S lines. Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com> --- sound/soc/amd/raven/acp3x-pcm-dma.c | 144 +-------------------------- sound/soc/amd/raven/acp3x.h | 7 ++ sound/soc/amd/raven/pci-acp3x.c | 188 +++++++++++++++++++++++++++++++++++- 3 files changed, 195 insertions(+), 144 deletions(-)