2012/11/30 Anisse Astier <anisse@xxxxxxxxx>:
> On Fri, 30 Nov 2012 18:04:37 +0200, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote :
>
>> Add MSI Wind support to msi-wmi driver. MSI Wind has different GUID for
>> key events, different WMI key scan codes, it does not need filtering
>> consequent identical events and it does not support backlight control
>> via MSIWMI_BIOS_GUID WMI. Tested on MSI Wind U100.
>>
>> Signed-off-by: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
>> ---
>> drivers/platform/x86/msi-wmi.c | 255 ++++++++++++++++++++++++++++-------------
>> 1 file changed, 177 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
>> index b96766b..31a7221 100644
>> --- a/drivers/platform/x86/msi-wmi.c
>> +++ b/drivers/platform/x86/msi-wmi.c
>> @@ -34,29 +34,63 @@ MODULE_AUTHOR("Thomas Renninger <trenn@xxxxxxx>");
>> MODULE_DESCRIPTION("MSI laptop WMI hotkeys driver");
>> MODULE_LICENSE("GPL");
>>
>> -MODULE_ALIAS("wmi:551A1F84-FBDD-4125-91DB-3EA8F44F1D45");
>> -MODULE_ALIAS("wmi:B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2");
>> -
>> #define DRV_NAME "msi-wmi"
>>
>> #define MSIWMI_BIOS_GUID "551A1F84-FBDD-4125-91DB-3EA8F44F1D45"
>> -#define MSIWMI_EVENT_GUID "B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2"
>> -
>> -#define SCANCODE_BASE 0xD0
>> -#define MSI_WMI_BRIGHTNESSUP SCANCODE_BASE
>> -#define MSI_WMI_BRIGHTNESSDOWN (SCANCODE_BASE + 1)
>> -#define MSI_WMI_VOLUMEUP (SCANCODE_BASE + 2)
>> -#define MSI_WMI_VOLUMEDOWN (SCANCODE_BASE + 3)
>> -#define MSI_WMI_MUTE (SCANCODE_BASE + 4)
>> +#define MSIWMI_MSI_EVENT_GUID "B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2"
>> +#define MSIWMI_WIND_EVENT_GUID "5B3CC38A-40D9-7245-8AE6-1145B751BE3F"
>> +
>> +MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
>> +MODULE_ALIAS("wmi:" MSIWMI_MSI_EVENT_GUID);
>> +MODULE_ALIAS("wmi:" MSIWMI_WIND_EVENT_GUID);
>> +
>> +enum msi_scancodes {
>> + MSI_SCANCODE_BASE = 0xD0,
>> + MSI_KEY_BRIGHTNESSUP = MSI_SCANCODE_BASE,
>> + MSI_KEY_BRIGHTNESSDOWN,
>> + MSI_KEY_VOLUMEUP,
>> + MSI_KEY_VOLUMEDOWN,
>> + MSI_KEY_MUTE,
>> +};
>> static struct key_entry msi_wmi_keymap[] = {
>> - { KE_KEY, MSI_WMI_BRIGHTNESSUP, {KEY_BRIGHTNESSUP} },
>> - { KE_KEY, MSI_WMI_BRIGHTNESSDOWN, {KEY_BRIGHTNESSDOWN} },
>> - { KE_KEY, MSI_WMI_VOLUMEUP, {KEY_VOLUMEUP} },
>> - { KE_KEY, MSI_WMI_VOLUMEDOWN, {KEY_VOLUMEDOWN} },
>> - { KE_KEY, MSI_WMI_MUTE, {KEY_MUTE} },
>> - { KE_END, 0}
>> + { KE_KEY, MSI_KEY_BRIGHTNESSUP, { KEY_BRIGHTNESSUP } },
>> + { KE_KEY, MSI_KEY_BRIGHTNESSDOWN, { KEY_BRIGHTNESSDOWN } },
>> + { KE_KEY, MSI_KEY_VOLUMEUP, { KEY_VOLUMEUP } },
>> + { KE_KEY, MSI_KEY_VOLUMEDOWN, { KEY_VOLUMEDOWN} },
>> + { KE_KEY, MSI_KEY_MUTE, { KEY_MUTE } },
>> + { KE_END, 0 }
>> +};
>> +static ktime_t *last_pressed;
>> +
>> +enum wind_scancodes {
>> + /* Fn+F3 touchpad toggle */
>> + WIND_KEY_TOUCHPAD = 0x08,
>> + /* Fn+F11 Bluetooth toggle */
>> + WIND_KEY_BLUETOOTH = 0x56,
>> + /* Fn+F6 webcam toggle */
>> + WIND_KEY_CAMERA = 0x57,
>> + /* Fn+F11 Wi-Fi toggle */
>> + WIND_KEY_WLAN = 0x5f,
>> + /* Fn+F10 turbo mode toggle */
>> + WIND_KEY_TURBO = 0x60,
>> + /* Fn+F10 ECO mode toggle */
>> + WIND_KEY_ECO = 0x69,
>> +};
>
> Looking at the scancode numbers and the key function, I see no collision
> that necessitate using a separate sparse keymap, or a separate enum.
> Merging them would simplify a lot of your patch.
By merging keymaps we will lose information about what laptop
corresponds the key to. Should I add this information in comments or
is it better to use some other way?
>
>> +static struct key_entry wind_wmi_keymap[] = {
>> + /* These keys work without WMI. Ignore them to avoid double keycodes */
>> + { KE_IGNORE, WIND_KEY_TOUCHPAD, { KEY_TOUCHPAD_TOGGLE } },
>> + { KE_IGNORE, WIND_KEY_BLUETOOTH, { KEY_BLUETOOTH } },
>> + { KE_IGNORE, WIND_KEY_CAMERA, { KEY_CAMERA } },
>> + { KE_IGNORE, WIND_KEY_WLAN, { KEY_WLAN } },
>> + /* These are unknown WMI events */
>> + { KE_IGNORE, 0x00 },
>> + { KE_IGNORE, 0x62 },
>> + { KE_IGNORE, 0x63 },
>> + /* These are keys that should be handled via WMI */
>> + { KE_KEY, WIND_KEY_TURBO, { KEY_PROG1 } },
>> + { KE_KEY, WIND_KEY_ECO, { KEY_PROG2 } },
>> + { KE_END, 0 }
>> };
>> -static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
>>
>> static struct backlight_device *backlight;
>>
>> @@ -64,6 +98,38 @@ static int backlight_map[] = { 0x00, 0x33, 0x66, 0x99, 0xCC, 0xFF };
>>
>> static struct input_dev *msi_wmi_input_dev;
>>
>> +struct msi_wmi_table_entry {
>> + const char *guid;
>> + struct key_entry *keymap;
>> +
>> + /* Some MSI laptops send lots of identical events for one key press
>> + * within short period of time. Set quirk_last_pressed to true if such
>> + * repeated events need to be filtered. All scan codes must be
>> + * sequential numbers. Set scancode_base to first of them and
>> + * scancode_count to their count
>> + */
>> + bool quirk_last_pressed;
>> + u32 scancode_base;
>> + size_t scancode_count;
>> +};
>> +
>> +static struct msi_wmi_table_entry msi_wmi_table[] = {
>> + {
>> + .guid = MSIWMI_MSI_EVENT_GUID,
>> + .keymap = msi_wmi_keymap,
>> + .quirk_last_pressed = true,
>> + .scancode_base = MSI_SCANCODE_BASE,
>> + .scancode_count = ARRAY_SIZE(msi_wmi_keymap)-1,
>> + },
>> + {
>> + .guid = MSIWMI_WIND_EVENT_GUID,
>> + .keymap = wind_wmi_keymap,
>> + },
>> + {}
>> +};
> If you merge them you would only need the quirk_last_pressed variable,
> and could do away with this struct.
Yes, you're right. But what if I would completely remove
quirk_last_pressed and would use last_pressed if event 0xD0 - 0xD4
happens, otherwise skip last_pressed check? Is it good way too?
>
>> +
>> +static struct msi_wmi_table_entry *wmi;
>> +
>> static int msi_wmi_query_block(int instance, int *ret)
>> {
>> acpi_status status;
>> @@ -165,11 +231,15 @@ static void msi_wmi_notify(u32 value, void *context)
>> pr_debug("Eventcode: 0x%x\n", eventcode);
>> key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
>> eventcode);
>> - if (key) {
>> + if (!key) {
>> + pr_info("Unknown key pressed - %x\n", eventcode);
>> + goto msi_wmi_notify_exit;
>> + }
>> + if (wmi->quirk_last_pressed) {
>> ktime_t diff;
>> cur = ktime_get_real();
>> diff = ktime_sub(cur, last_pressed[key->code -
>> - SCANCODE_BASE]);
>> + wmi->scancode_base]);
>> /* Ignore event if the same event happened in a 50 ms
>> timeframe -> Key press may result in 10-20 GPEs */
>> if (ktime_to_us(diff) < 1000 * 50) {
>> @@ -178,21 +248,19 @@ static void msi_wmi_notify(u32 value, void *context)
>> key->code, ktime_to_us(diff));
>> goto msi_wmi_notify_exit;
>> }
>> - last_pressed[key->code - SCANCODE_BASE] = cur;
>> -
>> - if (key->type == KE_KEY &&
>> - /* Brightness is served via acpi video driver */
>> - (!acpi_video_backlight_support() ||
>
> You modify behaviour by removing this test. Backlight might be
> registered, but we may not want to modify it directly from here because
> the acpi video driver does that for us.
I don't remove the test. I only change !acpi_video_backlight_support()
condition by !backlight condition (actually, it's a typo, condition
have to look like 'backlight', not '!backlight', I'm sorry for this).
There is a check in msi_wmi_backlight_setup(). I call
acpi_video_backlight_support() from there and backlight will be NULL
if acpi video driver supports backlight. So checking backlight != NULL
is the same as checking acpi_video_backlight_support() plus fallback
if acpi video driver doesn't support backlight and WMI driver failed
to initialize backlight.
>
>> - (key->code != MSI_WMI_BRIGHTNESSUP &&
>> - key->code != MSI_WMI_BRIGHTNESSDOWN))) {
>> - pr_debug("Send key: 0x%X - "
>> - "Input layer keycode: %d\n",
>> - key->code, key->keycode);
>> - sparse_keymap_report_entry(msi_wmi_input_dev,
>> - key, 1, true);
>> - }
>> - } else
>> - pr_info("Unknown key pressed - %x\n", eventcode);
>
> Why did you remove this message ? It's useful because it helps users
> report new keys for new hardware.
No, I didn't remove the message, I only moved these lines above. I
understand importance of this message.
>
>> + last_pressed[key->code - wmi->scancode_base] = cur;
>> + }
>> +
>> + if (key->type == KE_KEY &&
>> + /* Brightness is served via acpi video driver */
>> + (!backlight ||
>> + (key->keycode != KEY_BRIGHTNESSUP &&
>> + key->keycode != KEY_BRIGHTNESSDOWN))) {
>> + pr_debug("Send key: 0x%X - Input layer keycode: %d\n",
>> + key->code, key->keycode);
>> + sparse_keymap_report_entry(msi_wmi_input_dev,
>> + key, 1, true);
>> + }
>> } else
>> pr_info("Unknown event received\n");
>>
>> @@ -200,10 +268,45 @@ msi_wmi_notify_exit:
>> kfree(response.pointer);
>> }
>>
>> +static int __init msi_wmi_backlight_setup(void)
>> +{
>> + int err;
>> + struct backlight_properties props;
>> +
>> + if (!wmi_has_guid(MSIWMI_BIOS_GUID) || acpi_video_backlight_support())
>> + return -ENODEV;
>> +
>> + memset(&props, 0, sizeof(struct backlight_properties));
>> + props.type = BACKLIGHT_PLATFORM;
>> + props.max_brightness = ARRAY_SIZE(backlight_map) - 1;
>> + backlight = backlight_device_register(DRV_NAME, NULL, NULL,
>> + &msi_backlight_ops,
>> + &props);
>> + if (IS_ERR(backlight))
>> + return PTR_ERR(backlight);
>> +
>> + err = bl_get(NULL);
>> + if (err < 0) {
>> + backlight_device_unregister(backlight);
>> + return err;
>> + }
>> +
>> + backlight->props.brightness = err;
>> +
>> + return 0;
>> +}
>> +
>> static int __init msi_wmi_input_setup(void)
>> {
>> int err;
>>
>> + if (wmi->quirk_last_pressed) {
>> + last_pressed = kcalloc(wmi->scancode_count,
>> + sizeof(last_pressed[0]), GFP_KERNEL);
>
> I see why you separated the keymaps. I'm pretty sure we can have an
> unified keymap while keeping the last_pressed array working.
I think I could check in msi_wmi_notify if event is between 0xD0 and
0xD4 and if so, use last_pressed.
>> + if (!last_pressed)
>> + return -ENOMEM;
>> + }
>> +
>> msi_wmi_input_dev = input_allocate_device();
>> if (!msi_wmi_input_dev)
>> return -ENOMEM;
>> @@ -212,7 +315,7 @@ static int __init msi_wmi_input_setup(void)
>> msi_wmi_input_dev->phys = "wmi/input0";
>> msi_wmi_input_dev->id.bustype = BUS_HOST;
>>
>> - err = sparse_keymap_setup(msi_wmi_input_dev, msi_wmi_keymap, NULL);
>> + err = sparse_keymap_setup(msi_wmi_input_dev, wmi->keymap, NULL);
>> if (err)
>> goto err_free_dev;
>>
>> @@ -221,8 +324,6 @@ static int __init msi_wmi_input_setup(void)
>> if (err)
>> goto err_free_keymap;
>>
>> - memset(last_pressed, 0, sizeof(last_pressed));
>> -
>> return 0;
>>
>> err_free_keymap:
>> @@ -235,61 +336,59 @@ err_free_dev:
>> static int __init msi_wmi_init(void)
>> {
>> int err;
>> + bool found = false;
>> + size_t i;
>> +
>> + for (i = 0; msi_wmi_table[i].guid; i++) {
>> + if (wmi_has_guid(msi_wmi_table[i].guid)) {
>> + found = true;
>> + wmi = &msi_wmi_table[i];
>> +
>> + err = msi_wmi_input_setup();
>> + if (err) {
>> + pr_err("Unable to setup input device\n");
>> + wmi = NULL;
>> + return err;
>> + }
>>
>> - if (!wmi_has_guid(MSIWMI_EVENT_GUID)) {
>> - pr_err("This machine doesn't have MSI-hotkeys through WMI\n");
>> - return -ENODEV;
>> - }
>> - err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
>> - msi_wmi_notify, NULL);
>> - if (ACPI_FAILURE(err))
>> - return -EINVAL;
>> + err = wmi_install_notify_handler(wmi->guid,
>> + msi_wmi_notify, NULL);
>> + if (ACPI_FAILURE(err)) {
>> + pr_err("Unable to setup WMI notify handler\n");
>> + sparse_keymap_free(msi_wmi_input_dev);
>> + input_unregister_device(msi_wmi_input_dev);
>> + wmi = NULL;
>> + return -EINVAL;
>> + }
>>
>> - err = msi_wmi_input_setup();
>> - if (err)
>> - goto err_uninstall_notifier;
>> -
>> - if (!acpi_video_backlight_support()) {
>> - struct backlight_properties props;
>> - memset(&props, 0, sizeof(struct backlight_properties));
>> - props.type = BACKLIGHT_PLATFORM;
>> - props.max_brightness = ARRAY_SIZE(backlight_map) - 1;
>> - backlight = backlight_device_register(DRV_NAME, NULL, NULL,
>> - &msi_backlight_ops,
>> - &props);
>> - if (IS_ERR(backlight)) {
>> - err = PTR_ERR(backlight);
>> - goto err_free_input;
>> + pr_debug("Event handler installed\n");
>> + break;
>> }
>> + }
>>
>> - err = bl_get(NULL);
>> - if (err < 0)
>> - goto err_free_backlight;
>> + if (!msi_wmi_backlight_setup()) {
>> + found = true;
>> + pr_debug("Backlight device created\n");
>> + }
>>
>> - backlight->props.brightness = err;
>> + if (!found) {
>> + pr_err("This machine doesn't have neither MSI-hotkeys nor backlight through WMI\n");
>> + return -ENODEV;
>> }
>> - pr_debug("Event handler installed\n");
>>
>> return 0;
>> -
>> -err_free_backlight:
>> - backlight_device_unregister(backlight);
>> -err_free_input:
>> - sparse_keymap_free(msi_wmi_input_dev);
>> - input_unregister_device(msi_wmi_input_dev);
>> -err_uninstall_notifier:
>> - wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
>> - return err;
>> }
>>
>> static void __exit msi_wmi_exit(void)
>> {
>> - if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
>> - wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
>> + kfree(last_pressed);
>> + if (wmi) {
>> + wmi_remove_notify_handler(wmi->guid);
>> sparse_keymap_free(msi_wmi_input_dev);
>> input_unregister_device(msi_wmi_input_dev);
>> - backlight_device_unregister(backlight);
>> }
>> + if (backlight)
>> + backlight_device_unregister(backlight);
>> }
>
> There's a lot of restructuring in here that aren't directly related to
> adding support for new hardware (enums, init path for backlight, quirk,
> etc.), which make this patch hard to review. You should try to put these
> in separate patches.
OK, I will do that and resend later, when I'll receive comments on
other patches and fix all needed things.
> Regards,
>
> Anisse
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
