Re: [PATCH] platform/x86: add support for Acer Predator LEDs

[Hans de Goede <hdegoede@redhat.com>]

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@60228.dev>

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@mail.gmail.com/

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