Re: [PATCH net-next v11 09/13] dpll: sit9531x: add support to get and set frequency on pins

[Ali Rouhi <arouhi@sitime.com>]

On Mon, 5 Oct 2026, netdev-bot+sashiko@kernel.org wrote:

Replies inline, in the order of the summary list.

> [Severity: Medium]
> What does this report for an input that has no firmware node, or a node
> without supported-frequencies-hz?
>
> [...]
>
> For those pins ref->freq stays 0. Every pin-get, dump and notification
> now reports a frequency of 0 Hz with no supported-frequency entries.
> Before this patch the attribute was simply absent.

Fixed: an input the firmware gives no rate for has no frequency attribute
at all, through an ops table without the getter, rather than reporting
0 Hz. The core abandons the whole pin dump on an error from one pin,
which is why it is left out rather than failed.

> [Severity: Medium]
> With the new input frequency_get, userspace now sees this first entry as
> the current input rate. Does the binding give the first entry that
> meaning?
>
> [...]
>
> Inputs have no frequency_set, so userspace cannot correct the reported
> value.

An input has no divider the driver could read, so firmware is the only
source of its rate, and the first entry is that rate. The binding now
says so; see the reply on 05/13.

> [Severity: Medium]
> Elsewhere in this patch, in sit9531x_prg_enter() and in the
> sit9531x_output_divo_write() rollback, a write that reports an error is
> assumed to have possibly reached the part. Do the error paths here need
> the same handling?

Fixed, all three: a failed source select restores the original source, a
sibling is marked parked before it is cleared so a failure cannot leave
it unparked, and a failed arm disarms rather than unparks.

> [Severity: Medium]
> Should this path also wait out the settling time after the loop lock?
>
> [...]
>
> The abort path also never issues SIT9531X_UPDATE_NVM. Is LOOP_LOCK on its
> own a valid way to leave PRG_CMD on this part?

It is not. Fixed: a failed entry into the programming state is now left
the way a commit leaves it -- NVM update, the loop-lock retries, the
settling time and the debug lock -- instead of a bare loop lock, so the
exit sequence is one sequence, and it is the documented one for this
part. prg_abort() is gone.

> [Severity: Medium]
> Can this make frequency_set report success for a rate the output is not
> running at?
>
> [...]
>
> Using the out-of-band estimate also reports the result as exact, which is
> the outcome the comment rejects for the band edge. Would it be safer to
> refuse the set in this case?
>
> Also, dev_warn_once() fires once per call site, not once per device or
> PLL. After the first warning, other devices and PLLs are silent.

Fixed, in two parts. A derived rate below the low band is not a rate at
all -- a feedback divider below one cycle is unprogrammed -- and
sit9531x_get_fvco() reports no data for it, which also closes the 64-bit
divide the TDC and the phase code performed on it. A frequency set
against a rate outside the PLL's band is refused with -EINVAL rather than
programmed, since a divider computed from a rate the output is not at
would be reported as success. The warning is per PLL rather than once per
driver.

> [Severity: Low]
> This isn't a bug, but is this comment accurate? The priority table's
> latch is sit9531x_prio_prg_commit(), which writes once and does not
> retry:
>
> [...]
>
> The only retry in sit9531x_prio_table_commit() is the loop that releases
> the forced holdover, bounded by SIT9531X_HO_CLEAR_TRIES.

Right, it named the wrong retry: the priority commit writes its latch
once, what it retries is releasing the forced holdover. Reworded.