Re: [PATCH v3 3/6] iio: imu: Add driver for the ADI ADIS16607

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 15 Oct 2026 12:33:00 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> 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.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
Various comments inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> new file mode 100644
> index 000000000000..1d471f59f043
> --- /dev/null
> +++ b/drivers/iio/imu/adis16607.c

> +struct adis16607 {
> +	const struct adis16607_chip_info *info;
> +	struct adis adis;
> +	unsigned long clk_freq;	/* millihertz */
> +	u64 sample_period_ns;
> +	/* IIO buffer for reordered data - sized for max channels */
> +	u8 data[64] __aligned(IIO_DMA_MINALIGN);

I guess this gets used in a later patch?  Add it there instead.

> +};

> +
> +/*
> + * SPI Half-Duplex write operation for ADIS16607.
> + * Protocol:
> + *   - Send 3 bytes: [reg, val_high, val_low]
> + *   - Only 16-bit writes are supported
> + */
> +static int adis16607_spi_write(struct adis *adis, unsigned int reg,
> +			       unsigned int val, unsigned int size)
> +{
> +	if (size != 2)
> +		return -EINVAL;
> +
> +	adis->tx[0] = reg;
> +	adis->tx[1] = (val >> 8) & 0xFF;
> +	adis->tx[2] = val & 0xFF;

put_unaligned_be16()

as that will clearly document what is going on here.

> +
> +	return spi_write(adis->spi, adis->tx, 3);
> +}

> +/*
> + * Perform sensor self-test for ADIS16607.
> + * Reads self-test data before and after forcing self-test,
> + * then verifies delta values are within acceptable ranges.
> + */
> +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;
> +
> +	/* 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 < 6; i++) {
> +		ret = __adis_read_reg_16(adis, ADIS16607_REG_SELF_TEST_DATA(i),
> +					 &st1_val[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Force self-test */
> +	ret = __adis_write_reg_16(adis, ADIS16607_REG_SELF_TEST,
> +				  ADIS16607_SNSR_SELF_TEST_MASK | ADIS16607_ST_FORCE_MASK);
> +	if (ret)
> +		return ret;
> +
> +	/* 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 < 6; i++) {
> +		ret = __adis_read_reg_16(adis, ADIS16607_REG_SELF_TEST_DATA(i),
> +					 &st2_val[i]);
> +		if (ret)
> +			return ret;
> +
> +		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);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Clear self-test register */
> +	ret = __adis_write_reg_16(adis, ADIS16607_REG_SELF_TEST, 0);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
	return __adis_write_reg_16()

> +}


> +static int adis16607_get_freq(struct adis16607 *st, u32 *freq)
> +{
> +	u16 dec = 0;
> +	int ret;
> +
> +	mutex_lock(&st->adis.state_lock);
> +
> +	ret = __adis_read_reg_16(&st->adis, ADIS16607_REG_DEC_RATE, &dec);
> +	if (ret)
> +		goto unlock;
> +
> +	*freq = DIV_ROUND_CLOSEST(st->clk_freq, dec + 1);
> +
> +unlock:
> +	mutex_unlock(&st->adis.state_lock);

As below. We have guards for this and a special helper
to make them even easier to use for this lock.

> +	return ret;
> +}
> +
> +static int adis16607_set_freq(struct adis16607 *st, u32 freq)
> +{
> +	u32 dec;
> +	int ret;
> +
> +	if (!freq)
> +		return -EINVAL;
> +
> +	mutex_lock(&st->adis.state_lock);

Use guard.  However, don't we have more specific helpers for these?

adis_dev_autolock()

> +
> +	dec = DIV_ROUND_CLOSEST(st->clk_freq, freq);
> +	if (dec)
> +		dec--;
> +
> +	dec = min_t(u32, dec, st->info->max_dec);
> +
> +	ret = __adis_write_reg_16(&st->adis, ADIS16607_REG_DEC_RATE, dec);
> +	if (!ret)
With a guard you can have the error path out of line.
Always do that unless it makes the code a lot less redable.
	if (ret)
		return ret;

	st->sample_period_ns = ...

> +		st->sample_period_ns = DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * 1000ULL * (dec + 1),
> +							     st->clk_freq);
> +
> +	mutex_unlock(&st->adis.state_lock);
> +	return ret;
> +}



> +#define ADIS16607_MOD_CHAN(_type, _mod, _address, _si, _bits) { \
> +	.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 = (_bits), \

Always 24?  If so hard code it.

> +		.storagebits = 32, \
> +		.endianness = IIO_BE, \
> +	}, \
> +}
> +
> +#define ADIS16607_ACCEL_CHANNEL(_mod) \
> +	ADIS16607_MOD_CHAN(IIO_ACCEL, IIO_MOD_ ## _mod, \
> +			   ADIS16607_REG_ ## _mod ## _ACCEL, \
> +			   ADIS16607_SCAN_ACCEL_ ## _mod, 24)
> +
> +#define ADIS16607_GYRO_CHANNEL(_mod) \
> +	ADIS16607_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \
> +			   ADIS16607_REG_ ## _mod ## _GYRO, \
> +			   ADIS16607_SCAN_GYRO_ ## _mod, 24)
> +
> +#define ADIS16607_TEMP_CHANNEL() { \
> +	.type = IIO_TEMP, \
> +	.indexed = 1, \
> +	.channel = 0, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_SCALE) | \
> +			      BIT(IIO_CHAN_INFO_OFFSET), \
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.address = ADIS16607_REG_TEMP, \
> +	.scan_index = ADIS16607_SCAN_TEMP, \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +		.endianness = IIO_BE, \
> +	}, \

Only one of these. Just put it inline.  The macro isn't adding anything
useful that I can see.

> +}
> +
> +#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, \
> +	}, \
> +}
> +
> +#define ADIS16607_DELTANG_CHAN(_mod) \
> +	ADIS16607_MOD_CHAN_DELTA(IIO_DELTA_ANGL, IIO_MOD_ ## _mod, \
> +				 ADIS16607_REG_ ## _mod ## _DELTANG, \
> +				 ADIS16607_SCAN_DELTANG_ ## _mod)
> +
> +#define ADIS16607_DELTVEL_CHAN(_mod) \
> +	ADIS16607_MOD_CHAN_DELTA(IIO_DELTA_VELOCITY, IIO_MOD_ ## _mod, \
> +				 ADIS16607_REG_ ## _mod ## _DELTVEL, \
> +				 ADIS16607_SCAN_DELTVEL_ ## _mod)
> +
> +static const struct iio_chan_spec adis16607_channels[] = {
> +	ADIS16607_ACCEL_CHANNEL(X),
> +	ADIS16607_ACCEL_CHANNEL(Y),
> +	ADIS16607_ACCEL_CHANNEL(Z),
> +	ADIS16607_GYRO_CHANNEL(X),
> +	ADIS16607_GYRO_CHANNEL(Y),
> +	ADIS16607_GYRO_CHANNEL(Z),
> +	ADIS16607_DELTVEL_CHAN(X),
> +	ADIS16607_DELTVEL_CHAN(Y),
> +	ADIS16607_DELTVEL_CHAN(Z),
> +	ADIS16607_DELTANG_CHAN(X),
> +	ADIS16607_DELTANG_CHAN(Y),
> +	ADIS16607_DELTANG_CHAN(Z),
> +	ADIS16607_TEMP_CHANNEL(),
> +	IIO_CHAN_SOFT_TIMESTAMP(13),
> +};
> +
> +/*
> + * ADIS16607-2: 450 deg/s gyro, 40g accel
> + * ADIS16607-3: 2000 deg/s gyro, 40g accel
> + */
> +#define ADIS16607_CHIP_INFO_COMMON					\
> +	.channels	= adis16607_channels,				\
> +	.num_channels	= ARRAY_SIZE(adis16607_channels),		\
> +	.accel_max_val	= IIO_G_TO_M_S_2(40),				\
> +	.accel_max_scale = 31250 << 8,					\
> +	.temp_scale	= 5,						\
> +	.deltvel_max_val = IIO_G_TO_M_S_2(40),				\
> +	.int_clk	= 8000,						\
> +	.max_dec	= 65535

For only two instances, I'd just duplicate this in the assignments.

> +
> +static const struct adis16607_chip_info adis16607_2_chip_info = {
> +	ADIS16607_CHIP_INFO_COMMON,
> +	.name		= "adis16607-2",
> +	.gyro_max_val	= IIO_DEGREE_TO_RAD(450),
> +	.gyro_max_scale	= 30000 << 8,
> +	.deltang_max_val = IIO_DEGREE_TO_RAD(450),
> +};
> +
> +static const struct adis16607_chip_info adis16607_3_chip_info = {
> +	ADIS16607_CHIP_INFO_COMMON,
> +	.name		= "adis16607-3",
> +	.gyro_max_val	= IIO_DEGREE_TO_RAD(2000),
> +	.gyro_max_scale	= 31250 << 8,
> +	.deltang_max_val = IIO_DEGREE_TO_RAD(2000),
> +};