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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch silently adds a new MAINTAINERS section ("WIZNET W5100
  ETHERNET DRIVER") naming the submitter as maintainer, but the commit…
- [Low] In Documentation/devicetree/bindings/net/wiznet,w5100.yaml the
  new schema states a single, unconditional 'spi-max-frequency: maximum:…

--

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

This converts the Wiznet w5x00 SPI Ethernet controller binding from plain
text to a YAML DT schema, and changes the documented interrupt trigger type
from edge-falling to level-low to match the driver. It also adds a new
MAINTAINERS section for the binding.

Two questions below.

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

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - wiznet,w5100
> +      - wiznet,w5200
> +      - wiznet,w5500
> +
> +  interrupts:
> +    maxItems: 1
> +    description: Main interrupt (INT pin on the device).
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description:
> +      According to the datasheets, w5500 and w5200 allow a maximum of 80 MHz
> +      while w5100 is reported by users to allow a maximum of 14 MHz. However,
> +      board designs may need to limit this value.
> +    maximum: 80000000

[Severity: Low]
The description here says the w5100 tops out around 14 MHz, but the only
machine-checkable constraint is the loosest value, 80000000, and it applies
to all three compatibles.

Should the w5100 limit be expressed as a conditional instead, so that it is
actually validated?

  allOf:
    - if:
        properties:
          compatible:
            contains:
              const: wiznet,w5100
      then:
        properties:
          spi-max-frequency:
            maximum: 14000000

As written, a node with:

    compatible = "wiznet,w5100";
    spi-max-frequency = <80000000>;

passes dt_binding_check and dtbs_check with no diagnostic. Nothing in
drivers/net/ethernet/wiznet/ looks at spi->max_speed_hz either, so the
14 MHz figure is not enforced anywhere.

This is still the case at the end of the series: the allOf if/then block
added later by "dt-bindings: net: wiznet,w5100: add link status interrupt"
only constrains interrupts, and spi-max-frequency keeps the unconditional
80 MHz ceiling.

Separately, the old wiznet,w5x00.txt documented the 80 MHz figure for the
w5500 only:

    - spi-max-frequency: Maximum frequency of the SPI bus when accessing
      the w5500.

The new schema extends that ceiling to w5100 and w5200 as well. Could the
commit message mention that change?

[ ... ]

> diff --git a/MAINTAINERS b/MAINTAINERS
> index df8ab9b824024..9f84e4c8163e4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -29450,6 +29450,12 @@ M:	Miloslav Trmac <mitr@volny.cz>
>  S:	Maintained
>  F:	drivers/input/misc/wistron_btns.c
>  
> +WIZNET W5100 ETHERNET DRIVER
> +M:	Arthur Crepin Leblond <arthur@marmottus.net>
> +L:	netdev@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> +

[Severity: Low]
The commit message describes only the .txt to .yaml conversion and the
trigger type change, but this hunk also creates a new maintainership entry.
The "v8: Add a myself in MAINTAINERS" note is below the --- line and will
not land in git history.

Could the commit message describe the MAINTAINERS addition too?

Also, at this commit the section is titled "WIZNET W5100 ETHERNET DRIVER"
while the only F: pattern matches the YAML binding, so get_maintainer.pl
still reports no maintainer for drivers/net/ethernet/wiznet/. That gap is
closed later in the series: "w5100: detect carrier state using link status
bit and optional interrupt" adds F: drivers/net/ethernet/wiznet/ to the
same block. Would it make sense to add that F: line here, where the section
is introduced?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-wiznet-link-gpio-v10-0-5874a7a147a2%40marmottus.net