Hi thanks for the patch. I have added a couple comments inline. I have also CCd the linux-leds mailing list so that you can hopefully receive some feedback from there as well. 2021. június 16., szerda 0:19 keltezéssel, leo60228 írta: > 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@xxxxxxxxx> > --- > MAINTAINERS | 6 ++ > drivers/platform/x86/Kconfig | 13 +++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/acer-led.c | 156 ++++++++++++++++++++++++++++++++ > 4 files changed, 176 insertions(+) > create mode 100644 drivers/platform/x86/acer-led.c > > 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@xxxxxxxxx> > +L: platform-driver-x86@xxxxxxxxxxxxxxx > +S: Maintained > +F: drivers/platform/x86/acer-led.c > + > ACER WMI LAPTOP EXTRAS > M: "Lee, Chun-Yi" <jlee@xxxxxxxx> > L: platform-driver-x86@xxxxxxxxxxxxxxx > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 60592fb88..7dc4fd1ef 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 > + 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..82a7b099a > --- /dev/null > +++ b/drivers/platform/x86/acer-led.c > @@ -0,0 +1,156 @@ > +// 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/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 { > + char name[32]; > + struct led_classdev cdev; > + struct acer_led_zone *zone; > +}; > + > +struct acer_led_zone { > + int id; > + struct acer_led red; > + struct acer_led green; > + struct acer_led blue; There are multicolor leds (see Documentation/leds/leds-class-multicolor), maybe that would be a better fit instead of creating 4x3 LED devices? > +}; > + > +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_update_zone(struct acer_led_zone *zone) > +{ > + int status; Use `acpi_status` instead of `int`. > + > + struct led_zone_set_param set_params = { > + .zone = 1 << zone->id, You could potentially use `BIT(zone->id)` here. > + .red = zone->red.cdev.brightness, > + .green = zone->green.cdev.brightness, > + .blue = zone->blue.cdev.brightness, > + }; > + struct acpi_buffer set_input = { > + sizeof(struct led_zone_set_param), I think `sizeof(set_params)` would be better here. > + &set_params > + }; > + > + status = wmi_evaluate_method( > + ACER_LED_METHOD_GUID, 0, 0x6, &set_input, NULL); > + if (ACPI_FAILURE(status)) > + return -EINVAL; I'm not sure if `EINVAL` is the most appropriate error code in this case. Maybe `EIO`? Or something similar? > + > + return 0; > +} > + > +static int acer_led_set(struct led_classdev *cdev, > + enum led_brightness value) > +{ > + struct acer_led *led = container_of(cdev, struct acer_led, cdev); > + > + return acer_led_update_zone(led->zone); > +} > + > +static int acer_led_setup_led(struct wmi_device *wdev, > + struct acer_led *led, > + struct acer_led_zone *zone, > + const char *color) > +{ > + snprintf(led->name, sizeof(led->name), "%s:kbd_backlight-%d", > + color, zone->id + 1); This is not an appropriate LED class device name. Please see Documentation/leds/leds-class for details. > + led->cdev.name = led->name; > + led->cdev.max_brightness = 255; > + led->cdev.brightness_set_blocking = acer_led_set; > + led->zone = zone; > + > + return devm_led_classdev_register(&wdev->dev, &led->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 < 4; i++) { I'd suggest `i < ARRAY_SIZE(priv->zones)` here. > + priv->zones[i].id = i; > + > + err = acer_led_setup_led(wdev, &priv->zones[i].red, > + &priv->zones[i], "red"); > + if (err) > + return err; > + > + err = acer_led_setup_led(wdev, &priv->zones[i].green, > + &priv->zones[i], "green"); > + if (err) > + return err; > + > + err = acer_led_setup_led(wdev, &priv->zones[i].blue, > + &priv->zones[i], "blue"); > + 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(struct acer_led_priv), GFP_KERNEL); `sizeof(*priv)` is preferred. > + 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, > +}; > + > +static int __init acer_led_init(void) > +{ > + return wmi_driver_register(&acer_led_driver); > +} > +late_initcall(acer_led_init); > + > +static void __exit acer_led_exit(void) > +{ > + wmi_driver_unregister(&acer_led_driver); > +} > +module_exit(acer_led_exit); You don't need to define init or exit methods explicitly. Just use module_wmi_driver(acer_led_driver); that should take care of everything. > + > +MODULE_DEVICE_TABLE(wmi, acer_led_id_table); > > base-commit: 009c9aa5be652675a06d5211e1640e02bbb1c33d > -- > 2.28.0 Regards, Barnabás Pőcze
