Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

["Marek Behún" <kabel@kernel.org>]

On Fri, 18 Aug 2023 10:09:21 +0100
Lee Jones <lee@kernel.org> 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