| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <295ce358-c00e-74ce-50c1-1df32c2c6cf7@st.com>
Date: Fri, 5 Jan 2018 11:45:43 +0100
From: Arnaud Pouliquen <arnaud.pouliquen@...com>
To: Matthias Kaehlcke <mka@...omium.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>
CC: "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Bhumika Goyal <bhumirks@...il.com>,
huang lin <hl@...k-chips.com>,
Brian Norris <briannorris@...omium.org>,
Dylan Reid <dgreid@...omium.org>
Subject: Re: [PATCH] ASoC: codecs: dmic: Use channel map for configs with a
single mic
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@...omium.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;
>> }
Powered by blists - more mailing lists