Re: Set eapd to follow stream on or off

[Kailang <kailang@realtek.com>]

[-- Attachment #1: Type: text/plain, Size: 4151 bytes --]

Hi Takashi,

Attach is the update patch.
Many thanks.

BR,
Kailang


> At Wed, 21 May 2014 13:58:14 +0800,
> Kailang wrote:
>>
>>
>> > At Tue, 20 May 2014 18:05:42 +0800,
>> > Kailang wrote:
>> >>
>> >> > At Tue, 20 May 2014 17:20:44 +0800,
>> >> > Kailang wrote:
>> >> >>
>> >> >>
>> >> >> > At Tue, 20 May 2014 16:52:43 +0800,
>> >> >> > Kailang wrote:
>> >> >> >>
>> >> >> >>
>> >> >> >> > At Tue, 20 May 2014 15:15:49 +0800,
>> >> >> >> > Kailang wrote:
>> >> >> >> >>
>> >> >> >> >> Hi Takashi,
>> >> >> >> >>
>> >> >> >> >> HP machine had pop noise via speaker.
>> >> >> >> >> If EAPD on or off, speaker will have pop noise.
>> >> >> >> >> So, they want to set eapd to follow with stream on or off.
>> >> >> >> >
>> >> >> >> > I can apply this as is, but still I wonder whether this 
>> >> >> >> > results
>> >> >> >> > in
>> >> >> >> > other problems.  Doing EAPD on/off there means that it happens
>> >> >> >> > far
>> >> >> >> > more often.  For example, what happens if you do like below?
>> >> >> >> >
>> >> >> >> >     while true; do
>> >> >> >> >       echo -n > /dev/snd/pcmC0D0p
>> >> >> >> >     done
>> >> >> >> >
>> >> >> >> > And, what if the device is resumed from power-saving by an 
>> >> >> >> > audio
>> >> >> >> > playback?  Immediately after the power up, it'll start the
>> >> >> >> > streaming,
>> >> >> >> > so it'll turn on EAPD.
>> >> >> >> >
>> >> >> >> > That said, isn't it just a need for some delay before EAPD
>> >> >> >> > on/off?
>> >> >> >>
>> >> >> >> No, eapd on or off will cause pop noise.
>> >> >> >
>> >> >> > Does it do always?
>> >> >> No, pop noise just one pop sound. It can not remove by software.
>> >> >> But this machine need to do this always.
>> >> >
>> >> > So, doing EAPD on/off often is rather worse.  It's what your patch
>> >> > does.
>> >> >
>> >> > Once again: did you try the script above?  If the pop noise always
>> >>
>> >> I had try it on my develop machine. I have no HP issue machine on my
>> >> site.
>> >>
>> >> > happens, the operation like above is the worst scenario.
>> >> >
>> >> > Then, let's think again how to *reduce* it.  We need one EAPD on at
>> >> > least after each power up.  But it's needed only when you actually
>> >> > plays a stream.  That is, we need only enable EAPD at playback 
>> >> > stream
>> >> > open, but leave EAPD as is at close.
>> >> >
>> >>
>> >> [we need only enable EAPD at playback stream open, but leave EAPD as 
>> >> is
>> >> at
>> >> close.]
>> >> Yes.
>> >> So, I need to create eapd low in alc269_shutup. Right?
>> >
>> > Better to create own shutup callback (you can call alc269_shutup()
>> > from there, too) and set it to spec->shutup in the fixup.  The pincfg
>> > cleanup isn't often needed or may have bad effect.  You should test
>> > with real machines what are needed and what are not.
>>
>> static void alc290_fixup_pop_noise(struct hda_codec *codec,
>>            const struct hda_fixup *fix, int action)
>> {
>>  struct alc_spec *spec = codec->spec;
>>
>>  if (action == HDA_FIXUP_ACT_PRE_PROBE) {
>>   spec->gen.pcm_playback_hook = ALC290_playback_pcm_hook;
>>  }
>> if (action ==   HDA_FIXUP_ACT_INIT) {
>>   set_eapd(codec, 0x14, false);
>>  }
>> }
>>
>> Could I modify as upper code?
>> To set eapd low in HDA_FIXUP_ACT_INIT state.
>> If system resume, it will initial eapd to low.
>> To write eapd to same value, it will have no pop noise.
>> It will keep always low for eapd, only stream open and close have change
>> state for eapd.
>
> Yes, it looks OK.
>
>
> Takashi
>
>
>>
>> >
>> >
>> > Takashi
>> >
>> >>
>> >> >
>> >> > Takashi
>> >> >
>> >> >> >> Our customer just want to do eapd on when stream is open.
>> >> >> >> The pop noise will not clear. Because music gain was biger than 
>> >> >> >> pop
>> >> >> >> noise.
>> >> >> >
>> >> >> > Opening a stream doesn't mean that it follows the loud sound
>> >> >> > immediately.  The script above wouldn't produce any sound but 
>> >> >> > just
>> >> >> > does open/close the stream.  Did you try that?
>> >> >> >
>> >> >> >
>> >> >> > Takashi
>> >> >> >
>> >> >> > ------Please consider the environment before printing this 
>> >> >> > e-mail.
>> >> >>
>> >>
>> 

[-- Attachment #2: 0001-set-eapd-follow-stream-on-off.patch --]
[-- Type: application/octet-stream, Size: 3400 bytes --]

>From 71a093ba3198666c4500b30938e080bd887647cb Mon Sep 17 00:00:00 2001
From: Kailang Yang <kailang@realtek.com>
Date: Wed, 21 May 2014 14:25:08 +0800
Subject: [PATCH] ALSA: hda/realtek - Set EAPD to follow stream on or off

HP machine has pop noise via speaker.
Set EAPD on when system open stream.
Set EAPD off when system close stream.

Signed-off-by: Kailang Yang <kailang@realtek.com>

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 35acbd0..87973e2 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4175,6 +4175,39 @@ static void alc290_fixup_mono_speakers(struct hda_codec *codec,
 	}
 }
 
+/*
+ * ALC290 PCM hooks
+ */
+static void ALC290_playback_pcm_hook(struct hda_pcm_stream *hinfo,
+				   struct hda_codec *codec,
+				   struct snd_pcm_substream *substream,
+				   int action)
+{
+	switch (action) {
+	case HDA_GEN_PCM_ACT_OPEN:
+		set_eapd(codec, 0x14, true);
+		break;
+	case HDA_GEN_PCM_ACT_CLOSE:
+		set_eapd(codec, 0x14, false);
+		break;
+	}
+}
+
+static void alc290_fixup_pop_noise(struct hda_codec *codec,
+				       const struct hda_fixup *fix, int action)
+{
+	struct alc_spec *spec = codec->spec;
+
+	switch (action) {
+	case HDA_FIXUP_ACT_PRE_PROBE:
+		spec->gen.pcm_playback_hook = ALC290_playback_pcm_hook;
+		break;
+	case HDA_FIXUP_ACT_INIT:
+		set_eapd(codec, 0x14, false);
+		break;
+	}
+}
+
 /* for hda_fixup_thinkpad_acpi() */
 #include "thinkpad_helper.c"
 
@@ -4237,6 +4270,7 @@ enum {
 	ALC255_FIXUP_HEADSET_MODE_NO_HP_MIC,
 	ALC293_FIXUP_DELL1_MIC_NO_PRESENCE,
 	ALC292_FIXUP_TPT440_DOCK,
+	ALC290_FIXUP_HP_DEPOP_WITH_MUTE_LED,
 };
 
 static const struct hda_fixup alc269_fixups[] = {
@@ -4650,6 +4684,12 @@ static const struct hda_fixup alc269_fixups[] = {
 		.chained = true,
 		.chain_id = ALC269_FIXUP_LIMIT_INT_MIC_BOOST
 	},
+	[ALC290_FIXUP_HP_DEPOP_WITH_MUTE_LED] = {
+		.type = HDA_FIXUP_FUNC,
+		.v.func = alc290_fixup_pop_noise,
+		.chained = true,
+		.chain_id = ALC269_FIXUP_HP_MUTE_LED_MIC1,
+	},
 };
 
 static const struct snd_pci_quirk alc269_fixup_tbl[] = {
@@ -4772,12 +4812,14 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
 	SND_PCI_QUIRK(0x103c, 0x2280, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
 	SND_PCI_QUIRK(0x103c, 0x2281, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
 	SND_PCI_QUIRK(0x103c, 0x2282, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
-	SND_PCI_QUIRK(0x103c, 0x2289, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
+	SND_PCI_QUIRK(0x103c, 0x2289, "HP", ALC290_FIXUP_HP_DEPOP_WITH_MUTE_LED),
 	SND_PCI_QUIRK(0x103c, 0x228a, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
 	SND_PCI_QUIRK(0x103c, 0x228b, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
-	SND_PCI_QUIRK(0x103c, 0x228c, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
+	SND_PCI_QUIRK(0x103c, 0x228c, "HP", ALC290_FIXUP_HP_DEPOP_WITH_MUTE_LED),
 	SND_PCI_QUIRK(0x103c, 0x228d, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
 	SND_PCI_QUIRK(0x103c, 0x228e, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
+	SND_PCI_QUIRK(0x103c, 0x228f, "HP", ALC290_FIXUP_HP_DEPOP_WITH_MUTE_LED),
+	SND_PCI_QUIRK(0x103c, 0x22a8, "HP", ALC290_FIXUP_HP_DEPOP_WITH_MUTE_LED),
 	SND_PCI_QUIRK(0x103c, 0x22c5, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
 	SND_PCI_QUIRK(0x103c, 0x22c6, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),
 	SND_PCI_QUIRK(0x103c, 0x22c7, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC1),

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]