[alsa-devel] [RFC PATCH] Fixup automute for Realtek auto parser
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.
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.
thanks,
Takashi
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.
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.
thanks,
Takashi
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;
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.
Honestly, would you have accepted such a patch if I were the one proposing it? :-)
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.
thanks,
Takashi
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.
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.
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.
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.
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)) return 0; spec->automute = 1; spec->automute_lines = 0;
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?
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;
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@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 jayne.han@canonical.com
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;
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@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 jayne.han@canonical.com
Thanks, now committed to sound git tree.
Takashi
participants (2)
-
David Henningsson -
Takashi Iwai