Re: [PATCH net-next v8 06/15] dpll: sit9531x: implement input pin state on a DPLL

[Ali Rouhi <arouhi@sitime.com>]

Replies inline.

> The comment says "The five pin ops tables below", while at this commit only
> three exist [...] Could the commit message avoid crediting this patch with a
> comment block it does not add?

The contract comment deliberately describes the complete
design so each later table can point at one authoritative statement instead of
growing its own; the count is correct at the end of the series.  We will
reword the v9 changelog so it does not read as if this patch adds the block.

> Can this index sitdev->ref[] out of bounds when called for the INTSYNC
> destination pin? [...] Would an explicit index check in both helpers, and a
> separate ops table for the INTSYNC destination, be preferable?

Yes.  With the v9 registration restructure the
INTSYNC destination pin is only registered together with its dedicated ops
table, so it can no longer reach the generic input helpers at any commit; and
the helpers gain an explicit index bound check plus a
lockdep_assert_held(&sitdev->multiop_lock) annotation as suggested.

> The force override is asserted before its state bit is cleared, and an error
> in between returns with no attempt to undo it.

v9 finishes the force/state sequence and keeps
the first error, the same shape the output helpers already use.

> This isn't a bug, but the reference is circular. [prio_set holdover/latch
> comments]

Fixed in v9: the three comments were reworded so the holdover/latch
sequence is documented where it lives (the table-commit helper) and the
callers no longer cite each other circularly.

> Is the unconditional mask refresh correct on the error paths?

Reworked in v9: the latch is still issued unconditionally --
slots written before a failed write are in the hardware table regardless, so
latching keeps the device and the mask rebuild consistent with what was
actually written -- and the mask is rebuilt from the array that was being
written, which is closer to the truth than the pre-write mask either way.  The
periodic poll reads the table back and corrects whatever a partial write left
behind, so the cache converges to hardware within one poll period.

> Does this silently evict whatever already occupied the last slot?

No longer -- v9 returns -ENOSPC when the table is genuinely full (see
also the reply on patch 05).

> Should the N-lane code be folded onto the P lane, the way the probe path
> already does?

v9 folds the N code onto the P lane
in the priority paths for differentially configured pairs, matching what
sit9531x_ref_pll_mask_fetch() already does, so a blob that lists a DE pair
under its N code is found by the getters and setters.

> When sit9531x_input_prio_add() fails here, the receiver has already been
> enabled and ref->pll_mask is left at zero. [...] Would calling
> sit9531x_input_disable() on this error path keep the enable and the claim in
> step?

Yes -- v9 disables the receiver again when the
table add fails, so an enable is never left without a matching claim.