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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dpll: add basic SiTime SiT9531x support

This commit adds a basic I2C driver for the SiTime SiT9531x clock
generators. It uses a paged regmap range configuration and adds register
access helpers. Probe gets the xtal rate, takes the optional reset GPIO and
identifies the variant from page 0 offset 0x02.

> diff --git a/drivers/dpll/sit9531x/Kconfig b/drivers/dpll/sit9531x/Kconfig
> new file mode 100644
> index 0000000000000..c306d1dd1c58a
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config SIT9531X_DPLL
> +	tristate "SiTime SiT9531x DPLL driver"
> +	depends on I2C && NET
> +	select REGMAP_I2C

[Severity: Low]
At this commit, can SIT9531X_DPLL build anything unless some other driver
selects DPLL?

The only way kbuild enters drivers/dpll/ is through drivers/Makefile:

    obj-$(CONFIG_DPLL)		+= 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.

A later commit in the series, "dpll: sit9531x: register DPLL devices and
pins", adds select DPLL, and the v10 notes say the move is deliberate. So
this only affects build coverage and bisection at this intermediate commit.

[ ... ]

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> new file mode 100644
> index 0000000000000..cf1cda87fa52c
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> +/*
> + * Everything the device holds can change without the driver writing it,
> + * so nothing here is cacheable -- except the page selector, which only
> + * this driver moves.  Caching that one spares a read of it before every
> + * access: the range code selects the page through a read-modify-write,
> + * and with no cache that read goes to the bus each time.
> + */
> +static bool sit9531x_volatile_reg(struct device *dev __maybe_unused,
> +				  unsigned int reg)
> +{
> +	return reg != SIT9531X_PAGE_SEL;
> +}

[ ... ]

> +const struct regmap_config sit9531x_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= SIT9531X_RANGE_OFFSET +
> +			  (SIT9531X_NUM_PAGES * SIT9531X_PAGE_SIZE) - 1,
> +	.ranges		= &sit9531x_regmap_range,
> +	.num_ranges	= 1,
> +	.volatile_reg	= sit9531x_volatile_reg,
> +	.cache_type	= REGCACHE_MAPLE,
> +};

[Severity: Medium]
Can the cached page selector get out of sync with the chip, and then stay
that way?

_regmap_select_page() changes pages with _regmap_update_bits() on
selector_reg. Since 0xFF is non-volatile, the old value comes from the
cache, and the write is skipped when that cached value already matches:

drivers/base/regmap/regmap.c:_regmap_update_bits() {
    ...
		ret = _regmap_read(map, reg, &orig);
    ...
		if (force_write || (tmp != orig) || map->force_write_field) {
			ret = _regmap_write(map, reg, tmp);
    ...
}

On adapters that only support SMBus byte data, devm_regmap_init_i2c() uses
the regmap_smbus_byte bus. On that bus, _regmap_write() updates the cache
before it does the bus write:

drivers/base/regmap/regmap.c:_regmap_write() {
    ...
		ret = regcache_write(map, reg, val);
    ...
	ret = map->reg_write(context, reg, val);
    ...
}

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 the next access to page B skip the selector write? It would then
read or write the same offset on page A without returning an error. That
could be another PLL's page (0x0A-0x0D/0x1A-0x1D) or one of the
output/system pages.

On error, sit9531x_read_u8() and sit9531x_write_u8() only log and return
rc. Nothing in the series calls regcache_drop_region(),
regcache_mark_dirty() or regcache_cache_bypass(). The sit9531x_resume()
added later only re-queues the poll worker.

Would a selector reset on the chip side also go uncorrected, for example
power loss across suspend or an internal reload?

Before v10 the selector was volatile, so a stale value was fixed on the
next access. The missing drop on the SMBus path is in regmap core, but
caching the selector here is what exposes it. The zl3073x driver uses a
similar pattern.

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

> +/*
> + * sit9531x_read_u8 - read an 8-bit register
> + * @reg:	register in SIT9531X_REG(page, offset) form
> + * @val:	output value
> + */
> +int sit9531x_read_u8(struct sit9531x_dev *sitdev, unsigned int reg,
> +		     u8 *val)
> +{
> +	unsigned int tmp;
> +	int rc;
> +
> +	reg = (SIT9531X_REG_PAGE(reg) * SIT9531X_PAGE_SIZE) +
> +	      SIT9531X_REG_OFFSET(reg) + SIT9531X_RANGE_OFFSET;
> +
> +	rc = regmap_read(sitdev->regmap, reg, &tmp);
> +	if (rc)
> +		dev_err(sitdev->dev, "Failed to read reg 0x%04x: %d\n",
> +			reg, rc);

[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?

reg has already been rewritten at this point. Since SIT9531X_RANGE_OFFSET
equals SIT9531X_PAGE_SIZE, the printed value is exactly one page above the
regs.h encoding.

For example, a failed VARIANT_ID read, SIT9531X_REG(0x00, 0x02) = 0x0002,
is logged as "reg 0x0102", which looks like page 1, offset 0x02.

Would it make sense to keep the original value for the message? The same
applies to "Failed to write reg" in sit9531x_write_u8() below.

> +	else
> +		*val = (u8)tmp;
> +
> +	return rc;
> +}
> +
> +/*
> + * sit9531x_write_u8 - write an 8-bit register
> + * @reg:	register in SIT9531X_REG(page, offset) form
> + * @val:	value to write
> + */
> +int sit9531x_write_u8(struct sit9531x_dev *sitdev, unsigned int reg,
> +		      u8 val)
> +{
> +	int rc;
> +
> +	reg = (SIT9531X_REG_PAGE(reg) * SIT9531X_PAGE_SIZE) +
> +	      SIT9531X_REG_OFFSET(reg) + SIT9531X_RANGE_OFFSET;
> +
> +	rc = regmap_write(sitdev->regmap, reg, val);
> +	if (rc)
> +		dev_err(sitdev->dev, "Failed to write reg 0x%04x: %d\n",
> +			reg, rc);

[ ... ]