This is another attempt on adding this feature on LEDs, hoping this is
the right time and someone finally notice this.
Most of the times Switch/PHY have connected multiple LEDs that are
controlled by HW based on some rules/event. Currently we lack any
support for a generic way to control the HW part and normally we
either never implement the feature or only add control for brightness
or hw blink.
This is based on Marek idea of providing some API to cled but use a
different implementation that in theory should be more generilized.
The current idea is:
- LED driver implement 3 API (hw_control_status/start/stop).
They are used to put the LED in hardware mode and to configure the
various trigger.
- We have hardware triggers that are used to expose to userspace the
supported hardware mode and set the hardware mode on trigger
activation.
- We can also have triggers that both support hardware and software mode.
- The LED driver will declare each supported hardware blink mode and
communicate with the trigger all the supported blink modes that will
be available by sysfs.
- A trigger will use blink_set to configure the blink mode to active
in hardware mode.
- On hardware trigger activation, only the hardware mode is enabled but
the blink modes are not configured. The LED driver should reset any
link mode active by default.
Each LED driver will have to declare explicit support for the offload
trigger (or return not supported error code) as we the trigger_data that
the LED driver will elaborate and understand what is referring to (based
on the current active trigger).
I posted a user for this new implementation that will benefit from this
and will add a big feature to it. Currently qca8k can have up to 3 LEDs
connected to each PHY port and we have some device that have only one of
them connected and the default configuration won't work for that.
The netdev trigger is expanded and it does now support hardware only
triggers.
The idea is to use hardware mode when a device_name is not defined.
An additional sysfs entry is added to give some info about the available
trigger modes supported in the current configuration.
It was reported that at least 3 other switch family would benefits by
this as they all lack support for a generic way to setup their leds and
netdev team NACK each try to add special code to support LEDs present
on switch in favor of a generic solution.
v7:
- Rebase on top of net-next (for qca8k changes)
- Fix some typo in commit description
- Fix qca8k leds documentation warning
- Remove RFC tag
v6:
- Back to RFC.
- Drop additional trigger
- Rework netdev trigger to support common modes used by switch and
hardware only triggers
- Refresh qca8k leds logic and driver
v5:
- Move out of RFC. (no comments from Andrew this is the right path?)
- Fix more spelling mistake (thx Randy)
- Fix error reported by kernel test bot
- Drop the additional HW_CONTROL flag. It does simplify CONFIG
handling and hw control should be available anyway to support
triggers as module.
v4:
- Rework implementation and drop hw_configure logic.
We now expand blink_set.
- Address even more spelling mistake. (thx a lot Randy)
- Drop blink option and use blink_set delay.
- Rework phy-activity trigger to actually make the groups dynamic.
v3:
- Rework start/stop as Andrew asked.
- Introduce more logic to permit a trigger to run in hardware mode.
- Add additional patch with netdev hardware support.
- Use test_bit API to check flag passed to hw_control_configure.
- Added a new cmd to hw_control_configure to reset any active blink_mode.
- Refactor all the patches to follow this new implementation.
v2:
- Fix spelling mistake (sorry)
- Drop patch 02 "permit to declare supported offload triggers".
Change the logic, now the LED driver declare support for them
using the configure_offload with the cmd TRIGGER_SUPPORTED.
- Rework code to follow this new implementation.
- Update Documentation to better describe how this offload
implementation work.
Christian Marangi (11):
leds: add support for hardware driven LEDs
leds: add function to configure hardware controlled LED
leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode
leds: trigger: netdev: rename and expose NETDEV trigger enum modes
leds: trigger: netdev: convert device attr to macro
leds: trigger: netdev: add hardware control support
leds: trigger: netdev: use mutex instead of spinlocks
leds: trigger: netdev: add available mode sysfs attr
leds: trigger: netdev: add additional hardware only triggers
net: dsa: qca8k: add LEDs support
dt-bindings: net: dsa: qca8k: add LEDs definition example
.../devicetree/bindings/net/dsa/qca8k.yaml | 24 ++
Documentation/leds/leds-class.rst | 53 +++
drivers/leds/Kconfig | 11 +
drivers/leds/led-class.c | 27 ++
drivers/leds/led-triggers.c | 29 ++
drivers/leds/trigger/ledtrig-netdev.c | 385 ++++++++++++-----
drivers/net/dsa/qca/Kconfig | 9 +
drivers/net/dsa/qca/Makefile | 1 +
drivers/net/dsa/qca/qca8k-8xxx.c | 4 +
drivers/net/dsa/qca/qca8k-leds.c | 406 ++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 62 +++
include/linux/leds.h | 103 ++++-
12 files changed, 1015 insertions(+), 99 deletions(-)
create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
--
2.37.2
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.37.2
Add LEDs support for qca8k Switch Family. This will provide the LEDs
hardware API to permit the PHY LED to support hardware mode.
Each port have at least 3 LEDs and they can HW blink, set on/off or
follow blink modes configured with the LED in hardware mode.
Adds support for leds netdev trigger to support hardware triggers.
Signed-off-by: Christian Marangi <[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 | 406 +++++++++++++++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 62 +++++
5 files changed, 482 insertions(+)
create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
index ba339747362c..be67164a444e 100644
--- a/drivers/net/dsa/qca/Kconfig
+++ b/drivers/net/dsa/qca/Kconfig
@@ -15,3 +15,12 @@ config NET_DSA_QCA8K
help
This enables support for the Qualcomm Atheros QCA8K Ethernet
switch chips.
+
+config NET_DSA_QCA8K_LEDS_SUPPORT
+ tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
+ depends on NET_DSA_QCA8K
+ select LEDS_OFFLOAD_TRIGGERS
+ help
+ This enabled support for LEDs present on the Qualcomm Atheros
+ QCA8K Ethernet switch chips. This require the LEDs offload
+ triggers support as it can run in offload mode.
diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
index 701f1d199e93..330ae389e489 100644
--- a/drivers/net/dsa/qca/Makefile
+++ b/drivers/net/dsa/qca/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
qca8k-y += qca8k-common.o qca8k-8xxx.o
+obj-$(CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT) += qca8k-leds.o
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index c5c3b4e92f28..90916d8d8afe 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1690,6 +1690,10 @@ qca8k_setup(struct dsa_switch *ds)
if (ret)
return ret;
+ ret = qca8k_setup_led_ctrl(priv);
+ if (ret)
+ return ret;
+
qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
new file mode 100644
index 000000000000..b51cdcae31b2
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -0,0 +1,406 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <net/dsa.h>
+#include <linux/regmap.h>
+
+#include "qca8k.h"
+
+static int
+qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+ int shift;
+
+ switch (port_num) {
+ case 0:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = 14;
+ break;
+ case 1:
+ case 2:
+ case 3:
+ reg_info->reg = QCA8K_LED_CTRL_REG(3);
+ shift = 2 * led_num + (6 * (port_num - 1));
+
+ reg_info->shift = 8 + shift;
+
+ break;
+ case 4:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = 30;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+qca8k_get_control_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+
+ /* 6 total control rule:
+ * 3 control rules for phy0-3 that applies to all their leds
+ * 3 control rules for phy4
+ */
+ if (port_num == 4)
+ reg_info->shift = 16;
+ else
+ reg_info->shift = 0;
+
+ return 0;
+}
+
+static int
+qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger, u32 *mask)
+{
+ /* Parsing specific to netdev trigger */
+ if (test_bit(TRIGGER_NETDEV_LINK, &rules))
+ *offload_trigger = QCA8K_LED_LINK_10M_EN_MASK |
+ QCA8K_LED_LINK_100M_EN_MASK |
+ QCA8K_LED_LINK_1000M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+ *offload_trigger = QCA8K_LED_LINK_10M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+ *offload_trigger = QCA8K_LED_LINK_100M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+ *offload_trigger = QCA8K_LED_LINK_1000M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+ *offload_trigger = QCA8K_LED_HALF_DUPLEX_MASK;
+ if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+ *offload_trigger = QCA8K_LED_FULL_DUPLEX_MASK;
+ if (test_bit(TRIGGER_NETDEV_TX, &rules))
+ *offload_trigger = QCA8K_LED_TX_BLINK_MASK;
+ if (test_bit(TRIGGER_NETDEV_RX, &rules))
+ *offload_trigger = QCA8K_LED_RX_BLINK_MASK;
+ if (test_bit(TRIGGER_NETDEV_BLINK_2HZ, &rules))
+ *offload_trigger = QCA8K_LED_BLINK_2HZ;
+ if (test_bit(TRIGGER_NETDEV_BLINK_4HZ, &rules))
+ *offload_trigger = QCA8K_LED_BLINK_4HZ;
+ if (test_bit(TRIGGER_NETDEV_BLINK_8HZ, &rules))
+ *offload_trigger = QCA8K_LED_BLINK_8HZ;
+
+ if (!*offload_trigger)
+ return -EOPNOTSUPP;
+
+ if (test_bit(TRIGGER_NETDEV_BLINK_2HZ, &rules) ||
+ test_bit(TRIGGER_NETDEV_BLINK_4HZ, &rules) ||
+ test_bit(TRIGGER_NETDEV_BLINK_8HZ, &rules)) {
+ *mask = QCA8K_LED_BLINK_FREQ_MASK;
+ } else {
+ *mask = *offload_trigger;
+ }
+
+ return 0;
+}
+
+static int
+qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
+ enum blink_mode_cmd cmd)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+ struct led_trigger *trigger = ldev->trigger;
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 offload_trigger = 0, mask, val;
+ int ret;
+
+ /* Check trigger compatibility */
+ if (strcmp(trigger->name, "netdev"))
+ return -EOPNOTSUPP;
+
+ if (!strcmp(trigger->name, "netdev"))
+ ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
+
+ if (ret)
+ return ret;
+
+ qca8k_get_control_led_reg(led->port_num, led->led_num, ®_info);
+
+ switch (cmd) {
+ case BLINK_MODE_SUPPORTED:
+ /* We reach this point, we are sure the trigger is supported */
+ return 1;
+ case BLINK_MODE_ZERO:
+ /* We set 4hz by default */
+ u32 default_reg = QCA8K_LED_BLINK_4HZ;
+
+ ret = regmap_update_bits(priv->regmap, reg_info.reg,
+ QCA8K_LED_RULE_MASK << reg_info.shift,
+ default_reg << reg_info.shift);
+ break;
+ case BLINK_MODE_ENABLE:
+ ret = regmap_update_bits(priv->regmap, reg_info.reg,
+ mask << reg_info.shift,
+ offload_trigger << reg_info.shift);
+ break;
+ case BLINK_MODE_DISABLE:
+ ret = regmap_update_bits(priv->regmap, reg_info.reg,
+ mask << reg_info.shift,
+ 0);
+ break;
+ case BLINK_MODE_READ:
+ ret = regmap_read(priv->regmap, reg_info.reg, &val);
+ if (ret)
+ return ret;
+
+ val >>= reg_info.shift;
+ val &= offload_trigger;
+
+ /* Special handling for LED_BLINK_2HZ */
+ if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
+ val = 1;
+
+ return val;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return ret;
+}
+
+static void
+qca8k_led_brightness_set(struct qca8k_led *led,
+ enum led_brightness b)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val = QCA8K_LED_ALWAYS_OFF;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ if (b)
+ val = QCA8K_LED_ALWAYS_ON;
+
+ regmap_update_bits(priv->regmap, reg_info.reg,
+ GENMASK(1, 0) << reg_info.shift,
+ val << reg_info.shift);
+}
+
+static void
+qca8k_cled_brightness_set(struct led_classdev *ldev,
+ enum led_brightness b)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ return qca8k_led_brightness_set(led, b);
+}
+
+static enum led_brightness
+qca8k_led_brightness_get(struct qca8k_led *led)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val;
+ int ret;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ ret = regmap_read(priv->regmap, reg_info.reg, &val);
+ if (ret)
+ return 0;
+
+ val >>= reg_info.shift;
+ val &= GENMASK(1, 0);
+
+ return val > 0 ? 1 : 0;
+}
+
+static enum led_brightness
+qca8k_cled_brightness_get(struct led_classdev *ldev)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ return qca8k_led_brightness_get(led);
+}
+
+static int
+qca8k_cled_blink_set(struct led_classdev *ldev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+
+ if (*delay_on == 0 && *delay_off == 0) {
+ *delay_on = 125;
+ *delay_off = 125;
+ }
+
+ if (*delay_on != 125 || *delay_off != 125) {
+ /* The hardware only supports blinking at 4Hz. Fall back
+ * to software implementation in other cases.
+ */
+ return -EINVAL;
+ }
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ regmap_update_bits(priv->regmap, reg_info.reg,
+ GENMASK(1, 0) << reg_info.shift,
+ QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
+
+ return 0;
+}
+
+static int
+qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val = QCA8K_LED_ALWAYS_OFF;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ if (enable)
+ val = QCA8K_LED_RULE_CONTROLLED;
+
+ return regmap_update_bits(priv->regmap, reg_info.reg,
+ GENMASK(1, 0) << reg_info.shift,
+ val << reg_info.shift);
+}
+
+static int
+qca8k_cled_hw_control_start(struct led_classdev *led_cdev)
+{
+ return qca8k_cled_trigger_offload(led_cdev, true);
+}
+
+static int
+qca8k_cled_hw_control_stop(struct led_classdev *led_cdev)
+{
+ return qca8k_cled_trigger_offload(led_cdev, false);
+}
+
+static bool
+qca8k_cled_hw_control_status(struct led_classdev *ldev)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ regmap_read(priv->regmap, reg_info.reg, &val);
+
+ val >>= reg_info.shift;
+ val &= GENMASK(1, 0);
+
+ return val == QCA8K_LED_RULE_CONTROLLED;
+}
+
+static int
+qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
+{
+ struct fwnode_handle *led = NULL, *leds = NULL;
+ struct led_init_data init_data = { };
+ struct qca8k_led *port_led;
+ int led_num, port_index;
+ const char *state;
+ int ret;
+
+ leds = fwnode_get_named_child_node(port, "leds");
+ if (!leds) {
+ dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
+ port_num);
+ return 0;
+ }
+
+ fwnode_for_each_child_node(leds, led) {
+ /* Reg represent the led number of the port.
+ * Each port can have at least 3 leds attached
+ * Commonly:
+ * 1. is gigabit led
+ * 2. is mbit led
+ * 3. additional status led
+ */
+ if (fwnode_property_read_u32(led, "reg", &led_num))
+ continue;
+
+ if (led_num >= QCA8K_LED_PORT_COUNT) {
+ dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
+ continue;
+ }
+
+ port_index = 3 * port_num + led_num;
+
+ port_led = &priv->ports_led[port_index];
+ port_led->port_num = port_num;
+ port_led->led_num = led_num;
+ port_led->priv = priv;
+
+ ret = fwnode_property_read_string(led, "default-state", &state);
+ if (!ret) {
+ if (!strcmp(state, "on")) {
+ port_led->cdev.brightness = 1;
+ qca8k_led_brightness_set(port_led, 1);
+ } else if (!strcmp(state, "off")) {
+ port_led->cdev.brightness = 0;
+ qca8k_led_brightness_set(port_led, 0);
+ } else if (!strcmp(state, "keep")) {
+ port_led->cdev.brightness =
+ qca8k_led_brightness_get(port_led);
+ }
+ }
+
+ /* 3 brightness settings can be applied from Documentation:
+ * 0 always off
+ * 1 blink at 4Hz
+ * 2 always on
+ * 3 rule controlled
+ * Suppots only 2 mode: (pcb limitation, with always on and blink
+ * only the last led is set to this mode)
+ * 0 always off (sets all leds off)
+ * 3 rule controlled
+ */
+ port_led->cdev.blink_mode = SOFTWARE_HARDWARE_CONTROLLED;
+ port_led->cdev.max_brightness = 1;
+ port_led->cdev.brightness_set = qca8k_cled_brightness_set;
+ port_led->cdev.brightness_get = qca8k_cled_brightness_get;
+ port_led->cdev.blink_set = qca8k_cled_blink_set;
+ port_led->cdev.hw_control_start = qca8k_cled_hw_control_start;
+ port_led->cdev.hw_control_stop = qca8k_cled_hw_control_stop;
+ port_led->cdev.hw_control_status = qca8k_cled_hw_control_status;
+ port_led->cdev.hw_control_configure = qca8k_cled_hw_control_configure;
+ init_data.default_label = ":port";
+ init_data.devicename = "qca8k";
+ init_data.fwnode = led;
+
+ ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
+ if (ret)
+ dev_warn(priv->dev, "Failed to int led");
+ }
+
+ return 0;
+}
+
+int
+qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ struct fwnode_handle *mdio, *port;
+ int port_num;
+ int ret;
+
+ mdio = device_get_named_child_node(priv->dev, "mdio");
+ if (!mdio) {
+ dev_info(priv->dev, "No MDIO node specified in device tree!\n");
+ return 0;
+ }
+
+ fwnode_for_each_child_node(mdio, port) {
+ if (fwnode_property_read_u32(port, "reg", &port_num))
+ continue;
+
+ /* Each port can have at least 3 different leds attached */
+ ret = qca8k_parse_port_leds(priv, port, port_num);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 0b7a5cb12321..5c535baa7139 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/regmap.h>
#include <linux/gpio.h>
+#include <linux/leds.h>
#include <linux/dsa/tag_qca.h>
#define QCA8K_ETHERNET_MDIO_PRIORITY 7
@@ -85,6 +86,43 @@
#define QCA8K_MDIO_MASTER_DATA(x) FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
#define QCA8K_MDIO_MASTER_MAX_PORTS 5
#define QCA8K_MDIO_MASTER_MAX_REG 32
+
+/* LED control register */
+#define QCA8K_LED_COUNT 15
+#define QCA8K_LED_PORT_COUNT 3
+#define QCA8K_LED_RULE_COUNT 6
+#define QCA8K_LED_RULE_MAX 11
+#define QCA8K_LED_CTRL_REG(_i) (0x050 + (_i) * 4)
+#define QCA8K_LED_CTRL0_REG 0x50
+#define QCA8K_LED_CTRL1_REG 0x54
+#define QCA8K_LED_CTRL2_REG 0x58
+#define QCA8K_LED_CTRL3_REG 0x5C
+#define QCA8K_LED_CTRL_SHIFT(_i) (((_i) % 2) * 16)
+#define QCA8K_LED_CTRL_MASK GENMASK(15, 0)
+#define QCA8K_LED_RULE_MASK GENMASK(13, 0)
+#define QCA8K_LED_BLINK_FREQ_MASK GENMASK(1, 0)
+#define QCA8K_LED_BLINK_FREQ_SHITF 0
+#define QCA8K_LED_BLINK_2HZ 0
+#define QCA8K_LED_BLINK_4HZ 1
+#define QCA8K_LED_BLINK_8HZ 2
+#define QCA8K_LED_BLINK_AUTO 3
+#define QCA8K_LED_LINKUP_OVER_MASK BIT(2)
+#define QCA8K_LED_TX_BLINK_MASK BIT(4)
+#define QCA8K_LED_RX_BLINK_MASK BIT(5)
+#define QCA8K_LED_COL_BLINK_MASK BIT(7)
+#define QCA8K_LED_LINK_10M_EN_MASK BIT(8)
+#define QCA8K_LED_LINK_100M_EN_MASK BIT(9)
+#define QCA8K_LED_LINK_1000M_EN_MASK BIT(10)
+#define QCA8K_LED_POWER_ON_LIGHT_MASK BIT(11)
+#define QCA8K_LED_HALF_DUPLEX_MASK BIT(12)
+#define QCA8K_LED_FULL_DUPLEX_MASK BIT(13)
+#define QCA8K_LED_PATTERN_EN_MASK GENMASK(15, 14)
+#define QCA8K_LED_PATTERN_EN_SHIFT 14
+#define QCA8K_LED_ALWAYS_OFF 0
+#define QCA8K_LED_ALWAYS_BLINK_4HZ 1
+#define QCA8K_LED_ALWAYS_ON 2
+#define QCA8K_LED_RULE_CONTROLLED 3
+
#define QCA8K_GOL_MAC_ADDR0 0x60
#define QCA8K_GOL_MAC_ADDR1 0x64
#define QCA8K_MAX_FRAME_SIZE 0x78
@@ -388,6 +426,19 @@ struct qca8k_pcs {
int port;
};
+struct qca8k_led_pattern_en {
+ u32 reg;
+ u8 shift;
+};
+
+struct qca8k_led {
+ u8 port_num;
+ u8 led_num;
+ u16 old_rule;
+ struct qca8k_priv *priv;
+ struct led_classdev cdev;
+};
+
struct qca8k_priv {
u8 switch_id;
u8 switch_revision;
@@ -412,6 +463,7 @@ struct qca8k_priv {
struct qca8k_pcs pcs_port_0;
struct qca8k_pcs pcs_port_6;
const struct qca8k_match_data *info;
+ struct qca8k_led ports_led[QCA8K_LED_COUNT];
};
struct qca8k_mib_desc {
@@ -517,4 +569,14 @@ int qca8k_port_lag_join(struct dsa_switch *ds, int port, struct dsa_lag lag,
int qca8k_port_lag_leave(struct dsa_switch *ds, int port,
struct dsa_lag lag);
+/* Leds Support function */
+#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
+#else
+static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ return 0;
+}
+#endif
+
#endif /* __QCA8K_H */
--
2.37.2
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.37.2
Some LEDs can be driven by hardware (for example a LED connected to
an ethernet PHY or an ethernet switch can be configured to blink on
activity on the network, which in software is done by the netdev trigger).
To do such offloading, LED driver must support this and a supported
trigger must be used.
LED driver should declare the correct blink_mode supported and should set
the blink_mode parameter to one of HARDWARE_CONTROLLED or
SOFTWARE_HARDWARE_CONTROLLED.
The trigger will check this option and fail to activate if the blink_mode
is not supported. By default if a LED driver doesn't declare blink_mode,
SOFTWARE_CONTROLLED is assumed.
The LED must implement 3 main API:
- hw_control_status(): This asks the LED driver if hardware mode is
enabled or not.
Triggers will check if the offload mode is supported and will be
activated accordingly. If the trigger can't run in software mode,
return -EOPNOTSUPP as the blinking can't be simulated by software.
- hw_control_start(): This will simply enable the hardware mode for
the LED.
With this not declared and hw_control_status() returning true,
it's assumed that the LED is always in hardware mode.
- hw_control_stop(): This will simply disable the hardware mode for
the LED.
With this not declared and hw_control_status() returning true,
it's assumed that the LED is always in hardware mode.
It's advised to the driver to put the LED in the old state but this
is not enforcerd and putting the LED off is also accepted.
With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is
optional and any software only trigger will reject activation as the LED
supports only hardware mode.
An additional config CONFIG_LEDS_HARDWARE_CONTROL is added to add support
for LEDs that can be controlled by hardware.
Cc: Marek Behún <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
Documentation/leds/leds-class.rst | 28 ++++++++++++++++++
drivers/leds/Kconfig | 11 ++++++++
drivers/leds/led-class.c | 27 ++++++++++++++++++
drivers/leds/led-triggers.c | 29 +++++++++++++++++++
include/linux/leds.h | 47 ++++++++++++++++++++++++++++++-
5 files changed, 141 insertions(+), 1 deletion(-)
diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index cd155ead8703..645940b78d81 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -169,6 +169,34 @@ Setting the brightness to zero with brightness_set() callback function
should completely turn off the LED and cancel the previously programmed
hardware blinking function, if any.
+Hardware driven LEDs
+===================================
+
+Some LEDs can be driven by hardware (for example a LED connected to
+an ethernet PHY or an ethernet switch can be configured to blink on activity on
+the network, which in software is done by the netdev trigger).
+
+To do such offloading, LED driver must support this and a supported trigger must
+be used.
+
+LED driver should declare the correct blink_mode supported and should set the
+blink_mode parameter to one of HARDWARE_CONTROLLED or SOFTWARE_HARDWARE_CONTROLLED.
+The trigger will check this option and fail to activate if the blink_mode is not
+supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTROLLED
+is assumed.
+
+The LED must implement 3 main API:
+- hw_control_status(): This asks the LED driver if hardware mode is enabled
+ or not. Triggers will check if the hardware mode is active and will try
+ to offload their triggers if supported by the driver.
+- hw_control_start(): This will simply enable the hardware mode for the LED.
+- hw_control_stop(): This will simply disable the hardware mode for the LED.
+ It's advised to the driver to put the LED in the old state but this is not
+ enforcerd and putting the LED off is also accepted.
+
+With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
+and any software only trigger will reject activation as the LED supports only
+hardware mode.
Known Issues
============
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 499d0f215a8b..891f4821b2c8 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -49,6 +49,17 @@ config LEDS_BRIGHTNESS_HW_CHANGED
See Documentation/ABI/testing/sysfs-class-led for details.
+config LEDS_HARDWARE_CONTROL
+ bool "LED Hardware Control support"
+ help
+ This option enabled Hardware control support used by leds that
+ can be driven in hardware by using supported triggers.
+
+ Hardware blink modes will be exposed by sysfs class in
+ /sys/class/leds based on the trigger currently active.
+
+ If unsure, say Y.
+
comment "LED drivers"
config LEDS_88PM860X
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 6a8ea94834fa..3516ae3c4c3c 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -164,6 +164,27 @@ static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
}
#endif
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+static int led_classdev_check_blink_mode_functions(struct led_classdev *led_cdev)
+{
+ int mode = led_cdev->blink_mode;
+
+ if (mode == SOFTWARE_HARDWARE_CONTROLLED &&
+ (!led_cdev->hw_control_status ||
+ !led_cdev->hw_control_start ||
+ !led_cdev->hw_control_stop))
+ return -EINVAL;
+
+ if (mode == SOFTWARE_CONTROLLED &&
+ (led_cdev->hw_control_status ||
+ led_cdev->hw_control_start ||
+ led_cdev->hw_control_stop))
+ return -EINVAL;
+
+ return 0;
+}
+#endif
+
/**
* led_classdev_suspend - suspend an led_classdev.
* @led_cdev: the led_classdev to suspend.
@@ -367,6 +388,12 @@ int led_classdev_register_ext(struct device *parent,
if (ret < 0)
return ret;
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+ ret = led_classdev_check_blink_mode_functions(led_cdev);
+ if (ret < 0)
+ return ret;
+#endif
+
mutex_init(&led_cdev->led_access);
mutex_lock(&led_cdev->led_access);
led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 072491d3e17b..693c5d0fa980 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -154,6 +154,27 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
}
EXPORT_SYMBOL_GPL(led_trigger_read);
+static bool led_trigger_is_supported(struct led_classdev *led_cdev,
+ struct led_trigger *trigger)
+{
+ switch (led_cdev->blink_mode) {
+ case SOFTWARE_CONTROLLED:
+ if (trigger->supported_blink_modes == HARDWARE_ONLY)
+ return 0;
+ break;
+ case HARDWARE_CONTROLLED:
+ if (trigger->supported_blink_modes == SOFTWARE_ONLY)
+ return 0;
+ break;
+ case SOFTWARE_HARDWARE_CONTROLLED:
+ break;
+ default:
+ return 0;
+ }
+
+ return 1;
+}
+
/* Caller must ensure led_cdev->trigger_lock held */
int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
{
@@ -179,6 +200,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
cancel_work_sync(&led_cdev->set_brightness_work);
led_stop_software_blink(led_cdev);
+ /* Disable hardware mode on trigger change if supported */
+ if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
+ led_cdev->hw_control_status(led_cdev))
+ led_cdev->hw_control_stop(led_cdev);
if (led_cdev->trigger->deactivate)
led_cdev->trigger->deactivate(led_cdev);
device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
@@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
led_set_brightness(led_cdev, LED_OFF);
}
if (trig) {
+ /* Make sure the trigger support the LED blink mode */
+ if (!led_trigger_is_supported(led_cdev, trig))
+ return -EINVAL;
+
spin_lock(&trig->leddev_list_lock);
list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
spin_unlock(&trig->leddev_list_lock);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..09ff1dc6f48d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -67,6 +67,12 @@ struct led_hw_trigger_type {
int dummy;
};
+enum led_blink_modes {
+ SOFTWARE_CONTROLLED = 0x0,
+ HARDWARE_CONTROLLED,
+ SOFTWARE_HARDWARE_CONTROLLED,
+};
+
struct led_classdev {
const char *name;
unsigned int brightness;
@@ -154,6 +160,32 @@ struct led_classdev {
/* LEDs that have private triggers have this set */
struct led_hw_trigger_type *trigger_type;
+
+ /* This report the supported blink_mode. The driver should report the
+ * correct LED capabilities.
+ * With this set to HARDWARE_CONTROLLED, LED is always in offload mode
+ * and triggers can't be simulated by software.
+ * If the led is HARDWARE_CONTROLLED, status/start/stop function
+ * are optional.
+ * By default SOFTWARE_CONTROLLED is set as blink_mode.
+ */
+ enum led_blink_modes blink_mode;
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+ /* Ask the LED driver if hardware mode is enabled or not.
+ * If the option is not declared by the LED driver, SOFTWARE_CONTROLLED
+ * is assumed.
+ * Triggers will check if the hardware mode is supported and will be
+ * activated accordingly. If the trigger can't run in hardware mode,
+ * return -EOPNOTSUPP as the blinking can't be simulated by software.
+ */
+ bool (*hw_control_status)(struct led_classdev *led_cdev);
+ /* Set LED in hardware mode */
+ int (*hw_control_start)(struct led_classdev *led_cdev);
+ /* Disable hardware mode for LED. It's advised to the LED driver to put it to
+ * the old status but that is not mandatory and also putting it off is accepted.
+ */
+ int (*hw_control_stop)(struct led_classdev *led_cdev);
+#endif
#endif
#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -215,7 +247,6 @@ extern struct led_classdev *of_led_get(struct device_node *np, int index);
extern void led_put(struct led_classdev *led_cdev);
struct led_classdev *__must_check devm_of_led_get(struct device *dev,
int index);
-
/**
* led_blink_set - set blinking with software fallback
* @led_cdev: the LED to start blinking
@@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
#define TRIG_NAME_MAX 50
+enum led_trigger_blink_supported_modes {
+ SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
+ HARDWARE_ONLY = HARDWARE_CONTROLLED,
+ SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
+};
+
struct led_trigger {
/* Trigger Properties */
const char *name;
int (*activate)(struct led_classdev *led_cdev);
void (*deactivate)(struct led_classdev *led_cdev);
+ /* Declare if the Trigger supports hardware control to
+ * offload triggers or supports only software control.
+ * A trigger can also declare support for hardware control
+ * if its task is to only configure LED blink modes and expose
+ * them in sysfs.
+ */
+ enum led_trigger_blink_supported_modes supported_blink_modes;
+
/* LED-private triggers have this set */
struct led_hw_trigger_type *trigger_type;
--
2.37.2
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 | 25 ++++++++++++++++++++
include/linux/leds.h | 39 +++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+)
diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index 645940b78d81..efd2f68c46a7 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -198,6 +198,31 @@ With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
and any software only trigger will reject activation as the LED supports only
hardware mode.
+A trigger once he declared support for hardware controlled blinks, will use the function
+hw_control_configure() provided by the driver to check support for a particular blink mode.
+This function passes as the first argument (flag) a u32 flag.
+The second argument (cmd) of hw_control_configure() method can be used to do various
+operations for the specific blink mode. We currently support ENABLE, DISABLE, READ, ZERO
+and SUPPORTED to enable, disable, read the state of the blink mode, ask the LED
+driver if it does supports the specific blink mode and to reset any blink mode active.
+
+In ENABLE/DISABLE hw_control_configure() should configure the LED to enable/disable the
+requested blink mode (flag).
+In READ hw_control_configure() should return 0 or 1 based on the status of the requested
+blink mode (flag).
+In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
+requested blink mode (flags) or not.
+In ZERO hw_control_configure() should return 0 with success operation or error.
+
+The unsigned long flag is specific to the trigger and change across them. It's in the LED
+driver interest know how to elaborate this flag and to declare support for a
+particular trigger. For this exact reason explicit support for the specific
+trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
+with a not supported trigger.
+If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
+fail as the driver doesn't support that specific offload trigger or doesn't know
+how to handle the provided flags.
+
Known Issues
============
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 09ff1dc6f48d..b5aad67fecfb 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -73,6 +73,16 @@ enum led_blink_modes {
SOFTWARE_HARDWARE_CONTROLLED,
};
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+enum blink_mode_cmd {
+ BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
+ BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
+ BLINK_MODE_READ, /* Read the status of the hardware blink mode */
+ BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
+ BLINK_MODE_ZERO, /* Disable any hardware blink active */
+};
+#endif
+
struct led_classdev {
const char *name;
unsigned int brightness;
@@ -185,6 +195,17 @@ struct led_classdev {
* the old status but that is not mandatory and also putting it off is accepted.
*/
int (*hw_control_stop)(struct led_classdev *led_cdev);
+ /* This will be used to configure the various blink modes LED support in hardware
+ * mode.
+ * The LED driver require to support the active trigger and will elaborate the
+ * unsigned long flag and do the operation based on the provided cmd.
+ * Current operation are enable,disable,supported and status.
+ * A trigger will use this to enable or disable the asked blink mode, check the
+ * status of the blink mode or ask if the blink mode can run in hardware mode.
+ */
+ int (*hw_control_configure)(struct led_classdev *led_cdev,
+ unsigned long flag,
+ enum blink_mode_cmd cmd);
#endif
#endif
@@ -454,6 +475,24 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
return led_cdev->trigger_data;
}
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev,
+ unsigned long flag)
+{
+ int ret;
+
+ /* Sanity check: make sure led support hw mode */
+ if (led_cdev->blink_mode == SOFTWARE_CONTROLLED)
+ return false;
+
+ ret = led_cdev->hw_control_configure(led_cdev, flag, BLINK_MODE_SUPPORTED);
+ if (ret > 0)
+ return true;
+
+ return false;
+}
+#endif
+
/**
* led_trigger_rename_static - rename a trigger
* @name: the new trigger name
--
2.37.2
Rename NETDEV trigger enum modes to a more simbolic name and move them
in leds.h to make them accessible by any user.
Signed-off-by: Christian Marangi <[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 b5aad67fecfb..13862f8b1e07 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -548,6 +548,13 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
#endif /* CONFIG_LEDS_TRIGGERS */
+/* Trigger specific enum */
+enum led_trigger_netdev_modes {
+ TRIGGER_NETDEV_LINK = 1,
+ TRIGGER_NETDEV_TX,
+ TRIGGER_NETDEV_RX,
+};
+
/* Trigger specific functions */
#ifdef CONFIG_LEDS_TRIGGER_DISK
void ledtrig_disk_activity(bool write);
--
2.37.2
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link: https://lore.kernel.org/r/20221214235438.30271-11-ansuelsmth%40gmail.com
patch subject: [PATCH v7 10/11] net: dsa: qca8k: add LEDs support
config: sh-allmodconfig
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c3cb4f8cc2edb081c2b31733ed02cb2c022320f1
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
git checkout c3cb4f8cc2edb081c2b31733ed02cb2c022320f1
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/net/dsa/qca/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/net/dsa/qca/qca8k-leds.c:383:1: error: redefinition of 'qca8k_setup_led_ctrl'
383 | qca8k_setup_led_ctrl(struct qca8k_priv *priv)
| ^~~~~~~~~~~~~~~~~~~~
In file included from drivers/net/dsa/qca/qca8k-leds.c:5:
drivers/net/dsa/qca/qca8k.h:576:19: note: previous definition of 'qca8k_setup_led_ctrl' with type 'int(struct qca8k_priv *)'
576 | static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
| ^~~~~~~~~~~~~~~~~~~~
vim +/qca8k_setup_led_ctrl +383 drivers/net/dsa/qca/qca8k-leds.c
381
382 int
> 383 qca8k_setup_led_ctrl(struct qca8k_priv *priv)
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Christian,
On Thu, 2022-12-15 at 00:54 +0100, Christian Marangi wrote:
> Add LEDs support for qca8k Switch Family. This will provide the LEDs
> hardware API to permit the PHY LED to support hardware mode.
> Each port have at least 3 LEDs and they can HW blink, set on/off or
> follow blink modes configured with the LED in hardware mode.
> Adds support for leds netdev trigger to support hardware triggers.
>
> Signed-off-by: Christian Marangi <[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 | 406
> +++++++++++++++++++++++++++++++
> drivers/net/dsa/qca/qca8k.h | 62 +++++
> 5 files changed, 482 insertions(+)
> create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
>
>
> diff --git a/drivers/net/dsa/qca/qca8k-leds.c
> b/drivers/net/dsa/qca/qca8k-leds.c
> new file mode 100644
> index 000000000000..b51cdcae31b2
> --- /dev/null
> +++ b/drivers/net/dsa/qca/qca8k-leds.c
> @@ -0,0 +1,406 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <net/dsa.h>
> +#include <linux/regmap.h>
Alphabatical order
> +
> +#include "qca8k.h"
> +
> +static int
> +qca8k_get_enable_led_reg(int port_num, int led_num, struct
> qca8k_led_pattern_en *reg_info)
> +{
> + int shift;
> +
> + switch (port_num) {
> + case 0:
> + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> + reg_info->shift = 14;
Any macros other than magic number for 14, 30
> + break;
> + case 1:
> + case 2:
> + case 3:
> + reg_info->reg = QCA8K_LED_CTRL_REG(3);
> + shift = 2 * led_num + (6 * (port_num - 1));
> +
> + reg_info->shift = 8 + shift;
> +
> + break;
> + case 4:
> + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> + reg_info->shift = 30;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
>
> +
> +static void
> +qca8k_led_brightness_set(struct qca8k_led *led,
> + enum led_brightness b)
variable name other than b to have readability in code
> +{
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num,
> ®_info);
> +
> + if (b)
> + val = QCA8K_LED_ALWAYS_ON;
> +
> + regmap_update_bits(priv->regmap, reg_info.reg,
> + GENMASK(1, 0) << reg_info.shift,
GENMASK(1, 0) used in multiple place, replace with macro for
readability
> + val << reg_info.shift);
> +}
> +
>
> +
> +static int
> +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led,
> cdev);
> +
Blank line
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num,
> ®_info);
> +
> + if (enable)
> + val = QCA8K_LED_RULE_CONTROLLED;
> +
> + return regmap_update_bits(priv->regmap, reg_info.reg,
> + GENMASK(1, 0) << reg_info.shift,
> + val << reg_info.shift);
> +}
> +
>
> +static bool
> +qca8k_cled_hw_control_status(struct led_classdev *ldev)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led,
> cdev);
> +
Blank line
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 val;
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num,
> ®_info);
> +
> + regmap_read(priv->regmap, reg_info.reg, &val);
> +
> + val >>= reg_info.shift;
> + val &= GENMASK(1, 0);
> +
> + return val == QCA8K_LED_RULE_CONTROLLED;
> +}
> +
>
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link: https://lore.kernel.org/r/20221214235438.30271-2-ansuelsmth%40gmail.com
patch subject: [PATCH v7 01/11] leds: add support for hardware driven LEDs
config: riscv-randconfig-r001-20221214
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 98b13979fb05f3ed288a900deb843e7b27589e58)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
git checkout f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/leds/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/leds/led-triggers.c:205:17: error: no member named 'hw_control_status' in 'struct led_classdev'
led_cdev->hw_control_status(led_cdev))
~~~~~~~~ ^
>> drivers/leds/led-triggers.c:206:14: error: no member named 'hw_control_stop' in 'struct led_classdev'
led_cdev->hw_control_stop(led_cdev);
~~~~~~~~ ^
2 errors generated.
vim +205 drivers/leds/led-triggers.c
177
178 /* Caller must ensure led_cdev->trigger_lock held */
179 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
180 {
181 char *event = NULL;
182 char *envp[2];
183 const char *name;
184 int ret;
185
186 if (!led_cdev->trigger && !trig)
187 return 0;
188
189 name = trig ? trig->name : "none";
190 event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
191
192 /* Remove any existing trigger */
193 if (led_cdev->trigger) {
194 spin_lock(&led_cdev->trigger->leddev_list_lock);
195 list_del_rcu(&led_cdev->trig_list);
196 spin_unlock(&led_cdev->trigger->leddev_list_lock);
197
198 /* ensure it's no longer visible on the led_cdevs list */
199 synchronize_rcu();
200
201 cancel_work_sync(&led_cdev->set_brightness_work);
202 led_stop_software_blink(led_cdev);
203 /* Disable hardware mode on trigger change if supported */
204 if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
> 205 led_cdev->hw_control_status(led_cdev))
> 206 led_cdev->hw_control_stop(led_cdev);
207 if (led_cdev->trigger->deactivate)
208 led_cdev->trigger->deactivate(led_cdev);
209 device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
210 led_cdev->trigger = NULL;
211 led_cdev->trigger_data = NULL;
212 led_cdev->activated = false;
213 led_set_brightness(led_cdev, LED_OFF);
214 }
215 if (trig) {
216 /* Make sure the trigger support the LED blink mode */
217 if (!led_trigger_is_supported(led_cdev, trig))
218 return -EINVAL;
219
220 spin_lock(&trig->leddev_list_lock);
221 list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
222 spin_unlock(&trig->leddev_list_lock);
223 led_cdev->trigger = trig;
224
225 if (trig->activate)
226 ret = trig->activate(led_cdev);
227 else
228 ret = 0;
229
230 if (ret)
231 goto err_activate;
232
233 ret = device_add_groups(led_cdev->dev, trig->groups);
234 if (ret) {
235 dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
236 goto err_add_groups;
237 }
238 }
239
240 if (event) {
241 envp[0] = event;
242 envp[1] = NULL;
243 if (kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp))
244 dev_err(led_cdev->dev,
245 "%s: Error sending uevent\n", __func__);
246 kfree(event);
247 }
248
249 return 0;
250
251 err_add_groups:
252
253 if (trig->deactivate)
254 trig->deactivate(led_cdev);
255 err_activate:
256
257 spin_lock(&led_cdev->trigger->leddev_list_lock);
258 list_del_rcu(&led_cdev->trig_list);
259 spin_unlock(&led_cdev->trigger->leddev_list_lock);
260 synchronize_rcu();
261 led_cdev->trigger = NULL;
262 led_cdev->trigger_data = NULL;
263 led_set_brightness(led_cdev, LED_OFF);
264 kfree(event);
265
266 return ret;
267 }
268 EXPORT_SYMBOL_GPL(led_trigger_set);
269
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link: https://lore.kernel.org/r/20221214235438.30271-2-ansuelsmth%40gmail.com
patch subject: [PATCH v7 01/11] leds: add support for hardware driven LEDs
config: arc-randconfig-r005-20221214
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
git checkout f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/leds/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/leds/led-triggers.c: In function 'led_trigger_set':
>> drivers/leds/led-triggers.c:205:29: error: 'struct led_classdev' has no member named 'hw_control_status'
205 | led_cdev->hw_control_status(led_cdev))
| ^~
>> drivers/leds/led-triggers.c:206:33: error: 'struct led_classdev' has no member named 'hw_control_stop'
206 | led_cdev->hw_control_stop(led_cdev);
| ^~
vim +205 drivers/leds/led-triggers.c
177
178 /* Caller must ensure led_cdev->trigger_lock held */
179 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
180 {
181 char *event = NULL;
182 char *envp[2];
183 const char *name;
184 int ret;
185
186 if (!led_cdev->trigger && !trig)
187 return 0;
188
189 name = trig ? trig->name : "none";
190 event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
191
192 /* Remove any existing trigger */
193 if (led_cdev->trigger) {
194 spin_lock(&led_cdev->trigger->leddev_list_lock);
195 list_del_rcu(&led_cdev->trig_list);
196 spin_unlock(&led_cdev->trigger->leddev_list_lock);
197
198 /* ensure it's no longer visible on the led_cdevs list */
199 synchronize_rcu();
200
201 cancel_work_sync(&led_cdev->set_brightness_work);
202 led_stop_software_blink(led_cdev);
203 /* Disable hardware mode on trigger change if supported */
204 if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
> 205 led_cdev->hw_control_status(led_cdev))
> 206 led_cdev->hw_control_stop(led_cdev);
207 if (led_cdev->trigger->deactivate)
208 led_cdev->trigger->deactivate(led_cdev);
209 device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
210 led_cdev->trigger = NULL;
211 led_cdev->trigger_data = NULL;
212 led_cdev->activated = false;
213 led_set_brightness(led_cdev, LED_OFF);
214 }
215 if (trig) {
216 /* Make sure the trigger support the LED blink mode */
217 if (!led_trigger_is_supported(led_cdev, trig))
218 return -EINVAL;
219
220 spin_lock(&trig->leddev_list_lock);
221 list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
222 spin_unlock(&trig->leddev_list_lock);
223 led_cdev->trigger = trig;
224
225 if (trig->activate)
226 ret = trig->activate(led_cdev);
227 else
228 ret = 0;
229
230 if (ret)
231 goto err_activate;
232
233 ret = device_add_groups(led_cdev->dev, trig->groups);
234 if (ret) {
235 dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
236 goto err_add_groups;
237 }
238 }
239
240 if (event) {
241 envp[0] = event;
242 envp[1] = NULL;
243 if (kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp))
244 dev_err(led_cdev->dev,
245 "%s: Error sending uevent\n", __func__);
246 kfree(event);
247 }
248
249 return 0;
250
251 err_add_groups:
252
253 if (trig->deactivate)
254 trig->deactivate(led_cdev);
255 err_activate:
256
257 spin_lock(&led_cdev->trigger->leddev_list_lock);
258 list_del_rcu(&led_cdev->trig_list);
259 spin_unlock(&led_cdev->trigger->leddev_list_lock);
260 synchronize_rcu();
261 led_cdev->trigger = NULL;
262 led_cdev->trigger_data = NULL;
263 led_set_brightness(led_cdev, LED_OFF);
264 kfree(event);
265
266 return ret;
267 }
268 EXPORT_SYMBOL_GPL(led_trigger_set);
269
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi,
thanks for the new series.
Am Donnerstag, 15. Dezember 2022, 00:54:27 CET schrieb Christian Marangi:
> This is another attempt on adding this feature on LEDs, hoping this is
> the right time and someone finally notice this.
Unfortunately I'm out of office from next week on, so there is only limited
feedback from my side.
> Most of the times Switch/PHY have connected multiple LEDs that are
> controlled by HW based on some rules/event. Currently we lack any
> support for a generic way to control the HW part and normally we
> either never implement the feature or only add control for brightness
> or hw blink.
>
> This is based on Marek idea of providing some API to cled but use a
> different implementation that in theory should be more generilized.
>
> The current idea is:
> - LED driver implement 3 API (hw_control_status/start/stop).
> They are used to put the LED in hardware mode and to configure the
> various trigger.
> - We have hardware triggers that are used to expose to userspace the
> supported hardware mode and set the hardware mode on trigger
> activation.
> - We can also have triggers that both support hardware and software mode.
> - The LED driver will declare each supported hardware blink mode and
> communicate with the trigger all the supported blink modes that will
> be available by sysfs.
> - A trigger will use blink_set to configure the blink mode to active
> in hardware mode.
> - On hardware trigger activation, only the hardware mode is enabled but
> the blink modes are not configured. The LED driver should reset any
> link mode active by default.
I'm a bit confused about that blink mode is supposed to mean. I don't know
what to implement for blink_set. Reading qca8k_cled_blink_set it seems to just
configure the blink interval for the corresponding LED.
Unfortunately that's not possible for all PHYs. In my case, DP83867, I can
configure the blink interval only globally. I'm not sure how this will fit
into this LED trigger.
> Each LED driver will have to declare explicit support for the offload
> trigger (or return not supported error code) as we the trigger_data that
> the LED driver will elaborate and understand what is referring to (based
> on the current active trigger).
>
> I posted a user for this new implementation that will benefit from this
> and will add a big feature to it. Currently qca8k can have up to 3 LEDs
> connected to each PHY port and we have some device that have only one of
> them connected and the default configuration won't work for that.
My DP83867 proof of concept implementation also supports several LEDs, but my
actual hardware also only has 2 of them attached (LED0 and LED2).
Best regards,
Alexander
> The netdev trigger is expanded and it does now support hardware only
> triggers.
> The idea is to use hardware mode when a device_name is not defined.
> An additional sysfs entry is added to give some info about the available
> trigger modes supported in the current configuration.
>
>
> It was reported that at least 3 other switch family would benefits by
> this as they all lack support for a generic way to setup their leds and
> netdev team NACK each try to add special code to support LEDs present
> on switch in favor of a generic solution.
>
> v7:
> - Rebase on top of net-next (for qca8k changes)
> - Fix some typo in commit description
> - Fix qca8k leds documentation warning
> - Remove RFC tag
> v6:
> - Back to RFC.
> - Drop additional trigger
> - Rework netdev trigger to support common modes used by switch and
> hardware only triggers
> - Refresh qca8k leds logic and driver
> v5:
> - Move out of RFC. (no comments from Andrew this is the right path?)
> - Fix more spelling mistake (thx Randy)
> - Fix error reported by kernel test bot
> - Drop the additional HW_CONTROL flag. It does simplify CONFIG
> handling and hw control should be available anyway to support
> triggers as module.
> v4:
> - Rework implementation and drop hw_configure logic.
> We now expand blink_set.
> - Address even more spelling mistake. (thx a lot Randy)
> - Drop blink option and use blink_set delay.
> - Rework phy-activity trigger to actually make the groups dynamic.
> v3:
> - Rework start/stop as Andrew asked.
> - Introduce more logic to permit a trigger to run in hardware mode.
> - Add additional patch with netdev hardware support.
> - Use test_bit API to check flag passed to hw_control_configure.
> - Added a new cmd to hw_control_configure to reset any active blink_mode.
> - Refactor all the patches to follow this new implementation.
> v2:
> - Fix spelling mistake (sorry)
> - Drop patch 02 "permit to declare supported offload triggers".
> Change the logic, now the LED driver declare support for them
> using the configure_offload with the cmd TRIGGER_SUPPORTED.
> - Rework code to follow this new implementation.
> - Update Documentation to better describe how this offload
> implementation work.
>
> Christian Marangi (11):
> leds: add support for hardware driven LEDs
> leds: add function to configure hardware controlled LED
> leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode
> leds: trigger: netdev: rename and expose NETDEV trigger enum modes
> leds: trigger: netdev: convert device attr to macro
> leds: trigger: netdev: add hardware control support
> leds: trigger: netdev: use mutex instead of spinlocks
> leds: trigger: netdev: add available mode sysfs attr
> leds: trigger: netdev: add additional hardware only triggers
> net: dsa: qca8k: add LEDs support
> dt-bindings: net: dsa: qca8k: add LEDs definition example
>
> .../devicetree/bindings/net/dsa/qca8k.yaml | 24 ++
> Documentation/leds/leds-class.rst | 53 +++
> drivers/leds/Kconfig | 11 +
> drivers/leds/led-class.c | 27 ++
> drivers/leds/led-triggers.c | 29 ++
> drivers/leds/trigger/ledtrig-netdev.c | 385 ++++++++++++-----
> drivers/net/dsa/qca/Kconfig | 9 +
> drivers/net/dsa/qca/Makefile | 1 +
> drivers/net/dsa/qca/qca8k-8xxx.c | 4 +
> drivers/net/dsa/qca/qca8k-leds.c | 406 ++++++++++++++++++
> drivers/net/dsa/qca/qca8k.h | 62 +++
> include/linux/leds.h | 103 ++++-
> 12 files changed, 1015 insertions(+), 99 deletions(-)
> create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
Hi Christian,
Thanks for the patch.
I think Andrew's email is offline at the moment.
On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> +static bool led_trigger_is_supported(struct led_classdev *led_cdev,
> + struct led_trigger *trigger)
> +{
> + switch (led_cdev->blink_mode) {
> + case SOFTWARE_CONTROLLED:
> + if (trigger->supported_blink_modes == HARDWARE_ONLY)
> + return 0;
> + break;
> + case HARDWARE_CONTROLLED:
> + if (trigger->supported_blink_modes == SOFTWARE_ONLY)
> + return 0;
> + break;
> + case SOFTWARE_HARDWARE_CONTROLLED:
> + break;
> + default:
> + return 0;
> + }
> +
> + return 1;
Should be returning true/false. I'm not sure I'm a fan of the style of
this though - wouldn't the following be easier to read?
switch (led_cdev->blink_mode) {
case SOFTWARE_CONTROLLED:
return trigger->supported_blink_modes != HARDWARE_ONLY;
case HARDWARE_CONTROLLED:
return trigger->supported_blink_modes != SOFTWARE_ONLY;
case SOFTWARE_HARDWARE_CONTROLLED:
return true;
}
?
Also, does it really need a default case - without it, when the
led_blink_modes enum is expanded and the switch statement isn't
updated, we'll get a compiler warning which will prompt this to be
updated - whereas, with a default case, it won't.
> @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> led_set_brightness(led_cdev, LED_OFF);
> }
> if (trig) {
> + /* Make sure the trigger support the LED blink mode */
> + if (!led_trigger_is_supported(led_cdev, trig))
> + return -EINVAL;
Shouldn't validation happen before we start taking any actions? In other
words, before we remove the previous trigger?
> @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
>
> #define TRIG_NAME_MAX 50
>
> +enum led_trigger_blink_supported_modes {
> + SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> + HARDWARE_ONLY = HARDWARE_CONTROLLED,
> + SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
I suspect all these generic names are asking for eventual namespace
clashes. Maybe prefix them with LED_ ?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi Christian,
On Thu, Dec 15, 2022 at 12:54:29AM +0100, Christian Marangi wrote:
> +A trigger once he declared support for hardware controlled blinks, will use the function
> +hw_control_configure() provided by the driver to check support for a particular blink mode.
Please improve the above. I think what is actually meant is "Where a
trigger has support for hardware controlled blink modes,
hw_control_configure() will be used to check whether a particular blink
mode is supported and configure the blink mode."
> +This function passes as the first argument (flag) a u32 flag.
I don't think "(flag)" is necessary, as I think "a u32 flag"
sufficiently suggests that it's called "flag". In any case, it doesn't
appear to be a "u32" but an "unsigned long".
> +The second argument (cmd) of hw_control_configure() method can be used to do various
> +operations for the specific blink mode. We currently support ENABLE, DISABLE, READ, ZERO
> +and SUPPORTED to enable, disable, read the state of the blink mode, ask the LED
> +driver if it does supports the specific blink mode and to reset any blink mode active.
> +
> +In ENABLE/DISABLE hw_control_configure() should configure the LED to enable/disable the
> +requested blink mode (flag).
> +In READ hw_control_configure() should return 0 or 1 based on the status of the requested
> +blink mode (flag).
> +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
> +requested blink mode (flags) or not.
> +In ZERO hw_control_configure() should return 0 with success operation or error.
I think some kind of tabular description of this would be better.
Something like this but on docbook format:
hw_control_configure()'s second argument, cmd, is used to specify
various operations for the LED blink mode, and will be one of:
ENABLE - to enable the blink mode requested in flag. Returns ?
DISABLE - to disable the blink mode requested in flag. Returbns ?
READ - to indicate whether the blink mode requested in flag is enabled.
Returns 0 if disabled or 1 if enabled.
SUPPORTED - to indicate whether the blink mode requested in flag is
supported. Returns 0 if unsupported or 1 if supported.
ZERO - to disable all blink modes. Returns 0 or negative errno.
The problem with the way you've listed it is you've listed the
operations in a different order to the description in the same sentence,
so effectiely ZERO means to report whether supported and SUPPORTED means
to reset all blink modes!
> +
> +The unsigned long flag is specific to the trigger and change across them. It's in the LED
> +driver interest know how to elaborate this flag and to declare support for a
> +particular trigger.
Hmm, as a casual observer, this doesn't really give much information.
Does this mean that it's up to the hardware LED driver to decide what
each bit in the "flag" argument means? If not, I think this needs to
be worded better!
> For this exact reason explicit support for the specific
> +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
> +with a not supported trigger.
Seems to be a change in terminology - weren't we using "HARDWARE" and
"SOFTWARE" to describe the mode, rather than "offload" ?
> +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
> +fail as the driver doesn't support that specific offload trigger or doesn't know
> +how to handle the provided flags.
This gets rather ambiguous. When can -EOPNOTSUPP be returned - for which
cmds? Surely, if we have already tested for support using the SUPPORTED
cmd which returns a 0/1 value, we should not be going on to trigger a
request to enable something that isn't supported?
> +
> Known Issues
> ============
>
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 09ff1dc6f48d..b5aad67fecfb 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -73,6 +73,16 @@ enum led_blink_modes {
> SOFTWARE_HARDWARE_CONTROLLED,
> };
>
> +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> +enum blink_mode_cmd {
> + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> + BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> + BLINK_MODE_ZERO, /* Disable any hardware blink active */
> +};
> +#endif
Generally not a good idea to make definitions in header files
conditional on configuration symbols - it makes build-testing more
problematical.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi,
On Thu, Dec 15, 2022 at 12:54:37AM +0100, Christian Marangi wrote:
> +static int
> +qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
> + enum blink_mode_cmd cmd)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> + struct led_trigger *trigger = ldev->trigger;
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 offload_trigger = 0, mask, val;
> + int ret;
> +
> + /* Check trigger compatibility */
> + if (strcmp(trigger->name, "netdev"))
> + return -EOPNOTSUPP;
> +
> + if (!strcmp(trigger->name, "netdev"))
> + ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
I'm not sure how well the compiler will spot that, but as far as
readability goes, that second if() statement appears to be redundant.
> +
> + if (ret)
> + return ret;
> +
> + qca8k_get_control_led_reg(led->port_num, led->led_num, ®_info);
> +
> + switch (cmd) {
> + case BLINK_MODE_SUPPORTED:
> + /* We reach this point, we are sure the trigger is supported */
> + return 1;
> + case BLINK_MODE_ZERO:
> + /* We set 4hz by default */
> + u32 default_reg = QCA8K_LED_BLINK_4HZ;
> +
> + ret = regmap_update_bits(priv->regmap, reg_info.reg,
> + QCA8K_LED_RULE_MASK << reg_info.shift,
> + default_reg << reg_info.shift);
> + break;
> + case BLINK_MODE_ENABLE:
> + ret = regmap_update_bits(priv->regmap, reg_info.reg,
> + mask << reg_info.shift,
> + offload_trigger << reg_info.shift);
> + break;
> + case BLINK_MODE_DISABLE:
> + ret = regmap_update_bits(priv->regmap, reg_info.reg,
> + mask << reg_info.shift,
> + 0);
> + break;
I think it needs to be made more clear in the documentation that if
this is called with ENABLE, then _only_ those modes in flags... (or
is it now called "rules"?) are to be enabled and all other modes
should be disabled. Conversely, DISABLE is used to disable all
modes. However, if that's the case, then ZERO was misdescribed, and
should probably be called DEFAULT. At least that's the impression
that I get from the above code.
> + case BLINK_MODE_READ:
> + ret = regmap_read(priv->regmap, reg_info.reg, &val);
> + if (ret)
> + return ret;
> +
> + val >>= reg_info.shift;
> + val &= offload_trigger;
> +
> + /* Special handling for LED_BLINK_2HZ */
> + if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
> + val = 1;
Hmm, so if a number of different modes is in flags or rules, then
as long as one matches, this returns 1? So it's an "any of these
modes is enabled" test.
> +
> + return val;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static void
> +qca8k_led_brightness_set(struct qca8k_led *led,
> + enum led_brightness b)
> +{
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> +
> + if (b)
> + val = QCA8K_LED_ALWAYS_ON;
> +
> + regmap_update_bits(priv->regmap, reg_info.reg,
> + GENMASK(1, 0) << reg_info.shift,
> + val << reg_info.shift);
> +}
> +
> +static void
> +qca8k_cled_brightness_set(struct led_classdev *ldev,
> + enum led_brightness b)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> + return qca8k_led_brightness_set(led, b);
> +}
> +
> +static enum led_brightness
> +qca8k_led_brightness_get(struct qca8k_led *led)
> +{
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 val;
> + int ret;
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> +
> + ret = regmap_read(priv->regmap, reg_info.reg, &val);
> + if (ret)
> + return 0;
> +
> + val >>= reg_info.shift;
> + val &= GENMASK(1, 0);
> +
> + return val > 0 ? 1 : 0;
> +}
> +
> +static enum led_brightness
> +qca8k_cled_brightness_get(struct led_classdev *ldev)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> + return qca8k_led_brightness_get(led);
> +}
> +
> +static int
> +qca8k_cled_blink_set(struct led_classdev *ldev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> +
> + if (*delay_on == 0 && *delay_off == 0) {
> + *delay_on = 125;
> + *delay_off = 125;
> + }
> +
> + if (*delay_on != 125 || *delay_off != 125) {
> + /* The hardware only supports blinking at 4Hz. Fall back
> + * to software implementation in other cases.
> + */
> + return -EINVAL;
> + }
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> +
> + regmap_update_bits(priv->regmap, reg_info.reg,
> + GENMASK(1, 0) << reg_info.shift,
> + QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
> +
> + return 0;
> +}
> +
> +static int
> +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> +
> + if (enable)
> + val = QCA8K_LED_RULE_CONTROLLED;
> +
> + return regmap_update_bits(priv->regmap, reg_info.reg,
> + GENMASK(1, 0) << reg_info.shift,
> + val << reg_info.shift);
88e151x doesn't have the ability to change in this way - we have
a register with a 4-bit field which selects the LED mode from one
of many, or forces the LED on/off/hi-z/blink.
Not specifically for this patch, but talking generally about this
approach, the other issue I forsee with this is that yes, 88e151x has
three LEDs, but the LED modes are also used to implement control
signals (e.g., on a SFP, LOS can be implemented by programming mode
0 on LED2 (which makes it indicate link or not.) If we expose all the
LEDs we run the risk of the LED subsystem trampling over that
configuration and essentially messing up such modules. So the Marvell
PHY driver would need to know when it is appropriate to expose these
things to the LED subsystem.
I guess doing it dependent on firmware description as you do in
this driver would work - if there's no firmware description, they're
not exposed.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Dec 16, 2022 at 05:45:25PM +0100, Christian Marangi wrote:
> On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> > Hi Christian,
> >
> > Thanks for the patch.
> >
> > I think Andrew's email is offline at the moment.
> >
>
> Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
> Holidy times I guess?
Sadly, Andrew's email has done this a number of times - and Andrew
used to be on IRC so I could prod him about it, but it seems he
doesn't hang out on IRC anymore. It's been like it a few days now.
> > On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> > > @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> > > led_set_brightness(led_cdev, LED_OFF);
> > > }
> > > if (trig) {
> > > + /* Make sure the trigger support the LED blink mode */
> > > + if (!led_trigger_is_supported(led_cdev, trig))
> > > + return -EINVAL;
> >
> > Shouldn't validation happen before we start taking any actions? In other
> > words, before we remove the previous trigger?
> >
>
> trigger_set first remove any trigger and set the led off. Then apply the
> new trigger. So the validation is done only when a trigger is actually
> applied. Think we should understand the best case here.
I think this is a question that needs to be answered by the LEDs folk,
as it's an interface behaviour / quality of implementation issue.
> > > @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
> > >
> > > #define TRIG_NAME_MAX 50
> > >
> > > +enum led_trigger_blink_supported_modes {
> > > + SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> > > + HARDWARE_ONLY = HARDWARE_CONTROLLED,
> > > + SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
> >
> > I suspect all these generic names are asking for eventual namespace
> > clashes. Maybe prefix them with LED_ ?
>
> Agree they are pretty generic so I can see why... My only concern was
> making them too long... Maybe reduce them to SW or HW? LEDS_SW_ONLY...
> LEDS_SW_CONTROLLED?
Seems sensible to me - and as a bonus they get shorter than the above!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Dec 15, 2022 at 04:30:49PM +0000, Russell King (Oracle) wrote:
> Hi Christian,
>
> On Thu, Dec 15, 2022 at 12:54:29AM +0100, Christian Marangi wrote:
> > +A trigger once he declared support for hardware controlled blinks, will use the function
> > +hw_control_configure() provided by the driver to check support for a particular blink mode.
>
> Please improve the above. I think what is actually meant is "Where a
> trigger has support for hardware controlled blink modes,
> hw_control_configure() will be used to check whether a particular blink
> mode is supported and configure the blink mode."
>
Ok I will improve!
> > +This function passes as the first argument (flag) a u32 flag.
>
> I don't think "(flag)" is necessary, as I think "a u32 flag"
> sufficiently suggests that it's called "flag". In any case, it doesn't
> appear to be a "u32" but an "unsigned long".
>
Will drop flag and fix the type.
> > +The second argument (cmd) of hw_control_configure() method can be used to do various
> > +operations for the specific blink mode. We currently support ENABLE, DISABLE, READ, ZERO
> > +and SUPPORTED to enable, disable, read the state of the blink mode, ask the LED
> > +driver if it does supports the specific blink mode and to reset any blink mode active.
> > +
> > +In ENABLE/DISABLE hw_control_configure() should configure the LED to enable/disable the
> > +requested blink mode (flag).
> > +In READ hw_control_configure() should return 0 or 1 based on the status of the requested
> > +blink mode (flag).
> > +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
> > +requested blink mode (flags) or not.
> > +In ZERO hw_control_configure() should return 0 with success operation or error.
>
> I think some kind of tabular description of this would be better.
> Something like this but on docbook format:
>
> hw_control_configure()'s second argument, cmd, is used to specify
> various operations for the LED blink mode, and will be one of:
>
> ENABLE - to enable the blink mode requested in flag. Returns ?
> DISABLE - to disable the blink mode requested in flag. Returbns ?
> READ - to indicate whether the blink mode requested in flag is enabled.
> Returns 0 if disabled or 1 if enabled.
> SUPPORTED - to indicate whether the blink mode requested in flag is
> supported. Returns 0 if unsupported or 1 if supported.
> ZERO - to disable all blink modes. Returns 0 or negative errno.
>
> The problem with the way you've listed it is you've listed the
> operations in a different order to the description in the same sentence,
> so effectiely ZERO means to report whether supported and SUPPORTED means
> to reset all blink modes!
>
> > +
> > +The unsigned long flag is specific to the trigger and change across them. It's in the LED
> > +driver interest know how to elaborate this flag and to declare support for a
> > +particular trigger.
>
> Hmm, as a casual observer, this doesn't really give much information.
> Does this mean that it's up to the hardware LED driver to decide what
> each bit in the "flag" argument means? If not, I think this needs to
> be worded better!
>
The idea behind this is that the leds driver needs to have some code to
parse the flag provided by the supported trigger...
Example:
- netdev trigger provide flag with BLINK_TX BLINK_RX
- driver needs to have explicit support for netdev trigger and will
parse the passed flag to enable the function internally.
So no the driver needs to just reflect what it was requested by the
trigger. It's the trigger the one that define flags structure.
Ideally the driver will just include the trigger header and refer to the
same bits the trigger declare in the flag.
> > For this exact reason explicit support for the specific
> > +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
> > +with a not supported trigger.
>
> Seems to be a change in terminology - weren't we using "HARDWARE" and
> "SOFTWARE" to describe the mode, rather than "offload" ?
>
Will fix!
> > +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
> > +fail as the driver doesn't support that specific offload trigger or doesn't know
> > +how to handle the provided flags.
>
> This gets rather ambiguous. When can -EOPNOTSUPP be returned - for which
> cmds? Surely, if we have already tested for support using the SUPPORTED
> cmd which returns a 0/1 value, we should not be going on to trigger a
> request to enable something that isn't supported?
>
Currently we report -EOPNOTSUPP when a trigger is not
supported. hw_control_start/stop/status is just related to activate the
hw_control but not configuring it. In old implementation it was
suggested to have this kind of split and separation to not overload the
configure function and have them split.
(configure enable/disable are about specific trigger rules not the hw
control mode)
I will try to refactor this to be more explicit.
> > +
> > Known Issues
> > ============
> >
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 09ff1dc6f48d..b5aad67fecfb 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -73,6 +73,16 @@ enum led_blink_modes {
> > SOFTWARE_HARDWARE_CONTROLLED,
> > };
> >
> > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > +enum blink_mode_cmd {
> > + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> > + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> > + BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> > + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> > + BLINK_MODE_ZERO, /* Disable any hardware blink active */
> > +};
> > +#endif
>
> Generally not a good idea to make definitions in header files
> conditional on configuration symbols - it makes build-testing more
> problematical.
Guess I will have to add namespace tag and drop the ifdef.
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
--
Ansuel
On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> Hi Christian,
>
> Thanks for the patch.
>
> I think Andrew's email is offline at the moment.
>
Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
Holidy times I guess?
> On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> > +static bool led_trigger_is_supported(struct led_classdev *led_cdev,
> > + struct led_trigger *trigger)
> > +{
> > + switch (led_cdev->blink_mode) {
> > + case SOFTWARE_CONTROLLED:
> > + if (trigger->supported_blink_modes == HARDWARE_ONLY)
> > + return 0;
> > + break;
> > + case HARDWARE_CONTROLLED:
> > + if (trigger->supported_blink_modes == SOFTWARE_ONLY)
> > + return 0;
> > + break;
> > + case SOFTWARE_HARDWARE_CONTROLLED:
> > + break;
> > + default:
> > + return 0;
> > + }
> > +
> > + return 1;
>
> Should be returning true/false. I'm not sure I'm a fan of the style of
> this though - wouldn't the following be easier to read?
>
> switch (led_cdev->blink_mode) {
> case SOFTWARE_CONTROLLED:
> return trigger->supported_blink_modes != HARDWARE_ONLY;
>
> case HARDWARE_CONTROLLED:
> return trigger->supported_blink_modes != SOFTWARE_ONLY;
>
> case SOFTWARE_HARDWARE_CONTROLLED:
> return true;
> }
> ?
Much better!
>
> Also, does it really need a default case - without it, when the
> led_blink_modes enum is expanded and the switch statement isn't
> updated, we'll get a compiler warning which will prompt this to be
> updated - whereas, with a default case, it won't.
>
I added the default just to mute some compiler warning. But guess if
every enum is handled the warning should not be reported.
> > @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> > led_set_brightness(led_cdev, LED_OFF);
> > }
> > if (trig) {
> > + /* Make sure the trigger support the LED blink mode */
> > + if (!led_trigger_is_supported(led_cdev, trig))
> > + return -EINVAL;
>
> Shouldn't validation happen before we start taking any actions? In other
> words, before we remove the previous trigger?
>
trigger_set first remove any trigger and set the led off. Then apply the
new trigger. So the validation is done only when a trigger is actually
applied. Think we should understand the best case here.
> > @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
> >
> > #define TRIG_NAME_MAX 50
> >
> > +enum led_trigger_blink_supported_modes {
> > + SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> > + HARDWARE_ONLY = HARDWARE_CONTROLLED,
> > + SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
>
> I suspect all these generic names are asking for eventual namespace
> clashes. Maybe prefix them with LED_ ?
Agree they are pretty generic so I can see why... My only concern was
making them too long... Maybe reduce them to SW or HW? LEDS_SW_ONLY...
LEDS_SW_CONTROLLED?
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
--
Ansuel
On Thu, Dec 15, 2022 at 05:49:57PM +0000, Russell King (Oracle) wrote:
> Hi,
>
> On Thu, Dec 15, 2022 at 12:54:37AM +0100, Christian Marangi wrote:
> > +static int
> > +qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
> > + enum blink_mode_cmd cmd)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > + struct led_trigger *trigger = ldev->trigger;
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > + u32 offload_trigger = 0, mask, val;
> > + int ret;
> > +
> > + /* Check trigger compatibility */
> > + if (strcmp(trigger->name, "netdev"))
> > + return -EOPNOTSUPP;
> > +
> > + if (!strcmp(trigger->name, "netdev"))
> > + ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
>
> I'm not sure how well the compiler will spot that, but as far as
> readability goes, that second if() statement appears to be redundant.
>
Leftover when the driver supported 2 trigger. The idea was to provide a
"template" to show the flow of the function.
> > +
> > + if (ret)
> > + return ret;
> > +
> > + qca8k_get_control_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + switch (cmd) {
> > + case BLINK_MODE_SUPPORTED:
> > + /* We reach this point, we are sure the trigger is supported */
> > + return 1;
> > + case BLINK_MODE_ZERO:
> > + /* We set 4hz by default */
> > + u32 default_reg = QCA8K_LED_BLINK_4HZ;
> > +
> > + ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > + QCA8K_LED_RULE_MASK << reg_info.shift,
> > + default_reg << reg_info.shift);
> > + break;
> > + case BLINK_MODE_ENABLE:
> > + ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > + mask << reg_info.shift,
> > + offload_trigger << reg_info.shift);
> > + break;
> > + case BLINK_MODE_DISABLE:
> > + ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > + mask << reg_info.shift,
> > + 0);
> > + break;
>
> I think it needs to be made more clear in the documentation that if
> this is called with ENABLE, then _only_ those modes in flags... (or
> is it now called "rules"?) are to be enabled and all other modes
> should be disabled. Conversely, DISABLE is used to disable all
> modes. However, if that's the case, then ZERO was misdescribed, and
> should probably be called DEFAULT. At least that's the impression
> that I get from the above code.
>
ZERO disable all the rules. Driver can keep some rule enabled (this is
the case for qca8k blink mode at 4hz by default)
ENABLE/DISABLE only act on the provided thing in rules. In the current
imlementation parse rules, generate a mask and update the values
accordingly. Other rules are not touched. This is based on the fact that
the first and the last thing done is calling ZERO to reset all the rules
to a known state.
I will try to improve the Documentation on this aspect.
Hope you know understand better the calling flow.
> > + case BLINK_MODE_READ:
> > + ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > + if (ret)
> > + return ret;
> > +
> > + val >>= reg_info.shift;
> > + val &= offload_trigger;
> > +
> > + /* Special handling for LED_BLINK_2HZ */
> > + if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
> > + val = 1;
>
> Hmm, so if a number of different modes is in flags or rules, then
> as long as one matches, this returns 1? So it's an "any of these
> modes is enabled" test.
>
Ok this should be changed and you are right. READ should return true or
false if the rule (or rules) are enabled. This work for a single rule
but doesn't if multiple rules are provided.
Will fix that since this is wrong.
> > +
> > + return val;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void
> > +qca8k_led_brightness_set(struct qca8k_led *led,
> > + enum led_brightness b)
> > +{
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > + u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + if (b)
> > + val = QCA8K_LED_ALWAYS_ON;
> > +
> > + regmap_update_bits(priv->regmap, reg_info.reg,
> > + GENMASK(1, 0) << reg_info.shift,
> > + val << reg_info.shift);
> > +}
> > +
> > +static void
> > +qca8k_cled_brightness_set(struct led_classdev *ldev,
> > + enum led_brightness b)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > + return qca8k_led_brightness_set(led, b);
> > +}
> > +
> > +static enum led_brightness
> > +qca8k_led_brightness_get(struct qca8k_led *led)
> > +{
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > + u32 val;
> > + int ret;
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > + if (ret)
> > + return 0;
> > +
> > + val >>= reg_info.shift;
> > + val &= GENMASK(1, 0);
> > +
> > + return val > 0 ? 1 : 0;
> > +}
> > +
> > +static enum led_brightness
> > +qca8k_cled_brightness_get(struct led_classdev *ldev)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > + return qca8k_led_brightness_get(led);
> > +}
> > +
> > +static int
> > +qca8k_cled_blink_set(struct led_classdev *ldev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > +
> > + if (*delay_on == 0 && *delay_off == 0) {
> > + *delay_on = 125;
> > + *delay_off = 125;
> > + }
> > +
> > + if (*delay_on != 125 || *delay_off != 125) {
> > + /* The hardware only supports blinking at 4Hz. Fall back
> > + * to software implementation in other cases.
> > + */
> > + return -EINVAL;
> > + }
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + regmap_update_bits(priv->regmap, reg_info.reg,
> > + GENMASK(1, 0) << reg_info.shift,
> > + QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > + u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + if (enable)
> > + val = QCA8K_LED_RULE_CONTROLLED;
> > +
> > + return regmap_update_bits(priv->regmap, reg_info.reg,
> > + GENMASK(1, 0) << reg_info.shift,
> > + val << reg_info.shift);
>
> 88e151x doesn't have the ability to change in this way - we have
> a register with a 4-bit field which selects the LED mode from one
> of many, or forces the LED on/off/hi-z/blink.
>
> Not specifically for this patch, but talking generally about this
> approach, the other issue I forsee with this is that yes, 88e151x has
> three LEDs, but the LED modes are also used to implement control
> signals (e.g., on a SFP, LOS can be implemented by programming mode
> 0 on LED2 (which makes it indicate link or not.) If we expose all the
> LEDs we run the risk of the LED subsystem trampling over that
> configuration and essentially messing up such modules. So the Marvell
> PHY driver would need to know when it is appropriate to expose these
> things to the LED subsystem.
>
> I guess doing it dependent on firmware description as you do in
> this driver would work - if there's no firmware description, they're
> not exposed.
>
The idea is to provide a way to tell the driver what should be done and
tell the trigger that something is not doable and to revert to sw mode.
keeping the thing as simple and direct as possible and leave the rest to
the driver by doing minimal validation on the trigger side.
Do you have suggestion on how this can be improved even more and be more
flexible?
--
Ansuel
> > +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > + u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + if (enable)
> > + val = QCA8K_LED_RULE_CONTROLLED;
> > +
> > + return regmap_update_bits(priv->regmap, reg_info.reg,
> > + GENMASK(1, 0) << reg_info.shift,
> > + val << reg_info.shift);
>
> 88e151x doesn't have the ability to change in this way - we have
> a register with a 4-bit field which selects the LED mode from one
> of many, or forces the LED on/off/hi-z/blink.
>
> Not specifically for this patch, but talking generally about this
> approach, the other issue I forsee with this is that yes, 88e151x has
> three LEDs, but the LED modes are also used to implement control
> signals (e.g., on a SFP, LOS can be implemented by programming mode
> 0 on LED2 (which makes it indicate link or not.) If we expose all the
> LEDs we run the risk of the LED subsystem trampling over that
> configuration and essentially messing up such modules. So the Marvell
> PHY driver would need to know when it is appropriate to expose these
> things to the LED subsystem.
>
> I guess doing it dependent on firmware description as you do in
> this driver would work - if there's no firmware description, they're
> not exposed.
>
I expect there will always be some sort of firmware involved,
describing the hardware. Without that, we have no idea how many LEDs
are actually connected to pins of the PHY, for example.
I've not yet looked at this patchset in detail, but i hope the DT
binding code is reusable, so all PHYs will have the same basic
binding.
Andrew
On Fri, Dec 16, 2022 at 05:45:25PM +0100, Christian Marangi wrote:
> On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> > Hi Christian,
> >
> > Thanks for the patch.
> >
> > I think Andrew's email is offline at the moment.
> >
>
> Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
> Holidy times I guess?
That was part of the problem. Away travelling when foot gun hit foot,
and i didn't have access to the tools to fix it while away.
Andrew
On Thu, Dec 15, 2022 at 12:54:31AM +0100, Christian Marangi wrote:
> Rename NETDEV trigger enum modes to a more simbolic name and move them
simbolic -> symbolic.
Andrew
On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> The LED must implement 3 main API:
> - hw_control_status(): This asks the LED driver if hardware mode is
> enabled or not.
> Triggers will check if the offload mode is supported and will be
> activated accordingly. If the trigger can't run in software mode,
> return -EOPNOTSUPP as the blinking can't be simulated by software.
> - hw_control_start(): This will simply enable the hardware mode for
> the LED.
> With this not declared and hw_control_status() returning true,
> it's assumed that the LED is always in hardware mode.
> - hw_control_stop(): This will simply disable the hardware mode for
> the LED.
> With this not declared and hw_control_status() returning true,
> it's assumed that the LED is always in hardware mode.
> It's advised to the driver to put the LED in the old state but this
> is not enforcerd and putting the LED off is also accepted.
>
Building htmldocs with Sphinx, I got:
Documentation/leds/leds-class.rst:190: WARNING: Unexpected indentation.
Documentation/leds/leds-class.rst:192: WARNING: Block quote ends without a blank line; unexpected unindent.
I have applied the fixup on the API bullet list above:
---- >8 ----
diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index efd2f68c46a7f9..fc16b503747800 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -186,13 +186,15 @@ supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTR
is assumed.
The LED must implement 3 main API:
+
- hw_control_status(): This asks the LED driver if hardware mode is enabled
- or not. Triggers will check if the hardware mode is active and will try
- to offload their triggers if supported by the driver.
+ or not. Triggers will check if the hardware mode is active and will try
+ to offload their triggers if supported by the driver.
- hw_control_start(): This will simply enable the hardware mode for the LED.
+
- hw_control_stop(): This will simply disable the hardware mode for the LED.
- It's advised to the driver to put the LED in the old state but this is not
- enforcerd and putting the LED off is also accepted.
+ It's advised to the driver to put the LED in the old state but this is not
+ enforcerd and putting the LED off is also accepted.
With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
and any software only trigger will reject activation as the LED supports only
Thanks.
--
An old man doll... just what I always wanted! - Clara