Re: [PATCH net-next v7 1/3] dt-bindings: net: wiznet,w5100: convert to DT schema

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1 (1)

- [Low] In the newly added
  Documentation/devicetree/bindings/net/wiznet,w5100.yaml, `spi-max-…

Pre-existing issues:
- [Low] Documentation/devicetree/bindings/net/wiznet,w5100.yaml (added by
  this patch) constrains `interrupts` only with a count (`maxItems: 1`…

--

dt-bindings: net: wiznet,w5100: convert to DT schema

This converts the Wiznet w5x00 SPI Ethernet controller binding from plain
text to a YAML schema, adding the file wiznet,w5100.yaml and deleting
wiznet,w5x00.txt.  The new schema enumerates the three compatibles,
constrains reg/interrupts/spi-max-frequency, and carries over the
Raspberry Pi example.

A couple of questions below about the interrupt and spi-max-frequency
descriptions.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> new file mode 100644
> index 0000000000000..601d828bae927
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> @@ -0,0 +1,66 @@

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - wiznet,w5100
> +      - wiznet,w5200
> +      - wiznet,w5500
> +
> +  interrupts:
> +    maxItems: 1

[Severity: Low]
This isn't an issue introduced by this patch, but the interrupt polarity
disagreement between the binding and the driver is now the only in-tree
record of it, and the wording that described it is being dropped here.

The deleted wiznet,w5x00.txt said:

  w5x00 natively generates falling edge interrupts, however, additional
  board logic might invert the signal.

The new schema has no description and no type constraint under
interrupts, so that information disappears from the tree.

Meanwhile w5100_probe() in drivers/net/ethernet/wiznet/w5100.c hardcodes a
level-low trigger in both registration paths:

	err = request_threaded_irq(priv->irq, NULL, w5100_interrupt,
				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
				   netdev_name(ndev), ndev);
	} else {
		err = request_irq(priv->irq, w5100_interrupt,
				  IRQF_TRIGGER_LOW, netdev_name(ndev), ndev);

and __setup_irq() in kernel/irq/manage.c applies the driver flags over
whatever the DT node asked for:

		if (new->flags & IRQF_TRIGGER_MASK) {
			ret = __irq_set_trigger(desc,
						new->flags & IRQF_TRIGGER_MASK);

So no interrupts are lost, but the type in a node copied from the example
is never the one the line is programmed with, and request_irq() fails
outright on an irqchip that cannot do level triggering.

Would it make sense to either describe/constrain the type as level-low in
the schema, or have the driver pass IRQF_TRIGGER_NONE and honour the DT
type?

> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description:
> +      According to the w5500 datasheet, the chip allows a maximum of 80 MHz,
> +      however, board designs may need to limit this value.
> +    maximum: 80000000

[Severity: Low]
Should this ceiling be per-compatible?  The description scopes the 80 MHz
figure to the w5500 only ("According to the w5500 datasheet"), and the
deleted .txt scoped it the same way ("when accessing the w5500"), but the
constraint sits in the shared properties block so wiznet,w5100 and
wiznet,w5200 inherit it too.

The older parts have lower SCLK limits, so a node like:

    ethernet@0 {
        compatible = "wiznet,w5100";
        spi-max-frequency = <80000000>;
        ...
    };

passes make dt_binding_check with no diagnostic, and w5100-spi.c then
clocks the bus at that rate.

Other net SPI bindings in the same directory express this per variant
inside a compatible conditional, for example the spi-max-frequency block
in Documentation/devicetree/bindings/net/qca,qca7000.yaml:

        spi-max-frequency:
          default: 8000000
          maximum: 16000000
          minimum: 1000000

Would an allOf/if:properties:compatible:contains: block per part be
better here, or a description explaining why 80 MHz is safe for all
three?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:

[ ... ]

> +        ethernet@0 {
> +            compatible = "wiznet,w5500";
> +            reg = <0>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&eth1_pins>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;

[Severity: Low]
This is the same pre-existing mismatch noted above and not something this
patch introduces; the edge-falling type was in the old .txt example
verbatim.  Since this example is now the only surviving statement of the
expected trigger type, is IRQ_TYPE_LEVEL_LOW the value to show, given
w5100_probe() requests IRQF_TRIGGER_LOW?

Nothing in-tree uses these compatibles today (a grep for wiznet,w5 matches
only the binding and w5100-spi.c), so no shipped board DT is affected
either way.

> +            spi-max-frequency = <30000000>;
> +            local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
> +        };
> +    };
> +...

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-wiznet-link-gpio-v7-0-69f01b70c96f%40marmottus.net