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

[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: 1 · Low: 2

- [Medium] New DT property `sitime,pll-fvco`
  (Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml:51) omits…
- [Low] Concern (assessed invalid): the MAINTAINERS section "SITIME
  SIT9531X DPLL DRIVER" added at MAINTAINERS:25160 lists only the…
- [Low] Concern (assessed invalid): the schema does not bound DPLL
  channel count (`dpll-types` has no maxItems: 4) nor per-variant…

--

dt-bindings: dpll: add SiTime SiT95316 clock generator

Adds a new DT binding for the SiTime SiT95316/SiT95317 I2C clock
generators, described as a DPLL provider with four PLLs, eight
selectable inputs and up to twelve outputs, plus two vendor properties
(sitime,pll-fvco and sitime,output-pll-map).  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 000000000000..b3cf9d5a8267
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml
> @@ -0,0 +1,171 @@

[ ... ]

> +  sitime,pll-fvco:
> +    $ref: /schemas/types.yaml#/definitions/uint64-array
> +    minItems: 4
> +    maxItems: 4
> +    description:
> +      Per-PLL VCO frequency in Hz for PLLA, PLLB, PLLC, PLLD.  Neither
> +      standard frequency suffix fits.  The VCO bands are 4915.2 MHz to
> +      5898.24 MHz and 6875 MHz to 7812.5 MHz, so every valid value
> +      exceeds the 32 bits "-hz" allows, and the band edges are not
> +      whole megahertz either, so "-mhz" cannot express them.  Hence a
> +      plain uint64-array.  Override where the standard

[Severity: Medium]
Is the statement that "every valid value exceeds the 32 bits "-hz" allows"
correct?  A "-hz" property can carry 64-bit values, so the stated reason for
dropping the unit suffix does not seem to hold.

The peer binding in the same directory does exactly that:

Documentation/devicetree/bindings/dpll/microchip,zl30731.yaml
            supported-frequencies-hz = /bits/ 64 <10000 10000000>;

And this very file does it too, in its own examples:

                    supported-frequencies-hz = /bits/ 64 <156250000>;

Documentation/devicetree/bindings/opp/opp-v2-base.yaml also documents this
explicitly:

      opp-hz:
        description:
          Frequency in Hz, expressed as a 64-bit big-endian integer.

Since the values are already in Hz, could the property be named
sitime,pll-fvco-hz while keeping

    $ref: /schemas/types.yaml#/definitions/uint64-array

and the 32-bit rationale dropped from the description?  Once merged, the
suffix-less spelling becomes DT ABI and cannot be renamed later.

> +      Fvco = Fref * DIVN derivation does not match the running VCO --
> +      for example a PLL operating in INTSYNC mode.  A value of 0 keeps
> +      the register-derived computation for that PLL.

[ ... ]

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