Re: [PATCH net-next v3 1/4] dt-bindings: net: dsa: add MT7628 ESW

[Rob Herring <robh@kernel.org>]

On Thu, Apr 30, 2026 at 04:56:21PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email says "Since Andrew requested some changes already here is the
> AI review to look thru as well (could be false positives)."
> ---
> Subject: dt-bindings: net: dsa: add MT7628 ESW
> 
> This patch adds a new YAML device tree binding for the Embedded Switch
> block found in the MediaTek MT7628 SoC. The binding describes a five
> port switch with integrated 10/100 PHYs plus an internally connected
> 1G CPU port, together with reset lines and an optional MDIO subnode.
> 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7628-esw.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7628-esw.yaml
> > new file mode 100644
> > index 000000000000..a9db9057ee54
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7628-esw.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/dsa/mediatek,mt7628-esw.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mediatek MT7628 Embedded Ethernet Switch
> > +
> > +maintainers:
> > +  - Joris Vaisvila <joey@tinyisr.com>
> 
> This isn't a bug, but the maintainers list only contains the submitter
> at an unrelated domain. Existing in-tree MediaTek DSA bindings such as
> Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml list
> multiple MediaTek or long-standing MediaTek-community maintainers.
> 
> For an official MediaTek SoC's built-in switch IP, would it make sense
> to CC or co-list a MediaTek networking maintainer (Sean Wang, Landen
> Chao, Daniel Golle, ...) so the hardware description can be
> corroborated and the binding ABI has long-term stewardship?
> 
> > +
> > +description:
> > +  The MT7628 SoC's built-in Ethernet Switch is a five port switch with
> > +  integrated 10/100 PHYs. The switch registers are directly mapped in the SoC's
> > +  memory. The switch has an internally connected 1G CPU port and 5 user ports
> > +  connected to the built-in Fast Ethernet PHYs.
> 
> Is the description internally consistent?
> 
> The first sentence says "a five port switch with integrated 10/100
> PHYs", but the following sentences describe "an internally connected
> 1G CPU port and 5 user ports", which is six ports in total. The
> example below also enumerates six ports (reg = 0..4 and 6).
> 
> Could this be reworded as e.g. "5 user ports plus 1 internal CPU
> port" to avoid the ambiguity?
> 
> Separately, there is no justification in the commit message or
> binding for the absence of a clocks property. MT7628 peripherals are
> typically gated by syscon clock outputs (see
> arch/mips/boot/dts/ralink/mt7628a.dtsi), and
> drivers/net/ethernet/mediatek/mtk_eth_soc.c already treats "esw" as a
> named clock.
> 
> The MT7628 ESW driver added alongside this binding deliberately does
> not call any clk_* API, which is a valid design choice if the esw
> gate is always on after reset. Since DT bindings are ABI, though,
> adding a clocks property later would be a backward-incompatible
> change.
> 
> Could the commit message explain why no clocks property is needed, or
> alternatively could clocks/clock-names be described in the binding
> now even if the initial driver ignores them?
> 
> > +
> > +unevaluatedProperties: false
> > +
> > +allOf:
> > +  - $ref: dsa.yaml#/$defs/ethernet-ports
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,mt7628-esw
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    items:
> > +      - description: internal switch block reset
> > +      - description: internal phy package reset
> > +
> > +  reset-names:
> > +    items:
> > +      - const: esw
> > +      - const: ephy
> > +
> > +  mdio:
> > +    $ref: /schemas/net/mdio.yaml#
> > +    unevaluatedProperties: false
> 
> The mdio subnode is referenced here and is actually consumed by the
> driver (drivers/net/dsa/mt7628.c mt7628_setup_internal_mdio() calls
> of_get_available_child_by_name(..., "mdio") and
> devm_of_mdiobus_register()), but the binding provides no description
> of when or why a user would populate it.
> 
> Given the top-level description says the PHYs are "integrated" and
> "internally connected", when should an mdio subnode be present? Is an
> external MDIO master ever supported?
> 
> A short description attached to the mdio property would make the
> intent clear to DT authors.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - resets
> > +  - reset-names
> > +  - ethernet-ports
> 
> Should the valid port reg values be constrained here?
> 
> The MT7628 ESW has a fixed, well-known set of valid port indices
> (0..4 for user ports and 6 for the CPU, with port 5 not existing).
> See mt7628_phylink_get_caps() in drivers/net/dsa/mt7628.c, which
> explicitly comments that "port 5 does not exist on MT7628".
> 
> As written the schema accepts ethernet-port@5 or ethernet-port@7 at
> dt_binding_check time, even though the driver will silently ignore
> them. Would adding a reg enum constraint on the ethernet-port
> children (e.g. via patternProperties, or an override of the
> $defs/ethernet-ports reg) catch these mistakes at schema-validation
> time?
> 
> > +
> > +examples:
> > +  - |
> > +    switch0: switch@10110000 {

This label is unused and should be dropped.

> > +        compatible = "mediatek,mt7628-esw";
> > +
> > +        reg = <0x10110000 0x8000>;
> > +
> > +        resets = <&sysc 23>, <&sysc 24>;
> > +        reset-names = "esw", "ephy";
> > +
> > +        ethernet-ports {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> [ ... ]
> > +            ethernet-port@6 {
> > +                reg = <6>;
> > +                phy-mode = "internal";
> > +                ethernet = <&ethernet>;
> 
> The example references an undefined phandle here: ethernet-port@6
> uses ethernet = <&ethernet> but no node labelled "ethernet" is defined
> in the example snippet.
> 
> Other DSA binding examples in Documentation/devicetree/bindings/net/dsa/
> typically either provide a minimal stub for referenced phandles inside
> the example or use a reg-based reference. Will dt_binding_check emit a
> warning about the unresolved phandle as written?

No, example phandles don't have to resolve to a provider and we 
actively don't want providers in the examples. 

Rob