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@xxxxxxxxxxx>
>> ---
>> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sound" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Index of Archives]
[Pulseaudio]
[Linux Audio Users]
[ALSA Devel]
[Fedora Desktop]
[Fedora SELinux]
[Big List of Linux Books]
[Yosemite News]
[KDE Users]
