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

From: Luke Jones <luke@ljones.dev>
To: Takashi Iwai <tiwai@suse.de>
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 20:09:25 +1200	[thread overview]
Message-ID: <PZ3AGQ.SOCXB34FKYDH1@ljones.dev> (raw)
In-Reply-To: <s5ha6y2132u.wl-tiwai@suse.de>

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.