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

[Syed Saba Kareem]

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@xxxxxxx>
> > ---
> >   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;
> > +}
> > +
>
> ...