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,

Quoted 7 lines · David Henningsson

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.

Quoted 2 lines

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.

From f57b75cf26dc8c858bdb5f84babb9962c72a02ac Mon Sep 17 00:00:00 2001

[1 <text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (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,

Quoted 22 lines · David Henningsson

[1 <text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 2 lines

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.

At Fri, 16 Sep 2011 11:35:31 +0200,

Quoted 25 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:22:01 +0200,

Quoted 22 lines · David Henningsson

[1<text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 8 lines

Quoted 2 lines

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

---

sound/pci/hda/patch_realtek.c +0 -0

index 70ba45e..5c4625e 100644

Quoted 55 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:35:31 +0200,

Quoted 25 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:22:01 +0200,

Quoted 22 lines · David Henningsson

[1<text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 8 lines

Quoted 2 lines

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

---

sound/pci/hda/patch_realtek.c +1 -1

index 70ba45e..5c4625e 100644
@@ -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)

sound/pci/hda/patch_realtek.c static void update_speakers(struct hda_codec *codec) · line 562

Quoted 4 lines

sound/pci/hda/patch_realtek.c +0 -0

  		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,

Quoted 57 lines · David Henningsson

Quoted 55 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:35:31 +0200,

Quoted 25 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:22:01 +0200,

Quoted 22 lines · David Henningsson

[1<text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 8 lines

Quoted 2 lines

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

---

sound/pci/hda/patch_realtek.c +1 -1

index 70ba45e..5c4625e 100644
@@ -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)

Quoted 8 lines

sound/pci/hda/patch_realtek.c static void update_speakers(struct hda_codec *codec) · line 562

Quoted 4 lines

sound/pci/hda/patch_realtek.c +0 -0

   		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.

Quoted 2 lines

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.

Quoted 75 lines · Takashi Iwai

At Fri, 16 Sep 2011 12:45:06 +0200,

Quoted 57 lines · David Henningsson

Quoted 55 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:35:31 +0200,

Quoted 25 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:22:01 +0200,

Quoted 22 lines · David Henningsson

[1<text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 8 lines

Quoted 2 lines

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

---

sound/pci/hda/patch_realtek.c +1 -1

index 70ba45e..5c4625e 100644
@@ -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)

Quoted 8 lines

sound/pci/hda/patch_realtek.c static void update_speakers(struct hda_codec *codec) · line 562

Quoted 4 lines

sound/pci/hda/patch_realtek.c +0 -0

   		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.

Quoted 2 lines

Honestly, would you have accepted such a patch if I were the one proposing it? :-)

Yes, certainly I would. And I asked for it.

Quoted 2 lines

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,

Quoted 79 lines · David Henningsson

Quoted 77 lines · Takashi Iwai

At Fri, 16 Sep 2011 12:45:06 +0200,

Quoted 57 lines · David Henningsson

Quoted 55 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:35:31 +0200,

Quoted 25 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:22:01 +0200,

Quoted 22 lines · David Henningsson

[1<text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 8 lines

Quoted 2 lines

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

---

sound/pci/hda/patch_realtek.c +1 -1

index 70ba45e..5c4625e 100644
@@ -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)

Quoted 8 lines

sound/pci/hda/patch_realtek.c static void update_speakers(struct hda_codec *codec) · line 562

Quoted 4 lines

sound/pci/hda/patch_realtek.c +0 -0

    		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.

Quoted 2 lines

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.

Quoted 4 lines

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

---

sound/pci/hda/patch_realtek.c +3 -3

index 70ba45e..1883dcc 100644
@@ -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;

Quoted 81 lines · Takashi Iwai

At Fri, 16 Sep 2011 13:22:25 +0200,

Quoted 79 lines · David Henningsson

Quoted 77 lines · Takashi Iwai

At Fri, 16 Sep 2011 12:45:06 +0200,

Quoted 57 lines · David Henningsson

Quoted 55 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:35:31 +0200,

Quoted 25 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:22:01 +0200,

Quoted 22 lines · David Henningsson

[1<text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 8 lines

Quoted 2 lines

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

---

sound/pci/hda/patch_realtek.c +1 -1

index 70ba45e..5c4625e 100644
@@ -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)

Quoted 8 lines

sound/pci/hda/patch_realtek.c static void update_speakers(struct hda_codec *codec) · line 562

Quoted 4 lines

sound/pci/hda/patch_realtek.c +0 -0

    		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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 40 lines

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

---

sound/pci/hda/patch_realtek.c +3 -3

index 70ba45e..1883dcc 100644
@@ -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;

sound/pci/hda/patch_realtek.c static int alc_automute_mode_put(struct snd_kcontrol *kcontrol, · line 842

Quoted 6 lines

sound/pci/hda/patch_realtek.c +2 -1

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

Quoted 148 lines · David Henningsson

Quoted 81 lines · Takashi Iwai

At Fri, 16 Sep 2011 13:22:25 +0200,

Quoted 79 lines · David Henningsson

Quoted 77 lines · Takashi Iwai

At Fri, 16 Sep 2011 12:45:06 +0200,

Quoted 57 lines · David Henningsson

Quoted 55 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:35:31 +0200,

Quoted 25 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:22:01 +0200,

Quoted 22 lines · David Henningsson

[1<text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 8 lines

Quoted 2 lines

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

---

sound/pci/hda/patch_realtek.c +1 -1

index 70ba45e..5c4625e 100644
@@ -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)

Quoted 8 lines

sound/pci/hda/patch_realtek.c static void update_speakers(struct hda_codec *codec) · line 562

Quoted 4 lines

sound/pci/hda/patch_realtek.c +0 -0

     		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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 40 lines

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

---

sound/pci/hda/patch_realtek.c +3 -3

index 70ba45e..1883dcc 100644
@@ -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;

sound/pci/hda/patch_realtek.c static int alc_automute_mode_put(struct snd_kcontrol *kcontrol, · line 842

Quoted 6 lines

sound/pci/hda/patch_realtek.c +2 -1

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

Quoted 4 lines

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

---

sound/pci/hda/patch_realtek.c +5 -4

index 70ba45e..1b3c89c 100644
@@ -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;

Quoted 150 lines · Takashi Iwai

At Fri, 16 Sep 2011 16:22:17 +0200,

Quoted 148 lines · David Henningsson

Quoted 81 lines · Takashi Iwai

At Fri, 16 Sep 2011 13:22:25 +0200,

Quoted 79 lines · David Henningsson

Quoted 77 lines · Takashi Iwai

At Fri, 16 Sep 2011 12:45:06 +0200,

Quoted 57 lines · David Henningsson

Quoted 55 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:35:31 +0200,

Quoted 25 lines · Takashi Iwai

At Fri, 16 Sep 2011 11:22:01 +0200,

Quoted 22 lines · David Henningsson

[1<text/plain; ISO-8859-1 (7bit)>]

Quoted 9 lines · Takashi Iwai

At Fri, 16 Sep 2011 10:01:48 +0200,

Quoted 7 lines · David Henningsson

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.

Quoted 6 lines

Quoted 2 lines

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: [email protected] (hmm, is that working even though kernel.org is down?)

Quoted 8 lines

Quoted 2 lines

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

---

sound/pci/hda/patch_realtek.c +1 -1

index 70ba45e..5c4625e 100644
@@ -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)

Quoted 8 lines

sound/pci/hda/patch_realtek.c static void update_speakers(struct hda_codec *codec) · line 562

Quoted 4 lines

sound/pci/hda/patch_realtek.c +0 -0

     		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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 40 lines

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

---

sound/pci/hda/patch_realtek.c +3 -3

index 70ba45e..1883dcc 100644
@@ -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;

sound/pci/hda/patch_realtek.c static int alc_automute_mode_put(struct snd_kcontrol *kcontrol, · line 842

Quoted 6 lines

sound/pci/hda/patch_realtek.c +2 -1

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

Quoted 8 lines

Quoted 4 lines

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 [email protected] (3.0)

BugLink http://bugs.launchpad.net/bugs/851697

Tested-by Jayne Han <[email protected]>