Re: [PATCH net-next v8 00/15] dpll: add SiTime SiT9531x DPLL clock driver

[Paolo Abeni]

On 9/2/26 11:40 PM, Ali Rouhi wrote:
> This series adds a DPLL subsystem driver for the SiTime SiT95316 and
> SiT95317 I2C clock generators. Each device integrates four PLLs with
> automatic reference selection and on-chip TDC phase-offset measurement,
> and is used for synchronization in telecom, networking, and data-center
> timing.
> 
> The series contains the device-tree binding, the driver under
> drivers/dpll/sit9531x/, and the MAINTAINERS entry.
> 
> v1: https://lore.kernel.org/netdev/20260511211143.19792-1-arouhi@xxxxxxxxxx/
> v2: https://lore.kernel.org/netdev/20260520191943.73938-1-arouhi@xxxxxxxxxx/
> v3: https://lore.kernel.org/netdev/20260731180951.65725-1-arouhi@xxxxxxxxxx/
> v4: https://lore.kernel.org/netdev/20260806232439.27551-1-arouhi@xxxxxxxxxx/
> v5: https://lore.kernel.org/netdev/20260810230439.22866-1-arouhi@xxxxxxxxxx/
> v6: https://lore.kernel.org/netdev/20260812175337.18155-1-arouhi@xxxxxxxxxx/
> v7: https://lore.kernel.org/netdev/20260815221919.64226-1-arouhi@xxxxxxxxxx/
> 
> The driver was a single 5.6k-line patch through v7. Vadim and Ivan asked
> for it to be split into logical parts, with the first zl3073x series as
> the model, so v8 introduces it feature by feature instead:
> 
>   1-2   bindings: vendor prefix, then the device schema
>   3     basic support: paged regmap, variant detection, probe
>   4     DPLL types and pin properties from system firmware
>   5     register the DPLL devices and pins, and keep their state
>   6     input pin state on a DPLL, with the pin-state contract
>   7     input pin priority
>   8     pin frequency, both directions
>   9     output pin state (mute)
>   10    output phase adjust
>   11    esync on outputs
>   12    phase offset through the TDC
>   13    fractional frequency offset of the selected reference
>   14    the inter-PLL sync net as a pair of pins
>   15    optional device-tree overrides for two board facts
> 
> The two bindings patches come first, so the driver never matches on a
> compatible string before the schema that describes it is in the tree.
> 
> Each of the thirteen driver patches was built on its own, with W=1 on
> x86_64 and i386, and is sparse clean. Each one also links: no patch
> calls something that a later patch introduces, so the module builds at
> every step of the series and a bisect cannot land on a tree that fails
> to compile.
> 
> The split makes no functional change of its own. The tree this series
> ends at compiles to objects byte-identical to the tree the single patch
> produced once the fixes below are applied to it.
> 
> Changes in v8:
> 
>   - Split the driver patch into the thirteen patches listed above.
> 
>   - Advertise automatic mode only. v7 mapped DPLL_MODE_MANUAL onto the
>     chip's free-run bit and then refused DPLL_PIN_STATE_CONNECTED on
>     input pins, so it offered the mode whose one purpose is manual
>     selection and refused the only request that mode exists for.
>     Free-run selects no input at all, so on this device it is a lock
>     state, not a selection mode. v8 advertises AUTOMATIC only and
>     refuses anything else from .mode_set(), which now clears the outer
>     loop disable bit so a PLL left free-running by the loaded
>     configuration returns to selecting from the priority table.
>     Free-run is reported through lock status as UNLOCKED.
> 
>     The device can also pin one reference while the loop keeps running,
>     so real MANUAL support is possible later. It is left out here
>     because the mode mapping becomes ABI once merged, and it belongs in
>     the same series as the userspace that needs it rather than being
>     approximated now.
> 
>   - State the pin-state contract once, above the pin ops, and let the
>     two selection-role pins share one implementation of it. The driver
>     had five ops tables and five hand-written encodings of the same
>     state machine, and every review round has found another place where
>     two of them disagreed. This closes several findings at once:
>     CONNECTED reported while the outer loop was disabled, membership
>     taken from the shared-receiver refcount rather than from the
>     priority table, SELECTABLE on an INTSYNC destination userspace had
>     disconnected, .prio_set() inserting a source that was never in the
>     table, and .prio_remove() returning 0 when the disconnect would
>     have emptied it. The last two are now -EINVAL and -EBUSY.
> 
>   - Report LOCKED_HO_ACQ only once the holdover estimate is valid. It
>     was asserted whenever the loop was locked, without reading the bit
>     that says the device has accumulated a frequency estimate good
>     enough to fall back on. That bit is distinct from the one that says
>     the PLL has already switched over to holdover.
> 
>   - Fix the 32-bit build. The output phase adjust reduced a negative
>     request modulo the output period with % on two u64 values, leaving
>     the module with an undefined __umoddi3; it now uses div64_u64_rem(),
>     which the rest of the arithmetic there already uses. Reported by
>     Jakub Kicinski.
> 
>   - Correct the register model comment: the page selector is at 0xFF and
>     a page holds 256 registers, as the definitions right below the
>     comment already said. Reported by Ivan Vecera.
> 
>   - Type ordinary clock outputs as DPLL_PIN_TYPE_EXT instead of
>     DPLL_PIN_TYPE_GNSS, which claimed a GNSS-disciplined timing port.
>     Advertise a phase-adjust range on outputs only, and drop the input
>     pins' .phase_adjust_get with it: inputs have no .phase_adjust_set,
>     so a non-zero range promised something every set would refuse.
> 
>   - Always issue the loop lock in the programming commit, so a transient
>     I2C error cannot leave the chip in PRG_CMD with its loops open. Undo
>     the global enable when the INTSYNC enable fails partway. Take
>     multiop_lock before testing the selected reference in the frequency
>     offset getter, as the sibling phase-offset getter already does.
> 
>   - Latch every read of the chip's debug block the way the phase read
>     does. That block returns a sample of an internal measurement tap,
>     selected by a read code and latched by reading a trigger register.
>     The generic helper read the trigger once where the phase path reads
>     it three times, and a single read returns the previous latch. On
>     hardware this showed as a frequency offset that would not move while
>     a phase measurement of the same PLL tracked a temperature step
>     cleanly.
> 
>   - Report which PLLs came up and which EEPROM profile was loaded, and
>     warn when a PLL drives outputs without having reached its active
>     state, or when the profile's stored and computed CRCs disagree. In
>     both cases every register still reads back a plausible value, so
>     nothing about the failure is visible until the measurements come out
>     wrong.
> 
>   - Say what the phase offset getter measures, next to the callback: the
>     loop's residual error sampled with the loop closed, which is the
>     quantity the ABI asks for -- not the vendor's open-loop
>     phase-difference procedure, which would need the digital loop filter
>     frozen and would leave the PLL undisciplined until something
>     released it. A netlink read must not do that.
> 
>   - Drop fifteen register definitions that have never had a caller.
>     Eleven describe that open-loop sequence and the PLL restart path,
>     neither of which the driver performs. The other four -- the PLL page
>     macro and the three DIVN2 fields -- had no user in any patch of the
>     split, so there was no honest patch to introduce them in. Each will
>     come back with its first user.
> 
>   - Binding: put the example VCO frequency inside a band. 6251500000 sat
>     in the gap between the device's two bands, which the property's own
>     description rules out; it is now 6900000000, which a shipping
>     profile on our SiT95316 board runs. This is the only change to the
>     binding since v6 -- the property and its description are unchanged
>     -- so Krzysztof's Reviewed-by is carried across it.
> 
> The AI review's use-after-free report on the pin properties remains a
> false positive: dpll_pin_alloc() calls dpll_pin_prop_dup(), which copies
> freq_supported with kmemdup() and the labels with kstrdup(), so the core
> owns its copies and the driver has to free its own.
Clashiko had several comments, and some of them definitely look real.
Please note that you are supposed to handle such comments proactively,
see commit c82ff94592fb68f529afe63ca7f5ddb7dae4ba83

/P