Re: [PATCH v2] ASoC: dmic: Add optional wakeup delay

From: Matthias Kaehlcke
Date: Fri Feb 16 2018 - 11:47:52 EST


El Thu, Feb 15, 2018 at 06:47:29PM -0800 Brian Norris ha dit:

> Hi,
>
> On Thu, Feb 15, 2018 at 06:24:16PM -0800, Matthias Kaehlcke wrote:
> > On some systems a delay is needed after switching on the clocks, to allow
> > the output to stabilize and avoid a popping noise at the beginning of
> > the recording. Add the optional device tree property 'wakeup-delay-ms'
> > and apply the specified delay after enabling the mic. A blocking delay
> > can't be applied in dmic_daiops_trigger() since the function is called
> > in atomic context. Instead use a DAPM event handler to set the enable
> > GPIO and apply the delay in the handler.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > ---
> > Changes in v2:
> > - use DAPM event handler instead of _prepare() and get rid of
> > _trigger()
> > - skip error check for optional 'wakeup-delay-ms' property
> > - updated commit message
>
> Looks good to me in general:
>
> Brian Norris <briannorris@xxxxxxxxxxxx>
>
> But I don't know much about the audio subsystem. One comment below.
>
> >
> > .../devicetree/bindings/sound/dmic.txt | 2 +
> > sound/soc/codecs/dmic.c | 63 ++++++++++---------
> > 2 files changed, 37 insertions(+), 28 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/dmic.txt b/Documentation/devicetree/bindings/sound/dmic.txt
> > index f7bf65611453..e957b4136716 100644
> > --- a/Documentation/devicetree/bindings/sound/dmic.txt
> > +++ b/Documentation/devicetree/bindings/sound/dmic.txt
> > @@ -8,6 +8,7 @@ Required properties:
> > Optional properties:
> > - dmicen-gpios: GPIO specifier for dmic to control start and stop
> > - num-channels: Number of microphones on this DAI
> > + - wakeup-delay-ms: Delay (in ms) after enabling the DMIC
> >
> > Example node:
> >
> > @@ -15,4 +16,5 @@ Example node:
> > compatible = "dmic-codec";
> > dmicen-gpios = <&gpio4 3 GPIO_ACTIVE_HIGH>;
> > num-channels = <1>;
> > + wakeup-delay-ms <50>;
> > };
> > diff --git a/sound/soc/codecs/dmic.c b/sound/soc/codecs/dmic.c
> > index cf83c423394d..b5f60ac22ace 100644
> > --- a/sound/soc/codecs/dmic.c
> > +++ b/sound/soc/codecs/dmic.c
> [...]
>
> > static const struct snd_soc_dapm_widget dmic_dapm_widgets[] = {
> > - SND_SOC_DAPM_AIF_OUT("DMIC AIF", "Capture", 0,
> > - SND_SOC_NOPM, 0, 0),
> > + SND_SOC_DAPM_AIF_OUT_E("DMIC AIF", "Capture", 0,
> > + SND_SOC_NOPM, 0, 0, dmic_aif_event,
> > + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
>
> Any good reason both are _POST_? It seems like you'd want them to be
> inverses (e.g., power-power-up, and pre-power-down).

Post-power-up is needed to avoid a popping noise when the clock is
enabled (see 3a6f9dce6116 ("ASoC: rk3399_gru_sound: fix recording pop
at first attempt") and disabling the mic before switching off the
clock (pre-power-down) could cause noise at the end of the recording.