[net-next PATCH v5 00/15] net: Add basic LED support for switch/phy

[net-next PATCH v5 00/15] net: Add basic LED support for switch/phy

[Christian Marangi]

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/20230216013230.22978-1-ansuelsmth@gmail.com/

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 is 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 (8):
  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: net: ethernet-controller: Document support for LEDs node
  dt-bindings: net: dsa: qca8k: add LEDs definition example
  arm: qcom: dt: Drop unevaluated properties in switch nodes for rb3011
  arm: qcom: dt: Add Switch LED for each port for rb3011
  dt-bindings: net: phy: Document support for LEDs node

 .../devicetree/bindings/net/dsa/qca8k.yaml    |  24 ++
 .../bindings/net/ethernet-controller.yaml     |  21 ++
 .../devicetree/bindings/net/ethernet-phy.yaml |  31 ++
 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/marvell.c                     |  81 +++++-
 drivers/net/phy/phy_device.c                  | 102 +++++++
 include/linux/leds.h                          |  18 ++
 include/linux/phy.h                           |  35 +++
 15 files changed, 817 insertions(+), 24 deletions(-)
 create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
 create mode 100644 drivers/net/dsa/qca/qca8k_leds.h

-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 01/15] net: dsa: qca8k: move qca8k_port_to_phy() to header

[Christian Marangi]

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 <ansuelsmth@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 02/15] net: dsa: qca8k: add LEDs basic support

[Christian Marangi]

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 <ansuelsmth@gmail.com>
---
 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..5a29413ba94a
--- /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, &reg_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 first 2 bit (1, 0) 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 (17, 16) 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, &reg_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 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 %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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 03/15] net: dsa: qca8k: add LEDs blink_set() support

[Christian Marangi]

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 <ansuelsmth@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 5a29413ba94a..d0a37660faa2 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, &reg_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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 04/15] leds: Provide stubs for when CLASS_LED is disabled

[Christian Marangi]

From: Andrew Lunn <andrew@lunn.ch>

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
is 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 <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/leds.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index d71201a968b6..f6dec57453da 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_LEDS_CLASS)
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 05/15] net: phy: Add a binding for PHY LEDs

[Christian Marangi]

From: Andrew Lunn <andrew@lunn.ch>

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.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy_device.c | 75 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 16 ++++++++
 2 files changed, 91 insertions(+)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 06/15] net: phy: phy_device: Call into the PHY driver to set LED brightness

[Christian Marangi]

From: Andrew Lunn <andrew@lunn.ch>

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 <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 07/15] net: phy: marvell: Add software control of the LEDs

[Christian Marangi]

From: Andrew Lunn <andrew@lunn.ch>

Add a brightness function, so the LEDs can be controlled from
software using the standard Linux LED infrastructure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 08/15] net: phy: phy_device: Call into the PHY driver to set LED blinking

[Christian Marangi]

From: Andrew Lunn <andrew@lunn.ch>

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 <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 09/15] net: phy: marvell: Implement led_blink_set()

[Christian Marangi]

From: Andrew Lunn <andrew@lunn.ch>

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 <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 10/15] dt-bindings: net: ethernet-controller: Document support for LEDs node

[Christian Marangi]

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 <ansuelsmth@gmail.com>
---
 .../bindings/net/ethernet-controller.yaml     | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 00be387984ac..a93673592314 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -222,6 +222,27 @@ properties:
         required:
           - speed
 
+  leds:
+    type: object
+    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.
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      '^led(@[a-f0-9]+)?$':
+        $ref: /schemas/leds/common.yaml#
+
+    additionalProperties: false
+
 dependencies:
   pcs-handle-names: [pcs-handle]
 
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 11/15] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Christian Marangi]

Add LEDs definition example for qca8k Switch Family to describe how they
should be defined for a correct usage.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../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..2e9c14af0223 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 least 3 LEDs connected and can be declared
+  using the standard LEDs structure.
 
 properties:
   compatible:
@@ -117,6 +119,7 @@ unevaluatedProperties: false
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
 
     mdio {
         #address-cells = <1>;
@@ -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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 12/15] arm: qcom: dt: Drop unevaluated properties in switch nodes for rb3011

[Christian Marangi]

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 <noodles@earth.li>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 13/15] arm: qcom: dt: Add Switch LED for each port for rb3011

[Christian Marangi]

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 <noodles@earth.li>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 14/15] dt-bindings: net: phy: Document support for LEDs node

[Christian Marangi]

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 <ansuelsmth@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 .../devicetree/bindings/net/ethernet-phy.yaml | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 1327b81f15a2..84e15cee27c7 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -197,6 +197,22 @@ properties:
       PHY's that have configurable TX internal delays. If this property is
       present then the PHY applies the TX delay.
 
+  leds:
+    type: object
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      '^led(@[a-f0-9]+)?$':
+        $ref: /schemas/leds/common.yaml#
+
+    additionalProperties: false
+
 required:
   - reg
 
@@ -204,6 +220,8 @@ additionalProperties: true
 
 examples:
   - |
+    #include <dt-bindings/leds/common.h>
+
     ethernet {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -219,5 +237,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[net-next PATCH v5 15/15] arm: mvebu: dt: Add PHY LED support for 370-rd WAN port

[Christian Marangi]

From: Andrew Lunn <andrew@lunn.ch>

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 <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 04/15] leds: Provide stubs for when CLASS_LED is disabled

[Andrew Lunn]

> +#if IS_ENABLED(CONFIG_LEDS_CLASS)
>  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

0-day is telling me i have this wrong. The function is in led-core.c,
so this should be CONFIG_NEW_LEDS, not CONFIG_LEDS_CLASS.

	    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 02/15] net: dsa: qca8k: add LEDs basic support

[Michal Kubiak]

On Sun, Mar 19, 2023 at 08:18:01PM +0100, Christian Marangi wrote:
> 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 <ansuelsmth@gmail.com>
> ---

Hi Christian,

The patch looks good to me. I just found one nitpick in the comment.

Thanks,
Michal

> +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 least 3 leds attached

Nitpick: "at least" -> "at most"


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 01/15] net: dsa: qca8k: move qca8k_port_to_phy() to header

[Michal Kubiak]

On Sun, Mar 19, 2023 at 08:18:00PM +0100, 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 <ansuelsmth@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---


Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 02/15] net: dsa: qca8k: add LEDs basic support

[Christian Marangi]

On Mon, Mar 20, 2023 at 05:30:05PM +0100, Michal Kubiak wrote:
> On Sun, Mar 19, 2023 at 08:18:01PM +0100, Christian Marangi wrote:
> > 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 <ansuelsmth@gmail.com>
> > ---
> 
> Hi Christian,
> 
> The patch looks good to me. I just found one nitpick in the comment.
> 
> Thanks,
> Michal
> 
> > +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 least 3 leds attached
> 
> Nitpick: "at least" -> "at most"
>

Oh god... I added the extra comment and totally missed the other.
Sorry!

Btw ok for the description of the LED mapping? It's a bit complex so
tried to do my best to describe them.

-- 
	Ansuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 05/15] net: phy: Add a binding for PHY LEDs

[Michal Kubiak]

On Sun, Mar 19, 2023 at 08:18:04PM +0100, Christian Marangi wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> 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.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 06/15] net: phy: phy_device: Call into the PHY driver to set LED brightness

[Michal Kubiak]

On Sun, Mar 19, 2023 at 08:18:05PM +0100, Christian Marangi wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> 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 <andrew@lunn.ch>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 02/15] net: dsa: qca8k: add LEDs basic support

[Michal Kubiak]

On Mon, Mar 20, 2023 at 05:33:56PM +0100, Christian Marangi wrote:
> 
> Btw ok for the description of the LED mapping? It's a bit complex so
> tried to do my best to describe them.
> 

Yes, now it is much easier to understand the logic behind LED mapping.
Thanks for adding that! I think it will save some time for anyone who
will be working with that code in the future.

The only thing I still do not understand is the initial 14 bit shift:

>	if (led->port_num == 0 || led->port_num == 4) {
>		mask = QCA8K_LED_PATTERN_EN_MASK;
>		val <<= QCA8K_LED_PATTERN_EN_SHIFT;

For example, according to the code above, for port 4:
	- the value is shifted by 14 bits - to bits (15,14)
	- mask is also set to bits (15,14)
	- then, both mask and value are shifted again by 16 bits:

>		return regmap_update_bits(priv->regmap, reg_info.reg,
>					  mask << reg_info.shift,
>					  val << reg_info.shift);

because reg_info.shift == QCA8K_LED_PHY4_CONTROL_RULE_SHIFT == 16 for
port_num == 4.

It means, in fact, for controlling port 4 we use bits (31,30) which
seems to be inconsistent with your comment below.

>	 * To control port 4:
>	 * - the 2 bit (17, 16) of:
>	 *   - QCA8K_LED_CTRL0_REG for led1
>	 *   - QCA8K_LED_CTRL1_REG for led2
>	 *   - QCA8K_LED_CTRL2_REG for led3
>	 *

Are values for ports 0 and 4 correct in your description in
"qca8k_led_brightness_set()"?

Thanks,
Michal

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 04/15] leds: Provide stubs for when CLASS_LED is disabled

[Christian Marangi]

On Sun, Mar 19, 2023 at 11:49:02PM +0100, Andrew Lunn wrote:
> > +#if IS_ENABLED(CONFIG_LEDS_CLASS)
> >  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
> 
> 0-day is telling me i have this wrong. The function is in led-core.c,
> so this should be CONFIG_NEW_LEDS, not CONFIG_LEDS_CLASS.
> 

Any idea why? NEW_LEDS just enable LEDS_CLASS selection so why we need
to use that? Should not make a difference (in theory)

Anyway hoping every other patch and Documentation patch gets some review
tag, v6 should be last revision I hope? (so we can move to LEDs stuff)

-- 
	Ansuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 02/15] net: dsa: qca8k: add LEDs basic support

[Christian Marangi]

On Mon, Mar 20, 2023 at 06:48:51PM +0100, Michal Kubiak wrote:
> On Mon, Mar 20, 2023 at 05:33:56PM +0100, Christian Marangi wrote:
> > 
> > Btw ok for the description of the LED mapping? It's a bit complex so
> > tried to do my best to describe them.
> > 
> 
> Yes, now it is much easier to understand the logic behind LED mapping.
> Thanks for adding that! I think it will save some time for anyone who
> will be working with that code in the future.
> 
> The only thing I still do not understand is the initial 14 bit shift:
> 
> >	if (led->port_num == 0 || led->port_num == 4) {
> >		mask = QCA8K_LED_PATTERN_EN_MASK;
> >		val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> 
> For example, according to the code above, for port 4:
> 	- the value is shifted by 14 bits - to bits (15,14)
> 	- mask is also set to bits (15,14)
> 	- then, both mask and value are shifted again by 16 bits:
> 
> >		return regmap_update_bits(priv->regmap, reg_info.reg,
> >					  mask << reg_info.shift,
> >					  val << reg_info.shift);
> 
> because reg_info.shift == QCA8K_LED_PHY4_CONTROL_RULE_SHIFT == 16 for
> port_num == 4.
> 
> It means, in fact, for controlling port 4 we use bits (31,30) which
> seems to be inconsistent with your comment below.
> 
> >	 * To control port 4:
> >	 * - the 2 bit (17, 16) of:
> >	 *   - QCA8K_LED_CTRL0_REG for led1
> >	 *   - QCA8K_LED_CTRL1_REG for led2
> >	 *   - QCA8K_LED_CTRL2_REG for led3
> >	 *
> 
> Are values for ports 0 and 4 correct in your description in
> "qca8k_led_brightness_set()"?
> 

Code is correct, comment is not.

QCA8K_LED_CTRL0_REG is split in 2 part.
- first 16 bit for phy0
- second part (31, 16) for phy4

In these 16 half there are the bit that control the hw control blink
rules AND on the last 2 part of the half, the bit that control the state
of the LED (off, on, always-blink, hw control)

So I just didn't add on top of that MASK the required shift for
QCA8K_LED_PATTERN_EN_SHIFT.

so for phy0

GENMASK(1, 0) << QCA8K_LED_PATTERN_EN_SHIFT << QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT
GENMASK(1, 0) << 14 << 0 

for phy4

GENMASK(1, 0) << QCA8K_LED_PATTERN_EN_SHIFT << QCA8K_LED_PHY4_CONTROL_RULE_SHIFT
GENMASK(1, 0) << 14 << 16

Thanks for the other review tag, will fix the last bit in v6.

-- 
	Ansuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 04/15] leds: Provide stubs for when CLASS_LED is disabled

[Andrew Lunn]

On Mon, Mar 20, 2023 at 06:52:47PM +0100, Christian Marangi wrote:
> On Sun, Mar 19, 2023 at 11:49:02PM +0100, Andrew Lunn wrote:
> > > +#if IS_ENABLED(CONFIG_LEDS_CLASS)
> > >  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
> > 
> > 0-day is telling me i have this wrong. The function is in led-core.c,
> > so this should be CONFIG_NEW_LEDS, not CONFIG_LEDS_CLASS.
> > 
> 
> Any idea why? NEW_LEDS just enable LEDS_CLASS selection so why we need
> to use that? Should not make a difference (in theory)

0-day came up with a configuration which resulted in NEW_LEDS enabled
but LEDS_CLASS disabled. That then resulted in multiple definitions of 
led_init_default_state_get() when linking.

I _guess_ this is because select is used, which is not mandatory. So
randconfig can turn off something which is enabled by select.

I updated my tree, and so far 0-day has not complained, but it can
take a few days when it is busy.

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 04/15] leds: Provide stubs for when CLASS_LED is disabled

[Christian Marangi]

On Mon, Mar 20, 2023 at 08:31:36PM +0100, Andrew Lunn wrote:
> On Mon, Mar 20, 2023 at 06:52:47PM +0100, Christian Marangi wrote:
> > On Sun, Mar 19, 2023 at 11:49:02PM +0100, Andrew Lunn wrote:
> > > > +#if IS_ENABLED(CONFIG_LEDS_CLASS)
> > > >  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
> > > 
> > > 0-day is telling me i have this wrong. The function is in led-core.c,
> > > so this should be CONFIG_NEW_LEDS, not CONFIG_LEDS_CLASS.
> > > 
> > 
> > Any idea why? NEW_LEDS just enable LEDS_CLASS selection so why we need
> > to use that? Should not make a difference (in theory)
> 
> 0-day came up with a configuration which resulted in NEW_LEDS enabled
> but LEDS_CLASS disabled. That then resulted in multiple definitions of 
> led_init_default_state_get() when linking.
> 
> I _guess_ this is because select is used, which is not mandatory. So
> randconfig can turn off something which is enabled by select.
> 
> I updated my tree, and so far 0-day has not complained, but it can
> take a few days when it is busy.
> 

BTW yes I repro the problem.

Checked the makefile and led-core.c is compiled with NEW_LEDS and
led-class is compiled with LEDS_CLASS.

led_init_default_state_get is in led-core.c and this is the problem with
using LEDS_CLASS instead of NEW_LEDS...

But actually why we are putting led_init_default_state_get behind a
config? IMHO we should compile it anyway.

So my suggestion is to keep the LEDS_CLASS and just remove the part for 
led_init_default_state_get.

Also why IS_ENABLED instead of a simple ifdef? (in leds.h there is a mix
of both so I wonder if we should use one or the other)

-- 
	Ansuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 04/15] leds: Provide stubs for when CLASS_LED is disabled

[Andrew Lunn]

> Also why IS_ENABLED instead of a simple ifdef? (in leds.h there is a mix
> of both so I wonder if we should use one or the other)

/*
 * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
 * 0 otherwise.  Note that CONFIG_FOO=y results in "#define CONFIG_FOO 1" in
 * autoconf.h, while CONFIG_FOO=m results in "#define CONFIG_FOO_MODULE 1".
 */
#define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))

It cleanly handles the module case, which i guess most people would
get wrong.

    Andrew



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 04/15] leds: Provide stubs for when CLASS_LED is disabled

[Andrew Lunn]

> BTW yes I repro the problem.
> 
> Checked the makefile and led-core.c is compiled with NEW_LEDS and
> led-class is compiled with LEDS_CLASS.
> 
> led_init_default_state_get is in led-core.c and this is the problem with
> using LEDS_CLASS instead of NEW_LEDS...
> 
> But actually why we are putting led_init_default_state_get behind a
> config? IMHO we should compile it anyway.

It is pointless if you don't have any LED support. To make it always
compiled, you would probably need to move it into leds.h. And then you
bloat every user with some code which is not hot path.

      Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 04/15] leds: Provide stubs for when CLASS_LED is disabled

[Christian Marangi]

On Tue, Mar 21, 2023 at 05:02:42PM +0100, Andrew Lunn wrote:
> > BTW yes I repro the problem.
> > 
> > Checked the makefile and led-core.c is compiled with NEW_LEDS and
> > led-class is compiled with LEDS_CLASS.
> > 
> > led_init_default_state_get is in led-core.c and this is the problem with
> > using LEDS_CLASS instead of NEW_LEDS...
> > 
> > But actually why we are putting led_init_default_state_get behind a
> > config? IMHO we should compile it anyway.
> 
> It is pointless if you don't have any LED support. To make it always
> compiled, you would probably need to move it into leds.h. And then you
> bloat every user with some code which is not hot path.
> 

Ok think just to be safe we should wait one more day for the 0 day bot
and then finally send v6? 

-- 
	Ansuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 10/15] dt-bindings: net: ethernet-controller: Document support for LEDs node

[Rob Herring]

On Sun, Mar 19, 2023 at 08:18:09PM +0100, Christian Marangi wrote:
> 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 <ansuelsmth@gmail.com>
> ---
>  .../bindings/net/ethernet-controller.yaml     | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 00be387984ac..a93673592314 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -222,6 +222,27 @@ properties:
>          required:
>            - speed
>  
> +  leds:
> +    type: object
> +    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.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      '^led(@[a-f0-9]+)?$':
> +        $ref: /schemas/leds/common.yaml#

Are specific ethernet controllers allowed to add their own properties in 
led nodes? If so, this doesn't work. As-is, this allows any other 
properties. You need 'unevaluatedProperties: false' here to prevent 
that. But then no one can add properties. If you want to support that, 
then you need this to be a separate schema that devices can optionally 
include if they don't extend the properties, and then devices that 
extend the binding would essentially have the above with:

$ref: /schemas/leds/common.yaml#
unevaluatedProperties: false
properties:
  a-custom-device-prop: ...


If you wanted to define both common ethernet LED properties and 
device specific properties, then you'd need to replace leds/common.yaml 
above  with the ethernet one.

This is all the same reasons the DSA/switch stuff and graph bindings are 
structured the way they are.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 11/15] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Rob Herring]

On Sun, Mar 19, 2023 at 08:18:10PM +0100, 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 <ansuelsmth@gmail.com>
> ---
>  .../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..2e9c14af0223 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 least 3 LEDs connected and can be declared
> +  using the standard LEDs structure.
>  
>  properties:
>    compatible:
> @@ -117,6 +119,7 @@ unevaluatedProperties: false
>  examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
>  
>      mdio {
>          #address-cells = <1>;
> @@ -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>;

Once 'unevaluatedProperties' is properly implemented in the schema, this 
will be a warning. You didn't define 'reg' in the schema.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 14/15] dt-bindings: net: phy: Document support for LEDs node

[Rob Herring]

On Sun, Mar 19, 2023 at 08:18:13PM +0100, Christian Marangi wrote:
> 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 <ansuelsmth@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  .../devicetree/bindings/net/ethernet-phy.yaml | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 1327b81f15a2..84e15cee27c7 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -197,6 +197,22 @@ properties:
>        PHY's that have configurable TX internal delays. If this property is
>        present then the PHY applies the TX delay.
>  
> +  leds:
> +    type: object
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      '^led(@[a-f0-9]+)?$':
> +        $ref: /schemas/leds/common.yaml#

Same questions/issues in this one.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 10/15] dt-bindings: net: ethernet-controller: Document support for LEDs node

[Christian Marangi]

On Tue, Mar 21, 2023 at 04:19:53PM -0500, Rob Herring wrote:
> On Sun, Mar 19, 2023 at 08:18:09PM +0100, Christian Marangi wrote:
> > 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 <ansuelsmth@gmail.com>
> > ---
> >  .../bindings/net/ethernet-controller.yaml     | 21 +++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > index 00be387984ac..a93673592314 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > @@ -222,6 +222,27 @@ properties:
> >          required:
> >            - speed
> >  
> > +  leds:
> > +    type: object
> > +    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.
> > +
> > +    properties:
> > +      '#address-cells':
> > +        const: 1
> > +
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    patternProperties:
> > +      '^led(@[a-f0-9]+)?$':
> > +        $ref: /schemas/leds/common.yaml#
> 
> Are specific ethernet controllers allowed to add their own properties in 
> led nodes? If so, this doesn't work. As-is, this allows any other 
> properties. You need 'unevaluatedProperties: false' here to prevent 
> that. But then no one can add properties. If you want to support that, 
> then you need this to be a separate schema that devices can optionally 
> include if they don't extend the properties, and then devices that 
> extend the binding would essentially have the above with:
> 
> $ref: /schemas/leds/common.yaml#
> unevaluatedProperties: false
> properties:
>   a-custom-device-prop: ...
> 
> 
> If you wanted to define both common ethernet LED properties and 
> device specific properties, then you'd need to replace leds/common.yaml 
> above  with the ethernet one.
> 
> This is all the same reasons the DSA/switch stuff and graph bindings are 
> structured the way they are.
> 

Hi Rob, thanks for the review/questions.

The idea of all of this is to keep leds node as standard as possible.
It was asked to add unevaluatedProperties: False but I didn't understood
it was needed also for the led nodes.

leds/common.yaml have additionalProperties set to true but I guess that
is not OK for the final schema and we need something more specific.

Looking at the common.yaml schema reg binding is missing so an
additional schema is needed.

Reg is needed for ethernet LEDs and PHY but I think we should also permit
to skip that if the device actually have just one LED. (if this wouldn't
complicate the implementation. Maybe some hints from Andrew about this
decision?)

If we decide that reg is a must, if I understood it correctly we should
create something like leds-ethernet.yaml that would reference common and
add reg binding? Is it correct? This schema should be laded in leds
directory and not in the net/ethernet.

Also with setting reg mandatory I will have to fix the regex to require
@ in the node name.


Also also if we decide for a more specific schema, I guess I can
reference that directly in ethernet-phy.yaml and ethernet-controller.yaml
with something like:

leds:
  $ref: /schemas/leds/leds-ethernet.yaml#

Again thanks for the review and hope you can give some
hint/clarification if I got everything right.

-- 
	Ansuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 10/15] dt-bindings: net: ethernet-controller: Document support for LEDs node

[Andrew Lunn]

> > Are specific ethernet controllers allowed to add their own properties in 
> > led nodes? If so, this doesn't work. As-is, this allows any other 
> > properties. You need 'unevaluatedProperties: false' here to prevent 
> > that. But then no one can add properties. If you want to support that, 
> > then you need this to be a separate schema that devices can optionally 
> > include if they don't extend the properties, and then devices that 
> > extend the binding would essentially have the above with:
> > 
> > $ref: /schemas/leds/common.yaml#
> > unevaluatedProperties: false
> > properties:
> >   a-custom-device-prop: ...
> > 
> > 
> > If you wanted to define both common ethernet LED properties and 
> > device specific properties, then you'd need to replace leds/common.yaml 
> > above  with the ethernet one.
> > 
> > This is all the same reasons the DSA/switch stuff and graph bindings are 
> > structured the way they are.
> > 
> 
> Hi Rob, thanks for the review/questions.
> 
> The idea of all of this is to keep leds node as standard as possible.
> It was asked to add unevaluatedProperties: False but I didn't understood
> it was needed also for the led nodes.
> 
> leds/common.yaml have additionalProperties set to true but I guess that
> is not OK for the final schema and we need something more specific.
> 
> Looking at the common.yaml schema reg binding is missing so an
> additional schema is needed.
> 
> Reg is needed for ethernet LEDs and PHY but I think we should also permit
> to skip that if the device actually have just one LED. (if this wouldn't
> complicate the implementation. Maybe some hints from Andrew about this
> decision?)

I would make reg mandatory.

We should not encourage additional properties, but i also think we
cannot block it.

The problem we have is that there is absolutely no standardisation
here. Vendors are free to do whatever they want, and they do. So i
would not be too surprised if some vendor properties are needed
eventually.

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 10/15] dt-bindings: net: ethernet-controller: Document support for LEDs node

[Christian Marangi]

On Wed, Mar 22, 2023 at 12:23:59AM +0100, Andrew Lunn wrote:
> > > Are specific ethernet controllers allowed to add their own properties in 
> > > led nodes? If so, this doesn't work. As-is, this allows any other 
> > > properties. You need 'unevaluatedProperties: false' here to prevent 
> > > that. But then no one can add properties. If you want to support that, 
> > > then you need this to be a separate schema that devices can optionally 
> > > include if they don't extend the properties, and then devices that 
> > > extend the binding would essentially have the above with:
> > > 
> > > $ref: /schemas/leds/common.yaml#
> > > unevaluatedProperties: false
> > > properties:
> > >   a-custom-device-prop: ...
> > > 
> > > 
> > > If you wanted to define both common ethernet LED properties and 
> > > device specific properties, then you'd need to replace leds/common.yaml 
> > > above  with the ethernet one.
> > > 
> > > This is all the same reasons the DSA/switch stuff and graph bindings are 
> > > structured the way they are.
> > > 
> > 
> > Hi Rob, thanks for the review/questions.
> > 
> > The idea of all of this is to keep leds node as standard as possible.
> > It was asked to add unevaluatedProperties: False but I didn't understood
> > it was needed also for the led nodes.
> > 
> > leds/common.yaml have additionalProperties set to true but I guess that
> > is not OK for the final schema and we need something more specific.
> > 
> > Looking at the common.yaml schema reg binding is missing so an
> > additional schema is needed.
> > 
> > Reg is needed for ethernet LEDs and PHY but I think we should also permit
> > to skip that if the device actually have just one LED. (if this wouldn't
> > complicate the implementation. Maybe some hints from Andrew about this
> > decision?)
> 
> I would make reg mandatory.
>

Ok will add a new schema and change the regex.

> We should not encourage additional properties, but i also think we
> cannot block it.
> 
> The problem we have is that there is absolutely no standardisation
> here. Vendors are free to do whatever they want, and they do. So i
> would not be too surprised if some vendor properties are needed
> eventually.
>

Think that will come later with defining a more specific schema. But I
honestly think most of the special implementation will be handled to the
driver internally and not with special binding in DT.

-- 
	Ansuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 03/15] net: dsa: qca8k: add LEDs blink_set() support

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 430 bytes --]

Hi!

> 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 <ansuelsmth@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 07/15] net: phy: marvell: Add software control of the LEDs

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 443 bytes --]

On Sun 2023-03-19 20:18:06, Christian Marangi wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> Add a brightness function, so the LEDs can be controlled from
> software using the standard Linux LED infrastructure.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 08/15] net: phy: phy_device: Call into the PHY driver to set LED blinking

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 454 bytes --]

On Sun 2023-03-19 20:18:07, Christian Marangi wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> 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 <andrew@lunn.ch>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Pavel Machek <pavel@ucw.cz>

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 09/15] net: phy: marvell: Implement led_blink_set()

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 480 bytes --]

On Sun 2023-03-19 20:18:08, Christian Marangi wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> 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 <andrew@lunn.ch>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Pavel Machek <pavel@ucw.cz>

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 13/15] arm: qcom: dt: Add Switch LED for each port for rb3011

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1101 bytes --]

On Sun 2023-03-19 20:18:12, Christian Marangi wrote:
> 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 <noodles@earth.li>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Pavel Machek <pavel@ucw.cz>

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 15/15] arm: mvebu: dt: Add PHY LED support for 370-rd WAN port

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 910 bytes --]

Hi!

> From: Andrew Lunn <andrew@lunn.ch>
> 
> 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 <andrew@lunn.ch>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

> @@ -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";
> +			};
> +		};
>  	};
>  

How will this end up looking in sysfs? Should documentation be added
to Documentation/leds/leds-blinkm.rst ?

BR,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 15/15] arm: mvebu: dt: Add PHY LED support for 370-rd WAN port

[Andrew Lunn]

On Thu, Mar 23, 2023 at 01:04:53PM +0100, Pavel Machek wrote:
> Hi!
> 
> > From: Andrew Lunn <andrew@lunn.ch>
> > 
> > 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 <andrew@lunn.ch>
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> 
> > @@ -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";
> > +			};
> > +		};
> >  	};
> >  
> 
> How will this end up looking in sysfs?

Hi Pavel

It is just a plain boring LED, so it will look like all other LEDs.
There is nothing special here.

> Should documentation be added to Documentation/leds/leds-blinkm.rst
>  ?

This has nothing to do with blinkm, which appears to be an i2c LED
driver.

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 15/15] arm: mvebu: dt: Add PHY LED support for 370-rd WAN port

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1542 bytes --]

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.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > 
> > > @@ -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";
> > > +			};
> > > +		};
> > >  	};
> > >  
> > 
> > How will this end up looking in sysfs?
> 
> Hi Pavel
> 
> It is just a plain boring LED, so it will look like all other LEDs.
> There is nothing special here.

Well, AFAICT it will end up as /sys/class/leds/WAN, which is really
not what we want. (Plus the netdev trigger should be tested; we'll
need some kind of link to the ethernet device if we want this to work
on multi-ethernet systems).

> > Should documentation be added to Documentation/leds/leds-blinkm.rst
> >  ?
> 
> This has nothing to do with blinkm, which appears to be an i2c LED
> driver.

Sorry, I meant

Should documentation be added to Documentation/leds/well-known-leds.txt ?

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 15/15] arm: mvebu: dt: Add PHY LED support for 370-rd WAN port

[Andrew Lunn]

> > Hi Pavel
> > 
> > It is just a plain boring LED, so it will look like all other LEDs.
> > There is nothing special here.
> 
> Well, AFAICT it will end up as /sys/class/leds/WAN, which is really
> not what we want.

Why not? It is just a plain boring LED. It can be used for anything,
heartbeat, panic SOS in Morse code, shift lock, disk activity. Any of
the triggers can be applied to it.

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/

> (Plus the netdev trigger should be tested; we'll
> need some kind of link to the ethernet device if we want this to work
> on multi-ethernet systems).

Since this is a plain boring LED, it could actually blink for any
netdev. When we get to offloading blinking to hardware, then things
change, we need to check the netdev which is configured in the
ledtrig-netdev is the same one the PHY is associated to. But i have a
patchset for that which will appear later.

> Should documentation be added to Documentation/leds/well-known-leds.txt ?

Saying what. That there might be LEDs in your RJ45 connector, which
can be used for anything which is supported by an Linux LED trigger?

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 12/15] arm: qcom: dt: Drop unevaluated properties in switch nodes for rb3011

[Krzysztof Kozlowski]

On 19/03/2023 20:18, 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.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

ARM: dts: qcom: ipq8064-rb3011:

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 10/15] dt-bindings: net: ethernet-controller: Document support for LEDs node

[Rob Herring]

On Wed, Mar 22, 2023 at 12:39:48AM +0100, Christian Marangi wrote:
> On Wed, Mar 22, 2023 at 12:23:59AM +0100, Andrew Lunn wrote:
> > > > Are specific ethernet controllers allowed to add their own properties in 
> > > > led nodes? If so, this doesn't work. As-is, this allows any other 
> > > > properties. You need 'unevaluatedProperties: false' here to prevent 
> > > > that. But then no one can add properties. If you want to support that, 
> > > > then you need this to be a separate schema that devices can optionally 
> > > > include if they don't extend the properties, and then devices that 
> > > > extend the binding would essentially have the above with:
> > > > 
> > > > $ref: /schemas/leds/common.yaml#
> > > > unevaluatedProperties: false
> > > > properties:
> > > >   a-custom-device-prop: ...
> > > > 
> > > > 
> > > > If you wanted to define both common ethernet LED properties and 
> > > > device specific properties, then you'd need to replace leds/common.yaml 
> > > > above  with the ethernet one.
> > > > 
> > > > This is all the same reasons the DSA/switch stuff and graph bindings are 
> > > > structured the way they are.
> > > > 
> > > 
> > > Hi Rob, thanks for the review/questions.
> > > 
> > > The idea of all of this is to keep leds node as standard as possible.
> > > It was asked to add unevaluatedProperties: False but I didn't understood
> > > it was needed also for the led nodes.
> > > 
> > > leds/common.yaml have additionalProperties set to true but I guess that
> > > is not OK for the final schema and we need something more specific.
> > > 
> > > Looking at the common.yaml schema reg binding is missing so an
> > > additional schema is needed.
> > > 
> > > Reg is needed for ethernet LEDs and PHY but I think we should also permit
> > > to skip that if the device actually have just one LED. (if this wouldn't
> > > complicate the implementation. Maybe some hints from Andrew about this
> > > decision?)
> > 
> > I would make reg mandatory.
> >
> 
> Ok will add a new schema and change the regex.
> 
> > We should not encourage additional properties, but i also think we
> > cannot block it.
> > 
> > The problem we have is that there is absolutely no standardisation
> > here. Vendors are free to do whatever they want, and they do. So i
> > would not be too surprised if some vendor properties are needed
> > eventually.
> >
> 
> Think that will come later with defining a more specific schema. But I
> honestly think most of the special implementation will be handled to the
> driver internally and not with special binding in DT.

Then encourage no additional properties by letting whomever wants to add 
them to restructure the schema. ;)

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [net-next PATCH v5 10/15] dt-bindings: net: ethernet-controller: Document support for LEDs node

[Rob Herring]

On Tue, Mar 21, 2023 at 11:54:46PM +0100, Christian Marangi wrote:
> On Tue, Mar 21, 2023 at 04:19:53PM -0500, Rob Herring wrote:
> > On Sun, Mar 19, 2023 at 08:18:09PM +0100, Christian Marangi wrote:
> > > 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 <ansuelsmth@gmail.com>
> > > ---
> > >  .../bindings/net/ethernet-controller.yaml     | 21 +++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > index 00be387984ac..a93673592314 100644
> > > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > @@ -222,6 +222,27 @@ properties:
> > >          required:
> > >            - speed
> > >  
> > > +  leds:
> > > +    type: object
> > > +    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.
> > > +
> > > +    properties:
> > > +      '#address-cells':
> > > +        const: 1
> > > +
> > > +      '#size-cells':
> > > +        const: 0
> > > +
> > > +    patternProperties:
> > > +      '^led(@[a-f0-9]+)?$':
> > > +        $ref: /schemas/leds/common.yaml#
> > 
> > Are specific ethernet controllers allowed to add their own properties in 
> > led nodes? If so, this doesn't work. As-is, this allows any other 
> > properties. You need 'unevaluatedProperties: false' here to prevent 
> > that. But then no one can add properties. If you want to support that, 
> > then you need this to be a separate schema that devices can optionally 
> > include if they don't extend the properties, and then devices that 
> > extend the binding would essentially have the above with:
> > 
> > $ref: /schemas/leds/common.yaml#
> > unevaluatedProperties: false
> > properties:
> >   a-custom-device-prop: ...
> > 
> > 
> > If you wanted to define both common ethernet LED properties and 
> > device specific properties, then you'd need to replace leds/common.yaml 
> > above  with the ethernet one.
> > 
> > This is all the same reasons the DSA/switch stuff and graph bindings are 
> > structured the way they are.
> > 
> 
> Hi Rob, thanks for the review/questions.
> 
> The idea of all of this is to keep leds node as standard as possible.
> It was asked to add unevaluatedProperties: False but I didn't understood
> it was needed also for the led nodes.
> 
> leds/common.yaml have additionalProperties set to true but I guess that
> is not OK for the final schema and we need something more specific.

Yes, every node needs a schema with all possible properties and then 
'unevaluatedProperties: false' to not allow any other properties.

> Looking at the common.yaml schema reg binding is missing so an
> additional schema is needed.
> 
> Reg is needed for ethernet LEDs and PHY but I think we should also permit
> to skip that if the device actually have just one LED. (if this wouldn't
> complicate the implementation. Maybe some hints from Andrew about this
> decision?)
> 
> If we decide that reg is a must, if I understood it correctly we should
> create something like leds-ethernet.yaml that would reference common and
> add reg binding? Is it correct? This schema should be laded in leds
> directory and not in the net/ethernet.

You need 'reg' in properties, but whether it is required or not just 
depends on putting it in 'required'. I don't have a strong opinion on 
that, but generally it's only use 'reg' when there's more than 1.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel