[alsa-devel] [RFC PATCH] Fixup automute for Realtek auto parser
Takashi Iwai
tiwai at suse.de
Fri Sep 16 16:30:26 CEST 2011
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.
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;
More information about the Alsa-devel
mailing list