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

[David Henningsson]

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.

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.

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