From: Matthias Kaehlcke <mka@chromium.org> To: Peter Ujfalusi <peter.ujfalusi@ti.com> Cc: Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, devicetree@vger.kernel.org, alsa-devel@alsa-project.org, huang lin <hl@rock-chips.com>, Brian Norris <briannorris@chromium.org>, linux-kernel@vger.kernel.org, Dylan Reid <dgreid@chromium.org> Subject: Re: [alsa-devel] [PATCH] ASoC: dmic: Add optional wakeup delay Date: Thu, 15 Feb 2018 10:18:00 -0800 [thread overview] Message-ID: <20180215181800.GB99727@google.com> (raw) In-Reply-To: <7c9d5202-bcac-f989-6be2-f286de15a5de@ti.com> El Thu, Feb 15, 2018 at 09:42:01AM +0200 Peter Ujfalusi ha dit: > > > On 2018-02-15 01:51, Matthias Kaehlcke wrote: > > On some systems a delay is needed after switching on the clocks, to allow > > the DMIC 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 in the new dmic_daiops_prepare(). > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > .../devicetree/bindings/sound/dmic.txt | 2 + > > sound/soc/codecs/dmic.c | 54 ++++++++++++++----- > > 2 files changed, 42 insertions(+), 14 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/sound/dmic.txt b/Documentation/devicetree/bindings/sound/dmic.txt > > index 54c8ef6498a8..de741c6609d0 100644 > > --- a/Documentation/devicetree/bindings/sound/dmic.txt > > +++ b/Documentation/devicetree/bindings/sound/dmic.txt > > @@ -7,10 +7,12 @@ Required properties: > > > > Optional properties: > > - dmicen-gpios: GPIO specifier for dmic to control start and stop > > + - wakeup-delay-ms: Delay (in ms) after enabling the DMIC > > > > Example node: > > > > dmic_codec: dmic@0 { > > compatible = "dmic-codec"; > > dmicen-gpios = <&gpio4 3 GPIO_ACTIVE_HIGH>; > > + wakeup-delay-ms <50>; > > }; > > diff --git a/sound/soc/codecs/dmic.c b/sound/soc/codecs/dmic.c > > index b88a1ee66f80..11f6abf11074 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,24 +30,38 @@ > > #include <sound/soc.h> > > #include <sound/soc-dapm.h> > > > > +struct dmic { > > + struct gpio_desc *gpio_en; > > + int wakeup_delay; > > +}; > > + > > +static int dmic_daiops_prepare(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai) > > +{ > > + struct dmic *dmic = snd_soc_dai_get_drvdata(dai); > > + > > + if (dmic->gpio_en) > > + gpiod_set_value(dmic->gpio_en, 1); > > You don't need the 'if (dmic->gpio_en)' True, personally I think the if statement makes it clearer that the GPIO is optional. I'm fine with removing it if the general sense is that it's just noise. > > + > > + if (dmic->wakeup_delay) > > + msleep(dmic->wakeup_delay); > > + > > + return 0; > > +} > > + > > 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 *dmic = snd_soc_dai_get_drvdata(dai); > > > > - if (!dmic_en) > > + if (!dmic->gpio_en) > > return 0; > > > > switch (cmd) { > > - case SNDRV_PCM_TRIGGER_START: > > - case SNDRV_PCM_TRIGGER_RESUME: > > - case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > > - gpiod_set_value(dmic_en, 1); > > - break; > > case SNDRV_PCM_TRIGGER_STOP: > > case SNDRV_PCM_TRIGGER_SUSPEND: > > case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > > - gpiod_set_value(dmic_en, 0); > > + gpiod_set_value(dmic->gpio_en, 0); > > break; > > } > > What if instead of this trickery we try to handle the gpio/delay via > DAPM? SND_SOC_DAPM_AIF_OUT_E() might be just what we need? > > We could remove the trigger, and will have no need for the prepare > callback either.. SND_SOC_DAPM_AIF_OUT_E() looks promising, thanks for the pointer! > > @@ -54,6 +69,7 @@ static int dmic_daiops_trigger(struct snd_pcm_substream *substream, > > } > > > > static const struct snd_soc_dai_ops dmic_dai_ops = { > > + .prepare = dmic_daiops_prepare, > > .trigger = dmic_daiops_trigger, > > }; > > > > @@ -73,14 +89,24 @@ static struct snd_soc_dai_driver dmic_dai = { > > > > static int dmic_codec_probe(struct snd_soc_codec *codec) > > { > > - struct gpio_desc *dmic_en; > > + struct dmic *dmic; > > + int err; > > + > > + 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); > > + err = device_property_read_u32(codec->dev, "wakeup-delay-ms", > > + &dmic->wakeup_delay); > > + if (err && (err != -EINVAL)) > > + return err; > > > > - snd_soc_codec_set_drvdata(codec, dmic_en); > > + snd_soc_codec_set_drvdata(codec, dmic); > > > > return 0; > > } > > > > - Péter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
WARNING: multiple messages have this Message-ID (diff)
From: Matthias Kaehlcke <mka@chromium.org> To: Peter Ujfalusi <peter.ujfalusi@ti.com> Cc: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, alsa-devel@alsa-project.org, huang lin <hl@rock-chips.com>, linux-kernel@vger.kernel.org, Brian Norris <briannorris@chromium.org>, Takashi Iwai <tiwai@suse.com>, Rob Herring <robh+dt@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, Dylan Reid <dgreid@chromium.org> Subject: Re: [PATCH] ASoC: dmic: Add optional wakeup delay Date: Thu, 15 Feb 2018 10:18:00 -0800 [thread overview] Message-ID: <20180215181800.GB99727@google.com> (raw) In-Reply-To: <7c9d5202-bcac-f989-6be2-f286de15a5de@ti.com> El Thu, Feb 15, 2018 at 09:42:01AM +0200 Peter Ujfalusi ha dit: > > > On 2018-02-15 01:51, Matthias Kaehlcke wrote: > > On some systems a delay is needed after switching on the clocks, to allow > > the DMIC 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 in the new dmic_daiops_prepare(). > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > .../devicetree/bindings/sound/dmic.txt | 2 + > > sound/soc/codecs/dmic.c | 54 ++++++++++++++----- > > 2 files changed, 42 insertions(+), 14 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/sound/dmic.txt b/Documentation/devicetree/bindings/sound/dmic.txt > > index 54c8ef6498a8..de741c6609d0 100644 > > --- a/Documentation/devicetree/bindings/sound/dmic.txt > > +++ b/Documentation/devicetree/bindings/sound/dmic.txt > > @@ -7,10 +7,12 @@ Required properties: > > > > Optional properties: > > - dmicen-gpios: GPIO specifier for dmic to control start and stop > > + - wakeup-delay-ms: Delay (in ms) after enabling the DMIC > > > > Example node: > > > > dmic_codec: dmic@0 { > > compatible = "dmic-codec"; > > dmicen-gpios = <&gpio4 3 GPIO_ACTIVE_HIGH>; > > + wakeup-delay-ms <50>; > > }; > > diff --git a/sound/soc/codecs/dmic.c b/sound/soc/codecs/dmic.c > > index b88a1ee66f80..11f6abf11074 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,24 +30,38 @@ > > #include <sound/soc.h> > > #include <sound/soc-dapm.h> > > > > +struct dmic { > > + struct gpio_desc *gpio_en; > > + int wakeup_delay; > > +}; > > + > > +static int dmic_daiops_prepare(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai) > > +{ > > + struct dmic *dmic = snd_soc_dai_get_drvdata(dai); > > + > > + if (dmic->gpio_en) > > + gpiod_set_value(dmic->gpio_en, 1); > > You don't need the 'if (dmic->gpio_en)' True, personally I think the if statement makes it clearer that the GPIO is optional. I'm fine with removing it if the general sense is that it's just noise. > > + > > + if (dmic->wakeup_delay) > > + msleep(dmic->wakeup_delay); > > + > > + return 0; > > +} > > + > > 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 *dmic = snd_soc_dai_get_drvdata(dai); > > > > - if (!dmic_en) > > + if (!dmic->gpio_en) > > return 0; > > > > switch (cmd) { > > - case SNDRV_PCM_TRIGGER_START: > > - case SNDRV_PCM_TRIGGER_RESUME: > > - case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > > - gpiod_set_value(dmic_en, 1); > > - break; > > case SNDRV_PCM_TRIGGER_STOP: > > case SNDRV_PCM_TRIGGER_SUSPEND: > > case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > > - gpiod_set_value(dmic_en, 0); > > + gpiod_set_value(dmic->gpio_en, 0); > > break; > > } > > What if instead of this trickery we try to handle the gpio/delay via > DAPM? SND_SOC_DAPM_AIF_OUT_E() might be just what we need? > > We could remove the trigger, and will have no need for the prepare > callback either.. SND_SOC_DAPM_AIF_OUT_E() looks promising, thanks for the pointer! > > @@ -54,6 +69,7 @@ static int dmic_daiops_trigger(struct snd_pcm_substream *substream, > > } > > > > static const struct snd_soc_dai_ops dmic_dai_ops = { > > + .prepare = dmic_daiops_prepare, > > .trigger = dmic_daiops_trigger, > > }; > > > > @@ -73,14 +89,24 @@ static struct snd_soc_dai_driver dmic_dai = { > > > > static int dmic_codec_probe(struct snd_soc_codec *codec) > > { > > - struct gpio_desc *dmic_en; > > + struct dmic *dmic; > > + int err; > > + > > + 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); > > + err = device_property_read_u32(codec->dev, "wakeup-delay-ms", > > + &dmic->wakeup_delay); > > + if (err && (err != -EINVAL)) > > + return err; > > > > - snd_soc_codec_set_drvdata(codec, dmic_en); > > + snd_soc_codec_set_drvdata(codec, dmic); > > > > return 0; > > } > > > > - Péter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2018-02-15 18:18 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-14 23:51 [PATCH] ASoC: dmic: Add optional wakeup delay Matthias Kaehlcke 2018-02-14 23:51 ` Matthias Kaehlcke 2018-02-15 0:22 ` Brian Norris 2018-02-15 2:44 ` Matthias Kaehlcke 2018-02-15 2:44 ` Matthias Kaehlcke 2018-02-15 7:42 ` [alsa-devel] " Peter Ujfalusi 2018-02-15 7:42 ` Peter Ujfalusi 2018-02-15 18:18 ` Matthias Kaehlcke [this message] 2018-02-15 18:18 ` Matthias Kaehlcke
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180215181800.GB99727@google.com \ --to=mka@chromium.org \ --cc=alsa-devel@alsa-project.org \ --cc=briannorris@chromium.org \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=dgreid@chromium.org \ --cc=hl@rock-chips.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=perex@perex.cz \ --cc=peter.ujfalusi@ti.com \ --cc=robh+dt@kernel.org \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.