On 01/25/2017 10:03 AM, Rafał Miłecki wrote:
> On 21 January 2017 at 22:42, Jacek Anaszewski
> <jacek.anaszewski@xxxxxxxxx> wrote:
>> On 01/21/2017 05:24 PM, Rafał Miłecki wrote:
>>> On 20 January 2017 at 23:35, Jacek Anaszewski
>>> <jacek.anaszewski@xxxxxxxxx> wrote:
>>>> On 01/20/2017 10:56 PM, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxx>
>>>>> ---
>>>>> 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.
>
> I of course agree with your nice usbport trigger driver summary.
>
>
>> Therefore it isn't logically justified to rename usb-ports property
>> to led-triggers. The property should be specific to this trigger.
>
> Only if you mean to limit this property to the usbport trigger driver.
> Rob wants to have this property cover other cases and I mean to work
> on more trigger drivers using it as well. So I hope we can look at a
> bigger picture focusing on a nice design that will allow reusing this
> property in other places.
>
> I'd like to work on netdev / netif / netiface trigger driver in the
> future. I was going to use this DT property there as well.
>
> Sorry, I should have make it clear from the beginning I mean to re-use
> "led-triggers" in other trigger drivers as well.
>
>
>>> 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.
>>> I think we could live with such limitation in Linux support for this
>>> generic "led-triggers" property. I think it's also not very common to
>>> have LED with different types of devices assigned. Personally I've
>>> never seen devices with LED label "USB & CPU" or whatever.
>>
>> I agree. Nonetheless led-triggers seems to support such a design.
>
> That's right. DT property "led-triggers" supports by design more than
> Linux drivers can handle right now. That was my main point of this
> e-mail.
Having my above explanation, I see your led-triggers property as a means
for limiting the scope of LED Triggers available for a LED class
device. Physical usb port, not being a trigger on its own, wouldn't
fall into this category.
>>> What do you think about this? Maybe we could also document this
>>> limitation in trigger docs.
>>
>> I'm not entirely sure if I got your point. Your reasoning seems
>> self contradicting to me in few places.
>
> I'm sorry, I obviously had to fail at some point explaining my idea.
> Let's see if this e-mail made it clearer.
>
--
Best regards,
Jacek Anaszewski
