On Mon, 12 Aug 2019 00:19:17 +0200, Dennis Mungai wrote: > > Hello there, > > The patch attached below is based on Jeremy Soller's prior work on: > https://patchwork.kernel.org/patch/9552671/ , copied herein. > > Cleaned up to pass checkpatch.pl tests. > > I can confirm that this patch does indeed fix the audio DAC init on > the Clevo based P775TM1-R chassis with no issues. Thanks for the patch. But this needs more consideration, unfortunately. > From 9f6b7f51a8be738bb588e8d6b0f4d9fb8ac5a0ce Mon Sep 17 00:00:00 2001 > From: brainiarc7 <dmngaie@xxxxxxxxx> > Date: Mon, 12 Aug 2019 00:43:55 +0300 > Subject: [PATCH 1/1] Fix ESS Sabre DAC init for Clevo laptops > > Signed-off-by: brainiarc7 <dmngaie@xxxxxxxxx> First off, the sign-off needs to have the real name. It's a mandatory legal matter. And we need a proper description of the patch in the changelog. About the code change: > +static void alc898_clevo_dac_hook(struct hda_codec *codec, > + struct hda_jack_callback *jack) > +{ > + int val; > + > + // Read ESS DAC status > + snd_hda_codec_write(codec, codec->core.afg, 0, AC_VERB_SET_GPIO_MASK, 4); > + snd_hda_codec_write(codec, codec->core.afg, 0, AC_VERB_SET_GPIO_DIRECTION, 0); Usually the mask and the direction bits are set once at initialization. > + val = snd_hda_codec_read(codec, codec->core.afg, 0, AC_VERB_GET_GPIO_DATA, 0); > + val &= 0x04; > + > + if (val == 0) { > + // Set VREF on headphone pin to 80% > + val = snd_hda_codec_get_pin_target(codec, 0x1b); > + val |= AC_PINCTL_VREF_80; > + snd_hda_set_pin_ctl(codec, 0x1b, val); > + } else { > + // Set VREF on headphone pin to HIZ > + val = snd_hda_codec_get_pin_target(codec, 0x1b); > + val = val & 0xfff8; > + snd_hda_set_pin_ctl(codec, 0x1b, val); > + } ... and this whole function looks strange. Why it's a jack detection callback although the jack state isn't checked at all? > +} > + > +static void alc898_fixup_clevo(struct hda_codec *codec, > + const struct hda_fixup *fix, int action) > +{ > + switch (action) { > + case HDA_FIXUP_ACT_PRE_PROBE: > + snd_hda_jack_detect_enable_callback(codec, 0x1b, alc898_clevo_dac_hook); Is it really the jack detection on NID 0x1b? This is no pin widget but a mixer widget which has no such capability. > + break; > + case HDA_FIXUP_ACT_INIT: > + snd_hda_codec_read(codec, 0x1b, 0, AC_VERB_SET_PIN_WIDGET_CONTROL, 4); Use the proper constant. > + break; > + } > +} > + > static void alc_fixup_bass_chmap(struct hda_codec *codec, > const struct hda_fixup *fix, int action); > > @@ -2350,7 +2388,11 @@ static const struct hda_fixup alc882_fixups[] = { > [ALC887_FIXUP_BASS_CHMAP] = { > .type = HDA_FIXUP_FUNC, > .v.func = alc_fixup_bass_chmap, > - }, > + }, [ALC898_FIXUP_CLEVO_SPDIF] = { > + .type = HDA_FIXUP_FUNC, > + .v.func = alc898_fixup_clevo, > + }, > + > [ALC1220_FIXUP_GB_DUAL_CODECS] = { > .type = HDA_FIXUP_FUNC, > .v.func = alc1220_fixup_gb_dual_codecs, > @@ -2459,6 +2501,11 @@ static const struct snd_pci_quirk alc882_fixup_tbl[] = { > {} > }; > > +static const struct snd_pci_quirk alc898_fixup_tbl[] = { > + SND_PCI_QUIRK_VENDOR(0x1558, "Clevo laptop", ALC898_FIXUP_CLEVO_SPDIF), > + {} > +}; > + > static const struct hda_model_fixup alc882_fixup_models[] = { > {.id = ALC882_FIXUP_ABIT_AW9D_MAX, .name = "abit-aw9d"}, > {.id = ALC882_FIXUP_LENOVO_Y530, .name = "lenovo-y530"}, > @@ -2524,6 +2571,11 @@ static int patch_alc882(struct hda_codec *codec) > case 0x10ec0900: > case 0x10ec1220: > break; > + case 0x10ec0898: > + case 0x10ec0899: > + snd_hda_pick_fixup(codec, NULL, alc898_fixup_tbl, > + alc882_fixups); > + break; This is no-go, will break other machines. You can put the vendor catch-all entry in the existing table, and check the codec IDs in the fixup function instead. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
