[PATCH leds v2 00/10] Add support for offloading netdev trigger to HW + example implementation for Turris Omnia

Hello,

this is v2 of series adding support for offloading LED triggers to HW.
The netdev trigger is the first user and leds-turris-omnia is the first
example implementation.

A video comparing SW (left LED) vs HW (right LED) netdev trigger on
Omnia
  https://secure.nic.cz/files/mbehun/omnia-wan-netdev-trig-offload.mp4

Changes since v1:
- changed typo in doc
- the netdev trigger data structure now lives in
  include/linux/ledtrig-netdev.h instead of ledtrig.h, as suggested by
  Andrew. Also the structure is always defined, no guard against
  CONFIG_LEDS_TRIGGER_NETDEV
- we do not export netdev_led_trigger variable. The trigger_offload()
  method can look at led_cdev->trigger->name to see which trigger it
  should try to offload, i.e. compare the string to "netdev"
- netdev trigger is being offloaded only if link is up, and at least one
  of the rx, tx parameters are set. No need to offload otherwise
- a patch is added that moves setting flag LED_UNREGISTERING in
  led_classdev_unregister() before unsetting trigger. This makes it
  possible for the trigger_offload() method to determine whether the
  offloading is being disabled because the LED is being unregistered.
  The driver may put the LED into HW triggering mode in this case, to
  achieve behaviour as was before the driver was loaded
- an example implementation for offloading the netdev trigger for the
  WAN LED on Turris Omnia is added. LAN LEDs are not yet supported

Changes since RFC:
- split the patch adding HW offloading support to netdev trigger into
  several separate patches (suggested by Pavel):
  1. move trigger data structure to include/linux/ledtrig.h
  2. support HW offloading
  3. change spinlock to mutex
- fixed bug where the .offloaded variable was not set to false when
  offloading was disabled (suggested by Pavel)
- removed the code saving one call to set_baseline_state() on the
  NETDEV_CHANGE event. It is not needed, the trigger_offload() method
  can handle this situation on its own (suggested by Pavel)
- documentation now explicitly says that when offloading is being
  disabled, the function must return 0 (no error) (suggested by Pavel)

Marek Behún (10):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: move trigger data structure to global include
    dir
  leds: trigger: netdev: support HW offloading
  leds: trigger: netdev: change spinlock to mutex
  leds: core: inform trigger that it's deactivation is due to LED
    removal
  leds: turris-omnia: refactor sw mode setting code into separate
    function
  leds: turris-omnia: refactor brightness setting function
  leds: turris-omnia: initialize each multicolor LED to white color
  leds: turris-omnia: support offloading netdev trigger for WAN LED

 Documentation/leds/leds-class.rst     |  22 ++
 drivers/leds/Kconfig                  |   3 +
 drivers/leds/led-class.c              |   4 +-
 drivers/leds/led-triggers.c           |   1 +
 drivers/leds/leds-turris-omnia.c      | 284 ++++++++++++++++++++++++--
 drivers/leds/trigger/ledtrig-netdev.c |  56 ++---
 include/linux/leds.h                  |  29 +++
 include/linux/ledtrig-netdev.h        |  34 +++
 8 files changed, 377 insertions(+), 56 deletions(-)
 create mode 100644 include/linux/ledtrig-netdev.h

-- 
2.26.3

The trigger_data struct is allocated with kzalloc, so we do not need to
explicitly set members to zero.

Signed-off-by: Marek Behún <kabel@kernel.org>
Acked-by: Pavel Machek <redacted>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/leds/trigger/ledtrig-netdev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d83021..4f6b73e3b491 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c

@@ -406,12 +406,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	INIT_DELAYED_WORK(&trigger_data->work, netdev_trig_work);
 
 	trigger_data->led_cdev = led_cdev;
-	trigger_data->net_dev = NULL;
-	trigger_data->device_name[0] = 0;
 
-	trigger_data->mode = 0;
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
-	trigger_data->last_activity = 0;
 
 	led_set_trigger_data(led_cdev, trigger_data);
 

-- 
2.26.3

Add method trigger_offload() and member variable `offloaded` to struct
led_classdev. Add helper functions led_trigger_offload() and
led_trigger_offload_stop().

The trigger_offload() method, when implemented by the LED driver, should
be called (via led_trigger_offload() function) from trigger code wanting
to be offloaded at the moment when configuration of the trigger changes.

If the trigger is successfully offloaded, this method returns 0 and the
trigger does not have to blink the LED in software.

If the trigger with given configuration cannot be offloaded, the method
should return -EOPNOTSUPP, in which case the trigger must blink the LED
in SW.

The second argument to trigger_offload() being false means that the
offloading is being disabled. In this case the function must return 0,
errors are not permitted.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 Documentation/leds/leds-class.rst | 22 ++++++++++++++++++++++
 drivers/leds/led-triggers.c       |  1 +
 include/linux/leds.h              | 29 +++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index cd155ead8703..15fa05c14afe 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst

@@ -169,6 +169,28 @@ Setting the brightness to zero with brightness_set() callback function
 should completely turn off the LED and cancel the previously programmed
 hardware blinking function, if any.
 
+Hardware offloading of LED triggers
+===================================
+
+Some LEDs can offload SW triggers to hardware (for example a LED connected to
+an ethernet PHY or an ethernet switch can be configured to blink on activity on
+the network, which in software is done by the netdev trigger).
+
+To do such offloading, both the trigger code and LED driver must support this.
+The LED must implement the trigger_offload() method and the trigger code must
+try to call this method (via led_trigger_offload() function) when configuration
+of the trigger (trigger_data) changes.
+
+The implementation of the trigger_offload() method by the LED driver must return
+0 if the offload is successful and -EOPNOTSUPP if the requested trigger
+configuration is not supported and the trigger should be executed in software.
+If trigger_offload() returns negative value, the triggering will be done in
+software, so any active offloading must also be disabled.
+
+If the second argument (enable) to the trigger_offload() method is false, any
+active HW offloading must be deactivated. In this case errors are not permitted
+in the trigger_offload() method.
+
 
 Known Issues
 ============

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 4e7b78a84149..372980791b87 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c

@@ -177,6 +177,7 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 			flags);
 		cancel_work_sync(&led_cdev->set_brightness_work);
 		led_stop_software_blink(led_cdev);
+		led_trigger_offload_stop(led_cdev);
 		if (led_cdev->trigger->deactivate)
 			led_cdev->trigger->deactivate(led_cdev);
 		device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);

diff --git a/include/linux/leds.h b/include/linux/leds.h
index 329fd914cf24..b331e7bceac3 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h

@@ -148,6 +148,11 @@ struct led_classdev {
 
 	/* LEDs that have private triggers have this set */
 	struct led_hw_trigger_type	*trigger_type;
+
+	/* some LEDs may be able to offload some SW triggers to HW */
+	int		(*trigger_offload)(struct led_classdev *led_cdev,
+					   bool enable);
+	bool			offloaded;
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED

@@ -403,6 +408,30 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 	return led_cdev->trigger_data;
 }
 
+static inline int led_trigger_offload(struct led_classdev *led_cdev)
+{
+	int ret;
+
+	if (!led_cdev->trigger_offload)
+		return -EOPNOTSUPP;
+
+	ret = led_cdev->trigger_offload(led_cdev, true);
+	led_cdev->offloaded = !ret;
+
+	return ret;
+}
+
+static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
+{
+	if (!led_cdev->trigger_offload)
+		return;
+
+	if (led_cdev->offloaded) {
+		led_cdev->trigger_offload(led_cdev, false);
+		led_cdev->offloaded = false;
+	}
+}
+
 /**
  * led_trigger_rename_static - rename a trigger
  * @name: the new trigger name

-- 
2.26.3

In preparation for HW offloading of netdev trigger, move struct
led_netdev_data into global include directory, into file
linux/ledtrig-netdev.h, so that drivers wanting to offload the trigger
can see the requested settings.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 23 +-----------------
 include/linux/ledtrig-netdev.h        | 34 +++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 22 deletions(-)
 create mode 100644 include/linux/ledtrig-netdev.h

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 4f6b73e3b491..9a98f9c5b8d0 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c

@@ -10,17 +10,16 @@
 //  Copyright 2005-2006 Openedhand Ltd.
 //  Author: Richard Purdie <rpurdie@openedhand.com>
 
-#include <linux/atomic.h>
 #include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
+#include <linux/ledtrig-netdev.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
-#include <linux/spinlock.h>
 #include <linux/timer.h>
 #include "../leds.h"
 

@@ -36,26 +35,6 @@
  *
  */
 
-struct led_netdev_data {
-	spinlock_t lock;
-
-	struct delayed_work work;
-	struct notifier_block notifier;
-
-	struct led_classdev *led_cdev;
-	struct net_device *net_dev;
-
-	char device_name[IFNAMSIZ];
-	atomic_t interval;
-	unsigned int last_activity;
-
-	unsigned long mode;
-#define NETDEV_LED_LINK	0
-#define NETDEV_LED_TX	1
-#define NETDEV_LED_RX	2
-#define NETDEV_LED_MODE_LINKUP	3
-};
-
 enum netdev_led_attr {
 	NETDEV_ATTR_LINK,
 	NETDEV_ATTR_TX,

diff --git a/include/linux/ledtrig-netdev.h b/include/linux/ledtrig-netdev.h
new file mode 100644
index 000000000000..d6be039e1247
--- /dev/null
+++ b/include/linux/ledtrig-netdev.h

@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * LED trigger shared structures
+ */
+
+#ifndef __LINUX_LEDTRIG_NETDEV_H__
+#define __LINUX_LEDTRIG_NETDEV_H__
+
+#include <linux/atomic.h>
+#include <linux/leds.h>
+#include <linux/netdevice.h>
+#include <linux/spinlock.h>
+
+struct led_netdev_data {
+	spinlock_t lock;
+
+	struct delayed_work work;
+	struct notifier_block notifier;
+
+	struct led_classdev *led_cdev;
+	struct net_device *net_dev;
+
+	char device_name[IFNAMSIZ];
+	atomic_t interval;
+	unsigned int last_activity;
+
+	unsigned long mode;
+#define NETDEV_LED_LINK		0
+#define NETDEV_LED_TX		1
+#define NETDEV_LED_RX		2
+#define NETDEV_LED_MODE_LINKUP	3
+};
+
+#endif /* __LINUX_LEDTRIG_NETDEV_H__ */

-- 
2.26.3

Add support for HW offloading of the netdev trigger.

We are only offloading if the link is up and rx/tx blinking is
requested.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 9a98f9c5b8d0..1f1b63d5a78d 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c

@@ -52,9 +52,17 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	if (!led_cdev->blink_brightness)
 		led_cdev->blink_brightness = led_cdev->max_brightness;
 
-	if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
+	if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode)) {
+		led_trigger_offload_stop(led_cdev);
 		led_set_brightness(led_cdev, LED_OFF);
-	else {
+	} else {
+		bool blink = test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
+			     test_bit(NETDEV_LED_RX, &trigger_data->mode);
+		/* Try offload to HW only if RX/TX blinking is requested */
+		if (blink)
+			if (!led_trigger_offload(led_cdev))
+				return;
+
 		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
 			led_set_brightness(led_cdev,
 					   led_cdev->blink_brightness);

@@ -64,8 +72,7 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 		/* If we are looking for RX/TX start periodically
 		 * checking stats
 		 */
-		if (test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
-		    test_bit(NETDEV_LED_RX, &trigger_data->mode))
+		if (blink)
 			schedule_delayed_work(&trigger_data->work, 0);
 	}
 }

-- 
2.26.3

Using spinlocks requires that the trigger_offload() method cannot sleep.
This can be problematic for some hardware, since access to registers may
sleep.

We can change the spinlock to mutex because, according to Jacek:
  register_netdevice_notifier() registers raw notifier chain,
  whose callbacks are not called from atomic context and there are
  no restrictions on callbacks. See include/linux/notifier.h.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 14 +++++++-------
 include/linux/ledtrig-netdev.h        |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 1f1b63d5a78d..341e1f174c5b 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c

@@ -83,9 +83,9 @@ static ssize_t device_name_show(struct device *dev,
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	ssize_t len;
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 	len = sprintf(buf, "%s\n", trigger_data->device_name);
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return len;
 }

@@ -101,7 +101,7 @@ static ssize_t device_name_store(struct device *dev,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 
 	if (trigger_data->net_dev) {
 		dev_put(trigger_data->net_dev);

@@ -125,7 +125,7 @@ static ssize_t device_name_store(struct device *dev,
 	trigger_data->last_activity = 0;
 
 	set_baseline_state(trigger_data);
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return size;
 }

@@ -299,7 +299,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 
 	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
 	switch (evt) {

@@ -323,7 +323,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	set_baseline_state(trigger_data);
 
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return NOTIFY_DONE;
 }

@@ -384,7 +384,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	if (!trigger_data)
 		return -ENOMEM;
 
-	spin_lock_init(&trigger_data->lock);
+	mutex_init(&trigger_data->lock);
 
 	trigger_data->notifier.notifier_call = netdev_trig_notify;
 	trigger_data->notifier.priority = 10;

diff --git a/include/linux/ledtrig-netdev.h b/include/linux/ledtrig-netdev.h
index d6be039e1247..b93687ecc801 100644
--- a/include/linux/ledtrig-netdev.h
+++ b/include/linux/ledtrig-netdev.h

@@ -8,11 +8,11 @@
 
 #include <linux/atomic.h>
 #include <linux/leds.h>
+#include <linux/mutex.h>
 #include <linux/netdevice.h>
-#include <linux/spinlock.h>
 
 struct led_netdev_data {
-	spinlock_t lock;
+	struct mutex lock;
 
 	struct delayed_work work;
 	struct notifier_block notifier;

-- 
2.26.3

Move setting of the LED_UNREGISTERING before deactivating the trigger in
led_classdev_unregister().

It can be useful for a LED trigger to know whether it is being
deactivated due to the LED being unregistered. This makes it possible
for LED drivers which implement trigger offloading to leave the LED in
HW triggering mode when the LED is unregistered, instead of disabling
it.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/led-class.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 2e495ff67856..0486129a7f31 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c

@@ -436,6 +436,8 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	if (IS_ERR_OR_NULL(led_cdev->dev))
 		return;
 
+	led_cdev->flags |= LED_UNREGISTERING;
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	down_write(&led_cdev->trigger_lock);
 	if (led_cdev->trigger)

@@ -443,8 +445,6 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	up_write(&led_cdev->trigger_lock);
 #endif
 
-	led_cdev->flags |= LED_UNREGISTERING;
-
 	/* Stop blinking */
 	led_stop_software_blink(led_cdev);
 

-- 
2.26.3

In order to make trigger offloading code more readable, put the code
that sets/unsets software mode into a separate function.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/leds-turris-omnia.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 2f9a289ab245..c5a40afe5d45 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c

@@ -73,6 +73,13 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 	return ret;
 }
 
+static int omnia_led_set_sw_mode(struct i2c_client *client, int led, bool sw)
+{
+	return i2c_smbus_write_byte_data(client, CMD_LED_MODE,
+					 CMD_LED_MODE_LED(led) |
+					 (sw ? CMD_LED_MODE_USER : 0));
+}
+
 static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 			      struct device_node *np)
 {

@@ -114,9 +121,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
 
 	/* put the LED into software mode */
-	ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE,
-					CMD_LED_MODE_LED(led->reg) |
-					CMD_LED_MODE_USER);
+	ret = omnia_led_set_sw_mode(client, led->reg, true);
 	if (ret < 0) {
 		dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np,
 			ret);

@@ -250,8 +255,7 @@ static int omnia_leds_remove(struct i2c_client *client)
 	u8 buf[5];
 
 	/* put all LEDs into default (HW triggered) mode */
-	i2c_smbus_write_byte_data(client, CMD_LED_MODE,
-				  CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
+	omnia_led_set_sw_mode(client, OMNIA_BOARD_LEDS, false);
 
 	/* set all LEDs color to [255, 255, 255] */
 	buf[0] = CMD_LED_COLOR;

-- 
2.26.3

Move the code of brightness setting function guarded by mutex into
separate function. This will be useful when used from trigger offload
method.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/leds-turris-omnia.c | 35 +++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index c5a40afe5d45..2b51c14b8363 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c

@@ -41,32 +41,43 @@ struct omnia_leds {
 	struct omnia_led leds[];
 };
 
-static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
-					     enum led_brightness brightness)
+static int omnia_led_brightness_set(struct i2c_client *client,
+				    struct omnia_led *led,
+				    enum led_brightness brightness)
 {
-	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
-	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
-	struct omnia_led *led = to_omnia_led(mc_cdev);
 	u8 buf[5], state;
 	int ret;
 
-	mutex_lock(&leds->lock);
-
 	led_mc_calc_color_components(&led->mc_cdev, brightness);
 
 	buf[0] = CMD_LED_COLOR;
 	buf[1] = led->reg;
-	buf[2] = mc_cdev->subled_info[0].brightness;
-	buf[3] = mc_cdev->subled_info[1].brightness;
-	buf[4] = mc_cdev->subled_info[2].brightness;
+	buf[2] = led->mc_cdev.subled_info[0].brightness;
+	buf[3] = led->mc_cdev.subled_info[1].brightness;
+	buf[4] = led->mc_cdev.subled_info[2].brightness;
 
 	state = CMD_LED_STATE_LED(led->reg);
 	if (buf[2] || buf[3] || buf[4])
 		state |= CMD_LED_STATE_ON;
 
-	ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_STATE, state);
+	ret = i2c_smbus_write_byte_data(client, CMD_LED_STATE, state);
 	if (ret >= 0 && (state & CMD_LED_STATE_ON))
-		ret = i2c_master_send(leds->client, buf, 5);
+		ret = i2c_master_send(client, buf, 5);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
+					     enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+	struct omnia_led *led = to_omnia_led(mc_cdev);
+	int ret;
+
+	mutex_lock(&leds->lock);
+
+	ret = omnia_led_brightness_set(leds->client, led, brightness);
 
 	mutex_unlock(&leds->lock);
 

-- 
2.26.3

Initialize the intensity of each multi-color LED to white color (255,
255, 255).

This is what the hardware does by default when the driver is not
present.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/leds-turris-omnia.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 2b51c14b8363..b3581b98c75d 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c

@@ -117,10 +117,13 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 
 	led->subled_info[0].color_index = LED_COLOR_ID_RED;
 	led->subled_info[0].channel = 0;
+	led->subled_info[0].intensity = 255;
 	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
 	led->subled_info[1].channel = 1;
+	led->subled_info[1].intensity = 255;
 	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
 	led->subled_info[2].channel = 2;
+	led->subled_info[2].intensity = 255;
 
 	led->mc_cdev.subled_info = led->subled_info;
 	led->mc_cdev.num_colors = OMNIA_LED_NUM_CHANNELS;

-- 
2.26.3

Add support for offloading netdev trigger for WAN LED.

Support for LAN LEDs will be added later, because it needs changes in
the mv88e6xxx driver.

Here is a simplified schema of how the corresponding chips are connected
on Turris Omnia:

                       [eth2]    +-----+   [eth0 & eth1]
                     /-----------< SOC >-----------------\
                     |           +--v--+                 |
                     |              |    [i2c]           |
                     |              \-------------\      |
   [MOD_DEF0] +------v--------+                   |      |
    /---------> SerDes switch |    [LED0_pin]  +--v--+   |  +----------+
    |         +--v-------v----+  /-------------> MCU >---|--> RGB LEDs |
    |    [srds0] |       |       |             +--^--+   |  +----------+
    |    /-------/       |       |                |      |
    |    |        [srds1]|       |      [LED_pins]|      |
  +-^----v---+       +---v-------^---+    +-------^------v-+
  | SFP cage |       |  88E1512 PHY  |    | 88E6176 Swtich |
  +----------+       | with WAN port |    | with LAN ports |
                     +---------------+    +----------------+

The RGB LEDs are controlled by the MCU and can be configured into the
following modes:
- SW mode - both color and whether the LED is on/off is controlled via
            I2C
- HW mode - color is controlled via I2C, on/off state is controlled by
            HW depending on LED:
  - WAN LED on/off state reflects LED0_pin from the 88E1512 PHY
  - LAN LED on/off states reflect corresponding LED_pins from 88E6176
    switch [1]
  - PCIe on/off states reflect the corresponding WWAN/WLAN/MSATA LED
    pins from the MiniPCIe ports [1]
  - Power LED is always on in HW mode
  - User LEDs are always off in HW mode

Adding netdev trigger offload support for the WAN LED therefore
requires:
- checking whether the netdevice for which the netdev trigger should
  trigger is indeed the WAN device
- checking whether SFP cage is empty. If there is a SFP module in the
  cage, the 88E1512 PHY is not used and we have to trigger in SW.
  Currently this is done by simply checking if sfp_bus is NULL, because
  phylink does not yet have support for how the SFP cage is wired on
  Omnia (via SerDes switch)
- configuring the behaviour of LED0_pin of the Marvell 88E1512 PHY
  according to requested netdev trigger settings
- putting the WAN LED into HW mode

[1] For more info look at
    https://wiki.turris.cz/doc/_media/rtrom01-schema.pdf

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/Kconfig             |   3 +
 drivers/leds/leds-turris-omnia.c | 232 +++++++++++++++++++++++++++++++
 2 files changed, 235 insertions(+)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 49d99cb084db..e2950636f093 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig

@@ -182,6 +182,9 @@ config LEDS_TURRIS_OMNIA
 	depends on I2C
 	depends on MACH_ARMADA_38X || COMPILE_TEST
 	depends on OF
+	depends on PHYLIB
+	select LEDS_TRIGGERS
+	select LEDS_TRIGGER_NETDEV
 	help
 	  This option enables basic support for the LEDs found on the front
 	  side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index b3581b98c75d..b9ea0ce261eb 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c

@@ -7,9 +7,11 @@
 
 #include <linux/i2c.h>
 #include <linux/led-class-multicolor.h>
+#include <linux/ledtrig-netdev.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/phy.h>
 #include "leds.h"
 
 #define OMNIA_BOARD_LEDS	12

@@ -27,10 +29,20 @@
 #define CMD_LED_SET_BRIGHTNESS	7
 #define CMD_LED_GET_BRIGHTNESS	8
 
+#define MII_MARVELL_LED_PAGE		0x03
+#define MII_PHY_LED_CTRL		0x10
+#define MII_PHY_LED_TCR			0x12
+#define MII_PHY_LED_TCR_PULSESTR_MASK	0x7000
+#define MII_PHY_LED_TCR_PULSESTR_SHIFT	12
+#define MII_PHY_LED_TCR_BLINKRATE_MASK	0x0700
+#define MII_PHY_LED_TCR_BLINKRATE_SHIFT	8
+
 struct omnia_led {
 	struct led_classdev_mc mc_cdev;
 	struct mc_subled subled_info[OMNIA_LED_NUM_CHANNELS];
 	int reg;
+	struct device_node *trig_src_np;
+	struct phy_device *phydev;
 };
 
 #define to_omnia_led(l)		container_of(l, struct omnia_led, mc_cdev)

@@ -38,6 +50,7 @@ struct omnia_led {
 struct omnia_leds {
 	struct i2c_client *client;
 	struct mutex lock;
+	int count;
 	struct omnia_led leds[];
 };
 

@@ -91,6 +104,208 @@ static int omnia_led_set_sw_mode(struct i2c_client *client, int led, bool sw)
 					 (sw ? CMD_LED_MODE_USER : 0));
 }
 
+static int wan_led_round_blink_rate(unsigned long *period)
+{
+	/* Each interval is (0.7 * p, 1.3 * p), where p is the period supported
+	 * by the chip. Should we change this so that there are no holes between
+	 * these intervals?
+	 */
+	switch (*period) {
+	case 29 ... 55:
+		*period = 42;
+		return 0;
+	case 58 ... 108:
+		*period = 84;
+		return 1;
+	case 119 ... 221:
+		*period = 170;
+		return 2;
+	case 238 ... 442:
+		*period = 340;
+		return 3;
+	case 469 ... 871:
+		*period = 670;
+		return 4;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int omnia_led_trig_offload_wan(struct omnia_leds *leds,
+				      struct omnia_led *led,
+				      struct led_netdev_data *trig)
+{
+	unsigned long period;
+	int ret, blink_rate;
+	bool link, rx, tx;
+	u8 fun;
+
+	/* HW offload on WAN port is supported only via internal PHY */
+	if (trig->net_dev->sfp_bus || !trig->net_dev->phydev)
+		return -EOPNOTSUPP;
+
+	link = test_bit(NETDEV_LED_LINK, &trig->mode);
+	rx = test_bit(NETDEV_LED_RX, &trig->mode);
+	tx = test_bit(NETDEV_LED_TX, &trig->mode);
+
+	if (link && rx && tx)
+		fun = 0x1;
+	else if (!link && rx && tx)
+		fun = 0x4;
+	else
+		return -EOPNOTSUPP;
+
+	period = jiffies_to_msecs(atomic_read(&trig->interval)) * 2;
+	blink_rate = wan_led_round_blink_rate(&period);
+	if (blink_rate < 0)
+		return blink_rate;
+
+	mutex_lock(&leds->lock);
+
+	if (!led->phydev) {
+		led->phydev = trig->net_dev->phydev;
+		get_device(&led->phydev->mdio.dev);
+	}
+
+	/* set PHY's LED[0] pin to blink according to trigger setting */
+	ret = phy_modify_paged(led->phydev, MII_MARVELL_LED_PAGE,
+			       MII_PHY_LED_TCR,
+			       MII_PHY_LED_TCR_PULSESTR_MASK |
+			       MII_PHY_LED_TCR_BLINKRATE_MASK,
+			       (0 << MII_PHY_LED_TCR_PULSESTR_SHIFT) |
+			       (blink_rate << MII_PHY_LED_TCR_BLINKRATE_SHIFT));
+	if (ret)
+		goto unlock;
+
+	ret = phy_modify_paged(led->phydev, MII_MARVELL_LED_PAGE,
+			       MII_PHY_LED_CTRL, 0xf, fun);
+	if (ret)
+		goto unlock;
+
+	/* put the LED into HW mode */
+	ret = omnia_led_set_sw_mode(leds->client, led->reg, false);
+	if (ret)
+		goto unlock;
+
+	/* set blinking brightness according to led_cdev->blink_brighness */
+	ret = omnia_led_brightness_set(leds->client, led,
+				       led->mc_cdev.led_cdev.blink_brightness);
+	if (ret)
+		goto unlock;
+
+	atomic_set(&trig->interval, msecs_to_jiffies(period / 2));
+
+unlock:
+	mutex_unlock(&leds->lock);
+
+	if (ret)
+		dev_err(led->mc_cdev.led_cdev.dev,
+			"Error offloading trigger: %d\n", ret);
+
+	return ret;
+}
+
+static int omnia_led_trig_offload_off(struct omnia_leds *leds,
+				      struct omnia_led *led)
+{
+	int ret;
+
+	if (!led->phydev)
+		return 0;
+
+	mutex_lock(&leds->lock);
+
+	/* set PHY's LED[0] pin to default values */
+	ret = phy_modify_paged(led->phydev, MII_MARVELL_LED_PAGE,
+			       MII_PHY_LED_TCR,
+			       MII_PHY_LED_TCR_PULSESTR_MASK |
+			       MII_PHY_LED_TCR_BLINKRATE_MASK,
+			       (4 << MII_PHY_LED_TCR_PULSESTR_SHIFT) |
+			       (1 << MII_PHY_LED_TCR_BLINKRATE_SHIFT));
+
+	ret = phy_modify_paged(led->phydev, MII_MARVELL_LED_PAGE,
+			       MII_PHY_LED_CTRL, 0xf, 0xe);
+
+	/*
+	 * Return to software controlled mode, but only if we aren't being
+	 * called from led_classdev_unregister.
+	 */
+	if (!(led->mc_cdev.led_cdev.flags & LED_UNREGISTERING))
+		ret = omnia_led_set_sw_mode(leds->client, led->reg, true);
+
+	put_device(&led->phydev->mdio.dev);
+	led->phydev = NULL;
+
+	mutex_unlock(&leds->lock);
+
+	return 0;
+}
+
+static int omnia_led_trig_offload(struct led_classdev *cdev, bool enable)
+{
+	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct omnia_led *led = to_omnia_led(mc_cdev);
+	struct led_netdev_data *trig;
+	int ret = -EOPNOTSUPP;
+
+	if (!enable)
+		return omnia_led_trig_offload_off(leds, led);
+
+	if (!led->trig_src_np)
+		goto end;
+
+	/* only netdev trigger offloading is supported currently */
+	if (strcmp(cdev->trigger->name, "netdev"))
+		goto end;
+
+	trig = led_get_trigger_data(cdev);
+
+	if (!trig->net_dev)
+		goto end;
+
+	if (dev_of_node(trig->net_dev->dev.parent) != led->trig_src_np)
+		goto end;
+
+	ret = omnia_led_trig_offload_wan(leds, led, trig);
+
+end:
+	/*
+	 * if offloading failed (parameters not supported by HW), ensure any
+	 * previous offloading is disabled
+	 */
+	if (ret)
+		omnia_led_trig_offload_off(leds, led);
+
+	return ret;
+}
+
+static int read_trigger_sources(struct omnia_led *led, struct device_node *np)
+{
+	struct of_phandle_args args;
+	int ret;
+
+	ret = of_count_phandle_with_args(np, "trigger-sources",
+					 "#trigger-source-cells");
+	if (ret < 0)
+		return ret == -ENOENT ? 0 : ret;
+
+	if (!ret)
+		return 0;
+
+	ret = of_parse_phandle_with_args(np, "trigger-sources",
+					 "#trigger-source-cells", 0, &args);
+	if (ret)
+		return ret;
+
+	if (of_device_is_compatible(args.np, "marvell,armada-370-neta"))
+		led->trig_src_np = args.np;
+	else
+		of_node_put(args.np);
+
+	return 0;
+}
+
 static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 			      struct device_node *np)
 {

@@ -115,6 +330,13 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 		return 0;
 	}
 
+	ret = read_trigger_sources(led, np);
+	if (ret) {
+		dev_warn(dev, "Node %pOF: failed reading trigger sources: %d\n",
+			 np, ret);
+		return 0;
+	}
+
 	led->subled_info[0].color_index = LED_COLOR_ID_RED;
 	led->subled_info[0].channel = 0;
 	led->subled_info[0].intensity = 255;

@@ -133,6 +355,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	cdev = &led->mc_cdev.led_cdev;
 	cdev->max_brightness = 255;
 	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
+	if (led->trig_src_np)
+		cdev->trigger_offload = omnia_led_trig_offload;
 
 	/* put the LED into software mode */
 	ret = omnia_led_set_sw_mode(client, led->reg, true);

@@ -256,6 +480,7 @@ static int omnia_leds_probe(struct i2c_client *client,
 		}
 
 		led += ret;
+		++leds->count;
 	}
 
 	if (devm_device_add_groups(dev, omnia_led_controller_groups))

@@ -266,8 +491,15 @@ static int omnia_leds_probe(struct i2c_client *client,
 
 static int omnia_leds_remove(struct i2c_client *client)
 {
+	struct omnia_leds *leds = i2c_get_clientdata(client);
+	struct omnia_led *led;
 	u8 buf[5];
 
+	/* put away trigger source OF nodes */
+	for (led = &leds->leds[0]; led < &leds->leds[leds->count]; ++led)
+		if (led->trig_src_np)
+			of_node_put(led->trig_src_np);
+
 	/* put all LEDs into default (HW triggered) mode */
 	omnia_led_set_sw_mode(client, OMNIA_BOARD_LEDS, false);
 

-- 
2.26.3

+static int omnia_led_trig_offload_off(struct omnia_leds *leds,
+				      struct omnia_led *led)
+{
+	int ret;
+
+	if (!led->phydev)
+		return 0;
+
+	mutex_lock(&leds->lock);
+
+	/* set PHY's LED[0] pin to default values */
+	ret = phy_modify_paged(led->phydev, MII_MARVELL_LED_PAGE,
+			       MII_PHY_LED_TCR,
+			       MII_PHY_LED_TCR_PULSESTR_MASK |
+			       MII_PHY_LED_TCR_BLINKRATE_MASK,
+			       (4 << MII_PHY_LED_TCR_PULSESTR_SHIFT) |
+			       (1 << MII_PHY_LED_TCR_BLINKRATE_SHIFT));
+
+	ret = phy_modify_paged(led->phydev, MII_MARVELL_LED_PAGE,
+			       MII_PHY_LED_CTRL, 0xf, 0xe);
+

I forgot to add warnings here if ret < 0, since offload disabling must
return 0.

On Tue, Jun 01, 2021 at 02:51:51AM +0200, Marek Behún wrote:

Move setting of the LED_UNREGISTERING before deactivating the trigger in
led_classdev_unregister().

It can be useful for a LED trigger to know whether it is being
deactivated due to the LED being unregistered. This makes it possible
for LED drivers which implement trigger offloading to leave the LED in
HW triggering mode when the LED is unregistered, instead of disabling
it.

Humm, i'm not sure that is a good idea. I don't expect my Ethernet
switch to keep forwarding frames when i unload the driver.

       Andrew

+static int omnia_led_trig_offload_wan(struct omnia_leds *leds,
+				      struct omnia_led *led,
+				      struct led_netdev_data *trig)
+{
+	unsigned long period;
+	int ret, blink_rate;
+	bool link, rx, tx;
+	u8 fun;
+
+	/* HW offload on WAN port is supported only via internal PHY */
+	if (trig->net_dev->sfp_bus || !trig->net_dev->phydev)
+		return -EOPNOTSUPP;
+
+	link = test_bit(NETDEV_LED_LINK, &trig->mode);
+	rx = test_bit(NETDEV_LED_RX, &trig->mode);
+	tx = test_bit(NETDEV_LED_TX, &trig->mode);
+
+	if (link && rx && tx)
+		fun = 0x1;
+	else if (!link && rx && tx)
+		fun = 0x4;
+	else
+		return -EOPNOTSUPP;
+
+	period = jiffies_to_msecs(atomic_read(&trig->interval)) * 2;
+	blink_rate = wan_led_round_blink_rate(&period);
+	if (blink_rate < 0)
+		return blink_rate;
+
+	mutex_lock(&leds->lock);
+
+	if (!led->phydev) {
+		led->phydev = trig->net_dev->phydev;
+		get_device(&led->phydev->mdio.dev);
+	}
+
+	/* set PHY's LED[0] pin to blink according to trigger setting */
+	ret = phy_modify_paged(led->phydev, MII_MARVELL_LED_PAGE,
+			       MII_PHY_LED_TCR,
+			       MII_PHY_LED_TCR_PULSESTR_MASK |
+			       MII_PHY_LED_TCR_BLINKRATE_MASK,
+			       (0 << MII_PHY_LED_TCR_PULSESTR_SHIFT) |
+			       (blink_rate << MII_PHY_LED_TCR_BLINKRATE_SHIFT));
+	if (ret)
+		goto unlock;
+
+	ret = phy_modify_paged(led->phydev, MII_MARVELL_LED_PAGE,
+			       MII_PHY_LED_CTRL, 0xf, fun);
+	if (ret)
+		goto unlock;

This needs to be in the Marvell PHY driver. Please add a generic
interface any PHY driver can implement to allow its LEDs to be
controlled.

	Andrew

On Tue, 1 Jun 2021 23:12:57 +0200
Andrew Lunn [off-list ref] wrote:

On Tue, Jun 01, 2021 at 02:51:51AM +0200, Marek Behún wrote:

quoted

Move setting of the LED_UNREGISTERING before deactivating the
trigger in led_classdev_unregister().

It can be useful for a LED trigger to know whether it is being
deactivated due to the LED being unregistered. This makes it
possible for LED drivers which implement trigger offloading to
leave the LED in HW triggering mode when the LED is unregistered,
instead of disabling it.  

Humm, i'm not sure that is a good idea. I don't expect my Ethernet
switch to keep forwarding frames when i unload the driver.

We want to make it so that when leds-turris-omnia driver is unloaded,
the LEDs will start blinking in HW mode as they did before the driver
was loaded. This is needed for that.

Marek

Hi Marek,

On Tue, Jun 1, 2021 at 1:53 AM Marek Behún [off-list ref] wrote:

Hello,

this is v2 of series adding support for offloading LED triggers to HW.
The netdev trigger is the first user and leds-turris-omnia is the first
example implementation.

A video comparing SW (left LED) vs HW (right LED) netdev trigger on
Omnia
  https://secure.nic.cz/files/mbehun/omnia-wan-netdev-trig-offload.mp4

Changes since v1:
- changed typo in doc
- the netdev trigger data structure now lives in
  include/linux/ledtrig-netdev.h instead of ledtrig.h, as suggested by
  Andrew. Also the structure is always defined, no guard against
  CONFIG_LEDS_TRIGGER_NETDEV
- we do not export netdev_led_trigger variable. The trigger_offload()
  method can look at led_cdev->trigger->name to see which trigger it
  should try to offload, i.e. compare the string to "netdev"
- netdev trigger is being offloaded only if link is up, and at least one
  of the rx, tx parameters are set. No need to offload otherwise
- a patch is added that moves setting flag LED_UNREGISTERING in
  led_classdev_unregister() before unsetting trigger. This makes it
  possible for the trigger_offload() method to determine whether the
  offloading is being disabled because the LED is being unregistered.
  The driver may put the LED into HW triggering mode in this case, to
  achieve behaviour as was before the driver was loaded
- an example implementation for offloading the netdev trigger for the
  WAN LED on Turris Omnia is added. LAN LEDs are not yet supported

Changes since RFC:
- split the patch adding HW offloading support to netdev trigger into
  several separate patches (suggested by Pavel):
  1. move trigger data structure to include/linux/ledtrig.h
  2. support HW offloading
  3. change spinlock to mutex
- fixed bug where the .offloaded variable was not set to false when
  offloading was disabled (suggested by Pavel)
- removed the code saving one call to set_baseline_state() on the
  NETDEV_CHANGE event. It is not needed, the trigger_offload() method
  can handle this situation on its own (suggested by Pavel)
- documentation now explicitly says that when offloading is being
  disabled, the function must return 0 (no error) (suggested by Pavel)

Marek Behún (10):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: move trigger data structure to global include
    dir
  leds: trigger: netdev: support HW offloading
  leds: trigger: netdev: change spinlock to mutex
  leds: core: inform trigger that it's deactivation is due to LED
    removal
  leds: turris-omnia: refactor sw mode setting code into separate
    function
  leds: turris-omnia: refactor brightness setting function
  leds: turris-omnia: initialize each multicolor LED to white color
  leds: turris-omnia: support offloading netdev trigger for WAN LED

Do you plan to progress with the above series anytime soon? If not I
want to give this patch [0] again a respin.

[0] https://lore.kernel.org/all/20210930125747.2511954-1-frieder@fris.de/

Cheers,
Prabhakar

 Documentation/leds/leds-class.rst     |  22 ++
 drivers/leds/Kconfig                  |   3 +
 drivers/leds/led-class.c              |   4 +-
 drivers/leds/led-triggers.c           |   1 +
 drivers/leds/leds-turris-omnia.c      | 284 ++++++++++++++++++++++++--
 drivers/leds/trigger/ledtrig-netdev.c |  56 ++---
 include/linux/leds.h                  |  29 +++
 include/linux/ledtrig-netdev.h        |  34 +++
 8 files changed, 377 insertions(+), 56 deletions(-)
 create mode 100644 include/linux/ledtrig-netdev.h

--
2.26.3

quoted

Marek Behún (10):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: move trigger data structure to global include
    dir
  leds: trigger: netdev: support HW offloading
  leds: trigger: netdev: change spinlock to mutex
  leds: core: inform trigger that it's deactivation is due to LED
    removal
  leds: turris-omnia: refactor sw mode setting code into separate
    function
  leds: turris-omnia: refactor brightness setting function
  leds: turris-omnia: initialize each multicolor LED to white color
  leds: turris-omnia: support offloading netdev trigger for WAN LED

Do you plan to progress with the above series anytime soon? If not I
want to give this patch [0] again a respin.

What features are you missing from the current kernel code, which this
series adds?

	Andrew

Hi Andrew,

On Thu, Oct 16, 2025 at 2:14 PM Andrew Lunn [off-list ref] wrote:

quoted

quoted

Marek Behún (10):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: move trigger data structure to global include
    dir
  leds: trigger: netdev: support HW offloading
  leds: trigger: netdev: change spinlock to mutex
  leds: core: inform trigger that it's deactivation is due to LED
    removal
  leds: turris-omnia: refactor sw mode setting code into separate
    function
  leds: turris-omnia: refactor brightness setting function
  leds: turris-omnia: initialize each multicolor LED to white color
  leds: turris-omnia: support offloading netdev trigger for WAN LED

Do you plan to progress with the above series anytime soon? If not I
want to give this patch [0] again a respin.

What features are you missing from the current kernel code, which this
series adds?

I’m working on a platform that uses the VSC8541 PHY. On this platform,
LED0 and LED1 are connected to the external connector, and LED1 is
also connected to the Ethernet switch to indicate the PHY link status.
As a result, whenever there is link activity, the PHY link status
signal to the switch toggles, causing the switch to incorrectly detect
the link as going up and down.

Cheers,
Prabhakar

On Thu, Oct 16, 2025 at 07:53:17PM +0100, Lad, Prabhakar wrote:

Hi Andrew,

On Thu, Oct 16, 2025 at 2:14 PM Andrew Lunn [off-list ref] wrote:

quoted

quoted

quoted

Marek Behún (10):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: move trigger data structure to global include
    dir
  leds: trigger: netdev: support HW offloading
  leds: trigger: netdev: change spinlock to mutex
  leds: core: inform trigger that it's deactivation is due to LED
    removal
  leds: turris-omnia: refactor sw mode setting code into separate
    function
  leds: turris-omnia: refactor brightness setting function
  leds: turris-omnia: initialize each multicolor LED to white color
  leds: turris-omnia: support offloading netdev trigger for WAN LED

Do you plan to progress with the above series anytime soon? If not I
want to give this patch [0] again a respin.

What features are you missing from the current kernel code, which this
series adds?

I’m working on a platform that uses the VSC8541 PHY. On this platform,
LED0 and LED1 are connected to the external connector, and LED1 is
also connected to the Ethernet switch to indicate the PHY link status.
As a result, whenever there is link activity, the PHY link status
signal to the switch toggles, causing the switch to incorrectly detect
the link as going up and down.

So you think the current /sys/class/leds code is not sufficient. You
can use it from udev etc, to make the LED indicate link, but then
userspace could change it to something else. I _think_ only root can
use /sys/class/leds to change the function of the LED, so it is not
too bad as is? Or do you really want to make the configuration read
only?

	Andrew

Hi Andrew,

On Thu, Oct 16, 2025 at 8:11 PM Andrew Lunn [off-list ref] wrote:

On Thu, Oct 16, 2025 at 07:53:17PM +0100, Lad, Prabhakar wrote:

quoted

Hi Andrew,

On Thu, Oct 16, 2025 at 2:14 PM Andrew Lunn [off-list ref] wrote:

quoted

quoted

quoted

Marek Behún (10):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: move trigger data structure to global include
    dir
  leds: trigger: netdev: support HW offloading
  leds: trigger: netdev: change spinlock to mutex
  leds: core: inform trigger that it's deactivation is due to LED
    removal
  leds: turris-omnia: refactor sw mode setting code into separate
    function
  leds: turris-omnia: refactor brightness setting function
  leds: turris-omnia: initialize each multicolor LED to white color
  leds: turris-omnia: support offloading netdev trigger for WAN LED

Do you plan to progress with the above series anytime soon? If not I
want to give this patch [0] again a respin.

What features are you missing from the current kernel code, which this
series adds?

I’m working on a platform that uses the VSC8541 PHY. On this platform,
LED0 and LED1 are connected to the external connector, and LED1 is
also connected to the Ethernet switch to indicate the PHY link status.
As a result, whenever there is link activity, the PHY link status
signal to the switch toggles, causing the switch to incorrectly detect
the link as going up and down.

So you think the current /sys/class/leds code is not sufficient. You
can use it from udev etc, to make the LED indicate link, but then
userspace could change it to something else. I _think_ only root can
use /sys/class/leds to change the function of the LED, so it is not
too bad as is? Or do you really want to make the configuration read
only?

I haven't explored the current leds code tbh. Can you please point me
to any PHY which uses leds if any.

Cheers,
Prabhakar

I haven't explored the current leds code tbh. Can you please point me
to any PHY which uses leds if any.

~/linux/drivers/net/phy$ grep .led_brightness_set *
air_en8811h.c:static int air_led_brightness_set(struct phy_device *phydev, u8 index,
air_en8811h.c:	.led_brightness_set	= air_led_brightness_set,
as21xxx.c:static int as21xxx_led_brightness_set(struct phy_device *phydev,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:		.led_brightness_set = as21xxx_led_brightness_set,
bcm-phy-lib.c:int bcm_phy_led_brightness_set(struct phy_device *phydev,
bcm-phy-lib.c:EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set);
bcm-phy-lib.h:int bcm_phy_led_brightness_set(struct phy_device *phydev,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
broadcom.c:	.led_brightness_set	= bcm_phy_led_brightness_set,
dp83867.c:dp83867_led_brightness_set(struct phy_device *phydev,
dp83867.c:		.led_brightness_set = dp83867_led_brightness_set,
dp83td510.c:static int dp83td510_led_brightness_set(struct phy_device *phydev, u8 index,
dp83td510.c:	.led_brightness_set = dp83td510_led_brightness_set,
intel-xway.c:static int xway_gphy_led_brightness_set(struct phy_device *phydev,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:		.led_brightness_set = xway_gphy_led_brightness_set,
marvell.c:static int m88e1318_led_brightness_set(struct phy_device *phydev,
marvell.c:		.led_brightness_set = m88e1318_led_brightness_set,
marvell.c:		.led_brightness_set = m88e1318_led_brightness_set,
marvell.c:		.led_brightness_set = m88e1318_led_brightness_set,
marvell.c:		.led_brightness_set = m88e1318_led_brightness_set,
marvell.c:		.led_brightness_set = m88e1318_led_brightness_set,
mxl-86110.c:static int mxl86110_led_brightness_set(struct phy_device *phydev,
mxl-86110.c:		.led_brightness_set	= mxl86110_led_brightness_set,
mxl-86110.c:		.led_brightness_set	= mxl86110_led_brightness_set,
mxl-gpy.c:static int gpy_led_brightness_set(struct phy_device *phydev,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:		.led_brightness_set = gpy_led_brightness_set,

	Andrew

Hi Andrew,

On Thu, Oct 16, 2025 at 8:44 PM Andrew Lunn [off-list ref] wrote:

quoted

I haven't explored the current leds code tbh. Can you please point me
to any PHY which uses leds if any.

~/linux/drivers/net/phy$ grep .led_brightness_set *
air_en8811h.c:static int air_led_brightness_set(struct phy_device *phydev, u8 index,
air_en8811h.c:  .led_brightness_set     = air_led_brightness_set,
as21xxx.c:static int as21xxx_led_brightness_set(struct phy_device *phydev,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
bcm-phy-lib.c:int bcm_phy_led_brightness_set(struct phy_device *phydev,
bcm-phy-lib.c:EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set);
bcm-phy-lib.h:int bcm_phy_led_brightness_set(struct phy_device *phydev,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
dp83867.c:dp83867_led_brightness_set(struct phy_device *phydev,
dp83867.c:              .led_brightness_set = dp83867_led_brightness_set,
dp83td510.c:static int dp83td510_led_brightness_set(struct phy_device *phydev, u8 index,
dp83td510.c:    .led_brightness_set = dp83td510_led_brightness_set,
intel-xway.c:static int xway_gphy_led_brightness_set(struct phy_device *phydev,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
marvell.c:static int m88e1318_led_brightness_set(struct phy_device *phydev,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
mxl-86110.c:static int mxl86110_led_brightness_set(struct phy_device *phydev,
mxl-86110.c:            .led_brightness_set     = mxl86110_led_brightness_set,
mxl-86110.c:            .led_brightness_set     = mxl86110_led_brightness_set,
mxl-gpy.c:static int gpy_led_brightness_set(struct phy_device *phydev,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,

Thank you for the pointers.

Cheers,
Prabhakar

On Fri, 17 Oct 2025, Lad, Prabhakar wrote:

Hi Andrew,

[...]

quoted

mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,

[...]

Thank you for the pointers.

Yes, thank you, Andrew.

-- 
Lee Jones [李琼斯]

Hi Andrew,

On Fri, Oct 17, 2025 at 4:23 PM Lad, Prabhakar
[off-list ref] wrote:

Hi Andrew,

On Thu, Oct 16, 2025 at 8:44 PM Andrew Lunn [off-list ref] wrote:

quoted

quoted

I haven't explored the current leds code tbh. Can you please point me
to any PHY which uses leds if any.

~/linux/drivers/net/phy$ grep .led_brightness_set *
air_en8811h.c:static int air_led_brightness_set(struct phy_device *phydev, u8 index,
air_en8811h.c:  .led_brightness_set     = air_led_brightness_set,
as21xxx.c:static int as21xxx_led_brightness_set(struct phy_device *phydev,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
as21xxx.c:              .led_brightness_set = as21xxx_led_brightness_set,
bcm-phy-lib.c:int bcm_phy_led_brightness_set(struct phy_device *phydev,
bcm-phy-lib.c:EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set);
bcm-phy-lib.h:int bcm_phy_led_brightness_set(struct phy_device *phydev,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
broadcom.c:     .led_brightness_set     = bcm_phy_led_brightness_set,
dp83867.c:dp83867_led_brightness_set(struct phy_device *phydev,
dp83867.c:              .led_brightness_set = dp83867_led_brightness_set,
dp83td510.c:static int dp83td510_led_brightness_set(struct phy_device *phydev, u8 index,
dp83td510.c:    .led_brightness_set = dp83td510_led_brightness_set,
intel-xway.c:static int xway_gphy_led_brightness_set(struct phy_device *phydev,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
intel-xway.c:           .led_brightness_set = xway_gphy_led_brightness_set,
marvell.c:static int m88e1318_led_brightness_set(struct phy_device *phydev,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
marvell.c:              .led_brightness_set = m88e1318_led_brightness_set,
mxl-86110.c:static int mxl86110_led_brightness_set(struct phy_device *phydev,
mxl-86110.c:            .led_brightness_set     = mxl86110_led_brightness_set,
mxl-86110.c:            .led_brightness_set     = mxl86110_led_brightness_set,
mxl-gpy.c:static int gpy_led_brightness_set(struct phy_device *phydev,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,
mxl-gpy.c:              .led_brightness_set = gpy_led_brightness_set,

Sorry for the delayed response.

I started investigating adding PHY leds. In page 53 section "4.2.27
LED Behavior" [0] we have an option for LED0/1 combine feature
disable. For this is it OK to add a new DT property?

[0] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/VMDS-10513_VSC8541-02_VSC8541-05_Datasheet.pdf

Cheers,
Prabhakar

Sorry for the delayed response.

I started investigating adding PHY leds. In page 53 section "4.2.27
LED Behavior" [0] we have an option for LED0/1 combine feature
disable. For this is it OK to add a new DT property?

Why do you need a new property?

You just need to set this bit depending on what has been selected via
/sys/class/led.

And if the user asks for a mode which the hardware does not supported,
the core will fall back to use on/off and blink the LED itself.

PHY LEDs are the wild west. Every vendor has its own idea what is
important, and adds features which other vendors don't have. But that
does not mean we need to support all the features in Linux. So the
core has a reasonable set of features which we expect most PHYs can
support. I don't want to add more features unless you have a big
business case it is needed, and other PHY also have the same feature.

     Andrew

Hi Andrew,

On Wed, Nov 5, 2025 at 3:49 PM Andrew Lunn [off-list ref] wrote:

quoted

Sorry for the delayed response.

I started investigating adding PHY leds. In page 53 section "4.2.27
LED Behavior" [0] we have an option for LED0/1 combine feature
disable. For this is it OK to add a new DT property?

Why do you need a new property?

You just need to set this bit depending on what has been selected via
/sys/class/led.

Ahh I get you now. When I trigger the sysfs file I get the below files:

# ls
brightness  device  device_name  full_duplex  half_duplex  interval
link  link_10  link_100  max_brightness  offloaded  power  rx  rx_err
subsystem  trigger  tx  tx_err  uevent

As per HW manual [0] we have,
0: Combine enabled (link/activity, duplex/collision).
1: Disable combination (link only, duplex only).

# Combine DISABLED (link + duplex only)
echo netdev > trigger
echo 1 > link
echo 1 > full_duplex  # or half_duplex
echo 0 > rx
echo 0 > tx

# Combine ENABLED (link + activity + duplex + collision)
echo netdev > trigger
echo 1 > link
echo 1 > rx
echo 1 > tx

So to Enable/Disable LEDx combine feature we just need to write as
above. Is my understanding correct?

[0] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/VMDS-10513_VSC8541-02_VSC8541-05_Datasheet.pdf

And if the user asks for a mode which the hardware does not supported,
the core will fall back to use on/off and blink the LED itself.

Ok.

PHY LEDs are the wild west. Every vendor has its own idea what is
important, and adds features which other vendors don't have. But that
does not mean we need to support all the features in Linux. So the
core has a reasonable set of features which we expect most PHYs can
support. I don't want to add more features unless you have a big
business case it is needed, and other PHY also have the same feature.

Agreed.

Cheers,
Prabhakar

# ls
brightness  device  device_name  full_duplex  half_duplex  interval
link  link_10  link_100  max_brightness  offloaded  power  rx  rx_err
subsystem  trigger  tx  tx_err  uevent

As per HW manual [0] we have,
0: Combine enabled (link/activity, duplex/collision).
1: Disable combination (link only, duplex only).

# Combine DISABLED (link + duplex only)
echo netdev > trigger
echo 1 > link
echo 1 > full_duplex  # or half_duplex
echo 0 > rx
echo 0 > tx

# Combine ENABLED (link + activity + duplex + collision)
echo netdev > trigger
echo 1 > link
echo 1 > rx
echo 1 > tx

So to Enable/Disable LEDx combine feature we just need to write as
above. Is my understanding correct?

Yes. The PHY driver gets passed a bitmap of each features to
enable. Looking that those bits you need to decide on the 4 bit LED
mode value, and the combine bit. Or return -EOPNOTSUPP.

	Andrew