Re: [PATCH net-next v11 05/13] dpll: sit9531x: read DPLL types and pin properties from system firmware

[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: 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] sit9531x_pin_props_get() in drivers/dpll/sit9531x/prop.c takes
  the first entry of an input pin's "supported-frequencies-hz" as the…

--

dpll: sit9531x: read DPLL types and pin properties from system firmware

This commit adds prop.c to the sit9531x driver. It builds DPLL pin
properties (package and board labels, pin type, capabilities and supported
frequencies) from the input-pins/output-pins firmware nodes, and uses
defaults when a node is missing. It also reads per-channel DPLL types from a
"dpll-types" property, and adds input/output state arrays and pair/lane
helpers to core.h.

> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> new file mode 100644
> index 0000000000000..3635ea497e415
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/prop.c
> @@ -0,0 +1,427 @@

[ ... ]

> +struct sit9531x_pin_props *
> +sit9531x_pin_props_get(struct sit9531x_dev *sitdev,
> +		       enum dpll_pin_direction dir, u8 index)
> +{

[ ... ]

> +	/*
> +	 * Seed an input's runtime ref->freq with the first DT-listed
> +	 * supported frequency: for an input the board lists the rate that is
> +	 * physically wired to it first.  An output's current rate is what its
> +	 * divider produces, not an entry of the list it may be set to.
> +	 */
> +	if (num_freqs > 0 && dir == DPLL_PIN_DIRECTION_INPUT &&
> +	    index != SIT9531X_MAX_INPUTS)
> +		curr_freq = freqs[0];

[Severity: Medium]
Do the bindings define an order for supported-frequencies-hz?

The generic binding in dpll-pin.yaml describes it only as "List of
supported frequencies for this pin, expressed in Hz.". The input-pins
section of sitime,sit95316.yaml documents the reg numbering, but it says
nothing about the first entry being the rate wired to the input. The commit
message doesn't mention this rule either. It only appears in the comment
above.

The value picked here is then cached further down in
sit9531x_pin_props_get():

    if (dir == DPLL_PIN_DIRECTION_INPUT)
        sitdev->ref[index].freq = curr_freq;

and it is listed first in freq_supported. Later in the series, "dpll:
sit9531x: add support to get and set frequency on pins" makes
sit9531x_dpll_input_pin_frequency_get() report it directly:

    ref = sit9531x_ref_state_get(sitdpll->dev, dpin->id);
    *frequency = ref->freq;

Input pins have no frequency setter that could correct it. That commit's
message says only that the rate is "reported from the firmware
description".

Suppose a board DT lists an input's frequencies in some other order, for
example ascending. It would pass dt_binding_check, but the driver would
report an input frequency over DPLL netlink that the board does not
present.

Since this ordering would become de facto DT ABI once boards ship, could the
binding document it? Two other options: carry the wired rate in a separate
property, or stop inferring the current rate from its position in the list.

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