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

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

[leo60228]

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>
---
 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@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..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;
+};
+
+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;
+
+	struct led_zone_set_param set_params = {
+		.zone = 1 << zone->id,
+		.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),
+		&set_params
+	};
+
+	status = wmi_evaluate_method(
+		ACER_LED_METHOD_GUID, 0, 0x6, &set_input, NULL);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	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);
+	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++) {
+		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);
+	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);
+
+MODULE_DEVICE_TABLE(wmi, acer_led_id_table);

base-commit: 009c9aa5be652675a06d5211e1640e02bbb1c33d
-- 
2.28.0

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

[Barnabás Pőcze]

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

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

[Barnabás Pőcze]

Hi

please use "reply-all" in the future when replying so that all participants
receive the email.


2021. június 16., szerda 1:40 keltezéssel, leo60228 írta:

> Thanks for the feedback, I'll start working on a V2. I have a few
> questions, though.
>
> > There are multicolor leds (see Documentation/leds/leds-class-multicolor), maybe that would be a better fit instead of creating 4x3 LED devices?
>
> I could definitely be incorrect here, but my understanding is that
> multicolor LEDs were an abstraction automatically created from
> separate red, green, and blue LEDs.

I'm not entirely sure what you mean. I'm not aware that LED multicolor class
devices would be instantiated from 3 ordinary LED devices - although it's possible
I have missed it.


>
> > This is not an appropriate LED class device name. Please see Documentation/leds/leds-class for details.
>
> I was worried I misunderstood something, but I thought this was
> correct. "white:status" is given as an example of a correct name, and
> this paragraph from the leds-class docs made me think that appending
> -N was the correct format for multiple LEDs with the same function:
>

Ahh, you're right. My bad. The "devicename" part is indeed optional.


> > It is possible that more than one LED with the same color and function will be required for given platform, differing only with an ordinal number. In this case it is preferable to just concatenate the predefined LED_FUNCTION_* name with required “-N” suffix in the driver. fwnode based drivers can use function-enumerator property for that and then the concatenation will be handled automatically by the LED core upon LED class device registration.


Regards,
Barnabás Pőcze

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

[leo60228]

On 6/15/21 8:21 PM, Barnabás Pőcze wrote:
> please use "reply-all" in the future when replying so that all participants
> receive the email.

Will do.

> I'm not entirely sure what you mean. I'm not aware that LED multicolor class
> devices would be instantiated from 3 ordinary LED devices - although it's possible
> I have missed it.

I misunderstood the docs, I looked a bit more into it and I've figured 
it out. I'm working on moving the driver to use that, thanks for the tip.

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

[leo60228]

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

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);
-- 
2.28.0

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

[Enrico Weigelt, metux IT consult]

On 16.06.21 02:51, leo60228 wrote:
> The Acer Predator Helios 500's keyboard has four zones of RGB LEDs.
> 
> This driver allows them to be controlled from Linux.

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)


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

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

[leo60228]

> 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.

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

[Enrico Weigelt, metux IT consult]

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.

CC'ing linux-input list: guys, could you have a look at this ? is input
sysbsys already capable of handling this ?



--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

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

[Hans de Goede]

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

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

[Enrico Weigelt, metux IT consult]

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
-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

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

[Hans de Goede]

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

input devices vs. keyboard backlights [WAS: [PATCH v2] platform/x86: add support for Acer Predator LEDs]

[Enrico Weigelt, metux IT consult]

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

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: input devices vs. keyboard backlights [WAS: [PATCH v2] platform/x86: add support for Acer Predator LEDs]

[Hans de Goede]

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

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

[Enrico Weigelt, metux IT consult]

On 16.06.21 00:19, leo60228 wrote:

Hi,

> The Acer Predator Helios 500's keyboard has four zones of RGB LEDs.
> 
> This driver allows them to be controlled from Linux.

have you checked whether these LEDs are behind a GPIO controller 
(possibly in the SoC itself), that's potentially used for other things,
too ?

If that's the case, we might run into trouble when one's implementing
an gpio driver and using it with generic drivers (eg. buttons).

I've haven't got a data sheet for the i9-11900 yet, but I'm quite sure
it does have gpios and suspect they're used for those things, as well
as extra keys, etc. This is a usual approach for such mainboards.

You can get a better idea by looking at the acpi methods - look out for
some other methods accessing very nearby registers.

Oh, and have you checked whether there're also some gpio definitions
in the acpi tables ? It's relatively new but some vendors already using
it.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

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

[leo60228]

> have you checked whether these LEDs are behind a GPIO controller 
> (possibly in the SoC itself), that's potentially used for other things,
> too ?

They seem to be connected to the embedded controller 
(\_SB.PCI0.LPC0.EC0.GGSI). I'm having trouble finding documentation on 
that, though.

> I've haven't got a data sheet for the i9-11900 yet, but I'm quite sure
> it does have gpios and suspect they're used for those things, as well
> as extra keys, etc. This is a usual approach for such mainboards.

There's several models of this laptop. Mine has a Ryzen 2700.

> Oh, and have you checked whether there're also some gpio definitions
> in the acpi tables ? It's relatively new but some vendors already using
> it.

There's one, but that's it. I'd expect more than one pin to be necessary 
here.

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

[Enrico Weigelt, metux IT consult]

On 16.06.21 18:06, leo60228 wrote:>> have you checked whether these LEDs 
are behind a GPIO controller
 >> (possibly in the SoC itself), that's potentially used for other things,
 >> too ?
 >
 > They seem to be connected to the embedded controller
 > (\_SB.PCI0.LPC0.EC0.GGSI). I'm having trouble finding documentation on
 > that, though.
Can you tell what this acpi code is doing ?
Talking to the EC ?

 >> Oh, and have you checked whether there're also some gpio definitions
 >> in the acpi tables ? It's relatively new but some vendors already using
 >> it.
 >
 > There's one, but that's it. I'd expect more than one pin to be necessary
 > here.
hmm, have you tried playing around w/ that pin ?


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

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

[Hans de Goede]

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

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

[leo60228]

> We only accept contributions under real-names, so you need to
> use your real first + lastname here.

I'm thinking about what to do here. I generally don't use my real name 
online.

> 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.

I looked at that driver and our hardware seems very different (mine 
doesn't have any support for the special effects, for example). I agree 
that we should likely collaborate, even if that's just to ensure this 
driver works on more hardware.

> All these lines can be replaced by a single:
> 
> module_wmi_driver(acer_led_driver);
> 
> statement.
I already submitted a V2 fixing this among other things, sorry abou
> module_wmi_driver(acer_led_driver);
> 
> statement.

I already submitted a V2 fixing this among other things, sorry about 
that. Do you know if there's anything I could've done to make it more 
discoverable?

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

[Hans de Goede]

Hi,

On 6/16/21 6:16 PM, leo60228 wrote:
>> We only accept contributions under real-names, so you need to
>> use your real first + lastname here.
> 
> I'm thinking about what to do here. I generally don't use my real name online.

Ok, I'm afraid that using your real-name is a hard requirements for kernel
contributions.

>> 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.
> 
> I looked at that driver and our hardware seems very different (mine doesn't have any support for the special effects, for example).

Hmm, but you do have both 4 RGB LED zones which are addressable per zone, are you saying that the code for supporting that basic functionality is also different ? Note Jafar's driver did not use the LED API sofar, it was simply forwarding an array of bytes from userspace to a WMI call.

> I agree that we should likely collaborate, even if that's just to ensure this driver works on more hardware.

Ack.

>> All these lines can be replaced by a single:
>>
>> module_wmi_driver(acer_led_driver);
>>
>> statement.
> I already submitted a V2 fixing this among other things, sorry abou
>> module_wmi_driver(acer_led_driver);
>>
>> statement.
> 
> I already submitted a V2 fixing this among other things, sorry about that. Do you know if there's anything I could've done to make it more discoverable?

My bad, I did see the v2, but I accidentally replied to the v1.

Regards,

Hans

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

[leo60228]

> Ok, I'm afraid that using your real-name is a hard requirements for kernel
> contributions.

Okay, thank you.

> Hmm, but you do have both 4 RGB LED zones which are addressable per zone, are you saying that the code for supporting that basic functionality is also different ? Note Jafar's driver did not use the LED API sofar, it was simply forwarding an array of bytes from userspace to a WMI call.

Jafar's patch is for method ID 20 with a 16-byte buffer, while my 
hardware uses method ID 6 with a 4-byte buffer. Method ID 20 doesn't 
seem to do anything on my machine.

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

[Jafar Akhondali]

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@redhat.com> 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@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
>