Re: [PATCH net-next v8 2/3] dt-bindings: net: wiznet,w5100: add link status interrupt
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-17 20:08:52
Also in:
lkml, netdev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 2 - [Low] The new `allOf` `if/then/else` conditional in Documentation/devicetree/bindings/net/wiznet,w5100.yaml encodes a… - [Low] Concern (assessed as NOT valid): the binding documents an optional second (LINKLED) interrupt for wiznet,w5500 and the semantics… -- dt-bindings: net: wiznet,w5100: add link status interrupt This documents an optional second interrupt on the wiznet SPI ethernet binding, sourced from the LINKLED pin, for reporting link status changes. The interrupts property is turned into a conditional list that allows two entries for wiznet,w5500 and keeps a single entry for the other compatibles. No Link: tags present in the commit header.
quoted hunk ↗ jump to hunk
diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml index 25098308eb6e5..da03f4e07f29a 100644 --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
[ ... ]
quoted hunk ↗ jump to hunk
@@ -24,8 +20,11 @@ properties: - wiznet,w5500 interrupts: - maxItems: 1 - description: Main interrupt (INT pin on the device). + description: + The main interrupt (INT pin) is mandatory on all devices. The w5500 + additionally supports an optional second interrupt for link status changes + (LINKLED pin). If it is not specified, link state changes will not be + reported.
[Severity: Low]
At this commit, drivers/net/ethernet/wiznet/w5100.c only consumes spi->irq
and does not report carrier state, so the sentence "If it is not specified,
link state changes will not be reported" describes behaviour that has no
consumer yet.
Checking the rest of the series, the driver side does land in the following
patch, where w5100_probe() picks up the second entry:
priv->link_irq = ops->chip_id == W5500 ?
fwnode_irq_get(dev_fwnode(dev), 1) : -EINVAL;
and w5500_report_carrier_state() calls netif_carrier_on()/
netif_carrier_off(). So this looks fine as a binding-before-driver
ordering, and is only noted here for completeness.
quoted hunk ↗ jump to hunk
reg: maxItems: 1@@ -42,6 +41,30 @@ required: - reg - interrupts +allOf: + - $ref: /schemas/net/ethernet-controller.yaml# + - $ref: /schemas/spi/spi-peripheral-props.yaml# + - if: + properties: + compatible: + contains: + enum: + - wiznet,w5500 + then: + properties: + interrupts: + minItems: 1 + items: + - description: Main interrupt (INT pin on the device). + - description: + Optional interrupt for link status changes (LINKLED pin on + w5500). + else: + properties: + interrupts: + maxItems: 1 + description: Main interrupt (INT pin on the device).
[Severity: Low]
Is this else branch describing the hardware, or the current Linux driver?
The LINKLED output exists on the w5100 and w5200 packages as well, so a
board that routes w5100/w5200 LINKLED to a host GPIO cannot be described
here:
compatible = "wiznet,w5200";
interrupts = <25 IRQ_TYPE_LEVEL_LOW>, <24 IRQ_TYPE_EDGE_BOTH>;
The second entry is rejected by maxItems: 1 during dt_binding_check.
The only in-tree justification found for the w5500 restriction is the
driver gate added later in this series, in w5100_probe():
priv->link_irq = ops->chip_id == W5500 ?
fwnode_irq_get(dev_fwnode(dev), 1) : -EINVAL;
which is a software limitation (register readback of link state via w5500
PHYCFGR) rather than absence of the pin. Would it be preferable to allow
two interrupts for all three compatibles and note in the interrupts
description which parts can actually report link state, so this constraint
does not have to be relaxed later?
unevaluatedProperties: false examples:
[ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-wiznet-link-gpio-v8-0-d173622474cc%40marmottus.net