Implementing a platform driver w/ LEDs

Implementing a platform driver w/ LEDs

[Mario Limonciello]

[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]

Hi,

I recently submitted a patch to add a platform driver for Alienware that one of the functions will be to control some lighting zones on the machine.  I originally started implementing it using the LED class but started to find it didn't really fit the LED class well so decided to do a custom sysfs interface.

After submitting it, the platform driver maintainer (mjg59) recommended that I instead reach out to you to extend the LED class to better fit how the system is architected.

The system has 3 lighting zones (HEAD, Left, Right).  Each of those lighting zones can independently customize the shades of red, green, blue, as well as the brightness intensity for the zone.

So each zone has:
0x00 - 0xFF red shade
0x00 - 0xFF blue shade
0x00 - 0xFF green shade
0x00 - 0xFF brightness intensity

Additionally the zone can be configured for different lighting combinations for three different states: Running, Booting, Suspended.  Running immediately modifies the colors in the zone. Booting modifies the colors when turned on but before the next modification of "running".  Suspended modifies the colors while the system is in S3.

When I first started to implement this using the LED class I create it with 36 different LED nodes:

alienware-wmi::running::head_red
alienware-wmi::running::head_blue
alienware-wmi::running::head_green
alienware-wmi::running::head_brightness
alienware-wmi::running::left_red
alienware-wmi::running::left_blue
alienware-wmi::running::left_green
alienware-wmi::running::left_brightness
alienware-wmi::running::right_red
alienware-wmi::running::right_blue
alienware-wmi::running::right_green
alienware-wmi::running::right_brightness

alienware-wmi::booting::head_red
alienware-wmi::booting::head_blue
alienware-wmi::booting::head_green
alienware-wmi::booting::head_brightness
alienware-wmi::booting::left_red
alienware-wmi::booting::left_blue
alienware-wmi::booting::left_green
alienware-wmi::booting::left_brightness
alienware-wmi::booting::right_red
alienware-wmi::booting::right_blue
alienware-wmi::booting::right_green
alienware-wmi::booting::right_brightness

alienware-wmi::suspend::head_red
alienware-wmi::suspend::head_blue
alienware-wmi::suspend::head_green
alienware-wmi::suspend::head_brightness
alienware-wmi::suspend::left_red
alienware-wmi::suspend::left_blue
alienware-wmi::suspend::left_green
alienware-wmi::suspend::left_brightness
alienware-wmi::suspend::right_red
alienware-wmi::suspend::right_blue
alienware-wmi::suspend::right_green
alienware-wmi::suspend::right_brightness

I thought this was rather confusing though because each individual node only has a single "brightness" member which doesn't really identify what is being changed.  The brightness node changes the overall brightness of the whole zone.  The color shades selected for each node are mixed to come up with the overall color for the zone.

The three different modes (running/booting/suspend) all modify the same LEDs too, so it didn't seem to make sense to me that they had their own nodes.

So given all of that, can you advise how you think this should be implemented?  Would it make sense to extend the LED class to better adapt to this?  Or do you think this is better suited for a custom sysfs interface as I've already done?  I'll attach the patch for the driver so you can see how I put it together with the custom sysfs interface.

Thanks,

[-- Attachment #2: 0001-Add-WMI-driver-for-controling-AlienFX-on-Alienware.patch --]
[-- Type: text/x-diff, Size: 11116 bytes --]

>From e8e4cadc5e467835f3aabfa603eb44e757bdf9a8 Mon Sep 17 00:00:00 2001
From: Mario Limonciello <mario_limonciello@dell.com>
Date: Wed, 5 Feb 2014 10:08:13 -0600
Subject: [PATCH 1/1] Add WMI driver for controlling AlienFX on Alienware

Specifically for platforms that don't contain an AlienFX USB based MCU
such as the Alienware X51-R2.

Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
---
 drivers/platform/x86/Kconfig         |   13 ++
 drivers/platform/x86/Makefile        |    1 +
 drivers/platform/x86/alienware-wmi.c |  345 ++++++++++++++++++++++++++++++++++
 3 files changed, 359 insertions(+)
 create mode 100644 drivers/platform/x86/alienware-wmi.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 5ae65c1..d030525 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -55,6 +55,19 @@ config ACERHDF
 	  If you have an Acer Aspire One netbook, say Y or M
 	  here.
 
+config ALIENWARE_WMI
+	tristate "Alienware Special feature control"
+	depends on ACPI
+	depends on ACPI_WMI
+	---help---
+	  This is a driver for controlling ALienware BIOS driven
+	  features.  It exposes an interface for controlling the AlienFX
+	  zones on Alienware machines that don't contain a dedicated AlienFX
+	  USB MCU such as the X51-R2.
+
+	  If you have an ACPI-WMI compatible Alienware desktop, say Y or M
+	  here.
+
 config ASUS_LAPTOP
 	tristate "Asus Laptop Extras"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 9b87cfc..0e9b157 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_INTEL_SMARTCONNECT)	+= intel-smartconnect.o
 
 obj-$(CONFIG_PVPANIC)           += pvpanic.o
 obj-$(CONFIG_INTEL_BAYTRAIL_MBI)	+= intel_baytrail.o
+obj-$(CONFIG_ALIENWARE_WMI) 	+= alienware-wmi.o
diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
new file mode 100644
index 0000000..76087c9
--- /dev/null
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -0,0 +1,345 @@
+/*
+ * Alienware AlienFX control
+ *
+ * Copyright (C) 2014 Dell Inc <mario_limonciello@dell.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/dmi.h>
+#include <linux/acpi.h>
+
+MODULE_AUTHOR("Mario Limonciello <mario_limonciello@dell.com>");
+MODULE_DESCRIPTION("Alienware special feature control");
+MODULE_LICENSE("GPL");
+
+static struct platform_driver platform_driver = {
+	.driver = {
+		.name = "alienware-wmi",
+		.owner = THIS_MODULE,
+	}
+};
+
+static struct platform_device *platform_device;
+
+static const struct dmi_system_id alienware_device_table[] __initconst = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "TBD"),
+		},
+	},
+	{}
+};
+
+MODULE_DEVICE_TABLE(dmi, alienware_device_table);
+
+#define RUNNING_CONTROL_GUID		"A90597CE-A997-11DA-B012-B622A1EF5492"
+#define POWERSTATE_CONTROL_GUID		"A80593CE-A997-11DA-B012-B622A1EF5492"
+#define HDMI_TOGGLE_GUID		"TBD"
+#define HDMI_MUX_GUID			"TBD"
+
+MODULE_ALIAS("wmi:" RUNNING_CONTROL_GUID);
+
+/*
+  Lighting Zone control groups
+*/
+
+#define ALIENWARE_HEAD_ZONE	1
+#define ALIENWARE_LEFT_ZONE	2
+#define ALIENWARE_RIGHT_ZONE	3
+
+enum LIGHTING_CONTROL_STATE {
+	RUNNING = 1,
+	BOOTING = 0,
+	SUSPEND = 3,
+};
+
+struct color_platform {
+	u8 blue;
+	u8 green;
+	u8 red;
+	u8 brightness;
+} __packed;
+
+struct platform_zone {
+	struct color_platform colors;
+	u8 location;
+};
+
+static struct platform_zone head = {
+	.location = ALIENWARE_HEAD_ZONE,
+};
+
+static struct platform_zone left = {
+	.location = ALIENWARE_LEFT_ZONE,
+};
+
+static struct platform_zone right = {
+	.location = ALIENWARE_RIGHT_ZONE,
+};
+
+static u8 lighting_control_state = RUNNING;
+
+static int update_leds(struct platform_zone zone, unsigned long count)
+{
+	acpi_status status;
+	char *guid;
+	struct platform_zone args = {
+		.colors = zone.colors,
+		.location = lighting_control_state
+	};
+	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
+	if (lighting_control_state == BOOTING ||
+		lighting_control_state == SUSPEND)
+		guid = POWERSTATE_CONTROL_GUID;
+	else
+		guid = RUNNING_CONTROL_GUID;
+	pr_debug("alienware-wmi: evaluate [ guid %s | zone %d ]\n",
+		guid, zone.location);
+
+	status = wmi_evaluate_method(guid, 1, zone.location, &input, NULL);
+	if (ACPI_FAILURE(status))
+		pr_err("alienware-wmi: zone set failure: %u\n", status);
+
+	return count;
+}
+
+/*
+	All color values need to be in range from 0 - 255.
+*/
+static long sanitize_input(const char *buf, unsigned long count, int *val)
+{
+	if (!count)
+		return 0;
+	if (sscanf(buf, "%i", val) != 1)
+		return -EINVAL;
+	if (*val < 0)
+		*val = 0;
+	if (*val > 255)
+		*val = 255;
+	return count;
+}
+
+#define ALIENWARE_WMI_CREATE_DEVICE_ATTR(_zone, _color, _mode)		\
+	static ssize_t show_##_zone##_color(struct device *dev,		\
+		struct device_attribute *attr, char *buf)		\
+	{								\
+		return sprintf(buf, "%d\n", _zone.colors._color);	\
+	}								\
+	static ssize_t store_##_zone##_color(struct device *dev,	\
+		struct device_attribute *attr, const char *buf,		\
+		size_t count)						\
+	{								\
+		int val, rc;						\
+		rc = sanitize_input(buf, count, &val);			\
+		_zone.colors._color = val;				\
+		return update_leds(_zone, rc);				\
+	}								\
+	static struct device_attribute dev_attr_##_zone##_##_color = {	\
+		.attr = {						\
+			.name = __stringify(_zone##_##_color),		\
+			.mode = _mode },				\
+		.show = show_##_zone##_color,				\
+		.store = store_##_zone##_color,				\
+	}
+
+static ssize_t show_control_state(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", lighting_control_state);
+}
+
+static ssize_t set_control_state(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, ssize_t count)
+{
+	int val, rc;
+	rc = sanitize_input(buf, count, &val);
+	if (val != BOOTING && val != SUSPEND)
+		val = RUNNING;
+	lighting_control_state = val;
+	pr_debug("alienware-wmi: updated control state to %d\n", val);
+	return rc;
+}
+
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(head, blue, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(head, green, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(head, red, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(head, brightness, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(left, blue, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(left, green, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(left, red, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(left, brightness, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(right, blue, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(right, green, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(right, red, 0644);
+ALIENWARE_WMI_CREATE_DEVICE_ATTR(right, brightness, 0644);
+
+static DEVICE_ATTR(lighting_control_state, S_IRUGO | S_IWUSR,
+		   show_control_state, set_control_state);
+
+static struct attribute *zone_attributes[] = {
+	&dev_attr_head_blue.attr,
+	&dev_attr_head_red.attr,
+	&dev_attr_head_green.attr,
+	&dev_attr_head_brightness.attr,
+	&dev_attr_left_blue.attr,
+	&dev_attr_left_red.attr,
+	&dev_attr_left_green.attr,
+	&dev_attr_left_brightness.attr,
+	&dev_attr_right_blue.attr,
+	&dev_attr_right_red.attr,
+	&dev_attr_right_green.attr,
+	&dev_attr_right_brightness.attr,
+	&dev_attr_lighting_control_state.attr,
+	NULL
+};
+
+static struct attribute_group zone_attribute_group = {
+	.attrs = zone_attributes
+};
+
+static void remove_zones(struct platform_device *dev)
+{
+	sysfs_remove_group(&dev->dev.kobj, &zone_attribute_group);
+}
+
+static int prepare_zones(struct platform_device *dev)
+{
+	return sysfs_create_group(&dev->dev.kobj, &zone_attribute_group);
+}
+
+/*
+  HDMI toggle control (interface and platform still TBD)
+*/
+
+static ssize_t show_hdmi(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	acpi_status status;
+	status = wmi_evaluate_method(HDMI_MUX_GUID, 1, 3, NULL, NULL);
+	if (status == 1) {
+		sprintf(buf, "hdmi-i\n");
+		return 0;
+	} else if (status == 2) {
+		sprintf(buf, "gpu\n");
+		return 0;
+	}
+	sprintf(buf, "error reading mux\n");
+	pr_err("alienware-wmi: HDMI mux read failed: results: %u\n", status);
+	return status;
+}
+
+static ssize_t toggle_hdmi(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	acpi_status status;
+	status = wmi_evaluate_method(HDMI_TOGGLE_GUID, 1, 3, NULL, NULL);
+	if (ACPI_FAILURE(status))
+		pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
+			   status);
+	return count;
+}
+
+static DEVICE_ATTR(hdmi, S_IRUGO | S_IWUSR, show_hdmi, toggle_hdmi);
+
+static void remove_hdmi(struct platform_device *device)
+{
+	device_remove_file(&device->dev, &dev_attr_hdmi);
+}
+
+static int create_hdmi(void)
+{
+	int ret = -ENOMEM;
+
+	ret = device_create_file(&platform_device->dev, &dev_attr_hdmi);
+	if (ret)
+		goto error_create_hdmi;
+	return 0;
+
+error_create_hdmi:
+	remove_hdmi(platform_device);
+	return ret;
+}
+
+static int __init alienware_wmi_init(void)
+{
+	int ret;
+
+	if (!wmi_has_guid(RUNNING_CONTROL_GUID)) {
+		pr_warn("No known WMI GUID found\n");
+		return -ENODEV;
+	}
+
+	ret = platform_driver_register(&platform_driver);
+	if (ret)
+		goto fail_platform_driver;
+	platform_device = platform_device_alloc("alienware-wmi", -1);
+	if (!platform_device) {
+		ret = -ENOMEM;
+		goto fail_platform_device1;
+	}
+	ret = platform_device_add(platform_device);
+	if (ret)
+		goto fail_platform_device2;
+
+	/*
+		No systems with HDMI-i yet, for later.
+		might make this a WMI check at that time.
+	*/
+	if (dmi_check_system(alienware_device_table)) {
+		ret = create_hdmi();
+		if (ret)
+			goto fail_prep_hdmi;
+	}
+	/*
+		Only present on AW platforms w/o MCU.
+	*/
+	if (wmi_has_guid(RUNNING_CONTROL_GUID)) {
+		ret = prepare_zones(platform_device);
+		if (ret)
+			goto fail_prep_zones;
+	}
+
+	return 0;
+
+fail_prep_zones:
+	remove_zones(platform_device);
+fail_prep_hdmi:
+	platform_device_del(platform_device);
+fail_platform_device2:
+	platform_device_put(platform_device);
+fail_platform_device1:
+	platform_driver_unregister(&platform_driver);
+fail_platform_driver:
+	return ret;
+}
+
+module_init(alienware_wmi_init);
+
+static void __exit alienware_wmi_exit(void)
+{
+	remove_zones(platform_device);
+	remove_hdmi(platform_device);
+	if (platform_device) {
+		platform_device_unregister(platform_device);
+		platform_driver_unregister(&platform_driver);
+	}
+}
+
+module_exit(alienware_wmi_exit);
-- 
1.7.9.5

Re: Implementing a platform driver w/ LEDs

[Bryan Wu]

On Wed, Feb 5, 2014 at 10:43 AM, Mario Limonciello
<mario_limonciello@dell.com> wrote:
> Hi,
>
> I recently submitted a patch to add a platform driver for Alienware that one
> of the functions will be to control some lighting zones on the machine.  I
> originally started implementing it using the LED class but started to find
> it didn't really fit the LED class well so decided to do a custom sysfs
> interface.
>
> After submitting it, the platform driver maintainer (mjg59) recommended that
> I instead reach out to you to extend the LED class to better fit how the
> system is architected.
>
> The system has 3 lighting zones (HEAD, Left, Right).  Each of those lighting
> zones can independently customize the shades of red, green, blue, as well as
> the brightness intensity for the zone.
>
> So each zone has:
> 0x00 - 0xFF red shade
> 0x00 - 0xFF blue shade
> 0x00 - 0xFF green shade
> 0x00 - 0xFF brightness intensity
>
> Additionally the zone can be configured for different lighting combinations
> for three different states: Running, Booting, Suspended.  Running
> immediately modifies the colors in the zone. Booting modifies the colors
> when turned on but before the next modification of "running".  Suspended
> modifies the colors while the system is in S3.
>

I think for this use case, we need 2 drivers:

1. LED device driver for each lighting zone, it will supports red,
blue, green and brightness operation.

2. LED trigger driver provides events trigger when system running,
booting and suspended. It can send out event to your Alienware LED
device driver to do proper LED operations.

So the basic concept is splitting out real LED operations and event triggers.

> When I first started to implement this using the LED class I create it with
> 36 different LED nodes:
>
> alienware-wmi::running::head_red
> alienware-wmi::running::head_blue
> alienware-wmi::running::head_green
> alienware-wmi::running::head_brightness
> alienware-wmi::running::left_red
> alienware-wmi::running::left_blue
> alienware-wmi::running::left_green
> alienware-wmi::running::left_brightness
> alienware-wmi::running::right_red
> alienware-wmi::running::right_blue
> alienware-wmi::running::right_green
> alienware-wmi::running::right_brightness
>
> alienware-wmi::booting::head_red
> alienware-wmi::booting::head_blue
> alienware-wmi::booting::head_green
> alienware-wmi::booting::head_brightness
> alienware-wmi::booting::left_red
> alienware-wmi::booting::left_blue
> alienware-wmi::booting::left_green
> alienware-wmi::booting::left_brightness
> alienware-wmi::booting::right_red
> alienware-wmi::booting::right_blue
> alienware-wmi::booting::right_green
> alienware-wmi::booting::right_brightness
>
> alienware-wmi::suspend::head_red
> alienware-wmi::suspend::head_blue
> alienware-wmi::suspend::head_green
> alienware-wmi::suspend::head_brightness
> alienware-wmi::suspend::left_red
> alienware-wmi::suspend::left_blue
> alienware-wmi::suspend::left_green
> alienware-wmi::suspend::left_brightness
> alienware-wmi::suspend::right_red
> alienware-wmi::suspend::right_blue
> alienware-wmi::suspend::right_green
> alienware-wmi::suspend::right_brightness
>
> I thought this was rather confusing though because each individual node only
> has a single "brightness" member which doesn't really identify what is being
> changed.  The brightness node changes the overall brightness of the whole
> zone.  The color shades selected for each node are mixed to come up with the
> overall color for the zone.
>
> The three different modes (running/booting/suspend) all modify the same LEDs
> too, so it didn't seem to make sense to me that they had their own nodes.
>
> So given all of that, can you advise how you think this should be
> implemented?  Would it make sense to extend the LED class to better adapt to
> this?  Or do you think this is better suited for a custom sysfs interface as
> I've already done?  I'll attach the patch for the driver so you can see how
> I put it together with the custom sysfs interface.
>

I just went through the patch you attached quickly, but failed to find
anything related to system booting/running/suspended. So you plan to
do that by using user space script to talk with those sysfs interface?

Thanks,
-Bryan

Re: Implementing a platform driver w/ LEDs

[Mario Limonciello]

On 02/05/2014 01:25 PM, Bryan Wu wrote:
>
> I think for this use case, we need 2 drivers:
>
> 1. LED device driver for each lighting zone, it will supports red,
> blue, green and brightness operation.
>
> 2. LED trigger driver provides events trigger when system running,
> booting and suspended. It can send out event to your Alienware LED
> device driver to do proper LED operations.
>
> So the basic concept is splitting out real LED operations and event triggers.
Sorry, I should have clarified - the LEDs operations themselves are BIOS controlled.  I don't think there should be any trigger operation necessary.  It's a different WMI BIOS call to configure the LEDs in each different state.
>> When I first started to implement this using the LED class I create it with
>> 36 different LED nodes:
>>
>> alienware-wmi::running::head_red
>> alienware-wmi::running::head_blue
>> alienware-wmi::running::head_green
>> alienware-wmi::running::head_brightness
>> alienware-wmi::running::left_red
>> alienware-wmi::running::left_blue
>> alienware-wmi::running::left_green
>> alienware-wmi::running::left_brightness
>> alienware-wmi::running::right_red
>> alienware-wmi::running::right_blue
>> alienware-wmi::running::right_green
>> alienware-wmi::running::right_brightness
>>
>> alienware-wmi::booting::head_red
>> alienware-wmi::booting::head_blue
>> alienware-wmi::booting::head_green
>> alienware-wmi::booting::head_brightness
>> alienware-wmi::booting::left_red
>> alienware-wmi::booting::left_blue
>> alienware-wmi::booting::left_green
>> alienware-wmi::booting::left_brightness
>> alienware-wmi::booting::right_red
>> alienware-wmi::booting::right_blue
>> alienware-wmi::booting::right_green
>> alienware-wmi::booting::right_brightness
>>
>> alienware-wmi::suspend::head_red
>> alienware-wmi::suspend::head_blue
>> alienware-wmi::suspend::head_green
>> alienware-wmi::suspend::head_brightness
>> alienware-wmi::suspend::left_red
>> alienware-wmi::suspend::left_blue
>> alienware-wmi::suspend::left_green
>> alienware-wmi::suspend::left_brightness
>> alienware-wmi::suspend::right_red
>> alienware-wmi::suspend::right_blue
>> alienware-wmi::suspend::right_green
>> alienware-wmi::suspend::right_brightness
>>
>> I thought this was rather confusing though because each individual node only
>> has a single "brightness" member which doesn't really identify what is being
>> changed.  The brightness node changes the overall brightness of the whole
>> zone.  The color shades selected for each node are mixed to come up with the
>> overall color for the zone.
>>
>> The three different modes (running/booting/suspend) all modify the same LEDs
>> too, so it didn't seem to make sense to me that they had their own nodes.
>>
>> So given all of that, can you advise how you think this should be
>> implemented?  Would it make sense to extend the LED class to better adapt to
>> this?  Or do you think this is better suited for a custom sysfs interface as
>> I've already done?  I'll attach the patch for the driver so you can see how
>> I put it together with the custom sysfs interface.
>>
> I just went through the patch you attached quickly, but failed to find
> anything related to system booting/running/suspended. So you plan to
> do that by using user space script to talk with those sysfs interface?
It's already in there enum LIGHTING_CONTROL_STATE defines the three different states.  There's a sysfs node called lighting_control_state that was made to control it.

static DEVICE_ATTR(lighting_control_state, S_IRUGO | S_IWUSR,
                    show_control_state, set_control_state);

The idea here should be that later a userspace GUI will be able to toggle lighting_control_state to the right enum and then make any lighting color change requests.  The BIOS would handle the actual implementation of the right time it would be effective for.

Re: Implementing a platform driver w/ LEDs

[Bryan Wu]

On Wed, Feb 5, 2014 at 12:10 PM, Mario Limonciello
<mario_limonciello@dell.com> wrote:
> On 02/05/2014 01:25 PM, Bryan Wu wrote:
>>
>>
>> I think for this use case, we need 2 drivers:
>>
>> 1. LED device driver for each lighting zone, it will supports red,
>> blue, green and brightness operation.
>>
>> 2. LED trigger driver provides events trigger when system running,
>> booting and suspended. It can send out event to your Alienware LED
>> device driver to do proper LED operations.
>>
>> So the basic concept is splitting out real LED operations and event
>> triggers.
>
> Sorry, I should have clarified - the LEDs operations themselves are BIOS
> controlled.  I don't think there should be any trigger operation necessary.
> It's a different WMI BIOS call to configure the LEDs in each different
> state.
>

OK, I got it. So you might just need to write a LED device driver
which implements some interface like .brightness_set() and also you
can provide your own sysfs interface in the same driver.

Take a look at drivers/leds/dell-led.c, I think the idea is similar
and you just need to think about support your multiple zones and sysfs
interface.

Actually I think your fully custom driver here should also work since
BIOS controls when to blink and how to blink. Normally, the LED device
driver deals with how to blink no matter it's software control or
hardware control and LED trigger driver deals with when to blink and
what pattern to blink. But looks like your driver just simple setting
and let BIOS to handle everything.

Thanks,
-Bryan

>>> When I first started to implement this using the LED class I create it
>>> with
>>> 36 different LED nodes:
>>>
>>> alienware-wmi::running::head_red
>>> alienware-wmi::running::head_blue
>>> alienware-wmi::running::head_green
>>> alienware-wmi::running::head_brightness
>>> alienware-wmi::running::left_red
>>> alienware-wmi::running::left_blue
>>> alienware-wmi::running::left_green
>>> alienware-wmi::running::left_brightness
>>> alienware-wmi::running::right_red
>>> alienware-wmi::running::right_blue
>>> alienware-wmi::running::right_green
>>> alienware-wmi::running::right_brightness
>>>
>>> alienware-wmi::booting::head_red
>>> alienware-wmi::booting::head_blue
>>> alienware-wmi::booting::head_green
>>> alienware-wmi::booting::head_brightness
>>> alienware-wmi::booting::left_red
>>> alienware-wmi::booting::left_blue
>>> alienware-wmi::booting::left_green
>>> alienware-wmi::booting::left_brightness
>>> alienware-wmi::booting::right_red
>>> alienware-wmi::booting::right_blue
>>> alienware-wmi::booting::right_green
>>> alienware-wmi::booting::right_brightness
>>>
>>> alienware-wmi::suspend::head_red
>>> alienware-wmi::suspend::head_blue
>>> alienware-wmi::suspend::head_green
>>> alienware-wmi::suspend::head_brightness
>>> alienware-wmi::suspend::left_red
>>> alienware-wmi::suspend::left_blue
>>> alienware-wmi::suspend::left_green
>>> alienware-wmi::suspend::left_brightness
>>> alienware-wmi::suspend::right_red
>>> alienware-wmi::suspend::right_blue
>>> alienware-wmi::suspend::right_green
>>> alienware-wmi::suspend::right_brightness
>>>
>>> I thought this was rather confusing though because each individual node
>>> only
>>> has a single "brightness" member which doesn't really identify what is
>>> being
>>> changed.  The brightness node changes the overall brightness of the whole
>>> zone.  The color shades selected for each node are mixed to come up with
>>> the
>>> overall color for the zone.
>>>
>>> The three different modes (running/booting/suspend) all modify the same
>>> LEDs
>>> too, so it didn't seem to make sense to me that they had their own nodes.
>>>
>>> So given all of that, can you advise how you think this should be
>>> implemented?  Would it make sense to extend the LED class to better adapt
>>> to
>>> this?  Or do you think this is better suited for a custom sysfs interface
>>> as
>>> I've already done?  I'll attach the patch for the driver so you can see
>>> how
>>> I put it together with the custom sysfs interface.
>>>
>> I just went through the patch you attached quickly, but failed to find
>> anything related to system booting/running/suspended. So you plan to
>> do that by using user space script to talk with those sysfs interface?
>
> It's already in there enum LIGHTING_CONTROL_STATE defines the three
> different states.  There's a sysfs node called lighting_control_state that
> was made to control it.
>
> static DEVICE_ATTR(lighting_control_state, S_IRUGO | S_IWUSR,
>                    show_control_state, set_control_state);
>
> The idea here should be that later a userspace GUI will be able to toggle
> lighting_control_state to the right enum and then make any lighting color
> change requests.  The BIOS would handle the actual implementation of the
> right time it would be effective for.