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@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;