Hi Takashi,
On 15.09.2016 00:14, Takashi Iwai wrote:
> On Wed, 14 Sep 2016 14:15: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.
>> 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@xxxxxxxxxxx>
>> ---
>>
>> Changes v1 -> v2 (suggested by Takashi Iwai):
>> * Adapt to not rename fixup_thinkpad_acpi to fixup_thinkpad
>> * Made sure that it works even with one of kconfigs is disabled
>>
>> 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 (codec->core.subsystem_id >> 16 == 0x17aa) &&
>
> This should be
> return is_thinkpad(codec) &&
>
yes, ok.
>> (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;
>
> Isn't old_vmaster_hook initialized twice when both interfaces are
> present...?
>
>
> thanks,
>
> Takashi
>
No, this is not possible because of fixup thinkpad_acpi is initialized at first and fixup hid-lenovo after that. fixup hid-lenovo has the additional check:
if (update_thinkpad_mute_led != spec->vmaster_mute.hook)
old_vmaster_hook = spec->vmaster_mute.hook;
Which means if the thinkpad_acpi has already registered hid-lenovo will not register the old_vmaster_hook.
If there would be a double registration this would result in an infinite recursion loop because old_vmaster_hook is function update_thinkpad_mute_led and it will call itself the hole time.
I will prepare a v3 but unfortunately it will take some time (vacation, other work). I think it is not much to do here but a bit more regarding [PATCH v2 1/2]. This patch can not work without [PATCH v2 1/2] thats why I have to fix [PATCH v2 1/2] first.
Best regards,
Dennis
>> + 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
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@xxxxxxxxxxxxxxxx
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
