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

[Lee Jones <lee@kernel.org>]

On Mon, 21 Aug 2023, Marek Behún wrote:

> 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?

Yes, thank you.

-- 
Lee Jones [李琼斯]