Re: [PATCH net-next v11 13/13] dpll: sit9531x: model the inter-PLL sync net as a pair of pins

[Ali Rouhi]

On Mon, 5 Oct 2026, netdev-bot+sashiko@xxxxxxxxxx wrote:

Replies inline, in the order of the summary list.

> [Severity: Medium]
> Will this report STANDBY even when no PLL drives the net?
>
> [...]
>
> Should the destination pin report NO_SIGNAL when intsync_src < 0?

Fixed: a destination pin whose net no PLL drives reports NO_SIGNAL, as an
external input that lost its clock does, and is not active. The poll's
change check goes through the same helper.

The test is not intsync_src < 0 on its own. A source the driver enabled,
or whose configuration its scan found already in place, says the net is
driven; a source found only partly configured does not. Past that, a
loaded profile can drive the net with a configuration of its own, so a
PLL the device feeds from the net and holds locked also counts as the net
carrying a signal.

> [Severity: Medium]
> If the request fails but this re-scan moves intsync_src, what sends the
> pin change notification?
>
> [...]
>
> Without the same handling here, won't subscribers keep the stale SYNCOUT
> state?

Fixed: the source setter takes whether this PLL owns the net before and
after the request and sends a pin-change notification when a failed
request moved it, which is what the output state setter already did.

> [Severity: Medium]
> If only this last step fails, should restore_global still set the global
> bit again?
>
> [...]
>
> The EXT page then holds no enable pattern, so detect reports no owner.
> The global enable stays asserted, and the driver has no owner on record
> that would ever clear it.

Fixed, both halves. A failure of the final per-PLL latch no longer
restores the global enable, since the net is not configured at that
point. The rollback of a failed enable uses a variant that does not
restore it either, so an enable that failed cannot leave a global bit no
PLL owns.

> [Severity: Medium]
> What happens to the cached owner if this rollback also fails?
>
> [...]
>
> Isn't this the case the setter's comment says it has to prevent ("a cache
> that wrongly says nobody drives the net would let a second PLL be
> configured to drive it")?

Fixed: when the scan after a failed enable fails too, the requesting PLL
is recorded as a partial owner, so a retry re-runs the enable instead of
being refused as busy while the configuration sits half in place.

> [Severity: Medium]
> Does this write need a page-0 small update after it?
>
> [...]
>
> Later, an unrelated priority-table change on any PLL ends in
> sit9531x_prio_prg_commit(), which also writes SIT9531X_REG_GLOBAL_UPDATE.
> Could that latch the pending bit and turn inter-PLL sync back on over a
> half-written EXT page, without anyone requesting it?

Fixed: the restore write is followed by the page-0 small update and its
settle, as every other write to that register is.

> [Severity: Low]
> Once this runs, is there any way to tell a half-configured source from a
> fully connected one?
>
> [...]
>
> It returns success without running the enable sequence again. Does the
> configuration ever get completed in that case?

That is what this revision changed: the probe-time scan records a partly
configured source as partial, the source pin reports it disconnected, and
a connect on it re-runs the enable rather than returning early.

> [Severity: Low]
> Should INTSYNC ownership also be detected again on resume?
>
> [...]
>
> - A's SYNCOUT pin still reports CONNECTED.
> - A CONNECTED on A returns success without programming anything.
> - A CONNECTED on another PLL fails with -EBUSY.
> - A's INTSYNC destination pin is forced to DISCONNECTED.

Fixed: resume re-detects the net's owner and reads every output's mute
state back from the part before the poll and the interrupt are
re-enabled, and marks an output whose read failed for a read-through.