| Message ID | 20210616005147.26212-1-leo@60228.dev |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] platform/x86: add support for Acer Predator LEDs | expand |
> Can you please tell a bit more what these LEDs are actually used for ? > Do they have some names or symbols ? Are they also controlled by > something else (e.g. numlock or shiftlock leds) They're used for the keyboard backlight. This functionality is pretty common on gaming laptops.
Hi, On 6/16/21 7:20 PM, Enrico Weigelt, metux IT consult wrote: > On 16.06.21 17:56, leo60228 wrote: >>> Can you please tell a bit more what these LEDs are actually used for ? >>> Do they have some names or symbols ? Are they also controlled by >>> something else (e.g. numlock or shiftlock leds) >> >> They're used for the keyboard backlight. This functionality is pretty common on gaming laptops. > > hmm, keyboard backlight ... don't we already have something for that > in input subsys ? I believe that some lone LEDs aren't the right subsys > for those stuff. Actually the standardized userspace API for exporting keyboard backlights is using the LED class sysfs API, e.g.: cat /sys/class/leds/tpacpi\:\:kbd_backlight/brightnes And the same for Dell and other kbd backlights, also the upower daemon even has code for dealing with kbd-backlights: https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/up-kbd-backlight.c exporting them over its dbus API so that non-root users can control them. Basically using the LED class for kbd-backlight functionality basically is the defacto standard under Linux, so exposing this through the LED class is definitely the right thing to do. Since these are zones however, we probably wat to avoid the kbd_backlight suffix of the name, otherwise upower will pick the first device it enumerates and control that, while leaving the other zones alone, which is not what we want. Regards, Hans
On 16.06.21 19:50, Hans de Goede wrote: Hi, >> hmm, keyboard backlight ... don't we already have something for that >> in input subsys ? I believe that some lone LEDs aren't the right subsys >> for those stuff. > > Actually the standardized userspace API for exporting keyboard backlights > is using the LED class sysfs API, e.g.: > > cat /sys/class/leds/tpacpi\:\:kbd_backlight/brightnes Sounds like we don't have an API for that particular case at all. Everbody just exposes LED class devices and userland always needs hardware specific code to practically use it. We should at least have some standard mechanism for get least getting the connection between an input device and it's backlight device(s). > And the same for Dell and other kbd backlights, also the upower > daemon even has code for dealing with kbd-backlights: > https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/up-kbd-backlight.c > exporting them over its dbus API so that non-root users can > control them. Looks like a very complicated way to do that. But actually I've never understood why I should use this strange upower thing anways :p > Basically using the LED class for kbd-backlight functionality > basically is the defacto standard under Linux, so exposing this > through the LED class is definitely the right thing to do. In general, LED class isn't so bad, as it already gives us LED control (*1), but I don't see any portable way for finding the corresponding LED for some input device. In DRM I see the backlight as subdevice. --mtx *1) just recognized that on my toshiba portege (TOS6208) it only works for readout - writing to "brightness" does nothing at all
Hi, On 6/21/21 9:23 PM, Enrico Weigelt, metux IT consult wrote: > On 16.06.21 19:50, Hans de Goede wrote: > > Hi, > >>> hmm, keyboard backlight ... don't we already have something for that >>> in input subsys ? I believe that some lone LEDs aren't the right subsys >>> for those stuff. >> >> Actually the standardized userspace API for exporting keyboard backlights >> is using the LED class sysfs API, e.g.: >> >> cat /sys/class/leds/tpacpi\:\:kbd_backlight/brightnes > > Sounds like we don't have an API for that particular case at all. > Everbody just exposes LED class devices and userland always needs > hardware specific code to practically use it. The LED API actually has specific features which are typically only used with kbd-backlights, such as the brightness_hw_changed attribute which was specifically added to allow userspace to monitor when a laptops embedded controller changes the kbd-backlight brightness in response to a Fn + somekey hotkey keypress, so that userspace can show an on-screen-display notification that the kbd brightness has changed (like how it typically does for audio volume changes too) and also showing the new brightness level. See: Documentation/ABI/testing/sysfs-class-led for the docs for the /sys/class/leds/<led>/brightness_hw_changed So yes this very much is the standardized API for dealing with kbd-backlights and has been so far years. > We should at least have some standard mechanism for get least getting > the connection between an input device and it's backlight device(s). > >> And the same for Dell and other kbd backlights, also the upower >> daemon even has code for dealing with kbd-backlights: >> https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/up-kbd-backlight.c >> exporting them over its dbus API so that non-root users can >> control them. > > Looks like a very complicated way to do that. But actually I've never > understood why I should use this strange upower thing anways :p Just because you don't have a use for it does not mean that it is not useful (and widely used) in cases where people use Linux as a desktop OS, rather then for more embedded cases. >> Basically using the LED class for kbd-backlight functionality >> basically is the defacto standard under Linux, so exposing this >> through the LED class is definitely the right thing to do. > > In general, LED class isn't so bad, as it already gives us LED control > (*1), but I don't see any portable way for finding the corresponding > LED for some input device. In DRM I see the backlight as subdevice. With USB-HID keyboards the LED class device will have the same HID-device as parent as the input device. If there is no HID parent-device, then any foo_kbd_backlight device will belong to the atkbd (PS/2) input-device. Regards, Hans
On 21.06.21 21:43, Hans de Goede wrote: Hi, > The LED API actually has specific features which are typically > only used with kbd-backlights, such as the brightness_hw_changed > attribute which was specifically added to allow userspace to > monitor when a laptops embedded controller changes the kbd-backlight > brightness in response to a Fn + somekey hotkey keypress, so that > userspace can show an on-screen-display notification that the > kbd brightness has changed (like how it typically does for > audio volume changes too) and also showing the new brightness > level. See: Documentation/ABI/testing/sysfs-class-led for > the docs for the /sys/class/leds/<led>/brightness_hw_changed yes, that's great. but it seems we're still lacking a direct standard way for associating an input device with the corresponding backlight LED. Or am I missing something ? >> Looks like a very complicated way to do that. But actually I've never >> understood why I should use this strange upower thing anways :p > > Just because you don't have a use for it does not mean that it > is not useful (and widely used) in cases where people use Linux > as a desktop OS, rather then for more embedded cases. I'm actually using Linux on desktop for over 25 years now. But let's go back to the kernel side. >> In general, LED class isn't so bad, as it already gives us LED control >> (*1), but I don't see any portable way for finding the corresponding >> LED for some input device. In DRM I see the backlight as subdevice. > > With USB-HID keyboards the LED class device will have the same HID-device > as parent as the input device. If there is no HID parent-device, then any > foo_kbd_backlight device will belong to the atkbd (PS/2) input-device. Still a lot of if's and guessing :( Why can't we make it appear the same way like the other leds (eg. caps- lock) ? Here's how it looks on my Portege: | ~ ls -l /dev/input/input0: | | drwxr-xr-x 2 root root 0 Jun 22 11:42 capabilities | lrwxrwxrwx 1 root root 0 Jun 22 11:42 device -> ../../../serio0 | drwxr-xr-x 3 root root 0 Jun 22 11:42 event0 | drwxr-xr-x 2 root root 0 Jun 22 11:42 id | drwxr-xr-x 3 root root 0 Jun 22 11:37 input0::capslock | drwxr-xr-x 3 root root 0 Jun 22 11:42 input0::numlock | drwxr-xr-x 3 root root 0 Jun 22 11:42 input0::scrolllock | -r--r--r-- 1 root root 4096 Jun 22 11:42 modalias | -r--r--r-- 1 root root 4096 Jun 22 11:42 name | -r--r--r-- 1 root root 4096 Jun 22 11:42 phys | drwxr-xr-x 2 root root 0 Jun 22 11:42 power | -r--r--r-- 1 root root 4096 Jun 22 11:42 properties | lrwxrwxrwx 1 root root 0 Jun 22 11:42 subsystem -> ../../../../../../class/input | -rw-r--r-- 1 root root 4096 Jun 22 11:42 uevent | -r--r--r-- 1 root root 4096 Jun 22 11:42 uniq I'd imagine also having some "input0::backlight" here. These leds seem to be sub-devices of the keyboard device, that's perhaps tricky to do with the current backlight drivers - but maybe just add some symlink to it ? --mtx
Hi, On 6/22/21 12:19 PM, Enrico Weigelt, metux IT consult wrote: > On 21.06.21 21:43, Hans de Goede wrote: > > Hi, > >> The LED API actually has specific features which are typically >> only used with kbd-backlights, such as the brightness_hw_changed >> attribute which was specifically added to allow userspace to >> monitor when a laptops embedded controller changes the kbd-backlight >> brightness in response to a Fn + somekey hotkey keypress, so that >> userspace can show an on-screen-display notification that the >> kbd brightness has changed (like how it typically does for >> audio volume changes too) and also showing the new brightness >> level. See: Documentation/ABI/testing/sysfs-class-led for >> the docs for the /sys/class/leds/<led>/brightness_hw_changed > > yes, that's great. but it seems we're still lacking a direct standard > way for associating an input device with the corresponding backlight > LED. True. > Or am I missing something ? In most cases the lacking way to associate the 2 is not a problem because almost all systems have only a single keyboard backlight and all existing standard Linux userspace code around kbd-backlights (like upower) has been written with this assumption. This is actually a problem when you plug in a fancy USB kbd with backlight into a laptop where the laptop kbd also has a backlight ... Fixing this would be great, but it requires fixes all over the stack and associating the backlight with a specific kbd is probably the smallest of the issues to solve here. >>> Looks like a very complicated way to do that. But actually I've never >>> understood why I should use this strange upower thing anways :p >> >> Just because you don't have a use for it does not mean that it >> is not useful (and widely used) in cases where people use Linux >> as a desktop OS, rather then for more embedded cases. > > I'm actually using Linux on desktop for over 25 years now. > > But let's go back to the kernel side. > >>> In general, LED class isn't so bad, as it already gives us LED control >>> (*1), but I don't see any portable way for finding the corresponding >>> LED for some input device. In DRM I see the backlight as subdevice. >> >> With USB-HID keyboards the LED class device will have the same HID-device >> as parent as the input device. If there is no HID parent-device, then any >> foo_kbd_backlight device will belong to the atkbd (PS/2) input-device. > > Still a lot of if's and guessing :( > > Why can't we make it appear the same way like the other leds (eg. caps- > lock) ? > Here's how it looks on my Portege: > > | ~ ls -l /dev/input/input0: So you have an input0 symlink under /dev/input to /sys/class/input/input0/ that is non standard, but that is not really relevant you get the same output if you directly do: ls -l /sys/class/input/input0/ > | > | drwxr-xr-x 2 root root 0 Jun 22 11:42 capabilities > | lrwxrwxrwx 1 root root 0 Jun 22 11:42 device -> ../../../serio0 > | drwxr-xr-x 3 root root 0 Jun 22 11:42 event0 > | drwxr-xr-x 2 root root 0 Jun 22 11:42 id > | drwxr-xr-x 3 root root 0 Jun 22 11:37 input0::capslock > | drwxr-xr-x 3 root root 0 Jun 22 11:42 input0::numlock > | drwxr-xr-x 3 root root 0 Jun 22 11:42 input0::scrolllock > | -r--r--r-- 1 root root 4096 Jun 22 11:42 modalias > | -r--r--r-- 1 root root 4096 Jun 22 11:42 name > | -r--r--r-- 1 root root 4096 Jun 22 11:42 phys > | drwxr-xr-x 2 root root 0 Jun 22 11:42 power > | -r--r--r-- 1 root root 4096 Jun 22 11:42 properties > | lrwxrwxrwx 1 root root 0 Jun 22 11:42 subsystem -> > ../../../../../../class/input > | -rw-r--r-- 1 root root 4096 Jun 22 11:42 uevent > | -r--r--r-- 1 root root 4096 Jun 22 11:42 uniq > > I'd imagine also having some "input0::backlight" here. > > These leds seem to be sub-devices of the keyboard device, They are child devices of the input-device instantiated for the keyboard device, the keyboard device itself is likely actually: /sys/devices/platform/i8042/serio0 If you do "ls -l /sys/class/input/input0" then you will see that this is a symlink pointing to /sys/devices/platform/i8042/serio0/input/input0 The LED class devices for the kbd LEDs are instantiated by the input-subsys itself, hence they use the input-device as parent. For HID based keyboards the HID device which is the parent of the input-device will also be the parent of the kbd-backlight LED class device since both are instantiated by the HID driver. E.g. if I plug in a Logitech G510 keyboard I get this: [hans@x1 ~]$ ls -l /sys/bus/hid/devices/0003:046D:C22D.000D/{leds,input}/ '/sys/bus/hid/devices/0003:046D:C22D.000D/input/': total 0 drwxr-xr-x. 6 root root 0 Jun 22 12:32 input53 drwxr-xr-x. 6 root root 0 Jun 22 12:32 input54 '/sys/bus/hid/devices/0003:046D:C22D.000D/leds/': total 0 drwxr-xr-x. 3 root root 0 Jun 22 12:32 g15::kbd_backlight drwxr-xr-x. 3 root root 0 Jun 22 12:32 g15::macro_preset1 drwxr-xr-x. 3 root root 0 Jun 22 12:32 g15::macro_preset2 drwxr-xr-x. 3 root root 0 Jun 22 12:32 g15::macro_preset3 drwxr-xr-x. 3 root root 0 Jun 22 12:32 g15::macro_record drwxr-xr-x. 3 root root 0 Jun 22 12:32 g15::power_on_backlight_val So the input and LED class devices can already be associated with each other by finding a common parent. This is not unusual, this is e.g. also how video4linux tv-apps like xawtv and tvtime find the alsa record/input device to get the audio from the tvcard, they look for an alsa device which has the same parent (which can be either USB or PCI) as the /dev/video# device from which they are grabbing the video. As I already mentioned, this linking the 2 by finding a common parent works fine for HID devices, but is a problem for laptops with a PS/2 kbd + embedded-controller controlled kbd-backlight since on one hand we have the PS/2 stack and on the other hand we have some drivers/platform/x86 driver talking some vendor specific ACPI (or WMI or some-such) interface to the embedded-controller for controlling the kbd-backlight. So in this case we have to fallback to some heuristics and guess that the kbd-backlight belongs to the keyboard registered by the atkbd driver. Now we could make the kernel do this heuristics for us and have it register a symlink but that brings interesting questions like which code is going to be responsible for registering the symlink + possible probe-ordering issues between the 2 drivers; and it would still be just a heuristic, which will likely need some quirk tables for some exceptions. We are simply better of handling these heuristics in userspace, so that we can e.g. use hwdb for any quirks. We could even have a udev rule adding a symlink if that is seen as useful, but I don't believe that these kinda heuristics belong in the kernel. Regards, Hans
diff --git a/MAINTAINERS b/MAINTAINERS index bc0ceef87..f647ea81c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -327,6 +327,12 @@ S: Maintained W: http://piie.net/?section=acerhdf F: drivers/platform/x86/acerhdf.c +ACER PREDATOR LAPTOP LEDS +M: leo60228 <leo@60228.dev> +L: platform-driver-x86@vger.kernel.org +S: Maintained +F: drivers/platform/x86/acer-led.c + ACER WMI LAPTOP EXTRAS M: "Lee, Chun-Yi" <jlee@suse.com> L: platform-driver-x86@vger.kernel.org diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 60592fb88..a6d6ed2ac 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -190,6 +190,19 @@ config ACER_WMI If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M here. +config ACER_LED + tristate "Acer Predator Laptop LEDs" + depends on ACPI + depends on ACPI_WMI + depends on LEDS_CLASS_MULTICOLOR + depends on NEW_LEDS + help + This is a driver for the RGB keyboard LEDs in Acer Predator laptops. + It was designed for the Acer Predator Helios 500. + + If you choose to compile this driver as a module the module will be + called acer-led. + config AMD_PMC tristate "AMD SoC PMC driver" depends on ACPI && PCI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index dcc8cdb95..36722207b 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -21,6 +21,7 @@ obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o obj-$(CONFIG_ACERHDF) += acerhdf.o obj-$(CONFIG_ACER_WIRELESS) += acer-wireless.o obj-$(CONFIG_ACER_WMI) += acer-wmi.o +obj-$(CONFIG_ACER_LED) += acer-led.o # AMD obj-$(CONFIG_AMD_PMC) += amd-pmc.o diff --git a/drivers/platform/x86/acer-led.c b/drivers/platform/x86/acer-led.c new file mode 100644 index 000000000..d618830e0 --- /dev/null +++ b/drivers/platform/x86/acer-led.c @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Acer LED Driver + * + * Copyright (C) 2021 leo60228 + */ + +#include <linux/acpi.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/types.h> +#include <linux/leds.h> +#include <linux/led-class-multicolor.h> +#include <linux/wmi.h> + +MODULE_AUTHOR("leo60228"); +MODULE_DESCRIPTION("Acer LED Driver"); +MODULE_LICENSE("GPL"); + +#define ACER_LED_METHOD_GUID "7A4DDFE7-5B5D-40B4-8595-4408E0CC7F56" + +struct acer_led_zone { + char name[32]; + int id; + struct mc_subled channels[3]; + struct led_classdev_mc cdev; +}; + +struct acer_led_priv { + struct acer_led_zone zones[4]; +}; + +struct led_zone_set_param { + u8 zone; + u8 red; + u8 green; + u8 blue; +} __packed; + +static int acer_led_set(struct led_classdev *lcdev, + enum led_brightness value) +{ + struct led_classdev_mc *mccdev; + struct acer_led_zone *zone; + struct led_zone_set_param set_params; + struct acpi_buffer set_input; + int err; + acpi_status status; + + mccdev = lcdev_to_mccdev(lcdev); + + err = led_mc_calc_color_components(mccdev, value); + if (err) + return err; + + zone = container_of(mccdev, struct acer_led_zone, cdev); + + set_params = (struct led_zone_set_param) { + .zone = BIT(zone->id), + .red = zone->channels[0].brightness, + .green = zone->channels[1].brightness, + .blue = zone->channels[2].brightness, + }; + set_input = (struct acpi_buffer) { + sizeof(set_params), + &set_params + }; + + status = wmi_evaluate_method( + ACER_LED_METHOD_GUID, 0, 0x6, &set_input, NULL); + if (ACPI_FAILURE(status)) + return -EIO; + + return 0; +} + +static int acer_led_setup_zone(struct wmi_device *wdev, + struct acer_led_zone *zone) +{ + snprintf(zone->name, sizeof(zone->name), ":kbd_backlight-%d", + zone->id + 1); + + zone->channels[0].color_index = LED_COLOR_ID_RED; + zone->channels[0].channel = 0; + zone->channels[1].color_index = LED_COLOR_ID_GREEN; + zone->channels[1].channel = 1; + zone->channels[2].color_index = LED_COLOR_ID_BLUE; + zone->channels[2].channel = 2; + + zone->cdev.num_colors = 3; + zone->cdev.subled_info = zone->channels; + + zone->cdev.led_cdev.name = zone->name; + zone->cdev.led_cdev.max_brightness = 255; + zone->cdev.led_cdev.brightness_set_blocking = acer_led_set; + + return devm_led_classdev_multicolor_register(&wdev->dev, &zone->cdev); +} + +static int acer_led_setup(struct wmi_device *wdev) +{ + struct acer_led_priv *priv = dev_get_drvdata(&wdev->dev); + int i, err = 0; + + for (i = 0; i < ARRAY_SIZE(priv->zones); i++) { + priv->zones[i].id = i; + + err = acer_led_setup_zone(wdev, &priv->zones[i]); + if (err) + return err; + } + + return 0; +} + +static int acer_led_probe(struct wmi_device *wdev, const void *context) +{ + struct acer_led_priv *priv; + + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + dev_set_drvdata(&wdev->dev, priv); + + return acer_led_setup(wdev); +} + +static const struct wmi_device_id acer_led_id_table[] = { + { .guid_string = ACER_LED_METHOD_GUID }, + { }, +}; + +static struct wmi_driver acer_led_driver = { + .driver = { + .name = "acer-led", + }, + .id_table = acer_led_id_table, + .probe = acer_led_probe, +}; + +module_wmi_driver(acer_led_driver); + +MODULE_DEVICE_TABLE(wmi, acer_led_id_table);
The Acer Predator Helios 500's keyboard has four zones of RGB LEDs. This driver allows them to be controlled from Linux. Signed-off-by: leo60228 <leo@60228.dev> --- In addition to some smaller concerns from review, v2 registers multicolor LEDs instead of independent red, green, and blue ones. MAINTAINERS | 6 ++ drivers/platform/x86/Kconfig | 13 +++ drivers/platform/x86/Makefile | 1 + drivers/platform/x86/acer-led.c | 143 ++++++++++++++++++++++++++++++++ 4 files changed, 163 insertions(+) create mode 100644 drivers/platform/x86/acer-led.c