Hi, On 6/16/21 12:19 AM, leo60228 wrote: > 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> We only accept contributions under real-names, so you need to use your real first + lastname here. Also the GUID you are using: #define ACER_LED_METHOD_GUID "7A4DDFE7-5B5D-40B4-8595-4408E0CC7F56" Is the same one as used by another recent patch for adding keyboard LED zones support for Acer laptops: https://lore.kernel.org/platform-driver-x86/CAMW3L+24ZGowtpURUbjoCoA+eZMF0wDae1izxS+HM2uz1L9Rig@xxxxxxxxxxxxxx/ I've added Jafar to the Cc here. So it looks like we have 2 people working on the same driver, please coordinate between the 2 of you to submit a single driver. FWIW I do believe that this submission, which adds this as a new driver for the new UUID, rather then adding extra code to acer-wmi.c is the better approach. Jafar's version does have the benefit of also adding support for some of the special effect modes, but there is still a discussion ongoing on how the userspace API should look for those, so starting with a clean driver like this, which does not support the effects might be best for now. <snip> I've not done anything close to a full review, but one thing stood out on a quick scan of the driver: > +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); All these lines can be replaced by a single: module_wmi_driver(acer_led_driver); statement. Regards, Hans
