Re: [PATCH net-next v24 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[netdev-bot+sashiko]

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 YT8824 binding gives the package node's `reg` a device-
  specific meaning: the MDIO address of the shared 'top extend' register…

--

dt-bindings: net: Document Motorcomm YT8824 PHY package

This commit adds a devicetree binding for the Motorcomm YT8824, a package
of four PHYs. The binding is built on ethernet-phy-package.yaml. It adds a
motorcomm,package-mode property and gives the package reg a
device-specific meaning. MAINTAINERS is updated to list the new file and
a new maintainer.

> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> new file mode 100644
> index 0000000000000..4c67f8afcf27a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml

[ ... ]

> +  motorcomm,package-mode:
> +    description: |
> +      PHY package can be configured in 2 modes:
> +      - internal: YT8824 is embedded in the switch, using
> +        an internal interface to the MAC.
> +      - 10g-qxgmii: YT8824 is a standalone external PHY,
> +        connected via 10G QXGMII.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ internal, 10g-qxgmii ]
> +
> +  reg:
> +    description:
> +      The absolute MDIO address of the YT8824 shared top extend
> +      register block.
> +      For internal mode, this is fixed at 9;
> +      For external mode, it is the base MDIO address of the four
> +      member PHYs plus 4, where base is typically 0x0.
> +      Note that this is NOT the reg of the first member PHY.

[Severity: Low]
Should the schema enforce the constraints that this description states?

The text says reg is fixed at 9 in internal mode. In 10g-qxgmii mode it
says reg is the member PHY base plus 4. The binding has no allOf if/then
block that ties reg to motorcomm,package-mode, so any reg value passes
dt_binding_check. For example, nothing like this exists:

  if:
    properties:
      motorcomm,package-mode:
        const: internal
  then:
    properties:
      reg:
        const: 9

This meaning of reg also differs from ethernet-phy-package.yaml. That file
describes reg as "The base ID number for the PHY package. Commonly the ID
of the first PHY in the PHY package". A DT that follows the common
convention would pass validation here without any warning.

This matters for the driver added later in this series.
ytphy_read_top_ext() and ytphy_write_top_ext() in
drivers/net/phy/motorcomm.c send every top extend access to package
offset 0:

  ret = __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, regnum);

phy_package_address() in drivers/net/phy/phy_package.c turns offset 0
into base_addr:

  return base_addr + addr_offset;

of_phy_package_join() reads base_addr directly from the package node's
reg. The call path is:

  of_phy_package_join()
    of_property_read_u32(package_node, "reg", &base_addr)
  phy_package_join()
  ytphy_write_top_ext()
    __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, ...)
      phy_package_address() -> base_addr

If a board DT sets reg to the first member PHY's address, would the driver
write the top extend page select and data values into that member PHY's
registers 0x1e/0x1f?

Could a per-mode if/then block be added to constrain reg? For internal
mode this could be const: 9.

> +
> +required:
> +  - compatible
> +  - motorcomm,package-mode
> +  - reg

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002153034.934814-2-kyle.switch%40motor-comm.com