[alsa-devel] [PATCH 14/31] HDA patch_via.c: Add Jack detect feature for VT1708.
[ALSA] HDA VIA: Add Jack detect feature for VT1708.
Signed-off-by: Lydia Wang lydiawang@viatech.com.cn
Index: sound-2.6/sound/pci/hda/patch_via.c =================================================================== --- sound-2.6.orig/sound/pci/hda/patch_via.c 2009-10-05 15:10:22.000000000 +0800 +++ sound-2.6/sound/pci/hda/patch_via.c 2009-10-05 15:10:36.000000000 +0800 @@ -257,6 +257,11 @@ unsigned int smart51_enabled; enum VIA_HDA_CODEC codec_type;
+ /* work to check hp jack state */ + struct hda_codec *codec; + struct delayed_work hp_jack_work; + int vt1708_jack_detectect; + #ifdef CONFIG_SND_HDA_POWER_SAVE struct hda_loopback_check loopback; #endif @@ -966,6 +971,8 @@ {0x20, AC_VERB_SET_CONNECT_SEL, 0x1}, /* PW9 Output enable */ {0x25, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x40}, + /* power down jack detect function */ + {0x1, 0xf81, 0x1}, { } };
@@ -1340,6 +1347,10 @@ return;
via_free_kctls(codec); + if (spec->codec_type == VT1708) { + cancel_delayed_work(&spec->hp_jack_work); + flush_scheduled_work(); + } kfree(codec->spec); }
@@ -1724,6 +1735,52 @@ return; }
+static int vt1708_jack_detectect_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct via_spec *spec = codec->spec; + + if (spec->codec_type != VT1708) + return 0; + spec->vt1708_jack_detectect = + !((snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8) & 0x1); + ucontrol->value.integer.value[0] = spec->vt1708_jack_detectect; + return 0; +} + +static int vt1708_jack_detectect_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct via_spec *spec = codec->spec; + int change; + + if (spec->codec_type != VT1708) + return 0; + spec->vt1708_jack_detectect = ucontrol->value.integer.value[0]; + snd_hda_codec_write(codec, 0x1, 0, 0xf81, !spec->vt1708_jack_detectect); + change = (0x1 & (snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8)) + == !spec->vt1708_jack_detectect; + if (spec->vt1708_jack_detectect) { + mute_aa_path(codec, 1); + notify_aa_path_ctls(codec); + } + return change; +} + +static struct snd_kcontrol_new vt1708_jack_detectect[] = { + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Jack Detect", + .count = 1, + .info = snd_ctl_boolean_mono_info, + .get = vt1708_jack_detectect_get, + .put = vt1708_jack_detectect_put, + }, + {} /* end */ +}; + static int vt1708_parse_auto_config(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1751,6 +1808,10 @@ err = vt1708_auto_create_analog_input_ctls(spec, &spec->autocfg); if (err < 0) return err; + /* add jack detect on/off control */ + err = snd_hda_add_new_ctls(codec, vt1708_jack_detectect); + if (err < 0) + return err;
spec->multiout.max_channels = spec->multiout.num_dacs * 2;
@@ -1784,6 +1845,24 @@ return 0; }
+static void update_hp_jack_state(struct work_struct *work) +{ + struct via_spec *spec = container_of(work, struct via_spec, + hp_jack_work.work); + static int present = 1; + + if (spec->codec_type != VT1708) + return; + /* if jack state toggled */ + if (present + != (snd_hda_codec_read(spec->codec, spec->autocfg.hp_pins[0], 0, + AC_VERB_GET_PIN_SENSE, 0) >> 31)) { + present ^= 1; + via_hp_automute(spec->codec); + } + schedule_delayed_work(&spec->hp_jack_work, msecs_to_jiffies(100)); +} + static int get_mux_nids(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1860,7 +1939,9 @@ #ifdef CONFIG_SND_HDA_POWER_SAVE spec->loopback.amplist = vt1708_loopbacks; #endif - + spec->codec = codec; + INIT_DELAYED_WORK(&spec->hp_jack_work, update_hp_jack_state); + schedule_delayed_work(&spec->hp_jack_work, msecs_to_jiffies(100)); return 0; }
At Mon, 5 Oct 2009 22:27:20 +0800, Li Bo wrote:
[ALSA] HDA VIA: Add Jack detect feature for VT1708.
Why is it needed? Isn't the unsolicited event handled properly at HP plugging?
Also, this implementation doesn't look good. If I understand correctly, it invokes a workqueue which re-schedules itself at each 100ms...
Takashi
Signed-off-by: Lydia Wang lydiawang@viatech.com.cn
Index: sound-2.6/sound/pci/hda/patch_via.c
--- sound-2.6.orig/sound/pci/hda/patch_via.c 2009-10-05 15:10:22.000000000 +0800 +++ sound-2.6/sound/pci/hda/patch_via.c 2009-10-05 15:10:36.000000000 +0800 @@ -257,6 +257,11 @@ unsigned int smart51_enabled; enum VIA_HDA_CODEC codec_type;
- /* work to check hp jack state */
- struct hda_codec *codec;
- struct delayed_work hp_jack_work;
- int vt1708_jack_detectect;
#ifdef CONFIG_SND_HDA_POWER_SAVE struct hda_loopback_check loopback; #endif @@ -966,6 +971,8 @@ {0x20, AC_VERB_SET_CONNECT_SEL, 0x1}, /* PW9 Output enable */ {0x25, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x40},
- /* power down jack detect function */
- {0x1, 0xf81, 0x1}, { }
};
@@ -1340,6 +1347,10 @@ return;
via_free_kctls(codec);
- if (spec->codec_type == VT1708) {
cancel_delayed_work(&spec->hp_jack_work);flush_scheduled_work();- } kfree(codec->spec);
}
@@ -1724,6 +1735,52 @@ return; }
+static int vt1708_jack_detectect_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct via_spec *spec = codec->spec;
- if (spec->codec_type != VT1708)
return 0;- spec->vt1708_jack_detectect =
!((snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8) & 0x1);- ucontrol->value.integer.value[0] = spec->vt1708_jack_detectect;
- return 0;
+}
+static int vt1708_jack_detectect_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct via_spec *spec = codec->spec;
- int change;
- if (spec->codec_type != VT1708)
return 0;- spec->vt1708_jack_detectect = ucontrol->value.integer.value[0];
- snd_hda_codec_write(codec, 0x1, 0, 0xf81, !spec->vt1708_jack_detectect);
- change = (0x1 & (snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8))
== !spec->vt1708_jack_detectect;- if (spec->vt1708_jack_detectect) {
mute_aa_path(codec, 1);notify_aa_path_ctls(codec);- }
- return change;
+}
+static struct snd_kcontrol_new vt1708_jack_detectect[] = {
- {
.iface = SNDRV_CTL_ELEM_IFACE_MIXER,.name = "Jack Detect",.count = 1,.info = snd_ctl_boolean_mono_info,.get = vt1708_jack_detectect_get,.put = vt1708_jack_detectect_put,- },
- {} /* end */
+};
static int vt1708_parse_auto_config(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1751,6 +1808,10 @@ err = vt1708_auto_create_analog_input_ctls(spec, &spec->autocfg); if (err < 0) return err;
/* add jack detect on/off control */
err = snd_hda_add_new_ctls(codec, vt1708_jack_detectect);
if (err < 0)
return err;spec->multiout.max_channels = spec->multiout.num_dacs * 2;
@@ -1784,6 +1845,24 @@ return 0; }
+static void update_hp_jack_state(struct work_struct *work) +{
- struct via_spec *spec = container_of(work, struct via_spec,
hp_jack_work.work);- static int present = 1;
- if (spec->codec_type != VT1708)
return;- /* if jack state toggled */
- if (present
!= (snd_hda_codec_read(spec->codec, spec->autocfg.hp_pins[0], 0,AC_VERB_GET_PIN_SENSE, 0) >> 31)) {present ^= 1;via_hp_automute(spec->codec);- }
- schedule_delayed_work(&spec->hp_jack_work, msecs_to_jiffies(100));
+}
static int get_mux_nids(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1860,7 +1939,9 @@ #ifdef CONFIG_SND_HDA_POWER_SAVE spec->loopback.amplist = vt1708_loopbacks; #endif
- spec->codec = codec;
- INIT_DELAYED_WORK(&spec->hp_jack_work, update_hp_jack_state);
- schedule_delayed_work(&spec->hp_jack_work, msecs_to_jiffies(100)); return 0;
}
VT1708 HW doesn't support unsolicited response, so we polling with jack detect function instead. As you describe, we implement it with self-reschedule workqueue, acting like a timer. Is there some advice to implement timer-like task, I tested with timer and it will crash the driver.
On Mon, Oct 5, 2009 at 11:12 PM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 5 Oct 2009 22:27:20 +0800, Li Bo wrote:
[ALSA] HDA VIA: Add Jack detect feature for VT1708.
Why is it needed? Isn't the unsolicited event handled properly at HP plugging?
Also, this implementation doesn't look good. If I understand correctly, it invokes a workqueue which re-schedules itself at each 100ms...
Takashi
Signed-off-by: Lydia Wang lydiawang@viatech.com.cn
Index: sound-2.6/sound/pci/hda/patch_via.c
--- sound-2.6.orig/sound/pci/hda/patch_via.c 2009-10-05 15:10:22.000000000 +0800 +++ sound-2.6/sound/pci/hda/patch_via.c 2009-10-05 15:10:36.000000000 +0800 @@ -257,6 +257,11 @@ unsigned int smart51_enabled; enum VIA_HDA_CODEC codec_type;
- /* work to check hp jack state */
- struct hda_codec *codec;
- struct delayed_work hp_jack_work;
- int vt1708_jack_detectect;
#ifdef CONFIG_SND_HDA_POWER_SAVE struct hda_loopback_check loopback; #endif @@ -966,6 +971,8 @@ {0x20, AC_VERB_SET_CONNECT_SEL, 0x1}, /* PW9 Output enable */ {0x25, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x40},
- /* power down jack detect function */
- {0x1, 0xf81, 0x1},
{ } };
@@ -1340,6 +1347,10 @@ return;
via_free_kctls(codec);
- if (spec->codec_type == VT1708) {
- cancel_delayed_work(&spec->hp_jack_work);
- flush_scheduled_work();
- }
kfree(codec->spec); }
@@ -1724,6 +1735,52 @@ return; }
+static int vt1708_jack_detectect_get(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct via_spec *spec = codec->spec;
- if (spec->codec_type != VT1708)
- return 0;
- spec->vt1708_jack_detectect =
- !((snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8) & 0x1);
- ucontrol->value.integer.value[0] = spec->vt1708_jack_detectect;
- return 0;
+}
+static int vt1708_jack_detectect_put(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct via_spec *spec = codec->spec;
- int change;
- if (spec->codec_type != VT1708)
- return 0;
- spec->vt1708_jack_detectect = ucontrol->value.integer.value[0];
- snd_hda_codec_write(codec, 0x1, 0, 0xf81, !spec->vt1708_jack_detectect);
- change = (0x1 & (snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8))
- == !spec->vt1708_jack_detectect;
- if (spec->vt1708_jack_detectect) {
- mute_aa_path(codec, 1);
- notify_aa_path_ctls(codec);
- }
- return change;
+}
+static struct snd_kcontrol_new vt1708_jack_detectect[] = {
- {
- .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
- .name = "Jack Detect",
- .count = 1,
- .info = snd_ctl_boolean_mono_info,
- .get = vt1708_jack_detectect_get,
- .put = vt1708_jack_detectect_put,
- },
- {} /* end */
+};
static int vt1708_parse_auto_config(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1751,6 +1808,10 @@ err = vt1708_auto_create_analog_input_ctls(spec, &spec->autocfg); if (err < 0) return err;
- /* add jack detect on/off control */
- err = snd_hda_add_new_ctls(codec, vt1708_jack_detectect);
- if (err < 0)
- return err;
spec->multiout.max_channels = spec->multiout.num_dacs * 2;
@@ -1784,6 +1845,24 @@ return 0; }
+static void update_hp_jack_state(struct work_struct *work) +{
- struct via_spec *spec = container_of(work, struct via_spec,
- hp_jack_work.work);
- static int present = 1;
- if (spec->codec_type != VT1708)
- return;
- /* if jack state toggled */
- if (present
- != (snd_hda_codec_read(spec->codec, spec->autocfg.hp_pins[0], 0,
- AC_VERB_GET_PIN_SENSE, 0) >> 31)) {
- present ^= 1;
- via_hp_automute(spec->codec);
- }
- schedule_delayed_work(&spec->hp_jack_work, msecs_to_jiffies(100));
+}
static int get_mux_nids(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1860,7 +1939,9 @@ #ifdef CONFIG_SND_HDA_POWER_SAVE spec->loopback.amplist = vt1708_loopbacks; #endif
- spec->codec = codec;
- INIT_DELAYED_WORK(&spec->hp_jack_work, update_hp_jack_state);
- schedule_delayed_work(&spec->hp_jack_work, msecs_to_jiffies(100));
return 0; }
At Tue, 6 Oct 2009 13:06:40 +0800, Li Bo wrote:
VT1708 HW doesn't support unsolicited response, so we polling with jack detect function instead.
Ah OK. Please write this essential information in the changelog text!
As you describe, we implement it with self-reschedule workqueue, acting like a timer. Is there some advice to implement timer-like task, I tested with timer and it will crash the driver.
Right, you can't call it in a softirq.
A few problems, however: - present variable should be in struct via_spec, instead of a static variable in update_hp_jack_state(). - This mechanism is unconditionally invoked on VT1708 although, in theory, you can have a hardware that doesn't need HP jack detection - Waking up each 100ms for the *whole* time is really bad regarding the power-saving; can't it be optimized? - You need to cancel/trigger the work at suspend/resume, at least
Takashi
On Mon, Oct 5, 2009 at 11:12 PM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 5 Oct 2009 22:27:20 +0800, Li Bo wrote:
[ALSA] HDA VIA: Add Jack detect feature for VT1708.
Why is it needed? Isn't the unsolicited event handled properly at HP plugging?
Also, this implementation doesn't look good. If I understand correctly, it invokes a workqueue which re-schedules itself at each 100ms...
Takashi
Signed-off-by: Lydia Wang lydiawang@viatech.com.cn
Index: sound-2.6/sound/pci/hda/patch_via.c
--- sound-2.6.orig/sound/pci/hda/patch_via.c 2009-10-05 15:10:22.000000000 +0800 +++ sound-2.6/sound/pci/hda/patch_via.c 2009-10-05 15:10:36.000000000 +0800 @@ -257,6 +257,11 @@ unsigned int smart51_enabled; enum VIA_HDA_CODEC codec_type;
- /* work to check hp jack state */
- struct hda_codec *codec;
- struct delayed_work hp_jack_work;
- int vt1708_jack_detectect;
#ifdef CONFIG_SND_HDA_POWER_SAVE struct hda_loopback_check loopback; #endif @@ -966,6 +971,8 @@ {0x20, AC_VERB_SET_CONNECT_SEL, 0x1}, /* PW9 Output enable */ {0x25, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x40},
- /* power down jack detect function */
- {0x1, 0xf81, 0x1},
{ } };
@@ -1340,6 +1347,10 @@ return;
via_free_kctls(codec);
- if (spec->codec_type == VT1708) {
- cancel_delayed_work(&spec->hp_jack_work);
- flush_scheduled_work();
- }
kfree(codec->spec); }
@@ -1724,6 +1735,52 @@ return; }
+static int vt1708_jack_detectect_get(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct via_spec *spec = codec->spec;
- if (spec->codec_type != VT1708)
- return 0;
- spec->vt1708_jack_detectect =
- !((snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8) & 0x1);
- ucontrol->value.integer.value[0] = spec->vt1708_jack_detectect;
- return 0;
+}
+static int vt1708_jack_detectect_put(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct via_spec *spec = codec->spec;
- int change;
- if (spec->codec_type != VT1708)
- return 0;
- spec->vt1708_jack_detectect = ucontrol->value.integer.value[0];
- snd_hda_codec_write(codec, 0x1, 0, 0xf81, !spec->vt1708_jack_detectect);
- change = (0x1 & (snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8))
- == !spec->vt1708_jack_detectect;
- if (spec->vt1708_jack_detectect) {
- mute_aa_path(codec, 1);
- notify_aa_path_ctls(codec);
- }
- return change;
+}
+static struct snd_kcontrol_new vt1708_jack_detectect[] = {
- {
- .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
- .name = "Jack Detect",
- .count = 1,
- .info = snd_ctl_boolean_mono_info,
- .get = vt1708_jack_detectect_get,
- .put = vt1708_jack_detectect_put,
- },
- {} /* end */
+};
static int vt1708_parse_auto_config(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1751,6 +1808,10 @@ err = vt1708_auto_create_analog_input_ctls(spec, &spec->autocfg); if (err < 0) return err;
- /* add jack detect on/off control */
- err = snd_hda_add_new_ctls(codec, vt1708_jack_detectect);
- if (err < 0)
- return err;
spec->multiout.max_channels = spec->multiout.num_dacs * 2;
@@ -1784,6 +1845,24 @@ return 0; }
+static void update_hp_jack_state(struct work_struct *work) +{
- struct via_spec *spec = container_of(work, struct via_spec,
- hp_jack_work.work);
- static int present = 1;
- if (spec->codec_type != VT1708)
- return;
- /* if jack state toggled */
- if (present
- != (snd_hda_codec_read(spec->codec, spec->autocfg.hp_pins[0], 0,
- AC_VERB_GET_PIN_SENSE, 0) >> 31)) {
- present ^= 1;
- via_hp_automute(spec->codec);
- }
- schedule_delayed_work(&spec->hp_jack_work, msecs_to_jiffies(100));
+}
static int get_mux_nids(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1860,7 +1939,9 @@ #ifdef CONFIG_SND_HDA_POWER_SAVE spec->loopback.amplist = vt1708_loopbacks; #endif
- spec->codec = codec;
- INIT_DELAYED_WORK(&spec->hp_jack_work, update_hp_jack_state);
- schedule_delayed_work(&spec->hp_jack_work, msecs_to_jiffies(100));
return 0; }
Hi all,
On Tue, Oct 06, 2009 at 08:01:40AM +0200, Takashi Iwai wrote:
As you describe, we implement it with self-reschedule workqueue, acting like a timer. Is there some advice to implement timer-like task, I tested with timer and it will crash the driver.
Right, you can't call it in a softirq.
A few problems, however:
- present variable should be in struct via_spec, instead of a static variable in update_hp_jack_state().
I agree
- This mechanism is unconditionally invoked on VT1708 although, in theory, you can have a hardware that doesn't need HP jack detection
Yes, Logan/Li/Lydia: If we generate this kind of workaround with a timer, it should only be used on those particular chips that absolutely need it.
- Waking up each 100ms for the *whole* time is really bad regarding the power-saving; can't it be optimized?
I would personally actually suggest to simply not support headphone plug events in case you are using such a [buggy?] chipset that does not support them.
I mean, what do you gain from that event? It's not something super-important.
waking up unconditionally every 100ms is really a too high burden on the battery life.
Another option might be to make it a module load time parameter, so people can choose if they actually want to pay the high power/batyery price for this small feature.
At Tue, 6 Oct 2009 14:17:40 +0200, Harald Welte wrote:
Hi all,
On Tue, Oct 06, 2009 at 08:01:40AM +0200, Takashi Iwai wrote:
As you describe, we implement it with self-reschedule workqueue, acting like a timer. Is there some advice to implement timer-like task, I tested with timer and it will crash the driver.
Right, you can't call it in a softirq.
A few problems, however:
- present variable should be in struct via_spec, instead of a static variable in update_hp_jack_state().
I agree
- This mechanism is unconditionally invoked on VT1708 although, in theory, you can have a hardware that doesn't need HP jack detection
Yes, Logan/Li/Lydia: If we generate this kind of workaround with a timer, it should only be used on those particular chips that absolutely need it.
As far as I see, the code is only for VT1708, so it's already specific to the codec chip. But, even with this chip, you might have a pin configuration that doesn't need the HP jack detection.
- Waking up each 100ms for the *whole* time is really bad regarding the power-saving; can't it be optimized?
I would personally actually suggest to simply not support headphone plug events in case you are using such a [buggy?] chipset that does not support them.
I mean, what do you gain from that event? It's not something super-important.
waking up unconditionally every 100ms is really a too high burden on the battery life.
Another option might be to make it a module load time parameter, so people can choose if they actually want to pay the high power/batyery price for this small feature.
Yes.
Basically, if we can drop the analog-loopback feature, the HP jack probing can be simplified very much. The probing is needed only when the PCM is opened/prepared and running. It can be implemented in a similar way for the volume-update procedure at PCM trigger. And, during PCM running, (relatively) high-frequent polling isn't so critical.
thanks,
Takashi
On Tue, Oct 6, 2009 at 8:38 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 6 Oct 2009 14:17:40 +0200, Harald Welte wrote:
Hi all,
On Tue, Oct 06, 2009 at 08:01:40AM +0200, Takashi Iwai wrote:
As you describe, we implement it with self-reschedule workqueue, acting like a timer. Is there some advice to implement timer-like task, I tested with timer and it will crash the driver.
Right, you can't call it in a softirq.
A few problems, however:
- present variable should be in struct via_spec, instead of a static
variable in update_hp_jack_state().
I agree
- This mechanism is unconditionally invoked on VT1708 although, in
theory, you can have a hardware that doesn't need HP jack detection
Yes, Logan/Li/Lydia: If we generate this kind of workaround with a timer, it should only be used on those particular chips that absolutely need it.
As far as I see, the code is only for VT1708, so it's already specific to the codec chip. But, even with this chip, you might have a pin configuration that doesn't need the HP jack detection.
Yes, it's only for VT1708, and HP exists on most cases, and so HP jack detection are needed. And present will put into via_spec.
- Waking up each 100ms for the *whole* time is really bad regarding
the power-saving; can't it be optimized?
I would personally actually suggest to simply not support headphone plug events in case you are using such a [buggy?] chipset that does not support them.
I mean, what do you gain from that event? It's not something super-important.
waking up unconditionally every 100ms is really a too high burden on the battery life.
Another option might be to make it a module load time parameter, so people can choose if they actually want to pay the high power/batyery price for this small feature.
Yes.
Basically, if we can drop the analog-loopback feature, the HP jack probing can be simplified very much. The probing is needed only when the PCM is opened/prepared and running. It can be implemented in a similar way for the volume-update procedure at PCM trigger. And, during PCM running, (relatively) high-frequent polling isn't so critical.
thanks,
Takashi
I know that jack detect on VT1708 that do not support unsolicited response is weird, but it's for customer request (and so are some other patches:), we just have to do something.
I think it's good in open/close callback, I'll update.
About self-reschedule workqueue, maybe there are some other choices, or should we keep using it?
At Tue, 6 Oct 2009 23:27:35 +0800, Li Bo wrote:
On Tue, Oct 6, 2009 at 8:38 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 6 Oct 2009 14:17:40 +0200, Harald Welte wrote:
Hi all,
On Tue, Oct 06, 2009 at 08:01:40AM +0200, Takashi Iwai wrote:
As you describe, we implement it with self-reschedule workqueue, acting like a timer. Is there some advice to implement timer-like task, I tested with timer and it will crash the driver.
Right, you can't call it in a softirq.
A few problems, however:
- present variable should be in struct via_spec, instead of a static
variable in update_hp_jack_state().
I agree
- This mechanism is unconditionally invoked on VT1708 although, in
theory, you can have a hardware that doesn't need HP jack detection
Yes, Logan/Li/Lydia: If we generate this kind of workaround with a timer, it should only be used on those particular chips that absolutely need it.
As far as I see, the code is only for VT1708, so it's already specific to the codec chip. But, even with this chip, you might have a pin configuration that doesn't need the HP jack detection.
Yes, it's only for VT1708, and HP exists on most cases, and so HP jack detection are needed. And present will put into via_spec.
If it's restricted only during the opened (better running) PCM stream, it's not too bad. But, it'd be even better to check the availability of HP jack.
- Waking up each 100ms for the *whole* time is really bad regarding
the power-saving; can't it be optimized?
I would personally actually suggest to simply not support headphone plug events in case you are using such a [buggy?] chipset that does not support them.
I mean, what do you gain from that event? It's not something super-important.
waking up unconditionally every 100ms is really a too high burden on the battery life.
Another option might be to make it a module load time parameter, so people can choose if they actually want to pay the high power/batyery price for this small feature.
Yes.
Basically, if we can drop the analog-loopback feature, the HP jack probing can be simplified very much. The probing is needed only when the PCM is opened/prepared and running. It can be implemented in a similar way for the volume-update procedure at PCM trigger. And, during PCM running, (relatively) high-frequent polling isn't so critical.
thanks,
Takashi
I know that jack detect on VT1708 that do not support unsolicited response is weird, but it's for customer request (and so are some other patches:), we just have to do something.
I think it's good in open/close callback, I'll update.
About self-reschedule workqueue, maybe there are some other choices, or should we keep using it?
A delayed workqueue is OK. But, it should check some flag before rescheduling itself. Also, don't forget about suspend/resume.
thanks,
Takashi
On Tue, Oct 6, 2009 at 11:34 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 6 Oct 2009 23:27:35 +0800, Li Bo wrote:
On Tue, Oct 6, 2009 at 8:38 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 6 Oct 2009 14:17:40 +0200, Harald Welte wrote:
Hi all,
On Tue, Oct 06, 2009 at 08:01:40AM +0200, Takashi Iwai wrote:
As you describe, we implement it with self-reschedule workqueue, acting like a timer. Is there some advice to implement timer-like task, I tested with timer and it will crash the driver.
Right, you can't call it in a softirq.
A few problems, however:
- present variable should be in struct via_spec, instead of a static
variable in update_hp_jack_state().
I agree
- This mechanism is unconditionally invoked on VT1708 although, in
theory, you can have a hardware that doesn't need HP jack detection
Yes, Logan/Li/Lydia: If we generate this kind of workaround with a timer, it should only be used on those particular chips that absolutely need it.
As far as I see, the code is only for VT1708, so it's already specific to the codec chip. But, even with this chip, you might have a pin configuration that doesn't need the HP jack detection.
Yes, it's only for VT1708, and HP exists on most cases, and so HP jack detection are needed. And present will put into via_spec.
If it's restricted only during the opened (better running) PCM stream, it's not too bad. But, it'd be even better to check the availability of HP jack.
Sorry about analog loopback is not considered, I think analog loopback mute/unmute callback should also be added, is that OK?
- Waking up each 100ms for the *whole* time is really bad regarding
the power-saving; can't it be optimized?
I would personally actually suggest to simply not support headphone plug events in case you are using such a [buggy?] chipset that does not support them.
I mean, what do you gain from that event? It's not something super-important.
waking up unconditionally every 100ms is really a too high burden on the battery life.
Another option might be to make it a module load time parameter, so people can choose if they actually want to pay the high power/batyery price for this small feature.
Yes.
Basically, if we can drop the analog-loopback feature, the HP jack probing can be simplified very much. The probing is needed only when the PCM is opened/prepared and running. It can be implemented in a similar way for the volume-update procedure at PCM trigger. And, during PCM running, (relatively) high-frequent polling isn't so critical.
thanks,
Takashi
I know that jack detect on VT1708 that do not support unsolicited response is weird, but it's for customer request (and so are some other patches:), we just have to do something.
I think it's good in open/close callback, I'll update.
About self-reschedule workqueue, maybe there are some other choices, or should we keep using it?
A delayed workqueue is OK. But, it should check some flag before rescheduling itself. Also, don't forget about suspend/resume.
thanks,
Takashi
Thank you, I will rewrite like this.
At Wed, 7 Oct 2009 13:56:35 +0800, Li Bo wrote:
On Tue, Oct 6, 2009 at 11:34 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 6 Oct 2009 23:27:35 +0800, Li Bo wrote:
On Tue, Oct 6, 2009 at 8:38 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 6 Oct 2009 14:17:40 +0200, Harald Welte wrote:
Hi all,
On Tue, Oct 06, 2009 at 08:01:40AM +0200, Takashi Iwai wrote:
> As you describe, we implement it with self-reschedule workqueue, acting > like a timer. > Is there some advice to implement timer-like task, I tested with timer and > it will crash the driver.
Right, you can't call it in a softirq.
A few problems, however:
- present variable should be in struct via_spec, instead of a static
variable in update_hp_jack_state().
I agree
- This mechanism is unconditionally invoked on VT1708 although, in
theory, you can have a hardware that doesn't need HP jack detection
Yes, Logan/Li/Lydia: If we generate this kind of workaround with a timer, it should only be used on those particular chips that absolutely need it.
As far as I see, the code is only for VT1708, so it's already specific to the codec chip. But, even with this chip, you might have a pin configuration that doesn't need the HP jack detection.
Yes, it's only for VT1708, and HP exists on most cases, and so HP jack detection are needed. And present will put into via_spec.
If it's restricted only during the opened (better running) PCM stream, it's not too bad. But, it'd be even better to check the availability of HP jack.
Sorry about analog loopback is not considered, I think analog loopback mute/unmute callback should also be added, is that OK?
Honestly, I think we can remove the analog-loopback part completely for VT1708 + HP-jack. If the loopback is inevitably needed, you can add a check of codec "hint" interface to determine whether to build the HP-jack detection or not at probing. About hints, look at snd_hda_get_bool_hint() and HD-Audio.txt.
thanks,
Takashi
On Wed, Oct 7, 2009 at 3:03 PM, Takashi Iwai tiwai@suse.de wrote:
At Wed, 7 Oct 2009 13:56:35 +0800, Li Bo wrote:
On Tue, Oct 6, 2009 at 11:34 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 6 Oct 2009 23:27:35 +0800, Li Bo wrote:
On Tue, Oct 6, 2009 at 8:38 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 6 Oct 2009 14:17:40 +0200, Harald Welte wrote:
Hi all,
On Tue, Oct 06, 2009 at 08:01:40AM +0200, Takashi Iwai wrote:
> > As you describe, we implement it with self-reschedule workqueue, acting > > like a timer. > > Is there some advice to implement timer-like task, I tested with timer and > > it will crash the driver. > > Right, you can't call it in a softirq. > > A few problems, however: > - present variable should be in struct via_spec, instead of a static > variable in update_hp_jack_state().
I agree > - This mechanism is unconditionally invoked on VT1708 although, in > theory, you can have a hardware that doesn't need HP jack detection
Yes, Logan/Li/Lydia: If we generate this kind of workaround with a timer, it should only be used on those particular chips that absolutely need it.
As far as I see, the code is only for VT1708, so it's already specific to the codec chip. But, even with this chip, you might have a pin configuration that doesn't need the HP jack detection.
Yes, it's only for VT1708, and HP exists on most cases, and so HP jack detection are needed. And present will put into via_spec.
If it's restricted only during the opened (better running) PCM stream, it's not too bad. But, it'd be even better to check the availability of HP jack.
Sorry about analog loopback is not considered, I think analog loopback mute/unmute callback should also be added, is that OK?
Honestly, I think we can remove the analog-loopback part completely for VT1708 + HP-jack. If the loopback is inevitably needed, you can add a check of codec "hint" interface to determine whether to build the HP-jack detection or not at probing. About hints, look at snd_hda_get_bool_hint() and HD-Audio.txt.
thanks,
Takashi
OK, I add hp detect to prepare/cleanup, and check hint "analog_loopback_hp_detect" before adding it to analog mute/unmute. Following will be the update patch
[ALSA] HDA VIA: Add Jack detect feature for VT1708.
VT1708 does not support unsolicited response, but we need hp detect to automute speaker. Implemented in workqueue.
Signed-off-by: Lydia Wang lydiawang@viatech.com.cn
--- sound/pci/hda/patch_via.c | 224 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 168 insertions(+), 56 deletions(-)
--- a/sound/pci/hda/patch_via.c +++ b/sound/pci/hda/patch_via.c @@ -89,6 +89,64 @@ CODEC_TYPES, };
+struct via_spec { + /* codec parameterization */ + struct snd_kcontrol_new *mixers[4]; + unsigned int num_mixers; + + struct hda_verb *init_verbs[5]; + unsigned int num_iverbs; + + char *stream_name_analog; + struct hda_pcm_stream *stream_analog_playback; + struct hda_pcm_stream *stream_analog_capture; + + char *stream_name_digital; + struct hda_pcm_stream *stream_digital_playback; + struct hda_pcm_stream *stream_digital_capture; + + /* playback */ + struct hda_multi_out multiout; + hda_nid_t slave_dig_outs[2]; + + /* capture */ + unsigned int num_adc_nids; + hda_nid_t *adc_nids; + hda_nid_t mux_nids[3]; + hda_nid_t dig_in_nid; + hda_nid_t dig_in_pin; + + /* capture source */ + const struct hda_input_mux *input_mux; + unsigned int cur_mux[3]; + + /* PCM information */ + struct hda_pcm pcm_rec[3]; + + /* dynamic controls, init_verbs and input_mux */ + struct auto_pin_cfg autocfg; + struct snd_array kctls; + struct hda_input_mux private_imux[2]; + hda_nid_t private_dac_nids[AUTO_CFG_MAX_OUTS]; + + /* HP mode source */ + const struct hda_input_mux *hp_mux; + unsigned int hp_independent_mode; + unsigned int hp_independent_mode_index; + unsigned int smart51_enabled; + + enum VIA_HDA_CODEC codec_type; + + /* work to check hp jack state */ + struct hda_codec *codec; + struct delayed_work vt1708_hp_work; + int vt1708_jack_detectect; + int vt1708_hp_present; +#ifdef CONFIG_SND_HDA_POWER_SAVE + struct hda_loopback_check loopback; +#endif +}; + static enum VIA_HDA_CODEC get_codec_type(struct hda_codec *codec) { u32 vendor_id = codec->vendor_id; @@ -181,6 +239,28 @@
static void analog_low_current_mode(struct hda_codec *codec, int stream_idle); static void set_jack_power_state(struct hda_codec *codec); +static int is_aa_path_mute(struct hda_codec *codec); + +static void vt1708_start_hp_work(struct via_spec *spec) +{ + if (spec->codec_type != VT1708) + return; + snd_hda_codec_write(spec->codec, 0x1, 0, 0xf81, + !spec->vt1708_jack_detectect); + if (!delayed_work_pending(&spec->vt1708_hp_work)) + schedule_delayed_work(&spec->vt1708_hp_work, + msecs_to_jiffies(100)); +} + +static void vt1708_stop_hp_work(struct via_spec *spec) +{ + if (spec->codec_type != VT1708) + return; + snd_hda_codec_write(spec->codec, 0x1, 0, 0xf81, + !spec->vt1708_jack_detectect); + cancel_delayed_work(&spec->vt1708_hp_work); + flush_scheduled_work(); +}
static int analog_input_switch_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) @@ -190,6 +270,12 @@
set_jack_power_state(codec); analog_low_current_mode(snd_kcontrol_chip(kcontrol), -1); + if (snd_hda_get_bool_hint(codec, "analog_loopback_hp_detect") == 1) { + if (is_aa_path_mute(codec)) + vt1708_start_hp_work(codec->spec); + else + vt1708_stop_hp_work(codec->spec); + } return change; }
@@ -210,59 +296,6 @@ };
-struct via_spec { - /* codec parameterization */ - struct snd_kcontrol_new *mixers[4]; - unsigned int num_mixers; - - struct hda_verb *init_verbs[5]; - unsigned int num_iverbs; - - char *stream_name_analog; - struct hda_pcm_stream *stream_analog_playback; - struct hda_pcm_stream *stream_analog_capture; - - char *stream_name_digital; - struct hda_pcm_stream *stream_digital_playback; - struct hda_pcm_stream *stream_digital_capture; - - /* playback */ - struct hda_multi_out multiout; - hda_nid_t slave_dig_outs[2]; - - /* capture */ - unsigned int num_adc_nids; - hda_nid_t *adc_nids; - hda_nid_t mux_nids[3]; - hda_nid_t dig_in_nid; - hda_nid_t dig_in_pin; - - /* capture source */ - const struct hda_input_mux *input_mux; - unsigned int cur_mux[3]; - - /* PCM information */ - struct hda_pcm pcm_rec[3]; - - /* dynamic controls, init_verbs and input_mux */ - struct auto_pin_cfg autocfg; - struct snd_array kctls; - struct hda_input_mux private_imux[2]; - hda_nid_t private_dac_nids[AUTO_CFG_MAX_OUTS]; - - /* HP mode source */ - const struct hda_input_mux *hp_mux; - unsigned int hp_independent_mode; - unsigned int hp_independent_mode_index; - unsigned int smart51_enabled; - - enum VIA_HDA_CODEC codec_type; - -#ifdef CONFIG_SND_HDA_POWER_SAVE - struct hda_loopback_check loopback; -#endif -}; - static hda_nid_t vt1708_adc_nids[2] = { /* ADC1-2 */ 0x15, 0x27 @@ -1094,7 +1127,7 @@ snd_hda_codec_setup_stream(codec, mout->hp_nid, stream_tag, 0, format); } - + vt1708_start_hp_work(spec); return 0; }
@@ -1134,7 +1167,7 @@ snd_hda_codec_setup_stream(codec, mout->hp_nid, 0, 0, 0); } - + vt1708_stop_hp_work(spec); return 0; }
@@ -1345,6 +1378,7 @@ return;
via_free_kctls(codec); + vt1708_stop_hp_work(spec); kfree(codec->spec); }
@@ -1464,6 +1498,15 @@ return 0; }
+#ifdef SND_HDA_NEEDS_RESUME +static int via_suspend(struct hda_codec *codec, pm_message_t state) +{ + struct via_spec *spec = codec->spec; + vt1708_stop_hp_work(spec); + return 0; +} +#endif + #ifdef CONFIG_SND_HDA_POWER_SAVE static int via_check_power_status(struct hda_codec *codec, hda_nid_t nid) { @@ -1479,6 +1522,9 @@ .build_pcms = via_build_pcms, .init = via_init, .free = via_free, +#ifdef SND_HDA_NEEDS_RESUME + .suspend = via_suspend, +#endif #ifdef CONFIG_SND_HDA_POWER_SAVE .check_power_status = via_check_power_status, #endif @@ -1728,6 +1774,51 @@ return; }
+static int vt1708_jack_detectect_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct via_spec *spec = codec->spec; + + if (spec->codec_type != VT1708) + return 0; + spec->vt1708_jack_detectect = + !((snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8) & 0x1); + ucontrol->value.integer.value[0] = spec->vt1708_jack_detectect; + return 0; +} + +static int vt1708_jack_detectect_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct via_spec *spec = codec->spec; + int change; + + if (spec->codec_type != VT1708) + return 0; + spec->vt1708_jack_detectect = ucontrol->value.integer.value[0]; + change = (0x1 & (snd_hda_codec_read(codec, 0x1, 0, 0xf84, 0) >> 8)) + == !spec->vt1708_jack_detectect; + if (spec->vt1708_jack_detectect) { + mute_aa_path(codec, 1); + notify_aa_path_ctls(codec); + } + return change; +} + +static struct snd_kcontrol_new vt1708_jack_detectect[] = { + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Jack Detect", + .count = 1, + .info = snd_ctl_boolean_mono_info, + .get = vt1708_jack_detectect_get, + .put = vt1708_jack_detectect_put, + }, + {} /* end */ +}; + static int vt1708_parse_auto_config(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1755,6 +1846,10 @@ err = vt1708_auto_create_analog_input_ctls(spec, &spec->autocfg); if (err < 0) return err; + /* add jack detect on/off control */ + err = snd_hda_add_new_ctls(codec, vt1708_jack_detectect); + if (err < 0) + return err;
spec->multiout.max_channels = spec->multiout.num_dacs * 2;
@@ -1788,6 +1883,22 @@ return 0; }
+static void vt1708_update_hp_jack_state(struct work_struct *work) +{ + struct via_spec *spec = container_of(work, struct via_spec, + vt1708_hp_work.work); + if (spec->codec_type != VT1708) + return; + /* if jack state toggled */ + if (spec->vt1708_hp_present + != (snd_hda_codec_read(spec->codec, spec->autocfg.hp_pins[0], 0, + AC_VERB_GET_PIN_SENSE, 0) >> 31)) { + spec->vt1708_hp_present ^= 1; + via_hp_automute(spec->codec); + } + vt1708_start_hp_work(spec); +} + static int get_mux_nids(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -1864,7 +1975,8 @@ #ifdef CONFIG_SND_HDA_POWER_SAVE spec->loopback.amplist = vt1708_loopbacks; #endif - + spec->codec = codec; + INIT_DELAYED_WORK(&spec->vt1708_hp_work, vt1708_update_hp_jack_state); return 0; }
participants (3)
-
Harald Welte -
Li Bo -
Takashi Iwai