[RFC] Fix internal mic for Lenovo Ideapad U300s

[RFC] Fix internal mic for Lenovo Ideapad U300s

[David Henningsson]

The internal mic input is phase inverted on one channel.
To avoid people in userspace summing the channels together
and get zero result, use a separate mixer control for the
inverted channel.

BugLink: https://bugs.launchpad.net/bugs/903853
Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---

Inverted Internal Mic, take 2.

I refined my idea a little. By calling the left channel "Internal Mic"
and the right channel "Inverted Internal Mic", we're giving the user a choice
whether to activate it or keeping it muted.

For PulseAudio, the quick fix is just to make sure the "Inverted Internal Mic"
control is muted. If we in the long run want to actually handle this and swap 
the phase around, we now also have a reasonable indicator if we should do this, 
by checking if the "Inverted Internal Mic" control exists.

For the ALSA init DB (i e, should the "Inverted Internal Mic" be initially muted?)
that is no big deal for me, since this will be overwritten by PulseAudio later
anyway. 

I'm also open for better names than "Inverted Internal Mic" if you have any.

As with the previous version, this is a draft version of the patch, and could be
done slightly more elegant I guess.

What do you say?

 sound/pci/hda/patch_conexant.c |   45 +++++++++++++++++++++++++++++----------
 1 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index e6eafb1..cd3859b 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -142,6 +142,7 @@ struct conexant_spec {
 	unsigned int asus:1;
 	unsigned int pin_eapd_ctrls:1;
 	unsigned int single_adc_amp:1;
+	unsigned int fixup_stereo_dmic:1;
 
 	unsigned int adc_switching:1;
 
@@ -4107,9 +4108,9 @@ static int cx_auto_init(struct hda_codec *codec)
 
 static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
 			      const char *dir, int cidx,
-			      hda_nid_t nid, int hda_dir, int amp_idx)
+			      hda_nid_t nid, int hda_dir, int amp_idx, int chs)
 {
-	static char name[32];
+	static char name[44];
 	static struct snd_kcontrol_new knew[] = {
 		HDA_CODEC_VOLUME(name, 0, 0, 0),
 		HDA_CODEC_MUTE(name, 0, 0, 0),
@@ -4119,7 +4120,7 @@ static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
 
 	for (i = 0; i < 2; i++) {
 		struct snd_kcontrol *kctl;
-		knew[i].private_value = HDA_COMPOSE_AMP_VAL(nid, 3, amp_idx,
+		knew[i].private_value = HDA_COMPOSE_AMP_VAL(nid, chs, amp_idx,
 							    hda_dir);
 		knew[i].subdevice = HDA_SUBDEV_AMP_FLAG;
 		knew[i].index = cidx;
@@ -4138,7 +4139,7 @@ static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
 }
 
 #define cx_auto_add_volume(codec, str, dir, cidx, nid, hda_dir)		\
-	cx_auto_add_volume_idx(codec, str, dir, cidx, nid, hda_dir, 0)
+	cx_auto_add_volume_idx(codec, str, dir, cidx, nid, hda_dir, 0, 3)
 
 #define cx_auto_add_pb_volume(codec, nid, str, idx)			\
 	cx_auto_add_volume(codec, str, " Playback", idx, nid, HDA_OUTPUT)
@@ -4216,14 +4217,24 @@ static int cx_auto_add_capture_volume(struct hda_codec *codec, hda_nid_t nid,
 	int i;
 
 	for (i = 0; i < spec->num_adc_nids; i++) {
+		int chs = 3;
 		hda_nid_t adc_nid = spec->adc_nids[i];
 		int idx = get_input_connection(codec, adc_nid, nid);
 		if (idx < 0)
 			continue;
 		if (spec->single_adc_amp)
 			idx = 0;
+
+		if (spec->fixup_stereo_dmic && (strcmp("Internal Mic", label) == 0)) {
+			/* Make two independent kcontrols */
+		        int err = cx_auto_add_volume_idx(codec, "Inverted Internal Mic", pfx,
+					      cidx, adc_nid, HDA_INPUT, idx, 2);
+			if (err < 0)
+				return err;
+			chs = 1;
+		}
 		return cx_auto_add_volume_idx(codec, label, pfx,
-					      cidx, adc_nid, HDA_INPUT, idx);
+					      cidx, adc_nid, HDA_INPUT, idx, chs);
 	}
 	return 0;
 }
@@ -4405,22 +4416,30 @@ static void apply_pincfg(struct hda_codec *codec, const struct cxt_pincfg *cfg)
 
 }
 
-static void apply_pin_fixup(struct hda_codec *codec,
+enum {
+	CXT_PINCFG_LENOVO_X200,
+	CXT_FIXUP_STEREO_DMIC,
+};
+
+static void apply_fixup(struct hda_codec *codec,
 			    const struct snd_pci_quirk *quirk,
 			    const struct cxt_pincfg **table)
 {
+	struct conexant_spec *spec = codec->spec;
+
 	quirk = snd_pci_quirk_lookup(codec->bus->pci, quirk);
-	if (quirk) {
+	if (quirk && table[quirk->value]) {
 		snd_printdd(KERN_INFO "hda_codec: applying pincfg for %s\n",
 			    quirk->name);
 		apply_pincfg(codec, table[quirk->value]);
 	}
+	if (quirk->value == CXT_FIXUP_STEREO_DMIC) {
+		snd_printdd(KERN_INFO "hda_codec: applying internal mic workaround for %s\n",
+			    quirk->name);
+		spec->fixup_stereo_dmic = 1;
+	}
 }
 
-enum {
-	CXT_PINCFG_LENOVO_X200,
-};
-
 static const struct cxt_pincfg cxt_pincfg_lenovo_x200[] = {
 	{ 0x16, 0x042140ff }, /* HP (seq# overridden) */
 	{ 0x17, 0x21a11000 }, /* dock-mic */
@@ -4431,10 +4450,12 @@ static const struct cxt_pincfg cxt_pincfg_lenovo_x200[] = {
 
 static const struct cxt_pincfg *cxt_pincfg_tbl[] = {
 	[CXT_PINCFG_LENOVO_X200] = cxt_pincfg_lenovo_x200,
+	[CXT_FIXUP_STEREO_DMIC] = NULL,
 };
 
 static const struct snd_pci_quirk cxt_fixups[] = {
 	SND_PCI_QUIRK(0x17aa, 0x20f2, "Lenovo X200", CXT_PINCFG_LENOVO_X200),
+	SND_PCI_QUIRK(0x17aa, 0x3975, "Lenovo U300s", CXT_FIXUP_STEREO_DMIC),
 	{}
 };
 
@@ -4477,7 +4498,7 @@ static int patch_conexant_auto(struct hda_codec *codec)
 		break;
 	}
 
-	apply_pin_fixup(codec, cxt_fixups, cxt_pincfg_tbl);
+	apply_fixup(codec, cxt_fixups, cxt_pincfg_tbl);
 
 	/* Show mute-led control only on HP laptops
 	 * This is a sort of white-list: on HP laptops, EAPD corresponds
-- 
1.7.9.1

Re: [RFC] Fix internal mic for Lenovo Ideapad U300s

[Takashi Iwai]

At Thu, 29 Mar 2012 22:48:20 +0200,
David Henningsson wrote:
> 
> The internal mic input is phase inverted on one channel.
> To avoid people in userspace summing the channels together
> and get zero result, use a separate mixer control for the
> inverted channel.
> 
> BugLink: https://bugs.launchpad.net/bugs/903853
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
> 
> Inverted Internal Mic, take 2.
> 
> I refined my idea a little. By calling the left channel "Internal Mic"
> and the right channel "Inverted Internal Mic", we're giving the user a choice
> whether to activate it or keeping it muted.
> 
> For PulseAudio, the quick fix is just to make sure the "Inverted Internal Mic"
> control is muted. If we in the long run want to actually handle this and swap 
> the phase around, we now also have a reasonable indicator if we should do this, 
> by checking if the "Inverted Internal Mic" control exists.
> 
> For the ALSA init DB (i e, should the "Inverted Internal Mic" be initially muted?)
> that is no big deal for me, since this will be overwritten by PulseAudio later
> anyway. 
> 
> I'm also open for better names than "Inverted Internal Mic" if you have any.
> 
> As with the previous version, this is a draft version of the patch, and could be
> done slightly more elegant I guess.
> 
> What do you say?

Good, this sounds like a reasonable solution.
About the name, I'm not sure, too.  I'm no good godfather.
Maybe other people have a better clue.

Once when the patch is finialized, let me know.

I think we clean up the fixup functions by moving the alc_fixup_*() as
common functions.  ALC_FIXUP_SKU can be converted to an ALC_FIXUP_FUNC,
then there is no Realtek-specific things there.


thanks,

Takashi


> 
>  sound/pci/hda/patch_conexant.c |   45 +++++++++++++++++++++++++++++----------
>  1 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> index e6eafb1..cd3859b 100644
> --- a/sound/pci/hda/patch_conexant.c
> +++ b/sound/pci/hda/patch_conexant.c
> @@ -142,6 +142,7 @@ struct conexant_spec {
>  	unsigned int asus:1;
>  	unsigned int pin_eapd_ctrls:1;
>  	unsigned int single_adc_amp:1;
> +	unsigned int fixup_stereo_dmic:1;
>  
>  	unsigned int adc_switching:1;
>  
> @@ -4107,9 +4108,9 @@ static int cx_auto_init(struct hda_codec *codec)
>  
>  static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
>  			      const char *dir, int cidx,
> -			      hda_nid_t nid, int hda_dir, int amp_idx)
> +			      hda_nid_t nid, int hda_dir, int amp_idx, int chs)
>  {
> -	static char name[32];
> +	static char name[44];
>  	static struct snd_kcontrol_new knew[] = {
>  		HDA_CODEC_VOLUME(name, 0, 0, 0),
>  		HDA_CODEC_MUTE(name, 0, 0, 0),
> @@ -4119,7 +4120,7 @@ static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
>  
>  	for (i = 0; i < 2; i++) {
>  		struct snd_kcontrol *kctl;
> -		knew[i].private_value = HDA_COMPOSE_AMP_VAL(nid, 3, amp_idx,
> +		knew[i].private_value = HDA_COMPOSE_AMP_VAL(nid, chs, amp_idx,
>  							    hda_dir);
>  		knew[i].subdevice = HDA_SUBDEV_AMP_FLAG;
>  		knew[i].index = cidx;
> @@ -4138,7 +4139,7 @@ static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
>  }
>  
>  #define cx_auto_add_volume(codec, str, dir, cidx, nid, hda_dir)		\
> -	cx_auto_add_volume_idx(codec, str, dir, cidx, nid, hda_dir, 0)
> +	cx_auto_add_volume_idx(codec, str, dir, cidx, nid, hda_dir, 0, 3)
>  
>  #define cx_auto_add_pb_volume(codec, nid, str, idx)			\
>  	cx_auto_add_volume(codec, str, " Playback", idx, nid, HDA_OUTPUT)
> @@ -4216,14 +4217,24 @@ static int cx_auto_add_capture_volume(struct hda_codec *codec, hda_nid_t nid,
>  	int i;
>  
>  	for (i = 0; i < spec->num_adc_nids; i++) {
> +		int chs = 3;
>  		hda_nid_t adc_nid = spec->adc_nids[i];
>  		int idx = get_input_connection(codec, adc_nid, nid);
>  		if (idx < 0)
>  			continue;
>  		if (spec->single_adc_amp)
>  			idx = 0;
> +
> +		if (spec->fixup_stereo_dmic && (strcmp("Internal Mic", label) == 0)) {
> +			/* Make two independent kcontrols */
> +		        int err = cx_auto_add_volume_idx(codec, "Inverted Internal Mic", pfx,
> +					      cidx, adc_nid, HDA_INPUT, idx, 2);
> +			if (err < 0)
> +				return err;
> +			chs = 1;
> +		}
>  		return cx_auto_add_volume_idx(codec, label, pfx,
> -					      cidx, adc_nid, HDA_INPUT, idx);
> +					      cidx, adc_nid, HDA_INPUT, idx, chs);
>  	}
>  	return 0;
>  }
> @@ -4405,22 +4416,30 @@ static void apply_pincfg(struct hda_codec *codec, const struct cxt_pincfg *cfg)
>  
>  }
>  
> -static void apply_pin_fixup(struct hda_codec *codec,
> +enum {
> +	CXT_PINCFG_LENOVO_X200,
> +	CXT_FIXUP_STEREO_DMIC,
> +};
> +
> +static void apply_fixup(struct hda_codec *codec,
>  			    const struct snd_pci_quirk *quirk,
>  			    const struct cxt_pincfg **table)
>  {
> +	struct conexant_spec *spec = codec->spec;
> +
>  	quirk = snd_pci_quirk_lookup(codec->bus->pci, quirk);
> -	if (quirk) {
> +	if (quirk && table[quirk->value]) {
>  		snd_printdd(KERN_INFO "hda_codec: applying pincfg for %s\n",
>  			    quirk->name);
>  		apply_pincfg(codec, table[quirk->value]);
>  	}
> +	if (quirk->value == CXT_FIXUP_STEREO_DMIC) {
> +		snd_printdd(KERN_INFO "hda_codec: applying internal mic workaround for %s\n",
> +			    quirk->name);
> +		spec->fixup_stereo_dmic = 1;
> +	}
>  }
>  
> -enum {
> -	CXT_PINCFG_LENOVO_X200,
> -};
> -
>  static const struct cxt_pincfg cxt_pincfg_lenovo_x200[] = {
>  	{ 0x16, 0x042140ff }, /* HP (seq# overridden) */
>  	{ 0x17, 0x21a11000 }, /* dock-mic */
> @@ -4431,10 +4450,12 @@ static const struct cxt_pincfg cxt_pincfg_lenovo_x200[] = {
>  
>  static const struct cxt_pincfg *cxt_pincfg_tbl[] = {
>  	[CXT_PINCFG_LENOVO_X200] = cxt_pincfg_lenovo_x200,
> +	[CXT_FIXUP_STEREO_DMIC] = NULL,
>  };
>  
>  static const struct snd_pci_quirk cxt_fixups[] = {
>  	SND_PCI_QUIRK(0x17aa, 0x20f2, "Lenovo X200", CXT_PINCFG_LENOVO_X200),
> +	SND_PCI_QUIRK(0x17aa, 0x3975, "Lenovo U300s", CXT_FIXUP_STEREO_DMIC),
>  	{}
>  };
>  
> @@ -4477,7 +4498,7 @@ static int patch_conexant_auto(struct hda_codec *codec)
>  		break;
>  	}
>  
> -	apply_pin_fixup(codec, cxt_fixups, cxt_pincfg_tbl);
> +	apply_fixup(codec, cxt_fixups, cxt_pincfg_tbl);
>  
>  	/* Show mute-led control only on HP laptops
>  	 * This is a sort of white-list: on HP laptops, EAPD corresponds
> -- 
> 1.7.9.1
> 

Re: [RFC] Fix internal mic for Lenovo Ideapad U300s

[David Henningsson]

On 03/30/2012 04:31 PM, Takashi Iwai wrote:
> At Thu, 29 Mar 2012 22:48:20 +0200,
> David Henningsson wrote:
>>
>> The internal mic input is phase inverted on one channel.
>> To avoid people in userspace summing the channels together
>> and get zero result, use a separate mixer control for the
>> inverted channel.
>>
>> BugLink: https://bugs.launchpad.net/bugs/903853
>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>> ---
>>
>> Inverted Internal Mic, take 2.
>>
>> I refined my idea a little. By calling the left channel "Internal Mic"
>> and the right channel "Inverted Internal Mic", we're giving the user a choice
>> whether to activate it or keeping it muted.
>>
>> For PulseAudio, the quick fix is just to make sure the "Inverted Internal Mic"
>> control is muted. If we in the long run want to actually handle this and swap
>> the phase around, we now also have a reasonable indicator if we should do this,
>> by checking if the "Inverted Internal Mic" control exists.
>>
>> For the ALSA init DB (i e, should the "Inverted Internal Mic" be initially muted?)
>> that is no big deal for me, since this will be overwritten by PulseAudio later
>> anyway.
>>
>> I'm also open for better names than "Inverted Internal Mic" if you have any.
>>
>> As with the previous version, this is a draft version of the patch, and could be
>> done slightly more elegant I guess.
>>
>> What do you say?
>
> Good, this sounds like a reasonable solution.

I'm happy you like it!

> About the name, I'm not sure, too.  I'm no good godfather.
> Maybe other people have a better clue.
>
> Once when the patch is finialized, let me know.

I've been working with it today, and I'm sending a new patch in a 
separate email.

> I think we clean up the fixup functions by moving the alc_fixup_*() as
> common functions.  ALC_FIXUP_SKU can be converted to an ALC_FIXUP_FUNC,
> then there is no Realtek-specific things there.

I'm happy for all consolidating between codec drivers. Perhaps this 
could be a good start for a new hda_auto.c file that separates the 
low-level communication in hda_codec.c, from the auto-parser related 
stuff which could reside in hda_auto.c?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: [RFC] Fix internal mic for Lenovo Ideapad U300s

[Takashi Iwai]

At Mon, 02 Apr 2012 15:39:30 +0200,
David Henningsson wrote:
> 
> On 03/30/2012 04:31 PM, Takashi Iwai wrote:
> > At Thu, 29 Mar 2012 22:48:20 +0200,
> > David Henningsson wrote:
> >>
> >> The internal mic input is phase inverted on one channel.
> >> To avoid people in userspace summing the channels together
> >> and get zero result, use a separate mixer control for the
> >> inverted channel.
> >>
> >> BugLink: https://bugs.launchpad.net/bugs/903853
> >> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
> >> ---
> >>
> >> Inverted Internal Mic, take 2.
> >>
> >> I refined my idea a little. By calling the left channel "Internal Mic"
> >> and the right channel "Inverted Internal Mic", we're giving the user a choice
> >> whether to activate it or keeping it muted.
> >>
> >> For PulseAudio, the quick fix is just to make sure the "Inverted Internal Mic"
> >> control is muted. If we in the long run want to actually handle this and swap
> >> the phase around, we now also have a reasonable indicator if we should do this,
> >> by checking if the "Inverted Internal Mic" control exists.
> >>
> >> For the ALSA init DB (i e, should the "Inverted Internal Mic" be initially muted?)
> >> that is no big deal for me, since this will be overwritten by PulseAudio later
> >> anyway.
> >>
> >> I'm also open for better names than "Inverted Internal Mic" if you have any.
> >>
> >> As with the previous version, this is a draft version of the patch, and could be
> >> done slightly more elegant I guess.
> >>
> >> What do you say?
> >
> > Good, this sounds like a reasonable solution.
> 
> I'm happy you like it!
> 
> > About the name, I'm not sure, too.  I'm no good godfather.
> > Maybe other people have a better clue.
> >
> > Once when the patch is finialized, let me know.
> 
> I've been working with it today, and I'm sending a new patch in a 
> separate email.
> 
> > I think we clean up the fixup functions by moving the alc_fixup_*() as
> > common functions.  ALC_FIXUP_SKU can be converted to an ALC_FIXUP_FUNC,
> > then there is no Realtek-specific things there.
> 
> I'm happy for all consolidating between codec drivers. Perhaps this 
> could be a good start for a new hda_auto.c file that separates the 
> low-level communication in hda_codec.c, from the auto-parser related 
> stuff which could reside in hda_auto.c?

Yeah, some code reorganization would be needed in near future.


Takashi