diff mbox

[v2] ASoC: dmic: Add optional wakeup delay

Message ID 20180216022416.208265-1-mka@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Matthias Kaehlcke Feb. 16, 2018, 2:24 a.m. UTC 
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

 .../devicetree/bindings/sound/dmic.txt        |  2 +
 sound/soc/codecs/dmic.c                       | 63 ++++++++++---------
 2 files changed, 37 insertions(+), 28 deletions(-)

Comments

Brian Norris Feb. 16, 2018, 2:47 a.m. UTC | #1 
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).

>  	SND_SOC_DAPM_INPUT("DMic"),
>  };
>  

Brian
Mark Brown Feb. 16, 2018, 11:28 a.m. UTC | #2 
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'

This doesn't apply against current code, please check and resend.
Matthias Kaehlcke Feb. 16, 2018, 4:47 p.m. UTC | #3 
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.
Matthias Kaehlcke Feb. 16, 2018, 5:09 p.m. UTC | #4 
El Fri, Feb 16, 2018 at 11:28:01AM +0000 Mark Brown ha dit:

> 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'
> 
> This doesn't apply against current code, please check and resend.

Sorry about that, apparently 6d6c3946d877 ("ASoC: dmic: replace codec
to component") was merged just after syncing my tree. Will rebase and
resend.
diff mbox

Patch

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
@@ -19,6 +19,7 @@ 
  *
  */
 
+#include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/platform_device.h>
@@ -29,34 +30,33 @@ 
 #include <sound/soc.h>
 #include <sound/soc-dapm.h>
 
-static int dmic_daiops_trigger(struct snd_pcm_substream *substream,
-		int cmd, struct snd_soc_dai *dai)
-{
-	struct gpio_desc *dmic_en = snd_soc_dai_get_drvdata(dai);
+struct dmic {
+	struct gpio_desc *gpio_en;
+	int wakeup_delay;
+};
 
-	if (!dmic_en)
-		return 0;
+static int dmic_aif_event(struct snd_soc_dapm_widget *w,
+			  struct snd_kcontrol *kcontrol, int event) {
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct dmic *dmic = snd_soc_codec_get_drvdata(codec);
 
-	switch (cmd) {
-	case SNDRV_PCM_TRIGGER_START:
-	case SNDRV_PCM_TRIGGER_RESUME:
-	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		gpiod_set_value(dmic_en, 1);
+	switch (event) {
+	case SND_SOC_DAPM_POST_PMU:
+		if (dmic->gpio_en)
+			gpiod_set_value(dmic->gpio_en, 1);
+
+		if (dmic->wakeup_delay)
+			msleep(dmic->wakeup_delay);
 		break;
-	case SNDRV_PCM_TRIGGER_STOP:
-	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		gpiod_set_value(dmic_en, 0);
+	case SND_SOC_DAPM_POST_PMD:
+		if (dmic->gpio_en)
+			gpiod_set_value(dmic->gpio_en, 0);
 		break;
 	}
 
 	return 0;
 }
 
-static const struct snd_soc_dai_ops dmic_dai_ops = {
-	.trigger	= dmic_daiops_trigger,
-};
-
 static struct snd_soc_dai_driver dmic_dai = {
 	.name = "dmic-hifi",
 	.capture = {
@@ -68,26 +68,33 @@  static struct snd_soc_dai_driver dmic_dai = {
 			| SNDRV_PCM_FMTBIT_S24_LE
 			| SNDRV_PCM_FMTBIT_S16_LE,
 	},
-	.ops    = &dmic_dai_ops,
 };
 
 static int dmic_codec_probe(struct snd_soc_codec *codec)
 {
-	struct gpio_desc *dmic_en;
+	struct dmic *dmic;
+
+	dmic = devm_kzalloc(codec->dev, sizeof(*dmic), GFP_KERNEL);
+	if (!dmic)
+		return -ENOMEM;
+
+	dmic->gpio_en = devm_gpiod_get_optional(codec->dev,
+						"dmicen", GPIOD_OUT_LOW);
+	if (IS_ERR(dmic->gpio_en))
+		return PTR_ERR(dmic->gpio_en);
 
-	dmic_en = devm_gpiod_get_optional(codec->dev,
-					"dmicen", GPIOD_OUT_LOW);
-	if (IS_ERR(dmic_en))
-		return PTR_ERR(dmic_en);
+	device_property_read_u32(codec->dev, "wakeup-delay-ms",
+				 &dmic->wakeup_delay);
 
-	snd_soc_codec_set_drvdata(codec, dmic_en);
+	snd_soc_codec_set_drvdata(codec, dmic);
 
 	return 0;
 }
 
 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),
 	SND_SOC_DAPM_INPUT("DMic"),
 };