[PATCH v7 00/11] Adds support for PHY LEDs with offload triggers

[PATCH v7 00/11] Adds support for PHY LEDs with offload triggers

[Christian Marangi]

This is another attempt on adding this feature on LEDs, hoping this is
the right time and someone finally notice this.


Most of the times Switch/PHY have connected multiple LEDs that are
controlled by HW based on some rules/event. Currently we lack any
support for a generic way to control the HW part and normally we
either never implement the feature or only add control for brightness
or hw blink.

This is based on Marek idea of providing some API to cled but use a
different implementation that in theory should be more generilized.

The current idea is:
- LED driver implement 3 API (hw_control_status/start/stop).
  They are used to put the LED in hardware mode and to configure the
  various trigger.
- We have hardware triggers that are used to expose to userspace the
  supported hardware mode and set the hardware mode on trigger
  activation.
- We can also have triggers that both support hardware and software mode.
- The LED driver will declare each supported hardware blink mode and
  communicate with the trigger all the supported blink modes that will
  be available by sysfs.
- A trigger will use blink_set to configure the blink mode to active
  in hardware mode.
- On hardware trigger activation, only the hardware mode is enabled but
  the blink modes are not configured. The LED driver should reset any
  link mode active by default.

Each LED driver will have to declare explicit support for the offload
trigger (or return not supported error code) as we the trigger_data that
the LED driver will elaborate and understand what is referring to (based
on the current active trigger).

I posted a user for this new implementation that will benefit from this
and will add a big feature to it. Currently qca8k can have up to 3 LEDs
connected to each PHY port and we have some device that have only one of
them connected and the default configuration won't work for that.

The netdev trigger is expanded and it does now support hardware only
triggers.
The idea is to use hardware mode when a device_name is not defined.
An additional sysfs entry is added to give some info about the available
trigger modes supported in the current configuration.


It was reported that at least 3 other switch family would benefits by
this as they all lack support for a generic way to setup their leds and
netdev team NACK each try to add special code to support LEDs present
on switch in favor of a generic solution.

v7:
- Rebase on top of net-next (for qca8k changes)
- Fix some typo in commit description
- Fix qca8k leds documentation warning
- Remove RFC tag
v6:
- Back to RFC.
- Drop additional trigger
- Rework netdev trigger to support common modes used by switch and
  hardware only triggers
- Refresh qca8k leds logic and driver
v5:
- Move out of RFC. (no comments from Andrew this is the right path?)
- Fix more spelling mistake (thx Randy)
- Fix error reported by kernel test bot
- Drop the additional HW_CONTROL flag. It does simplify CONFIG
  handling and hw control should be available anyway to support
  triggers as module.
v4:
- Rework implementation and drop hw_configure logic.
  We now expand blink_set.
- Address even more spelling mistake. (thx a lot Randy)
- Drop blink option and use blink_set delay.
- Rework phy-activity trigger to actually make the groups dynamic.
v3:
- Rework start/stop as Andrew asked.
- Introduce more logic to permit a trigger to run in hardware mode.
- Add additional patch with netdev hardware support.
- Use test_bit API to check flag passed to hw_control_configure.
- Added a new cmd to hw_control_configure to reset any active blink_mode.
- Refactor all the patches to follow this new implementation.
v2:
- Fix spelling mistake (sorry)
- Drop patch 02 "permit to declare supported offload triggers".
  Change the logic, now the LED driver declare support for them
  using the configure_offload with the cmd TRIGGER_SUPPORTED.
- Rework code to follow this new implementation.
- Update Documentation to better describe how this offload
  implementation work.

Christian Marangi (11):
  leds: add support for hardware driven LEDs
  leds: add function to configure hardware controlled LED
  leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode
  leds: trigger: netdev: rename and expose NETDEV trigger enum modes
  leds: trigger: netdev: convert device attr to macro
  leds: trigger: netdev: add hardware control support
  leds: trigger: netdev: use mutex instead of spinlocks
  leds: trigger: netdev: add available mode sysfs attr
  leds: trigger: netdev: add additional hardware only triggers
  net: dsa: qca8k: add LEDs support
  dt-bindings: net: dsa: qca8k: add LEDs definition example

 .../devicetree/bindings/net/dsa/qca8k.yaml    |  24 ++
 Documentation/leds/leds-class.rst             |  53 +++
 drivers/leds/Kconfig                          |  11 +
 drivers/leds/led-class.c                      |  27 ++
 drivers/leds/led-triggers.c                   |  29 ++
 drivers/leds/trigger/ledtrig-netdev.c         | 385 ++++++++++++-----
 drivers/net/dsa/qca/Kconfig                   |   9 +
 drivers/net/dsa/qca/Makefile                  |   1 +
 drivers/net/dsa/qca/qca8k-8xxx.c              |   4 +
 drivers/net/dsa/qca/qca8k-leds.c              | 406 ++++++++++++++++++
 drivers/net/dsa/qca/qca8k.h                   |  62 +++
 include/linux/leds.h                          | 103 ++++-
 12 files changed, 1015 insertions(+), 99 deletions(-)
 create mode 100644 drivers/net/dsa/qca/qca8k-leds.c

-- 
2.37.2

[PATCH v7 01/11] leds: add support for hardware driven LEDs

[Christian Marangi]

Some LEDs can be driven by 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, LED driver must support this and a supported
trigger must be used.

LED driver should declare the correct blink_mode supported and should set
the blink_mode parameter to one of HARDWARE_CONTROLLED or
SOFTWARE_HARDWARE_CONTROLLED.
The trigger will check this option and fail to activate if the blink_mode
is not supported. By default if a LED driver doesn't declare blink_mode,
SOFTWARE_CONTROLLED is assumed.

The LED must implement 3 main API:
- hw_control_status(): This asks the LED driver if hardware mode is
    enabled or not.
    Triggers will check if the offload mode is supported and will be
    activated accordingly. If the trigger can't run in software mode,
    return -EOPNOTSUPP as the blinking can't be simulated by software.
- hw_control_start(): This will simply enable the hardware mode for
    the LED.
    With this not declared and hw_control_status() returning true,
    it's assumed that the LED is always in hardware mode.
- hw_control_stop(): This will simply disable the hardware mode for
    the LED.
    With this not declared and hw_control_status() returning true,
    it's assumed that the LED is always in hardware mode.
    It's advised to the driver to put the LED in the old state but this
    is not enforcerd and putting the LED off is also accepted.

With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is
optional and any software only trigger will reject activation as the LED
supports only hardware mode.

An additional config CONFIG_LEDS_HARDWARE_CONTROL is added to add support
for LEDs that can be controlled by hardware.

Cc: Marek Behún <kabel@kernel.org>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 Documentation/leds/leds-class.rst | 28 ++++++++++++++++++
 drivers/leds/Kconfig              | 11 ++++++++
 drivers/leds/led-class.c          | 27 ++++++++++++++++++
 drivers/leds/led-triggers.c       | 29 +++++++++++++++++++
 include/linux/leds.h              | 47 ++++++++++++++++++++++++++++++-
 5 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index cd155ead8703..645940b78d81 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -169,6 +169,34 @@ 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 driven LEDs
+===================================
+
+Some LEDs can be driven by 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, LED driver must support this and a supported trigger must
+be used.
+
+LED driver should declare the correct blink_mode supported and should set the
+blink_mode parameter to one of HARDWARE_CONTROLLED or SOFTWARE_HARDWARE_CONTROLLED.
+The trigger will check this option and fail to activate if the blink_mode is not
+supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTROLLED
+is assumed.
+
+The LED must implement 3 main API:
+- hw_control_status(): This asks the LED driver if hardware mode is enabled
+    or not. Triggers will check if the hardware mode is active and will try
+    to offload their triggers if supported by the driver.
+- hw_control_start(): This will simply enable the hardware mode for the LED.
+- hw_control_stop(): This will simply disable the hardware mode for the LED.
+    It's advised to the driver to put the LED in the old state but this is not
+    enforcerd and putting the LED off is also accepted.
+
+With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
+and any software only trigger will reject activation as the LED supports only
+hardware mode.
 
 Known Issues
 ============
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 499d0f215a8b..891f4821b2c8 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -49,6 +49,17 @@ config LEDS_BRIGHTNESS_HW_CHANGED
 
 	  See Documentation/ABI/testing/sysfs-class-led for details.
 
+config LEDS_HARDWARE_CONTROL
+	bool "LED Hardware Control support"
+	help
+	  This option enabled Hardware control support used by leds that
+	  can be driven in hardware by using supported triggers.
+
+	  Hardware blink modes will be exposed by sysfs class in
+	  /sys/class/leds based on the trigger currently active.
+
+	  If unsure, say Y.
+
 comment "LED drivers"
 
 config LEDS_88PM860X
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 6a8ea94834fa..3516ae3c4c3c 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -164,6 +164,27 @@ static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
 }
 #endif
 
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+static int led_classdev_check_blink_mode_functions(struct led_classdev *led_cdev)
+{
+	int mode = led_cdev->blink_mode;
+
+	if (mode == SOFTWARE_HARDWARE_CONTROLLED &&
+	    (!led_cdev->hw_control_status ||
+	    !led_cdev->hw_control_start ||
+	    !led_cdev->hw_control_stop))
+		return -EINVAL;
+
+	if (mode == SOFTWARE_CONTROLLED &&
+	    (led_cdev->hw_control_status ||
+	    led_cdev->hw_control_start ||
+	    led_cdev->hw_control_stop))
+		return -EINVAL;
+
+	return 0;
+}
+#endif
+
 /**
  * led_classdev_suspend - suspend an led_classdev.
  * @led_cdev: the led_classdev to suspend.
@@ -367,6 +388,12 @@ int led_classdev_register_ext(struct device *parent,
 	if (ret < 0)
 		return ret;
 
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+	ret = led_classdev_check_blink_mode_functions(led_cdev);
+	if (ret < 0)
+		return ret;
+#endif
+
 	mutex_init(&led_cdev->led_access);
 	mutex_lock(&led_cdev->led_access);
 	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 072491d3e17b..693c5d0fa980 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -154,6 +154,27 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(led_trigger_read);
 
+static bool led_trigger_is_supported(struct led_classdev *led_cdev,
+				     struct led_trigger *trigger)
+{
+	switch (led_cdev->blink_mode) {
+	case SOFTWARE_CONTROLLED:
+		if (trigger->supported_blink_modes == HARDWARE_ONLY)
+			return 0;
+		break;
+	case HARDWARE_CONTROLLED:
+		if (trigger->supported_blink_modes == SOFTWARE_ONLY)
+			return 0;
+		break;
+	case SOFTWARE_HARDWARE_CONTROLLED:
+		break;
+	default:
+		return 0;
+	}
+
+	return 1;
+}
+
 /* Caller must ensure led_cdev->trigger_lock held */
 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 {
@@ -179,6 +200,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 
 		cancel_work_sync(&led_cdev->set_brightness_work);
 		led_stop_software_blink(led_cdev);
+		/* Disable hardware mode on trigger change if supported */
+		if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
+		    led_cdev->hw_control_status(led_cdev))
+			led_cdev->hw_control_stop(led_cdev);
 		if (led_cdev->trigger->deactivate)
 			led_cdev->trigger->deactivate(led_cdev);
 		device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
@@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 		led_set_brightness(led_cdev, LED_OFF);
 	}
 	if (trig) {
+		/* Make sure the trigger support the LED blink mode */
+		if (!led_trigger_is_supported(led_cdev, trig))
+			return -EINVAL;
+
 		spin_lock(&trig->leddev_list_lock);
 		list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
 		spin_unlock(&trig->leddev_list_lock);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..09ff1dc6f48d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -67,6 +67,12 @@ struct led_hw_trigger_type {
 	int dummy;
 };
 
+enum led_blink_modes {
+	SOFTWARE_CONTROLLED = 0x0,
+	HARDWARE_CONTROLLED,
+	SOFTWARE_HARDWARE_CONTROLLED,
+};
+
 struct led_classdev {
 	const char		*name;
 	unsigned int brightness;
@@ -154,6 +160,32 @@ struct led_classdev {
 
 	/* LEDs that have private triggers have this set */
 	struct led_hw_trigger_type	*trigger_type;
+
+	/* This report the supported blink_mode. The driver should report the
+	 * correct LED capabilities.
+	 * With this set to HARDWARE_CONTROLLED, LED is always in offload mode
+	 * and triggers can't be simulated by software.
+	 * If the led is HARDWARE_CONTROLLED, status/start/stop function
+	 * are optional.
+	 * By default SOFTWARE_CONTROLLED is set as blink_mode.
+	 */
+	enum led_blink_modes	blink_mode;
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+	/* Ask the LED driver if hardware mode is enabled or not.
+	 * If the option is not declared by the LED driver, SOFTWARE_CONTROLLED
+	 * is assumed.
+	 * Triggers will check if the hardware mode is supported and will be
+	 * activated accordingly. If the trigger can't run in hardware mode,
+	 * return -EOPNOTSUPP as the blinking can't be simulated by software.
+	 */
+	bool			(*hw_control_status)(struct led_classdev *led_cdev);
+	/* Set LED in hardware mode */
+	int			(*hw_control_start)(struct led_classdev *led_cdev);
+	/* Disable hardware mode for LED. It's advised to the LED driver to put it to
+	 * the old status but that is not mandatory and also putting it off is accepted.
+	 */
+	int			(*hw_control_stop)(struct led_classdev *led_cdev);
+#endif
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -215,7 +247,6 @@ extern struct led_classdev *of_led_get(struct device_node *np, int index);
 extern void led_put(struct led_classdev *led_cdev);
 struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 						  int index);
-
 /**
  * led_blink_set - set blinking with software fallback
  * @led_cdev: the LED to start blinking
@@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
 
 #define TRIG_NAME_MAX 50
 
+enum led_trigger_blink_supported_modes {
+	SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
+	HARDWARE_ONLY = HARDWARE_CONTROLLED,
+	SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
+};
+
 struct led_trigger {
 	/* Trigger Properties */
 	const char	 *name;
 	int		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
 
+	/* Declare if the Trigger supports hardware control to
+	 * offload triggers or supports only software control.
+	 * A trigger can also declare support for hardware control
+	 * if its task is to only configure LED blink modes and expose
+	 * them in sysfs.
+	 */
+	enum led_trigger_blink_supported_modes supported_blink_modes;
+
 	/* LED-private triggers have this set */
 	struct led_hw_trigger_type *trigger_type;
 
-- 
2.37.2

[PATCH v7 02/11] leds: add function to configure hardware controlled LED

[Christian Marangi]

Add hw_control_configure helper to configure how the LED should work in
hardware mode. The function require to support the particular trigger and
will use the passed flag to elaborate the data and apply the
correct configuration. This function will then be used by the trigger to
request and update hardware configuration.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 Documentation/leds/leds-class.rst | 25 ++++++++++++++++++++
 include/linux/leds.h              | 39 +++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index 645940b78d81..efd2f68c46a7 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -198,6 +198,31 @@ With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
 and any software only trigger will reject activation as the LED supports only
 hardware mode.
 
+A trigger once he declared support for hardware controlled blinks, will use the function
+hw_control_configure() provided by the driver to check support for a particular blink mode.
+This function passes as the first argument (flag) a u32 flag.
+The second argument (cmd) of hw_control_configure() method can be used to do various
+operations for the specific blink mode. We currently support ENABLE, DISABLE, READ, ZERO
+and SUPPORTED to enable, disable, read the state of the blink mode, ask the LED
+driver if it does supports the specific blink mode and to reset any blink mode active.
+
+In ENABLE/DISABLE hw_control_configure() should configure the LED to enable/disable the
+requested blink mode (flag).
+In READ hw_control_configure() should return 0 or 1 based on the status of the requested
+blink mode (flag).
+In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
+requested blink mode (flags) or not.
+In ZERO hw_control_configure() should return 0 with success operation or error.
+
+The unsigned long flag is specific to the trigger and change across them. It's in the LED
+driver interest know how to elaborate this flag and to declare support for a
+particular trigger. For this exact reason explicit support for the specific
+trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
+with a not supported trigger.
+If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
+fail as the driver doesn't support that specific offload trigger or doesn't know
+how to handle the provided flags.
+
 Known Issues
 ============
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 09ff1dc6f48d..b5aad67fecfb 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -73,6 +73,16 @@ enum led_blink_modes {
 	SOFTWARE_HARDWARE_CONTROLLED,
 };
 
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+enum blink_mode_cmd {
+	BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
+	BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
+	BLINK_MODE_READ, /* Read the status of the hardware blink mode */
+	BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
+	BLINK_MODE_ZERO, /* Disable any hardware blink active */
+};
+#endif
+
 struct led_classdev {
 	const char		*name;
 	unsigned int brightness;
@@ -185,6 +195,17 @@ struct led_classdev {
 	 * the old status but that is not mandatory and also putting it off is accepted.
 	 */
 	int			(*hw_control_stop)(struct led_classdev *led_cdev);
+	/* This will be used to configure the various blink modes LED support in hardware
+	 * mode.
+	 * The LED driver require to support the active trigger and will elaborate the
+	 * unsigned long flag and do the operation based on the provided cmd.
+	 * Current operation are enable,disable,supported and status.
+	 * A trigger will use this to enable or disable the asked blink mode, check the
+	 * status of the blink mode or ask if the blink mode can run in hardware mode.
+	 */
+	int			(*hw_control_configure)(struct led_classdev *led_cdev,
+							unsigned long flag,
+							enum blink_mode_cmd cmd);
 #endif
 #endif
 
@@ -454,6 +475,24 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 	return led_cdev->trigger_data;
 }
 
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev,
+						       unsigned long flag)
+{
+	int ret;
+
+	/* Sanity check: make sure led support hw mode */
+	if (led_cdev->blink_mode == SOFTWARE_CONTROLLED)
+		return false;
+
+	ret = led_cdev->hw_control_configure(led_cdev, flag, BLINK_MODE_SUPPORTED);
+	if (ret > 0)
+		return true;
+
+	return false;
+}
+#endif
+
 /**
  * led_trigger_rename_static - rename a trigger
  * @name: the new trigger name
-- 
2.37.2

[PATCH v7 03/11] leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode

[Christian Marangi]

Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool
that will be true or false based on the carrier link. No functional
change intended.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d83021..66a81cc9b64d 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -50,10 +50,10 @@ struct led_netdev_data {
 	unsigned int last_activity;
 
 	unsigned long mode;
+	bool carrier_link_up;
 #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 {
@@ -73,9 +73,9 @@ 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 (!trigger_data->carrier_link_up) {
 		led_set_brightness(led_cdev, LED_OFF);
-	else {
+	} else {
 		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
 			led_set_brightness(led_cdev,
 					   led_cdev->blink_brightness);
@@ -131,10 +131,9 @@ static ssize_t device_name_store(struct device *dev,
 		trigger_data->net_dev =
 		    dev_get_by_name(&init_net, trigger_data->device_name);
 
-	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+	trigger_data->carrier_link_up = false;
 	if (trigger_data->net_dev != NULL)
-		if (netif_carrier_ok(trigger_data->net_dev))
-			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+		trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
 
 	trigger_data->last_activity = 0;
 
@@ -315,7 +314,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	spin_lock_bh(&trigger_data->lock);
 
-	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+	trigger_data->carrier_link_up = false;
 	switch (evt) {
 	case NETDEV_CHANGENAME:
 	case NETDEV_REGISTER:
@@ -330,8 +329,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 		break;
 	case NETDEV_UP:
 	case NETDEV_CHANGE:
-		if (netif_carrier_ok(dev))
-			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+		trigger_data->carrier_link_up = netif_carrier_ok(dev);
 		break;
 	}
 
-- 
2.37.2

[PATCH v7 04/11] leds: trigger: netdev: rename and expose NETDEV trigger enum modes

[Christian Marangi]

Rename NETDEV trigger enum modes to a more simbolic name and move them
in leds.h to make them accessible by any user.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 53 +++++++++------------------
 include/linux/leds.h                  |  7 ++++
 2 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 66a81cc9b64d..6872da08676b 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -51,15 +51,6 @@ struct led_netdev_data {
 
 	unsigned long mode;
 	bool carrier_link_up;
-#define NETDEV_LED_LINK	0
-#define NETDEV_LED_TX	1
-#define NETDEV_LED_RX	2
-};
-
-enum netdev_led_attr {
-	NETDEV_ATTR_LINK,
-	NETDEV_ATTR_TX,
-	NETDEV_ATTR_RX
 };
 
 static void set_baseline_state(struct led_netdev_data *trigger_data)
@@ -76,7 +67,7 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	if (!trigger_data->carrier_link_up) {
 		led_set_brightness(led_cdev, LED_OFF);
 	} else {
-		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
+		if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
 			led_set_brightness(led_cdev,
 					   led_cdev->blink_brightness);
 		else
@@ -85,8 +76,8 @@ 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 (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ||
+		    test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
 			schedule_delayed_work(&trigger_data->work, 0);
 	}
 }
@@ -146,20 +137,16 @@ static ssize_t device_name_store(struct device *dev,
 static DEVICE_ATTR_RW(device_name);
 
 static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
-	enum netdev_led_attr attr)
+				    enum led_trigger_netdev_modes attr)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	int bit;
 
 	switch (attr) {
-	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
-		break;
-	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
-		break;
-	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+	case TRIGGER_NETDEV_LINK:
+	case TRIGGER_NETDEV_TX:
+	case TRIGGER_NETDEV_RX:
+		bit = attr;
 		break;
 	default:
 		return -EINVAL;
@@ -169,7 +156,7 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
 }
 
 static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
-	size_t size, enum netdev_led_attr attr)
+				     size_t size, enum led_trigger_netdev_modes attr)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	unsigned long state;
@@ -181,14 +168,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 		return ret;
 
 	switch (attr) {
-	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
-		break;
-	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
-		break;
-	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+	case TRIGGER_NETDEV_LINK:
+	case TRIGGER_NETDEV_TX:
+	case TRIGGER_NETDEV_RX:
+		bit = attr;
 		break;
 	default:
 		return -EINVAL;
@@ -358,21 +341,21 @@ static void netdev_trig_work(struct work_struct *work)
 	}
 
 	/* If we are not looking for RX/TX then return  */
-	if (!test_bit(NETDEV_LED_TX, &trigger_data->mode) &&
-	    !test_bit(NETDEV_LED_RX, &trigger_data->mode))
+	if (!test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
+	    !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
 		return;
 
 	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
 	new_activity =
-	    (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
+	    (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ?
 		dev_stats->tx_packets : 0) +
-	    (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
+	    (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) ?
 		dev_stats->rx_packets : 0);
 
 	if (trigger_data->last_activity != new_activity) {
 		led_stop_software_blink(trigger_data->led_cdev);
 
-		invert = test_bit(NETDEV_LED_LINK, &trigger_data->mode);
+		invert = test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode);
 		interval = jiffies_to_msecs(
 				atomic_read(&trigger_data->interval));
 		/* base state is ON (link present) */
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b5aad67fecfb..13862f8b1e07 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -548,6 +548,13 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 
 #endif /* CONFIG_LEDS_TRIGGERS */
 
+/* Trigger specific enum */
+enum led_trigger_netdev_modes {
+	TRIGGER_NETDEV_LINK = 1,
+	TRIGGER_NETDEV_TX,
+	TRIGGER_NETDEV_RX,
+};
+
 /* Trigger specific functions */
 #ifdef CONFIG_LEDS_TRIGGER_DISK
 void ledtrig_disk_activity(bool write);
-- 
2.37.2

[PATCH v7 05/11] leds: trigger: netdev: convert device attr to macro

[Christian Marangi]

Convert link tx and rx device attr to a common macro to reduce common
code and in preparation for additional attr.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 57 ++++++++-------------------
 1 file changed, 16 insertions(+), 41 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 6872da08676b..dd63cadb896e 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -189,47 +189,22 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	return size;
 }
 
-static ssize_t link_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_LINK);
-}
-
-static ssize_t link_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t size)
-{
-	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_LINK);
-}
-
-static DEVICE_ATTR_RW(link);
-
-static ssize_t tx_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_TX);
-}
-
-static ssize_t tx_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t size)
-{
-	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_TX);
-}
-
-static DEVICE_ATTR_RW(tx);
-
-static ssize_t rx_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_RX);
-}
-
-static ssize_t rx_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t size)
-{
-	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_RX);
-}
-
-static DEVICE_ATTR_RW(rx);
+#define DEFINE_NETDEV_TRIGGER(trigger_name, trigger) \
+	static ssize_t trigger_name##_show(struct device *dev, \
+		struct device_attribute *attr, char *buf) \
+	{ \
+		return netdev_led_attr_show(dev, buf, trigger); \
+	} \
+	static ssize_t trigger_name##_store(struct device *dev, \
+		struct device_attribute *attr, const char *buf, size_t size) \
+	{ \
+		return netdev_led_attr_store(dev, buf, size, trigger); \
+	} \
+	static DEVICE_ATTR_RW(trigger_name)
+
+DEFINE_NETDEV_TRIGGER(link, TRIGGER_NETDEV_LINK);
+DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
+DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
 
 static ssize_t interval_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
-- 
2.37.2

[PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Christian Marangi]

Add hardware control support for the Netdev trigger.
The trigger on config change will check if the requested trigger can set
to blink mode using LED hardware mode and if every blink mode is supported,
the trigger will enable hardware mode with the requested configuration.
If there is at least one trigger that is not supported and can't run in
hardware mode, then software mode will be used instead.
A validation is done on every value change and on fail the old value is
restored and -EINVAL is returned.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
 1 file changed, 149 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index dd63cadb896e..ed019cb5867c 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -37,6 +37,7 @@
  */
 
 struct led_netdev_data {
+	enum led_blink_modes blink_mode;
 	spinlock_t lock;
 
 	struct delayed_work work;
@@ -53,11 +54,105 @@ struct led_netdev_data {
 	bool carrier_link_up;
 };
 
+struct netdev_led_attr_detail {
+	char *name;
+	bool hardware_only;
+	enum led_trigger_netdev_modes bit;
+};
+
+static struct netdev_led_attr_detail attr_details[] = {
+	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
+	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
+	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
+};
+
+static bool validate_baseline_state(struct led_netdev_data *trigger_data)
+{
+	struct led_classdev *led_cdev = trigger_data->led_cdev;
+	struct netdev_led_attr_detail *detail;
+	u32 hw_blink_mode_supported = 0;
+	bool force_sw = false;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+		detail = &attr_details[i];
+
+		/* Mode not active, skip */
+		if (!test_bit(detail->bit, &trigger_data->mode))
+			continue;
+
+		/* Hardware only mode enabled on software controlled led */
+		if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
+		    detail->hardware_only)
+			return false;
+
+		/* Check if the mode supports hardware mode */
+		if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
+			/* With a net dev set, force software mode.
+			 * With modes are handled by hardware, led will blink
+			 * based on his own events and will ignore any event
+			 * from the provided dev.
+			 */
+			if (trigger_data->net_dev) {
+				force_sw = true;
+				continue;
+			}
+
+			/* With empty dev, check if the mode is supported */
+			if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
+				hw_blink_mode_supported |= BIT(detail->bit);
+		}
+	}
+
+	/* We can't run modes handled by both software and hardware.
+	 * Check if we run hardware modes and check if all the modes
+	 * can be handled by hardware.
+	 */
+	if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
+		return false;
+
+	/* Modes are valid. Decide now the running mode to later
+	 * set the baseline.
+	 * Software mode is enforced with net_dev set. With an empty
+	 * one hardware mode is selected by default (if supported).
+	 */
+	if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
+		trigger_data->blink_mode = SOFTWARE_CONTROLLED;
+	else
+		trigger_data->blink_mode = HARDWARE_CONTROLLED;
+
+	return true;
+}
+
 static void set_baseline_state(struct led_netdev_data *trigger_data)
 {
+	int i;
 	int current_brightness;
+	struct netdev_led_attr_detail *detail;
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
 
+	/* Modes already validated. Directly apply hw trigger modes */
+	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
+		/* We are refreshing the blink modes. Reset them */
+		led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
+					       BLINK_MODE_ZERO);
+
+		for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+			detail = &attr_details[i];
+
+			if (!test_bit(detail->bit, &trigger_data->mode))
+				continue;
+
+			led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
+						       BLINK_MODE_ENABLE);
+		}
+
+		led_cdev->hw_control_start(led_cdev);
+
+		return;
+	}
+
+	/* Handle trigger modes by software */
 	current_brightness = led_cdev->brightness;
 	if (current_brightness)
 		led_cdev->blink_brightness = current_brightness;
@@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
 				 size_t size)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+	struct net_device *old_net = trigger_data->net_dev;
+	char old_device_name[IFNAMSIZ];
 
 	if (size >= IFNAMSIZ)
 		return -EINVAL;
 
+	/* Backup old device name */
+	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
+
 	cancel_delayed_work_sync(&trigger_data->work);
 
 	spin_lock_bh(&trigger_data->lock);
@@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
 		trigger_data->net_dev =
 		    dev_get_by_name(&init_net, trigger_data->device_name);
 
+	if (!validate_baseline_state(trigger_data)) {
+		/* Restore old net_dev and device_name */
+		if (trigger_data->net_dev)
+			dev_put(trigger_data->net_dev);
+
+		dev_hold(old_net);
+		trigger_data->net_dev = old_net;
+		memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
+
+		spin_unlock_bh(&trigger_data->lock);
+		return -EINVAL;
+	}
+
 	trigger_data->carrier_link_up = false;
 	if (trigger_data->net_dev != NULL)
 		trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
@@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 				     size_t size, enum led_trigger_netdev_modes attr)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
-	unsigned long state;
+	unsigned long state, old_mode = trigger_data->mode;
 	int ret;
 	int bit;
 
@@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	else
 		clear_bit(bit, &trigger_data->mode);
 
+	if (!validate_baseline_state(trigger_data)) {
+		/* Restore old mode on validation fail */
+		trigger_data->mode = old_mode;
+		return -EINVAL;
+	}
+
 	set_baseline_state(trigger_data);
 
 	return size;
@@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
 			      size_t size)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+	int old_interval = atomic_read(&trigger_data->interval);
+	u32 old_mode = trigger_data->mode;
 	unsigned long value;
 	int ret;
 
@@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
 		return ret;
 
 	/* impose some basic bounds on the timer interval */
-	if (value >= 5 && value <= 10000) {
-		cancel_delayed_work_sync(&trigger_data->work);
+	if (value < 5 || value > 10000)
+		return -EINVAL;
+
+	cancel_delayed_work_sync(&trigger_data->work);
+
+	atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
 
-		atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
-		set_baseline_state(trigger_data);	/* resets timer */
+	if (!validate_baseline_state(trigger_data)) {
+		/* Restore old interval on validation error */
+		atomic_set(&trigger_data->interval, old_interval);
+		trigger_data->mode = old_mode;
+		return -EINVAL;
 	}
 
+	set_baseline_state(trigger_data);	/* resets timer */
+
 	return size;
 }
 
@@ -368,13 +498,25 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	trigger_data->mode = 0;
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
 	trigger_data->last_activity = 0;
+	if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
+		/* With hw mode enabled reset any rule set by default */
+		if (led_cdev->hw_control_status(led_cdev)) {
+			rc = led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
+							    BLINK_MODE_ZERO);
+			if (rc)
+				goto err;
+		}
+	}
 
 	led_set_trigger_data(led_cdev, trigger_data);
 
 	rc = register_netdevice_notifier(&trigger_data->notifier);
 	if (rc)
-		kfree(trigger_data);
+		goto err;
 
+	return 0;
+err:
+	kfree(trigger_data);
 	return rc;
 }
 
@@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 
 static struct led_trigger netdev_led_trigger = {
 	.name = "netdev",
+	.supported_blink_modes = SOFTWARE_HARDWARE,
 	.activate = netdev_trig_activate,
 	.deactivate = netdev_trig_deactivate,
 	.groups = netdev_trig_groups,
-- 
2.37.2

[PATCH v7 07/11] leds: trigger: netdev: use mutex instead of spinlocks

[Christian Marangi]

Some LEDs may require to sleep to apply their hardware rules. Convert to
mutex lock to fix warning for sleeping under spinlock softirq.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index ed019cb5867c..a471e0cde836 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,7 +20,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/timer.h>
 #include "../leds.h"
 
@@ -38,7 +38,7 @@
 
 struct led_netdev_data {
 	enum led_blink_modes blink_mode;
-	spinlock_t lock;
+	struct mutex lock;
 
 	struct delayed_work work;
 	struct notifier_block notifier;
@@ -183,9 +183,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;
 }
@@ -206,7 +206,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);
@@ -231,7 +231,7 @@ static ssize_t device_name_store(struct device *dev,
 		trigger_data->net_dev = old_net;
 		memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
 
-		spin_unlock_bh(&trigger_data->lock);
+		mutex_unlock(&trigger_data->lock);
 		return -EINVAL;
 	}
 
@@ -242,7 +242,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;
 }
@@ -400,7 +400,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);
 
 	trigger_data->carrier_link_up = false;
 	switch (evt) {
@@ -423,7 +423,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;
 }
@@ -484,7 +484,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;
-- 
2.37.2

[PATCH v7 08/11] leds: trigger: netdev: add available mode sysfs attr

[Christian Marangi]

Add avaiable_mode sysfs attr to show and give some details about the
supported modes and how they can be handled by the trigger.
This is in preparation for hardware only modes that doesn't support
software fallback.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 32 +++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index a471e0cde836..3a3b77bb41fb 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -33,6 +33,8 @@
  *         (has carrier) or not
  * tx -  LED blinks on transmitted data
  * rx -  LED blinks on receive data
+ * available_mode - Display available mode and how they can be handled
+ *                  by the LED
  *
  */
 
@@ -370,12 +372,42 @@ static ssize_t interval_store(struct device *dev,
 
 static DEVICE_ATTR_RW(interval);
 
+static ssize_t available_mode_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+	struct netdev_led_attr_detail *detail;
+	int i, len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+		detail = &attr_details[i];
+
+		if (led_trigger_blink_mode_is_supported(trigger_data->led_cdev, detail->bit)) {
+			if (!trigger_data->net_dev) {
+				if (detail->hardware_only)
+					len += sprintf(buf + len, "%s [hardware]\n",
+						       detail->name);
+				else
+					len += sprintf(buf + len, "%s [software-hardware]\n",
+						       detail->name);
+			}
+		} else {
+			len += sprintf(buf + len, "%s [software]\n", detail->name);
+		}
+	}
+
+	return len;
+}
+
+static DEVICE_ATTR_RO(available_mode);
+
 static struct attribute *netdev_trig_attrs[] = {
 	&dev_attr_device_name.attr,
 	&dev_attr_link.attr,
 	&dev_attr_rx.attr,
 	&dev_attr_tx.attr,
 	&dev_attr_interval.attr,
+	&dev_attr_available_mode.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(netdev_trig);
-- 
2.37.2

[PATCH v7 09/11] leds: trigger: netdev: add additional hardware only triggers

[Christian Marangi]

Add additional hardware only triggers commonly supported by switch LEDs.

Additional modes:
link_10: LED on with link up AND speed 10mbps
link_100: LED on with link up AND speed 100mbps
link_1000: LED on with link up AND speed 1000mbps
half_duplex: LED on with link up AND half_duplex mode
full_duplex: LED on with link up AND full duplex mode

Additional blink interval modes:
blink_2hz: LED blink on any even at 2Hz (250ms)
blink_4hz: LED blink on any even at 4Hz (125ms)
blink_8hz: LED blink on any even at 8Hz (62ms)

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 58 +++++++++++++++++++++++++++
 include/linux/leds.h                  | 10 +++++
 2 files changed, 68 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 3a3b77bb41fb..880d5e65f7a2 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -29,8 +29,20 @@
  *
  * device_name - network device name to monitor
  * interval - duration of LED blink, in milliseconds
+ *            (in hardware mode 2hz (62ms), 4hz (125ms) or 8hz (250ms)
+ *             are supported)
  * link -  LED's normal state reflects whether the link is up
  *         (has carrier) or not
+ * link_10 - LED's normal state reflects whether the link is
+ *           up and at 10mbps speed (hardware only)
+ * link_100 - LED's normal state reflects whether the link is
+ *            up and at 100mbps speed (hardware only)
+ * link_1000 - LED's normal state reflects whether the link is
+ *             up and at 1000mbps speed (hardware only)
+ * half_duplex - LED's normal state reflects whether the link is
+ *               up and hafl duplex (hardware only)
+ * full_duplex - LED's normal state reflects whether the link is
+ *               up and full duplex (hardware only)
  * tx -  LED blinks on transmitted data
  * rx -  LED blinks on receive data
  * available_mode - Display available mode and how they can be handled
@@ -64,8 +76,19 @@ struct netdev_led_attr_detail {
 
 static struct netdev_led_attr_detail attr_details[] = {
 	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
+	{ .name = "link_10", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_10},
+	{ .name = "link_100", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_100},
+	{ .name = "link_1000", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_1000},
+	{ .name = "half_duplex", .hardware_only = true, .bit = TRIGGER_NETDEV_HALF_DUPLEX},
+	{ .name = "full_duplex", .hardware_only = true, .bit = TRIGGER_NETDEV_FULL_DUPLEX},
 	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
 	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
+	{ .name = "hw blink 2hz (interval set to 62)", .hardware_only = true,
+	  .bit = TRIGGER_NETDEV_BLINK_2HZ},
+	{ .name = "hw blink 4hz (interval set to 125)", .hardware_only = true,
+	  .bit = TRIGGER_NETDEV_BLINK_4HZ},
+	{ .name = "hw blink 8hz (interval set to 250)", .hardware_only = true,
+	  .bit = TRIGGER_NETDEV_BLINK_8HZ},
 };
 
 static bool validate_baseline_state(struct led_netdev_data *trigger_data)
@@ -259,6 +282,11 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
 
 	switch (attr) {
 	case TRIGGER_NETDEV_LINK:
+	case TRIGGER_NETDEV_LINK_10:
+	case TRIGGER_NETDEV_LINK_100:
+	case TRIGGER_NETDEV_LINK_1000:
+	case TRIGGER_NETDEV_HALF_DUPLEX:
+	case TRIGGER_NETDEV_FULL_DUPLEX:
 	case TRIGGER_NETDEV_TX:
 	case TRIGGER_NETDEV_RX:
 		bit = attr;
@@ -284,6 +312,11 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 
 	switch (attr) {
 	case TRIGGER_NETDEV_LINK:
+	case TRIGGER_NETDEV_LINK_10:
+	case TRIGGER_NETDEV_LINK_100:
+	case TRIGGER_NETDEV_LINK_1000:
+	case TRIGGER_NETDEV_HALF_DUPLEX:
+	case TRIGGER_NETDEV_FULL_DUPLEX:
 	case TRIGGER_NETDEV_TX:
 	case TRIGGER_NETDEV_RX:
 		bit = attr;
@@ -324,6 +357,11 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	static DEVICE_ATTR_RW(trigger_name)
 
 DEFINE_NETDEV_TRIGGER(link, TRIGGER_NETDEV_LINK);
+DEFINE_NETDEV_TRIGGER(link_10, TRIGGER_NETDEV_LINK_10);
+DEFINE_NETDEV_TRIGGER(link_100, TRIGGER_NETDEV_LINK_100);
+DEFINE_NETDEV_TRIGGER(link_1000, TRIGGER_NETDEV_LINK_1000);
+DEFINE_NETDEV_TRIGGER(half_duplex, TRIGGER_NETDEV_HALF_DUPLEX);
+DEFINE_NETDEV_TRIGGER(full_duplex, TRIGGER_NETDEV_FULL_DUPLEX);
 DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
 DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
 
@@ -356,6 +394,21 @@ static ssize_t interval_store(struct device *dev,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
+	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
+		/* Interval are handled as triggers. Reset them. */
+		trigger_data->mode &= ~(BIT(TRIGGER_NETDEV_BLINK_8HZ) |
+					BIT(TRIGGER_NETDEV_BLINK_4HZ) |
+					BIT(TRIGGER_NETDEV_BLINK_2HZ));
+
+		/* Support a common value of 2Hz, 4Hz and 8Hz. */
+		if (value > 5 && value <= 62) /* 8Hz */
+			trigger_data->mode |= BIT(TRIGGER_NETDEV_BLINK_8HZ);
+		else if (value > 63 && value <= 125) /* 4Hz */
+			trigger_data->mode |= BIT(TRIGGER_NETDEV_BLINK_4HZ);
+		else /* 2Hz */
+			trigger_data->mode |= BIT(TRIGGER_NETDEV_BLINK_2HZ);
+	}
+
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
 
 	if (!validate_baseline_state(trigger_data)) {
@@ -404,6 +457,11 @@ static DEVICE_ATTR_RO(available_mode);
 static struct attribute *netdev_trig_attrs[] = {
 	&dev_attr_device_name.attr,
 	&dev_attr_link.attr,
+	&dev_attr_link_10.attr,
+	&dev_attr_link_100.attr,
+	&dev_attr_link_1000.attr,
+	&dev_attr_half_duplex.attr,
+	&dev_attr_full_duplex.attr,
 	&dev_attr_rx.attr,
 	&dev_attr_tx.attr,
 	&dev_attr_interval.attr,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 13862f8b1e07..5fcc6d233757 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -551,8 +551,18 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 /* Trigger specific enum */
 enum led_trigger_netdev_modes {
 	TRIGGER_NETDEV_LINK = 1,
+	TRIGGER_NETDEV_LINK_10,
+	TRIGGER_NETDEV_LINK_100,
+	TRIGGER_NETDEV_LINK_1000,
+	TRIGGER_NETDEV_HALF_DUPLEX,
+	TRIGGER_NETDEV_FULL_DUPLEX,
 	TRIGGER_NETDEV_TX,
 	TRIGGER_NETDEV_RX,
+
+	/* Hardware Interval options */
+	TRIGGER_NETDEV_BLINK_2HZ,
+	TRIGGER_NETDEV_BLINK_4HZ,
+	TRIGGER_NETDEV_BLINK_8HZ,
 };
 
 /* Trigger specific functions */
-- 
2.37.2

[PATCH v7 10/11] net: dsa: qca8k: add LEDs support

[Christian Marangi]

Add LEDs support for qca8k Switch Family. This will provide the LEDs
hardware API to permit the PHY LED to support hardware mode.
Each port have at least 3 LEDs and they can HW blink, set on/off or
follow blink modes configured with the LED in hardware mode.
Adds support for leds netdev trigger to support hardware triggers.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/Kconfig      |   9 +
 drivers/net/dsa/qca/Makefile     |   1 +
 drivers/net/dsa/qca/qca8k-8xxx.c |   4 +
 drivers/net/dsa/qca/qca8k-leds.c | 406 +++++++++++++++++++++++++++++++
 drivers/net/dsa/qca/qca8k.h      |  62 +++++
 5 files changed, 482 insertions(+)
 create mode 100644 drivers/net/dsa/qca/qca8k-leds.c

diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
index ba339747362c..be67164a444e 100644
--- a/drivers/net/dsa/qca/Kconfig
+++ b/drivers/net/dsa/qca/Kconfig
@@ -15,3 +15,12 @@ config NET_DSA_QCA8K
 	help
 	  This enables support for the Qualcomm Atheros QCA8K Ethernet
 	  switch chips.
+
+config NET_DSA_QCA8K_LEDS_SUPPORT
+	tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
+	depends on NET_DSA_QCA8K
+	select LEDS_OFFLOAD_TRIGGERS
+	help
+	  This enabled support for LEDs present on the Qualcomm Atheros
+	  QCA8K Ethernet switch chips. This require the LEDs offload
+	  triggers support as it can run in offload mode.
diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
index 701f1d199e93..330ae389e489 100644
--- a/drivers/net/dsa/qca/Makefile
+++ b/drivers/net/dsa/qca/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_NET_DSA_AR9331)	+= ar9331.o
 obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
 qca8k-y 			+= qca8k-common.o qca8k-8xxx.o
+obj-$(CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT) += qca8k-leds.o
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index c5c3b4e92f28..90916d8d8afe 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1690,6 +1690,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_led_ctrl(priv);
+	if (ret)
+		return ret;
+
 	qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
 	qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
 
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
new file mode 100644
index 000000000000..b51cdcae31b2
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -0,0 +1,406 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <net/dsa.h>
+#include <linux/regmap.h>
+
+#include "qca8k.h"
+
+static int
+qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+	int shift;
+
+	switch (port_num) {
+	case 0:
+		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+		reg_info->shift = 14;
+		break;
+	case 1:
+	case 2:
+	case 3:
+		reg_info->reg = QCA8K_LED_CTRL_REG(3);
+		shift = 2 * led_num + (6 * (port_num - 1));
+
+		reg_info->shift = 8 + shift;
+
+		break;
+	case 4:
+		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+		reg_info->shift = 30;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_get_control_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+	reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+
+	/* 6 total control rule:
+	 * 3 control rules for phy0-3 that applies to all their leds
+	 * 3 control rules for phy4
+	 */
+	if (port_num == 4)
+		reg_info->shift = 16;
+	else
+		reg_info->shift = 0;
+
+	return 0;
+}
+
+static int
+qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger, u32 *mask)
+{
+	/* Parsing specific to netdev trigger */
+	if (test_bit(TRIGGER_NETDEV_LINK, &rules))
+		*offload_trigger = QCA8K_LED_LINK_10M_EN_MASK |
+				   QCA8K_LED_LINK_100M_EN_MASK |
+				   QCA8K_LED_LINK_1000M_EN_MASK;
+	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+		*offload_trigger = QCA8K_LED_LINK_10M_EN_MASK;
+	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+		*offload_trigger = QCA8K_LED_LINK_100M_EN_MASK;
+	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+		*offload_trigger = QCA8K_LED_LINK_1000M_EN_MASK;
+	if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+		*offload_trigger = QCA8K_LED_HALF_DUPLEX_MASK;
+	if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+		*offload_trigger = QCA8K_LED_FULL_DUPLEX_MASK;
+	if (test_bit(TRIGGER_NETDEV_TX, &rules))
+		*offload_trigger = QCA8K_LED_TX_BLINK_MASK;
+	if (test_bit(TRIGGER_NETDEV_RX, &rules))
+		*offload_trigger = QCA8K_LED_RX_BLINK_MASK;
+	if (test_bit(TRIGGER_NETDEV_BLINK_2HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_2HZ;
+	if (test_bit(TRIGGER_NETDEV_BLINK_4HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_4HZ;
+	if (test_bit(TRIGGER_NETDEV_BLINK_8HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_8HZ;
+
+	if (!*offload_trigger)
+		return -EOPNOTSUPP;
+
+	if (test_bit(TRIGGER_NETDEV_BLINK_2HZ, &rules) ||
+	    test_bit(TRIGGER_NETDEV_BLINK_4HZ, &rules) ||
+	    test_bit(TRIGGER_NETDEV_BLINK_8HZ, &rules)) {
+		*mask = QCA8K_LED_BLINK_FREQ_MASK;
+	} else {
+		*mask = *offload_trigger;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
+				enum blink_mode_cmd cmd)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+	struct led_trigger *trigger = ldev->trigger;
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 offload_trigger = 0, mask, val;
+	int ret;
+
+	/* Check trigger compatibility */
+	if (strcmp(trigger->name, "netdev"))
+		return -EOPNOTSUPP;
+
+	if (!strcmp(trigger->name, "netdev"))
+		ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
+
+	if (ret)
+		return ret;
+
+	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
+
+	switch (cmd) {
+	case BLINK_MODE_SUPPORTED:
+		/* We reach this point, we are sure the trigger is supported */
+		return 1;
+	case BLINK_MODE_ZERO:
+		/* We set 4hz by default */
+		u32 default_reg = QCA8K_LED_BLINK_4HZ;
+
+		ret = regmap_update_bits(priv->regmap, reg_info.reg,
+					 QCA8K_LED_RULE_MASK << reg_info.shift,
+					 default_reg << reg_info.shift);
+		break;
+	case BLINK_MODE_ENABLE:
+		ret = regmap_update_bits(priv->regmap, reg_info.reg,
+					 mask << reg_info.shift,
+					 offload_trigger << reg_info.shift);
+		break;
+	case BLINK_MODE_DISABLE:
+		ret = regmap_update_bits(priv->regmap, reg_info.reg,
+					 mask << reg_info.shift,
+					 0);
+		break;
+	case BLINK_MODE_READ:
+		ret = regmap_read(priv->regmap, reg_info.reg, &val);
+		if (ret)
+			return ret;
+
+		val >>= reg_info.shift;
+		val &= offload_trigger;
+
+		/* Special handling for LED_BLINK_2HZ */
+		if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
+			val = 1;
+
+		return val;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
+static void
+qca8k_led_brightness_set(struct qca8k_led *led,
+			 enum led_brightness b)
+{
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val = QCA8K_LED_ALWAYS_OFF;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	if (b)
+		val = QCA8K_LED_ALWAYS_ON;
+
+	regmap_update_bits(priv->regmap, reg_info.reg,
+			   GENMASK(1, 0) << reg_info.shift,
+			   val << reg_info.shift);
+}
+
+static void
+qca8k_cled_brightness_set(struct led_classdev *ldev,
+			  enum led_brightness b)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	return qca8k_led_brightness_set(led, b);
+}
+
+static enum led_brightness
+qca8k_led_brightness_get(struct qca8k_led *led)
+{
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val;
+	int ret;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	ret = regmap_read(priv->regmap, reg_info.reg, &val);
+	if (ret)
+		return 0;
+
+	val >>= reg_info.shift;
+	val &= GENMASK(1, 0);
+
+	return val > 0 ? 1 : 0;
+}
+
+static enum led_brightness
+qca8k_cled_brightness_get(struct led_classdev *ldev)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	return qca8k_led_brightness_get(led);
+}
+
+static int
+qca8k_cled_blink_set(struct led_classdev *ldev,
+		     unsigned long *delay_on,
+		     unsigned long *delay_off)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		*delay_on = 125;
+		*delay_off = 125;
+	}
+
+	if (*delay_on != 125 || *delay_off != 125) {
+		/* The hardware only supports blinking at 4Hz. Fall back
+		 * to software implementation in other cases.
+		 */
+		return -EINVAL;
+	}
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	regmap_update_bits(priv->regmap, reg_info.reg,
+			   GENMASK(1, 0) << reg_info.shift,
+			   QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
+
+	return 0;
+}
+
+static int
+qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val = QCA8K_LED_ALWAYS_OFF;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	if (enable)
+		val = QCA8K_LED_RULE_CONTROLLED;
+
+	return regmap_update_bits(priv->regmap, reg_info.reg,
+				  GENMASK(1, 0) << reg_info.shift,
+				  val << reg_info.shift);
+}
+
+static int
+qca8k_cled_hw_control_start(struct led_classdev *led_cdev)
+{
+	return qca8k_cled_trigger_offload(led_cdev, true);
+}
+
+static int
+qca8k_cled_hw_control_stop(struct led_classdev *led_cdev)
+{
+	return qca8k_cled_trigger_offload(led_cdev, false);
+}
+
+static bool
+qca8k_cled_hw_control_status(struct led_classdev *ldev)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	regmap_read(priv->regmap, reg_info.reg, &val);
+
+	val >>= reg_info.shift;
+	val &= GENMASK(1, 0);
+
+	return val == QCA8K_LED_RULE_CONTROLLED;
+}
+
+static int
+qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
+{
+	struct fwnode_handle *led = NULL, *leds = NULL;
+	struct led_init_data init_data = { };
+	struct qca8k_led *port_led;
+	int led_num, port_index;
+	const char *state;
+	int ret;
+
+	leds = fwnode_get_named_child_node(port, "leds");
+	if (!leds) {
+		dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
+			port_num);
+		return 0;
+	}
+
+	fwnode_for_each_child_node(leds, led) {
+		/* Reg represent the led number of the port.
+		 * Each port can have at least 3 leds attached
+		 * Commonly:
+		 * 1. is gigabit led
+		 * 2. is mbit led
+		 * 3. additional status led
+		 */
+		if (fwnode_property_read_u32(led, "reg", &led_num))
+			continue;
+
+		if (led_num >= QCA8K_LED_PORT_COUNT) {
+			dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
+			continue;
+		}
+
+		port_index = 3 * port_num + led_num;
+
+		port_led = &priv->ports_led[port_index];
+		port_led->port_num = port_num;
+		port_led->led_num = led_num;
+		port_led->priv = priv;
+
+		ret = fwnode_property_read_string(led, "default-state", &state);
+		if (!ret) {
+			if (!strcmp(state, "on")) {
+				port_led->cdev.brightness = 1;
+				qca8k_led_brightness_set(port_led, 1);
+			} else if (!strcmp(state, "off")) {
+				port_led->cdev.brightness = 0;
+				qca8k_led_brightness_set(port_led, 0);
+			} else if (!strcmp(state, "keep")) {
+				port_led->cdev.brightness =
+					qca8k_led_brightness_get(port_led);
+			}
+		}
+
+		/* 3 brightness settings can be applied from Documentation:
+		 * 0 always off
+		 * 1 blink at 4Hz
+		 * 2 always on
+		 * 3 rule controlled
+		 * Suppots only 2 mode: (pcb limitation, with always on and blink
+		 * only the last led is set to this mode)
+		 * 0 always off (sets all leds off)
+		 * 3 rule controlled
+		 */
+		port_led->cdev.blink_mode = SOFTWARE_HARDWARE_CONTROLLED;
+		port_led->cdev.max_brightness = 1;
+		port_led->cdev.brightness_set = qca8k_cled_brightness_set;
+		port_led->cdev.brightness_get = qca8k_cled_brightness_get;
+		port_led->cdev.blink_set = qca8k_cled_blink_set;
+		port_led->cdev.hw_control_start = qca8k_cled_hw_control_start;
+		port_led->cdev.hw_control_stop = qca8k_cled_hw_control_stop;
+		port_led->cdev.hw_control_status = qca8k_cled_hw_control_status;
+		port_led->cdev.hw_control_configure = qca8k_cled_hw_control_configure;
+		init_data.default_label = ":port";
+		init_data.devicename = "qca8k";
+		init_data.fwnode = led;
+
+		ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
+		if (ret)
+			dev_warn(priv->dev, "Failed to int led");
+	}
+
+	return 0;
+}
+
+int
+qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+	struct fwnode_handle *mdio, *port;
+	int port_num;
+	int ret;
+
+	mdio = device_get_named_child_node(priv->dev, "mdio");
+	if (!mdio) {
+		dev_info(priv->dev, "No MDIO node specified in device tree!\n");
+		return 0;
+	}
+
+	fwnode_for_each_child_node(mdio, port) {
+		if (fwnode_property_read_u32(port, "reg", &port_num))
+			continue;
+
+		/* Each port can have at least 3 different leds attached */
+		ret = qca8k_parse_port_leds(priv, port, port_num);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 0b7a5cb12321..5c535baa7139 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -11,6 +11,7 @@
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/gpio.h>
+#include <linux/leds.h>
 #include <linux/dsa/tag_qca.h>
 
 #define QCA8K_ETHERNET_MDIO_PRIORITY			7
@@ -85,6 +86,43 @@
 #define   QCA8K_MDIO_MASTER_DATA(x)			FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
 #define   QCA8K_MDIO_MASTER_MAX_PORTS			5
 #define   QCA8K_MDIO_MASTER_MAX_REG			32
+
+/* LED control register */
+#define QCA8K_LED_COUNT					15
+#define QCA8K_LED_PORT_COUNT				3
+#define QCA8K_LED_RULE_COUNT				6
+#define QCA8K_LED_RULE_MAX				11
+#define QCA8K_LED_CTRL_REG(_i)				(0x050 + (_i) * 4)
+#define QCA8K_LED_CTRL0_REG				0x50
+#define QCA8K_LED_CTRL1_REG				0x54
+#define QCA8K_LED_CTRL2_REG				0x58
+#define QCA8K_LED_CTRL3_REG				0x5C
+#define   QCA8K_LED_CTRL_SHIFT(_i)			(((_i) % 2) * 16)
+#define   QCA8K_LED_CTRL_MASK				GENMASK(15, 0)
+#define QCA8K_LED_RULE_MASK				GENMASK(13, 0)
+#define QCA8K_LED_BLINK_FREQ_MASK			GENMASK(1, 0)
+#define QCA8K_LED_BLINK_FREQ_SHITF			0
+#define   QCA8K_LED_BLINK_2HZ				0
+#define   QCA8K_LED_BLINK_4HZ				1
+#define   QCA8K_LED_BLINK_8HZ				2
+#define   QCA8K_LED_BLINK_AUTO				3
+#define QCA8K_LED_LINKUP_OVER_MASK			BIT(2)
+#define QCA8K_LED_TX_BLINK_MASK				BIT(4)
+#define QCA8K_LED_RX_BLINK_MASK				BIT(5)
+#define QCA8K_LED_COL_BLINK_MASK			BIT(7)
+#define QCA8K_LED_LINK_10M_EN_MASK			BIT(8)
+#define QCA8K_LED_LINK_100M_EN_MASK			BIT(9)
+#define QCA8K_LED_LINK_1000M_EN_MASK			BIT(10)
+#define QCA8K_LED_POWER_ON_LIGHT_MASK			BIT(11)
+#define QCA8K_LED_HALF_DUPLEX_MASK			BIT(12)
+#define QCA8K_LED_FULL_DUPLEX_MASK			BIT(13)
+#define QCA8K_LED_PATTERN_EN_MASK			GENMASK(15, 14)
+#define QCA8K_LED_PATTERN_EN_SHIFT			14
+#define   QCA8K_LED_ALWAYS_OFF				0
+#define   QCA8K_LED_ALWAYS_BLINK_4HZ			1
+#define   QCA8K_LED_ALWAYS_ON				2
+#define   QCA8K_LED_RULE_CONTROLLED			3
+
 #define QCA8K_GOL_MAC_ADDR0				0x60
 #define QCA8K_GOL_MAC_ADDR1				0x64
 #define QCA8K_MAX_FRAME_SIZE				0x78
@@ -388,6 +426,19 @@ struct qca8k_pcs {
 	int port;
 };
 
+struct qca8k_led_pattern_en {
+	u32 reg;
+	u8 shift;
+};
+
+struct qca8k_led {
+	u8 port_num;
+	u8 led_num;
+	u16 old_rule;
+	struct qca8k_priv *priv;
+	struct led_classdev cdev;
+};
+
 struct qca8k_priv {
 	u8 switch_id;
 	u8 switch_revision;
@@ -412,6 +463,7 @@ struct qca8k_priv {
 	struct qca8k_pcs pcs_port_0;
 	struct qca8k_pcs pcs_port_6;
 	const struct qca8k_match_data *info;
+	struct qca8k_led ports_led[QCA8K_LED_COUNT];
 };
 
 struct qca8k_mib_desc {
@@ -517,4 +569,14 @@ int qca8k_port_lag_join(struct dsa_switch *ds, int port, struct dsa_lag lag,
 int qca8k_port_lag_leave(struct dsa_switch *ds, int port,
 			 struct dsa_lag lag);
 
+/* Leds Support function */
+#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
+#else
+static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+	return 0;
+}
+#endif
+
 #endif /* __QCA8K_H */
-- 
2.37.2

[PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Christian Marangi]

Add LEDs definition example for qca8k using the offload trigger as the
default trigger and add all the supported offload triggers by the
switch.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index 978162df51f7..4090cf65c41c 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -65,6 +65,8 @@ properties:
                  internal mdio access is used.
                  With the legacy mapping the reg corresponding to the internal
                  mdio is the switch reg with an offset of -1.
+                 Each phy have at least 3 LEDs connected and can be declared
+                 using the standard LEDs structure.
 
 patternProperties:
   "^(ethernet-)?ports$":
@@ -202,6 +204,7 @@ examples:
     };
   - |
     #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
 
     mdio {
         #address-cells = <1>;
@@ -284,6 +287,27 @@ examples:
 
                 internal_phy_port1: ethernet-phy@0 {
                     reg = <0>;
+
+                    leds {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+
+                        led@0 {
+                            reg = <0>;
+                            color = <LED_COLOR_ID_WHITE>;
+                            function = LED_FUNCTION_LAN;
+                            function-enumerator = <1>;
+                            linux,default-trigger = "netdev";
+                        };
+
+                        led@1 {
+                            reg = <1>;
+                            color = <LED_COLOR_ID_AMBER>;
+                            function = LED_FUNCTION_LAN;
+                            function-enumerator = <1>;
+                            linux,default-trigger = "netdev";
+                        };
+                    };
                 };
 
                 internal_phy_port2: ethernet-phy@1 {
-- 
2.37.2

Re: [PATCH v7 10/11] net: dsa: qca8k: add LEDs support

[Arun.Ramadoss]

Hi Christian,

On Thu, 2022-12-15 at 00:54 +0100, Christian Marangi wrote:
> Add LEDs support for qca8k Switch Family. This will provide the LEDs
> hardware API to permit the PHY LED to support hardware mode.
> Each port have at least 3 LEDs and they can HW blink, set on/off or
> follow blink modes configured with the LED in hardware mode.
> Adds support for leds netdev trigger to support hardware triggers.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca/Kconfig      |   9 +
>  drivers/net/dsa/qca/Makefile     |   1 +
>  drivers/net/dsa/qca/qca8k-8xxx.c |   4 +
>  drivers/net/dsa/qca/qca8k-leds.c | 406
> +++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca/qca8k.h      |  62 +++++
>  5 files changed, 482 insertions(+)
>  create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
> 
> 
> diff --git a/drivers/net/dsa/qca/qca8k-leds.c
> b/drivers/net/dsa/qca/qca8k-leds.c
> new file mode 100644
> index 000000000000..b51cdcae31b2
> --- /dev/null
> +++ b/drivers/net/dsa/qca/qca8k-leds.c
> @@ -0,0 +1,406 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <net/dsa.h>
> +#include <linux/regmap.h>

Alphabatical order

> +
> +#include "qca8k.h"
> +
> +static int
> +qca8k_get_enable_led_reg(int port_num, int led_num, struct
> qca8k_led_pattern_en *reg_info)
> +{
> +	int shift;
> +
> +	switch (port_num) {
> +	case 0:
> +		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> +		reg_info->shift = 14;

Any macros other than magic number for 14, 30 

> +		break;
> +	case 1:
> +	case 2:
> +	case 3:
> +		reg_info->reg = QCA8K_LED_CTRL_REG(3);
> +		shift = 2 * led_num + (6 * (port_num - 1));
> +
> +		reg_info->shift = 8 + shift;
> +
> +		break;
> +	case 4:
> +		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> +		reg_info->shift = 30;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> 
> +
> +static void
> +qca8k_led_brightness_set(struct qca8k_led *led,
> +			 enum led_brightness b)

variable name other than b to have readability in code

> +{
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num,
> &reg_info);
> +
> +	if (b)
> +		val = QCA8K_LED_ALWAYS_ON;
> +
> +	regmap_update_bits(priv->regmap, reg_info.reg,
> +			   GENMASK(1, 0) << reg_info.shift,

GENMASK(1, 0) used in multiple place, replace with macro for
readability

> +			   val << reg_info.shift);
> +}
> +
> 
> +
> +static int
> +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led,
> cdev);
> +

Blank line

> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num,
> &reg_info);
> +
> +	if (enable)
> +		val = QCA8K_LED_RULE_CONTROLLED;
> +
> +	return regmap_update_bits(priv->regmap, reg_info.reg,
> +				  GENMASK(1, 0) << reg_info.shift,
> +				  val << reg_info.shift);
> +}
> +
> 
> +static bool
> +qca8k_cled_hw_control_status(struct led_classdev *ldev)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led,
> cdev);
> +

Blank line

> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num,
> &reg_info);
> +
> +	regmap_read(priv->regmap, reg_info.reg, &val);
> +
> +	val >>= reg_info.shift;
> +	val &= GENMASK(1, 0);
> +
> +	return val == QCA8K_LED_RULE_CONTROLLED;
> +}
> +
> 

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Alexander Stein]

Hi,

thanks for the v7 series.

Am Donnerstag, 15. Dezember 2022, 00:54:33 CET schrieb Christian Marangi:
> Add hardware control support for the Netdev trigger.
> The trigger on config change will check if the requested trigger can set
> to blink mode using LED hardware mode and if every blink mode is supported,
> the trigger will enable hardware mode with the requested configuration.
> If there is at least one trigger that is not supported and can't run in
> hardware mode, then software mode will be used instead.
> A validation is done on every value change and on fail the old value is
> restored and -EINVAL is returned.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
>  1 file changed, 149 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c
> b/drivers/leds/trigger/ledtrig-netdev.c index dd63cadb896e..ed019cb5867c
> 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -37,6 +37,7 @@
>   */
> 
>  struct led_netdev_data {
> +	enum led_blink_modes blink_mode;
>  	spinlock_t lock;
> 
>  	struct delayed_work work;
> @@ -53,11 +54,105 @@ struct led_netdev_data {
>  	bool carrier_link_up;
>  };
> 
> +struct netdev_led_attr_detail {
> +	char *name;
> +	bool hardware_only;
> +	enum led_trigger_netdev_modes bit;
> +};
> +
> +static struct netdev_led_attr_detail attr_details[] = {
> +	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
> +	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
> +	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
> +};
> +
> +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> +{
> +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> +	struct netdev_led_attr_detail *detail;
> +	u32 hw_blink_mode_supported = 0;
> +	bool force_sw = false;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> +		detail = &attr_details[i];
> +
> +		/* Mode not active, skip */
> +		if (!test_bit(detail->bit, &trigger_data->mode))
> +			continue;
> +
> +		/* Hardware only mode enabled on software controlled led 
*/
> +		if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> +		    detail->hardware_only)
> +			return false;
> +
> +		/* Check if the mode supports hardware mode */
> +		if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> +			/* With a net dev set, force software mode.
> +			 * With modes are handled by hardware, led will 
blink
> +			 * based on his own events and will ignore any 
event
> +			 * from the provided dev.
> +			 */
> +			if (trigger_data->net_dev) {
> +				force_sw = true;
> +				continue;
> +			}
> +
> +			/* With empty dev, check if the mode is 
supported */
> +			if 
(led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> +				hw_blink_mode_supported |= BIT(detail-
>bit);

Shouldn't this be BIT(detail->bit)?

> +		}
> +	}
> +
> +	/* We can't run modes handled by both software and hardware.
> +	 * Check if we run hardware modes and check if all the modes
> +	 * can be handled by hardware.
> +	 */
> +	if (hw_blink_mode_supported && hw_blink_mode_supported !=
> trigger_data->mode) +		return false;
> +
> +	/* Modes are valid. Decide now the running mode to later
> +	 * set the baseline.
> +	 * Software mode is enforced with net_dev set. With an empty
> +	 * one hardware mode is selected by default (if supported).
> +	 */
> +	if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)

IMHO '|| !hw_blink_mode_supported' should be added here for blink_modes. This 
might happen if a PHY LED is SOFTWARE_HARDWARE_CONTROLLED, but some blink mode 
is not supported by hardware, thus hw_blink_mode_supported=0.

Best regards,
Alexander

> +		trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> +	else
> +		trigger_data->blink_mode = HARDWARE_CONTROLLED;
> +
> +	return true;
> +}
> +
>  static void set_baseline_state(struct led_netdev_data *trigger_data)
>  {
> +	int i;
>  	int current_brightness;
> +	struct netdev_led_attr_detail *detail;
>  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> 
> +	/* Modes already validated. Directly apply hw trigger modes */
> +	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> +		/* We are refreshing the blink modes. Reset them */
> +		led_cdev->hw_control_configure(led_cdev, 
BIT(TRIGGER_NETDEV_LINK),
> +					       BLINK_MODE_ZERO);
> +
> +		for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> +			detail = &attr_details[i];
> +
> +			if (!test_bit(detail->bit, &trigger_data->mode))
> +				continue;
> +
> +			led_cdev->hw_control_configure(led_cdev, 
BIT(detail->bit),
> +						       
BLINK_MODE_ENABLE);

Shouldn't this be BIT(detail->bit)?

> +		}
> +
> +		led_cdev->hw_control_start(led_cdev);
> +
> +		return;
> +	}
> +
> +	/* Handle trigger modes by software */
>  	current_brightness = led_cdev->brightness;
>  	if (current_brightness)
>  		led_cdev->blink_brightness = current_brightness;
> @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
>  				 size_t size)
>  {
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> +	struct net_device *old_net = trigger_data->net_dev;
> +	char old_device_name[IFNAMSIZ];
> 
>  	if (size >= IFNAMSIZ)
>  		return -EINVAL;
> 
> +	/* Backup old device name */
> +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> +
>  	cancel_delayed_work_sync(&trigger_data->work);
> 
>  	spin_lock_bh(&trigger_data->lock);
> @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
>  		trigger_data->net_dev =
>  		    dev_get_by_name(&init_net, trigger_data->device_name);
> 
> +	if (!validate_baseline_state(trigger_data)) {
> +		/* Restore old net_dev and device_name */
> +		if (trigger_data->net_dev)
> +			dev_put(trigger_data->net_dev);
> +
> +		dev_hold(old_net);
> +		trigger_data->net_dev = old_net;
> +		memcpy(trigger_data->device_name, old_device_name, 
IFNAMSIZ);
> +
> +		spin_unlock_bh(&trigger_data->lock);
> +		return -EINVAL;
> +	}
> +
>  	trigger_data->carrier_link_up = false;
>  	if (trigger_data->net_dev != NULL)
>  		trigger_data->carrier_link_up = 
netif_carrier_ok(trigger_data->net_dev);
> @@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device *dev,
> const char *buf, size_t size, enum led_trigger_netdev_modes attr)
>  {
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> -	unsigned long state;
> +	unsigned long state, old_mode = trigger_data->mode;
>  	int ret;
>  	int bit;
> 
> @@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device
> *dev, const char *buf, else
>  		clear_bit(bit, &trigger_data->mode);
> 
> +	if (!validate_baseline_state(trigger_data)) {
> +		/* Restore old mode on validation fail */
> +		trigger_data->mode = old_mode;
> +		return -EINVAL;
> +	}
> +
>  	set_baseline_state(trigger_data);
> 
>  	return size;
> @@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
>  			      size_t size)
>  {
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> +	int old_interval = atomic_read(&trigger_data->interval);
> +	u32 old_mode = trigger_data->mode;
>  	unsigned long value;
>  	int ret;
> 
> @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
>  		return ret;
> 
>  	/* impose some basic bounds on the timer interval */
> -	if (value >= 5 && value <= 10000) {
> -		cancel_delayed_work_sync(&trigger_data->work);
> +	if (value < 5 || value > 10000)
> +		return -EINVAL;
> +
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
> +	atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> 
> -		atomic_set(&trigger_data->interval, 
msecs_to_jiffies(value));
> -		set_baseline_state(trigger_data);	/* resets timer 
*/
> +	if (!validate_baseline_state(trigger_data)) {
> +		/* Restore old interval on validation error */
> +		atomic_set(&trigger_data->interval, old_interval);
> +		trigger_data->mode = old_mode;
> +		return -EINVAL;
>  	}
> 
> +	set_baseline_state(trigger_data);	/* resets timer */
> +
>  	return size;
>  }
> 
> @@ -368,13 +498,25 @@ static int netdev_trig_activate(struct led_classdev
> *led_cdev) trigger_data->mode = 0;
>  	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
>  	trigger_data->last_activity = 0;
> +	if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> +		/* With hw mode enabled reset any rule set by default */
> +		if (led_cdev->hw_control_status(led_cdev)) {
> +			rc = led_cdev->hw_control_configure(led_cdev, 
BIT(TRIGGER_NETDEV_LINK),
> +							    
BLINK_MODE_ZERO);
> +			if (rc)
> +				goto err;
> +		}
> +	}
> 
>  	led_set_trigger_data(led_cdev, trigger_data);
> 
>  	rc = register_netdevice_notifier(&trigger_data->notifier);
>  	if (rc)
> -		kfree(trigger_data);
> +		goto err;
> 
> +	return 0;
> +err:
> +	kfree(trigger_data);
>  	return rc;
>  }
> 
> @@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct led_classdev
> *led_cdev)
> 
>  static struct led_trigger netdev_led_trigger = {
>  	.name = "netdev",
> +	.supported_blink_modes = SOFTWARE_HARDWARE,
>  	.activate = netdev_trig_activate,
>  	.deactivate = netdev_trig_deactivate,
>  	.groups = netdev_trig_groups,

Re: [PATCH v7 00/11] Adds support for PHY LEDs with offload triggers

[Alexander Stein]

Hi,

thanks for the new series.

Am Donnerstag, 15. Dezember 2022, 00:54:27 CET schrieb Christian Marangi:
> This is another attempt on adding this feature on LEDs, hoping this is
> the right time and someone finally notice this.

Unfortunately I'm out of office from next week on, so there is only limited 
feedback from my side.

> Most of the times Switch/PHY have connected multiple LEDs that are
> controlled by HW based on some rules/event. Currently we lack any
> support for a generic way to control the HW part and normally we
> either never implement the feature or only add control for brightness
> or hw blink.
> 
> This is based on Marek idea of providing some API to cled but use a
> different implementation that in theory should be more generilized.
> 
> The current idea is:
> - LED driver implement 3 API (hw_control_status/start/stop).
>   They are used to put the LED in hardware mode and to configure the
>   various trigger.
> - We have hardware triggers that are used to expose to userspace the
>   supported hardware mode and set the hardware mode on trigger
>   activation.
> - We can also have triggers that both support hardware and software mode.
> - The LED driver will declare each supported hardware blink mode and
>   communicate with the trigger all the supported blink modes that will
>   be available by sysfs.
> - A trigger will use blink_set to configure the blink mode to active
>   in hardware mode.
> - On hardware trigger activation, only the hardware mode is enabled but
>   the blink modes are not configured. The LED driver should reset any
>   link mode active by default.

I'm a bit confused about that blink mode is supposed to mean. I don't know 
what to implement for blink_set. Reading qca8k_cled_blink_set it seems to just 
configure the blink interval for the corresponding LED.
Unfortunately that's not possible for all PHYs. In my case, DP83867, I can 
configure the blink interval only globally. I'm not sure how this will fit 
into this LED trigger.

> Each LED driver will have to declare explicit support for the offload
> trigger (or return not supported error code) as we the trigger_data that
> the LED driver will elaborate and understand what is referring to (based
> on the current active trigger).
> 
> I posted a user for this new implementation that will benefit from this
> and will add a big feature to it. Currently qca8k can have up to 3 LEDs
> connected to each PHY port and we have some device that have only one of
> them connected and the default configuration won't work for that.

My DP83867 proof of concept implementation also supports several LEDs, but my 
actual hardware also only has 2 of them attached (LED0 and LED2).

Best regards,
Alexander

> The netdev trigger is expanded and it does now support hardware only
> triggers.
> The idea is to use hardware mode when a device_name is not defined.
> An additional sysfs entry is added to give some info about the available
> trigger modes supported in the current configuration.
> 
> 
> It was reported that at least 3 other switch family would benefits by
> this as they all lack support for a generic way to setup their leds and
> netdev team NACK each try to add special code to support LEDs present
> on switch in favor of a generic solution.
> 
> v7:
> - Rebase on top of net-next (for qca8k changes)
> - Fix some typo in commit description
> - Fix qca8k leds documentation warning
> - Remove RFC tag
> v6:
> - Back to RFC.
> - Drop additional trigger
> - Rework netdev trigger to support common modes used by switch and
>   hardware only triggers
> - Refresh qca8k leds logic and driver
> v5:
> - Move out of RFC. (no comments from Andrew this is the right path?)
> - Fix more spelling mistake (thx Randy)
> - Fix error reported by kernel test bot
> - Drop the additional HW_CONTROL flag. It does simplify CONFIG
>   handling and hw control should be available anyway to support
>   triggers as module.
> v4:
> - Rework implementation and drop hw_configure logic.
>   We now expand blink_set.
> - Address even more spelling mistake. (thx a lot Randy)
> - Drop blink option and use blink_set delay.
> - Rework phy-activity trigger to actually make the groups dynamic.
> v3:
> - Rework start/stop as Andrew asked.
> - Introduce more logic to permit a trigger to run in hardware mode.
> - Add additional patch with netdev hardware support.
> - Use test_bit API to check flag passed to hw_control_configure.
> - Added a new cmd to hw_control_configure to reset any active blink_mode.
> - Refactor all the patches to follow this new implementation.
> v2:
> - Fix spelling mistake (sorry)
> - Drop patch 02 "permit to declare supported offload triggers".
>   Change the logic, now the LED driver declare support for them
>   using the configure_offload with the cmd TRIGGER_SUPPORTED.
> - Rework code to follow this new implementation.
> - Update Documentation to better describe how this offload
>   implementation work.
> 
> Christian Marangi (11):
>   leds: add support for hardware driven LEDs
>   leds: add function to configure hardware controlled LED
>   leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode
>   leds: trigger: netdev: rename and expose NETDEV trigger enum modes
>   leds: trigger: netdev: convert device attr to macro
>   leds: trigger: netdev: add hardware control support
>   leds: trigger: netdev: use mutex instead of spinlocks
>   leds: trigger: netdev: add available mode sysfs attr
>   leds: trigger: netdev: add additional hardware only triggers
>   net: dsa: qca8k: add LEDs support
>   dt-bindings: net: dsa: qca8k: add LEDs definition example
> 
>  .../devicetree/bindings/net/dsa/qca8k.yaml    |  24 ++
>  Documentation/leds/leds-class.rst             |  53 +++
>  drivers/leds/Kconfig                          |  11 +
>  drivers/leds/led-class.c                      |  27 ++
>  drivers/leds/led-triggers.c                   |  29 ++
>  drivers/leds/trigger/ledtrig-netdev.c         | 385 ++++++++++++-----
>  drivers/net/dsa/qca/Kconfig                   |   9 +
>  drivers/net/dsa/qca/Makefile                  |   1 +
>  drivers/net/dsa/qca/qca8k-8xxx.c              |   4 +
>  drivers/net/dsa/qca/qca8k-leds.c              | 406 ++++++++++++++++++
>  drivers/net/dsa/qca/qca8k.h                   |  62 +++
>  include/linux/leds.h                          | 103 ++++-
>  12 files changed, 1015 insertions(+), 99 deletions(-)
>  create mode 100644 drivers/net/dsa/qca/qca8k-leds.c

Re: [PATCH v7 01/11] leds: add support for hardware driven LEDs

[Russell King (Oracle)]

Hi Christian,

Thanks for the patch.

I think Andrew's email is offline at the moment.

On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> +static bool led_trigger_is_supported(struct led_classdev *led_cdev,
> +				     struct led_trigger *trigger)
> +{
> +	switch (led_cdev->blink_mode) {
> +	case SOFTWARE_CONTROLLED:
> +		if (trigger->supported_blink_modes == HARDWARE_ONLY)
> +			return 0;
> +		break;
> +	case HARDWARE_CONTROLLED:
> +		if (trigger->supported_blink_modes == SOFTWARE_ONLY)
> +			return 0;
> +		break;
> +	case SOFTWARE_HARDWARE_CONTROLLED:
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return 1;

Should be returning true/false. I'm not sure I'm a fan of the style of
this though - wouldn't the following be easier to read?

	switch (led_cdev->blink_mode) {
	case SOFTWARE_CONTROLLED:
		return trigger->supported_blink_modes != HARDWARE_ONLY;

	case HARDWARE_CONTROLLED:
		return trigger->supported_blink_modes != SOFTWARE_ONLY;

	case SOFTWARE_HARDWARE_CONTROLLED:
		return true;
	}
?

Also, does it really need a default case - without it, when the
led_blink_modes enum is expanded and the switch statement isn't
updated, we'll get a compiler warning which will prompt this to be
updated - whereas, with a default case, it won't.

> @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
>  		led_set_brightness(led_cdev, LED_OFF);
>  	}
>  	if (trig) {
> +		/* Make sure the trigger support the LED blink mode */
> +		if (!led_trigger_is_supported(led_cdev, trig))
> +			return -EINVAL;

Shouldn't validation happen before we start taking any actions? In other
words, before we remove the previous trigger?

> @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
>  
>  #define TRIG_NAME_MAX 50
>  
> +enum led_trigger_blink_supported_modes {
> +	SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> +	HARDWARE_ONLY = HARDWARE_CONTROLLED,
> +	SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,

I suspect all these generic names are asking for eventual namespace
clashes. Maybe prefix them with LED_ ?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v7 02/11] leds: add function to configure hardware controlled LED

[Russell King (Oracle)]

Hi Christian,

On Thu, Dec 15, 2022 at 12:54:29AM +0100, Christian Marangi wrote:
> +A trigger once he declared support for hardware controlled blinks, will use the function
> +hw_control_configure() provided by the driver to check support for a particular blink mode.

Please improve the above. I think what is actually meant is "Where a
trigger has support for hardware controlled blink modes,
hw_control_configure() will be used to check whether a particular blink
mode is supported and configure the blink mode."

> +This function passes as the first argument (flag) a u32 flag.

I don't think "(flag)" is necessary, as I think "a u32 flag"
sufficiently suggests that it's called "flag". In any case, it doesn't
appear to be a "u32" but an "unsigned long".

> +The second argument (cmd) of hw_control_configure() method can be used to do various
> +operations for the specific blink mode. We currently support ENABLE, DISABLE, READ, ZERO
> +and SUPPORTED to enable, disable, read the state of the blink mode, ask the LED
> +driver if it does supports the specific blink mode and to reset any blink mode active.
> +
> +In ENABLE/DISABLE hw_control_configure() should configure the LED to enable/disable the
> +requested blink mode (flag).
> +In READ hw_control_configure() should return 0 or 1 based on the status of the requested
> +blink mode (flag).
> +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
> +requested blink mode (flags) or not.
> +In ZERO hw_control_configure() should return 0 with success operation or error.

I think some kind of tabular description of this would be better.
Something like this but on docbook format:

hw_control_configure()'s second argument, cmd, is used to specify
various operations for the LED blink mode, and will be one of:

ENABLE - to enable the blink mode requested in flag. Returns ?
DISABLE - to disable the blink mode requested in flag. Returbns ?
READ - to indicate whether the blink mode requested in flag is enabled.
       Returns 0 if disabled or 1 if enabled.
SUPPORTED - to indicate whether the blink mode requested in flag is
            supported. Returns 0 if unsupported or 1 if supported.
ZERO - to disable all blink modes. Returns 0 or negative errno.

The problem with the way you've listed it is you've listed the
operations in a different order to the description in the same sentence,
so effectiely ZERO means to report whether supported and SUPPORTED means
to reset all blink modes!

> +
> +The unsigned long flag is specific to the trigger and change across them. It's in the LED
> +driver interest know how to elaborate this flag and to declare support for a
> +particular trigger.

Hmm, as a casual observer, this doesn't really give much information.
Does this mean that it's up to the hardware LED driver to decide what
each bit in the "flag" argument means? If not, I think this needs to
be worded better!

> For this exact reason explicit support for the specific
> +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
> +with a not supported trigger.

Seems to be a change in terminology - weren't we using "HARDWARE" and
"SOFTWARE" to describe the mode, rather than "offload" ?

> +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
> +fail as the driver doesn't support that specific offload trigger or doesn't know
> +how to handle the provided flags.

This gets rather ambiguous. When can -EOPNOTSUPP be returned - for which
cmds? Surely, if we have already tested for support using the SUPPORTED
cmd which returns a 0/1 value, we should not be going on to trigger a
request to enable something that isn't supported?

> +
>  Known Issues
>  ============
>  
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 09ff1dc6f48d..b5aad67fecfb 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -73,6 +73,16 @@ enum led_blink_modes {
>  	SOFTWARE_HARDWARE_CONTROLLED,
>  };
>  
> +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> +enum blink_mode_cmd {
> +	BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> +	BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> +	BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> +	BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> +	BLINK_MODE_ZERO, /* Disable any hardware blink active */
> +};
> +#endif

Generally not a good idea to make definitions in header files
conditional on configuration symbols - it makes build-testing more
problematical.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Russell King (Oracle)]

On Thu, Dec 15, 2022 at 12:54:33AM +0100, Christian Marangi wrote:
> Add hardware control support for the Netdev trigger.
> The trigger on config change will check if the requested trigger can set
> to blink mode using LED hardware mode and if every blink mode is supported,
> the trigger will enable hardware mode with the requested configuration.
> If there is at least one trigger that is not supported and can't run in
> hardware mode, then software mode will be used instead.
> A validation is done on every value change and on fail the old value is
> restored and -EINVAL is returned.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
>  1 file changed, 149 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index dd63cadb896e..ed019cb5867c 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -37,6 +37,7 @@
>   */
>  
>  struct led_netdev_data {
> +	enum led_blink_modes blink_mode;
>  	spinlock_t lock;
>  
>  	struct delayed_work work;
> @@ -53,11 +54,105 @@ struct led_netdev_data {
>  	bool carrier_link_up;
>  };
>  
> +struct netdev_led_attr_detail {
> +	char *name;
> +	bool hardware_only;
> +	enum led_trigger_netdev_modes bit;
> +};
> +
> +static struct netdev_led_attr_detail attr_details[] = {
> +	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
> +	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
> +	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
> +};
> +
> +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> +{
> +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> +	struct netdev_led_attr_detail *detail;
> +	u32 hw_blink_mode_supported = 0;
> +	bool force_sw = false;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> +		detail = &attr_details[i];
> +
> +		/* Mode not active, skip */
> +		if (!test_bit(detail->bit, &trigger_data->mode))
> +			continue;
> +
> +		/* Hardware only mode enabled on software controlled led */
> +		if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> +		    detail->hardware_only)
> +			return false;
> +
> +		/* Check if the mode supports hardware mode */
> +		if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> +			/* With a net dev set, force software mode.
> +			 * With modes are handled by hardware, led will blink
> +			 * based on his own events and will ignore any event
> +			 * from the provided dev.
> +			 */
> +			if (trigger_data->net_dev) {
> +				force_sw = true;
> +				continue;
> +			}
> +
> +			/* With empty dev, check if the mode is supported */
> +			if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> +				hw_blink_mode_supported |= BIT(detail->bit);
> +		}
> +	}
> +
> +	/* We can't run modes handled by both software and hardware.
> +	 * Check if we run hardware modes and check if all the modes
> +	 * can be handled by hardware.
> +	 */
> +	if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
> +		return false;
> +
> +	/* Modes are valid. Decide now the running mode to later
> +	 * set the baseline.
> +	 * Software mode is enforced with net_dev set. With an empty
> +	 * one hardware mode is selected by default (if supported).
> +	 */
> +	if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> +		trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> +	else
> +		trigger_data->blink_mode = HARDWARE_CONTROLLED;
> +
> +	return true;
> +}
> +
>  static void set_baseline_state(struct led_netdev_data *trigger_data)
>  {
> +	int i;
>  	int current_brightness;
> +	struct netdev_led_attr_detail *detail;
>  	struct led_classdev *led_cdev = trigger_data->led_cdev;
>  
> +	/* Modes already validated. Directly apply hw trigger modes */
> +	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> +		/* We are refreshing the blink modes. Reset them */
> +		led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
> +					       BLINK_MODE_ZERO);
> +
> +		for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> +			detail = &attr_details[i];
> +
> +			if (!test_bit(detail->bit, &trigger_data->mode))
> +				continue;
> +
> +			led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
> +						       BLINK_MODE_ENABLE);
> +		}
> +
> +		led_cdev->hw_control_start(led_cdev);
> +
> +		return;
> +	}
> +
> +	/* Handle trigger modes by software */
>  	current_brightness = led_cdev->brightness;
>  	if (current_brightness)
>  		led_cdev->blink_brightness = current_brightness;
> @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
>  				 size_t size)
>  {
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> +	struct net_device *old_net = trigger_data->net_dev;
> +	char old_device_name[IFNAMSIZ];
>  
>  	if (size >= IFNAMSIZ)
>  		return -EINVAL;
>  
> +	/* Backup old device name */
> +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> +
>  	cancel_delayed_work_sync(&trigger_data->work);
>  
>  	spin_lock_bh(&trigger_data->lock);
> @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
>  		trigger_data->net_dev =
>  		    dev_get_by_name(&init_net, trigger_data->device_name);
>  
> +	if (!validate_baseline_state(trigger_data)) {
> +		/* Restore old net_dev and device_name */
> +		if (trigger_data->net_dev)
> +			dev_put(trigger_data->net_dev);
> +
> +		dev_hold(old_net);
> +		trigger_data->net_dev = old_net;
> +		memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
> +
> +		spin_unlock_bh(&trigger_data->lock);
> +		return -EINVAL;

I'm not sure this is the best way... putting the net_dev but holding a
reference, to leter regain the reference via dev_hold() just feels
wrong. Also, I wonder what happens if two threads try to change the
netdev together - will the read of the old device name be potentially
corrupted (since we're not holding the trigger's lock?)

Maybe instead:

+	struct net_device *old_net;
...
-	if (trigger_data->net_dev) {
-		dev_put(trigger_data->net_dev);
-		trigger_data->net_dev = NULL;
-	}
+	old_net = trigger_data->net_dev;
+	trigger_data->net_dev = NULL;
+	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
...
	... extract out the setup of trigger_data->device_name
...
+	if (!validate_baseline_state(trigger_data)) {
+		if (trigger_data->net_dev)
+			dev_put(trigger_data->net_dev);
+
+		/* Restore device settings */
+		trigger_data->net_dev = old_dev;
+		memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
+		spin_unlock_bh(&trigger_data->lock);
+		return -EINVAL;
+	} else {
+		dev_put(old_net);
+	}

would be safer all round?

One thought on this approach though - if one has a PHY that supports
"activity" but not independent "rx" and "tx" activity indications
and it doesn't support software control, how would one enable activity
mode? There isn't a way to simultaneously enable both at the same
time... However, I need to check whether there are any PHYs that fall
into this category.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v7 09/11] leds: trigger: netdev: add additional hardware only triggers

[Russell King (Oracle)]

On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> Add additional hardware only triggers commonly supported by switch LEDs.
> 
> Additional modes:
> link_10: LED on with link up AND speed 10mbps
> link_100: LED on with link up AND speed 100mbps
> link_1000: LED on with link up AND speed 1000mbps
> half_duplex: LED on with link up AND half_duplex mode
> full_duplex: LED on with link up AND full duplex mode

Looking at Marvell 88e151x, I don't think this is usable there.
We have the option of supporting link_1000 on one of the LEDs,
link_100 on another, and link_10 on the other. It's rather rare
for all three leds to be wired though.

This is also a PHY where "activity" mode is supported (illuminated
or blinking if any traffic is transmitted or received) but may not
support individual directional traffic in hardware. However, it
does support forcing the LED on or off, so software mode can handle
those until the user selects a combination of modes that are
supported in the hardware.

> Additional blink interval modes:
> blink_2hz: LED blink on any even at 2Hz (250ms)
> blink_4hz: LED blink on any even at 4Hz (125ms)
> blink_8hz: LED blink on any even at 8Hz (62ms)

This seems too restrictive. For example, Marvell 88e151x supports
none of these, but does support 42, 84, 170, 340, 670ms.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v7 10/11] net: dsa: qca8k: add LEDs support

[Russell King (Oracle)]

Hi,

On Thu, Dec 15, 2022 at 12:54:37AM +0100, Christian Marangi wrote:
> +static int
> +qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
> +				enum blink_mode_cmd cmd)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +	struct led_trigger *trigger = ldev->trigger;
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 offload_trigger = 0, mask, val;
> +	int ret;
> +
> +	/* Check trigger compatibility */
> +	if (strcmp(trigger->name, "netdev"))
> +		return -EOPNOTSUPP;
> +
> +	if (!strcmp(trigger->name, "netdev"))
> +		ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);

I'm not sure how well the compiler will spot that, but as far as
readability goes, that second if() statement appears to be redundant.

> +
> +	if (ret)
> +		return ret;
> +
> +	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	switch (cmd) {
> +	case BLINK_MODE_SUPPORTED:
> +		/* We reach this point, we are sure the trigger is supported */
> +		return 1;
> +	case BLINK_MODE_ZERO:
> +		/* We set 4hz by default */
> +		u32 default_reg = QCA8K_LED_BLINK_4HZ;
> +
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 QCA8K_LED_RULE_MASK << reg_info.shift,
> +					 default_reg << reg_info.shift);
> +		break;
> +	case BLINK_MODE_ENABLE:
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 mask << reg_info.shift,
> +					 offload_trigger << reg_info.shift);
> +		break;
> +	case BLINK_MODE_DISABLE:
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 mask << reg_info.shift,
> +					 0);
> +		break;

I think it needs to be made more clear in the documentation that if
this is called with ENABLE, then _only_ those modes in flags... (or
is it now called "rules"?) are to be enabled and all other modes
should be disabled. Conversely, DISABLE is used to disable all
modes. However, if that's the case, then ZERO was misdescribed, and
should probably be called DEFAULT. At least that's the impression
that I get from the above code.

> +	case BLINK_MODE_READ:
> +		ret = regmap_read(priv->regmap, reg_info.reg, &val);
> +		if (ret)
> +			return ret;
> +
> +		val >>= reg_info.shift;
> +		val &= offload_trigger;
> +
> +		/* Special handling for LED_BLINK_2HZ */
> +		if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
> +			val = 1;

Hmm, so if a number of different modes is in flags or rules, then
as long as one matches, this returns 1? So it's an "any of these
modes is enabled" test.

> +
> +		return val;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ret;
> +}
> +
> +static void
> +qca8k_led_brightness_set(struct qca8k_led *led,
> +			 enum led_brightness b)
> +{
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	if (b)
> +		val = QCA8K_LED_ALWAYS_ON;
> +
> +	regmap_update_bits(priv->regmap, reg_info.reg,
> +			   GENMASK(1, 0) << reg_info.shift,
> +			   val << reg_info.shift);
> +}
> +
> +static void
> +qca8k_cled_brightness_set(struct led_classdev *ldev,
> +			  enum led_brightness b)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	return qca8k_led_brightness_set(led, b);
> +}
> +
> +static enum led_brightness
> +qca8k_led_brightness_get(struct qca8k_led *led)
> +{
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val;
> +	int ret;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	ret = regmap_read(priv->regmap, reg_info.reg, &val);
> +	if (ret)
> +		return 0;
> +
> +	val >>= reg_info.shift;
> +	val &= GENMASK(1, 0);
> +
> +	return val > 0 ? 1 : 0;
> +}
> +
> +static enum led_brightness
> +qca8k_cled_brightness_get(struct led_classdev *ldev)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	return qca8k_led_brightness_get(led);
> +}
> +
> +static int
> +qca8k_cled_blink_set(struct led_classdev *ldev,
> +		     unsigned long *delay_on,
> +		     unsigned long *delay_off)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		*delay_on = 125;
> +		*delay_off = 125;
> +	}
> +
> +	if (*delay_on != 125 || *delay_off != 125) {
> +		/* The hardware only supports blinking at 4Hz. Fall back
> +		 * to software implementation in other cases.
> +		 */
> +		return -EINVAL;
> +	}
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	regmap_update_bits(priv->regmap, reg_info.reg,
> +			   GENMASK(1, 0) << reg_info.shift,
> +			   QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	if (enable)
> +		val = QCA8K_LED_RULE_CONTROLLED;
> +
> +	return regmap_update_bits(priv->regmap, reg_info.reg,
> +				  GENMASK(1, 0) << reg_info.shift,
> +				  val << reg_info.shift);

88e151x doesn't have the ability to change in this way - we have
a register with a 4-bit field which selects the LED mode from one
of many, or forces the LED on/off/hi-z/blink.

Not specifically for this patch, but talking generally about this
approach, the other issue I forsee with this is that yes, 88e151x has
three LEDs, but the LED modes are also used to implement control
signals (e.g., on a SFP, LOS can be implemented by programming mode
0 on LED2 (which makes it indicate link or not.) If we expose all the
LEDs we run the risk of the LED subsystem trampling over that
configuration and essentially messing up such modules. So the Marvell
PHY driver would need to know when it is appropriate to expose these
things to the LED subsystem.

I guess doing it dependent on firmware description as you do in
this driver would work - if there's no firmware description, they're
not exposed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v7 01/11] leds: add support for hardware driven LEDs

[Christian Marangi]

On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> Hi Christian,
> 
> Thanks for the patch.
> 
> I think Andrew's email is offline at the moment.
>

Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
Holidy times I guess?

> On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> > +static bool led_trigger_is_supported(struct led_classdev *led_cdev,
> > +				     struct led_trigger *trigger)
> > +{
> > +	switch (led_cdev->blink_mode) {
> > +	case SOFTWARE_CONTROLLED:
> > +		if (trigger->supported_blink_modes == HARDWARE_ONLY)
> > +			return 0;
> > +		break;
> > +	case HARDWARE_CONTROLLED:
> > +		if (trigger->supported_blink_modes == SOFTWARE_ONLY)
> > +			return 0;
> > +		break;
> > +	case SOFTWARE_HARDWARE_CONTROLLED:
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +	return 1;
> 
> Should be returning true/false. I'm not sure I'm a fan of the style of
> this though - wouldn't the following be easier to read?
> 
> 	switch (led_cdev->blink_mode) {
> 	case SOFTWARE_CONTROLLED:
> 		return trigger->supported_blink_modes != HARDWARE_ONLY;
> 
> 	case HARDWARE_CONTROLLED:
> 		return trigger->supported_blink_modes != SOFTWARE_ONLY;
> 
> 	case SOFTWARE_HARDWARE_CONTROLLED:
> 		return true;
> 	}
> ?

Much better!

> 
> Also, does it really need a default case - without it, when the
> led_blink_modes enum is expanded and the switch statement isn't
> updated, we'll get a compiler warning which will prompt this to be
> updated - whereas, with a default case, it won't.
> 

I added the default just to mute some compiler warning. But guess if
every enum is handled the warning should not be reported.

> > @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> >  		led_set_brightness(led_cdev, LED_OFF);
> >  	}
> >  	if (trig) {
> > +		/* Make sure the trigger support the LED blink mode */
> > +		if (!led_trigger_is_supported(led_cdev, trig))
> > +			return -EINVAL;
> 
> Shouldn't validation happen before we start taking any actions? In other
> words, before we remove the previous trigger?
> 

trigger_set first remove any trigger and set the led off. Then apply the
new trigger. So the validation is done only when a trigger is actually
applied. Think we should understand the best case here.

> > @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
> >  
> >  #define TRIG_NAME_MAX 50
> >  
> > +enum led_trigger_blink_supported_modes {
> > +	SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> > +	HARDWARE_ONLY = HARDWARE_CONTROLLED,
> > +	SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
> 
> I suspect all these generic names are asking for eventual namespace
> clashes. Maybe prefix them with LED_ ?

Agree they are pretty generic so I can see why... My only concern was
making them too long... Maybe reduce them to SW or HW? LEDS_SW_ONLY...
LEDS_SW_CONTROLLED?

> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

-- 
	Ansuel

Re: [PATCH v7 01/11] leds: add support for hardware driven LEDs

[Russell King (Oracle)]

On Fri, Dec 16, 2022 at 05:45:25PM +0100, Christian Marangi wrote:
> On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> > Hi Christian,
> > 
> > Thanks for the patch.
> > 
> > I think Andrew's email is offline at the moment.
> >
> 
> Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
> Holidy times I guess?

Sadly, Andrew's email has done this a number of times - and Andrew
used to be on IRC so I could prod him about it, but it seems he
doesn't hang out on IRC anymore. It's been like it a few days now.

> > On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> > > @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> > >  		led_set_brightness(led_cdev, LED_OFF);
> > >  	}
> > >  	if (trig) {
> > > +		/* Make sure the trigger support the LED blink mode */
> > > +		if (!led_trigger_is_supported(led_cdev, trig))
> > > +			return -EINVAL;
> > 
> > Shouldn't validation happen before we start taking any actions? In other
> > words, before we remove the previous trigger?
> > 
> 
> trigger_set first remove any trigger and set the led off. Then apply the
> new trigger. So the validation is done only when a trigger is actually
> applied. Think we should understand the best case here.

I think this is a question that needs to be answered by the LEDs folk,
as it's an interface behaviour / quality of implementation issue.

> > > @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
> > >  
> > >  #define TRIG_NAME_MAX 50
> > >  
> > > +enum led_trigger_blink_supported_modes {
> > > +	SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> > > +	HARDWARE_ONLY = HARDWARE_CONTROLLED,
> > > +	SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
> > 
> > I suspect all these generic names are asking for eventual namespace
> > clashes. Maybe prefix them with LED_ ?
> 
> Agree they are pretty generic so I can see why... My only concern was
> making them too long... Maybe reduce them to SW or HW? LEDS_SW_ONLY...
> LEDS_SW_CONTROLLED?

Seems sensible to me - and as a bonus they get shorter than the above!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v7 02/11] leds: add function to configure hardware controlled LED

[Christian Marangi]

On Thu, Dec 15, 2022 at 04:30:49PM +0000, Russell King (Oracle) wrote:
> Hi Christian,
> 
> On Thu, Dec 15, 2022 at 12:54:29AM +0100, Christian Marangi wrote:
> > +A trigger once he declared support for hardware controlled blinks, will use the function
> > +hw_control_configure() provided by the driver to check support for a particular blink mode.
> 
> Please improve the above. I think what is actually meant is "Where a
> trigger has support for hardware controlled blink modes,
> hw_control_configure() will be used to check whether a particular blink
> mode is supported and configure the blink mode."
>

Ok I will improve!

> > +This function passes as the first argument (flag) a u32 flag.
> 
> I don't think "(flag)" is necessary, as I think "a u32 flag"
> sufficiently suggests that it's called "flag". In any case, it doesn't
> appear to be a "u32" but an "unsigned long".
> 

Will drop flag and fix the type.

> > +The second argument (cmd) of hw_control_configure() method can be used to do various
> > +operations for the specific blink mode. We currently support ENABLE, DISABLE, READ, ZERO
> > +and SUPPORTED to enable, disable, read the state of the blink mode, ask the LED
> > +driver if it does supports the specific blink mode and to reset any blink mode active.
> > +
> > +In ENABLE/DISABLE hw_control_configure() should configure the LED to enable/disable the
> > +requested blink mode (flag).
> > +In READ hw_control_configure() should return 0 or 1 based on the status of the requested
> > +blink mode (flag).
> > +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
> > +requested blink mode (flags) or not.
> > +In ZERO hw_control_configure() should return 0 with success operation or error.
> 
> I think some kind of tabular description of this would be better.
> Something like this but on docbook format:
> 
> hw_control_configure()'s second argument, cmd, is used to specify
> various operations for the LED blink mode, and will be one of:
> 
> ENABLE - to enable the blink mode requested in flag. Returns ?
> DISABLE - to disable the blink mode requested in flag. Returbns ?
> READ - to indicate whether the blink mode requested in flag is enabled.
>        Returns 0 if disabled or 1 if enabled.
> SUPPORTED - to indicate whether the blink mode requested in flag is
>             supported. Returns 0 if unsupported or 1 if supported.
> ZERO - to disable all blink modes. Returns 0 or negative errno.
> 
> The problem with the way you've listed it is you've listed the
> operations in a different order to the description in the same sentence,
> so effectiely ZERO means to report whether supported and SUPPORTED means
> to reset all blink modes!
> 
> > +
> > +The unsigned long flag is specific to the trigger and change across them. It's in the LED
> > +driver interest know how to elaborate this flag and to declare support for a
> > +particular trigger.
> 
> Hmm, as a casual observer, this doesn't really give much information.
> Does this mean that it's up to the hardware LED driver to decide what
> each bit in the "flag" argument means? If not, I think this needs to
> be worded better!
> 

The idea behind this is that the leds driver needs to have some code to
parse the flag provided by the supported trigger...

Example:
- netdev trigger provide flag with BLINK_TX BLINK_RX
- driver needs to have explicit support for netdev trigger and will
  parse the passed flag to enable the function internally.

So no the driver needs to just reflect what it was requested by the
trigger. It's the trigger the one that define flags structure.

Ideally the driver will just include the trigger header and refer to the
same bits the trigger declare in the flag.

> > For this exact reason explicit support for the specific
> > +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
> > +with a not supported trigger.
> 
> Seems to be a change in terminology - weren't we using "HARDWARE" and
> "SOFTWARE" to describe the mode, rather than "offload" ?
> 

Will fix!

> > +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
> > +fail as the driver doesn't support that specific offload trigger or doesn't know
> > +how to handle the provided flags.
> 
> This gets rather ambiguous. When can -EOPNOTSUPP be returned - for which
> cmds? Surely, if we have already tested for support using the SUPPORTED
> cmd which returns a 0/1 value, we should not be going on to trigger a
> request to enable something that isn't supported?
> 

Currently we report -EOPNOTSUPP when a trigger is not
supported. hw_control_start/stop/status is just related to activate the
hw_control but not configuring it. In old implementation it was
suggested to have this kind of split and separation to not overload the
configure function and have them split.

(configure enable/disable are about specific trigger rules not the hw
control mode)

I will try to refactor this to be more explicit.

> > +
> >  Known Issues
> >  ============
> >  
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 09ff1dc6f48d..b5aad67fecfb 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -73,6 +73,16 @@ enum led_blink_modes {
> >  	SOFTWARE_HARDWARE_CONTROLLED,
> >  };
> >  
> > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > +enum blink_mode_cmd {
> > +	BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> > +	BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> > +	BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> > +	BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> > +	BLINK_MODE_ZERO, /* Disable any hardware blink active */
> > +};
> > +#endif
> 
> Generally not a good idea to make definitions in header files
> conditional on configuration symbols - it makes build-testing more
> problematical.

Guess I will have to add namespace tag and drop the ifdef.

> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

-- 
	Ansuel

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Christian Marangi]

On Thu, Dec 15, 2022 at 04:27:17PM +0100, Alexander Stein wrote:
> Hi,
> 
> thanks for the v7 series.
> 
> Am Donnerstag, 15. Dezember 2022, 00:54:33 CET schrieb Christian Marangi:
> > Add hardware control support for the Netdev trigger.
> > The trigger on config change will check if the requested trigger can set
> > to blink mode using LED hardware mode and if every blink mode is supported,
> > the trigger will enable hardware mode with the requested configuration.
> > If there is at least one trigger that is not supported and can't run in
> > hardware mode, then software mode will be used instead.
> > A validation is done on every value change and on fail the old value is
> > restored and -EINVAL is returned.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> >  1 file changed, 149 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c
> > b/drivers/leds/trigger/ledtrig-netdev.c index dd63cadb896e..ed019cb5867c
> > 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -37,6 +37,7 @@
> >   */
> > 
> >  struct led_netdev_data {
> > +	enum led_blink_modes blink_mode;
> >  	spinlock_t lock;
> > 
> >  	struct delayed_work work;
> > @@ -53,11 +54,105 @@ struct led_netdev_data {
> >  	bool carrier_link_up;
> >  };
> > 
> > +struct netdev_led_attr_detail {
> > +	char *name;
> > +	bool hardware_only;
> > +	enum led_trigger_netdev_modes bit;
> > +};
> > +
> > +static struct netdev_led_attr_detail attr_details[] = {
> > +	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
> > +	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
> > +	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
> > +};
> > +
> > +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> > +{
> > +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > +	struct netdev_led_attr_detail *detail;
> > +	u32 hw_blink_mode_supported = 0;
> > +	bool force_sw = false;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > +		detail = &attr_details[i];
> > +
> > +		/* Mode not active, skip */
> > +		if (!test_bit(detail->bit, &trigger_data->mode))
> > +			continue;
> > +
> > +		/* Hardware only mode enabled on software controlled led 
> */
> > +		if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> > +		    detail->hardware_only)
> > +			return false;
> > +
> > +		/* Check if the mode supports hardware mode */
> > +		if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > +			/* With a net dev set, force software mode.
> > +			 * With modes are handled by hardware, led will 
> blink
> > +			 * based on his own events and will ignore any 
> event
> > +			 * from the provided dev.
> > +			 */
> > +			if (trigger_data->net_dev) {
> > +				force_sw = true;
> > +				continue;
> > +			}
> > +
> > +			/* With empty dev, check if the mode is 
> supported */
> > +			if 
> (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> > +				hw_blink_mode_supported |= BIT(detail-
> >bit);
> 
> Shouldn't this be BIT(detail->bit)?
>

I think I didn't understand?

> > +		}
> > +	}
> > +
> > +	/* We can't run modes handled by both software and hardware.
> > +	 * Check if we run hardware modes and check if all the modes
> > +	 * can be handled by hardware.
> > +	 */
> > +	if (hw_blink_mode_supported && hw_blink_mode_supported !=
> > trigger_data->mode) +		return false;
> > +
> > +	/* Modes are valid. Decide now the running mode to later
> > +	 * set the baseline.
> > +	 * Software mode is enforced with net_dev set. With an empty
> > +	 * one hardware mode is selected by default (if supported).
> > +	 */
> > +	if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> 
> IMHO '|| !hw_blink_mode_supported' should be added here for blink_modes. This 
> might happen if a PHY LED is SOFTWARE_HARDWARE_CONTROLLED, but some blink mode 
> is not supported by hardware, thus hw_blink_mode_supported=0.
> 

Will check this and report back.

> Best regards,
> Alexander
> 
> > +		trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> > +	else
> > +		trigger_data->blink_mode = HARDWARE_CONTROLLED;
> > +
> > +	return true;
> > +}
> > +
> >  static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  {
> > +	int i;
> >  	int current_brightness;
> > +	struct netdev_led_attr_detail *detail;
> >  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > 
> > +	/* Modes already validated. Directly apply hw trigger modes */
> > +	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> > +		/* We are refreshing the blink modes. Reset them */
> > +		led_cdev->hw_control_configure(led_cdev, 
> BIT(TRIGGER_NETDEV_LINK),
> > +					       BLINK_MODE_ZERO);
> > +
> > +		for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > +			detail = &attr_details[i];
> > +
> > +			if (!test_bit(detail->bit, &trigger_data->mode))
> > +				continue;
> > +
> > +			led_cdev->hw_control_configure(led_cdev, 
> BIT(detail->bit),
> > +						       
> BLINK_MODE_ENABLE);
> 
> Shouldn't this be BIT(detail->bit)?
> 
> > +		}
> > +
> > +		led_cdev->hw_control_start(led_cdev);
> > +
> > +		return;
> > +	}
> > +
> > +	/* Handle trigger modes by software */
> >  	current_brightness = led_cdev->brightness;
> >  	if (current_brightness)
> >  		led_cdev->blink_brightness = current_brightness;
> > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> >  				 size_t size)
> >  {
> >  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > +	struct net_device *old_net = trigger_data->net_dev;
> > +	char old_device_name[IFNAMSIZ];
> > 
> >  	if (size >= IFNAMSIZ)
> >  		return -EINVAL;
> > 
> > +	/* Backup old device name */
> > +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> > +
> >  	cancel_delayed_work_sync(&trigger_data->work);
> > 
> >  	spin_lock_bh(&trigger_data->lock);
> > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> >  		trigger_data->net_dev =
> >  		    dev_get_by_name(&init_net, trigger_data->device_name);
> > 
> > +	if (!validate_baseline_state(trigger_data)) {
> > +		/* Restore old net_dev and device_name */
> > +		if (trigger_data->net_dev)
> > +			dev_put(trigger_data->net_dev);
> > +
> > +		dev_hold(old_net);
> > +		trigger_data->net_dev = old_net;
> > +		memcpy(trigger_data->device_name, old_device_name, 
> IFNAMSIZ);
> > +
> > +		spin_unlock_bh(&trigger_data->lock);
> > +		return -EINVAL;
> > +	}
> > +
> >  	trigger_data->carrier_link_up = false;
> >  	if (trigger_data->net_dev != NULL)
> >  		trigger_data->carrier_link_up = 
> netif_carrier_ok(trigger_data->net_dev);
> > @@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device *dev,
> > const char *buf, size_t size, enum led_trigger_netdev_modes attr)
> >  {
> >  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > -	unsigned long state;
> > +	unsigned long state, old_mode = trigger_data->mode;
> >  	int ret;
> >  	int bit;
> > 
> > @@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device
> > *dev, const char *buf, else
> >  		clear_bit(bit, &trigger_data->mode);
> > 
> > +	if (!validate_baseline_state(trigger_data)) {
> > +		/* Restore old mode on validation fail */
> > +		trigger_data->mode = old_mode;
> > +		return -EINVAL;
> > +	}
> > +
> >  	set_baseline_state(trigger_data);
> > 
> >  	return size;
> > @@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
> >  			      size_t size)
> >  {
> >  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > +	int old_interval = atomic_read(&trigger_data->interval);
> > +	u32 old_mode = trigger_data->mode;
> >  	unsigned long value;
> >  	int ret;
> > 
> > @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
> >  		return ret;
> > 
> >  	/* impose some basic bounds on the timer interval */
> > -	if (value >= 5 && value <= 10000) {
> > -		cancel_delayed_work_sync(&trigger_data->work);
> > +	if (value < 5 || value > 10000)
> > +		return -EINVAL;
> > +
> > +	cancel_delayed_work_sync(&trigger_data->work);
> > +
> > +	atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> > 
> > -		atomic_set(&trigger_data->interval, 
> msecs_to_jiffies(value));
> > -		set_baseline_state(trigger_data);	/* resets timer 
> */
> > +	if (!validate_baseline_state(trigger_data)) {
> > +		/* Restore old interval on validation error */
> > +		atomic_set(&trigger_data->interval, old_interval);
> > +		trigger_data->mode = old_mode;
> > +		return -EINVAL;
> >  	}
> > 
> > +	set_baseline_state(trigger_data);	/* resets timer */
> > +
> >  	return size;
> >  }
> > 
> > @@ -368,13 +498,25 @@ static int netdev_trig_activate(struct led_classdev
> > *led_cdev) trigger_data->mode = 0;
> >  	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
> >  	trigger_data->last_activity = 0;
> > +	if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > +		/* With hw mode enabled reset any rule set by default */
> > +		if (led_cdev->hw_control_status(led_cdev)) {
> > +			rc = led_cdev->hw_control_configure(led_cdev, 
> BIT(TRIGGER_NETDEV_LINK),
> > +							    
> BLINK_MODE_ZERO);
> > +			if (rc)
> > +				goto err;
> > +		}
> > +	}
> > 
> >  	led_set_trigger_data(led_cdev, trigger_data);
> > 
> >  	rc = register_netdevice_notifier(&trigger_data->notifier);
> >  	if (rc)
> > -		kfree(trigger_data);
> > +		goto err;
> > 
> > +	return 0;
> > +err:
> > +	kfree(trigger_data);
> >  	return rc;
> >  }
> > 
> > @@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct led_classdev
> > *led_cdev)
> > 
> >  static struct led_trigger netdev_led_trigger = {
> >  	.name = "netdev",
> > +	.supported_blink_modes = SOFTWARE_HARDWARE,
> >  	.activate = netdev_trig_activate,
> >  	.deactivate = netdev_trig_deactivate,
> >  	.groups = netdev_trig_groups,
> 
> 
> 
> 

-- 
	Ansuel

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Christian Marangi]

On Thu, Dec 15, 2022 at 05:07:31PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 15, 2022 at 12:54:33AM +0100, Christian Marangi wrote:
> > Add hardware control support for the Netdev trigger.
> > The trigger on config change will check if the requested trigger can set
> > to blink mode using LED hardware mode and if every blink mode is supported,
> > the trigger will enable hardware mode with the requested configuration.
> > If there is at least one trigger that is not supported and can't run in
> > hardware mode, then software mode will be used instead.
> > A validation is done on every value change and on fail the old value is
> > restored and -EINVAL is returned.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> >  1 file changed, 149 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index dd63cadb896e..ed019cb5867c 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -37,6 +37,7 @@
> >   */
> >  
> >  struct led_netdev_data {
> > +	enum led_blink_modes blink_mode;
> >  	spinlock_t lock;
> >  
> >  	struct delayed_work work;
> > @@ -53,11 +54,105 @@ struct led_netdev_data {
> >  	bool carrier_link_up;
> >  };
> >  
> > +struct netdev_led_attr_detail {
> > +	char *name;
> > +	bool hardware_only;
> > +	enum led_trigger_netdev_modes bit;
> > +};
> > +
> > +static struct netdev_led_attr_detail attr_details[] = {
> > +	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
> > +	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
> > +	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
> > +};
> > +
> > +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> > +{
> > +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > +	struct netdev_led_attr_detail *detail;
> > +	u32 hw_blink_mode_supported = 0;
> > +	bool force_sw = false;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > +		detail = &attr_details[i];
> > +
> > +		/* Mode not active, skip */
> > +		if (!test_bit(detail->bit, &trigger_data->mode))
> > +			continue;
> > +
> > +		/* Hardware only mode enabled on software controlled led */
> > +		if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> > +		    detail->hardware_only)
> > +			return false;
> > +
> > +		/* Check if the mode supports hardware mode */
> > +		if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > +			/* With a net dev set, force software mode.
> > +			 * With modes are handled by hardware, led will blink
> > +			 * based on his own events and will ignore any event
> > +			 * from the provided dev.
> > +			 */
> > +			if (trigger_data->net_dev) {
> > +				force_sw = true;
> > +				continue;
> > +			}
> > +
> > +			/* With empty dev, check if the mode is supported */
> > +			if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> > +				hw_blink_mode_supported |= BIT(detail->bit);
> > +		}
> > +	}
> > +
> > +	/* We can't run modes handled by both software and hardware.
> > +	 * Check if we run hardware modes and check if all the modes
> > +	 * can be handled by hardware.
> > +	 */
> > +	if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
> > +		return false;
> > +
> > +	/* Modes are valid. Decide now the running mode to later
> > +	 * set the baseline.
> > +	 * Software mode is enforced with net_dev set. With an empty
> > +	 * one hardware mode is selected by default (if supported).
> > +	 */
> > +	if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> > +		trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> > +	else
> > +		trigger_data->blink_mode = HARDWARE_CONTROLLED;
> > +
> > +	return true;
> > +}
> > +
> >  static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  {
> > +	int i;
> >  	int current_brightness;
> > +	struct netdev_led_attr_detail *detail;
> >  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> >  
> > +	/* Modes already validated. Directly apply hw trigger modes */
> > +	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> > +		/* We are refreshing the blink modes. Reset them */
> > +		led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
> > +					       BLINK_MODE_ZERO);
> > +
> > +		for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > +			detail = &attr_details[i];
> > +
> > +			if (!test_bit(detail->bit, &trigger_data->mode))
> > +				continue;
> > +
> > +			led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
> > +						       BLINK_MODE_ENABLE);
> > +		}
> > +
> > +		led_cdev->hw_control_start(led_cdev);
> > +
> > +		return;
> > +	}
> > +
> > +	/* Handle trigger modes by software */
> >  	current_brightness = led_cdev->brightness;
> >  	if (current_brightness)
> >  		led_cdev->blink_brightness = current_brightness;
> > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> >  				 size_t size)
> >  {
> >  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > +	struct net_device *old_net = trigger_data->net_dev;
> > +	char old_device_name[IFNAMSIZ];
> >  
> >  	if (size >= IFNAMSIZ)
> >  		return -EINVAL;
> >  
> > +	/* Backup old device name */
> > +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> > +
> >  	cancel_delayed_work_sync(&trigger_data->work);
> >  
> >  	spin_lock_bh(&trigger_data->lock);
> > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> >  		trigger_data->net_dev =
> >  		    dev_get_by_name(&init_net, trigger_data->device_name);
> >  
> > +	if (!validate_baseline_state(trigger_data)) {
> > +		/* Restore old net_dev and device_name */
> > +		if (trigger_data->net_dev)
> > +			dev_put(trigger_data->net_dev);
> > +
> > +		dev_hold(old_net);
> > +		trigger_data->net_dev = old_net;
> > +		memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
> > +
> > +		spin_unlock_bh(&trigger_data->lock);
> > +		return -EINVAL;
> 
> I'm not sure this is the best way... putting the net_dev but holding a
> reference, to leter regain the reference via dev_hold() just feels
> wrong. Also, I wonder what happens if two threads try to change the
> netdev together - will the read of the old device name be potentially
> corrupted (since we're not holding the trigger's lock?)
> 
> Maybe instead:
> 
> +	struct net_device *old_net;
> ...
> -	if (trigger_data->net_dev) {
> -		dev_put(trigger_data->net_dev);
> -		trigger_data->net_dev = NULL;
> -	}
> +	old_net = trigger_data->net_dev;
> +	trigger_data->net_dev = NULL;
> +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> ...
> 	... extract out the setup of trigger_data->device_name
> ...
> +	if (!validate_baseline_state(trigger_data)) {
> +		if (trigger_data->net_dev)
> +			dev_put(trigger_data->net_dev);
> +
> +		/* Restore device settings */
> +		trigger_data->net_dev = old_dev;
> +		memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
> +		spin_unlock_bh(&trigger_data->lock);
> +		return -EINVAL;
> +	} else {
> +		dev_put(old_net);
> +	}
> 
> would be safer all round?

Need to check but if I'm not wrong all this thing was to handle the very
corner case where net can be removed while we are changing trigger and
something goes wrong down the line... Holding that means it won't get
actually removed till everything is ok.

> 
> One thought on this approach though - if one has a PHY that supports
> "activity" but not independent "rx" and "tx" activity indications
> and it doesn't support software control, how would one enable activity
> mode? There isn't a way to simultaneously enable both at the same
> time... However, I need to check whether there are any PHYs that fall
> into this category.
>

Problem is that for such feature and to have at least something working
we need to face compromise. We really can't support each switch feature
and have a generic API for everything. My original idea was to have
something VERY dynamic with a totally dedicated and dumb trigger... But
that was NACK as netdev was the correct way to handle these stuff...

But adapting everything to netdev trigger is hard since you have just
another generic abstraction layer. My idea at times was that in such
case the trigger rule will be rejected and only enabled if both tx and
rx were enabled. An alternative is to add another flag for activity
rule. (for switch supporting independent tx and rx with activity rule
enable both tx and rx event are enabled. for switch not supporting
independent tx and rx just fallback to sw and say that the mode is not
suported.)

I already had the idea of Documenting all this case but if we decide to
follow this approach then creating a schema file is a must at this
point. (but wanted to introduce that later if and ever this feature will
be accepted to permit to set trigger rules directly in DT following
something like linux,default-trigger.

-- 
	Ansuel

Re: [PATCH v7 09/11] leds: trigger: netdev: add additional hardware only triggers

[Christian Marangi]

On Thu, Dec 15, 2022 at 05:35:47PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> > Add additional hardware only triggers commonly supported by switch LEDs.
> > 
> > Additional modes:
> > link_10: LED on with link up AND speed 10mbps
> > link_100: LED on with link up AND speed 100mbps
> > link_1000: LED on with link up AND speed 1000mbps
> > half_duplex: LED on with link up AND half_duplex mode
> > full_duplex: LED on with link up AND full duplex mode
> 
> Looking at Marvell 88e151x, I don't think this is usable there.
> We have the option of supporting link_1000 on one of the LEDs,
> link_100 on another, and link_10 on the other. It's rather rare
> for all three leds to be wired though.

On qca8k it's just anarchy. You can apply the same rule table to
whatever led you want. They are all the same... OEM decide what to do
(add white led, amber, green...)

Most common configuration is 2 leds, one react with port 1000 and the
other with port 100. But each led can be set to be powered on to any
speed by enabling 10 100 and 1000 rule.

Rejecting a configuration falls in the driver returning error on
configure.

> 
> This is also a PHY where "activity" mode is supported (illuminated
> or blinking if any traffic is transmitted or received) but may not
> support individual directional traffic in hardware. However, it
> does support forcing the LED on or off, so software mode can handle
> those until the user selects a combination of modes that are
> supported in the hardware.
> 
> > Additional blink interval modes:
> > blink_2hz: LED blink on any even at 2Hz (250ms)
> > blink_4hz: LED blink on any even at 4Hz (125ms)
> > blink_8hz: LED blink on any even at 8Hz (62ms)
> 
> This seems too restrictive. For example, Marvell 88e151x supports
> none of these, but does support 42, 84, 170, 340, 670ms.
> 

Eh this is really bad, it was an idea to support blink internal for each
event but I expected other switch had something different... But the
generic function is still there...

Wonder if we should consider adding generic naming like SLOW, NORMAL(?),
FAST. And just assign them from the driver side?

Permit to have some kind of configuration while also keeping it generic
enough? 

Just as reference... The blink mode already had a compromise since qca8k
can support a way to blink differently based on the link speed.

-- 
	Ansuel

Re: [PATCH v7 10/11] net: dsa: qca8k: add LEDs support

[Christian Marangi]

On Thu, Dec 15, 2022 at 05:49:57PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> On Thu, Dec 15, 2022 at 12:54:37AM +0100, Christian Marangi wrote:
> > +static int
> > +qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
> > +				enum blink_mode_cmd cmd)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +	struct led_trigger *trigger = ldev->trigger;
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 offload_trigger = 0, mask, val;
> > +	int ret;
> > +
> > +	/* Check trigger compatibility */
> > +	if (strcmp(trigger->name, "netdev"))
> > +		return -EOPNOTSUPP;
> > +
> > +	if (!strcmp(trigger->name, "netdev"))
> > +		ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
> 
> I'm not sure how well the compiler will spot that, but as far as
> readability goes, that second if() statement appears to be redundant.
>

Leftover when the driver supported 2 trigger. The idea was to provide a
"template" to show the flow of the function.

> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	switch (cmd) {
> > +	case BLINK_MODE_SUPPORTED:
> > +		/* We reach this point, we are sure the trigger is supported */
> > +		return 1;
> > +	case BLINK_MODE_ZERO:
> > +		/* We set 4hz by default */
> > +		u32 default_reg = QCA8K_LED_BLINK_4HZ;
> > +
> > +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > +					 QCA8K_LED_RULE_MASK << reg_info.shift,
> > +					 default_reg << reg_info.shift);
> > +		break;
> > +	case BLINK_MODE_ENABLE:
> > +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > +					 mask << reg_info.shift,
> > +					 offload_trigger << reg_info.shift);
> > +		break;
> > +	case BLINK_MODE_DISABLE:
> > +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > +					 mask << reg_info.shift,
> > +					 0);
> > +		break;
> 
> I think it needs to be made more clear in the documentation that if
> this is called with ENABLE, then _only_ those modes in flags... (or
> is it now called "rules"?) are to be enabled and all other modes
> should be disabled. Conversely, DISABLE is used to disable all
> modes. However, if that's the case, then ZERO was misdescribed, and
> should probably be called DEFAULT. At least that's the impression
> that I get from the above code.
> 

ZERO disable all the rules. Driver can keep some rule enabled (this is
the case for qca8k blink mode at 4hz by default)

ENABLE/DISABLE only act on the provided thing in rules. In the current
imlementation parse rules, generate a mask and update the values
accordingly. Other rules are not touched. This is based on the fact that
the first and the last thing done is calling ZERO to reset all the rules
to a known state.

I will try to improve the Documentation on this aspect.
Hope you know understand better the calling flow.

> > +	case BLINK_MODE_READ:
> > +		ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		val >>= reg_info.shift;
> > +		val &= offload_trigger;
> > +
> > +		/* Special handling for LED_BLINK_2HZ */
> > +		if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
> > +			val = 1;
> 
> Hmm, so if a number of different modes is in flags or rules, then
> as long as one matches, this returns 1? So it's an "any of these
> modes is enabled" test.
> 

Ok this should be changed and you are right. READ should return true or
false if the rule (or rules) are enabled. This work for a single rule
but doesn't if multiple rules are provided.

Will fix that since this is wrong.

> > +
> > +		return val;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void
> > +qca8k_led_brightness_set(struct qca8k_led *led,
> > +			 enum led_brightness b)
> > +{
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	if (b)
> > +		val = QCA8K_LED_ALWAYS_ON;
> > +
> > +	regmap_update_bits(priv->regmap, reg_info.reg,
> > +			   GENMASK(1, 0) << reg_info.shift,
> > +			   val << reg_info.shift);
> > +}
> > +
> > +static void
> > +qca8k_cled_brightness_set(struct led_classdev *ldev,
> > +			  enum led_brightness b)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > +	return qca8k_led_brightness_set(led, b);
> > +}
> > +
> > +static enum led_brightness
> > +qca8k_led_brightness_get(struct qca8k_led *led)
> > +{
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 val;
> > +	int ret;
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > +	if (ret)
> > +		return 0;
> > +
> > +	val >>= reg_info.shift;
> > +	val &= GENMASK(1, 0);
> > +
> > +	return val > 0 ? 1 : 0;
> > +}
> > +
> > +static enum led_brightness
> > +qca8k_cled_brightness_get(struct led_classdev *ldev)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > +	return qca8k_led_brightness_get(led);
> > +}
> > +
> > +static int
> > +qca8k_cled_blink_set(struct led_classdev *ldev,
> > +		     unsigned long *delay_on,
> > +		     unsigned long *delay_off)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +
> > +	if (*delay_on == 0 && *delay_off == 0) {
> > +		*delay_on = 125;
> > +		*delay_off = 125;
> > +	}
> > +
> > +	if (*delay_on != 125 || *delay_off != 125) {
> > +		/* The hardware only supports blinking at 4Hz. Fall back
> > +		 * to software implementation in other cases.
> > +		 */
> > +		return -EINVAL;
> > +	}
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	regmap_update_bits(priv->regmap, reg_info.reg,
> > +			   GENMASK(1, 0) << reg_info.shift,
> > +			   QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	if (enable)
> > +		val = QCA8K_LED_RULE_CONTROLLED;
> > +
> > +	return regmap_update_bits(priv->regmap, reg_info.reg,
> > +				  GENMASK(1, 0) << reg_info.shift,
> > +				  val << reg_info.shift);
> 
> 88e151x doesn't have the ability to change in this way - we have
> a register with a 4-bit field which selects the LED mode from one
> of many, or forces the LED on/off/hi-z/blink.
> 
> Not specifically for this patch, but talking generally about this
> approach, the other issue I forsee with this is that yes, 88e151x has
> three LEDs, but the LED modes are also used to implement control
> signals (e.g., on a SFP, LOS can be implemented by programming mode
> 0 on LED2 (which makes it indicate link or not.) If we expose all the
> LEDs we run the risk of the LED subsystem trampling over that
> configuration and essentially messing up such modules. So the Marvell
> PHY driver would need to know when it is appropriate to expose these
> things to the LED subsystem.
> 
> I guess doing it dependent on firmware description as you do in
> this driver would work - if there's no firmware description, they're
> not exposed.
> 

The idea is to provide a way to tell the driver what should be done and
tell the trigger that something is not doable and to revert to sw mode.

keeping the thing as simple and direct as possible and leave the rest to
the driver by doing minimal validation on the trigger side.

Do you have suggestion on how this can be improved even more and be more
flexible? 

-- 
	Ansuel

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Rob Herring]

On Thu, Dec 15, 2022 at 12:54:38AM +0100, Christian Marangi wrote:
> Add LEDs definition example for qca8k using the offload trigger as the
> default trigger and add all the supported offload triggers by the
> switch.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index 978162df51f7..4090cf65c41c 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -65,6 +65,8 @@ properties:
>                   internal mdio access is used.
>                   With the legacy mapping the reg corresponding to the internal
>                   mdio is the switch reg with an offset of -1.
> +                 Each phy have at least 3 LEDs connected and can be declared
> +                 using the standard LEDs structure.
>  
>  patternProperties:
>    "^(ethernet-)?ports$":
> @@ -202,6 +204,7 @@ examples:
>      };
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
>  
>      mdio {
>          #address-cells = <1>;
> @@ -284,6 +287,27 @@ examples:
>  
>                  internal_phy_port1: ethernet-phy@0 {
>                      reg = <0>;
> +
> +                    leds {
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +
> +                        led@0 {
> +                            reg = <0>;
> +                            color = <LED_COLOR_ID_WHITE>;
> +                            function = LED_FUNCTION_LAN;
> +                            function-enumerator = <1>;
> +                            linux,default-trigger = "netdev";

'function' should replace this. Don't encourage more users. 

Also, 'netdev' is not documented which leaves me wondering why there's 
no warning? Either this patch didn't apply or there's a problem in the 
schema that's not checking this node.

> +                        };
> +
> +                        led@1 {
> +                            reg = <1>;
> +                            color = <LED_COLOR_ID_AMBER>;
> +                            function = LED_FUNCTION_LAN;
> +                            function-enumerator = <1>;

Typo? These are supposed to be unique. Can't you use 'reg' in your case?


> +                            linux,default-trigger = "netdev";
> +                        };
> +                    };
>                  };
>  
>                  internal_phy_port2: ethernet-phy@1 {
> -- 
> 2.37.2
> 
> 

Re: [PATCH v7 10/11] net: dsa: qca8k: add LEDs support

[Andrew Lunn]

> > +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	if (enable)
> > +		val = QCA8K_LED_RULE_CONTROLLED;
> > +
> > +	return regmap_update_bits(priv->regmap, reg_info.reg,
> > +				  GENMASK(1, 0) << reg_info.shift,
> > +				  val << reg_info.shift);
> 
> 88e151x doesn't have the ability to change in this way - we have
> a register with a 4-bit field which selects the LED mode from one
> of many, or forces the LED on/off/hi-z/blink.
> 
> Not specifically for this patch, but talking generally about this
> approach, the other issue I forsee with this is that yes, 88e151x has
> three LEDs, but the LED modes are also used to implement control
> signals (e.g., on a SFP, LOS can be implemented by programming mode
> 0 on LED2 (which makes it indicate link or not.) If we expose all the
> LEDs we run the risk of the LED subsystem trampling over that
> configuration and essentially messing up such modules. So the Marvell
> PHY driver would need to know when it is appropriate to expose these
> things to the LED subsystem.
> 
> I guess doing it dependent on firmware description as you do in
> this driver would work - if there's no firmware description, they're
> not exposed.
> 

I expect there will always be some sort of firmware involved,
describing the hardware. Without that, we have no idea how many LEDs
are actually connected to pins of the PHY, for example.

I've not yet looked at this patchset in detail, but i hope the DT
binding code is reusable, so all PHYs will have the same basic
binding.

    Andrew

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Andrew Lunn]

On Tue, Dec 20, 2022 at 11:39:58AM -0600, Rob Herring wrote:
> On Thu, Dec 15, 2022 at 12:54:38AM +0100, Christian Marangi wrote:
> > Add LEDs definition example for qca8k using the offload trigger as the
> > default trigger and add all the supported offload triggers by the
> > switch.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > index 978162df51f7..4090cf65c41c 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > @@ -65,6 +65,8 @@ properties:
> >                   internal mdio access is used.
> >                   With the legacy mapping the reg corresponding to the internal
> >                   mdio is the switch reg with an offset of -1.
> > +                 Each phy have at least 3 LEDs connected and can be declared
> > +                 using the standard LEDs structure.
> >  
> >  patternProperties:
> >    "^(ethernet-)?ports$":
> > @@ -202,6 +204,7 @@ examples:
> >      };
> >    - |
> >      #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/leds/common.h>
> >  
> >      mdio {
> >          #address-cells = <1>;
> > @@ -284,6 +287,27 @@ examples:
> >  
> >                  internal_phy_port1: ethernet-phy@0 {
> >                      reg = <0>;
> > +
> > +                    leds {
> > +                        #address-cells = <1>;
> > +                        #size-cells = <0>;
> > +
> > +                        led@0 {
> > +                            reg = <0>;
> > +                            color = <LED_COLOR_ID_WHITE>;
> > +                            function = LED_FUNCTION_LAN;
> > +                            function-enumerator = <1>;
> > +                            linux,default-trigger = "netdev";
> 
> 'function' should replace this. Don't encourage more users. 
> 
> Also, 'netdev' is not documented which leaves me wondering why there's 
> no warning? Either this patch didn't apply or there's a problem in the 
> schema that's not checking this node.

It is probably the usual limitation that the tools require a
compatible, where as the kernel does not.

> > +                        };
> > +
> > +                        led@1 {
> > +                            reg = <1>;
> > +                            color = <LED_COLOR_ID_AMBER>;
> > +                            function = LED_FUNCTION_LAN;
> > +                            function-enumerator = <1>;
> 
> Typo? These are supposed to be unique. Can't you use 'reg' in your case?

reg in this context is the address of the PHY on the MDIO bus. This is
an Ethernet switch, so has many PHYs, each with its own address.

   Andrew

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Andrew Lunn]

On Thu, Dec 15, 2022 at 12:54:38AM +0100, Christian Marangi wrote:
> Add LEDs definition example for qca8k using the offload trigger as the
> default trigger and add all the supported offload triggers by the
> switch.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index 978162df51f7..4090cf65c41c 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -65,6 +65,8 @@ properties:
>                   internal mdio access is used.
>                   With the legacy mapping the reg corresponding to the internal
>                   mdio is the switch reg with an offset of -1.
> +                 Each phy have at least 3 LEDs connected and can be declared
> +                 using the standard LEDs structure.
>  
>  patternProperties:
>    "^(ethernet-)?ports$":
> @@ -202,6 +204,7 @@ examples:
>      };
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
>  
>      mdio {
>          #address-cells = <1>;
> @@ -284,6 +287,27 @@ examples:
>  
>                  internal_phy_port1: ethernet-phy@0 {
>                      reg = <0>;
> +
> +                    leds {
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +
> +                        led@0 {
> +                            reg = <0>;
> +                            color = <LED_COLOR_ID_WHITE>;
> +                            function = LED_FUNCTION_LAN;
> +                            function-enumerator = <1>;
> +                            linux,default-trigger = "netdev";
> +                        };
> +
> +                        led@1 {
> +                            reg = <1>;
> +                            color = <LED_COLOR_ID_AMBER>;
> +                            function = LED_FUNCTION_LAN;
> +                            function-enumerator = <1>;
> +                            linux,default-trigger = "netdev";
> +                        };
> +                    };
>                  };

I don't see anything here which is specific to the QCA8K. I really
hope the same binding should work for any PHY which has LEDs. So
please move this into ethernet-phy.yaml.

       Andrew

Re: [PATCH v7 01/11] leds: add support for hardware driven LEDs

[Andrew Lunn]

On Fri, Dec 16, 2022 at 05:45:25PM +0100, Christian Marangi wrote:
> On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> > Hi Christian,
> > 
> > Thanks for the patch.
> > 
> > I think Andrew's email is offline at the moment.
> >
> 
> Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
> Holidy times I guess?

That was part of the problem. Away travelling when foot gun hit foot,
and i didn't have access to the tools to fix it while away.

    Andrew

Re: [PATCH v7 04/11] leds: trigger: netdev: rename and expose NETDEV trigger enum modes

[Andrew Lunn]

On Thu, Dec 15, 2022 at 12:54:31AM +0100, Christian Marangi wrote:
> Rename NETDEV trigger enum modes to a more simbolic name and move them

simbolic -> symbolic. 

	 Andrew

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Andrew Lunn]

> > One thought on this approach though - if one has a PHY that supports
> > "activity" but not independent "rx" and "tx" activity indications
> > and it doesn't support software control, how would one enable activity
> > mode? There isn't a way to simultaneously enable both at the same
> > time... However, I need to check whether there are any PHYs that fall
> > into this category.
> >
> 
> Problem is that for such feature and to have at least something working
> we need to face compromise. We really can't support each switch feature
> and have a generic API for everything.

I agree we need to make compromises. We cannot support every LED
feature of every PHY, they are simply too diverse. Hopefully we can
support some features of every PHY. In the worst case, a PHY simply
cannot be controlled via this method, which is the current state
today. So it is not worse off.

       Andrew

Re: [PATCH v7 09/11] leds: trigger: netdev: add additional hardware only triggers

[Andrew Lunn]

On Thu, Dec 15, 2022 at 05:35:47PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> > Add additional hardware only triggers commonly supported by switch LEDs.
> > 
> > Additional modes:
> > link_10: LED on with link up AND speed 10mbps
> > link_100: LED on with link up AND speed 100mbps
> > link_1000: LED on with link up AND speed 1000mbps
> > half_duplex: LED on with link up AND half_duplex mode
> > full_duplex: LED on with link up AND full duplex mode
> 
> Looking at Marvell 88e151x, I don't think this is usable there.
> We have the option of supporting link_1000 on one of the LEDs,
> link_100 on another, and link_10 on the other. It's rather rare
> for all three leds to be wired though.

The 88e151x will need to enumerate what it actually supports from the
above list, per LED. I also think we can carefully expand the list
above, adding a few more modes. We just need to ensure what is added
is reasonably generic, modes we expect multiple PHY to support. What
we need to avoid is adding every single mode a PHY supports, but no
other PHY has.

> This is also a PHY where "activity" mode is supported (illuminated
> or blinking if any traffic is transmitted or received) but may not
> support individual directional traffic in hardware. However, it
> does support forcing the LED on or off, so software mode can handle
> those until the user selects a combination of modes that are
> supported in the hardware.
> 
> > Additional blink interval modes:
> > blink_2hz: LED blink on any even at 2Hz (250ms)
> > blink_4hz: LED blink on any even at 4Hz (125ms)
> > blink_8hz: LED blink on any even at 8Hz (62ms)
> 
> This seems too restrictive. For example, Marvell 88e151x supports
> none of these, but does support 42, 84, 170, 340, 670ms.

I would actually drop this whole idea of being able to configure the
blink period. It seems like it is going to cause problems. I expect
most PHYs actual share the period across multiple LEDs, which you
cannot easily model here.

So i would have the driver hard coded to pick a frequency at thats' it.

   Andrew

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Andrew Lunn]

> > > +                        };
> > > +
> > > +                        led@1 {
> > > +                            reg = <1>;
> > > +                            color = <LED_COLOR_ID_AMBER>;
> > > +                            function = LED_FUNCTION_LAN;
> > > +                            function-enumerator = <1>;
> > 
> > Typo? These are supposed to be unique. Can't you use 'reg' in your case?
> 
> reg in this context is the address of the PHY on the MDIO bus. This is
> an Ethernet switch, so has many PHYs, each with its own address.

Actually, i'm wrong about that. reg in this context is the LED number
of the PHY. Typically there are 2 or 3 LEDs per PHY.

There is no reason the properties need to be unique. Often the LEDs
have 8 or 16 functions, identical for each LED, but with different
reset defaults so they show different things.

   Andrew

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Russell King (Oracle)]

On Wed, Dec 21, 2022 at 12:59:55AM +0100, Andrew Lunn wrote:
> > > One thought on this approach though - if one has a PHY that supports
> > > "activity" but not independent "rx" and "tx" activity indications
> > > and it doesn't support software control, how would one enable activity
> > > mode? There isn't a way to simultaneously enable both at the same
> > > time... However, I need to check whether there are any PHYs that fall
> > > into this category.
> > >
> > 
> > Problem is that for such feature and to have at least something working
> > we need to face compromise. We really can't support each switch feature
> > and have a generic API for everything.
> 
> I agree we need to make compromises. We cannot support every LED
> feature of every PHY, they are simply too diverse. Hopefully we can
> support some features of every PHY. In the worst case, a PHY simply
> cannot be controlled via this method, which is the current state
> today. So it is not worse off.

... and that compromise is that it's not going to be possible to enable
activity mode on 88e151x with how the code stands and with the
independent nature of "rx" and "tx" activity control currently in the
netdev trigger... making this whole approach somewhat useless for
Marvell PHYs.

We really need to see a working implementation for this code for more
than just one PHY to prove that it is actually possible for it to
support other PHYs. If not, it isn't actually solving the problem,
and we're going to continue getting custom implementations to configure
the LED settings.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Christian Marangi]

On Wed, Dec 21, 2022 at 02:41:50AM +0100, Andrew Lunn wrote:
> > > > +                        };
> > > > +
> > > > +                        led@1 {
> > > > +                            reg = <1>;
> > > > +                            color = <LED_COLOR_ID_AMBER>;
> > > > +                            function = LED_FUNCTION_LAN;
> > > > +                            function-enumerator = <1>;
> > > 
> > > Typo? These are supposed to be unique. Can't you use 'reg' in your case?
> > 
> > reg in this context is the address of the PHY on the MDIO bus. This is
> > an Ethernet switch, so has many PHYs, each with its own address.
> 
> Actually, i'm wrong about that. reg in this context is the LED number
> of the PHY. Typically there are 2 or 3 LEDs per PHY.
> 
> There is no reason the properties need to be unique. Often the LEDs
> have 8 or 16 functions, identical for each LED, but with different
> reset defaults so they show different things.
> 

Are we taking about reg or function-enumerator?

For reg it's really specific to the driver... My idea was that since a
single phy can have multiple leds attached, reg will represent the led
number.

This is an example of the dt implemented on a real device.

		mdio {
			#address-cells = <1>;
			#size-cells = <0>;

			phy_port1: phy@0 {
				reg = <0>;

				leds {
					#address-cells = <1>;
					#size-cells = <0>;

					lan1_led@0 {
						reg = <0>;
						color = <LED_COLOR_ID_WHITE>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <1>;
						linux,default-trigger = "netdev";
					};

					lan1_led@1 {
						reg = <1>;
						color = <LED_COLOR_ID_AMBER>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <1>;
						linux,default-trigger = "netdev";
					};
				};
			};

			phy_port2: phy@1 {
				reg = <1>;

				leds {
					#address-cells = <1>;
					#size-cells = <0>;


					lan2_led@0 {
						reg = <0>;
						color = <LED_COLOR_ID_WHITE>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <2>;
						linux,default-trigger = "netdev";
					};

					lan2_led@1 {
						reg = <1>;
						color = <LED_COLOR_ID_AMBER>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <2>;
						linux,default-trigger = "netdev";
					};
				};
			};

			phy_port3: phy@2 {
				reg = <2>;

				leds {
					#address-cells = <1>;
					#size-cells = <0>;

					lan3_led@0 {
						reg = <0>;
						color = <LED_COLOR_ID_WHITE>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <3>;
						linux,default-trigger = "netdev";
					};

					lan3_led@1 {
						reg = <1>;
						color = <LED_COLOR_ID_AMBER>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <3>;
						linux,default-trigger = "netdev";
					};
				};
			};

In the following implementation. Each port have 2 leds attached (out of
3) one white and one amber. The driver parse the reg and calculate the
offset to set the correct option with the regs by also checking the phy
number.

An alternative way would be set the reg to be the global led number in
the switch and deatch the phy from the calculation.

Something like
port 0 led 0 = reg 0
port 0 led 1 = reg 1
port 1 led 0 = reg 2
port 1 led 1 = reg 3
...

Using the function-enumerator can be problematic since ideally someone
would declare a dedicated function for wan led.

I'm very open to discuss and improve/fix this!

-- 
	Ansuel

Re: [PATCH v7 09/11] leds: trigger: netdev: add additional hardware only triggers

[Christian Marangi]

On Wed, Dec 21, 2022 at 01:12:16AM +0100, Andrew Lunn wrote:
> On Thu, Dec 15, 2022 at 05:35:47PM +0000, Russell King (Oracle) wrote:
> > On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> > > Add additional hardware only triggers commonly supported by switch LEDs.
> > > 
> > > Additional modes:
> > > link_10: LED on with link up AND speed 10mbps
> > > link_100: LED on with link up AND speed 100mbps
> > > link_1000: LED on with link up AND speed 1000mbps
> > > half_duplex: LED on with link up AND half_duplex mode
> > > full_duplex: LED on with link up AND full duplex mode
> > 
> > Looking at Marvell 88e151x, I don't think this is usable there.
> > We have the option of supporting link_1000 on one of the LEDs,
> > link_100 on another, and link_10 on the other. It's rather rare
> > for all three leds to be wired though.
> 
> The 88e151x will need to enumerate what it actually supports from the
> above list, per LED. I also think we can carefully expand the list
> above, adding a few more modes. We just need to ensure what is added
> is reasonably generic, modes we expect multiple PHY to support. What
> we need to avoid is adding every single mode a PHY supports, but no
> other PHY has.
> 
> > This is also a PHY where "activity" mode is supported (illuminated
> > or blinking if any traffic is transmitted or received) but may not
> > support individual directional traffic in hardware. However, it
> > does support forcing the LED on or off, so software mode can handle
> > those until the user selects a combination of modes that are
> > supported in the hardware.
> > 
> > > Additional blink interval modes:
> > > blink_2hz: LED blink on any even at 2Hz (250ms)
> > > blink_4hz: LED blink on any even at 4Hz (125ms)
> > > blink_8hz: LED blink on any even at 8Hz (62ms)
> > 
> > This seems too restrictive. For example, Marvell 88e151x supports
> > none of these, but does support 42, 84, 170, 340, 670ms.
> 
> I would actually drop this whole idea of being able to configure the
> blink period. It seems like it is going to cause problems. I expect
> most PHYs actual share the period across multiple LEDs, which you
> cannot easily model here.
> 
> So i would have the driver hard coded to pick a frequency at thats' it.
> 

Yes I think "for now" it's the only way and just drop blink
configuration support.

-- 
	Ansuel

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Andrew Lunn]

> An alternative way would be set the reg to be the global led number in
> the switch and deatch the phy from the calculation.
> 
> Something like
> port 0 led 0 = reg 0
> port 0 led 1 = reg 1
> port 1 led 0 = reg 2
> port 1 led 1 = reg 3

I would not do this. It will make LED controllers embedded in switches
different to LEDs controllers embedded in PHYs. Ideally we want them
identical. One binding to rule them all.

	   Andrew

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Christian Marangi]

On Wed, Dec 21, 2022 at 09:54:43AM +0000, Russell King (Oracle) wrote:
> On Wed, Dec 21, 2022 at 12:59:55AM +0100, Andrew Lunn wrote:
> > > > One thought on this approach though - if one has a PHY that supports
> > > > "activity" but not independent "rx" and "tx" activity indications
> > > > and it doesn't support software control, how would one enable activity
> > > > mode? There isn't a way to simultaneously enable both at the same
> > > > time... However, I need to check whether there are any PHYs that fall
> > > > into this category.
> > > >
> > > 
> > > Problem is that for such feature and to have at least something working
> > > we need to face compromise. We really can't support each switch feature
> > > and have a generic API for everything.
> > 
> > I agree we need to make compromises. We cannot support every LED
> > feature of every PHY, they are simply too diverse. Hopefully we can
> > support some features of every PHY. In the worst case, a PHY simply
> > cannot be controlled via this method, which is the current state
> > today. So it is not worse off.
> 
> ... and that compromise is that it's not going to be possible to enable
> activity mode on 88e151x with how the code stands and with the
> independent nature of "rx" and "tx" activity control currently in the
> netdev trigger... making this whole approach somewhat useless for
> Marvell PHYs.

Again we can consider adding an activity mode. It seems logical that
some switch may only support global traffic instead of independend tx or
rx... The feature are not mutually exclusive. One include the other 2.

We already a simple workaround for the link mode where on the current
driver, if the link mode is enabled just all rule for 10 100 and 1000
mbps are enabled simulating a global link event.

> 
> We really need to see a working implementation for this code for more
> than just one PHY to prove that it is actually possible for it to
> support other PHYs. If not, it isn't actually solving the problem,
> and we're going to continue getting custom implementations to configure
> the LED settings.
> 

Agree that we need other user for this to catch some problem in the
implementation of this generic API.

-- 
	Ansuel

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Andrew Lunn]

> > > I agree we need to make compromises. We cannot support every LED
> > > feature of every PHY, they are simply too diverse. Hopefully we can
> > > support some features of every PHY. In the worst case, a PHY simply
> > > cannot be controlled via this method, which is the current state
> > > today. So it is not worse off.
> > 
> > ... and that compromise is that it's not going to be possible to enable
> > activity mode on 88e151x with how the code stands and with the
> > independent nature of "rx" and "tx" activity control currently in the
> > netdev trigger... making this whole approach somewhat useless for
> > Marvell PHYs.
> 
> Again we can consider adding an activity mode. It seems logical that
> some switch may only support global traffic instead of independend tx or
> rx... The feature are not mutually exclusive. One include the other 2.

Looking at the software trigger, adding NETDEV_LED_RXTX looks simple
to do. I also suspect it will be used by more than Marvell.

> > We really need to see a working implementation for this code for more
> > than just one PHY to prove that it is actually possible for it to
> > support other PHYs. If not, it isn't actually solving the problem,
> > and we're going to continue getting custom implementations to configure
> > the LED settings.
> > 
> 
> Agree that we need other user for this to catch some problem in the
> implementation of this generic API.

We need a PHY driver implementation. The phylib core needs to be
involved, the cled code needs to call generic phylib functions which
take the phydev->lock before calling into the PHY driver. Probably the
phylib core can do all the memory allocation, and registration of the
LED to the LED core. If it is not too ugly, i would also do the DT
binding parsing in the core, so we don't end up with subtle
differences.

	Andrew

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Rob Herring]

On Wed, Dec 21, 2022 at 6:55 AM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Wed, Dec 21, 2022 at 02:41:50AM +0100, Andrew Lunn wrote:
> > > > > +                        };
> > > > > +
> > > > > +                        led@1 {
> > > > > +                            reg = <1>;
> > > > > +                            color = <LED_COLOR_ID_AMBER>;
> > > > > +                            function = LED_FUNCTION_LAN;
> > > > > +                            function-enumerator = <1>;
> > > >
> > > > Typo? These are supposed to be unique. Can't you use 'reg' in your case?
> > >
> > > reg in this context is the address of the PHY on the MDIO bus. This is
> > > an Ethernet switch, so has many PHYs, each with its own address.
> >
> > Actually, i'm wrong about that. reg in this context is the LED number
> > of the PHY. Typically there are 2 or 3 LEDs per PHY.
> >
> > There is no reason the properties need to be unique. Often the LEDs
> > have 8 or 16 functions, identical for each LED, but with different
> > reset defaults so they show different things.
> >
>
> Are we taking about reg or function-enumerator?
>
> For reg it's really specific to the driver... My idea was that since a
> single phy can have multiple leds attached, reg will represent the led
> number.
>
> This is an example of the dt implemented on a real device.
>
>                 mdio {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>
>                         phy_port1: phy@0 {
>                                 reg = <0>;
>
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>
>                                         lan1_led@0 {
>                                                 reg = <0>;
>                                                 color = <LED_COLOR_ID_WHITE>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <1>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>
>                                         lan1_led@1 {
>                                                 reg = <1>;
>                                                 color = <LED_COLOR_ID_AMBER>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <1>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>                                 };
>                         };
>
>                         phy_port2: phy@1 {
>                                 reg = <1>;
>
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>
>
>                                         lan2_led@0 {
>                                                 reg = <0>;
>                                                 color = <LED_COLOR_ID_WHITE>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <2>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>
>                                         lan2_led@1 {
>                                                 reg = <1>;
>                                                 color = <LED_COLOR_ID_AMBER>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <2>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>                                 };
>                         };
>
>                         phy_port3: phy@2 {
>                                 reg = <2>;
>
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>
>                                         lan3_led@0 {
>                                                 reg = <0>;
>                                                 color = <LED_COLOR_ID_WHITE>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <3>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>
>                                         lan3_led@1 {
>                                                 reg = <1>;
>                                                 color = <LED_COLOR_ID_AMBER>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <3>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>                                 };
>                         };
>
> In the following implementation. Each port have 2 leds attached (out of
> 3) one white and one amber. The driver parse the reg and calculate the
> offset to set the correct option with the regs by also checking the phy
> number.

Okay, the full example makes more sense. But I still thought
'function-enumerator' values should be globally unique within a value
of 'function'. Maybe Jacek has an opinion on this?

You are using it to distinguish phys/ports, but there's already enough
information in the DT to do that. You have the parent nodes and I
assume you have port numbers under 'ethernet-ports'. For each port,
get the phy node and then get the LEDs.

> An alternative way would be set the reg to be the global led number in
> the switch and deatch the phy from the calculation.
>
> Something like
> port 0 led 0 = reg 0
> port 0 led 1 = reg 1
> port 1 led 0 = reg 2
> port 1 led 1 = reg 3
> ...

No.

Rob

Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support

[Alexander Stein]

Am Freitag, 16. Dezember 2022, 18:00:45 CET schrieb Christian Marangi:
> On Thu, Dec 15, 2022 at 04:27:17PM +0100, Alexander Stein wrote:
> > Hi,
> > 
> > thanks for the v7 series.
> > 
> > Am Donnerstag, 15. Dezember 2022, 00:54:33 CET schrieb Christian Marangi:
> > > Add hardware control support for the Netdev trigger.
> > > The trigger on config change will check if the requested trigger can set
> > > to blink mode using LED hardware mode and if every blink mode is
> > > supported,
> > > the trigger will enable hardware mode with the requested configuration.
> > > If there is at least one trigger that is not supported and can't run in
> > > hardware mode, then software mode will be used instead.
> > > A validation is done on every value change and on fail the old value is
> > > restored and -EINVAL is returned.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > > 
> > >  drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> > >  1 file changed, 149 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/leds/trigger/ledtrig-netdev.c
> > > b/drivers/leds/trigger/ledtrig-netdev.c index dd63cadb896e..ed019cb5867c
> > > 100644
> > > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > > @@ -37,6 +37,7 @@
> > > 
> > >   */
> > >  
> > >  struct led_netdev_data {
> > > 
> > > +	enum led_blink_modes blink_mode;
> > > 
> > >  	spinlock_t lock;
> > >  	
> > >  	struct delayed_work work;
> > > 
> > > @@ -53,11 +54,105 @@ struct led_netdev_data {
> > > 
> > >  	bool carrier_link_up;
> > >  
> > >  };
> > > 
> > > +struct netdev_led_attr_detail {
> > > +	char *name;
> > > +	bool hardware_only;
> > > +	enum led_trigger_netdev_modes bit;
> > > +};
> > > +
> > > +static struct netdev_led_attr_detail attr_details[] = {
> > > +	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
> > > +	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
> > > +	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
> > > +};
> > > +
> > > +static bool validate_baseline_state(struct led_netdev_data
> > > *trigger_data)
> > > +{
> > > +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > > +	struct netdev_led_attr_detail *detail;
> > > +	u32 hw_blink_mode_supported = 0;
> > > +	bool force_sw = false;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > > +		detail = &attr_details[i];
> > > +
> > > +		/* Mode not active, skip */
> > > +		if (!test_bit(detail->bit, &trigger_data->mode))
> > > +			continue;
> > > +
> > > +		/* Hardware only mode enabled on software controlled led
> > 
> > */
> > 
> > > +		if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> > > +		    detail->hardware_only)
> > > +			return false;
> > > +
> > > +		/* Check if the mode supports hardware mode */
> > > +		if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > > +			/* With a net dev set, force software mode.
> > > +			 * With modes are handled by hardware, led will
> > 
> > blink
> > 
> > > +			 * based on his own events and will ignore any
> > 
> > event
> > 
> > > +			 * from the provided dev.
> > > +			 */
> > > +			if (trigger_data->net_dev) {
> > > +				force_sw = true;
> > > +				continue;
> > > +			}
> > > +
> > > +			/* With empty dev, check if the mode is
> > 
> > supported */
> > 
> > > +			if
> > 
> > (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> > 
> > > +				hw_blink_mode_supported |= BIT(detail-
> > >
> > >bit);
> > 
> > Shouldn't this be BIT(detail->bit)?
> 
> I think I didn't understand?

The name 'bit' indicates this is a single bit number rather than a bitmask. 
AFAICS the value (detail->bit) passed to led_trigger_blink_mode_is_supported 
is eventually used within test_bit inside dp83867_parse_netdev. I assume you 
have to actually pass the bitmask with this single bit set, not the bit number 
itself.

Best regards,
Alexander

> > > +		}
> > > +	}
> > > +
> > > +	/* We can't run modes handled by both software and hardware.
> > > +	 * Check if we run hardware modes and check if all the modes
> > > +	 * can be handled by hardware.
> > > +	 */
> > > +	if (hw_blink_mode_supported && hw_blink_mode_supported !=
> > > trigger_data->mode) +		return false;
> > > +
> > > +	/* Modes are valid. Decide now the running mode to later
> > > +	 * set the baseline.
> > > +	 * Software mode is enforced with net_dev set. With an empty
> > > +	 * one hardware mode is selected by default (if supported).
> > > +	 */
> > > +	if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> > 
> > IMHO '|| !hw_blink_mode_supported' should be added here for blink_modes.
> > This might happen if a PHY LED is SOFTWARE_HARDWARE_CONTROLLED, but some
> > blink mode is not supported by hardware, thus hw_blink_mode_supported=0.
> 
> Will check this and report back.
> 
> > Best regards,
> > Alexander
> > 
> > > +		trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> > > +	else
> > > +		trigger_data->blink_mode = HARDWARE_CONTROLLED;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > 
> > >  static void set_baseline_state(struct led_netdev_data *trigger_data)
> > >  {
> > > 
> > > +	int i;
> > > 
> > >  	int current_brightness;
> > > 
> > > +	struct netdev_led_attr_detail *detail;
> > > 
> > >  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > > 
> > > +	/* Modes already validated. Directly apply hw trigger modes */
> > > +	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> > > +		/* We are refreshing the blink modes. Reset them */
> > > +		led_cdev->hw_control_configure(led_cdev,
> > 
> > BIT(TRIGGER_NETDEV_LINK),
> > 
> > > +					       BLINK_MODE_ZERO);
> > > +
> > > +		for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > > +			detail = &attr_details[i];
> > > +
> > > +			if (!test_bit(detail->bit, &trigger_data->mode))
> > > +				continue;
> > > +
> > > +			led_cdev->hw_control_configure(led_cdev,
> > 
> > BIT(detail->bit),
> > 
> > > +
> > 
> > BLINK_MODE_ENABLE);
> > 
> > Shouldn't this be BIT(detail->bit)?
> > 
> > > +		}
> > > +
> > > +		led_cdev->hw_control_start(led_cdev);
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	/* Handle trigger modes by software */
> > > 
> > >  	current_brightness = led_cdev->brightness;
> > >  	if (current_brightness)
> > >  	
> > >  		led_cdev->blink_brightness = current_brightness;
> > > 
> > > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device
> > > *dev,
> > > 
> > >  				 size_t size)
> > >  
> > >  {
> > >  
> > >  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > > 
> > > +	struct net_device *old_net = trigger_data->net_dev;
> > > +	char old_device_name[IFNAMSIZ];
> > > 
> > >  	if (size >= IFNAMSIZ)
> > >  	
> > >  		return -EINVAL;
> > > 
> > > +	/* Backup old device name */
> > > +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> > > +
> > > 
> > >  	cancel_delayed_work_sync(&trigger_data->work);
> > >  	
> > >  	spin_lock_bh(&trigger_data->lock);
> > > 
> > > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device
> > > *dev,
> > > 
> > >  		trigger_data->net_dev =
> > >  		
> > >  		    dev_get_by_name(&init_net, trigger_data->device_name);
> > > 
> > > +	if (!validate_baseline_state(trigger_data)) {
> > > +		/* Restore old net_dev and device_name */
> > > +		if (trigger_data->net_dev)
> > > +			dev_put(trigger_data->net_dev);
> > > +
> > > +		dev_hold(old_net);
> > > +		trigger_data->net_dev = old_net;
> > > +		memcpy(trigger_data->device_name, old_device_name,
> > 
> > IFNAMSIZ);
> > 
> > > +
> > > +		spin_unlock_bh(&trigger_data->lock);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > 
> > >  	trigger_data->carrier_link_up = false;
> > >  	if (trigger_data->net_dev != NULL)
> > >  	
> > >  		trigger_data->carrier_link_up =
> > 
> > netif_carrier_ok(trigger_data->net_dev);
> > 
> > > @@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device
> > > *dev, const char *buf, size_t size, enum led_trigger_netdev_modes attr)
> > > 
> > >  {
> > >  
> > >  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > > 
> > > -	unsigned long state;
> > > +	unsigned long state, old_mode = trigger_data->mode;
> > > 
> > >  	int ret;
> > >  	int bit;
> > > 
> > > @@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device
> > > *dev, const char *buf, else
> > > 
> > >  		clear_bit(bit, &trigger_data->mode);
> > > 
> > > +	if (!validate_baseline_state(trigger_data)) {
> > > +		/* Restore old mode on validation fail */
> > > +		trigger_data->mode = old_mode;
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > 
> > >  	set_baseline_state(trigger_data);
> > >  	
> > >  	return size;
> > > 
> > > @@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
> > > 
> > >  			      size_t size)
> > >  
> > >  {
> > >  
> > >  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > > 
> > > +	int old_interval = atomic_read(&trigger_data->interval);
> > > +	u32 old_mode = trigger_data->mode;
> > > 
> > >  	unsigned long value;
> > >  	int ret;
> > > 
> > > @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
> > > 
> > >  		return ret;
> > >  	
> > >  	/* impose some basic bounds on the timer interval */
> > > 
> > > -	if (value >= 5 && value <= 10000) {
> > > -		cancel_delayed_work_sync(&trigger_data->work);
> > > +	if (value < 5 || value > 10000)
> > > +		return -EINVAL;
> > > +
> > > +	cancel_delayed_work_sync(&trigger_data->work);
> > > +
> > > +	atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> > > 
> > > -		atomic_set(&trigger_data->interval,
> > 
> > msecs_to_jiffies(value));
> > 
> > > -		set_baseline_state(trigger_data);	/* resets timer
> > 
> > */
> > 
> > > +	if (!validate_baseline_state(trigger_data)) {
> > > +		/* Restore old interval on validation error */
> > > +		atomic_set(&trigger_data->interval, old_interval);
> > > +		trigger_data->mode = old_mode;
> > > +		return -EINVAL;
> > > 
> > >  	}
> > > 
> > > +	set_baseline_state(trigger_data);	/* resets timer */
> > > +
> > > 
> > >  	return size;
> > >  
> > >  }
> > > 
> > > @@ -368,13 +498,25 @@ static int netdev_trig_activate(struct
> > > led_classdev
> > > *led_cdev) trigger_data->mode = 0;
> > > 
> > >  	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
> > >  	trigger_data->last_activity = 0;
> > > 
> > > +	if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > > +		/* With hw mode enabled reset any rule set by default */
> > > +		if (led_cdev->hw_control_status(led_cdev)) {
> > > +			rc = led_cdev->hw_control_configure(led_cdev,
> > 
> > BIT(TRIGGER_NETDEV_LINK),
> > 
> > > +
> > 
> > BLINK_MODE_ZERO);
> > 
> > > +			if (rc)
> > > +				goto err;
> > > +		}
> > > +	}
> > > 
> > >  	led_set_trigger_data(led_cdev, trigger_data);
> > >  	
> > >  	rc = register_netdevice_notifier(&trigger_data->notifier);
> > >  	if (rc)
> > > 
> > > -		kfree(trigger_data);
> > > +		goto err;
> > > 
> > > +	return 0;
> > > +err:
> > > +	kfree(trigger_data);
> > > 
> > >  	return rc;
> > >  
> > >  }
> > > 
> > > @@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct
> > > led_classdev
> > > *led_cdev)
> > > 
> > >  static struct led_trigger netdev_led_trigger = {
> > >  
> > >  	.name = "netdev",
> > > 
> > > +	.supported_blink_modes = SOFTWARE_HARDWARE,
> > > 
> > >  	.activate = netdev_trig_activate,
> > >  	.deactivate = netdev_trig_deactivate,
> > >  	.groups = netdev_trig_groups,

Re: [PATCH v7 01/11] leds: add support for hardware driven LEDs

[Bagas Sanjaya]

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

On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> The LED must implement 3 main API:
> - hw_control_status(): This asks the LED driver if hardware mode is
>     enabled or not.
>     Triggers will check if the offload mode is supported and will be
>     activated accordingly. If the trigger can't run in software mode,
>     return -EOPNOTSUPP as the blinking can't be simulated by software.
> - hw_control_start(): This will simply enable the hardware mode for
>     the LED.
>     With this not declared and hw_control_status() returning true,
>     it's assumed that the LED is always in hardware mode.
> - hw_control_stop(): This will simply disable the hardware mode for
>     the LED.
>     With this not declared and hw_control_status() returning true,
>     it's assumed that the LED is always in hardware mode.
>     It's advised to the driver to put the LED in the old state but this
>     is not enforcerd and putting the LED off is also accepted.
> 

Building htmldocs with Sphinx, I got:

Documentation/leds/leds-class.rst:190: WARNING: Unexpected indentation.
Documentation/leds/leds-class.rst:192: WARNING: Block quote ends without a blank line; unexpected unindent.

I have applied the fixup on the API bullet list above:

---- >8 ----

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index efd2f68c46a7f9..fc16b503747800 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -186,13 +186,15 @@ supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTR
 is assumed.
 
 The LED must implement 3 main API:
+
 - hw_control_status(): This asks the LED driver if hardware mode is enabled
-    or not. Triggers will check if the hardware mode is active and will try
-    to offload their triggers if supported by the driver.
+  or not. Triggers will check if the hardware mode is active and will try
+  to offload their triggers if supported by the driver.
 - hw_control_start(): This will simply enable the hardware mode for the LED.
+
 - hw_control_stop(): This will simply disable the hardware mode for the LED.
-    It's advised to the driver to put the LED in the old state but this is not
-    enforcerd and putting the LED off is also accepted.
+  It's advised to the driver to put the LED in the old state but this is not
+  enforcerd and putting the LED off is also accepted.
 
 With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
 and any software only trigger will reject activation as the LED supports only

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Sander Vanheule]

Hi Christian,

On Wed, 2022-12-21 at 13:54 +0100, Christian Marangi wrote:
> For reg it's really specific to the driver... My idea was that since a
> single phy can have multiple leds attached, reg will represent the led
> number.
> 
> This is an example of the dt implemented on a real device.
> 
>                 mdio {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         phy_port1: phy@0 {
>                                 reg = <0>;
> 
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
[...]
>                                 };
>                         };
[...]
>                 };
> 
> In the following implementation. Each port have 2 leds attached (out of
> 3) one white and one amber. The driver parse the reg and calculate the
> offset to set the correct option with the regs by also checking the phy
> number.

With switch silicon allowing user control of the LEDs, vendors can (and will)
use the switch's LED peripheral to drive other LEDs (or worse). E.g. on a Cisco
SG220-26 switch, using a Realtek RTL8382 SoC, the LEDs associated with some
unused switch ports are used to display a global device status. My concern here
is that one would have to specify switch ports, that aren't connected to
anything, just to describe those non-ethernet LEDs.

Would an alternative with a 'trigger-sources' property pointing to the right phy
be an option? The trade-off I see would be that extra port info has to be
provided on a separate LED controller, which your example can avoid thanks to
the phy's reg property.

Building on your example this may become:

       switch {
           mdio {
                #address-cells = <1>;
                #size-cells = <0>;
                
                switch_phy0: phy@0 {
                    reg = <0>;
                    #trigger-source-cells = <1>;
                };
            };

            leds {
                #address-cells = <2>;
                #size-cells = <0>;

                /* First port, first LED */
                /* Port status, can be offloaded */
                led@0.0 {
                    reg = <0 0>;
                    trigger-sources = <&switch_phy0 (NET_LINK | NET_SPEED_1000)>;
                    function = color = <LED_COLOR_ID_WHITE>;
                    function = LED_FUNCTION_LAN;
                    function-enumerator = <1>;
                    linux,default-trigger = "netdev";
                };

                /* First port, first LED */
                /* Port status, can be offloaded */
                led@0.1 {
                    reg = <0 1>;
                    trigger-sources = <&switch_phy0 (NET_LINK | NET_SPEED_100 | NET_SPEED_10)>;
                    function = color = <LED_COLOR_ID_AMBER>;
                    function = LED_FUNCTION_LAN;
                    function-enumerator = <1>;
                    linux,default-trigger = "netdev";
                };

                /* Last port (not used in hardware), first LED */
                /* Device status, software controlled */
                led@7.0 {
                    reg = <7 0>;
                    function = color = <LED_COLOR_ID_AMBER>;
                    function = LED_FUNCTION_STATUS;
                    linux,default-trigger = "default-on";
                };
            };
        };


To be a bit less verbose, the &switch_mdio node might serve as trigger provider
with a single cell, but the above would allow only defined phy-s to be
referenced.

The trigger-source cells could be used for a more fine grained control of what
should be offloaded (link up/down, Rx/Tx activity, link speed, ...). Although
this selectivity is most likely runtime configurable, this could serve as a
description of static device labeling (e.g. "LINK/ACT 1000").

Switching to the implementation and driver side, the 'trigger-sources' property
could be used by the netdev trigger to determine if a status LED can be
offloaded. The netdev trigger could just hide the whole hardware/software
control aspect then. Much like how the timer trigger always offloads if an
implementation is provided, even when offloading is less flexible than the
software implementation of the timer trigger.


Best,
Sander

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Andrew Lunn]

> > This is an example of the dt implemented on a real device.
> > 
> >                 mdio {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> > 
> >                         phy_port1: phy@0 {
> >                                 reg = <0>;
> > 
> >                                 leds {
> >                                         #address-cells = <1>;
> >                                         #size-cells = <0>;
> [...]
> >                                 };
> >                         };
> [...]
> >                 };
> > 
> > In the following implementation. Each port have 2 leds attached (out of
> > 3) one white and one amber. The driver parse the reg and calculate the
> > offset to set the correct option with the regs by also checking the phy
> > number.
> 
> With switch silicon allowing user control of the LEDs, vendors can (and will)
> use the switch's LED peripheral to drive other LEDs (or worse). E.g. on a Cisco
> SG220-26 switch, using a Realtek RTL8382 SoC, the LEDs associated with some
> unused switch ports are used to display a global device status. My concern here
> is that one would have to specify switch ports, that aren't connected to
> anything, just to describe those non-ethernet LEDs.

Note that the binding is adding properties to the PHY nodes, not the
switch port nodes. Is this how the RTL8382 works? Marvell Switches
have LED registers which are not in the PHY register space.

But the point is, the PHYs will probe if listed. They don't have to
have a MAC pointing to them with a phandle. So the phydev will exist,
and that should be enough to get the LED class device registered. If
there is basic on/off support, that should be enough for you to attach
the Morse code panic trigger, the heartbeat handler, or any other LED
trigger.

	Andrew

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Sander Vanheule]

Hi Andrew,

On Sun, 2023-01-29 at 23:02 +0100, Andrew Lunn wrote:
> > > This is an example of the dt implemented on a real device.
> > > 
> > >                 mdio {
> > >                         #address-cells = <1>;
> > >                         #size-cells = <0>;
> > > 
> > >                         phy_port1: phy@0 {
> > >                                 reg = <0>;
> > > 
> > >                                 leds {
> > >                                         #address-cells = <1>;
> > >                                         #size-cells = <0>;
> > [...]
> > >                                 };
> > >                         };
> > [...]
> > >                 };
> > > 
> > > In the following implementation. Each port have 2 leds attached (out of
> > > 3) one white and one amber. The driver parse the reg and calculate the
> > > offset to set the correct option with the regs by also checking the phy
> > > number.
> > 
> > With switch silicon allowing user control of the LEDs, vendors can (and
> > will)
> > use the switch's LED peripheral to drive other LEDs (or worse). E.g. on a
> > Cisco
> > SG220-26 switch, using a Realtek RTL8382 SoC, the LEDs associated with some
> > unused switch ports are used to display a global device status. My concern
> > here
> > is that one would have to specify switch ports, that aren't connected to
> > anything, just to describe those non-ethernet LEDs.
> 
> Note that the binding is adding properties to the PHY nodes, not the
> switch port nodes. Is this how the RTL8382 works? Marvell Switches
> have LED registers which are not in the PHY register space.

Thanks for the quick clarification. Because you mention this, I realised that
the RTL8382's LED controller is actually not in the PHYs. These SoCs use
external PHYs, which may have their own, independent, LED controllers. For
example the RTL8212D [1].

[1]
https://datasheet.lcsc.com/lcsc/2203252253_Realtek-Semicon-RTL8218D-CG_C2901898.pdf

> 
> But the point is, the PHYs will probe if listed. They don't have to
> have a MAC pointing to them with a phandle. So the phydev will exist,
> and that should be enough to get the LED class device registered. If
> there is basic on/off support, that should be enough for you to attach
> the Morse code panic trigger, the heartbeat handler, or any other LED
> trigger.

OK, this makes sense for (external) PHYs which need to be probed anyway to have
access to the LEDs.

Looking at the RTL8212D's datasheet (Table 11, p. 24), it appears to be possible
to assign an LED to any of the eight PHYs. Perhaps to allow more freedom in the
board layout. Maybe I'm just not seeing it, but I don't think the example with
an 'leds' node under a PHY contains enough information to perform such a non-
trivial mapping. On the other hand, I'm not sure where else that info might go.
Maybe a 'trigger-sources' property cross-referencing another PHY in the same
package?

Best,
Sander

Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Andrew Lunn]

> Thanks for the quick clarification. Because you mention this, I realised that
> the RTL8382's LED controller is actually not in the PHYs. These SoCs use
> external PHYs, which may have their own, independent, LED controllers. For
> example the RTL8212D [1].
> 
> [1]
> https://datasheet.lcsc.com/lcsc/2203252253_Realtek-Semicon-RTL8218D-CG_C2901898.pdf
> 
> > 
> > But the point is, the PHYs will probe if listed. They don't have to
> > have a MAC pointing to them with a phandle. So the phydev will exist,
> > and that should be enough to get the LED class device registered. If
> > there is basic on/off support, that should be enough for you to attach
> > the Morse code panic trigger, the heartbeat handler, or any other LED
> > trigger.
> 
> OK, this makes sense for (external) PHYs which need to be probed anyway to have
> access to the LEDs.
> 
> Looking at the RTL8212D's datasheet (Table 11, p. 24), it appears to be possible
> to assign an LED to any of the eight PHYs. Perhaps to allow more freedom in the
> board layout. Maybe I'm just not seeing it, but I don't think the example with
> an 'leds' node under a PHY contains enough information to perform such a non-
> trivial mapping. On the other hand, I'm not sure where else that info might go.

The binding is defining all the generic properties need for generic
PHY LED. For most PHYs, it is probably sufficient. However, there is
nothing stopping you from adding PHY specific properties. So for
example, for each PHY LED you could have a property which maps it to
a LED00-LED35.

So propose a binding for the RTL8218D with whatever extra properties
you think are needed, and it will be reviewed in the normal way.

    Andrew