[PATCH RESEND 0/5] dt-bindings: support Ethernet devices as LED triggers

[PATCH RESEND 0/5] dt-bindings: support Ethernet devices as LED triggers

[Rafał Miłecki]

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

Some LEDs are designed to represent a state of another device. That may
be USB port, Ethernet interface, CPU, hard drive and more.

We already have support for LEDs that are designed to indicate USB port
(e.g. light on when USB device gets connected). There is DT binding for
that and Linux implementation in USB trigger.

This patchset adds support for describing LEDs that should react to
Ethernet interface status. That is commonly used in routers. They often
have LED to display state and activity of selected physical port. It's
also common to have multiple LEDs, each reacting to a specific link
speed.

Patch 5/5 is proof of concept and is not meant to be applied yet.

Rafał Miłecki (5):
  dt-bindings: net: add bitfield defines for Ethernet speeds
  dt-bindings: net: allow Ethernet devices as LED triggers
  dt-bindings: leds: add Ethernet triggered LEDs to example
  ARM: dts: BCM5301X: Add triggers for Luxul XWR-1200 network LEDs
  leds: trigger: netdev: support DT "trigger-sources" property

 .../devicetree/bindings/leds/common.yaml      | 21 +++++++++++++++
 .../bindings/net/ethernet-controller.yaml     |  3 +++
 arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts | 22 +++++++++++----
 drivers/leds/trigger/ledtrig-netdev.c         | 26 ++++++++++++++++++
 include/dt-bindings/net/eth.h                 | 27 +++++++++++++++++++
 5 files changed, 94 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/net/eth.h

-- 
2.34.1

[PATCH RESEND 1/5] dt-bindings: net: add bitfield defines for Ethernet speeds

[Rafał Miłecki]

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

This allows specifying multiple Ethernet speeds in a single DT uint32
value.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 include/dt-bindings/net/eth.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 include/dt-bindings/net/eth.h

diff --git a/include/dt-bindings/net/eth.h b/include/dt-bindings/net/eth.h
new file mode 100644
index 000000000000..89caff09179b
--- /dev/null
+++ b/include/dt-bindings/net/eth.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Device Tree constants for the Ethernet
+ */
+
+#ifndef _DT_BINDINGS_ETH_H
+#define _DT_BINDINGS_ETH_H
+
+#define SPEED_UNSPEC		0
+#define SPEED_10		(1 << 0)
+#define SPEED_100		(1 << 1)
+#define SPEED_1000		(1 << 2)
+#define SPEED_2000		(1 << 3)
+#define SPEED_2500		(1 << 4)
+#define SPEED_5000		(1 << 5)
+#define SPEED_10000		(1 << 6)
+#define SPEED_14000		(1 << 7)
+#define SPEED_20000		(1 << 8)
+#define SPEED_25000		(1 << 9)
+#define SPEED_40000		(1 << 10)
+#define SPEED_50000		(1 << 11)
+#define SPEED_56000		(1 << 12)
+#define SPEED_100000		(1 << 13)
+#define SPEED_200000		(1 << 14)
+#define SPEED_400000		(1 << 15)
+
+#endif
-- 
2.34.1

Re: [PATCH RESEND 1/5] dt-bindings: net: add bitfield defines for Ethernet speeds

[Rob Herring]

On Thu, May 05, 2022 at 03:55:08PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This allows specifying multiple Ethernet speeds in a single DT uint32
> value.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  include/dt-bindings/net/eth.h | 27 +++++++++++++++++++++++++++

ethernet.h

>  1 file changed, 27 insertions(+)
>  create mode 100644 include/dt-bindings/net/eth.h
> 
> diff --git a/include/dt-bindings/net/eth.h b/include/dt-bindings/net/eth.h
> new file mode 100644
> index 000000000000..89caff09179b
> --- /dev/null
> +++ b/include/dt-bindings/net/eth.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

Dual license

> +/*
> + * Device Tree constants for the Ethernet
> + */
> +
> +#ifndef _DT_BINDINGS_ETH_H
> +#define _DT_BINDINGS_ETH_H
> +
> +#define SPEED_UNSPEC		0
> +#define SPEED_10		(1 << 0)
> +#define SPEED_100		(1 << 1)
> +#define SPEED_1000		(1 << 2)
> +#define SPEED_2000		(1 << 3)
> +#define SPEED_2500		(1 << 4)
> +#define SPEED_5000		(1 << 5)
> +#define SPEED_10000		(1 << 6)
> +#define SPEED_14000		(1 << 7)
> +#define SPEED_20000		(1 << 8)
> +#define SPEED_25000		(1 << 9)
> +#define SPEED_40000		(1 << 10)
> +#define SPEED_50000		(1 << 11)
> +#define SPEED_56000		(1 << 12)
> +#define SPEED_100000		(1 << 13)
> +#define SPEED_200000		(1 << 14)
> +#define SPEED_400000		(1 << 15)

These should probably have some namespace. ETH_*?
> +
> +#endif
> -- 
> 2.34.1
> 
> 

[PATCH RESEND 2/5] dt-bindings: net: allow Ethernet devices as LED triggers

[Rafał Miłecki]

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

This allows specifying Ethernet interfaces and switch ports as triggers
for LEDs activity.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 Documentation/devicetree/bindings/net/ethernet-controller.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 4f15463611f8..ebeb4446d253 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -232,6 +232,9 @@ properties:
           required:
             - speed
 
+allOf:
+  - $ref: /schemas/leds/trigger-source.yaml
+
 additionalProperties: true
 
 ...
-- 
2.34.1

Re: [PATCH RESEND 2/5] dt-bindings: net: allow Ethernet devices as LED triggers

[Rob Herring]

On Thu, May 05, 2022 at 03:55:09PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This allows specifying Ethernet interfaces and switch ports as triggers
> for LEDs activity.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  Documentation/devicetree/bindings/net/ethernet-controller.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 4f15463611f8..ebeb4446d253 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -232,6 +232,9 @@ properties:
>            required:
>              - speed
>  
> +allOf:
> +  - $ref: /schemas/leds/trigger-source.yaml

There's no need to add this here. A device binding still has to list 
'#trigger-source-cells' and set it's value to 0 or 1 cell.

Rob

[PATCH RESEND 3/5] dt-bindings: leds: add Ethernet triggered LEDs to example

[Rafał Miłecki]

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

This adds 3 entries to existing example:
1. LED triggered by switch port in 10 / 100 Mbps link state
2. LED triggered by switch port in 1000 Mbps link
3. LED triggered by Ethernet interface (any speed)

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/leds/common.yaml      | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
index 328952d7acbb..6c72121a1656 100644
--- a/Documentation/devicetree/bindings/leds/common.yaml
+++ b/Documentation/devicetree/bindings/leds/common.yaml
@@ -168,6 +168,7 @@ examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
     #include <dt-bindings/leds/common.h>
+    #include <dt-bindings/net/eth.h>
 
     led-controller {
         compatible = "gpio-leds";
@@ -183,6 +184,26 @@ examples:
             gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
             trigger-sources = <&ohci_port1>, <&ehci_port1>;
         };
+
+        led-2 {
+            function = LED_FUNCTION_WAN;
+            color = <LED_COLOR_ID_AMBER>;
+            gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
+            trigger-sources = <&wan_port (SPEED_10 | SPEED_100)>;
+        };
+
+        led-3 {
+            function = LED_FUNCTION_WAN;
+            color = <LED_COLOR_ID_GREEN>;
+            gpios = <&gpio0 3 GPIO_ACTIVE_LOW>;
+            trigger-sources = <&wan_port SPEED_1000>;
+        };
+
+        led-4 {
+            function = LED_FUNCTION_LAN;
+            gpios = <&gpio0 4 GPIO_ACTIVE_LOW>;
+            trigger-sources = <&gmac 0>;
+        };
     };
 
   - |
-- 
2.34.1

[PATCH RESEND 4/5] ARM: dts: BCM5301X: Add triggers for Luxul XWR-1200 network LEDs

[Rafał Miłecki]

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

Those LEDs are not hardware controlled so bootloader / operating system
may want to control them manually depending on switch ports state.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts | 22 ++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts b/arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts
index 9316a36434f7..ee1b0fd3eb86 100644
--- a/arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts
+++ b/arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts
@@ -5,6 +5,8 @@
 
 /dts-v1/;
 
+#include <dt-bindings/net/eth.h>
+
 #include "bcm47081.dtsi"
 #include "bcm5301x-nand-cs0-bch4.dtsi"
 
@@ -38,24 +40,28 @@ power {
 		lan3 {
 			label = "bcm53xx:green:lan3";
 			gpios = <&chipcommon 1 GPIO_ACTIVE_LOW>;
+			trigger-sources = <&port_lan3 SPEED_UNSPEC>;
 			linux,default-trigger = "none";
 		};
 
 		lan4 {
 			label = "bcm53xx:green:lan4";
 			gpios = <&chipcommon 2 GPIO_ACTIVE_LOW>;
+			trigger-sources = <&port_lan4 SPEED_UNSPEC>;
 			linux,default-trigger = "none";
 		};
 
 		wan {
 			label = "bcm53xx:green:wan";
 			gpios = <&chipcommon 3 GPIO_ACTIVE_LOW>;
+			trigger-sources = <&port_wan SPEED_UNSPEC>;
 			linux,default-trigger = "none";
 		};
 
 		lan2 {
 			label = "bcm53xx:green:lan2";
 			gpios = <&chipcommon 6 GPIO_ACTIVE_LOW>;
+			trigger-sources = <&port_lan2 SPEED_UNSPEC>;
 			linux,default-trigger = "none";
 		};
 
@@ -87,6 +93,7 @@ status {
 		lan1 {
 			label = "bcm53xx:green:lan1";
 			gpios = <&chipcommon 15 GPIO_ACTIVE_LOW>;
+			trigger-sources = <&port_lan1 SPEED_UNSPEC>;
 			linux,default-trigger = "none";
 		};
 	};
@@ -114,29 +121,34 @@ &srab {
 	status = "okay";
 
 	ports {
-		port@0 {
+		port_lan4: port@0 {
 			reg = <0>;
 			label = "lan4";
+			#trigger-source-cells = <1>;
 		};
 
-		port@1 {
+		port_lan3: port@1 {
 			reg = <1>;
 			label = "lan3";
+			#trigger-source-cells = <1>;
 		};
 
-		port@2 {
+		port_lan2: port@2 {
 			reg = <2>;
 			label = "lan2";
+			#trigger-source-cells = <1>;
 		};
 
-		port@3 {
+		port_lan1: port@3 {
 			reg = <3>;
 			label = "lan1";
+			#trigger-source-cells = <1>;
 		};
 
-		port@4 {
+		port_wan: port@4 {
 			reg = <4>;
 			label = "wan";
+			#trigger-source-cells = <1>;
 		};
 
 		port@5 {
-- 
2.34.1

[PATCH RESEND PoC] leds: trigger: netdev: support DT "trigger-sources" property

[Rafał Miłecki]

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

Parse "trigger-sources", find referenced netdev & use it as default
trigger source.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This is a proof on concept, please don't apply this patch. I'll rework
this on top of Ansuel's netdev trigger refactoring once it gets accepted
----
 drivers/leds/trigger/ledtrig-netdev.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d83021..c036a3671773 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,6 +20,8 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/of_net.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include "../leds.h"
@@ -389,6 +391,28 @@ static void netdev_trig_work(struct work_struct *work)
 			(atomic_read(&trigger_data->interval)*2));
 }
 
+static void netdev_trig_of_init(struct led_classdev *led_cdev)
+{
+	struct device *dev = led_cdev->dev;
+	struct device_node *np = dev->of_node;
+	struct of_phandle_args args;
+	struct net_device *netdev;
+	int err;
+
+	err = of_parse_phandle_with_args(np, "trigger-sources", "#trigger-source-cells", 0, &args);
+	if (err || WARN_ON(!args.np)) {
+		dev_err(dev, "Failed to get trigger source phandle: %d\n", err);
+		return;
+	}
+
+	netdev = of_find_net_device_by_node(args.np);
+	if (netdev) {
+		device_name_store(dev, NULL, netdev->name, strlen(netdev->name) + 1);
+	}
+
+	of_node_put(args.np);
+}
+
 static int netdev_trig_activate(struct led_classdev *led_cdev)
 {
 	struct led_netdev_data *trigger_data;
@@ -415,6 +439,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 
 	led_set_trigger_data(led_cdev, trigger_data);
 
+	netdev_trig_of_init(led_cdev);
+
 	rc = register_netdevice_notifier(&trigger_data->notifier);
 	if (rc)
 		kfree(trigger_data);
-- 
2.34.1

Re: [PATCH RESEND 0/5] dt-bindings: support Ethernet devices as LED triggers

[Ansuel Smith]

On Thu, May 05, 2022 at 03:55:07PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Some LEDs are designed to represent a state of another device. That may
> be USB port, Ethernet interface, CPU, hard drive and more.
> 
> We already have support for LEDs that are designed to indicate USB port
> (e.g. light on when USB device gets connected). There is DT binding for
> that and Linux implementation in USB trigger.
> 
> This patchset adds support for describing LEDs that should react to
> Ethernet interface status. That is commonly used in routers. They often
> have LED to display state and activity of selected physical port. It's
> also common to have multiple LEDs, each reacting to a specific link
> speed.
>

I notice this is specific to ethernet speed... I wonder if we should
expand this also to other thing like duplex state or even rx/tx.

> Patch 5/5 is proof of concept and is not meant to be applied yet.
> 
> Rafał Miłecki (5):
>   dt-bindings: net: add bitfield defines for Ethernet speeds
>   dt-bindings: net: allow Ethernet devices as LED triggers
>   dt-bindings: leds: add Ethernet triggered LEDs to example
>   ARM: dts: BCM5301X: Add triggers for Luxul XWR-1200 network LEDs
>   leds: trigger: netdev: support DT "trigger-sources" property
> 
>  .../devicetree/bindings/leds/common.yaml      | 21 +++++++++++++++
>  .../bindings/net/ethernet-controller.yaml     |  3 +++
>  arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts | 22 +++++++++++----
>  drivers/leds/trigger/ledtrig-netdev.c         | 26 ++++++++++++++++++
>  include/dt-bindings/net/eth.h                 | 27 +++++++++++++++++++
>  5 files changed, 94 insertions(+), 5 deletions(-)
>  create mode 100644 include/dt-bindings/net/eth.h
> 
> -- 
> 2.34.1
> 

-- 
	Ansuel

Re: [PATCH RESEND 0/5] dt-bindings: support Ethernet devices as LED triggers

[Rafał Miłecki]

On 5.05.2022 16:02, Ansuel Smith wrote:
> On Thu, May 05, 2022 at 03:55:07PM +0200, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Some LEDs are designed to represent a state of another device. That may
>> be USB port, Ethernet interface, CPU, hard drive and more.
>>
>> We already have support for LEDs that are designed to indicate USB port
>> (e.g. light on when USB device gets connected). There is DT binding for
>> that and Linux implementation in USB trigger.
>>
>> This patchset adds support for describing LEDs that should react to
>> Ethernet interface status. That is commonly used in routers. They often
>> have LED to display state and activity of selected physical port. It's
>> also common to have multiple LEDs, each reacting to a specific link
>> speed.
>>
> 
> I notice this is specific to ethernet speed... I wonder if we should
> expand this also to other thing like duplex state or even rx/tx.

I didn't see any router with separated Rx/Tx LEDs, but it still sounds
like a valid case.

We could add flags for that in proposed field like:
trigger-sources = <&port (SPEED_1000 | LINK | TX)>;

Or add separated field for non-speed flags like:
trigger-sources = <&port SPEED_1000 (LINK | TX)>;

Let's see what DT experts say about it.

Re: [PATCH RESEND 0/5] dt-bindings: support Ethernet devices as LED triggers

[Ansuel Smith]

On Thu, May 05, 2022 at 04:21:33PM +0200, Rafał Miłecki wrote:
> On 5.05.2022 16:02, Ansuel Smith wrote:
> > On Thu, May 05, 2022 at 03:55:07PM +0200, Rafał Miłecki wrote:
> > > From: Rafał Miłecki <rafal@milecki.pl>
> > > 
> > > Some LEDs are designed to represent a state of another device. That may
> > > be USB port, Ethernet interface, CPU, hard drive and more.
> > > 
> > > We already have support for LEDs that are designed to indicate USB port
> > > (e.g. light on when USB device gets connected). There is DT binding for
> > > that and Linux implementation in USB trigger.
> > > 
> > > This patchset adds support for describing LEDs that should react to
> > > Ethernet interface status. That is commonly used in routers. They often
> > > have LED to display state and activity of selected physical port. It's
> > > also common to have multiple LEDs, each reacting to a specific link
> > > speed.
> > > 
> > 
> > I notice this is specific to ethernet speed... I wonder if we should
> > expand this also to other thing like duplex state or even rx/tx.
> 
> I didn't see any router with separated Rx/Tx LEDs, but it still sounds
> like a valid case.
>

Not a normal configuration but it's doable. For qca8k you can really set
the led to do whatever you want.

> We could add flags for that in proposed field like:
> trigger-sources = <&port (SPEED_1000 | LINK | TX)>;
> 
> Or add separated field for non-speed flags like:
> trigger-sources = <&port SPEED_1000 (LINK | TX)>;
> 
> Let's see what DT experts say about it.

-- 
	Ansuel