From: Dan Carpenter <dan.carpenter@oracle.com> To: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com> Cc: Alexander.Deucher@amd.com, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, Vijendar Mukunda <Vijendar.Mukunda@amd.com>, Maruthi Bayyavarapu <maruthi.bayyavarapu@amd.com>, Colin Ian King <colin.king@canonical.com>, YueHaibing <yuehaibing@huawei.com>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, Sanju R Mehta <sanju.mehta@amd.com>, "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." <alsa-devel@alsa-project.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 6/7] ASoC: amd: ACP powergating should be done by controller Date: Fri, 18 Oct 2019 13:39:10 +0300 [thread overview] Message-ID: <20191018103910.GL21344@kadam> (raw) In-Reply-To: <1571432760-3008-6-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com> On Sat, Oct 19, 2019 at 02:35:44AM +0530, Ravulapati Vishnu vardhan rao wrote: > diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c > index 7f435b3..b74ecf6 100644 > --- a/sound/soc/amd/raven/pci-acp3x.c > +++ b/sound/soc/amd/raven/pci-acp3x.c > @@ -19,11 +19,140 @@ struct acp3x_dev_data { > struct platform_device *pdev[ACP3x_DEVS]; > }; > > +static int acp3x_power_on(void __iomem *acp3x_base) > +{ > + u32 val; > + u32 timeout = 0; > + int ret = 0; > + > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + if (val) { Just flip this around. if (val == 0) return 0; > + 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 (true) { while (++timeout < 500) { > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + if (!val) > + break; return 0; > + udelay(1); > + if (timeout > 500) { > + pr_err("ACP is Not Powered ON\n"); > + ret = -ETIMEDOUT; > + break; > + } > + timeout++; > + } > + if (ret) { > + pr_err("ACP is not powered on status:%d\n", ret); Just one error message is enough. pr_err("ACP is Not Powered ON\n"); return -ETIMEDOUT; > + return ret; > + } > + } > + return ret; > +} > + > +static int acp3x_power_off(void __iomem *acp3x_base) > +{ > + u32 val; > + u32 timeout = 0; > + int ret = 0; > + > + val = rv_readl(acp3x_base + mmACP_PGFSM_CONTROL); val is not used. We want to turn on set but not used warnings eventually. > + rv_writel(ACP_PGFSM_CNTL_POWER_OFF_MASK, > + acp3x_base + mmACP_PGFSM_CONTROL); > + while (true) { > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + if ((val & ACP_PGFSM_STATUS_MASK) == ACP_POWERED_OFF) > + break; > + udelay(1); > + if (timeout > 500) { > + pr_err("ACP is Not Powered OFF\n"); > + ret = -ETIMEDOUT; > + break; > + } > + timeout++; > + } > + if (ret) > + pr_err("ACP is not powered off status:%d\n", ret); > + return ret; Same as above. > +} > + > + > +static int acp3x_reset(void __iomem *acp3x_base) > +{ > + u32 val, timeout; > + > + rv_writel(1, acp3x_base + mmACP_SOFT_RESET); > + timeout = 0; > + while (true) { while (++timeout < 100) { > + 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; Duplicated needlessly. > + return -ENODEV; > + } > + timeout++; > + cpu_relax(); > + } if (timeout == 100) return -ENODEV; > + rv_writel(0, acp3x_base + mmACP_SOFT_RESET); > + timeout = 0; > + while (true) { > + val = rv_readl(acp3x_base + mmACP_SOFT_RESET); Split the "if (!val) break;" into it's own condition instead of part of the ||. > + 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); > + 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) > { > int ret; > - u32 addr, val, i; > + u32 addr, val, i, status; > struct acp3x_dev_data *adata; > struct platform_device_info pdevinfo[ACP3x_DEVS]; > unsigned int irqflags; > @@ -63,6 +192,10 @@ static int snd_acp3x_probe(struct pci_dev *pci, > } > pci_set_master(pci); > pci_set_drvdata(pci, adata); > + status = acp3x_init(adata->acp3x_base); > + if (status) > + return -ENODEV; Why do we need both "status" and "ret". Preserve the error code? > + > > val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG); > switch (val) { > @@ -132,6 +265,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, > return 0; > > unmap_mmio: > + status = acp3x_deinit(adata->acp3x_base); > + if (status) > + dev_err(&pci->dev, "ACP de-init failed\n"); > + else > + dev_info(&pci->dev, "ACP de-initialized\n"); > for (i = 0 ; i < ACP3x_DEVS ; i++) > platform_device_unregister(adata->pdev[i]); > kfree(adata->res); > @@ -153,6 +291,11 @@ static void snd_acp3x_remove(struct pci_dev *pci) > for (i = 0 ; i < ACP3x_DEVS ; i++) > platform_device_unregister(adata->pdev[i]); > } > + i = acp3x_deinit(adata->acp3x_base); Please don't re-use "i" like this. Declare "ret" or "status" or something. > + if (i) > + dev_err(&pci->dev, "ACP de-init failed\n"); > + else > + dev_info(&pci->dev, "ACP de-initialized\n"); > iounmap(adata->acp3x_base); > > pci_disable_msi(pci); regards, dan carpenter
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com> To: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com> Cc: "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." <alsa-devel@alsa-project.org>, Maruthi Bayyavarapu <maruthi.bayyavarapu@amd.com>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, open list <linux-kernel@vger.kernel.org>, YueHaibing <yuehaibing@huawei.com>, Takashi Iwai <tiwai@suse.com>, Liam Girdwood <lgirdwood@gmail.com>, Sanju R Mehta <sanju.mehta@amd.com>, Mark Brown <broonie@kernel.org>, Vijendar Mukunda <Vijendar.Mukunda@amd.com>, Alexander.Deucher@amd.com, Colin Ian King <colin.king@canonical.com> Subject: Re: [alsa-devel] [PATCH 6/7] ASoC: amd: ACP powergating should be done by controller Date: Fri, 18 Oct 2019 13:39:10 +0300 [thread overview] Message-ID: <20191018103910.GL21344@kadam> (raw) In-Reply-To: <1571432760-3008-6-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com> On Sat, Oct 19, 2019 at 02:35:44AM +0530, Ravulapati Vishnu vardhan rao wrote: > diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c > index 7f435b3..b74ecf6 100644 > --- a/sound/soc/amd/raven/pci-acp3x.c > +++ b/sound/soc/amd/raven/pci-acp3x.c > @@ -19,11 +19,140 @@ struct acp3x_dev_data { > struct platform_device *pdev[ACP3x_DEVS]; > }; > > +static int acp3x_power_on(void __iomem *acp3x_base) > +{ > + u32 val; > + u32 timeout = 0; > + int ret = 0; > + > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + if (val) { Just flip this around. if (val == 0) return 0; > + 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 (true) { while (++timeout < 500) { > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + if (!val) > + break; return 0; > + udelay(1); > + if (timeout > 500) { > + pr_err("ACP is Not Powered ON\n"); > + ret = -ETIMEDOUT; > + break; > + } > + timeout++; > + } > + if (ret) { > + pr_err("ACP is not powered on status:%d\n", ret); Just one error message is enough. pr_err("ACP is Not Powered ON\n"); return -ETIMEDOUT; > + return ret; > + } > + } > + return ret; > +} > + > +static int acp3x_power_off(void __iomem *acp3x_base) > +{ > + u32 val; > + u32 timeout = 0; > + int ret = 0; > + > + val = rv_readl(acp3x_base + mmACP_PGFSM_CONTROL); val is not used. We want to turn on set but not used warnings eventually. > + rv_writel(ACP_PGFSM_CNTL_POWER_OFF_MASK, > + acp3x_base + mmACP_PGFSM_CONTROL); > + while (true) { > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + if ((val & ACP_PGFSM_STATUS_MASK) == ACP_POWERED_OFF) > + break; > + udelay(1); > + if (timeout > 500) { > + pr_err("ACP is Not Powered OFF\n"); > + ret = -ETIMEDOUT; > + break; > + } > + timeout++; > + } > + if (ret) > + pr_err("ACP is not powered off status:%d\n", ret); > + return ret; Same as above. > +} > + > + > +static int acp3x_reset(void __iomem *acp3x_base) > +{ > + u32 val, timeout; > + > + rv_writel(1, acp3x_base + mmACP_SOFT_RESET); > + timeout = 0; > + while (true) { while (++timeout < 100) { > + 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; Duplicated needlessly. > + return -ENODEV; > + } > + timeout++; > + cpu_relax(); > + } if (timeout == 100) return -ENODEV; > + rv_writel(0, acp3x_base + mmACP_SOFT_RESET); > + timeout = 0; > + while (true) { > + val = rv_readl(acp3x_base + mmACP_SOFT_RESET); Split the "if (!val) break;" into it's own condition instead of part of the ||. > + 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); > + 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) > { > int ret; > - u32 addr, val, i; > + u32 addr, val, i, status; > struct acp3x_dev_data *adata; > struct platform_device_info pdevinfo[ACP3x_DEVS]; > unsigned int irqflags; > @@ -63,6 +192,10 @@ static int snd_acp3x_probe(struct pci_dev *pci, > } > pci_set_master(pci); > pci_set_drvdata(pci, adata); > + status = acp3x_init(adata->acp3x_base); > + if (status) > + return -ENODEV; Why do we need both "status" and "ret". Preserve the error code? > + > > val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG); > switch (val) { > @@ -132,6 +265,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, > return 0; > > unmap_mmio: > + status = acp3x_deinit(adata->acp3x_base); > + if (status) > + dev_err(&pci->dev, "ACP de-init failed\n"); > + else > + dev_info(&pci->dev, "ACP de-initialized\n"); > for (i = 0 ; i < ACP3x_DEVS ; i++) > platform_device_unregister(adata->pdev[i]); > kfree(adata->res); > @@ -153,6 +291,11 @@ static void snd_acp3x_remove(struct pci_dev *pci) > for (i = 0 ; i < ACP3x_DEVS ; i++) > platform_device_unregister(adata->pdev[i]); > } > + i = acp3x_deinit(adata->acp3x_base); Please don't re-use "i" like this. Declare "ret" or "status" or something. > + if (i) > + dev_err(&pci->dev, "ACP de-init failed\n"); > + else > + dev_info(&pci->dev, "ACP de-initialized\n"); > iounmap(adata->acp3x_base); > > pci_disable_msi(pci); regards, dan carpenter _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2019-10-18 10:40 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-18 21:05 [PATCH 1/7] ASoC: amd: Create multiple I2S platform device endpoints Ravulapati Vishnu vardhan rao 2019-10-18 21:05 ` [alsa-devel] " Ravulapati Vishnu vardhan rao 2019-10-18 21:05 ` [PATCH 2/7] ASoC: amd: Refactoring of DAI from DMA driver Ravulapati Vishnu vardhan rao 2019-10-18 21:05 ` [alsa-devel] " Ravulapati Vishnu vardhan rao 2019-10-25 7:06 ` vishnu 2019-10-25 7:06 ` [alsa-devel] " vishnu 2019-10-18 21:05 ` [PATCH 3/7] ASoC: amd: Enabling I2S instance in DMA and DAI Ravulapati Vishnu vardhan rao 2019-10-18 21:05 ` [alsa-devel] " Ravulapati Vishnu vardhan rao 2019-10-24 11:40 ` Mark Brown 2019-10-24 11:40 ` [alsa-devel] " Mark Brown 2019-10-25 6:53 ` vishnu 2019-10-25 6:53 ` [alsa-devel] " vishnu 2019-10-25 9:55 ` Mark Brown 2019-10-25 9:55 ` [alsa-devel] " Mark Brown 2019-10-25 10:01 ` vishnu 2019-10-25 10:01 ` [alsa-devel] " vishnu 2019-10-25 6:57 ` vishnu 2019-10-25 6:57 ` [alsa-devel] " vishnu 2019-10-18 21:05 ` [PATCH 4/7] ASoC: amd: add ACP3x TDM mode support Ravulapati Vishnu vardhan rao 2019-10-18 21:05 ` [alsa-devel] " Ravulapati Vishnu vardhan rao 2019-10-25 7:06 ` vishnu 2019-10-25 7:06 ` [alsa-devel] " vishnu 2019-10-18 21:05 ` [PATCH 5/7] ASoC: amd: handle ACP3x i2s-sp watermark interrupt Ravulapati Vishnu vardhan rao 2019-10-18 21:05 ` [alsa-devel] " Ravulapati Vishnu vardhan rao 2019-10-25 7:07 ` vishnu 2019-10-25 7:07 ` [alsa-devel] " vishnu 2019-10-18 21:05 ` [PATCH 6/7] ASoC: amd: ACP powergating should be done by controller Ravulapati Vishnu vardhan rao 2019-10-18 21:05 ` [alsa-devel] " Ravulapati Vishnu vardhan rao 2019-10-18 10:39 ` Dan Carpenter [this message] 2019-10-18 10:39 ` Dan Carpenter 2019-11-06 14:11 ` vishnu 2019-11-06 14:11 ` [alsa-devel] " vishnu 2019-10-18 21:05 ` [PATCH 7/7] ASoC: amd: Added ACP3x system resume and runtime pm ops Ravulapati Vishnu vardhan rao 2019-10-18 21:05 ` [alsa-devel] " Ravulapati Vishnu vardhan rao 2019-10-25 7:05 ` [PATCH 1/7] ASoC: amd: Create multiple I2S platform device endpoints vishnu 2019-10-25 7:05 ` [alsa-devel] " vishnu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191018103910.GL21344@kadam \ --to=dan.carpenter@oracle.com \ --cc=Alexander.Deucher@amd.com \ --cc=Vijendar.Mukunda@amd.com \ --cc=Vishnuvardhanrao.Ravulapati@amd.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=colin.king@canonical.com \ --cc=kuninori.morimoto.gx@renesas.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=maruthi.bayyavarapu@amd.com \ --cc=perex@perex.cz \ --cc=sanju.mehta@amd.com \ --cc=tiwai@suse.com \ --cc=yuehaibing@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.