[alsa-devel] [RFC PATCH] Fixup automute for Realtek auto parser

[David Henningsson]

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?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic