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

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

[Rafał Miłecki]

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 (assuming 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.
+
 Required properties for flash LED child nodes:
 - flash-max-microamp : Maximum flash LED supply current in microamperes.
 - flash-max-timeout-us : Maximum timeout in microseconds after which the flash
@@ -69,6 +80,11 @@ gpio-leds {
 		linux,default-trigger = "heartbeat";
 		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
 	};
+
+	usb {
+		gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
+		led-triggers = <&ohci_port1>, <&ehci_port1>;
+	};
 };
 
 max77693-led {
-- 
2.11.0

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

[PATCH V2 2/2] usb: core: read USB ports from DT in the usbport LED trigger driver

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This adds support for using description of relation between LEDs and USB
ports from device tree. If DT has properly described LEDs, trigger will
know when to turn them on.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update to use "led-triggers"
---
 drivers/usb/core/ledtrig-usbport.c | 54 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/usb/core/ledtrig-usbport.c b/drivers/usb/core/ledtrig-usbport.c
index 1713248ab15a..aee80748eb7a 100644
--- a/drivers/usb/core/ledtrig-usbport.c
+++ b/drivers/usb/core/ledtrig-usbport.c
@@ -11,8 +11,10 @@
 #include <linux/device.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/usb.h>
+#include <linux/usb/of.h>
 
 struct usbport_trig_data {
 	struct led_classdev *led_cdev;
@@ -123,6 +125,55 @@ static const struct attribute_group ports_group = {
  * Adding & removing ports
  ***************************************/
 
+/**
+ * usbport_trig_port_observed - Check if port should be observed
+ */
+static bool usbport_trig_port_observed(struct usbport_trig_data *usbport_data,
+				       struct usb_device *usb_dev, int port1)
+{
+	struct device *dev = usbport_data->led_cdev->dev;
+	struct device_node *led_np = dev->of_node;
+	struct of_phandle_args args;
+	struct device_node *port_np;
+	int count, i;
+
+	if (!led_np)
+		return false;
+
+	/* Get node of port being added */
+	port_np = usb_of_get_child_node(usb_dev->dev.of_node, port1);
+	if (!port_np)
+		return false;
+
+	/* Amount of ports this LED references */
+	count = of_count_phandle_with_args(led_np, "led-triggers", NULL);
+	if (count < 0) {
+		dev_warn(dev, "Failed to get USB ports for %s\n",
+			 led_np->full_name);
+		return false;
+	}
+
+	/* Check if port is on this LED's list */
+	for (i = 0; i < count; i++) {
+		int err;
+
+		err = of_parse_phandle_with_args(led_np, "led-triggers", NULL,
+						 i, &args);
+		if (err) {
+			dev_err(dev, "Failed to get USB port phandle at index %d: %d\n",
+				i, err);
+			continue;
+		}
+
+		of_node_put(args.np);
+
+		if (args.np == port_np)
+			return true;
+	}
+
+	return false;
+}
+
 static int usbport_trig_add_port(struct usbport_trig_data *usbport_data,
 				 struct usb_device *usb_dev,
 				 const char *hub_name, int portnum)
@@ -141,6 +192,8 @@ static int usbport_trig_add_port(struct usbport_trig_data *usbport_data,
 	port->data = usbport_data;
 	port->hub = usb_dev;
 	port->portnum = portnum;
+	port->observed = usbport_trig_port_observed(usbport_data, usb_dev,
+						    portnum);
 
 	len = strlen(hub_name) + 8;
 	port->port_name = kzalloc(len, GFP_KERNEL);
@@ -255,6 +308,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
 	if (err)
 		goto err_free;
 	usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports);
+	usbport_trig_update_count(usbport_data);
 
 	/* Notifications */
 	usbport_data->nb.notifier_call = usbport_trig_notify,
-- 
2.11.0

[EXAMPLE V2 3/2] ARM: BCM53573: Specify ports for USB LED for Tenda AC9

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This patch *should not* be applied. It's only an EXAMPLE and that's why it uses
weird 3/2 number.

It's a proof of concept, it was tested & will be submitted through ARM tree if
previous patches get accepted.
---
 arch/arm/boot/dts/bcm47189-tenda-ac9.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/bcm47189-tenda-ac9.dts b/arch/arm/boot/dts/bcm47189-tenda-ac9.dts
index 4403ae8790c2..f484d17a2270 100644
--- a/arch/arm/boot/dts/bcm47189-tenda-ac9.dts
+++ b/arch/arm/boot/dts/bcm47189-tenda-ac9.dts
@@ -24,6 +24,7 @@
 		compatible = "gpio-leds";
 
 		usb {
+			led-triggers = <&ohci_port1>, <&ehci_port1>;
 			label = "bcm53xx:blue:usb";
 			gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
 			linux,default-trigger = "default-off";
-- 
2.11.0

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

[Jacek Anaszewski]

Hi Rafał,

On 01/20/2017 10:56 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> 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@milecki.pl>
> ---
> 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.

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.

>  Required properties for flash LED child nodes:
>  - flash-max-microamp : Maximum flash LED supply current in microamperes.
>  - flash-max-timeout-us : Maximum timeout in microseconds after which the flash
> @@ -69,6 +80,11 @@ gpio-leds {
>  		linux,default-trigger = "heartbeat";
>  		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
>  	};
> +
> +	usb {
> +		gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> +		led-triggers = <&ohci_port1>, <&ehci_port1>;
> +	};
>  };
>  
>  max77693-led {
> 

-- 
Best regards,
Jacek Anaszewski

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

[Rafał Miłecki]

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.

I think we owe some extra explanation to people not closely familiar
with the triggers.

Linux has multiple trigger drivers each supporting some *type* of
events. Linux supports assigning only one trigger driver to LED at a
time.
This means you can use e.g.:
1) "usbport" trigger driver that supports USB events
XOR
2) "timer" trigger driver that uses timer to control LED
XOR
3) "cpu" trigger driver that supports CPU events
at once.

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

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.

What do you think about this? Maybe we could also document this
limitation in trigger docs.
--
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

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

[Jacek Anaszewski]

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.
> 
> I think we owe some extra explanation to people not closely familiar
> with the triggers.
> 
> Linux has multiple trigger drivers each supporting some *type* of
> events. Linux supports assigning only one trigger driver to LED at a
> time.
> This means you can use e.g.:
> 1) "usbport" trigger driver that supports USB events
> XOR
> 2) "timer" trigger driver that uses timer to control LED
> XOR
> 3) "cpu" trigger driver that supports CPU events
> at once.

Correct.

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

Therefore it isn't logically justified to rename usb-ports property
to led-triggers. The property should be specific to this trigger.

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

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

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

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

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

[Rafał Miłecki]

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.

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.


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


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

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jacek Anaszewski]

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

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

[Rafał Miłecki]

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@gmail.com> wrote:
>>> On 01/21/2017 05:24 PM, Rafał Miłecki wrote:
>>>> On 20 January 2017 at 23:35, Jacek Anaszewski
>>>> <jacek.anaszewski@gmail.com> wrote:
>>>>> On 01/20/2017 10:56 PM, Rafał Miłecki wrote:
>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>
>>>>>> 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@milecki.pl>
>>>>>> ---
>>>>>> 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 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"
Do you think any of them would work? Or can you think of any better name?

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

[Jacek Anaszewski]

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

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

[Rafał Miłecki]

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@gmail.com> wrote:
>>>>> On 01/21/2017 05:24 PM, Rafał Miłecki wrote:
>>>>>> On 20 January 2017 at 23:35, Jacek Anaszewski
>>>>>> <jacek.anaszewski@gmail.com> wrote:
>>>>>>> On 01/20/2017 10:56 PM, Rafał Miłecki wrote:
>>>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>>>
>>>>>>>> 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@milecki.pl>
>>>>>>>> ---
>>>>>>>> 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.

OK, let's try "trigger-sources" then!

I'll rename this property and send V3 with hopefully a better description.

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

[Rafał Miłecki]

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

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

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

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

[Jacek Anaszewski]

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-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.
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafał Miłecki]

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.


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


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


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

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?

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

[Jacek Anaszewski]

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

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

[Rafał Miłecki]

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.


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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

Hi!

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

Actually... I'm not sure I like the "multiple sources". It is somehow
justified for ohci/ehci_port, because they... represent single
physical port. Could we introduce something for the physical port into
the DTS, too?

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

I am not sure if variable-length parameters here is good idea. Would
be nice to keep it simple... Having the led representing voltage on
gpio line is somehow strange to me. I'd rather have dts explaining
what that voltage means ("it is battery charging signal") and than
have led connected to that...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jacek Anaszewski]

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

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

[Rob Herring]

On Wed, Feb 1, 2017 at 3:55 PM, Rafał Miłecki <zajec5@gmail.com> 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.

+1 for the name.

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

It is a bit problematic, but this could work as long as you know the
possible arg types and a given node only has 1 match (nodes with
interrupt and gpio being likely). You look up the phandle, iterate
thru possible "#*-cells" properties for that phandle, then parse the
cells. You could define rules of what is the required cell type (e.g.
must use gpio cells if a phandle has both gpio and interrupt cells
defined.).

Another, simpler approach would be defining #trigger-cells in each
source. This is probably the better option as all the infrastructure
is there and could handle unforeseen cases.

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

Yes, they are separate problems.

Rob

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

[Rob Herring]

On Fri, Jan 20, 2017 at 11:35:20PM +0100, Jacek Anaszewski wrote:
> Hi Rafał,
> 
> 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.

Not really relevant for designing (and future proofing) a binding.

> There is linux,default-trigger property which allows to define one
> trigger that will be initially assigned.
> 
> 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 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.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jacek Anaszewski]

On 01/23/2017 05:45 PM, Rob Herring wrote:
> On Fri, Jan 20, 2017 at 11:35:20PM +0100, Jacek Anaszewski wrote:
>> Hi Rafał,
>>
>> On 01/20/2017 10:56 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> 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@milecki.pl>
>>> ---
>>> 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.
> 
> Not really relevant for designing (and future proofing) a binding.

But relevant for LED Trigger ABI, which would have to be changed to
support the semantics in this form. I think that changing the property
name to linux,trigger-sources would make its purpose more clear.

Note that we have to also associate somehow this property with the
triggers that will make use of the information it provides. I can
imagine also other compound triggers that could benefit on it.

What follows, the trigger sources would trigger LED activity only
if the LED class device was assigned appropriate trigger. We need to
define somewhere which triggers support this property.

Trigger specific bindings?

>> There is linux,default-trigger property which allows to define one
>> trigger that will be initially assigned.
>>
>> 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 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.


-- 
Best regards,
Jacek Anaszewski

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

[Rafał Miłecki]

On 01/23/2017 09:51 PM, Jacek Anaszewski wrote:
> On 01/23/2017 05:45 PM, Rob Herring wrote:
>> On Fri, Jan 20, 2017 at 11:35:20PM +0100, Jacek Anaszewski wrote:
>>> Hi Rafał,
>>>
>>> On 01/20/2017 10:56 PM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> 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@milecki.pl>
>>>> ---
>>>> 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.
>>
>> Not really relevant for designing (and future proofing) a binding.
>
> But relevant for LED Trigger ABI, which would have to be changed to
> support the semantics in this form. I think that changing the property
> name to linux,trigger-sources would make its purpose more clear.
>
> Note that we have to also associate somehow this property with the
> triggers that will make use of the information it provides. I can
> imagine also other compound triggers that could benefit on it.
>
> What follows, the trigger sources would trigger LED activity only
> if the LED class device was assigned appropriate trigger. We need to
> define somewhere which triggers support this property.
>
> Trigger specific bindings?

Maybe this is something I'm missing. Why adding this "led-triggers" property
would mean/require ABI breakage?

AFAIU adding support for "led-triggers" to the "usbport" trigger driver (see
patch 2/2) didn't break anything, I hope?

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

[Jacek Anaszewski]

On 01/25/2017 10:18 AM, Rafał Miłecki wrote:
> On 01/23/2017 09:51 PM, Jacek Anaszewski wrote:
>> On 01/23/2017 05:45 PM, Rob Herring wrote:
>>> On Fri, Jan 20, 2017 at 11:35:20PM +0100, Jacek Anaszewski wrote:
>>>> Hi Rafał,
>>>>
>>>> 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.
>>>
>>> Not really relevant for designing (and future proofing) a binding.
>>
>> But relevant for LED Trigger ABI, which would have to be changed to
>> support the semantics in this form. I think that changing the property
>> name to linux,trigger-sources would make its purpose more clear.
>>
>> Note that we have to also associate somehow this property with the
>> triggers that will make use of the information it provides. I can
>> imagine also other compound triggers that could benefit on it.
>>
>> What follows, the trigger sources would trigger LED activity only
>> if the LED class device was assigned appropriate trigger. We need to
>> define somewhere which triggers support this property.
>>
>> Trigger specific bindings?
> 
> Maybe this is something I'm missing. Why adding this "led-triggers"
> property
> would mean/require ABI breakage?

As I explained in the other message, LED Triggers are drivers registered
in the LED Trigger core. LED subsystem allows for only one active
trigger at a time (triggers sysfs file reports selected trigger by
wrapping its name with square brackets on the space separated list
of all available LED Triggers).

The problem is in the property name, which can easily lead to wrong
conclusions for the reader not familiar with the LED Trigger core
design.

> AFAIU adding support for "led-triggers" to the "usbport" trigger driver
> (see
> patch 2/2) didn't break anything, I hope?
> 

No, it didn't, but ambiguity in the documentation can lead to
misunderstanding and hinder learning LED subsystem API.

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

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

[Rafał Miłecki]

On 01/23/2017 05:45 PM, Rob Herring wrote:
> On Fri, Jan 20, 2017 at 11:35:20PM +0100, Jacek Anaszewski wrote:
>> Hi Rafał,
>>
>> 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.
>
> Not really relevant for designing (and future proofing) a binding.

This is what I really like in this "led-triggers" property: it's future proof. I
will be able to reuse it in other trigger drivers and I won't be trying to sneak
properties like
* usb-port-triggers
* net-interface-triggers
* cpu-triggers
etc.

Saying that thanks Rob for rejecting my initial "usb-ports" property try :P
--
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

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

[Jacek Anaszewski]

On 01/25/2017 10:08 AM, Rafał Miłecki wrote:
> On 01/23/2017 05:45 PM, Rob Herring wrote:
>> On Fri, Jan 20, 2017 at 11:35:20PM +0100, Jacek Anaszewski wrote:
>>> Hi Rafał,
>>>
>>> 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.
>>
>> Not really relevant for designing (and future proofing) a binding.
> 
> This is what I really like in this "led-triggers" property: it's future
> proof. I
> will be able to reuse it in other trigger drivers and I won't be trying
> to sneak
> properties like
> * usb-port-triggers
> * net-interface-triggers
> * cpu-triggers
> etc.

Very well, but led-triggers in this approach are not LED trigger drivers
but sources of events. Therefore I proposed trigger-sources.

> Saying that thanks Rob for rejecting my initial "usb-ports" property try :P
> .
> 

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