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