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

[Syed Saba Kareem <ssabakar@amd.com>]

On 8/12/22 19:50, Amadeusz Sławiński wrote:
> [CAUTION: External Email]
>
> 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.
>
Okay Will fix it.
>> +     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?
>
Will fix it.
>> +     return ret;
>> +}
>> +
>
> ...
>