Re: [PATCH net-next v8 04/15] dpll: sit9531x: read DPLL types and pin properties from system firmware
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-08 12:45:13
Also in:
linux-devicetree, lkml
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 1 · Low: 5 - [Medium] sit9531x_pin_props_get() fabricates the pin's advertised frequency capability set instead of deriving it from firmware: after… - [Low] Concern (largely INVALID at the end of the series): the per-pin 'hardware state' (sitdev->ref[].freq / sitdev->out[].freq and the… - [Low] sit9531x_pin_props_get() accepts two 'connection-type' spellings that the device-tree binding forbids: it compares the property against… - [Low] Concern (verified as practically unreachable): supported- frequencies-hz is a 64-bit DT property and dpll_pin_frequency.min/max… - [Low] The XO (crystal) input pin has two conflicting sources of truth for its frequency. - [Low] sit9531x_pin_props_get() ignores the return value of fwnode_property_read_u64_array() for "supported-frequencies-hz": it… -- dpll: sit9531x: read DPLL types and pin properties from system firmware This patch adds drivers/dpll/sit9531x/prop.c, which builds the per-pin dpll_pin_properties (package label, type, capabilities, supported frequency list) and the per-channel dpll_type from the firmware node, falling back to defaults when the node does not describe them. A few questions below about the frequency list and the connection-type parsing.
quoted hunk ↗ jump to hunk
diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c new file mode 100644 index 000000000000..94f3da716297 --- /dev/null +++ b/drivers/dpll/sit9531x/prop.c@@ -0,0 +1,406 @@
[ ... ]
quoted hunk ↗ jump to hunk
+ } else if (dir == DPLL_PIN_DIRECTION_INPUT && + index == SIT9531X_MAX_INPUTS) { + /* The XO reference is fixed: no state or priority control. */ + props->dpll_props.type = DPLL_PIN_TYPE_INT_OSCILLATOR; + props->dpll_props.capabilities = 0; + sitdev->ref[index].freq = sitdev->xtal_freq; + curr_freq = sitdev->xtal_freq;
[Severity: Low]
Does the XO pin end up with two sources of truth for its frequency here?
The branch seeds sitdev->ref[SIT9531X_MAX_INPUTS].freq from
sitdev->xtal_freq (the clk_get_rate() value read at probe), but unlike the
two INTSYNC pins the XO index is not excluded from the firmware-node path
further down in sit9531x_pin_props_get().
dpll-device.yaml allows an input-pins child for this index:
patternProperties:
"^pin@[0-9a-f]+$":
$ref: /schemas/dpll/dpll-pin.yaml
so a schema-valid DT can supply supported-frequencies-hz for pin 8, and the
seeding block later overwrites ref[8].freq (and curr_freq) with freqs[0].From then on sit9531x_dpll_input_pin_frequency_get() reports one crystal
rate to userspace while all the divider and VCO math keeps using sitdev->xtal_freq. Would it be simpler to skip the frequency-seeding path for index SIT9531X_MAX_INPUTS the same way both INTSYNC pins already are? [ ... ]
quoted hunk ↗ jump to hunk
+ /* Look for "connection-type" property -> pin type enum */ + if (!fwnode_property_read_string(props->fwnode, "connection-type", + &type)) { + if (!strcmp(type, "ext")) + props->dpll_props.type = DPLL_PIN_TYPE_EXT; + else if (!strcmp(type, "gnss")) + props->dpll_props.type = DPLL_PIN_TYPE_GNSS; + else if (!strcmp(type, "int") || + !strcmp(type, "int-oscillator")) + props->dpll_props.type = DPLL_PIN_TYPE_INT_OSCILLATOR; + else if (!strcmp(type, "synce") || + !strcmp(type, "synce-eth-port")) + props->dpll_props.type = DPLL_PIN_TYPE_SYNCE_ETH_PORT;
[Severity: Low]
Can the "int-oscillator" and "synce-eth-port" comparisons ever match?
Documentation/devicetree/bindings/dpll/dpll-pin.yaml restricts the property
to a closed set:
connection-type:
description: Connection type of the pin
$ref: /schemas/types.yaml#/definitions/string
enum: [ext, gnss, int, mux, synce]
and sitime,sit95316.yaml inherits that pin schema through dpll-device.yaml.
A DT using either alias fails dt_binding_check/dtbs_check, so those two
comparisons look like dead code for any schema-valid DT. Should they just
be dropped, or should the binding gain the aliases?
quoted hunk ↗ jump to hunk
+ num_freqs = fwnode_property_count_u64(props->fwnode, + "supported-frequencies-hz"); + if (num_freqs <= 0) { + num_freqs = 0; + goto skip_fwnode_props; + } + + freqs = kcalloc(num_freqs, sizeof(*freqs), GFP_KERNEL); + if (!freqs) { + rc = -ENOMEM; + goto err_alloc_freqs; + } + + fwnode_property_read_u64_array(props->fwnode, + "supported-frequencies-hz", + freqs, num_freqs);
[Severity: Low] Should the return value of fwnode_property_read_u64_array() be checked before freqs[] is consumed? For device tree the element count and the array read parse the same property length and cannot disagree. For an ACPI-described device (this driver can still be instantiated through PRP0001, which matches on of_match_table and takes properties from _DSD), acpi_data_prop_read() returns the package element count without type-checking each element, while acpi_copy_property_array_uint() fails with -EPROTO on the first non-integer element. Since the failure is discarded, the remaining kcalloc zeros are then used as data: ref[index].freq / out[index].freq and curr_freq get freqs[0], and every entry including the zeros is published as a supported range.
quoted hunk ↗ jump to hunk
+ /* + * Seed the runtime ref->freq / out->freq with the first DT-listed + * supported frequency so the netlink frequency_get callback reports + * a sane initial value before any pin_set occurs. DT lists the + * physically-wired reference frequency for each input pin and the + * default output frequency for each output pin. + */ + if (num_freqs > 0) { + if (dir == DPLL_PIN_DIRECTION_INPUT) + sitdev->ref[index].freq = (u32)freqs[0]; + else + sitdev->out[index].freq = (u32)freqs[0]; + curr_freq = (u32)freqs[0]; + } + +skip_fwnode_props:
[ ... ]
quoted hunk ↗ jump to hunk
+ /* Allocate frequency ranges list -- DT discrete entries + current + * freq + one catch-all wide range so the subsystem never pre- + * rejects a frequency_set call. The chip's real admissible set + * is bounded by VCO / divider math in sit9531x_output_freq_set(). + */ + ranges = kcalloc(num_freqs + 2, sizeof(*ranges), GFP_KERNEL); + if (!ranges) { + rc = -ENOMEM; + goto err_alloc_ranges; + } + + /* Current freq as first entry */ + ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq); + j = 1;
[ ... ]
quoted hunk ↗ jump to hunk
+ /* Always append a wide catch-all range */ + ranges[j].min = 1; + ranges[j].max = 1000000000ULL; /* 1 GHz */ + j++; + + props->dpll_props.freq_supported = ranges; + props->dpll_props.freq_supported_num = j;
[Severity: Medium]
Does the appended catch-all range make the firmware-declared frequency list
meaningless, and does it publish a capability set the driver cannot honour?
The core treats every advertised range as genuinely supported, both when
validating a set and when exporting the list to userspace:
drivers/dpll/dpll_netlink.c:dpll_pin_is_freq_supported() {
for (fs = 0; fs < pin->prop.freq_supported_num; fs++)
if (freq >= pin->prop.freq_supported[fs].min &&
freq <= pin->prop.freq_supported[fs].max)
return true;
return false;
}
and it is the only generic pre-validation of a userspace request:
drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
if (!dpll_pin_is_freq_supported(pin, freq)) {
NL_SET_ERR_MSG_ATTR(extack, a, "frequency is not supported by the device");
return -EINVAL;
}
With {1, 1000000000} counted in freq_supported_num for every non-INTSYNC
pin, the DT-listed entries never constrain anything, and the same range is
attached to the XO pin whose capabilities are deliberately 0 and to every
input pin, which has no .frequency_set at all.
The array is also copied into the pin by dpll_pin_prop_dup() and exported
through DPLL_A_PIN_FREQUENCY_SUPPORTED, so 1 Hz..1 GHz becomes what
userspace is told this device supports.
Separately, ranges[0] is always DPLL_PIN_FREQUENCY(curr_freq), and
curr_freq stays 0 for any pin with no firmware node or no
supported-frequencies-hz list (both permitted by dpll-device.yaml and
dpll-pin.yaml), so those pins advertise 0 Hz as a supported frequency.
The commit message says:
"which frequencies a board actually presents on a given input, and
whether an output's embedded sync is wired to be controllable, are
board facts -- so they come from the firmware node"
Is that consistent with the catch-all range, which overrides the board
facts for every pin?
For reference, Documentation/devicetree/bindings/dpll/dpll-pin.yaml
describes the property as the pin's supported set:
supported-frequencies-hz:
description: List of supported frequencies for this pin, expressed in Hz.
Would it work to advertise only the firmware-listed entries (plus the
current frequency) and let sit9531x_output_freq_set() reject the rest with
-EINVAL, rather than widening the advertised set?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902214030.20955-1-arouhi%40sitime.com