* [RFC PATCH] Fixup automute for Realtek auto parser
@ 2011-09-16 8:01 David Henningsson
2011-09-16 8:31 ` Takashi Iwai
0 siblings, 1 reply; 13+ messages in thread
From: David Henningsson @ 2011-09-16 8:01 UTC (permalink / raw)
To: ALSA Development Mailing List; +Cc: Takashi Iwai
[-- Attachment #1: Type: text/plain, Size: 543 bytes --]
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.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
[-- Attachment #2: realtek.patch --]
[-- Type: text/x-patch, Size: 8888 bytes --]
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 70ba45e..e3d3553 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -33,6 +33,10 @@
#include "hda_local.h"
#include "hda_beep.h"
+/* FIXME: Temporary since it won't compile otherwise */
+#undef CONFIG_SND_HDA_ENABLE_REALTEK_QUIRKS
+
+
/* unsol event tags */
#define ALC_FRONT_EVENT 0x01
#define ALC_DCVOL_EVENT 0x02
@@ -162,15 +166,17 @@ struct alc_spec {
void (*automute_hook)(struct hda_codec *codec);
/* for pin sensing */
- unsigned int jack_present: 1;
+ unsigned int hp_jack_present: 1;
unsigned int line_jack_present:1;
unsigned int master_mute:1;
unsigned int auto_mic:1;
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_hp_lo:1; /* both HP and LO available */
+ unsigned int detect_hp:1; /* Headphones detection enabled */
+ unsigned int detect_line:1; /* Line-out detection enabled */
+ unsigned int automute_speaker:1; /* automute speakers */
+ unsigned int automute_lo:1; /* automute line-out as well */
+ unsigned int automute_speaker_possible:1; /* there are speakers and either LO or HP */
+ unsigned int automute_lo_possible:1; /* there are line outs and HP */
/* other flags */
unsigned int no_analog :1; /* digital I/O only */
@@ -530,8 +536,8 @@ static void do_automute(struct hda_codec *codec, int num_pins, hda_nid_t *pins,
}
}
-/* Toggle internal speakers muting */
-static void update_speakers(struct hda_codec *codec)
+/* Toggle outputs muting */
+static void update_outputs(struct hda_codec *codec)
{
struct alc_spec *spec = codec->spec;
int on;
@@ -543,10 +549,10 @@ static void update_speakers(struct hda_codec *codec)
do_automute(codec, ARRAY_SIZE(spec->autocfg.hp_pins),
spec->autocfg.hp_pins, spec->master_mute, true);
- if (!spec->automute)
+ if (!spec->automute_speaker)
on = 0;
else
- on = spec->jack_present | spec->line_jack_present;
+ on = spec->hp_jack_present | spec->line_jack_present;
on |= spec->master_mute;
do_automute(codec, ARRAY_SIZE(spec->autocfg.speaker_pins),
spec->autocfg.speaker_pins, on, false);
@@ -556,22 +562,22 @@ 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_lo)
on = 0;
else
- on = spec->jack_present;
+ on = spec->hp_jack_present;
on |= spec->master_mute;
do_automute(codec, ARRAY_SIZE(spec->autocfg.line_out_pins),
spec->autocfg.line_out_pins, on, false);
}
-static void call_update_speakers(struct hda_codec *codec)
+static void call_update_outputs(struct hda_codec *codec)
{
struct alc_spec *spec = codec->spec;
if (spec->automute_hook)
spec->automute_hook(codec);
else
- update_speakers(codec);
+ update_outputs(codec);
}
/* standard HP-automute helper */
@@ -579,12 +585,12 @@ static void alc_hp_automute(struct hda_codec *codec)
{
struct alc_spec *spec = codec->spec;
- spec->jack_present =
+ spec->hp_jack_present =
detect_jacks(codec, ARRAY_SIZE(spec->autocfg.hp_pins),
spec->autocfg.hp_pins);
- if (!spec->automute)
+ if (!spec->detect_hp || (!spec->automute_speaker && !spec->automute_lo))
return;
- call_update_speakers(codec);
+ call_update_outputs(codec);
}
/* standard line-out-automute helper */
@@ -595,9 +601,9 @@ static void alc_line_automute(struct hda_codec *codec)
spec->line_jack_present =
detect_jacks(codec, ARRAY_SIZE(spec->autocfg.line_out_pins),
spec->autocfg.line_out_pins);
- if (!spec->automute || !spec->detect_line)
+ if (!spec->automute_speaker || !spec->detect_line)
return;
- call_update_speakers(codec);
+ call_update_outputs(codec);
}
#define get_connection_index(codec, mux, nid) \
@@ -795,7 +801,7 @@ static int alc_automute_mode_info(struct snd_kcontrol *kcontrol,
uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
uinfo->count = 1;
- if (spec->automute_hp_lo) {
+ if (spec->automute_speaker_possible && spec->automute_lo_possible) {
uinfo->value.enumerated.items = 3;
texts = texts3;
} else {
@@ -814,13 +820,12 @@ static int alc_automute_mode_get(struct snd_kcontrol *kcontrol,
{
struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
struct alc_spec *spec = codec->spec;
- unsigned int val;
- if (!spec->automute)
- val = 0;
- else if (!spec->automute_lines)
- val = 1;
- else
- val = 2;
+ unsigned int val = 0;
+ if (spec->automute_speaker)
+ val++;
+ if (spec->automute_lo)
+ val++;
+
ucontrol->value.enumerated.item[0] = val;
return 0;
}
@@ -833,28 +838,38 @@ static int alc_automute_mode_put(struct snd_kcontrol *kcontrol,
switch (ucontrol->value.enumerated.item[0]) {
case 0:
- if (!spec->automute)
+ if (!spec->automute_speaker && !spec->automute_lo)
return 0;
- spec->automute = 0;
+ spec->automute_speaker = 0;
+ spec->automute_lo = 0;
break;
case 1:
- if (spec->automute && !spec->automute_lines)
- return 0;
- spec->automute = 1;
- spec->automute_lines = 0;
+ if (spec->automute_speaker_possible) {
+ if (!spec->automute_lo && spec->automute_speaker)
+ return 0;
+ spec->automute_speaker = 1;
+ spec->automute_lo = 0;
+ }
+ else if (spec->automute_lo_possible) {
+ if (spec->automute_lo)
+ return 0;
+ spec->automute_lo = 1;
+ }
+ else
+ return -EINVAL;
break;
case 2:
- if (!spec->automute_hp_lo)
+ if (!spec->automute_lo_possible || !spec->automute_speaker_possible)
return -EINVAL;
- if (spec->automute && spec->automute_lines)
+ if (spec->automute_speaker && spec->automute_lo)
return 0;
- spec->automute = 1;
- spec->automute_lines = 1;
+ spec->automute_speaker = 1;
+ spec->automute_lo = 1;
break;
default:
return -EINVAL;
}
- call_update_speakers(codec);
+ call_update_outputs(codec);
return 1;
}
@@ -906,8 +921,6 @@ static void alc_init_auto_hp(struct hda_codec *codec)
present++;
if (present < 2) /* need two different output types */
return;
- if (present == 3)
- spec->automute_hp_lo = 1; /* both HP and LO automute */
if (!cfg->speaker_pins[0] &&
cfg->line_out_type == AUTO_PIN_SPEAKER_OUT) {
@@ -923,6 +936,8 @@ static void alc_init_auto_hp(struct hda_codec *codec)
cfg->hp_outs = cfg->line_outs;
}
+ spec->automute_mode = ALC_AUTOMUTE_PIN;
+
for (i = 0; i < cfg->hp_outs; i++) {
hda_nid_t nid = cfg->hp_pins[i];
if (!is_jack_detectable(codec, nid))
@@ -932,28 +947,32 @@ static void alc_init_auto_hp(struct hda_codec *codec)
snd_hda_codec_write_cache(codec, nid, 0,
AC_VERB_SET_UNSOLICITED_ENABLE,
AC_USRSP_EN | ALC_HP_EVENT);
- spec->automute = 1;
- spec->automute_mode = ALC_AUTOMUTE_PIN;
- }
- if (spec->automute && cfg->line_out_pins[0] &&
- cfg->speaker_pins[0] &&
- cfg->line_out_pins[0] != cfg->hp_pins[0] &&
- cfg->line_out_pins[0] != cfg->speaker_pins[0]) {
- for (i = 0; i < cfg->line_outs; i++) {
- hda_nid_t nid = cfg->line_out_pins[i];
- if (!is_jack_detectable(codec, nid))
- continue;
- snd_printdd("realtek: Enable Line-Out auto-muting "
- "on NID 0x%x\n", nid);
- snd_hda_codec_write_cache(codec, nid, 0,
- AC_VERB_SET_UNSOLICITED_ENABLE,
- AC_USRSP_EN | ALC_FRONT_EVENT);
- spec->detect_line = 1;
+ spec->detect_hp = 1;
+ }
+
+ if (cfg->line_out_type == AUTO_PIN_LINE_OUT && cfg->line_outs) {
+ if (cfg->speaker_outs)
+ for (i = 0; i < cfg->line_outs; i++) {
+ hda_nid_t nid = cfg->line_out_pins[i];
+ if (!is_jack_detectable(codec, nid))
+ continue;
+ snd_printdd("realtek: Enable Line-Out "
+ "auto-muting on NID 0x%x\n", nid);
+ snd_hda_codec_write_cache(codec, nid, 0,
+ AC_VERB_SET_UNSOLICITED_ENABLE,
+ AC_USRSP_EN | ALC_FRONT_EVENT);
+ spec->detect_line = 1;
}
- spec->automute_lines = spec->detect_line;
+ spec->automute_lo_possible = spec->detect_hp;
}
- if (spec->automute) {
+ spec->automute_speaker_possible = cfg->speaker_outs &&
+ (spec->detect_hp || spec->detect_line);
+
+ spec->automute_lo = spec->automute_lo_possible;
+ spec->automute_speaker = spec->automute_speaker_possible;
+
+ if (spec->automute_speaker_possible || spec->automute_lo_possible) {
/* create a control for automute mode */
alc_add_automute_mode_enum(codec);
spec->unsol_event = alc_sku_unsol_event;
@@ -4640,7 +4659,7 @@ static void alc269_fixup_stereo_dmic(struct hda_codec *codec,
static void alc269_quanta_automute(struct hda_codec *codec)
{
- update_speakers(codec);
+ update_outputs(codec);
snd_hda_codec_write(codec, 0x20, 0,
AC_VERB_SET_COEF_INDEX, 0x0c);
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 8:01 [RFC PATCH] Fixup automute for Realtek auto parser David Henningsson
@ 2011-09-16 8:31 ` Takashi Iwai
2011-09-16 9:22 ` David Henningsson
0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2011-09-16 8:31 UTC (permalink / raw)
To: David Henningsson; +Cc: ALSA Development Mailing List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 8:31 ` Takashi Iwai
@ 2011-09-16 9:22 ` David Henningsson
2011-09-16 9:35 ` Takashi Iwai
0 siblings, 1 reply; 13+ messages in thread
From: David Henningsson @ 2011-09-16 9:22 UTC (permalink / raw)
To: Takashi Iwai; +Cc: ALSA Development Mailing List
[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]
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.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
[-- Attachment #2: 0001-ALSA-HDA-Fix-Realtek-automute-for-Headphone-to-Line-.patch --]
[-- Type: text/x-patch, Size: 6602 bytes --]
>From f57b75cf26dc8c858bdb5f84babb9962c72a02ac Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Fri, 16 Sep 2011 11:12:04 +0200
Subject: [PATCH] ALSA: HDA: Fix Realtek automute for Headphone to Line Out
If the machine has "Headphone out" and "Line Out", but no
"Speaker out", the Realtek autoparser currently creates an
Automute kcontrol, but it isn't working. This patch enables
automuting of "Line out" when headphones are plugged, provided
that the automute kcontrol is set to "enabled".
BugLink: http://bugs.launchpad.net/bugs/851697
Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
sound/pci/hda/patch_realtek.c | 92 ++++++++++++++++++++++++-----------------
1 files changed, 54 insertions(+), 38 deletions(-)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 70ba45e..8dff8d2 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -167,10 +167,12 @@ struct alc_spec {
unsigned int master_mute:1;
unsigned int auto_mic:1;
unsigned int auto_mic_valid_imux:1; /* valid imux for auto-mic */
- unsigned int automute:1; /* HP automute enabled */
+ unsigned int automute:1; /* Speaker automute enabled */
+ unsigned int detect_hp:1; /* Headphones detection enabled */
unsigned int detect_line:1; /* Line-out detection enabled */
unsigned int automute_lines:1; /* automute line-out as well */
- unsigned int automute_hp_lo:1; /* both HP and LO available */
+ unsigned int automute_speaker_possible:1; /* there are speakers and either LO or HP */
+ unsigned int automute_lo_possible:1; /* there are line outs and HP */
/* other flags */
unsigned int no_analog :1; /* digital I/O only */
@@ -556,7 +558,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_lines)
on = 0;
else
on = spec->jack_present;
@@ -582,7 +584,7 @@ static void alc_hp_automute(struct hda_codec *codec)
spec->jack_present =
detect_jacks(codec, ARRAY_SIZE(spec->autocfg.hp_pins),
spec->autocfg.hp_pins);
- if (!spec->automute)
+ if (!spec->detect_hp || (!spec->automute && !spec->automute_lines))
return;
call_update_speakers(codec);
}
@@ -795,7 +797,7 @@ static int alc_automute_mode_info(struct snd_kcontrol *kcontrol,
uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
uinfo->count = 1;
- if (spec->automute_hp_lo) {
+ if (spec->automute_speaker_possible && spec->automute_lo_possible) {
uinfo->value.enumerated.items = 3;
texts = texts3;
} else {
@@ -814,13 +816,12 @@ static int alc_automute_mode_get(struct snd_kcontrol *kcontrol,
{
struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
struct alc_spec *spec = codec->spec;
- unsigned int val;
- if (!spec->automute)
- val = 0;
- else if (!spec->automute_lines)
- val = 1;
- else
- val = 2;
+ unsigned int val = 0;
+ if (spec->automute)
+ val++;
+ if (spec->automute_lines)
+ val++;
+
ucontrol->value.enumerated.item[0] = val;
return 0;
}
@@ -833,18 +834,29 @@ static int alc_automute_mode_put(struct snd_kcontrol *kcontrol,
switch (ucontrol->value.enumerated.item[0]) {
case 0:
- if (!spec->automute)
+ if (!spec->automute && !spec->automute_lines)
return 0;
spec->automute = 0;
+ spec->automute_lines = 0;
break;
case 1:
- if (spec->automute && !spec->automute_lines)
- return 0;
- spec->automute = 1;
- spec->automute_lines = 0;
+ if (spec->automute_speaker_possible) {
+ if (!spec->automute_lines && spec->automute)
+ return 0;
+ spec->automute = 1;
+ spec->automute_lines = 0;
+ }
+ else if (spec->automute_lo_possible) {
+ if (spec->automute_lines)
+ return 0;
+ spec->automute_lines = 1;
+ }
+ else
+ return -EINVAL;
+
break;
case 2:
- if (!spec->automute_hp_lo)
+ if (!spec->automute_lo_possible || !spec->automute_speaker_possible)
return -EINVAL;
if (spec->automute && spec->automute_lines)
return 0;
@@ -906,8 +918,6 @@ static void alc_init_auto_hp(struct hda_codec *codec)
present++;
if (present < 2) /* need two different output types */
return;
- if (present == 3)
- spec->automute_hp_lo = 1; /* both HP and LO automute */
if (!cfg->speaker_pins[0] &&
cfg->line_out_type == AUTO_PIN_SPEAKER_OUT) {
@@ -923,6 +933,8 @@ static void alc_init_auto_hp(struct hda_codec *codec)
cfg->hp_outs = cfg->line_outs;
}
+ spec->automute_mode = ALC_AUTOMUTE_PIN;
+
for (i = 0; i < cfg->hp_outs; i++) {
hda_nid_t nid = cfg->hp_pins[i];
if (!is_jack_detectable(codec, nid))
@@ -932,28 +944,32 @@ static void alc_init_auto_hp(struct hda_codec *codec)
snd_hda_codec_write_cache(codec, nid, 0,
AC_VERB_SET_UNSOLICITED_ENABLE,
AC_USRSP_EN | ALC_HP_EVENT);
- spec->automute = 1;
- spec->automute_mode = ALC_AUTOMUTE_PIN;
- }
- if (spec->automute && cfg->line_out_pins[0] &&
- cfg->speaker_pins[0] &&
- cfg->line_out_pins[0] != cfg->hp_pins[0] &&
- cfg->line_out_pins[0] != cfg->speaker_pins[0]) {
- for (i = 0; i < cfg->line_outs; i++) {
- hda_nid_t nid = cfg->line_out_pins[i];
- if (!is_jack_detectable(codec, nid))
- continue;
- snd_printdd("realtek: Enable Line-Out auto-muting "
- "on NID 0x%x\n", nid);
- snd_hda_codec_write_cache(codec, nid, 0,
- AC_VERB_SET_UNSOLICITED_ENABLE,
- AC_USRSP_EN | ALC_FRONT_EVENT);
- spec->detect_line = 1;
+ spec->detect_hp = 1;
+ }
+
+ if (cfg->line_out_type == AUTO_PIN_LINE_OUT && cfg->line_outs) {
+ if (cfg->speaker_outs)
+ for (i = 0; i < cfg->line_outs; i++) {
+ hda_nid_t nid = cfg->line_out_pins[i];
+ if (!is_jack_detectable(codec, nid))
+ continue;
+ snd_printdd("realtek: Enable Line-Out "
+ "auto-muting on NID 0x%x\n", nid);
+ snd_hda_codec_write_cache(codec, nid, 0,
+ AC_VERB_SET_UNSOLICITED_ENABLE,
+ AC_USRSP_EN | ALC_FRONT_EVENT);
+ spec->detect_line = 1;
}
- spec->automute_lines = spec->detect_line;
+ spec->automute_lo_possible = spec->detect_hp;
}
- if (spec->automute) {
+ spec->automute_speaker_possible = cfg->speaker_outs &&
+ (spec->detect_hp || spec->detect_line);
+
+ spec->automute_lines = spec->automute_lo_possible;
+ spec->automute = spec->automute_speaker_possible;
+
+ if (spec->automute_speaker_possible || spec->automute_lo_possible) {
/* create a control for automute mode */
alc_add_automute_mode_enum(codec);
spec->unsol_event = alc_sku_unsol_event;
--
1.7.5.4
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 9:22 ` David Henningsson
@ 2011-09-16 9:35 ` Takashi Iwai
2011-09-16 10:03 ` Takashi Iwai
0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2011-09-16 9:35 UTC (permalink / raw)
To: David Henningsson; +Cc: ALSA Development Mailing List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 9:35 ` Takashi Iwai
@ 2011-09-16 10:03 ` Takashi Iwai
2011-09-16 10:45 ` David Henningsson
0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2011-09-16 10:03 UTC (permalink / raw)
To: David Henningsson; +Cc: ALSA Development Mailing List
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;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 10:03 ` Takashi Iwai
@ 2011-09-16 10:45 ` David Henningsson
2011-09-16 11:07 ` Takashi Iwai
0 siblings, 1 reply; 13+ messages in thread
From: David Henningsson @ 2011-09-16 10:45 UTC (permalink / raw)
To: Takashi Iwai; +Cc: ALSA Development Mailing List
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? :-)
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 10:45 ` David Henningsson
@ 2011-09-16 11:07 ` Takashi Iwai
2011-09-16 11:22 ` David Henningsson
0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2011-09-16 11:07 UTC (permalink / raw)
To: David Henningsson; +Cc: ALSA Development Mailing List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 11:07 ` Takashi Iwai
@ 2011-09-16 11:22 ` David Henningsson
2011-09-16 12:44 ` Takashi Iwai
0 siblings, 1 reply; 13+ messages in thread
From: David Henningsson @ 2011-09-16 11:22 UTC (permalink / raw)
To: Takashi Iwai; +Cc: ALSA Development Mailing List
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.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 11:22 ` David Henningsson
@ 2011-09-16 12:44 ` Takashi Iwai
2011-09-16 14:22 ` David Henningsson
0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2011-09-16 12:44 UTC (permalink / raw)
To: David Henningsson; +Cc: ALSA Development Mailing List
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;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 12:44 ` Takashi Iwai
@ 2011-09-16 14:22 ` David Henningsson
2011-09-16 14:30 ` Takashi Iwai
0 siblings, 1 reply; 13+ messages in thread
From: David Henningsson @ 2011-09-16 14:22 UTC (permalink / raw)
To: Takashi Iwai; +Cc: ALSA Development Mailing List
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?
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 14:22 ` David Henningsson
@ 2011-09-16 14:30 ` Takashi Iwai
2011-09-19 9:28 ` David Henningsson
0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2011-09-16 14:30 UTC (permalink / raw)
To: David Henningsson; +Cc: ALSA Development Mailing List
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;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-16 14:30 ` Takashi Iwai
@ 2011-09-19 9:28 ` David Henningsson
2011-09-19 9:37 ` Takashi Iwai
0 siblings, 1 reply; 13+ messages in thread
From: David Henningsson @ 2011-09-19 9:28 UTC (permalink / raw)
To: Takashi Iwai; +Cc: ALSA Development Mailing List
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;
> spec->automute = 1;
> spec->automute_lines = 0;
>
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] Fixup automute for Realtek auto parser
2011-09-19 9:28 ` David Henningsson
@ 2011-09-19 9:37 ` Takashi Iwai
0 siblings, 0 replies; 13+ messages in thread
From: Takashi Iwai @ 2011-09-19 9:37 UTC (permalink / raw)
To: David Henningsson; +Cc: ALSA Development Mailing List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-09-19 9:37 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-16 8:01 [RFC PATCH] Fixup automute for Realtek auto parser David Henningsson
2011-09-16 8:31 ` Takashi Iwai
2011-09-16 9:22 ` David Henningsson
2011-09-16 9:35 ` Takashi Iwai
2011-09-16 10:03 ` Takashi Iwai
2011-09-16 10:45 ` David Henningsson
2011-09-16 11:07 ` Takashi Iwai
2011-09-16 11:22 ` David Henningsson
2011-09-16 12:44 ` Takashi Iwai
2011-09-16 14:22 ` David Henningsson
2011-09-16 14:30 ` Takashi Iwai
2011-09-19 9:28 ` David Henningsson
2011-09-19 9:37 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.