Re: [PATCH] ASoC: codecs: dmic: Use channel map for configs with a single mic

[Arnaud Pouliquen]

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@xxxxxxxxxxxx>
>> ---
>>Â 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;
>>Â }