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

[Nuno Sá]

On Thu, 15 Oct 2026 12:33:00 +0300, Radu Sabau <radu.sabau@xxxxxxxxxx> 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.
> 
> Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>

Hopefully I did not repeated too much feedback.

>
>
> 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
> @@ -0,0 +1,855 @@
> [ ... skip 285 lines ... ]
> +
> +		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;

Only used in probe no? If so dev_err_probe()

> +		}
> +	}
> +
> +	/* Clear self-test register */
> +	ret = __adis_write_reg_16(adis, ADIS16607_REG_SELF_TEST, 0);
> +	if (ret)
> +		return ret;
> +

Just return __adis_write_reg_16()

> +	return 0;
> +}
> +
> +/*
> + * Post-reset bring-up callback for struct adis_data (see adis16607_data
> + * below). __adis_initial_startup() calls this, unlocked, right after it
> + * has handled reset (hardware GPIO or SW reset, adislib's own job) and
> + * before self-test - no mutex is taken here either, matching that
> + * convention. Does the device-specific startup steps adislib has no
> + * concept of: SPI Half-Duplex locking, dev_id verification, the
> + * bootloader-busy check, and (if wired up) a reset-status GPIO output.
> + */
> +static int adis16607_post_reset_setup(struct adis *adis)

The below API seems a bit over commented!

> [ ... skip 78 lines ... ]
> +	if (ret)
> +		return ret;
> +
> +	ret = __adis_check_status(adis);
> +	if (ret)
> +		dev_err(dev, "status check after self-test failed: %d\n", ret);

Internally the API already logs the individual errors so the above is
more noise.

> [ ... skip 125 lines ... ]
> +	int ret;
> +
> +	if (!freq)
> +		return -EINVAL;
> +
> +	mutex_lock(&st->adis.state_lock);

As mentioned, use the existing helpers.

> +
> +	dec = DIV_ROUND_CLOSEST(st->clk_freq, freq);
> +	if (dec)
> +		dec--;
> +
> +	dec = min_t(u32, dec, st->info->max_dec);

min() should be enough no?

> [ ... skip 16 lines ... ]
> +	u32 tmp;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		return adis_single_conversion(indio_dev, chan, 0, val);
> +

No new line.

> [ ... skip 23 lines ... ]
> +		}
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		/* Temperature: 0 raw counts = 25°C; offset = 25000 mdeg / 5 mdeg per LSB = 5000 */
> +		*val = 5000;
> +		return IIO_VAL_INT;

same

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

ditto

-- 
Nuno Sá <nuno.sa@xxxxxxxxxx>