[PATCH 00/11] leds: introduce new LED hw control APIs

[PATCH 00/11] leds: introduce new LED hw control APIs

[Christian Marangi]

This is a continue of [1]. It was decided to take a more gradual
approach to implement LEDs support for switch and phy starting with
basic support and then implementing the hw control part when we have all
the prereq done.

This is the main part of the series, the one that actually implement the
hw control API.

Some history about this feature and why
=======================================

This proposal is highly requested by the entire net community but the API
is not strictly designed for net usage but for a more generic usage.

Initial version were very flexible and designed to try to support every
aspect of the LED driver with many complex function that served multiple
purpose. There was an idea to have sw only and hw only LEDs and sw only
and hw only LEDs.

With some heads up from Andrew from the net mailing list, it was suggested
to implement a more basic yet easy to implement system.

These API strictly work with a designated trigger to offload their
function.
This may be confused with hw blink offload but LED may have an even more
advanced configuration where the entire aspect of the trigger is
offloaded and completely handled by the hardware.

An example of this usage are PHY or switch port LEDs. Almost every of
these kind of device have multiple LED attached and provide info of the
current port state.

Currently we lack any support of them but these device always provide a
way to configure them, from basic feature like turning the LED off or no
(implemented in previous series related to this feature) or even entirely
driven by the hw and power on/off/blink based on some events, like tx/rx
traffic, ethernet cable attached, link speed of 10mbps, 100mbps, 1000mbps
or more. They can also support multiple logic like blink with traffic only
if a particular link speed is attached. (an example of this is when a LED
is designated to be turned on only with 100mbps link speed and configured
to blink on traffic and a secondary LED of a different color is present to
serve the same function but only when the link speed is 1000mbps)

These case are very common for a PHY or a switch but they were never
standardized so OEM support all kind of variant and configuration.

Again with Andrew we compared some feature and we reached a common set
of modes that are for sure present in every kind of devices.

And this concludes history and why.

What is present in this series
==============================

This patch contain the required API to support this feature, I decided on
the name of hw control to quickly describe this feature.

I documented each require API in the related Documentation for leds-class
so I think it might me redundant to expose them here. Feel free to tell me
how to improve it if anything is not clear.

On an abstract idea, this feature require this:

    - The trigger needs to make use of it, this is currently implemented
      for the netdev trigger but other trigger can be expanded if the
      device expose these function. An idea might be a anything that
      handle a storage disk and have the LED configurable to blink when
      there is any activity to the disk.

    - The LED driver needs to expose and implement these new API.

Currently a LED driver supports only a trigger. The trigger should use
the related helper to check if the LED can be driven hy hardware.

The different modes a trigger support are exposed in the kernel include
leds.h header and are used by the LED driver to understand what to do.

The LED driver expose a mask of the different modes supported and trigger
use this to validate the modes and decide what to enable.

From a user standpoint, he should enable modes as usual from sysfs and if
anything is not supported warned.

Final words and missing piece from this series
==============================================

I honestly hope this feature can finally be implemented.

This series originally had also additional modes and logic to add to the
netdev trigger, but I decided to strip them and implement only the API
and support basic tx and rx. After this is merged, I will quickly propose
these additional modes.

Currently this is limited to tx and rx and this is what the current user
qca8k use. Marvell PHY support link and a generic blink with any kind of
traffic (both rx and tx). qca8k switch supports keeping the LED on based on
link speed.

The next series will add the concept of hw control only modes to the netdev
trigger and support for these additional modes:
- link_10
- link_100
- link_1000
- activity

The current implementation is voluntary basic and limited to put the ground
work and have something easy to implement and usable. 99% part of the logic
is done on the trigger side, leaving to the LED driver only the validating
and the apply part.

As shown for the PHY led binding, people are really intrested in this
feature as quickly after they were merged, people were already working on
adding support for it.

[1] https://lore.kernel.org/lkml/20230216013230.22978-1-ansuelsmth@gmail.com/

Changes from previous v8 series:
- Rewrite Documentation from scratch and move to separate commit
- Strip additional trigger modes (to propose in a different series)
- Strip from qca8k driver additional modes (to implement in the different
  series)
- Split the netdev chages to smaller piece to permit easier review

Changelog in the previous v8 series: (stripped of unrelated changes)
v8:
- Improve the documentation of the new feature
- Rename to a more symbolic name
- Fix some bug in netdev trigger (not using BIT())
- Add more define for qca8k-leds driver
- Drop interval support
- Fix many bugs in the validate option in the netdev trigger
v7:
- 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.
v3:
- Rework start/stop as Andrew asked.
- 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 binding for LEDs hw control
  leds: add binding to check support for LED hw control
  leds: add helper function to use trigger in hw blink mode
  Documentation: leds: leds-class: Document new Hardware driven LEDs
    APIs
  leds: trigger: netdev: introduce validating requested mode
  leds: trigger: netdev: add knob to set hw control possible
  leds: trigger: netdev: reject interval and device store for hw_control
  leds: trigger: netdev: add support for LED hw control
  leds: trigger: netdev: init mode if hw control already active
  leds: trigger: netdev: expose netdev trigger modes in linux include
  net: dsa: qca8k: implement hw_control ops

 Documentation/leds/leds-class.rst     |  56 +++++++++
 drivers/leds/trigger/ledtrig-netdev.c | 111 +++++++++++++++---
 drivers/net/dsa/qca/qca8k-leds.c      | 156 ++++++++++++++++++++++++++
 include/linux/leds.h                  |  46 ++++++++
 4 files changed, 355 insertions(+), 14 deletions(-)

-- 
2.39.2

[PATCH 01/11] leds: add binding for LEDs hw control

[Christian Marangi]

Add an option to permit LED driver to declare support for a specific
trigger to use hw control and setup the LED to blink based on specific
provided modes.

Add binding for LEDs hw control. These functions will be used to activate
hardware control where a LED will use the provided flags, from an
unique defined supported trigger, to setup the LED to be driven by
hardware.

Deactivate hardware blink control by setting brightness to LED_OFF via
the brightness_set() callback.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/leds.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..b3bd1cc8ace8 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -154,6 +154,26 @@ struct led_classdev {
 
 	/* LEDs that have private triggers have this set */
 	struct led_hw_trigger_type	*trigger_type;
+
+	/* Unique trigger name supported by LED set in hw control mode */
+	const char		*hw_control_trigger;
+	/*
+	 * Activate hardware control, LED driver will use the provided flags
+	 * from the supported trigger and setup the LED to be driven by hardware
+	 * following the requested mode from the trigger flags.
+	 * Deactivate hardware blink control by setting brightness to LED_OFF via
+	 * the brightness_set() callback.
+	 */
+	int			(*hw_control_set)(struct led_classdev *led_cdev,
+						  unsigned long flags);
+	/*
+	 * Get from the LED driver the current mode that the LED is set in hw
+	 * control mode and put them in flags.
+	 * Trigger can use this to get the initial state of a LED already set in
+	 * hardware blink control.
+	 */
+	int			(*hw_control_get)(struct led_classdev *led_cdev,
+						  unsigned long *flags);
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
-- 
2.39.2

[PATCH 02/11] leds: add binding to check support for LED hw control

[Christian Marangi]

Add 2 binding to facilitate checking support for LED hw control.

Add a mask for the LED driver to declare support for specific modes of
the defined hw control trigger.

Add hw_control_is_supported() to ask the LED driver if the requested
mode by the trigger are supported and the LED can be setup to follow
the requested modes.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/leds.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index b3bd1cc8ace8..06a67c62ed6e 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -157,6 +157,8 @@ struct led_classdev {
 
 	/* Unique trigger name supported by LED set in hw control mode */
 	const char		*hw_control_trigger;
+	/* Mask of the different supported trigger mode in hw control mode */
+	unsigned long		trigger_supported_flags_mask;
 	/*
 	 * Activate hardware control, LED driver will use the provided flags
 	 * from the supported trigger and setup the LED to be driven by hardware
@@ -174,6 +176,12 @@ struct led_classdev {
 	 */
 	int			(*hw_control_get)(struct led_classdev *led_cdev,
 						  unsigned long *flags);
+	/*
+	 * Check if the LED driver supports the requested mode provided by the
+	 * defined supported trigger to setup the LED to hw control mode.
+	 */
+	int			(*hw_control_is_supported)(struct led_classdev *led_cdev,
+							   unsigned long flags);
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
-- 
2.39.2

[PATCH 03/11] leds: add helper function to use trigger in hw blink mode

[Christian Marangi]

Add helper function to use trigger in hw blink mode.

Add function led_trigger_can_hw_control() that will check if hw_control
ops are defined and if the current trigger match the one supported for
hw_control.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/leds.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index 06a67c62ed6e..b9152bff3a96 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -425,6 +425,14 @@ void led_trigger_blink_oneshot(struct led_trigger *trigger,
 void led_trigger_set_default(struct led_classdev *led_cdev);
 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger);
 void led_trigger_remove(struct led_classdev *led_cdev);
+static inline bool led_trigger_can_hw_control(struct led_classdev *led_cdev)
+{
+	if (!led_cdev->hw_control_get || !led_cdev->hw_control_set ||
+	    !led_cdev->hw_control_is_supported)
+		return false;
+
+	return !strcmp(led_cdev->hw_control_trigger, led_cdev->trigger->name);
+}
 
 static inline void led_set_trigger_data(struct led_classdev *led_cdev,
 					void *trigger_data)
-- 
2.39.2

[PATCH 04/11] Documentation: leds: leds-class: Document new Hardware driven LEDs APIs

[Christian Marangi]

Document new Hardware driven LEDs APIs.

Some LEDs can be programmed to be entirely driven by hw. This is not
limited to blink but also to turn off or on autonomously.
To support this feature, a LED needs to implement various additional
ops and needs to declare specific support for the supported triggers.

Add documentation for each required value and API to make hw control
possible and implementable by both LEDs and triggers.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 Documentation/leds/leds-class.rst | 56 +++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index cd155ead8703..a7bc31e9b919 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -169,6 +169,62 @@ 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 programmed to be entirely driven by hw. This is not
+limited to blink but also to turn off or on autonomously.
+To support this feature, a LED needs to implement various additional
+ops and needs to declare specific support for the supported triggers.
+
+With hw control we refer to the LED driven by hardware.
+
+LED driver must define the following value to support hw control:
+
+    - hw_control_trigger:
+               unique trigger name supported by the LED in hw control
+               mode.
+
+    - trigger_supported_flags_mask:
+                mask of the different supported trigger mode for the
+                defined trigger in hw control mode.
+
+LED driver must implement the following API to support hw control:
+
+     - hw_control_set:
+                activate hw control, LED driver will use the provided
+                flags passed from the supported trigger, parse them to
+                a set of mode and setup the LED to be driven by hardware
+                following the requested modes.
+
+                Set LED_OFF via the brightness_set to deactivate hw control.
+
+    - hw_control_get:
+                get from a LED already in hw control, the active modes,
+                parse them and set in flags the current active flags for
+                the supported trigger.
+
+    - hw_control_is_supported:
+                check if the flags passed by the supported trigger can
+                be parsed and activate hw control on the LED.
+
+LED driver can activate additional modes by default to workaround the
+impossibility of supporting each different mode on the supported trigger.
+Example are hardcoding the blink speed to a set interval, enable special
+feature like bypassing blink if some requirements are not met.
+
+A helper led_trigger_can_hw_control() is provided to check if the LED
+can actually run in hw control.
+
+A trigger should first use such helper to verify if hw control is possible,
+use hw_control_is_supported to check if the flags are supported and only at
+the end use hw_control_set to activate hw control.
+
+A trigger can use hw_control_get to check if a LED is already in hw control
+and init their flags.
+
+When the LED is in hw control, no software blink is possible and doing so
+will effectively disable hw control.
 
 Known Issues
 ============
-- 
2.39.2

[PATCH 05/11] leds: trigger: netdev: introduce validating requested mode

[Christian Marangi]

Introduce function to validate the requested mode in preparation for
hw control support. Currently everything is handled in software so
every mode is validated and accepted.

Requested mode are always validated before making any change, to follow
this rework the attr_store function to set the mode to a temp variable
and then apply the new mode only if accepted and validated.

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

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 115f2bae9eee..81e0b0083f2f 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -91,6 +91,12 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	}
 }
 
+static int validate_requested_mode(struct led_netdev_data *trigger_data,
+				   unsigned long mode)
+{
+	return 0;
+}
+
 static ssize_t device_name_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -168,7 +174,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, new_mode = trigger_data->mode;
 	int ret;
 	int bit;
 
@@ -186,12 +192,18 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 		return -EINVAL;
 	}
 
-	cancel_delayed_work_sync(&trigger_data->work);
-
 	if (state)
-		set_bit(bit, &trigger_data->mode);
+		set_bit(bit, &new_mode);
 	else
-		clear_bit(bit, &trigger_data->mode);
+		clear_bit(bit, &new_mode);
+
+	ret = validate_requested_mode(trigger_data, new_mode);
+	if (ret)
+		return ret;
+
+	cancel_delayed_work_sync(&trigger_data->work);
+
+	trigger_data->mode = new_mode;
 
 	set_baseline_state(trigger_data);
 
-- 
2.39.2

[PATCH 06/11] leds: trigger: netdev: add knob to set hw control possible

[Christian Marangi]

Add knob with the new value hw_control in trigger_data struct to
set hw control possible. Useful for future implementation to implement
in set_baseline_state() the required function to set the requested mode
using LEDs hw control ops and in other function to reject set if hw
control is currently active.

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

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 81e0b0083f2f..28c4465a2584 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -51,6 +51,7 @@ struct led_netdev_data {
 
 	unsigned long mode;
 	bool carrier_link_up;
+	bool hw_control;
 };
 
 enum led_trigger_netdev_modes {
@@ -92,7 +93,7 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 }
 
 static int validate_requested_mode(struct led_netdev_data *trigger_data,
-				   unsigned long mode)
+				   unsigned long mode, bool *can_use_hw_control)
 {
 	return 0;
 }
@@ -175,6 +176,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	unsigned long state, new_mode = trigger_data->mode;
+	bool can_use_hw_control = false;
 	int ret;
 	int bit;
 
@@ -197,13 +199,15 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	else
 		clear_bit(bit, &new_mode);
 
-	ret = validate_requested_mode(trigger_data, new_mode);
+	ret = validate_requested_mode(trigger_data, new_mode,
+				      &can_use_hw_control);
 	if (ret)
 		return ret;
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
 	trigger_data->mode = new_mode;
+	trigger_data->hw_control = can_use_hw_control;
 
 	set_baseline_state(trigger_data);
 
-- 
2.39.2

[PATCH 07/11] leds: trigger: netdev: reject interval and device store for hw_control

[Christian Marangi]

Reject interval and device store with hw_control enabled. They are
currently not supported and MUST be empty with hw_control enabled.

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

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 28c4465a2584..8cd876647a27 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -124,6 +124,11 @@ static ssize_t device_name_store(struct device *dev,
 
 	mutex_lock(&trigger_data->lock);
 
+	if (trigger_data->hw_control) {
+		size = -EINVAL;
+		goto out;
+	}
+
 	if (trigger_data->net_dev) {
 		dev_put(trigger_data->net_dev);
 		trigger_data->net_dev = NULL;
@@ -145,6 +150,8 @@ static ssize_t device_name_store(struct device *dev,
 	trigger_data->last_activity = 0;
 
 	set_baseline_state(trigger_data);
+
+out:
 	mutex_unlock(&trigger_data->lock);
 
 	return size;
@@ -248,6 +255,13 @@ static ssize_t interval_store(struct device *dev,
 	unsigned long value;
 	int ret;
 
+	mutex_lock(&trigger_data->lock);
+
+	if (trigger_data->hw_control) {
+		size = -EINVAL;
+		goto out;
+	}
+
 	ret = kstrtoul(buf, 0, &value);
 	if (ret)
 		return ret;
@@ -260,6 +274,9 @@ static ssize_t interval_store(struct device *dev,
 		set_baseline_state(trigger_data);	/* resets timer */
 	}
 
+out:
+	mutex_unlock(&trigger_data->lock);
+
 	return size;
 }
 
-- 
2.39.2

[PATCH 08/11] leds: trigger: netdev: add support for LED hw control

[Christian Marangi]

Add support for LED hw control for the netdev trigger.

The trigger on calling set_baseline_state to configure a new mode, will
do various check to verify if hw control can be used for the requested
mode in the validate_requested_mode() function.

It will first check if the LED driver supports hw control for the netdev
trigger, then will check if the requested mode are in the trigger mode
mask and finally will call hw_control_set() to apply the requested mode.

To use such mode, interval MUST be set to the default value and net_dev
MUST be empty. If one of these 2 value are not valid, hw control will
never be used and normal software fallback is used.

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

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 8cd876647a27..61bc19fd0c7a 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -68,6 +68,13 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	int current_brightness;
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
 
+	/* Already validated, hw control is possible with the requested mode */
+	if (trigger_data->hw_control) {
+		led_cdev->hw_control_set(led_cdev, trigger_data->mode);
+
+		return;
+	}
+
 	current_brightness = led_cdev->brightness;
 	if (current_brightness)
 		led_cdev->blink_brightness = current_brightness;
@@ -95,6 +102,51 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 static int validate_requested_mode(struct led_netdev_data *trigger_data,
 				   unsigned long mode, bool *can_use_hw_control)
 {
+	unsigned int interval = atomic_read(&trigger_data->interval);
+	unsigned long hw_supported_mode, hw_mode = 0, sw_mode = 0;
+	struct led_classdev *led_cdev = trigger_data->led_cdev;
+	unsigned long default_interval = msecs_to_jiffies(50);
+	bool force_sw = false;
+	int i, ret;
+
+	hw_supported_mode = led_cdev->trigger_supported_flags_mask;
+
+	/* Check if trigger can use hw control */
+	if (!led_trigger_can_hw_control(led_cdev))
+		force_sw = true;
+
+	/* Compose a list of mode that can run in hw or sw */
+	for (i = 0; i < __TRIGGER_NETDEV_MAX; i++) {
+		/* Skip checking mode not active */
+		if (!test_bit(i, &mode))
+			continue;
+
+		/* net_dev is not supported and must be empty for hw control.
+		 * interval must be set to the default value. Any different
+		 * value is rejected if in hw control.
+		 */
+		if (interval == default_interval && !trigger_data->net_dev &&
+		    !force_sw && test_bit(i, &hw_supported_mode))
+			set_bit(i, &hw_mode);
+		else
+			set_bit(i, &sw_mode);
+	}
+
+	/* We can't run modes handled by both sw and hw */
+	if (sw_mode && hw_mode)
+		return -EINVAL;
+
+	/* Exit early if we are using software fallback */
+	if (sw_mode)
+		return 0;
+
+	/* Check if the requested mode is supported */
+	ret = led_cdev->hw_control_is_supported(led_cdev, hw_mode);
+	if (ret)
+		return ret;
+
+	*can_use_hw_control = true;
+
 	return 0;
 }
 
-- 
2.39.2

[PATCH 09/11] leds: trigger: netdev: init mode if hw control already active

[Christian Marangi]

On netdev trigger activation, hw control may be already active by
default. If this is the case, init the already active mode and set the
bool to hw_control bool to true to reflect the already set mode in the
trigger_data.

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

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 61bc19fd0c7a..27df24e6d559 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -465,6 +465,13 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
 	trigger_data->last_activity = 0;
 
+	/* Check if hw control is active by default on the LED.
+	 * Init already enabled mode in hw control.
+	 */
+	if (led_trigger_can_hw_control(led_cdev) &&
+	    !led_cdev->hw_control_get(led_cdev, &trigger_data->mode))
+		trigger_data->hw_control = true;
+
 	led_set_trigger_data(led_cdev, trigger_data);
 
 	rc = register_netdevice_notifier(&trigger_data->notifier);
-- 
2.39.2

[PATCH 10/11] leds: trigger: netdev: expose netdev trigger modes in linux include

[Christian Marangi]

Expose netdev trigger modes to make them accessible by LED driver that
will support netdev trigger for hw control.

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

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 27df24e6d559..7d01e8becb08 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -54,15 +54,6 @@ struct led_netdev_data {
 	bool hw_control;
 };
 
-enum led_trigger_netdev_modes {
-	TRIGGER_NETDEV_LINK = 0,
-	TRIGGER_NETDEV_TX,
-	TRIGGER_NETDEV_RX,
-
-	/* keep last */
-	__TRIGGER_NETDEV_MAX,
-};
-
 static void set_baseline_state(struct led_netdev_data *trigger_data)
 {
 	int current_brightness;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b9152bff3a96..a55f6c528dce 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -500,6 +500,16 @@ 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 = 0,
+	TRIGGER_NETDEV_TX,
+	TRIGGER_NETDEV_RX,
+
+	/* keep last */
+	__TRIGGER_NETDEV_MAX,
+};
+
 /* Trigger specific functions */
 #ifdef CONFIG_LEDS_TRIGGER_DISK
 void ledtrig_disk_activity(bool write);
-- 
2.39.2

[PATCH 11/11] net: dsa: qca8k: implement hw_control ops

[Christian Marangi]

Implement hw_control ops to drive Switch LEDs based on hardware events.

Netdev trigger is the declared supported trigger for hw control
operation and supports the following mode:
- tx
- rx

When hw_control_set is called, LEDs are set to follow the requested
mode.
Each LEDs will blink at 4Hz by default.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/qca8k-leds.c | 156 +++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
index 1ecb29953600..54d3ff46e00a 100644
--- a/drivers/net/dsa/qca/qca8k-leds.c
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -30,6 +30,43 @@ qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en
 	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 = QCA8K_LED_PHY4_CONTROL_RULE_SHIFT;
+	else
+		reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
+
+	return 0;
+}
+
+static int
+qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger)
+{
+	/* Parsing specific to netdev trigger */
+	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 (rules && !*offload_trigger)
+		return -EOPNOTSUPP;
+
+	/* Enable some default rule by default to the requested mode:
+	 * - Blink at 4Hz by default
+	 */
+	*offload_trigger |= QCA8K_LED_BLINK_4HZ;
+
+	return 0;
+}
+
 static int
 qca8k_led_brightness_set(struct qca8k_led *led,
 			 enum led_brightness brightness)
@@ -135,6 +172,119 @@ qca8k_cled_blink_set(struct led_classdev *ldev,
 	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 mask, 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;
+
+	if (led->port_num == 0 || led->port_num == 4) {
+		mask = QCA8K_LED_PATTERN_EN_MASK;
+		val <<= QCA8K_LED_PATTERN_EN_SHIFT;
+	} else {
+		mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
+	}
+
+	return regmap_update_bits(priv->regmap, reg_info.reg, mask << 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);
+
+	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;
+
+	if (led->port_num == 0 || led->port_num == 4) {
+		val &= QCA8K_LED_PATTERN_EN_MASK;
+		val >>= QCA8K_LED_PATTERN_EN_SHIFT;
+	} else {
+		val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
+	}
+
+	return val == QCA8K_LED_RULE_CONTROLLED;
+}
+
+static int
+qca8k_cled_hw_control_is_supported(struct led_classdev *ldev, unsigned long rules)
+{
+	u32 offload_trigger = 0;
+
+	return qca8k_parse_netdev(rules, &offload_trigger);
+}
+
+static int
+qca8k_cled_hw_control_set(struct led_classdev *ldev, unsigned long rules)
+{
+	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 offload_trigger = 0;
+	int ret;
+
+	ret = qca8k_parse_netdev(rules, &offload_trigger);
+	if (ret)
+		return ret;
+
+	ret = qca8k_cled_trigger_offload(ldev, true);
+	if (ret)
+		return ret;
+
+	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
+
+	return regmap_update_bits(priv->regmap, reg_info.reg,
+				  QCA8K_LED_RULE_MASK << reg_info.shift,
+				  offload_trigger << reg_info.shift);
+}
+
+static int
+qca8k_cled_hw_control_get(struct led_classdev *ldev, unsigned long *rules)
+{
+	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;
+	int ret;
+
+	/* With hw control not active return err */
+	if (!qca8k_cled_hw_control_status(ldev))
+		return -EINVAL;
+
+	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
+
+	ret = regmap_read(priv->regmap, reg_info.reg, &val);
+	if (ret)
+		return ret;
+
+	val >>= reg_info.shift;
+	val &= QCA8K_LED_RULE_MASK;
+
+	/* Parsing specific to netdev trigger */
+	if (val & QCA8K_LED_TX_BLINK_MASK)
+		set_bit(TRIGGER_NETDEV_TX, rules);
+	if (val & QCA8K_LED_RX_BLINK_MASK)
+		set_bit(TRIGGER_NETDEV_RX, rules);
+
+	return 0;
+}
+
 static int
 qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
 {
@@ -193,6 +343,12 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
 		port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
 		port_led->cdev.brightness_get = qca8k_cled_brightness_get;
 		port_led->cdev.blink_set = qca8k_cled_blink_set;
+		port_led->cdev.hw_control_is_supported = qca8k_cled_hw_control_is_supported;
+		port_led->cdev.hw_control_set = qca8k_cled_hw_control_set;
+		port_led->cdev.hw_control_get = qca8k_cled_hw_control_get;
+		port_led->cdev.hw_control_trigger = "netdev";
+		port_led->cdev.trigger_supported_flags_mask = BIT(TRIGGER_NETDEV_TX) |
+							      BIT(TRIGGER_NETDEV_RX);
 		init_data.default_label = ":port";
 		init_data.devicename = "qca8k";
 		init_data.fwnode = led;
-- 
2.39.2

Re: [PATCH 08/11] leds: trigger: netdev: add support for LED hw control

[Andrew Lunn]

On Thu, Apr 27, 2023 at 02:15:38AM +0200, Christian Marangi wrote:
> Add support for LED hw control for the netdev trigger.
> 
> The trigger on calling set_baseline_state to configure a new mode, will
> do various check to verify if hw control can be used for the requested
> mode in the validate_requested_mode() function.
> 
> It will first check if the LED driver supports hw control for the netdev
> trigger, then will check if the requested mode are in the trigger mode
> mask and finally will call hw_control_set() to apply the requested mode.
> 
> To use such mode, interval MUST be set to the default value and net_dev
> MUST be empty. If one of these 2 value are not valid, hw control will
> never be used and normal software fallback is used.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 52 +++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 8cd876647a27..61bc19fd0c7a 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -68,6 +68,13 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
>  	int current_brightness;
>  	struct led_classdev *led_cdev = trigger_data->led_cdev;
>  
> +	/* Already validated, hw control is possible with the requested mode */
> +	if (trigger_data->hw_control) {
> +		led_cdev->hw_control_set(led_cdev, trigger_data->mode);
> +
> +		return;
> +	}
> +
>  	current_brightness = led_cdev->brightness;
>  	if (current_brightness)
>  		led_cdev->blink_brightness = current_brightness;
> @@ -95,6 +102,51 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
>  static int validate_requested_mode(struct led_netdev_data *trigger_data,
>  				   unsigned long mode, bool *can_use_hw_control)
>  {
> +	unsigned int interval = atomic_read(&trigger_data->interval);
> +	unsigned long hw_supported_mode, hw_mode = 0, sw_mode = 0;
> +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> +	unsigned long default_interval = msecs_to_jiffies(50);
> +	bool force_sw = false;
> +	int i, ret;
> +
> +	hw_supported_mode = led_cdev->trigger_supported_flags_mask;
> +

> +		if (interval == default_interval && !trigger_data->net_dev &&
> +		    !force_sw && test_bit(i, &hw_supported_mode))
> +			set_bit(i, &hw_mode);
> +		else
> +			set_bit(i, &sw_mode);
> +	}
> +

> +	/* Check if the requested mode is supported */
> +	ret = led_cdev->hw_control_is_supported(led_cdev, hw_mode);
> +	if (ret)
> +		return ret;

Hi Christian

What is the purpose of led_cdev->trigger_supported_flags_mask? I don't
see why it is needed when you are also going to ask the PHY if it can
support the specific blink pattern the user is requesting.

The problem i have with the Marvell PHY, and other PHYs i've looked at
datasheets for, is that hardware does not work like this. It has a
collection of blinking modes, which are a mixture of link speeds, rx
activity, and tx activity. It supports just a subset of all
possibilities.

I think this function can be simplified. Simply ask the LED via
hw_control_is_supported() does it support this mode. If yes, offload
it, if not use software blinking.

    Andrew

Re: [PATCH 02/11] leds: add binding to check support for LED hw control

[Andrew Lunn]

> +	/*
> +	 * Check if the LED driver supports the requested mode provided by the
> +	 * defined supported trigger to setup the LED to hw control mode.
> +	 */
> +	int			(*hw_control_is_supported)(struct led_classdev *led_cdev,
> +							   unsigned long flags);

Hi Christian

This needs better documentation. What is the expected return value? My
initial implementation for the Marvell driver did not work. I returned
-EINVAL if it was not supported and some value >= 0 if it was
supported. And most times, it was > 0, not 0.

However, when i look at the trigger code:

	/* Check if the requested mode is supported */
	ret = led_cdev->hw_control_is_supported(led_cdev, hw_mode);
	if (ret)
		return ret;

	*can_use_hw_control = true;

Anything other than 0 means it is not supported.

	 Andrew

Re: [PATCH 05/11] leds: trigger: netdev: introduce validating requested mode

[Andrew Lunn]

> @@ -168,7 +174,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, new_mode = trigger_data->mode;
>  	int ret;
>  	int bit;
>  
> @@ -186,12 +192,18 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  		return -EINVAL;
>  	}
>  
> -	cancel_delayed_work_sync(&trigger_data->work);
> -
>  	if (state)
> -		set_bit(bit, &trigger_data->mode);
> +		set_bit(bit, &new_mode);
>  	else
> -		clear_bit(bit, &trigger_data->mode);
> +		clear_bit(bit, &new_mode);
> +
> +	ret = validate_requested_mode(trigger_data, new_mode);
> +	if (ret)
> +		return ret;
> +
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
> +	trigger_data->mode = new_mode;
>  
>  	set_baseline_state(trigger_data);

I think you need to hold the trigger_data lock here, otherwise there
are potential race conditions.

    Andrew

Re: [PATCH 08/11] leds: trigger: netdev: add support for LED hw control

[Christian Marangi]

On Sun, Apr 30, 2023 at 07:55:13PM +0200, Andrew Lunn wrote:
> On Thu, Apr 27, 2023 at 02:15:38AM +0200, Christian Marangi wrote:
> > Add support for LED hw control for the netdev trigger.
> > 
> > The trigger on calling set_baseline_state to configure a new mode, will
> > do various check to verify if hw control can be used for the requested
> > mode in the validate_requested_mode() function.
> > 
> > It will first check if the LED driver supports hw control for the netdev
> > trigger, then will check if the requested mode are in the trigger mode
> > mask and finally will call hw_control_set() to apply the requested mode.
> > 
> > To use such mode, interval MUST be set to the default value and net_dev
> > MUST be empty. If one of these 2 value are not valid, hw control will
> > never be used and normal software fallback is used.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 52 +++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index 8cd876647a27..61bc19fd0c7a 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -68,6 +68,13 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  	int current_brightness;
> >  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> >  
> > +	/* Already validated, hw control is possible with the requested mode */
> > +	if (trigger_data->hw_control) {
> > +		led_cdev->hw_control_set(led_cdev, trigger_data->mode);
> > +
> > +		return;
> > +	}
> > +
> >  	current_brightness = led_cdev->brightness;
> >  	if (current_brightness)
> >  		led_cdev->blink_brightness = current_brightness;
> > @@ -95,6 +102,51 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  static int validate_requested_mode(struct led_netdev_data *trigger_data,
> >  				   unsigned long mode, bool *can_use_hw_control)
> >  {
> > +	unsigned int interval = atomic_read(&trigger_data->interval);
> > +	unsigned long hw_supported_mode, hw_mode = 0, sw_mode = 0;
> > +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > +	unsigned long default_interval = msecs_to_jiffies(50);
> > +	bool force_sw = false;
> > +	int i, ret;
> > +
> > +	hw_supported_mode = led_cdev->trigger_supported_flags_mask;
> > +
> 
> > +		if (interval == default_interval && !trigger_data->net_dev &&
> > +		    !force_sw && test_bit(i, &hw_supported_mode))
> > +			set_bit(i, &hw_mode);
> > +		else
> > +			set_bit(i, &sw_mode);
> > +	}
> > +
> 
> > +	/* Check if the requested mode is supported */
> > +	ret = led_cdev->hw_control_is_supported(led_cdev, hw_mode);
> > +	if (ret)
> > +		return ret;
> 
> Hi Christian
> 
> What is the purpose of led_cdev->trigger_supported_flags_mask? I don't
> see why it is needed when you are also going to ask the PHY if it can
> support the specific blink pattern the user is requesting.

The idea is to have a place where a trigger can quickly check the single
mode supported before the entire mode map is validated, but I understand
that this can totally be dropped with some extra code from both trigger
and LED driver.

While refactoring the netdev triger mode validation I notice it was very
handy and simplified the check logic having a mask of the single mode
supported. But this might be not needed for now and we can think of a
better approach later when we will introduce hardware only modes.

> 
> The problem i have with the Marvell PHY, and other PHYs i've looked at
> datasheets for, is that hardware does not work like this. It has a
> collection of blinking modes, which are a mixture of link speeds, rx
> activity, and tx activity. It supports just a subset of all
> possibilities.
> 
> I think this function can be simplified. Simply ask the LED via
> hw_control_is_supported() does it support this mode. If yes, offload
> it, if not use software blinking.

Yep, I will consider dropping it to slim this series even further.

> 
>     Andrew

-- 
	Ansuel

Re: [PATCH 07/11] leds: trigger: netdev: reject interval and device store for hw_control

[Sascha Hauer]

Hi Christian,

On Thu, Apr 27, 2023 at 02:15:37AM +0200, Christian Marangi wrote:
> Reject interval and device store with hw_control enabled. They are
> currently not supported and MUST be empty with hw_control enabled.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 28c4465a2584..8cd876647a27 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -248,6 +255,13 @@ static ssize_t interval_store(struct device *dev,
>  	unsigned long value;
>  	int ret;
>  
> +	mutex_lock(&trigger_data->lock);
> +
> +	if (trigger_data->hw_control) {
> +		size = -EINVAL;
> +		goto out;
> +	}
> +
>  	ret = kstrtoul(buf, 0, &value);
>  	if (ret)
>  		return ret;

You return with the mutex held here. You could do the kstrtoul() before
acquiring the mutex.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 00/11] leds: introduce new LED hw control APIs

[Sascha Hauer]

Hi Christian,

On Thu, Apr 27, 2023 at 02:15:30AM +0200, Christian Marangi wrote:
> This is a continue of [1]. It was decided to take a more gradual
> approach to implement LEDs support for switch and phy starting with
> basic support and then implementing the hw control part when we have all
> the prereq done.

I tried to apply this series to give it a try. To what tree should this
series be applied upon?

> [1] https://lore.kernel.org/lkml/20230216013230.22978-1-ansuelsmth@gmail.com/
> 
> Changes from previous v8 series:
> - Rewrite Documentation from scratch and move to separate commit
> - Strip additional trigger modes (to propose in a different series)
> - Strip from qca8k driver additional modes (to implement in the different
>   series)
> - Split the netdev chages to smaller piece to permit easier review

The changelog reads as if it should be applied instead of v8, but this
series doesn't apply on a vanilla kernel. For example, TRIGGER_NETDEV_TX
is moved around in this series, but not present in vanilla Linux.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 00/11] leds: introduce new LED hw control APIs

[Christian Marangi]

On Mon, May 08, 2023 at 02:25:14PM +0200, Sascha Hauer wrote:
> Hi Christian,
> 
> On Thu, Apr 27, 2023 at 02:15:30AM +0200, Christian Marangi wrote:
> > This is a continue of [1]. It was decided to take a more gradual
> > approach to implement LEDs support for switch and phy starting with
> > basic support and then implementing the hw control part when we have all
> > the prereq done.
> 
> I tried to apply this series to give it a try. To what tree should this
> series be applied upon?
>

Hi,
since this feature affect multiple branch, the prereq of this branch are
still not in linux-next. (the prereq series got accepted but still has
to be merged)

Lee created a branch.

We are waiting for RC stage to request a stable branch so we can
reference ti to correctly test this.

Anyway you should be able to apply this series on top of this branch [1]

Consider that a v2 is almost ready with some crucial changes that should
improve the implementation. (so if you are planning on adding support
for other device I advice to check also v2, just an additional ops to
implement)

[1] https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/log/?h=for-leds-next-next
[2] https://github.com/Ansuel/linux/commits/leds-offload-support-reduced

> > [1] https://lore.kernel.org/lkml/20230216013230.22978-1-ansuelsmth@gmail.com/
> > 
> > Changes from previous v8 series:
> > - Rewrite Documentation from scratch and move to separate commit
> > - Strip additional trigger modes (to propose in a different series)
> > - Strip from qca8k driver additional modes (to implement in the different
> >   series)
> > - Split the netdev chages to smaller piece to permit easier review
> 
> The changelog reads as if it should be applied instead of v8, but this
> series doesn't apply on a vanilla kernel. For example, TRIGGER_NETDEV_TX
> is moved around in this series, but not present in vanilla Linux.
> 

> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
	Ansuel

Re: [PATCH 00/11] leds: introduce new LED hw control APIs

[Sascha Hauer]

On Mon, May 08, 2023 at 02:33:25PM +0200, Christian Marangi wrote:
> On Mon, May 08, 2023 at 02:25:14PM +0200, Sascha Hauer wrote:
> > Hi Christian,
> > 
> > On Thu, Apr 27, 2023 at 02:15:30AM +0200, Christian Marangi wrote:
> > > This is a continue of [1]. It was decided to take a more gradual
> > > approach to implement LEDs support for switch and phy starting with
> > > basic support and then implementing the hw control part when we have all
> > > the prereq done.
> > 
> > I tried to apply this series to give it a try. To what tree should this
> > series be applied upon?
> >
> 
> Hi,
> since this feature affect multiple branch, the prereq of this branch are
> still not in linux-next. (the prereq series got accepted but still has
> to be merged)
> 
> Lee created a branch.

Ok, this explains it, thanks.

> 
> We are waiting for RC stage to request a stable branch so we can
> reference ti to correctly test this.
> 
> Anyway you should be able to apply this series on top of this branch [1]
> 
> Consider that a v2 is almost ready with some crucial changes that should
> improve the implementation. (so if you are planning on adding support
> for other device I advice to check also v2, just an additional ops to
> implement)

I'll wait for v2 then. My ultimate goal is to implement LED trigger support
for the DP83867 phy. It would be great if you could Cc me on v2 so I get
a trigger once it's out.

Thanks for working on this topic. It pops up here every once in a while
and it would be good to get it solved.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 00/11] leds: introduce new LED hw control APIs

[Andrew Lunn]

> I'll wait for v2 then. My ultimate goal is to implement LED trigger support
> for the DP83867 phy. It would be great if you could Cc me on v2 so I get
> a trigger once it's out.

Hi Sascha

Full hardware offload is going to take a few patch sets. The v2 to be
posted soon gives basic support. It is will be missing linking the PHY
LED to the netdev. We have patches for that. And then there is a DT
binding, which again we have patches for. It could also be my Marvell
PHY patches are a separate series. You might want those to get an
example for your DP83867 work.

I'm hoping we can move faster than last cycles, there is less LED and
more networking, so we might be closer to 3 day review cycles than 2
weeks.

	Andrew

Re: [PATCH 04/11] Documentation: leds: leds-class: Document new Hardware driven LEDs APIs

[Bagas Sanjaya]

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

On Thu, Apr 27, 2023 at 02:15:34AM +0200, Christian Marangi wrote:
> +     - hw_control_set:
> +                activate hw control, LED driver will use the provided
                   "activate hw control. LED driver will use ..."
> +                flags passed from the supported trigger, parse them to
> +                a set of mode and setup the LED to be driven by hardware
> +                following the requested modes.
> +
> +                Set LED_OFF via the brightness_set to deactivate hw control.
> +
> +    - hw_control_get:
> +                get from a LED already in hw control, the active modes,
                   "get active modes from a LED already in hw control ..."
> +                parse them and set in flags the current active flags for
> +                the supported trigger.
> +
Thanks.

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

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