Re: [PATCH net-next v11 03/13] dt-bindings: dpll: add SiTime SiT95316 clock generator

[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] Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml
  defines no power-supply properties (for example a core VDD supply and…

--

dt-bindings: dpll: add SiTime SiT95316 clock generator

This adds a devicetree binding for the SiTime SiT95316/SiT95317 clock
generators. They are I2C-controlled DPLL providers with four PLLs, up to
eight inputs and up to twelve outputs. The XO is described either as a
clock or as a clock-frequency property.

> diff --git a/Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml b/Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml
> new file mode 100644
> index 0000000000000..997b410e8f526
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - sitime,sit95316
> +      - sitime,sit95317
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: xtal
> +
> +  clock-frequency:
> +    description:
> +      XO rate in Hz feeding XIN/XO_CLK, for firmware that describes the
> +      oscillator as a property rather than as a clock provider, such as
> +      ACPI.  Use either this or "clocks", not both.
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      GPIO connected to the chip's active-low reset pin (RESETB).
> +
> +  interrupts:
> +    maxItems: 1
> +    description:
> +      Interrupt from the chip's active-low INTRB output.  Asserted when
> +      the device detects a status change such as lock acquisition or loss.
> +
> +  dpll-types:
> +    maxItems: 4
> +
> +  input-pins:

[Severity: Low]
This isn't a bug, but should the binding describe the chip's power
supplies here? For example a core VDD supply and the per-bank VDDO
output supplies.

The schema ends with:

    unevaluatedProperties: false

With that, a board that adds a *-supply property to the dpll node fails
dt_binding_check. A board that powers the part from switchable regulators
then has no valid way to describe those rails. Without them, a driver
cannot make sure the rails are on before it releases RESETB and starts
I2C access during probe.

Documentation/devicetree/bindings/writing-bindings.rst says:

    DO attempt to make bindings complete even if a driver doesn't
    support some features.

The supply names were not checked against the SiTime datasheet. Any
supply properties added should use the chip's actual VDD/VDDO pin
names.

The existing microchip,zl30731.yaml DPLL binding also leaves out
supplies, so there is precedent for this in the subsystem. Optional
supply properties could also be added later without breaking existing
DTs.

[ ... ]

> +unevaluatedProperties: false

[ ... ]

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