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