Re: [PATCH net-next v8 08/15] dpll: sit9531x: add support to get and set frequency on pins

[Ali Rouhi]

Replies inline.

> Does a single pin set also carry two side effects the message does not
> mention? [NVM shadow write, sibling phase restart, ~100 ms]

The v9 changelog now states that a per-pin frequency set runs
inside the chip's programming state: it updates the NVM shadow, re-locks the
loops with the required settle time, and restarts the output dividers of the
owning PLL phase-aligned.  That is the device's programming model for divider
changes, not something the driver can decompose.

> This comment describes "the previous split between free-run and sync
> formulas" [...] Could the paragraph be dropped [...]?

Dropped in v9 -- the sit9531x_get_fvco() kernel-doc now describes what
the function does, without referring to out-of-tree history.

> Is SIT9531X_PLL_PHFL_ON_DEMAND_EN meant to stay set after the flush?

No -- fixed in v9: the on-demand enable is disarmed after the one-shot
flush, together with the trigger-source restore, so a later assertion of the
restored trigger cannot re-flush the PLL's outputs.

> Is it safe to program a real divider from a guessed VCO rate here? [...]
> Would returning an error when the VCO cannot be read be preferable, and
> should sit9531x_get_fvco() distinguish a bus error from a dormant PLL?

v9 makes sit9531x_get_fvco()
propagate a register-read failure as an error distinct from "DIVN not
programmed", and the frequency setter fails the request instead of programming
a divider from a band-edge guess.

> Can a request that the driver advertised as supported be satisfied at a
> materially different rate here?

The advertised set was the real problem and is fixed
in v9: pins now advertise the concrete firmware-listed rates (plus the current
one), so the normal path validates against rates the divider chain actually
produces.  The wide range remains only on outputs whose DT lists nothing; for
those the divider can only produce Fvco/N and v9 reports the effective rate
back through frequency_get, which is the value the core then exposes.  We
prefer reporting the achievable truth over rejecting, since a board that
declares its rates never hits the rounding path.

> What happens to the 34-bit divider when one of these five writes fails
> partway through?

Fixed in v9: the five previous bytes are read and saved before the
write, and a mid-sequence failure restores them before the commit, so a mixed
old/new divider is never latched.

> Is a u32 wide enough for the effective rate this line caches?

The reachable overflow is closed in v9: the setter rejects any request
above U32_MAX (see below), so no accepted request can cache a wrapping rate.

> The Return: block lists 0, -ENODEV and register access errors, but this
> -EINVAL [...] is not among them. [and step numbering]

Both fixed in v9 -- the Return: block lists -EINVAL and the step
numbering in the doc block and the body agree.

> The changelog says "An input's frequency is what the board presents".  Is
> ref->freq actually that value when a board lists several rates?

The device cannot measure an input's rate, so the board's declaration
is the only source there is.  When a board lists several rates the first entry
is taken as the nominal one presently wired -- that is a board-authoring
convention we will spell out in the binding description.  Boards for which
this matters can list the live rate first or list only one rate.

> does this read past ref[] for the INTSYNC pin at this point in the series?

Resolved by the v9 registration restructure: the
INTSYNC pins are not registered until their dedicated ops tables exist, so no
intermediate commit exposes ref[9]/out[12] through the frequency callbacks.

> does this also swallow I2C and regmap errors? [freq_get fallback]

v9 falls back to the cached value
only for -ENODEV (the genuinely unresolvable routing case the comment
describes) and propagates register access errors, so a transient bus failure
is not reported as a live frequency and cannot spoof the set path's
old == new short-circuit.

> Should this setter sanity-check the upper bound of frequency itself?

Yes -- fixed in v9: the setter rejects any request above U32_MAX,
closing the "4294967297 Hz validates as 1 Hz" hole, and independently rejects
a DIVO that does not fit its 34-bit field.