> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Tuesday, May 12, 2020 3:42 AM
> To: Alex Deucher <alexdeucher@xxxxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx;
> broonie@xxxxxxxxxx; Mukunda, Vijendar <Vijendar.Mukunda@xxxxxxx>;
> tiwai@xxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> 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.
