Re: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Logical contradiction between the property's documented default value and the commit message's explicit configuration requirement.
- [Low] The `rockchip,sgmii-mac-sel` property is defined globally and not conditionally constrained to the RK3568 SoC.
--

--- Patch [2]: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property ---
commit 0d950b4055dd144fecf5059f16535f2c8d94b5d3
Author: Coia Prant <coiaprant@gmail.com>

dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property

This commit adds the "rockchip,sgmii-mac-sel" property to the Rockchip
Naneng combphy device tree binding. It allows selecting between GMAC0
and GMAC1 for the SGMII interface on the RK3568 SoC.

> This is necessary for boards such as the Ariaboard Photonicat, where
> the SGMII interface is connected to GMAC0 and needs to be explicitly
> configured.

[Severity: Medium]
Is it actually necessary to explicitly configure the property to 0 for
GMAC0? 

The schema defines the default as 0, which implies that omitting the property
would naturally default the hardware to GMAC0. If explicit configuration is
required, does this mean the hardware or bootloader leaves the register at 1,
making the `default: 0` documented in the schema incorrect?

> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> index 379b08bd9e97a..6173192e31ab2 100644
> --- a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
[ ... ]
> @@ -80,6 +80,13 @@ properties:
>      description:
>        Some additional pipe settings are accessed through GRF regs.
>  
> +  rockchip,sgmii-mac-sel:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

[Severity: Low]
Should this property be conditionally restricted to the RK3568 compatible
string? 

Because it is defined in the top-level properties block, it will pass
validation for all other hardware variants (like RK3528 or RK3588) that do
not actually support SGMII. Could an `if-then` block in the `allOf` section
be used to set this property to false for compatibles other than
`rockchip,rk3568-naneng-combphy`?

> +    default: 0
> +    description:
> +      Select gmac0 or gmac1 to be used as SGMII controller.
> +
>    "#phy-cells":
>      const: 1
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=2