Re: [PATCH net-next] dt-bindings: net: Convert hisilicon,hns-mdio to DT schema

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dt-bindings: net: hisilicon,hns-mdio documents incorrect subctrl-vbase register offsets
- [Medium] dt-bindings: net: hisilicon,hns-mdio restricts compatible property breaking backwards compatibility
- [Medium] dt-bindings: net: hisilicon,hns-mdio omits functionally required property subctrl-vbase
--

commit a57c7bf53ebe7bce1c71d128ab3f7430b9797694
Author: Rob Herring (Arm) <robh@kernel.org>

dt-bindings: net: Convert hisilicon,hns-mdio to DT schema

This commit converts the hisilicon,hns-mdio binding to DT schema format.
It also adds the subctrl-vbase property which is currently in use.

> diff --git a/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml b/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml
> new file mode 100644
> index 0000000000000..b8350794650d4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hisilicon,hns-mdio.yaml

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - hisilicon,hns-mdio
> +      - hisilicon,mdio

[Severity: Medium]
Does this new enum restriction unintentionally break backwards compatibility?

The original txt binding documented a fallback array format that is now
rejected by this schema. As shown in the deleted txt example, arrays like:

compatible = "hisilicon,hns-mdio", "hisilicon,mdio";

were previously valid. Existing device trees using that format will now fail
local dtbs validation.

[ ... ]

> +  subctrl-vbase:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: syscon phandle
> +          - description: MDIO clock enable register offset
> +          - description: MDIO reset request register offset
> +          - description: MDIO reset done register offset
> +          - description: MDIO reset deassert register offset

[Severity: High]
Are these register descriptions for subctrl-vbase accurate?

The driver implementation maps the third argument to clock status and the
fourth argument to reset status, computing the reset deassert offset
internally by adding 4 to the second argument.

Documenting the third argument as reset done and the fourth argument as reset
deassert could lead hardware engineers to provide incorrect offsets. Following
this documentation causes the driver to poll incorrect syscon registers
during reset, leading to timeouts and functional failures during probe.

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - '#address-cells'
> +  - '#size-cells'

[Severity: Medium]
Should subctrl-vbase be included in this list of required properties?

The driver unconditionally checks for subctrl-vbase during probe on DT
systems and fails with -ENODEV if it is missing. Device trees authored
without this property will pass dt-schema validation, but the hardware will
fail to initialize at runtime.

[ ... ]

> +examples:
> +  - |
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        mdio@803c0000 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "hisilicon,hns-mdio";
> +            reg = <0x0 0x803c0000 0x0 0x10000>;
> +
> +            ethernet-phy@0 {
> +                reg = <0>;
> +            };
> +        };
> +    };

[Severity: Medium]
Since subctrl-vbase appears to be functionally required for initialization,
should it be included in this example? Adding it here would ensure the
example still passes validation if the property is correctly marked as
required.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925215911.2950356-1-robh@kernel.org?part=1