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

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

On 02/03/2017 12:00 AM, Rafał Miłecki wrote:
> On 02/02/2017 09:44 PM, Jacek Anaszewski wrote:
>> 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.
> 
> That sounds OK. Now I spent some time thinking about this I think it can
> work.
> First of all we may need something like #sources-cells to extend our
> property
> in the future.
> Secondly it should be possible to detect type of phandle node by trying
> things
> one by one. We should be e.g. able to check is phandle is for GPIO by
> trying
> of_find_gpiochip_by_xlate.
> 
> 
>>>>> 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.
> 
> Agree.
> 
> 
>>>> 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?
> 
> It wouldn't. It would simply ignore it.
> 
> We could modify ledtrig-gpio.c however to support this new property
> (ex-"led-triggers") and make use of specified GPIO(s). Of course
> ledtrig-gpio
> would ignore specified USB ports. One type per LED trigger driver.

Frankly speaking I don't like this too much. It would be hard
to infer from the bindings which trigger sources will be used by which
trigger.

I'd like to propose something different, i.e. trigger specific
trigger-sources property, but defined in a separate DT node
for each LED trigger:

led1-usbport-trig: usbport-trig {
	trigger-type = "usbport";
	trigger-sources = <&usb-port1>, <&usb-port2>, <usb-port3>;
	led-dev = <&led1>;
};

led1-compound-gpio-trig: compound-gpio-trig {
	trigger-type = "oneshot";
	trigger-sources = <&gpio1>, <&gpio2>, <&gpio3>;
	led-dev = <&led1>;
};

led1: led1 {
	compound-triggers = <&led1-usbport-trig>. <&led1-compound-gpio-trig>;
}

This way when applying usbport trigger to led1, we would know how
to configure it for this particular LED. If we had led2, we could
define different subset of usb ports to trigger it. In case of
hypothetical compound-gpio trigger, the driver would know
which gpios should generate LED events.

This is a freshly devised idea, just a subject for a discussion.

Best regards,
Jacek Anaszewski

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