Re: [PATCH v10 06/14] dpll: sit9531x: implement input pin state on a DPLL

[Ali Rouhi <arouhi@sitime.com>]

On Fri, 25 Sep 2026, Jakub Kicinski wrote:
> [Severity: Medium]
> Does the holdover force bit take effect without a PLL-page small-change
> update?
>
> If HO_CTRL follows the same rule, the PLL would never enter forced
> holdover while the table is inconsistent.
>
> Nothing reads HO_FREEZE to confirm that holdover was entered, so the
> 10-12 ms sleep is also an assumption. All of this depends on the HO_CTRL
> latch behaviour in the datasheet, which I couldn't confirm here.

HO_CTRL takes effect when it is written. The small change update
latches the priority slots; the forced-holdover bit is not staged
behind it, which is why the PLL is in holdover for the whole of the
table write rather than only from the latch onwards.

v11 writes the sequence above the code -- force holdover, write the
slots, small change update, release holdover, with a 10 ms settle after
the force -- and sit9531x_prio_prg_commit() now says why a small update
is all the table needs, and why the NVM-bank and loop-lock directives
the output system issues do not belong here.

Your second point stands, and it is better to say so than to imply
otherwise. The release can still fail. It is retried and then logged,
so the failure is visible, but a PLL that loses every retry reports
holdover until the next table write on the same PLL clears the bit,
which may never come.

There is also a path that leaves the bit set on purpose, which is worth
separating from the failure case: a table naming no source keeps the
PLL in the holdover forced above. That is the one state in which it
follows no input, which is exactly what disconnecting every input asks
for, and the selection nibble alone would not achieve it -- it still
names the old source, and the PLL keeps following that one for as long
as it has signal. The next table write that lists a source releases it.

> [Severity: Medium]
> Does reporting hardware selection through DPLL_A_PIN_STATE match the
> documented semantics? Documentation/driver-api/dpll.rst says:
>
>     Pin state (DPLL_A_PIN_STATE) reflects the administrative intent set
>     by the user.
>
> It also says that pin operational state (DPLL_A_PIN_OPERSTATE) reflects
> what the hardware is actually doing with the pin.
>
> The setter refuses CONNECTED. Yet whenever selected_ref == pin_id, this
> getter reports a pin the user set to SELECTABLE as CONNECTED, without
> any user action.
>
> Would it fit the uAPI better to keep state administrative and report the
> selection through operstate?

It would, and that is the structural change in v11.

State now answers what userspace asked for: SELECTABLE when the source
is in this PLL's priority table, DISCONNECTED when it is not. What the
device is doing moved to a new .operstate_on_dpll_get, on the physical
inputs and on the inter-PLL sync destination, reporting ACTIVE,
NO_SIGNAL, QUAL_FAILED or STANDBY. For a selection-role pin CONNECTED
is now -EOPNOTSUPP rather than merely refused, since the device selects
by priority and has no mode that pins one reference.

Two things about the active predicate are worth having in the archive,
because neither is obvious from the register names.

It requires the loop to be locked as well as the pin to be selected.
The selection register holds what the driver last wrote or what the
device last chose, which is not proof the loop is using it; a
free-running, frozen or unlocked PLL follows nothing.

It also requires the named lane to have signal, and that carries a
limitation. When the device fails over on its own to another source in
the table, the registers this driver reads do not name the source it
moved to -- the selection register still names the lane that died. We
report no pin as active rather than report the dead one, so on an
autonomous failover userspace gets notification that something changed
but not identity. That is a limitation of the driver and not of the
part: there are status registers that report the reference a PLL is
actually locked to, and reading those is work for a later version.

This patch is patch 7 of v11.

Ali