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

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 02/01/2017 10:55 PM, Rafał Miłecki wrote:
> On 02/01/2017 10:26 PM, Jacek Anaszewski wrote:
>> 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:
>>>>> 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.
> 
> This matches my knowledge as well.

Having that, we would be limited to a list of fixed size
tuples IMHO.

> 
>>> 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.
> 
> I'm OK with reworking Linux's triggers if it need be. I think we should
> agree
> on binding(s) first though.

I was thinking of creating entirely new mechanism (new sysfs file),
as we can't break existing users.


> 
>> 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?
> 
> That was exactly my point with above trigger-sources example including 2
> USB
> ports & GPIO.

How usbport trigger would make use use of information about GPIO?
Does it mean that in this approach triggers would arbitrarily choose
trigger-sources applicable for them?

> 
>> 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?
> 
> That would simplify things, but it gets us back to *multiple* properties
> like
> led-usb-triggers (or led-usb-trigger-sources)
> led-pci-triggers (or led-pci-trigger-sources)
> led-gpio-triggers (or led-gpio-trigger-sources)
> etc.

Right, I was rather skeptical in my question - I don't like this
solution too.

> Last time Rob said he's not going to accept something like this, see:
> 
> On 23 January 2017 at 17:45, Rob Herring <robh@kernel.org> wrote:
>> I'm not going to accept something specific to USB. So the choice is make
>> the existing property work for USB or design something more flexible and
>> generic.
> 
> So I think the main question right now is if this is possible to support
> mixed
> entries in a list like that
> trigger-sources =
>     <&ohci_port1>,
>     <&ehci_port1>,
>     <&gpio 1 GPIO_ACTIVE_HIGH>;
> 
> I posted as example.
> 
> I was also trying to find help on IRC #devicetree channel but didn't get
> any
> response:
> [09:15] <rmilecki> hi, i've a question about mixing various entries in a
> single list
> [09:16] <rmilecki> is this possible to have different /syntax/ in every
> list element
> [09:16] <rmilecki> and have driver detect what does it reference?
> [09:16] <rmilecki> i'll post an example
> [09:17] <rmilecki> trigger-sources = <&ohci_port1>, <&ehci_port1>,
> <&gpio 1 GPIO_ACTIVE_HIGH>;
> [09:18] <rmilecki> this question is related to my work in [PATCH V2 1/2]
> dt-bindings: leds: document new led-triggers property
> [09:18] <rmilecki> beware, there is a long discussion in that thread
> 
> Rob: do you still follow this thread? We'd like to use single property
> as you
> suggested, but is it possible to work with something like trigger-sources
> example posted above?
> 

-- 
Best regards,
Jacek Anaszewski