[alsa-devel] [PATCH] ALSA: hda - Remove speaker clicks on CX20549
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@canonical.com --- sound/pci/hda/patch_conexant.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c index 7d941ef..941bf6c 100644 --- a/sound/pci/hda/patch_conexant.c +++ b/sound/pci/hda/patch_conexant.c @@ -3350,6 +3350,7 @@ 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;
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@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.
Takashi
sound/pci/hda/patch_conexant.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c index 7d941ef..941bf6c 100644 --- a/sound/pci/hda/patch_conexant.c +++ b/sound/pci/hda/patch_conexant.c @@ -3350,6 +3350,7 @@ 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 */-- 1.7.9.5
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@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.
What do you think?
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@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);
On 02/18/2013 10:28 AM, Takashi Iwai wrote:
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@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)@@ -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 */
I don't mind moving it into patch_stac9200 if you think it's the right thing to do, but if we do, perhaps the default_power_filter / snd_hda_codec_eapd_power_filter function should move there too, and become local to patch_sigmatel.c?
If so the below check has to be changed into yet another flag.
- 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 */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);
At Mon, 18 Feb 2013 10:40:55 +0100, David Henningsson wrote:
On 02/18/2013 10:28 AM, Takashi Iwai wrote:
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@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)@@ -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 */
I don't mind moving it into patch_stac9200 if you think it's the right thing to do, but if we do, perhaps the default_power_filter / snd_hda_codec_eapd_power_filter function should move there too, and become local to patch_sigmatel.c?
I didn't move it yet, in case where any other codecs / models need the same stuff. Once after we confirm it's really only for Gateway, we can move the stuff to local.
If so the below check has to be changed into yet another flag.
- if (!codec->power_filter || codec->power_filter == default_power_filter)
- if (!codec->power_filter ||
codec->power_filter == snd_hda_codec_eapd_power_filter)
Yeah, but also we can omit the check of snd_hda_codec_eapd_power_filter, too. It's only for optimization.
Takashi
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 */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);
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
participants (2)
-
David Henningsson -
Takashi Iwai