mbox series

[00/17] Add an experimental driver for Intel NUC leds

Message ID cover.1621161037.git.mchehab+huawei@kernel.org
Headers show
Series Add an experimental driver for Intel NUC leds | expand

Message

Mauro Carvalho Chehab May 16, 2021, 10:53 a.m. UTC
Hi Greg,

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 programmable,
which allow them to indicate several different hardware events.

They can even 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.

There are 3 different API types, and there is 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, 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 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

I wrote this driver mainly for my NUC8 (NUC8i7HNK), but I 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 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.

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, I'm opting to submit this first to staging.

This should allow adjusting the uAPI if needed, and to get feedback from
people using it on NUC6, NUC10 and on other NUC models that would be
compatible with it.

Mauro Carvalho Chehab (17):
  staging: add support for NUC WMI LEDs
  staging: nuc-wmi: detect WMI API detection
  staging: nuc-wmi: add support for changing S0 brightness
  staging: nuc-wmi: add all types of brightness
  staging: nuc-wmi: allow changing the LED colors
  staging: nuc-wmi: add support for WMI API version 1.0
  staging: nuc-wmi: add basic support for NUC6 WMI
  staging: muc-wmi: add brightness and color for NUC6 API
  staging: nuc-wmi: Add support to blink behavior for NUC8/10
  staging: nuc-wmi: get rid of an unused variable
  staging: nuc-wmi: implement blink control for NUC6
  staging: nuc-wmi: better detect NUC6/NUC7 devices
  staging: nuc-led: add support for HDD activity default
  staging: nuc-wmi: fix software blink behavior logic
  staging: nuc-wmi: add support for changing the ethernet type indicator
  staging: nuc-wmi: add support for changing the power limit scheme
  staging: nuc-led: update the TODOs

 MAINTAINERS                       |    6 +
 drivers/staging/Kconfig           |    2 +
 drivers/staging/Makefile          |    1 +
 drivers/staging/nuc-led/Kconfig   |   11 +
 drivers/staging/nuc-led/Makefile  |    3 +
 drivers/staging/nuc-led/TODO      |    8 +
 drivers/staging/nuc-led/nuc-wmi.c | 2100 +++++++++++++++++++++++++++++
 7 files changed, 2131 insertions(+)
 create mode 100644 drivers/staging/nuc-led/Kconfig
 create mode 100644 drivers/staging/nuc-led/Makefile
 create mode 100644 drivers/staging/nuc-led/TODO
 create mode 100644 drivers/staging/nuc-led/nuc-wmi.c

Comments

Pavel Machek May 16, 2021, 6:21 p.m. UTC | #1
Hi!

> -  Need to validate the uAPI and document it before moving
>    this driver out of staging.

>  - Stabilize and document its sysfs interface.
   
Would you mind starting with this one? We should have existing APIs
for most of functionality described...

We really don't want to merge code with bad API, not even to staging.


Best regards,
							Pavel
Mauro Carvalho Chehab May 17, 2021, 6:30 a.m. UTC | #2
Hi Pavel,

Em Sun, 16 May 2021 20:21:50 +0200
Pavel Machek <pavel@ucw.cz> escreveu:

> Hi!

> 

> > -  Need to validate the uAPI and document it before moving

> >    this driver out of staging.  

> 

> >  - Stabilize and document its sysfs interface.  

>    

> Would you mind starting with this one?


Do you mean writing the ABI document for it? Surely I can do that,
but I'm not sure where to put such document while it is on staging.

> We should have existing APIs

> for most of functionality described...


I tried to stay as close as possible to the existing API, but
there are some things that required a different one.

For instance, with WMI rev 0.64 and 1.0, any LED of the device
can be programmed to be a power indicator.

When a LED is programmed this way, there are up to 3 (on rev 1.0) or up
to 4 (on rev 0.64) different brightness level of the LED, and those
are associated with a power status (like S0, S3, S5, "ready mode").

So, the LED API standard "brightness" is meaningless. On the other
hand, when the same LED is programmed to monitor, let's say, the
WiFi or one of the two Ethernets (or both at the same time), the
standard "brightness" level makes sense.

> 

> We really don't want to merge code with bad API, not even to staging.


See, this is the API that it is exposed on with a NUC8:

	$ tree /sys/class/leds/nuc\:\:front1/
	/sys/class/leds/nuc::front1/
	├── blink_behavior
	├── blink_frequency
	├── brightness
	├── color
	├── device -> ../../../8C5DA44C-CDC3-46B3-8619-4E26D34390B7
	├── ethernet_type
	├── hdd_default
	├── indicator
	├── max_brightness
	├── power
	│   ├── autosuspend_delay_ms
	│   ├── control
	│   ├── runtime_active_time
	│   ├── runtime_status
	│   └── runtime_suspended_time
	├── 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
	├── subsystem -> ../../../../../../../../class/leds
	├── trigger
	└── uevent

As the behavior of the LEDs can be dynamically changed, each
LED expose parameters for all types of hardware event it can
deal, but only the ones that are applied to its current indicator
type can be seen/changed.

On other words, the "indicator" tells what type of hardware event
the LED is currently measuring:

	$ cat /sys/class/leds/nuc\:\:front1/indicator 
	Power State  [HDD Activity]  Ethernet  WiFi  Software  Power Limit  Disable  

In this case, as it is measuring the HDD activity. If one tries to
read/write something to, let's say, the Ethernet type, a -EINVAL
is returned:

	$ cat /sys/class/leds/nuc\:\:front1/ethernet_type 
	cat: '/sys/class/leds/nuc::front1/ethernet_type': Invalid argument

So, before being able to use the ethernet_type, the indicator needs
to be changed:

	$ echo Ethernet > /sys/class/leds/nuc\:\:front1/indicator 
	$ cat /sys/class/leds/nuc\:\:front1/ethernet_type
	LAN1  LAN2  [LAN1+LAN2]  

Anyway, I suspect that besides a document under ABI, it would
make sense to add a .rst file describing it under admin-guide,
explaining how to use the ABI.

That should likely be easier to discuss if any changes at the
ABI would be needed. Before moving it out of staging, I would
add another one under Documentation/ABI describing the meaning
of each sysfs node.

Would that work for you?

Thanks,
Mauro
Pavel Machek May 17, 2021, 8:05 a.m. UTC | #3
Hi!

> > > -  Need to validate the uAPI and document it before moving

> > >    this driver out of staging.  

> > 

> > >  - Stabilize and document its sysfs interface.  

> >    

> > Would you mind starting with this one?

> 

> Do you mean writing the ABI document for it? Surely I can do that,

> but I'm not sure where to put such document while it is on staging.


No need for formal ABI just yet, but it needs to be clear what the interface
is.

> > We should have existing APIs

> > for most of functionality described...

> 

> I tried to stay as close as possible to the existing API, but

> there are some things that required a different one.


I believe it should be possible to move _way_ closer to existing APIs.

> For instance, with WMI rev 0.64 and 1.0, any LED of the device

> can be programmed to be a power indicator.

> 

> When a LED is programmed this way, there are up to 3 (on rev 1.0) or up

> to 4 (on rev 0.64) different brightness level of the LED, and those

> are associated with a power status (like S0, S3, S5, "ready mode").


I'll need a description how this works.

> 	/sys/class/leds/nuc::front1/

> 	├── blink_behavior

> 	├── blink_frequency


We have timer trigger for these.

> 	├── ethernet_type

> 	├── hdd_default

> 	├── indicator

> 	├── 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


No. Take a look at triggers; for example hdd monitor should look very
much like existing disk trigger.

> On other words, the "indicator" tells what type of hardware event

> the LED is currently measuring:

> 

> 	$ cat /sys/class/leds/nuc\:\:front1/indicator 

> 	Power State  [HDD Activity]  Ethernet  WiFi  Software  Power Limit  Disable  


So this will use existing "trigger" infrastructure.

> That should likely be easier to discuss if any changes at the

> ABI would be needed. Before moving it out of staging, I would

> add another one under Documentation/ABI describing the meaning

> of each sysfs node.


Lets get reasonable ABI before moving it _into_ tree, staging or
otherwise.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek
Greg KH May 17, 2021, 8:18 a.m. UTC | #4
On Sun, May 16, 2021 at 12:53:28PM +0200, Mauro Carvalho Chehab wrote:
> Hi Greg,

> 

> 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 programmable,

> which allow them to indicate several different hardware events.

> 

> They can even 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.

> 

> There are 3 different API types, and there is 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, I opted to write a new driver from scratch,

> unifying support for all different versions and using sysfs

> via the leds class.


Just do this the "right way" and not add it to staging first.  Just use
the existing LED class apis and all should be fine, no need for doing
anything unusual here.

thanks,m

greg k-h
Greg KH May 17, 2021, 8:20 a.m. UTC | #5
On Sun, May 16, 2021 at 12:53:29PM +0200, Mauro Carvalho Chehab wrote:
> Some Intel Next Unit of Computing (NUC) machines have

> software-configured LEDs that can be used to display a

> variety of events:

> 

> 	- Power State

> 	- HDD Activity

> 	- Ethernet

> 	- WiFi

> 	- Power Limit


<snip>

One nit:

> +static void nuc_wmi_remove(struct wmi_device *wdev)

> +{

> +	struct device *dev = &wdev->dev;

> +

> +	dev_info(dev, "NUC WMI driver removed.\n");

> +}


Drivers, when working properly, should be quiet.  No need for noisy
stuff like this, it just slows down booting/loading for everyone.

thanks,

greg k-h
Mauro Carvalho Chehab May 17, 2021, 8:57 a.m. UTC | #6
Em Mon, 17 May 2021 10:05:27 +0200
Pavel Machek <pavel@ucw.cz> escreveu:

> Hi!

> 

> > > > -  Need to validate the uAPI and document it before moving

> > > >    this driver out of staging.    

> > >   

> > > >  - Stabilize and document its sysfs interface.    

> > >    

> > > Would you mind starting with this one?  

> > 

> > Do you mean writing the ABI document for it? Surely I can do that,

> > but I'm not sure where to put such document while it is on staging.  

> 

> No need for formal ABI just yet, but it needs to be clear what the interface

> is.

> 

> > > We should have existing APIs

> > > for most of functionality described...  

> > 

> > I tried to stay as close as possible to the existing API, but

> > there are some things that required a different one.  

> 

> I believe it should be possible to move _way_ closer to existing APIs.

> 

> > For instance, with WMI rev 0.64 and 1.0, any LED of the device

> > can be programmed to be a power indicator.

> > 

> > When a LED is programmed this way, there are up to 3 (on rev 1.0) or up

> > to 4 (on rev 0.64) different brightness level of the LED, and those

> > are associated with a power status (like S0, S3, S5, "ready mode").  

> 

> I'll need a description how this works.

> 

> > 	/sys/class/leds/nuc::front1/

> > 	├── blink_behavior

> > 	├── blink_frequency  

> 

> We have timer trigger for these.


Not really. The LED blink behavior is provided by the hardware itself.

The LEDs can blink *even* when the device is suspended or is
hibernating. That's something that a timer trigger can't do ;-)

See below for a draft of the ABI description.

> 

> > 	├── ethernet_type

> > 	├── hdd_default

> > 	├── indicator

> > 	├── 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  

> 

> No. Take a look at triggers; for example hdd monitor should look very

> much like existing disk trigger.


Ok, I'll double-check how this works. Yeah, it would be a way better if
the sysfs nodes could be hidden when changing the indicator type.

For instance, when monitoring disk activity, only those 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.
    =================================	=======================================

(color is only available for multi-colored or RGB leds).

> 

> > On other words, the "indicator" tells what type of hardware event

> > the LED is currently measuring:

> > 

> > 	$ cat /sys/class/leds/nuc\:\:front1/indicator 

> > 	Power State  [HDD Activity]  Ethernet  WiFi  Software  Power Limit  Disable    

> 

> So this will use existing "trigger" infrastructure.


Ok, I will take a look on that. Are there any driver that I could use
as an example, using it in a configurable way?

> > That should likely be easier to discuss if any changes at the

> > ABI would be needed. Before moving it out of staging, I would

> > add another one under Documentation/ABI describing the meaning

> > of each sysfs node.  

> 

> Lets get reasonable ABI before moving it _into_ tree, staging or

> otherwise.


I'm enclosing a document that I started to write today, describing the
way the current ABI was designed. The document doesn't describe in
full the NUC6 variant (which is really limited: just two LEDs
with fixed behavior).

Thanks,
Mauro


==================
Intel NUC WMI LEDs
==================

Some models of the Intel Next Unit of Computing (NUC) may have programmable
LEDs on its panel via its BIOS. A subset of those may also be programmed on
user space.

There are currently three different APIs on such devices, depending on the
NUC generation:

* NUC 6/7:
  https://www.intel.com/content/www/us/en/support/articles/000023426/intel-nuc/intel-nuc-kits.html
* NUC 8/9:
  https://raw.githubusercontent.com/nomego/intel_nuc_led/master/specs/INTEL_WMI_LED_0.64.pdf
* NUC 10 and newer:
  https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf

This document describes how to use the LEDs API, as supported by the "nuc-wmi"
driver.

Please notice that the LEDs can only be programmed if the BIOS settings
are allowing the Operational System to control them. Instructions about
how to enable it can be found at the manual of each specific NUC, if
the LEDs are userspace programmed for an specific device.

LED devices
===========

When the driver detects NUC LEDs, some sysfs device nodes are created under
the leds class.

On NUC 6, there are (up to) two LEDs available:

=============	==============================
LED name	sysfs device node
=============	==============================
Power		``/sys/class/leds/nuc::power``
Ring		``/sys/class/leds/nuc::ring``
=============	==============================

The NUC 6 API is limited, as it allows only to change the LED color, and
its blink state. Its API will be described on :ref:`NUC6 API`.

On NUC 8 and newer generations, up to seven LEDs are supported:

=============	===============================
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``
=============	===============================

The API for NUC 8 and newer allows full control of the LEDs meaning.

NUC 6 API
=========

TODO: describe the limited NUC6 API

NUC 8 and newer generations API
===============================

On NUC8, and newer, several sysfs nodes will allow to control the
functionality of each LED::

    /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::

   For the entire NUC8+ API, the following rules apply:

   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.

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.


NUC6/7
------

On NUC6 API, the power LED color can be:

    +---------+
    | disable |
    +---------+
    | blue    |
    +---------+
    | amber   |
    +---------+

And the ring LED color can be:

    +---------+
    | disable |
    +---------+
    | cyan    |
    +---------+
    | pink    |
    +---------+
    | yellow  |
    +---------+
    | blue    |
    +---------+
    | red     |
    +---------+
    | green   |
    +---------+
    | white   |
    +---------+

NUC 8 and newer generations
---------------------------

On NUC10 API, the color can be:

    ============	======	=====	=====
    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.
Mauro Carvalho Chehab May 17, 2021, 9:02 a.m. UTC | #7
Em Mon, 17 May 2021 10:18:57 +0200
Greg KH <gregkh@linuxfoundation.org> escreveu:

> On Sun, May 16, 2021 at 12:53:28PM +0200, Mauro Carvalho Chehab wrote:

> > Hi Greg,

> > 

> > 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 programmable,

> > which allow them to indicate several different hardware events.

> > 

> > They can even 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.

> > 

> > There are 3 different API types, and there is 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, I opted to write a new driver from scratch,

> > unifying support for all different versions and using sysfs

> > via the leds class.  

> 

> Just do this the "right way" and not add it to staging first.  Just use

> the existing LED class apis and all should be fine, no need for doing

> anything unusual here.


I'm using the standard LED class already (but not triggers), and the
standard WMI support.

Still, this API is complex, as it controls the LED behavior even when
the machine is suspended. I would feel more comfortable if the ABI
is not set into a stone at the beginning.

But it is your and Pavel's call.

Thanks,
Mauro
Greg KH May 17, 2021, 9:08 a.m. UTC | #8
On Mon, May 17, 2021 at 11:02:58AM +0200, Mauro Carvalho Chehab wrote:
> Em Mon, 17 May 2021 10:18:57 +0200

> Greg KH <gregkh@linuxfoundation.org> escreveu:

> 

> > On Sun, May 16, 2021 at 12:53:28PM +0200, Mauro Carvalho Chehab wrote:

> > > Hi Greg,

> > > 

> > > 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 programmable,

> > > which allow them to indicate several different hardware events.

> > > 

> > > They can even 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.

> > > 

> > > There are 3 different API types, and there is 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, I opted to write a new driver from scratch,

> > > unifying support for all different versions and using sysfs

> > > via the leds class.  

> > 

> > Just do this the "right way" and not add it to staging first.  Just use

> > the existing LED class apis and all should be fine, no need for doing

> > anything unusual here.

> 

> I'm using the standard LED class already (but not triggers), and the

> standard WMI support.

> 

> Still, this API is complex, as it controls the LED behavior even when

> the machine is suspended. I would feel more comfortable if the ABI

> is not set into a stone at the beginning.


code in drivers/staging/ does not mean that you can mess with the
userspace api at will.  It still follows the same "rules" of any other
kernel code when it comes to that.

So just work with the LED developers to come to a valid api that works
properly within that framework please.

thanks,

greg k-h
Mauro Carvalho Chehab May 17, 2021, 9:12 a.m. UTC | #9
Em Mon, 17 May 2021 10:57:49 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Mon, 17 May 2021 10:05:27 +0200

> Pavel Machek <pavel@ucw.cz> escreveu:


> > No. Take a look at triggers; for example hdd monitor should look very

> > much like existing disk trigger.  


Btw, is there a way to trigger brightness?

When a LED is monitoring the power state, brightness should be
hidden, as, instead of a single brightness parameter, the device
will now have one brightness per different power state, e. g.:

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> is different, depending on the WMI API version:

On version 0.64 (NUC8/9):

    +------------+
    | s0	 |
    +------------+
    | s3	 |
    +------------+
    | s5	 |
    +------------+
    | ready_mode |
    +------------+

Btw, I've no idea what "ready mode" is, as the specs don't explain it.
This particular mode is disabled on my NUC8 device, so I can't test it.

On version 1.0 (NUC10+):

    +------------+
    | s0	 |
    +------------+
    | s3	 |
    +------------+
    | standby	 |
    +------------+

Note: At the specs, "Standby" is actually "Modern Standby". I ended
simplifying it, as just "standby_brightness" sounds good enough.

Thanks,
Mauro
Dan Carpenter May 17, 2021, 9:44 a.m. UTC | #10
On Sun, May 16, 2021 at 12:53:35PM +0200, Mauro Carvalho Chehab wrote:
> +static int nuc_wmi_query_leds_nuc6(struct device *dev)

> +{

> +	// FIXME: add a check for the specific models that are known to work

> +	struct nuc_wmi *priv = dev_get_drvdata(dev);

> +	u8 cmd, input[NUM_INPUT_ARGS] = { 0 };

> +	u8 output[NUM_OUTPUT_ARGS];

> +	struct nuc_nmi_led *led;

> +	int ret;

> +

> +	cmd = LED_OLD_GET_STATUS;

> +	input[0] = LED_OLD_GET_S0_POWER;

> +	ret = nuc_nmi_cmd(dev, cmd, input, output);

> +	if (ret) {

> +		dev_warn(dev, "Get S0 Power: error %d\n", ret);

> +		return ret;

> +	}

> +

> +	led = &priv->led[priv->num_leds];

> +	led->id = POWER_LED;

> +	led->color_type = LED_BLUE_AMBER;

> +	led->avail_indicators = LED_IND_POWER_STATE;

> +	led->indicator = fls(led->avail_indicators);

> +	priv->num_leds++;

> +

> +	cmd = LED_OLD_GET_STATUS;

> +	input[0] = LED_OLD_GET_S0_RING;

> +	ret = nuc_nmi_cmd(dev, cmd, input, output);

> +	if (ret) {

> +		dev_warn(dev, "Get S0 Ring: error %d\n", ret);

> +		return ret;

> +	}

> +	led = &priv->led[priv->num_leds];

> +	led->id = RING_LED;

> +	led->color_type = LED_BLUE_AMBER;

> +	led->avail_indicators = LED_IND_SOFTWARE;

> +	led->indicator = fls(led->avail_indicators);

> +	priv->num_leds++;

> +

> +	return ret;


return 0;

> +}

> +

>  static int nuc_wmi_query_leds(struct device *dev, enum led_api_rev *api_rev)

>  {

>  	struct nuc_wmi *priv = dev_get_drvdata(dev);

>  	u8 input[NUM_INPUT_ARGS] = { 0 };

>  	u8 output[NUM_OUTPUT_ARGS];

> -	int id, ret, ver = LED_API_UNKNOWN;

> +	int id, ret, ver = LED_API_UNKNOWN, nuc_ver = 0;

>  	u8 leds;

> +	const char *dmi_name;

> +

> +	dmi_name = dmi_get_system_info(DMI_PRODUCT_NAME);

> +	if (!dmi_name || !*dmi_name)

> +		dmi_name = dmi_get_system_info(DMI_BOARD_NAME);

> +

> +	if (strncmp(dmi_name, "NUC", 3))

> +		return -ENODEV;

> +

> +	dmi_name +=3;

> +	while (*dmi_name) {

> +		if (*dmi_name < '0' || *dmi_name > '9')

> +			break;

> +		nuc_ver = (*dmi_name - '0') + nuc_ver * 10;

> +		dmi_name++;

> +	}

> +

> +	if (nuc_ver < 6)

> +		return -ENODEV;

> +

> +	if (nuc_ver < 8) {

> +		*api_rev = LED_API_NUC6;

> +		return nuc_wmi_query_leds_nuc6(dev);

> +	}

>  

> -	/*

> -	 * List all LED types support in the platform

> -	 *

> -	 * Should work with both NUC8iXXX and NUC10iXXX

> -	 *

> -	 * FIXME: Should add a fallback code for it to work with older NUCs,

> -	 * as LED_QUERY returns an error on older devices like Skull Canyon.

> -	 */

>  	input[0] = LED_QUERY_LIST_ALL;

>  	ret = nuc_nmi_cmd(dev, LED_QUERY, input, output);

> -	if (ret == -ENOENT) {

> -		ver = LED_API_NUC6;

> -	} else if (ret) {

> +	if (ret) {

>  		dev_warn(dev, "error %d while listing all LEDs\n", ret);

>  		return ret;

>  	} else {

>  		leds = output[0];

>  	}


Delete the else and pull the assignment in a tab.

>  

> -	if (ver != LED_API_NUC6) {

> -		ret = nuc_nmi_cmd(dev, LED_VERSION_CONTROL, input, output);

> -		ver = output[0] | output[1] << 16;

> -		if (!ver)

> -			ver = LED_API_REV_0_64;

> -		else if (ver == 0x0126)

> -			ver = LED_API_REV_1_0;

> -	}

> +	ret = nuc_nmi_cmd(dev, LED_VERSION_CONTROL, input, output);

> +	ver = output[0] | output[1] << 16;

> +	if (!ver)

> +		*api_rev = LED_API_REV_0_64;

> +	else if (ver == 0x0126)

> +		*api_rev = LED_API_REV_1_0;

>  


regards,
dan carpenter
Mauro Carvalho Chehab May 17, 2021, 12:19 p.m. UTC | #11
Em Mon, 17 May 2021 10:05:27 +0200
Pavel Machek <pavel@ucw.cz> escreveu:

> No. Take a look at triggers; for example hdd monitor should look very

> much like existing disk trigger.


Hmm... after looking at triggers, I'm not sure if this is the right
interface, nor if we're talking about the same thing.

See, at least the way ledtrig-disk.c uses it, two drivers are triggering 
the LED based on software events:

	drivers/ata/libata-core.c:      ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
	drivers/ide/ide-disk.c: ledtrig_disk_activity(rq_data_dir(rq) == WRITE);

This is not how the NUC LEDs are work. On NUC, the hardware itself 
triggers the event and/or blinks the LED(*).

(*) By default, blink is enabled only when the device is suspended
    or it is hibernating.

There's no need to do any software emulation.

The API is meant to just program the hardware (and/or the firmware) 
for it to do the behavior expected by the user.

So, for instance, setting the indicator event that will trigger the
LED is done by sending a WMI message for this GUID object:
8C5DA44C-CDC3-46b3-8619-4E26D34390B7 (somewhat similar to using
the way ACPI works, but WMI is a different firmware interface).

The method at the WMI API which sets the LED indicator is this one:

	0x05 message (Set an Indicator option for the LED type)

Such method receives two parameters. The first one is the LED 
number, which can be:

	0 - Power button LED
	1 - HDD LED
	2 - Skull LED
	3 - Eyes LED
	4 - Front LED 1
	5 - Front LED 1
	6 - Front LED 1

The second one tells which hardware event will trigger the LED:

=====	==============	=======================================================
Value	Indicator type	Meaning
=====	==============	=======================================================
0	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).
1	HDD Activity	Indicates if the LED is measuring the hard disk (or
			SDD) activity.
2	Ethernet	Indicates the activity Ethernet adapter(s)
3	WiFi		Indicates if WiFi is enabled
4	Software	Doesn't indicate any hardware level. Instead, the LED
			status is controlled via software.
5	Power Limit	Changes the LED color when the computer is throttling
			its power limits.
6	Disable		The LED was disabled (either by BIOS or via WMI).
=====	==============	=======================================================

There is an example at page 7 of the datasheet:

	https://raw.githubusercontent.com/nomego/intel_nuc_led/master/specs/INTEL_WMI_LED_0.64.pdf

Where it shows what happens if the WMI message is filled with:

	<0x05>  <0x00>  <0x01>

On such example, it changes the behavior of the button named "Power button" 
to change it to monitor the disk activity, instead of monitoring if the
device is powered on or not.

Such setting is even persistent after rebooting, and the event is
hardware/firmware triggered.

So, IMO, it would only makes sense to use something else from the LED
class if are there a way for the sysfs nodes to dynamically be shown/hidden
when the indicator changes.

At least on my eyes, that doesn't seem to be what triggers do.

Thanks,
Mauro