Hello Marek, On Fri, Jul 03, 2020 at 12:06:02PM +0200, Marek Behún wrote: > On Thu, 2 Jul 2020 16:47:11 +0200 > Ondrej Jirman <megous@xxxxxxxxxx> wrote: > > > Some LED controllers may come with an internal HW triggering mechanism > > for the LED and an ability to switch between user control of the LED, > > or the internal control. One such example is AXP20X PMIC, that allows > > wither for user control of the LED, or for internal control based on > > the state of the battery charger. > > > > Add support for registering per-LED device trigger. > > > > Names of private triggers need to be globally unique, but may clash > > with other private triggers. This is enforced during trigger > > registration. Developers can register private triggers just like > > the normal triggers, by setting private_led to a classdev > > of the LED the trigger is associated with. > > [...] > > Hi Ondrej, > > Some criticism to this approach to HW triggers: > - every hw trigger for each LED has to be registered via current trigger > API. This will grow code size and memory footprint once this API is > widely used > - one HW trigger can only master one LED device (via private_led > member). So if I have, for example an ethernet switch with 8 ports, > and each port has 2 LEDs, and each LED has 10 possible HW triggering > mechanisms, with your proposed API one would need to register 8*2*10 > = 160 triggers Do you have such a switch? Also what's your real usecase? My usecase is a PMIC which has either a user-controllable or self-working mode for a single LED it controls. I want to be able to switch between those quickly and easily. I want the LED to be mostly controlled by PMIC, because that way PMIC can signal events that are not exposed to OS like overvoltage, overheating, etc. ... all automagically, but also be able to control it sometimes via SW (for non PMIC related notifications, eg.). So in my mindset LED is either controlled by Linux via various SW triggers, or it's self-working in some arbitrary device specific configuration that doesn't need any passing of the data via CPU for the LED to reflect some HW state. So I'd expose a 'hw-trigger' only on the LED device that allows this, that you can select among all the other regular triggers as you do now, and then configure its precise mode/options in sysfs (on the trigger kobj). The driver would come with some sane device specific defaults for the self-working mode. User can then select hw-trigger, in the triggers and would get a nice PMIC LED behavior controlled by HW, or a common LED behavior of the ehternet port, or on the wireless card, or whatever. >From the perspective of this use case the interface is nice and generic: - you set LED to hw-trigger mode on boot - you set trigger to none and poke the LED with a pattern you want for the notification and put it back to hw-trigger mode again afterwards We can standardize on hw-trigger, or self-controlled, or some other name for this kind of private LED trigger, and then the userspace would not need to even care about the specific LED device type, when sitching between SW controlled and self-working modes. You'd be able to take SW control of the ethernet PHY controlled LEDs this way just the same as the PMIC LED with the same interface, described above. And if you don't like the default self-controled mode, you can change it via sysfs attributes on the trigger. It would also allow the user to switch between SW and HW control, without having to remember the previous HW triggering mode, because that could be persisted by the LED HW trigger device. So you can go back to previous HW triggering mode just by 'echo hw-trigger > your-led/trigger'. I've read through the discussions, and this seems like a workable interface. Most of the LED devices would just add one extra private trigger to the triggers_list, so it would not explode in the way you describe above. Also benefit of this approach is that it fits quite nicely with the existing code, and requires minimal changes. The trigger already allows for specifying sysfs attributes, too. regards, o. > I too have been thinking about an API for HW LED triggers, and I > tinkered with it a little. Some time ago I sent some emails, with > subjects: > "RFC: LED hw triggering API" > "about my trigger-sources work" > My current thoughts about how HW LED triggers could work nicely is as > such: > - these members (maybe with different names) shall be added to struct > led_classdev: > available_hw_triggers() > - shall return a NULL terminated list of HW trigger names > available for this LED > set_hw_trigger() > - sets HW trigger for this LED. The LED triggering API shall > call this method after previous LED trigger is unset. If > called with NULL parameter, unsets HW trigger > current_hw_trigger > - name of the currently set HW LED trigger for this LED > - the driver registering the LED cdev informs abouth the LED being > capable of HW triggering - members available_hw_triggers and > set_hw_trigger must be set > - SW LED trigger and HW LED trigger are mutualy exclusive on one LED > - the trigger file in sysfs (/sys/class/leds/LED/trigger) shall first > list the available SW triggers, and then available hw triggers for > this LED, prefixed with "hw:" > When written, if the written trigger name starts with "hw:", > instead of setting SW trigger, a HW trigger is set via > set_hw_trigger() method > > This could also nicely work with trigger-source DTS property - when the > driver registers a LED, it can determine what other device is > triggering the LED (for example a netdevice), and correspondingly set > the correct name for the LED. Code for that could be shared between HW > and SW triggering API. > > Marek
