This is another attempt on adding this feature on LEDs, hoping this is
the right time and someone finally notice this.
Most of the times Switch/PHY have connected multiple LEDs that are
controlled by HW based on some rules/event. Currently we lack any
support for a generic way to control the HW part and normally we
either never implement the feature or only add control for brightness
or hw blink.
This is based on Marek idea of providing some API to cled but use a
different implementation that in theory should be more generilized.
The current idea is:
- LED driver implement 3 API (hw_control_status/start/stop).
They are used to put the LED in hardware mode and to configure the
various trigger.
- We have hardware triggers that are used to expose to userspace the
supported hardware mode and set the hardware mode on trigger
activation.
- We can also have triggers that both support hardware and software mode.
- The LED driver will declare each supported hardware blink mode and
communicate with the trigger all the supported blink modes that will
be available by sysfs.
- A trigger will use blink_set to configure the blink mode to active
in hardware mode.
- On hardware trigger activation, only the hardware mode is enabled but
the blink modes are not configured. The LED driver should reset any
link mode active by default.
Each LED driver will have to declare explicit support for the offload
trigger (or return not supported error code) as we the trigger_data that
the LED driver will elaborate and understand what is referring to (based
on the current active trigger).
I posted a user for this new implementation that will benefit from this
and will add a big feature to it. Currently qca8k can have up to 3 LEDs
connected to each PHY port and we have some device that have only one of
them connected and the default configuration won't work for that.
The netdev trigger is expanded and it does now support hardware only
triggers.
The idea is to use hardware mode when a device_name is not defined.
An additional sysfs entry is added to give some info about the available
trigger modes supported in the current configuration.
It was reported that at least 3 other switch family would benefits by
this as they all lack support for a generic way to setup their leds and
netdev team NACK each try to add special code to support LEDs present
on switch in favor of a generic solution.
v8:
- Improve the documentation of the new feature
- Rename to a more symbolic name
- Fix some bug in netdev trigger (not using BIT())
- Add more define for qca8k-leds driver
- Add activity led mode
- Drop interval support
- Move qca8k brightness set to blocking variant (can sleep while
setting the mode)
- More some function out of config define and provide variant if not
selected
- Fix many bugs in the validate option in the netdev trigger
- Add phy generic schema for leds support
- Add additional required Documentation changes
v7:
- Rebase on top of net-next (for qca8k changes)
- Fix some typo in commit description
- Fix qca8k leds documentation warning
- Remove RFC tag
v6:
- Back to RFC.
- Drop additional trigger
- Rework netdev trigger to support common modes used by switch and
hardware only triggers
- Refresh qca8k leds logic and driver
v5:
- Move out of RFC. (no comments from Andrew this is the right path?)
- Fix more spelling mistake (thx Randy)
- Fix error reported by kernel test bot
- Drop the additional HW_CONTROL flag. It does simplify CONFIG
handling and hw control should be available anyway to support
triggers as module.
v4:
- Rework implementation and drop hw_configure logic.
We now expand blink_set.
- Address even more spelling mistake. (thx a lot Randy)
- Drop blink option and use blink_set delay.
- Rework phy-activity trigger to actually make the groups dynamic.
v3:
- Rework start/stop as Andrew asked.
- Introduce more logic to permit a trigger to run in hardware mode.
- Add additional patch with netdev hardware support.
- Use test_bit API to check flag passed to hw_control_configure.
- Added a new cmd to hw_control_configure to reset any active blink_mode.
- Refactor all the patches to follow this new implementation.
v2:
- Fix spelling mistake (sorry)
- Drop patch 02 "permit to declare supported offload triggers".
Change the logic, now the LED driver declare support for them
using the configure_offload with the cmd TRIGGER_SUPPORTED.
- Rework code to follow this new implementation.
- Update Documentation to better describe how this offload
implementation work.
Christian Marangi (13):
leds: add support for hardware driven LEDs
leds: add function to configure hardware controlled LED
leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode
leds: trigger: netdev: rename and expose NETDEV trigger enum modes
leds: trigger: netdev: convert device attr to macro
leds: trigger: netdev: add hardware control support
leds: trigger: netdev: use mutex instead of spinlocks
leds: trigger: netdev: add available mode sysfs attr
leds: trigger: netdev: add additional hardware only triggers
net: dsa: qca8k: add LEDs support
dt-bindings: leds: Document netdev trigger
dt-bindings: net: phy: Document support for leds node
dt-bindings: net: dsa: qca8k: add LEDs definition example
.../devicetree/bindings/leds/common.yaml | 2 +
.../devicetree/bindings/net/dsa/qca8k.yaml | 24 +
.../devicetree/bindings/net/ethernet-phy.yaml | 22 +
Documentation/leds/leds-class.rst | 94 ++++
drivers/leds/Kconfig | 11 +
drivers/leds/led-class.c | 27 ++
drivers/leds/led-triggers.c | 38 ++
drivers/leds/trigger/ledtrig-netdev.c | 414 ++++++++++++-----
drivers/net/dsa/qca/Kconfig | 9 +
drivers/net/dsa/qca/Makefile | 1 +
drivers/net/dsa/qca/qca8k-8xxx.c | 4 +
drivers/net/dsa/qca/qca8k-leds.c | 419 ++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 69 +++
include/linux/leds.h | 95 +++-
14 files changed, 1126 insertions(+), 103 deletions(-)
create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
--
2.38.1
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 LED_BLINK_HW_CONTROLLED or
LED_BLINK_SWHW_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,
LED_BLINK_SW_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.
- 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 LED_BLINK_HW_CONTROLLED blink_mode hw_control_status/start/stop is
optional and any software only trigger will reject activation as the LED
supports only hardware mode.
An additional config CONFIG_LEDS_HARDWARE_CONTROL is added to add support
for LEDs that can be controlled by hardware.
Cc: Marek Behún <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
Documentation/leds/leds-class.rst | 34 +++++++++++++++++++++++++++
drivers/leds/Kconfig | 11 +++++++++
drivers/leds/led-class.c | 27 +++++++++++++++++++++
drivers/leds/led-triggers.c | 38 ++++++++++++++++++++++++++++++
include/linux/leds.h | 39 ++++++++++++++++++++++++++++++-
5 files changed, 148 insertions(+), 1 deletion(-)
diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index cd155ead8703..984d73499d83 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -169,6 +169,40 @@ 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 LED_BLINK_HW_CONTROLLED or LED_BLINK_SWHW_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, LED_BLINK_SW_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.
+
+- 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 LED_BLINK_HW_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 9dbce09eabac..019b4f344e01 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 a6b3adcd044a..10408bff8e10 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -166,6 +166,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_hw_mode_functions(struct led_classdev *led_cdev)
+{
+ int mode = led_cdev->blink_mode;
+
+ if (mode == LED_BLINK_SWHW_CONTROLLED &&
+ (!led_cdev->hw_control_status ||
+ !led_cdev->hw_control_start ||
+ !led_cdev->hw_control_stop))
+ return -EINVAL;
+
+ if (mode == LED_BLINK_SW_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.
@@ -466,6 +487,12 @@ int led_classdev_register_ext(struct device *parent,
if (ret < 0)
return ret;
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+ ret = led_classdev_check_blink_hw_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..00d9f6b06f5c 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -154,6 +154,38 @@ 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 LED_BLINK_SW_CONTROLLED:
+ return trigger->supported_blink_modes != LED_TRIGGER_HW_ONLY;
+
+ case LED_BLINK_HW_CONTROLLED:
+ return trigger->supported_blink_modes != LED_TRIGGER_SW_ONLY;
+
+ case LED_BLINK_SWHW_CONTROLLED:
+ return true;
+ }
+
+ return 1;
+}
+
+static void led_trigger_hw_mode_stop(struct led_classdev *led_cdev)
+{
+ /* check if LED is in HW block mode */
+ if (led_cdev->blink_mode == LED_BLINK_SW_CONTROLLED)
+ return;
+
+ /*
+ * We can assume these function are always present as
+ * for LED support hw blink mode they MUST be provided or register
+ * fail.
+ */
+ if (led_cdev->hw_control_status(led_cdev))
+ led_cdev->hw_control_stop(led_cdev);
+}
+
/* Caller must ensure led_cdev->trigger_lock held */
int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
{
@@ -179,6 +211,8 @@ 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 */
+ led_trigger_hw_mode_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 +222,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 d71201a968b6..5c360fba9ccf 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -88,6 +88,12 @@ struct led_hw_trigger_type {
int dummy;
};
+enum led_blink_modes {
+ LED_BLINK_SW_CONTROLLED = 0x0,
+ LED_BLINK_HW_CONTROLLED,
+ LED_BLINK_SWHW_CONTROLLED,
+};
+
struct led_classdev {
const char *name;
unsigned int brightness;
@@ -175,6 +181,24 @@ 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 LED_BLINK_HW_CONTROLLED, LED is always in offload
+ * mode and triggers can't be simulated by software.
+ * If the led is LED_BLINK_HW_CONTROLLED, status/start/stop function
+ * are optional.
+ * By default LED_BLINK_SW_CONTROLLED is set as blink_mode.
+ */
+ enum led_blink_modes blink_mode;
+ /* Ask the LED driver if hardware mode is enabled or not */
+ 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
#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -242,7 +266,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
@@ -377,12 +400,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
#define TRIG_NAME_MAX 50
+enum led_trigger_blink_supported_modes {
+ LED_TRIGGER_SW_ONLY = LED_BLINK_SW_CONTROLLED,
+ LED_TRIGGER_HW_ONLY = LED_BLINK_HW_CONTROLLED,
+ LED_TRIGGER_SWHW = LED_BLINK_SWHW_CONTROLLED,
+};
+
struct led_trigger {
/* Trigger Properties */
const char *name;
int (*activate)(struct led_classdev *led_cdev);
void (*deactivate)(struct led_classdev *led_cdev);
+ /* Declare if the Trigger supports hardware control to
+ * offload triggers or supports only software control.
+ * A trigger can also declare support for hardware control
+ * if its task is to only configure LED blink modes and expose
+ * them in sysfs.
+ */
+ enum led_trigger_blink_supported_modes supported_blink_modes;
+
/* LED-private triggers have this set */
struct led_hw_trigger_type *trigger_type;
--
2.38.1
Add hw_control_configure helper to configure how the LED should work in
hardware mode. The function require to support the particular trigger and
will use the passed flag to elaborate the data and apply the
correct configuration. This function will then be used by the trigger to
request and update hardware configuration.
Signed-off-by: Christian Marangi <[email protected]>
---
Documentation/leds/leds-class.rst | 60 +++++++++++++++++++++++++++++++
include/linux/leds.h | 43 ++++++++++++++++++++++
2 files changed, 103 insertions(+)
diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index 984d73499d83..8a23589e9fca 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -200,10 +200,70 @@ The LED must implement 3 main API:
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.
+- hw_control_configure():
+ This will be used to configure the various blink modes LED support
+ in hardware mode.
+
With LED_BLINK_HW_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.
+Where a trigger has support for hardware controlled blink modes,
+hw_control_configure() will be used to check whether a particular blink mode
+is supported and configure the blink mode using various specific command.
+
+hw_control_configure() takes 3 arguments:
+
+- struct led_classdev *led_cdev
+
+- unsigned long flag:
+ This can be used for multiple purpose based on passed command
+ in the 3rd argument of this function.
+ It may be NULL if the 3rd argument doesn't require them.
+
+ The unsigned long flag is specific to the trigger and its meaning
+ change across different triggers.
+ For this exact reason LED driver needs to declare explicit support
+ for the trigger supporting hardware blink mode.
+ The driver should return -EOPNOTSUPP if asked to enter in hardware
+ blink mode with an unsupported trigger.
+
+ The LED driver may also report -EOPNOTSUPP if the requested flag
+ are rejected and can't be handled in hw blink mode by the LED.
+
+ Flag can both be a single blink mode or a set of multiple blink
+ mode. LED driver must be able to handle both cases.
+
+- enum led_blink_hw_cmd cmd:
+ This is used to request to the LED driver various operation.
+
+ They may return -EOPNOTSUPP or -EINVAL based on the provided flags.
+
+hw_control_configure() supports the following cmd:
+
+- LED_BLINK_HW_ENABLE:
+ enable the blink mode requested in flag. Returns 0 or a negative
+ error.
+
+- LED_BLINK_HW_DISABLE:
+ disable the blink mode requested in flag. Returns 0 or a negative
+ error.
+
+- LED_BLINK_HW_STATUS:
+ read the status of the blink mode requested in flag. Return a mask
+ of the enabled blink mode requested in flag or a negative error.
+
+- LED_BLINK_HW_SUPPORTED:
+ ask the LED driver if the blink mode requested in flag is supported.
+ Return 1 if supported or a negative error in any other case.
+
+- LED_BLINK_HW_RESET:
+ reset any blink mode currently active. Value in flag are ignored.
+ Return 0 or a negative error.
+
+ LED driver can set the blink mode to a default state or keep everything
+ disabled.
+
Known Issues
============
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 5c360fba9ccf..c25558ca5f85 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -94,6 +94,14 @@ enum led_blink_modes {
LED_BLINK_SWHW_CONTROLLED,
};
+enum led_blink_hw_cmd {
+ LED_BLINK_HW_ENABLE, /* Enable the hardware blink mode */
+ LED_BLINK_HW_DISABLE, /* Disable the hardware blink mode */
+ LED_BLINK_HW_STATUS, /* Read the status of the hardware blink mode */
+ LED_BLINK_HW_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
+ LED_BLINK_HW_RESET, /* Reset any hardware blink active */
+};
+
struct led_classdev {
const char *name;
unsigned int brightness;
@@ -199,6 +207,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 led_blink_hw_cmd cmd);
#endif
#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -473,6 +492,30 @@ 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 == LED_BLINK_SW_CONTROLLED)
+ return false;
+
+ ret = led_cdev->hw_control_configure(led_cdev, flag, LED_BLINK_HW_SUPPORTED);
+ if (ret > 0)
+ return true;
+
+ return false;
+}
+#else
+static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev,
+ unsigned long flag)
+{
+ return false;
+}
+#endif
+
/**
* led_trigger_rename_static - rename a trigger
* @name: the new trigger name
--
2.38.1
Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool
that will be true or false based on the carrier link. No functional
change intended.
Signed-off-by: Christian Marangi <[email protected]>
---
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.38.1
Rename NETDEV trigger enum modes to a more symbolic name and move them
in leds.h to make them accessible by any user.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 53 +++++++++------------------
include/linux/leds.h | 7 ++++
2 files changed, 25 insertions(+), 35 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 66a81cc9b64d..6872da08676b 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -51,15 +51,6 @@ struct led_netdev_data {
unsigned long mode;
bool carrier_link_up;
-#define NETDEV_LED_LINK 0
-#define NETDEV_LED_TX 1
-#define NETDEV_LED_RX 2
-};
-
-enum netdev_led_attr {
- NETDEV_ATTR_LINK,
- NETDEV_ATTR_TX,
- NETDEV_ATTR_RX
};
static void set_baseline_state(struct led_netdev_data *trigger_data)
@@ -76,7 +67,7 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
if (!trigger_data->carrier_link_up) {
led_set_brightness(led_cdev, LED_OFF);
} else {
- if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
+ if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
led_set_brightness(led_cdev,
led_cdev->blink_brightness);
else
@@ -85,8 +76,8 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
/* If we are looking for RX/TX start periodically
* checking stats
*/
- if (test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
- test_bit(NETDEV_LED_RX, &trigger_data->mode))
+ if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ||
+ test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
schedule_delayed_work(&trigger_data->work, 0);
}
}
@@ -146,20 +137,16 @@ static ssize_t device_name_store(struct device *dev,
static DEVICE_ATTR_RW(device_name);
static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
- enum netdev_led_attr attr)
+ enum led_trigger_netdev_modes attr)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
int bit;
switch (attr) {
- case NETDEV_ATTR_LINK:
- bit = NETDEV_LED_LINK;
- break;
- case NETDEV_ATTR_TX:
- bit = NETDEV_LED_TX;
- break;
- case NETDEV_ATTR_RX:
- bit = NETDEV_LED_RX;
+ case TRIGGER_NETDEV_LINK:
+ case TRIGGER_NETDEV_TX:
+ case TRIGGER_NETDEV_RX:
+ bit = attr;
break;
default:
return -EINVAL;
@@ -169,7 +156,7 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
}
static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
- size_t size, enum netdev_led_attr attr)
+ size_t size, enum led_trigger_netdev_modes attr)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
unsigned long state;
@@ -181,14 +168,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
return ret;
switch (attr) {
- case NETDEV_ATTR_LINK:
- bit = NETDEV_LED_LINK;
- break;
- case NETDEV_ATTR_TX:
- bit = NETDEV_LED_TX;
- break;
- case NETDEV_ATTR_RX:
- bit = NETDEV_LED_RX;
+ case TRIGGER_NETDEV_LINK:
+ case TRIGGER_NETDEV_TX:
+ case TRIGGER_NETDEV_RX:
+ bit = attr;
break;
default:
return -EINVAL;
@@ -358,21 +341,21 @@ static void netdev_trig_work(struct work_struct *work)
}
/* If we are not looking for RX/TX then return */
- if (!test_bit(NETDEV_LED_TX, &trigger_data->mode) &&
- !test_bit(NETDEV_LED_RX, &trigger_data->mode))
+ if (!test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
+ !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
return;
dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
new_activity =
- (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
+ (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ?
dev_stats->tx_packets : 0) +
- (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
+ (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) ?
dev_stats->rx_packets : 0);
if (trigger_data->last_activity != new_activity) {
led_stop_software_blink(trigger_data->led_cdev);
- invert = test_bit(NETDEV_LED_LINK, &trigger_data->mode);
+ invert = test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode);
interval = jiffies_to_msecs(
atomic_read(&trigger_data->interval));
/* base state is ON (link present) */
diff --git a/include/linux/leds.h b/include/linux/leds.h
index c25558ca5f85..a31f158e5351 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -571,6 +571,13 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
#endif /* CONFIG_LEDS_TRIGGERS */
+/* Trigger specific enum */
+enum led_trigger_netdev_modes {
+ TRIGGER_NETDEV_LINK = 1,
+ TRIGGER_NETDEV_TX,
+ TRIGGER_NETDEV_RX,
+};
+
/* Trigger specific functions */
#ifdef CONFIG_LEDS_TRIGGER_DISK
void ledtrig_disk_activity(bool write);
--
2.38.1
Some LEDs may require to sleep to apply their hardware rules. Convert to
mutex lock to fix warning for sleeping under spinlock softirq.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d85be325e492..6dd04f4d70ea 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,7 +20,7 @@
#include <linux/list.h>
#include <linux/module.h>
#include <linux/netdevice.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
#include <linux/timer.h>
#include "../leds.h"
@@ -40,7 +40,7 @@
struct led_netdev_data {
enum led_blink_modes blink_mode;
- spinlock_t lock;
+ struct mutex lock;
struct delayed_work work;
struct notifier_block notifier;
@@ -191,9 +191,9 @@ static ssize_t device_name_show(struct device *dev,
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
ssize_t len;
- spin_lock_bh(&trigger_data->lock);
+ mutex_lock(&trigger_data->lock);
len = sprintf(buf, "%s\n", trigger_data->device_name);
- spin_unlock_bh(&trigger_data->lock);
+ mutex_unlock(&trigger_data->lock);
return len;
}
@@ -211,7 +211,7 @@ static ssize_t device_name_store(struct device *dev,
cancel_delayed_work_sync(&trigger_data->work);
- spin_lock_bh(&trigger_data->lock);
+ mutex_lock(&trigger_data->lock);
/* Backup old device name and save old net */
old_net = trigger_data->net_dev;
@@ -236,7 +236,7 @@ static ssize_t device_name_store(struct device *dev,
trigger_data->net_dev = old_net;
memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
- spin_unlock_bh(&trigger_data->lock);
+ mutex_unlock(&trigger_data->lock);
return -EINVAL;
}
@@ -250,7 +250,7 @@ static ssize_t device_name_store(struct device *dev,
trigger_data->last_activity = 0;
set_baseline_state(trigger_data);
- spin_unlock_bh(&trigger_data->lock);
+ mutex_unlock(&trigger_data->lock);
return size;
}
@@ -412,7 +412,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
cancel_delayed_work_sync(&trigger_data->work);
- spin_lock_bh(&trigger_data->lock);
+ mutex_lock(&trigger_data->lock);
trigger_data->carrier_link_up = false;
switch (evt) {
@@ -435,7 +435,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
set_baseline_state(trigger_data);
- spin_unlock_bh(&trigger_data->lock);
+ mutex_unlock(&trigger_data->lock);
return NOTIFY_DONE;
}
@@ -496,7 +496,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
if (!trigger_data)
return -ENOMEM;
- spin_lock_init(&trigger_data->lock);
+ mutex_init(&trigger_data->lock);
trigger_data->notifier.notifier_call = netdev_trig_notify;
trigger_data->notifier.priority = 10;
--
2.38.1
Add avaiable_mode sysfs attr to show and give some details about the
supported modes and how they can be handled by the trigger.
this can be used to get an overview of the different modes currently
available and active.
A mode with [hardware] can hw blink.
A mode with [software] con blink with sw support.
A mode with [hardware][software] support both mode but will fallback to
sw mode if needed.
A mode with [unavailable] will reject any option and can't be enabled
due to hw limitation or current configuration.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 41 +++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 6dd04f4d70ea..b992d617f406 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -35,6 +35,8 @@
* (has carrier) or not
* tx - LED blinks on transmitted data
* rx - LED blinks on receive data
+ * available_mode - Display available mode and how they can be handled
+ * by the LED
*
*/
@@ -382,12 +384,51 @@ static ssize_t interval_store(struct device *dev,
static DEVICE_ATTR_RW(interval);
+static ssize_t available_mode_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+ struct netdev_led_attr_detail *detail;
+ bool support_hw_mode;
+ int i, len = 0;
+
+ for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+ detail = &attr_details[i];
+ support_hw_mode = led_trigger_blink_mode_is_supported(trigger_data->led_cdev,
+ BIT(detail->bit));
+
+ len += sprintf(buf + len, "%s ", detail->name);
+
+ if (detail->hardware_only) {
+ if (trigger_data->net_dev || !support_hw_mode)
+ len += sprintf(buf + len, "[unavailable]");
+ else
+ len += sprintf(buf + len, "[hardware]");
+ } else {
+ len += sprintf(buf + len, "[software]");
+
+ if (support_hw_mode && !trigger_data->net_dev)
+ len += sprintf(buf + len, "[hardware]");
+ }
+
+ if (test_bit(detail->bit, &trigger_data->mode))
+ len += sprintf(buf + len, "[on]");
+
+ len += sprintf(buf + len, "\n");
+ }
+
+ return len;
+}
+
+static DEVICE_ATTR_RO(available_mode);
+
static struct attribute *netdev_trig_attrs[] = {
&dev_attr_device_name.attr,
&dev_attr_link.attr,
&dev_attr_rx.attr,
&dev_attr_tx.attr,
&dev_attr_interval.attr,
+ &dev_attr_available_mode.attr,
NULL
};
ATTRIBUTE_GROUPS(netdev_trig);
--
2.38.1
Add hardware control support for the Netdev trigger.
The trigger on config change will check if the requested trigger can set
to blink mode using LED hardware mode and if every blink mode is supported,
the trigger will enable hardware mode with the requested configuration.
If there is at least one trigger that is not supported and can't run in
hardware mode, then software mode will be used instead.
A validation is done on every value change and on fail the old value is
restored and -EINVAL is returned.
In HW blink mode interval setting is not supported as it's handled
internally.
To use HW blink mode, dev MUST be empty. If set, SW blink mode is
forced.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 175 ++++++++++++++++++++++++--
1 file changed, 165 insertions(+), 10 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index dd63cadb896e..d85be325e492 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -28,7 +28,9 @@
* Configurable sysfs attributes:
*
* device_name - network device name to monitor
+ * (not supported in hw mode)
* interval - duration of LED blink, in milliseconds
+ * (not supported in hw mode)
* link - LED's normal state reflects whether the link is up
* (has carrier) or not
* tx - LED blinks on transmitted data
@@ -37,6 +39,7 @@
*/
struct led_netdev_data {
+ enum led_blink_modes blink_mode;
spinlock_t lock;
struct delayed_work work;
@@ -53,11 +56,111 @@ struct led_netdev_data {
bool carrier_link_up;
};
+struct netdev_led_attr_detail {
+ char *name;
+ bool hardware_only;
+ enum led_trigger_netdev_modes bit;
+};
+
+static struct netdev_led_attr_detail attr_details[] = {
+ { .name = "link", .bit = TRIGGER_NETDEV_LINK},
+ { .name = "tx", .bit = TRIGGER_NETDEV_TX},
+ { .name = "rx", .bit = TRIGGER_NETDEV_RX},
+};
+
+static bool validate_baseline_state(struct led_netdev_data *trigger_data)
+{
+ struct led_classdev *led_cdev = trigger_data->led_cdev;
+ unsigned long hw_blink_modes = 0, sw_blink_modes = 0;
+ struct netdev_led_attr_detail *detail;
+ bool force_sw = false;
+ int i;
+
+ /* Check if we need to force sw mode for some feature */
+ if (trigger_data->net_dev)
+ force_sw = true;
+
+ /* Hardware only controlled LED can't run in sw mode */
+ if (force_sw && led_cdev->blink_mode == LED_BLINK_HW_CONTROLLED)
+ return false;
+
+ /* Check each attr and make sure they are all supported */
+ for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+ detail = &attr_details[i];
+
+ /* Mode not active, skip */
+ if (!test_bit(detail->bit, &trigger_data->mode))
+ continue;
+
+ /* Hardware only mode enabled on software controlled LED */
+ if ((force_sw || led_cdev->blink_mode == LED_BLINK_SW_CONTROLLED) &&
+ detail->hardware_only)
+ return false;
+
+ /* Check if the mode supports hardware mode */
+ if (led_cdev->blink_mode != LED_BLINK_SW_CONTROLLED) {
+ /* Track modes that should be handled by sw */
+ if (force_sw) {
+ sw_blink_modes |= BIT(detail->bit);
+ continue;
+ }
+
+ /* Check if the mode is supported */
+ if (led_trigger_blink_mode_is_supported(led_cdev, BIT(detail->bit)))
+ hw_blink_modes |= BIT(detail->bit);
+ } else {
+ sw_blink_modes |= BIT(detail->bit);
+ }
+ }
+
+ /* We can't run modes handled by both software and hardware. */
+ if (hw_blink_modes && sw_blink_modes)
+ return false;
+
+ /* Make sure we support each requested mode */
+ if (hw_blink_modes && hw_blink_modes != trigger_data->mode)
+ return false;
+
+ /* Modes are valid. Decide now the running mode to later
+ * set the baseline.
+ */
+ if (sw_blink_modes)
+ trigger_data->blink_mode = LED_BLINK_SW_CONTROLLED;
+ else
+ trigger_data->blink_mode = LED_BLINK_HW_CONTROLLED;
+
+ return true;
+}
+
static void set_baseline_state(struct led_netdev_data *trigger_data)
{
+ int i;
int current_brightness;
+ struct netdev_led_attr_detail *detail;
struct led_classdev *led_cdev = trigger_data->led_cdev;
+ /* Modes already validated. Directly apply hw trigger modes */
+ if (trigger_data->blink_mode == LED_BLINK_HW_CONTROLLED) {
+ /* We are refreshing the blink modes. Reset them */
+ led_cdev->hw_control_configure(led_cdev, 0,
+ LED_BLINK_HW_RESET);
+
+ for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+ detail = &attr_details[i];
+
+ if (!test_bit(detail->bit, &trigger_data->mode))
+ continue;
+
+ led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
+ LED_BLINK_HW_ENABLE);
+ }
+
+ led_cdev->hw_control_start(led_cdev);
+
+ return;
+ }
+
+ /* Handle trigger modes by software */
current_brightness = led_cdev->brightness;
if (current_brightness)
led_cdev->blink_brightness = current_brightness;
@@ -100,6 +203,8 @@ static ssize_t device_name_store(struct device *dev,
size_t size)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+ char old_device_name[IFNAMSIZ];
+ struct net_device *old_net;
if (size >= IFNAMSIZ)
return -EINVAL;
@@ -108,11 +213,12 @@ static ssize_t device_name_store(struct device *dev,
spin_lock_bh(&trigger_data->lock);
- if (trigger_data->net_dev) {
- dev_put(trigger_data->net_dev);
- trigger_data->net_dev = NULL;
- }
+ /* Backup old device name and save old net */
+ old_net = trigger_data->net_dev;
+ trigger_data->net_dev = NULL;
+ memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
+ /* Set the new device name */
memcpy(trigger_data->device_name, buf, size);
trigger_data->device_name[size] = 0;
if (size > 0 && trigger_data->device_name[size - 1] == '\n')
@@ -122,6 +228,21 @@ static ssize_t device_name_store(struct device *dev,
trigger_data->net_dev =
dev_get_by_name(&init_net, trigger_data->device_name);
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old net_dev and device_name */
+ dev_put(trigger_data->net_dev);
+
+ /* Restore device settings */
+ trigger_data->net_dev = old_net;
+ memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
+
+ spin_unlock_bh(&trigger_data->lock);
+ return -EINVAL;
+ }
+
+ /* Everything is ok. We can drop reference to the old net */
+ dev_put(old_net);
+
trigger_data->carrier_link_up = false;
if (trigger_data->net_dev != NULL)
trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
@@ -159,7 +280,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
size_t size, enum led_trigger_netdev_modes attr)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
- unsigned long state;
+ unsigned long state, old_mode = trigger_data->mode;
int ret;
int bit;
@@ -184,6 +305,12 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
else
clear_bit(bit, &trigger_data->mode);
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old mode on validation fail */
+ trigger_data->mode = old_mode;
+ return -EINVAL;
+ }
+
set_baseline_state(trigger_data);
return size;
@@ -220,6 +347,8 @@ static ssize_t interval_store(struct device *dev,
size_t size)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+ int old_interval = atomic_read(&trigger_data->interval);
+ u32 old_mode = trigger_data->mode;
unsigned long value;
int ret;
@@ -228,13 +357,26 @@ static ssize_t interval_store(struct device *dev,
return ret;
/* impose some basic bounds on the timer interval */
- if (value >= 5 && value <= 10000) {
- cancel_delayed_work_sync(&trigger_data->work);
+ if (value < 5 || value > 10000)
+ return -EINVAL;
+
+ /* With hw blink the blink interval is handled internally */
+ if (trigger_data->blink_mode == LED_BLINK_HW_CONTROLLED)
+ return -EINVAL;
+
+ cancel_delayed_work_sync(&trigger_data->work);
+
+ atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
- atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
- set_baseline_state(trigger_data); /* resets timer */
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old interval on validation error */
+ atomic_set(&trigger_data->interval, old_interval);
+ trigger_data->mode = old_mode;
+ return -EINVAL;
}
+ set_baseline_state(trigger_data); /* resets timer */
+
return size;
}
@@ -368,13 +510,25 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
trigger_data->mode = 0;
atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
trigger_data->last_activity = 0;
+ if (led_cdev->blink_mode != LED_BLINK_SW_CONTROLLED) {
+ /* With hw mode enabled reset any rule set by default */
+ if (led_cdev->hw_control_status(led_cdev)) {
+ rc = led_cdev->hw_control_configure(led_cdev, 0,
+ LED_BLINK_HW_RESET);
+ if (rc)
+ goto err;
+ }
+ }
led_set_trigger_data(led_cdev, trigger_data);
rc = register_netdevice_notifier(&trigger_data->notifier);
if (rc)
- kfree(trigger_data);
+ goto err;
+ return 0;
+err:
+ kfree(trigger_data);
return rc;
}
@@ -394,6 +548,7 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
static struct led_trigger netdev_led_trigger = {
.name = "netdev",
+ .supported_blink_modes = LED_TRIGGER_SWHW,
.activate = netdev_trig_activate,
.deactivate = netdev_trig_deactivate,
.groups = netdev_trig_groups,
--
2.38.1
Convert link tx and rx device attr to a common macro to reduce common
code and in preparation for additional attr.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 57 ++++++++-------------------
1 file changed, 16 insertions(+), 41 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 6872da08676b..dd63cadb896e 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -189,47 +189,22 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
return size;
}
-static ssize_t link_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return netdev_led_attr_show(dev, buf, NETDEV_ATTR_LINK);
-}
-
-static ssize_t link_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
-{
- return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_LINK);
-}
-
-static DEVICE_ATTR_RW(link);
-
-static ssize_t tx_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return netdev_led_attr_show(dev, buf, NETDEV_ATTR_TX);
-}
-
-static ssize_t tx_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
-{
- return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_TX);
-}
-
-static DEVICE_ATTR_RW(tx);
-
-static ssize_t rx_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return netdev_led_attr_show(dev, buf, NETDEV_ATTR_RX);
-}
-
-static ssize_t rx_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
-{
- return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_RX);
-}
-
-static DEVICE_ATTR_RW(rx);
+#define DEFINE_NETDEV_TRIGGER(trigger_name, trigger) \
+ static ssize_t trigger_name##_show(struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+ { \
+ return netdev_led_attr_show(dev, buf, trigger); \
+ } \
+ static ssize_t trigger_name##_store(struct device *dev, \
+ struct device_attribute *attr, const char *buf, size_t size) \
+ { \
+ return netdev_led_attr_store(dev, buf, size, trigger); \
+ } \
+ static DEVICE_ATTR_RW(trigger_name)
+
+DEFINE_NETDEV_TRIGGER(link, TRIGGER_NETDEV_LINK);
+DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
+DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
static ssize_t interval_show(struct device *dev,
struct device_attribute *attr, char *buf)
--
2.38.1
Add additional hardware only triggers commonly supported by switch LEDs.
Additional modes:
link_10: LED on with link up AND speed 10mbps
link_100: LED on with link up AND speed 100mbps
link_1000: LED on with link up AND speed 1000mbps
half_duplex: LED on with link up AND half_duplex mode
full_duplex: LED on with link up AND full duplex mode
activity: LED blink on tx or rx event
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 58 +++++++++++++++++++++++++++
include/linux/leds.h | 6 +++
2 files changed, 64 insertions(+)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index b992d617f406..b229bdb69501 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -33,6 +33,17 @@
* (not supported in hw mode)
* link - LED's normal state reflects whether the link is up
* (has carrier) or not
+ * link_10 - LED's normal state reflects whether the link is
+ * up and at 10mbps speed (hardware only)
+ * link_100 - LED's normal state reflects whether the link is
+ * up and at 100mbps speed (hardware only)
+ * link_1000 - LED's normal state reflects whether the link is
+ * up and at 1000mbps speed (hardware only)
+ * half_duplex - LED's normal state reflects whether the link is
+ * up and hafl duplex (hardware only)
+ * full_duplex - LED's normal state reflects whether the link is
+ * up and full duplex (hardware only)
+ * activity - LED's blinks on transmitted or received data (hardware only)
* tx - LED blinks on transmitted data
* rx - LED blinks on receive data
* available_mode - Display available mode and how they can be handled
@@ -66,6 +77,12 @@ struct netdev_led_attr_detail {
static struct netdev_led_attr_detail attr_details[] = {
{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
+ { .name = "link_10", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_10},
+ { .name = "link_100", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_100},
+ { .name = "link_1000", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_1000},
+ { .name = "half_duplex", .hardware_only = true, .bit = TRIGGER_NETDEV_HALF_DUPLEX},
+ { .name = "full_duplex", .hardware_only = true, .bit = TRIGGER_NETDEV_FULL_DUPLEX},
+ { .name = "activity", .hardware_only = true, .bit = TRIGGER_NETDEV_ACTIVITY },
{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
};
@@ -123,6 +140,23 @@ static bool validate_baseline_state(struct led_netdev_data *trigger_data)
if (hw_blink_modes && hw_blink_modes != trigger_data->mode)
return false;
+ /* Check conflicts single rx or tx can't be active if activity is
+ * active.
+ */
+ if (test_bit(TRIGGER_NETDEV_ACTIVITY, &hw_blink_modes) &&
+ (test_bit(TRIGGER_NETDEV_TX, &hw_blink_modes) ||
+ test_bit(TRIGGER_NETDEV_RX, &hw_blink_modes)))
+ return false;
+
+ /* Check conflicts single link speed can't be active if link is
+ * active.
+ */
+ if (test_bit(TRIGGER_NETDEV_LINK, &hw_blink_modes) &&
+ (test_bit(TRIGGER_NETDEV_LINK_10, &hw_blink_modes) ||
+ test_bit(TRIGGER_NETDEV_LINK_100, &hw_blink_modes) ||
+ test_bit(TRIGGER_NETDEV_LINK_1000, &hw_blink_modes)))
+ return false;
+
/* Modes are valid. Decide now the running mode to later
* set the baseline.
*/
@@ -267,6 +301,12 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
switch (attr) {
case TRIGGER_NETDEV_LINK:
+ case TRIGGER_NETDEV_LINK_10:
+ case TRIGGER_NETDEV_LINK_100:
+ case TRIGGER_NETDEV_LINK_1000:
+ case TRIGGER_NETDEV_HALF_DUPLEX:
+ case TRIGGER_NETDEV_FULL_DUPLEX:
+ case TRIGGER_NETDEV_ACTIVITY:
case TRIGGER_NETDEV_TX:
case TRIGGER_NETDEV_RX:
bit = attr;
@@ -292,6 +332,12 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
switch (attr) {
case TRIGGER_NETDEV_LINK:
+ case TRIGGER_NETDEV_LINK_10:
+ case TRIGGER_NETDEV_LINK_100:
+ case TRIGGER_NETDEV_LINK_1000:
+ case TRIGGER_NETDEV_HALF_DUPLEX:
+ case TRIGGER_NETDEV_FULL_DUPLEX:
+ case TRIGGER_NETDEV_ACTIVITY:
case TRIGGER_NETDEV_TX:
case TRIGGER_NETDEV_RX:
bit = attr;
@@ -332,6 +378,12 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
static DEVICE_ATTR_RW(trigger_name)
DEFINE_NETDEV_TRIGGER(link, TRIGGER_NETDEV_LINK);
+DEFINE_NETDEV_TRIGGER(link_10, TRIGGER_NETDEV_LINK_10);
+DEFINE_NETDEV_TRIGGER(link_100, TRIGGER_NETDEV_LINK_100);
+DEFINE_NETDEV_TRIGGER(link_1000, TRIGGER_NETDEV_LINK_1000);
+DEFINE_NETDEV_TRIGGER(half_duplex, TRIGGER_NETDEV_HALF_DUPLEX);
+DEFINE_NETDEV_TRIGGER(full_duplex, TRIGGER_NETDEV_FULL_DUPLEX);
+DEFINE_NETDEV_TRIGGER(activity, TRIGGER_NETDEV_ACTIVITY);
DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
@@ -425,6 +477,12 @@ static DEVICE_ATTR_RO(available_mode);
static struct attribute *netdev_trig_attrs[] = {
&dev_attr_device_name.attr,
&dev_attr_link.attr,
+ &dev_attr_link_10.attr,
+ &dev_attr_link_100.attr,
+ &dev_attr_link_1000.attr,
+ &dev_attr_half_duplex.attr,
+ &dev_attr_full_duplex.attr,
+ &dev_attr_activity.attr,
&dev_attr_rx.attr,
&dev_attr_tx.attr,
&dev_attr_interval.attr,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index a31f158e5351..9071ab768776 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -574,6 +574,12 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
/* Trigger specific enum */
enum led_trigger_netdev_modes {
TRIGGER_NETDEV_LINK = 1,
+ TRIGGER_NETDEV_LINK_10,
+ TRIGGER_NETDEV_LINK_100,
+ TRIGGER_NETDEV_LINK_1000,
+ TRIGGER_NETDEV_HALF_DUPLEX,
+ TRIGGER_NETDEV_FULL_DUPLEX,
+ TRIGGER_NETDEV_ACTIVITY,
TRIGGER_NETDEV_TX,
TRIGGER_NETDEV_RX,
};
--
2.38.1
Add LEDs support for qca8k Switch Family. This will provide the LEDs
hardware API to permit the PHY LED to support hardware mode.
Each port have at least 3 LEDs and they can HW blink, set on/off or
follow blink modes configured with the LED in hardware mode.
Adds support for leds netdev trigger to support hardware triggers.
These LEDs supports only blocking variant of the brightness_set function
since they can sleep during access of the switch leds to set the
brightness.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/dsa/qca/Kconfig | 9 +
drivers/net/dsa/qca/Makefile | 1 +
drivers/net/dsa/qca/qca8k-8xxx.c | 4 +
drivers/net/dsa/qca/qca8k-leds.c | 419 +++++++++++++++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 69 +++++
5 files changed, 502 insertions(+)
create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
index ba339747362c..be67164a444e 100644
--- a/drivers/net/dsa/qca/Kconfig
+++ b/drivers/net/dsa/qca/Kconfig
@@ -15,3 +15,12 @@ config NET_DSA_QCA8K
help
This enables support for the Qualcomm Atheros QCA8K Ethernet
switch chips.
+
+config NET_DSA_QCA8K_LEDS_SUPPORT
+ tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
+ depends on NET_DSA_QCA8K
+ select LEDS_OFFLOAD_TRIGGERS
+ help
+ This enabled support for LEDs present on the Qualcomm Atheros
+ QCA8K Ethernet switch chips. This require the LEDs offload
+ triggers support as it can run in offload mode.
diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
index 701f1d199e93..330ae389e489 100644
--- a/drivers/net/dsa/qca/Makefile
+++ b/drivers/net/dsa/qca/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
qca8k-y += qca8k-common.o qca8k-8xxx.o
+obj-$(CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT) += qca8k-leds.o
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 55df4479ea30..a983f7c96b0d 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1798,6 +1798,10 @@ qca8k_setup(struct dsa_switch *ds)
if (ret)
return ret;
+ ret = qca8k_setup_led_ctrl(priv);
+ if (ret)
+ return ret;
+
qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
new file mode 100644
index 000000000000..3653ad9bd90b
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -0,0 +1,419 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/regmap.h>
+#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)
+{
+ switch (port_num) {
+ case 0:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
+ break;
+ case 1:
+ case 2:
+ case 3:
+ /* Port 123 are controlled on a different reg */
+ reg_info->reg = QCA8K_LED_CTRL_REG(3);
+ reg_info->shift = QCA8K_LED_PHY123_PATTERN_EN_SHIFT(port_num, led_num);
+ break;
+ case 4:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
+ 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 = QCA8K_LED_PHY4_CONTROL_RULE_SHIFT;
+ else
+ reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
+
+ return 0;
+}
+
+static int
+qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger, u32 *mask)
+{
+ /* Parsing specific to netdev trigger */
+ if (test_bit(TRIGGER_NETDEV_LINK, &rules))
+ *offload_trigger |= QCA8K_LED_LINK_10M_EN_MASK |
+ QCA8K_LED_LINK_100M_EN_MASK |
+ QCA8K_LED_LINK_1000M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+ *offload_trigger |= QCA8K_LED_LINK_10M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+ *offload_trigger |= QCA8K_LED_LINK_100M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+ *offload_trigger |= QCA8K_LED_LINK_1000M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+ *offload_trigger |= QCA8K_LED_HALF_DUPLEX_MASK;
+ if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+ *offload_trigger |= QCA8K_LED_FULL_DUPLEX_MASK;
+ if (test_bit(TRIGGER_NETDEV_ACTIVITY, &rules))
+ *offload_trigger |= QCA8K_LED_TX_BLINK_MASK |
+ QCA8K_LED_RX_BLINK_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 (rules && !*offload_trigger)
+ return -EOPNOTSUPP;
+
+ *mask = *offload_trigger;
+
+ return 0;
+}
+
+static int
+qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
+ enum led_blink_hw_cmd cmd)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+ struct led_trigger *trigger = ldev->trigger;
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 offload_trigger = 0, mask, val;
+ int ret;
+
+ /* Check trigger compatibility */
+ if (strcmp(trigger->name, "netdev"))
+ return -EOPNOTSUPP;
+
+ if (!strcmp(trigger->name, "netdev"))
+ ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
+
+ if (ret)
+ return ret;
+
+ qca8k_get_control_led_reg(led->port_num, led->led_num, ®_info);
+
+ switch (cmd) {
+ case LED_BLINK_HW_SUPPORTED:
+ /* We reach this point, we are sure the trigger is supported */
+ return 1;
+ case LED_BLINK_HW_RESET:
+ /* We set 4hz by default */
+ ret = regmap_update_bits(priv->regmap, reg_info.reg,
+ QCA8K_LED_RULE_MASK << reg_info.shift,
+ QCA8K_LED_BLINK_4HZ << reg_info.shift);
+ break;
+ case LED_BLINK_HW_ENABLE:
+ ret = regmap_update_bits(priv->regmap, reg_info.reg,
+ mask << reg_info.shift,
+ offload_trigger << reg_info.shift);
+ break;
+ case LED_BLINK_HW_DISABLE:
+ ret = regmap_update_bits(priv->regmap, reg_info.reg,
+ mask << reg_info.shift,
+ 0);
+ break;
+ case LED_BLINK_HW_STATUS:
+ ret = regmap_read(priv->regmap, reg_info.reg, &val);
+ if (ret)
+ return ret;
+
+ val >>= reg_info.shift;
+ val &= offload_trigger;
+
+ return val;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return ret;
+}
+
+static int
+qca8k_led_brightness_set(struct qca8k_led *led,
+ enum led_brightness brightness)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 mask, val = QCA8K_LED_ALWAYS_OFF;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ if (brightness)
+ val = QCA8K_LED_ALWAYS_ON;
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ mask = QCA8K_LED_PATTERN_EN_MASK;
+ val <<= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ return regmap_update_bits(priv->regmap, reg_info.reg,
+ mask << reg_info.shift,
+ val << reg_info.shift);
+}
+
+static int
+qca8k_cled_brightness_set_blocking(struct led_classdev *ldev,
+ enum led_brightness brightness)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ return qca8k_led_brightness_set(led, brightness);
+}
+
+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, ®_info);
+
+ ret = regmap_read(priv->regmap, reg_info.reg, &val);
+ if (ret)
+ return 0;
+
+ val >>= reg_info.shift;
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ val &= QCA8K_LED_PATTERN_EN_MASK;
+ val >>= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ return val > 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);
+ u32 mask, val = QCA8K_LED_ALWAYS_BLINK_4HZ;
+ 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, ®_info);
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ mask = QCA8K_LED_PATTERN_EN_MASK;
+ val <<= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ regmap_update_bits(priv->regmap, reg_info.reg, mask << reg_info.shift,
+ val << 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 mask, val = QCA8K_LED_ALWAYS_OFF;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ if (enable)
+ val = QCA8K_LED_RULE_CONTROLLED;
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ mask = QCA8K_LED_PATTERN_EN_MASK;
+ val <<= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ return regmap_update_bits(priv->regmap, reg_info.reg, mask << reg_info.shift,
+ val << reg_info.shift);
+}
+
+static 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, ®_info);
+
+ regmap_read(priv->regmap, reg_info.reg, &val);
+
+ val >>= reg_info.shift;
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ val &= QCA8K_LED_PATTERN_EN_MASK;
+ val >>= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ return val == QCA8K_LED_RULE_CONTROLLED;
+}
+
+static int
+qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
+{
+ struct fwnode_handle *led = NULL, *leds = NULL;
+ struct led_init_data init_data = { };
+ struct qca8k_led *port_led;
+ int led_num, port_index;
+ const char *state;
+ int ret;
+
+ leds = fwnode_get_named_child_node(port, "leds");
+ if (!leds) {
+ dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
+ port_num);
+ return 0;
+ }
+
+ fwnode_for_each_child_node(leds, led) {
+ /* Reg represent the led number of the port.
+ * Each port can have at least 3 leds attached
+ * Commonly:
+ * 1. is gigabit led
+ * 2. is mbit led
+ * 3. additional status led
+ */
+ if (fwnode_property_read_u32(led, "reg", &led_num))
+ continue;
+
+ if (led_num >= QCA8K_LED_PORT_COUNT) {
+ dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
+ continue;
+ }
+
+ port_index = 3 * port_num + led_num;
+
+ port_led = &priv->ports_led[port_index];
+ port_led->port_num = port_num;
+ port_led->led_num = led_num;
+ port_led->priv = priv;
+
+ ret = fwnode_property_read_string(led, "default-state", &state);
+ if (!ret) {
+ if (!strcmp(state, "on")) {
+ port_led->cdev.brightness = 1;
+ qca8k_led_brightness_set(port_led, 1);
+ } else if (!strcmp(state, "off")) {
+ port_led->cdev.brightness = 0;
+ qca8k_led_brightness_set(port_led, 0);
+ } else if (!strcmp(state, "keep")) {
+ port_led->cdev.brightness =
+ qca8k_led_brightness_get(port_led);
+ }
+ }
+
+ /* 3 brightness settings can be applied from Documentation:
+ * 0 always off
+ * 1 blink at 4Hz
+ * 2 always on
+ * 3 rule controlled
+ * Suppots only 2 mode: (pcb limitation, with always on and blink
+ * only the last led is set to this mode)
+ * 0 always off (sets all leds off)
+ * 3 rule controlled
+ */
+ port_led->cdev.blink_mode = LED_BLINK_SWHW_CONTROLLED;
+ port_led->cdev.max_brightness = 1;
+ port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
+ port_led->cdev.brightness_get = qca8k_cled_brightness_get;
+ port_led->cdev.blink_set = qca8k_cled_blink_set;
+ port_led->cdev.hw_control_start = qca8k_cled_hw_control_start;
+ port_led->cdev.hw_control_stop = qca8k_cled_hw_control_stop;
+ port_led->cdev.hw_control_status = qca8k_cled_hw_control_status;
+ port_led->cdev.hw_control_configure = qca8k_cled_hw_control_configure;
+ init_data.default_label = ":port";
+ init_data.devicename = "qca8k";
+ init_data.fwnode = led;
+
+ ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
+ if (ret)
+ dev_warn(priv->dev, "Failed to int led");
+ }
+
+ return 0;
+}
+
+int
+qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ struct fwnode_handle *mdio, *port;
+ int port_num;
+ int ret;
+
+ mdio = device_get_named_child_node(priv->dev, "mdio");
+ if (!mdio) {
+ dev_info(priv->dev, "No MDIO node specified in device tree!\n");
+ return 0;
+ }
+
+ fwnode_for_each_child_node(mdio, port) {
+ if (fwnode_property_read_u32(port, "reg", &port_num))
+ continue;
+
+ /* Each port can have at least 3 different leds attached */
+ ret = qca8k_parse_port_leds(priv, port, port_num);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 7996975d29d3..b5c0dd95160c 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/regmap.h>
#include <linux/gpio.h>
+#include <linux/leds.h>
#include <linux/dsa/tag_qca.h>
#define QCA8K_ETHERNET_MDIO_PRIORITY 7
@@ -85,6 +86,50 @@
#define QCA8K_MDIO_MASTER_DATA(x) FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
#define QCA8K_MDIO_MASTER_MAX_PORTS 5
#define QCA8K_MDIO_MASTER_MAX_REG 32
+
+/* LED control register */
+#define QCA8K_LED_COUNT 15
+#define QCA8K_LED_PORT_COUNT 3
+#define QCA8K_LED_RULE_COUNT 6
+#define QCA8K_LED_RULE_MAX 11
+
+#define QCA8K_LED_PHY123_PATTERN_EN_SHIFT(_phy, _led) ((((_phy) - 1) * 6) + 8 + (2 * (_led)))
+#define QCA8K_LED_PHY123_PATTERN_EN_MASK GENMASK(1, 0)
+
+#define QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT 0
+#define QCA8K_LED_PHY4_CONTROL_RULE_SHIFT 16
+
+#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
@@ -382,6 +427,19 @@ struct qca8k_pcs {
int port;
};
+struct qca8k_led_pattern_en {
+ u32 reg;
+ u8 shift;
+};
+
+struct qca8k_led {
+ u8 port_num;
+ u8 led_num;
+ u16 old_rule;
+ struct qca8k_priv *priv;
+ struct led_classdev cdev;
+};
+
struct qca8k_priv {
u8 switch_id;
u8 switch_revision;
@@ -406,6 +464,7 @@ struct qca8k_priv {
struct qca8k_pcs pcs_port_0;
struct qca8k_pcs pcs_port_6;
const struct qca8k_match_data *info;
+ struct qca8k_led ports_led[QCA8K_LED_COUNT];
};
struct qca8k_mib_desc {
@@ -511,4 +570,14 @@ int qca8k_port_lag_join(struct dsa_switch *ds, int port, struct dsa_lag lag,
int qca8k_port_lag_leave(struct dsa_switch *ds, int port,
struct dsa_lag lag);
+/* Leds Support function */
+#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
+#else
+static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ return 0;
+}
+#endif
+
#endif /* __QCA8K_H */
--
2.38.1
Document the netdev trigger that makes the LED blink or turn on based on
switch/phy events or an attached network interface.
Signed-off-by: Christian Marangi <[email protected]>
---
Documentation/devicetree/bindings/leds/common.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
index d34bb58c0037..6e016415a4d8 100644
--- a/Documentation/devicetree/bindings/leds/common.yaml
+++ b/Documentation/devicetree/bindings/leds/common.yaml
@@ -98,6 +98,8 @@ properties:
# LED alters the brightness for the specified duration with one software
# timer (requires "led-pattern" property)
- pattern
+ # LED blink and turns on based on netdev events
+ - netdev
- pattern: "^cpu[0-9]*$"
- pattern: "^hci[0-9]+-power$"
# LED is triggered by Bluetooth activity
--
2.38.1
Document support for leds node in phy and add an example for it.
Phy led will have to match led-phy pattern and should be treated as a
generic led.
Signed-off-by: Christian Marangi <[email protected]>
---
.../devicetree/bindings/net/ethernet-phy.yaml | 22 +++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 1327b81f15a2..34a5a688c13b 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -197,6 +197,13 @@ properties:
PHY's that have configurable TX internal delays. If this property is
present then the PHY applies the TX delay.
+ leds:
+ type: object
+
+ patternProperties:
+ '^led-phy(@[a-f0-9]+)?$':
+ $ref: /schemas/leds/common.yaml#
+
required:
- reg
@@ -204,6 +211,8 @@ additionalProperties: true
examples:
- |
+ #include <dt-bindings/leds/common.h>
+
ethernet {
#address-cells = <1>;
#size-cells = <0>;
@@ -219,5 +228,18 @@ examples:
reset-gpios = <&gpio1 4 1>;
reset-assert-us = <1000>;
reset-deassert-us = <2000>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led-phy@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "netdev";
+ };
+ };
};
};
--
2.38.1
Add LEDs definition example for qca8k using the offload trigger as the
default trigger and add all the supported offload triggers by the
switch.
Signed-off-by: Christian Marangi <[email protected]>
---
.../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index 389892592aac..ba3821364039 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -18,6 +18,8 @@ description:
PHY it is connected to. In this config, an internal mdio-bus is registered and
the MDIO master is used for communication. Mixed external and internal
mdio-bus configurations are not supported by the hardware.
+ Each phy have at least 3 LEDs connected and can be declared
+ using the standard LEDs structure.
properties:
compatible:
@@ -117,6 +119,7 @@ unevaluatedProperties: false
examples:
- |
#include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/leds/common.h>
mdio {
#address-cells = <1>;
@@ -276,6 +279,27 @@ examples:
internal_phy_port1: ethernet-phy@0 {
reg = <0>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "netdev";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "netdev";
+ };
+ };
};
internal_phy_port2: ethernet-phy@1 {
--
2.38.1
On Thu, 16 Feb 2023 02:32:29 +0100, Christian Marangi wrote:
> Document support for leds node in phy and add an example for it.
> Phy led will have to match led-phy pattern and should be treated as a
> generic led.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> .../devicetree/bindings/net/ethernet-phy.yaml | 22 +++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ethernet-phy.example.dtb: ethernet-phy@0: leds:led-phy@0:linux,default-trigger: 'oneOf' conditional failed, one must be fixed:
'netdev' is not one of ['backlight', 'default-on', 'heartbeat', 'disk-activity', 'ide-disk', 'timer', 'pattern']
'netdev' does not match '^mmc[0-9]+$'
'netdev' does not match '^cpu[0-9]*$'
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ethernet-phy.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On Wed, Feb 15, 2023 at 08:32:11PM -0600, Rob Herring wrote:
>
> On Thu, 16 Feb 2023 02:32:29 +0100, Christian Marangi wrote:
> > Document support for leds node in phy and add an example for it.
> > Phy led will have to match led-phy pattern and should be treated as a
> > generic led.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
> > .../devicetree/bindings/net/ethernet-phy.yaml | 22 +++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ethernet-phy.example.dtb: ethernet-phy@0: leds:led-phy@0:linux,default-trigger: 'oneOf' conditional failed, one must be fixed:
> 'netdev' is not one of ['backlight', 'default-on', 'heartbeat', 'disk-activity', 'ide-disk', 'timer', 'pattern']
> 'netdev' does not match '^mmc[0-9]+$'
> 'netdev' does not match '^cpu[0-9]*$'
> From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>
Hi, I could be wrong but this should be fixed by the previous patch that
adds netdev to the trigger list.
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>
--
Ansuel
On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote:
> This is another attempt on adding this feature on LEDs, hoping this is
> the right time and someone finally notice this.
Hi Christian
Thanks for keeping working on this.
I want to review it, and maybe implement LED support in a PHY
driver. But i'm busy with reworking EEE at the moment.
The merge window is about to open, so patches are not going to be
accepted for the next two weeks. So i will take a look within that
time and give you feedback.
Andrew
On Fri, Feb 17, 2023 at 03:30:13PM +0100, Andrew Lunn wrote:
> On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote:
> > This is another attempt on adding this feature on LEDs, hoping this is
> > the right time and someone finally notice this.
>
> Hi Christian
>
> Thanks for keeping working on this.
>
> I want to review it, and maybe implement LED support in a PHY
> driver. But i'm busy with reworking EEE at the moment.
>
> The merge window is about to open, so patches are not going to be
> accepted for the next two weeks. So i will take a look within that
> time and give you feedback.
>
Sure take your time happy to discuss any improvement to this.
--
Ansuel
On Thu, Feb 16, 2023 at 02:32:28AM +0100, Christian Marangi wrote:
> Document the netdev trigger that makes the LED blink or turn on based on
> switch/phy events or an attached network interface.
NAK. What is netdev?
Don't add new linux,default-trigger entries either. We have better ways
to define trigger sources, namely 'trigger-sources'.
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> Documentation/devicetree/bindings/leds/common.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
> index d34bb58c0037..6e016415a4d8 100644
> --- a/Documentation/devicetree/bindings/leds/common.yaml
> +++ b/Documentation/devicetree/bindings/leds/common.yaml
> @@ -98,6 +98,8 @@ properties:
> # LED alters the brightness for the specified duration with one software
> # timer (requires "led-pattern" property)
> - pattern
> + # LED blink and turns on based on netdev events
> + - netdev
> - pattern: "^cpu[0-9]*$"
> - pattern: "^hci[0-9]+-power$"
> # LED is triggered by Bluetooth activity
> --
> 2.38.1
>
On Fri, Feb 17, 2023 at 05:03:46PM -0600, Rob Herring wrote:
> On Thu, Feb 16, 2023 at 02:32:28AM +0100, Christian Marangi wrote:
> > Document the netdev trigger that makes the LED blink or turn on based on
> > switch/phy events or an attached network interface.
>
> NAK. What is netdev?
But netdev is a trigger, nothing new. Actually it was never documented.
Is the linux,default-trigger getting deprecated?
>
> Don't add new linux,default-trigger entries either. We have better ways
> to define trigger sources, namely 'trigger-sources'.
>
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
> > Documentation/devicetree/bindings/leds/common.yaml | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
> > index d34bb58c0037..6e016415a4d8 100644
> > --- a/Documentation/devicetree/bindings/leds/common.yaml
> > +++ b/Documentation/devicetree/bindings/leds/common.yaml
> > @@ -98,6 +98,8 @@ properties:
> > # LED alters the brightness for the specified duration with one software
> > # timer (requires "led-pattern" property)
> > - pattern
> > + # LED blink and turns on based on netdev events
> > + - netdev
> > - pattern: "^cpu[0-9]*$"
> > - pattern: "^hci[0-9]+-power$"
> > # LED is triggered by Bluetooth activity
> > --
> > 2.38.1
> >
--
Ansuel
On Thu, Feb 16, 2023 at 11:00:49AM +0100, Christian Marangi wrote:
> On Wed, Feb 15, 2023 at 08:32:11PM -0600, Rob Herring wrote:
> >
> > On Thu, 16 Feb 2023 02:32:29 +0100, Christian Marangi wrote:
> > > Document support for leds node in phy and add an example for it.
> > > Phy led will have to match led-phy pattern and should be treated as a
> > > generic led.
> > >
> > > Signed-off-by: Christian Marangi <[email protected]>
> > > ---
> > > .../devicetree/bindings/net/ethernet-phy.yaml | 22 +++++++++++++++++++
> > > 1 file changed, 22 insertions(+)
> > >
> >
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ethernet-phy.example.dtb: ethernet-phy@0: leds:led-phy@0:linux,default-trigger: 'oneOf' conditional failed, one must be fixed:
> > 'netdev' is not one of ['backlight', 'default-on', 'heartbeat', 'disk-activity', 'ide-disk', 'timer', 'pattern']
> > 'netdev' does not match '^mmc[0-9]+$'
> > 'netdev' does not match '^cpu[0-9]*$'
> > From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> >
>
> Hi, I could be wrong but this should be fixed by the previous patch that
> adds netdev to the trigger list.
If so, then it didn't apply for me which is what PW says. So what tree
does this series apply too? linux-next? That's a tree no one can apply
patches from.
Rob
On Fri, Feb 17, 2023 at 06:58:39AM +0100, Christian Marangi wrote:
> On Fri, Feb 17, 2023 at 05:03:46PM -0600, Rob Herring wrote:
> > On Thu, Feb 16, 2023 at 02:32:28AM +0100, Christian Marangi wrote:
> > > Document the netdev trigger that makes the LED blink or turn on based on
> > > switch/phy events or an attached network interface.
> >
> > NAK. What is netdev?
>
> But netdev is a trigger, nothing new. Actually it was never documented.
Okay, please state that in the commit message.
> Is the linux,default-trigger getting deprecated?
Not quite, but it shouldn't be used for anything tied to a device IMO.
Rob
On Thu, Feb 16, 2023 at 02:32:30AM +0100, Christian Marangi wrote:
> Add LEDs definition example for qca8k using the offload trigger as the
> default trigger and add all the supported offload triggers by the
> switch.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index 389892592aac..ba3821364039 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -18,6 +18,8 @@ description:
> PHY it is connected to. In this config, an internal mdio-bus is registered and
> the MDIO master is used for communication. Mixed external and internal
> mdio-bus configurations are not supported by the hardware.
> + Each phy have at least 3 LEDs connected and can be declared
> + using the standard LEDs structure.
>
> properties:
> compatible:
> @@ -117,6 +119,7 @@ unevaluatedProperties: false
> examples:
> - |
> #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/leds/common.h>
>
> mdio {
> #address-cells = <1>;
> @@ -276,6 +279,27 @@ examples:
>
> internal_phy_port1: ethernet-phy@0 {
> reg = <0>;
> +
> + leds {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led@0 {
> + reg = <0>;
> + color = <LED_COLOR_ID_WHITE>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
> + linux,default-trigger = "netdev";
Wouldn't the default for an ethernet phy controlled LED always be
"netdev"? If a PHY has LED nodes, then just always set it to "netdev"
unless something else is selected.
Rob
On Fri, 17 Feb 2023, Andrew Lunn wrote:
> On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote:
> > This is another attempt on adding this feature on LEDs, hoping this is
> > the right time and someone finally notice this.
>
> Hi Christian
>
> Thanks for keeping working on this.
>
> I want to review it, and maybe implement LED support in a PHY
> driver. But i'm busy with reworking EEE at the moment.
>
> The merge window is about to open, so patches are not going to be
> accepted for the next two weeks. So i will take a look within that
> time and give you feedback.
Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm
going to need all the help I can get with complex submissions such as
these.
--
Lee Jones [李琼斯]
On Wed, Feb 22, 2023 at 03:02:04PM +0000, Lee Jones wrote:
> On Fri, 17 Feb 2023, Andrew Lunn wrote:
>
> > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote:
> > > This is another attempt on adding this feature on LEDs, hoping this is
> > > the right time and someone finally notice this.
> >
> > Hi Christian
> >
> > Thanks for keeping working on this.
> >
> > I want to review it, and maybe implement LED support in a PHY
> > driver. But i'm busy with reworking EEE at the moment.
> >
> > The merge window is about to open, so patches are not going to be
> > accepted for the next two weeks. So i will take a look within that
> > time and give you feedback.
>
> Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm
> going to need all the help I can get with complex submissions such as
> these.
>
Hi Lee,
thanks for stepping in. Just wanted to tell you I got some message with
Andrew to make this thing less problematic and to dry/make it more
review friendly.
We decided on pushing this in 3 step:
1. Propose most basic things for some switch and some PHY. (brightness
and blink_set support only, already supported by LED core)
2. A small series that should be just a cleanup for the netdev trigger
3. Support for hw_control in the most possible clean and way with small
patch to they are not hard to track and understand the concept of this
feature.
I'm starting with the step 1 and sending some of my patch and Andrew
patch to add basic support and I will add you and LED mailing list in
Cc.
Again thanks for starting checking this and feel free to ask any
question about this to me also privately, I'm very open to any help.
--
Ansuel
On Mon, 06 Mar 2023, Christian Marangi wrote:
> On Wed, Feb 22, 2023 at 03:02:04PM +0000, Lee Jones wrote:
> > On Fri, 17 Feb 2023, Andrew Lunn wrote:
> >
> > > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote:
> > > > This is another attempt on adding this feature on LEDs, hoping this is
> > > > the right time and someone finally notice this.
> > >
> > > Hi Christian
> > >
> > > Thanks for keeping working on this.
> > >
> > > I want to review it, and maybe implement LED support in a PHY
> > > driver. But i'm busy with reworking EEE at the moment.
> > >
> > > The merge window is about to open, so patches are not going to be
> > > accepted for the next two weeks. So i will take a look within that
> > > time and give you feedback.
> >
> > Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm
> > going to need all the help I can get with complex submissions such as
> > these.
> >
>
> Hi Lee,
> thanks for stepping in. Just wanted to tell you I got some message with
> Andrew to make this thing less problematic and to dry/make it more
> review friendly.
>
> We decided on pushing this in 3 step:
> 1. Propose most basic things for some switch and some PHY. (brightness
> and blink_set support only, already supported by LED core)
> 2. A small series that should be just a cleanup for the netdev trigger
> 3. Support for hw_control in the most possible clean and way with small
> patch to they are not hard to track and understand the concept of this
> feature.
>
> I'm starting with the step 1 and sending some of my patch and Andrew
> patch to add basic support and I will add you and LED mailing list in
> Cc.
Sounds like a plan. Thank you both.
> Again thanks for starting checking this and feel free to ask any
> question about this to me also privately, I'm very open to any help.
--
Lee Jones [李琼斯]
Hi Christian,
thanks for your patch!
On Thu, Feb 16, 2023 at 2:36 AM Christian Marangi <[email protected]> wrote:
> The current idea is:
> - LED driver implement 3 API (hw_control_status/start/stop).
> They are used to put the LED in hardware mode and to configure the
> various trigger.
> - We have hardware triggers that are used to expose to userspace the
> supported hardware mode and set the hardware mode on trigger
> activation.
> - We can also have triggers that both support hardware and software mode.
> - The LED driver will declare each supported hardware blink mode and
> communicate with the trigger all the supported blink modes that will
> be available by sysfs.
> - A trigger will use blink_set to configure the blink mode to active
> in hardware mode.
> - On hardware trigger activation, only the hardware mode is enabled but
> the blink modes are not configured. The LED driver should reset any
> link mode active by default.
The series looks good as a start.
There are some drivers and HW definitions etc for switch-controlled
LEDs, which is great.
I am a bit reluctant on the ambition to rely on configuration from sysfs
for the triggers, and I am also puzzled to how a certain trigger on a
certain LED is going to associate itself with, say, a certain port.
I want to draw your attention to this recently merged patch series
from Hans de Goede:
https://lore.kernel.org/linux-leds/[email protected]/
This adds the devm_led_get() API which works similar to getting
regulators, clocks, GPIOs or any other resources.
It is not yet (I think) hooked into the device tree framework, but it
supports software nodes so adding DT handling should be sort of
trivial.
I think the ambition should be something like this (conjured example)
for a DSA switch:
platform {
switch {
compatible = "foo";
leds {
#address-cells = <1>;
#size-cells = <0>;
led0: led@0 {
reg = <0>;
color =...
function = ...
function-enumerator = ...
default-state = ...
};
led1: led@1 {
reg = <1>;
color =...
function = ...
function-enumerator = ...
default-state = ...
};
};
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
label = "lan0";
phy-handle = <&phy0>;
leds = <&led0>;
};
port@1 {
reg = <1>;
label = "lan1";
phy-handle = <&phy1>;
leds = <&led0>;
};
};
mdio {
compatible = "foo-mdio";
#address-cells = <1>;
#size-cells = <0>;
phy0: ethernet-phy@0 {
reg = <0>;
};
phy1: ethernet-phy@1 {
reg = <1>;
};
};
};
};
I am not the man to tell whether the leds = <&led0>; phandle should be on
the port or actually on the phy, it may even vary. You guys know the answer
to this.
But certainly something like this resource phandle will be necessary to
assign the right LED to the right port or phy, I hope you were not going
to rely on strings and naming conventions?
Yours,
Linus Walleij
Hi,
On 3/9/23 10:09, Linus Walleij wrote:
> Hi Christian,
>
> thanks for your patch!
>
> On Thu, Feb 16, 2023 at 2:36 AM Christian Marangi <[email protected]> wrote:
>
>> The current idea is:
>> - LED driver implement 3 API (hw_control_status/start/stop).
>> They are used to put the LED in hardware mode and to configure the
>> various trigger.
>> - We have hardware triggers that are used to expose to userspace the
>> supported hardware mode and set the hardware mode on trigger
>> activation.
>> - We can also have triggers that both support hardware and software mode.
>> - The LED driver will declare each supported hardware blink mode and
>> communicate with the trigger all the supported blink modes that will
>> be available by sysfs.
>> - A trigger will use blink_set to configure the blink mode to active
>> in hardware mode.
>> - On hardware trigger activation, only the hardware mode is enabled but
>> the blink modes are not configured. The LED driver should reset any
>> link mode active by default.
>
> The series looks good as a start.
> There are some drivers and HW definitions etc for switch-controlled
> LEDs, which is great.
>
> I am a bit reluctant on the ambition to rely on configuration from sysfs
> for the triggers, and I am also puzzled to how a certain trigger on a
> certain LED is going to associate itself with, say, a certain port.
>
> I want to draw your attention to this recently merged patch series
> from Hans de Goede:
> https://lore.kernel.org/linux-leds/[email protected]/
>
> This adds the devm_led_get() API which works similar to getting
> regulators, clocks, GPIOs or any other resources.
>
> It is not yet (I think) hooked into the device tree framework, but it
> supports software nodes so adding DT handling should be sort of
> trivial.
That series contains this (unmerged) patch to hookup DT handling:
https://lore.kernel.org/linux-leds/[email protected]/
this was not merged because there are no current users, but adding
support is as easy as picking up that patch :)
Note there also already is a devicetree *only*:
struct led_classdev *of_led_get(struct device_node *np, int index);
Since I was working on a x86/ACPI platform I needed something more
generic though and ideally new code would use the generic approach.
Regards,
Hans
>
> I think the ambition should be something like this (conjured example)
> for a DSA switch:
>
> platform {
> switch {
> compatible = "foo";
>
> leds {
> #address-cells = <1>;
> #size-cells = <0>;
> led0: led@0 {
> reg = <0>;
> color =...
> function = ...
> function-enumerator = ...
> default-state = ...
> };
> led1: led@1 {
> reg = <1>;
> color =...
> function = ...
> function-enumerator = ...
> default-state = ...
> };
> };
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> port@0 {
> reg = <0>;
> label = "lan0";
> phy-handle = <&phy0>;
> leds = <&led0>;
> };
> port@1 {
> reg = <1>;
> label = "lan1";
> phy-handle = <&phy1>;
> leds = <&led0>;
> };
> };
>
> mdio {
> compatible = "foo-mdio";
> #address-cells = <1>;
> #size-cells = <0>;
>
> phy0: ethernet-phy@0 {
> reg = <0>;
> };
> phy1: ethernet-phy@1 {
> reg = <1>;
> };
> };
> };
> };
>
> I am not the man to tell whether the leds = <&led0>; phandle should be on
> the port or actually on the phy, it may even vary. You guys know the answer
> to this.
>
> But certainly something like this resource phandle will be necessary to
> assign the right LED to the right port or phy, I hope you were not going
> to rely on strings and naming conventions?
>
> Yours,
> Linus Walleij
>
> I am a bit reluctant on the ambition to rely on configuration from sysfs
> for the triggers, and I am also puzzled to how a certain trigger on a
> certain LED is going to associate itself with, say, a certain port.
Hi Linus
There will need to be a device tree binding for the default
trigger. That is what nearly all the rejected hacks so far have been
about, write registers in the PHYs LEDs registers to put it into a
specific mode. I don't see that part of the overall problem too
problematic, apart from the old issue, is it describing configuration,
not hardware.
As to 'how a certain trigger on a certain LED is going to associate
itself with, say, a certain port' is clearly a property of the
hardware, when offloading is supported. I've not seen a switch you can
arbitrarily assign LEDs to ports. The Marvell switches have the LED
registers within the port registers, for example, two LEDs per port.
>
> I want to draw your attention to this recently merged patch series
> from Hans de Goede:
> https://lore.kernel.org/linux-leds/[email protected]/
>
> This adds the devm_led_get() API which works similar to getting
> regulators, clocks, GPIOs or any other resources.
Interesting. Thanks for pointing this out. But i don't think it is of
interest in our use case, which is hardware offload. For a purely
software controlled LED, where the LED could be anywhere,
devm_led_get() makes sense. But in our case, the LED is in a well
defined place, either the MAC or the PHY, where it has access to the
RX and TX packets, link status etc. So we don't have the problem of
finding it in an arbitrary location.
There is also one additional problem we have, both for MAC and PHY
LEDs. The trigger is ledtrig-netdev. All trigger state revolves around
a netdev. A DSA port is not a netdev. A PHY is not a netdev. Each of
these three things have there own life cycle. Often, a PHY does not
know what netdev it is associated to until the interface is opened,
ie. ip link set eth0 up. But once it is associated, phylib knows this
information, so can return it, without any additional configuration
data in DT. A DSA switch port can be created before the netdev
associated to it is created. But again, the DSA core does know the
mapping between a netdev and a port. Using devm_led_get() does not
gain us anything.
Andrew
On Thu, Mar 9, 2023 at 4:22 PM Andrew Lunn <[email protected]> wrote:
> As to 'how a certain trigger on a certain LED is going to associate
> itself with, say, a certain port' is clearly a property of the
> hardware, when offloading is supported. I've not seen a switch you can
> arbitrarily assign LEDs to ports. The Marvell switches have the LED
> registers within the port registers, for example, two LEDs per port.
Aha so there is an implicit HW dependency between the port and the
LED, that we just cannot see in the device tree. Okay, it makes sense.
I think there will be a day when a switch without LED controller appears,
but the system has a few LEDs for the ports connected to an
arbitrary GPIO controller, and then we will need this. But we have
not seen that yet :)
Yours,
Linus Walleij
On Fri, Mar 10, 2023 at 10:37:25AM +0100, Linus Walleij wrote:
> On Thu, Mar 9, 2023 at 4:22 PM Andrew Lunn <[email protected]> wrote:
>
> > As to 'how a certain trigger on a certain LED is going to associate
> > itself with, say, a certain port' is clearly a property of the
> > hardware, when offloading is supported. I've not seen a switch you can
> > arbitrarily assign LEDs to ports. The Marvell switches have the LED
> > registers within the port registers, for example, two LEDs per port.
>
> Aha so there is an implicit HW dependency between the port and the
> LED, that we just cannot see in the device tree. Okay, it makes sense.
Well, i would say the dependency is in the device tree, in that the
LEDs are described in the ports, not as a block of their own at a
higher level within the switch. And in some switches, they might
actually be a block of registers in there own space, rather than in
the port registers. But i still expect there is a fixed mapping
between LED and port.
> I think there will be a day when a switch without LED controller appears,
> but the system has a few LEDs for the ports connected to an
> arbitrary GPIO controller, and then we will need this. But we have
> not seen that yet :)
The microchip sparx5 might be going in that direction. It has what
looks like a reasonably generic sgpio controller:
drivers/pinctrl/pinctrl-microchip-sgpio.c
But this not just about switches. It is also plain NICs. And using
ledtrig-netdev, you could make your keyboard LED blink based on
network traffic etc. So yes, using a phandle to an LED could very well
be useful in the future.
Andrew
> > I think there will be a day when a switch without LED controller appears,
> > but the system has a few LEDs for the ports connected to an
> > arbitrary GPIO controller, and then we will need this. But we have
> > not seen that yet :)
>
> The microchip sparx5 might be going in that direction. It has what
> looks like a reasonably generic sgpio controller:
> drivers/pinctrl/pinctrl-microchip-sgpio.c
That gpio controller supports both, some kind of hardware controlled
and pure software controlled mode. AFAIK the driver only supports
software controlled mode (yet?). In any case, our board
(arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt.dtsi) is broken in
a way that we are forced to use the software controlled mode anyway.
Therefore, there is already such a board ;)
-michael