* [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC @ 2021-05-18 15:08 Mauro Carvalho Chehab 2021-05-18 15:08 ` [PATCH v2 01/17] docs: describe the API used to set NUC LEDs Mauro Carvalho Chehab 2021-05-19 11:11 ` [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC Pavel Machek 0 siblings, 2 replies; 10+ messages in thread From: Mauro Carvalho Chehab @ 2021-05-18 15:08 UTC (permalink / raw) Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, gregkh, Pavel Machek, linux-doc, linux-kernel, linux-leds 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 01/17] docs: describe the API used to set NUC LEDs 2021-05-18 15:08 [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC Mauro Carvalho Chehab @ 2021-05-18 15:08 ` Mauro Carvalho Chehab 2021-05-19 11:11 ` [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC Pavel Machek 1 sibling, 0 replies; 10+ messages in thread From: Mauro Carvalho Chehab @ 2021-05-18 15:08 UTC (permalink / raw) Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Dan Murphy, Greg Kroah-Hartman, Jacek Anaszewski, Jonathan Corbet, linux-doc, linux-kernel Some NUC6 have LEDs that can be configurated dynamically from the operational system, via WMI. Describe how the API for such devices should work. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- Documentation/leds/index.rst | 1 + Documentation/leds/leds-nuc.rst | 447 ++++++++++++++++++++++++++++++++ 2 files changed, 448 insertions(+) create mode 100644 Documentation/leds/leds-nuc.rst diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst index e5d63b940045..4fdf9b60bb86 100644 --- a/Documentation/leds/index.rst +++ b/Documentation/leds/index.rst @@ -25,4 +25,5 @@ LEDs leds-lp5562 leds-lp55xx leds-mlxcpld + leds-nuc leds-sc27xx diff --git a/Documentation/leds/leds-nuc.rst b/Documentation/leds/leds-nuc.rst new file mode 100644 index 000000000000..02e1c2602dd3 --- /dev/null +++ b/Documentation/leds/leds-nuc.rst @@ -0,0 +1,447 @@ +================== +Intel NUC WMI LEDs +================== + +Some models of the Intel Next Unit of Computing (NUC) may have programmable +LEDs. Those can be partially programmed by opening the BIOS configuration. + +Among those models, some of them also allows the Operational System to +adjust the LED parameters via a firmware interface, called Windows Management +Interface - WMI. + +There are currently three different versions of WMI API for NUC, depending +on the NUC generation: + +For NUC 6 and 7, the WMI API is defined at: + + - https://www.intel.com/content/www/us/en/support/articles/000023426/intel-nuc/intel-nuc-kits.html + +For NUC 8 and 8, the WMI API is defined at: + + - https://raw.githubusercontent.com/nomego/intel_nuc_led/master/specs/INTEL_WMI_LED_0.64.pdf + +For NUC 10 and newer generations, the WMI API is defined at: + - https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf + +The nuc-wmi driver provides userspace support for changing the LED +configuration, and supports WMI API since NUC 6. Yet, the NUC6 WMI API +functionality is limited when compared with the newer NUC generations. + +Although there are some internal differences, the features supported for +NUC 10 WMI API are almost identical to the ones supported by NUC 8 WMI API. + +.. note:: + + Even when the firmware supports setting LEDs via WMI API, the + BIOS configuration has some parameters to either allow the Operational + System to also control them. Instructions about how to enable it can + be found at the manual of each specific NUC model. + +NUC 6 and NUC 7 +=============== + +When the driver detects NUC LEDs, up to two directories will be created +under sysfs. They're asocciated with the button(s) named "Power" and "Ring". + +Assuming that sysfs is mounted under ``/sys``, those are the +directories: + +============= ============================== +LED name sysfs device directory +============= ============================== +Power ``/sys/class/leds/nuc::power`` +Ring ``/sys/class/leds/nuc::ring`` +============= ============================== + +For each of the above directory, some sysfs nodes will allow to control the +functionality for each button:: + + . + |-- blink_behavior + |-- blink_frequency + |-- brightness + |-- color + |-- device -> ../../../8C5DA44C-CDC3-46B3-8619-4E26D34390B7 + `-- max_brightness + +.. note:: + + 1. any user can read the LEDs parameter; + 2. changing a LED parameter is limited to the owner of the sysfs device + nodes (usually, the ``root`` user); + 3. changing a LED parameter is case-insensitive + +Brightness +---------- + +The ``brightness`` adjusts the LED brightness, and can be set from +0 to ``max_brightness``. + +So, for instance, in order to put the power LED with 50% of the bright:: + + $ cat /sys/class/leds/nuc::power/max_brightness + 100 + # echo 50 > /sys/class/leds/nuc::power/max_brightness + +Color +----- + +On NUC6 API, the power LED color can be be dual colored. Those are +the valid color values: + + +---------+ + | disable | + +---------+ + | blue | + +---------+ + | amber | + +---------+ + +And the ring LED color can be multi colored. Those are the valid color +values: + + +---------+ + | disable | + +---------+ + | cyan | + +---------+ + | pink | + +---------+ + | yellow | + +---------+ + | blue | + +---------+ + | red | + +---------+ + | green | + +---------+ + | white | + +---------+ + +Changing the ring color of the ring LED can be done with:: + + $ cat /sys/class/leds/nuc::ring/color + [disable] cyan pink yellow blue red green white + # echo "cyan" > /sys/class/leds/nuc::ring/color + +Blink behavior and frequency +---------------------------- + +The NUC6 API supports those blink behaviors: + + +-------+ + | Solid | + +-------+ + | Blink | + +-------+ + | Fade | + +-------+ + + +When in blink and/or fade mode, it supports the following frequencies: + + +---------+ + | 1 Hz | + +---------+ + | 0.5 Hz | + +---------+ + | 0.25 Hz | + +---------+ + +Changing the blink behavior of the power LED, for instance, can be done +with:: + + $ cat /sys/class/leds/nuc::power/blink_behavior + [solid] blink fade + $ cat /sys/class/leds/nuc::power/blink_frequency + [1] 0.5 0.25 + # echo "blink" > /sys/class/leds/nuc::power/blink_behavior + # echo 0.5 > /sys/class/leds/nuc::power/blink_frequency + +.. note:: + + The blink/fade behavior and frequencies can support only a subset of + the above values on old BIOS. + +NUC 8 and newer generations +=========================== + +When the driver detects NUC LEDs, up to seven directories will be +created under sysfs. Each one for each different LED. + +Assuming that sysfs is mounted under ``/sys``, those are the +directories: + +============= =============================== +LED name sysfs device node +============= =============================== +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 directory, some sysfs nodes will allow to control the +functionality for each button:: + + /sys/class/leds/nuc::front1 + |-- blink_behavior + |-- blink_frequency + |-- brightness + |-- color + |-- ethernet_type + |-- hdd_default + |-- indicator + |-- max_brightness + |-- power_limit_scheme + |-- ready_mode_blink_behavior + |-- ready_mode_blink_frequency + |-- ready_mode_brightness + |-- s0_blink_behavior + |-- s0_blink_frequency + |-- s0_brightness + |-- s3_blink_behavior + |-- s3_blink_frequency + |-- s3_brightness + |-- s5_blink_behavior + |-- s5_blink_frequency + `-- s5_brightness + +The sessions below will explain the meaning of each aspect of the API. + +.. note:: + + 1. any user can read the LEDs parameter; + 2. changing a LED parameter is limited to the owner of the sysfs device + nodes (usually, the ``root`` user); + 3. changing a LED parameter is case-insensitive; + 4. The LED ``indicator`` parameter controls the function of the LED. + All other parameters can be enabled or disabled in runtime, depending + on it. When a certain parameter is disabled, an error code will be + returned; + 5. The hardware and its firmware actually controls the LED. The interface + provided by the driver just changes the LED settings in runtime. + Such changes can persist even after rebooting. + +LED indicator +------------- + +Despite the LED's name, the LED API may allow them to indicate different +hardware events. + +This is controlled via the ``indicator`` device node. Reading from it displays +all the supported events for a giving LED, and the currently ative one:: + + $ cat /sys/class/leds/nuc::front1/indicator + Power State [HDD Activity] Ethernet WiFi Software Power Limit Disable + +Each LED may support the following indicator types: + + ============== ======================================================= + Indicator type Meaning + ============== ======================================================= + Power State Shows if the device is powered and what power level + it is (e. g. if the device is suspended or not, and + on which kind of suspended level). + HDD Activity Indicates if the LED is measuring the hard disk (or + SDD) activity. + Ethernet Indicates the activity Ethernet adapter(s) + WiFi Indicates if WiFi is enabled + Software Doesn't indicate any hardware level. Instead, the LED + status is controlled via software. + Power Limit Changes the LED color when the computer is throttling + its power limits. + Disable The LED was disabled. + ============== ======================================================= + +In order to change the type of indicator, you should +just write a new value to the indicator type:: + + # echo "wifi" > /sys/class/leds/nuc::front1/indicator + + $ cat /sys/class/leds/nuc::front1/indicator + Power State HDD Activity Ethernet [WiFi] Software Power Limit Disable + + +Power State parameters +---------------------- + +When the LED indicator is measuring *Power State*, the following parameters +may be available: + + ================================= ======================================= + Parameter Meaning + ================================= ======================================= + <power_state>_brightness Brightness in percent (from 0 to 100) + <power_state>_blink_behavior type of blink. + See :ref:`nuc_blink_behavior`. + <power_state>_blink_frequency Blink frequency. + See :ref:`nuc_blink_behavior`. + <power_state>_color LED color + See :ref:`nuc_color`. + ================================= ======================================= + +Where <power_state> can be: + +On NUC8/9 API: + + +------------+ + | S0 | + +------------+ + | S3 | + +------------+ + | S5 | + +------------+ + | Ready mode | + +------------+ + +On NUC10 API: + + +------------+ + | S0 | + +------------+ + | S3 | + +------------+ + | Standby | + +------------+ + +HDD Activity parameters +----------------------- + +When the LED indicator is measuring *HDD Activity*, the following parameters +may be available: + + ================================= ======================================= + Parameter Meaning + ================================= ======================================= + brightness Brightness in percent (from 0 to 100) + color LED color. + See :ref:`nuc_color`. + hdd_default Default is LED turned ON or OFF. + When set toOFF, the LED will turn on + at disk activity. + When set to ON, the LED will be turned + on by default, turning off at disk + activity. + ================================= ======================================= + +Ethernet parameters +------------------- + +When the LED indicator is measuring *Ethernet*, the following parameters +may be available: + + ================================= ======================================= + Parameter Meaning + ================================= ======================================= + brightness Brightness in percent (from 0 to 100) + color LED color. + See :ref:`nuc_color`. + ethernet_type What Ethernet interface is monitored. + Can be: + LAN1, LAN2 or LAN1+LAN2. + ================================= ======================================= + +Power limit parameters +---------------------- + +When the LED indicator is measuring *Power limit*, the following parameters +may be available: + + ================================= ======================================= + Parameter Meaning + ================================= ======================================= + brightness Brightness in percent (from 0 to 100) + color LED color. + See :ref:`nuc_color`. + power_limit_scheme Indication scheme can be either: + - green to red + - single color + ================================= ======================================= + + +.. _nuc_color: + +NUC LED colors +-------------- + +The NUC LED API may support 3 types of LEDs: + +- Mono-colored LEDs; +- Dual-colored LEDs; +- multi-colored LEDs (only on NUC6/7); +- RGB LEDs. + +Also, when a let is set to be a *Power limit* indicator, despite the +physical device's LED color, the API may limit it to be a led that +can display only green and red, or just a single color. + +The ``color`` and ``<power_state>_color`` parameter supports all those +different settings. + +The color parameter can be set to those values: + + ============ ====== ===== ===== + Color name Red Green Blue + ============ ====== ===== ===== + blue 0 0 255 + amber 255 191 0 + white 255 255 255 + red 255 0 0 + green 0 255 0 + yellow 255 255 0 + cyan 0 255 255 + magenta 255 0 255 + <r>,<g>,<b> <r> <g> <b> + ============ ====== ===== ===== + +The color parameter will refuse to set a LED on a color that it is not +supported by the hardware or when the setting is incompatible with the +indicator type. So, when the indicator is set to *Power limit*, and +the ``power_limit_scheme`` is set to ``green to red``, it doesn't +let to set the LED's color. + +On the other hand, the behavior is identical if a color is written using +the color's name or its RGB value. + +So:: + + $ cat /sys/class/leds/nuc::front1/color + red + # echo "green" > /sys/class/leds/nuc::front1/color + $ cat /sys/class/leds/nuc::front1/color + green + # echo "255,0,0" > /sys/class/leds/nuc::front1/color + $ cat /sys/class/leds/nuc::front1/color + red + +.. _nuc_blink_behavior: + +NUC Blink behavior +------------------ + +The NUC LEDs hardware supports the following types of blink behavior: + + +------------+ + | Solid | + +------------+ + | Breathing | + +------------+ + | Pulsing | + +------------+ + | Strobing | + +------------+ + +Changing the blink behavior will change how the led will be turning +on and off when blinking. Setting it to ``Solid`` disables blinking. + +Please notice that not all types of indicator supports blinking. + +When blinking, the blink frequency can be changed via ``blink_frequency`` +or ``<power_state>_blink_frequency``, depending on the indicator. + +Setting it allows to change the blink frequency in Hz, ranging from 0.1 Hz +to 1.0 Hz, in multiples of 0.1 Hz. -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC 2021-05-18 15:08 [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC Mauro Carvalho Chehab 2021-05-18 15:08 ` [PATCH v2 01/17] docs: describe the API used to set NUC LEDs Mauro Carvalho Chehab @ 2021-05-19 11:11 ` Pavel Machek 2021-05-19 12:15 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 10+ messages in thread From: Pavel Machek @ 2021-05-19 11:11 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linuxarm, mauro.chehab, gregkh, linux-doc, linux-kernel, linux-leds [-- Attachment #1: Type: text/plain, Size: 755 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC 2021-05-19 11:11 ` [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC Pavel Machek @ 2021-05-19 12:15 ` Mauro Carvalho Chehab 2021-05-19 19:41 ` Pavel Machek 0 siblings, 1 reply; 10+ messages in thread From: Mauro Carvalho Chehab @ 2021-05-19 12:15 UTC (permalink / raw) To: Pavel Machek Cc: linuxarm, mauro.chehab, gregkh, linux-doc, linux-kernel, linux-leds Em Wed, 19 May 2021 13:11:07 +0200 Pavel Machek <pavel@ucw.cz> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC 2021-05-19 12:15 ` Mauro Carvalho Chehab @ 2021-05-19 19:41 ` Pavel Machek 2021-05-19 23:07 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 10+ messages in thread From: Pavel Machek @ 2021-05-19 19:41 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linuxarm, mauro.chehab, gregkh, linux-doc, linux-kernel, linux-leds [-- Attachment #1: Type: text/plain, Size: 1708 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC 2021-05-19 19:41 ` Pavel Machek @ 2021-05-19 23:07 ` Mauro Carvalho Chehab 2021-05-20 16:19 ` Marek Behún 0 siblings, 1 reply; 10+ messages in thread From: Mauro Carvalho Chehab @ 2021-05-19 23:07 UTC (permalink / raw) To: Pavel Machek Cc: linuxarm, mauro.chehab, gregkh, linux-doc, linux-kernel, linux-leds Em Wed, 19 May 2021 21:41:15 +0200 Pavel Machek <pavel@ucw.cz> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC 2021-05-19 23:07 ` Mauro Carvalho Chehab @ 2021-05-20 16:19 ` Marek Behún 2021-05-20 19:16 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 10+ messages in thread From: Marek Behún @ 2021-05-20 16:19 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Pavel Machek, linuxarm, mauro.chehab, gregkh, linux-doc, linux-kernel, linux-leds On Thu, 20 May 2021 01:07:20 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC 2021-05-20 16:19 ` Marek Behún @ 2021-05-20 19:16 ` Mauro Carvalho Chehab 2021-05-20 19:43 ` Marek Behún 0 siblings, 1 reply; 10+ messages in thread From: Mauro Carvalho Chehab @ 2021-05-20 19:16 UTC (permalink / raw) To: Marek Behún Cc: Pavel Machek, linuxarm, mauro.chehab, gregkh, linux-doc, linux-kernel, linux-leds Em Thu, 20 May 2021 18:19:19 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Thu, 20 May 2021 01:07:20 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC 2021-05-20 19:16 ` Mauro Carvalho Chehab @ 2021-05-20 19:43 ` Marek Behún 2021-05-21 9:57 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 10+ messages in thread From: Marek Behún @ 2021-05-20 19:43 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Pavel Machek, linuxarm, mauro.chehab, gregkh, linux-doc, linux-kernel, linux-leds On Thu, 20 May 2021 21:16:15 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC 2021-05-20 19:43 ` Marek Behún @ 2021-05-21 9:57 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 10+ messages in thread From: Mauro Carvalho Chehab @ 2021-05-21 9:57 UTC (permalink / raw) To: Marek Behún Cc: Pavel Machek, linuxarm, mauro.chehab, gregkh, linux-doc, linux-kernel, linux-leds Em Thu, 20 May 2021 21:43:56 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Thu, 20 May 2021 21:16:15 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-21 9:59 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-18 15:08 [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC Mauro Carvalho Chehab 2021-05-18 15:08 ` [PATCH v2 01/17] docs: describe the API used to set NUC LEDs Mauro Carvalho Chehab 2021-05-19 11:11 ` [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC Pavel Machek 2021-05-19 12:15 ` Mauro Carvalho Chehab 2021-05-19 19:41 ` Pavel Machek 2021-05-19 23:07 ` Mauro Carvalho Chehab 2021-05-20 16:19 ` Marek Behún 2021-05-20 19:16 ` Mauro Carvalho Chehab 2021-05-20 19:43 ` Marek Behún 2021-05-21 9:57 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).