[RFC PATCH v3 0/8] Adds support for PHY LEDs with offload triggers

[RFC PATCH v3 0/8] Adds support for PHY LEDs with offload triggers

[Ansuel Smith]

This is another attempt in adding support for PHY LEDs. 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 5 API (hw_control_status/start/stop/configure).
  They are used to put the LED in offload mode and to configure the
  various trigger.
- We have hardware triggers that are used to expose to userspace the
  supported offload triggers and set the offload mode on trigger
  activation.
- We can also have triggers that both support hardware and software mode.
- The LED driver will declare each supported hardware trigger and
  communicate with the trigger all the supported blink modes that will
  be available by sysfs.
- The LED driver will set how the hardware mode is activated/disabled and
  will set a configure function to set each supported triggers.
  This function provide 5 different mode enable/disable/read/supported/zero
  that will enable or disable the offload trigger, read the current status,
  check if supported or reset any active blink mode.
- On hardware trigger activation, only the hardware mode is enabled but
  the blink modes are not configured. This means that the LED will
  run in hardware mode but will have the default rules/event set by the
  device by default. To change this the user will have to operate via
  userspace (or we can consider adding another binding in the dts to
  declare also a default trigger configuration)

Each LED driver will have to declare explicit support for the offload
trigger (or return not supported error code) as we pass a flag 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.

I also posted the netdev trigger expanded with the hardware support.

More polish is required but this is just to understand if I'm taking
the correct path with this implementation.

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.

Ansuel Smith (8):
  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: add hardware control support
  leds: trigger: add hardware-phy-activity trigger
  net: dsa: qca8k: add LEDs support
  dt-bindings: net: dsa: qca8k: add LEDs definition example

 .../devicetree/bindings/net/dsa/qca8k.yaml    |  20 +
 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/Kconfig                  |  28 ++
 drivers/leds/trigger/Makefile                 |   1 +
 .../trigger/ledtrig-hardware-phy-activity.c   | 171 +++++++
 drivers/leds/trigger/ledtrig-netdev.c         | 108 +++--
 drivers/net/dsa/Kconfig                       |   9 +
 drivers/net/dsa/Makefile                      |   1 +
 drivers/net/dsa/qca8k-leds.c                  | 429 ++++++++++++++++++
 drivers/net/dsa/qca8k.c                       |   8 +-
 drivers/net/dsa/qca8k.h                       |  65 +++
 include/linux/leds.h                          | 115 ++++-
 15 files changed, 1045 insertions(+), 30 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-hardware-phy-activity.c
 create mode 100644 drivers/net/dsa/qca8k-leds.c

-- 
2.32.0

[RFC PATCH v3 1/8] leds: add support for hardware driven LEDs

[Ansuel Smith]

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:
- trigger_offload_status(): This asks the LED driver if offload 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.
- trigger_offload_start(): This will simply enable the offload mode for
    the LED.
    With this not declared and trigger_offload_status() returning true,
    it's assumed that the LED is always in offload mode.
- trigger_offload_stop(): This will simply disable the offload mode for
    the LED.
    With this not declared and trigger_offload_status() returning true,
    it's assumed that the LED is always in offload 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 trigger_offload_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: Ansuel Smith <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 ed800f5da7d8..bd2b19cc77ec 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 f4bb02f6e042..1df782ecac66 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..cf0c6005c297 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 is task is 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.32.0

Re: [RFC PATCH v3 1/8] leds: add support for hardware driven LEDs

[Randy Dunlap]

On 11/8/21 6:26 PM, Ansuel Smith wrote:
> 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:

                                  APIs:

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

                     for the driver

> +    enforcerd and putting the LED off is also accepted.

        enforced

> +
> +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 ed800f5da7d8..bd2b19cc77ec 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

	              enables                                  LEDs

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


-- 
~Randy

Re: [RFC PATCH v3 1/8] leds: add support for hardware driven LEDs

[kernel test robot]

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

Hi Ansuel,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on net/master]
[also build test ERROR on linus/master next-20211109]
[cannot apply to pavel-leds/for-next robh/for-next net-next/master v5.15]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ansuel-Smith/Adds-support-for-PHY-LEDs-with-offload-triggers/20211109-102721
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git c45231a7668d6b632534f692b10592ea375b55b0
config: nios2-defconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/3c1a17a4c47130a1b03b57bc11b643bcb62d088c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ansuel-Smith/Adds-support-for-PHY-LEDs-with-offload-triggers/20211109-102721
        git checkout 3c1a17a4c47130a1b03b57bc11b643bcb62d088c
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nios2 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/leds/led-triggers.c: In function 'led_trigger_set':
>> drivers/leds/led-triggers.c:205:29: error: 'struct led_classdev' has no member named 'hw_control_status'
     205 |                     led_cdev->hw_control_status(led_cdev))
         |                             ^~
>> drivers/leds/led-triggers.c:206:33: error: 'struct led_classdev' has no member named 'hw_control_stop'
     206 |                         led_cdev->hw_control_stop(led_cdev);
         |                                 ^~


vim +205 drivers/leds/led-triggers.c

   177	
   178	/* Caller must ensure led_cdev->trigger_lock held */
   179	int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
   180	{
   181		char *event = NULL;
   182		char *envp[2];
   183		const char *name;
   184		int ret;
   185	
   186		if (!led_cdev->trigger && !trig)
   187			return 0;
   188	
   189		name = trig ? trig->name : "none";
   190		event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
   191	
   192		/* Remove any existing trigger */
   193		if (led_cdev->trigger) {
   194			spin_lock(&led_cdev->trigger->leddev_list_lock);
   195			list_del_rcu(&led_cdev->trig_list);
   196			spin_unlock(&led_cdev->trigger->leddev_list_lock);
   197	
   198			/* ensure it's no longer visible on the led_cdevs list */
   199			synchronize_rcu();
   200	
   201			cancel_work_sync(&led_cdev->set_brightness_work);
   202			led_stop_software_blink(led_cdev);
   203			/* Disable hardware mode on trigger change if supported */
   204			if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
 > 205			    led_cdev->hw_control_status(led_cdev))
 > 206				led_cdev->hw_control_stop(led_cdev);
   207			if (led_cdev->trigger->deactivate)
   208				led_cdev->trigger->deactivate(led_cdev);
   209			device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
   210			led_cdev->trigger = NULL;
   211			led_cdev->trigger_data = NULL;
   212			led_cdev->activated = false;
   213			led_set_brightness(led_cdev, LED_OFF);
   214		}
   215		if (trig) {
   216			/* Make sure the trigger support the LED blink mode */
   217			if (!led_trigger_is_supported(led_cdev, trig))
   218				return -EINVAL;
   219	
   220			spin_lock(&trig->leddev_list_lock);
   221			list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
   222			spin_unlock(&trig->leddev_list_lock);
   223			led_cdev->trigger = trig;
   224	
   225			if (trig->activate)
   226				ret = trig->activate(led_cdev);
   227			else
   228				ret = 0;
   229	
   230			if (ret)
   231				goto err_activate;
   232	
   233			ret = device_add_groups(led_cdev->dev, trig->groups);
   234			if (ret) {
   235				dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
   236				goto err_add_groups;
   237			}
   238		}
   239	
   240		if (event) {
   241			envp[0] = event;
   242			envp[1] = NULL;
   243			if (kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp))
   244				dev_err(led_cdev->dev,
   245					"%s: Error sending uevent\n", __func__);
   246			kfree(event);
   247		}
   248	
   249		return 0;
   250	
   251	err_add_groups:
   252	
   253		if (trig->deactivate)
   254			trig->deactivate(led_cdev);
   255	err_activate:
   256	
   257		spin_lock(&led_cdev->trigger->leddev_list_lock);
   258		list_del_rcu(&led_cdev->trig_list);
   259		spin_unlock(&led_cdev->trigger->leddev_list_lock);
   260		synchronize_rcu();
   261		led_cdev->trigger = NULL;
   262		led_cdev->trigger_data = NULL;
   263		led_set_brightness(led_cdev, LED_OFF);
   264		kfree(event);
   265	
   266		return ret;
   267	}
   268	EXPORT_SYMBOL_GPL(led_trigger_set);
   269	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 10326 bytes --]

Re: [RFC PATCH v3 1/8] leds: add support for hardware driven LEDs

[Andrew Lunn]

On Tue, Nov 09, 2021 at 03:26:01AM +0100, Ansuel Smith wrote:
> 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:
> - trigger_offload_status(): This asks the LED driver if offload 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.

I don't understand this last part. The LED controller is not
implementing software mode, other than providing a method to manually
turn the LED on and off. And there is a well defined call for that. If
that call is a NULL, it is clear it is not implemented. There is no
need to ask the driver.

     Andrew

Re: [RFC PATCH v3 1/8] leds: add support for hardware driven LEDs

[Ansuel Smith]

On Tue, Nov 09, 2021 at 09:34:23PM +0100, Andrew Lunn wrote:
> On Tue, Nov 09, 2021 at 03:26:01AM +0100, Ansuel Smith wrote:
> > 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:
> > - trigger_offload_status(): This asks the LED driver if offload 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.
> 
> I don't understand this last part. The LED controller is not
> implementing software mode, other than providing a method to manually
> turn the LED on and off. And there is a well defined call for that. If
> that call is a NULL, it is clear it is not implemented. There is no
> need to ask the driver.
> 
>      Andrew

You are right I have to remove the last part as it doesn't make sense.
We already use blink_mode. Will remove in v4.

-- 
	Ansuel

[RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED

[Ansuel Smith]

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: Ansuel Smith <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 cf0c6005c297..00bc4d6ed7ca 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.32.0

Re: [RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED

[Marek Behún]

Hello Ansuel,

On Tue,  9 Nov 2021 03:26:02 +0100
Ansuel Smith <ansuelsmth@gmail.com> wrote:

> 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: Ansuel Smith <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 cf0c6005c297..00bc4d6ed7ca 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

this is a strange proposal for the API.

Anyway, led_classdev already has the blink_set() method, which is documented as
	/*
	  * Activate hardware accelerated blink, delays are in milliseconds
	  * and if both are zero then a sensible default should be chosen.
	  * The call should adjust the timings in that case and if it can't
	  * match the values specified exactly.
	  * Deactivate blinking again when the brightness is set to LED_OFF
	  * via the brightness_set() callback.
	  */
	int		(*blink_set)(struct led_classdev *led_cdev,
				     unsigned long *delay_on,
				     unsigned long *delay_off);

So we already have a method to set hardware blkinking, we don't need
another one.

Marek

Re: [RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED

[Ansuel Smith]

On Tue, Nov 09, 2021 at 04:01:03AM +0100, Marek Behún wrote:
> Hello Ansuel,
> 
> On Tue,  9 Nov 2021 03:26:02 +0100
> Ansuel Smith <ansuelsmth@gmail.com> wrote:
> 
> > 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: Ansuel Smith <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 cf0c6005c297..00bc4d6ed7ca 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
> 
> this is a strange proposal for the API.
> 
> Anyway, led_classdev already has the blink_set() method, which is documented as
> 	/*
> 	  * Activate hardware accelerated blink, delays are in milliseconds
> 	  * and if both are zero then a sensible default should be chosen.
> 	  * The call should adjust the timings in that case and if it can't
> 	  * match the values specified exactly.
> 	  * Deactivate blinking again when the brightness is set to LED_OFF
> 	  * via the brightness_set() callback.
> 	  */
> 	int		(*blink_set)(struct led_classdev *led_cdev,
> 				     unsigned long *delay_on,
> 				     unsigned long *delay_off);
> 
> So we already have a method to set hardware blkinking, we don't need
> another one.
> 
> Marek

But that is about hardware blink, not a LED controlled by hardware based
on some rules/modes.
Doesn't really match the use for the hardware control.
Blink_set makes the LED blink contantly at the declared delay.
The blink_mode_cmd are used to request stuff to a LED in hardware mode.

Doesn't seem correct to change/enhance the blink_set function with
something that would do something completely different.

-- 
	Ansuel

Re: [RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED

[Andrew Lunn]

> > > +#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
> > 
> > this is a strange proposal for the API.
> > 
> > Anyway, led_classdev already has the blink_set() method, which is documented as
> > 	/*
> > 	  * Activate hardware accelerated blink, delays are in milliseconds
> > 	  * and if both are zero then a sensible default should be chosen.
> > 	  * The call should adjust the timings in that case and if it can't
> > 	  * match the values specified exactly.
> > 	  * Deactivate blinking again when the brightness is set to LED_OFF
> > 	  * via the brightness_set() callback.
> > 	  */
> > 	int		(*blink_set)(struct led_classdev *led_cdev,
> > 				     unsigned long *delay_on,
> > 				     unsigned long *delay_off);
> > 
> > So we already have a method to set hardware blkinking, we don't need
> > another one.
> > 
> > Marek
> 
> But that is about hardware blink, not a LED controlled by hardware based
> on some rules/modes.
> Doesn't really match the use for the hardware control.
> Blink_set makes the LED blink contantly at the declared delay.
> The blink_mode_cmd are used to request stuff to a LED in hardware mode.
> 
> Doesn't seem correct to change/enhance the blink_set function with
> something that would do something completely different.

Humm. I can see merits for both.

What i like about reusing blink_set() is that it is well understood.
There is a defined sysfs API for it. ledtrig-oneshot.c also uses it,
for a non-repeating blink. So i think that also fits the PHY LED use
case.

	Andrew

Re: [RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED

[Ansuel Smith]

On Tue, Nov 09, 2021 at 09:49:35PM +0100, Andrew Lunn wrote:
> > > > +#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
> > > 
> > > this is a strange proposal for the API.
> > > 
> > > Anyway, led_classdev already has the blink_set() method, which is documented as
> > > 	/*
> > > 	  * Activate hardware accelerated blink, delays are in milliseconds
> > > 	  * and if both are zero then a sensible default should be chosen.
> > > 	  * The call should adjust the timings in that case and if it can't
> > > 	  * match the values specified exactly.
> > > 	  * Deactivate blinking again when the brightness is set to LED_OFF
> > > 	  * via the brightness_set() callback.
> > > 	  */
> > > 	int		(*blink_set)(struct led_classdev *led_cdev,
> > > 				     unsigned long *delay_on,
> > > 				     unsigned long *delay_off);
> > > 
> > > So we already have a method to set hardware blkinking, we don't need
> > > another one.
> > > 
> > > Marek
> > 
> > But that is about hardware blink, not a LED controlled by hardware based
> > on some rules/modes.
> > Doesn't really match the use for the hardware control.
> > Blink_set makes the LED blink contantly at the declared delay.
> > The blink_mode_cmd are used to request stuff to a LED in hardware mode.
> > 
> > Doesn't seem correct to change/enhance the blink_set function with
> > something that would do something completely different.
> 
> Humm. I can see merits for both.
> 
> What i like about reusing blink_set() is that it is well understood.
> There is a defined sysfs API for it. ledtrig-oneshot.c also uses it,
> for a non-repeating blink. So i think that also fits the PHY LED use
> case.
> 
> 	Andrew

If we should reuse blink_set to control hw blink I need to understand 2
thing.

The idea to implement the function hw_control_configure was to provide
the triggers some way to configure the various blink_mode. (and by using
the cmd enum provide different info based on the return value)

The advised path from Marek is to make the changes in the trigger_data
and the LED driver will then use that to configure blink mode.

I need to call an example to explain my concern:
qca8k switch. Can both run in hardware mode and software mode (by
controlling the reg to trigger manual blink) and also there is an extra
mode to blink permanently at 4hz.

Now someone would declare the brightness_set to control the led
manually and blink_set (for the permanent 4hz blink feature)
So that's where my idea comes about introducting another function and
the fact that it wouldn't match that well with blink_set with some LED.

I mean if we really want to use blink_set also for this(hw_control), we
can consider adding a bool to understand when hw_control is active or not.
So blink_set can be used for both feature.

Is that acceptable?

Also if we want to use blink_set I think we will have to drop all the
cmd mess and remove some error handling. Don't like that but if that's
what is needed for the feature, it's ok for me.

-- 
	Ansuel

Re: [RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED

[Andrew Lunn]

> If we should reuse blink_set to control hw blink I need to understand 2
> thing.
> 
> The idea to implement the function hw_control_configure was to provide
> the triggers some way to configure the various blink_mode. (and by using
> the cmd enum provide different info based on the return value)
> 
> The advised path from Marek is to make the changes in the trigger_data
> and the LED driver will then use that to configure blink mode.
> 
> I need to call an example to explain my concern:
> qca8k switch. Can both run in hardware mode and software mode (by
> controlling the reg to trigger manual blink) and also there is an extra
> mode to blink permanently at 4hz.
> 
> Now someone would declare the brightness_set to control the led
> manually and blink_set (for the permanent 4hz blink feature)
> So that's where my idea comes about introducting another function and
> the fact that it wouldn't match that well with blink_set with some LED.
> 
> I mean if we really want to use blink_set also for this(hw_control), we
> can consider adding a bool to understand when hw_control is active or not.
> So blink_set can be used for both feature.

I don't see why we need the bool. The driver should know that speeds
it supports. If asked to do something it cannot do in the current mode
it should return either -EINVAL, or maybe -EOPNOTSUPP. Depending on
how to the trigger works, we might want -EOPNOTSUPP when in a hardware
offload mode, which gets returned to user space. If we are in a
software blinking mode -EINVAL, so that the trigger does the blinking
in software.

   Andrew

Re: [RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED

[Marek Behún]

On Tue, 9 Nov 2021 15:22:53 +0100
Ansuel Smith <ansuelsmth@gmail.com> wrote:

> On Tue, Nov 09, 2021 at 04:01:03AM +0100, Marek Behún wrote:
> > Hello Ansuel,
> > 
> > On Tue,  9 Nov 2021 03:26:02 +0100
> > Ansuel Smith <ansuelsmth@gmail.com> wrote:
> >   
> > > 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: Ansuel Smith <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 cf0c6005c297..00bc4d6ed7ca 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  
> > 
> > this is a strange proposal for the API.
> > 
> > Anyway, led_classdev already has the blink_set() method, which is documented as
> > 	/*
> > 	  * Activate hardware accelerated blink, delays are in milliseconds
> > 	  * and if both are zero then a sensible default should be chosen.
> > 	  * The call should adjust the timings in that case and if it can't
> > 	  * match the values specified exactly.
> > 	  * Deactivate blinking again when the brightness is set to LED_OFF
> > 	  * via the brightness_set() callback.
> > 	  */
> > 	int		(*blink_set)(struct led_classdev *led_cdev,
> > 				     unsigned long *delay_on,
> > 				     unsigned long *delay_off);
> > 
> > So we already have a method to set hardware blkinking, we don't need
> > another one.
> > 
> > Marek  
> 
> But that is about hardware blink, not a LED controlled by hardware based
> on some rules/modes.
> Doesn't really match the use for the hardware control.

I think there is a miscommunication, because I don't quite understand
what you are trying to say here.

How is "hardware blink" different from "a LED controlled by hardware
based on some rules/modes", when it is used for blinking ?

If the hardware, any hardware, supports blinking with different
frequencies, it should implement the blink_set() method.

> Blink_set makes the LED blink contantly at the declared delay.
> The blink_mode_cmd are used to request stuff to a LED in hardware mode.
> 
> Doesn't seem correct to change/enhance the blink_set function with
> something that would do something completely different.
> 

Re: [RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED

[Randy Dunlap]

On 11/8/21 6:26 PM, Ansuel Smith wrote:
> 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

Maybe this:
    Once a trigger has declared support for hardware-controller blinks, it will us 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

                                                          changes

> +driver interest know how to elaborate this flag and to declare support for a

    driver's interest to know how to

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


-- 
~Randy

[RFC PATCH v3 3/8] leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode

[Ansuel Smith]

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: Ansuel Smith <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.32.0

Re: [RFC PATCH v3 3/8] leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode

[Marek Behún]

On Tue,  9 Nov 2021 03:26:03 +0100
Ansuel Smith <ansuelsmth@gmail.com> wrote:

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

The last time I tried this, I did it for all the fields that are now in
the bitmap, and I was told that the bitmap guarantees atomic access, so
it should be used...

But why do you needs this? I guess I will see in another patch.

Marek

Re: [RFC PATCH v3 3/8] leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode

[Ansuel Smith]

On Tue, Nov 09, 2021 at 04:02:57AM +0100, Marek Behún wrote:
> On Tue,  9 Nov 2021 03:26:03 +0100
> Ansuel Smith <ansuelsmth@gmail.com> wrote:
> 
> > 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.
> 
> The last time I tried this, I did it for all the fields that are now in
> the bitmap, and I was told that the bitmap guarantees atomic access, so
> it should be used...
> 
> But why do you needs this? I guess I will see in another patch.
>

The link_up seems something internal to the netdev trigger and not
something strictly related to the blink modes. Why put a status in the
mode variable?

> Marek

-- 
	Ansuel

Re: [RFC PATCH v3 3/8] leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode

[Andrew Lunn]

On Tue, Nov 09, 2021 at 04:02:57AM +0100, Marek Behún wrote:
> On Tue,  9 Nov 2021 03:26:03 +0100
> Ansuel Smith <ansuelsmth@gmail.com> wrote:
> 
> > 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.
> 
> The last time I tried this, I did it for all the fields that are now in
> the bitmap, and I was told that the bitmap guarantees atomic access, so
> it should be used...
> 
> But why do you needs this? I guess I will see in another patch.

I agree with Marek here. The commit message says what you have done,
which is not very useful, i can read the patch. What it should include
is why you have made this change. The why is very important in the
commit message.

       Andrew

[RFC PATCH v3 4/8] leds: trigger: netdev: rename and expose NETDEV trigger enum modes

[Ansuel Smith]

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: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 31 ++++++++++++---------------
 include/linux/leds.h                  |  7 ++++++
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 66a81cc9b64d..0a3c0b54dbb9 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -51,9 +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 {
@@ -76,7 +73,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 +82,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);
 	}
 }
@@ -153,13 +150,13 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
 
 	switch (attr) {
 	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
+		bit = TRIGGER_NETDEV_LINK;
 		break;
 	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
+		bit = TRIGGER_NETDEV_TX;
 		break;
 	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+		bit = TRIGGER_NETDEV_RX;
 		break;
 	default:
 		return -EINVAL;
@@ -182,13 +179,13 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 
 	switch (attr) {
 	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
+		bit = TRIGGER_NETDEV_LINK;
 		break;
 	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
+		bit = TRIGGER_NETDEV_TX;
 		break;
 	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+		bit = TRIGGER_NETDEV_RX;
 		break;
 	default:
 		return -EINVAL;
@@ -358,21 +355,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 00bc4d6ed7ca..087debe6716d 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,
+	TRIGGER_NETDEV_TX,
+	TRIGGER_NETDEV_RX,
+};
+
 /* Trigger specific functions */
 #ifdef CONFIG_LEDS_TRIGGER_DISK
 void ledtrig_disk_activity(bool write);
-- 
2.32.0

Re: [RFC PATCH v3 4/8] leds: trigger: netdev: rename and expose NETDEV trigger enum modes

[Andrew Lunn]

On Tue, Nov 09, 2021 at 03:26:04AM +0100, Ansuel Smith wrote:
> Rename NETDEV trigger enum modes to a more simbolic name and move them

symbolic. Randy is slipping :-)

> in leds.h to make them accessible by any user.

any user? I would be more specific than that. Other triggers dealing
with netdev states?

> +++ 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 */

You probably want netdev in the comment above. Things could get
interesting if other ledtrig-*.c started using them.

> +enum led_trigger_netdev_modes {
> +	TRIGGER_NETDEV_LINK,
> +	TRIGGER_NETDEV_TX,
> +	TRIGGER_NETDEV_RX,
> +};
> +
>  /* Trigger specific functions */
>  #ifdef CONFIG_LEDS_TRIGGER_DISK
>  void ledtrig_disk_activity(bool write);
> -- 
> 2.32.0
> 

Re: [RFC PATCH v3 4/8] leds: trigger: netdev: rename and expose NETDEV trigger enum modes

[Ansuel Smith]

On Tue, Nov 09, 2021 at 09:58:27PM +0100, Andrew Lunn wrote:
> On Tue, Nov 09, 2021 at 03:26:04AM +0100, Ansuel Smith wrote:
> > Rename NETDEV trigger enum modes to a more simbolic name and move them
> 
> symbolic. Randy is slipping :-)
> 
> > in leds.h to make them accessible by any user.
> 
> any user? I would be more specific than that. Other triggers dealing
> with netdev states?
>

Ok will be more specific. A LED driver require to explicitly support the
trigger to run in hardware mode. The LED driver will take the
trigger_data and elaborate his struct to parse all the option
(blink_mode bitmap, interval)

So the user would be a LED driver that adds support for that specific
trigger. That is also the reason we need to export them.

> > +++ 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 */
> 
> You probably want netdev in the comment above. Things could get
> interesting if other ledtrig-*.c started using them.
> 
> > +enum led_trigger_netdev_modes {
> > +	TRIGGER_NETDEV_LINK,
> > +	TRIGGER_NETDEV_TX,
> > +	TRIGGER_NETDEV_RX,
> > +};
> > +
> >  /* Trigger specific functions */
> >  #ifdef CONFIG_LEDS_TRIGGER_DISK
> >  void ledtrig_disk_activity(bool write);
> > -- 
> > 2.32.0
> > 

-- 
	Ansuel

Re: [RFC PATCH v3 4/8] leds: trigger: netdev: rename and expose NETDEV trigger enum modes

[Andrew Lunn]

On Wed, Nov 10, 2021 at 08:57:24PM +0100, Ansuel Smith wrote:
> On Tue, Nov 09, 2021 at 09:58:27PM +0100, Andrew Lunn wrote:
> > On Tue, Nov 09, 2021 at 03:26:04AM +0100, Ansuel Smith wrote:
> > > Rename NETDEV trigger enum modes to a more simbolic name and move them
> > 
> > symbolic. Randy is slipping :-)
> > 
> > > in leds.h to make them accessible by any user.
> > 
> > any user? I would be more specific than that. Other triggers dealing
> > with netdev states?
> >
> 
> Ok will be more specific. A LED driver require to explicitly support the
> trigger to run in hardware mode. The LED driver will take the
> trigger_data and elaborate his struct to parse all the option
> (blink_mode bitmap, interval)
> 
> So the user would be a LED driver that adds support for that specific
> trigger. That is also the reason we need to export them.

Say i have a SATA controller where i can configure it to blink on
reads, or writes, or both. It also needs to understand these netdev
modes? I was meaning you need to narrow the comment down to a trigger
which has something to do with netdev. I assume other sorts of
hardware offloads will appear in the future, once the generic
infrastructure is there.

	 Andrew

[RFC PATCH v3 5/8] leds: trigger: netdev: add hardware control support

[Ansuel Smith]

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.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 61 ++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 0a3c0b54dbb9..28d9def2fbd0 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -37,6 +37,7 @@
  */
 
 struct led_netdev_data {
+	bool hw_mode_supported;
 	spinlock_t lock;
 
 	struct delayed_work work;
@@ -61,9 +62,52 @@ enum netdev_led_attr {
 
 static void set_baseline_state(struct led_netdev_data *trigger_data)
 {
+	bool can_offload;
 	int current_brightness;
+	u32 hw_blink_mode_supported;
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
 
+	if (trigger_data->hw_mode_supported) {
+		if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_LINK))
+			hw_blink_mode_supported |= TRIGGER_NETDEV_LINK;
+		if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_TX))
+			hw_blink_mode_supported |= TRIGGER_NETDEV_TX;
+		if (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_RX))
+			hw_blink_mode_supported |= TRIGGER_NETDEV_RX;
+
+		/* All the requested blink mode can be triggered by hardware.
+		 * Put it in hardware mode.
+		 */
+		if (hw_blink_mode_supported == trigger_data->mode)
+			can_offload = true;
+
+		if (can_offload) {
+			/* We are refreshing the blink modes. Reset them */
+			led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
+						       BLINK_MODE_ZERO);
+
+			if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
+				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
+							       BLINK_MODE_ENABLE);
+			if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode))
+				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_TX,
+							       BLINK_MODE_ENABLE);
+			if (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
+				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_RX,
+							       BLINK_MODE_ENABLE);
+			led_cdev->hw_control_start(led_cdev);
+
+			return;
+		}
+	}
+
+	/* If LED supports only hardware mode then skip any software handling */
+	if (led_cdev->blink_mode == HARDWARE_CONTROLLED)
+		return;
+
 	current_brightness = led_cdev->brightness;
 	if (current_brightness)
 		led_cdev->blink_brightness = current_brightness;
@@ -407,13 +451,27 @@ 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) {
+		trigger_data->hw_mode_supported = true;
+
+		/* 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, 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;
 }
 
@@ -433,6 +491,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.32.0

Re: [RFC PATCH v3 5/8] leds: trigger: netdev: add hardware control support

[Marek Behún]

On Tue,  9 Nov 2021 03:26:05 +0100
Ansuel Smith <ansuelsmth@gmail.com> 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.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 61 ++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 0a3c0b54dbb9..28d9def2fbd0 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -37,6 +37,7 @@
>   */
>  
>  struct led_netdev_data {
> +	bool hw_mode_supported;
>  	spinlock_t lock;
>  
>  	struct delayed_work work;
> @@ -61,9 +62,52 @@ enum netdev_led_attr {
>  
>  static void set_baseline_state(struct led_netdev_data *trigger_data)
>  {
> +	bool can_offload;
>  	int current_brightness;
> +	u32 hw_blink_mode_supported;
>  	struct led_classdev *led_cdev = trigger_data->led_cdev;
>  
> +	if (trigger_data->hw_mode_supported) {
> +		if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode) &&
> +		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_LINK))
> +			hw_blink_mode_supported |= TRIGGER_NETDEV_LINK;
> +		if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
> +		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_TX))
> +			hw_blink_mode_supported |= TRIGGER_NETDEV_TX;
> +		if (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) &&
> +		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_RX))
> +			hw_blink_mode_supported |= TRIGGER_NETDEV_RX;
> +
> +		/* All the requested blink mode can be triggered by hardware.
> +		 * Put it in hardware mode.
> +		 */
> +		if (hw_blink_mode_supported == trigger_data->mode)
> +			can_offload = true;
> +
> +		if (can_offload) {
> +			/* We are refreshing the blink modes. Reset them */
> +			led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
> +						       BLINK_MODE_ZERO);
> +
> +			if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
> +				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
> +							       BLINK_MODE_ENABLE);
> +			if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode))
> +				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_TX,
> +							       BLINK_MODE_ENABLE);
> +			if (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
> +				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_RX,
> +							       BLINK_MODE_ENABLE);
> +			led_cdev->hw_control_start(led_cdev);

Please nooo :)
This is exactly what I wanted to avoid, this logic should be in the LED
driver itself.

Can you please at least read my last proposal?

patch 2
https://lore.kernel.org/linux-leds/20210601005155.27997-3-kabel@kernel.org/
adds the trigger_offload() method. This may need to get changed to
trigger_offload_start() and trigger_offload_stop(), as per Andrew's
request.

patch 3
https://lore.kernel.org/linux-leds/20210601005155.27997-4-kabel@kernel.org/
moves the whole struct led_netdev_data to global include file
include/linux/ledtrig-netdev.h

patch 4
https://lore.kernel.org/linux-leds/20210601005155.27997-5-kabel@kernel.org/
makes netdev trigger to try to call the trigger_offload() method.

So after patch 4, netdev trigger calls trigger_offload() methods and
passes itself into it.

Example implementation is then in patch 10 of the series
https://lore.kernel.org/linux-leds/20210601005155.27997-11-kabel@kernel.org/
Look at omnia_led_trig_offload() function.

The benefit of this API is that it is flexible - all existing triggers
an be theoretically offloaded to HW (if there is HW that supports it)
without change to this API, and with minimal changes to the sw
implementations of the triggers.

Could you please at least try it?

I am willing to work with you on this. We can make a conference call
tomorrow, if you are able to.

Marek

Re: [RFC PATCH v3 5/8] leds: trigger: netdev: add hardware control support

[Ansuel Smith]

On Tue, Nov 09, 2021 at 04:12:36AM +0100, Marek Behún wrote:
> On Tue,  9 Nov 2021 03:26:05 +0100
> Ansuel Smith <ansuelsmth@gmail.com> 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.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 61 ++++++++++++++++++++++++++-
> >  1 file changed, 60 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index 0a3c0b54dbb9..28d9def2fbd0 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -37,6 +37,7 @@
> >   */
> >  
> >  struct led_netdev_data {
> > +	bool hw_mode_supported;
> >  	spinlock_t lock;
> >  
> >  	struct delayed_work work;
> > @@ -61,9 +62,52 @@ enum netdev_led_attr {
> >  
> >  static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  {
> > +	bool can_offload;
> >  	int current_brightness;
> > +	u32 hw_blink_mode_supported;
> >  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> >  
> > +	if (trigger_data->hw_mode_supported) {
> > +		if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode) &&
> > +		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_LINK))
> > +			hw_blink_mode_supported |= TRIGGER_NETDEV_LINK;
> > +		if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
> > +		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_TX))
> > +			hw_blink_mode_supported |= TRIGGER_NETDEV_TX;
> > +		if (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) &&
> > +		    led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_RX))
> > +			hw_blink_mode_supported |= TRIGGER_NETDEV_RX;
> > +
> > +		/* All the requested blink mode can be triggered by hardware.
> > +		 * Put it in hardware mode.
> > +		 */
> > +		if (hw_blink_mode_supported == trigger_data->mode)
> > +			can_offload = true;
> > +
> > +		if (can_offload) {
> > +			/* We are refreshing the blink modes. Reset them */
> > +			led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
> > +						       BLINK_MODE_ZERO);
> > +
> > +			if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
> > +				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
> > +							       BLINK_MODE_ENABLE);
> > +			if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode))
> > +				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_TX,
> > +							       BLINK_MODE_ENABLE);
> > +			if (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
> > +				led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_RX,
> > +							       BLINK_MODE_ENABLE);
> > +			led_cdev->hw_control_start(led_cdev);
> 
> Please nooo :)
> This is exactly what I wanted to avoid, this logic should be in the LED
> driver itself.
>

Don't know about this. Honestly I think a better way would be to make
the LED driver very stupid (current way) and just leave to the trigger
how to handle the different case.
My idea is that LED driver should declare feature and do operation and
then the trigger should elaborate them.

> Can you please at least read my last proposal?
> 

About this sorry but I lost track of it. Thanks for posting it here.

> patch 2
> https://lore.kernel.org/linux-leds/20210601005155.27997-3-kabel@kernel.org/
> adds the trigger_offload() method. This may need to get changed to
> trigger_offload_start() and trigger_offload_stop(), as per Andrew's
> request.
> 
> patch 3
> https://lore.kernel.org/linux-leds/20210601005155.27997-4-kabel@kernel.org/
> moves the whole struct led_netdev_data to global include file
> include/linux/ledtrig-netdev.h
> 
> patch 4
> https://lore.kernel.org/linux-leds/20210601005155.27997-5-kabel@kernel.org/
> makes netdev trigger to try to call the trigger_offload() method.
> 
> So after patch 4, netdev trigger calls trigger_offload() methods and
> passes itself into it.
> 
> Example implementation is then in patch 10 of the series
> https://lore.kernel.org/linux-leds/20210601005155.27997-11-kabel@kernel.org/
> Look at omnia_led_trig_offload() function.
> 
> The benefit of this API is that it is flexible - all existing triggers
> an be theoretically offloaded to HW (if there is HW that supports it)
> without change to this API, and with minimal changes to the sw
> implementations of the triggers.

Looking at the patchset the main problem with my implementation is that
a LED driver would benefits by having more data as it can also configure
other stuff. So I see using the logic of using directly the trigger
data instead of some values correct.
What I still don't like is the fact that moving the entire logic to the
LED driver doesn't seems correct. 
We can mix the 2 logic and create an hybrid.
- I would still have some type of direct control from a trigger.
  (We reduce the current cmd enum to something like APPLY, ZERO,
  SUPPORTED) just so the hardware configure part is not a big blackbox
  that do thing and we doesn't really know what happen on any error.
- On the LED driver part, it will use the data from the trigger and
  report what it was asked.

That would change the LED trigger transaction to.
1. Set the bitmap in the trigger data
2. Send supported command (the LED driver do his check and return a
   bool)
3. Send apply command (the LED driver actually apply the config)

Also the trigger will ZERO any blink configuration on init.
(to handle LEDs that configure some blink mode by default in hardware
mode)

The first question would be why have all these extra modes when we can
just use one function like hw_control_setup. Well some other trigger
would benefit from having more control and we would get better error
handling by separating the main operation (supported and apply)

What do you think? This should be even more flexible than before and
would also remove the flag problem (or better saying moving it to
exporting the entire trigger struct)

Also about exporting struct. Should we really create another include or
should we just put it in the generic led include?

> 
> Could you please at least try it?
> 

If you are fine with the idea I have in mind, I will start working on v4
ASAP.

> I am willing to work with you on this. We can make a conference call
> tomorrow, if you are able to.
> 

A bit busy so I work on this on very late night but I think the way we
are comunicating is fine. Lets continue like that.

> Marek

-- 
	Ansuel

[RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Ansuel Smith]

Add Hardware Only Trigger for PHY Activity. This special trigger is used to
configure and expose the different HW trigger that are provided by the
PHY. Each blink mode can be configured by sysfs and on trigger
activation the hardware mode is enabled.

This currently implement these hw triggers:
  - blink_tx: Blink LED on tx packet receive
  - blink_rx: Blink LED on rx packet receive
  - keep_link_10m: Keep LED on with 10m link speed
  - keep_link_100m: Keep LED on with 100m link speed
  - kepp_link_1000m: Keep LED on with 1000m link speed
  - keep_half_duplex: Keep LED on with half duplex link
  - keep_full_duplex: Keep LED on with full duplex link
  - option_linkup_over: Ignore blink tx/rx with link keep not active
  - option_power_on_reset: Keep LED on with switch reset
  - option_blink_2hz: Set blink speed at 2hz for every blink event
  - option_blink_4hz: Set blink speed at 4hz for every blink event
  - option_blink_8hz: Set blink speed at 8hz for every blink event
  - option_blink_auto: Set blink speed at 2hz for 10m link speed,
      4hz for 100m and 8hz for 1000m

The trigger will check if the LED driver support the various blink modes
and will expose the blink modes in sysfs.
It will finally enable hw mode for the LED without configuring any rule.
This mean that the LED will blink/follow whatever offload trigger is
active by default and the user needs to manually configure the desired
offload triggers using sysfs.
A flag is passed to configure_offload with the related rule from this
trigger to active or disable.
It's in the led driver interest the detection and knowing how to
elaborate the passed flags and should report -EOPNOTSUPP otherwise.

The different hw triggers are exposed in the led sysfs dir under the
offload-phy-activity subdir.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/Kconfig                  |  28 +++
 drivers/leds/trigger/Makefile                 |   1 +
 .../trigger/ledtrig-hardware-phy-activity.c   | 171 ++++++++++++++++++
 include/linux/leds.h                          |  22 +++
 4 files changed, 222 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-hardware-phy-activity.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index dc6816d36d06..b947b238be3f 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -154,4 +154,32 @@ config LEDS_TRIGGER_TTY
 
 	  When build as a module this driver will be called ledtrig-tty.
 
+config LEDS_TRIGGER_HARDWARE_PHY_ACTIVITY
+	tristate "LED Trigger for PHY Activity for Hardware Controlled LED"
+	depends on LEDS_HARDWARE_CONTROL
+	help
+	  This allows LEDs to be configured to run by hardware and offloaded
+	  based on some rules. The LED will blink or be on based on the PHY
+	  Activity for example on packet receive or based on the link speed.
+
+	  The current supported offload triggers are:
+	  - blink_tx: Blink LED on tx packet receive
+	  - blink_rx: Blink LED on rx packet receive
+	  - keep_link_10m: Keep LED on with 10m link speed
+	  - keep_link_100m: Keep LED on with 100m link speed
+	  - keep_link_1000m: Keep LED on with 1000m link speed
+	  - keep_half_duplex: Keep LED on with half duplex link
+	  - keep_full_duplex: Keep LED on with full duplex link
+	  - option_linkup_over: Blink rules are ignored with absent link
+	  - option_power_on_reset: Power ON Led on Switch/PHY reset
+	  - option_blink_2hz: Set blink speed at 2hz for every blink event
+	  - option_blink_4hz: Set blink speed at 4hz for every blink event
+	  - option_blink_8hz: Set blink speed at 8hz for every blink event
+
+	  These blink modes are present in the LED sysfs dir under
+	  hardware-phy-activity if supported by the LED driver.
+
+	  This trigger can be used only by LEDs that supports Hardware mode
+
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 25c4db97cdd4..f5d0d0057d2b 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
 obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
+obj-$(CONFIG_LEDS_TRIGGER_HARDWARE_PHY_ACTIVITY) += ledtrig-hardware-phy-activity.o
diff --git a/drivers/leds/trigger/ledtrig-hardware-phy-activity.c b/drivers/leds/trigger/ledtrig-hardware-phy-activity.c
new file mode 100644
index 000000000000..7fd0557fe878
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-hardware-phy-activity.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
+
+#define PHY_ACTIVITY_MAX_TRIGGERS 12
+
+#define DEFINE_OFFLOAD_TRIGGER(trigger_name, trigger) \
+	static ssize_t trigger_name##_show(struct device *dev, \
+				struct device_attribute *attr, char *buf) \
+	{ \
+		struct led_classdev *led_cdev = led_trigger_get_led(dev); \
+		int val; \
+		val = led_cdev->hw_control_configure(led_cdev, trigger, BLINK_MODE_READ); \
+		return sprintf(buf, "%d\n", val ? 1 : 0); \
+	} \
+	static ssize_t trigger_name##_store(struct device *dev, \
+					struct device_attribute *attr, \
+					const char *buf, size_t size) \
+	{ \
+		struct led_classdev *led_cdev = led_trigger_get_led(dev); \
+		unsigned long state; \
+		int cmd, ret; \
+		ret = kstrtoul(buf, 0, &state); \
+		if (ret) \
+			return ret; \
+		cmd = !!state ? BLINK_MODE_ENABLE : BLINK_MODE_DISABLE; \
+		/* Update the configuration with every change */ \
+		led_cdev->hw_control_configure(led_cdev, trigger, cmd); \
+		return size; \
+	} \
+	DEVICE_ATTR_RW(trigger_name)
+
+/* Expose sysfs for every blink to be configurable from userspace */
+DEFINE_OFFLOAD_TRIGGER(blink_tx, BLINK_TX);
+DEFINE_OFFLOAD_TRIGGER(blink_rx, BLINK_RX);
+DEFINE_OFFLOAD_TRIGGER(keep_link_10m, KEEP_LINK_10M);
+DEFINE_OFFLOAD_TRIGGER(keep_link_100m, KEEP_LINK_100M);
+DEFINE_OFFLOAD_TRIGGER(keep_link_1000m, KEEP_LINK_1000M);
+DEFINE_OFFLOAD_TRIGGER(keep_half_duplex, KEEP_HALF_DUPLEX);
+DEFINE_OFFLOAD_TRIGGER(keep_full_duplex, KEEP_FULL_DUPLEX);
+DEFINE_OFFLOAD_TRIGGER(option_linkup_over, OPTION_LINKUP_OVER);
+DEFINE_OFFLOAD_TRIGGER(option_power_on_reset, OPTION_POWER_ON_RESET);
+DEFINE_OFFLOAD_TRIGGER(option_blink_2hz, OPTION_BLINK_2HZ);
+DEFINE_OFFLOAD_TRIGGER(option_blink_4hz, OPTION_BLINK_4HZ);
+DEFINE_OFFLOAD_TRIGGER(option_blink_8hz, OPTION_BLINK_8HZ);
+
+/* The attrs will be placed dynamically based on the supported triggers */
+static struct attribute *phy_activity_attrs[PHY_ACTIVITY_MAX_TRIGGERS + 1];
+
+static int offload_phy_activity_activate(struct led_classdev *led_cdev)
+{
+	u32 checked_list = 0;
+	int i, trigger, ret;
+
+	/* Scan the supported offload triggers and expose them in sysfs if supported */
+	for (trigger = 0, i = 0; trigger < PHY_ACTIVITY_MAX_TRIGGERS; trigger++) {
+		if (!(checked_list & BLINK_TX) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, BLINK_TX)) {
+			phy_activity_attrs[i++] = &dev_attr_blink_tx.attr;
+			checked_list |= BLINK_TX;
+		}
+
+		if (!(checked_list & BLINK_RX) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, BLINK_RX)) {
+			phy_activity_attrs[i++] = &dev_attr_blink_rx.attr;
+			checked_list |= BLINK_RX;
+		}
+
+		if (!(checked_list & KEEP_LINK_10M) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, KEEP_LINK_10M)) {
+			phy_activity_attrs[i++] = &dev_attr_keep_link_10m.attr;
+			checked_list |= KEEP_LINK_10M;
+		}
+
+		if (!(checked_list & KEEP_LINK_100M) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, KEEP_LINK_100M)) {
+			phy_activity_attrs[i++] = &dev_attr_keep_link_100m.attr;
+			checked_list |= KEEP_LINK_100M;
+		}
+
+		if (!(checked_list & KEEP_LINK_1000M) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, KEEP_LINK_1000M)) {
+			phy_activity_attrs[i++] = &dev_attr_keep_link_1000m.attr;
+			checked_list |= KEEP_LINK_1000M;
+		}
+
+		if (!(checked_list & KEEP_HALF_DUPLEX) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, KEEP_HALF_DUPLEX)) {
+			phy_activity_attrs[i++] = &dev_attr_keep_half_duplex.attr;
+			checked_list |= KEEP_HALF_DUPLEX;
+		}
+
+		if (!(checked_list & KEEP_FULL_DUPLEX) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, KEEP_FULL_DUPLEX)) {
+			phy_activity_attrs[i++] = &dev_attr_keep_full_duplex.attr;
+			checked_list |= KEEP_FULL_DUPLEX;
+		}
+
+		if (!(checked_list & OPTION_LINKUP_OVER) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, OPTION_LINKUP_OVER)) {
+			phy_activity_attrs[i++] = &dev_attr_option_linkup_over.attr;
+			checked_list |= OPTION_LINKUP_OVER;
+		}
+
+		if (!(checked_list & OPTION_POWER_ON_RESET) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, OPTION_POWER_ON_RESET)) {
+			phy_activity_attrs[i++] = &dev_attr_option_power_on_reset.attr;
+			checked_list |= OPTION_POWER_ON_RESET;
+		}
+
+		if (!(checked_list & OPTION_BLINK_2HZ) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, OPTION_BLINK_2HZ)) {
+			phy_activity_attrs[i++] = &dev_attr_option_blink_2hz.attr;
+			checked_list |= OPTION_BLINK_2HZ;
+		}
+
+		if (!(checked_list & OPTION_BLINK_4HZ) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, OPTION_BLINK_4HZ)) {
+			phy_activity_attrs[i++] = &dev_attr_option_blink_4hz.attr;
+			checked_list |= OPTION_BLINK_4HZ;
+		}
+
+		if (!(checked_list & OPTION_BLINK_8HZ) &&
+		    led_trigger_blink_mode_is_supported(led_cdev, OPTION_BLINK_8HZ)) {
+			phy_activity_attrs[i++] = &dev_attr_option_blink_8hz.attr;
+			checked_list |= OPTION_BLINK_8HZ;
+		}
+	}
+
+	/* Enable hardware mode. No custom configuration is applied,
+	 * the LED driver will use whatever default configuration is
+	 * currently configured.
+	 */
+	ret = led_cdev->hw_control_start(led_cdev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void offload_phy_activity_deactivate(struct led_classdev *led_cdev)
+{
+	led_cdev->hw_control_stop(led_cdev);
+}
+
+static const struct attribute_group phy_activity_group = {
+	.name = "hardware-phy-activity",
+	.attrs = phy_activity_attrs,
+};
+
+static const struct attribute_group *phy_activity_groups[] = {
+	&phy_activity_group,
+	NULL,
+};
+
+static struct led_trigger offload_phy_activity_trigger = {
+	.supported_blink_modes = HARDWARE_ONLY,
+	.name       = "hardware-phy-activity",
+	.activate   = offload_phy_activity_activate,
+	.deactivate = offload_phy_activity_deactivate,
+	.groups     = phy_activity_groups,
+};
+
+static int __init offload_phy_activity_init(void)
+{
+	return led_trigger_register(&offload_phy_activity_trigger);
+}
+device_initcall(offload_phy_activity_init);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 087debe6716d..3d2e06db6839 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -555,6 +555,28 @@ enum led_trigger_netdev_modes {
 	TRIGGER_NETDEV_RX,
 };
 
+#ifdef CONFIG_LEDS_TRIGGER_HARDWARE_PHY_ACTIVITY
+/* 3 main category for offload trigger:
+ * - blink: the led will blink on trigger
+ * - keep: the led will be kept on with condition
+ * - option: a configuration value internal to the led on how offload works
+ */
+enum hardware_phy_activity {
+	BLINK_TX, /* Blink on TX traffic */
+	BLINK_RX, /* Blink on RX traffic */
+	KEEP_LINK_10M, /* LED on with 10M link */
+	KEEP_LINK_100M, /* LED on with 100M link */
+	KEEP_LINK_1000M, /* LED on with 1000M link */
+	KEEP_HALF_DUPLEX, /* LED on with Half-Duplex link */
+	KEEP_FULL_DUPLEX, /* LED on with Full-Duplex link */
+	OPTION_LINKUP_OVER, /* LED will be on with KEEP blink mode and blink on BLINK traffic */
+	OPTION_POWER_ON_RESET, /* LED will be on Switch reset */
+	OPTION_BLINK_2HZ, /* LED will blink with any offload_trigger at 2Hz */
+	OPTION_BLINK_4HZ, /* LED will blink with any offload_trigger at 4Hz */
+	OPTION_BLINK_8HZ, /* LED will blink with any offload_trigger at 8Hz */
+};
+#endif
+
 /* Trigger specific functions */
 #ifdef CONFIG_LEDS_TRIGGER_DISK
 void ledtrig_disk_activity(bool write);
-- 
2.32.0

Re: [RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Marek Behún]

On Tue,  9 Nov 2021 03:26:06 +0100
Ansuel Smith <ansuelsmth@gmail.com> wrote:

> Add Hardware Only Trigger for PHY Activity. This special trigger is used to
> configure and expose the different HW trigger that are provided by the
> PHY. Each blink mode can be configured by sysfs and on trigger
> activation the hardware mode is enabled.
> 
> This currently implement these hw triggers:
>   - blink_tx: Blink LED on tx packet receive
>   - blink_rx: Blink LED on rx packet receive
>   - keep_link_10m: Keep LED on with 10m link speed
>   - keep_link_100m: Keep LED on with 100m link speed
>   - kepp_link_1000m: Keep LED on with 1000m link speed
>   - keep_half_duplex: Keep LED on with half duplex link
>   - keep_full_duplex: Keep LED on with full duplex link
>   - option_linkup_over: Ignore blink tx/rx with link keep not active
>   - option_power_on_reset: Keep LED on with switch reset
>   - option_blink_2hz: Set blink speed at 2hz for every blink event
>   - option_blink_4hz: Set blink speed at 4hz for every blink event
>   - option_blink_8hz: Set blink speed at 8hz for every blink event
>   - option_blink_auto: Set blink speed at 2hz for 10m link speed,
>       4hz for 100m and 8hz for 1000m
> 
> The trigger will check if the LED driver support the various blink modes
> and will expose the blink modes in sysfs.
> It will finally enable hw mode for the LED without configuring any rule.
> This mean that the LED will blink/follow whatever offload trigger is
> active by default and the user needs to manually configure the desired
> offload triggers using sysfs.
> A flag is passed to configure_offload with the related rule from this
> trigger to active or disable.
> It's in the led driver interest the detection and knowing how to
> elaborate the passed flags and should report -EOPNOTSUPP otherwise.
> 
> The different hw triggers are exposed in the led sysfs dir under the
> offload-phy-activity subdir.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/Kconfig                  |  28 +++
>  drivers/leds/trigger/Makefile                 |   1 +
>  .../trigger/ledtrig-hardware-phy-activity.c   | 171 ++++++++++++++++++
>  include/linux/leds.h                          |  22 +++
>  4 files changed, 222 insertions(+)
>  create mode 100644 drivers/leds/trigger/ledtrig-hardware-phy-activity.c
> 
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index dc6816d36d06..b947b238be3f 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -154,4 +154,32 @@ config LEDS_TRIGGER_TTY
>  
>  	  When build as a module this driver will be called ledtrig-tty.
>  
> +config LEDS_TRIGGER_HARDWARE_PHY_ACTIVITY
> +	tristate "LED Trigger for PHY Activity for Hardware Controlled LED"
> +	depends on LEDS_HARDWARE_CONTROL
> +	help
> +	  This allows LEDs to be configured to run by hardware and offloaded
> +	  based on some rules. The LED will blink or be on based on the PHY
> +	  Activity for example on packet receive or based on the link speed.
> +
> +	  The current supported offload triggers are:
> +	  - blink_tx: Blink LED on tx packet receive
> +	  - blink_rx: Blink LED on rx packet receive
> +	  - keep_link_10m: Keep LED on with 10m link speed
> +	  - keep_link_100m: Keep LED on with 100m link speed
> +	  - keep_link_1000m: Keep LED on with 1000m link speed
> +	  - keep_half_duplex: Keep LED on with half duplex link
> +	  - keep_full_duplex: Keep LED on with full duplex link
> +	  - option_linkup_over: Blink rules are ignored with absent link
> +	  - option_power_on_reset: Power ON Led on Switch/PHY reset
> +	  - option_blink_2hz: Set blink speed at 2hz for every blink event
> +	  - option_blink_4hz: Set blink speed at 4hz for every blink event
> +	  - option_blink_8hz: Set blink speed at 8hz for every blink event
> +
> +	  These blink modes are present in the LED sysfs dir under
> +	  hardware-phy-activity if supported by the LED driver.
> +
> +	  This trigger can be used only by LEDs that supports Hardware mode
> +
> +
>  endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 25c4db97cdd4..f5d0d0057d2b 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
>  obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
>  obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
>  obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
> +obj-$(CONFIG_LEDS_TRIGGER_HARDWARE_PHY_ACTIVITY) += ledtrig-hardware-phy-activity.o
> diff --git a/drivers/leds/trigger/ledtrig-hardware-phy-activity.c b/drivers/leds/trigger/ledtrig-hardware-phy-activity.c
> new file mode 100644
> index 000000000000..7fd0557fe878
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-hardware-phy-activity.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/leds.h>
> +#include <linux/slab.h>
> +
> +#define PHY_ACTIVITY_MAX_TRIGGERS 12
> +
> +#define DEFINE_OFFLOAD_TRIGGER(trigger_name, trigger) \
> +	static ssize_t trigger_name##_show(struct device *dev, \
> +				struct device_attribute *attr, char *buf) \
> +	{ \
> +		struct led_classdev *led_cdev = led_trigger_get_led(dev); \
> +		int val; \
> +		val = led_cdev->hw_control_configure(led_cdev, trigger, BLINK_MODE_READ); \
> +		return sprintf(buf, "%d\n", val ? 1 : 0); \
> +	} \
> +	static ssize_t trigger_name##_store(struct device *dev, \
> +					struct device_attribute *attr, \
> +					const char *buf, size_t size) \
> +	{ \
> +		struct led_classdev *led_cdev = led_trigger_get_led(dev); \
> +		unsigned long state; \
> +		int cmd, ret; \
> +		ret = kstrtoul(buf, 0, &state); \
> +		if (ret) \
> +			return ret; \
> +		cmd = !!state ? BLINK_MODE_ENABLE : BLINK_MODE_DISABLE; \
> +		/* Update the configuration with every change */ \
> +		led_cdev->hw_control_configure(led_cdev, trigger, cmd); \
> +		return size; \
> +	} \
> +	DEVICE_ATTR_RW(trigger_name)
> +
> +/* Expose sysfs for every blink to be configurable from userspace */
> +DEFINE_OFFLOAD_TRIGGER(blink_tx, BLINK_TX);
> +DEFINE_OFFLOAD_TRIGGER(blink_rx, BLINK_RX);
> +DEFINE_OFFLOAD_TRIGGER(keep_link_10m, KEEP_LINK_10M);
> +DEFINE_OFFLOAD_TRIGGER(keep_link_100m, KEEP_LINK_100M);
> +DEFINE_OFFLOAD_TRIGGER(keep_link_1000m, KEEP_LINK_1000M);
> +DEFINE_OFFLOAD_TRIGGER(keep_half_duplex, KEEP_HALF_DUPLEX);
> +DEFINE_OFFLOAD_TRIGGER(keep_full_duplex, KEEP_FULL_DUPLEX);
> +DEFINE_OFFLOAD_TRIGGER(option_linkup_over, OPTION_LINKUP_OVER);
> +DEFINE_OFFLOAD_TRIGGER(option_power_on_reset, OPTION_POWER_ON_RESET);
> +DEFINE_OFFLOAD_TRIGGER(option_blink_2hz, OPTION_BLINK_2HZ);
> +DEFINE_OFFLOAD_TRIGGER(option_blink_4hz, OPTION_BLINK_4HZ);
> +DEFINE_OFFLOAD_TRIGGER(option_blink_8hz, OPTION_BLINK_8HZ);

This is very strange. Is option_blink_2hz a trigger on itself? Or just
an option for another trigger? It seems that it is an option, so that I
can set something like
  blink_tx,option_blink_2hz
and the LED will blink on tx activity with frequency 2 Hz... If that is
so, I think you are misnaming your macros or something, since you are
defining option_blink_2hz as a trigger with
 DEFINE_OFFLOAD_TRIGGER

Marek

Re: [RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Andrew Lunn]

> > +/* Expose sysfs for every blink to be configurable from userspace */
> > +DEFINE_OFFLOAD_TRIGGER(blink_tx, BLINK_TX);
> > +DEFINE_OFFLOAD_TRIGGER(blink_rx, BLINK_RX);
> > +DEFINE_OFFLOAD_TRIGGER(keep_link_10m, KEEP_LINK_10M);
> > +DEFINE_OFFLOAD_TRIGGER(keep_link_100m, KEEP_LINK_100M);
> > +DEFINE_OFFLOAD_TRIGGER(keep_link_1000m, KEEP_LINK_1000M);

You might get warnings about CamelCase, but i suggest keep_link_10M,
keep_link_100M and keep_link_1000M. These are megabits, not millibits.

> > +DEFINE_OFFLOAD_TRIGGER(keep_half_duplex, KEEP_HALF_DUPLEX);
> > +DEFINE_OFFLOAD_TRIGGER(keep_full_duplex, KEEP_FULL_DUPLEX);

What does keep mean in this context?

> > +DEFINE_OFFLOAD_TRIGGER(option_linkup_over, OPTION_LINKUP_OVER);
> > +DEFINE_OFFLOAD_TRIGGER(option_power_on_reset, OPTION_POWER_ON_RESET);
> > +DEFINE_OFFLOAD_TRIGGER(option_blink_2hz, OPTION_BLINK_2HZ);
> > +DEFINE_OFFLOAD_TRIGGER(option_blink_4hz, OPTION_BLINK_4HZ);
> > +DEFINE_OFFLOAD_TRIGGER(option_blink_8hz, OPTION_BLINK_8HZ);
> 
> This is very strange. Is option_blink_2hz a trigger on itself? Or just
> an option for another trigger? It seems that it is an option, so that I
> can set something like
>   blink_tx,option_blink_2hz
> and the LED will blink on tx activity with frequency 2 Hz... If that is
> so, I think you are misnaming your macros or something, since you are
> defining option_blink_2hz as a trigger with
>  DEFINE_OFFLOAD_TRIGGER

Yes, i already said this needs handling differently. The 2Hz, 4Hz and
8Hz naturally fit the delay_on, delay_of sysfs attributes.

    Andrew

Re: [RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Ansuel Smith]

On Tue, Nov 09, 2021 at 10:09:40PM +0100, Andrew Lunn wrote:
> > > +/* Expose sysfs for every blink to be configurable from userspace */
> > > +DEFINE_OFFLOAD_TRIGGER(blink_tx, BLINK_TX);
> > > +DEFINE_OFFLOAD_TRIGGER(blink_rx, BLINK_RX);
> > > +DEFINE_OFFLOAD_TRIGGER(keep_link_10m, KEEP_LINK_10M);
> > > +DEFINE_OFFLOAD_TRIGGER(keep_link_100m, KEEP_LINK_100M);
> > > +DEFINE_OFFLOAD_TRIGGER(keep_link_1000m, KEEP_LINK_1000M);
> 
> You might get warnings about CamelCase, but i suggest keep_link_10M,
> keep_link_100M and keep_link_1000M. These are megabits, not millibits.
> 
> > > +DEFINE_OFFLOAD_TRIGGER(keep_half_duplex, KEEP_HALF_DUPLEX);
> > > +DEFINE_OFFLOAD_TRIGGER(keep_full_duplex, KEEP_FULL_DUPLEX);
> 
> What does keep mean in this context?
>

LED is turned on but doesn't blink. Hint for a better name?

> > > +DEFINE_OFFLOAD_TRIGGER(option_linkup_over, OPTION_LINKUP_OVER);
> > > +DEFINE_OFFLOAD_TRIGGER(option_power_on_reset, OPTION_POWER_ON_RESET);
> > > +DEFINE_OFFLOAD_TRIGGER(option_blink_2hz, OPTION_BLINK_2HZ);
> > > +DEFINE_OFFLOAD_TRIGGER(option_blink_4hz, OPTION_BLINK_4HZ);
> > > +DEFINE_OFFLOAD_TRIGGER(option_blink_8hz, OPTION_BLINK_8HZ);
> > 
> > This is very strange. Is option_blink_2hz a trigger on itself? Or just
> > an option for another trigger? It seems that it is an option, so that I
> > can set something like
> >   blink_tx,option_blink_2hz
> > and the LED will blink on tx activity with frequency 2 Hz... If that is
> > so, I think you are misnaming your macros or something, since you are
> > defining option_blink_2hz as a trigger with
> >  DEFINE_OFFLOAD_TRIGGER
> 
> Yes, i already said this needs handling differently. The 2Hz, 4Hz and
> 8Hz naturally fit the delay_on, delay_of sysfs attributes.
> 
>     Andrew

You are totally right. I tought the blink control was something specific
that only some PHY had, but considering we have more than 1 that
supports a variant of it, I can see how it should be handled separately.
I assume even more have this kind of customizzation.

-- 
	Ansuel

Re: [RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Andrew Lunn]

On Wed, Nov 10, 2021 at 09:04:34PM +0100, Ansuel Smith wrote:
> On Tue, Nov 09, 2021 at 10:09:40PM +0100, Andrew Lunn wrote:
> > > > +/* Expose sysfs for every blink to be configurable from userspace */
> > > > +DEFINE_OFFLOAD_TRIGGER(blink_tx, BLINK_TX);
> > > > +DEFINE_OFFLOAD_TRIGGER(blink_rx, BLINK_RX);
> > > > +DEFINE_OFFLOAD_TRIGGER(keep_link_10m, KEEP_LINK_10M);
> > > > +DEFINE_OFFLOAD_TRIGGER(keep_link_100m, KEEP_LINK_100M);
> > > > +DEFINE_OFFLOAD_TRIGGER(keep_link_1000m, KEEP_LINK_1000M);
> > 
> > You might get warnings about CamelCase, but i suggest keep_link_10M,
> > keep_link_100M and keep_link_1000M. These are megabits, not millibits.
> > 
> > > > +DEFINE_OFFLOAD_TRIGGER(keep_half_duplex, KEEP_HALF_DUPLEX);
> > > > +DEFINE_OFFLOAD_TRIGGER(keep_full_duplex, KEEP_FULL_DUPLEX);
> > 
> > What does keep mean in this context?
> >
> 
> LED is turned on but doesn't blink. Hint for a better name?

I would just drop the keep. You have blink_ as a prefix for those
modes that blink.

      Andrew

Re: [RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Randy Dunlap]

On 11/8/21 6:26 PM, Ansuel Smith wrote:
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index dc6816d36d06..b947b238be3f 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -154,4 +154,32 @@ config LEDS_TRIGGER_TTY
>   
>   	  When build as a module this driver will be called ledtrig-tty.
>   
> +config LEDS_TRIGGER_HARDWARE_PHY_ACTIVITY
> +	tristate "LED Trigger for PHY Activity for Hardware Controlled LED"
> +	depends on LEDS_HARDWARE_CONTROL
> +	help
> +	  This allows LEDs to be configured to run by hardware and offloaded
> +	  based on some rules. The LED will blink or be on based on the PHY

	                                          or be "on" based on the PHY

> +	  Activity for example on packet receive or based on the link speed.

	  activity

> +
> +	  The current supported offload triggers are:
> +	  - blink_tx: Blink LED on tx packet receive
> +	  - blink_rx: Blink LED on rx packet receive
> +	  - keep_link_10m: Keep LED on with 10m link speed
> +	  - keep_link_100m: Keep LED on with 100m link speed
> +	  - keep_link_1000m: Keep LED on with 1000m link speed
> +	  - keep_half_duplex: Keep LED on with half duplex link
> +	  - keep_full_duplex: Keep LED on with full duplex link
> +	  - option_linkup_over: Blink rules are ignored with absent link
> +	  - option_power_on_reset: Power ON Led on Switch/PHY reset
> +	  - option_blink_2hz: Set blink speed at 2hz for every blink event
> +	  - option_blink_4hz: Set blink speed at 4hz for every blink event
> +	  - option_blink_8hz: Set blink speed at 8hz for every blink event
> +
> +	  These blink modes are present in the LED sysfs dir under
> +	  hardware-phy-activity if supported by the LED driver.
> +
> +	  This trigger can be used only by LEDs that supports Hardware mode

	                                             support Hardware mode.


Ansuel, do you read and consider these comments?
It's difficult to tell if you do or not.

thanks.
-- 
~Randy

Re: [RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Ansuel Smith]

On Mon, Nov 08, 2021 at 10:02:22PM -0800, Randy Dunlap wrote:
> On 11/8/21 6:26 PM, Ansuel Smith wrote:
> > diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> > index dc6816d36d06..b947b238be3f 100644
> > --- a/drivers/leds/trigger/Kconfig
> > +++ b/drivers/leds/trigger/Kconfig
> > @@ -154,4 +154,32 @@ config LEDS_TRIGGER_TTY
> >   	  When build as a module this driver will be called ledtrig-tty.
> > +config LEDS_TRIGGER_HARDWARE_PHY_ACTIVITY
> > +	tristate "LED Trigger for PHY Activity for Hardware Controlled LED"
> > +	depends on LEDS_HARDWARE_CONTROL
> > +	help
> > +	  This allows LEDs to be configured to run by hardware and offloaded
> > +	  based on some rules. The LED will blink or be on based on the PHY
> 
> 	                                          or be "on" based on the PHY
> 
> > +	  Activity for example on packet receive or based on the link speed.
> 
> 	  activity
> 
> > +
> > +	  The current supported offload triggers are:
> > +	  - blink_tx: Blink LED on tx packet receive
> > +	  - blink_rx: Blink LED on rx packet receive
> > +	  - keep_link_10m: Keep LED on with 10m link speed
> > +	  - keep_link_100m: Keep LED on with 100m link speed
> > +	  - keep_link_1000m: Keep LED on with 1000m link speed
> > +	  - keep_half_duplex: Keep LED on with half duplex link
> > +	  - keep_full_duplex: Keep LED on with full duplex link
> > +	  - option_linkup_over: Blink rules are ignored with absent link
> > +	  - option_power_on_reset: Power ON Led on Switch/PHY reset
> > +	  - option_blink_2hz: Set blink speed at 2hz for every blink event
> > +	  - option_blink_4hz: Set blink speed at 4hz for every blink event
> > +	  - option_blink_8hz: Set blink speed at 8hz for every blink event
> > +
> > +	  These blink modes are present in the LED sysfs dir under
> > +	  hardware-phy-activity if supported by the LED driver.
> > +
> > +	  This trigger can be used only by LEDs that supports Hardware mode
> 
> 	                                             support Hardware mode.
> 
> 
> Ansuel, do you read and consider these comments?
> It's difficult to tell if you do or not.
>

Yes and with every new version I'm fixing the errors.
Just we are changing implementation many times and more errors comes up.
Thanks a lot for the comments and sorry if I'm not answering them.

> thanks.
> -- 
> ~Randy

-- 
	Ansuel

Re: [RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Andrew Lunn]

> +#define DEFINE_OFFLOAD_TRIGGER(trigger_name, trigger) \
> +	static ssize_t trigger_name##_show(struct device *dev, \
> +				struct device_attribute *attr, char *buf) \
> +	{ \
> +		struct led_classdev *led_cdev = led_trigger_get_led(dev); \
> +		int val; \
> +		val = led_cdev->hw_control_configure(led_cdev, trigger, BLINK_MODE_READ); \
> +		return sprintf(buf, "%d\n", val ? 1 : 0); \
> +	} \
> +	static ssize_t trigger_name##_store(struct device *dev, \
> +					struct device_attribute *attr, \
> +					const char *buf, size_t size) \
> +	{ \
> +		struct led_classdev *led_cdev = led_trigger_get_led(dev); \
> +		unsigned long state; \
> +		int cmd, ret; \
> +		ret = kstrtoul(buf, 0, &state); \
> +		if (ret) \
> +			return ret; \
> +		cmd = !!state ? BLINK_MODE_ENABLE : BLINK_MODE_DISABLE; \
> +		/* Update the configuration with every change */ \
> +		led_cdev->hw_control_configure(led_cdev, trigger, cmd); \
> +		return size; \
> +	} \
> +	DEVICE_ATTR_RW(trigger_name)

These are pretty big macro magic functions. And there is little actual
macro in them. So make them simple functions which call helpers

	static ssize_t trigger_name##_show(struct device *dev, \
				struct device_attribute *attr, char *buf) \
	{ \
		return trigger_generic_store(dev, attr, buf, size, trigger); \
	} \
	static ssize_t trigger_name##_store(struct device *dev, \
					struct device_attribute *attr, \
					const char *buf, size_t size) \
	{ \
		return trigger_generic_store(dev, attr, buf, size, trigger); \
	} \

> +/* The attrs will be placed dynamically based on the supported triggers */
> +static struct attribute *phy_activity_attrs[PHY_ACTIVITY_MAX_TRIGGERS + 1];
> +
> +static int offload_phy_activity_activate(struct led_classdev *led_cdev)
> +{
> +	u32 checked_list = 0;
> +	int i, trigger, ret;
> +
> +	/* Scan the supported offload triggers and expose them in sysfs if supported */
> +	for (trigger = 0, i = 0; trigger < PHY_ACTIVITY_MAX_TRIGGERS; trigger++) {
> +		if (!(checked_list & BLINK_TX) &&
> +		    led_trigger_blink_mode_is_supported(led_cdev, BLINK_TX)) {
> +			phy_activity_attrs[i++] = &dev_attr_blink_tx.attr;
> +			checked_list |= BLINK_TX;
> +		}

Please re-write this using tables, rather than all this repeated code.

       Andrew

Re: [RFC PATCH v3 6/8] leds: trigger: add hardware-phy-activity trigger

[Andrew Lunn]

> +/* The attrs will be placed dynamically based on the supported triggers */
> +static struct attribute *phy_activity_attrs[PHY_ACTIVITY_MAX_TRIGGERS + 1];

This cannot be global. I have boards with a mixture of different PHYs
and switches. Each will have their own collection of supported
modes. And some PHYs have different modes per LED. 

       Andrew

[RFC PATCH v3 7/8] net: dsa: qca8k: add LEDs support

[Ansuel Smith]

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..
This adds support for 2 hardware trigger netdev and
hardware-phy-activity.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/Kconfig      |   9 +
 drivers/net/dsa/Makefile     |   1 +
 drivers/net/dsa/qca8k-leds.c | 429 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.c      |   8 +-
 drivers/net/dsa/qca8k.h      |  65 ++++++
 5 files changed, 510 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/dsa/qca8k-leds.c

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 7b1457a6e327..89e36f3a8195 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -67,6 +67,15 @@ config NET_DSA_QCA8K
 	  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"
+	select NET_DSA_QCA8K
+	select LEDS_OFFLOAD_TRIGGERS
+	help
+	  Thsi 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.
+
 config NET_DSA_REALTEK_SMI
 	tristate "Realtek SMI Ethernet switch family support"
 	select NET_DSA_TAG_RTL4_A
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index 8da1569a34e6..fb1e7d0cf126 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_NET_DSA_LANTIQ_GSWIP) += lantiq_gswip.o
 obj-$(CONFIG_NET_DSA_MT7530)	+= mt7530.o
 obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
 obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
+obj-$(CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT) += qca8k-leds.o
 obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
 realtek-smi-objs		:= realtek-smi-core.o rtl8366.o rtl8366rb.o rtl8365mb.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
diff --git a/drivers/net/dsa/qca8k-leds.c b/drivers/net/dsa/qca8k-leds.c
new file mode 100644
index 000000000000..0ca4e4c88f2f
--- /dev/null
+++ b/drivers/net/dsa/qca8k-leds.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <net/dsa.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_hardware_phy_activity(unsigned long rules, u32 *offload_trigger,
+				  u32 *mask)
+{
+	/* Parsing specific to hardware-phy-activity trigger */
+	if (test_bit(BLINK_TX, &rules))
+		*offload_trigger = QCA8K_LED_TX_BLINK_MASK;
+	if (test_bit(BLINK_RX, &rules))
+		*offload_trigger = QCA8K_LED_RX_BLINK_MASK;
+	if (test_bit(KEEP_LINK_10M, &rules))
+		*offload_trigger = QCA8K_LED_LINK_10M_EN_MASK;
+	if (test_bit(KEEP_LINK_100M, &rules))
+		*offload_trigger = QCA8K_LED_LINK_100M_EN_MASK;
+	if (test_bit(KEEP_LINK_1000M, &rules))
+		*offload_trigger = QCA8K_LED_LINK_1000M_EN_MASK;
+	if (test_bit(KEEP_HALF_DUPLEX, &rules))
+		*offload_trigger = QCA8K_LED_HALF_DUPLEX_MASK;
+	if (test_bit(KEEP_FULL_DUPLEX, &rules))
+		*offload_trigger = QCA8K_LED_FULL_DUPLEX_MASK;
+	if (test_bit(OPTION_LINKUP_OVER, &rules))
+		*offload_trigger = QCA8K_LED_LINKUP_OVER_MASK;
+	if (test_bit(OPTION_BLINK_2HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_2HZ;
+	if (test_bit(OPTION_BLINK_4HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_4HZ;
+	if (test_bit(OPTION_BLINK_8HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_8HZ;
+
+	if (!*offload_trigger)
+		return -EOPNOTSUPP;
+
+	if (test_bit(OPTION_BLINK_2HZ, &rules) ||
+	    test_bit(OPTION_BLINK_4HZ, &rules) ||
+	    test_bit(OPTION_BLINK_8HZ, &rules)) {
+		*mask = QCA8K_LED_BLINK_FREQ_MASK;
+	} else {
+		*mask = *offload_trigger;
+	}
+
+	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_TX, &rules))
+		*offload_trigger = QCA8K_LED_TX_BLINK_MASK;
+	if (test_bit(TRIGGER_NETDEV_RX, &rules))
+		*offload_trigger = QCA8K_LED_RX_BLINK_MASK;
+
+	if (!*offload_trigger)
+		return -EOPNOTSUPP;
+
+	*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, "hardware-phy-activity") &&
+	    strcmp(trigger->name, "netdev"))
+		return -EOPNOTSUPP;
+
+	if (!strcmp(trigger->name, "hardware-phy-activity"))
+		ret = qca8k_parse_hardware_phy_activity(rules, &offload_trigger, &mask);
+
+	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 reache 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 = qca8k_rmw(priv, reg_info.reg,
+				QCA8K_LED_RULE_MASK << reg_info.shift,
+				default_reg << reg_info.shift);
+		break;
+	case BLINK_MODE_ENABLE:
+		ret = qca8k_rmw(priv, reg_info.reg,
+				mask << reg_info.shift,
+				offload_trigger << reg_info.shift);
+		break;
+	case BLINK_MODE_DISABLE:
+		ret = qca8k_rmw(priv, reg_info.reg,
+				mask << reg_info.shift,
+				0);
+		break;
+	case BLINK_MODE_READ:
+		ret = qca8k_read(priv, 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;
+
+	qca8k_rmw(priv, 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 = qca8k_read(priv, 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);
+
+	qca8k_rmw(priv, 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 qca8k_rmw(priv, 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);
+
+	qca8k_read(priv, 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 rapresent 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/qca8k.c b/drivers/net/dsa/qca8k.c
index a429c9750add..10b2b47b2156 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -148,7 +148,7 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
 	return 0;
 }
 
-static int
+int
 qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val)
 {
 	struct mii_bus *bus = priv->bus;
@@ -192,7 +192,7 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 	return ret;
 }
 
-static int
+int
 qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
 {
 	struct mii_bus *bus = priv->bus;
@@ -1109,6 +1109,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_led_ctrl(priv);
+	if (ret)
+		return ret;
+
 	/* Make sure MAC06 is disabled */
 	ret = qca8k_reg_clear(priv, QCA8K_REG_PORT0_PAD_CTRL,
 			      QCA8K_PORT0_PAD_MAC06_EXCHANGE_EN);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 128b8cf85e08..f68f037fe909 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -11,6 +11,7 @@
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/gpio.h>
+#include <linux/leds.h>
 
 #define QCA8K_NUM_PORTS					7
 #define QCA8K_NUM_CPU_PORTS				2
@@ -74,6 +75,43 @@
 #define   QCA8K_MDIO_MASTER_DATA_MASK			GENMASK(15, 0)
 #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
@@ -279,6 +317,19 @@ struct qca8k_ports_config {
 	u8 rgmii_tx_delay[QCA8K_NUM_CPU_PORTS]; /* 0: CPU port0, 1: CPU port6 */
 };
 
+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;
@@ -293,6 +344,7 @@ struct qca8k_priv {
 	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
 	unsigned int port_mtu[QCA8K_NUM_PORTS];
+	struct qca8k_led ports_led[QCA8K_LED_COUNT];
 };
 
 struct qca8k_mib_desc {
@@ -308,4 +360,17 @@ struct qca8k_fdb {
 	u8 mac[6];
 };
 
+/* Common function used by the LEDs module */
+int qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val);
+int qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val);
+
+#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.32.0

Re: [RFC PATCH v3 7/8] net: dsa: qca8k: add LEDs support

[Randy Dunlap]

On 11/8/21 6:26 PM, Ansuel Smith wrote:
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 7b1457a6e327..89e36f3a8195 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -67,6 +67,15 @@ config NET_DSA_QCA8K
>   	  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"
> +	select NET_DSA_QCA8K
> +	select LEDS_OFFLOAD_TRIGGERS
> +	help
> +	  Thsi enabled support for LEDs present on the Qualcomm Atheros

	  This enables

> +	  QCA8K Ethernet switch chips. This require the LEDs offload

	                                    requires

> +	  triggers support as it can run in offload mode.


-- 
~Randy

Re: [RFC PATCH v3 7/8] net: dsa: qca8k: add LEDs support

[Andrew Lunn]

> +static int
> +qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger, u32 *mask)

This is a rather oddly named function, given that it is not actually
passed a netdev. netdev has a very well defined meaning in the network
stack, struct net_device.

       Andrew

[RFC PATCH v3 8/8] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Ansuel Smith]

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: Ansuel Smith <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/dsa/qca8k.yaml    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index 48de0ace265d..106d95adc1e8 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -64,6 +64,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.
 
     properties:
       '#address-cells':
@@ -340,6 +342,24 @@ examples:
 
                 internal_phy_port1: ethernet-phy@0 {
                     reg = <0>;
+
+                    leds {
+                        led@0 {
+                            reg = <0>;
+                            color = <LED_COLOR_ID_WHITE>;
+                            function = LED_FUNCTION_LAN;
+                            function-enumerator = <1>;
+                            linux,default-trigger = "offload-phy-activity";
+                        };
+
+                        led@1 {
+                            reg = <1>;
+                            color = <LED_COLOR_ID_AMBER>;
+                            function = LED_FUNCTION_LAN;
+                            function-enumerator = <1>;
+                            linux,default-trigger = "offload-phy-activity";
+                        };
+                    };
                 };
 
                 internal_phy_port2: ethernet-phy@1 {
-- 
2.32.0