Hi, Thanks for adding me. Since the WMI GUID is used for gaming functions, which controlling LED is one of them, I think it's also better to use a generic name like "acer-gaming.c" instead of "acer-led.c". I look forward to possible co-operations with Leo too. On Wed, Jun 16, 2021 at 8:31 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > 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 >
