On 02/01/2017 04:56 PM, Rafał Miłecki wrote:
> On 01/31/2017 10:34 PM, Jacek Anaszewski wrote:
>> 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@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.
>>>>>>
>>>>>>> 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.
>
> I'd leave timer for later discussion but I'm glad you brought this problem.
> Maybe we could discuss this on another example that is more supported like
> GPIOs?
>
> We know this property allows specifying USB ports and we want it to support
> mixing various sources. So a very simple sample case seems to be using
> it for
> specifying GPIO as trigger source.
>
> Is this possible to mix various entries in a list assigned to single
> property?
> Let's say:
> trigger-sources =
> <&ohci_port1>,
> <&ehci_port1>,
> <&gpio 1 GPIO_ACTIVE_HIGH>;
According to my knowledge all elements in the list are elements
of one array, no matter if they are comma separated or space separated
in "<>" brackets. DT maintainer would have to confirm that though.
> Is this possible to support this? If so, which function I can use to detect
> what is pointed by a phandle?
> I'm not that familiar with DT and I'm not sure how to handle this.
I wonder if this is correct way to go. Maybe we should think of
creating entirely new trigger mechanism that would allow for having
multiple active triggers at a time.
Of course trigger-sources would be still useful for defining
a list of devices of the same type like in case of usbport trigger.
Nonetheless, I have a problem with this property being a part of
LED class device DT bindings. Triggers are not tightly coupled with
LED class devices, but trigger-sources being a list of usb ports
would be applicable only to usbport trigger. What if there was
another combo trigger e.g. for reporting activity on mulitple GPIOs?
Should we have separate list of trigger sources per any possible
existing trigger type? Aren't we going to far from pure hardware
description the Device Tree was initially predestined to?
--
Best regards,
Jacek Anaszewski
