Re: [PATCH net-next v24 03/10] dt-bindings: mfd: Document support for Airoha AN8855 Switch SoC

[netdev-bot+sashiko@kernel.org]

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

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

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 series documents two different MDIO clauses for the same
  AN8855 internal PHY (ethernet-phy-idc0ff.0410).
- [Medium] The binding defines reset-gpios with only 'maxItems: 1'
  (airoha,an8855.yaml lines 26-27).
- [Low] The two binding examples in the series map the per-port eFUSE TX
  calibration cells to PHY addresses in incompatible ways.

--

dt-bindings: mfd: Document support for Airoha AN8855 Switch SoC

This adds a new YAML binding for the Airoha AN8855 Switch SoC MFD node.
The node has efuse, ethernet-switch and mdio children and an optional
reset-gpios property.

> diff --git a/Documentation/devicetree/bindings/mfd/airoha,an8855.yaml b/Documentation/devicetree/bindings/mfd/airoha,an8855.yaml
> new file mode 100644
> index 0000000000000..48127e956c965
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/airoha,an8855.yaml
> @@ -0,0 +1,176 @@

[ ... ]

> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1

[Severity: Medium]
The reset-gpios property doesn't describe the pin or its active level. The
only example uses:

    reset-gpios = <&pio 39 GPIO_ACTIVE_HIGH>;

Does anything in the series read this property?
drivers/mfd/airoha-an8855.c includes linux/gpio/consumer.h but never calls
gpiod_get*(). drivers/net/dsa/mediatek/an8855.c has no reset GPIO handling
either.

The MDIO core doesn't handle it for this node. The airoha,an8855 node is
registered as a non-PHY mdio_device, and the reset GPIO is only requested
for PHYs:

drivers/net/phy/mdio_device.c:mdiobus_register_device() {
    ...
    if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY) {
        err = mdio_device_register_reset(mdiodev);
    ...
}

If the board's GPIO default holds RESET_N asserted, would the chip stay in
reset and cause an8855_read_switch_id() to fail?

Could the binding also state the expected polarity? When a consumer is
added, there are two options:

- It follows gpiod semantics (1 = assert). Then an ACTIVE_HIGH flag on an
  active-low RESET_N would hold the chip in reset.
- It follows the inverted mt7530 convention. Then the DT ABI is tied to an
  inverted flag.

I couldn't confirm the AN8855 pin polarity from a datasheet or from in-tree
code.

[ ... ]

> +            mdio {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                internal_phy1: ethernet-phy@1 {
> +                  compatible = "ethernet-phy-idc0ff.0410",
> +                               "ethernet-phy-ieee802.3-c22";

[Severity: Medium]
This example declares the internal PHYs as Clause 22. The PHY binding
added earlier in the series ("dt-bindings: net: Document support for AN8855
Switch Internal PHY") uses Clause 45 for the same PHY ID in
Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml:

    compatible = "ethernet-phy-idc0ff.0410",
                 "ethernet-phy-ieee802.3-c45";

Which one is intended?

With the MFD driver added later in the series, only c22 seems to work.
The bus comes from devm_mdio_regmap_register(), which only sets C22
accessors:

drivers/net/mdio/mdio-regmap.c:devm_mdio_regmap_register() {
    ...
    mii->read = mdio_regmap_read_c22;
    mii->write = mdio_regmap_write_c22;
    ...
}

If a board DT copies the PHY binding example, fwnode_mdiobus_register_phy()
sets is_c45 and the probe goes:

an8855_core_probe()
  fwnode_mdiobus_register_phy()
    get_phy_device(bus, addr, true)
      get_phy_c45_ids()
        get_phy_c45_devs_in_pkg()
          __mdiobus_c45_read()   <- -EOPNOTSUPP, no read_c45

get_phy_c45_ids() turns this into -EIO. an8855_core_probe() then takes the
err_put_fwnodes path, and none of the switch, efuse or PHY functions come
up.

dt_binding_check won't catch this, because ethernet-phy.yaml accepts both
compatibles. Should the PHY binding example use
ethernet-phy-ieee802.3-c22 to match this one?

> +                  reg = <1>;
> +
> +                  nvmem-cells = <&shift_sel_port0_tx_a>,
> +                      <&shift_sel_port0_tx_b>,
> +                      <&shift_sel_port0_tx_c>,
> +                      <&shift_sel_port0_tx_d>;
> +                  nvmem-cell-names = "tx_a", "tx_b", "tx_c", "tx_d";
> +                };
> +
> +                internal_phy2: ethernet-phy@2 {
> +                  compatible = "ethernet-phy-idc0ff.0410",
> +                               "ethernet-phy-ieee802.3-c22";
> +                  reg = <2>;
> +
> +                  nvmem-cells = <&shift_sel_port1_tx_a>,
> +                      <&shift_sel_port1_tx_b>,
> +                      <&shift_sel_port1_tx_c>,
> +                      <&shift_sel_port1_tx_d>;
> +                  nvmem-cell-names = "tx_a", "tx_b", "tx_c", "tx_d";
> +                };

[Severity: Low]
Here, ethernet-phy@1 (the phy-handle of port@0) gets the
shift_sel_port0_tx_* cells and ethernet-phy@2 gets shift_sel_port1_tx_*.
The example in net/airoha,an8855-phy.yaml is different: ethernet-phy@2
references shift_sel_port0_tx_* and ethernet-phy@1 has no nvmem-cells.

The MFD driver treats the switch address as port 0's PHY:

drivers/mfd/airoha-an8855.c:an8855_phy_bus_register() {
    ...
    phy_offset = addr - priv->switch_addr;
    if (phy_offset >= AN8855_MAX_PHY_PORT)
        return -EINVAL;
    ...
}

So when ethernet-phy@1 exists, ethernet-phy@2 can't be port 0. The PHY
driver in drivers/net/phy/air_an8855.c applies whatever tx_a..tx_d cells
the node references, with no cross-check.

Would a DT that follows the PHY binding example program port 0's TX
calibration into a different port? This example looks like the consistent
one. Should the PHY binding example be changed to match it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928194511.41878-1-ansuelsmth%40gmail.com