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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The shared DPLL pin property `supported-frequencies-hz` now
  uses two incompatible cell widths, depending on the device.

--

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 given either as a clock
or as clock-frequency, and the supplies are named after their pins.

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

[ ... ]

> +            input-pins {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                /* IN0P and IN0N: pair 0 runs single-ended */
> +                pin@0 {
> +                    reg = <0>;
> +                    label = "clkin0";
> +                    connection-type = "ext";
> +                    supported-frequencies-hz = <10000000>;

[Severity: Medium]
Should this entry, and the other supported-frequencies-hz entries in this
example, use /bits/ 64 like the existing binding for this shared property?

The property comes from dpll-pin.yaml, which gives it no explicit width:

  supported-frequencies-hz:
    description: List of supported frequencies for this pin, expressed in Hz.

The only other user, microchip,zl30731.yaml, always writes it as 64-bit
cells:

  supported-frequencies-hz = /bits/ 64 <10000 10000000>;

Its driver also reads it as u64:

drivers/dpll/zl3073x/prop.c:zl3073x_pin_props_get() {
    ...
	num_freqs = fwnode_property_count_u64(props->fwnode,
					      "supported-frequencies-hz");
	if (num_freqs <= 0) {
		num_freqs = 0;
    ...
}

This example uses 32-bit cells. The SiT9531x driver later in the series
reads u32 on purpose in sit9531x_pin_props_get():

	/* A -hz property is 32-bit; no rate of this part needs more. */
	num_freqs = fwnode_property_count_u32(props->fwnode, "supported-frequencies-hz");

So the driver in this series and this example agree with each other. The
shared property, though, would now be encoded differently depending on the
device.

Suppose a SiT95316/7 board DT uses the existing /bits/ 64 form, for
example /bits/ 64 <10000000>. Wouldn't sit9531x_pin_props_get() read that
as two u32 values, [0, 10000000]? For an input, curr_freq = freqs[0] would
then be 0, and the wired rate would never reach sitdev->ref[index].freq.

The reverse case is also a problem. A reader that follows the u64
convention, such as zl3073x or a future shared DPLL fwnode helper, would
fail the element count on the 4-byte SiT encoding and silently skip the
property.

It is not clear whether dt_binding_check would reject the 64-bit form
under the maxItems: 1 constraint in input-pins. That depends on how
dtschema decodes -hz properties.

Could the binding and driver use the same 64-bit encoding as
microchip,zl30731? Alternatively, could dpll-pin.yaml define a single
width for this property?

> +                };

[ ... ]

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