This is a continue of [1]. It was decided to take a more gradual
approach to implement LEDs support for switch and phy starting with
basic support and then implementing the hw control part when we have all
the prereq done.
This series implements only the brightness_set() and blink_set() ops.
An example of switch implementation is done with qca8k.
For PHY a more generic approach is used with implementing the LED
support in PHY core and with the user (in this case marvell) adding all
the required functions.
Currently we set the default-state as "keep" to not change the default
configuration of the declared LEDs since almost every switch have a
default configuration.
[1] https://lore.kernel.org/lkml/[email protected]/
Changes in new series v6:
- Add leds-ethernet.yaml to document reg in led node
- Update ethernet-controller and ethernet-phy to follow new leds-ethernet schema
- Fix comments in qca8k-leds.c (at least -> at most)
(wrong GENMASK for led phy 0 and 4)
- Add review and ack tag from Pavel Machek
- Changes in Andrew patch:
- leds: Provide stubs for when CLASS_LED & NEW_LEDS are disabled
- Change LED_CLASS to NEW_LEDS for led_init_default_state_get()
- net: phy: Add a binding for PHY LEDs
- Add dependency on LED_CLASS
- Drop review tag from Michal Kubiak (patch modified)
Changes in new series v5:
- Rebase everything on top of net-next/main
- Add more info on LED probe fail for qca8k
- Drop some additional raw number and move to define in qca8k header
- Add additional info on LED mapping on qca8k regs
- Checks port number in qca8k switch port parse
- Changes in Andrew patch:
- Add additional patch for stubs when CLASS_LED disabled
- Drop CLASS_LED dependency for PHYLIB (to fix kbot errors reported)
Changes in new series v4:
- Changes in Andrew patch:
- net: phy: Add a binding for PHY LEDs:
- Rename phy_led: led_list to list
- Rename phy_device: led_list to leds
- Remove phy_leds_remove() since devm_ should do what is needed
- Fixup documentation for struct phy_led
- Fail probe on LED errors
- net: phy: phy_device: Call into the PHY driver to set LED brightness
- Moved phy_led::phydev from previous patch to here since it is first
used here.
- net: phy: marvell: Implement led_blink_set()
- Use int instead of unsigned
- net: phy: marvell: Add software control of the LEDs
- Use int instead of unsigned
- Add depends on LED_CLASS for qca8k Kconfig
- Fix Makefile for qca8k as suggested
- Move qca8k_setup_led_ctrl to separate header
- Move Documentation from dsa-port to ethernet-controller
- Drop trailing . from Andrew patch fro consistency
Changes in new series v3:
- Move QCA8K_LEDS Kconfig option from tristate to bool
- Use new helper led_init_default_state_get for default-state in qca8k
- Drop cled_qca8k_brightness_get() as there isn't a good way to describe
the mode the led is currently in
- Rework qca8k_led_brightness_get() to return true only when LED is set
to always ON
Changes in new series v2:
- Add LEDs node for rb3011
- Fix rb3011 switch node unevaluated properties while running
make dtbs_check
- Fix a copypaste error in qca8k-leds.c for port 4 required shift
- Drop phy-handle usage for qca8k and use qca8k_port_to_phy()
- Add review tag from Andrew
- Add Christian Marangi SOB in each Andrew patch
- Add extra description for dsa-port stressing that PHY have no access
and LED are controlled by the related MAC
- Add missing additionalProperties for dsa-port.yaml and ethernet-phy.yaml
Changes from the old v8 series:
- Drop linux,default-trigger set to netdev.
- Dropped every hw control related patch and implement only
blink_set and brightness_set
- Add default-state to "keep" for each LED node example
Andrew Lunn (7):
leds: Provide stubs for when CLASS_LED & NEW_LEDS are disabled
net: phy: Add a binding for PHY LEDs
net: phy: phy_device: Call into the PHY driver to set LED brightness
net: phy: marvell: Add software control of the LEDs
net: phy: phy_device: Call into the PHY driver to set LED blinking
net: phy: marvell: Implement led_blink_set()
arm: mvebu: dt: Add PHY LED support for 370-rd WAN port
Christian Marangi (9):
net: dsa: qca8k: move qca8k_port_to_phy() to header
net: dsa: qca8k: add LEDs basic support
net: dsa: qca8k: add LEDs blink_set() support
dt-bindings: leds: Document support for generic ethernet LEDs
dt-bindings: net: ethernet-controller: Document support for LEDs node
dt-bindings: net: dsa: qca8k: add LEDs definition example
ARM: dts: qcom: ipq8064-rb3011: Drop unevaluated properties in switch
nodes
ARM: dts: qcom: ipq8064-rb3011: Add Switch LED for each port
dt-bindings: net: phy: Document support for LEDs node
.../bindings/leds/leds-ethernet.yaml | 76 +++++
.../devicetree/bindings/net/dsa/qca8k.yaml | 24 ++
.../bindings/net/ethernet-controller.yaml | 10 +
.../devicetree/bindings/net/ethernet-phy.yaml | 19 ++
arch/arm/boot/dts/armada-370-rd.dts | 14 +
arch/arm/boot/dts/qcom-ipq8064-rb3011.dts | 124 +++++++-
drivers/net/dsa/qca/Kconfig | 8 +
drivers/net/dsa/qca/Makefile | 3 +
drivers/net/dsa/qca/qca8k-8xxx.c | 20 +-
drivers/net/dsa/qca/qca8k-leds.c | 270 ++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 74 +++++
drivers/net/dsa/qca/qca8k_leds.h | 16 ++
drivers/net/phy/Kconfig | 1 +
drivers/net/phy/marvell.c | 81 +++++-
drivers/net/phy/phy_device.c | 102 +++++++
include/linux/leds.h | 18 ++
include/linux/phy.h | 35 +++
17 files changed, 871 insertions(+), 24 deletions(-)
create mode 100644 Documentation/devicetree/bindings/leds/leds-ethernet.yaml
create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
create mode 100644 drivers/net/dsa/qca/qca8k_leds.h
--
2.39.2
Add LEDs basic support for qca8k Switch Family by adding basic
brightness_set() support.
Since these LEDs refelect port status, the default label is set to
":port". DT binding should describe the color, function and number of
the leds using standard LEDs api.
These LEDs supports only blocking variant of the brightness_set()
function since they can sleep during access of the switch leds to set
the brightness.
While at it add to the qca8k header file each mode defined by the Switch
Documentation for future use.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/dsa/qca/Kconfig | 8 ++
drivers/net/dsa/qca/Makefile | 3 +
drivers/net/dsa/qca/qca8k-8xxx.c | 5 +
drivers/net/dsa/qca/qca8k-leds.c | 232 +++++++++++++++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 60 ++++++++
drivers/net/dsa/qca/qca8k_leds.h | 16 +++
6 files changed, 324 insertions(+)
create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
create mode 100644 drivers/net/dsa/qca/qca8k_leds.h
diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
index ba339747362c..7a86d6d6a246 100644
--- a/drivers/net/dsa/qca/Kconfig
+++ b/drivers/net/dsa/qca/Kconfig
@@ -15,3 +15,11 @@ config NET_DSA_QCA8K
help
This enables support for the Qualcomm Atheros QCA8K Ethernet
switch chips.
+
+config NET_DSA_QCA8K_LEDS_SUPPORT
+ bool "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
+ depends on NET_DSA_QCA8K
+ depends on LEDS_CLASS
+ help
+ This enabled support for LEDs present on the Qualcomm Atheros
+ QCA8K Ethernet switch chips.
diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
index 701f1d199e93..ce66b1984e5f 100644
--- a/drivers/net/dsa/qca/Makefile
+++ b/drivers/net/dsa/qca/Makefile
@@ -2,3 +2,6 @@
obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
qca8k-y += qca8k-common.o qca8k-8xxx.o
+ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+qca8k-y += qca8k-leds.o
+endif
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 459ea687444a..76bffd6d8e23 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -22,6 +22,7 @@
#include <linux/dsa/tag_qca.h>
#include "qca8k.h"
+#include "qca8k_leds.h"
static void
qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
@@ -1783,6 +1784,10 @@ qca8k_setup(struct dsa_switch *ds)
if (ret)
return ret;
+ ret = qca8k_setup_led_ctrl(priv);
+ if (ret)
+ return ret;
+
qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
new file mode 100644
index 000000000000..3ad5e54fcdfd
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/regmap.h>
+#include <net/dsa.h>
+
+#include "qca8k.h"
+#include "qca8k_leds.h"
+
+static int
+qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+ switch (port_num) {
+ case 0:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
+ break;
+ case 1:
+ case 2:
+ case 3:
+ /* Port 123 are controlled on a different reg */
+ reg_info->reg = QCA8K_LED_CTRL3_REG;
+ reg_info->shift = QCA8K_LED_PHY123_PATTERN_EN_SHIFT(port_num, led_num);
+ break;
+ case 4:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = QCA8K_LED_PHY4_CONTROL_RULE_SHIFT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+qca8k_led_brightness_set(struct qca8k_led *led,
+ enum led_brightness brightness)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 mask, val;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ val = QCA8K_LED_ALWAYS_OFF;
+ if (brightness)
+ val = QCA8K_LED_ALWAYS_ON;
+
+ /* HW regs to control brightness is special and port 1-2-3
+ * are placed in a different reg.
+ *
+ * To control port 0 brightness:
+ * - the 2 bit (15, 14) of:
+ * - QCA8K_LED_CTRL0_REG for led1
+ * - QCA8K_LED_CTRL1_REG for led2
+ * - QCA8K_LED_CTRL2_REG for led3
+ *
+ * To control port 4:
+ * - the 2 bit (31, 30) of:
+ * - QCA8K_LED_CTRL0_REG for led1
+ * - QCA8K_LED_CTRL1_REG for led2
+ * - QCA8K_LED_CTRL2_REG for led3
+ *
+ * To control port 1:
+ * - the 2 bit at (9, 8) of QCA8K_LED_CTRL3_REG are used for led1
+ * - the 2 bit at (11, 10) of QCA8K_LED_CTRL3_REG are used for led2
+ * - the 2 bit at (13, 12) of QCA8K_LED_CTRL3_REG are used for led3
+ *
+ * To control port 2:
+ * - the 2 bit at (15, 14) of QCA8K_LED_CTRL3_REG are used for led1
+ * - the 2 bit at (17, 16) of QCA8K_LED_CTRL3_REG are used for led2
+ * - the 2 bit at (19, 18) of QCA8K_LED_CTRL3_REG are used for led3
+ *
+ * To control port 3:
+ * - the 2 bit at (21, 20) of QCA8K_LED_CTRL3_REG are used for led1
+ * - the 2 bit at (23, 22) of QCA8K_LED_CTRL3_REG are used for led2
+ * - the 2 bit at (25, 24) of QCA8K_LED_CTRL3_REG are used for led3
+ *
+ * To abstract this and have less code, we use the port and led numm
+ * to calculate the shift and the correct reg due to this problem of
+ * not having a 1:1 map of LED with the regs.
+ */
+ if (led->port_num == 0 || led->port_num == 4) {
+ mask = QCA8K_LED_PATTERN_EN_MASK;
+ val <<= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ return regmap_update_bits(priv->regmap, reg_info.reg,
+ mask << reg_info.shift,
+ val << reg_info.shift);
+}
+
+static int
+qca8k_cled_brightness_set_blocking(struct led_classdev *ldev,
+ enum led_brightness brightness)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ return qca8k_led_brightness_set(led, brightness);
+}
+
+static enum led_brightness
+qca8k_led_brightness_get(struct qca8k_led *led)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val;
+ int ret;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ ret = regmap_read(priv->regmap, reg_info.reg, &val);
+ if (ret)
+ return 0;
+
+ val >>= reg_info.shift;
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ val &= QCA8K_LED_PATTERN_EN_MASK;
+ val >>= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ /* Assume brightness ON only when the LED is set to always ON */
+ return val == QCA8K_LED_ALWAYS_ON;
+}
+
+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 = { };
+ enum led_default_state state;
+ struct qca8k_led *port_led;
+ int led_num, led_index;
+ int ret;
+
+ leds = fwnode_get_named_child_node(port, "leds");
+ if (!leds) {
+ dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
+ port_num);
+ return 0;
+ }
+
+ fwnode_for_each_child_node(leds, led) {
+ /* Reg represent the led number of the port.
+ * Each port can have at most 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 %d defined for port %d",
+ led_num, port_num);
+ continue;
+ }
+
+ led_index = QCA8K_LED_PORT_INDEX(port_num, led_num);
+
+ port_led = &priv->ports_led[led_index];
+ port_led->port_num = port_num;
+ port_led->led_num = led_num;
+ port_led->priv = priv;
+
+ state = led_init_default_state_get(led);
+ switch (state) {
+ case LEDS_DEFSTATE_ON:
+ port_led->cdev.brightness = 1;
+ qca8k_led_brightness_set(port_led, 1);
+ break;
+ case LEDS_DEFSTATE_KEEP:
+ port_led->cdev.brightness =
+ qca8k_led_brightness_get(port_led);
+ break;
+ default:
+ port_led->cdev.brightness = 0;
+ qca8k_led_brightness_set(port_led, 0);
+ }
+
+ port_led->cdev.max_brightness = 1;
+ port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
+ 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 init LED %d for port %d", led_num, port_num);
+ }
+
+ return 0;
+}
+
+int
+qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ struct fwnode_handle *ports, *port;
+ int port_num;
+ int ret;
+
+ ports = device_get_named_child_node(priv->dev, "ports");
+ if (!ports) {
+ dev_info(priv->dev, "No ports node specified in device tree!");
+ return 0;
+ }
+
+ fwnode_for_each_child_node(ports, port) {
+ if (fwnode_property_read_u32(port, "reg", &port_num))
+ continue;
+
+ /* Skip checking for CPU port 0 and CPU port 6 as not supported */
+ if (port_num == 0 || port_num == 6)
+ continue;
+
+ /* Each port can have at most 3 different leds attached.
+ * Switch port starts from 0 to 6, but port 0 and 6 are CPU
+ * port. The port index needs to be decreased by one to identify
+ * the correct port for LED setup.
+ */
+ ret = qca8k_parse_port_leds(priv, port, qca8k_port_to_phy(port_num));
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index dd7deb9095d3..c5cc8a172d65 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/regmap.h>
#include <linux/gpio.h>
+#include <linux/leds.h>
#include <linux/dsa/tag_qca.h>
#define QCA8K_ETHERNET_MDIO_PRIORITY 7
@@ -85,6 +86,51 @@
#define QCA8K_MDIO_MASTER_DATA(x) FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
#define QCA8K_MDIO_MASTER_MAX_PORTS 5
#define QCA8K_MDIO_MASTER_MAX_REG 32
+
+/* LED control register */
+#define QCA8K_LED_PORT_COUNT 3
+#define QCA8K_LED_COUNT ((QCA8K_NUM_PORTS - QCA8K_NUM_CPU_PORTS) * QCA8K_LED_PORT_COUNT)
+#define QCA8K_LED_RULE_COUNT 6
+#define QCA8K_LED_RULE_MAX 11
+#define QCA8K_LED_PORT_INDEX(_phy, _led) (((_phy) * QCA8K_LED_PORT_COUNT) + (_led))
+
+#define QCA8K_LED_PHY123_PATTERN_EN_SHIFT(_phy, _led) ((((_phy) - 1) * 6) + 8 + (2 * (_led)))
+#define QCA8K_LED_PHY123_PATTERN_EN_MASK GENMASK(1, 0)
+
+#define QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT 0
+#define QCA8K_LED_PHY4_CONTROL_RULE_SHIFT 16
+
+#define QCA8K_LED_CTRL_REG(_i) (0x050 + (_i) * 4)
+#define QCA8K_LED_CTRL0_REG 0x50
+#define QCA8K_LED_CTRL1_REG 0x54
+#define QCA8K_LED_CTRL2_REG 0x58
+#define QCA8K_LED_CTRL3_REG 0x5C
+#define QCA8K_LED_CTRL_SHIFT(_i) (((_i) % 2) * 16)
+#define QCA8K_LED_CTRL_MASK GENMASK(15, 0)
+#define QCA8K_LED_RULE_MASK GENMASK(13, 0)
+#define QCA8K_LED_BLINK_FREQ_MASK GENMASK(1, 0)
+#define QCA8K_LED_BLINK_FREQ_SHITF 0
+#define QCA8K_LED_BLINK_2HZ 0
+#define QCA8K_LED_BLINK_4HZ 1
+#define QCA8K_LED_BLINK_8HZ 2
+#define QCA8K_LED_BLINK_AUTO 3
+#define QCA8K_LED_LINKUP_OVER_MASK BIT(2)
+#define QCA8K_LED_TX_BLINK_MASK BIT(4)
+#define QCA8K_LED_RX_BLINK_MASK BIT(5)
+#define QCA8K_LED_COL_BLINK_MASK BIT(7)
+#define QCA8K_LED_LINK_10M_EN_MASK BIT(8)
+#define QCA8K_LED_LINK_100M_EN_MASK BIT(9)
+#define QCA8K_LED_LINK_1000M_EN_MASK BIT(10)
+#define QCA8K_LED_POWER_ON_LIGHT_MASK BIT(11)
+#define QCA8K_LED_HALF_DUPLEX_MASK BIT(12)
+#define QCA8K_LED_FULL_DUPLEX_MASK BIT(13)
+#define QCA8K_LED_PATTERN_EN_MASK GENMASK(15, 14)
+#define QCA8K_LED_PATTERN_EN_SHIFT 14
+#define QCA8K_LED_ALWAYS_OFF 0
+#define QCA8K_LED_ALWAYS_BLINK_4HZ 1
+#define QCA8K_LED_ALWAYS_ON 2
+#define QCA8K_LED_RULE_CONTROLLED 3
+
#define QCA8K_GOL_MAC_ADDR0 0x60
#define QCA8K_GOL_MAC_ADDR1 0x64
#define QCA8K_MAX_FRAME_SIZE 0x78
@@ -382,6 +428,19 @@ struct qca8k_pcs {
int port;
};
+struct qca8k_led_pattern_en {
+ u32 reg;
+ u8 shift;
+};
+
+struct qca8k_led {
+ u8 port_num;
+ u8 led_num;
+ u16 old_rule;
+ struct qca8k_priv *priv;
+ struct led_classdev cdev;
+};
+
struct qca8k_priv {
u8 switch_id;
u8 switch_revision;
@@ -406,6 +465,7 @@ struct qca8k_priv {
struct qca8k_pcs pcs_port_0;
struct qca8k_pcs pcs_port_6;
const struct qca8k_match_data *info;
+ struct qca8k_led ports_led[QCA8K_LED_COUNT];
};
struct qca8k_mib_desc {
diff --git a/drivers/net/dsa/qca/qca8k_leds.h b/drivers/net/dsa/qca/qca8k_leds.h
new file mode 100644
index 000000000000..ab367f05b173
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k_leds.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __QCA8K_LEDS_H
+#define __QCA8K_LEDS_H
+
+/* Leds Support function */
+#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
+#else
+static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ return 0;
+}
+#endif
+
+#endif /* __QCA8K_LEDS_H */
--
2.39.2
Add LEDs blink_set() support to qca8k Switch Family.
These LEDs support hw accellerated blinking at a fixed rate
of 4Hz.
Reject any other value since not supported by the LEDs switch.
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
drivers/net/dsa/qca/qca8k-leds.c | 38 ++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
index 3ad5e54fcdfd..0c104afdeca1 100644
--- a/drivers/net/dsa/qca/qca8k-leds.c
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -127,6 +127,43 @@ qca8k_led_brightness_get(struct qca8k_led *led)
return val == QCA8K_LED_ALWAYS_ON;
}
+static int
+qca8k_cled_blink_set(struct led_classdev *ldev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+ u32 mask, val = QCA8K_LED_ALWAYS_BLINK_4HZ;
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+
+ if (*delay_on == 0 && *delay_off == 0) {
+ *delay_on = 125;
+ *delay_off = 125;
+ }
+
+ if (*delay_on != 125 || *delay_off != 125) {
+ /* The hardware only supports blinking at 4Hz. Fall back
+ * to software implementation in other cases.
+ */
+ return -EINVAL;
+ }
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ mask = QCA8K_LED_PATTERN_EN_MASK;
+ val <<= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ regmap_update_bits(priv->regmap, reg_info.reg, mask << reg_info.shift,
+ val << reg_info.shift);
+
+ return 0;
+}
+
static int
qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
{
@@ -185,6 +222,7 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
port_led->cdev.max_brightness = 1;
port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
+ port_led->cdev.blink_set = qca8k_cled_blink_set;
init_data.default_label = ":port";
init_data.devicename = "qca8k";
init_data.fwnode = led;
--
2.39.2
Move qca8k_port_to_phy() to qca8k header as it's useful for future
reference in Switch LEDs module since the same logic is applied to get
the right index of the switch port.
Make it inline as it's simple function that just decrease the port.
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Michal Kubiak <[email protected]>
---
drivers/net/dsa/qca/qca8k-8xxx.c | 15 ---------------
drivers/net/dsa/qca/qca8k.h | 14 ++++++++++++++
2 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 55df4479ea30..459ea687444a 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -772,21 +772,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
return ret;
}
-static u32
-qca8k_port_to_phy(int port)
-{
- /* From Andrew Lunn:
- * Port 0 has no internal phy.
- * Port 1 has an internal PHY at MDIO address 0.
- * Port 2 has an internal PHY at MDIO address 1.
- * ...
- * Port 5 has an internal PHY at MDIO address 4.
- * Port 6 has no internal PHY.
- */
-
- return port - 1;
-}
-
static int
qca8k_mdio_busy_wait(struct mii_bus *bus, u32 reg, u32 mask)
{
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 7996975d29d3..dd7deb9095d3 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -421,6 +421,20 @@ struct qca8k_fdb {
u8 mac[6];
};
+static inline u32 qca8k_port_to_phy(int port)
+{
+ /* From Andrew Lunn:
+ * Port 0 has no internal phy.
+ * Port 1 has an internal PHY at MDIO address 0.
+ * Port 2 has an internal PHY at MDIO address 1.
+ * ...
+ * Port 5 has an internal PHY at MDIO address 4.
+ * Port 6 has no internal PHY.
+ */
+
+ return port - 1;
+}
+
/* Common setup function */
extern const struct qca8k_mib_desc ar8327_mib[];
extern const struct regmap_access_table qca8k_readable_table;
--
2.39.2
From: Andrew Lunn <[email protected]>
Provide stubs for devm_led_classdev_register_ext() and
led_init_default_state_get() so that LED drivers embedded within other
drivers such as PHYs and Ethernet switches still build when LEDS_CLASS
or NEW_LEDS are disabled. This also helps with Kconfig dependencies,
which are somewhat hairy for phylib and mdio and only get worse when
adding a dependency on LED_CLASS.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
include/linux/leds.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index d71201a968b6..aa48e643f655 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -82,7 +82,15 @@ struct led_init_data {
bool devname_mandatory;
};
+#if IS_ENABLED(CONFIG_NEW_LEDS)
enum led_default_state led_init_default_state_get(struct fwnode_handle *fwnode);
+#else
+static inline enum led_default_state
+led_init_default_state_get(struct fwnode_handle *fwnode)
+{
+ return LEDS_DEFSTATE_OFF;
+}
+#endif
struct led_hw_trigger_type {
int dummy;
@@ -217,9 +225,19 @@ static inline int led_classdev_register(struct device *parent,
return led_classdev_register_ext(parent, led_cdev, NULL);
}
+#if IS_ENABLED(CONFIG_LEDS_CLASS)
int devm_led_classdev_register_ext(struct device *parent,
struct led_classdev *led_cdev,
struct led_init_data *init_data);
+#else
+static inline int
+devm_led_classdev_register_ext(struct device *parent,
+ struct led_classdev *led_cdev,
+ struct led_init_data *init_data)
+{
+ return 0;
+}
+#endif
static inline int devm_led_classdev_register(struct device *parent,
struct led_classdev *led_cdev)
--
2.39.2
From: Andrew Lunn <[email protected]>
Define common binding parsing for all PHY drivers with LEDs using
phylib. Parse the DT as part of the phy_probe and add LEDs to the
linux LED class infrastructure. For the moment, provide a dummy
brightness function, which will later be replaced with a call into the
PHY driver. This allows testing since the LED core might otherwise
reject an LED whose brightness cannot be set.
Add a dependency on LED_CLASS. It either needs to be built in, or not
enabled, since a modular build can result in linker errors.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/Kconfig | 1 +
drivers/net/phy/phy_device.c | 75 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 16 ++++++++
3 files changed, 92 insertions(+)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 54874555c921..ae03562efe3a 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -18,6 +18,7 @@ menuconfig PHYLIB
depends on NETDEVICES
select MDIO_DEVICE
select MDIO_DEVRES
+ depends on LEDS_CLASS || LEDS_CLASS=n
help
Ethernet controllers are usually attached to PHY
devices. This option provides infrastructure for
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c0760cbf534b..39af989947f9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -19,10 +19,12 @@
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/list.h>
#include <linux/mdio.h>
#include <linux/mii.h>
#include <linux/mm.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/netdevice.h>
#include <linux/phy.h>
#include <linux/phy_led_triggers.h>
@@ -674,6 +676,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
device_initialize(&mdiodev->dev);
dev->state = PHY_DOWN;
+ INIT_LIST_HEAD(&dev->leds);
mutex_init(&dev->lock);
INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
@@ -2988,6 +2991,73 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
return phydrv->config_intr && phydrv->handle_interrupt;
}
+/* Dummy implementation until calls into PHY driver are added */
+static int phy_led_set_brightness(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ return 0;
+}
+
+static int of_phy_led(struct phy_device *phydev,
+ struct device_node *led)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct led_init_data init_data = {};
+ struct led_classdev *cdev;
+ struct phy_led *phyled;
+ int err;
+
+ phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
+ if (!phyled)
+ return -ENOMEM;
+
+ cdev = &phyled->led_cdev;
+
+ err = of_property_read_u32(led, "reg", &phyled->index);
+ if (err)
+ return err;
+
+ cdev->brightness_set_blocking = phy_led_set_brightness;
+ cdev->max_brightness = 1;
+ init_data.devicename = dev_name(&phydev->mdio.dev);
+ init_data.fwnode = of_fwnode_handle(led);
+
+ err = devm_led_classdev_register_ext(dev, cdev, &init_data);
+ if (err)
+ return err;
+
+ list_add(&phyled->list, &phydev->leds);
+
+ return 0;
+}
+
+static int of_phy_leds(struct phy_device *phydev)
+{
+ struct device_node *node = phydev->mdio.dev.of_node;
+ struct device_node *leds, *led;
+ int err;
+
+ if (!IS_ENABLED(CONFIG_OF_MDIO))
+ return 0;
+
+ if (!node)
+ return 0;
+
+ leds = of_get_child_by_name(node, "leds");
+ if (!leds)
+ return 0;
+
+ for_each_available_child_of_node(leds, led) {
+ err = of_phy_led(phydev, led);
+ if (err) {
+ of_node_put(led);
+ return err;
+ }
+ }
+
+ return 0;
+}
+
/**
* fwnode_mdio_find_device - Given a fwnode, find the mdio_device
* @fwnode: pointer to the mdio_device's fwnode
@@ -3183,6 +3253,11 @@ static int phy_probe(struct device *dev)
/* Set the state to READY by default */
phydev->state = PHY_READY;
+ /* Get the LEDs from the device tree, and instantiate standard
+ * LEDs for them.
+ */
+ err = of_phy_leds(phydev);
+
out:
/* Re-assert the reset signal on error */
if (err)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fefd5091bc24..11fb76a1c507 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -14,6 +14,7 @@
#include <linux/compiler.h>
#include <linux/spinlock.h>
#include <linux/ethtool.h>
+#include <linux/leds.h>
#include <linux/linkmode.h>
#include <linux/netlink.h>
#include <linux/mdio.h>
@@ -600,6 +601,7 @@ struct macsec_ops;
* @phy_num_led_triggers: Number of triggers in @phy_led_triggers
* @led_link_trigger: LED trigger for link up/down
* @last_triggered: last LED trigger for link speed
+ * @leds: list of PHY LED structures
* @master_slave_set: User requested master/slave configuration
* @master_slave_get: Current master/slave advertisement
* @master_slave_state: Current master/slave configuration
@@ -699,6 +701,7 @@ struct phy_device {
struct phy_led_trigger *led_link_trigger;
#endif
+ struct list_head leds;
/*
* Interrupt number for this PHY
@@ -834,6 +837,19 @@ struct phy_plca_status {
bool pst;
};
+/**
+ * struct phy_led: An LED driven by the PHY
+ *
+ * @list: List of LEDs
+ * @led_cdev: Standard LED class structure
+ * @index: Number of the LED
+ */
+struct phy_led {
+ struct list_head list;
+ struct led_classdev led_cdev;
+ u32 index;
+};
+
/**
* struct phy_driver - Driver structure for a particular PHY type
*
--
2.39.2
From: Andrew Lunn <[email protected]>
Linux LEDs can be software controlled via the brightness file in /sys.
LED drivers need to implement a brightness_set function which the core
will call. Implement an intermediary in phy_device, which will call
into the phy driver if it implements the necessary function.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/phy_device.c | 15 ++++++++++++---
include/linux/phy.h | 11 +++++++++++
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 39af989947f9..141d63ef3897 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2991,11 +2991,18 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
return phydrv->config_intr && phydrv->handle_interrupt;
}
-/* Dummy implementation until calls into PHY driver are added */
static int phy_led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness value)
{
- return 0;
+ struct phy_led *phyled = to_phy_led(led_cdev);
+ struct phy_device *phydev = phyled->phydev;
+ int err;
+
+ mutex_lock(&phydev->lock);
+ err = phydev->drv->led_brightness_set(phydev, phyled->index, value);
+ mutex_unlock(&phydev->lock);
+
+ return err;
}
static int of_phy_led(struct phy_device *phydev,
@@ -3012,12 +3019,14 @@ static int of_phy_led(struct phy_device *phydev,
return -ENOMEM;
cdev = &phyled->led_cdev;
+ phyled->phydev = phydev;
err = of_property_read_u32(led, "reg", &phyled->index);
if (err)
return err;
- cdev->brightness_set_blocking = phy_led_set_brightness;
+ if (phydev->drv->led_brightness_set)
+ cdev->brightness_set_blocking = phy_led_set_brightness;
cdev->max_brightness = 1;
init_data.devicename = dev_name(&phydev->mdio.dev);
init_data.fwnode = of_fwnode_handle(led);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 11fb76a1c507..2a5ee66b79b0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -841,15 +841,19 @@ struct phy_plca_status {
* struct phy_led: An LED driven by the PHY
*
* @list: List of LEDs
+ * @phydev: PHY this LED is attached to
* @led_cdev: Standard LED class structure
* @index: Number of the LED
*/
struct phy_led {
struct list_head list;
+ struct phy_device *phydev;
struct led_classdev led_cdev;
u32 index;
};
+#define to_phy_led(d) container_of(d, struct phy_led, led_cdev)
+
/**
* struct phy_driver - Driver structure for a particular PHY type
*
@@ -1072,6 +1076,13 @@ struct phy_driver {
/** @get_plca_status: Return the current PLCA status info */
int (*get_plca_status)(struct phy_device *dev,
struct phy_plca_status *plca_st);
+
+ /* Set a PHY LED brightness. Index indicates which of the PHYs
+ * led should be set. Value follows the standard LED class meaning,
+ * e.g. LED_OFF, LED_HALF, LED_FULL.
+ */
+ int (*led_brightness_set)(struct phy_device *dev,
+ u32 index, enum led_brightness value);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
--
2.39.2
From: Andrew Lunn <[email protected]>
Linux LEDs can be requested to perform hardware accelerated
blinking. Pass this to the PHY driver, if it implements the op.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Michal Kubiak <[email protected]>
Reviewed-by: Pavel Machek <[email protected]>
---
drivers/net/phy/phy_device.c | 18 ++++++++++++++++++
include/linux/phy.h | 8 ++++++++
2 files changed, 26 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 141d63ef3897..79a52dc3c50a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3005,6 +3005,22 @@ static int phy_led_set_brightness(struct led_classdev *led_cdev,
return err;
}
+static int phy_led_blink_set(struct led_classdev *led_cdev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct phy_led *phyled = to_phy_led(led_cdev);
+ struct phy_device *phydev = phyled->phydev;
+ int err;
+
+ mutex_lock(&phydev->lock);
+ err = phydev->drv->led_blink_set(phydev, phyled->index,
+ delay_on, delay_off);
+ mutex_unlock(&phydev->lock);
+
+ return err;
+}
+
static int of_phy_led(struct phy_device *phydev,
struct device_node *led)
{
@@ -3027,6 +3043,8 @@ static int of_phy_led(struct phy_device *phydev,
if (phydev->drv->led_brightness_set)
cdev->brightness_set_blocking = phy_led_set_brightness;
+ if (phydev->drv->led_blink_set)
+ cdev->blink_set = phy_led_blink_set;
cdev->max_brightness = 1;
init_data.devicename = dev_name(&phydev->mdio.dev);
init_data.fwnode = of_fwnode_handle(led);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2a5ee66b79b0..cad757d55ec9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1083,6 +1083,14 @@ struct phy_driver {
*/
int (*led_brightness_set)(struct phy_device *dev,
u32 index, enum led_brightness value);
+ /* 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.
+ */
+ int (*led_blink_set)(struct phy_device *dev, u32 index,
+ unsigned long *delay_on,
+ unsigned long *delay_off);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
--
2.39.2
From: Andrew Lunn <[email protected]>
Add a brightness function, so the LEDs can be controlled from
software using the standard Linux LED infrastructure.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
drivers/net/phy/marvell.c | 45 ++++++++++++++++++++++++++++++++++-----
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 63a3644d86c9..5f4caca93983 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -144,11 +144,13 @@
/* WOL Event Interrupt Enable */
#define MII_88E1318S_PHY_CSIER_WOL_EIE BIT(7)
-/* LED Timer Control Register */
-#define MII_88E1318S_PHY_LED_TCR 0x12
-#define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
-#define MII_88E1318S_PHY_LED_TCR_INTn_ENABLE BIT(7)
-#define MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW BIT(11)
+#define MII_88E1318S_PHY_LED_FUNC 0x10
+#define MII_88E1318S_PHY_LED_FUNC_OFF (0x8)
+#define MII_88E1318S_PHY_LED_FUNC_ON (0x9)
+#define MII_88E1318S_PHY_LED_TCR 0x12
+#define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
+#define MII_88E1318S_PHY_LED_TCR_INTn_ENABLE BIT(7)
+#define MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW BIT(11)
/* Magic Packet MAC address registers */
#define MII_88E1318S_PHY_MAGIC_PACKET_WORD2 0x17
@@ -2832,6 +2834,34 @@ static int marvell_hwmon_probe(struct phy_device *phydev)
}
#endif
+static int m88e1318_led_brightness_set(struct phy_device *phydev,
+ u32 index, enum led_brightness value)
+{
+ int reg;
+
+ reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1318S_PHY_LED_FUNC);
+ if (reg < 0)
+ return reg;
+
+ switch (index) {
+ case 0:
+ case 1:
+ case 2:
+ reg &= ~(0xf << (4 * index));
+ if (value == LED_OFF)
+ reg |= MII_88E1318S_PHY_LED_FUNC_OFF << (4 * index);
+ else
+ reg |= MII_88E1318S_PHY_LED_FUNC_ON << (4 * index);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return phy_write_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1318S_PHY_LED_FUNC, reg);
+}
+
static int marvell_probe(struct phy_device *phydev)
{
struct marvell_priv *priv;
@@ -3081,6 +3111,7 @@ static struct phy_driver marvell_drivers[] = {
.get_sset_count = marvell_get_sset_count,
.get_strings = marvell_get_strings,
.get_stats = marvell_get_stats,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1145,
@@ -3187,6 +3218,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_start = marvell_vct7_cable_test_start,
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1540,
@@ -3213,6 +3245,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_start = marvell_vct7_cable_test_start,
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1545,
@@ -3239,6 +3272,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_start = marvell_vct7_cable_test_start,
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
{
.phy_id = MARVELL_PHY_ID_88E3016,
@@ -3380,6 +3414,7 @@ static struct phy_driver marvell_drivers[] = {
.get_stats = marvell_get_stats,
.get_tunable = m88e1540_get_tunable,
.set_tunable = m88e1540_set_tunable,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
};
--
2.39.2
From: Andrew Lunn <[email protected]>
The Marvell PHY can blink the LEDs, simple on/off. All LEDs blink at
the same rate, and the reset default is 84ms per blink, which is
around 12Hz.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Pavel Machek <[email protected]>
---
drivers/net/phy/marvell.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 5f4caca93983..c00f67f229f5 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -147,6 +147,8 @@
#define MII_88E1318S_PHY_LED_FUNC 0x10
#define MII_88E1318S_PHY_LED_FUNC_OFF (0x8)
#define MII_88E1318S_PHY_LED_FUNC_ON (0x9)
+#define MII_88E1318S_PHY_LED_FUNC_HI_Z (0xa)
+#define MII_88E1318S_PHY_LED_FUNC_BLINK (0xb)
#define MII_88E1318S_PHY_LED_TCR 0x12
#define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
#define MII_88E1318S_PHY_LED_TCR_INTn_ENABLE BIT(7)
@@ -2862,6 +2864,35 @@ static int m88e1318_led_brightness_set(struct phy_device *phydev,
MII_88E1318S_PHY_LED_FUNC, reg);
}
+static int m88e1318_led_blink_set(struct phy_device *phydev, u32 index,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ int reg;
+
+ reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1318S_PHY_LED_FUNC);
+ if (reg < 0)
+ return reg;
+
+ switch (index) {
+ case 0:
+ case 1:
+ case 2:
+ reg &= ~(0xf << (4 * index));
+ reg |= MII_88E1318S_PHY_LED_FUNC_BLINK << (4 * index);
+ /* Reset default is 84ms */
+ *delay_on = 84 / 2;
+ *delay_off = 84 / 2;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return phy_write_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1318S_PHY_LED_FUNC, reg);
+}
+
static int marvell_probe(struct phy_device *phydev)
{
struct marvell_priv *priv;
@@ -3112,6 +3143,7 @@ static struct phy_driver marvell_drivers[] = {
.get_strings = marvell_get_strings,
.get_stats = marvell_get_stats,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1145,
@@ -3219,6 +3251,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1540,
@@ -3246,6 +3279,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1545,
@@ -3273,6 +3307,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
{
.phy_id = MARVELL_PHY_ID_88E3016,
@@ -3415,6 +3450,7 @@ static struct phy_driver marvell_drivers[] = {
.get_tunable = m88e1540_get_tunable,
.set_tunable = m88e1540_set_tunable,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
};
--
2.39.2
Add documentation for support of generic ethernet LEDs.
These LEDs are ethernet port LED and are controllable by the ethernet
controller or the ethernet PHY.
A port may expose multiple LEDs and reg is used to provide an index to
differentiate them.
Ethernet port LEDs follow generic LED implementation.
Signed-off-by: Christian Marangi <[email protected]>
---
.../bindings/leds/leds-ethernet.yaml | 76 +++++++++++++++++++
1 file changed, 76 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-ethernet.yaml
diff --git a/Documentation/devicetree/bindings/leds/leds-ethernet.yaml b/Documentation/devicetree/bindings/leds/leds-ethernet.yaml
new file mode 100644
index 000000000000..0a03d65beea0
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-ethernet.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-ethernet.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common properties for the ethernet port LED.
+
+maintainers:
+ - Christian Marangi <[email protected]>
+
+description:
+ Bindings for the LEDs present in ethernet port and controllable by
+ the ethernet controller or the ethernet PHY regs.
+
+ These LEDs provide the same feature of a normal LED and follow
+ the same LED definitions.
+
+ An ethernet port may expose multiple LEDs, reg binding is used to
+ differentiate them.
+
+properties:
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+patternProperties:
+ '^led@[a-f0-9]+$':
+ $ref: /schemas/leds/common.yaml#
+
+ properties:
+ reg:
+ maxItems: 1
+ description:
+ This define the LED index in the PHY or the MAC. It's really
+ driver dependent and required for ports that define multiple
+ LED for the same port.
+
+ required:
+ - reg
+
+ unevaluatedProperties: false
+
+required:
+ - '#address-cells'
+ - '#size-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ default-state = "keep";
+ };
+ };
+...
--
2.39.2
Document support for LEDs node in ethernet-controller.
Ethernet Controller may support different LEDs that can be configured
for different operation like blinking on traffic event or port link.
Also add some Documentation to describe the difference of these nodes
compared to PHY LEDs, since ethernet-controller LEDs are controllable
by the ethernet controller regs and the possible intergated PHY doesn't
have control on them.
Signed-off-by: Christian Marangi <[email protected]>
---
.../devicetree/bindings/net/ethernet-controller.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 00be387984ac..e2558787531b 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -222,6 +222,16 @@ properties:
required:
- speed
+ leds:
+ description:
+ Describes the LEDs associated by Ethernet Controller.
+ These LEDs are not integrated in the PHY and PHY doesn't have any
+ control on them. Ethernet Controller regs are used to control
+ these defined LEDs.
+
+ allOf:
+ - $ref: /schemas/leds/leds-ethernet.yaml#
+
dependencies:
pcs-handle-names: [pcs-handle]
--
2.39.2
Add LEDs definition example for qca8k Switch Family to describe how they
should be defined for a correct usage.
Signed-off-by: Christian Marangi <[email protected]>
---
.../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index 389892592aac..ad354864187a 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -18,6 +18,8 @@ description:
PHY it is connected to. In this config, an internal mdio-bus is registered and
the MDIO master is used for communication. Mixed external and internal
mdio-bus configurations are not supported by the hardware.
+ Each phy has at most 3 LEDs connected and can be declared
+ using the standard LEDs structure.
properties:
compatible:
@@ -117,6 +119,7 @@ unevaluatedProperties: false
examples:
- |
#include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/leds/common.h>
mdio {
#address-cells = <1>;
@@ -226,6 +229,27 @@ examples:
label = "lan1";
phy-mode = "internal";
phy-handle = <&internal_phy_port1>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ default-state = "keep";
+ };
+ };
};
port@2 {
--
2.39.2
Add Switch LED for each port for MikroTik RB3011UiAS-RM.
MikroTik RB3011UiAS-RM is a 10 port device with 2 qca8337 switch chips
connected.
It was discovered that in the hardware design all 3 Switch LED trace of
the related port is connected to the same LED. This was discovered by
setting to 'always on' the related led in the switch regs and noticing
that all 3 LED for the specific port (for example for port 1) cause the
connected LED for port 1 to turn on. As an extra test we tried enabling
2 different LED for the port resulting in the LED turned off only if
every led in the reg was off.
Aside from this funny and strange hardware implementation, the device
itself have one green LED for each port, resulting in 10 green LED one
for each of the 10 supported port.
Cc: Jonathan McDowell <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
arch/arm/boot/dts/qcom-ipq8064-rb3011.dts | 120 ++++++++++++++++++++++
1 file changed, 120 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
index 47a5d1849c72..472b5a2912a1 100644
--- a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
+++ b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
@@ -65,26 +65,86 @@ fixed-link {
port@1 {
reg = <1>;
label = "sw1";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ };
+ };
};
port@2 {
reg = <2>;
label = "sw2";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <2>;
+ };
+ };
};
port@3 {
reg = <3>;
label = "sw3";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <3>;
+ };
+ };
};
port@4 {
reg = <4>;
label = "sw4";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <4>;
+ };
+ };
};
port@5 {
reg = <5>;
label = "sw5";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <5>;
+ };
+ };
};
};
};
@@ -130,26 +190,86 @@ fixed-link {
port@1 {
reg = <1>;
label = "sw6";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <6>;
+ };
+ };
};
port@2 {
reg = <2>;
label = "sw7";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <7>;
+ };
+ };
};
port@3 {
reg = <3>;
label = "sw8";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <8>;
+ };
+ };
};
port@4 {
reg = <4>;
label = "sw9";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <9>;
+ };
+ };
};
port@5 {
reg = <5>;
label = "sw10";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <10>;
+ };
+ };
};
};
};
--
2.39.2
IPQ8064 MikroTik RB3011UiAS-RM DT have currently unevaluted properties
in the 2 switch nodes. The bindings #address-cells and #size-cells are
redundant and cause warning for 'Unevaluated properties are not
allowed'.
Drop these bindings to mute these warning as they should not be there
from the start.
Cc: Jonathan McDowell <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
arch/arm/boot/dts/qcom-ipq8064-rb3011.dts | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
index f908889c4f95..47a5d1849c72 100644
--- a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
+++ b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
@@ -38,8 +38,6 @@ mdio0: mdio-0 {
switch0: switch@10 {
compatible = "qca,qca8337";
- #address-cells = <1>;
- #size-cells = <0>;
dsa,member = <0 0>;
@@ -105,8 +103,6 @@ mdio1: mdio-1 {
switch1: switch@14 {
compatible = "qca,qca8337";
- #address-cells = <1>;
- #size-cells = <0>;
dsa,member = <1 0>;
--
2.39.2
From: Andrew Lunn <[email protected]>
The WAN port of the 370-RD has a Marvell PHY, with one LED on
the front panel. List this LED in the device tree.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
arch/arm/boot/dts/armada-370-rd.dts | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
index be005c9f42ef..15b36aa34ef4 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -20,6 +20,7 @@
/dts-v1/;
#include <dt-bindings/input/input.h>
#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/leds/common.h>
#include <dt-bindings/gpio/gpio.h>
#include "armada-370.dtsi"
@@ -135,6 +136,19 @@ &mdio {
pinctrl-names = "default";
phy0: ethernet-phy@0 {
reg = <0>;
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ label = "WAN";
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "netdev";
+ };
+ };
};
switch: switch@10 {
--
2.39.2
Document support for LEDs node in phy and add an example for it.
PHY LED will have to match led pattern and should be treated as a
generic led.
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
.../devicetree/bindings/net/ethernet-phy.yaml | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 1327b81f15a2..1e55e028c918 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -197,6 +197,10 @@ properties:
PHY's that have configurable TX internal delays. If this property is
present then the PHY applies the TX delay.
+ leds:
+ allOf:
+ - $ref: /schemas/leds/leds-ethernet.yaml#
+
required:
- reg
@@ -204,6 +208,8 @@ additionalProperties: true
examples:
- |
+ #include <dt-bindings/leds/common.h>
+
ethernet {
#address-cells = <1>;
#size-cells = <0>;
@@ -219,5 +225,18 @@ examples:
reset-gpios = <&gpio1 4 1>;
reset-assert-us = <1000>;
reset-deassert-us = <2000>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ default-state = "keep";
+ };
+ };
};
};
--
2.39.2
On Mon, 27 Mar 2023 16:10:15 +0200 Christian Marangi wrote:
> This is a continue of [1]. It was decided to take a more gradual
> approach to implement LEDs support for switch and phy starting with
> basic support and then implementing the hw control part when we have all
> the prereq done.
>
> This series implements only the brightness_set() and blink_set() ops.
> An example of switch implementation is done with qca8k.
>
> For PHY a more generic approach is used with implementing the LED
> support in PHY core and with the user (in this case marvell) adding all
> the required functions.
>
> Currently we set the default-state as "keep" to not change the default
> configuration of the declared LEDs since almost every switch have a
> default configuration.
Please run ./scripts/kernel-doc -none on the headers,
the new ops added in patches 6 and 8 need to be kdoc'ed.
On Mon 2023-03-27 16:10:31, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> The WAN port of the 370-RD has a Marvell PHY, with one LED on
> the front panel. List this LED in the device tree.
> @@ -135,6 +136,19 @@ &mdio {
> pinctrl-names = "default";
> phy0: ethernet-phy@0 {
> reg = <0>;
> + leds {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led@0 {
> + reg = <0>;
> + label = "WAN";
> + color = <LED_COLOR_ID_WHITE>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
> + linux,default-trigger = "netdev";
> + };
/sys/class/leds/WAN is not acceptable.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
On Tue, Mar 28, 2023 at 10:31:16AM +0200, Pavel Machek wrote:
> On Mon 2023-03-27 16:10:31, Christian Marangi wrote:
> > From: Andrew Lunn <[email protected]>
> >
> > The WAN port of the 370-RD has a Marvell PHY, with one LED on
> > the front panel. List this LED in the device tree.
>
> > @@ -135,6 +136,19 @@ &mdio {
> > pinctrl-names = "default";
> > phy0: ethernet-phy@0 {
> > reg = <0>;
> > + leds {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + led@0 {
> > + reg = <0>;
> > + label = "WAN";
> > + color = <LED_COLOR_ID_WHITE>;
> > + function = LED_FUNCTION_LAN;
> > + function-enumerator = <1>;
> > + linux,default-trigger = "netdev";
> > + };
>
> /sys/class/leds/WAN is not acceptable.
As i said here, that is not what it gets called:
https://lore.kernel.org/netdev/[email protected]/T/#m6c72bd355df3fcf8babc0d01dd6bf2697d069407
> It can be found in /sys/class/leds/f1072004.mdio-mii:00:WAN. But when
> we come to using it for ledtrig-netdev, the user is more likely to follow
> /sys/class/net/eth0/phydev/leds/f1072004.mdio-mii\:00\:WAN/
Is that acceptable?
What are the acceptance criteria?
Andrew
On Mon, Mar 27, 2023 at 04:10:25PM +0200, Christian Marangi wrote:
> Add documentation for support of generic ethernet LEDs.
> These LEDs are ethernet port LED and are controllable by the ethernet
> controller or the ethernet PHY.
>
> A port may expose multiple LEDs and reg is used to provide an index to
> differentiate them.
> Ethernet port LEDs follow generic LED implementation.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> .../bindings/leds/leds-ethernet.yaml | 76 +++++++++++++++++++
> 1 file changed, 76 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-ethernet.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-ethernet.yaml b/Documentation/devicetree/bindings/leds/leds-ethernet.yaml
> new file mode 100644
> index 000000000000..0a03d65beea0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-ethernet.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-ethernet.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common properties for the ethernet port LED.
> +
> +maintainers:
> + - Christian Marangi <[email protected]>
> +
> +description:
> + Bindings for the LEDs present in ethernet port and controllable by
> + the ethernet controller or the ethernet PHY regs.
> +
> + These LEDs provide the same feature of a normal LED and follow
> + the same LED definitions.
> +
> + An ethernet port may expose multiple LEDs, reg binding is used to
> + differentiate them.
> +
> +properties:
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> +patternProperties:
> + '^led@[a-f0-9]+$':
> + $ref: /schemas/leds/common.yaml#
> +
> + properties:
> + reg:
> + maxItems: 1
> + description:
> + This define the LED index in the PHY or the MAC. It's really
> + driver dependent and required for ports that define multiple
> + LED for the same port.
> +
> + required:
> + - reg
> +
> + unevaluatedProperties: false
This does nothing to help the issues I raised. If the 'led' nodes have
custom properties, then you need a schema for the 'led' nodes and just
the 'led' nodes. Not a schema for the 'leds' container node.
If your not going to allow extending, then this can all be 1 file like
you had (with unevaluatedProperties added of course).
Rob
On Mon, Mar 27, 2023 at 04:10:31PM +0200, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> The WAN port of the 370-RD has a Marvell PHY, with one LED on
> the front panel. List this LED in the device tree.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> arch/arm/boot/dts/armada-370-rd.dts | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
> index be005c9f42ef..15b36aa34ef4 100644
> --- a/arch/arm/boot/dts/armada-370-rd.dts
> +++ b/arch/arm/boot/dts/armada-370-rd.dts
> @@ -20,6 +20,7 @@
> /dts-v1/;
> #include <dt-bindings/input/input.h>
> #include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/leds/common.h>
> #include <dt-bindings/gpio/gpio.h>
> #include "armada-370.dtsi"
>
> @@ -135,6 +136,19 @@ &mdio {
> pinctrl-names = "default";
> phy0: ethernet-phy@0 {
> reg = <0>;
> + leds {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led@0 {
> + reg = <0>;
> + label = "WAN";
WAN or
> + color = <LED_COLOR_ID_WHITE>;
> + function = LED_FUNCTION_LAN;
LAN?
> + function-enumerator = <1>;
> + linux,default-trigger = "netdev";
> + };
> + };
> };
>
> switch: switch@10 {
> --
> 2.39.2
>
> > + leds {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + led@0 {
> > + reg = <0>;
> > + label = "WAN";
>
> WAN or
>
> > + color = <LED_COLOR_ID_WHITE>;
> > + function = LED_FUNCTION_LAN;
>
> LAN?
Hi Rob
I did not know there was LED_FUNCTION_WAN. I just blindly copied it
from some other DT fragment.
I will change this, thanks.
Andrew
Hi!
> > > The WAN port of the 370-RD has a Marvell PHY, with one LED on
> > > the front panel. List this LED in the device tree.
> >
> > > @@ -135,6 +136,19 @@ &mdio {
> > > pinctrl-names = "default";
> > > phy0: ethernet-phy@0 {
> > > reg = <0>;
> > > + leds {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + led@0 {
> > > + reg = <0>;
> > > + label = "WAN";
> > > + color = <LED_COLOR_ID_WHITE>;
> > > + function = LED_FUNCTION_LAN;
> > > + function-enumerator = <1>;
> > > + linux,default-trigger = "netdev";
> > > + };
> >
> > /sys/class/leds/WAN is not acceptable.
>
> As i said here, that is not what it gets called:
>
> https://lore.kernel.org/netdev/[email protected]/T/#m6c72bd355df3fcf8babc0d01dd6bf2697d069407
>
> > It can be found in /sys/class/leds/f1072004.mdio-mii:00:WAN. But when
> > we come to using it for ledtrig-netdev, the user is more likely to follow
> > /sys/class/net/eth0/phydev/leds/f1072004.mdio-mii\:00\:WAN/
>
> Is that acceptable?
>
> What are the acceptance criteria?
Acceptance criteria would be "consistent with documentation and with
other similar users". If the LED is really white, it should be
f1072004.mdio-mii\:white\:WAN, but you probably want
f1072004.mdio-mii\:white\:LAN (or :activity), as discussed elsewhere in the thread.
Documentation is in Documentation/leds/well-known-leds.txt , so you
should probably add a new section about networking, and explain naming
scheme for network activity LEDs. When next users appear, I'll point
them to the documentation.
Does that sound ok?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
> Acceptance criteria would be "consistent with documentation and with
> other similar users". If the LED is really white, it should be
> f1072004.mdio-mii\:white\:WAN, but you probably want
> f1072004.mdio-mii\:white\:LAN (or :activity), as discussed elsewhere in the thread.
Hi Pavel
What i ended up with is:
f1072004.mdio-mii:00:white:wan
The label on the box is WAN, since it is meant to be a WiFi routers,
and this port should connected to your WAN. And this is what the LED
code came up with, given my DT description for this device.
> Documentation is in Documentation/leds/well-known-leds.txt , so you
> should probably add a new section about networking, and explain naming
> scheme for network activity LEDs. When next users appear, I'll point
> them to the documentation.
I added a patch with the following text:
* Ethernet LEDs
Currently two types of Network LEDs are support, those controlled by
the PHY and those by the MAC. In theory both can be present at the
same time for one Linux netdev, hence the names need to differ between
MAC and PHY.
Do not use the netdev name, such as eth0, enp1s0. These are not stable
and are not unique. They also don't differentiate between MAC and PHY.
** MAC LEDs
Good: f1070000.ethernet:white:WAN
Good: mdio_mux-0.1:00:green:left
Good: 0000:02:00.0:yellow:top
The first part must uniquely name the MAC controller. Then follows the
colour. WAN/LAN should be used for a single LED. If there are
multiple LEDs, use left/right, or top/bottom to indicate their
position on the RJ45 socket.
** PHY LEDs
Good: f1072004.mdio-mii:00: white:WAN
Good: !mdio-mux!mdio@2!switch@0!mdio:01:green:right
Good: r8169-0-200:00:yellow:bottom
The first part must uniquely name the PHY. This often means uniquely
identifying the MDIO bus controller, and the address on the bus.
Andrew
Hi!
> > Acceptance criteria would be "consistent with documentation and with
> > other similar users". If the LED is really white, it should be
> > f1072004.mdio-mii\:white\:WAN, but you probably want
> > f1072004.mdio-mii\:white\:LAN (or :activity), as discussed elsewhere in the thread.
>
> Hi Pavel
>
> What i ended up with is:
>
> f1072004.mdio-mii:00:white:wan
>
> The label on the box is WAN, since it is meant to be a WiFi routers,
> and this port should connected to your WAN. And this is what the LED
> code came up with, given my DT description for this device.
Ok, thanks for explanation.
> > Documentation is in Documentation/leds/well-known-leds.txt , so you
> > should probably add a new section about networking, and explain naming
> > scheme for network activity LEDs. When next users appear, I'll point
> > them to the documentation.
>
> I added a patch with the following text:
>
> * Ethernet LEDs
>
> Currently two types of Network LEDs are support, those controlled by
> the PHY and those by the MAC. In theory both can be present at the
> same time for one Linux netdev, hence the names need to differ between
> MAC and PHY.
>
> Do not use the netdev name, such as eth0, enp1s0. These are not stable
> and are not unique. They also don't differentiate between MAC and PHY.
>
> ** MAC LEDs
>
> Good: f1070000.ethernet:white:WAN
> Good: mdio_mux-0.1:00:green:left
> Good: 0000:02:00.0:yellow:top
> The first part must uniquely name the MAC controller. Then follows the
> colour. WAN/LAN should be used for a single LED. If there are
> multiple LEDs, use left/right, or top/bottom to indicate their
> position on the RJ45 socket.
I don't think basing stuff on position is reasonable. (And am not sure
if making difference between MAC and PHY leds is good idea).
Normally, there's ethernet port with two LEDs, one is usually green
and indicates link, second being yellow and indicates activity,
correct?
On devices like ADSL modems, there is one LED per port, typically on
with link and blinking with activity.
Could we use that distinction instead? (id):green:link,
(id):yellow:activity, (id):?:linkact -- for combined LED as it seems.
Are there any other common leds? I seem to remember "100mbps" lights
from time where 100mbit was fast...?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
> I don't think basing stuff on position is reasonable. (And am not sure
> if making difference between MAC and PHY leds is good idea).
>
> Normally, there's ethernet port with two LEDs, one is usually green
> and indicates link, second being yellow and indicates activity,
> correct?
Nope. I have machines with 1, 2 or 3 LEDs. I have green, yellow, white
and red LEDs.
Part of the problem is 802.3 says absolutely nothing about LEDs. So
every vendor is free to do whatever why want. There is no
standardisation at all. So we have to assume every vendor does
something different.
> On devices like ADSL modems, there is one LED per port, typically on
> with link and blinking with activity.
>
> Could we use that distinction instead? (id):green:link,
> (id):yellow:activity, (id):?:linkact -- for combined LED as it seems.
>
> Are there any other common leds? I seem to remember "100mbps" lights
> from time where 100mbit was fast...?
But what about 2.5G, 5G, 10G, 40G... And 10Mbps for automotive. And
collision for 1/2 duplex, which is making a bit of a comeback in
automotive.
Plus, we are using ledtrig-netdev. A wifi device is a netdev. A CAN
bus devices is a netdev. Link speed has a totally different meaning
for 802.11 and CAN.
You are also assuming the LEDs have fixed meaning. But they are not
fixed, they mean whatever the ledtrig-netdev is configured to make
them blink. I even have one of my boxes blinking heartbeat, because
if has a habit of crashing... And i think for Linux LEDs in general,
we should not really tie an LED to a meaning. Maybe tie it to a label
on the case, but the meaning of an LED is all about software, what
ledtrig- is controlling it.
As to differentiating MAC and PHY, we need to, because as i said, both
could offer LEDs. Generally, Ethernet switches have LED controllers
per MAC port. Most switches have internal PHYs, and those PHYs don't
have LED controllers. However, not all ports have internal PHYs, there
can be external PHYs with its own LED controller. So in that case,
both the MAC and the PHY could register an LED controller for the same
netdev. It comes down to DT to indicate what LED controllers are
actually wired to an LED.
Andrew
On Mon, Mar 27, 2023 at 04:10:27PM +0200, Christian Marangi wrote:
> Add LEDs definition example for qca8k Switch Family to describe how they
> should be defined for a correct usage.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index 389892592aac..ad354864187a 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -18,6 +18,8 @@ description:
> PHY it is connected to. In this config, an internal mdio-bus is registered and
> the MDIO master is used for communication. Mixed external and internal
> mdio-bus configurations are not supported by the hardware.
> + Each phy has at most 3 LEDs connected and can be declared
> + using the standard LEDs structure.
>
> properties:
> compatible:
> @@ -117,6 +119,7 @@ unevaluatedProperties: false
> examples:
> - |
> #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/leds/common.h>
>
> mdio {
> #address-cells = <1>;
> @@ -226,6 +229,27 @@ examples:
> label = "lan1";
> phy-mode = "internal";
> phy-handle = <&internal_phy_port1>;
> +
> + leds {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led@0 {
> + reg = <0>;
> + color = <LED_COLOR_ID_WHITE>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
> + default-state = "keep";
> + };
> +
> + led@1 {
> + reg = <1>;
> + color = <LED_COLOR_ID_AMBER>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
Isn't function-enumerator supposed to be unique within a given
'function'?
> + default-state = "keep";
> + };
> + };
> };
>
> port@2 {
> --
> 2.39.2
>
On Mon, Mar 27, 2023 at 04:10:28PM +0200, Christian Marangi wrote:
> IPQ8064 MikroTik RB3011UiAS-RM DT have currently unevaluted properties
> in the 2 switch nodes. The bindings #address-cells and #size-cells are
> redundant and cause warning for 'Unevaluated properties are not
> allowed'.
>
> Drop these bindings to mute these warning as they should not be there
> from the start.
Looks legit (and no particular reason it needs to wait for the rest of
the series).
Reviewed-By: Jonathan McDowell <[email protected]>
Tested-By: Jonathan McDowell <[email protected]>
> Cc: Jonathan McDowell <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> arch/arm/boot/dts/qcom-ipq8064-rb3011.dts | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
> index f908889c4f95..47a5d1849c72 100644
> --- a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
> +++ b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
> @@ -38,8 +38,6 @@ mdio0: mdio-0 {
>
> switch0: switch@10 {
> compatible = "qca,qca8337";
> - #address-cells = <1>;
> - #size-cells = <0>;
>
> dsa,member = <0 0>;
>
> @@ -105,8 +103,6 @@ mdio1: mdio-1 {
>
> switch1: switch@14 {
> compatible = "qca,qca8337";
> - #address-cells = <1>;
> - #size-cells = <0>;
>
> dsa,member = <1 0>;
>
> --
> 2.39.2
>
J.
--
Are you happy with your wash?
On Thu, Apr 06, 2023 at 09:10:18AM -0500, Rob Herring wrote:
> On Mon, Mar 27, 2023 at 04:10:27PM +0200, Christian Marangi wrote:
> > Add LEDs definition example for qca8k Switch Family to describe how they
> > should be defined for a correct usage.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
> > .../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > index 389892592aac..ad354864187a 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > @@ -18,6 +18,8 @@ description:
> > PHY it is connected to. In this config, an internal mdio-bus is registered and
> > the MDIO master is used for communication. Mixed external and internal
> > mdio-bus configurations are not supported by the hardware.
> > + Each phy has at most 3 LEDs connected and can be declared
> > + using the standard LEDs structure.
> >
> > properties:
> > compatible:
> > @@ -117,6 +119,7 @@ unevaluatedProperties: false
> > examples:
> > - |
> > #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/leds/common.h>
> >
> > mdio {
> > #address-cells = <1>;
> > @@ -226,6 +229,27 @@ examples:
> > label = "lan1";
> > phy-mode = "internal";
> > phy-handle = <&internal_phy_port1>;
> > +
> > + leds {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + led@0 {
> > + reg = <0>;
> > + color = <LED_COLOR_ID_WHITE>;
> > + function = LED_FUNCTION_LAN;
> > + function-enumerator = <1>;
> > + default-state = "keep";
> > + };
> > +
> > + led@1 {
> > + reg = <1>;
> > + color = <LED_COLOR_ID_AMBER>;
> > + function = LED_FUNCTION_LAN;
> > + function-enumerator = <1>;
>
> Isn't function-enumerator supposed to be unique within a given
> 'function'?
>
In the following example the output would be:
- amber:lan-1
- white:lan-1
So in theory it's unique for the same color and function. Is it
acceptable? Seems sane that there may be multiple color for the same
function (and enum)
> > + default-state = "keep";
> > + };
> > + };
> > };
> >
> > port@2 {
> > --
> > 2.39.2
> >
--
Ansuel
> > > examples:
> > > - |
> > > #include <dt-bindings/gpio/gpio.h>
> > > + #include <dt-bindings/leds/common.h>
> > >
> > > mdio {
> > > #address-cells = <1>;
> > > @@ -226,6 +229,27 @@ examples:
> > > label = "lan1";
> > > phy-mode = "internal";
> > > phy-handle = <&internal_phy_port1>;
> > > +
> > > + leds {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + led@0 {
> > > + reg = <0>;
> > > + color = <LED_COLOR_ID_WHITE>;
> > > + function = LED_FUNCTION_LAN;
> > > + function-enumerator = <1>;
> > > + default-state = "keep";
> > > + };
> > > +
> > > + led@1 {
> > > + reg = <1>;
> > > + color = <LED_COLOR_ID_AMBER>;
> > > + function = LED_FUNCTION_LAN;
> > > + function-enumerator = <1>;
> >
> > Isn't function-enumerator supposed to be unique within a given
> > 'function'?
> >
>
> In the following example the output would be:
> - amber:lan-1
> - white:lan-1
>
> So in theory it's unique for the same color and function. Is it
> acceptable? Seems sane that there may be multiple color for the same
> function (and enum)
But what does the -1 actually mean?
At Pavel's request, i documented 'good' names for these LEDs. I
suggested that if there are multiple LEDs for one MAC/PHY, you use
something like 'left' or 'right' to indicate their position on the
RJ45 socket. That has a clear meaning.
Andrew
On 3/27/2023 7:10 AM, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> Add a brightness function, so the LEDs can be controlled from
> software using the standard Linux LED infrastructure.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/27/2023 7:10 AM, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> Linux LEDs can be requested to perform hardware accelerated
> blinking. Pass this to the PHY driver, if it implements the op.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Michal Kubiak <[email protected]>
> Reviewed-by: Pavel Machek <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/27/2023 7:10 AM, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> Linux LEDs can be software controlled via the brightness file in /sys.
> LED drivers need to implement a brightness_set function which the core
> will call. Implement an intermediary in phy_device, which will call
> into the phy driver if it implements the necessary function.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
> + int (*led_brightness_set)(struct phy_device *dev,
> + u32 index, enum led_brightness value);
I think I would have made this an u8, 4 billion LEDs, man, that's a lot!
--
Florian
On 3/27/2023 7:10 AM, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> Provide stubs for devm_led_classdev_register_ext() and
> led_init_default_state_get() so that LED drivers embedded within other
> drivers such as PHYs and Ethernet switches still build when LEDS_CLASS
> or NEW_LEDS are disabled. This also helps with Kconfig dependencies,
> which are somewhat hairy for phylib and mdio and only get worse when
> adding a dependency on LED_CLASS.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/27/2023 7:10 AM, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> The Marvell PHY can blink the LEDs, simple on/off. All LEDs blink at
> the same rate, and the reset default is 84ms per blink, which is
> around 12Hz.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Pavel Machek <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Thu, Apr 13, 2023 at 06:57:51AM -0700, Florian Fainelli wrote:
>
>
> On 3/27/2023 7:10 AM, Christian Marangi wrote:
> > From: Andrew Lunn <[email protected]>
> >
> > Linux LEDs can be software controlled via the brightness file in /sys.
> > LED drivers need to implement a brightness_set function which the core
> > will call. Implement an intermediary in phy_device, which will call
> > into the phy driver if it implements the necessary function.
> >
> > Signed-off-by: Andrew Lunn <[email protected]>
> > Signed-off-by: Christian Marangi <[email protected]>
>
> Reviewed-by: Florian Fainelli <[email protected]>
>
> > + int (*led_brightness_set)(struct phy_device *dev,
> > + u32 index, enum led_brightness value);
>
> I think I would have made this an u8, 4 billion LEDs, man, that's a lot!
If andrew is ok we can still consider to reduce it. (but just to joke
about it... A MAN CAN DREAM OF A FULL HD SCREEN ON THEIR OWN SPECIAL
PORT)
--
Ansuel
On 4/12/2023 4:11 PM, Christian Marangi wrote:
> On Thu, Apr 13, 2023 at 06:57:51AM -0700, Florian Fainelli wrote:
>>
>>
>> On 3/27/2023 7:10 AM, Christian Marangi wrote:
>>> From: Andrew Lunn <[email protected]>
>>>
>>> Linux LEDs can be software controlled via the brightness file in /sys.
>>> LED drivers need to implement a brightness_set function which the core
>>> will call. Implement an intermediary in phy_device, which will call
>>> into the phy driver if it implements the necessary function.
>>>
>>> Signed-off-by: Andrew Lunn <[email protected]>
>>> Signed-off-by: Christian Marangi <[email protected]>
>>
>> Reviewed-by: Florian Fainelli <[email protected]>
>>
>>> + int (*led_brightness_set)(struct phy_device *dev,
>>> + u32 index, enum led_brightness value);
>>
>> I think I would have made this an u8, 4 billion LEDs, man, that's a lot!
>
> If andrew is ok we can still consider to reduce it. (but just to joke
> about it... A MAN CAN DREAM OF A FULL HD SCREEN ON THEIR OWN SPECIAL
> PORT)
Humm just thought of something else, is it OK for led_brightness_set and
led_blink_set to call into functions that might require sleeping (MDIO,
I2C, SPI, etc.)?
--
Florian
On 3/27/2023 7:10 AM, Christian Marangi wrote:
> Move qca8k_port_to_phy() to qca8k header as it's useful for future
> reference in Switch LEDs module since the same logic is applied to get
> the right index of the switch port.
> Make it inline as it's simple function that just decrease the port.
>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> Reviewed-by: Michal Kubiak <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/27/2023 7:10 AM, Christian Marangi wrote:
> Add LEDs blink_set() support to qca8k Switch Family.
> These LEDs support hw accellerated blinking at a fixed rate
> of 4Hz.
>
> Reject any other value since not supported by the LEDs switch.
>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
> Humm just thought of something else, is it OK for led_brightness_set and
> led_blink_set to call into functions that might require sleeping (MDIO, I2C,
> SPI, etc.)?
Hi Florian
That is fine. The LED class is similar to GPIOs. There is a can sleep
version, and an atomic version. For phylib we are using the can sleep
version. The LED core then hides the differences from the users.
Andrew
On Thu, Apr 13, 2023 at 06:57:51AM -0700, Florian Fainelli wrote:
>
>
> On 3/27/2023 7:10 AM, Christian Marangi wrote:
> > From: Andrew Lunn <[email protected]>
> >
> > Linux LEDs can be software controlled via the brightness file in /sys.
> > LED drivers need to implement a brightness_set function which the core
> > will call. Implement an intermediary in phy_device, which will call
> > into the phy driver if it implements the necessary function.
> >
> > Signed-off-by: Andrew Lunn <[email protected]>
> > Signed-off-by: Christian Marangi <[email protected]>
>
> Reviewed-by: Florian Fainelli <[email protected]>
>
> > + int (*led_brightness_set)(struct phy_device *dev,
> > + u32 index, enum led_brightness value);
>
> I think I would have made this an u8, 4 billion LEDs, man, that's a lot!
That can be done. We need to change:
err = of_property_read_u32(led, "reg", &phyled->index);
if (err)
return err;
to a u8, to avoid overflow problems in other places. It looks like
of_property_read_u8() does the correct thing if somebody tried to use
4 billion - 1.
Andrew