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

[permalink] [raw]

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

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

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

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

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

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

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index dc6816d36d06..b947b238be3f 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -154,4 +154,32 @@ config LEDS_TRIGGER_TTY

When build as a module this driver will be called ledtrig-tty.

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

+#ifdef CONFIG_LEDS_TRIGGER_HARDWARE_PHY_ACTIVITY
+/* 3 main category for offload trigger:
+ * - blink: the led will blink on trigger
+ * - keep: the led will be kept on with condition
+ * - option: a configuration value internal to the led on how offload works
+ */
+enum hardware_phy_activity {
+ BLINK_TX, /* Blink on TX traffic */
+ BLINK_RX, /* Blink on RX traffic */
+ KEEP_LINK_10M, /* LED on with 10M link */
+ KEEP_LINK_100M, /* LED on with 100M link */
+ KEEP_LINK_1000M, /* LED on with 1000M link */
+ KEEP_HALF_DUPLEX, /* LED on with Half-Duplex link */
+ KEEP_FULL_DUPLEX, /* LED on with Full-Duplex link */
+ OPTION_LINKUP_OVER, /* LED will be on with KEEP blink mode and blink on BLINK traffic */
+ OPTION_POWER_ON_RESET, /* LED will be on Switch reset */
+ OPTION_BLINK_2HZ, /* LED will blink with any offload_trigger at 2Hz */
+ OPTION_BLINK_4HZ, /* LED will blink with any offload_trigger at 4Hz */
+ OPTION_BLINK_8HZ, /* LED will blink with any offload_trigger at 8Hz */
+};
+#endif
+
/* Trigger specific functions */
#ifdef CONFIG_LEDS_TRIGGER_DISK
void ledtrig_disk_activity(bool write);
--
2.32.0

2021-11-09 12:18:36

[permalink] [raw]

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

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

Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 31 ++++++++++++---------------
include/linux/leds.h | 7 ++++++
2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 66a81cc9b64d..0a3c0b54dbb9 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -51,9 +51,6 @@ struct led_netdev_data {

unsigned long mode;
bool carrier_link_up;
-#define NETDEV_LED_LINK 0
-#define NETDEV_LED_TX 1
-#define NETDEV_LED_RX 2
};

enum netdev_led_attr {
@@ -76,7 +73,7 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
if (!trigger_data->carrier_link_up) {
led_set_brightness(led_cdev, LED_OFF);
} else {
- if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
+ if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
led_set_brightness(led_cdev,
led_cdev->blink_brightness);
else
@@ -85,8 +82,8 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
/* If we are looking for RX/TX start periodically
* checking stats
*/
- if (test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
- test_bit(NETDEV_LED_RX, &trigger_data->mode))
+ if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ||
+ test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
schedule_delayed_work(&trigger_data->work, 0);
}
}
@@ -153,13 +150,13 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,

switch (attr) {
case NETDEV_ATTR_LINK:
- bit = NETDEV_LED_LINK;
+ bit = TRIGGER_NETDEV_LINK;
break;
case NETDEV_ATTR_TX:
- bit = NETDEV_LED_TX;
+ bit = TRIGGER_NETDEV_TX;
break;
case NETDEV_ATTR_RX:
- bit = NETDEV_LED_RX;
+ bit = TRIGGER_NETDEV_RX;
break;
default:
return -EINVAL;
@@ -182,13 +179,13 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,

switch (attr) {
case NETDEV_ATTR_LINK:
- bit = NETDEV_LED_LINK;
+ bit = TRIGGER_NETDEV_LINK;
break;
case NETDEV_ATTR_TX:
- bit = NETDEV_LED_TX;
+ bit = TRIGGER_NETDEV_TX;
break;
case NETDEV_ATTR_RX:
- bit = NETDEV_LED_RX;
+ bit = TRIGGER_NETDEV_RX;
break;
default:
return -EINVAL;
@@ -358,21 +355,21 @@ static void netdev_trig_work(struct work_struct *work)
}

/* If we are not looking for RX/TX then return */
- if (!test_bit(NETDEV_LED_TX, &trigger_data->mode) &&
- !test_bit(NETDEV_LED_RX, &trigger_data->mode))
+ if (!test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
+ !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
return;

dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
new_activity =
- (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
+ (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ?
dev_stats->tx_packets : 0) +
- (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
+ (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) ?
dev_stats->rx_packets : 0);

if (trigger_data->last_activity != new_activity) {
led_stop_software_blink(trigger_data->led_cdev);

- invert = test_bit(NETDEV_LED_LINK, &trigger_data->mode);
+ invert = test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode);
interval = jiffies_to_msecs(
atomic_read(&trigger_data->interval));
/* base state is ON (link present) */
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 00bc4d6ed7ca..087debe6716d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -548,6 +548,13 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)

#endif /* CONFIG_LEDS_TRIGGERS */

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

2021-11-09 12:18:38

[permalink] [raw]

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

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

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

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 7b1457a6e327..89e36f3a8195 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -67,6 +67,15 @@ config NET_DSA_QCA8K
This enables support for the Qualcomm Atheros QCA8K Ethernet
switch chips.

+config NET_DSA_QCA8K_LEDS_SUPPORT
+ tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
+ select NET_DSA_QCA8K
+ select LEDS_OFFLOAD_TRIGGERS
+ help
+ Thsi enabled support for LEDs present on the Qualcomm Atheros
+ QCA8K Ethernet switch chips. This require the LEDs offload
+ triggers support as it can run in offload mode.
+
config NET_DSA_REALTEK_SMI
tristate "Realtek SMI Ethernet switch family support"
select NET_DSA_TAG_RTL4_A
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index 8da1569a34e6..fb1e7d0cf126 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_NET_DSA_LANTIQ_GSWIP) += lantiq_gswip.o
obj-$(CONFIG_NET_DSA_MT7530) += mt7530.o
obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
+obj-$(CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT) += qca8k-leds.o
obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
realtek-smi-objs := realtek-smi-core.o rtl8366.o rtl8366rb.o rtl8365mb.o
obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
diff --git a/drivers/net/dsa/qca8k-leds.c b/drivers/net/dsa/qca8k-leds.c
new file mode 100644
index 000000000000..0ca4e4c88f2f
--- /dev/null
+++ b/drivers/net/dsa/qca8k-leds.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <net/dsa.h>
+
+#include "qca8k.h"
+
+static int
+qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+ int shift;
+
+ switch (port_num) {
+ case 0:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = 14;
+ break;
+ case 1:
+ case 2:
+ case 3:
+ reg_info->reg = QCA8K_LED_CTRL_REG(3);
+ shift = 2 * led_num + (6 * (port_num - 1));
+
+ reg_info->shift = 8 + shift;
+
+ break;
+ case 4:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = 30;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+qca8k_get_control_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+
+ /* 6 total control rule:
+ * 3 control rules for phy0-3 that applies to all their leds
+ * 3 control rules for phy4
+ */
+ if (port_num == 4)
+ reg_info->shift = 16;
+ else
+ reg_info->shift = 0;
+
+ return 0;
+}
+
+static int
+qca8k_parse_hardware_phy_activity(unsigned long rules, u32 *offload_trigger,
+ u32 *mask)
+{
+ /* Parsing specific to hardware-phy-activity trigger */
+ if (test_bit(BLINK_TX, &rules))
+ *offload_trigger = QCA8K_LED_TX_BLINK_MASK;
+ if (test_bit(BLINK_RX, &rules))
+ *offload_trigger = QCA8K_LED_RX_BLINK_MASK;
+ if (test_bit(KEEP_LINK_10M, &rules))
+ *offload_trigger = QCA8K_LED_LINK_10M_EN_MASK;
+ if (test_bit(KEEP_LINK_100M, &rules))
+ *offload_trigger = QCA8K_LED_LINK_100M_EN_MASK;
+ if (test_bit(KEEP_LINK_1000M, &rules))
+ *offload_trigger = QCA8K_LED_LINK_1000M_EN_MASK;
+ if (test_bit(KEEP_HALF_DUPLEX, &rules))
+ *offload_trigger = QCA8K_LED_HALF_DUPLEX_MASK;
+ if (test_bit(KEEP_FULL_DUPLEX, &rules))
+ *offload_trigger = QCA8K_LED_FULL_DUPLEX_MASK;
+ if (test_bit(OPTION_LINKUP_OVER, &rules))
+ *offload_trigger = QCA8K_LED_LINKUP_OVER_MASK;
+ if (test_bit(OPTION_BLINK_2HZ, &rules))
+ *offload_trigger = QCA8K_LED_BLINK_2HZ;
+ if (test_bit(OPTION_BLINK_4HZ, &rules))
+ *offload_trigger = QCA8K_LED_BLINK_4HZ;
+ if (test_bit(OPTION_BLINK_8HZ, &rules))
+ *offload_trigger = QCA8K_LED_BLINK_8HZ;
+
+ if (!*offload_trigger)
+ return -EOPNOTSUPP;
+
+ if (test_bit(OPTION_BLINK_2HZ, &rules) ||
+ test_bit(OPTION_BLINK_4HZ, &rules) ||
+ test_bit(OPTION_BLINK_8HZ, &rules)) {
+ *mask = QCA8K_LED_BLINK_FREQ_MASK;
+ } else {
+ *mask = *offload_trigger;
+ }
+
+ return 0;
+}
+
+static int
+qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger, u32 *mask)
+{
+ /* Parsing specific to netdev trigger */
+ if (test_bit(TRIGGER_NETDEV_LINK, &rules))
+ *offload_trigger = QCA8K_LED_LINK_10M_EN_MASK |
+ QCA8K_LED_LINK_100M_EN_MASK |
+ QCA8K_LED_LINK_1000M_EN_MASK;
+ if (test_bit(TRIGGER_NETDEV_TX, &rules))
+ *offload_trigger = QCA8K_LED_TX_BLINK_MASK;
+ if (test_bit(TRIGGER_NETDEV_RX, &rules))
+ *offload_trigger = QCA8K_LED_RX_BLINK_MASK;
+
+ if (!*offload_trigger)
+ return -EOPNOTSUPP;
+
+ *mask = *offload_trigger;
+
+ return 0;
+}
+
+static int
+qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
+ enum blink_mode_cmd cmd)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+ struct led_trigger *trigger = ldev->trigger;
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 offload_trigger = 0, mask, val;
+ int ret;
+
+ /* Check trigger compatibility */
+ if (strcmp(trigger->name, "hardware-phy-activity") &&
+ strcmp(trigger->name, "netdev"))
+ return -EOPNOTSUPP;
+
+ if (!strcmp(trigger->name, "hardware-phy-activity"))
+ ret = qca8k_parse_hardware_phy_activity(rules, &offload_trigger, &mask);
+
+ if (!strcmp(trigger->name, "netdev"))
+ ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
+
+ if (ret)
+ return ret;
+
+ qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
+
+ switch (cmd) {
+ case BLINK_MODE_SUPPORTED:
+ /* We reache this point, we are sure the trigger is supported */
+ return 1;
+ case BLINK_MODE_ZERO:
+ /* We set 4hz by default */
+ u32 default_reg = QCA8K_LED_BLINK_4HZ;
+
+ ret = qca8k_rmw(priv, reg_info.reg,
+ QCA8K_LED_RULE_MASK << reg_info.shift,
+ default_reg << reg_info.shift);
+ break;
+ case BLINK_MODE_ENABLE:
+ ret = qca8k_rmw(priv, reg_info.reg,
+ mask << reg_info.shift,
+ offload_trigger << reg_info.shift);
+ break;
+ case BLINK_MODE_DISABLE:
+ ret = qca8k_rmw(priv, reg_info.reg,
+ mask << reg_info.shift,
+ 0);
+ break;
+ case BLINK_MODE_READ:
+ ret = qca8k_read(priv, reg_info.reg, &val);
+ if (ret)
+ return ret;
+
+ val >>= reg_info.shift;
+ val &= offload_trigger;
+
+ /* Special handling for LED_BLINK_2HZ */
+ if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
+ val = 1;
+
+ return val;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return ret;
+}
+
+static void
+qca8k_led_brightness_set(struct qca8k_led *led,
+ enum led_brightness b)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val = QCA8K_LED_ALWAYS_OFF;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+ if (b)
+ val = QCA8K_LED_ALWAYS_ON;
+
+ qca8k_rmw(priv, reg_info.reg,
+ GENMASK(1, 0) << reg_info.shift,
+ val << reg_info.shift);
+}
+
+static void
+qca8k_cled_brightness_set(struct led_classdev *ldev,
+ enum led_brightness b)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ return qca8k_led_brightness_set(led, b);
+}
+
+static enum led_brightness
+qca8k_led_brightness_get(struct qca8k_led *led)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val;
+ int ret;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+ ret = qca8k_read(priv, reg_info.reg, &val);
+ if (ret)
+ return 0;
+
+ val >>= reg_info.shift;
+ val &= GENMASK(1, 0);
+
+ return val > 0 ? 1 : 0;
+}
+
+static enum led_brightness
+qca8k_cled_brightness_get(struct led_classdev *ldev)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ return qca8k_led_brightness_get(led);
+}
+
+static int
+qca8k_cled_blink_set(struct led_classdev *ldev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+
+ if (*delay_on == 0 && *delay_off == 0) {
+ *delay_on = 125;
+ *delay_off = 125;
+ }
+
+ if (*delay_on != 125 || *delay_off != 125) {
+ /* The hardware only supports blinking at 4Hz. Fall back
+ * to software implementation in other cases.
+ */
+ return -EINVAL;
+ }
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+ qca8k_rmw(priv, reg_info.reg,
+ GENMASK(1, 0) << reg_info.shift,
+ QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
+
+ return 0;
+}
+
+static int
+qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val = QCA8K_LED_ALWAYS_OFF;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+ if (enable)
+ val = QCA8K_LED_RULE_CONTROLLED;
+
+ return qca8k_rmw(priv, reg_info.reg,
+ GENMASK(1, 0) << reg_info.shift,
+ val << reg_info.shift);
+}
+
+static int
+qca8k_cled_hw_control_start(struct led_classdev *led_cdev)
+{
+ return qca8k_cled_trigger_offload(led_cdev, true);
+}
+
+static int
+qca8k_cled_hw_control_stop(struct led_classdev *led_cdev)
+{
+ return qca8k_cled_trigger_offload(led_cdev, false);
+}
+
+static bool
+qca8k_cled_hw_control_status(struct led_classdev *ldev)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+ qca8k_read(priv, reg_info.reg, &val);
+
+ val >>= reg_info.shift;
+ val &= GENMASK(1, 0);
+
+ return val == QCA8K_LED_RULE_CONTROLLED;
+}
+
+static int
+qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
+{
+ struct fwnode_handle *led = NULL, *leds = NULL;
+ struct led_init_data init_data = { };
+ struct qca8k_led *port_led;
+ int led_num, port_index;
+ const char *state;
+ int ret;
+
+ leds = fwnode_get_named_child_node(port, "leds");
+ if (!leds) {
+ dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
+ port_num);
+ return 0;
+ }
+
+ fwnode_for_each_child_node(leds, led) {
+ /* Reg rapresent the led number of the port.
+ * Each port can have at least 3 leds attached
+ * Commonly:
+ * 1. is gigabit led
+ * 2. is mbit led
+ * 3. additional status led
+ */
+ if (fwnode_property_read_u32(led, "reg", &led_num))
+ continue;
+
+ if (led_num >= QCA8K_LED_PORT_COUNT) {
+ dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
+ continue;
+ }
+
+ port_index = 3 * port_num + led_num;
+
+ port_led = &priv->ports_led[port_index];
+ port_led->port_num = port_num;
+ port_led->led_num = led_num;
+ port_led->priv = priv;
+
+ ret = fwnode_property_read_string(led, "default-state", &state);
+ if (!ret) {
+ if (!strcmp(state, "on")) {
+ port_led->cdev.brightness = 1;
+ qca8k_led_brightness_set(port_led, 1);
+ } else if (!strcmp(state, "off")) {
+ port_led->cdev.brightness = 0;
+ qca8k_led_brightness_set(port_led, 0);
+ } else if (!strcmp(state, "keep")) {
+ port_led->cdev.brightness =
+ qca8k_led_brightness_get(port_led);
+ }
+ }
+
+ /* 3 brightness settings can be applied from Documentation:
+ * 0 always off
+ * 1 blink at 4Hz
+ * 2 always on
+ * 3 rule controlled
+ * Suppots only 2 mode: (pcb limitation, with always on and blink
+ * only the last led is set to this mode)
+ * 0 always off (sets all leds off)
+ * 3 rule controlled
+ */
+ port_led->cdev.blink_mode = SOFTWARE_HARDWARE_CONTROLLED;
+ port_led->cdev.max_brightness = 1;
+ port_led->cdev.brightness_set = qca8k_cled_brightness_set;
+ port_led->cdev.brightness_get = qca8k_cled_brightness_get;
+ port_led->cdev.blink_set = qca8k_cled_blink_set;
+ port_led->cdev.hw_control_start = qca8k_cled_hw_control_start;
+ port_led->cdev.hw_control_stop = qca8k_cled_hw_control_stop;
+ port_led->cdev.hw_control_status = qca8k_cled_hw_control_status;
+ port_led->cdev.hw_control_configure = qca8k_cled_hw_control_configure;
+ init_data.default_label = ":port";
+ init_data.devicename = "qca8k";
+ init_data.fwnode = led;
+
+ ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
+ if (ret)
+ dev_warn(priv->dev, "Failed to int led");
+ }
+
+ return 0;
+}
+
+int
+qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ struct fwnode_handle *mdio, *port;
+ int port_num;
+ int ret;
+
+ mdio = device_get_named_child_node(priv->dev, "mdio");
+ if (!mdio) {
+ dev_info(priv->dev, "No MDIO node specified in device tree!\n");
+ return 0;
+ }
+
+ fwnode_for_each_child_node(mdio, port) {
+ if (fwnode_property_read_u32(port, "reg", &port_num))
+ continue;
+
+ /* Each port can have at least 3 different leds attached */
+ ret = qca8k_parse_port_leds(priv, port, port_num);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a429c9750add..10b2b47b2156 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -148,7 +148,7 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
return 0;
}

-static int
+int
qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val)
{
struct mii_bus *bus = priv->bus;
@@ -192,7 +192,7 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
return ret;
}

-static int
+int
qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
{
struct mii_bus *bus = priv->bus;
@@ -1109,6 +1109,10 @@ qca8k_setup(struct dsa_switch *ds)
if (ret)
return ret;

+ ret = qca8k_setup_led_ctrl(priv);
+ if (ret)
+ return ret;
+
/* Make sure MAC06 is disabled */
ret = qca8k_reg_clear(priv, QCA8K_REG_PORT0_PAD_CTRL,
QCA8K_PORT0_PAD_MAC06_EXCHANGE_EN);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 128b8cf85e08..f68f037fe909 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/regmap.h>
#include <linux/gpio.h>
+#include <linux/leds.h>

#define QCA8K_NUM_PORTS 7
#define QCA8K_NUM_CPU_PORTS 2
@@ -74,6 +75,43 @@
#define QCA8K_MDIO_MASTER_DATA_MASK GENMASK(15, 0)
#define QCA8K_MDIO_MASTER_MAX_PORTS 5
#define QCA8K_MDIO_MASTER_MAX_REG 32
+
+/* LED control register */
+#define QCA8K_LED_COUNT 15
+#define QCA8K_LED_PORT_COUNT 3
+#define QCA8K_LED_RULE_COUNT 6
+#define QCA8K_LED_RULE_MAX 11
+#define QCA8K_LED_CTRL_REG(_i) (0x050 + (_i) * 4)
+#define QCA8K_LED_CTRL0_REG 0x50
+#define QCA8K_LED_CTRL1_REG 0x54
+#define QCA8K_LED_CTRL2_REG 0x58
+#define QCA8K_LED_CTRL3_REG 0x5C
+#define QCA8K_LED_CTRL_SHIFT(_i) (((_i) % 2) * 16)
+#define QCA8K_LED_CTRL_MASK GENMASK(15, 0)
+#define QCA8K_LED_RULE_MASK GENMASK(13, 0)
+#define QCA8K_LED_BLINK_FREQ_MASK GENMASK(1, 0)
+#define QCA8K_LED_BLINK_FREQ_SHITF 0
+#define QCA8K_LED_BLINK_2HZ 0
+#define QCA8K_LED_BLINK_4HZ 1
+#define QCA8K_LED_BLINK_8HZ 2
+#define QCA8K_LED_BLINK_AUTO 3
+#define QCA8K_LED_LINKUP_OVER_MASK BIT(2)
+#define QCA8K_LED_TX_BLINK_MASK BIT(4)
+#define QCA8K_LED_RX_BLINK_MASK BIT(5)
+#define QCA8K_LED_COL_BLINK_MASK BIT(7)
+#define QCA8K_LED_LINK_10M_EN_MASK BIT(8)
+#define QCA8K_LED_LINK_100M_EN_MASK BIT(9)
+#define QCA8K_LED_LINK_1000M_EN_MASK BIT(10)
+#define QCA8K_LED_POWER_ON_LIGHT_MASK BIT(11)
+#define QCA8K_LED_HALF_DUPLEX_MASK BIT(12)
+#define QCA8K_LED_FULL_DUPLEX_MASK BIT(13)
+#define QCA8K_LED_PATTERN_EN_MASK GENMASK(15, 14)
+#define QCA8K_LED_PATTERN_EN_SHIFT 14
+#define QCA8K_LED_ALWAYS_OFF 0
+#define QCA8K_LED_ALWAYS_BLINK_4HZ 1
+#define QCA8K_LED_ALWAYS_ON 2
+#define QCA8K_LED_RULE_CONTROLLED 3
+
#define QCA8K_GOL_MAC_ADDR0 0x60
#define QCA8K_GOL_MAC_ADDR1 0x64
#define QCA8K_MAX_FRAME_SIZE 0x78
@@ -279,6 +317,19 @@ struct qca8k_ports_config {
u8 rgmii_tx_delay[QCA8K_NUM_CPU_PORTS]; /* 0: CPU port0, 1: CPU port6 */
};

+struct qca8k_led_pattern_en {
+ u32 reg;
+ u8 shift;
+};
+
+struct qca8k_led {
+ u8 port_num;
+ u8 led_num;
+ u16 old_rule;
+ struct qca8k_priv *priv;
+ struct led_classdev cdev;
+};
+
struct qca8k_priv {
u8 switch_id;
u8 switch_revision;
@@ -293,6 +344,7 @@ struct qca8k_priv {
struct dsa_switch_ops ops;
struct gpio_desc *reset_gpio;
unsigned int port_mtu[QCA8K_NUM_PORTS];
+ struct qca8k_led ports_led[QCA8K_LED_COUNT];
};

struct qca8k_mib_desc {
@@ -308,4 +360,17 @@ struct qca8k_fdb {
u8 mac[6];
};

+/* Common function used by the LEDs module */
+int qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val);
+int qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val);
+
+#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
+#else
+static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ return 0;
+}
+#endif
+
#endif /* __QCA8K_H */
--
2.32.0

2021-11-09 12:31:27

[permalink] [raw]

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

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

Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 61 ++++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 0a3c0b54dbb9..28d9def2fbd0 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -37,6 +37,7 @@
*/

struct led_netdev_data {
+ bool hw_mode_supported;
spinlock_t lock;

struct delayed_work work;
@@ -61,9 +62,52 @@ enum netdev_led_attr {

static void set_baseline_state(struct led_netdev_data *trigger_data)
{
+ bool can_offload;
int current_brightness;
+ u32 hw_blink_mode_supported;
struct led_classdev *led_cdev = trigger_data->led_cdev;

+ if (trigger_data->hw_mode_supported) {
+ if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode) &&
+ led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_LINK))
+ hw_blink_mode_supported |= TRIGGER_NETDEV_LINK;
+ if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
+ led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_TX))
+ hw_blink_mode_supported |= TRIGGER_NETDEV_TX;
+ if (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) &&
+ led_trigger_blink_mode_is_supported(led_cdev, TRIGGER_NETDEV_RX))
+ hw_blink_mode_supported |= TRIGGER_NETDEV_RX;
+
+ /* All the requested blink mode can be triggered by hardware.
+ * Put it in hardware mode.
+ */
+ if (hw_blink_mode_supported == trigger_data->mode)
+ can_offload = true;
+
+ if (can_offload) {
+ /* We are refreshing the blink modes. Reset them */
+ led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
+ BLINK_MODE_ZERO);
+
+ if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
+ led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
+ BLINK_MODE_ENABLE);
+ if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode))
+ led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_TX,
+ BLINK_MODE_ENABLE);
+ if (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
+ led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_RX,
+ BLINK_MODE_ENABLE);
+ led_cdev->hw_control_start(led_cdev);
+
+ return;
+ }
+ }
+
+ /* If LED supports only hardware mode then skip any software handling */
+ if (led_cdev->blink_mode == HARDWARE_CONTROLLED)
+ return;
+
current_brightness = led_cdev->brightness;
if (current_brightness)
led_cdev->blink_brightness = current_brightness;
@@ -407,13 +451,27 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
trigger_data->mode = 0;
atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
trigger_data->last_activity = 0;
+ if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
+ trigger_data->hw_mode_supported = true;
+
+ /* With hw mode enabled reset any rule set by default */
+ if (led_cdev->hw_control_status(led_cdev)) {
+ rc = led_cdev->hw_control_configure(led_cdev, TRIGGER_NETDEV_LINK,
+ BLINK_MODE_ZERO);
+ if (rc)
+ goto err;
+ }
+ }

led_set_trigger_data(led_cdev, trigger_data);

rc = register_netdevice_notifier(&trigger_data->notifier);
if (rc)
- kfree(trigger_data);
+ goto err;

+ return 0;
+err:
+ kfree(trigger_data);
return rc;
}

@@ -433,6 +491,7 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)

static struct led_trigger netdev_led_trigger = {
.name = "netdev",
+ .supported_blink_modes = SOFTWARE_HARDWARE,
.activate = netdev_trig_activate,
.deactivate = netdev_trig_deactivate,
.groups = netdev_trig_groups,
--
2.32.0

2021-11-09 12:31:27

[permalink] [raw]

Subject: [RFC PATCH v3 1/8] leds: add support for 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:
- trigger_offload_status(): This asks the LED driver if offload mode is
enabled or not.
Triggers will check if the offload mode is supported and will be
activated accordingly. If the trigger can't run in software mode,
return -EOPNOTSUPP as the blinking can't be simulated by software.
- trigger_offload_start(): This will simply enable the offload mode for
the LED.
With this not declared and trigger_offload_status() returning true,
it's assumed that the LED is always in offload mode.
- trigger_offload_stop(): This will simply disable the offload mode for
the LED.
With this not declared and trigger_offload_status() returning true,
it's assumed that the LED is always in offload mode.
It's advised to the driver to put the LED in the old state but this
is not enforcerd and putting the LED off is also accepted.

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

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

Cc: Marek Behún <[email protected]>
Signed-off-by: Ansuel Smith <[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 ed800f5da7d8..bd2b19cc77ec 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -49,6 +49,17 @@ config LEDS_BRIGHTNESS_HW_CHANGED

See Documentation/ABI/testing/sysfs-class-led for details.

+config LEDS_HARDWARE_CONTROL
+ bool "LED Hardware Control support"
+ help
+ This option enabled Hardware control support used by leds that
+ can be driven in hardware by using supported triggers.
+
+ Hardware blink modes will be exposed by sysfs class in
+ /sys/class/leds based on the trigger currently active.
+
+ If unsure, say Y.
+
comment "LED drivers"

config LEDS_88PM860X
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f4bb02f6e042..1df782ecac66 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -164,6 +164,27 @@ static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
}
#endif

+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+static int led_classdev_check_blink_mode_functions(struct led_classdev *led_cdev)
+{
+ int mode = led_cdev->blink_mode;
+
+ if (mode == SOFTWARE_HARDWARE_CONTROLLED &&
+ (!led_cdev->hw_control_status ||
+ !led_cdev->hw_control_start ||
+ !led_cdev->hw_control_stop))
+ return -EINVAL;
+
+ if (mode == SOFTWARE_CONTROLLED &&
+ (led_cdev->hw_control_status ||
+ led_cdev->hw_control_start ||
+ led_cdev->hw_control_stop))
+ return -EINVAL;
+
+ return 0;
+}
+#endif
+
/**
* led_classdev_suspend - suspend an led_classdev.
* @led_cdev: the led_classdev to suspend.
@@ -367,6 +388,12 @@ int led_classdev_register_ext(struct device *parent,
if (ret < 0)
return ret;

+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+ ret = led_classdev_check_blink_mode_functions(led_cdev);
+ if (ret < 0)
+ return ret;
+#endif
+
mutex_init(&led_cdev->led_access);
mutex_lock(&led_cdev->led_access);
led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 072491d3e17b..693c5d0fa980 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -154,6 +154,27 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
}
EXPORT_SYMBOL_GPL(led_trigger_read);

+static bool led_trigger_is_supported(struct led_classdev *led_cdev,
+ struct led_trigger *trigger)
+{
+ switch (led_cdev->blink_mode) {
+ case SOFTWARE_CONTROLLED:
+ if (trigger->supported_blink_modes == HARDWARE_ONLY)
+ return 0;
+ break;
+ case HARDWARE_CONTROLLED:
+ if (trigger->supported_blink_modes == SOFTWARE_ONLY)
+ return 0;
+ break;
+ case SOFTWARE_HARDWARE_CONTROLLED:
+ break;
+ default:
+ return 0;
+ }
+
+ return 1;
+}
+
/* Caller must ensure led_cdev->trigger_lock held */
int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
{
@@ -179,6 +200,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)

cancel_work_sync(&led_cdev->set_brightness_work);
led_stop_software_blink(led_cdev);
+ /* Disable hardware mode on trigger change if supported */
+ if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
+ led_cdev->hw_control_status(led_cdev))
+ led_cdev->hw_control_stop(led_cdev);
if (led_cdev->trigger->deactivate)
led_cdev->trigger->deactivate(led_cdev);
device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
@@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
led_set_brightness(led_cdev, LED_OFF);
}
if (trig) {
+ /* Make sure the trigger support the LED blink mode */
+ if (!led_trigger_is_supported(led_cdev, trig))
+ return -EINVAL;
+
spin_lock(&trig->leddev_list_lock);
list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
spin_unlock(&trig->leddev_list_lock);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..cf0c6005c297 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -67,6 +67,12 @@ struct led_hw_trigger_type {
int dummy;
};

+enum led_blink_modes {
+ SOFTWARE_CONTROLLED = 0x0,
+ HARDWARE_CONTROLLED,
+ SOFTWARE_HARDWARE_CONTROLLED,
+};
+
struct led_classdev {
const char *name;
unsigned int brightness;
@@ -154,6 +160,32 @@ struct led_classdev {

/* LEDs that have private triggers have this set */
struct led_hw_trigger_type *trigger_type;
+
+ /* This report the supported blink_mode. The driver should report the
+ * correct LED capabilities.
+ * With this set to HARDWARE_CONTROLLED, LED is always in offload mode
+ * and triggers can't be simulated by software.
+ * If the led is HARDWARE_CONTROLLED, status/start/stop function
+ * are optional.
+ * By default SOFTWARE_CONTROLLED is set as blink_mode.
+ */
+ enum led_blink_modes blink_mode;
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+ /* Ask the LED driver if hardware mode is enabled or not.
+ * If the option is not declared by the LED driver, SOFTWARE_CONTROLLED
+ * is assumed.
+ * Triggers will check if the hardware mode is supported and will be
+ * activated accordingly. If the trigger can't run in hardware mode,
+ * return -EOPNOTSUPP as the blinking can't be simulated by software.
+ */
+ bool (*hw_control_status)(struct led_classdev *led_cdev);
+ /* Set LED in hardware mode */
+ int (*hw_control_start)(struct led_classdev *led_cdev);
+ /* Disable hardware mode for LED. It's advised to the LED driver to put it to
+ * the old status but that is not mandatory and also putting it off is accepted.
+ */
+ int (*hw_control_stop)(struct led_classdev *led_cdev);
+#endif
#endif

#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -215,7 +247,6 @@ extern struct led_classdev *of_led_get(struct device_node *np, int index);
extern void led_put(struct led_classdev *led_cdev);
struct led_classdev *__must_check devm_of_led_get(struct device *dev,
int index);
-
/**
* led_blink_set - set blinking with software fallback
* @led_cdev: the LED to start blinking
@@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)

#define TRIG_NAME_MAX 50

+enum led_trigger_blink_supported_modes {
+ SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
+ HARDWARE_ONLY = HARDWARE_CONTROLLED,
+ SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
+};
+
struct led_trigger {
/* Trigger Properties */
const char *name;
int (*activate)(struct led_classdev *led_cdev);
void (*deactivate)(struct led_classdev *led_cdev);

+ /* Declare if the Trigger supports hardware control to
+ * offload triggers or supports only software control.
+ * A trigger can also declare support for hardware control
+ * if is task is only configure LED blink modes and expose
+ * them in sysfs.
+ */
+ enum led_trigger_blink_supported_modes supported_blink_modes;
+
/* LED-private triggers have this set */
struct led_hw_trigger_type *trigger_type;

--
2.32.0

2021-11-09 12:40:00

[permalink] [raw]

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

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

Signed-off-by: Ansuel Smith <[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 cf0c6005c297..00bc4d6ed7ca 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -73,6 +73,16 @@ enum led_blink_modes {
SOFTWARE_HARDWARE_CONTROLLED,
};

+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+enum blink_mode_cmd {
+ BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
+ BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
+ BLINK_MODE_READ, /* Read the status of the hardware blink mode */
+ BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
+ BLINK_MODE_ZERO, /* Disable any hardware blink active */
+};
+#endif
+
struct led_classdev {
const char *name;
unsigned int brightness;
@@ -185,6 +195,17 @@ struct led_classdev {
* the old status but that is not mandatory and also putting it off is accepted.
*/
int (*hw_control_stop)(struct led_classdev *led_cdev);
+ /* This will be used to configure the various blink modes LED support in hardware
+ * mode.
+ * The LED driver require to support the active trigger and will elaborate the
+ * unsigned long flag and do the operation based on the provided cmd.
+ * Current operation are enable,disable,supported and status.
+ * A trigger will use this to enable or disable the asked blink mode, check the
+ * status of the blink mode or ask if the blink mode can run in hardware mode.
+ */
+ int (*hw_control_configure)(struct led_classdev *led_cdev,
+ unsigned long flag,
+ enum blink_mode_cmd cmd);
#endif
#endif

@@ -454,6 +475,24 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
return led_cdev->trigger_data;
}

+#ifdef CONFIG_LEDS_HARDWARE_CONTROL
+static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev,
+ unsigned long flag)
+{
+ int ret;
+
+ /* Sanity check: make sure led support hw mode */
+ if (led_cdev->blink_mode == SOFTWARE_CONTROLLED)
+ return false;
+
+ ret = led_cdev->hw_control_configure(led_cdev, flag, BLINK_MODE_SUPPORTED);
+ if (ret > 0)
+ return true;
+
+ return false;
+}
+#endif
+
/**
* led_trigger_rename_static - rename a trigger
* @name: the new trigger name
--
2.32.0

2021-11-09 12:46:30

[permalink] [raw]

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

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

Signed-off-by: Ansuel Smith <[email protected]>
---
.../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index 48de0ace265d..106d95adc1e8 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -64,6 +64,8 @@ properties:
internal mdio access is used.
With the legacy mapping the reg corresponding to the internal
mdio is the switch reg with an offset of -1.
+ Each phy have at least 3 LEDs connected and can be declared
+ using the standard LEDs structure.

properties:
'#address-cells':
@@ -340,6 +342,24 @@ examples:

internal_phy_port1: ethernet-phy@0 {
reg = <0>;
+
+ leds {
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "offload-phy-activity";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "offload-phy-activity";
+ };
+ };
};

internal_phy_port2: ethernet-phy@1 {
--
2.32.0

2021-11-09 12:52:37

by Marek Behún

[permalink] [raw]

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

Hello Ansuel,

On Tue, 9 Nov 2021 03:26:02 +0100
Ansuel Smith <[email protected]> wrote:

> Add hw_control_configure helper to configure how the LED should work in
> hardware mode. The function require to support the particular trigger and
> will use the passed flag to elaborate the data and apply the
> correct configuration. This function will then be used by the trigger to
> request and update hardware configuration.
>
> Signed-off-by: Ansuel Smith <[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 cf0c6005c297..00bc4d6ed7ca 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -73,6 +73,16 @@ enum led_blink_modes {
> SOFTWARE_HARDWARE_CONTROLLED,
> };
>
> +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> +enum blink_mode_cmd {
> + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> + BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> + BLINK_MODE_ZERO, /* Disable any hardware blink active */
> +};
> +#endif

this is a strange proposal for the API.

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

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

Marek

2021-11-09 12:53:27

by Marek Behún

[permalink] [raw]

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

On Tue, 9 Nov 2021 03:26:06 +0100
Ansuel Smith <[email protected]> wrote:

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

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

Marek

2021-11-09 13:00:11

[permalink] [raw]

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

On 11/8/21 6:26 PM, Ansuel Smith wrote:
> diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
> index cd155ead8703..645940b78d81 100644
> --- a/Documentation/leds/leds-class.rst
> +++ b/Documentation/leds/leds-class.rst
> @@ -169,6 +169,34 @@ Setting the brightness to zero with brightness_set() callback function
> should completely turn off the LED and cancel the previously programmed
> hardware blinking function, if any.
>
> +Hardware driven LEDs
> +===================================
> +
> +Some LEDs can be driven by hardware (for example a LED connected to
> +an ethernet PHY or an ethernet switch can be configured to blink on activity on
> +the network, which in software is done by the netdev trigger).
> +
> +To do such offloading, LED driver must support this and a supported trigger must
> +be used.
> +
> +LED driver should declare the correct blink_mode supported and should set the
> +blink_mode parameter to one of HARDWARE_CONTROLLED or SOFTWARE_HARDWARE_CONTROLLED.
> +The trigger will check this option and fail to activate if the blink_mode is not
> +supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTROLLED
> +is assumed.
> +
> +The LED must implement 3 main API:

APIs:

> +- hw_control_status(): This asks the LED driver if hardware mode is enabled
> + or not. Triggers will check if the hardware mode is active and will try
> + to offload their triggers if supported by the driver.
> +- hw_control_start(): This will simply enable the hardware mode for the LED.
> +- hw_control_stop(): This will simply disable the hardware mode for the LED.
> + It's advised to the driver to put the LED in the old state but this is not

for the driver

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

enforced

> +
> +With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
> +and any software only trigger will reject activation as the LED supports only
> +hardware mode.
>
> Known Issues
> ============
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ed800f5da7d8..bd2b19cc77ec 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -49,6 +49,17 @@ config LEDS_BRIGHTNESS_HW_CHANGED
>
> See Documentation/ABI/testing/sysfs-class-led for details.
>
> +config LEDS_HARDWARE_CONTROL
> + bool "LED Hardware Control support"
> + help
> + This option enabled Hardware control support used by leds that

enables LEDs

> + can be driven in hardware by using supported triggers.
> +
> + Hardware blink modes will be exposed by sysfs class in
> + /sys/class/leds based on the trigger currently active.
> +
> + If unsure, say Y.

--
~Randy

2021-11-09 14:13:11

[permalink] [raw]

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

On 11/8/21 6:26 PM, Ansuel Smith wrote:
> diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
> index 645940b78d81..efd2f68c46a7 100644
> --- a/Documentation/leds/leds-class.rst
> +++ b/Documentation/leds/leds-class.rst
> @@ -198,6 +198,31 @@ With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
> and any software only trigger will reject activation as the LED supports only
> hardware mode.
>
> +A trigger once he declared support for hardware controlled blinks, will use the function

Maybe this:
Once a trigger has declared support for hardware-controller blinks, it will us the function

> +hw_control_configure() provided by the driver to check support for a particular blink mode.
> +This function passes as the first argument (flag) a u32 flag.
> +The second argument (cmd) of hw_control_configure() method can be used to do various
> +operations for the specific blink mode. We currently support ENABLE, DISABLE, READ, ZERO
> +and SUPPORTED to enable, disable, read the state of the blink mode, ask the LED
> +driver if it does supports the specific blink mode and to reset any blink mode active.
> +
> +In ENABLE/DISABLE hw_control_configure() should configure the LED to enable/disable the
> +requested blink mode (flag).
> +In READ hw_control_configure() should return 0 or 1 based on the status of the requested
> +blink mode (flag).
> +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the
> +requested blink mode (flags) or not.
> +In ZERO hw_control_configure() should return 0 with success operation or error.
> +
> +The unsigned long flag is specific to the trigger and change across them. It's in the LED

changes

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

driver's interest to know how to

> +particular trigger. For this exact reason explicit support for the specific
> +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
> +with a not supported trigger.
> +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will
> +fail as the driver doesn't support that specific offload trigger or doesn't know
> +how to handle the provided flags.

--
~Randy

2021-11-09 14:33:12

[permalink] [raw]

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

On 11/8/21 6:26 PM, Ansuel Smith wrote:
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 7b1457a6e327..89e36f3a8195 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -67,6 +67,15 @@ config NET_DSA_QCA8K
> This enables support for the Qualcomm Atheros QCA8K Ethernet
> switch chips.
>
> +config NET_DSA_QCA8K_LEDS_SUPPORT
> + tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
> + select NET_DSA_QCA8K
> + select LEDS_OFFLOAD_TRIGGERS
> + help
> + Thsi enabled support for LEDs present on the Qualcomm Atheros

This enables

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

requires

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

--
~Randy

2021-11-09 14:33:21

[permalink] [raw]

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

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

or be "on" based on the PHY

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

activity

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

support Hardware mode.

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

thanks.
--
~Randy

2021-11-09 23:55:24

[permalink] [raw]

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

On Tue, Nov 09, 2021 at 04:01:03AM +0100, Marek Beh?n wrote:
> Hello Ansuel,
>
> On Tue, 9 Nov 2021 03:26:02 +0100
> Ansuel Smith <[email protected]> wrote:
>
> > Add hw_control_configure helper to configure how the LED should work in
> > hardware mode. The function require to support the particular trigger and
> > will use the passed flag to elaborate the data and apply the
> > correct configuration. This function will then be used by the trigger to
> > request and update hardware configuration.
> >
> > Signed-off-by: Ansuel Smith <[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 cf0c6005c297..00bc4d6ed7ca 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -73,6 +73,16 @@ enum led_blink_modes {
> > SOFTWARE_HARDWARE_CONTROLLED,
> > };
> >
> > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > +enum blink_mode_cmd {
> > + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> > + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> > + BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> > + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> > + BLINK_MODE_ZERO, /* Disable any hardware blink active */
> > +};
> > +#endif
>
> this is a strange proposal for the API.
>
> Anyway, led_classdev already has the blink_set() method, which is documented as
> /*
> * Activate hardware accelerated blink, delays are in milliseconds
> * and if both are zero then a sensible default should be chosen.
> * The call should adjust the timings in that case and if it can't
> * match the values specified exactly.
> * Deactivate blinking again when the brightness is set to LED_OFF
> * via the brightness_set() callback.
> */
> int (*blink_set)(struct led_classdev *led_cdev,
> unsigned long *delay_on,
> unsigned long *delay_off);
>
> So we already have a method to set hardware blkinking, we don't need
> another one.
>
> Marek

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

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

--
Ansuel

2021-11-09 23:58:52

[permalink] [raw]

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

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

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

> thanks.
> --
> ~Randy

--
Ansuel

2021-11-10 00:18:08

[permalink] [raw]

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

On Tue, Nov 09, 2021 at 03:26:01AM +0100, Ansuel Smith wrote:
> Some LEDs can be driven by hardware (for example a LED connected to
> an ethernet PHY or an ethernet switch can be configured to blink on
> activity on the network, which in software is done by the netdev trigger).
>
> To do such offloading, LED driver must support this and a supported
> trigger must be used.
>
> LED driver should declare the correct blink_mode supported and should set
> the blink_mode parameter to one of HARDWARE_CONTROLLED or
> SOFTWARE_HARDWARE_CONTROLLED.
> The trigger will check this option and fail to activate if the blink_mode
> is not supported. By default if a LED driver doesn't declare blink_mode,
> SOFTWARE_CONTROLLED is assumed.
>
> The LED must implement 3 main API:
> - trigger_offload_status(): This asks the LED driver if offload mode is
> enabled or not.
> Triggers will check if the offload mode is supported and will be
> activated accordingly. If the trigger can't run in software mode,
> return -EOPNOTSUPP as the blinking can't be simulated by software.

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

Andrew

2021-11-10 00:19:12

[permalink] [raw]

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

On Tue, Nov 09, 2021 at 09:34:23PM +0100, Andrew Lunn wrote:
> On Tue, Nov 09, 2021 at 03:26:01AM +0100, Ansuel Smith wrote:
> > Some LEDs can be driven by hardware (for example a LED connected to
> > an ethernet PHY or an ethernet switch can be configured to blink on
> > activity on the network, which in software is done by the netdev trigger).
> >
> > To do such offloading, LED driver must support this and a supported
> > trigger must be used.
> >
> > LED driver should declare the correct blink_mode supported and should set
> > the blink_mode parameter to one of HARDWARE_CONTROLLED or
> > SOFTWARE_HARDWARE_CONTROLLED.
> > The trigger will check this option and fail to activate if the blink_mode
> > is not supported. By default if a LED driver doesn't declare blink_mode,
> > SOFTWARE_CONTROLLED is assumed.
> >
> > The LED must implement 3 main API:
> > - trigger_offload_status(): This asks the LED driver if offload mode is
> > enabled or not.
> > Triggers will check if the offload mode is supported and will be
> > activated accordingly. If the trigger can't run in software mode,
> > return -EOPNOTSUPP as the blinking can't be simulated by software.
>
> I don't understand this last part. The LED controller is not
> implementing software mode, other than providing a method to manually
> turn the LED on and off. And there is a well defined call for that. If
> that call is a NULL, it is clear it is not implemented. There is no
> need to ask the driver.
>
> Andrew

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

--
Ansuel

2021-11-10 00:19:12

[permalink] [raw]

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

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

Humm. I can see merits for both.

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

Andrew

2021-11-10 00:19:50

[permalink] [raw]

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

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

symbolic. Randy is slipping :-)

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

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

> +++ b/include/linux/leds.h
> @@ -548,6 +548,13 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
>
> #endif /* CONFIG_LEDS_TRIGGERS */
>
> +/* Trigger specific enum */

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

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

2021-11-10 00:21:00

[permalink] [raw]

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

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

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

Andrew

2021-11-10 00:21:23

[permalink] [raw]

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

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

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

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

What does keep mean in this context?

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

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

Andrew

2021-11-10 00:21:59

[permalink] [raw]

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

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

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

Andrew

2021-11-10 00:22:15

[permalink] [raw]

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

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

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

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

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

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

Andrew

2021-11-10 00:32:05

by Marek Behún

[permalink] [raw]

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

On Tue, 9 Nov 2021 15:22:53 +0100
Ansuel Smith <[email protected]> wrote:

> On Tue, Nov 09, 2021 at 04:01:03AM +0100, Marek Behún wrote:
> > Hello Ansuel,
> >
> > On Tue, 9 Nov 2021 03:26:02 +0100
> > Ansuel Smith <[email protected]> wrote:
> >
> > > Add hw_control_configure helper to configure how the LED should work in
> > > hardware mode. The function require to support the particular trigger and
> > > will use the passed flag to elaborate the data and apply the
> > > correct configuration. This function will then be used by the trigger to
> > > request and update hardware configuration.
> > >
> > > Signed-off-by: Ansuel Smith <[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 cf0c6005c297..00bc4d6ed7ca 100644
> > > --- a/include/linux/leds.h
> > > +++ b/include/linux/leds.h
> > > @@ -73,6 +73,16 @@ enum led_blink_modes {
> > > SOFTWARE_HARDWARE_CONTROLLED,
> > > };
> > >
> > > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > > +enum blink_mode_cmd {
> > > + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> > > + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> > > + BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> > > + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> > > + BLINK_MODE_ZERO, /* Disable any hardware blink active */
> > > +};
> > > +#endif
> >
> > this is a strange proposal for the API.
> >
> > Anyway, led_classdev already has the blink_set() method, which is documented as
> > /*
> > * Activate hardware accelerated blink, delays are in milliseconds
> > * and if both are zero then a sensible default should be chosen.
> > * The call should adjust the timings in that case and if it can't
> > * match the values specified exactly.
> > * Deactivate blinking again when the brightness is set to LED_OFF
> > * via the brightness_set() callback.
> > */
> > int (*blink_set)(struct led_classdev *led_cdev,
> > unsigned long *delay_on,
> > unsigned long *delay_off);
> >
> > So we already have a method to set hardware blkinking, we don't need
> > another one.
> >
> > Marek
>
> But that is about hardware blink, not a LED controlled by hardware based
> on some rules/modes.
> Doesn't really match the use for the hardware control.

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

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

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

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

2021-11-10 19:51:21

[permalink] [raw]

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

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

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

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

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

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

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

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

Is that acceptable?

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

--
Ansuel

2021-11-10 19:57:32

[permalink] [raw]

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

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

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

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

> > +++ b/include/linux/leds.h
> > @@ -548,6 +548,13 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
> >
> > #endif /* CONFIG_LEDS_TRIGGERS */
> >
> > +/* Trigger specific enum */
>
> You probably want netdev in the comment above. Things could get
> interesting if other ledtrig-*.c started using them.
>
> > +enum led_trigger_netdev_modes {
> > + TRIGGER_NETDEV_LINK,
> > + TRIGGER_NETDEV_TX,
> > + TRIGGER_NETDEV_RX,
> > +};
> > +
> > /* Trigger specific functions */
> > #ifdef CONFIG_LEDS_TRIGGER_DISK
> > void ledtrig_disk_activity(bool write);
> > --
> > 2.32.0
> >

--
Ansuel

2021-11-10 20:04:43

[permalink] [raw]

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

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

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

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

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

--
Ansuel

2021-11-10 22:25:10

[permalink] [raw]

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

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

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

Andrew

2021-11-10 22:29:28

[permalink] [raw]

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

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

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

Andrew

2021-11-10 22:32:28