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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

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