Re: [PATCH V2 1/2] dt-bindings: leds: document new led-triggers property

[Jacek Anaszewski <jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Rafał,

On 01/31/2017 05:11 PM, Rafał Miłecki wrote:
> On 01/25/2017 10:04 PM, Jacek Anaszewski wrote:
>> On 01/25/2017 10:03 AM, Rafał Miłecki wrote:
>>> On 21 January 2017 at 22:42, Jacek Anaszewski
>>> <jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 01/21/2017 05:24 PM, Rafał Miłecki wrote:
>>>>> On 20 January 2017 at 23:35, Jacek Anaszewski
>>>>> <jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> On 01/20/2017 10:56 PM, Rafał Miłecki wrote:
>>>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>>>
>>>>>>> Some LEDs can be related to particular devices described in DT. This
>>>>>>> property allows specifying such relations. E.g. USB LED should
>>>>>>> usually
>>>>>>> be used to indicate some USB port(s) state.
>>>>>>>
>>>>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>>> ---
>>>>>>> V2: Replace "usb-ports" with "led-triggers" property which is
>>>>>>> more generic and
>>>>>>>     allows specifying other devices as well.
>>>>>>>
>>>>>>> When bindings patch is related to some followup implementation,
>>>>>>> they usually go
>>>>>>> through the same tree.
>>>>>>>
>>>>>>> Greg: this patch is based on top of e64b8cc72bf9 ("DT: leds:
>>>>>>> Improve examples by
>>>>>>> adding some context") from kernel/git/j.anaszewski/linux-leds.git
>>>>>>> . Is there any
>>>>>>> way to solve this dependency issue? Or should this patch wait
>>>>>>> until 3.11 is
>>>>>>> released?
>>>>>>> ---
>>>>>>>  Documentation/devicetree/bindings/leds/common.txt | 16
>>>>>>> ++++++++++++++++
>>>>>>>  1 file changed, 16 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/leds/common.txt
>>>>>>> b/Documentation/devicetree/bindings/leds/common.txt
>>>>>>> index 24b656014089..17632a041196 100644
>>>>>>> --- a/Documentation/devicetree/bindings/leds/common.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/leds/common.txt
>>>>>>> @@ -49,6 +49,17 @@ Optional properties for child nodes:
>>>>>>>  - panic-indicator : This property specifies that the LED should
>>>>>>> be used,
>>>>>>>                   if at all possible, as a panic indicator.
>>>>>>>
>>>>>>> +- led-triggers : List of devices that should trigger this LED
>>>>>>> activity. Some
>>>>>>> +              LEDs can be related to a specific device and
>>>>>>> should somehow
>>>>>>> +              indicate its state. E.g. USB 2.0 LED may react to
>>>>>>> device(s) in
>>>>>>> +              a USB 2.0 port(s). Another common example is
>>>>>>> switch or router
>>>>>>> +              with multiple Ethernet ports each of them having
>>>>>>> its own LED
>>>>>>> +              assigned (assuminled-trigger-usbportg they are not
>>>>>>> hardwired).
>>>>>>> +              In such cases this property should contain
>>>>>>> phandle(s) of
>>>>>>> +              related device(s). In many cases LED can be
>>>>>>> related to more
>>>>>>> +              than one device (e.g. one USB LED vs. multiple USB
>>>>>>> ports) so a
>>>>>>> +              list of entries can be specified.
>>>>>>> +
>>>>>>
>>>>>> This implies that it is possible to define multiple triggers for
>>>>>> a LED class device but it is not supported by LED Trigger core.
>>>>>> There is linux,default-trigger property which allows to define one
>>>>>> trigger that will be initially assigned.
>>>>
>>>>> With proposed "led-triggers" property one could assign different
>>>>> trigger *sources* to a LED. You could e.g. assign 2 USB ports, network
>>>>> device & CPU to a single LED. For reason explained above Linux
>>>>> couldn't support all of them at once.
>>>>>
>>>>>
>>>>>> I am aware that this is renamed usb-ports property from v1,
>>>>>> that attempts to address Rob's comment, but we can't do that this
>>>>>> way.
>>>>>> Maybe usb-ports property could be documented in led-trigger-usbport's
>>>>>> specific bindings and a reference to it could be added next to the
>>>>>> related entry on the list of the available LED triggers (which is
>>>>>> actually missing) in
>>>>>> Documentation/devicetree/bindings/leds/common.txt.
>>>>>
>>>>> I'm wondering if we need to care about this Linux limitation and if it
>>>>> should stop us from adding this flexible DT property. Maybe we could
>>>>> live with Linux having limitation of supporting one trigger type at a
>>>>> time?
>>>>
>>>> That's what I meant. Generally I have objections to the generic
>>>> property for defining list of allowed triggers. That's why I proposed
>>>> to stay by usb-ports property that would be specific to only one
>>>> trigger: led-trigger-usbport.
>>>>
>>>> led-trigger-usbport in fact is an entirely new type of LED trigger.
>>>> LED triggers is a kernel based source of events. All existing triggers
>>>> react only to a single, well defined source of events, whereas
>>>> led-trigger-usbport allows to define the scope of events (usb ports)
>>>> to notify about. Activity on each port is treated similarly and the LED
>>>> class device that listens to the trigger notifications doesn't know
>>>> which exact port triggered the notification.
>>>>
>>>> From this POV led-trigger-usbport is kind of facade, which allows
>>>> for it to fit well into the LED Trigger design and API, and usb ports
>>>> are not identical with LED triggers, but sit rather one level below.
>>>> It is led-trigger-usbport which is visible to the LED subsystem, and
>>>> not every single usb port.
>>>>
>>>>> So e.g. if one assigns 2 USB ports + network device + CPU and
>>>>> decides to use "usport" trigger driver he'll get LED indicating status
>>>>> of USB ports only.
>>>>
>>>> How would it be different from the current state? Only in limiting
>>>> the scope of triggers available for a LED class device.
>>>
>>> It won't differ from the current state. I just wanted to make it clear
>>> Linux trigger drivers may respect only selected "led-triggers" values
>>> (phandles). Like "usbport" respecting USB port phandles but ignoring
>>> CPU ones, net ones, etc.
>>
>> This is the ambiguity I want to avoid. According to my analysis from
>> the previous message, physical usb port is one level below usbport LED
>> trigger, and it should be reflected in DT binding documentation. You
>> can't write usb1-port1 (using the convention from Documentation/leds/
>> ledtrig-usbport.txt) to the "triggers" sysfs file. You can only register
>> usbport trigger which can be configured to notify about activity on
>> usb1-port1.
>>
>> That's why I proposed linux,trigger-sources name for that.
>> Let's not confuse LED triggers with events originating from physical
>> devices or other sources of kernel events, being in turn translated by
>> LED triggers to LED brightness changes.
>>
>> This is a thing about naming. It is tempting to call sources of kernel
>> events "triggers", but they are not LED triggers on they own. LED
>> trigger is a driver that registers itself in LED Trigger core using
>> led_trigger_register() API.
> 
> Thanks a lot Jacek for this explanation (and sorry but I needed a bit of
> time to
> think about this). I can finally see your point, I think we're on the
> same page
> now.
> 
> I think that for all this time I was thinking from the pure DT
> perspective. If
> you ignore fact that Linux has multiple LED trigger drivers, then
> "led-triggers"
> may not sound that bad.
> 
> After reading your description however I can see this property can be
> misleading
> as Linux people may think "drivers" when seeing "triggers".
> 
> I still think this is a useful property and I hope we can still find a
> way to
> name it in a sane way: to be nice from DT PoV and march Linux LEDs
> subsystem.

I also see the need for this property.

> I think we should still work on some generic property (without linux,
> prefix) as
> this seems to be something generic, not really specific to Linux
> implementation.
> Specifying relation between LED and devices is something than AFAICS can be
> reused by other systems as well.
> 
> So my suggestion is to try some new name & leave linux,default-trigger
> to allow
> specifying one single trigger driver as required by current Linux
> implementation.
> 
> What do you think about this?
> 
> There are few suggestions to came to my mind:
> "trigger-sources"
> "trigger-devices"
> "led-trigger-devices"
> "led-related-devices"
> "led-event-devices"

trigger-devices would work in this specific use case,
where we can give phandles to usb ports. Nonetheless, we
have also other kernel based events serving as trigger
sources - e.g. timer.

In this case I'd go for trigger-sources, but we'd need
to have some generic way of defining the source like timer.

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html