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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux