From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Henningsson Subject: Re: [RFC PATCH] Fixup automute for Realtek auto parser Date: Mon, 19 Sep 2011 11:28:08 +0200 Message-ID: <4E770B28.50000@canonical.com> References: <4E73026C.5090307@canonical.com> <4E731539.6050509@canonical.com> <4E7328B2.2080501@canonical.com> <4E733171.2050107@canonical.com> <4E735B99.7090309@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by alsa0.perex.cz (Postfix) with ESMTP id 1D5392498D for ; Mon, 19 Sep 2011 11:28:12 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: ALSA Development Mailing List List-Id: alsa-devel@alsa-project.org 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] >>>>>>>>> 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@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@kernel.org (3.0) BugLink: http://bugs.launchpad.net/bugs/851697 Tested-By: Jayne Han 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