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

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

Critical: 0 · High: 0 · Medium: 1 · Low: 5

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new binding
  Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml redefines…
- [Low] The patch adds `M: Kyle Switch [off-list ref]` as a
  second maintainer for the whole existing MOTORCOMM PHY DRIVER section…
- [Low] The `reg` description in motorcomm,yt8824.yaml gives its address
  rule in terms of 'internal mode' and 'external mode'.
- [Low] (Discarded as a false positive in the final series state.) The
  concern was that no in-tree code reads the required `phy-mode` or uses…
- [Low] motorcomm,yt8824.yaml says the package `reg` is 'fixed at 9' in
  internal mode, but the schema does not enforce it.
- [Low] (Discarded as a false positive.) The concern was that the example
  (member PHYs at 4..7, package reg 9) contradicts the 'base + 4' rule.

--

dt-bindings: net: Document Motorcomm YT8824 PHY package

This commit adds a devicetree binding for the Motorcomm YT8824, a package
of four Ethernet PHYs. The binding is built on ethernet-phy-package.yaml.
The commit also updates the MOTORCOMM PHY DRIVER section in MAINTAINERS
with the new file and a new maintainer.

[ ... ]

[Severity: Medium]
Would a vendor-prefixed property fit better here than phy-mode?

ethernet-controller.yaml defines phy-mode as the interface type between
the MAC and the PHY:

  phy-mode:
    $ref: "#/properties/phy-connection-type"

This binding gives phy-mode its own string type and enum on the package
node and uses it to pick the package operating mode. In the driver patch
later in this series, yt8824_phy_package_probe_once() reads it only to
choose between the internal and external init/reset sequences:

  res = of_get_phy_mode(np, &priv->interface_mode);

qcom,qca807x.yaml has a similar PHY package and uses a vendor property
for the same purpose:

  qcom,package-mode:
    ...
    $ref: /schemas/types.yaml#/definitions/string
    enum:
      - qsgmii
      - psgmii

writing-bindings.rst also asks for a vendor prefix on device-specific
property names.

This value on the package node also duplicates the MAC's phy-mode, which
phylink already passes to the PHY as phydev->interface. Can the two
disagree? The YT8824 driver never looks at phydev->interface.

phy-mode is also listed under required, so every YT8824 DT will carry it.
Changing its name or meaning after merge would break those DTs.

[Severity: Low]
Which phy-mode value does "external mode" mean? The phy-mode enum above
only allows internal and 10g-qxgmii. The text never says that the base + 4
rule applies to phy-mode = "10g-qxgmii".

That can only be worked out from the driver patch later in the series,
which treats any mode other than PHY_INTERFACE_MODE_INTERNAL as external.

The value matters. of_phy_package_join() stores this reg as
shared->base_addr, and ytphy_read_top_ext() and ytphy_write_top_ext()
reach the top block at that address.

[Severity: Low]
Could the schema enforce the "fixed at 9" rule for internal mode? No
allOf/if block ties reg to phy-mode, so a node with phy-mode = "internal"
and reg = <5> still passes dt_binding_check and dtbs_check.

Something along these lines would cover it:

  allOf:
    - if:
        properties:
          phy-mode:
            const: internal
      then:
        properties:
          reg:
            const: 9

With the driver added later in the series, a wrong reg value is used
directly at runtime:

of_phy_package_join()
  phy_package_join(phydev, base_addr, ...)

ytphy_write_top_ext()
  __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, ...)

In that case, writes to the top block would go to the wrong MDIO device.
The schema can't express the external base + 4 rule, but it can express
the internal-mode constant.

[Severity: Low]
This line makes Kyle Switch a second maintainer of the whole MOTORCOMM PHY
DRIVER section. That section also covers drivers/net/phy/motorcomm.c and
motorcomm,yt8xxx.yaml, not just the new binding.

The commit message only says:

  Motorcomm YT8824 Ethernet PHY is PHY package of 4 PHY-s.

It doesn't mention the maintainer change, and there's no Acked-by from the
current maintainer.

Could this be split into its own patch, or at least be described in the
changelog?

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