Re: [PATCH net-next v11 12/13] dpll: sit9531x: add support to get phase offset on the connected input pin

[Ali Rouhi]

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

Replies inline, in the order of the summary list. Four are fixed; the
third is fixed in its loss-of-signal half and answered in its lock half.

> [Severity: Medium]
> What limits the size of the quotient here?
>
> [...]
>
> Should the TDC conversion get a similar bound, for example by rejecting
> an fvco outside the VCO band? Later patches in the series don't appear to
> change this code.

Fixed by the VCO floor described on 09/13: sit9531x_get_fvco() reports no
data below the low band, so the conversion never runs on a rate small
enough to overflow the divide.

> [Severity: Medium]
> Should this loop check sitdev->out[i].state_stale?
>
> [...]
>
> Could stale outputs be refreshed here too, the way the output pin getter
> does it?

Fixed: an output whose cached state is marked stale is read back from the
device before it counts as driving, and a failed read fails the request
with the same message the output getter uses.

> [Severity: Medium]
> Do these two live reads catch the case they are meant to catch?
>
> [...]
>
> The callback comment says the live read exists because "a cache up to a
> poll period old could attribute a live measurement to the pin that used
> to be selected". Should lock, holdover and LOS for this pin also be read
> live around the sample? Later patches in the series don't appear to
> change this code.

The selection is re-read live because it is the one thing that moves the
measurement to another pin. Lock and holdover do not: a sample taken
after an unlock is still the phase between this pin and the DPLL, which
is the quantity the attribute defines, and a value during holdover is the
drift against the reference the pin carries.

The loss-of-signal case is different and the point holds: a lane that
lost its clock after the last poll makes the device fall back on its own,
and the sample would be credited to a pin the PLL is no longer on. The
lane monitors are now read live before the pin is judged active, so that
check is live as well.

> [Severity: Low]
> The commit message says:
>
> [...]
>
> Could the commit message also say that the connected pin reads 0 when no
> sample was taken? That way a 0 on that pin isn't taken as a measured
> value.

Fixed in the commit message: the selected pin reads zero too when no
sample was taken -- the PLL is not tracking, drives no un-muted output,
has no usable VCO rate, or its selection or the lane's signal moved
between the check and the sample.

> [Severity: Low]
> Is the reason given in this comment accurate? One TDC code is
> 1e12 / fvco ps, which is usually not a whole number. A 4.9152 GHz VCO
> gives about 203.45 ps per code, and a fractional-DIVN Fvco is usually
> not a round value either.
>
> [...]
>
> Documentation/driver-api/dpll.rst defines a 3-digit fractional part for
> this value. Could the conversion compute |tdc| * 1e15 / fvco, with an
> overflow bound, so the fraction is kept? If not, could the comment be
> corrected?

Fixed: the conversion keeps the fraction of a picosecond the converter
resolves, and the multiply by the attribute's divider that followed the
truncation is gone.