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

[Ali Rouhi]

On Mon, 5 Oct 2026, netdev-bot+sashiko@xxxxxxxxxx wrote:

Replies inline, in the order of the summary list.

> [Severity: Medium]
> If the table write fails, does the driver keep the rejected priority
> anyway? sit9531x_input_prio_set() commits the new value before calling
> the step that can fail:
>
> [...]
>
>   - prio_get reports a priority that the device rejected.
>   - The next poll sees cfg_prio != pin->prio and sends
>     dpll_pin_change_ntf() for a priority the device never took.

Fixed, and answered on 07/13 as well: the configured priority and its
known flag are saved before the apply and put back when it fails.

> [Severity: Medium]
> Can the first poll pass silently absorb a real operstate or priority
> change?
>
> [...]
>
> This patch adds operstate and prio to the comparison, so those attributes
> now share the gap. The cache is already valid at registration, so could
> the baseline be taken there instead?

Fixed, and taken there: a pin's baseline -- state, operational state and
priority -- is taken when the pin is registered, under the device lock,
rather than by the first tick, so a change between the probe-time fetch
and that tick is reported instead of absorbed. The per-pin "seen" flag is
gone with it; every listed pin has a baseline before the poll can reach
it.

> [Severity: Medium]
> With prio_set now available to userspace, can a reorder steer the PLL
> onto an input whose monitor has disqualified it? sit9531x_prio_src_live()
> only looks at LOS:
>
> [...]
>
> That seems to contradict the commit message, which says "it takes the
> first slot of its priority table whose signal is qualified".

The code is self-consistent: sit9531x_prio_src_live() means "has signal",
and the selection, the ACTIVE operational state and the commit all use
that one meaning.

The commit message said "qualified", which is the monitor's stronger
condition, and that word was wrong; it now says "has signal". The driver
does not veto an input on drift.

> [Severity: Low]
> Is the claim that "only the named pin changes" accurate when the reorder
> moves the active reference?
>
> [...]
>
> The sibling pins, and the corrected operstate, are only notified by the
> next poll, up to SIT9531X_STATUS_POLL_MS later.

The claim is about priorities and is accurate for them: setting one
input's priority moves no other input's priority, so the core's own
notification for the named pin covers the attribute that changed.

The operational state is a different attribute. When the reorder puts
another live source first, the active reference moves, and that shows in
the siblings' operstate on the next poll, up to one period later. The
period is 500 ms and a re-selection itself takes the PLL several seconds
to relock, so the report trails the device by less than the device trails
the request.

The kernel-doc and the commit message now say "priority" where they said
"pin", and the message says the siblings follow on the next poll.