Re: ThinkPad T480s & LED_MUTE, LED_MICMUTE

[Takashi Iwai <tiwai@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On Mon, 18 Jun 2018 13:26:18 +0200,
Pali Rohár wrote:
> 
> On Monday 18 June 2018 13:21:21 Pali Rohár wrote:
> > Now I see that in Debian backports is some 4.16.12 kernel version. I can
> > try this one (and later compile one from Linus tree).
> 
> Tested, this version is working fine and leds works as expected. And now
> also snd_hda_codec_realtek is loaded.

Good to hear :)

Below are test patches to allow user choosing the mic mute LED
behavior on Thinkpad, too.  Note: totally untested!

Let me know if these work for you.


thanks,

Takashi


[-- Attachment #2: 0001-ALSA-hda-Move-mic-mute-LED-helper-to-the-generic-par.patch --]
[-- Type: application/octet-stream, Size: 10421 bytes --]

From 1cd2cac7e1fb62a733bcc3e202d5e68de8001b61 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Mon, 18 Jun 2018 17:26:12 +0200
Subject: [PATCH 1/2] ALSA: hda - Move mic mute LED helper to the generic
 parser

Move the code for setting up and controlling the mic mute LED hook
from dell-wmi helper to the generic parser, so that it can be referred
from the multiple driver codes.

No functional change.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/dell_wmi_helper.c | 115 ++------------------------
 sound/pci/hda/hda_generic.c     | 141 ++++++++++++++++++++++++++++++++
 sound/pci/hda/hda_generic.h     |  16 ++++
 3 files changed, 163 insertions(+), 109 deletions(-)

diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c
index 1b48a8c19d28..28780772008e 100644
--- a/sound/pci/hda/dell_wmi_helper.c
+++ b/sound/pci/hda/dell_wmi_helper.c
@@ -6,107 +6,15 @@
 #if IS_ENABLED(CONFIG_DELL_LAPTOP)
 #include <linux/dell-led.h>
 
-enum {
-	MICMUTE_LED_ON,
-	MICMUTE_LED_OFF,
-	MICMUTE_LED_FOLLOW_CAPTURE,
-	MICMUTE_LED_FOLLOW_MUTE,
-};
-
-static int dell_led_mode = MICMUTE_LED_FOLLOW_MUTE;
-static int dell_capture;
-static int dell_led_value;
 static int (*dell_micmute_led_set_func)(int);
-static void (*dell_old_cap_hook)(struct hda_codec *,
-			         struct snd_kcontrol *,
-				 struct snd_ctl_elem_value *);
-
-static void call_micmute_led_update(void)
-{
-	int val;
-
-	switch (dell_led_mode) {
-	case MICMUTE_LED_ON:
-		val = 1;
-		break;
-	case MICMUTE_LED_OFF:
-		val = 0;
-		break;
-	case MICMUTE_LED_FOLLOW_CAPTURE:
-		val = dell_capture;
-		break;
-	case MICMUTE_LED_FOLLOW_MUTE:
-	default:
-		val = !dell_capture;
-		break;
-	}
-
-	if (val == dell_led_value)
-		return;
-	dell_led_value = val;
-	dell_micmute_led_set_func(dell_led_value);
-}
-
-static void update_dell_wmi_micmute_led(struct hda_codec *codec,
-				        struct snd_kcontrol *kcontrol,
-					struct snd_ctl_elem_value *ucontrol)
-{
-	if (dell_old_cap_hook)
-		dell_old_cap_hook(codec, kcontrol, ucontrol);
-
-	if (!ucontrol || !dell_micmute_led_set_func)
-		return;
-	if (strcmp("Capture Switch", ucontrol->id.name) == 0 && ucontrol->id.index == 0) {
-		/* TODO: How do I verify if it's a mono or stereo here? */
-		dell_capture = (ucontrol->value.integer.value[0] ||
-				ucontrol->value.integer.value[1]);
-		call_micmute_led_update();
-	}
-}
 
-static int dell_mic_mute_led_mode_info(struct snd_kcontrol *kcontrol,
-				       struct snd_ctl_elem_info *uinfo)
+static void dell_micmute_update(struct hda_codec *codec)
 {
-	static const char * const texts[] = {
-		"On", "Off", "Follow Capture", "Follow Mute",
-	};
-
-	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
-}
+	struct hda_gen_spec *spec = codec->spec;
 
-static int dell_mic_mute_led_mode_get(struct snd_kcontrol *kcontrol,
-				      struct snd_ctl_elem_value *ucontrol)
-{
-	ucontrol->value.enumerated.item[0] = dell_led_mode;
-	return 0;
+	dell_micmute_led_set_func(spec->micmute_led.led_value);
 }
 
-static int dell_mic_mute_led_mode_put(struct snd_kcontrol *kcontrol,
-				      struct snd_ctl_elem_value *ucontrol)
-{
-	unsigned int mode;
-
-	mode = ucontrol->value.enumerated.item[0];
-	if (mode > MICMUTE_LED_FOLLOW_MUTE)
-		mode = MICMUTE_LED_FOLLOW_MUTE;
-	if (mode == dell_led_mode)
-		return 0;
-	dell_led_mode = mode;
-	call_micmute_led_update();
-	return 1;
-}
-
-static const struct snd_kcontrol_new dell_mic_mute_mode_ctls[] = {
-	{
-		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
-		.name = "Mic Mute-LED Mode",
-		.info = dell_mic_mute_led_mode_info,
-		.get = dell_mic_mute_led_mode_get,
-		.put = dell_mic_mute_led_mode_put,
-	},
-	{}
-};
-
 static void alc_fixup_dell_wmi(struct hda_codec *codec,
 			       const struct hda_fixup *fix, int action)
 {
@@ -121,25 +29,14 @@ static void alc_fixup_dell_wmi(struct hda_codec *codec,
 			return;
 		}
 
-		removefunc = true;
-		if (dell_micmute_led_set_func(false) >= 0) {
-			dell_led_value = 0;
-			if (spec->gen.num_adc_nids > 1 && !spec->gen.dyn_adc_switch)
-				codec_dbg(codec, "Skipping micmute LED control due to several ADCs");
-			else {
-				dell_old_cap_hook = spec->gen.cap_sync_hook;
-				spec->gen.cap_sync_hook = update_dell_wmi_micmute_led;
-				removefunc = false;
-				add_mixer(spec, dell_mic_mute_mode_ctls);
-			}
-		}
-
+		removefunc = (dell_micmute_led_set_func(false) < 0) ||
+			(snd_hda_gen_add_micmute_led(codec,
+						     dell_micmute_update) <= 0);
 	}
 
 	if (dell_micmute_led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
 		symbol_put(dell_micmute_led_set);
 		dell_micmute_led_set_func = NULL;
-		dell_old_cap_hook = NULL;
 	}
 }
 
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index db773e219aaa..707614e38aeb 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3899,6 +3899,147 @@ static int parse_mic_boost(struct hda_codec *codec)
 	return 0;
 }
 
+/*
+ * mic mute LED hook helpers
+ */
+enum {
+	MICMUTE_LED_ON,
+	MICMUTE_LED_OFF,
+	MICMUTE_LED_FOLLOW_CAPTURE,
+	MICMUTE_LED_FOLLOW_MUTE,
+};
+
+static void call_micmute_led_update(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+	unsigned int val;
+
+	switch (spec->micmute_led.led_mode) {
+	case MICMUTE_LED_ON:
+		val = 1;
+		break;
+	case MICMUTE_LED_OFF:
+		val = 0;
+		break;
+	case MICMUTE_LED_FOLLOW_CAPTURE:
+		val = spec->micmute_led.capture;
+		break;
+	case MICMUTE_LED_FOLLOW_MUTE:
+	default:
+		val = !spec->micmute_led.capture;
+		break;
+	}
+
+	if (val == spec->micmute_led.led_value)
+		return;
+	spec->micmute_led.led_value = val;
+	if (spec->micmute_led.update)
+		spec->micmute_led.update(codec);
+}
+
+
+static void update_micmute_led(struct hda_codec *codec,
+			       struct snd_kcontrol *kcontrol,
+			       struct snd_ctl_elem_value *ucontrol)
+{
+	struct hda_gen_spec *spec = codec->spec;
+
+	if (spec->micmute_led.old_hook)
+		spec->micmute_led.old_hook(codec, kcontrol, ucontrol);
+
+	if (!ucontrol)
+		return;
+	if (!strcmp("Capture Switch", ucontrol->id.name) &&
+	    !ucontrol->id.index) {
+		/* TODO: How do I verify if it's a mono or stereo here? */
+		spec->micmute_led.capture = (ucontrol->value.integer.value[0] ||
+					     ucontrol->value.integer.value[1]);
+		call_micmute_led_update(codec);
+	}
+}
+
+static int micmute_led_mode_info(struct snd_kcontrol *kcontrol,
+				 struct snd_ctl_elem_info *uinfo)
+{
+	static const char * const texts[] = {
+		"On", "Off", "Follow Capture", "Follow Mute",
+	};
+
+	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
+}
+
+static int micmute_led_mode_get(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
+	struct hda_gen_spec *spec = codec->spec;
+
+	ucontrol->value.enumerated.item[0] = spec->micmute_led.led_mode;
+	return 0;
+}
+
+static int micmute_led_mode_put(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
+	struct hda_gen_spec *spec = codec->spec;
+	unsigned int mode;
+
+	mode = ucontrol->value.enumerated.item[0];
+	if (mode > MICMUTE_LED_FOLLOW_MUTE)
+		mode = MICMUTE_LED_FOLLOW_MUTE;
+	if (mode == spec->micmute_led.led_mode)
+		return 0;
+	spec->micmute_led.led_mode = mode;
+	call_micmute_led_update(codec);
+	return 1;
+}
+
+static const struct snd_kcontrol_new micmute_led_mode_ctl = {
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+	.name = "Mic Mute-LED Mode",
+	.info = micmute_led_mode_info,
+	.get = micmute_led_mode_get,
+	.put = micmute_led_mode_put,
+};
+
+/**
+ * snd_hda_gen_add_micmute_led - helper for setting up mic mute LED hook
+ * @codec: the HDA codec
+ * @hook: the callback for updating LED
+ *
+ * Called from the codec drivers for offering the mic mute LED controls.
+ * Only valid for a single ADC (or a single input).  When established, it
+ * sets up cap_sync_hook and triggers the callback at each time when the
+ * capture mixer switch changes.  The callback is supposed to update the LED
+ * accordingly.
+ *
+ * Returns 1 if the hook is established, 0 if skipped (no valid config), or
+ * a negative error code.
+ */
+int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
+				void (*hook)(struct hda_codec *))
+{
+	struct hda_gen_spec *spec = codec->spec;
+
+	if (spec->num_adc_nids > 1 && !spec->dyn_adc_switch) {
+		codec_dbg(codec,
+			  "Skipping micmute LED control due to several ADCs");
+		return 0;
+	}
+
+	spec->micmute_led.led_mode = MICMUTE_LED_FOLLOW_MUTE;
+	spec->micmute_led.capture = 0;
+	spec->micmute_led.led_value = 0;
+	spec->micmute_led.old_hook = spec->cap_sync_hook;
+	spec->micmute_led.update = hook;
+	spec->cap_sync_hook = update_micmute_led;
+	if (!snd_hda_gen_add_kctl(spec, NULL, &micmute_led_mode_ctl))
+		return -ENOMEM;
+	return 1;
+}
+EXPORT_SYMBOL_GPL(snd_hda_gen_add_micmute_led);
+
 /*
  * parse digital I/Os and set up NIDs in BIOS auto-parse mode
  */
diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
index 61772317de46..10123664fa61 100644
--- a/sound/pci/hda/hda_generic.h
+++ b/sound/pci/hda/hda_generic.h
@@ -86,6 +86,16 @@ struct badness_table {
 extern const struct badness_table hda_main_out_badness;
 extern const struct badness_table hda_extra_out_badness;
 
+struct hda_micmute_hook {
+	unsigned int led_mode;
+	unsigned int capture;
+	unsigned int led_value;
+	void (*update)(struct hda_codec *codec);
+	void (*old_hook)(struct hda_codec *codec,
+			 struct snd_kcontrol *kcontrol,
+			 struct snd_ctl_elem_value *ucontrol);
+};
+
 struct hda_gen_spec {
 	char stream_name_analog[32];	/* analog PCM stream */
 	const struct hda_pcm_stream *stream_analog_playback;
@@ -276,6 +286,9 @@ struct hda_gen_spec {
 			      struct snd_kcontrol *kcontrol,
 			      struct snd_ctl_elem_value *ucontrol);
 
+	/* mic mute LED hook; called via cap_sync_hook */
+	struct hda_micmute_hook micmute_led;
+
 	/* PCM hooks */
 	void (*pcm_playback_hook)(struct hda_pcm_stream *hinfo,
 				  struct hda_codec *codec,
@@ -342,4 +355,7 @@ unsigned int snd_hda_gen_path_power_filter(struct hda_codec *codec,
 void snd_hda_gen_stream_pm(struct hda_codec *codec, hda_nid_t nid, bool on);
 int snd_hda_gen_fix_pin_power(struct hda_codec *codec, hda_nid_t pin);
 
+int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
+				void (*hook)(struct hda_codec *));
+
 #endif /* __SOUND_HDA_GENERIC_H */
-- 
2.17.1


[-- Attachment #3: 0002-ALSA-hda-Use-the-common-helper-for-thinkpad_acpi-mic.patch --]
[-- Type: application/octet-stream, Size: 2513 bytes --]

From 2730426826f8dbc702efb0f4723abe7795d8e4a2 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Mon, 18 Jun 2018 17:28:45 +0200
Subject: [PATCH 2/2] ALSA: hda - Use the common helper for thinkpad_acpi mic
 mute LED handling

Use the new common helper for setting up and controlling the mic mute
LED over thinkpad_acpi.  This also provides a new mixer enum "Mic
Mute-LED Mode" (that was present only for Dell models), which allows
user to choose the mic mute LED behavior.  For example, if you want
the mic mute LED turned on only while mic is on, choose "Follow
Capture" there.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/thinkpad_helper.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index 65bb3ac6af4c..97f49b751e6e 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -27,17 +27,11 @@ static void update_tpacpi_mute_led(void *private_data, int enabled)
 		led_set_func(TPACPI_LED_MUTE, !enabled);
 }
 
-static void update_tpacpi_micmute_led(struct hda_codec *codec,
-				      struct snd_kcontrol *kcontrol,
-				      struct snd_ctl_elem_value *ucontrol)
+static void update_tpacpi_micmute(struct hda_codec *codec)
 {
-	if (!ucontrol || !led_set_func)
-		return;
-	if (strcmp("Capture Switch", ucontrol->id.name) == 0 && ucontrol->id.index == 0) {
-		/* TODO: How do I verify if it's a mono or stereo here? */
-		bool val = ucontrol->value.integer.value[0] || ucontrol->value.integer.value[1];
-		led_set_func(TPACPI_LED_MICMUTE, !val);
-	}
+	struct hda_gen_spec *spec = codec->spec;
+
+	led_set_func(TPACPI_LED_MICMUTE, spec->micmute_led.led_value);
 }
 
 static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
@@ -63,15 +57,10 @@ static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
 			spec->vmaster_mute.hook = update_tpacpi_mute_led;
 			removefunc = false;
 		}
-		if (led_set_func(TPACPI_LED_MICMUTE, false) >= 0) {
-			if (spec->num_adc_nids > 1 && !spec->dyn_adc_switch)
-				codec_dbg(codec,
-					  "Skipping micmute LED control due to several ADCs");
-			else {
-				spec->cap_sync_hook = update_tpacpi_micmute_led;
-				removefunc = false;
-			}
-		}
+		if (led_set_func(TPACPI_LED_MICMUTE, false) >= 0 &&
+		    snd_hda_gen_add_micmute_led(codec,
+						update_tpacpi_micmute) > 0)
+			removefunc = false;
 	}
 
 	if (led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
-- 
2.17.1