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

[Ali Rouhi]

Replies inline.

> How wide is the TDC sample meant to be?  Three places in this patch
> disagree. [and: the 0x07 is the only unnamed field mask]

Fixed in v9: the width is stated consistently (code[34:0] plus sign),
the regs.h annotation is corrected, and the magnitude mask has a named define
(SIT9531X_TDC_MAG_HI_MASK) next to SIT9531X_TDC_SIGN_BIT.

> Is "Fvco==0 means DIVN is not programmed" the only case?  [bus error becomes
> a successful 0 ps measurement]

With sit9531x_get_fvco() reworked to
propagate read failures (see the patch 08 reply), a bus error inside the Fvco
derivation is returned as an error like the trigger/data read failures, not
published as a 0 ps measurement.

> The comment states the conversion as tdc_code * 1e12 / fvco, but the code
> truncates the denominator to whole MHz first.

Fixed in v9: the conversion uses mul_u64_u64_div_u64() with the exact
Hz denominator, removing the ~40 ppm scale error; the MHz truncation and its
overflow rationale are gone.

> Where is that debugfs operation?

The sentence refers to a
planned debug facility that is not in this series.  The comment is
reworded in v9 so the rationale (a netlink read must not freeze the loop, so
the callback reports the closed-loop residual) stands on its own without
pointing at code that does not exist.

> If a reading taken in that state "is simply not meaningful", what stops it
> from being reported as a measurement?

The gate the driver applies is selection: only the pin the PLL is
actually tracking reports a phase offset (others report zero), and in v9 the
FFO getter applies the stricter full-selection gate as well.  Whether the PLL
drives any output does not change the validity of the input-side TDC residual
-- the comparison is between the reference and the feedback path, not the
output drivers -- so we do not think ZDB/OUT_MAP gating belongs here.  The v9
changelog describes the measurement's scope.

> Can the pin the sample is attributed to differ from the pin the TDC actually
> measured?

The window is one poll period (500 ms) after an autonomous
switchover, the same staleness bound every cached attribute in the driver has
(lock status, mode, LOS/OOF).  Re-reading the active-select register inside
the sampling lock would close it for this one attribute while every
neighbouring attribute keeps the poll-period bound, at the cost of an extra
paged read per pin-get; we prefer the uniform bound.  After a switchover the
next poll corrects the attribution, and the misattributed sample is still a
real TDC reading of the same PLL.

> The value cached here is in ABI units (1/1000 ps), but the field it is
> stored in is documented as picoseconds

Fixed in v9 -- the field comment now states the ABI unit
(1/DPLL_PHASE_OFFSET_DIVIDER ps).