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

[Nuno Sá]

On Fri, 23 Oct 2026 12:30:22 +0300, Radu Sabau <radu.sabau@xxxxxxxxxx> wrote:
> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> new file mode 100644
> index 000000000000..2dda306d7b54
> --- /dev/null
> +++ b/drivers/iio/imu/adis16607.c
> @@ -0,0 +1,842 @@
> [ ... skip 102 lines ... ]
> +	ADIS16607_SCAN_DELTANG_Z,
> +	ADIS16607_SCAN_TEMP,
> +};
> +
> +/*
> + * SPI Half-Duplex read operation for ADIS16607.

I assume you use half duplex because burst mode is not available in full
duplex?

> + * Protocol:
> + *   - First transfer: Send [reg | 0x80, 0, 0, 0] - device latches command
> + *   - Second transfer: Send same command, device responds in bytes 2-3
> + *   - CS toggles between transfers (cs_change = 1 on first xfer)
> + * For 4-byte reads (24-bit sensor data):
> + *   - Read MSW from reg, read LSW from reg+1
> + *   - Combine as: ((val_msw << 8) & 0xFFFF00) | val_lsw

Just repeat the above down in the code. I would only mention it close to
the code,

> + */
> +static int adis16607_spi_read(struct adis *adis, unsigned int reg,
> +			      unsigned int *val, unsigned int size)
> +{
> +	struct spi_message msg;
> +	struct spi_transfer xfers[2] = {
> +		{
> +			.tx_buf = adis->tx,
> +			.len = 4,
> +			.bits_per_word = 8,

Isn't it the default already?

> [ ... skip 13 lines ... ]
> +	case 2:
> +		adis->tx[0] = reg | BIT(7);
> +		adis->tx[1] = 0;
> +		adis->tx[2] = 0;
> +		adis->tx[3] = 0;
> +

You should not need to set those values to 0.

> [ ... skip 179 lines ... ]
> +	}
> +
> +	/* Verify the correct device is attached */
> +	ret = __adis_read_reg_16(adis, ADIS16607_REG_DEV_ID, &dev_id);
> +	if (ret)
> +		return ret;

This is not consistent. Above you print an error. Here you don't. FWIW,
I would not print it above.

> +
> +	if (dev_id != adis->data->prod_id) {
> +		dev_err(dev, "device ID mismatch: expected 0x%04x, got 0x%04x\n",
> +			adis->data->prod_id, dev_id);
> +		return -ENODEV;
> +	}
> +
> +	/* Read diag_stat to clear any latched faults */
> +	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 failed: %d\n", ret);

ditto

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

The above already logs the specific failures! I would not add more log
on top of it. So, `return __adis_check_status()`

> [ ... skip 79 lines ... ]
> +static int adis16607_get_freq(struct adis16607 *st, u32 *freq)
> +{
> +	u16 dec = 0;
> +	int ret;
> +
> +	adis_dev_auto_lock(&st->adis);

I don't see a reason for the explicit locking. At least in this patch,
adis_read_reg_16(() should be enough...

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

You can reduce the scope of the lock

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