Hi Marek,
Thank you for the design proposal.
Please find my comments below.
On 3/27/19 6:30 AM, Marek Behun wrote:
Hello,
so I've been thinking a little about LED HW triggering API since Jacek
replied to my patch for Turris LEDs.
There are many chips which expose SW control of LEDs which can also be
put into HW control mode, for example network PHYs, switches, wireless
cards etc.
Jacek proposed a hardware_controlled file to the LED core, exposed only
if supported by hardware when the a new flag LED_DEV_CAP_HW_CONTROL is
set on the LED.
What I then propose is this:
- if 1 is written to hardware_controlled
- any sw triggers should be disabled on this LED
I'd just return -EBUSY from brightenss_set{_blocking} in this case
and not mess with trigger availability.
- if the device supports only one hardware controlled mode, it is
put into this mode
- if the device supports several hardware controlled modes, it is
put into one of these modes and a hardware_trigger file is
created, via which these mode can be changed.
This hardware_trigger file behaves similarly to classic trigger
file, ie. on read it returns space-separated list of supported
hardware triggers, the currently selected one in brackets.
We already have had a report of trigger list exceeding PAGE_SIZE in case
of a system with huge amount of cpus (dedicated cpuN trigger created for
each cpu). Effectively, sooner or later we will need to fix this
interface.
It is better not to get exposed to this risk anymore.
Instead of space separated list I propose a directory with
files named after trigger names. The files will accept/print
1/0 values signifying whether the trigger is enabled/disabled.
- if 0 is written to hardware_controlled
- if the LED was in HW controlled mode, it should be put back into
SW mode. hardware_trigger file is removed if it exists
- OPTIONAL: in order to support directly setting specific hardware
controlled mode on LEDs which support more than one mode, the
specific mode name can be written to hardware_controlled file. The
behaviour is same as if writing 1 to hardware_controlled and then
mode name into hardware_trigger, but it is atomic.
I'd not necessarily go for it. File name will suggest logical value
and allowing writing a trigger name would be semantically incorrect.
(this would be similar to gpio sysfs interface for the direction
file)
I'll accept that provided you will obtain ack from Greg.
- the hardware_controlled file is available only if the LED has the
LED_DEV_CAP_HW_CONTROL capability
- internally a function pointer
int (*hardware_trigger_set)(struct led_classdev *led,
const char *name);
should be implemented in led_classdev structure for LEDs supporting
hardware control, and also a
const char *current_hardware_trigger
- since hardware triggers are completely hardware specific, it does
not make sense to register them globally like classic triggers.
Option 1 is to have a list of supported hardware triggers in the
led_classdev structure, which is given there before LED
registration. This could be a NULL terminated array of strings.
Writing 1 to hardware_controlled calls the ->hardware_trigger_set
method with parameter name = NULL.
Writing to hardware_trigger checks if the specific trigger is in
the array of supported triggers, and if yes, it calls the
->hardware_trigger_set method with the name of the hw trigger.
Option 2 is to read the supported hw triggers dynamically, by having
a hardware_triggers_get() function, but I do not think this is
needed.
At first glance both options seem to have the same cost. I have no
strong opinion at the moment.
What do you guys think about this? Is this acceptable? How does this
work with hw blinking/pattern setting?
Both ops should return -EBUSY when an LED is under hw trigger control.
--
Best regards,
Jacek Anaszewski
