This series add support for the LEDs found at Intel NUCs since
NUC version 6.
On several NUC models, the function of the LEDs are controlled by the NUC firmware
and are programmable, which allow them to indicate several different hardware events.
They can also be programmed to represent an userspace-driven event.
Some models come with single colored or dual-colored LEDs, but high end models
have RGB LEDs.
Programming them can ether be done via BIOS or by the OS, however, BIOS settings
are limited. So, the vendor offers a Windows application that allows to fully use the
functionality provided by the firmware/hardware.
It should be noticed that there are 3 different API types, and there are already some
OOT drivers that were written to support them, using procfs, each one using a
different (and IMO confusing) API.
After looking at the existing drivers and not liking the uAPI interfaces there,
and needed to ajust the LEDs again after BIOS config reset, as this is a
recommended procedure after BIOS upgrades, I opted to write a new driver
from scratch, unifying support for all different versions and using sysfs via
the leds class.
It should be noticed that those devices use the ACPI Windows Management
Interface (WMI). There are actually 3 different implementations for it:
- one for NUC6/NUC7, which has limited support for programming just
two LEDs;
- a complely re-written interface for NUC8, which can program up to
seven LEDs, named version 0.64;
- an extended version of the NUC8 API, added for NUC10, called version
1.0, with has a few differences from version 0.64.
Such WMI APIs are documented at:
- https://www.intel.com/content/www/us/en/support/articles/000023426/intel-nuc/intel-nuc-kits.html
- https://raw.githubusercontent.com/nomego/intel_nuc_led/master/specs/INTEL_WMI_LED_0.64.pdf
- https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf
It should be noticed that, I wrote this driver mainly for my NUC8 (NUC8i7HNK),
but I also used a NUC6 in order to double-check if NUC6 support was not
crashing. Yet, while the NUC6 model I have accepts the WMI LED API, it
doesn't really work, as it seems that the BIOS of my NUC6 doesn't let
userspace to program the LEDs.
I don't have any devices using NUC10 API, so the few differences between it
and NUC8 API weren't tested.
Due to the lack of full tests on NUC6 and NUC10, and because I wrote a new
uAPI that's different than the procfs-based ones found at the OOT drivers,
IMO, the better would be to merge this via staging, but as Greg's feedback
were to apply it directly under drivers/leds, this version was changed
considering such premise.
PS. : after having the series accepted, I'll submit an extra patch for
Documentation/ABI, summarizing the ABI documentation found on patch 01.
-
- v2:
- Added an ABI documentation at patch 01 and dropped the TODO;
- Removed the .remove function, as it was just printing a message;
- Add a check for a return code, as suggested by Dan Carpenter;
- Did some code cleanups as also suggested by Dan Carpenter;
- Changed the Kconfig description as suggested by Randy Dunlap.
Mauro Carvalho Chehab (17):
docs: describe the API used to set NUC LEDs
leds: add support for NUC WMI LEDs
leds: leds-nuc: detect WMI API detection
leds: leds-nuc: add support for changing S0 brightness
leds: leds-nuc: add all types of brightness
leds: leds-nuc: allow changing the LED colors
leds: leds-nuc: add support for WMI API version 1.0
leds: leds-nuc: add basic support for NUC6 WMI
leds: leds-nuc: add brightness and color for NUC6 API
leds: leds-nuc: Add support to blink behavior for NUC8/10
leds: leds-nuc: get rid of an unused variable
leds: leds-nuc: implement blink control for NUC6
leds: leds-nuc: better detect NUC6/NUC7 devices
leds: leds-nuc: add support for HDD activity default
leds: leds-nuc: fix software blink behavior logic
leds: leds-nuc: add support for changing the ethernet type indicator
leds: leds-nuc: add support for changing the power limit scheme
Documentation/leds/index.rst | 1 +
Documentation/leds/leds-nuc.rst | 447 +++++++
MAINTAINERS | 7 +
drivers/leds/Kconfig | 8 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-nuc.c | 2097 +++++++++++++++++++++++++++++++
6 files changed, 2561 insertions(+)
create mode 100644 Documentation/leds/leds-nuc.rst
create mode 100644 drivers/leds/leds-nuc.c
--
2.31.1
Now that the core logic is in place, let's add support to
adjust the S0 brightness level.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/leds/leds-nuc.c | 79 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/drivers/leds/leds-nuc.c b/drivers/leds/leds-nuc.c
index 26bc4a4bb57c..e12fa2e7a488 100644
--- a/drivers/leds/leds-nuc.c
+++ b/drivers/leds/leds-nuc.c
@@ -395,7 +395,85 @@ static ssize_t store_indicator(struct device *dev,
return -EINVAL;
}
+/* Get S0 brightness */
+static ssize_t show_s0_brightness(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev);
+ u8 cmd, input[NUM_INPUT_ARGS] = { 0 };
+ u8 output[NUM_OUTPUT_ARGS];
+ int ret;
+
+ cmd = LED_NEW_GET_STATUS;
+ input[0] = LED_NEW_GET_CONTROL_ITEM;
+ input[1] = led->id;
+ input[2] = led->indicator;
+ input[3] = 0;
+
+ ret = nuc_nmi_cmd(dev, cmd, input, output);
+ if (ret)
+ return ret;
+
+ /* Multicolor uses a scale from 0 to 100 */
+ if (led->color_type & (LED_BLUE_AMBER | LED_BLUE_WHITE | LED_RGB))
+ return scnprintf(buf, PAGE_SIZE, "%d%%\n", output[0]);
+
+ /* single color uses 0, 50% and 100% */
+ return scnprintf(buf, PAGE_SIZE, "%d%%\n", output[0] * 50);
+}
+
+/* Change S0 brightness */
+static ssize_t store_s0_brightness(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev);
+ u8 cmd, input[NUM_INPUT_ARGS] = { 0 };
+ int ret;
+ u8 val;
+
+ if (led->indicator == LED_IND_DISABLE) {
+ dev_dbg(dev, "Led %s is disabled. ignoring it.\n", cdev->name);
+ return -EACCES;
+ }
+
+ if (kstrtou8(buf, 0, &val) || val > 100)
+ return -EINVAL;
+
+ /*
+ * For single-color LEDs, the value should be between 0 to 2, but,
+ * in order to have a consistent API, let's always handle it as if
+ * it is a percentage, for both multicolor and single color LEDs.
+ * So:
+ * value == 0 will disable the LED
+ * value up to 74% will set it the brightness to 50%
+ * value equal or above 75% will use the maximum brightness.
+ */
+ if (!(led->color_type & (LED_BLUE_AMBER | LED_BLUE_WHITE | LED_RGB))) {
+ if (val > 0 && val < 75)
+ val = 1;
+ if (val >= 75)
+ val = 2;
+ }
+
+ cmd = LED_SET_VALUE;
+ input[0] = led->id;
+ input[1] = led->indicator;
+ input[2] = 0; /* FIXME: replace by an enum */
+ input[3] = val;
+
+ ret = nuc_nmi_cmd(dev, cmd, input, NULL);
+ if (ret)
+ return ret;
+
+ return len;
+}
+
static LED_ATTR_RW(indicator);
+static LED_ATTR_RW(s0_brightness);
/*
* Attributes for multicolor LEDs
@@ -403,6 +481,7 @@ static LED_ATTR_RW(indicator);
static struct attribute *nuc_wmi_multicolor_led_attr[] = {
&dev_attr_indicator.attr,
+ &dev_attr_s0_brightness.attr,
NULL,
};
--
2.31.1
Some Intel Next Unit of Computing (NUC) machines have
software-configured LEDs that can be used to display a
variety of events:
- Power State
- HDD Activity
- Ethernet
- WiFi
- Power Limit
They can even be controlled directly via software, without
any hardware-specific indicator connected into them.
Some devices have mono-colored LEDs, but the more advanced
ones have RGB leds that can show any color.
Different color and 4 blink states can be programmed for
thee system states:
- powered on (S0);
- S3;
- Standby.
The NUC BIOSes allow to partially set them for S0, but doesn't
provide any control for the other states, nor does allow
changing the blinking logic.
They all use a WMI interface using GUID:
8C5DA44C-CDC3-46b3-8619-4E26D34390B7
But there are 3 different revisions of the spec, all using
the same GUID, but two different APIs:
- the original one, for NUC6 and to NUCi7:
- https://www.intel.com/content/www/us/en/support/articles/000023426/intel-nuc/intel-nuc-kits.html
- a new one, starting with NUCi8, with two revisions:
- https://raw.githubusercontent.com/nomego/intel_nuc_led/master/specs/INTEL_WMI_LED_0.64.pdf
- https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf
There are some OOT drivers for them, but they use procfs
and use a messy interface to setup it. Also, there are
different drivers with the same name, each with support
for each NUC family.
Let's start a new driver from scratch, using the x86 platform
WMI core and the LED class.
This initial version is compatible with NUCi8 and above, and it
was tested with a Hades Canyon NUC (NUC8i7HNK).
It provides just the more basic WMI support, allowing to change
the LED hardware/firmware indicator for each LED in runtime.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
MAINTAINERS | 7 +
drivers/leds/Kconfig | 8 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-nuc.c | 481 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 497 insertions(+)
create mode 100644 drivers/leds/leds-nuc.c
diff --git a/MAINTAINERS b/MAINTAINERS
index bd7aff0c120f..316f0e552ca6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13063,6 +13063,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/aia21/ntfs.git
F: Documentation/filesystems/ntfs.rst
F: fs/ntfs/
+NUC LED DRIVER
+M: Mauro Carvalho Chehab <[email protected]>
+L: [email protected]
+S: Maintained
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git
+F: drivers/staging/nuc-led
+
NUBUS SUBSYSTEM
M: Finn Thain <[email protected]>
L: [email protected]
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 49d99cb084db..f5b7f7a02df5 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -271,6 +271,14 @@ config LEDS_MT6323
This option enables support for on-chip LED drivers found on
Mediatek MT6323 PMIC.
+config LEDS_NUC_WMI
+ tristate "LED Support for Intel NUC"
+ depends on LEDS_CLASS
+ depends on ACPI_WMI
+ help
+ This option enables support for the WMI interface for LEDs
+ present on certain Intel NUC models.
+
config LEDS_S3C24XX
tristate "LED Support for Samsung S3C24XX GPIO LEDs"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 7e604d3028c8..11a4d29bf9a0 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o
obj-$(CONFIG_LEDS_NET48XX) += leds-net48xx.o
obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o
obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o
+obj-$(CONFIG_LEDS_NUC_WMI) += leds-nuc.o
obj-$(CONFIG_LEDS_NS2) += leds-ns2.o
obj-$(CONFIG_LEDS_OT200) += leds-ot200.o
obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o
diff --git a/drivers/leds/leds-nuc.c b/drivers/leds/leds-nuc.c
new file mode 100644
index 000000000000..69bab319122e
--- /dev/null
+++ b/drivers/leds/leds-nuc.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Intel NUC WMI Control WMI Driver
+ *
+ * Currently, it implements only the LED support
+ *
+ * Copyright(c) 2021 Mauro Carvalho Chehab
+ *
+ * Inspired on WMI from https://github.com/nomego/intel_nuc_led
+ *
+ * It follows this spec:
+ * https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf
+ */
+
+#include <linux/acpi.h>
+#include <linux/bits.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/wmi.h>
+
+#define NUC_LED_WMI_GUID "8C5DA44C-CDC3-46B3-8619-4E26D34390B7"
+
+#define MAX_LEDS 7
+#define NUM_INPUT_ARGS 4
+#define NUM_OUTPUT_ARGS 3
+
+enum led_cmds {
+ LED_QUERY = 0x03,
+ LED_NEW_GET_STATUS = 0x04,
+ LED_SET_INDICATOR = 0x05,
+ LED_SET_VALUE = 0x06,
+ LED_NOTIFICATION = 0x07,
+ LED_SWITCH_TYPE = 0x08,
+};
+
+enum led_query_subcmd {
+ LED_QUERY_LIST_ALL = 0x00,
+ LED_QUERY_COLOR_TYPE = 0x01,
+ LED_QUERY_INDICATOR_OPTIONS = 0x02,
+ LED_QUERY_CONTROL_ITEMS = 0x03,
+};
+
+enum led_new_get_subcmd {
+ LED_NEW_GET_CURRENT_INDICATOR = 0x00,
+ LED_NEW_GET_CONTROL_ITEM = 0x01,
+};
+
+/* LED color indicator */
+#define LED_BLUE_AMBER BIT(0)
+#define LED_BLUE_WHITE BIT(1)
+#define LED_RGB BIT(2)
+#define LED_SINGLE_COLOR BIT(3)
+
+/* LED indicator options */
+#define LED_IND_POWER_STATE BIT(0)
+#define LED_IND_HDD_ACTIVITY BIT(1)
+#define LED_IND_ETHERNET BIT(2)
+#define LED_IND_WIFI BIT(3)
+#define LED_IND_SOFTWARE BIT(4)
+#define LED_IND_POWER_LIMIT BIT(5)
+#define LED_IND_DISABLE BIT(6)
+
+static const char * const led_names[] = {
+ "nuc::power",
+ "nuc::hdd",
+ "nuc::skull",
+ "nuc::eyes",
+ "nuc::front1",
+ "nuc::front2",
+ "nuc::front3",
+};
+
+struct nuc_nmi_led {
+ struct led_classdev cdev;
+ struct device *dev;
+ u8 id;
+ u8 indicator;
+ u32 color_type;
+ u32 avail_indicators;
+ u32 control_items;
+};
+
+struct nuc_wmi {
+ struct nuc_nmi_led led[MAX_LEDS * 3]; /* Worse case: RGB LEDs */
+ int num_leds;
+
+ /* Avoid concurrent access to WMI */
+ struct mutex wmi_lock;
+};
+
+static int nuc_nmi_led_error(u8 error_code)
+{
+ switch (error_code) {
+ case 0:
+ return 0;
+ case 0xe1: /* Function not support */
+ return -ENOENT;
+ case 0xe2: /* Undefined device */
+ return -ENODEV;
+ case 0xe3: /* EC no respond */
+ return -EIO;
+ case 0xe4: /* Invalid Parameter */
+ return -EINVAL;
+ case 0xef: /* Unexpected error */
+ return -EFAULT;
+
+ /* Revision 1.0 Errors */
+ case 0xe5: /* Node busy */
+ return -EBUSY;
+ case 0xe6: /* Destination device is disabled or unavailable */
+ return -EACCES;
+ case 0xe7: /* Invalid CEC Opcode */
+ return -ENOENT;
+ case 0xe8: /* Data Buffer size is not enough */
+ return -ENOSPC;
+
+ default: /* Reserved */
+ return -EPROTO;
+ }
+}
+
+static int nuc_nmi_cmd(struct device *dev,
+ u8 cmd,
+ u8 input_args[NUM_INPUT_ARGS],
+ u8 output_args[NUM_OUTPUT_ARGS])
+{
+ struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+ struct nuc_wmi *priv = dev_get_drvdata(dev);
+ struct acpi_buffer input;
+ union acpi_object *obj;
+ acpi_status status;
+ int size, ret;
+ u8 *p;
+
+ input.length = NUM_INPUT_ARGS;
+ input.pointer = input_args;
+
+ mutex_lock(&priv->wmi_lock);
+ status = wmi_evaluate_method(NUC_LED_WMI_GUID, 0, cmd,
+ &input, &output);
+ mutex_unlock(&priv->wmi_lock);
+ if (ACPI_FAILURE(status)) {
+ dev_warn(dev, "cmd %02x (%*ph): ACPI failure: %d\n",
+ cmd, (int)input.length, input_args, ret);
+ return status;
+ }
+
+ obj = output.pointer;
+ if (!obj) {
+ dev_warn(dev, "cmd %02x (%*ph): no output\n",
+ cmd, (int)input.length, input_args);
+ return -EINVAL;
+ }
+
+ if (obj->type == ACPI_TYPE_BUFFER) {
+ if (obj->buffer.length < NUM_OUTPUT_ARGS + 1) {
+ ret = -EINVAL;
+ goto err;
+ }
+ p = (u8 *)obj->buffer.pointer;
+ } else if (obj->type == ACPI_TYPE_INTEGER) {
+ p = (u8 *)&obj->integer.value;
+ } else {
+ return -EINVAL;
+ }
+
+ ret = nuc_nmi_led_error(p[0]);
+ if (ret) {
+ dev_warn(dev, "cmd %02x (%*ph): WMI error code: %02x\n",
+ cmd, (int)input.length, input_args, p[0]);
+ goto err;
+ }
+
+ size = NUM_OUTPUT_ARGS + 1;
+
+ if (output_args) {
+ memcpy(output_args, p + 1, NUM_OUTPUT_ARGS);
+
+ dev_info(dev, "cmd %02x (%*ph), return: %*ph\n",
+ cmd, (int)input.length, input_args, NUM_OUTPUT_ARGS, output_args);
+ } else {
+ dev_info(dev, "cmd %02x (%*ph)\n",
+ cmd, (int)input.length, input_args);
+ }
+
+err:
+ kfree(obj);
+ return ret;
+}
+
+static int nuc_wmi_query_leds(struct device *dev)
+{
+ struct nuc_wmi *priv = dev_get_drvdata(dev);
+ u8 cmd, input[NUM_INPUT_ARGS] = { 0 };
+ u8 output[NUM_OUTPUT_ARGS];
+ int i, id, ret;
+ u8 leds;
+
+ /*
+ * List all LED types support in the platform
+ *
+ * Should work with both NUC8iXXX and NUC10iXXX
+ *
+ * FIXME: Should add a fallback code for it to work with older NUCs,
+ * as LED_QUERY returns an error on older devices like Skull Canyon.
+ */
+ cmd = LED_QUERY;
+ input[0] = LED_QUERY_LIST_ALL;
+ ret = nuc_nmi_cmd(dev, cmd, input, output);
+ if (ret) {
+ dev_warn(dev, "error %d while listing all LEDs\n", ret);
+ return ret;
+ }
+
+ leds = output[0];
+ if (!leds) {
+ dev_warn(dev, "No LEDs found\n");
+ return -ENODEV;
+ }
+
+ for (id = 0; id < MAX_LEDS; id++) {
+ struct nuc_nmi_led *led = &priv->led[priv->num_leds];
+
+ if (!(leds & BIT(id)))
+ continue;
+
+ led->id = id;
+
+ cmd = LED_QUERY;
+ input[0] = LED_QUERY_COLOR_TYPE;
+ input[1] = id;
+ ret = nuc_nmi_cmd(dev, cmd, input, output);
+ if (ret) {
+ dev_warn(dev, "error %d on led %i\n", ret, i);
+ return ret;
+ }
+
+ led->color_type = output[0] |
+ output[1] << 8 |
+ output[2] << 16;
+
+ cmd = LED_NEW_GET_STATUS;
+ input[0] = LED_NEW_GET_CURRENT_INDICATOR;
+ input[1] = i;
+ ret = nuc_nmi_cmd(dev, cmd, input, output);
+ if (ret) {
+ dev_warn(dev, "error %d on led %i\n", ret, i);
+ return ret;
+ }
+
+ led->indicator = output[0];
+
+ cmd = LED_QUERY;
+ input[0] = LED_QUERY_INDICATOR_OPTIONS;
+ input[1] = i;
+ ret = nuc_nmi_cmd(dev, cmd, input, output);
+ if (ret) {
+ dev_warn(dev, "error %d on led %i\n", ret, i);
+ return ret;
+ }
+
+ led->avail_indicators = output[0] |
+ output[1] << 8 |
+ output[2] << 16;
+
+ cmd = LED_QUERY;
+ input[0] = LED_QUERY_CONTROL_ITEMS;
+ input[1] = i;
+ input[2] = led->indicator;
+ ret = nuc_nmi_cmd(dev, cmd, input, output);
+ if (ret) {
+ dev_warn(dev, "error %d on led %i\n", ret, i);
+ return ret;
+ }
+
+ led->control_items = output[0] |
+ output[1] << 8 |
+ output[2] << 16;
+
+ dev_dbg(dev, "%s: id: %02x, color type: %06x, indicator: %06x, control items: %06x\n",
+ led_names[led->id], led->id,
+ led->color_type, led->indicator, led->control_items);
+
+ priv->num_leds++;
+ }
+
+ return 0;
+}
+
+/*
+ * LED show/store routines
+ */
+
+#define LED_ATTR_RW(_name) \
+ DEVICE_ATTR(_name, 0644, show_##_name, store_##_name)
+
+static const char * const led_indicators[] = {
+ "Power State",
+ "HDD Activity",
+ "Ethernet",
+ "WiFi",
+ "Software",
+ "Power Limit",
+ "Disable"
+};
+
+static ssize_t show_indicator(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev);
+ int size = PAGE_SIZE;
+ char *p = buf;
+ int i, n;
+
+ for (i = 0; i < fls(led->avail_indicators); i++) {
+ if (!(led->avail_indicators & BIT(i)))
+ continue;
+ if (i == led->indicator)
+ n = scnprintf(p, size, "[%s] ", led_indicators[i]);
+ else
+ n = scnprintf(p, size, "%s ", led_indicators[i]);
+ p += n;
+ size -= n;
+ }
+ size -= scnprintf(p, size, "\n");
+
+ return PAGE_SIZE - size;
+}
+
+static ssize_t store_indicator(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev);
+ u8 cmd, input[NUM_INPUT_ARGS] = { 0 };
+ const char *tmp;
+ int ret, i;
+
+ tmp = strsep((char **)&buf, "\n");
+
+ for (i = 0; i < fls(led->avail_indicators); i++) {
+ if (!(led->avail_indicators & BIT(i)))
+ continue;
+
+ if (!strcasecmp(tmp, led_indicators[i])) {
+ cmd = LED_SET_INDICATOR;
+ input[0] = led->id;
+ input[1] = i;
+
+ dev_dbg(dev, "set led %s indicator to %s\n",
+ cdev->name, led_indicators[i]);
+
+ ret = nuc_nmi_cmd(dev, cmd, input, NULL);
+ if (ret)
+ return ret;
+
+ led->indicator = i;
+
+ return len;
+ }
+ }
+
+ return -EINVAL;
+}
+
+static LED_ATTR_RW(indicator);
+
+/*
+ * Attributes for multicolor LEDs
+ */
+
+static struct attribute *nuc_wmi_multicolor_led_attr[] = {
+ &dev_attr_indicator.attr,
+ NULL,
+};
+
+static const struct attribute_group nuc_wmi_led_attribute_group = {
+ .attrs = nuc_wmi_multicolor_led_attr,
+};
+
+static const struct attribute_group *nuc_wmi_led_attribute_groups[] = {
+ &nuc_wmi_led_attribute_group,
+ NULL
+};
+
+static int nuc_wmi_led_register(struct device *dev, struct nuc_nmi_led *led)
+{
+ led->cdev.name = led_names[led->id];
+
+ led->dev = dev;
+ led->cdev.groups = nuc_wmi_led_attribute_groups;
+
+ /*
+ * It can't let the classdev to manage the brightness due to several
+ * reasons:
+ *
+ * 1) classdev has some internal logic to manage the brightness,
+ * at set_brightness_delayed(), which starts disabling the LEDs;
+ * While this makes sense on most cases, here, it would appear
+ * that the NUC was powered off, which is not what happens;
+ * 2) classdev unconditionally tries to set brightness for all
+ * leds, including the ones that were software-disabled or
+ * disabled disabled via BIOS menu;
+ * 3) There are 3 types of brightness values for each LED, depending
+ * on the CPU power state: S0, S3 and S5.
+ *
+ * So, the best seems to export everything via sysfs attributes
+ * directly. This would require some further changes at the
+ * LED class, though, or we would need to create our own LED
+ * class, which seems wrong.
+ */
+
+ return devm_led_classdev_register(dev, &led->cdev);
+}
+
+static int nuc_wmi_leds_setup(struct device *dev)
+{
+ struct nuc_wmi *priv = dev_get_drvdata(dev);
+ int ret, i;
+
+ ret = nuc_wmi_query_leds(dev);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < priv->num_leds; i++) {
+ ret = nuc_wmi_led_register(dev, &priv->led[i]);
+ if (ret) {
+ dev_err(dev, "Failed to register led %d: %s\n",
+ i, led_names[priv->led[i].id]);
+ while (--i >= 0)
+ devm_led_classdev_unregister(dev, &priv->led[i].cdev);
+
+ return ret;
+ }
+ }
+ return 0;
+}
+
+static int nuc_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct device *dev = &wdev->dev;
+ struct nuc_wmi *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ mutex_init(&priv->wmi_lock);
+
+ dev_set_drvdata(dev, priv);
+
+ ret = nuc_wmi_leds_setup(dev);
+ if (ret)
+ return ret;
+
+ dev_info(dev, "NUC WMI driver initialized.\n");
+ return 0;
+}
+
+static const struct wmi_device_id nuc_wmi_descriptor_id_table[] = {
+ { .guid_string = NUC_LED_WMI_GUID },
+ { },
+};
+
+static struct wmi_driver nuc_wmi_driver = {
+ .driver = {
+ .name = "nuc-wmi",
+ },
+ .probe = nuc_wmi_probe,
+ .id_table = nuc_wmi_descriptor_id_table,
+};
+
+module_wmi_driver(nuc_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, nuc_wmi_descriptor_id_table);
+MODULE_AUTHOR("Mauro Carvalho Chehab <[email protected]>");
+MODULE_DESCRIPTION("Intel NUC WMI LED driver");
+MODULE_LICENSE("GPL");
--
2.31.1
Hi!
> Some models come with single colored or dual-colored LEDs, but high end models
> have RGB LEDs.
>
> Programming them can ether be done via BIOS or by the OS, however, BIOS settings
> are limited. So, the vendor offers a Windows application that allows to fully use the
> functionality provided by the firmware/hardware.
I'm not sure why you are submitting v2 in the middle of interface
discussion.
Marek and I are saying the same thing -- this needs to use close to
existing APIs.
If you want to get something merged quickly, please submit basic
functionality only (toggling the LED on/off) that completely fits
existing APIs. We can review that.
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
Em Wed, 19 May 2021 13:11:07 +0200
Pavel Machek <[email protected]> escreveu:
> Hi!
>
> > Some models come with single colored or dual-colored LEDs, but high end models
> > have RGB LEDs.
> >
> > Programming them can ether be done via BIOS or by the OS, however, BIOS settings
> > are limited. So, the vendor offers a Windows application that allows to fully use the
> > functionality provided by the firmware/hardware.
>
> I'm not sure why you are submitting v2 in the middle of interface
> discussion.
I'll refrain sending a new version while we're discussing the interface.
> Marek and I are saying the same thing -- this needs to use close to
> existing APIs.
Ok, but I'm not seeing an existing API that provides what those
LEDs need.
> If you want to get something merged quickly, please submit basic
> functionality only (toggling the LED on/off) that completely fits
> existing APIs. We can review that.
If you prefer working this way, I can send an initial patch with
just the very basic. Actually, if you apply just patch 2 of this
series, it will provide support for for just setting the brightness
on NUC8.
However, the main reason why someone (including myself) want this
driver is to allow to dynamically change what hardware event will
be triggering the LED and how, and if suspend will blink or not[1].
Being able to also change the LED color is a plus.
[1] Disabling blink at suspend/hibernate is one of the things that
I use here: as the machine is at my bedroom, I don't want it to be
blinking all night long when the machine is sleeping :-)
Thanks,
Mauro
Hi!
> > Marek and I are saying the same thing -- this needs to use close to
> > existing APIs.
>
> Ok, but I'm not seeing an existing API that provides what those
> LEDs need.
Well, there "close to" part comes into play.
> > If you want to get something merged quickly, please submit basic
> > functionality only (toggling the LED on/off) that completely fits
> > existing APIs. We can review that.
>
> If you prefer working this way, I can send an initial patch with
> just the very basic. Actually, if you apply just patch 2 of this
> series, it will provide support for for just setting the brightness
> on NUC8.
I don't care much. We can discuss minimal interface additions
neccessary to support your usecases.
But what you proposed was nowhere near close.
Note that we don't want to support every crazy feature, just because
hardware can do it.
> However, the main reason why someone (including myself) want this
> driver is to allow to dynamically change what hardware event will
> be triggering the LED and how, and if suspend will blink or not[1].
> Being able to also change the LED color is a plus.
This one is hard if the LED does not support full color.
> [1] Disabling blink at suspend/hibernate is one of the things that
> I use here: as the machine is at my bedroom, I don't want it to be
> blinking all night long when the machine is sleeping :-)
Ok, so lets start with the blink at suspend thing?
Having power LED on when machine is on, and slowly "breathing" when
machine is suspended is something I have seen before. Is that what
your hardware is doing?
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
Em Wed, 19 May 2021 21:41:15 +0200
Pavel Machek <[email protected]> escreveu:
> Hi!
>
> > > Marek and I are saying the same thing -- this needs to use close to
> > > existing APIs.
> >
> > Ok, but I'm not seeing an existing API that provides what those
> > LEDs need.
>
> Well, there "close to" part comes into play.
>
> > > If you want to get something merged quickly, please submit basic
> > > functionality only (toggling the LED on/off) that completely fits
> > > existing APIs. We can review that.
> >
> > If you prefer working this way, I can send an initial patch with
> > just the very basic. Actually, if you apply just patch 2 of this
> > series, it will provide support for for just setting the brightness
> > on NUC8.
>
> I don't care much. We can discuss minimal interface additions
> neccessary to support your usecases.
>
> But what you proposed was nowhere near close.
Ok. Let's try to come into an agreement about what's needed.
On my discussions with Marek, it sounds to me that the features
from the trigger API won't fit, as the attributes there won't
be supported by the NUC leds (and vice-versa).
Yet, we could try to have something that would look similar.
>
> Note that we don't want to support every crazy feature, just because
> hardware can do it.
Neither do I ;-)
I don't care much for Software controlled LEDs, nor for a feature
that would allow the BIOS to "hide" the LED colors as if it were
a single colored one, for instance.
Yet, the remaining stuff seems pretty much OK.
>
> > However, the main reason why someone (including myself) want this
> > driver is to allow to dynamically change what hardware event will
> > be triggering the LED and how, and if suspend will blink or not[1].
>
> > Being able to also change the LED color is a plus.
>
> This one is hard if the LED does not support full color.
Only a subset of devices support RGB colors, but the API has support
for dual-color and 8-color LEDs. On those, the color is selected like
an enum.
The NUC8 device I use her has RGB color LEDs.
>
> > [1] Disabling blink at suspend/hibernate is one of the things that
> > I use here: as the machine is at my bedroom, I don't want it to be
> > blinking all night long when the machine is sleeping :-)
>
> Ok, so lets start with the blink at suspend thing?
Yeah, that sounds to be a good start point.
>
> Having power LED on when machine is on, and slowly "breathing" when
> machine is suspended is something I have seen before. Is that what
> your hardware is doing?
Yes, but see: my device has 6 leds (API supports up to 7 leds).
Any of them can be programmed to act as a power LED at runtime.
So, the first thing that the API needs is a way to tell what LED
is monitoring the device's power state.
Then, for each power state (S0, S3, S5), define if the LED will
be ON all the times or not.
The "slowing breathing" is one of the possible blink patterns.
The driver supports 4 other blink patterns
- Solid - the LED won't blink;
- Breathing - it looks like a sinusoidal wave pattern;
- Pulsing - it looks like a square wave pattern;
- Strobing - it turns ON suddenly, and then it slowly turns OFF.
The speed of the blink is also adjustable, ranging from 0.1 Hz to 1 Hz,
on 0.1 Hz steps.
---
Let me explain this specific part of the API from my original proposal.
Those are the led names from the datasheets (NUC 8 and above),
and my proposal for the sysfs class directory name:
============= ===============================
LED name sysfs
============= ===============================
Skull ``/sys/class/leds/nuc::skull``
Skull eyes ``/sys/class/leds/nuc::eyes``
Power ``/sys/class/leds/nuc::power``
HDD ``/sys/class/leds/nuc::hdd``
Front1 ``/sys/class/leds/nuc::front1``
Front2 ``/sys/class/leds/nuc::front2``
Front3 ``/sys/class/leds/nuc::front3``
============= ===============================
For each of the above, there's the need to identify what
hardware function is monitored (if any).
My proposal were to add an "indicator" node (the name came from
the Intel datasheets) that shows what led will monitor the power state.
Then, one blink_behavior and one blink_frequency per power state,
e. g.:
/sys/class/leds/nuc::front1
|-- indicator
|-- s0_blink_behavior
|-- s0_blink_frequency
|-- s3_blink_behavior
|-- s3_blink_frequency
|-- s5_blink_behavior
`-- s5_blink_frequency
PS.: I don't care much about what names we'll use. Feel free to
rename them, if you think the above is not clear or generic enough.
-
To make part of the API complete, there's also the need of a node
to control the max brightness that the leds will achieve at the
ON state, and another one to control the color on each state,
as one could define, let's say, "white" when powered on, "blue"
when suspended and "yellow" when hibernating. The colors at the
NUC I have are RGB (but other models can use an enum for the
supported colors).
/sys/class/leds/nuc::front1
|-- s0_brightness
|-- s0_color # only shown on colored leds
|-- s3_brightness
|-- s3_color # only shown on colored leds
|-- s0_brightness
`-- s5_color # only shown on colored leds
Thanks,
Mauro
On Thu, 20 May 2021 01:07:20 +0200
Mauro Carvalho Chehab <[email protected]> wrote:
> So, the first thing that the API needs is a way to tell what LED
> is monitoring the device's power state.
If a LED can monitor the device's power state in HW, register a LED
private trigger for this LED. If the LED is configured into this state
by default, you can set this trigger to be the default_trigger prior
registering the LED. The name of this private trigger can be
"hw:powerstate" or something like that (I wonder what others will
think about this name).
> Then, for each power state (S0, S3, S5), define if the LED will
> be ON all the times or not.
>
> The "slowing breathing" is one of the possible blink patterns.
> The driver supports 4 other blink patterns
>
> - Solid - the LED won't blink;
> - Breathing - it looks like a sinusoidal wave pattern;
> - Pulsing - it looks like a square wave pattern;
> - Strobing - it turns ON suddenly, and then it slowly turns OFF.
>
> The speed of the blink is also adjustable, ranging from 0.1 Hz to 1 Hz,
> on 0.1 Hz steps.
Is the speed of breathing/strobing also adjustable? Or only when
pulsing?
When this "hw:powerstate" trigger is enabled for this LED,
only then another sysfs files should appear in this LED's sysfs
directory.
> ---
>
> Let me explain this specific part of the API from my original proposal.
>
> Those are the led names from the datasheets (NUC 8 and above),
> and my proposal for the sysfs class directory name:
>
> ============= ===============================
> LED name sysfs
> ============= ===============================
> Skull ``/sys/class/leds/nuc::skull``
> Skull eyes ``/sys/class/leds/nuc::eyes``
> Power ``/sys/class/leds/nuc::power``
> HDD ``/sys/class/leds/nuc::hdd``
> Front1 ``/sys/class/leds/nuc::front1``
> Front2 ``/sys/class/leds/nuc::front2``
> Front3 ``/sys/class/leds/nuc::front3``
> ============= ===============================
>
> For each of the above, there's the need to identify what
> hardware function is monitored (if any).
>
> My proposal were to add an "indicator" node (the name came from
> the Intel datasheets) that shows what led will monitor the power state.
>
> Then, one blink_behavior and one blink_frequency per power state,
> e. g.:
>
> /sys/class/leds/nuc::front1
> |-- indicator
> |-- s0_blink_behavior
> |-- s0_blink_frequency
> |-- s3_blink_behavior
> |-- s3_blink_frequency
> |-- s5_blink_behavior
> `-- s5_blink_frequency
I'd rather use one file for frequencies and one for intervals, and map
in to an array, but that is just my preference...
>
> PS.: I don't care much about what names we'll use. Feel free to
> rename them, if you think the above is not clear or generic enough.
>
> -
>
> To make part of the API complete, there's also the need of a node
> to control the max brightness that the leds will achieve at the
> ON state, and another one to control the color on each state,
> as one could define, let's say, "white" when powered on, "blue"
> when suspended and "yellow" when hibernating. The colors at the
> NUC I have are RGB (but other models can use an enum for the
> supported colors).
>
> /sys/class/leds/nuc::front1
> |-- s0_brightness
> |-- s0_color # only shown on colored leds
> |-- s3_brightness
> |-- s3_color # only shown on colored leds
> |-- s0_brightness
> `-- s5_color # only shown on colored leds
If the BIOS reports a LED being full RGB LED, you should register it
via multicolor framework. Regarding the enum with 8 colors: are these
colors red, yellow, green, cyan, blue, magenta? Because if so, then
this is RGB with each channel being binary :) So you can again use
multicolor framework.
Em Thu, 20 May 2021 21:43:56 +0200
Marek Behún <[email protected]> escreveu:
> On Thu, 20 May 2021 21:16:15 +0200
> Mauro Carvalho Chehab <[email protected]> wrote:
>
> > So, assuming that we will have one trigger per each hardware
> > state, it could have something like (names subject to change):
> >
> > - hw:powerstate
> > - hw:disk_activity
> > - hw:ethernet_activity
> > - hw:wifi_active
> > - hw:power_limit
> >
> > Right?
>
> Yes, but we should really try to map ethernet_activity to netdev and
> disk_activity to a potential blkdev trigger :-) That's my opinion.
>
> > It still needs to indicate two other possible states:
> >
> > - software controlled led;
> > - led is disabled.
> >
> > Setting led's brightness to zero is different than disabling
> > it.
> >
> > Disabling can be done via BIOS, but BIOS config doesn't allow
> > setting the brightness. There are other difference on BIOS settings:
> > it allow disabling each/all LED controls and/or to disable software
> > control of each LED.
> >
> > So, we need a way at the API to uniquely identify when the LED
> > is software-controlled and when it is disabled.
> > Would it be something like:
> >
> > - hw:disable
> >
> > trigger? or better to implement it on a different way?
>
> What is the functional difference (visible to the user) between zero
> brightness and disabled LED? IMO if user says
> echo 0 >brightness
> you can just disable the LED. Or is this impossible?
echo 0 >brightness will turn off the LED, but it won't
disable it. A trigger can still be enabled on it.
With a disabled LED, depending on how it was disabled,
it can't be enabled in runtime. One may need to boot the
machine and use BIOS setup to enable it. Trying to change
such LED in runtime will return an error.
>
> > > Is the speed of breathing/strobing also adjustable? Or only when
> > > pulsing?
> >
> > Yes, speed is also adjustable, from 0.1 to 1.0 HZ, in 0.1 Hz
> > (NUC 8 and above).
> >
> > The NUC6 API is more limited than NUC8+: it has just two
> > blink patterns (blink, fade), and only 3 frequencies are allowed
> > (0.25 Hz, 0.50 Hz and 1.0 Hz).
> >
> > > When this "hw:powerstate" trigger is enabled for this LED,
> > > only then another sysfs files should appear in this LED's sysfs
> > > directory.
> >
> > OK, makes sense.
> >
> > Out of curiosity: is it reliable to make sysfs nodes appear and
> > disappear dynamically? Does inotify (or something similar) can
> > be used to identify when such nodes appear/disappear?
> >
> > I remember a long time ago I wanted to use something like that
> > at the media (or edac?) subsystem, but someone (Greg, I think)
> > recommended otherwise due to some potential racing issues.
>
> No idea, but I would guess yes.
>
> > > I'd rather use one file for frequencies and one for intervals, and map
> > > in to an array, but that is just my preference...
> >
> > By intervals are you meaning 1/frequency? So, basically exposing
> > the frequency as two fields? If so, it sounds overkill to me to have both.
>
> Sorry, I meant one file for frequencies and one for patterns.
Ah, makes sense. Yeah, that's how I mapped it.
> > Btw, maybe instead of "blink_behavior" it could use "blink_pattern".
> >
> > This would diverge from the datahseet name, but it probably describes
> > better what will be controlled when blink is enabled:
> >
> > - frequency (or inverval)
> > - pattern
> >
> > > Regarding the enum with 8 colors: are these
> > > colors red, yellow, green, cyan, blue, magenta? Because if so, then
> > > this is RGB with each channel being binary :) So you can again use
> > > multicolor framework.
> >
> > The dual-colored ones aren't RGB. Two types are supported:
> > - Blue/Amber
> > - Blue/White
>
> These would need a new API, ignore these for now.
This affects mainly NUC6 part of the API. I'll postpone it.
Yet, IMHO, the best here is to do exactly how I did: use the
"normal" leds class and add a "color" attribute that can
either be "blue" or "amber" written on it (for a blue/amber
kind of LED).
Thanks,
Mauro
On Thu, 20 May 2021 21:16:15 +0200
Mauro Carvalho Chehab <[email protected]> wrote:
> So, assuming that we will have one trigger per each hardware
> state, it could have something like (names subject to change):
>
> - hw:powerstate
> - hw:disk_activity
> - hw:ethernet_activity
> - hw:wifi_active
> - hw:power_limit
>
> Right?
Yes, but we should really try to map ethernet_activity to netdev and
disk_activity to a potential blkdev trigger :-) That's my opinion.
> It still needs to indicate two other possible states:
>
> - software controlled led;
> - led is disabled.
>
> Setting led's brightness to zero is different than disabling
> it.
>
> Disabling can be done via BIOS, but BIOS config doesn't allow
> setting the brightness. There are other difference on BIOS settings:
> it allow disabling each/all LED controls and/or to disable software
> control of each LED.
>
> So, we need a way at the API to uniquely identify when the LED
> is software-controlled and when it is disabled.
> Would it be something like:
>
> - hw:disable
>
> trigger? or better to implement it on a different way?
What is the functional difference (visible to the user) between zero
brightness and disabled LED? IMO if user says
echo 0 >brightness
you can just disable the LED. Or is this impossible?
> > Is the speed of breathing/strobing also adjustable? Or only when
> > pulsing?
>
> Yes, speed is also adjustable, from 0.1 to 1.0 HZ, in 0.1 Hz
> (NUC 8 and above).
>
> The NUC6 API is more limited than NUC8+: it has just two
> blink patterns (blink, fade), and only 3 frequencies are allowed
> (0.25 Hz, 0.50 Hz and 1.0 Hz).
>
> > When this "hw:powerstate" trigger is enabled for this LED,
> > only then another sysfs files should appear in this LED's sysfs
> > directory.
>
> OK, makes sense.
>
> Out of curiosity: is it reliable to make sysfs nodes appear and
> disappear dynamically? Does inotify (or something similar) can
> be used to identify when such nodes appear/disappear?
>
> I remember a long time ago I wanted to use something like that
> at the media (or edac?) subsystem, but someone (Greg, I think)
> recommended otherwise due to some potential racing issues.
No idea, but I would guess yes.
> > I'd rather use one file for frequencies and one for intervals, and map
> > in to an array, but that is just my preference...
>
> By intervals are you meaning 1/frequency? So, basically exposing
> the frequency as two fields? If so, it sounds overkill to me to have both.
Sorry, I meant one file for frequencies and one for patterns.
>
> Btw, maybe instead of "blink_behavior" it could use "blink_pattern".
>
> This would diverge from the datahseet name, but it probably describes
> better what will be controlled when blink is enabled:
>
> - frequency (or inverval)
> - pattern
>
> > Regarding the enum with 8 colors: are these
> > colors red, yellow, green, cyan, blue, magenta? Because if so, then
> > this is RGB with each channel being binary :) So you can again use
> > multicolor framework.
>
> The dual-colored ones aren't RGB. Two types are supported:
> - Blue/Amber
> - Blue/White
These would need a new API, ignore these for now.
Marek
Em Thu, 20 May 2021 18:19:19 +0200
Marek Behún <[email protected]> escreveu:
> On Thu, 20 May 2021 01:07:20 +0200
> Mauro Carvalho Chehab <[email protected]> wrote:
>
> > So, the first thing that the API needs is a way to tell what LED
> > is monitoring the device's power state.
>
> If a LED can monitor the device's power state in HW, register a LED
> private trigger for this LED. If the LED is configured into this state
> by default, you can set this trigger to be the default_trigger prior
> registering the LED. The name of this private trigger can be
> "hw:powerstate" or something like that (I wonder what others will
> think about this name).
Ok.
So, assuming that we will have one trigger per each hardware
state, it could have something like (names subject to change):
- hw:powerstate
- hw:disk_activity
- hw:ethernet_activity
- hw:wifi_active
- hw:power_limit
Right?
It still needs to indicate two other possible states:
- software controlled led;
- led is disabled.
Setting led's brightness to zero is different than disabling
it.
Disabling can be done via BIOS, but BIOS config doesn't allow
setting the brightness. There are other difference on BIOS settings:
it allow disabling each/all LED controls and/or to disable software
control of each LED.
So, we need a way at the API to uniquely identify when the LED
is software-controlled and when it is disabled.
Would it be something like:
- hw:disable
trigger? or better to implement it on a different way?
> > Then, for each power state (S0, S3, S5), define if the LED will
> > be ON all the times or not.
> >
> > The "slowing breathing" is one of the possible blink patterns.
> > The driver supports 4 other blink patterns
> >
> > - Solid - the LED won't blink;
> > - Breathing - it looks like a sinusoidal wave pattern;
> > - Pulsing - it looks like a square wave pattern;
> > - Strobing - it turns ON suddenly, and then it slowly turns OFF.
> >
> > The speed of the blink is also adjustable, ranging from 0.1 Hz to 1 Hz,
> > on 0.1 Hz steps.
>
> Is the speed of breathing/strobing also adjustable? Or only when
> pulsing?
Yes, speed is also adjustable, from 0.1 to 1.0 HZ, in 0.1 Hz
(NUC 8 and above).
The NUC6 API is more limited than NUC8+: it has just two
blink patterns (blink, fade), and only 3 frequencies are allowed
(0.25 Hz, 0.50 Hz and 1.0 Hz).
> When this "hw:powerstate" trigger is enabled for this LED,
> only then another sysfs files should appear in this LED's sysfs
> directory.
OK, makes sense.
Out of curiosity: is it reliable to make sysfs nodes appear and
disappear dynamically? Does inotify (or something similar) can
be used to identify when such nodes appear/disappear?
I remember a long time ago I wanted to use something like that
at the media (or edac?) subsystem, but someone (Greg, I think)
recommended otherwise due to some potential racing issues.
>
> > ---
> >
> > Let me explain this specific part of the API from my original proposal.
> >
> > Those are the led names from the datasheets (NUC 8 and above),
> > and my proposal for the sysfs class directory name:
> >
> > ============= ===============================
> > LED name sysfs
> > ============= ===============================
> > Skull ``/sys/class/leds/nuc::skull``
> > Skull eyes ``/sys/class/leds/nuc::eyes``
> > Power ``/sys/class/leds/nuc::power``
> > HDD ``/sys/class/leds/nuc::hdd``
> > Front1 ``/sys/class/leds/nuc::front1``
> > Front2 ``/sys/class/leds/nuc::front2``
> > Front3 ``/sys/class/leds/nuc::front3``
> > ============= ===============================
> >
> > For each of the above, there's the need to identify what
> > hardware function is monitored (if any).
> >
> > My proposal were to add an "indicator" node (the name came from
> > the Intel datasheets) that shows what led will monitor the power state.
> >
> > Then, one blink_behavior and one blink_frequency per power state,
> > e. g.:
> >
> > /sys/class/leds/nuc::front1
> > |-- indicator
> > |-- s0_blink_behavior
> > |-- s0_blink_frequency
> > |-- s3_blink_behavior
> > |-- s3_blink_frequency
> > |-- s5_blink_behavior
> > `-- s5_blink_frequency
>
> I'd rather use one file for frequencies and one for intervals, and map
> in to an array, but that is just my preference...
By intervals are you meaning 1/frequency? So, basically exposing
the frequency as two fields? If so, it sounds overkill to me to have both.
Btw, maybe instead of "blink_behavior" it could use "blink_pattern".
This would diverge from the datahseet name, but it probably describes
better what will be controlled when blink is enabled:
- frequency (or inverval)
- pattern
>
> >
> > PS.: I don't care much about what names we'll use. Feel free to
> > rename them, if you think the above is not clear or generic enough.
> >
> > -
> >
> > To make part of the API complete, there's also the need of a node
> > to control the max brightness that the leds will achieve at the
> > ON state, and another one to control the color on each state,
> > as one could define, let's say, "white" when powered on, "blue"
> > when suspended and "yellow" when hibernating. The colors at the
> > NUC I have are RGB (but other models can use an enum for the
> > supported colors).
> >
> > /sys/class/leds/nuc::front1
> > |-- s0_brightness
> > |-- s0_color # only shown on colored leds
> > |-- s3_brightness
> > |-- s3_color # only shown on colored leds
> > |-- s0_brightness
> > `-- s5_color # only shown on colored leds
>
> If the BIOS reports a LED being full RGB LED, you should register it
> via multicolor framework.
OK.
> Regarding the enum with 8 colors: are these
> colors red, yellow, green, cyan, blue, magenta? Because if so, then
> this is RGB with each channel being binary :) So you can again use
> multicolor framework.
The dual-colored ones aren't RGB. Two types are supported:
- Blue/Amber
- Blue/White
the only one with 8 colors is at NUC6 API: the ring led. This one can be mapped
as RGB with 1 bit per color, as those are the colors:
+---------+
| disable |
+---------+
| cyan |
+---------+
| pink |
+---------+
| yellow |
+---------+
| blue |
+---------+
| red |
+---------+
| green |
+---------+
| white |
+---------+
Thanks,
Mauro