| Message ID | 20161023194652.24335-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sunday 23 October 2016 21:46:51 Hans de Goede wrote: > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > index da2fe18..f86e774 100644 > --- a/drivers/platform/x86/dell-wmi.c > +++ b/drivers/platform/x86/dell-wmi.c > @@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code) > if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request) > return; > > + if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea || > + code == 0x02eb || code == 0x02ec || code == 0x02f6)) > + dell_smbios_call_notifier( > + dell_smbios_kbd_backlight_brightness_changed, NULL); > + > sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true); > } > This part of patch is ugly. Some random numbers are checked and then notifier is called. We already have big table with explanation of those events... It is not possible to extend it with some flag or somehow other that value should be called via notifier? Btw, personally I would use uppercase DELL_SMBIOS_KBD_... name from that enum, but I do not know what is correct coding style here for kernel.
Hi, On 24-10-16 15:34, Pali Rohár wrote: > On Sunday 23 October 2016 21:46:51 Hans de Goede wrote: >> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c >> index da2fe18..f86e774 100644 >> --- a/drivers/platform/x86/dell-wmi.c >> +++ b/drivers/platform/x86/dell-wmi.c >> @@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code) >> if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request) >> return; >> >> + if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea || >> + code == 0x02eb || code == 0x02ec || code == 0x02f6)) >> + dell_smbios_call_notifier( >> + dell_smbios_kbd_backlight_brightness_changed, NULL); >> + >> sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true); >> } >> > > This part of patch is ugly. Some random numbers are checked and then > notifier is called. We already have big table with explanation of those > events... It is not possible to extend it with some flag or somehow > other that value should be called via notifier? Nope, sparse_keymaps are a well defined API for, well, keymaps! The problem really is this commit: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86?id=e075b3c898e4055ec62a1f0ed7f3b8e62814bfb6 Which mixes status-events and key-press events in one sparse-keymap, which happens to work because so far all the status events are using { KE_IGNORE, 0x...., { KEY_RESERVED } }, but now we want to actually do something and that shows that the above commit really is a bad idea (at least for the 0x0011 type events, if we (partially) revert that, then the ugly if goes away and I can simply insert the dell_smbios_call_notifier() above the break in the original switch-case handling for 0x0011 type events. So shall I revert the 0011 part of the mentioned commit? That is actually what I had in an earlier (never posted) version of the patch-set. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 24 October 2016 15:43:50 Hans de Goede wrote: > Hi, > > On 24-10-16 15:34, Pali Rohár wrote: > >On Sunday 23 October 2016 21:46:51 Hans de Goede wrote: > >>diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > >>index da2fe18..f86e774 100644 > >>--- a/drivers/platform/x86/dell-wmi.c > >>+++ b/drivers/platform/x86/dell-wmi.c > >>@@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code) > >> if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request) > >> return; > >> > >>+ if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea || > >>+ code == 0x02eb || code == 0x02ec || code == 0x02f6)) > >>+ dell_smbios_call_notifier( > >>+ dell_smbios_kbd_backlight_brightness_changed, NULL); > >>+ > >> sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true); > >> } > >> > > > >This part of patch is ugly. Some random numbers are checked and then > >notifier is called. We already have big table with explanation of those > >events... It is not possible to extend it with some flag or somehow > >other that value should be called via notifier? > > Nope, sparse_keymaps are a well defined API for, well, keymaps! The problem > really is this commit: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86?id=e075b3c898e4055ec62a1f0ed7f3b8e62814bfb6 > > Which mixes status-events and key-press events in one sparse-keymap, > which happens to work because so far all the status events are > using { KE_IGNORE, 0x...., { KEY_RESERVED } }, but now we want to > actually do something and that shows that the above commit really > is a bad idea (at least for the 0x0011 type events, if we (partially) > revert that, then the ugly if goes away and I can simply insert > the dell_smbios_call_notifier() above the break in the original > switch-case handling for 0x0011 type events. > > So shall I revert the 0011 part of the mentioned commit? Does not help us, because keyboard backlight change event is also in dell_wmi_keymap_type_0000 table. Another idea: instead of struct key_entry create new structure which reflect information which comes from dell's WMI: u16 type (key, event or ignore) u16 code (wmi code) union { key, event } (linux keycode or enum notifier event)
Hi, On 24-10-16 15:51, Pali Rohár wrote: > On Monday 24 October 2016 15:43:50 Hans de Goede wrote: >> Hi, >> >> On 24-10-16 15:34, Pali Rohár wrote: >>> On Sunday 23 October 2016 21:46:51 Hans de Goede wrote: >>>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c >>>> index da2fe18..f86e774 100644 >>>> --- a/drivers/platform/x86/dell-wmi.c >>>> +++ b/drivers/platform/x86/dell-wmi.c >>>> @@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code) >>>> if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request) >>>> return; >>>> >>>> + if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea || >>>> + code == 0x02eb || code == 0x02ec || code == 0x02f6)) >>>> + dell_smbios_call_notifier( >>>> + dell_smbios_kbd_backlight_brightness_changed, NULL); >>>> + >>>> sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true); >>>> } >>>> >>> >>> This part of patch is ugly. Some random numbers are checked and then >>> notifier is called. We already have big table with explanation of those >>> events... It is not possible to extend it with some flag or somehow >>> other that value should be called via notifier? >> >> Nope, sparse_keymaps are a well defined API for, well, keymaps! The problem >> really is this commit: >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86?id=e075b3c898e4055ec62a1f0ed7f3b8e62814bfb6 >> >> Which mixes status-events and key-press events in one sparse-keymap, >> which happens to work because so far all the status events are >> using { KE_IGNORE, 0x...., { KEY_RESERVED } }, but now we want to >> actually do something and that shows that the above commit really >> is a bad idea (at least for the 0x0011 type events, if we (partially) >> revert that, then the ugly if goes away and I can simply insert >> the dell_smbios_call_notifier() above the break in the original >> switch-case handling for 0x0011 type events. >> >> So shall I revert the 0011 part of the mentioned commit? > > Does not help us, because keyboard backlight change event is also in > dell_wmi_keymap_type_0000 table. Hmm, the 0000 table contains: /* Key code is followed by keyboard illumination level */ { KE_IGNORE, 0xe00c, { KEY_KBDILLUMTOGGLE } }, We could use the same keycode in the 0011 table for kbd brightness events and then do: if (event->keycode == KEY_KBDILLUMTOGGLE) dell_smbios_kbd_backlight_brightness_changed, NULL); I will send an updated version using this. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 24 October 2016 15:57:41 Hans de Goede wrote: > Hmm, the 0000 table contains: > > /* Key code is followed by keyboard illumination level */ > { KE_IGNORE, 0xe00c, { KEY_KBDILLUMTOGGLE } }, > > We could use the same keycode in the 0011 table for kbd brightness events and then do: > > if (event->keycode == KEY_KBDILLUMTOGGLE) > dell_smbios_kbd_backlight_brightness_changed, NULL); > > I will send an updated version using this. Yea, that is great!
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c index 2c2f02b..5a96c25 100644 --- a/drivers/platform/x86/dell-laptop.c +++ b/drivers/platform/x86/dell-laptop.c @@ -1942,6 +1942,8 @@ static struct led_classdev kbd_led = { static int __init kbd_led_init(struct device *dev) { + int ret; + kbd_init(); if (!kbd_led_present) return -ENODEV; @@ -1953,7 +1955,11 @@ static int __init kbd_led_init(struct device *dev) if (kbd_led.max_brightness) kbd_led.max_brightness--; } - return led_classdev_register(dev, &kbd_led); + ret = led_classdev_register(dev, &kbd_led); + if (ret) + kbd_led_present = false; + + return ret; } static void brightness_set_exit(struct led_classdev *led_cdev, @@ -1970,6 +1976,23 @@ static void kbd_led_exit(void) led_classdev_unregister(&kbd_led); } +static int dell_laptop_notifier_call(struct notifier_block *nb, + unsigned long action, void *data) +{ + switch (action) { + case dell_smbios_kbd_backlight_brightness_changed: + if (kbd_led_present) + led_notify_brightness_change(&kbd_led); + break; + } + + return NOTIFY_OK; +} + +static struct notifier_block dell_laptop_notifier = { + .notifier_call = dell_laptop_notifier_call, +}; + static int __init dell_init(void) { struct calling_interface_buffer *buffer; @@ -2013,6 +2036,8 @@ static int __init dell_init(void) debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL, &dell_debugfs_fops); + dell_smbios_register_notifier(&dell_laptop_notifier); + if (acpi_video_get_backlight_type() != acpi_backlight_vendor) return 0; @@ -2064,6 +2089,7 @@ static int __init dell_init(void) static void __exit dell_exit(void) { + dell_smbios_unregister_notifier(&dell_laptop_notifier); debugfs_remove_recursive(dell_laptop_dir); if (quirks && quirks->touchpad_led) touchpad_led_exit(); diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index da2fe18..f86e774 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code) if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request) return; + if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea || + code == 0x02eb || code == 0x02ec || code == 0x02f6)) + dell_smbios_call_notifier( + dell_smbios_kbd_backlight_brightness_changed, NULL); + sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true); }
Make dell-wmi call led_notify_brightness_change on the kbd_led led_classdev registered by dell-laptop when the kbd backlight brightness changes. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/dell-laptop.c | 28 +++++++++++++++++++++++++++- drivers/platform/x86/dell-wmi.c | 5 +++++ 2 files changed, 32 insertions(+), 1 deletion(-)