Re: [PATCH net-next v8 10/15] dpll: sit9531x: add support to adjust output phase

[Ali Rouhi]

Replies inline.

> Could the commit message mention the NVM shadow write, the sibling-output
> phase restart, and the ~100 ms cost per call?

Yes -- the v9 changelog describes all three.  The PLL-wide
divider restart is the chip's only phase-apply mechanism; a per-output flush
does not exist in this hardware.

> Should this bail out apply to every request?  [zero cached freq rejects
> everything]

Fixed in v9: when the cache is empty the setter seeds the rate from
the divider chain (the same readback frequency_get uses), so boards whose DT
lists no output frequencies can still adjust phase; the request is only
rejected when the rate cannot be determined at all.

> Should the positive branch also reduce modulo the output period?

Yes -- fixed in v9: both branches reduce modulo the output period, so
a positive request larger than one period no longer programs millions of VCO
cycles of reset delay.

> nothing recomputes it when the frequency changes later.

Fixed in v9: a frequency change re-encodes the cached phase adjust
against the new period, so the programmed delay and the reported value stay
consistent across rate changes.

> Can a mid-sequence I2C failure commit a half-written delay word?

v9 saves the five delay bytes before the write
and restores them on a mid-sequence failure before the commit, the same shape
as the DIVO write in the frequency path.

> Does this match the commit message claim that "what the core reads back is
> what the registers hold rather than what was asked for"?

The getter reports the administrative request, which is the
convention the cache implements; the v9 changelog is reworded to say so.
Sub-step quantisation is bounded by the 30 ps fine step and the request is
preserved exactly in the cache, so the round trip is stable; reporting the
quantised register value instead would make a set followed by a get disagree
with what userspace asked, which the core's equality short-circuit then turns
into an un-clearable state.

> should struct dpll_pin_properties::phase_gran be populated too?

Yes -- fixed in v9: phase_gran is populated so
PHASE_ADJUST_MIN/MAX/GRAN are all reported as driver-api/dpll.rst prescribes.
It is set to 1 ps rather than the 30 ps fine step deliberately: requests are
accepted at picosecond resolution and encoded to the nearest achievable
coarse+fine delay, so the request granularity, not the hardware step, is what
the attribute must gate.

> Could both comments be reworded in terms of the slot [...]?

Fixed in v9 -- both blocks are stated in terms of the physical slot.