[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Make the thinkpad_helper able to support not only led control over
acpi with thinkpad_acpi driver but also led control over hid-lenovo.
The hid-lenovo driver adapted the led control api of thinkpad_acpi.

Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
---
 sound/pci/hda/thinkpad_helper.c | 149 ++++++++++++++++++++++++++++++----------
 1 file changed, 111 insertions(+), 38 deletions(-)

diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index 62741a7..c24a4a9 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -2,79 +2,152 @@
  * to be included from codec driver
  */
 
-#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
-
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI) || IS_ENABLED(CONFIG_HID_LENOVO)
 #include <linux/acpi.h>
+#include <linux/hid-lenovo.h>
 #include <linux/thinkpad_acpi.h>
 
-static int (*led_set_func)(int, bool);
+static int (*led_set_func_tpacpi)(int, bool);
+static int (*led_set_func_hid_lenovo)(int, bool);
 static void (*old_vmaster_hook)(void *, int);
 
 static bool is_thinkpad(struct hda_codec *codec)
 {
+	return (codec->core.subsystem_id >> 16 == 0x17aa);
+}
+
+static bool is_thinkpad_acpi(struct hda_codec *codec)
+{
 	return (codec->core.subsystem_id >> 16 == 0x17aa) &&
 	       (acpi_dev_found("LEN0068") || acpi_dev_found("IBM0068"));
 }
 
-static void update_tpacpi_mute_led(void *private_data, int enabled)
+static void update_thinkpad_mute_led(void *private_data, int enabled)
 {
 	if (old_vmaster_hook)
 		old_vmaster_hook(private_data, enabled);
 
-	if (led_set_func)
-		led_set_func(TPACPI_LED_MUTE, !enabled);
+	if (led_set_func_tpacpi)
+		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
+
+	if (led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
 }
 
-static void update_tpacpi_micmute_led(struct hda_codec *codec,
+
+
+static void update_thinkpad_micmute_led(struct hda_codec *codec,
 				      struct snd_kcontrol *kcontrol,
 				      struct snd_ctl_elem_value *ucontrol)
 {
-	if (!ucontrol || !led_set_func)
+	if (!ucontrol)
 		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);
+		if (led_set_func_tpacpi)
+			led_set_func_tpacpi(TPACPI_LED_MICMUTE, !val);
+		if (led_set_func_hid_lenovo)
+			led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, !val);
 	}
 }
 
-static void hda_fixup_thinkpad(struct hda_codec *codec,
-				    const struct hda_fixup *fix, int action)
+static int hda_fixup_thinkpad_acpi(struct hda_codec *codec)
 {
 	struct hda_gen_spec *spec = codec->spec;
-	bool removefunc = false;
+	int ret = -ENXIO;
 
-	if (action == HDA_FIXUP_ACT_PROBE) {
-		if (!is_thinkpad(codec))
-			return;
-		if (!led_set_func)
-			led_set_func = symbol_request(tpacpi_led_set);
-		if (!led_set_func) {
-			codec_warn(codec,
-				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
-			return;
-		}
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!is_thinkpad_acpi(codec))
+		return -ENODEV;
+	if (!led_set_func_tpacpi)
+		led_set_func_tpacpi = symbol_request(tpacpi_led_set);
+	if (!led_set_func_tpacpi) {
+		codec_warn(codec,
+			   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
+		return -ENOENT;
+	}
 
-		removefunc = true;
-		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
-			old_vmaster_hook = spec->vmaster_mute.hook;
-			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)
-				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(TPACPI_LED_MUTE, false) >= 0) {
+		old_vmaster_hook = spec->vmaster_mute.hook;
+		spec->vmaster_mute.hook = update_thinkpad_mute_led;
+		ret = 0;
+	}
+
+	if (led_set_func_tpacpi(TPACPI_LED_MICMUTE, false) >= 0) {
+		if (spec->num_adc_nids > 1)
+			codec_dbg(codec,
+				  "Skipping micmute LED control due to several ADCs");
+		else {
+			spec->cap_sync_hook = update_thinkpad_micmute_led;
+			ret = 0;
 		}
 	}
 
-	if (led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
+	return ret;
+}
+
+static int hda_fixup_thinkpad_hid(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+	int ret = 0;
+
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
+	if (!led_set_func_hid_lenovo) {
+		codec_warn(codec,
+			   "Failed to find hid-lenovo symbol hid_lenovo_led_set\n");
+		return -ENOENT;
+	}
+
+	if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
+		old_vmaster_hook = spec->vmaster_mute.hook;
+
+	// do not remove hook if setting delay does not work currently because
+	// it is a usb hid devices which is not connected right now
+	// maybe is will be connected later
+	led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, false);
+	spec->vmaster_mute.hook = update_thinkpad_mute_led;
+
+	led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, false);
+	spec->cap_sync_hook = update_thinkpad_micmute_led;
+
+	return ret;
+}
+
+static void hda_fixup_thinkpad(struct hda_codec *codec,
+				    const struct hda_fixup *fix, int action)
+{
+	int ret_fixup_acpi = 0;
+	int ret_fixup_hid = 0;
+	bool remove = 0;
+
+	if (action == HDA_FIXUP_ACT_PROBE) {
+		ret_fixup_acpi = hda_fixup_thinkpad_acpi(codec);
+		ret_fixup_hid = hda_fixup_thinkpad_hid(codec);
+	}
+
+	if (led_set_func_tpacpi &&
+		(action == HDA_FIXUP_ACT_FREE || ret_fixup_acpi)) {
+
 		symbol_put(tpacpi_led_set);
-		led_set_func = NULL;
+		remove = true;
+	}
+
+	if (led_set_func_hid_lenovo &&
+		(action == HDA_FIXUP_ACT_FREE || ret_fixup_hid)) {
+
+		symbol_put(hid_lenovo_led_set);
+		remove = true;
+	}
+
+
+	if (remove) {
+		led_set_func_tpacpi = NULL;
+		led_set_func_hid_lenovo = NULL;
 		old_vmaster_hook = NULL;
 	}
 }
-- 
1.9.1

[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Make the thinkpad_helper able to support not only led control over
acpi with thinkpad_acpi driver but also led control over hid-lenovo.
The hid-lenovo driver adapted the led control api of thinkpad_acpi.

Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
---
 sound/pci/hda/thinkpad_helper.c | 149 ++++++++++++++++++++++++++++++----------
 1 file changed, 111 insertions(+), 38 deletions(-)

diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index 62741a7..c24a4a9 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -2,79 +2,152 @@
  * to be included from codec driver
  */
 
-#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
-
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI) || IS_ENABLED(CONFIG_HID_LENOVO)
 #include <linux/acpi.h>
+#include <linux/hid-lenovo.h>
 #include <linux/thinkpad_acpi.h>
 
-static int (*led_set_func)(int, bool);
+static int (*led_set_func_tpacpi)(int, bool);
+static int (*led_set_func_hid_lenovo)(int, bool);
 static void (*old_vmaster_hook)(void *, int);
 
 static bool is_thinkpad(struct hda_codec *codec)
 {
+	return (codec->core.subsystem_id >> 16 = 0x17aa);
+}
+
+static bool is_thinkpad_acpi(struct hda_codec *codec)
+{
 	return (codec->core.subsystem_id >> 16 = 0x17aa) &&
 	       (acpi_dev_found("LEN0068") || acpi_dev_found("IBM0068"));
 }
 
-static void update_tpacpi_mute_led(void *private_data, int enabled)
+static void update_thinkpad_mute_led(void *private_data, int enabled)
 {
 	if (old_vmaster_hook)
 		old_vmaster_hook(private_data, enabled);
 
-	if (led_set_func)
-		led_set_func(TPACPI_LED_MUTE, !enabled);
+	if (led_set_func_tpacpi)
+		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
+
+	if (led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
 }
 
-static void update_tpacpi_micmute_led(struct hda_codec *codec,
+
+
+static void update_thinkpad_micmute_led(struct hda_codec *codec,
 				      struct snd_kcontrol *kcontrol,
 				      struct snd_ctl_elem_value *ucontrol)
 {
-	if (!ucontrol || !led_set_func)
+	if (!ucontrol)
 		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);
+		if (led_set_func_tpacpi)
+			led_set_func_tpacpi(TPACPI_LED_MICMUTE, !val);
+		if (led_set_func_hid_lenovo)
+			led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, !val);
 	}
 }
 
-static void hda_fixup_thinkpad(struct hda_codec *codec,
-				    const struct hda_fixup *fix, int action)
+static int hda_fixup_thinkpad_acpi(struct hda_codec *codec)
 {
 	struct hda_gen_spec *spec = codec->spec;
-	bool removefunc = false;
+	int ret = -ENXIO;
 
-	if (action = HDA_FIXUP_ACT_PROBE) {
-		if (!is_thinkpad(codec))
-			return;
-		if (!led_set_func)
-			led_set_func = symbol_request(tpacpi_led_set);
-		if (!led_set_func) {
-			codec_warn(codec,
-				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
-			return;
-		}
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!is_thinkpad_acpi(codec))
+		return -ENODEV;
+	if (!led_set_func_tpacpi)
+		led_set_func_tpacpi = symbol_request(tpacpi_led_set);
+	if (!led_set_func_tpacpi) {
+		codec_warn(codec,
+			   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
+		return -ENOENT;
+	}
 
-		removefunc = true;
-		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
-			old_vmaster_hook = spec->vmaster_mute.hook;
-			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)
-				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(TPACPI_LED_MUTE, false) >= 0) {
+		old_vmaster_hook = spec->vmaster_mute.hook;
+		spec->vmaster_mute.hook = update_thinkpad_mute_led;
+		ret = 0;
+	}
+
+	if (led_set_func_tpacpi(TPACPI_LED_MICMUTE, false) >= 0) {
+		if (spec->num_adc_nids > 1)
+			codec_dbg(codec,
+				  "Skipping micmute LED control due to several ADCs");
+		else {
+			spec->cap_sync_hook = update_thinkpad_micmute_led;
+			ret = 0;
 		}
 	}
 
-	if (led_set_func && (action = HDA_FIXUP_ACT_FREE || removefunc)) {
+	return ret;
+}
+
+static int hda_fixup_thinkpad_hid(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+	int ret = 0;
+
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
+	if (!led_set_func_hid_lenovo) {
+		codec_warn(codec,
+			   "Failed to find hid-lenovo symbol hid_lenovo_led_set\n");
+		return -ENOENT;
+	}
+
+	if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
+		old_vmaster_hook = spec->vmaster_mute.hook;
+
+	// do not remove hook if setting delay does not work currently because
+	// it is a usb hid devices which is not connected right now
+	// maybe is will be connected later
+	led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, false);
+	spec->vmaster_mute.hook = update_thinkpad_mute_led;
+
+	led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, false);
+	spec->cap_sync_hook = update_thinkpad_micmute_led;
+
+	return ret;
+}
+
+static void hda_fixup_thinkpad(struct hda_codec *codec,
+				    const struct hda_fixup *fix, int action)
+{
+	int ret_fixup_acpi = 0;
+	int ret_fixup_hid = 0;
+	bool remove = 0;
+
+	if (action = HDA_FIXUP_ACT_PROBE) {
+		ret_fixup_acpi = hda_fixup_thinkpad_acpi(codec);
+		ret_fixup_hid = hda_fixup_thinkpad_hid(codec);
+	}
+
+	if (led_set_func_tpacpi &&
+		(action = HDA_FIXUP_ACT_FREE || ret_fixup_acpi)) {
+
 		symbol_put(tpacpi_led_set);
-		led_set_func = NULL;
+		remove = true;
+	}
+
+	if (led_set_func_hid_lenovo &&
+		(action = HDA_FIXUP_ACT_FREE || ret_fixup_hid)) {
+
+		symbol_put(hid_lenovo_led_set);
+		remove = true;
+	}
+
+
+	if (remove) {
+		led_set_func_tpacpi = NULL;
+		led_set_func_hid_lenovo = NULL;
 		old_vmaster_hook = NULL;
 	}
 }
-- 
1.9.1

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Takashi Iwai]

On Mon, 12 Sep 2016 12:47:03 +0200,
Dennis Wassenberg wrote:
> 
> Make the thinkpad_helper able to support not only led control over
> acpi with thinkpad_acpi driver but also led control over hid-lenovo.
> The hid-lenovo driver adapted the led control api of thinkpad_acpi.
> 
> Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
> ---
>  sound/pci/hda/thinkpad_helper.c | 149 ++++++++++++++++++++++++++++++----------
>  1 file changed, 111 insertions(+), 38 deletions(-)
> 
> diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
> index 62741a7..c24a4a9 100644
> --- a/sound/pci/hda/thinkpad_helper.c
> +++ b/sound/pci/hda/thinkpad_helper.c
> @@ -2,79 +2,152 @@
>   * to be included from codec driver
>   */
>  
> -#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> -
> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI) || IS_ENABLED(CONFIG_HID_LENOVO)
>  #include <linux/acpi.h>
> +#include <linux/hid-lenovo.h>
>  #include <linux/thinkpad_acpi.h>
>  
> -static int (*led_set_func)(int, bool);
> +static int (*led_set_func_tpacpi)(int, bool);
> +static int (*led_set_func_hid_lenovo)(int, bool);
>  static void (*old_vmaster_hook)(void *, int);
>  
>  static bool is_thinkpad(struct hda_codec *codec)
>  {
> +	return (codec->core.subsystem_id >> 16 == 0x17aa);
> +}
> +
> +static bool is_thinkpad_acpi(struct hda_codec *codec)
> +{
>  	return (codec->core.subsystem_id >> 16 == 0x17aa) &&
>  	       (acpi_dev_found("LEN0068") || acpi_dev_found("IBM0068"));
>  }
>  
> -static void update_tpacpi_mute_led(void *private_data, int enabled)
> +static void update_thinkpad_mute_led(void *private_data, int enabled)
>  {
>  	if (old_vmaster_hook)
>  		old_vmaster_hook(private_data, enabled);
>  
> -	if (led_set_func)
> -		led_set_func(TPACPI_LED_MUTE, !enabled);
> +	if (led_set_func_tpacpi)
> +		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
> +
> +	if (led_set_func_hid_lenovo)
> +		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
>  }
>  
> -static void update_tpacpi_micmute_led(struct hda_codec *codec,
> +
> +
> +static void update_thinkpad_micmute_led(struct hda_codec *codec,
>  				      struct snd_kcontrol *kcontrol,
>  				      struct snd_ctl_elem_value *ucontrol)
>  {
> -	if (!ucontrol || !led_set_func)
> +	if (!ucontrol)
>  		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);
> +		if (led_set_func_tpacpi)
> +			led_set_func_tpacpi(TPACPI_LED_MICMUTE, !val);
> +		if (led_set_func_hid_lenovo)
> +			led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, !val);
>  	}
>  }
>  
> -static void hda_fixup_thinkpad(struct hda_codec *codec,
> -				    const struct hda_fixup *fix, int action)
> +static int hda_fixup_thinkpad_acpi(struct hda_codec *codec)
>  {
>  	struct hda_gen_spec *spec = codec->spec;
> -	bool removefunc = false;
> +	int ret = -ENXIO;
>  
> -	if (action == HDA_FIXUP_ACT_PROBE) {
> -		if (!is_thinkpad(codec))
> -			return;
> -		if (!led_set_func)
> -			led_set_func = symbol_request(tpacpi_led_set);
> -		if (!led_set_func) {
> -			codec_warn(codec,
> -				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
> -			return;
> -		}
> +	if (!is_thinkpad(codec))
> +		return -ENODEV;
> +	if (!is_thinkpad_acpi(codec))
> +		return -ENODEV;
> +	if (!led_set_func_tpacpi)
> +		led_set_func_tpacpi = symbol_request(tpacpi_led_set);

This would be performed even if CONFIG_THINKPAD_ACPI=n when
CONFIG_HID_LENOVO!=n.  You'd need to have a proper ifdef surrounding
the function.


> +	if (!led_set_func_tpacpi) {
> +		codec_warn(codec,
> +			   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
> +		return -ENOENT;
> +	}
>  
> -		removefunc = true;
> -		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
> -			old_vmaster_hook = spec->vmaster_mute.hook;
> -			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)
> -				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(TPACPI_LED_MUTE, false) >= 0) {
> +		old_vmaster_hook = spec->vmaster_mute.hook;
> +		spec->vmaster_mute.hook = update_thinkpad_mute_led;
> +		ret = 0;
> +	}
> +
> +	if (led_set_func_tpacpi(TPACPI_LED_MICMUTE, false) >= 0) {
> +		if (spec->num_adc_nids > 1)
> +			codec_dbg(codec,
> +				  "Skipping micmute LED control due to several ADCs");
> +		else {
> +			spec->cap_sync_hook = update_thinkpad_micmute_led;
> +			ret = 0;
>  		}
>  	}
>  
> -	if (led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
> +	return ret;
> +}
> +
> +static int hda_fixup_thinkpad_hid(struct hda_codec *codec)
> +{
> +	struct hda_gen_spec *spec = codec->spec;
> +	int ret = 0;
> +
> +	if (!is_thinkpad(codec))
> +		return -ENODEV;
> +	if (!led_set_func_hid_lenovo)
> +		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
> +	if (!led_set_func_hid_lenovo) {
> +		codec_warn(codec,
> +			   "Failed to find hid-lenovo symbol hid_lenovo_led_set\n");
> +		return -ENOENT;
> +	}
> +
> +	if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
> +		old_vmaster_hook = spec->vmaster_mute.hook;
> +
> +	// do not remove hook if setting delay does not work currently because
> +	// it is a usb hid devices which is not connected right now
> +	// maybe is will be connected later

The comment should be C style.  Try checkpatch.pl once before the
patch submission.


> +	led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, false);
> +	spec->vmaster_mute.hook = update_thinkpad_mute_led;
> +
> +	led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, false);
> +	spec->cap_sync_hook = update_thinkpad_micmute_led;
> +
> +	return ret;
> +}
> +
> +static void hda_fixup_thinkpad(struct hda_codec *codec,
> +				    const struct hda_fixup *fix, int action)
> +{
> +	int ret_fixup_acpi = 0;
> +	int ret_fixup_hid = 0;
> +	bool remove = 0;
> +
> +	if (action == HDA_FIXUP_ACT_PROBE) {
> +		ret_fixup_acpi = hda_fixup_thinkpad_acpi(codec);
> +		ret_fixup_hid = hda_fixup_thinkpad_hid(codec);

Basically these two are exclusive.  So you don't have to process the
latter when the former succeeds.  But...

> +	}
> +
> +	if (led_set_func_tpacpi &&
> +		(action == HDA_FIXUP_ACT_FREE || ret_fixup_acpi)) {
> +
>  		symbol_put(tpacpi_led_set);
> -		led_set_func = NULL;
> +		remove = true;
> +	}
> +
> +	if (led_set_func_hid_lenovo &&
> +		(action == HDA_FIXUP_ACT_FREE || ret_fixup_hid)) {
> +
> +		symbol_put(hid_lenovo_led_set);
> +		remove = true;
> +	}
> +
> +
> +	if (remove) {
> +		led_set_func_tpacpi = NULL;
> +		led_set_func_hid_lenovo = NULL;
>  		old_vmaster_hook = NULL;
>  	}
>  }

... reading through the patch, I find the code is a bit too
redundant.  The access is exclusive between ACPI and HID, and the hook
functions are in the same form but just the argument value is
different (TPACPI_LED_MUTE vs HID_LENOVO_LED_MUTE).  So we may need
only a single function pointer and pass the different value depending
on the model instead of keeping two individual function pointers.


thanks,

Takashi

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Takashi Iwai]

On Mon, 12 Sep 2016 12:47:03 +0200,
Dennis Wassenberg wrote:
> 
> Make the thinkpad_helper able to support not only led control over
> acpi with thinkpad_acpi driver but also led control over hid-lenovo.
> The hid-lenovo driver adapted the led control api of thinkpad_acpi.
> 
> Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
> ---
>  sound/pci/hda/thinkpad_helper.c | 149 ++++++++++++++++++++++++++++++----------
>  1 file changed, 111 insertions(+), 38 deletions(-)
> 
> diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
> index 62741a7..c24a4a9 100644
> --- a/sound/pci/hda/thinkpad_helper.c
> +++ b/sound/pci/hda/thinkpad_helper.c
> @@ -2,79 +2,152 @@
>   * to be included from codec driver
>   */
>  
> -#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> -
> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI) || IS_ENABLED(CONFIG_HID_LENOVO)
>  #include <linux/acpi.h>
> +#include <linux/hid-lenovo.h>
>  #include <linux/thinkpad_acpi.h>
>  
> -static int (*led_set_func)(int, bool);
> +static int (*led_set_func_tpacpi)(int, bool);
> +static int (*led_set_func_hid_lenovo)(int, bool);
>  static void (*old_vmaster_hook)(void *, int);
>  
>  static bool is_thinkpad(struct hda_codec *codec)
>  {
> +	return (codec->core.subsystem_id >> 16 = 0x17aa);
> +}
> +
> +static bool is_thinkpad_acpi(struct hda_codec *codec)
> +{
>  	return (codec->core.subsystem_id >> 16 = 0x17aa) &&
>  	       (acpi_dev_found("LEN0068") || acpi_dev_found("IBM0068"));
>  }
>  
> -static void update_tpacpi_mute_led(void *private_data, int enabled)
> +static void update_thinkpad_mute_led(void *private_data, int enabled)
>  {
>  	if (old_vmaster_hook)
>  		old_vmaster_hook(private_data, enabled);
>  
> -	if (led_set_func)
> -		led_set_func(TPACPI_LED_MUTE, !enabled);
> +	if (led_set_func_tpacpi)
> +		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
> +
> +	if (led_set_func_hid_lenovo)
> +		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
>  }
>  
> -static void update_tpacpi_micmute_led(struct hda_codec *codec,
> +
> +
> +static void update_thinkpad_micmute_led(struct hda_codec *codec,
>  				      struct snd_kcontrol *kcontrol,
>  				      struct snd_ctl_elem_value *ucontrol)
>  {
> -	if (!ucontrol || !led_set_func)
> +	if (!ucontrol)
>  		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);
> +		if (led_set_func_tpacpi)
> +			led_set_func_tpacpi(TPACPI_LED_MICMUTE, !val);
> +		if (led_set_func_hid_lenovo)
> +			led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, !val);
>  	}
>  }
>  
> -static void hda_fixup_thinkpad(struct hda_codec *codec,
> -				    const struct hda_fixup *fix, int action)
> +static int hda_fixup_thinkpad_acpi(struct hda_codec *codec)
>  {
>  	struct hda_gen_spec *spec = codec->spec;
> -	bool removefunc = false;
> +	int ret = -ENXIO;
>  
> -	if (action = HDA_FIXUP_ACT_PROBE) {
> -		if (!is_thinkpad(codec))
> -			return;
> -		if (!led_set_func)
> -			led_set_func = symbol_request(tpacpi_led_set);
> -		if (!led_set_func) {
> -			codec_warn(codec,
> -				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
> -			return;
> -		}
> +	if (!is_thinkpad(codec))
> +		return -ENODEV;
> +	if (!is_thinkpad_acpi(codec))
> +		return -ENODEV;
> +	if (!led_set_func_tpacpi)
> +		led_set_func_tpacpi = symbol_request(tpacpi_led_set);

This would be performed even if CONFIG_THINKPAD_ACPI=n when
CONFIG_HID_LENOVO!=n.  You'd need to have a proper ifdef surrounding
the function.


> +	if (!led_set_func_tpacpi) {
> +		codec_warn(codec,
> +			   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
> +		return -ENOENT;
> +	}
>  
> -		removefunc = true;
> -		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
> -			old_vmaster_hook = spec->vmaster_mute.hook;
> -			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)
> -				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(TPACPI_LED_MUTE, false) >= 0) {
> +		old_vmaster_hook = spec->vmaster_mute.hook;
> +		spec->vmaster_mute.hook = update_thinkpad_mute_led;
> +		ret = 0;
> +	}
> +
> +	if (led_set_func_tpacpi(TPACPI_LED_MICMUTE, false) >= 0) {
> +		if (spec->num_adc_nids > 1)
> +			codec_dbg(codec,
> +				  "Skipping micmute LED control due to several ADCs");
> +		else {
> +			spec->cap_sync_hook = update_thinkpad_micmute_led;
> +			ret = 0;
>  		}
>  	}
>  
> -	if (led_set_func && (action = HDA_FIXUP_ACT_FREE || removefunc)) {
> +	return ret;
> +}
> +
> +static int hda_fixup_thinkpad_hid(struct hda_codec *codec)
> +{
> +	struct hda_gen_spec *spec = codec->spec;
> +	int ret = 0;
> +
> +	if (!is_thinkpad(codec))
> +		return -ENODEV;
> +	if (!led_set_func_hid_lenovo)
> +		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
> +	if (!led_set_func_hid_lenovo) {
> +		codec_warn(codec,
> +			   "Failed to find hid-lenovo symbol hid_lenovo_led_set\n");
> +		return -ENOENT;
> +	}
> +
> +	if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
> +		old_vmaster_hook = spec->vmaster_mute.hook;
> +
> +	// do not remove hook if setting delay does not work currently because
> +	// it is a usb hid devices which is not connected right now
> +	// maybe is will be connected later

The comment should be C style.  Try checkpatch.pl once before the
patch submission.


> +	led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, false);
> +	spec->vmaster_mute.hook = update_thinkpad_mute_led;
> +
> +	led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, false);
> +	spec->cap_sync_hook = update_thinkpad_micmute_led;
> +
> +	return ret;
> +}
> +
> +static void hda_fixup_thinkpad(struct hda_codec *codec,
> +				    const struct hda_fixup *fix, int action)
> +{
> +	int ret_fixup_acpi = 0;
> +	int ret_fixup_hid = 0;
> +	bool remove = 0;
> +
> +	if (action = HDA_FIXUP_ACT_PROBE) {
> +		ret_fixup_acpi = hda_fixup_thinkpad_acpi(codec);
> +		ret_fixup_hid = hda_fixup_thinkpad_hid(codec);

Basically these two are exclusive.  So you don't have to process the
latter when the former succeeds.  But...

> +	}
> +
> +	if (led_set_func_tpacpi &&
> +		(action = HDA_FIXUP_ACT_FREE || ret_fixup_acpi)) {
> +
>  		symbol_put(tpacpi_led_set);
> -		led_set_func = NULL;
> +		remove = true;
> +	}
> +
> +	if (led_set_func_hid_lenovo &&
> +		(action = HDA_FIXUP_ACT_FREE || ret_fixup_hid)) {
> +
> +		symbol_put(hid_lenovo_led_set);
> +		remove = true;
> +	}
> +
> +
> +	if (remove) {
> +		led_set_func_tpacpi = NULL;
> +		led_set_func_hid_lenovo = NULL;
>  		old_vmaster_hook = NULL;
>  	}
>  }

... reading through the patch, I find the code is a bit too
redundant.  The access is exclusive between ACPI and HID, and the hook
functions are in the same form but just the argument value is
different (TPACPI_LED_MUTE vs HID_LENOVO_LED_MUTE).  So we may need
only a single function pointer and pass the different value depending
on the model instead of keeping two individual function pointers.


thanks,

Takashi

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Hi Takashi,

at my first shot published to github I had an approach which makes thinkpad_acpi and hid-lenovo mutual exclusive (https://github.com/dwassenberg/linux/tree/development/hid-rmi). This approach has some issues. For example in case of thinkpad_acpi and hid-lenovo module is loaded (e.g. at Lenovo ThinkPad X1 Tablet) the symbol request to tpacpi_led_set is successful so the thinkpad_helper will decide in favor of thinkpad_acpi. So I need to add something which checks the return code of tpacpi_led_set. If -ENODEV is returned it can try hid-lenovo.

But the reason why I made thinkpad_acpi and hid-lenovo work in parallel was that if it is mutual exclusive there is no possibility to control LEDs of external Lenovo USB keyboards which are connected to a Thinkpad. Currently (including the other patches) there is only the X1 Tablet Cover LEDs which is controllable from external code. But the hid-lenovo driver is able to handle different Lenovo USB Keyboards.

- ThinkPad USB Keyboard with TrackPoint (tpkbd)
- ThinkPad Compact Bluetooth Keyboard with TrackPoint (cptkbd)
- ThinkPad Compact USB Keyboard with TrackPoint (cptkbd)
- ThinkPad X1 Cover USB Keyboard with TrackPoint and Touchpad (tpx1cover)

These keyboards have these LEDs too. For some of these keyboards a LED control is implemented inside hid-lenovo too. If thinkpad_acpi and hid-lenovo is mutual exclusive it is impossible to let the thinkpad_helper control the LEDs of these external USB keyboards if they are connected to a thinkpad. I don't know if anyone will do this, but I don't want to take the possibility to do such thinks :)

So.. if there is the decision (from your side) that this should be mutual exclusive I will prepare a v2 which the mutual exclusive approach I described at the top.

Thank you & best regards,

Dennis

On 12.09.2016 14:38, Takashi Iwai wrote:
> On Mon, 12 Sep 2016 12:47:03 +0200,
> Dennis Wassenberg wrote:
>>
>> Make the thinkpad_helper able to support not only led control over
>> acpi with thinkpad_acpi driver but also led control over hid-lenovo.
>> The hid-lenovo driver adapted the led control api of thinkpad_acpi.
>>
>> Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
>> ---
>>  sound/pci/hda/thinkpad_helper.c | 149 ++++++++++++++++++++++++++++++----------
>>  1 file changed, 111 insertions(+), 38 deletions(-)
>>
>> diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
>> index 62741a7..c24a4a9 100644
>> --- a/sound/pci/hda/thinkpad_helper.c
>> +++ b/sound/pci/hda/thinkpad_helper.c
>> @@ -2,79 +2,152 @@
>>   * to be included from codec driver
>>   */
>>  
>> -#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
>> -
>> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI) || IS_ENABLED(CONFIG_HID_LENOVO)
>>  #include <linux/acpi.h>
>> +#include <linux/hid-lenovo.h>
>>  #include <linux/thinkpad_acpi.h>
>>  
>> -static int (*led_set_func)(int, bool);
>> +static int (*led_set_func_tpacpi)(int, bool);
>> +static int (*led_set_func_hid_lenovo)(int, bool);
>>  static void (*old_vmaster_hook)(void *, int);
>>  
>>  static bool is_thinkpad(struct hda_codec *codec)
>>  {
>> +	return (codec->core.subsystem_id >> 16 == 0x17aa);
>> +}
>> +
>> +static bool is_thinkpad_acpi(struct hda_codec *codec)
>> +{
>>  	return (codec->core.subsystem_id >> 16 == 0x17aa) &&
>>  	       (acpi_dev_found("LEN0068") || acpi_dev_found("IBM0068"));
>>  }
>>  
>> -static void update_tpacpi_mute_led(void *private_data, int enabled)
>> +static void update_thinkpad_mute_led(void *private_data, int enabled)
>>  {
>>  	if (old_vmaster_hook)
>>  		old_vmaster_hook(private_data, enabled);
>>  
>> -	if (led_set_func)
>> -		led_set_func(TPACPI_LED_MUTE, !enabled);
>> +	if (led_set_func_tpacpi)
>> +		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
>> +
>> +	if (led_set_func_hid_lenovo)
>> +		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
>>  }
>>  
>> -static void update_tpacpi_micmute_led(struct hda_codec *codec,
>> +
>> +
>> +static void update_thinkpad_micmute_led(struct hda_codec *codec,
>>  				      struct snd_kcontrol *kcontrol,
>>  				      struct snd_ctl_elem_value *ucontrol)
>>  {
>> -	if (!ucontrol || !led_set_func)
>> +	if (!ucontrol)
>>  		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);
>> +		if (led_set_func_tpacpi)
>> +			led_set_func_tpacpi(TPACPI_LED_MICMUTE, !val);
>> +		if (led_set_func_hid_lenovo)
>> +			led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, !val);
>>  	}
>>  }
>>  
>> -static void hda_fixup_thinkpad(struct hda_codec *codec,
>> -				    const struct hda_fixup *fix, int action)
>> +static int hda_fixup_thinkpad_acpi(struct hda_codec *codec)
>>  {
>>  	struct hda_gen_spec *spec = codec->spec;
>> -	bool removefunc = false;
>> +	int ret = -ENXIO;
>>  
>> -	if (action == HDA_FIXUP_ACT_PROBE) {
>> -		if (!is_thinkpad(codec))
>> -			return;
>> -		if (!led_set_func)
>> -			led_set_func = symbol_request(tpacpi_led_set);
>> -		if (!led_set_func) {
>> -			codec_warn(codec,
>> -				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
>> -			return;
>> -		}
>> +	if (!is_thinkpad(codec))
>> +		return -ENODEV;
>> +	if (!is_thinkpad_acpi(codec))
>> +		return -ENODEV;
>> +	if (!led_set_func_tpacpi)
>> +		led_set_func_tpacpi = symbol_request(tpacpi_led_set);
> 
> This would be performed even if CONFIG_THINKPAD_ACPI=n when
> CONFIG_HID_LENOVO!=n.  You'd need to have a proper ifdef surrounding
> the function.
> 
> 
>> +	if (!led_set_func_tpacpi) {
>> +		codec_warn(codec,
>> +			   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
>> +		return -ENOENT;
>> +	}
>>  
>> -		removefunc = true;
>> -		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
>> -			old_vmaster_hook = spec->vmaster_mute.hook;
>> -			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)
>> -				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(TPACPI_LED_MUTE, false) >= 0) {
>> +		old_vmaster_hook = spec->vmaster_mute.hook;
>> +		spec->vmaster_mute.hook = update_thinkpad_mute_led;
>> +		ret = 0;
>> +	}
>> +
>> +	if (led_set_func_tpacpi(TPACPI_LED_MICMUTE, false) >= 0) {
>> +		if (spec->num_adc_nids > 1)
>> +			codec_dbg(codec,
>> +				  "Skipping micmute LED control due to several ADCs");
>> +		else {
>> +			spec->cap_sync_hook = update_thinkpad_micmute_led;
>> +			ret = 0;
>>  		}
>>  	}
>>  
>> -	if (led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
>> +	return ret;
>> +}
>> +
>> +static int hda_fixup_thinkpad_hid(struct hda_codec *codec)
>> +{
>> +	struct hda_gen_spec *spec = codec->spec;
>> +	int ret = 0;
>> +
>> +	if (!is_thinkpad(codec))
>> +		return -ENODEV;
>> +	if (!led_set_func_hid_lenovo)
>> +		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
>> +	if (!led_set_func_hid_lenovo) {
>> +		codec_warn(codec,
>> +			   "Failed to find hid-lenovo symbol hid_lenovo_led_set\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
>> +		old_vmaster_hook = spec->vmaster_mute.hook;
>> +
>> +	// do not remove hook if setting delay does not work currently because
>> +	// it is a usb hid devices which is not connected right now
>> +	// maybe is will be connected later
> 
> The comment should be C style.  Try checkpatch.pl once before the
> patch submission.
> 
> 
>> +	led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, false);
>> +	spec->vmaster_mute.hook = update_thinkpad_mute_led;
>> +
>> +	led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, false);
>> +	spec->cap_sync_hook = update_thinkpad_micmute_led;
>> +
>> +	return ret;
>> +}
>> +
>> +static void hda_fixup_thinkpad(struct hda_codec *codec,
>> +				    const struct hda_fixup *fix, int action)
>> +{
>> +	int ret_fixup_acpi = 0;
>> +	int ret_fixup_hid = 0;
>> +	bool remove = 0;
>> +
>> +	if (action == HDA_FIXUP_ACT_PROBE) {
>> +		ret_fixup_acpi = hda_fixup_thinkpad_acpi(codec);
>> +		ret_fixup_hid = hda_fixup_thinkpad_hid(codec);
> 
> Basically these two are exclusive.  So you don't have to process the
> latter when the former succeeds.  But...
> 
>> +	}
>> +
>> +	if (led_set_func_tpacpi &&
>> +		(action == HDA_FIXUP_ACT_FREE || ret_fixup_acpi)) {
>> +
>>  		symbol_put(tpacpi_led_set);
>> -		led_set_func = NULL;
>> +		remove = true;
>> +	}
>> +
>> +	if (led_set_func_hid_lenovo &&
>> +		(action == HDA_FIXUP_ACT_FREE || ret_fixup_hid)) {
>> +
>> +		symbol_put(hid_lenovo_led_set);
>> +		remove = true;
>> +	}
>> +
>> +
>> +	if (remove) {
>> +		led_set_func_tpacpi = NULL;
>> +		led_set_func_hid_lenovo = NULL;
>>  		old_vmaster_hook = NULL;
>>  	}
>>  }
> 
> ... reading through the patch, I find the code is a bit too
> redundant.  The access is exclusive between ACPI and HID, and the hook
> functions are in the same form but just the argument value is
> different (TPACPI_LED_MUTE vs HID_LENOVO_LED_MUTE).  So we may need
> only a single function pointer and pass the different value depending
> on the model instead of keeping two individual function pointers.
> 
> 
> thanks,
> 
> Takashi
> 

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Hi Takashi,

at my first shot published to github I had an approach which makes thinkpad_acpi and hid-lenovo mutual exclusive (https://github.com/dwassenberg/linux/tree/development/hid-rmi). This approach has some issues. For example in case of thinkpad_acpi and hid-lenovo module is loaded (e.g. at Lenovo ThinkPad X1 Tablet) the symbol request to tpacpi_led_set is successful so the thinkpad_helper will decide in favor of thinkpad_acpi. So I need to add something which checks the return code of tpacpi_led_set. If -ENODEV is returned it can try hid-lenovo.

But the reason why I made thinkpad_acpi and hid-lenovo work in parallel was that if it is mutual exclusive there is no possibility to control LEDs of external Lenovo USB keyboards which are connected to a Thinkpad. Currently (including the other patches) there is only the X1 Tablet Cover LEDs which is controllable from external code. But the hid-lenovo driver is able to handle different Lenovo USB Keyboards.

- ThinkPad USB Keyboard with TrackPoint (tpkbd)
- ThinkPad Compact Bluetooth Keyboard with TrackPoint (cptkbd)
- ThinkPad Compact USB Keyboard with TrackPoint (cptkbd)
- ThinkPad X1 Cover USB Keyboard with TrackPoint and Touchpad (tpx1cover)

These keyboards have these LEDs too. For some of these keyboards a LED control is implemented inside hid-lenovo too. If thinkpad_acpi and hid-lenovo is mutual exclusive it is impossible to let the thinkpad_helper control the LEDs of these external USB keyboards if they are connected to a thinkpad. I don't know if anyone will do this, but I don't want to take the possibility to do such thinks :)

So.. if there is the decision (from your side) that this should be mutual exclusive I will prepare a v2 which the mutual exclusive approach I described at the top.

Thank you & best regards,

Dennis

On 12.09.2016 14:38, Takashi Iwai wrote:
> On Mon, 12 Sep 2016 12:47:03 +0200,
> Dennis Wassenberg wrote:
>>
>> Make the thinkpad_helper able to support not only led control over
>> acpi with thinkpad_acpi driver but also led control over hid-lenovo.
>> The hid-lenovo driver adapted the led control api of thinkpad_acpi.
>>
>> Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
>> ---
>>  sound/pci/hda/thinkpad_helper.c | 149 ++++++++++++++++++++++++++++++----------
>>  1 file changed, 111 insertions(+), 38 deletions(-)
>>
>> diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
>> index 62741a7..c24a4a9 100644
>> --- a/sound/pci/hda/thinkpad_helper.c
>> +++ b/sound/pci/hda/thinkpad_helper.c
>> @@ -2,79 +2,152 @@
>>   * to be included from codec driver
>>   */
>>  
>> -#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
>> -
>> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI) || IS_ENABLED(CONFIG_HID_LENOVO)
>>  #include <linux/acpi.h>
>> +#include <linux/hid-lenovo.h>
>>  #include <linux/thinkpad_acpi.h>
>>  
>> -static int (*led_set_func)(int, bool);
>> +static int (*led_set_func_tpacpi)(int, bool);
>> +static int (*led_set_func_hid_lenovo)(int, bool);
>>  static void (*old_vmaster_hook)(void *, int);
>>  
>>  static bool is_thinkpad(struct hda_codec *codec)
>>  {
>> +	return (codec->core.subsystem_id >> 16 = 0x17aa);
>> +}
>> +
>> +static bool is_thinkpad_acpi(struct hda_codec *codec)
>> +{
>>  	return (codec->core.subsystem_id >> 16 = 0x17aa) &&
>>  	       (acpi_dev_found("LEN0068") || acpi_dev_found("IBM0068"));
>>  }
>>  
>> -static void update_tpacpi_mute_led(void *private_data, int enabled)
>> +static void update_thinkpad_mute_led(void *private_data, int enabled)
>>  {
>>  	if (old_vmaster_hook)
>>  		old_vmaster_hook(private_data, enabled);
>>  
>> -	if (led_set_func)
>> -		led_set_func(TPACPI_LED_MUTE, !enabled);
>> +	if (led_set_func_tpacpi)
>> +		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
>> +
>> +	if (led_set_func_hid_lenovo)
>> +		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
>>  }
>>  
>> -static void update_tpacpi_micmute_led(struct hda_codec *codec,
>> +
>> +
>> +static void update_thinkpad_micmute_led(struct hda_codec *codec,
>>  				      struct snd_kcontrol *kcontrol,
>>  				      struct snd_ctl_elem_value *ucontrol)
>>  {
>> -	if (!ucontrol || !led_set_func)
>> +	if (!ucontrol)
>>  		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);
>> +		if (led_set_func_tpacpi)
>> +			led_set_func_tpacpi(TPACPI_LED_MICMUTE, !val);
>> +		if (led_set_func_hid_lenovo)
>> +			led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, !val);
>>  	}
>>  }
>>  
>> -static void hda_fixup_thinkpad(struct hda_codec *codec,
>> -				    const struct hda_fixup *fix, int action)
>> +static int hda_fixup_thinkpad_acpi(struct hda_codec *codec)
>>  {
>>  	struct hda_gen_spec *spec = codec->spec;
>> -	bool removefunc = false;
>> +	int ret = -ENXIO;
>>  
>> -	if (action = HDA_FIXUP_ACT_PROBE) {
>> -		if (!is_thinkpad(codec))
>> -			return;
>> -		if (!led_set_func)
>> -			led_set_func = symbol_request(tpacpi_led_set);
>> -		if (!led_set_func) {
>> -			codec_warn(codec,
>> -				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
>> -			return;
>> -		}
>> +	if (!is_thinkpad(codec))
>> +		return -ENODEV;
>> +	if (!is_thinkpad_acpi(codec))
>> +		return -ENODEV;
>> +	if (!led_set_func_tpacpi)
>> +		led_set_func_tpacpi = symbol_request(tpacpi_led_set);
> 
> This would be performed even if CONFIG_THINKPAD_ACPI=n when
> CONFIG_HID_LENOVO!=n.  You'd need to have a proper ifdef surrounding
> the function.
> 
> 
>> +	if (!led_set_func_tpacpi) {
>> +		codec_warn(codec,
>> +			   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
>> +		return -ENOENT;
>> +	}
>>  
>> -		removefunc = true;
>> -		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
>> -			old_vmaster_hook = spec->vmaster_mute.hook;
>> -			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)
>> -				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(TPACPI_LED_MUTE, false) >= 0) {
>> +		old_vmaster_hook = spec->vmaster_mute.hook;
>> +		spec->vmaster_mute.hook = update_thinkpad_mute_led;
>> +		ret = 0;
>> +	}
>> +
>> +	if (led_set_func_tpacpi(TPACPI_LED_MICMUTE, false) >= 0) {
>> +		if (spec->num_adc_nids > 1)
>> +			codec_dbg(codec,
>> +				  "Skipping micmute LED control due to several ADCs");
>> +		else {
>> +			spec->cap_sync_hook = update_thinkpad_micmute_led;
>> +			ret = 0;
>>  		}
>>  	}
>>  
>> -	if (led_set_func && (action = HDA_FIXUP_ACT_FREE || removefunc)) {
>> +	return ret;
>> +}
>> +
>> +static int hda_fixup_thinkpad_hid(struct hda_codec *codec)
>> +{
>> +	struct hda_gen_spec *spec = codec->spec;
>> +	int ret = 0;
>> +
>> +	if (!is_thinkpad(codec))
>> +		return -ENODEV;
>> +	if (!led_set_func_hid_lenovo)
>> +		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
>> +	if (!led_set_func_hid_lenovo) {
>> +		codec_warn(codec,
>> +			   "Failed to find hid-lenovo symbol hid_lenovo_led_set\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
>> +		old_vmaster_hook = spec->vmaster_mute.hook;
>> +
>> +	// do not remove hook if setting delay does not work currently because
>> +	// it is a usb hid devices which is not connected right now
>> +	// maybe is will be connected later
> 
> The comment should be C style.  Try checkpatch.pl once before the
> patch submission.
> 
> 
>> +	led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, false);
>> +	spec->vmaster_mute.hook = update_thinkpad_mute_led;
>> +
>> +	led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, false);
>> +	spec->cap_sync_hook = update_thinkpad_micmute_led;
>> +
>> +	return ret;
>> +}
>> +
>> +static void hda_fixup_thinkpad(struct hda_codec *codec,
>> +				    const struct hda_fixup *fix, int action)
>> +{
>> +	int ret_fixup_acpi = 0;
>> +	int ret_fixup_hid = 0;
>> +	bool remove = 0;
>> +
>> +	if (action = HDA_FIXUP_ACT_PROBE) {
>> +		ret_fixup_acpi = hda_fixup_thinkpad_acpi(codec);
>> +		ret_fixup_hid = hda_fixup_thinkpad_hid(codec);
> 
> Basically these two are exclusive.  So you don't have to process the
> latter when the former succeeds.  But...
> 
>> +	}
>> +
>> +	if (led_set_func_tpacpi &&
>> +		(action = HDA_FIXUP_ACT_FREE || ret_fixup_acpi)) {
>> +
>>  		symbol_put(tpacpi_led_set);
>> -		led_set_func = NULL;
>> +		remove = true;
>> +	}
>> +
>> +	if (led_set_func_hid_lenovo &&
>> +		(action = HDA_FIXUP_ACT_FREE || ret_fixup_hid)) {
>> +
>> +		symbol_put(hid_lenovo_led_set);
>> +		remove = true;
>> +	}
>> +
>> +
>> +	if (remove) {
>> +		led_set_func_tpacpi = NULL;
>> +		led_set_func_hid_lenovo = NULL;
>>  		old_vmaster_hook = NULL;
>>  	}
>>  }
> 
> ... reading through the patch, I find the code is a bit too
> redundant.  The access is exclusive between ACPI and HID, and the hook
> functions are in the same form but just the argument value is
> different (TPACPI_LED_MUTE vs HID_LENOVO_LED_MUTE).  So we may need
> only a single function pointer and pass the different value depending
> on the model instead of keeping two individual function pointers.
> 
> 
> thanks,
> 
> Takashi
> 

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Takashi Iwai]

On Wed, 14 Sep 2016 08:22:45 +0200,
Dennis Wassenberg wrote:
> 
> Hi Takashi,
> 
> at my first shot published to github I had an approach which makes thinkpad_acpi and hid-lenovo mutual exclusive (https://github.com/dwassenberg/linux/tree/development/hid-rmi). This approach has some issues. For example in case of thinkpad_acpi and hid-lenovo module is loaded (e.g. at Lenovo ThinkPad X1 Tablet) the symbol request to tpacpi_led_set is successful so the thinkpad_helper will decide in favor of thinkpad_acpi. So I need to add something which checks the return code of tpacpi_led_set. If -ENODEV is returned it can try hid-lenovo.
> 
> But the reason why I made thinkpad_acpi and hid-lenovo work in parallel was that if it is mutual exclusive there is no possibility to control LEDs of external Lenovo USB keyboards which are connected to a Thinkpad. Currently (including the other patches) there is only the X1 Tablet Cover LEDs which is controllable from external code. But the hid-lenovo driver is able to handle different Lenovo USB Keyboards.
> 
> - ThinkPad USB Keyboard with TrackPoint (tpkbd)
> - ThinkPad Compact Bluetooth Keyboard with TrackPoint (cptkbd)
> - ThinkPad Compact USB Keyboard with TrackPoint (cptkbd)
> - ThinkPad X1 Cover USB Keyboard with TrackPoint and Touchpad (tpx1cover)
> 
> These keyboards have these LEDs too. For some of these keyboards a LED control is implemented inside hid-lenovo too. If thinkpad_acpi and hid-lenovo is mutual exclusive it is impossible to let the thinkpad_helper control the LEDs of these external USB keyboards if they are connected to a thinkpad. I don't know if anyone will do this, but I don't want to take the possibility to do such thinks :)
> 
> So.. if there is the decision (from your side) that this should be mutual exclusive I will prepare a v2 which the mutual exclusive approach I described at the top.

OK, now understood.  But still make sure that it works even with one
of kconfigs is disabled.


thanks,

Takashi

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Takashi Iwai]

On Wed, 14 Sep 2016 08:22:45 +0200,
Dennis Wassenberg wrote:
> 
> Hi Takashi,
> 
> at my first shot published to github I had an approach which makes thinkpad_acpi and hid-lenovo mutual exclusive (https://github.com/dwassenberg/linux/tree/development/hid-rmi). This approach has some issues. For example in case of thinkpad_acpi and hid-lenovo module is loaded (e.g. at Lenovo ThinkPad X1 Tablet) the symbol request to tpacpi_led_set is successful so the thinkpad_helper will decide in favor of thinkpad_acpi. So I need to add something which checks the return code of tpacpi_led_set. If -ENODEV is returned it can try hid-lenovo.
> 
> But the reason why I made thinkpad_acpi and hid-lenovo work in parallel was that if it is mutual exclusive there is no possibility to control LEDs of external Lenovo USB keyboards which are connected to a Thinkpad. Currently (including the other patches) there is only the X1 Tablet Cover LEDs which is controllable from external code. But the hid-lenovo driver is able to handle different Lenovo USB Keyboards.
> 
> - ThinkPad USB Keyboard with TrackPoint (tpkbd)
> - ThinkPad Compact Bluetooth Keyboard with TrackPoint (cptkbd)
> - ThinkPad Compact USB Keyboard with TrackPoint (cptkbd)
> - ThinkPad X1 Cover USB Keyboard with TrackPoint and Touchpad (tpx1cover)
> 
> These keyboards have these LEDs too. For some of these keyboards a LED control is implemented inside hid-lenovo too. If thinkpad_acpi and hid-lenovo is mutual exclusive it is impossible to let the thinkpad_helper control the LEDs of these external USB keyboards if they are connected to a thinkpad. I don't know if anyone will do this, but I don't want to take the possibility to do such thinks :)
> 
> So.. if there is the decision (from your side) that this should be mutual exclusive I will prepare a v2 which the mutual exclusive approach I described at the top.

OK, now understood.  But still make sure that it works even with one
of kconfigs is disabled.


thanks,

Takashi

[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Make the thinkpad_helper able to support not only led control over
acpi with thinkpad_acpi driver but also led control over hid-lenovo.
The hid-lenovo driver adapted the led control api of thinkpad_acpi.
Make the thinkpad_acpi and hid-lenovo able to work combined to
support connected Lenovo USB keyboards at Lenovo Thinkpad devices.

Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
---

 sound/pci/hda/thinkpad_helper.c | 182 +++++++++++++++++++++++++++++++---------
 1 file changed, 144 insertions(+), 38 deletions(-)

diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index f0955fd..0d98624 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -1,83 +1,188 @@
 /* Helper functions for Thinkpad LED control;
  * to be included from codec driver
+ *
+ * These helper functions include both LED control over thinkpad_acpi and
+ * hid-lenovo
  */
 
-#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI) || IS_ENABLED(CONFIG_HID_LENOVO)
 #include <linux/acpi.h>
+#include <linux/hid-lenovo.h>
 #include <linux/thinkpad_acpi.h>
 
-static int (*led_set_func)(int, bool);
+static int (*led_set_func_tpacpi)(int, bool);
+static int (*led_set_func_hid_lenovo)(int, bool);
 static void (*old_vmaster_hook)(void *, int);
 
 static bool is_thinkpad(struct hda_codec *codec)
 {
+	return (codec->core.subsystem_id >> 16 == 0x17aa);
+}
+
+static bool is_thinkpad_acpi(struct hda_codec *codec)
+{
 	return is_thinkpad(codec) &&
 	       (acpi_dev_found("LEN0068") || acpi_dev_found("IBM0068"));
 }
 
-static void update_tpacpi_mute_led(void *private_data, int enabled)
+static void update_thinkpad_mute_led(void *private_data, int enabled)
 {
 	if (old_vmaster_hook)
 		old_vmaster_hook(private_data, enabled);
 
-	if (led_set_func)
-		led_set_func(TPACPI_LED_MUTE, !enabled);
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+	if (led_set_func_tpacpi)
+		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
+#endif
+#if IS_ENABLED(CONFIG_HID_LENOVO)
+	if (led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
+#endif
 }
 
-static void update_tpacpi_micmute_led(struct hda_codec *codec,
+
+
+static void update_thinkpad_micmute_led(struct hda_codec *codec,
 				      struct snd_kcontrol *kcontrol,
 				      struct snd_ctl_elem_value *ucontrol)
 {
-	if (!ucontrol || !led_set_func)
+	if (!ucontrol)
 		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);
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+		if (led_set_func_tpacpi)
+			led_set_func_tpacpi(TPACPI_LED_MICMUTE, !val);
+#endif
+#if IS_ENABLED(CONFIG_HID_LENOVO)
+		if (led_set_func_hid_lenovo)
+			led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, !val);
+#endif
 	}
 }
 
-static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
-				    const struct hda_fixup *fix, int action)
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+static int hda_fixup_thinkpad_acpi_prepare(struct hda_codec *codec)
 {
 	struct hda_gen_spec *spec = codec->spec;
-	bool removefunc = false;
+	int ret = -ENXIO;
 
-	if (action == HDA_FIXUP_ACT_PROBE) {
-		if (!is_thinkpad(codec))
-			return;
-		if (!led_set_func)
-			led_set_func = symbol_request(tpacpi_led_set);
-		if (!led_set_func) {
-			codec_warn(codec,
-				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
-			return;
-		}
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!is_thinkpad_acpi(codec))
+		return -ENODEV;
+	if (!led_set_func_tpacpi)
+		led_set_func_tpacpi = symbol_request(tpacpi_led_set);
+	if (!led_set_func_tpacpi) {
+		codec_warn(codec,
+			   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
+		return -ENOENT;
+	}
 
-		removefunc = true;
-		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
-			old_vmaster_hook = spec->vmaster_mute.hook;
-			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)
-				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(TPACPI_LED_MUTE, false) >= 0) {
+		old_vmaster_hook = spec->vmaster_mute.hook;
+		spec->vmaster_mute.hook = update_thinkpad_mute_led;
+		ret = 0;
+	}
+
+	if (led_set_func_tpacpi(TPACPI_LED_MICMUTE, false) >= 0) {
+		if (spec->num_adc_nids > 1)
+			codec_dbg(codec,
+				  "Skipping micmute LED control due to several ADCs");
+		else {
+			spec->cap_sync_hook = update_thinkpad_micmute_led;
+			ret = 0;
 		}
 	}
 
-	if (led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
+	return ret;
+}
+
+static void hda_fixup_thinkpad_acpi_setup(struct hda_codec *codec, int action)
+{
+	int ret = 0;
+
+	if (action == HDA_FIXUP_ACT_PROBE) {
+		ret = hda_fixup_thinkpad_acpi_prepare(codec);
+	}
+
+	if (led_set_func_tpacpi &&
+		(action == HDA_FIXUP_ACT_FREE || ret)) {
+
 		symbol_put(tpacpi_led_set);
-		led_set_func = NULL;
+		led_set_func_tpacpi = NULL;
+		old_vmaster_hook = NULL;
+	}
+}
+#else
+static void hda_fixup_thinkpad_acpi_setup(struct hda_codec *codec, int action)
+{
+	return 0;
+}
+#endif
+
+#if IS_ENABLED(CONFIG_HID_LENOVO)
+static int hda_fixup_thinkpad_hid_prepare(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+	int ret = 0;
+
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
+	if (!led_set_func_hid_lenovo) {
+		codec_warn(codec,
+			   "Failed to find hid-lenovo symbol hid_lenovo_led_set\n");
+		return -ENOENT;
+	}
+
+	if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
+		old_vmaster_hook = spec->vmaster_mute.hook;
+
+	/* do not remove hook if setting delay does not work currently because
+	 * it is a usb hid devices which is not connected right now
+	 * maybe is will be connected later
+	 */
+	led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, false);
+	spec->vmaster_mute.hook = update_thinkpad_mute_led;
+
+	led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, false);
+	spec->cap_sync_hook = update_thinkpad_micmute_led;
+
+	return ret;
+}
+
+static void hda_fixup_thinkpad_hid_setup(struct hda_codec *codec, int action)
+{
+	int ret = 0;
+
+	if (action == HDA_FIXUP_ACT_PROBE) {
+		ret = hda_fixup_thinkpad_hid_prepare(codec);
+	}
+
+	if (led_set_func_hid_lenovo &&
+		(action == HDA_FIXUP_ACT_FREE || ret)) {
+
+		symbol_put(hid_lenovo_led_set);
+		led_set_func_hid_lenovo = NULL;
 		old_vmaster_hook = NULL;
 	}
 }
+#else
+static void hda_fixup_thinkpad_hid_setup(struct hda_codec *codec, int action)
+{
+}
+#endif
+
+/** this fixup is not for acpi case only, it will handle hid-lenovo as well */
+static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
+				    const struct hda_fixup *fix, int action)
+{
+	hda_fixup_thinkpad_acpi_setup(codec, action);
+	hda_fixup_thinkpad_hid_setup(codec, action);
+}
 
 #else /* CONFIG_THINKPAD_ACPI */
 
@@ -87,3 +192,4 @@ static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
 }
 
 #endif /* CONFIG_THINKPAD_ACPI */
+
-- 
1.9.1
--

[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Make the thinkpad_helper able to support not only led control over
acpi with thinkpad_acpi driver but also led control over hid-lenovo.
The hid-lenovo driver adapted the led control api of thinkpad_acpi.
Make the thinkpad_acpi and hid-lenovo able to work combined to
support connected Lenovo USB keyboards at Lenovo Thinkpad devices.

Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
---

 sound/pci/hda/thinkpad_helper.c | 182 +++++++++++++++++++++++++++++++---------
 1 file changed, 144 insertions(+), 38 deletions(-)

diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index f0955fd..0d98624 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -1,83 +1,188 @@
 /* Helper functions for Thinkpad LED control;
  * to be included from codec driver
+ *
+ * These helper functions include both LED control over thinkpad_acpi and
+ * hid-lenovo
  */
 
-#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI) || IS_ENABLED(CONFIG_HID_LENOVO)
 #include <linux/acpi.h>
+#include <linux/hid-lenovo.h>
 #include <linux/thinkpad_acpi.h>
 
-static int (*led_set_func)(int, bool);
+static int (*led_set_func_tpacpi)(int, bool);
+static int (*led_set_func_hid_lenovo)(int, bool);
 static void (*old_vmaster_hook)(void *, int);
 
 static bool is_thinkpad(struct hda_codec *codec)
 {
+	return (codec->core.subsystem_id >> 16 = 0x17aa);
+}
+
+static bool is_thinkpad_acpi(struct hda_codec *codec)
+{
 	return is_thinkpad(codec) &&
 	       (acpi_dev_found("LEN0068") || acpi_dev_found("IBM0068"));
 }
 
-static void update_tpacpi_mute_led(void *private_data, int enabled)
+static void update_thinkpad_mute_led(void *private_data, int enabled)
 {
 	if (old_vmaster_hook)
 		old_vmaster_hook(private_data, enabled);
 
-	if (led_set_func)
-		led_set_func(TPACPI_LED_MUTE, !enabled);
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+	if (led_set_func_tpacpi)
+		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
+#endif
+#if IS_ENABLED(CONFIG_HID_LENOVO)
+	if (led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
+#endif
 }
 
-static void update_tpacpi_micmute_led(struct hda_codec *codec,
+
+
+static void update_thinkpad_micmute_led(struct hda_codec *codec,
 				      struct snd_kcontrol *kcontrol,
 				      struct snd_ctl_elem_value *ucontrol)
 {
-	if (!ucontrol || !led_set_func)
+	if (!ucontrol)
 		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);
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+		if (led_set_func_tpacpi)
+			led_set_func_tpacpi(TPACPI_LED_MICMUTE, !val);
+#endif
+#if IS_ENABLED(CONFIG_HID_LENOVO)
+		if (led_set_func_hid_lenovo)
+			led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, !val);
+#endif
 	}
 }
 
-static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
-				    const struct hda_fixup *fix, int action)
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+static int hda_fixup_thinkpad_acpi_prepare(struct hda_codec *codec)
 {
 	struct hda_gen_spec *spec = codec->spec;
-	bool removefunc = false;
+	int ret = -ENXIO;
 
-	if (action = HDA_FIXUP_ACT_PROBE) {
-		if (!is_thinkpad(codec))
-			return;
-		if (!led_set_func)
-			led_set_func = symbol_request(tpacpi_led_set);
-		if (!led_set_func) {
-			codec_warn(codec,
-				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
-			return;
-		}
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!is_thinkpad_acpi(codec))
+		return -ENODEV;
+	if (!led_set_func_tpacpi)
+		led_set_func_tpacpi = symbol_request(tpacpi_led_set);
+	if (!led_set_func_tpacpi) {
+		codec_warn(codec,
+			   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
+		return -ENOENT;
+	}
 
-		removefunc = true;
-		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
-			old_vmaster_hook = spec->vmaster_mute.hook;
-			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)
-				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(TPACPI_LED_MUTE, false) >= 0) {
+		old_vmaster_hook = spec->vmaster_mute.hook;
+		spec->vmaster_mute.hook = update_thinkpad_mute_led;
+		ret = 0;
+	}
+
+	if (led_set_func_tpacpi(TPACPI_LED_MICMUTE, false) >= 0) {
+		if (spec->num_adc_nids > 1)
+			codec_dbg(codec,
+				  "Skipping micmute LED control due to several ADCs");
+		else {
+			spec->cap_sync_hook = update_thinkpad_micmute_led;
+			ret = 0;
 		}
 	}
 
-	if (led_set_func && (action = HDA_FIXUP_ACT_FREE || removefunc)) {
+	return ret;
+}
+
+static void hda_fixup_thinkpad_acpi_setup(struct hda_codec *codec, int action)
+{
+	int ret = 0;
+
+	if (action = HDA_FIXUP_ACT_PROBE) {
+		ret = hda_fixup_thinkpad_acpi_prepare(codec);
+	}
+
+	if (led_set_func_tpacpi &&
+		(action = HDA_FIXUP_ACT_FREE || ret)) {
+
 		symbol_put(tpacpi_led_set);
-		led_set_func = NULL;
+		led_set_func_tpacpi = NULL;
+		old_vmaster_hook = NULL;
+	}
+}
+#else
+static void hda_fixup_thinkpad_acpi_setup(struct hda_codec *codec, int action)
+{
+	return 0;
+}
+#endif
+
+#if IS_ENABLED(CONFIG_HID_LENOVO)
+static int hda_fixup_thinkpad_hid_prepare(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+	int ret = 0;
+
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
+	if (!led_set_func_hid_lenovo) {
+		codec_warn(codec,
+			   "Failed to find hid-lenovo symbol hid_lenovo_led_set\n");
+		return -ENOENT;
+	}
+
+	if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
+		old_vmaster_hook = spec->vmaster_mute.hook;
+
+	/* do not remove hook if setting delay does not work currently because
+	 * it is a usb hid devices which is not connected right now
+	 * maybe is will be connected later
+	 */
+	led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, false);
+	spec->vmaster_mute.hook = update_thinkpad_mute_led;
+
+	led_set_func_hid_lenovo(HID_LENOVO_LED_MICMUTE, false);
+	spec->cap_sync_hook = update_thinkpad_micmute_led;
+
+	return ret;
+}
+
+static void hda_fixup_thinkpad_hid_setup(struct hda_codec *codec, int action)
+{
+	int ret = 0;
+
+	if (action = HDA_FIXUP_ACT_PROBE) {
+		ret = hda_fixup_thinkpad_hid_prepare(codec);
+	}
+
+	if (led_set_func_hid_lenovo &&
+		(action = HDA_FIXUP_ACT_FREE || ret)) {
+
+		symbol_put(hid_lenovo_led_set);
+		led_set_func_hid_lenovo = NULL;
 		old_vmaster_hook = NULL;
 	}
 }
+#else
+static void hda_fixup_thinkpad_hid_setup(struct hda_codec *codec, int action)
+{
+}
+#endif
+
+/** this fixup is not for acpi case only, it will handle hid-lenovo as well */
+static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
+				    const struct hda_fixup *fix, int action)
+{
+	hda_fixup_thinkpad_acpi_setup(codec, action);
+	hda_fixup_thinkpad_hid_setup(codec, action);
+}
 
 #else /* CONFIG_THINKPAD_ACPI */
 
@@ -87,3 +192,4 @@ static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
 }
 
 #endif /* CONFIG_THINKPAD_ACPI */
+
-- 
1.9.1
--

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Jiri Kosina]

On Fri, 9 Dec 2016, Dennis Wassenberg wrote:

> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> +	if (led_set_func_tpacpi)
> +		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
> +#endif
> +#if IS_ENABLED(CONFIG_HID_LENOVO)
> +	if (led_set_func_hid_lenovo)
> +		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);

Where does led_set_func_hid_lenovo() come from? I don't see it in Linus' 
tree as of today. Is that something else than tpacpi_led_set()?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Jiri Kosina]

On Fri, 9 Dec 2016, Dennis Wassenberg wrote:

> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> +	if (led_set_func_tpacpi)
> +		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
> +#endif
> +#if IS_ENABLED(CONFIG_HID_LENOVO)
> +	if (led_set_func_hid_lenovo)
> +		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);

Where does led_set_func_hid_lenovo() come from? I don't see it in Linus' 
tree as of today. Is that something else than tpacpi_led_set()?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Hi Jiri,

"led_set_func_hid_lenovo" is set to "hid_lenovo_led_set"

+#if IS_ENABLED(CONFIG_HID_LENOVO)
+static int hda_fixup_thinkpad_hid_prepare(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+	int ret = 0;
+
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);

"hid_lenovo_led_set" is introduced in "[PATCH v2 2/2] hda: thinkpad_helper: Add support for hid-lenovo LED control". This means that "[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control" can only work with applied "[PATCH 1/4] hid-lenovo: Add support for X1 Tablet cover special keys" and "[PATCH v2 2/2] hda: thinkpad_helper: Add support for hid-lenovo LED control".

Best regards,

Dennis
On 19.12.2016 11:09, Jiri Kosina wrote:
> On Fri, 9 Dec 2016, Dennis Wassenberg wrote:
> 
>> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
>> +	if (led_set_func_tpacpi)
>> +		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
>> +#endif
>> +#if IS_ENABLED(CONFIG_HID_LENOVO)
>> +	if (led_set_func_hid_lenovo)
>> +		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
> 
> Where does led_set_func_hid_lenovo() come from? I don't see it in Linus' 
> tree as of today. Is that something else than tpacpi_led_set()?
> 
> Thanks,
> 

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Hi Jiri,

"led_set_func_hid_lenovo" is set to "hid_lenovo_led_set"

+#if IS_ENABLED(CONFIG_HID_LENOVO)
+static int hda_fixup_thinkpad_hid_prepare(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+	int ret = 0;
+
+	if (!is_thinkpad(codec))
+		return -ENODEV;
+	if (!led_set_func_hid_lenovo)
+		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);

"hid_lenovo_led_set" is introduced in "[PATCH v2 2/2] hda: thinkpad_helper: Add support for hid-lenovo LED control". This means that "[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control" can only work with applied "[PATCH 1/4] hid-lenovo: Add support for X1 Tablet cover special keys" and "[PATCH v2 2/2] hda: thinkpad_helper: Add support for hid-lenovo LED control".

Best regards,

Dennis
On 19.12.2016 11:09, Jiri Kosina wrote:
> On Fri, 9 Dec 2016, Dennis Wassenberg wrote:
> 
>> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
>> +	if (led_set_func_tpacpi)
>> +		led_set_func_tpacpi(TPACPI_LED_MUTE, !enabled);
>> +#endif
>> +#if IS_ENABLED(CONFIG_HID_LENOVO)
>> +	if (led_set_func_hid_lenovo)
>> +		led_set_func_hid_lenovo(HID_LENOVO_LED_MUTE, !enabled);
> 
> Where does led_set_func_hid_lenovo() come from? I don't see it in Linus' 
> tree as of today. Is that something else than tpacpi_led_set()?
> 
> Thanks,
> 

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Jiri Kosina]

On Mon, 19 Dec 2016, Dennis Wassenberg wrote:

> "led_set_func_hid_lenovo" is set to "hid_lenovo_led_set"
> 
> +#if IS_ENABLED(CONFIG_HID_LENOVO)
> +static int hda_fixup_thinkpad_hid_prepare(struct hda_codec *codec)
> +{
> +	struct hda_gen_spec *spec = codec->spec;
> +	int ret = 0;
> +
> +	if (!is_thinkpad(codec))
> +		return -ENODEV;
> +	if (!led_set_func_hid_lenovo)
> +		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
> 
> "hid_lenovo_led_set" is introduced in "[PATCH v2 2/2] hda: 
> thinkpad_helper: Add support for hid-lenovo LED control". This means 
> that "[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED 
> control" can only work with applied "[PATCH 1/4] hid-lenovo: Add support 
> for X1 Tablet cover special keys" and "[PATCH v2 2/2] hda: 
> thinkpad_helper: Add support for hid-lenovo LED control".

Ah, okay, I wasn't aware of the other patchset. These two probably should 
be merged together. Is the other 2-patch set merged in any tree already? 
Where has it been submitted to?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Jiri Kosina]

On Mon, 19 Dec 2016, Dennis Wassenberg wrote:

> "led_set_func_hid_lenovo" is set to "hid_lenovo_led_set"
> 
> +#if IS_ENABLED(CONFIG_HID_LENOVO)
> +static int hda_fixup_thinkpad_hid_prepare(struct hda_codec *codec)
> +{
> +	struct hda_gen_spec *spec = codec->spec;
> +	int ret = 0;
> +
> +	if (!is_thinkpad(codec))
> +		return -ENODEV;
> +	if (!led_set_func_hid_lenovo)
> +		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
> 
> "hid_lenovo_led_set" is introduced in "[PATCH v2 2/2] hda: 
> thinkpad_helper: Add support for hid-lenovo LED control". This means 
> that "[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED 
> control" can only work with applied "[PATCH 1/4] hid-lenovo: Add support 
> for X1 Tablet cover special keys" and "[PATCH v2 2/2] hda: 
> thinkpad_helper: Add support for hid-lenovo LED control".

Ah, okay, I wasn't aware of the other patchset. These two probably should 
be merged together. Is the other 2-patch set merged in any tree already? 
Where has it been submitted to?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Hi Jiri,

On 19.12.2016 11:53, Jiri Kosina wrote:
> On Mon, 19 Dec 2016, Dennis Wassenberg wrote:
> 
>> "led_set_func_hid_lenovo" is set to "hid_lenovo_led_set"
>>
>> +#if IS_ENABLED(CONFIG_HID_LENOVO)
>> +static int hda_fixup_thinkpad_hid_prepare(struct hda_codec *codec)
>> +{
>> +	struct hda_gen_spec *spec = codec->spec;
>> +	int ret = 0;
>> +
>> +	if (!is_thinkpad(codec))
>> +		return -ENODEV;
>> +	if (!led_set_func_hid_lenovo)
>> +		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
>>
>> "hid_lenovo_led_set" is introduced in "[PATCH v2 2/2] hda: 
>> thinkpad_helper: Add support for hid-lenovo LED control". This means 
>> that "[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED 
>> control" can only work with applied "[PATCH 1/4] hid-lenovo: Add support 
>> for X1 Tablet cover special keys" and "[PATCH v2 2/2] hda: 
>> thinkpad_helper: Add support for hid-lenovo LED control".
> 
> Ah, okay, I wasn't aware of the other patchset. These two probably should 
> be merged together. Is the other 2-patch set merged in any tree already? 
> Where has it been submitted to?
> 
No 1/4, 2/4 and 3/4 are currently not merged to any tree. I reworked 1/4, 2/4 and 3/4 because of some comments from Benjamin Tissoires (mail from 14th September 2016).

The patches are submitted to all recipients at the To, CC-Line (e.g. linux-input mailing list).

Maybe I can merge 2/4 and 4/4 together. This is all for LED control. 1/4 is for key/input handling and 3/4 is a special FNLock handling where I should use a separate patch.

Currently I have no feedback regarding the reworked linux-input specific code. I hope I will get it soon :)

Best regards,

Dennis

Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control

[Dennis Wassenberg]

Hi Jiri,

On 19.12.2016 11:53, Jiri Kosina wrote:
> On Mon, 19 Dec 2016, Dennis Wassenberg wrote:
> 
>> "led_set_func_hid_lenovo" is set to "hid_lenovo_led_set"
>>
>> +#if IS_ENABLED(CONFIG_HID_LENOVO)
>> +static int hda_fixup_thinkpad_hid_prepare(struct hda_codec *codec)
>> +{
>> +	struct hda_gen_spec *spec = codec->spec;
>> +	int ret = 0;
>> +
>> +	if (!is_thinkpad(codec))
>> +		return -ENODEV;
>> +	if (!led_set_func_hid_lenovo)
>> +		led_set_func_hid_lenovo = symbol_request(hid_lenovo_led_set);
>>
>> "hid_lenovo_led_set" is introduced in "[PATCH v2 2/2] hda: 
>> thinkpad_helper: Add support for hid-lenovo LED control". This means 
>> that "[PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED 
>> control" can only work with applied "[PATCH 1/4] hid-lenovo: Add support 
>> for X1 Tablet cover special keys" and "[PATCH v2 2/2] hda: 
>> thinkpad_helper: Add support for hid-lenovo LED control".
> 
> Ah, okay, I wasn't aware of the other patchset. These two probably should 
> be merged together. Is the other 2-patch set merged in any tree already? 
> Where has it been submitted to?
> 
No 1/4, 2/4 and 3/4 are currently not merged to any tree. I reworked 1/4, 2/4 and 3/4 because of some comments from Benjamin Tissoires (mail from 14th September 2016).

The patches are submitted to all recipients at the To, CC-Line (e.g. linux-input mailing list).

Maybe I can merge 2/4 and 4/4 together. This is all for LED control. 1/4 is for key/input handling and 3/4 is a special FNLock handling where I should use a separate patch.

Currently I have no feedback regarding the reworked linux-input specific code. I hope I will get it soon :)

Best regards,

Dennis