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