Re: [PATCH net-next v11 06/13] dpll: sit9531x: register DPLL devices and pins

[Ali Rouhi]

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

Replies inline, in the order of the summary list.

> [Severity: Medium]
> Is DPLL_MODE_AUTOMATIC the right value for a PLL whose profile sets
> manual active select? sit9531x_manual_sel_report() detects PLL_CONFIG1F
> bit 6 at probe, but only warns:
>
> [...]
>
> With no .mode_set, the core already refuses mode changes. For a PLL where
> sit9531x_manual_sel_report() found bit 6 set, could mode_get report
> DPLL_MODE_MANUAL and supported_modes_get list only MANUAL?

A profile can set the manual active-select bit, and the driver reports
it: sit9531x_manual_sel_report() reads CONFIG1F bit 6, MISCINNER bit 5
and the MAN_IN_SEL bit at probe and says so once.

With the inner manual bit clear the PLL follows CLK_ACTIVESEL, which
this driver rewrites on every table commit, so the selection is the
driver's and AUTOMATIC is the right report. A read-only MANUAL would
describe only the case where the inner manual bit is set, which the
loaded configurations do not do. If one does, the message at probe says
which bit, and the mode can be made to follow it then.

> [Severity: Medium]
> Can a PLL that is frozen in holdover be reported as LOCKED here?
>
> [...]
>
> Should the ho_freeze test come before the locked test?

Fixed: holdover freeze is tested before the lock bit, which is also the
order the pin-state contract uses (a pin is active while the PLL is
locked and not frozen). A frozen PLL whose outer loss-of-lock bit is
clear reports holdover, not locked.

> [Severity: Low]
> The pin loop below takes a first-poll baseline through pin->seen, but
> this device-level comparison has no baseline.
>
> [...]
>
> Won't the first tick send a dpll_device_change_ntf() for every PLL that
> is locked or in holdover, even though nothing changed?

Fixed: the device's lock status is seeded at registration, through the
same getter the poll uses, so the first tick announces only what moved
since. The pins' baseline moved the same way, see the reply on 08/13.

> [Severity: Low]
> Slots 2n and 2n+1 share one register, so sit9531x_prio_reg() returns the
> same address on two consecutive iterations. Is it intended that 5 of the
> 6 priority registers are read twice, for every PLL, on every poll?
>
> [...]
>
> Could each priority register be read once, and the global status bytes
> once per tick? That would remove about 30 transfers per tick.

The count is right: the table is read a byte per slot although two slots
share a register, and the three page-0 status bytes are read once per
PLL rather than once per tick.

None of it is a correctness problem. The poll runs at 500 ms and the bus
is 100 kHz, which leaves it idle for most of the period. Reading the six
table registers once and hoisting the three global reads is a mechanical
change; it is planned for a later revision so this one stays about
behavior.