On Fri, 18 Aug 2023 10:09:21 +0100
Lee Jones <lee@xxxxxxxxxx> wrote:
> > + if (!err) {
> > + /* put the LED into MCU controlled mode */
>
> Nit: You improved the comment above to be more grammatically correct by
> starting with an uppercase character. Please continue with this
> improvement for all comments there in.
OK.
> > +static struct led_trigger omnia_hw_trigger = {
> > + .name = "omnia-mcu",
> > + .activate = omnia_hwtrig_activate,
> > + .deactivate = omnia_hwtrig_deactivate,
> > + .trigger_type = &omnia_hw_trigger_type,
>
> Not sure I understand this interface.
>
> Why not just set a bool instead of carrying around an empty struct?
Let me explain:
So, if a LED class device has the same trigger type as a LED trigger,
the trigger will be available for that LED (it is listed in the sysfs
trigger file and can be chosen).
So we have a mechanism to "pair" a LED with a given trigger, to make it
possible for the LED core to distinguish whether a given trigger is
available for the LED.
A boolean information would not be enough: if we used a bool, we would
know that the trigger is private. But the LED core would not know for
which LEDs the trigger should be avaiable.
In pseudocode:
list_triggers_for_led(led) {
for (trigger in trigger_list) {
if (!trigger.trigger_type || trigger.trigger_type ==
led.trigger_type)
trigger is available for led
else
trigger is not available for led
}
}
Is this explaination good enough?
Marek
