[PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Mauro Carvalho Chehab]

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

[PATCH v2 01/17] docs: describe the API used to set NUC LEDs

[Mauro Carvalho Chehab]

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

Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Pavel Machek]

[-- 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 --]

Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Mauro Carvalho Chehab]

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

Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Pavel Machek]

[-- 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 --]

Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Mauro Carvalho Chehab]

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

Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Marek Behún]

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.

Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Mauro Carvalho Chehab]

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

Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Marek Behún]

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

Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

[Mauro Carvalho Chehab]

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