Re: [alsa-devel] [PATCH] ASoC: dmic: Add optional wakeup delay
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> > --- > > .../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 = < 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 > > #include > > #include > > #include > > @@ -29,24 +30,38 @@ > > #include > > #include > > > > +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", > > + >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. >
Re: [alsa-devel] [PATCH] ASoC: dmic: Add optional wakeup delay
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 > > --- > > .../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 = < 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 > > #include > > #include > > #include > > @@ -29,24 +30,38 @@ > > #include > > #include > > > > +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", > > + >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.
Re: [alsa-devel] [PATCH] ASoC: dmic: Add optional wakeup delay
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> --- > .../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 = < 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 > #include > #include > #include > @@ -29,24 +30,38 @@ > #include > #include > > +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)' > + > + 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.. > > @@ -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", > +>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
Re: [alsa-devel] [PATCH] ASoC: dmic: Add optional wakeup delay
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 > --- > .../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 = < 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 > #include > #include > #include > @@ -29,24 +30,38 @@ > #include > #include > > +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)' > + > + 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.. > > @@ -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", > +>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