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

[Takashi Iwai]

At Mon, 19 Sep 2011 11:28:08 +0200,
David Henningsson wrote:
> 
> 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, now committed to sound git tree.


Takashi