Add support for Sabre HiFi on System76 and Clevo laptops
diff mbox series

Message ID CAKKYfmGOWD2w2Ozr8vwLTYpjdJ4bTegxWqPjhzyBtB5TVvqCzg@mail.gmail.com
State New
Headers show
Series
  • Add support for Sabre HiFi on System76 and Clevo laptops
Related show

Commit Message

Dennis Mungai Aug. 11, 2019, 10:19 p.m. UTC
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.

Warm regards,

Dennis Mungai

Comments

Takashi Iwai Aug. 14, 2019, 6:46 a.m. UTC | #1
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@gmail.com>
> 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@gmail.com>

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

Patch
diff mbox series

From 9f6b7f51a8be738bb588e8d6b0f4d9fb8ac5a0ce Mon Sep 17 00:00:00 2001
From: brainiarc7 <dmngaie@gmail.com>
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@gmail.com>
---
 sound/pci/hda/patch_realtek.c | 54 ++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index de224cbea..d8f9862cb 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -1899,6 +1899,7 @@  enum {
    ALC882_FIXUP_NO_PRIMARY_HP,
    ALC887_FIXUP_ASUS_BASS,
    ALC887_FIXUP_BASS_CHMAP,
+   ALC898_FIXUP_CLEVO_SPDIF,
    ALC1220_FIXUP_GB_DUAL_CODECS,
    ALC1220_FIXUP_CLEVO_P950,
    ALC1220_FIXUP_CLEVO_PB51ED,
@@ -2029,6 +2030,43 @@  static void alc882_fixup_no_primary_hp(struct hda_codec *codec,
    }
 }
 
+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);
+       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);
+       }
+}
+
+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);
+	       break;
+       case HDA_FIXUP_ACT_INIT:
+	       snd_hda_codec_read(codec, 0x1b, 0, AC_VERB_SET_PIN_WIDGET_CONTROL, 4);
+	       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;
    default:
        /* ALC883 and variants */
        alc_fix_pll_init(codec, 0x20, 0x0a, 10);
-- 
2.17.1