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

[Mukunda, Vijendar]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> Sent: Wednesday, May 6, 2020 3:25 AM
> To: Alex Deucher <alexdeucher at gmail.com>; alsa-devel at alsa-project.org;
> broonie at kernel.org; Mukunda, Vijendar <Vijendar.Mukunda at amd.com>;
> tiwai at suse.de
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH 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 < 20000) {
> > +		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(5);
> > +	}
> 
> maybe use human-readable defines for timeout and delay?
> e.g.
> 
> #define TIMEOUT_MS 100
> #define DELAY_US 5

Will fix it.
> 
> > +	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 < 20000) {
> > +			pdm_dma_enable = rn_readl(acp_base +
> > +
> ACP_WOV_PDM_DMA_ENABLE);
> > +			if ((pdm_dma_enable & 0x02) == 0x00)
> > +				return 0;
> > +			udelay(5);
> > +		}
> > +		if (timeout == 20000)
> 
> if this test needed, it'll be always true, no?

Sorry its my bad. It shouldn't return 0 when ACP DMA transfer is stopped.
It should break the while loop then it should continue to do register programming for stopping the PDM decoder and flushing the FIFO..
timeout check only to verify whether is there any timeout occurred for stopping the PDM DMA transfer or not.

Will modify the logic and  share the updated patch.
> 
> > +			return -ETIMEDOUT;
> > +	}
> > +	if (pdm_enable == ACP_PDM_ENABLE) {
> > +		pdm_enable = ACP_PDM_DISABLE;
> > +		rn_writel(pdm_enable, acp_base + ACP_WOV_PDM_ENABLE);
> > +	}
> > +	rn_writel(0x01, acp_base + ACP_WOV_PDM_FIFO_FLUSH);
> > +	return 0;
> > +}
> > +
> 
> > +static int acp_pdm_dai_hw_params(struct snd_pcm_substream *substream,
> > +				 struct snd_pcm_hw_params *params,
> > +				 struct snd_soc_dai *dai)
> > +{
> > +	struct pdm_stream_instance *rtd;
> > +	unsigned int ch_mask;
> > +
> > +	rtd = substream->runtime->private_data;
> > +	switch (params_channels(params)) {
> > +	case TWO_CH:
> > +	default:
> > +		ch_mask = 0x00;
> > +		break;
> > +	}
> 
> the switch doesn't appear very useful at the moment?

We kept it for future reference to extend support for more than 2 channels.
> 
> > +	config_pdm_stream_params(ch_mask, rtd->acp_base);
> > +	return 0;
> > +}
> > +