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

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

On Thu, Oct 15, 2026 at 12:33:00PM +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.

...

> +#include <linux/kernel.h>

No way. This header shouldn't appear in a simple driver like this.

> +#include <linux/units.h>
> +#include <linux/module.h>

Keep the list ordered.

> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>

...

> +struct adis16607 {
> +	const struct adis16607_chip_info *info;
> +	struct adis adis;

If you ever use container_of() on this, the moving it to be the first member
saves some .text code.

> +	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);
> +};

Note, whenever you optimize layout with `pahole` always check with
`bloat-o-meter` to see if the code generation is affected and how.

...

> +	case 2:
> +		adis->tx[0] = reg | BIT(7);
> +		adis->tx[1] = 0;
> +		adis->tx[2] = 0;
> +		adis->tx[3] = 0;
> +
> +		spi_message_init(&msg);
> +		spi_message_add_tail(&xfers[0], &msg);
> +		spi_message_add_tail(&xfers[1], &msg);

There is a function to add multiple transfers to the message.

> +		ret = spi_sync(adis->spi, &msg);
> +		if (ret)
> +			return ret;
> +
> +		*val = get_unaligned_be16(&adis->rx[2]);
> +		break;
> +
> +	case 4:
> +		/* Read MSW from reg */
> +		adis->tx[0] = reg | BIT(7);
> +		adis->tx[1] = 0;
> +		adis->tx[2] = 0;
> +		adis->tx[3] = 0;
> +
> +		spi_message_init(&msg);
> +		spi_message_add_tail(&xfers[0], &msg);
> +		spi_message_add_tail(&xfers[1], &msg);

Ditto.

> +		ret = spi_sync(adis->spi, &msg);
> +		if (ret)
> +			return ret;
> +
> +		val_msw = get_unaligned_be16(&adis->rx[2]);
> +
> +		/* Read LSW from reg+1 */
> +		adis->tx[0] = (reg + 1) | BIT(7);
> +
> +		spi_message_init(&msg);
> +		spi_message_add_tail(&xfers[0], &msg);
> +		spi_message_add_tail(&xfers[1], &msg);
> +
> +		ret = spi_sync(adis->spi, &msg);
> +		if (ret)
> +			return ret;
> +
> +		val_lsw = adis->rx[2];
> +
> +		/* Combine: 24-bit value with MSW in upper 16 bits, LSW in lower 8 bits */
> +		*val = ((val_msw << 8) & 0xFFFF00) | val_lsw;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;

Replace breaks with return 0 and drop this one.

...

> +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()

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

...

> +	/* Read first set of self-test data */
> +	for (i = 0; i < 6; i++) {

ARRAY_SIZE()

> +		ret = __adis_read_reg_16(adis, ADIS16607_REG_SELF_TEST_DATA(i),
> +					 &st1_val[i]);
> +		if (ret)
> +			return ret;
> +	}

...

> +	/* Read second set of self-test data */
> +	for (i = 0; i < 6; i++) {

Ditto.

> +		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 ..._write_reg_16(...);

...

> +	/* Additional 100ms required at initial startup per datasheet */

Add a reference where in DS it's required.

> +	msleep(100);

...

> +	/* Route a reset-status indicator to its own pin, if reset-gpios is wired */
> +	if (device_property_present(dev, "reset-gpios")) {

Huh?! Can we use reset framework instead and do something with help of it?

> +		ret = __adis_update_bits_base(adis, ADIS16607_REG_USER_GPIO_CFG,
> +					      ADIS16607_RESET_GPIO_MASK,
> +					      FIELD_PREP(ADIS16607_RESET_GPIO_MASK, 1), 2);
> +		if (ret)
> +			return ret;
> +	}

...

> +#ifdef CONFIG_DEBUG_FS
> +static int adis16607_show_firmware_id(void *arg, u64 *val)
> +{
> +	struct adis16607 *st = arg;
> +	u16 rev;
> +	int ret;
> +
> +	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_REV_ID, &rev);
> +	if (ret)
> +		return ret;
> +
> +	*val = (rev >> 8) & 0xff;
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16607_firmware_id_fops,
> +			 adis16607_show_firmware_id, NULL, "%llu\n");
> +
> +static int adis16607_show_revision_id(void *arg, u64 *val)
> +{
> +	struct adis16607 *st = arg;
> +	u16 rev;
> +	int ret;
> +
> +	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_REV_ID, &rev);
> +	if (ret)
> +		return ret;
> +
> +	*val = rev & 0xff;
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16607_revision_id_fops,
> +			 adis16607_show_revision_id, NULL, "%llu\n");

"%u.%u" (major.minor)

...

> +#else
> +static void adis16607_debugfs_init(struct iio_dev *indio_dev)
> +{
> +}
> +#endif

Why? The DebugFS is aware of this, no ugly ifdeffery needed.

...

> +static int adis16607_get_freq(struct adis16607 *st, u32 *freq)
> +{
> +	u16 dec = 0;
> +	int ret;
> +
> +	mutex_lock(&st->adis.state_lock);

What's wrong with guard()()?

> +	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);
> +	return ret;
> +}

...

> +static int adis16607_config_clk(struct adis16607 *st)
> +{
> +	st->clk_freq = (unsigned long)st->info->int_clk * 1000;

Why casting? What is the meaning of 1000? Why members are without _Hz or alike
units?

> +	st->sample_period_ns = DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * 1000ULL,
> +						     st->clk_freq);

Better to indent like

	st->sample_period_ns =
		DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * 1000ULL, st->clk_freq);

> +	return 0;
> +}

...

> +static int adis16607_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct adis16607 *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -EINVAL;

-ENODATA

> +	indio_dev->name = st->info->name;
> +	indio_dev->channels = st->info->channels;
> +	indio_dev->num_channels = st->info->num_channels;
> +	indio_dev->info = &adis16607_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	st->adis.ops = &adis16607_ops;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get vdd regulator\n");
> +
> +	ret = adis_init(&st->adis, indio_dev, spi, &adis16607_data);
> +	if (ret)
> +		return ret;
> +
> +	ret = __adis_initial_startup(&st->adis);
> +	if (ret)
> +		return ret;
> +
> +	ret = adis16607_config_clk(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	adis16607_debugfs_init(indio_dev);
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko