[net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Christian Marangi]

Add Documentation for QCA808x PHYs for the additional property for the
active high LED setting and also document the LED configuration for this
PHY.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/qca,qca808x.yaml  | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qca,qca808x.yaml

diff --git a/Documentation/devicetree/bindings/net/qca,qca808x.yaml b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
new file mode 100644
index 000000000000..73cfff357311
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: GPL-2.0+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/qca,qca808x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Atheros QCA808X PHY
+
+maintainers:
+  - Christian Marangi <ansuelsmth@gmail.com>
+
+description:
+  Bindings for Qualcomm Atheros QCA808X PHYs
+
+  QCA808X PHYs can have up to 3 LEDs attached.
+  All 3 LEDs are disabled by default.
+  2 LEDs have dedicated pins with the 3rd LED having the
+  double function of Interrupt LEDs/GPIO or additional LED.
+
+  By default this special PIN is set to LED function.
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - ethernet-phy-id004d.d101
+  required:
+    - compatible
+
+properties:
+  qca,led-active-high:
+    description: Set all the LEDs to active high to be turned on.
+    type: boolean
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy@0 {
+            compatible = "ethernet-phy-id004d.d101";
+            reg = <0>;
+            qca,led-active-high;
+
+            leds {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                led@0 {
+                    reg = <0>;
+                    color = <LED_COLOR_ID_GREEN>;
+                    function = LED_FUNCTION_WAN;
+                    default-state = "keep";
+                };
+            };
+        };
+    };
-- 
2.40.1

[net-next PATCH 2/2] net: phy: at803x: add LED support for qca808x

[Christian Marangi]

Add LED support for QCA8081 PHY.

Documentation for this LEDs PHY is very scarce even with NDA access
to Documentation for OEMs. Only the blink pattern are documented and are
very confusing most of the time. No documentation is present about
forcing the LED on/off or to always blink.

Those settings were reversed by poking the regs and trying to find the
correct bits to trigger these modes. Some bits mode are not clear and
maybe the documentation option are not 100% correct. For the sake of LED
support the reversed option are enough to add support for current LED
APIs.

Supported HW control modes are:
- tx
- rx
- link10
- link100
- link1000
- link2500
- half_duplex
- full_duplex

Also add an additional property in DT to set LED polarity to active
high, "qca,led-active-high". QSDK sets this value by default but PHY
reset value doesn't have this enabled by default.

QSDK also sets 2 additional bits but their usage is not clear, info about
this is added in the header. It was verified that for correct function
of the LED if active-high is needed, only BIT 6 is needed.

A more specific PHY probe function is introduced to parse this
additional property specific for QCA8081 PHYs.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 294 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 293 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 37fb033e1c29..e4620ff9fafc 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -272,6 +272,69 @@
 #define QCA808X_CDT_STATUS_STAT_OPEN		2
 #define QCA808X_CDT_STATUS_STAT_SHORT		3
 
+#define QCA808X_MMD7_LED2_CTRL			0x8074
+#define QCA808X_MMD7_LED2_FORCE_CTRL		0x8075
+#define QCA808X_MMD7_LED1_CTRL			0x8076
+#define QCA808X_MMD7_LED1_FORCE_CTRL		0x8077
+#define QCA808X_MMD7_LED0_CTRL			0x8078
+#define QCA808X_MMD7_LED_CTRL(x)		(0x8078 - ((x) * 2))
+
+/* LED hw control pattern is the same for every LED */
+#define QCA808X_LED_PATTERN_MASK		GENMASK(15, 0)
+#define QCA808X_LED_SPEED2500_ON		BIT(15)
+#define QCA808X_LED_SPEED2500_BLINK		BIT(14)
+/* Follow blink trigger even if duplex or speed condition doesn't match */
+#define QCA808X_LED_BLINK_CHECK_BYPASS		BIT(13)
+#define QCA808X_LED_FULL_DUPLEX_ON		BIT(12)
+#define QCA808X_LED_HALF_DUPLEX_ON		BIT(11)
+#define QCA808X_LED_TX_BLINK			BIT(10)
+#define QCA808X_LED_RX_BLINK			BIT(9)
+#define QCA808X_LED_TX_ON_10MS			BIT(8)
+#define QCA808X_LED_RX_ON_10MS			BIT(7)
+#define QCA808X_LED_SPEED1000_ON		BIT(6)
+#define QCA808X_LED_SPEED100_ON			BIT(5)
+#define QCA808X_LED_SPEED10_ON			BIT(4)
+#define QCA808X_LED_COLLISION_BLINK		BIT(3)
+#define QCA808X_LED_SPEED1000_BLINK		BIT(2)
+#define QCA808X_LED_SPEED100_BLINK		BIT(1)
+#define QCA808X_LED_SPEED10_BLINK		BIT(0)
+
+#define QCA808X_MMD7_LED0_FORCE_CTRL		0x8079
+#define QCA808X_MMD7_LED_FORCE_CTRL(x)		(0x8079 - ((x) * 2))
+
+/* LED force ctrl is the same for every LED
+ * No documentation exist for this, not even internal one
+ * with NDA as QCOM gives only info about configuring
+ * hw control pattern rules and doesn't indicate any way
+ * to force the LED to specific mode.
+ * These define comes from reverse and testing and maybe
+ * lack of some info or some info are not entirely correct.
+ * For the basic LED control and hw control these finding
+ * are enough to support LED control in all the required APIs.
+ */
+#define QCA808X_LED_FORCE_MASK			GENMASK(15, 13)
+#define QCA808X_LED_FORCE_BLINK_8HZ		FIELD_PREP(QCA808X_LED_FORCE_MASK, 0x7)
+#define QCA808X_LED_FORCE_BLINK_4HZ		FIELD_PREP(QCA808X_LED_FORCE_MASK, 0x6)
+#define QCA808X_LED_FORCE_ON			FIELD_PREP(QCA808X_LED_FORCE_MASK, 0x5)
+#define QCA808X_LED_FORCE_OFF			FIELD_PREP(QCA808X_LED_FORCE_MASK, 0x4)
+/* HW control option are confusing:
+ * - 0x3 is 50% on 50% off at 4hz
+ * - 0x2 is 75% on 25% off at 4hz
+ * - 0x1 is 25% on 75% off at 4hz
+ * - 0x0 is 50% on 50% off at 8hz and is set by default
+ * This comes from visual check and may not be 100% correct.
+ */
+#define QCA808X_LED_HW_CONTROL_50_50_4HZ	FIELD_PREP(QCA808X_LED_FORCE_MASK, 0x3)
+#define QCA808X_LED_HW_CONTROL_75_25		FIELD_PREP(QCA808X_LED_FORCE_MASK, 0x2)
+#define QCA808X_LED_HW_CONTROL_25_75		FIELD_PREP(QCA808X_LED_FORCE_MASK, 0x1)
+#define QCA808X_LED_HW_CONTROL_50_50_8HZ	FIELD_PREP(QCA808X_LED_FORCE_MASK, 0x0)
+
+#define QCA808X_MMD7_LED_POLARITY_CTRL		0x901a
+/* QSDK sets by default 0x46 to this reg that sets BIT 6 for
+ * LED to active high. It's not clear what BIT 3 and BIT 4 does.
+ */
+#define QCA808X_LED_ACTIVE_HIGH			BIT(6)
+
 /* QCA808X 1G chip type */
 #define QCA808X_PHY_MMD7_CHIP_TYPE		0x901d
 #define QCA808X_PHY_CHIP_TYPE_1G		BIT(0)
@@ -312,6 +375,7 @@ struct at803x_priv {
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
 	u64 stats[ARRAY_SIZE(at803x_hw_stats)];
+	bool led_active_high;
 };
 
 struct at803x_context {
@@ -1702,6 +1766,28 @@ static int qca83xx_suspend(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca808x_parse_dt(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct at803x_priv *priv = phydev->priv;
+
+	if (of_property_read_bool(node, "qca,led-active-high"))
+		priv->led_active_high = true;
+
+	return 0;
+}
+
+static int qca808x_probe(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = at803x_probe(phydev);
+	if (ret)
+		return ret;
+
+	return qca808x_parse_dt(phydev);
+}
+
 static int qca808x_phy_fast_retrain_config(struct phy_device *phydev)
 {
 	int ret;
@@ -1765,8 +1851,17 @@ static bool qca808x_has_fast_retrain_or_slave_seed(struct phy_device *phydev)
 
 static int qca808x_config_init(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int ret;
 
+	if (priv->led_active_high) {
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
+				       QCA808X_MMD7_LED_POLARITY_CTRL,
+				       QCA808X_LED_ACTIVE_HIGH);
+		if (ret)
+			return ret;
+	}
+
 	/* Active adc&vga on 802.3az for the link 1000M and 100M */
 	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_ADDR_CLD_CTRL7,
 			QCA808X_8023AZ_AFE_CTRL_MASK, QCA808X_8023AZ_AFE_EN);
@@ -2050,6 +2145,198 @@ static void qca808x_link_change_notify(struct phy_device *phydev)
 			QCA8081_PHY_FIFO_RSTN, phydev->link ? QCA8081_PHY_FIFO_RSTN : 0);
 }
 
+static int qca808x_led_parse_netdev(struct phy_device *phydev, unsigned long rules,
+				    u16 *offload_trigger)
+{
+	/* Parsing specific to netdev trigger */
+	if (test_bit(TRIGGER_NETDEV_TX, &rules))
+		*offload_trigger |= QCA808X_LED_TX_BLINK;
+	if (test_bit(TRIGGER_NETDEV_RX, &rules))
+		*offload_trigger |= QCA808X_LED_RX_BLINK;
+	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+		*offload_trigger |= QCA808X_LED_SPEED10_ON;
+	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+		*offload_trigger |= QCA808X_LED_SPEED100_ON;
+	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+		*offload_trigger |= QCA808X_LED_SPEED1000_ON;
+	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+		*offload_trigger |= QCA808X_LED_SPEED2500_ON;
+	if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+		*offload_trigger |= QCA808X_LED_HALF_DUPLEX_ON;
+	if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+		*offload_trigger |= QCA808X_LED_FULL_DUPLEX_ON;
+
+	if (rules && !*offload_trigger)
+		return -EOPNOTSUPP;
+
+	/* Enable BLINK_CHECK_BYPASS by default to make the LED
+	 * blink even with duplex or speed mode not enabled.
+	 */
+	*offload_trigger |= QCA808X_LED_BLINK_CHECK_BYPASS;
+
+	return 0;
+}
+
+static int qca808x_led_hw_control_enable(struct phy_device *phydev, u8 index)
+{
+	u16 reg;
+
+	if (index > 2)
+		return -EINVAL;
+
+	reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
+
+	return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, reg,
+				  QCA808X_LED_FORCE_MASK);
+}
+
+static int qca808x_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	u16 offload_trigger = 0;
+
+	if (index > 2)
+		return -EINVAL;
+
+	return qca808x_led_parse_netdev(phydev, rules, &offload_trigger);
+}
+
+static int qca808x_led_hw_control_set(struct phy_device *phydev, u8 index,
+				      unsigned long rules)
+{
+	u16 reg, offload_trigger = 0;
+	int ret;
+
+	if (index > 2)
+		return -EINVAL;
+
+	reg = QCA808X_MMD7_LED_CTRL(index);
+
+	ret = qca808x_led_parse_netdev(phydev, rules, &offload_trigger);
+	if (ret)
+		return ret;
+
+	ret = qca808x_led_hw_control_enable(phydev, index);
+	if (ret)
+		return ret;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_AN, reg,
+			      QCA808X_LED_PATTERN_MASK,
+			      offload_trigger);
+}
+
+static bool qca808x_led_hw_control_status(struct phy_device *phydev, u8 index)
+{
+	u16 reg;
+	int val;
+
+	if (index > 2)
+		return false;
+
+	reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
+
+	return !(val & QCA808X_LED_FORCE_MASK);
+}
+
+static int qca808x_led_hw_control_get(struct phy_device *phydev, u8 index,
+				      unsigned long *rules)
+{
+	u16 reg;
+	int val;
+
+	if (index > 2)
+		return -EINVAL;
+
+	/* Check if we have hw control enabled */
+	if (qca808x_led_hw_control_status(phydev, index))
+		return -EINVAL;
+
+	reg = QCA808X_MMD7_LED_CTRL(index);
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
+	if (val & QCA808X_LED_TX_BLINK)
+		set_bit(TRIGGER_NETDEV_TX, rules);
+	if (val & QCA808X_LED_RX_BLINK)
+		set_bit(TRIGGER_NETDEV_RX, rules);
+	if (val & QCA808X_LED_SPEED10_ON)
+		set_bit(TRIGGER_NETDEV_LINK_10, rules);
+	if (val & QCA808X_LED_SPEED100_ON)
+		set_bit(TRIGGER_NETDEV_LINK_100, rules);
+	if (val & QCA808X_LED_SPEED1000_ON)
+		set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+	if (val & QCA808X_LED_SPEED2500_ON)
+		set_bit(TRIGGER_NETDEV_LINK_2500, rules);
+	if (val & QCA808X_LED_HALF_DUPLEX_ON)
+		set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules);
+	if (val & QCA808X_LED_FULL_DUPLEX_ON)
+		set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules);
+
+	return 0;
+}
+
+static int qca808x_led_hw_control_reset(struct phy_device *phydev, u8 index)
+{
+	u16 reg;
+
+	if (index > 2)
+		return -EINVAL;
+
+	reg = QCA808X_MMD7_LED_CTRL(index);
+
+	return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, reg,
+				  QCA808X_LED_PATTERN_MASK);
+}
+
+static int qca808x_led_brightness_set(struct phy_device *phydev,
+				      u8 index, enum led_brightness value)
+{
+	u16 reg;
+	int ret;
+
+	if (index > 2)
+		return -EINVAL;
+
+	if (!value) {
+		ret = qca808x_led_hw_control_reset(phydev, index);
+		if (ret)
+			return ret;
+	}
+
+	reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
+
+	return phy_modify_mmd(phydev, MDIO_MMD_AN, reg,
+			      QCA808X_LED_FORCE_MASK,
+			      value ? QCA808X_LED_FORCE_ON :
+				      QCA808X_LED_FORCE_OFF);
+}
+
+static int qca808x_led_blink_set(struct phy_device *phydev, u8 index,
+				 unsigned long *delay_on,
+				 unsigned long *delay_off)
+{
+	u16 reg;
+	int ret;
+
+	if (index > 2)
+		return -EINVAL;
+
+	reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
+
+	ret = phy_modify_mmd(phydev, MDIO_MMD_AN, reg,
+			     QCA808X_LED_FORCE_MASK,
+			     QCA808X_LED_FORCE_BLINK_4HZ);
+	if (ret)
+		return ret;
+
+	/* We set blink to 4Hz, aka 250ms */
+	*delay_on = 250 / 2;
+	*delay_off = 250 / 2;
+
+	return 0;
+}
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* Qualcomm Atheros AR8035 */
@@ -2210,7 +2497,7 @@ static struct phy_driver at803x_driver[] = {
 	PHY_ID_MATCH_EXACT(QCA8081_PHY_ID),
 	.name			= "Qualcomm QCA8081",
 	.flags			= PHY_POLL_CABLE_TEST,
-	.probe			= at803x_probe,
+	.probe			= qca808x_probe,
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
@@ -2227,6 +2514,11 @@ static struct phy_driver at803x_driver[] = {
 	.cable_test_start	= qca808x_cable_test_start,
 	.cable_test_get_status	= qca808x_cable_test_get_status,
 	.link_change_notify	= qca808x_link_change_notify,
+	.led_brightness_set	= qca808x_led_brightness_set,
+	.led_blink_set		= qca808x_led_blink_set,
+	.led_hw_is_supported	= qca808x_led_hw_is_supported,
+	.led_hw_control_set	= qca808x_led_hw_control_set,
+	.led_hw_control_get	= qca808x_led_hw_control_get,
 }, };
 
 module_phy_driver(at803x_driver);
-- 
2.40.1

Re: [net-next PATCH 2/2] net: phy: at803x: add LED support for qca808x

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-at803x-add-LED-support-for-qca808x/20231209-095014
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231209014828.28194-2-ansuelsmth%40gmail.com
patch subject: [net-next PATCH 2/2] net: phy: at803x: add LED support for qca808x
config: arm-randconfig-003-20231209 (https://download.01.org/0day-ci/archive/20231209/202312092051.FcBofskz-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231209/202312092051.FcBofskz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312092051.FcBofskz-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/bitops.h:68,
                    from include/linux/log2.h:12,
                    from include/asm-generic/div64.h:55,
                    from arch/arm/include/asm/div64.h:107,
                    from include/linux/math.h:6,
                    from include/linux/math64.h:6,
                    from include/linux/time64.h:5,
                    from include/linux/restart_block.h:10,
                    from include/linux/thread_info.h:14,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/arm/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/phy.h:15,
                    from drivers/net/phy/at803x.c:10:
   drivers/net/phy/at803x.c: In function 'qca808x_led_hw_control_get':
>> drivers/net/phy/at803x.c:2270:25: error: 'TRIGGER_NETDEV_LINK_2500' undeclared (first use in this function); did you mean 'TRIGGER_NETDEV_LINK_1000'?
    2270 |                 set_bit(TRIGGER_NETDEV_LINK_2500, rules);
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~
   arch/arm/include/asm/bitops.h:183:31: note: in definition of macro 'ATOMIC_BITOP'
     183 |         (__builtin_constant_p(nr) ? ____atomic_##name(nr, p) : _##name(nr,p))
         |                               ^~
   drivers/net/phy/at803x.c:2270:17: note: in expansion of macro 'set_bit'
    2270 |                 set_bit(TRIGGER_NETDEV_LINK_2500, rules);
         |                 ^~~~~~~
   drivers/net/phy/at803x.c:2270:25: note: each undeclared identifier is reported only once for each function it appears in
    2270 |                 set_bit(TRIGGER_NETDEV_LINK_2500, rules);
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~
   arch/arm/include/asm/bitops.h:183:31: note: in definition of macro 'ATOMIC_BITOP'
     183 |         (__builtin_constant_p(nr) ? ____atomic_##name(nr, p) : _##name(nr,p))
         |                               ^~
   drivers/net/phy/at803x.c:2270:17: note: in expansion of macro 'set_bit'
    2270 |                 set_bit(TRIGGER_NETDEV_LINK_2500, rules);
         |                 ^~~~~~~


vim +2270 drivers/net/phy/at803x.c

  2242	
  2243	static int qca808x_led_hw_control_get(struct phy_device *phydev, u8 index,
  2244					      unsigned long *rules)
  2245	{
  2246		u16 reg;
  2247		int val;
  2248	
  2249		if (index > 2)
  2250			return -EINVAL;
  2251	
  2252		/* Check if we have hw control enabled */
  2253		if (qca808x_led_hw_control_status(phydev, index))
  2254			return -EINVAL;
  2255	
  2256		reg = QCA808X_MMD7_LED_CTRL(index);
  2257	
  2258		val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
  2259		if (val & QCA808X_LED_TX_BLINK)
  2260			set_bit(TRIGGER_NETDEV_TX, rules);
  2261		if (val & QCA808X_LED_RX_BLINK)
  2262			set_bit(TRIGGER_NETDEV_RX, rules);
  2263		if (val & QCA808X_LED_SPEED10_ON)
  2264			set_bit(TRIGGER_NETDEV_LINK_10, rules);
  2265		if (val & QCA808X_LED_SPEED100_ON)
  2266			set_bit(TRIGGER_NETDEV_LINK_100, rules);
  2267		if (val & QCA808X_LED_SPEED1000_ON)
  2268			set_bit(TRIGGER_NETDEV_LINK_1000, rules);
  2269		if (val & QCA808X_LED_SPEED2500_ON)
> 2270			set_bit(TRIGGER_NETDEV_LINK_2500, rules);
  2271		if (val & QCA808X_LED_HALF_DUPLEX_ON)
  2272			set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules);
  2273		if (val & QCA808X_LED_FULL_DUPLEX_ON)
  2274			set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules);
  2275	
  2276		return 0;
  2277	}
  2278	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [net-next PATCH 2/2] net: phy: at803x: add LED support for qca808x

[Christian Marangi]

On Sat, Dec 09, 2023 at 08:38:38PM +0800, kernel test robot wrote:
> Hi Christian,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on net-next/main]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-at803x-add-LED-support-for-qca808x/20231209-095014
> base:   net-next/main
> patch link:    https://lore.kernel.org/r/20231209014828.28194-2-ansuelsmth%40gmail.com
> patch subject: [net-next PATCH 2/2] net: phy: at803x: add LED support for qca808x
> config: arm-randconfig-003-20231209 (https://download.01.org/0day-ci/archive/20231209/202312092051.FcBofskz-lkp@intel.com/config)
> compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231209/202312092051.FcBofskz-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312092051.FcBofskz-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from include/linux/bitops.h:68,
>                     from include/linux/log2.h:12,
>                     from include/asm-generic/div64.h:55,
>                     from arch/arm/include/asm/div64.h:107,
>                     from include/linux/math.h:6,
>                     from include/linux/math64.h:6,
>                     from include/linux/time64.h:5,
>                     from include/linux/restart_block.h:10,
>                     from include/linux/thread_info.h:14,
>                     from include/asm-generic/preempt.h:5,
>                     from ./arch/arm/include/generated/asm/preempt.h:1,
>                     from include/linux/preempt.h:79,
>                     from include/linux/spinlock.h:56,
>                     from include/linux/phy.h:15,
>                     from drivers/net/phy/at803x.c:10:
>    drivers/net/phy/at803x.c: In function 'qca808x_led_hw_control_get':
> >> drivers/net/phy/at803x.c:2270:25: error: 'TRIGGER_NETDEV_LINK_2500' undeclared (first use in this function); did you mean 'TRIGGER_NETDEV_LINK_1000'?
>     2270 |                 set_bit(TRIGGER_NETDEV_LINK_2500, rules);
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~
>    arch/arm/include/asm/bitops.h:183:31: note: in definition of macro 'ATOMIC_BITOP'
>      183 |         (__builtin_constant_p(nr) ? ____atomic_##name(nr, p) : _##name(nr,p))
>          |                               ^~
>    drivers/net/phy/at803x.c:2270:17: note: in expansion of macro 'set_bit'
>     2270 |                 set_bit(TRIGGER_NETDEV_LINK_2500, rules);
>          |                 ^~~~~~~
>    drivers/net/phy/at803x.c:2270:25: note: each undeclared identifier is reported only once for each function it appears in
>     2270 |                 set_bit(TRIGGER_NETDEV_LINK_2500, rules);
>          |                         ^~~~~~~~~~~~~~~~~~~~~~~~
>    arch/arm/include/asm/bitops.h:183:31: note: in definition of macro 'ATOMIC_BITOP'
>      183 |         (__builtin_constant_p(nr) ? ____atomic_##name(nr, p) : _##name(nr,p))
>          |                               ^~
>    drivers/net/phy/at803x.c:2270:17: note: in expansion of macro 'set_bit'
>     2270 |                 set_bit(TRIGGER_NETDEV_LINK_2500, rules);
>          |                 ^~~~~~~
> 
> 
> vim +2270 drivers/net/phy/at803x.c

Hi,
this error is caused by the lack of the commits for the recently added
support for additional link speed in the netdev LED trigger.

These additional modes has been merged in Lee tree but I guess we need
an immutable branch for net-next to actually use them?

> 
>   2242	
>   2243	static int qca808x_led_hw_control_get(struct phy_device *phydev, u8 index,
>   2244					      unsigned long *rules)
>   2245	{
>   2246		u16 reg;
>   2247		int val;
>   2248	
>   2249		if (index > 2)
>   2250			return -EINVAL;
>   2251	
>   2252		/* Check if we have hw control enabled */
>   2253		if (qca808x_led_hw_control_status(phydev, index))
>   2254			return -EINVAL;
>   2255	
>   2256		reg = QCA808X_MMD7_LED_CTRL(index);
>   2257	
>   2258		val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
>   2259		if (val & QCA808X_LED_TX_BLINK)
>   2260			set_bit(TRIGGER_NETDEV_TX, rules);
>   2261		if (val & QCA808X_LED_RX_BLINK)
>   2262			set_bit(TRIGGER_NETDEV_RX, rules);
>   2263		if (val & QCA808X_LED_SPEED10_ON)
>   2264			set_bit(TRIGGER_NETDEV_LINK_10, rules);
>   2265		if (val & QCA808X_LED_SPEED100_ON)
>   2266			set_bit(TRIGGER_NETDEV_LINK_100, rules);
>   2267		if (val & QCA808X_LED_SPEED1000_ON)
>   2268			set_bit(TRIGGER_NETDEV_LINK_1000, rules);
>   2269		if (val & QCA808X_LED_SPEED2500_ON)
> > 2270			set_bit(TRIGGER_NETDEV_LINK_2500, rules);
>   2271		if (val & QCA808X_LED_HALF_DUPLEX_ON)
>   2272			set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules);
>   2273		if (val & QCA808X_LED_FULL_DUPLEX_ON)
>   2274			set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules);
>   2275	
>   2276		return 0;
>   2277	}
>   2278	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

-- 
	Ansuel

Re: [net-next PATCH 2/2] net: phy: at803x: add LED support for qca808x

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-phy-at803x-add-LED-support-for-qca808x/20231209-095014
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231209014828.28194-2-ansuelsmth%40gmail.com
patch subject: [net-next PATCH 2/2] net: phy: at803x: add LED support for qca808x
config: x86_64-randconfig-r132-20231209 (https://download.01.org/0day-ci/archive/20231210/202312100229.wXUmV6MA-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231210/202312100229.wXUmV6MA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312100229.wXUmV6MA-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/phy/at803x.c:2270:11: error: use of undeclared identifier 'TRIGGER_NETDEV_LINK_2500'
                   set_bit(TRIGGER_NETDEV_LINK_2500, rules);
                           ^
   1 error generated.


vim +/TRIGGER_NETDEV_LINK_2500 +2270 drivers/net/phy/at803x.c

  2242	
  2243	static int qca808x_led_hw_control_get(struct phy_device *phydev, u8 index,
  2244					      unsigned long *rules)
  2245	{
  2246		u16 reg;
  2247		int val;
  2248	
  2249		if (index > 2)
  2250			return -EINVAL;
  2251	
  2252		/* Check if we have hw control enabled */
  2253		if (qca808x_led_hw_control_status(phydev, index))
  2254			return -EINVAL;
  2255	
  2256		reg = QCA808X_MMD7_LED_CTRL(index);
  2257	
  2258		val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
  2259		if (val & QCA808X_LED_TX_BLINK)
  2260			set_bit(TRIGGER_NETDEV_TX, rules);
  2261		if (val & QCA808X_LED_RX_BLINK)
  2262			set_bit(TRIGGER_NETDEV_RX, rules);
  2263		if (val & QCA808X_LED_SPEED10_ON)
  2264			set_bit(TRIGGER_NETDEV_LINK_10, rules);
  2265		if (val & QCA808X_LED_SPEED100_ON)
  2266			set_bit(TRIGGER_NETDEV_LINK_100, rules);
  2267		if (val & QCA808X_LED_SPEED1000_ON)
  2268			set_bit(TRIGGER_NETDEV_LINK_1000, rules);
  2269		if (val & QCA808X_LED_SPEED2500_ON)
> 2270			set_bit(TRIGGER_NETDEV_LINK_2500, rules);
  2271		if (val & QCA808X_LED_HALF_DUPLEX_ON)
  2272			set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules);
  2273		if (val & QCA808X_LED_FULL_DUPLEX_ON)
  2274			set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules);
  2275	
  2276		return 0;
  2277	}
  2278	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Krzysztof Kozlowski]

On 09/12/2023 02:48, Christian Marangi wrote:
> Add Documentation for QCA808x PHYs for the additional property for the
> active high LED setting and also document the LED configuration for this
> PHY.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/qca,qca808x.yaml  | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qca,qca808x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/qca,qca808x.yaml b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> new file mode 100644
> index 000000000000..73cfff357311
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0+

Dual license as checkpath and writing-bindings ask.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/qca,qca808x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Atheros QCA808X PHY
> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +
> +description:
> +  Bindings for Qualcomm Atheros QCA808X PHYs

Drop "Bindings for" and then entire sentence seems not useful.

> +
> +  QCA808X PHYs can have up to 3 LEDs attached.
> +  All 3 LEDs are disabled by default.
> +  2 LEDs have dedicated pins with the 3rd LED having the
> +  double function of Interrupt LEDs/GPIO or additional LED.
> +
> +  By default this special PIN is set to LED function.
> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - ethernet-phy-id004d.d101

I have impression that this is continuation of some other patchset...
Anyway, id004d.d101 is specific to QCA808x?

> +  required:
> +    - compatible
> +
> +properties:
> +  qca,led-active-high:
> +    description: Set all the LEDs to active high to be turned on.
> +    type: boolean


Best regards,
Krzysztof

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Christian Marangi]

On Mon, Dec 11, 2023 at 11:19:50AM +0100, Krzysztof Kozlowski wrote:
> On 09/12/2023 02:48, Christian Marangi wrote:
> > Add Documentation for QCA808x PHYs for the additional property for the
> > active high LED setting and also document the LED configuration for this
> > PHY.
> > 
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
>

Yes sorry.

> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/net/qca,qca808x.yaml  | 66 +++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/qca,qca808x.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/qca,qca808x.yaml b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> > new file mode 100644
> > index 000000000000..73cfff357311
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> > @@ -0,0 +1,66 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> 
> Dual license as checkpath and writing-bindings ask.
> 

Oh didn't notice the warning.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/qca,qca808x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Atheros QCA808X PHY
> > +
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@gmail.com>
> > +
> > +description:
> > +  Bindings for Qualcomm Atheros QCA808X PHYs
> 
> Drop "Bindings for" and then entire sentence seems not useful.
> 

Was following the pattern used for other qcom PHY. Ok will drop!

> > +
> > +  QCA808X PHYs can have up to 3 LEDs attached.
> > +  All 3 LEDs are disabled by default.
> > +  2 LEDs have dedicated pins with the 3rd LED having the
> > +  double function of Interrupt LEDs/GPIO or additional LED.
> > +
> > +  By default this special PIN is set to LED function.
> > +
> > +allOf:
> > +  - $ref: ethernet-phy.yaml#
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - ethernet-phy-id004d.d101
> 
> I have impression that this is continuation of some other patchset...
> Anyway, id004d.d101 is specific to QCA808x?
> 

I used enum assuming eventually more qca808x PHY will come... Yes that
ID is specific and it's the id of QCA8081. Better to use const?

> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  qca,led-active-high:
> > +    description: Set all the LEDs to active high to be turned on.
> > +    type: boolean
> 
> 
> Best regards,
> Krzysztof
> 

-- 
	Ansuel

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Andrew Lunn]

> +properties:
> +  qca,led-active-high:
> +    description: Set all the LEDs to active high to be turned on.
> +    type: boolean

I would of expected active high is the default. An active low property
would make more sense. It should also be a generic property, not a
vendor property. As such, we either want the phylib core to parse it,
or the LED core.

       Andrew

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Christian Marangi]

On Mon, Dec 11, 2023 at 04:44:06PM +0100, Andrew Lunn wrote:
> > +properties:
> > +  qca,led-active-high:
> > +    description: Set all the LEDs to active high to be turned on.
> > +    type: boolean
> 
> I would of expected active high is the default. An active low property
> would make more sense. It should also be a generic property, not a
> vendor property. As such, we either want the phylib core to parse it,
> or the LED core.
>

Mhhh with a generic property and LED core or phylib handling it... How
it would work applying that setting on PHY side?

Adding the check to the set_brightness set_blink hw_control API?

-- 
	Ansuel

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Christian Marangi]

On Mon, Dec 11, 2023 at 04:44:06PM +0100, Andrew Lunn wrote:
> > +properties:
> > +  qca,led-active-high:
> > +    description: Set all the LEDs to active high to be turned on.
> > +    type: boolean
> 
> I would of expected active high is the default. An active low property
> would make more sense. It should also be a generic property, not a
> vendor property. As such, we either want the phylib core to parse it,
> or the LED core.
>

Also sorry for the double email... Any help for the problem of the
missing link_2500 define in net-next? (merged in Lee tree?)

-- 
	Ansuel

Re: [net-next PATCH 2/2] net: phy: at803x: add LED support for qca808x

[Andrew Lunn]

> Hi,
> this error is caused by the lack of the commits for the recently added
> support for additional link speed in the netdev LED trigger.
> 
> These additional modes has been merged in Lee tree but I guess we need
> an immutable branch for net-next to actually use them?

Yes. We cannot have build failures in net-next.

     Andrew

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Andrew Lunn]

On Mon, Dec 11, 2023 at 04:49:00PM +0100, Christian Marangi wrote:
> On Mon, Dec 11, 2023 at 04:44:06PM +0100, Andrew Lunn wrote:
> > > +properties:
> > > +  qca,led-active-high:
> > > +    description: Set all the LEDs to active high to be turned on.
> > > +    type: boolean
> > 
> > I would of expected active high is the default. An active low property
> > would make more sense. It should also be a generic property, not a
> > vendor property. As such, we either want the phylib core to parse it,
> > or the LED core.
> >
> 
> Also sorry for the double email... Any help for the problem of the
> missing link_2500 define in net-next? (merged in Lee tree?)

You need to email Lee and Jakub, ask for a stable branch which can be
pulled into net-next.

       Andrew

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Andrew Lunn]

> Mhhh with a generic property and LED core or phylib handling it... How
> it would work applying that setting on PHY side?

Add a .led_set_polarity callback to the PHY driver structure?

Take a look at other LED drivers. Does anything similar already exist?
It is unlikely that PHYs are the only sort of LED to have a polarity.

   Andrew

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Christian Marangi]

On Mon, Dec 11, 2023 at 04:51:56PM +0100, Andrew Lunn wrote:
> On Mon, Dec 11, 2023 at 04:49:00PM +0100, Christian Marangi wrote:
> > On Mon, Dec 11, 2023 at 04:44:06PM +0100, Andrew Lunn wrote:
> > > > +properties:
> > > > +  qca,led-active-high:
> > > > +    description: Set all the LEDs to active high to be turned on.
> > > > +    type: boolean
> > > 
> > > I would of expected active high is the default. An active low property
> > > would make more sense. It should also be a generic property, not a
> > > vendor property. As such, we either want the phylib core to parse it,
> > > or the LED core.
> > >
> > 
> > Also sorry for the double email... Any help for the problem of the
> > missing link_2500 define in net-next? (merged in Lee tree?)
> 
> You need to email Lee and Jakub, ask for a stable branch which can be
> pulled into net-next.
>

Thanks I sent a followup email to the merged series.

-- 
	Ansuel

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Krzysztof Kozlowski]

On 11/12/2023 13:18, Christian Marangi wrote:
>>> +  QCA808X PHYs can have up to 3 LEDs attached.
>>> +  All 3 LEDs are disabled by default.
>>> +  2 LEDs have dedicated pins with the 3rd LED having the
>>> +  double function of Interrupt LEDs/GPIO or additional LED.
>>> +
>>> +  By default this special PIN is set to LED function.
>>> +
>>> +allOf:
>>> +  - $ref: ethernet-phy.yaml#
>>> +
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        enum:
>>> +          - ethernet-phy-id004d.d101
>>
>> I have impression that this is continuation of some other patchset...
>> Anyway, id004d.d101 is specific to QCA808x?
>>
> 
> I used enum assuming eventually more qca808x PHY will come... Yes that
> ID is specific and it's the id of QCA8081. Better to use const?

No, it is fine. I just wanted to be sure that this will not be matched
by other bindings.

Best regards,
Krzysztof

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Christian Marangi]

On Mon, Dec 11, 2023 at 04:54:12PM +0100, Andrew Lunn wrote:
> > Mhhh with a generic property and LED core or phylib handling it... How
> > it would work applying that setting on PHY side?
> 
> Add a .led_set_polarity callback to the PHY driver structure?
> 
> Take a look at other LED drivers. Does anything similar already exist?
> It is unlikely that PHYs are the only sort of LED to have a polarity.
>

Interesting topic... With a quick grep on Documentation for polarity of
high, I can't find any use of it...

Also main problem is that the thing is controlled globally and not per
LED. (can be handled internally to the driver with some priv and check
magic)

Is it worth to impemement the additional API to control this? And I
guess a egenric binding should be added to ethernet-phy? Or should it be
added to LEDs?

-- 
	Ansuel

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Andrew Lunn]

On Mon, Dec 11, 2023 at 06:19:46PM +0100, Christian Marangi wrote:
> On Mon, Dec 11, 2023 at 04:54:12PM +0100, Andrew Lunn wrote:
> > > Mhhh with a generic property and LED core or phylib handling it... How
> > > it would work applying that setting on PHY side?
> > 
> > Add a .led_set_polarity callback to the PHY driver structure?
> > 
> > Take a look at other LED drivers. Does anything similar already exist?
> > It is unlikely that PHYs are the only sort of LED to have a polarity.
> >
> 
> Interesting topic... With a quick grep on Documentation for polarity of
> high, I can't find any use of it...

As i said, active-high is the default. So there is no need to specify
it. But if you look in Documentation/devicetree/binding/leds for
'active-low' you will find a few examples.

> Also main problem is that the thing is controlled globally and not per
> LED. (can be handled internally to the driver with some priv and check
> magic)

Ah, missed that. Marvell PHYs have polarity per LED.

It would be better to describe this correctly, so one property at a
higher level. We can then in the future add an 'active-low' property
per PHY.

> Is it worth to impemement the additional API to control this? And I
> guess a egenric binding should be added to ethernet-phy? Or should it be
> added to LEDs?

Since it is above individual LEDs, i would not add it to the generic
LED binding. But it could go inside the leds object of
ethernet-phy.yaml.

           leds {
                #address-cells = <1>;
                #size-cells = <0>;

		active-low;

                led@0 {
                    reg = <0>;
                    color = <LED_COLOR_ID_WHITE>;
                    function = LED_FUNCTION_LAN;
                    default-state = "keep";
                };

	Andrew

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Christian Marangi]

On Mon, Dec 11, 2023 at 06:49:02PM +0100, Andrew Lunn wrote:
> On Mon, Dec 11, 2023 at 06:19:46PM +0100, Christian Marangi wrote:
> > On Mon, Dec 11, 2023 at 04:54:12PM +0100, Andrew Lunn wrote:
> > > > Mhhh with a generic property and LED core or phylib handling it... How
> > > > it would work applying that setting on PHY side?
> > > 
> > > Add a .led_set_polarity callback to the PHY driver structure?
> > > 
> > > Take a look at other LED drivers. Does anything similar already exist?
> > > It is unlikely that PHYs are the only sort of LED to have a polarity.
> > >
> > 
> > Interesting topic... With a quick grep on Documentation for polarity of
> > high, I can't find any use of it...
> 
> As i said, active-high is the default. So there is no need to specify
> it. But if you look in Documentation/devicetree/binding/leds for
> 'active-low' you will find a few examples.
>

Yes I was searching more and I just notice active-low and led-active-low
usage for bcm6358.

> > Also main problem is that the thing is controlled globally and not per
> > LED. (can be handled internally to the driver with some priv and check
> > magic)
> 
> Ah, missed that. Marvell PHYs have polarity per LED.
> 
> It would be better to describe this correctly, so one property at a
> higher level. We can then in the future add an 'active-low' property
> per PHY.
> 
> > Is it worth to impemement the additional API to control this? And I
> > guess a egenric binding should be added to ethernet-phy? Or should it be
> > added to LEDs?
> 
> Since it is above individual LEDs, i would not add it to the generic
> LED binding. But it could go inside the leds object of
> ethernet-phy.yaml.
> 
>            leds {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
> 		active-low;
> 
>                 led@0 {
>                     reg = <0>;
>                     color = <LED_COLOR_ID_WHITE>;
>                     function = LED_FUNCTION_LAN;
>                     default-state = "keep";
>                 };
>

Ok! And I guess the additional API will (initially to be later expanded
for other usage?) take this value and call the set polarity based on
this correct? 

bool active_low = of_property_read_bool(leds_node, "active-low");

.led_set_polarity(struct phy_device *phydev, bool active_low);


Maybe a more flexible approach might be scan for both. (either in leds
node or in the led subnode)

.led_set_polarity(struct phy_device *phydev, int index, bool active_low);

Where index is -1 if it's global and the led index if it's in the led
node?

PHY driver will know what to ignore/use as I can't immagine to have a
PHY that have both global and per LED polarity. What do you think?

-- 
	Ansuel

Re: [net-next PATCH 1/2] dt-bindings: Document QCA808x PHYs

[Rob Herring]

On Sat, Dec 09, 2023 at 02:48:27AM +0100, Christian Marangi wrote:
> Add Documentation for QCA808x PHYs for the additional property for the
> active high LED setting and also document the LED configuration for this
> PHY.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/qca,qca808x.yaml  | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qca,qca808x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/qca,qca808x.yaml b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> new file mode 100644
> index 000000000000..73cfff357311
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qca,qca808x.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0+

Dual license as checkpatch.pl points out.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/qca,qca808x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Atheros QCA808X PHY
> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +
> +description:
> +  Bindings for Qualcomm Atheros QCA808X PHYs
> +
> +  QCA808X PHYs can have up to 3 LEDs attached.
> +  All 3 LEDs are disabled by default.
> +  2 LEDs have dedicated pins with the 3rd LED having the
> +  double function of Interrupt LEDs/GPIO or additional LED.
> +
> +  By default this special PIN is set to LED function.
> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - ethernet-phy-id004d.d101

Move this to properties and drop the select.

> +  required:
> +    - compatible
> +
> +properties:
> +  qca,led-active-high:
> +    description: Set all the LEDs to active high to be turned on.
> +    type: boolean
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy@0 {
> +            compatible = "ethernet-phy-id004d.d101";
> +            reg = <0>;
> +            qca,led-active-high;
> +
> +            leds {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                led@0 {
> +                    reg = <0>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    function = LED_FUNCTION_WAN;
> +                    default-state = "keep";
> +                };
> +            };
> +        };
> +    };
> -- 
> 2.40.1
>