Re: [PATCH net-next v14 04/12] 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] On the SiT95317, sit9531x_prop_pin_package_label_set() builds
  the user-visible DPLL package_label as "OUT%u" from…

--

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

This commit adds prop.c, which builds DPLL device types and per-pin
properties for the SiT9531x from the firmware node. The properties are
package and board labels, pin type, capabilities and supported
frequencies, with defaults when the node does not describe them. Input
labels are derived from the pair and lane layout, and output labels from
the variant's clkout_map.

> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> new file mode 100644
> index 0000000000000..8dd1d3fefbb54
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/prop.c

[ ... ]

> +	if (dir == DPLL_PIN_DIRECTION_INPUT)
> +		sit9531x_input_pin_label(sitdev, props, id);
> +	else
> +		/*
> +		 * Name the chip pin, not the driver's index for it.  The
> +		 * two differ on the variant whose outputs are bonded out
> +		 * from a subset of the twelve slots, and a package label
> +		 * that named the index would point at a pin that is not
> +		 * the one being driven.
> +		 */
> +		snprintf(props->package_label, sizeof(props->package_label),
> +			 "OUT%u", sitdev->info->clkout_map[id]);

[Severity: Medium]
On the SiT95317, does this label name the package pin from the
datasheet, or the internal die slot?

In sit9531x_prop_pin_package_label_set() the label comes from
clkout_map[id], and in core.c that is:

    static const u8 clkout_map_95317[] = {0, 3, 4, 5, 7, 8, 9, 11};

So the 8 outputs are advertised in DPLL_A_PIN_PACKAGE_LABEL as OUT0,
OUT3, OUT4, OUT5, OUT7, OUT8, OUT9 and OUT11.

The sitime,sit95316.yaml binding in this series describes the vddoN
supplies like this:

    The SiT95317 has one per output, VDDO0 to VDDO7, numbered as that
    part's datasheet numbers its outputs; in the pin numbering of this
    binding those are outputs 0, 3, 4, 5, 7, 8, 9 and 11, in that order.

If that is right, the 95317 datasheet numbers its outputs 0..7. Labels
such as OUT9 and OUT11 would then not exist on that package.

Some labels would also point at a different physical pin. Driver index 3
gets the label OUT5, but the datasheet's OUT5 is driver index 5, which
gets the label OUT9.

The top-level binding text ("the SiT95317 bonds out OUT0, OUT3, ...")
uses the slot naming instead, so the two descriptions disagree. Which
numbering does the 95317 datasheet use?

If it uses 0..7, should the package label use the driver index (or a
separate per-variant package name table) on this variant? The label is
visible to userspace through netlink, so it would be hard to change once
released. This code is unchanged at the end of the series.

> +
> +	props->dpll_props.package_label = props->package_label;
> +}

[ ... ]

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