[alsa-devel] [PATCH] ALSA: hda - Remove speaker clicks on CX20549

[Takashi Iwai]

At Mon, 18 Feb 2013 10:22:19 +0100,
David Henningsson wrote:
> 
> On 02/14/2013 12:00 PM, Takashi Iwai wrote:
> > At Thu, 14 Feb 2013 11:36:39 +0100,
> > David Henningsson wrote:
> >>
> >> This chip needs the speaker pin to go to D3 to avoid clicks,
> >> so default_power_filter does not work here.
> >>
> >> This was found on Thinkpad R61i/T61i but I guess it applies to
> >> the entire chip. If not, quirks should be set for at least
> >> PCI SSID 17aa:20ac.
> >>
> >> Thanks to c4pp4 for testing.
> >>
> >> BugLink: https://bugs.launchpad.net/bugs/886975
> >> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> >
> > Thanks, applied now.
> >
> > Just wonder, though, whether rather setting
> > spec->gen.power_down_unused = 1 works.  When it's set, the generic
> > parser applies the own power filter, and it doesn't have the EAPD
> > check either (plus it does more aggressive power-down of unused
> > widgets).
> >
> > Or, maybe we just drop the EAPD check, and move it to specific fixup.
> > AFAIK, it was required only for old Gateway laptops.
> 
> If gen.power_down_unused is what is believed to get the best power 
> savings (by powering down more sub-areas of the chip), maybe a 
> reasonable strategy to be to carefully move towards making 
> gen.power_down_unused the default, first for the stuff that we have hw 
> for and can test, or the new chips that we add, and if that works out 
> fine we could try activate it for older chips too.

Yes, power_down_unused is pretty aggressive, and should be enabled
selectively.  (And this can be enabled even now with a hint string.)

OTOH, EAPD check we had was really a workaround for some old machines,
AFAIK, only for Gateway machines.  This can be disabled as default
like the patch below.  But it's the thing for post-3.9, just to be
safer.


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 04b5738..c2fe09d 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1296,8 +1296,6 @@ static bool snd_hda_codec_get_supported_ps(struct hda_codec *codec,
 
 static unsigned int hda_set_power_state(struct hda_codec *codec,
 				unsigned int power_state);
-static unsigned int default_power_filter(struct hda_codec *codec, hda_nid_t nid,
-					 unsigned int power_state);
 
 /**
  * snd_hda_codec_new - create a HDA codec
@@ -1418,7 +1416,6 @@ int snd_hda_codec_new(struct hda_bus *bus,
 #endif
 	codec->epss = snd_hda_codec_get_supported_ps(codec, fg,
 					AC_PWRST_EPSS);
-	codec->power_filter = default_power_filter;
 
 	/* power-up all before initialization */
 	hda_set_power_state(codec, AC_PWRST_D0);
@@ -3759,8 +3756,9 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
 }
 
 /* don't power down the widget if it controls eapd and EAPD_BTLENABLE is set */
-static unsigned int default_power_filter(struct hda_codec *codec, hda_nid_t nid,
-					 unsigned int power_state)
+unsigned int snd_hda_codec_eapd_power_filter(struct hda_codec *codec,
+					     hda_nid_t nid,
+					     unsigned int power_state)
 {
 	if (power_state == AC_PWRST_D3 &&
 	    get_wcaps_type(get_wcaps(codec, nid)) == AC_WID_PIN &&
@@ -3772,6 +3770,7 @@ static unsigned int default_power_filter(struct hda_codec *codec, hda_nid_t nid,
 	}
 	return power_state;
 }
+EXPORT_SYMBOL_HDA(snd_hda_codec_eapd_power_filter);
 
 /*
  * set power state of the codec, and return the power state
@@ -3817,7 +3816,8 @@ static void sync_power_up_states(struct hda_codec *codec)
 	int i;
 
 	/* don't care if no or standard filter is used */
-	if (!codec->power_filter || codec->power_filter == default_power_filter)
+	if (!codec->power_filter ||
+	    codec->power_filter == snd_hda_codec_eapd_power_filter)
 		return;
 
 	for (i = 0; i < codec->num_nodes; i++, nid++) {
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 05f1d59..ef798bd 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -670,6 +670,10 @@ snd_hda_check_power_state(struct hda_codec *codec, hda_nid_t nid,
 	return (state != target_state);
 }
 
+unsigned int snd_hda_codec_eapd_power_filter(struct hda_codec *codec,
+					     hda_nid_t nid,
+					     unsigned int power_state);
+
 /*
  * AMP control callbacks
  */
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index 941bf6c..7d941ef 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -3350,7 +3350,6 @@ static int patch_conexant_auto(struct hda_codec *codec)
 	switch (codec->vendor_id) {
 	case 0x14f15045:
 		codec->single_adc_amp = 1;
-		codec->power_filter = NULL; /* Needs speaker amp to D3 to avoid click */
 		break;
 	case 0x14f15047:
 		codec->pin_amp_workaround = 1;
diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c
index 83d5335..d57c81e 100644
--- a/sound/pci/hda/patch_sigmatel.c
+++ b/sound/pci/hda/patch_sigmatel.c
@@ -3774,6 +3774,7 @@ static int patch_stac9200(struct hda_codec *codec)
 	spec->gen.own_eapd_ctl = 1;
 
 	codec->patch_ops = stac_patch_ops;
+	codec->power_filter = snd_hda_codec_eapd_power_filter;
 
 	snd_hda_add_verbs(codec, stac9200_eapd_init);