[alsa-devel] Realtek model parser problem with the new jack detection
David Henningsson
david.henningsson at canonical.com
Thu Jan 19 12:39:28 CET 2012
On 01/19/2012 12:17 PM, Takashi Iwai wrote:
> At Thu, 19 Jan 2012 11:35:23 +0100,
> Takashi Iwai wrote:
>>
>> At Wed, 18 Jan 2012 17:28:32 +0100,
>> David Henningsson wrote:
>>>
>>> Hi Takashi,
>>>
>>> I'm troubleshooting a problem with the Realtek quirk/model parsers. The
>>> problem is basically that add_jack_kctls call
>>> snd_hda_jack_detect_enable, which in turn overwrites the current unsol tag.
>>>
>>> E g, first the model parser sets a pin to AC_USRSP_EN | ALC_HP_EVENT,
>>> then comes snd_hda_jack_detect_enable and does AC_USRSP_EN | jack->tag.
>>>
>>> I'm not sure of the best way to resolve this; either keep the current
>>> tag (and mark all jacks as dirty on an unsol event), or read back the
>>> current tag and set it as jack->action (and then always use the jack
>>> table). What do you think?
>>
>> I think a safer way is to avoid calling snd_hda_jack_add_kctls()
>> for non-auto-parser, and let quirks use its own unsol_event handler
>> instead of alc_sku_unsol_event(). snd_hda_jack_detect() itself works
>> both with or without jack-kctls.
>>
>> I'll fix up the upstream tree.
>
> The patch below.
Thanks for quick handling. This will mean no jack kctls for model
parsers, which is a bit sad, but hopefully its an incentive for people
to move to auto-parsers.
One review comment below:
>
>
> Takashi
>
> ---
> From: Takashi Iwai<tiwai at suse.de>
> Subject: [PATCH] ALSA: hda/realtek - Avoid conflict of unsol-events with
> static quirks
>
> The recently added jack-kctl support sets the unsol event tags
> dynamically, while static quirks usually set the fixed tags in the
> init_verbs array. Due to this conflict, the own unsol event handler
> can't retrieve the tag and handle it properly any more.
>
> For fixing this, avoid calling snd_hda_jack_add_kctls() for static
> quirks, and always let them use own handlers instead of the standard
> one for the auto-pareser.
>
> Reported-by: David Henningsson<david.henningsson at canonical.com>
> Signed-off-by: Takashi Iwai<tiwai at suse.de>
> ---
> sound/pci/hda/alc880_quirks.c | 17 +++++++++++----
> sound/pci/hda/alc882_quirks.c | 15 +++++++++----
> sound/pci/hda/patch_realtek.c | 45 ++++++++++++++++++++++++++++------------
> 3 files changed, 53 insertions(+), 24 deletions(-)
>
> diff --git a/sound/pci/hda/alc880_quirks.c b/sound/pci/hda/alc880_quirks.c
> index 5b68435..501501e 100644
> --- a/sound/pci/hda/alc880_quirks.c
> +++ b/sound/pci/hda/alc880_quirks.c
> @@ -762,16 +762,22 @@ static void alc880_uniwill_unsol_event(struct hda_codec *codec,
> /* Looks like the unsol event is incompatible with the standard
> * definition. 4bit tag is placed at 28 bit!
> */
> - switch (res>> 28) {
> + res>>= 28;
> + switch (res) {
> case ALC_MIC_EVENT:
> alc88x_simple_mic_automute(codec);
> break;
> default:
> - alc_sku_unsol_event(codec, res);
> + alc_exec_unsol_event(codec, res);
> break;
> }
> }
>
> +static void alc880_unsol_event(struct hda_codec *codec, unsigned int res)
> +{
> + alc_exec_unsol_event(codec, res>> 28);
> +}
> +
> static void alc880_uniwill_p53_setup(struct hda_codec *codec)
> {
> struct alc_spec *spec = codec->spec;
> @@ -800,10 +806,11 @@ static void alc880_uniwill_p53_unsol_event(struct hda_codec *codec,
> /* Looks like the unsol event is incompatible with the standard
> * definition. 4bit tag is placed at 28 bit!
> */
> - if ((res>> 28) == ALC_DCVOL_EVENT)
> + res>>= 28;
> + if (res == ALC_DCVOL_EVENT)
> alc880_uniwill_p53_dcvol_automute(codec);
> else
> - alc_sku_unsol_event(codec, res);
> + alc_exec_unsol_event(codec, res);
> }
>
> /*
> @@ -1677,7 +1684,7 @@ static const struct alc_config_preset alc880_presets[] = {
> .channel_mode = alc880_lg_ch_modes,
> .need_dac_fix = 1,
> .input_mux =&alc880_lg_capture_source,
> - .unsol_event = alc_sku_unsol_event,
> + .unsol_event = alc880_unsol_event,
> .setup = alc880_lg_setup,
> .init_hook = alc_hp_automute,
> #ifdef CONFIG_SND_HDA_POWER_SAVE
> diff --git a/sound/pci/hda/alc882_quirks.c b/sound/pci/hda/alc882_quirks.c
> index bdf0ed4..bb364a5 100644
> --- a/sound/pci/hda/alc882_quirks.c
> +++ b/sound/pci/hda/alc882_quirks.c
> @@ -730,6 +730,11 @@ static void alc889A_mb31_unsol_event(struct hda_codec *codec, unsigned int res)
> alc889A_mb31_automute(codec);
> }
>
> +static void alc882_unsol_event(struct hda_codec *codec, unsigned int res)
> +{
> + alc_exec_unsol_event(codec, res>> 26);
> +}
> +
> /*
> * configuration and preset
> */
> @@ -775,7 +780,7 @@ static const struct alc_config_preset alc882_presets[] = {
> .channel_mode = alc885_mba21_ch_modes,
> .num_channel_mode = ARRAY_SIZE(alc885_mba21_ch_modes),
> .input_mux =&alc882_capture_source,
> - .unsol_event = alc_sku_unsol_event,
> + .unsol_event = alc882_unsol_event,
> .setup = alc885_mba21_setup,
> .init_hook = alc_hp_automute,
> },
> @@ -791,7 +796,7 @@ static const struct alc_config_preset alc882_presets[] = {
> .input_mux =&alc882_capture_source,
> .dig_out_nid = ALC882_DIGOUT_NID,
> .dig_in_nid = ALC882_DIGIN_NID,
> - .unsol_event = alc_sku_unsol_event,
> + .unsol_event = alc882_unsol_event,
> .setup = alc885_mbp3_setup,
> .init_hook = alc_hp_automute,
> },
> @@ -806,7 +811,7 @@ static const struct alc_config_preset alc882_presets[] = {
> .input_mux =&mb5_capture_source,
> .dig_out_nid = ALC882_DIGOUT_NID,
> .dig_in_nid = ALC882_DIGIN_NID,
> - .unsol_event = alc_sku_unsol_event,
> + .unsol_event = alc882_unsol_event,
> .setup = alc885_mb5_setup,
> .init_hook = alc_hp_automute,
> },
> @@ -821,7 +826,7 @@ static const struct alc_config_preset alc882_presets[] = {
> .input_mux =&macmini3_capture_source,
> .dig_out_nid = ALC882_DIGOUT_NID,
> .dig_in_nid = ALC882_DIGIN_NID,
> - .unsol_event = alc_sku_unsol_event,
> + .unsol_event = alc882_unsol_event,
> .setup = alc885_macmini3_setup,
> .init_hook = alc_hp_automute,
> },
> @@ -836,7 +841,7 @@ static const struct alc_config_preset alc882_presets[] = {
> .input_mux =&alc889A_imac91_capture_source,
> .dig_out_nid = ALC882_DIGOUT_NID,
> .dig_in_nid = ALC882_DIGIN_NID,
> - .unsol_event = alc_sku_unsol_event,
> + .unsol_event = alc882_unsol_event,
> .setup = alc885_imac91_setup,
> .init_hook = alc_hp_automute,
> },
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 61ccbe8..2326bf3 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -621,17 +621,10 @@ static void alc_mic_automute(struct hda_codec *codec)
> alc_mux_select(codec, 0, spec->int_mic_idx, false);
> }
>
> -/* unsolicited event for HP jack sensing */
> -static void alc_sku_unsol_event(struct hda_codec *codec, unsigned int res)
> +/* handle the specified unsol action (ALC_XXX_EVENT) */
> +static void alc_exec_unsol_event(struct hda_codec *codec, int action)
> {
> - struct alc_spec *spec = codec->spec;
> - if (codec->vendor_id == 0x10ec0880)
> - res>>= 28;
> - else
> - res>>= 26;
> - if (spec->use_jack_tbl)
> - res = snd_hda_jack_get_action(codec, res);
> - switch (res) {
> + switch (action) {
> case ALC_HP_EVENT:
> alc_hp_automute(codec);
> break;
> @@ -645,6 +638,19 @@ static void alc_sku_unsol_event(struct hda_codec *codec, unsigned int res)
> snd_hda_jack_report_sync(codec);
> }
>
> +/* unsolicited event for HP jack sensing */
> +static void alc_sku_unsol_event(struct hda_codec *codec, unsigned int res)
> +{
> + struct alc_spec *spec = codec->spec;
> + if (codec->vendor_id == 0x10ec0880)
> + res>>= 28;
> + else
> + res>>= 26;
> + if (spec->use_jack_tbl)
> + res = snd_hda_jack_get_action(codec, res);
Can the check for spec->use_jack_tbl can be removed here?
> + alc_exec_unsol_event(codec, res);
> +}
> +
> /* call init functions of standard auto-mute helpers */
> static void alc_inithook(struct hda_codec *codec)
> {
> @@ -1883,7 +1889,7 @@ static const struct snd_kcontrol_new alc_beep_mixer[] = {
> };
> #endif
>
> -static int alc_build_controls(struct hda_codec *codec)
> +static int __alc_build_controls(struct hda_codec *codec)
> {
> struct alc_spec *spec = codec->spec;
> struct snd_kcontrol *kctl = NULL;
> @@ -2029,11 +2035,16 @@ static int alc_build_controls(struct hda_codec *codec)
>
> alc_free_kctls(codec); /* no longer needed */
>
> - err = snd_hda_jack_add_kctls(codec,&spec->autocfg);
> + return 0;
> +}
> +
> +static int alc_build_controls(struct hda_codec *codec)
> +{
> + struct alc_spec *spec = codec->spec;
> + int err = __alc_build_controls(codec);
> if (err< 0)
> return err;
> -
> - return 0;
> + return snd_hda_jack_add_kctls(codec,&spec->autocfg);
> }
I think you can simplify this by
if (spec->use_jack_tbl)
err = snd_hda_jack_add_kctls(codec,&spec->autocfg);
If so, only one alc_build_controls is necessary.
>
>
> @@ -4168,6 +4179,8 @@ static int patch_alc880(struct hda_codec *codec)
> codec->patch_ops = alc_patch_ops;
> if (board_config == ALC_MODEL_AUTO)
> spec->init_hook = alc_auto_init_std;
> + else
> + codec->patch_ops.build_controls = __alc_build_controls;
> #ifdef CONFIG_SND_HDA_POWER_SAVE
> if (!spec->loopback.amplist)
> spec->loopback.amplist = alc880_loopbacks;
> @@ -4297,6 +4310,8 @@ static int patch_alc260(struct hda_codec *codec)
> codec->patch_ops = alc_patch_ops;
> if (board_config == ALC_MODEL_AUTO)
> spec->init_hook = alc_auto_init_std;
> + else
> + codec->patch_ops.build_controls = __alc_build_controls;
> spec->shutup = alc_eapd_shutup;
> #ifdef CONFIG_SND_HDA_POWER_SAVE
> if (!spec->loopback.amplist)
> @@ -4691,6 +4706,8 @@ static int patch_alc882(struct hda_codec *codec)
> codec->patch_ops = alc_patch_ops;
> if (board_config == ALC_MODEL_AUTO)
> spec->init_hook = alc_auto_init_std;
> + else
> + codec->patch_ops.build_controls = __alc_build_controls;
>
> #ifdef CONFIG_SND_HDA_POWER_SAVE
> if (!spec->loopback.amplist)
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
More information about the Alsa-devel
mailing list