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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux