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

[Matthias Kaehlcke <mka@chromium.org>]

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@chromium.org>
> > ---
> > 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@chromium.org>
> 
> 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.