From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control Date: Mon, 12 Sep 2016 14:38:11 +0200 Message-ID: References: <91729b41-7537-b506-6980-94b9938c8305@secunet.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Return-path: Received: from mx2.suse.de ([195.135.220.15]:35130 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757251AbcILMiO (ORCPT ); Mon, 12 Sep 2016 08:38:14 -0400 In-Reply-To: <91729b41-7537-b506-6980-94b9938c8305@secunet.com> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Dennis Wassenberg Cc: linux-input@vger.kernel.org, linux-sound@vger.kernel.org, alsa-devel@alsa-project.org, lukas@wunner.de, Benjamin Tissoires , Andrew Duggan , perex@perex.cz, david.henningsson@canonical.com, vinod.koul@intel.com, hui.wang@canonical.com, rafael.j.wysocki@intel.com, jikos@kernel.org 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 > --- > 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 > +#include > #include > > -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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Date: Mon, 12 Sep 2016 12:38:11 +0000 Subject: Re: [PATCH 4/4] hda: thinkpad_helper: Add support for hid-lenovo LED control Message-Id: List-Id: References: <91729b41-7537-b506-6980-94b9938c8305@secunet.com> In-Reply-To: <91729b41-7537-b506-6980-94b9938c8305@secunet.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dennis Wassenberg Cc: linux-input@vger.kernel.org, linux-sound@vger.kernel.org, alsa-devel@alsa-project.org, lukas@wunner.de, Benjamin Tissoires , Andrew Duggan , perex@perex.cz, david.henningsson@canonical.com, vinod.koul@intel.com, hui.wang@canonical.com, rafael.j.wysocki@intel.com, jikos@kernel.org 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 > --- > 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 > +#include > #include > > -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