Re: [PATCH v10 03/14] dpll: add basic SiTime SiT9531x support

[Ali Rouhi]

On Fri, 25 Sep 2026, Jakub Kicinski wrote:
> [Severity: Low]
> At this commit, can SIT9531X_DPLL build anything unless some other driver
> selects DPLL?
>
> DPLL is a hidden bool that has no prompt. If SIT9531X_DPLL is the only DPLL
> user in a config, the line
>
>     obj-$(CONFIG_SIT9531X_DPLL)	+= sit9531x/
>
> is never evaluated, and no sit9531x object or module gets built.

Fixed. "select DPLL" now sits in this patch's Kconfig rather than in
"dpll: sit9531x: register DPLL devices and pins", so this commit builds
what it adds and the series is bisectable for build coverage as well as
for correctness. Deferring the select was deliberate in v10 and it was
the wrong call.

> [Severity: Medium]
> Can the cached page selector get out of sync with the chip, and then stay
> that way?
>
> Suppose i2c_smbus_write_byte_data() fails for the selector write. The error
> comes back, but the cached value is not dropped. The raw write path in
> _regmap_raw_write_impl() does drop it with map->cache_ops->drop(). The chip
> then stays on page A while the cache says page B.
>
> Would a selector reset on the chip side also go uncorrected, for example
> power loss across suspend or an internal reload?

Both cases are real and both are handled in v11.

sit9531x_page_cache_drop() wraps regcache_drop_region() on the selector
and is called from four places: on failure in sit9531x_read_u8(), on
failure in sit9531x_write_u8(), on failure in sit9531x_update_pll_u8(),
and at the top of sit9531x_resume(). The first three cover the failed
transfer, the last covers a part that lost the selector across suspend.
The next access after any of them re-selects the page instead of
trusting the cache.

sit9531x_update_pll_u8() is worth calling out because it was not
covered by the first version of this fix. It computes the virtual
address itself and calls regmap_update_bits() directly, and a
read-modify-write is a read and a write, so either half can leave the
selector wrong. It now fails the way the single accessors do.

One thing your description gets at that is worth stating for the next
reader of this code: the missing drop is not ours to add here. It is in
regmap core, in _regmap_write(), which updates the cache before the bus
write and does not undo that on error, while _regmap_raw_write_impl()
does call map->cache_ops->drop(). So the asymmetry is between the two
write paths in regmap, and on adapters limited to SMBus byte data we
get the one without the drop. v11 works around it rather than fixing
it, which we think is the right scope for a new driver, but the
workaround should not read as belt-and-braces to whoever touches it
next.

> The comment says the selector is something "only this driver moves". Does
> that still hold once a transfer fails or the chip resets?

It does not, and the comment no longer says it.

> [Severity: Low]
> This isn't a bug, but is this message meant to print the regmap virtual
> address rather than the SIT9531X_REG(page, offset) value the caller passed
> in?
>
> For example, a failed VARIANT_ID read, SIT9531X_REG(0x00, 0x02) = 0x0002,
> is logged as "reg 0x0102", which looks like page 1, offset 0x02.

Fixed, and taken a little further than keeping the original value. The
translated address goes into a separate variable, so reg stays intact,
and the messages now print the two fields separately:

    "Failed to read page 0x%02x reg 0x%02x: %d\n"
    "Failed to write page 0x%02x reg 0x%02x: %d\n"

A failed VARIANT_ID read reports page 0x00 reg 0x02, with nothing left
for the reader to decode.

This patch is patch 4 of v11.

Ali