Re: [PATCH 17/17] staging: nuc-led: update the TODOs

[Mauro Carvalho Chehab]

Hi Pavel,

Em Sun, 16 May 2021 20:21:50 +0200
Pavel Machek <pavel@xxxxxx> 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