| Message ID | 20230217124151.236216-1-claudiu.beznea@microchip.com |
|---|---|
| Headers | show |
| Series | ASoC: mchp-pdmc: fix poc noises when starting capture | expand |
On 2/17/2023 1:41 PM, Claudiu Beznea wrote: > Add option to start DMA component after DAI trigger. This is done > by filling the new struct snd_soc_component_driver::start_dma_last. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > include/sound/soc-component.h | 2 ++ > sound/soc/soc-pcm.c | 27 ++++++++++++++++++++++----- > 2 files changed, 24 insertions(+), 5 deletions(-) > > diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h > index 3203d35bc8c1..0814ed143864 100644 > --- a/include/sound/soc-component.h > +++ b/include/sound/soc-component.h > @@ -190,6 +190,8 @@ struct snd_soc_component_driver { > bool use_dai_pcm_id; /* use DAI link PCM ID as PCM device number */ > int be_pcm_base; /* base device ID for all BE PCMs */ > > + unsigned int start_dma_last; > + > #ifdef CONFIG_DEBUG_FS > const char *debugfs_prefix; > #endif > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 005b179a770a..5eb056b942ce 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -1088,22 +1088,39 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, > static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd) > { > struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); > - int ret = -EINVAL, _ret = 0; > + struct snd_soc_component *component; > + int ret = -EINVAL, _ret = 0, start_dma_last = 0, i; > int rollback = 0; > > switch (cmd) { > case SNDRV_PCM_TRIGGER_START: > case SNDRV_PCM_TRIGGER_RESUME: > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + /* Do we need to start dma last? */ > + for_each_rtd_components(rtd, i, component) { > + if (component->driver->start_dma_last) { > + start_dma_last = 1; > + break; > + } > + } > + > ret = snd_soc_link_trigger(substream, cmd, 0); > if (ret < 0) > goto start_err; > > - ret = snd_soc_pcm_component_trigger(substream, cmd, 0); > - if (ret < 0) > - goto start_err; > + if (start_dma_last) { > + ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); > + if (ret < 0) > + goto start_err; > + > + ret = snd_soc_pcm_component_trigger(substream, cmd, 0); > + } else { > + ret = snd_soc_pcm_component_trigger(substream, cmd, 0); > + if (ret < 0) > + goto start_err; > > - ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); > + ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); > + } > start_err: > if (ret < 0) > rollback = 1; Can all of the above be implemented similarly to already present stop_dma_first? It looks similar and I don't see reason to have one flag in snd_soc_component_driver and other in snd_soc_dai_link.
On 17.02.2023 15:09, Amadeusz Sławiński wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On 2/17/2023 1:41 PM, Claudiu Beznea wrote: >> Add option to start DMA component after DAI trigger. This is done >> by filling the new struct snd_soc_component_driver::start_dma_last. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> include/sound/soc-component.h | 2 ++ >> sound/soc/soc-pcm.c | 27 ++++++++++++++++++++++----- >> 2 files changed, 24 insertions(+), 5 deletions(-) >> >> diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h >> index 3203d35bc8c1..0814ed143864 100644 >> --- a/include/sound/soc-component.h >> +++ b/include/sound/soc-component.h >> @@ -190,6 +190,8 @@ struct snd_soc_component_driver { >> bool use_dai_pcm_id; /* use DAI link PCM ID as PCM device number */ >> int be_pcm_base; /* base device ID for all BE PCMs */ >> >> + unsigned int start_dma_last; >> + >> #ifdef CONFIG_DEBUG_FS >> const char *debugfs_prefix; >> #endif >> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c >> index 005b179a770a..5eb056b942ce 100644 >> --- a/sound/soc/soc-pcm.c >> +++ b/sound/soc/soc-pcm.c >> @@ -1088,22 +1088,39 @@ static int soc_pcm_hw_params(struct >> snd_pcm_substream *substream, >> static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd) >> { >> struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); >> - int ret = -EINVAL, _ret = 0; >> + struct snd_soc_component *component; >> + int ret = -EINVAL, _ret = 0, start_dma_last = 0, i; >> int rollback = 0; >> >> switch (cmd) { >> case SNDRV_PCM_TRIGGER_START: >> case SNDRV_PCM_TRIGGER_RESUME: >> case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: >> + /* Do we need to start dma last? */ >> + for_each_rtd_components(rtd, i, component) { >> + if (component->driver->start_dma_last) { >> + start_dma_last = 1; >> + break; >> + } >> + } >> + >> ret = snd_soc_link_trigger(substream, cmd, 0); >> if (ret < 0) >> goto start_err; >> >> - ret = snd_soc_pcm_component_trigger(substream, cmd, 0); >> - if (ret < 0) >> - goto start_err; >> + if (start_dma_last) { >> + ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); >> + if (ret < 0) >> + goto start_err; >> + >> + ret = snd_soc_pcm_component_trigger(substream, cmd, >> 0); >> + } else { >> + ret = snd_soc_pcm_component_trigger(substream, cmd, >> 0); >> + if (ret < 0) >> + goto start_err; >> >> - ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); >> + ret = snd_soc_pcm_dai_trigger(substream, cmd, 0); >> + } >> start_err: >> if (ret < 0) >> rollback = 1; > > Can all of the above be implemented similarly to already present > stop_dma_first? It looks similar and I don't see reason to have one flag > in snd_soc_component_driver and other in snd_soc_dai_link. That was the other solution identified; I mentioned it in v1; from v1 cover letter/discussions: The other solution that was identified for this was to extend the already existing mechanism around struct snd_soc_dai_link::stop_dma_first. The downside of this was that a potential struct snd_soc_dai_link::start_dma_last would have to be populated on sound card driver thus, had to be taken into account in all sound card drivers. At the moment, the mchp-pdmc is used only with simple-audio-card. In case of simple-audio-card a new DT binding would had to be introduced to specify this action on dai-link descriptions (as of my investigation). > >
To start capture on Microchip PDMC the enable bits for each supported microphone need to be set. After this bit is set the PDMC starts to receive data from microphones and it considers this data as valid data. Thus if microphones are not ready the PDMC captures anyway data from its lines. This data is interpreted by the human ear as poc noises. To avoid this the following software workaround need to be applied when starting capture: 1/ enable PDMC channel 2/ wait 150ms 3/ execute 16 dummy reads from RHR 4/ clear interrupts 5/ enable interrupts 6/ enable DMA channel For this workaround to work step 6 need to be executed at the end. For step 6 was added patch 1/3 from this series. With this, component DAI driver sets its struct snd_soc_component_driver::start_dma_last = 1 and proper action is taken based on this flag when starting DAI trigger vs DMA. Thank you, Claudiu Beznea Changes in v2: - patch 1/3 from v1 is now "ASoC: soc-pcm: add option to start DMA after DAI" - pass start_dma_last from component DAI driver object (struct snd_soc_component_driver::start_dma_last); adapt patch 3/3 after this; - in patch 1/3 s/Do we need to start dma first/Do we need to start dma last in comment from soc_pcm_trigger() - collect review tag from Krzysztof Claudiu Beznea (3): ASoC: soc-pcm: add option to start DMA after DAI ASoC: dt-bindings: sama7g5-pdmc: add microchip,startup-delay-us binding ASoC: mchp-pdmc: fix poc noise at capture startup .../sound/microchip,sama7g5-pdmc.yaml | 6 ++ include/sound/soc-component.h | 2 + sound/soc/atmel/mchp-pdmc.c | 55 +++++++++++++++++-- sound/soc/soc-pcm.c | 27 +++++++-- 4 files changed, 80 insertions(+), 10 deletions(-)