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
