[alsa-devel] [RFC PATCH] Fixup automute for Realtek auto parser
David Henningsson
david.henningsson at canonical.com
Mon Sep 19 11:28:08 CEST 2011
On 09/16/2011 04:30 PM, Takashi Iwai wrote:
> At Fri, 16 Sep 2011 16:22:17 +0200,
> David Henningsson wrote:
>>
>> On 09/16/2011 02:44 PM, Takashi Iwai wrote:
>>> At Fri, 16 Sep 2011 13:22:25 +0200,
>>> David Henningsson wrote:
>>>>
>>>> On 09/16/2011 01:07 PM, Takashi Iwai wrote:
>>>>> At Fri, 16 Sep 2011 12:45:06 +0200,
>>>>> David Henningsson wrote:
>>>>>>
>>>>>> On 09/16/2011 12:03 PM, Takashi Iwai wrote:
>>>>>>> At Fri, 16 Sep 2011 11:35:31 +0200,
>>>>>>> Takashi Iwai wrote:
>>>>>>>>
>>>>>>>> At Fri, 16 Sep 2011 11:22:01 +0200,
>>>>>>>> David Henningsson wrote:
>>>>>>>>>
>>>>>>>>> [1<text/plain; ISO-8859-1 (7bit)>]
>>>>>>>>> On 09/16/2011 10:31 AM, Takashi Iwai wrote:
>>>>>>>>>> At Fri, 16 Sep 2011 10:01:48 +0200,
>>>>>>>>>> David Henningsson wrote:
>>>>>>>>>>>
>>>>>>>>>>> The ~6 months old functionality for configurable automuting is broken
>>>>>>>>>>> for the case that the user has only HP and LO (no speakers) - basically
>>>>>>>>>>> there is an "Automute mode" control, but enabling it does nothing. While
>>>>>>>>>>> fixing that, I also took the liberty of refactoring/rewriting some of
>>>>>>>>>>> that code, in order to make it easier to understand and maintain.
>>>>>>>>>>>
>>>>>>>>>>> Takashi, given you approve/like these changes, I'll go ahead and fix up
>>>>>>>>>>> the quirk files to fit the new variable names.
>>>>>>>>>>
>>>>>>>>>> I'm fine with renames but let's split patches: first fix the bug
>>>>>>>>>> of non-working auto-mute control, then refactoring. The former
>>>>>>>>>> should go to 3.1 while the latter is queued to 3.2.
>>>>>>>>>
>>>>>>>>> Ok. If it also should go to 3.0, feel free to add cc: stable at kernel.org
>>>>>>>>> (hmm, is that working even though kernel.org is down?)
>>>>>>>>>
>>>>>>>>> Here comes the 3.1 part of the patch. If you're happy with it, commit it
>>>>>>>>> and I'll continue with the 3.2 renames.
>>>>>>>>
>>>>>>>> Hm, it's more complicated than I wanted.
>>>>>>>>
>>>>>>>> Could you have an example codec proc or alsa-info.sh file to simulate
>>>>>>>> here? The logic is already there, so a few lines of corrections
>>>>>>>> should suffice to fix the behavior.
>>>>>>>
>>>>>>> I guess the oneliner below should fix the bug.
>>>>>>> Could you check it?
>>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Takashi
>>>>>>>
>>>>>>> ---
>>>>>>> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>>>>>>> index 70ba45e..5c4625e 100644
>>>>>>> --- a/sound/pci/hda/patch_realtek.c
>>>>>>> +++ b/sound/pci/hda/patch_realtek.c
>>>>>>> @@ -556,7 +556,7 @@ static void update_speakers(struct hda_codec *codec)
>>>>>>> if (spec->autocfg.line_out_pins[0] == spec->autocfg.hp_pins[0] ||
>>>>>>> spec->autocfg.line_out_pins[0] == spec->autocfg.speaker_pins[0])
>>>>>>> return;
>>>>>>> - if (!spec->automute_lines || !spec->automute)
>>>>>>> + if ((spec->automute_hp_lo&& !spec->automute_lines) || !spec->automute)
>>>>>>> on = 0;
>>>>>>> else
>>>>>>> on = spec->jack_present;
>>>>>>>
>>>>>>
>>>>>> It does in this particular case, but it's very confusing that
>>>>>> spec->automute_lines can be 0 while the lines are still automuted. To
>>>>>> avoid such confusion, a more complicated patch is needed.
>>>>>
>>>>> Yes, needed for 3.2, but not for 3.0/3.1 kernels.
>>>>>
>>>>>> Honestly, would you have accepted such a patch if I were the one
>>>>>> proposing it? :-)
>>>>>
>>>>> Yes, certainly I would. And I asked for it.
>>>>>
>>>>> Of course, a later rewrite would be needed, but as a fix at this late
>>>>> stage, smaller is more beautiful.
>>
>> Even though I might not agree, I understand that the smallness might
>> weigh heavier than the clarity at this point of the cycle.
>>
>>>> We'll have to disagree on that, then. For me, this is confusing, so I
>>>> have a hard time ensuring that the one-liner isn't causing problems in
>>>> some other use case. But you wrote the code, so you probably have a
>>>> better overview than I have.
>>>
>>> Yeah, the current code is tricky. It's because automute_lines flag
>>> depends on automute_hp_lo flag. When hp_lo isn't set, automute_lines
>>> doesn't mean anything.
>>
>> Ok. I didn't understand that was the intention until now.
>>
>>> So, a more comprehensive change would be like below. When
>>> automute_lines is checked, it must be always coupled with
>>> automute_hp_lo check.
>>>
>>>
>>> Takashi
>>>
>>> ---
>>> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>>> index 70ba45e..1883dcc 100644
>>> --- a/sound/pci/hda/patch_realtek.c
>>> +++ b/sound/pci/hda/patch_realtek.c
>>> @@ -169,7 +169,7 @@ struct alc_spec {
>>> unsigned int auto_mic_valid_imux:1; /* valid imux for auto-mic */
>>> unsigned int automute:1; /* HP automute enabled */
>>> unsigned int detect_line:1; /* Line-out detection enabled */
>>> - unsigned int automute_lines:1; /* automute line-out as well */
>>> + unsigned int automute_lines:1; /* automute line-out as well; NOP when automute_hp_lo isn't set */
>>> unsigned int automute_hp_lo:1; /* both HP and LO available */
>>>
>>> /* other flags */
>>> @@ -556,7 +556,7 @@ static void update_speakers(struct hda_codec *codec)
>>> if (spec->autocfg.line_out_pins[0] == spec->autocfg.hp_pins[0] ||
>>> spec->autocfg.line_out_pins[0] == spec->autocfg.speaker_pins[0])
>>> return;
>>> - if (!spec->automute_lines || !spec->automute)
>>> + if (!spec->automute || (spec->automute_hp_lo&& !spec->automute_lines))
>>> on = 0;
>>> else
>>> on = spec->jack_present;
>>> @@ -817,7 +817,7 @@ static int alc_automute_mode_get(struct snd_kcontrol *kcontrol,
>>> unsigned int val;
>>> if (!spec->automute)
>>> val = 0;
>>> - else if (!spec->automute_lines)
>>> + else if (!spec->automute_hp_lo || !spec->automute_lines)
>>> val = 1;
>>> else
>>> val = 2;
>>> @@ -838,7 +838,8 @@ static int alc_automute_mode_put(struct snd_kcontrol *kcontrol,
>>> spec->automute = 0;
>>> break;
>>> case 1:
>>> - if (spec->automute&& !spec->automute_lines)
>>> + if (spec->automute&&
>>> + !(spec->automute_hp_lo&& !spec->automute_lines))
>>
>> It seems the above will negate spec->automute_lines twice, that can't be
>> right, can it? Shouldn't this be:
>>
>> if (spec->automute&& (!spec->automute_hp_lo || !spec->automute_lines))
>> return 0;
>>
>> ...similar to the _get function?
>
> Ah right. It's the reason it needs a rewrite :)
>
> FWIW, the below is the fixed patch.
The below patch has now been tested by Jayne Han, and she confirmed it
was working. I also believe it could be queued up against 3.0, so
consider adding these lines to the commit if possible:
Cc: stable at kernel.org (3.0)
BugLink: http://bugs.launchpad.net/bugs/851697
Tested-By: Jayne Han <jayne.han at canonical.com>
Thanks,
// David
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 70ba45e..1b3c89c 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -169,7 +169,7 @@ struct alc_spec {
> unsigned int auto_mic_valid_imux:1; /* valid imux for auto-mic */
> unsigned int automute:1; /* HP automute enabled */
> unsigned int detect_line:1; /* Line-out detection enabled */
> - unsigned int automute_lines:1; /* automute line-out as well */
> + unsigned int automute_lines:1; /* automute line-out as well; NOP when automute_hp_lo isn't set */
> unsigned int automute_hp_lo:1; /* both HP and LO available */
>
> /* other flags */
> @@ -556,7 +556,7 @@ static void update_speakers(struct hda_codec *codec)
> if (spec->autocfg.line_out_pins[0] == spec->autocfg.hp_pins[0] ||
> spec->autocfg.line_out_pins[0] == spec->autocfg.speaker_pins[0])
> return;
> - if (!spec->automute_lines || !spec->automute)
> + if (!spec->automute || (spec->automute_hp_lo&& !spec->automute_lines))
> on = 0;
> else
> on = spec->jack_present;
> @@ -817,7 +817,7 @@ static int alc_automute_mode_get(struct snd_kcontrol *kcontrol,
> unsigned int val;
> if (!spec->automute)
> val = 0;
> - else if (!spec->automute_lines)
> + else if (!spec->automute_hp_lo || !spec->automute_lines)
> val = 1;
> else
> val = 2;
> @@ -838,7 +838,8 @@ static int alc_automute_mode_put(struct snd_kcontrol *kcontrol,
> spec->automute = 0;
> break;
> case 1:
> - if (spec->automute&& !spec->automute_lines)
> + if (spec->automute&&
> + (!spec->automute_hp_lo || !spec->automute_lines))
> return 0;
> spec->automute = 1;
> spec->automute_lines = 0;
>
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
More information about the Alsa-devel
mailing list