diff mbox series

ALSA: hda: fixup headset for ASUS GX502 laptop

Message ID 20200906082507.38091-1-luke@ljones.dev (mailing list archive)
State New, archived
Headers show
Series ALSA: hda: fixup headset for ASUS GX502 laptop | expand

Commit Message

Luke D. Jones Sept. 6, 2020, 8:25 a.m. UTC 
From: Luke D Jones <luke@ljones.dev>

The GX502 requires a few steps to enable the headset i/o: pincfg,
verbs to enable and unmute the amp used for headpone out, and
a jacksense callback to toggle output via internal or jack using
a verb.

Signed-off-by: Luke Jones <luke@ljones.dev>
---
 sound/pci/hda/patch_realtek.c | 70 ++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

Comments

Takashi Iwai Sept. 7, 2020, 7:03 a.m. UTC | #1 
On Sun, 06 Sep 2020 10:25:07 +0200,
Luke Jones wrote:
> 
> From: Luke D Jones <luke@ljones.dev>
> 
> The GX502 requires a few steps to enable the headset i/o: pincfg,
> verbs to enable and unmute the amp used for headpone out, and
> a jacksense callback to toggle output via internal or jack using
> a verb.

Thanks for the patch.  The code changes look mostly good, but a few
minor coding problems (mostly coding style) are seen.
Could you resubmit with fixes?

> Signed-off-by: Luke Jones <luke@ljones.dev>

The SOB doesn't match with From line.  Is it intentional?

....
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -5424,7 +5424,7 @@ static void alc_fixup_headset_mode_alc255_no_hp_mic(struct hda_codec *codec,
>  		struct alc_spec *spec = codec->spec;
>  		spec->parse_flags |= HDA_PINCFG_HEADSET_MIC;
>  		alc255_set_default_jack_type(codec);
> -	} 
> +	}
>  	else
>  		alc_fixup_headset_mode(codec, fix, action);
>  }

Please drop the unrelated change.

....
> +static void alc294_gx502_toggle_output(struct hda_codec *codec,
> +					struct hda_jack_callback *cb)
> +{
> +	/* The Windows driver sets the codec up in a very different way where
> +	 * it appears to leave 0x10 = 0x8a20 set. For Linux we need to toggle it
> +	 */
> +	if (snd_hda_jack_detect_state(codec, 0x21) == HDA_JACK_PRESENT) {
> +		alc_write_coef_idx(codec, 0x10, 0x8a20);
> +	} else {
> +		alc_write_coef_idx(codec, 0x10, 0x0a20);
> +	}

Remove braces for a single if line.  Checkpatch would suggest it, too.

....
> @@ -7338,6 +7376,35 @@ static const struct hda_fixup alc269_fixups[] = {
>  		.chained = true,
>  		.chain_id = ALC294_FIXUP_ASUS_HEADSET_MIC
>  	},
> +	[ALC294_FIXUP_ASUS_GX502_PINS] = {
> +		.type = HDA_FIXUP_PINS,
> +		.v.pins = (const struct hda_pintbl[]) {
> +			{ 0x19, 0x03a11050 }, /* front HP mic */
> +			{ 0x1a, 0x01a11830 }, //0x00a11030 }, /* rear external mic */

The doubly comments look awkward.  Please reformat it.

....
> +	[ALC294_FIXUP_ASUS_GX502_VERBS] = {
> +		.type = HDA_FIXUP_VERBS,
> +		.v.verbs = (const struct hda_verb[]) {
> +		    /* set 0x15 to HP-OUT ctrl */

Please align the comment at the right tab.

> +			{ 0x15, AC_VERB_SET_PIN_WIDGET_CONTROL, 0xc0 },
> +			/* unmute the 0x15 amp */
> +			{ 0x15, AC_VERB_SET_AMP_GAIN_MUTE, 0xb000 },
> +			/* set 0x0a input converter to digital */
> +			{ 0x0a, 0x70d, 0x01 },

Use AC_VERB_SET_DIGI_CONVERT_1.
And, how is this related with the headset fix?  Is this really
mandatory?  If this widget is really wired, usually the digital I/O is
controlled via IEC9158 status mixer.


thanks,

Takashi
Luke D. Jones Sept. 7, 2020, 8:09 a.m. UTC | #2 
On Mon, Sep 7, 2020 at 09:03, Takashi Iwai <tiwai@suse.de> wrote:
>>  Signed-off-by: Luke Jones <luke@ljones.dev>
> 
> The SOB doesn't match with From line.  Is it intentional?
I missed this sorry. Have corrected.

>>  -	}
>>  +	}
>>   	else
>>   		alc_fixup_headset_mode(codec, fix, action);
>>   }
> 
> Please drop the unrelated change.
Looks like my vim settings removed an ending white-space. I'll remove 
the change.

> ....
>>  +static void alc294_gx502_toggle_output(struct hda_codec *codec,
>>  +					struct hda_jack_callback *cb)
>>  +{
>>  +	/* The Windows driver sets the codec up in a very different way 
>> where
>>  +	 * it appears to leave 0x10 = 0x8a20 set. For Linux we need to 
>> toggle it
>>  +	 */
>>  +	if (snd_hda_jack_detect_state(codec, 0x21) == HDA_JACK_PRESENT) {
>>  +		alc_write_coef_idx(codec, 0x10, 0x8a20);
>>  +	} else {
>>  +		alc_write_coef_idx(codec, 0x10, 0x0a20);
>>  +	}
> 
> Remove braces for a single if line.  Checkpatch would suggest it, too.
Done

> ....
>>  @@ -7338,6 +7376,35 @@ static const struct hda_fixup 
>> alc269_fixups[] = {
>>   		.chained = true,
>>   		.chain_id = ALC294_FIXUP_ASUS_HEADSET_MIC
>>   	},
>>  +	[ALC294_FIXUP_ASUS_GX502_PINS] = {
>>  +		.type = HDA_FIXUP_PINS,
>>  +		.v.pins = (const struct hda_pintbl[]) {
>>  +			{ 0x19, 0x03a11050 }, /* front HP mic */
>>  +			{ 0x1a, 0x01a11830 }, //0x00a11030 }, /* rear external mic */
> 
> The doubly comments look awkward.  Please reformat it.
Missed this also, sorry :(

> ....
>>  +	[ALC294_FIXUP_ASUS_GX502_VERBS] = {
>>  +		.type = HDA_FIXUP_VERBS,
>>  +		.v.verbs = (const struct hda_verb[]) {
>>  +		    /* set 0x15 to HP-OUT ctrl */
> 
> Please align the comment at the right tab.
Done

>>  +			{ 0x15, AC_VERB_SET_PIN_WIDGET_CONTROL, 0xc0 },
>>  +			/* unmute the 0x15 amp */
>>  +			{ 0x15, AC_VERB_SET_AMP_GAIN_MUTE, 0xb000 },
>>  +			/* set 0x0a input converter to digital */
>>  +			{ 0x0a, 0x70d, 0x01 },
> 
> Use AC_VERB_SET_DIGI_CONVERT_1.
> And, how is this related with the headset fix?  Is this really
> mandatory?  If this widget is really wired, usually the digital I/O is
> controlled via IEC9158 status mixer.
This I'm not actually sure about, I'm sort of fumbling around trying to 
get the rear
mic jack working. I have now removed the comment and 0x0a line so the 
patch is
focused solely on the headset.

Many thanks for the code review. I will submit the fixed version now.

Kind regards,
Luke.
diff mbox series

Patch

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index c521a1f17096..d2052a6e3b13 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -5424,7 +5424,7 @@  static void alc_fixup_headset_mode_alc255_no_hp_mic(struct hda_codec *codec,
 		struct alc_spec *spec = codec->spec;
 		spec->parse_flags |= HDA_PINCFG_HEADSET_MIC;
 		alc255_set_default_jack_type(codec);
-	} 
+	}
 	else
 		alc_fixup_headset_mode(codec, fix, action);
 }
@@ -5993,6 +5993,41 @@  static void alc_fixup_disable_mic_vref(struct hda_codec *codec,
 		snd_hda_codec_set_pin_target(codec, 0x19, PIN_VREFHIZ);
 }
 
+
+static void alc294_gx502_toggle_output(struct hda_codec *codec,
+					struct hda_jack_callback *cb)
+{
+	/* The Windows driver sets the codec up in a very different way where
+	 * it appears to leave 0x10 = 0x8a20 set. For Linux we need to toggle it
+	 */
+	if (snd_hda_jack_detect_state(codec, 0x21) == HDA_JACK_PRESENT) {
+		alc_write_coef_idx(codec, 0x10, 0x8a20);
+	} else {
+		alc_write_coef_idx(codec, 0x10, 0x0a20);
+	}
+}
+
+static void alc294_fixup_gx502_hp(struct hda_codec *codec,
+					const struct hda_fixup *fix, int action)
+{
+	/* Pin 0x21: headphones/headset mic */
+	if (!is_jack_detectable(codec, 0x21))
+		return;
+
+	switch (action) {
+	case HDA_FIXUP_ACT_PRE_PROBE:
+		snd_hda_jack_detect_enable_callback(codec, 0x21,
+				alc294_gx502_toggle_output);
+		break;
+	case HDA_FIXUP_ACT_INIT:
+		/* Make sure to start in a correct state, i.e. if
+		 * headphones have been plugged in before powering up the system
+		 */
+		alc294_gx502_toggle_output(codec, NULL);
+		break;
+	}
+}
+
 static void  alc285_fixup_hp_gpio_amp_init(struct hda_codec *codec,
 			      const struct hda_fixup *fix, int action)
 {
@@ -6172,6 +6207,9 @@  enum {
 	ALC285_FIXUP_THINKPAD_X1_GEN7,
 	ALC285_FIXUP_THINKPAD_HEADSET_JACK,
 	ALC294_FIXUP_ASUS_HPE,
+	ALC294_FIXUP_ASUS_GX502_HP,
+	ALC294_FIXUP_ASUS_GX502_PINS,
+	ALC294_FIXUP_ASUS_GX502_VERBS,
 	ALC294_FIXUP_ASUS_COEF_1B,
 	ALC285_FIXUP_HP_GPIO_LED,
 	ALC285_FIXUP_HP_MUTE_LED,
@@ -7338,6 +7376,35 @@  static const struct hda_fixup alc269_fixups[] = {
 		.chained = true,
 		.chain_id = ALC294_FIXUP_ASUS_HEADSET_MIC
 	},
+	[ALC294_FIXUP_ASUS_GX502_PINS] = {
+		.type = HDA_FIXUP_PINS,
+		.v.pins = (const struct hda_pintbl[]) {
+			{ 0x19, 0x03a11050 }, /* front HP mic */
+			{ 0x1a, 0x01a11830 }, //0x00a11030 }, /* rear external mic */
+			{ 0x21, 0x03211020 }, /* HP out */
+			{ }
+		},
+		.chained = true,
+		.chain_id = ALC294_FIXUP_ASUS_GX502_VERBS
+	},
+	[ALC294_FIXUP_ASUS_GX502_VERBS] = {
+		.type = HDA_FIXUP_VERBS,
+		.v.verbs = (const struct hda_verb[]) {
+		    /* set 0x15 to HP-OUT ctrl */
+			{ 0x15, AC_VERB_SET_PIN_WIDGET_CONTROL, 0xc0 },
+			/* unmute the 0x15 amp */
+			{ 0x15, AC_VERB_SET_AMP_GAIN_MUTE, 0xb000 },
+			/* set 0x0a input converter to digital */
+			{ 0x0a, 0x70d, 0x01 },
+			{ }
+		},
+		.chained = true,
+		.chain_id = ALC294_FIXUP_ASUS_GX502_HP
+	},
+	[ALC294_FIXUP_ASUS_GX502_HP] = {
+		.type = HDA_FIXUP_FUNC,
+		.v.func = alc294_fixup_gx502_hp,
+	},
 	[ALC294_FIXUP_ASUS_COEF_1B] = {
 		.type = HDA_FIXUP_VERBS,
 		.v.verbs = (const struct hda_verb[]) {
@@ -7711,6 +7778,7 @@  static const struct snd_pci_quirk alc269_fixup_tbl[] = {
 	SND_PCI_QUIRK(0x1043, 0x1ccd, "ASUS X555UB", ALC256_FIXUP_ASUS_MIC),
 	SND_PCI_QUIRK(0x1043, 0x1e11, "ASUS Zephyrus G15", ALC289_FIXUP_ASUS_GA502),
 	SND_PCI_QUIRK(0x1043, 0x1f11, "ASUS Zephyrus G14", ALC289_FIXUP_ASUS_GA401),
+	SND_PCI_QUIRK(0x1043, 0x1881, "ASUS Zephyrus S/M", ALC294_FIXUP_ASUS_GX502_PINS),
 	SND_PCI_QUIRK(0x1043, 0x3030, "ASUS ZN270IE", ALC256_FIXUP_ASUS_AIO_GPIO2),
 	SND_PCI_QUIRK(0x1043, 0x831a, "ASUS P901", ALC269_FIXUP_STEREO_DMIC),
 	SND_PCI_QUIRK(0x1043, 0x834a, "ASUS S101", ALC269_FIXUP_STEREO_DMIC),