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

[Ali Rouhi]

Replies inline.  The esync path was substantially
reworked in v9, which resolves most of the points below at once.

> Is the embedded sync frequency here the marker rate or the carrier rate?

v8 conflated the two.  v9 implements the uAPI
semantics: the supported embedded-sync rate is the 1 Hz marker, the carrier
stays the pin's separately configured frequency, and the changelog now
describes exactly what the code does.

> Where is the pin's previous output rate saved so this path can restore it?

Fixed in v9: the pre-esync rate is saved when esync is enabled and
restored when it is disabled, so an enable/disable cycle returns the pin to
the rate userspace configured.

> What happens if the second or third step here fails?

Fixed in v9: a failure after the first committed step unwinds the
pulse-control write, and the cached esync state is only updated when the
whole sequence succeeded, so the reported state cannot claim "disabled" for a
pin whose carrier was already changed.

> Can dpin->esync_freq go stale here?  [frequency_set / disable do not clear
> it]

Closed in v9 from the other side: a frequency_set on a pin with esync
enabled is rejected with -EBUSY and an extack asking to disable esync first,
so the cached esync state and the carrier can no longer diverge through that
path.

> Should esync->pulse be reported as 0 when esync->freq is 0?

Yes -- fixed in v9, matching ice and zl3073x: both fields read 0 in
the disabled case.

> it looks like one constant is being used both as a uAPI percentage and as a
> raw register value.  Which of the two is it? [and: full-byte write]

For the single supported configuration (1 Hz marker) the
PROG0 pulse-control value that produces the emitted marker corresponds to the
50 percent duty the getter advertises, which is why one literal appears in
both roles; the v9 comment states this explicitly.  PROG0 in this mode is
wholly the pulse control byte, so the full-byte write does not clobber
unrelated fields; this was verified on hardware (SiT95317 evaluation board)
by checking the marker against the blob-programmed configuration.

> could this byte be written inside the window sit9531x_output_freq_set()
> already opens?

It could, and it is a good micro-optimisation, but merging the two
windows couples the esync path into the frequency helper's error handling for
a saving of one commit cycle on a rare configuration operation.  We kept the
operations separate for clarity; happy to revisit if the extra commit cycle
turns out to matter in practice.

> The comment says this byte applies in SYSREF / SYNCB modes.  The new esync
> path does not select either mode, so does the write take effect at all [...]?

The outputs wired for esync on our reference board
are configured by the NVM blob into the mode where the pulse byte applies;
the driver's esync-control DT flag is what marks exactly those outputs, so
the path is only reachable where the write takes effect.  This is
bench-verified on the SiT95317 board.