On 18/10/19 4:09 PM, Dan Carpenter wrote:
> 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.
Will address this thanks.
>
> 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.
Will address this thanks.
>
> 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.
>
Will address this thanks.
>> + 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.
>
Will address this thanks.
>> +}
>> +
>> +
>> +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.
Actually have the sequence like that.
First we have to set mmACP_SOFT_RESET with 1 then we need to check its
effect from reading same register and wait till timeout if error occurs
and then immediatly we need to reset with 0 and read the reset register
if it is not reset we will wait till timeout and exit with error.
Will address this by changing the duplication code thanks.
>
>> + 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 ||.
>
Will address this thanks.
>> + 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?
>
Will address this thanks.
>> +
>>
>> 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.
>
Will address this thanks.
>> + 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
>
Regards,
Vishnu
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
