Re: [PATCH 07/13] ASoC: amd: add acp6.2 pdm driver dma ops

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 8/12/2022 2:07 PM, Syed Saba kareem wrote:
> This patch adds PDM driver DMA operations for Pink Sardine Platform.
> 
> Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
> ---
>   sound/soc/amd/ps/acp62.h      |  41 +++++
>   sound/soc/amd/ps/ps-pdm-dma.c | 310 ++++++++++++++++++++++++++++++++++
>   2 files changed, 351 insertions(+)
> 
> diff --git a/sound/soc/amd/ps/acp62.h b/sound/soc/amd/ps/acp62.h
> index 563252834b09..525e8bd225a2 100644
> --- a/sound/soc/amd/ps/acp62.h
> +++ b/sound/soc/amd/ps/acp62.h
> @@ -27,6 +27,31 @@
>   #define ACP_EXT_INTR_STAT_CLEAR_MASK 0xFFFFFFFF
>   #define PDM_DMA_STAT 0x10
>   
> +#define PDM_DMA_INTR_MASK	0x10000
> +#define ACP_ERROR_STAT	29
> +#define PDM_DECIMATION_FACTOR	2
> +#define ACP_PDM_CLK_FREQ_MASK	7
> +#define ACP_WOV_MISC_CTRL_MASK	0x10
> +#define ACP_PDM_ENABLE		1
> +#define ACP_PDM_DISABLE		0
> +#define ACP_PDM_DMA_EN_STATUS	2
> +#define TWO_CH		2
> +#define DELAY_US	5
> +#define ACP_COUNTER	20000
> +
> +#define ACP_SRAM_PTE_OFFSET	0x03800000
> +#define PAGE_SIZE_4K_ENABLE	2
> +#define PDM_PTE_OFFSET		0
> +#define PDM_MEM_WINDOW_START	0x4000000
> +
> +#define CAPTURE_MIN_NUM_PERIODS     4
> +#define CAPTURE_MAX_NUM_PERIODS     4
> +#define CAPTURE_MAX_PERIOD_SIZE     8192
> +#define CAPTURE_MIN_PERIOD_SIZE     4096
> +
> +#define MAX_BUFFER (CAPTURE_MAX_PERIOD_SIZE * CAPTURE_MAX_NUM_PERIODS)
> +#define MIN_BUFFER MAX_BUFFER
> +
>   enum acp_config {
>   	ACP_CONFIG_0 = 0,
>   	ACP_CONFIG_1,
> @@ -46,6 +71,22 @@ enum acp_config {
>   	ACP_CONFIG_15,
>   };
>   
> +struct pdm_stream_instance {
> +	u16 num_pages;
> +	u16 channels;
> +	dma_addr_t dma_addr;
> +	u64 bytescount;
> +	void __iomem *acp62_base;
> +};
> +
> +union acp_pdm_dma_count {
> +	struct {
> +	u32 low;
> +	u32 high;
> +	} bcount;

Fix indentation.
Also you can remove struct name, and access fields directly, so instead 
of accessing somevariable.bcount.low, you would use somevariable.low, it 
would probably be more readable.

> +	u64 bytescount;
> +};
> +
>   struct pdm_dev_data {
>   	u32 pdm_irq;
>   	void __iomem *acp62_base;
> diff --git a/sound/soc/amd/ps/ps-pdm-dma.c b/sound/soc/amd/ps/ps-pdm-dma.c
> index bca2ac3e81f5..a98a2015015d 100644

...

> +
> +static int acp62_pdm_dma_open(struct snd_soc_component *component,
> +			      struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime;
> +	struct pdm_dev_data *adata;
> +	struct pdm_stream_instance *pdm_data;
> +	int ret;
> +
> +	runtime = substream->runtime;
> +	adata = dev_get_drvdata(component->dev);
> +	pdm_data = kzalloc(sizeof(*pdm_data), GFP_KERNEL);
> +	if (!pdm_data)
> +		return -EINVAL;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		runtime->hw = acp62_pdm_hardware_capture;
> +
> +	ret = snd_pcm_hw_constraint_integer(runtime,
> +					    SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0) {
> +		dev_err(component->dev, "set integer constraint failed\n");
> +		kfree(pdm_data);
> +		return ret;
> +	}
> +
> +	acp62_enable_pdm_interrupts(adata->acp62_base);
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		adata->capture_stream = substream;
> +
> +	pdm_data->acp62_base = adata->acp62_base;
> +	runtime->private_data = pdm_data;

Should probably kfree(runtime->private_data) in acp62_pdm_dma_close(), 
otherwise won't there be a memory leak?

> +	return ret;
> +}
> +

...