RE: [PATCH v2 08/14] ASoC: amd: add ACP PDM DMA driver dai ops

From: "Mukunda, Vijendar" <Vijendar.Mukunda@amd.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Alex Deucher <alexdeucher@gmail.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"tiwai@suse.de" <tiwai@suse.de>
Cc: "Deucher, Alexander" <Alexander.Deucher@amd.com>
Subject: RE: [PATCH v2 08/14] ASoC: amd: add ACP PDM DMA driver dai ops
Date: Tue, 12 May 2020 16:15:03 +0000	[thread overview]
Message-ID: <DM6PR12MB26338B02D772799482F1A01997BE0@DM6PR12MB2633.namprd12.prod.outlook.com> (raw)
In-Reply-To: <3c1a08a0-31e8-dc0c-a903-190861b7fa11@linux.intel.com>

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Tuesday, May 12, 2020 3:42 AM
> To: Alex Deucher <alexdeucher@gmail.com>; alsa-devel@alsa-project.org;
> broonie@kernel.org; Mukunda, Vijendar <Vijendar.Mukunda@amd.com>;
> tiwai@suse.de
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v2 08/14] ASoC: amd: add ACP PDM DMA driver dai ops
> 
> 
> 
> 
> > +static int start_pdm_dma(void __iomem *acp_base)
> > +{
> > +	u32 pdm_enable;
> > +	u32 pdm_dma_enable;
> > +	int timeout;
> > +
> > +	pdm_enable = 0x01;
> > +	pdm_dma_enable  = 0x01;
> > +
> > +	enable_pdm_clock(acp_base);
> > +	rn_writel(pdm_enable, acp_base + ACP_WOV_PDM_ENABLE);
> > +	rn_writel(pdm_dma_enable, acp_base +
> ACP_WOV_PDM_DMA_ENABLE);
> > +	pdm_dma_enable = 0x00;
> > +	timeout = 0;
> > +	while (++timeout < ACP_COUNTER) {
> > +		pdm_dma_enable = rn_readl(acp_base +
> ACP_WOV_PDM_DMA_ENABLE);
> > +		if ((pdm_dma_enable & 0x02) ==
> ACP_PDM_DMA_EN_STATUS)
> > +			return 0;
> > +		udelay(DELAY_US);
> > +	}
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +static int stop_pdm_dma(void __iomem *acp_base)
> > +{
> > +	u32 pdm_enable, pdm_dma_enable, pdm_fifo_flush;
> > +	int timeout;
> > +
> > +	pdm_enable = 0x00;
> > +	pdm_dma_enable  = 0x00;
> > +	pdm_fifo_flush = 0x00;
> > +
> > +	pdm_enable = rn_readl(acp_base + ACP_WOV_PDM_ENABLE);
> > +	pdm_dma_enable = rn_readl(acp_base +
> ACP_WOV_PDM_DMA_ENABLE);
> > +	if (pdm_dma_enable & 0x01) {
> > +		pdm_dma_enable = 0x02;
> > +		rn_writel(pdm_dma_enable, acp_base +
> ACP_WOV_PDM_DMA_ENABLE);
> > +		pdm_dma_enable = 0x00;
> > +		timeout = 0;
> > +		while (++timeout < ACP_COUNTER) {
> > +			pdm_dma_enable = rn_readl(acp_base +
> > +
> ACP_WOV_PDM_DMA_ENABLE);
> > +			if ((pdm_dma_enable & 0x02) == 0x00)
> > +				break;
> > +			udelay(DELAY_US);
> > +		}
> > +		if (timeout == ACP_COUNTER)
> 
> if you reach this point, timeout is by construction equal to ACP_COUNTER
> so this test is useless

Here we are checking current dma status. 
If DMA enable status bit is set to zero , it will exit the while loop.
It will continue rest of the registers programming to disable PDM
decoder and flushing fifo.

When  this condition "if ((pdm_dma_enable & 0x02) == 0x00)" is true,
"timeout" counter won't be exhausted i.e it won't cross ACP_COUNTER
value.

We have to return time out error only when DMA enable status bit is 
not cleared and timeout reached max value i.e ACP_COUNTER. i.e 
This check " if (timeout == ACP_COUNTER) " required to know the while 
loop exit condition.

> 
> You could also use a helper where you check if the value is
> ACP_PDM_DMA_EN_STATUS or zero so that you don't copy the same logic
> twice.

I believe it's only a single register check, still its hold to be good.