| Message ID | 20180104194848.198941-1-mka@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+ Brian & Dylan El Thu, Jan 04, 2018 at 11:48:48AM -0800 Matthias Kaehlcke ha dit: > The DMIC DAI driver specifies a number of 1 to 8 channels for each DAI. > The actual number of mics can currently not be configured in the device > tree or audio glue, but is derived from the min/max channels of the CPU > and codec DAI. A typical CPU DAI has two or more channels, in consequence > a single mic is treated as a stereo/multi channel device, even though > only one channel carries audio data. > > This change adds the option to specify the number of used DMIC channels > in the device tree. If a single channel is used we export a channel map > that marks all unused channels as invalid to indicate userspace that the > capture device is mono. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > Documentation/devicetree/bindings/sound/dmic.txt | 2 + > sound/soc/codecs/dmic.c | 72 +++++++++++++++++++++--- > 2 files changed, 65 insertions(+), 9 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/dmic.txt b/Documentation/devicetree/bindings/sound/dmic.txt > index 54c8ef6498a8..f7bf65611453 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 > + - num-channels: Number of microphones on this DAI > > Example node: > > dmic_codec: dmic@0 { > compatible = "dmic-codec"; > dmicen-gpios = <&gpio4 3 GPIO_ACTIVE_HIGH>; > + num-channels = <1>; > }; > diff --git a/sound/soc/codecs/dmic.c b/sound/soc/codecs/dmic.c > index b88a1ee66f80..c705a25b138e 100644 > --- a/sound/soc/codecs/dmic.c > +++ b/sound/soc/codecs/dmic.c > @@ -29,24 +29,29 @@ > #include <sound/soc.h> > #include <sound/soc-dapm.h> > > +struct dmic { > + struct gpio_desc *gpio_en; > + int channels; > +}; > + > 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 || !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); > + gpiod_set_value(dmic->gpio_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; > } > > @@ -57,6 +62,42 @@ static const struct snd_soc_dai_ops dmic_dai_ops = { > .trigger = dmic_daiops_trigger, > }; > > +const struct snd_pcm_chmap_elem dmic_chmaps_single[] = { > + { .channels = 1, > + .map = { SNDRV_CHMAP_MONO } }, > + { .channels = 2, > + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA } }, > + { .channels = 4, > + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, > + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, > + { .channels = 6, > + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, > + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, > + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, > + { .channels = 8, > + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, > + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, > + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, > + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, > + { } > +}; > + > +static int dmic_pcm_new(struct snd_soc_pcm_runtime *rtd, > + struct snd_soc_dai *dai) > +{ > + struct dmic *dmic = snd_soc_dai_get_drvdata(dai); > + int err; > + > + if (dmic->channels == 1) { > + err = snd_pcm_add_chmap_ctls(rtd->pcm, SNDRV_PCM_STREAM_CAPTURE, > + dmic_chmaps_single, 8, 0, NULL); > + if (err < 0) > + return err; > + } > + > + return 0; > +} > + > static struct snd_soc_dai_driver dmic_dai = { > .name = "dmic-hifi", > .capture = { > @@ -69,18 +110,31 @@ static struct snd_soc_dai_driver dmic_dai = { > | SNDRV_PCM_FMTBIT_S16_LE, > }, > .ops = &dmic_dai_ops, > + .pcm_new = dmic_pcm_new, > }; > > static int dmic_codec_probe(struct snd_soc_codec *codec) > { > - struct gpio_desc *dmic_en; > + struct dmic *dmic; > + int err; > + u32 pval; > + > + dmic = devm_kzalloc(codec->dev, sizeof(*dmic), GFP_KERNEL); > + if (!dmic) > + return -ENOMEM; > > - dmic_en = devm_gpiod_get_optional(codec->dev, > + dmic->gpio_en = devm_gpiod_get_optional(codec->dev, > "dmicen", GPIOD_OUT_LOW); > - if (IS_ERR(dmic_en)) > - return PTR_ERR(dmic_en); > + if (IS_ERR(dmic->gpio_en)) > + return PTR_ERR(dmic->gpio_en); > + > + err = of_property_read_u32(codec->dev->of_node, "num-channels", &pval); > + if (!err) > + dmic->channels = (int)pval; > + else if (err != -ENOENT) > + return err; > > - snd_soc_codec_set_drvdata(codec, dmic_en); > + snd_soc_codec_set_drvdata(codec, dmic); > > return 0; > }
Hello Matthias, Please find comments in line On 01/04/2018 08:54 PM, Matthias Kaehlcke wrote: > + Brian & Dylan > > El Thu, Jan 04, 2018 at 11:48:48AM -0800 Matthias Kaehlcke ha dit: > >> The DMIC DAI driver specifies a number of 1 to 8 channels for each DAI. >> The actual number of mics can currently not be configured in the device >> tree or audio glue, but is derived from the min/max channels of the CPU >> and codec DAI. A typical CPU DAI has two or more channels, in consequence >> a single mic is treated as a stereo/multi channel device, even though >> only one channel carries audio data. Look like you implement CPU DAI or application constraint in the codec... Here is a use case that does not match with your implementation: 2 dmics multiplexed on a SPI bus connected to the CPU DAI peripheral. The SoC peripheral is able to decimate and multiplex the both PDM streams in a stereo PCM stream. At PCM level this can be exposed as a stereo capture (Left + Right). In this case your channel map description does not match. >> >> This change adds the option to specify the number of used DMIC channels >> in the device tree. If a single channel is used we export a channel map >> that marks all unused channels as invalid to indicate userspace that the >> capture device is mono. >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> >> --- >> Documentation/devicetree/bindings/sound/dmic.txt | 2 + >> sound/soc/codecs/dmic.c | 72 +++++++++++++++++++++--- >> 2 files changed, 65 insertions(+), 9 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/sound/dmic.txt b/Documentation/devicetree/bindings/sound/dmic.txt >> index 54c8ef6498a8..f7bf65611453 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 >> + - num-channels: Number of microphones on this DAI >> >> Example node: >> >> dmic_codec: dmic@0 { >> compatible = "dmic-codec"; >> dmicen-gpios = <&gpio4 3 GPIO_ACTIVE_HIGH>; >> + num-channels = <1>; In your implementation seems not linked to hardware but software... DMIC driver description specifies the channels_max to 8 channels. I suppose that it is used for DMIC codecs that integrate filters and are connected to CPU DAI with I2S/PCM links. But it can be also used for DMIC connected to CPU DAI with a SPI link (in this case decimation filter in on Soc side). If we continue to support both use cases, specify the number of channels seems reasonable but this should be use to change the max channel constraint, not to declare a control. >> }; >> diff --git a/sound/soc/codecs/dmic.c b/sound/soc/codecs/dmic.c >> index b88a1ee66f80..c705a25b138e 100644 >> --- a/sound/soc/codecs/dmic.c >> +++ b/sound/soc/codecs/dmic.c >> @@ -29,24 +29,29 @@ >> #include <sound/soc.h> >> #include <sound/soc-dapm.h> >> >> +struct dmic { >> + struct gpio_desc *gpio_en; >> + int channels; >> +}; >> + >> 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 || !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); >> + gpiod_set_value(dmic->gpio_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; >> } >> >> @@ -57,6 +62,42 @@ static const struct snd_soc_dai_ops dmic_dai_ops = { >> .trigger = dmic_daiops_trigger, >> }; >> >> +const struct snd_pcm_chmap_elem dmic_chmaps_single[] = { >> + { .channels = 1, >> + .map = { SNDRV_CHMAP_MONO } }, >> + { .channels = 2, >> + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA } }, >> + { .channels = 4, >> + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, >> + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, >> + { .channels = 6, >> + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, >> + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, >> + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, >> + { .channels = 8, >> + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, >> + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, >> + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, >> + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, >> + { } >> +}; From my point of view the control should be in CPU DAI. Regards Arnaud >> + >> +static int dmic_pcm_new(struct snd_soc_pcm_runtime *rtd, >> + struct snd_soc_dai *dai) >> +{ >> + struct dmic *dmic = snd_soc_dai_get_drvdata(dai); >> + int err; >> + >> + if (dmic->channels == 1) { >> + err = snd_pcm_add_chmap_ctls(rtd->pcm, SNDRV_PCM_STREAM_CAPTURE, >> + dmic_chmaps_single, 8, 0, NULL); >> + if (err < 0) >> + return err; >> + } >> + >> + return 0; >> +} >> + >> static struct snd_soc_dai_driver dmic_dai = { >> .name = "dmic-hifi", >> .capture = { >> @@ -69,18 +110,31 @@ static struct snd_soc_dai_driver dmic_dai = { >> | SNDRV_PCM_FMTBIT_S16_LE, >> }, >> .ops = &dmic_dai_ops, >> + .pcm_new = dmic_pcm_new, >> }; >> >> static int dmic_codec_probe(struct snd_soc_codec *codec) >> { >> - struct gpio_desc *dmic_en; >> + struct dmic *dmic; >> + int err; >> + u32 pval; >> + >> + dmic = devm_kzalloc(codec->dev, sizeof(*dmic), GFP_KERNEL); >> + if (!dmic) >> + return -ENOMEM; >> >> - dmic_en = devm_gpiod_get_optional(codec->dev, >> + dmic->gpio_en = devm_gpiod_get_optional(codec->dev, >> "dmicen", GPIOD_OUT_LOW); >> - if (IS_ERR(dmic_en)) >> - return PTR_ERR(dmic_en); >> + if (IS_ERR(dmic->gpio_en)) >> + return PTR_ERR(dmic->gpio_en); >> + >> + err = of_property_read_u32(codec->dev->of_node, "num-channels", &pval); >> + if (!err) >> + dmic->channels = (int)pval; >> + else if (err != -ENOENT) >> + return err; >> >> - snd_soc_codec_set_drvdata(codec, dmic_en); >> + snd_soc_codec_set_drvdata(codec, dmic); >> >> return 0; >> }
On Fri, Jan 05, 2018 at 11:45:43AM +0100, Arnaud Pouliquen wrote: > >> + num-channels = <1>; > In your implementation seems not linked to hardware but software... > DMIC driver description specifies the channels_max to 8 channels. > I suppose that it is used for DMIC codecs that integrate filters and are > connected to CPU DAI with I2S/PCM links. But it can be also used for > DMIC connected to CPU DAI with a SPI link (in this case decimation > filter in on Soc side). The intention with the DMIC CODEC is that it's used when the CPU directly has PDM inputs and the DMICs are just directly wired to it (stereo is obviously the norm here but some SoCs may bunch things up further for use with mic arrays). > If we continue to support both use cases, specify the number of channels > seems reasonable but this should be use to change the max channel > constraint, not to declare a control. Yes, that would seem the most obvious thing - it's how we handle things like CPU DAIs that support very high channel counts when connected to stereo CODECs for example. It's not obvious why we'd use a channel map here instead.
El Fri, Jan 05, 2018 at 12:04:55PM +0000 Mark Brown ha dit: > On Fri, Jan 05, 2018 at 11:45:43AM +0100, Arnaud Pouliquen wrote: > > > >> + num-channels = <1>; > > > In your implementation seems not linked to hardware but software... > > > DMIC driver description specifies the channels_max to 8 channels. > > I suppose that it is used for DMIC codecs that integrate filters and are > > connected to CPU DAI with I2S/PCM links. But it can be also used for > > DMIC connected to CPU DAI with a SPI link (in this case decimation > > filter in on Soc side). > > The intention with the DMIC CODEC is that it's used when the CPU > directly has PDM inputs and the DMICs are just directly wired to it > (stereo is obviously the norm here but some SoCs may bunch things up > further for use with mic arrays). > > > If we continue to support both use cases, specify the number of channels > > seems reasonable but this should be use to change the max channel > > constraint, not to declare a control. > > Yes, that would seem the most obvious thing - it's how we handle things > like CPU DAIs that support very high channel counts when connected to > stereo CODECs for example. It's not obvious why we'd use a channel map > here instead. Thanks for the feedback! I experimented initially with changing channels_max, but overwriting dmic_dai.capture.channels_max didn't seem right since it would affect other possible instances of the codec. I overlooked that this can be avoided by passing a *copy* with adjusted channels_max to snd_soc_register_codec(). Matthias
diff --git a/Documentation/devicetree/bindings/sound/dmic.txt b/Documentation/devicetree/bindings/sound/dmic.txt index 54c8ef6498a8..f7bf65611453 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 + - num-channels: Number of microphones on this DAI Example node: dmic_codec: dmic@0 { compatible = "dmic-codec"; dmicen-gpios = <&gpio4 3 GPIO_ACTIVE_HIGH>; + num-channels = <1>; }; diff --git a/sound/soc/codecs/dmic.c b/sound/soc/codecs/dmic.c index b88a1ee66f80..c705a25b138e 100644 --- a/sound/soc/codecs/dmic.c +++ b/sound/soc/codecs/dmic.c @@ -29,24 +29,29 @@ #include <sound/soc.h> #include <sound/soc-dapm.h> +struct dmic { + struct gpio_desc *gpio_en; + int channels; +}; + 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 || !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); + gpiod_set_value(dmic->gpio_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; } @@ -57,6 +62,42 @@ static const struct snd_soc_dai_ops dmic_dai_ops = { .trigger = dmic_daiops_trigger, }; +const struct snd_pcm_chmap_elem dmic_chmaps_single[] = { + { .channels = 1, + .map = { SNDRV_CHMAP_MONO } }, + { .channels = 2, + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA } }, + { .channels = 4, + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, + { .channels = 6, + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, + { .channels = 8, + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_NA, + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA, + SNDRV_CHMAP_NA, SNDRV_CHMAP_NA } }, + { } +}; + +static int dmic_pcm_new(struct snd_soc_pcm_runtime *rtd, + struct snd_soc_dai *dai) +{ + struct dmic *dmic = snd_soc_dai_get_drvdata(dai); + int err; + + if (dmic->channels == 1) { + err = snd_pcm_add_chmap_ctls(rtd->pcm, SNDRV_PCM_STREAM_CAPTURE, + dmic_chmaps_single, 8, 0, NULL); + if (err < 0) + return err; + } + + return 0; +} + static struct snd_soc_dai_driver dmic_dai = { .name = "dmic-hifi", .capture = { @@ -69,18 +110,31 @@ static struct snd_soc_dai_driver dmic_dai = { | SNDRV_PCM_FMTBIT_S16_LE, }, .ops = &dmic_dai_ops, + .pcm_new = dmic_pcm_new, }; static int dmic_codec_probe(struct snd_soc_codec *codec) { - struct gpio_desc *dmic_en; + struct dmic *dmic; + int err; + u32 pval; + + dmic = devm_kzalloc(codec->dev, sizeof(*dmic), GFP_KERNEL); + if (!dmic) + return -ENOMEM; - dmic_en = devm_gpiod_get_optional(codec->dev, + dmic->gpio_en = devm_gpiod_get_optional(codec->dev, "dmicen", GPIOD_OUT_LOW); - if (IS_ERR(dmic_en)) - return PTR_ERR(dmic_en); + if (IS_ERR(dmic->gpio_en)) + return PTR_ERR(dmic->gpio_en); + + err = of_property_read_u32(codec->dev->of_node, "num-channels", &pval); + if (!err) + dmic->channels = (int)pval; + else if (err != -ENOENT) + return err; - snd_soc_codec_set_drvdata(codec, dmic_en); + snd_soc_codec_set_drvdata(codec, dmic); return 0; }
The DMIC DAI driver specifies a number of 1 to 8 channels for each DAI. The actual number of mics can currently not be configured in the device tree or audio glue, but is derived from the min/max channels of the CPU and codec DAI. A typical CPU DAI has two or more channels, in consequence a single mic is treated as a stereo/multi channel device, even though only one channel carries audio data. This change adds the option to specify the number of used DMIC channels in the device tree. If a single channel is used we export a channel map that marks all unused channels as invalid to indicate userspace that the capture device is mono. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- Documentation/devicetree/bindings/sound/dmic.txt | 2 + sound/soc/codecs/dmic.c | 72 +++++++++++++++++++++--- 2 files changed, 65 insertions(+), 9 deletions(-)