Re: [PATCH] ALSA: hda: fixup headset for ASUS GX502 laptop

From: Takashi Iwai <tiwai@suse.de>
To: Luke Jones <luke@ljones.dev>
Cc: hui.wang@canonical.com, alsa-devel@alsa-project.org, kailang@realtek.com
Subject: Re: [PATCH] ALSA: hda: fixup headset for ASUS GX502 laptop
Date: Mon, 07 Sep 2020 09:03:05 +0200	[thread overview]
Message-ID: <s5ha6y2132u.wl-tiwai@suse.de> (raw)
In-Reply-To: <20200906082507.38091-1-luke@ljones.dev>

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