Re: [PATCH net-next v9 02/15] dt-bindings: dpll: add SiTime SiT95316 clock generator

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] The binding asserts fixed hardware counts (four PLLs PLLA..PLLD,
  8 individually selectable inputs, 8 outputs on SiT95317 / 12 on…

--

dt-bindings: dpll: add SiTime SiT95316 clock generator

Adds a YAML binding for the SiTime SiT9531x family (SiT95316/SiT95317)
of I2C-controlled clock generators described as DPLL providers, with
four PLLs, up to eight inputs and up to twelve outputs.  Also adds a
MAINTAINERS entry for the new binding.

> diff --git a/Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml b/Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml
> new file mode 100644
> index 0000000000000..e71ee3611a830
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml
> @@ -0,0 +1,181 @@

[ ... ]

> +  Both parts have 4 differential input pairs whose lanes can also be
> +  driven independently as single-ended references, so 8 inputs are
> +  individually selectable.  SiT95317 drives 8 outputs, SiT95316
> +  drives 12.

[ ... ]

> +  sitime,pll-fvco:
> +    $ref: /schemas/types.yaml#/definitions/uint64-array
> +    minItems: 4
> +    maxItems: 4

[ ... ]

> +  sitime,output-pll-map:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 8
> +    maxItems: 12

[ ... ]

> +allOf:
> +  - $ref: /schemas/dpll/dpll-device.yaml#
> +  # SiT95317 exposes 8 outputs, SiT95316 exposes 12.  Bound the
> +  # output-pll-map length to the variant so a SiT95317 node cannot
> +  # describe more outputs than the part has.
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: sitime,sit95317
> +    then:
> +      properties:
> +        sitime,output-pll-map:
> +          maxItems: 8
> +  # SiT95316 has 12 outputs.  When the map is supplied it must describe
> +  # all of them, otherwise the trailing outputs are left ambiguous.
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: sitime,sit95316
> +    then:
> +      properties:
> +        sitime,output-pll-map:
> +          minItems: 12

[Severity: Low]
This isn't a bug in the strict sense, but the per-variant bounds here only
constrain the vendor properties, while the properties inherited from
dpll-device.yaml that describe the same hardware stay unbounded.  Was that
intentional?

The description states 4 PLLs, 8 selectable inputs, and 8 outputs
(SiT95317) or 12 outputs (SiT95316).  sitime,pll-fvco is pinned to exactly
4 entries and sitime,output-pll-map is bounded per variant, but
dpll-types in dpll-device.yaml has no length constraint:

  dpll-types:
    description: List of DPLL channel types, one per DPLL instance.
    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
    items:
      enum: [pps, eec]

so a node for this 4-PLL part can list five or more channel types and still
pass dt_binding_check.  Would adding dpll-types with maxItems: 4 here make
that match the hardware?

Similarly, pin subnodes come from dpll-pin.yaml, which deliberately leaves
the hardware index open:

  reg:
    description: Hardware index of the DPLL pin.
    maxItems: 1

With no per-variant maximum, input-pins/pin@9 validates cleanly even though
only 8 inputs are individually selectable, and a SiT95317 node can declare
output-pins/pin@8 while the same DTS is limited to an 8-entry
sitime,output-pll-map, i.e. the two descriptions can silently disagree.
Since sit9531x_prop_pin_fwnode_get() in drivers/dpll/sit9531x/prop.c looks
up the child whose reg matches a driver-generated hardware index, an
out-of-range pin node is silently ignored rather than rejected, so a
mistyped index gives no diagnostic.  Would per-variant reg maxima on the
input-pins and output-pins children be worth adding alongside the
output-pll-map constraints?

For context, the already merged microchip,zl30731.yaml (5 channels, 10
inputs, 20 outputs) is unbounded in the same way, so this looks like
existing looseness in the dpll bindings rather than something specific to
this patch.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915000015.80480-1-arouhi%40sitime.com