[PATCH net-next 0/3] net: phy: dap8211r: add LED hardware control

[PATCH net-next 0/3] net: phy: dap8211r: add LED hardware control

[Artem Shimko]

Hello,

This series adds support for hardware-controlled LEDs on the DAP8211R
Gigabit Ethernet PHY.

The first patch documents the optional "leds" node in the Device Tree
binding. The second patch introduces a small helper to write extended
registers, since the LED registers are accessed indirectly via
EXT_ADD/EXT_DATA. The third patch implements the three LED callbacks
required by the PHY library.

$ make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  CHKDT   ./Documentation/devicetree/bindings
  LINT    ./Documentation/devicetree/bindings
  STYLE   ./Documentation/devicetree/bindings
  DTEX    Documentation/devicetree/bindings/net/dptel,dap8211r.example.dts
  DTC [C] Documentation/devicetree/bindings/net/dptel,dap8211r.example.dtb

Required:
CONFIG_NEW_LEDS=y
    CONFIG_LEDS_CLASS=y
    CONFIG_LEDS_TRIGGERS=y
        CONFIG_LEDS_TRIGGER_NETDEV=y

Testing:
$ ip link set eth1 up
$ ls /sys/class/leds/
stmmac-1:01:amber:lan  stmmac-1:01:green:lan
$ echo eth1 > /sys/class/leds/stmmac-1:01:green:lan/device_name
$ echo 1 > /sys/class/leds/stmmac-1:01:green:lan/link_1000
$ cat /sys/class/leds/stmmac-1:01:green:lan/offloaded

$ echo eth1 > /sys/class/leds/stmmac-1:01:amber:lan/device_name
$ echo 1 > /sys/class/leds/stmmac-1:01:amber:lan/tx
$ echo 1 > /sys/class/leds/stmmac-1:01:amber:lan/rx
$ cat /sys/class/leds/stmmac-1:01:amber:lan/offloaded

$ ip addr add PEER_IP_ADDR dev eth1
$ ping -I eth1 ELP_ID_ADDR

--
Best regards,
artem shimko

Artem Shimko (3):
  dt-bindings: net: dptel,dap8211r: add led nodes
  net: phy: dap8211r: add extended register write helper
  net: phy: dap8211r: add led hardware control support

 .../bindings/net/dptel,dap8211r.yaml          |  38 ++++
 drivers/net/phy/dap8211r.c                    | 207 ++++++++++++++++++
 2 files changed, 245 insertions(+)

-- 
2.43.0

[PATCH net-next 1/3] dt-bindings: net: dptel,dap8211r: add led nodes

[Artem Shimko]

Document the optional "leds" node supported by the dap8211r phy.

The phy exposes three configurable led outputs, each of which can be
assigned a color, a function and a default trigger.

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
 .../bindings/net/dptel,dap8211r.yaml          | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml b/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
index 4cd0f9730d95..43425fc8e96e 100644
--- a/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
+++ b/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
@@ -20,6 +20,7 @@ description: |
     - IEEE 802.3az-2010 Energy Efficient Ethernet
     - IEEE 1588 SyncE support
     - Internal packet generator and checker for link diagnostics
+    - Three configurable LEDs (link, speed, duplex, TX/RX activity)
 
 allOf:
   - $ref: ethernet-phy.yaml#
@@ -28,6 +29,22 @@ properties:
   compatible:
     const: ethernet-phy-id0008.011b
 
+  leds:
+    type: object
+    description:
+      Optional node describing the three configurable LEDs of the PHY.
+      Each LED can be assigned a color, function and default trigger
+      (e.g. "netdev" for link/activity indication).
+    patternProperties:
+      '^led@[0-2]$':
+        type: object
+        properties:
+          reg:
+            maxItems: 1
+        required:
+          - reg
+        additionalProperties: true
+
   reg:
     maxItems: 1
 
@@ -49,6 +66,8 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/leds/common.h>
+
     mdio {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -58,5 +77,24 @@ examples:
             reg = <1>;
             rx-internal-delay-ps = <1950>;
             tx-internal-delay-ps = <1950>;
+
+            leds {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                led@0 {
+                    reg = <0>;
+                    color = <LED_COLOR_ID_GREEN>;
+                    function = LED_FUNCTION_LAN;
+                    linux,default-trigger = "netdev";
+                };
+
+                led@1 {
+                    reg = <1>;
+                    color = <LED_COLOR_ID_AMBER>;
+                    function = LED_FUNCTION_LAN;
+                    linux,default-trigger = "netdev";
+                };
+            };
         };
     };
-- 
2.43.0

Re: [PATCH net-next 1/3] dt-bindings: net: dptel,dap8211r: add led nodes

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

On Fri, Nov 13, 2026 at 02:05:43PM +0300, Artem Shimko wrote:
> Document the optional "leds" node supported by the dap8211r phy.
> 
> The phy exposes three configurable led outputs, each of which can be
> assigned a color, a function and a default trigger.
> 
> Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
> ---
>  .../bindings/net/dptel,dap8211r.yaml          | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml b/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
> index 4cd0f9730d95..43425fc8e96e 100644
> --- a/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
> +++ b/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
> @@ -20,6 +20,7 @@ description: |
>      - IEEE 802.3az-2010 Energy Efficient Ethernet
>      - IEEE 1588 SyncE support
>      - Internal packet generator and checker for link diagnostics
> +    - Three configurable LEDs (link, speed, duplex, TX/RX activity)
>  
>  allOf:
>    - $ref: ethernet-phy.yaml#
> @@ -28,6 +29,22 @@ properties:
>    compatible:
>      const: ethernet-phy-id0008.011b
>  
> +  leds:
> +    type: object
> +    description:
> +      Optional node describing the three configurable LEDs of the PHY.
> +      Each LED can be assigned a color, function and default trigger
> +      (e.g. "netdev" for link/activity indication).

Please add some blank lines here,

> +    patternProperties:
> +      '^led@[0-2]$':
> +        type: object

here,

> +        properties:
> +          reg:
> +            maxItems: 1

here,

> +        required:
> +          - reg

and here.

> +        additionalProperties: true

This whole leds node needs to go down below tx-internal-delay-ps, child
nodes are typically sorted after regular properties.

pw-bot: changes-requested

Thanks,
Conor.

> +
>    reg:
>      maxItems: 1
>  
> @@ -49,6 +66,8 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/leds/common.h>
> +
>      mdio {
>          #address-cells = <1>;
>          #size-cells = <0>;
> @@ -58,5 +77,24 @@ examples:
>              reg = <1>;
>              rx-internal-delay-ps = <1950>;
>              tx-internal-delay-ps = <1950>;
> +
> +            leds {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                led@0 {
> +                    reg = <0>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    function = LED_FUNCTION_LAN;
> +                    linux,default-trigger = "netdev";
> +                };
> +
> +                led@1 {
> +                    reg = <1>;
> +                    color = <LED_COLOR_ID_AMBER>;
> +                    function = LED_FUNCTION_LAN;
> +                    linux,default-trigger = "netdev";
> +                };
> +            };
>          };
>      };
> -- 
> 2.43.0
> 

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

Re: [PATCH net-next 1/3] dt-bindings: net: dptel,dap8211r: add led nodes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] dt-bindings: net: dptel,dap8211r: invalid and ineffective
  leds node schema
--

commit b253b1562298deb66ca7a0d9739c668ef65e6592
Author: Artem Shimko <a.shimko.dev@gmail.com>

dt-bindings: net: dptel,dap8211r: add led nodes

This commit documents the optional leds node and adds an example for the
DAP8211R PHY binding.

> diff --git a/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml b/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
> index 4cd0f9730d954..43425fc8e96e0 100644
> --- a/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml
> +++ b/Documentation/devicetree/bindings/net/dptel,dap8211r.yaml

[ ... ]

> @@ -28,6 +29,22 @@ properties:
>    compatible:
>      const: ethernet-phy-id0008.011b
>  
> +  leds:
> +    type: object
> +    description:
> +      Optional node describing the three configurable LEDs of the PHY.
> +      Each LED can be assigned a color, function and default trigger
> +      (e.g. "netdev" for link/activity indication).
> +    patternProperties:
> +      '^led@[0-2]$':
> +        type: object
> +        properties:
> +          reg:
> +            maxItems: 1
> +        required:
> +          - reg
> +        additionalProperties: true

[Severity: Medium]
Does this leds schema pass dt_binding_check or enforce the 0..2 LED index
limit?

The leds object omits additionalProperties: false (as well as
#address-cells and #size-cells), and ^led@[0-2]$ sets
additionalProperties: true without a compatible property, both of which
fail DT meta-schema validation.

Because ethernet-phy.yaml# is included in allOf and already defines leds
with ^led@[a-f0-9]+$, leaving additionalProperties unset on leds allows
child nodes outside ^led@[0-2]$ (such as led@3) to pass validation via
ethernet-phy.yaml#, while reg under ^led@[0-2]$ only specifies maxItems: 1
without restricting the value to 0..2.

Either drop the leds property block to inherit the definition from
ethernet-phy.yaml#, or define #address-cells, #size-cells, and
additionalProperties: false on leds and reference
/schemas/leds/common.yaml# with unevaluatedProperties: false and
maximum: 2 on reg under ^led@[0-2]$.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113110546.173624-1-a.shimko.dev@gmail.com?part=1

[PATCH net-next 2/3] net: phy: dap8211r: add extended register write helper

[Artem Shimko]

Add dap8211r_write_ext() to write extended registers via the indirect
access registers of the phy.

This will be used to write extended registers of the dap8211r.

No functional change.

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
 drivers/net/phy/dap8211r.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
index 0bf770f8ebbb..4eb50c26397f 100644
--- a/drivers/net/phy/dap8211r.c
+++ b/drivers/net/phy/dap8211r.c
@@ -88,6 +88,32 @@ static int dap8211r_read_ext(struct phy_device *phydev, u16 reg)
 	return ret;
 }
 
+/**
+ * dap8211r_write_ext() - Write extended register
+ * @phydev: PHY device structure
+ * @reg: Extended register address
+ * @data: Data to write
+ *
+ * Writes a PHY extended register using the indirect access method.
+ * The caller must hold the MDIO bus lock.
+ *
+ * Return: 0 on success, or negative error code
+ */
+static int dap8211r_write_ext(struct phy_device *phydev, u16 reg, u16 data)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = __phy_write(phydev, DAP8211R_EXT_ADD, reg);
+	if (ret < 0)
+		goto out;
+
+	ret = __phy_write(phydev, DAP8211R_EXT_DATA, data);
+out:
+	phy_unlock_mdio_bus(phydev);
+	return ret;
+}
+
 /**
  * dap8211r_modify_ext() - Modify extended register bits
  * @phydev: PHY device structure
-- 
2.43.0

Re: [PATCH net-next 2/3] net: phy: dap8211r: add extended register write helper

[Andrew Lunn]

> + * The caller must hold the MDIO bus lock.
> + *
> + * Return: 0 on success, or negative error code
> + */
> +static int dap8211r_write_ext(struct phy_device *phydev, u16 reg, u16 data)
> +{
> +	int ret;
> +
> +	phy_lock_mdio_bus(phydev);

How does this not deadlock, if the caller is already holding the MDIO
bus lock, as commented on above?

	Andrew

Re: [PATCH net-next 2/3] net: phy: dap8211r: add extended register write helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] net: phy: dap8211r: inverted locking comment on
  dap8211r_write_ext()
--

commit 80c461dbd61c7143cfae371b80c6ab5797de75b3
Author: Artem Shimko <a.shimko.dev@gmail.com>

net: phy: dap8211r: add extended register write helper

This commit adds the dap8211r_write_ext() helper to write extended PHY
registers via indirect addressing.

> diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
> index 0bf770f8ebbb4..4eb50c26397fb 100644
> --- a/drivers/net/phy/dap8211r.c
> +++ b/drivers/net/phy/dap8211r.c
> @@ -88,6 +88,32 @@ static int dap8211r_read_ext(struct phy_device *phydev, u16 reg)
>  	return ret;
>  }
>  
> +/**
> + * dap8211r_write_ext() - Write extended register
> + * @phydev: PHY device structure
> + * @reg: Extended register address
> + * @data: Data to write
> + *
> + * Writes a PHY extended register using the indirect access method.
> + * The caller must hold the MDIO bus lock.
> + *
> + * Return: 0 on success, or negative error code
> + */
> +static int dap8211r_write_ext(struct phy_device *phydev, u16 reg, u16 data)
> +{
> +	int ret;
> +
> +	phy_lock_mdio_bus(phydev);
> +	ret = __phy_write(phydev, DAP8211R_EXT_ADD, reg);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = __phy_write(phydev, DAP8211R_EXT_DATA, data);
> +out:
> +	phy_unlock_mdio_bus(phydev);
> +	return ret;
> +}

[Severity: Low]
Does the kernel-doc comment for dap8211r_write_ext() state the opposite of
its locking requirement?  dap8211r_write_ext() takes the MDIO bus mutex via
phy_lock_mdio_bus(phydev), so a caller that already holds the lock will
deadlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113110546.173624-1-a.shimko.dev@gmail.com?part=2

[PATCH net-next 3/3] net: phy: dap8211r: add led hardware control support

[Artem Shimko]

Add support for hardware-controlled leds on the dap8211r phy. The chip
exposes three led that can be programmed to react to link, speed,
duplex, and tx/rx activity.

Implement callbacks for led_hw_is_supported(), led_hw_control_set()
led_hw_control_get().

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
 drivers/net/phy/dap8211r.c | 181 +++++++++++++++++++++++++++++++++++++
 1 file changed, 181 insertions(+)

diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
index 4eb50c26397f..2e371c231d32 100644
--- a/drivers/net/phy/dap8211r.c
+++ b/drivers/net/phy/dap8211r.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/bitops.h>
 #include <linux/errno.h>
 #include <linux/ethtool.h>
 #include <linux/kernel.h>
@@ -57,6 +58,33 @@
  */
 #define DAP8211R_DEFAULT_DEL_SEL	0xD
 
+#define DAP8211R_LED_ID_0		0
+#define DAP8211R_LED_ID_1		1
+#define DAP8211R_LED_ID_2		2
+#define DAP8211R_LED_0			0xA00C
+#define DAP8211R_LED_1			0xA00D
+#define DAP8211R_LED_2			0xA00E
+#define DAP8211R_LED_ACT_BLINK		BIT(13)
+#define DAP8211R_LED_FULL_DUPLEX	BIT(12)
+#define DAP8211R_LED_HALF_DUPLEX	BIT(11)
+#define DAP8211R_LED_TX			BIT(10)
+#define DAP8211R_LED_RX			BIT(9)
+#define DAP8211R_LED_LINK_1000		BIT(6)
+#define DAP8211R_LED_LINK_100		BIT(5)
+#define DAP8211R_LED_LINK_10		BIT(4)
+
+#define DAP8211R_LED_LINK		(DAP8211R_LED_LINK_10	| \
+					DAP8211R_LED_LINK_100	| \
+					DAP8211R_LED_LINK_1000)
+
+#define DAP8211R_LED_SUPPORTED_TRG	(BIT(TRIGGER_NETDEV_LINK_10)	| \
+					BIT(TRIGGER_NETDEV_LINK_100)	| \
+					BIT(TRIGGER_NETDEV_LINK_1000)	| \
+					BIT(TRIGGER_NETDEV_TX)		| \
+					BIT(TRIGGER_NETDEV_RX)		| \
+					BIT(TRIGGER_NETDEV_HALF_DUPLEX)	| \
+					BIT(TRIGGER_NETDEV_FULL_DUPLEX))
+
 static const int dap8211r_internal_delay[] = {0, 150, 300, 450, 600, 750, 900,
 					      1050, 1200, 1350, 1500, 1650, 1800,
 					      1950, 2100, 2250};
@@ -142,6 +170,156 @@ static int dap8211r_modify_ext(struct phy_device *phydev, u16 reg, u16 mask, u16
 	return ret;
 }
 
+/**
+ * dap8211r_led_hw_is_supported() - Check if the requested LED rules are supported
+ * @phydev: PHY device structure
+ * @index: LED index
+ * @rules: Bitmask of requested netdev trigger rules
+ *
+ * Validates the requested combination of rules which can be represented in the
+ * hardware register.
+ *
+ * Return: 0 on success, -EINVAL for an invalid LED index,
+ *         or -EOPNOTSUPP if any requested rule is not supported by the PHY
+ */
+static int dap8211r_led_hw_is_supported(struct phy_device *phydev, u8 index, unsigned long rules)
+{
+	switch (index) {
+	case DAP8211R_LED_ID_0:
+		fallthrough;
+	case DAP8211R_LED_ID_1:
+		fallthrough;
+	case DAP8211R_LED_ID_2:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return (rules & ~DAP8211R_LED_SUPPORTED_TRG) ? -EOPNOTSUPP : 0;
+}
+
+/**
+ * dap8211r_led_hw_control_set() - Program the LED control register
+ * @phydev: PHY device structure
+ * @index: LED index
+ * @rules: Bitmask of requested netdev trigger rules
+ *
+ * Translates the requested trigger rules into the corresponding bit
+ * positions of the LED control register and writes the resulting value
+ * to the PHY.
+ *
+ * Note that the ACT_BLINK bit (13) must be set together with TX/RX rules,
+ * otherwise the LED will not blink on traffic if no other rule (such as
+ * link or duplex) is active.
+ *
+ * Return: 0 on success, -EINVAL for an invalid LED index, or a negative error code
+ */
+static int dap8211r_led_hw_control_set(struct phy_device *phydev, u8 index, unsigned long rules)
+{
+	u16 led_mode = 0, led_reg;
+
+	switch (index) {
+	case DAP8211R_LED_ID_0:
+		led_reg = DAP8211R_LED_0;
+		break;
+	case DAP8211R_LED_ID_1:
+		led_reg = DAP8211R_LED_1;
+		break;
+	case DAP8211R_LED_ID_2:
+		led_reg = DAP8211R_LED_2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (test_bit(TRIGGER_NETDEV_LINK, &rules))
+		led_mode |= DAP8211R_LED_LINK;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+		led_mode |= DAP8211R_LED_LINK_10;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+		led_mode |= DAP8211R_LED_LINK_100;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+		led_mode |= DAP8211R_LED_LINK_1000;
+
+	if (test_bit(TRIGGER_NETDEV_RX, &rules))
+		led_mode |= DAP8211R_LED_RX | DAP8211R_LED_ACT_BLINK;
+
+	if (test_bit(TRIGGER_NETDEV_TX, &rules))
+		led_mode |= DAP8211R_LED_TX | DAP8211R_LED_ACT_BLINK;
+
+	if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+		led_mode |= DAP8211R_LED_HALF_DUPLEX;
+
+	if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+		led_mode |= DAP8211R_LED_FULL_DUPLEX;
+
+	if (!led_mode)
+		return -EOPNOTSUPP;
+
+	return dap8211r_write_ext(phydev, led_reg, led_mode);
+}
+
+/**
+ * dap8211r_led_hw_control_get() - Read the current LED configuration
+ * @phydev: PHY device structure
+ * @index: LED index
+ * @rules: Pointer to a bitmask where the current rules will be stored
+ *
+ * Reads the LED control register and translates the hardware bit
+ * positions back into the corresponding netdev trigger rules bitmask.
+ *
+ * Return: 0 on success, -EINVAL for an invalid LED index, or a negative error code
+ */
+static int dap8211r_led_hw_control_get(struct phy_device *phydev, u8 index, unsigned long *rules)
+{
+	u16 led_reg;
+	int led_mode = 0;
+
+	switch (index) {
+	case DAP8211R_LED_ID_0:
+		led_reg = DAP8211R_LED_0;
+		break;
+	case DAP8211R_LED_ID_1:
+		led_reg = DAP8211R_LED_1;
+		break;
+	case DAP8211R_LED_ID_2:
+		led_reg = DAP8211R_LED_2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	led_mode = dap8211r_read_ext(phydev, led_reg);
+	if (led_mode < 0)
+		return led_mode;
+
+	if (led_mode & DAP8211R_LED_LINK_10)
+		__set_bit(TRIGGER_NETDEV_LINK_10, rules);
+
+	if (led_mode & DAP8211R_LED_LINK_100)
+		__set_bit(TRIGGER_NETDEV_LINK_100, rules);
+
+	if (led_mode & DAP8211R_LED_LINK_1000)
+		__set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+
+	if (led_mode & DAP8211R_LED_RX)
+		__set_bit(TRIGGER_NETDEV_RX, rules);
+
+	if (led_mode & DAP8211R_LED_TX)
+		__set_bit(TRIGGER_NETDEV_TX, rules);
+
+	if (led_mode & DAP8211R_LED_HALF_DUPLEX)
+		__set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules);
+
+	if (led_mode & DAP8211R_LED_FULL_DUPLEX)
+		__set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules);
+
+	return 0;
+}
+
 /**
  * dap8211r_config_init() - Initialize PHY
  * @phydev: PHY device structure
@@ -230,6 +408,9 @@ static struct phy_driver dap8211r_driver[] = {
 		.config_aneg	= genphy_config_aneg,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
+		.led_hw_is_supported	= dap8211r_led_hw_is_supported,
+		.led_hw_control_set	= dap8211r_led_hw_control_set,
+		.led_hw_control_get	= dap8211r_led_hw_control_get,
 	},
 };
 module_phy_driver(dap8211r_driver);
-- 
2.43.0

Re: [PATCH net-next 3/3] net: phy: dap8211r: add led hardware control support

[Andrew Lunn]

> +#define DAP8211R_LED_ACT_BLINK		BIT(13)
> +#define DAP8211R_LED_FULL_DUPLEX	BIT(12)
> +#define DAP8211R_LED_HALF_DUPLEX	BIT(11)
> +#define DAP8211R_LED_TX			BIT(10)
> +#define DAP8211R_LED_RX			BIT(9)
> +#define DAP8211R_LED_LINK_1000		BIT(6)
> +#define DAP8211R_LED_LINK_100		BIT(5)
> +#define DAP8211R_LED_LINK_10		BIT(4)

Do any of these bits allow forced on/off?

> +static int dap8211r_led_hw_is_supported(struct phy_device *phydev, u8 index, unsigned long rules)
> +{
> +	switch (index) {
> +	case DAP8211R_LED_ID_0:
> +		fallthrough;
> +	case DAP8211R_LED_ID_1:
> +		fallthrough;
> +	case DAP8211R_LED_ID_2:
> +		break;

You can collapse case statements, without fallthrough, if there is no
code.

It is also possible to do:

case DAP8211R_LED_ID_0..DAP8211R_LED_ID_2:

     Andrew

Re: [PATCH net-next 3/3] net: phy: dap8211r: add led hardware control support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: phy: dap8211r: link trigger rejected in
  dap8211r_led_hw_is_supported()
- [Medium] net: phy: dap8211r: LED register not cleared in
  dap8211r_led_hw_control_set()
--

commit 7e5f0824bef63e117be29a9ce8d9c67d5c813bfa
Author: Artem Shimko <a.shimko.dev@gmail.com>

net: phy: dap8211r: add led hardware control support

This commit adds hardware LED control support to the DAP8211R PHY driver by
implementing the led_hw_is_supported(), led_hw_control_set(), and
led_hw_control_get() callbacks.

> diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
> index 4eb50c26397fb..2e371c231d32c 100644
> --- a/drivers/net/phy/dap8211r.c
> +++ b/drivers/net/phy/dap8211r.c
[ ... ]
> @@ -57,6 +58,33 @@
>   */
>  #define DAP8211R_DEFAULT_DEL_SEL	0xD
>  
> +#define DAP8211R_LED_ID_0		0
> +#define DAP8211R_LED_ID_1		1
> +#define DAP8211R_LED_ID_2		2
> +#define DAP8211R_LED_0			0xA00C
> +#define DAP8211R_LED_1			0xA00D
> +#define DAP8211R_LED_2			0xA00E
> +#define DAP8211R_LED_ACT_BLINK		BIT(13)
> +#define DAP8211R_LED_FULL_DUPLEX	BIT(12)
> +#define DAP8211R_LED_HALF_DUPLEX	BIT(11)
> +#define DAP8211R_LED_TX			BIT(10)
> +#define DAP8211R_LED_RX			BIT(9)
> +#define DAP8211R_LED_LINK_1000		BIT(6)
> +#define DAP8211R_LED_LINK_100		BIT(5)
> +#define DAP8211R_LED_LINK_10		BIT(4)
> +
> +#define DAP8211R_LED_LINK		(DAP8211R_LED_LINK_10	| \
> +					DAP8211R_LED_LINK_100	| \
> +					DAP8211R_LED_LINK_1000)
> +
> +#define DAP8211R_LED_SUPPORTED_TRG	(BIT(TRIGGER_NETDEV_LINK_10)	| \
> +					BIT(TRIGGER_NETDEV_LINK_100)	| \
> +					BIT(TRIGGER_NETDEV_LINK_1000)	| \
> +					BIT(TRIGGER_NETDEV_TX)		| \
> +					BIT(TRIGGER_NETDEV_RX)		| \
> +					BIT(TRIGGER_NETDEV_HALF_DUPLEX)	| \
> +					BIT(TRIGGER_NETDEV_FULL_DUPLEX))

[Severity: Medium]
Should DAP8211R_LED_SUPPORTED_TRG include BIT(TRIGGER_NETDEV_LINK)?

Without it, dap8211r_led_hw_is_supported() returns -EOPNOTSUPP whenever
TRIGGER_NETDEV_LINK is requested, so enabling the netdev link trigger on a
DAP8211R LED fails and the TRIGGER_NETDEV_LINK handling in
dap8211r_led_hw_control_set() is never reached.

[ ... ]

> @@ -142,6 +170,156 @@ static int dap8211r_modify_ext(struct phy_device *phydev, u16 reg, u16 mask, u16
[ ... ]
> +static int dap8211r_led_hw_control_set(struct phy_device *phydev, u8 index, unsigned long rules)
> +{
> +	u16 led_mode = 0, led_reg;
> +
> +	switch (index) {
> +	case DAP8211R_LED_ID_0:
> +		led_reg = DAP8211R_LED_0;
> +		break;
> +	case DAP8211R_LED_ID_1:
> +		led_reg = DAP8211R_LED_1;
> +		break;
> +	case DAP8211R_LED_ID_2:
> +		led_reg = DAP8211R_LED_2;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (test_bit(TRIGGER_NETDEV_LINK, &rules))
> +		led_mode |= DAP8211R_LED_LINK;
> +
> +	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
> +		led_mode |= DAP8211R_LED_LINK_10;
> +
> +	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
> +		led_mode |= DAP8211R_LED_LINK_100;
> +
> +	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
> +		led_mode |= DAP8211R_LED_LINK_1000;
> +
> +	if (test_bit(TRIGGER_NETDEV_RX, &rules))
> +		led_mode |= DAP8211R_LED_RX | DAP8211R_LED_ACT_BLINK;
> +
> +	if (test_bit(TRIGGER_NETDEV_TX, &rules))
> +		led_mode |= DAP8211R_LED_TX | DAP8211R_LED_ACT_BLINK;
> +
> +	if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
> +		led_mode |= DAP8211R_LED_HALF_DUPLEX;
> +
> +	if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
> +		led_mode |= DAP8211R_LED_FULL_DUPLEX;
> +
> +	if (!led_mode)
> +		return -EOPNOTSUPP;
> +
> +	return dap8211r_write_ext(phydev, led_reg, led_mode);
> +}

[Severity: Medium]
Does returning -EOPNOTSUPP when led_mode is 0 leave the previous trigger bits
enabled in the PHY register when all trigger rules are cleared?

When the last active netdev trigger rule is disabled (rules == 0),
dap8211r_led_hw_is_supported() returns 0, so set_baseline_state() in
drivers/leds/trigger/ledtrig-netdev.c calls hw_control_set() with mode == 0
to turn off the hardware triggers.  Returning -EOPNOTSUPP here skips
dap8211r_write_ext(), leaving the old configuration active in led_reg.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113110546.173624-1-a.shimko.dev@gmail.com?part=3