Re: [PATCH v3 2/9] iio: adc: add the ti-ads1262 driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Inadequate delay after device reset will cause initialization to fail or silently leave the ADC in its default state, corrupting all data reads.
- [Medium] Regmap debugfs accesses bypass the driver's custom transfer lock, causing an unconditional lockdep splat.
--

--- Patch [2]: [PATCH v3 2/9] iio: adc: add the ti-ads1262 driver ---
commit 4680c0f9dff8858f99d46d1498d733421fe96922
Author: Kurt Borja <[email protected]>

iio: adc: add the ti-ads1262 driver

Add the ti-ads1262 driver with initial support for the primary ADC (ADC1).
The ADS1263 auxiliary ADC (ADC2) is handled by a separate driver and
interoperability considerations were taken into account.

> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> new file mode 100644
> index 0000000000000..d78e5e3ae13ee
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.c
[ ... ]
> +static int ads1262_dev_reset(struct ads1262 *st)
> +{
> +	int ret;
> +
> +	if (st->reset_gpiod) {
> +		ret = gpiod_set_value_cansleep(st->reset_gpiod, 1);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * The RESET pulse timing requirement is 4 clock cycles, at the
> +		 * minimum clock rate this is 4 microseconds.
> +		 */
> +		fsleep(4);
> +
> +		ret = gpiod_set_value_cansleep(st->reset_gpiod, 0);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * The RESET timing requirement is 8 clock cycles, at the
> +		 * minimum clock rate this is 8 microseconds
> +		 */
> +		fsleep(8);
> +	} else {
> +		ret = ads1262_dev_cmd(st, ADS1262_OPCODE_RESET);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * The RESET timing requirement is 8 clock cycles, at the
> +		 * minimum clock rate this is 8 microseconds
> +		 */
> +		fsleep(8);

[Severity: High]
Is the 8 microsecond delay sufficient for the device to initialize after reset?

The standard ADC initialization requirement is typically much longer, requiring
4096 clock cycles for recovery (which corresponds to ~555 microseconds at
7.3728 MHz or ~4 milliseconds at 1 MHz).

If the delay is severely inadequate, the device might ignore the subsequent
SPI configuration writes in ads1262_dev_configure().

This could leave the STATUS and CRC bytes enabled in the data output format
by default, causing ads1262_channel_read() to read the 1-byte STATUS followed
by the upper 24 bits of data instead of the actual 32-bit conversion data,
corrupting the readings.

> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static const struct regmap_config ads1262_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.writeable_reg = ads1262_writeable_reg,
> +	.readable_reg = ads1262_readable_reg,
> +	.volatile_reg = ads1262_volatile_reg,
> +	.reg_defaults = ads1262_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(ads1262_reg_defaults),
> +	.max_register = ADS1262_ADC2FSC1_REG,
> +	.can_sleep = true,
> +	.cache_type = REGCACHE_MAPLE,
> +};

[Severity: Medium]
Could this configuration cause a lockdep warning when accessing the regmap
debugfs interface?

The driver provides custom regmap callbacks that assert st->xfer_lock is held,
but ads1262_regmap_config does not define custom .lock and .unlock callbacks.
Because of this, the regmap core will use its default internal mutex.

When userspace reads the regmap debugfs file, it will call into
ads1262_regmap_read() without holding st->xfer_lock:

> +static int ads1262_regmap_read(void *context, const void *reg_buf,
> +			       size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct ads1262 *st = context;
> +	u8 tx[2];
> +
> +	lockdep_assert_held(&st->xfer_lock);

This assertion would unconditionally fail.

> +
> +	/*
> +	 * The register read operation uses a two byte command header followed
> +	 * by the register data:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2