On Tue, 14 Jul 2015 19:44:31 +0200, Keith Packard wrote:

This laptop needs GPIO4 pulled high to enable the headphone amplifier, and has a mute LED on GPIO3. I modelled the patch on the existing GPIO4 code which pulls the line low for the same purpose; this time, the HP amp line is pulled high.

Signed-off-by: Keith Packard keithp@keithp.com

sound/pci/hda/patch_realtek.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 6d01045..de031f7 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -3436,6 +3436,29 @@ static void alc280_fixup_hp_gpio4(struct hda_codec *codec, } }

+static void alc280_fixup_hp_gpio4_hp_amp(struct hda_codec *codec,

• 			 const struct hda_fixup *fix,
  

• 			 int action)
  

+{

• /* Pull GPIO4 high to enable headphone amp */
• struct alc_spec *spec = codec->spec;
• static const struct hda_verb gpio_init[] = {

• { 0x01, AC_VERB_SET_GPIO_MASK, 0x18 },
  

• { 0x01, AC_VERB_SET_GPIO_DIRECTION, 0x18 },
  

• { 0x01, AC_VERB_SET_GPIO_DATA, 0x10 },
  

• {}
  

• };
• if (action == HDA_FIXUP_ACT_PRE_PROBE) {

• spec->gen.vmaster_mute.hook = alc_fixup_gpio_mute_hook;
  

• spec->gpio_led = 0x10;
  

• spec->mute_led_polarity = 0;
  

• spec->gpio_mute_led_mask = 0x08;
  

• snd_hda_add_verbs(codec, gpio_init);
  

• codec->power_filter = led_power_filter;
  

• }

+}

static void gpio2_mic_hotkey_event(struct hda_codec *codec, struct hda_jack_callback *event) { @@ -4512,6 +4535,7 @@ enum { ALC286_FIXUP_HP_GPIO_LED, ALC280_FIXUP_HP_GPIO2_MIC_HOTKEY, ALC280_FIXUP_HP_DOCK_PINS,

• ALC280_FIXUP_HP_GPIO4_HP_AMP, ALC288_FIXUP_DELL_HEADSET_MODE, ALC288_FIXUP_DELL1_MIC_NO_PRESENCE, ALC288_FIXUP_DELL_XPS_13_GPIO6,

@@ -5012,6 +5036,10 @@ static const struct hda_fixup alc269_fixups[] = { .chained = true, .chain_id = ALC280_FIXUP_HP_GPIO4 },

• [ALC280_FIXUP_HP_GPIO4_HP_AMP] = {

• .type = HDA_FIXUP_FUNC,
  

• .v.func = alc280_fixup_hp_gpio4_hp_amp,
  

• }, [ALC288_FIXUP_DELL_HEADSET_MODE] = { .type = HDA_FIXUP_FUNC, .v.func = alc_fixup_headset_mode_dell_alc288,

@@ -5103,6 +5131,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = { SND_PCI_QUIRK(0x103c, 0x22b7, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1), SND_PCI_QUIRK(0x103c, 0x22bf, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1), SND_PCI_QUIRK(0x103c, 0x22cf, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),

• SND_PCI_QUIRK(0x103c, 0x22db, "HP", ALC280_FIXUP_HP_GPIO4_HP_AMP), SND_PCI_QUIRK(0x103c, 0x22dc, "HP", ALC269_FIXUP_HP_GPIO_MIC1_LED), SND_PCI_QUIRK(0x103c, 0x22fb, "HP", ALC269_FIXUP_HP_GPIO_MIC1_LED), /* ALC290 */

Thanks for the patch. But this looks suboptimal, unfortunately, since it keeps the amp always on, and more badly, it would block the power save of the widget root node.

Can just using gpio_mute_led_mask=0x18 and gpio_led=0 (also drop AC_VERB_SET_GPIO_DATA in gpio_init[]) work instead? If GPIO4 is the the amp, we can associate it with the master mute control together with the mute LED. The only concern would be the possible click noise, but it doesn't happen on most machines.

Takashi

On Wed, 15 Jul 2015 04:37:44 +0200, Keith Packard wrote:

Takashi Iwai tiwai@suse.de writes:

...

Thanks for the patch. But this looks suboptimal, unfortunately, since it keeps the amp always on, and more badly, it would block the power save of the widget root node.

Thanks very much for your feedback; I wasn't sure precisely how this code worked and tried to make a change that was as close as I could manage to existing examples.

...

Can just using gpio_mute_led_mask=0x18 and gpio_led=0 (also drop AC_VERB_SET_GPIO_DATA in gpio_init[]) work instead? If GPIO4 is the the amp, we can associate it with the master mute control together with the mute LED. The only concern would be the possible click noise, but it doesn't happen on most machines.

It's not quite that simple; the GPIO4 value is inverted from the mute LED value (the amp is powered up when GPIO4 is set).

Ah, right, there was already a fixup for GPIO4 low, and yours is for GPIO4 high.

What I've done is to make the amp powered only when a headphone is plugged in, and then removed the code which was disabling power saving, which lets everything (including the amp) get turned back off when the device goes idle.

Here's a second version of the patch.

Thanks! The new patch looks good, but I think we don't have to use the headset code, as your case looks more like a headphone, not a combo headset that needs the special handling. If so, the change can be reduced something like below. Could you check whether this is enough?

Takashi

--- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -3430,6 +3430,37 @@ static void alc280_fixup_hp_gpio4(struct hda_codec *codec, } }

+static void alc280_hp_gpio4_hp_automue(struct hda_codec *codec, + struct hda_jack_callback *jack) +{ + snd_hda_gen_hp_automute(codec, jack); + /* mute_led_polarity is set to 0, so we pass inverted value here */ + alc_update_gpio_led(codec, 0x10, !spec->gen.hp_jack_present); +} + +/* GPIO3 = mute LED, GPIO4 high = enable headphone amp */ +static void alc280_fixup_hp_gpio4_hp_amp(struct hda_codec *codec, + const struct hda_fixup *fix, + int action) +{ + struct alc_spec *spec = codec->spec; + static const struct hda_verb gpio_init[] = { + { 0x01, AC_VERB_SET_GPIO_MASK, 0x18 }, + { 0x01, AC_VERB_SET_GPIO_DIRECTION, 0x18 }, + {} + }; + + if (action == HDA_FIXUP_ACT_PRE_PROBE) { + spec->gen.vmaster_mute.hook = alc_fixup_gpio_mute_hook; + spec->gen.hp_automute_hook = alc280_hp_gpio4_hp_automue; + spec->gpio_led = 0; + spec->mute_led_polarity = 0; + spec->gpio_mute_led_mask = 0x08; + snd_hda_add_verbs(codec, gpio_init); + codec->power_filter = led_power_filter; + } +} +