Re: [PATCH net-next v2 4/5] dt-bindings: net: maxlinear: add MxL86211C lane mapping support

[netdev-bot+sashiko@kernel.org]

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] REFUTED (baseline decision false; kept only because
  requires_validation=true).
- [Low] The new line `else: properties: maxlinear,lane-asp-map: false` in
  maxlinear,gpy2xx.yaml can never take effect.

--

dt-bindings: net: maxlinear: add MxL86211C lane mapping support

This adds an optional maxlinear,lane-asp-map property to the GPY2xx PHY
binding. The property describes how the physical TPI lanes map to the
MxL86211C analog front ends (ASPs). The patch also extends the existing
else branch so the property is rejected on PHYs other than the MxL86211C.

> diff --git a/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml b/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
> index a0dfb6192d81b..4323be300c721 100644
> --- a/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
> +++ b/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
> @@ -35,6 +35,7 @@ allOf:
>      else:
>        properties:
>          vddp-supply: false
> +        maxlinear,lane-asp-map: false

[Severity: Low]
Can this else branch ever be evaluated? The top-level select of this schema
only matches nodes that carry the MxL86211C PHY ID compatible:

select:
  properties:
    compatible:
      contains:
        const: ethernet-phy-idc133.5400
  required:
    - compatible

The if inside allOf tests the same condition:

  - if:
      properties:
        compatible:
          contains:
            const: ethernet-phy-idc133.5400
      required:
        - compatible

So every node this schema applies to takes the then branch. No other binding
references maxlinear,gpy2xx.yaml, so the else branch has no other way to be
reached.

Other PHY nodes are checked only by ethernet-phy.yaml, which selects by
$nodename and has additionalProperties: true. That covers other GPY2xx PHYs,
and also MxL86211C PHYs that leave out the optional PHY ID compatible because
they are auto-probed.

On those nodes, maxlinear,lane-asp-map: false is never enforced. The
minItems/maxItems, minimum/maximum and uniqueItems constraints are never
checked either, so a value like <0 0> would pass dtbs_check. The problem
would then only show up at boot, when the driver's config_init rejects the
value.

The vddp-supply: false line added earlier in this series has the same
problem. This patch adds a second restriction that depends on it.

Would it make sense to change the select so it matches every node that could
use this binding, or to drop the else branch? Another option is to require
the PHY ID compatible whenever maxlinear,lane-asp-map is present.

> @@ -42,6 +43,21 @@ properties:
>        Regulator supplying the VDDP pin. Its voltage configures the MDIO,
>        MDC, and MDINT pad voltage and must be either 1.8 V or 3.3 V.
>  
> +  maxlinear,lane-asp-map:
> +    description: |
> +      Mapping of physical TPI lanes A through D to analog front ends (ASPs)
> +      A through D. The array index identifies the physical TPI lane, while
> +      its value identifies the analog front end, encoded as 0 through 3.
> +      Each analog front end must be mapped exactly once. Omit the property to
> +      retain the hardware reset mapping.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 4
> +    maxItems: 4
> +    uniqueItems: true
> +    items:
> +      minimum: 0
> +      maximum: 3

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016112033.949026-1-robert.marko%40sartura.hr