Re: [PATCH net-next v11 04/13] dpll: add basic SiTime SiT9531x support

[Ali Rouhi]

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

Reply inline.

> [Severity: Medium]
> Does dropping the cache entry on error keep the cached page selector in
> sync with the chip? The comment above sit9531x_volatile_reg() says it
> does, but some paths seem to be missed.

The entry is dropped on every error path the driver controls:
sit9531x_read_u8(), sit9531x_write_u8() and sit9531x_update_pll_u8()
all call sit9531x_page_cache_drop() when regmap returns an error, and
sit9531x_resume() drops it before the first access after a sleep. That
covers the SMBus byte-data case: the failed selector write returns
through one of those three, and the next access re-selects the page.

> If the failed selector write happens inside a debugfs read, nothing
> drops the entry at all. Can the next sit9531x_write_u8() to page N then
> land on page P without any error?

It can, and that window stays. It is not a path the driver takes: the
regmap debugfs registers file is a developer interface that bypasses
every driver on every regmap.

> A later patch in the series drops the entry on resume. Nothing covers a
> reset the driver cannot see, though. After one, could writes meant for
> the PLL pages 0x0A-0x0D go to page 0 instead?

Yes, until the next access that fails or the next resume. Dropping the
entry at the top of each poll tick would bound that window to one poll
period at the cost of one selector write per tick. It is a reasonable
addition and is noted for a later revision rather than folded into this
one.

> Would it be simpler to make SIT9531X_PAGE_SEL volatile as well?

SIT9531X_PAGE_SEL is the one register sit9531x_volatile_reg() reports
as non-volatile, so it is the only cached register in the map. Marking
it volatile makes the _regmap_update_bits() in _regmap_select_page()
read the selector from the bus before every access rather than from the
cache, which adds a transfer to each access instead of removing one.
The driver's own paths are already covered, so that cost would buy only
the two windows above.