[PATCH] ASoC: max98090: add digital mic mux to record path

[PATCH] ASoC: max98090: add digital mic mux to record path

[Andrew Bresticker]

The max98090 driver currently treats the digital mic enable as a supply
on the record path, causing the digital mic enable to always be turned on
when attempting to record.  This is incorrect, however, since the digital
mic enable is actually a mux control where 0 selects the ADC output as
input to the record-path DSP and 1 selects the digital mic.  This patch
converts the DMIC{L,R}_ENA supplies into DMIC{L,R} muxes so that we can
switch between the ADC and the digital mic for recording.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
 sound/soc/codecs/max98090.c | 45 +++++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index ce0d364..71b6b8a 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -857,6 +857,22 @@ static const struct soc_enum mic2_mux_enum =
 static const struct snd_kcontrol_new max98090_mic2_mux =
 	SOC_DAPM_ENUM("MIC2 Mux", mic2_mux_enum);
 
+static const char *dmic_mux_text[] = { "ADC", "DMIC" };
+
+static const struct soc_enum dmicl_mux_enum =
+	SOC_ENUM_SINGLE(M98090_REG_DIGITAL_MIC_ENABLE, M98090_DIGMICL_SHIFT,
+		ARRAY_SIZE(dmic_mux_text), dmic_mux_text);
+
+static const struct snd_kcontrol_new max98090_dmicl_mux =
+	SOC_DAPM_ENUM("DMICL Mux", dmicl_mux_enum);
+
+static const struct soc_enum dmicr_mux_enum =
+	SOC_ENUM_SINGLE(M98090_REG_DIGITAL_MIC_ENABLE, M98090_DIGMICR_SHIFT,
+		ARRAY_SIZE(dmic_mux_text), dmic_mux_text);
+
+static const struct snd_kcontrol_new max98090_dmicr_mux =
+	SOC_DAPM_ENUM("DMICR Mux", dmicr_mux_enum);
+
 static const char *max98090_micpre_text[] = { "Off", "On" };
 
 static const struct soc_enum max98090_pa1en_enum =
@@ -1127,10 +1143,6 @@ static const struct snd_soc_dapm_widget max98090_dapm_widgets[] = {
 		M98090_SDIEN_SHIFT, 0, NULL, 0),
 	SND_SOC_DAPM_SUPPLY("SDOEN", M98090_REG_IO_CONFIGURATION,
 		M98090_SDOEN_SHIFT, 0, NULL, 0),
-	SND_SOC_DAPM_SUPPLY("DMICL_ENA", M98090_REG_DIGITAL_MIC_ENABLE,
-		 M98090_DIGMICL_SHIFT, 0, NULL, 0),
-	SND_SOC_DAPM_SUPPLY("DMICR_ENA", M98090_REG_DIGITAL_MIC_ENABLE,
-		 M98090_DIGMICR_SHIFT, 0, NULL, 0),
 	SND_SOC_DAPM_SUPPLY("AHPF", M98090_REG_FILTER_CONFIG,
 		M98090_AHPF_SHIFT, 0, NULL, 0),
 
@@ -1144,6 +1156,12 @@ static const struct snd_soc_dapm_widget max98090_dapm_widgets[] = {
 	SND_SOC_DAPM_MUX("MIC2 Mux", SND_SOC_NOPM,
 		0, 0, &max98090_mic2_mux),
 
+	SND_SOC_DAPM_MUX("DMICL Mux", SND_SOC_NOPM,
+		0, 0, &max98090_dmicl_mux),
+
+	SND_SOC_DAPM_MUX("DMICR Mux", SND_SOC_NOPM,
+		0, 0, &max98090_dmicr_mux),
+
 	SND_SOC_DAPM_PGA_E("MIC1 Input", M98090_REG_MIC1_INPUT_LEVEL,
 		M98090_MIC_PA1EN_SHIFT, 0, NULL, 0, max98090_micinput_event,
 		SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
@@ -1281,8 +1299,6 @@ static const struct snd_soc_dapm_route max98090_dapm_routes[] = {
 	{"MIC1 Input", NULL, "MIC1"},
 	{"MIC2 Input", NULL, "MIC2"},
 
-	{"DMICL", NULL, "DMICL_ENA"},
-	{"DMICR", NULL, "DMICR_ENA"},
 	{"DMICL", NULL, "AHPF"},
 	{"DMICR", NULL, "AHPF"},
 
@@ -1336,11 +1352,14 @@ static const struct snd_soc_dapm_route max98090_dapm_routes[] = {
 	{"ADCL", NULL, "SHDN"},
 	{"ADCR", NULL, "SHDN"},
 
-	{"LBENL Mux", "Normal", "ADCL"},
-	{"LBENL Mux", "Normal", "DMICL"},
+	{"DMICL Mux", "ADC", "ADCL"},
+	{"DMICL Mux", "DMIC", "DMICL"},
+	{"DMICR Mux", "ADC", "ADCR"},
+	{"DMICR Mux", "DMIC", "DMICR"},
+
+	{"LBENL Mux", "Normal", "DMICL Mux"},
 	{"LBENL Mux", "Loopback", "LTENL Mux"},
-	{"LBENR Mux", "Normal", "ADCR"},
-	{"LBENR Mux", "Normal", "DMICR"},
+	{"LBENR Mux", "Normal", "DMICR Mux"},
 	{"LBENR Mux", "Loopback", "LTENR Mux"},
 
 	{"AIFOUTL", NULL, "LBENL Mux"},
@@ -1358,10 +1377,8 @@ static const struct snd_soc_dapm_route max98090_dapm_routes[] = {
 	{"DACL", NULL, "LTENL Mux"},
 	{"DACR", NULL, "LTENR Mux"},
 
-	{"STENL Mux", "Sidetone Left", "ADCL"},
-	{"STENL Mux", "Sidetone Left", "DMICL"},
-	{"STENR Mux", "Sidetone Right", "ADCR"},
-	{"STENR Mux", "Sidetone Right", "DMICR"},
+	{"STENL Mux", "Sidetone Left", "DMICL Mux"},
+	{"STENR Mux", "Sidetone Right", "DMICR Mux"},
 	{"DACL", "NULL", "STENL Mux"},
 	{"DACR", "NULL", "STENL Mux"},
 
-- 
1.8.2.1

Re: [PATCH] ASoC: max98090: add digital mic mux to record path

[Andrew Bresticker]

Hi Matthew,

> Default Routing:
>
> Of note (software can confirm) I believe the default DAPM routing for the
> driver was intentional setup such that :
>
> Record:
>
> o   If a headset MIC is present, record from it.
>
> o   If not, digital MIC is the default record input.

While this may have been the intention, that's not what the driver is
currently doing.  This is how the capture route looks before this
patch:

HiFi Capture -> AIFOUT{L,R} -> LBEN{L,R}_MUX[0] -> DMIC{L,R}_ENA

So, since DMIC{L,R}_ENA are configured as supplies, they will always
be turned on during record when the loopback mux is set to 0 (i.e. no
loopback)  That's obviously wrong, so this patch changes the
DMIC{L,R}_ENA to muxes.

> The defaults can be modified by each end user as needed (for analog
> microphones, line inputs, etc.). Please insure any patch to the baseline
> driver reflects the correct operation of the digital microphone enable bits,
> and preserves the intended default operation.

Ok, I'll change it so that the DMIC{L,R} muxes aren't individually controllable.

Thanks,
Andrew

Re: [PATCH] ASoC: max98090: add digital mic mux to record path

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]

On Wed, May 15, 2013 at 09:10:23PM -0700, Matthew Mowdy wrote:
> Hello Andrew,

Don't top post and fix your mailer to word wrap within paragraphs.  This
will help ensure that your messages are legible and comprehensible and
that people have context for whatever you're talking about.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ASoC: max98090: add digital mic mux to record path

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

On Wed, May 15, 2013 at 09:10:23PM -0700, Matthew Mowdy wrote:

Sorry, didn't manage to find the content I meant to reply to in here due
to the formatting issues I mentioned in my mail and the enormous reams
of datsheet that were pasted in.  Anyway....

> Default Routing:
> Of note (software can confirm) I believe the default DAPM routing for the driver was intentional setup such that :

This indicates that the driver is broken, the driver should be using the
chip defaults.

> Record:

> o   If a headset MIC is present, record from it.
> 
> o   If not, digital MIC is the default record input.

This is a bug, the driver should not be making any automatic routing
decisions.  The behaviour you describe would break some fairly obvious
use cases like speakerphone while headset is connected.

> Playback:

> o   When headphones are detected, playback is through the headphone output.

> o   When headphones are not present, playback was through the speaker outputs.

Similarly here, this is junk.  

> The defaults can be modified by each end user as needed (for analog
> microphones, line inputs, etc.). Please insure any patch to the
> baseline driver reflects the correct operation of the digital
> microphone enable bits, and preserves the intended default operation.

No, this is stupid and will be buggy.  The register default values need
to reflect the silicon defaults, the frameworks (both ASoC and regmap)
do things like suppress writes that have no effect and rely on knowing
the silicon defaults for that.

The framework already allows the application layer to configure the
device through the controls exposed to userspace, if people need to
modify a CODEC driver for their system that reflects a problem in the
CODEC driver.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2] ASoC: max98090: add digital mic mux to record path

[Andrew Bresticker]

The max98090 driver currently treats the digital mic enable as a supply
on the record path, causing the digital mic enable to always be turned on
when attempting to record.  This is incorrect, however, since the digital
mic enable is also a mux control where 0 selects the ADC output as input
to the record-path DSP and 1 selects the digital mic.  This patch adds
a virtual DMIC mux to the reocrd path so that we can switch between the
ADC and the digital mic for recording.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
 sound/soc/codecs/max98090.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index ce0d364..cbb272b 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -857,6 +857,14 @@ static const struct soc_enum mic2_mux_enum =
 static const struct snd_kcontrol_new max98090_mic2_mux =
 	SOC_DAPM_ENUM("MIC2 Mux", mic2_mux_enum);
 
+static const char *dmic_mux_text[] = { "ADC", "DMIC" };
+
+static const struct soc_enum dmic_mux_enum =
+	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(dmic_mux_text), dmic_mux_text);
+
+static const struct snd_kcontrol_new max98090_dmic_mux =
+	SOC_DAPM_ENUM_VIRT("DMIC Mux", dmic_mux_enum);
+
 static const char *max98090_micpre_text[] = { "Off", "On" };
 
 static const struct soc_enum max98090_pa1en_enum =
@@ -1144,6 +1152,9 @@ static const struct snd_soc_dapm_widget max98090_dapm_widgets[] = {
 	SND_SOC_DAPM_MUX("MIC2 Mux", SND_SOC_NOPM,
 		0, 0, &max98090_mic2_mux),
 
+	SND_SOC_DAPM_VIRT_MUX("DMIC Mux", SND_SOC_NOPM,
+		0, 0, &max98090_dmic_mux),
+
 	SND_SOC_DAPM_PGA_E("MIC1 Input", M98090_REG_MIC1_INPUT_LEVEL,
 		M98090_MIC_PA1EN_SHIFT, 0, NULL, 0, max98090_micinput_event,
 		SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
@@ -1336,11 +1347,14 @@ static const struct snd_soc_dapm_route max98090_dapm_routes[] = {
 	{"ADCL", NULL, "SHDN"},
 	{"ADCR", NULL, "SHDN"},
 
-	{"LBENL Mux", "Normal", "ADCL"},
-	{"LBENL Mux", "Normal", "DMICL"},
+	{"DMIC Mux", "ADC", "ADCL"},
+	{"DMIC Mux", "ADC", "ADCR"},
+	{"DMIC Mux", "DMIC", "DMICL"},
+	{"DMIC Mux", "DMIC", "DMICR"},
+
+	{"LBENL Mux", "Normal", "DMIC Mux"},
 	{"LBENL Mux", "Loopback", "LTENL Mux"},
-	{"LBENR Mux", "Normal", "ADCR"},
-	{"LBENR Mux", "Normal", "DMICR"},
+	{"LBENR Mux", "Normal", "DMIC Mux"},
 	{"LBENR Mux", "Loopback", "LTENR Mux"},
 
 	{"AIFOUTL", NULL, "LBENL Mux"},
-- 
1.8.2.1

RE: [PATCH] ASoC: max98090: add digital mic mux to record path

[Matthew Mowdy]

Understood Mark,

I apologize for any erroneous comments, as I am the hardware engineer not the driver author. I will stay out of the actual driver implementation as some of the notes I added may have been platform specific implementation (integration) and not core driver functionality. Our software engineer can clarify any questions or concerns with the core driver (I believe he is currently reviewing the proposed patch).

The key information I needed to add from a hardware side was the full functionality of the digital microphone enable bits. I was concerned, and wanted to insure any changes reflected that they are primarily enables (and are not just MUX control bits) and that they need to operate in tandem for proper hardware configuration. To that end I hope the valid use cases are helpful  to the continuing development. 

Thank you for your feedback, and I apologize for any odd formatting. It looks like all correspondence is in plain text and I was using HTML. 

Matthew Mowdy

-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org] 
Sent: Thursday, May 16, 2013 10:53 AM
To: Matthew Mowdy
Cc: abrestic@chromium.org; lgirdwood@gmail.com; perex@perex.cz; tiwai@suse.de; Sachin Kamat; Kuninori Morimoto; Dylan Reid; linux-kernel@vger.kernel.org; Ralph Birt; Evan Ragsdale
Subject: Re: [PATCH] ASoC: max98090: add digital mic mux to record path

On Wed, May 15, 2013 at 09:10:23PM -0700, Matthew Mowdy wrote:

Sorry, didn't manage to find the content I meant to reply to in here due to the formatting issues I mentioned in my mail and the enormous reams of datsheet that were pasted in.  Anyway....

> Default Routing:
> Of note (software can confirm) I believe the default DAPM routing for the driver was intentional setup such that :

This indicates that the driver is broken, the driver should be using the chip defaults.

> Record:

> o   If a headset MIC is present, record from it.
> 
> o   If not, digital MIC is the default record input.

This is a bug, the driver should not be making any automatic routing decisions.  The behaviour you describe would break some fairly obvious use cases like speakerphone while headset is connected.

> Playback:

> o   When headphones are detected, playback is through the headphone output.

> o   When headphones are not present, playback was through the speaker outputs.

Similarly here, this is junk.  

> The defaults can be modified by each end user as needed (for analog 
> microphones, line inputs, etc.). Please insure any patch to the 
> baseline driver reflects the correct operation of the digital 
> microphone enable bits, and preserves the intended default operation.

No, this is stupid and will be buggy.  The register default values need to reflect the silicon defaults, the frameworks (both ASoC and regmap) do things like suppress writes that have no effect and rely on knowing the silicon defaults for that.

The framework already allows the application layer to configure the device through the controls exposed to userspace, if people need to modify a CODEC driver for their system that reflects a problem in the CODEC driver.

Re: [PATCH v2] ASoC: max98090: add digital mic mux to record path

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

On Thu, May 16, 2013 at 12:03:54PM -0700, Andrew Bresticker wrote:
> The max98090 driver currently treats the digital mic enable as a supply
> on the record path, causing the digital mic enable to always be turned on
> when attempting to record.  This is incorrect, however, since the digital

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]