Re: [PATCH v4 5/8] iio: imu: Add driver for the ADI ADIS16607

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Oct 23, 2026 at 12:30:22PM +0300, Radu Sabau via B4 Relay wrote:

> The ADIS16607 is a Precision MEMS IMU from Analog Devices with
> triaxial gyroscope and accelerometer, delta angle/velocity, and
> temperature channels, communicating over a custom SPI Half-Duplex
> protocol. ADIS16607-2 has a ±450°/s gyroscope range, ADIS16607-3
> ±2000°/s; both share a ±40g accelerometer range and an 8 kHz internal
> clock.
> 
> This is the base driver: probe, raw channel reads, sampling-frequency
> control, startup/self-test via adislib, and debugfs identity entries.
> No buffered capture, external clock sync, or calibration bias yet -
> those follow in later patches.
> 
> Self-test leaves the device's SELF_TEST register cleared on every exit
> path, not just on success, so a read failure or a failed delta check
> doesn't leave the sensor stuck in its self-test/force state until the
> next reset.

...

> +++ b/drivers/iio/imu/Kconfig

> +	tristate "Analog Devices ADIS16607 and similar IMU driver"

(see below)


...

> +struct adis16607_chip_info {
> +	const struct iio_chan_spec *channels;
> +	const char *name;
> +	u32 num_channels;
> +	u32 gyro_max_val;
> +	u32 gyro_max_scale;
> +	u32 accel_max_val;
> +	u32 accel_max_scale;
> +	u32 temp_scale;
> +	u32 deltang_max_val;
> +	u32 deltvel_max_val;
> +	u32 int_clk_hz;

_Hz

> +	u16 max_dec;
> +};

...

> +struct adis16607 {
> +	struct adis adis;
> +	const struct adis16607_chip_info *info;
> +	unsigned long clk_freq_mhz;

_MHz or _mHz? (I believe it's the first one.)
This is why case-sensitive units (as per physics) are important.

> +	u64 sample_period_ns;
> +};

...

> +		/* Combine: 24-bit value with MSW in upper 16 bits, LSW in lower 8 bits */

Make it multi-line and to fix 80 characters.

> +		*val = ((val_msw << 8) & 0xFFFF00) | val_lsw;

Why not GENMASK()? Can val_msw be u16 or other 16-bit type?

...

> +static int adis16607_sensor_self_test(struct adis16607 *st)
> +{
> +	struct adis *adis = &st->adis;
> +	u16 st1_val[6], st2_val[6];
> +	s16 delta_max[6] = {
> +		ADIS16607_ACCEL_XY_DELTA_MAX,
> +		ADIS16607_ACCEL_XY_DELTA_MAX,
> +		ADIS16607_ACCEL_Z_DELTA_MAX,
> +		ADIS16607_GYRO_DELTA_MAX,
> +		ADIS16607_GYRO_DELTA_MAX,
> +		ADIS16607_GYRO_DELTA_MAX,
> +	};
> +	int ret, i;

Why is 'i' signed?

> +	/* Enable sensor self-test */
> +	ret = __adis_write_reg_16(adis, ADIS16607_REG_SELF_TEST,
> +				  ADIS16607_SNSR_SELF_TEST_MASK);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for self-test to settle before reading results */
> +	msleep(adis->data->timeouts->self_test_ms);
> +
> +	/* Read first set of self-test data */
> +	for (i = 0; i < ARRAY_SIZE(st1_val); i++) {
> +		ret = __adis_read_reg_16(adis, ADIS16607_REG_SELF_TEST_DATA(i),
> +					 &st1_val[i]);
> +		if (ret)
> +			goto clear_self_test;
> +	}
> +
> +	/* Force self-test */
> +	ret = __adis_write_reg_16(adis, ADIS16607_REG_SELF_TEST,
> +				  ADIS16607_SNSR_SELF_TEST_MASK | ADIS16607_ST_FORCE_MASK);
> +	if (ret)
> +		goto clear_self_test;
> +
> +	/* Wait for forced self-test to settle before reading results */
> +	msleep(adis->data->timeouts->self_test_ms);
> +
> +	/* Read second set of self-test data */
> +	for (i = 0; i < ARRAY_SIZE(st2_val); i++) {

In both for-loops iterator is local, so

	for (unsigned int i = 0; i < ARRAY_SIZE(st2_val); i++) {

> +		ret = __adis_read_reg_16(adis, ADIS16607_REG_SELF_TEST_DATA(i),
> +					 &st2_val[i]);
> +		if (ret)
> +			goto clear_self_test;
> +
> +		if (!adis16607_delta_check((s16)st1_val[i], (s16)st2_val[i],
> +					   delta_max[i])) {
> +			dev_err(&adis->spi->dev,
> +				"Self-test failed for axis %d: delta out of range\n", i);
> +			ret = -EINVAL;
> +			goto clear_self_test;
> +		}
> +	}
> +
> +	/* Clear self-test register */
> +	return __adis_write_reg_16(adis, ADIS16607_REG_SELF_TEST, 0);
> +
> +clear_self_test:
> +	/*
> +	 * Best-effort cleanup after a failure above - leave the device out
> +	 * of its self-test/force state rather than stuck in it until the
> +	 * next reset. Warn rather than returning this write's own result,
> +	 * so a cleanup failure doesn't mask the real error that got us here.
> +	 */
> +	if (__adis_write_reg_16(adis, ADIS16607_REG_SELF_TEST, 0))
> +		dev_warn(&adis->spi->dev, "failed to clear self-test register\n");
> +
> +	return ret;
> +}

...

> +static int adis16607_self_test(struct adis *adis)
> +{
> +	struct adis16607 *st = container_of(adis, struct adis16607, adis);
> +	struct device *dev = &adis->spi->dev;
> +	u16 diag_stat;
> +	int ret;
> +
> +	/* Inertial Settling Time (Table 1), needed before self-test reads sensor data */

Make it fit 80.

> +	msleep(40);
> +
> +	ret = adis16607_sensor_self_test(st);
> +	if (ret)
> +		return ret;
> +
> +	/* Read diag_stat again to clear any faults latched during self-test */
> +	ret = __adis_read_reg_16(adis, ADIS16607_REG_DIAG_STAT, &diag_stat);
> +	if (ret)
> +		return ret;
> +
> +	ret = __adis_check_status(adis);
> +	if (ret)
> +		dev_err(dev, "status check after self-test failed: %d\n", ret);
> +
> +	return ret;
> +}

...

> +static int adis16607_firmware_revision_show(struct seq_file *s, void *arg)
> +{
> +	struct adis16607 *st = s->private;
> +	u16 rev;

Can this be

	struct revision {
		u8 minor;
		u8 major;
	} rev;

?

> +	int ret;
> +
> +	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_REV_ID, &rev);
> +	if (ret)
> +		return ret;
> +
> +	seq_printf(s, "%u.%u\n", (rev >> 8) & 0xff, rev & 0xff);
> +	return 0;
> +}

...

> +static int adis16607_show_serial_number(void *arg, u64 *val)
> +{
> +	struct adis16607 *st = arg;
> +	u64 serial;
> +	u16 tmp;
> +	int ret;
> +
> +	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_SERIAL_NUM0, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	serial = (u64)tmp;

How is explicit casting helpful here?

> +	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_SERIAL_NUM1, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	serial |= ((u64)tmp << 16);
> +
> +	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_SERIAL_NUM2, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	serial |= ((u64)tmp << 32);

What about reversing the order and shift serial instead?

	u64 serial = 0;

	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_SERIAL_NUM2, &tmp);
	if (ret)
		return ret;

	serial |= tmp;

	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_SERIAL_NUM1, &tmp);
	if (ret)
		return ret;

	serial <<= 16;
	serial |= tmp;

	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_SERIAL_NUM0, &tmp);
	if (ret)
		return ret;

	serial <<= 16;
	serial |= tmp;

> +	*val = serial;
> +	return 0;
> +}

...

> +static int adis16607_set_freq(struct adis16607 *st, u32 freq)
> +{
> +	u32 dec;
> +	int ret;
> +
> +	if (!freq)
> +		return -EINVAL;
> +
> +	adis_dev_auto_lock(&st->adis);
> +
> +	dec = DIV_ROUND_CLOSEST(st->clk_freq_mhz, freq);
> +	if (dec)
> +		dec--;
> +
> +	dec = min_t(u32, dec, st->info->max_dec);

No min_t(). Make the types to be compatible for min().

> +	ret = __adis_write_reg_16(&st->adis, ADIS16607_REG_DEC_RATE, dec);
> +	if (ret)
> +		return ret;
> +
> +	st->sample_period_ns = DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * 1000ULL * (dec + 1),
> +						     st->clk_freq_mhz);

Make it fit. Using temporary variable usually helps. Also you don't need (u64)
casting or ULL suffix. On top of that, what is another 1000 multiplier for?

I would expect a comment on top with explaining magic(s).

> +	return 0;
> +}

...

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = adis16607_get_freq(st, &tmp);
> +		if (ret)
> +			return ret;
> +		*val = tmp / 1000;
> +		*val2 = (tmp % 1000) * 1000;

What is 1000? HZ_PER_KHZ? KHZ_PER_MHZ?

> +		return IIO_VAL_INT_PLUS_MICRO;

...

> +static int adis16607_write_raw(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       int val, int val2, long info)
> +{
> +	struct adis16607 *st = iio_priv(indio_dev);
> +	u32 tmp;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		tmp = val * 1000 + val2 / 1000;
> +		return adis16607_set_freq(st, tmp);

Ditto.

> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +#define ADIS16607_MOD_CHAN(_type, _mod, _address, _si) { \
> +	.type = (_type), \
> +	.modified = 1, \
> +	.channel2 = (_mod), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.address = (_address), \
> +	.scan_index = (_si), \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = 24, \
> +		.storagebits = 32, \
> +		.endianness = IIO_BE, \
> +	}, \
> +}

This is not a function, I would rather shift it right by a tab with a split
(also indent the backslashes, it makes it easier to read).

#define ADIS16607_MOD_CHAN(_type, _mod, _address, _si)		\
	{							\
		.type = (_type),				\
		...
	}

...

> +#define ADIS16607_MOD_CHAN_DELTA(_type, _mod, _address, _si) { \
> +	.type = (_type), \
> +	.modified = 1, \
> +	.channel2 = (_mod), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.address = (_address), \
> +	.scan_index = (_si), \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = 24, \
> +		.storagebits = 32, \
> +		.endianness = IIO_BE, \
> +	}, \
> +}

Ditto.

...

> +/*
> + * No external clock sync support yet: always run on the internal clock.
> + * Default dec=0 after reset, so period = 1 / clk_freq_mhz.
> + */
> +static int adis16607_config_clk(struct adis16607 *st)
> +{
> +	st->clk_freq_mhz = st->info->int_clk_hz * MILLI;

So, it's milli-hertz then? We have MILLIHZ_PER_HZ.

> +	st->sample_period_ns =
> +		DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * MILLI, st->clk_freq_mhz);
> +	return 0;
> +}

Yeah, this magic 1000 should be explained everywhere.

...

> +MODULE_DESCRIPTION("Analog Devices ADIS16607 IMU driver");

Make the text the same as per Kconfig. Whatever wording you choose.

-- 
With Best Regards,
Andy Shevchenko