[alsa-devel] [PATCH] ALSA: hda - Remove speaker clicks on CX20549
Takashi Iwai
tiwai at suse.de
Mon Feb 18 10:45:03 CET 2013
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 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 */
>
> 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)
> > return;
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 */
> > 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);
> >
> >
>
>
>
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
>
More information about the Alsa-devel
mailing list