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

[Takashi Iwai <tiwai@suse.de>]

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