Re: [PATCH 6/7] ASoC: amd: ACP powergating should be done by controller

[Dan Carpenter <dan.carpenter@oracle.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