| Message ID | 20230214161435.1088246-1-claudiu.beznea@microchip.com |
|---|---|
| Headers | show |
| Series | ASoC: mchp-pdmc: fix poc noises when starting capture | expand |
On 2/14/23 08:14, Claudiu Beznea wrote: > diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c > index 3b99f619e37e..264e87af6b58 100644 > --- a/sound/soc/soc-generic-dmaengine-pcm.c > +++ b/sound/soc/soc-generic-dmaengine-pcm.c > @@ -318,7 +318,7 @@ static int dmaengine_copy_user(struct snd_soc_component *component, > return 0; > } > > -static const struct snd_soc_component_driver dmaengine_pcm_component = { > +static struct snd_soc_component_driver dmaengine_pcm_component = { > .name = SND_DMAENGINE_PCM_DRV_NAME, > .probe_order = SND_SOC_COMP_ORDER_LATE, > .open = dmaengine_pcm_open, > @@ -329,7 +329,7 @@ static const struct snd_soc_component_driver dmaengine_pcm_component = { > .pcm_construct = dmaengine_pcm_new, > }; > > -static const struct snd_soc_component_driver dmaengine_pcm_component_process = { > +static struct snd_soc_component_driver dmaengine_pcm_component_process = { > .name = SND_DMAENGINE_PCM_DRV_NAME, > .probe_order = SND_SOC_COMP_ORDER_LATE, > .open = dmaengine_pcm_open, > @@ -425,7 +425,7 @@ static const struct snd_dmaengine_pcm_config snd_dmaengine_pcm_default_config = > int snd_dmaengine_pcm_register(struct device *dev, > const struct snd_dmaengine_pcm_config *config, unsigned int flags) > { > - const struct snd_soc_component_driver *driver; > + struct snd_soc_component_driver *driver; > struct dmaengine_pcm *pcm; > int ret; > > @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, > else > driver = &dmaengine_pcm_component; > > + driver->start_dma_last = config->start_dma_last; This will break if you have multiple sound cards in the system. dmaengine_pcm_component must stay const.
On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote: > On 2/14/23 08:14, Claudiu Beznea wrote: > > @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, > > else > > driver = &dmaengine_pcm_component; > > + driver->start_dma_last = config->start_dma_last; > This will break if you have multiple sound cards in the system. > dmaengine_pcm_component must stay const. Right, if we need to modify it we either need to select which of multiple const structs to register or to take a copy and modify that. I've not looked at the actual changes yet.
On 14.02.2023 23:26, Mark Brown wrote: > On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote: >> On 2/14/23 08:14, Claudiu Beznea wrote: >>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, >>> else >>> driver = &dmaengine_pcm_component; >>> + driver->start_dma_last = config->start_dma_last; >> This will break if you have multiple sound cards in the system. >> dmaengine_pcm_component must stay const. > Right, if we need to modify it we either need to select which of > multiple const structs to register or to take a copy and modify > that. I've not looked at the actual changes yet. OK, I will try that and return with a new patch. On the other hand do you think the other solution presented in cover letter would be better? From the cover letter: "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)." Thank you, Claudiu
On 2/16/23 01:49, Claudiu.Beznea@microchip.com wrote: > On 14.02.2023 23:26, Mark Brown wrote: >> On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote: >>> On 2/14/23 08:14, Claudiu Beznea wrote: >>>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, >>>> else >>>> driver = &dmaengine_pcm_component; >>>> + driver->start_dma_last = config->start_dma_last; >>> This will break if you have multiple sound cards in the system. >>> dmaengine_pcm_component must stay const. >> Right, if we need to modify it we either need to select which of >> multiple const structs to register or to take a copy and modify >> that. I've not looked at the actual changes yet. > OK, I will try that and return with a new patch. > > On the other hand do you think the other solution presented in cover letter > would be better? From the cover letter: > > "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)." > Can't you just set `start_dma_last` on the `mchp_pdmc_dai_component`? In your code you iterate over all the components of the link and if any of them has it set the DMA is started last.
On 16.02.2023 15:53, Lars-Peter Clausen wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On 2/16/23 01:49, Claudiu.Beznea@microchip.com wrote: >> On 14.02.2023 23:26, Mark Brown wrote: >>> On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote: >>>> On 2/14/23 08:14, Claudiu Beznea wrote: >>>>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev, >>>>> else >>>>> driver = &dmaengine_pcm_component; >>>>> + driver->start_dma_last = config->start_dma_last; >>>> This will break if you have multiple sound cards in the system. >>>> dmaengine_pcm_component must stay const. >>> Right, if we need to modify it we either need to select which of >>> multiple const structs to register or to take a copy and modify >>> that. I've not looked at the actual changes yet. >> OK, I will try that and return with a new patch. >> >> On the other hand do you think the other solution presented in cover letter >> would be better? From the cover letter: >> >> "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)." >> > Can't you just set `start_dma_last` on the `mchp_pdmc_dai_component`? In > your code you iterate over all the components of the link and if any of > them has it set the DMA is started last. Yes, that is also working. In this patch I chose to have it on DMA component as the operation is specific to DMA... It looked better to me this way. But is true that having it on mchp_pdmc_dai_component wouldn't affect the behavior. Thank you, Claudiu
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, driver sets its struct snd_dmaengine_pcm_config::start_dma_last = 1 and proper action is taken based on this flag when starting DAI trigger vs DMA. 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). Please advice what might be the best approach. Thank you, Claudiu Beznea Claudiu Beznea (3): ASoC: soc-generic-dmaengine-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/dmaengine_pcm.h | 1 + include/sound/soc-component.h | 2 + sound/soc/atmel/mchp-pdmc.c | 55 +++++++++++++++++-- sound/soc/soc-generic-dmaengine-pcm.c | 8 ++- sound/soc/soc-pcm.c | 27 +++++++-- 6 files changed, 86 insertions(+), 13 deletions(-)