Re: [PATCH v3 4/8] iio: imu: inv_icm45600: add IMU IIO devices

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 17 Jul 2025 13:25:56 +0000
Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@kernel.org> wrote:

> From: Remi Buisson <remi.buisson@tdk.com>
> 
> Add IIO devices for accelerometer and gyroscope sensors
> with data polling interface and FIFO parsing.
> Attributes: raw, scale, sampling_frequency, calibbias.
> Temperature is available as a processed channel.
> 
> Signed-off-by: Remi Buisson <remi.buisson@tdk.com>


> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_accel.c b/drivers/iio/imu/inv_icm45600/inv_icm45600_accel.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..bd6b85e25e1792744769916f6d529615f9acf723
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_accel.c

If possible it would have been better to do accel and gyro in different patches
(or more manageable size).  Note I didn't read this one on the assumption any
issues are likely to the same (and I'm out of time for today).

> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_gyro.c b/drivers/iio/imu/inv_icm45600/inv_icm45600_gyro.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..48373befafa0efe0e5dcb2c97b2c836436ce7d78
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_gyro.c
> +static const struct iio_chan_spec_ext_info inv_icm45600_gyro_ext_infos[] = {
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, inv_icm45600_get_mount_matrix),
> +	{ },

No comma on a terminating entry like this. We don't want to make it easy
to put things after it.

> +};

> +/* IIO format int + nano */
> +const int inv_icm45686_gyro_scale[][2] = {
> +	/* +/- 4000dps => 0.002130529 rad/s */
> +	[INV_ICM45686_GYRO_FS_4000DPS] = {0, 2130529},
> +	/* +/- 2000dps => 0.001065264 rad/s */
> +	[INV_ICM45686_GYRO_FS_2000DPS] = {0, 1065264},
> +	/* +/- 1000dps => 0.000532632 rad/s */
> +	[INV_ICM45686_GYRO_FS_1000DPS] = {0, 532632},
> +	/* +/- 500dps => 0.000266316 rad/s */
> +	[INV_ICM45686_GYRO_FS_500DPS] = {0, 266316},
> +	/* +/- 250dps => 0.000133158 rad/s */
> +	[INV_ICM45686_GYRO_FS_250DPS] = {0, 133158},
> +	/* +/- 125dps => 0.000066579 rad/s */
> +	[INV_ICM45686_GYRO_FS_125DPS] = {0, 66579},
> +	/* +/- 62.5dps => 0.000033290 rad/s */
> +	[INV_ICM45686_GYRO_FS_62_5DPS] = {0, 33290},
> +	/* +/- 31.25dps => 0.000016645 rad/s */
> +	[INV_ICM45686_GYRO_FS_31_25DPS] = {0, 16645},
> +	/* +/- 15.625dps => 0.000008322 rad/s */
> +	[INV_ICM45686_GYRO_FS_15_625DPS] = {0, 8322},

	[INV_ICM45686_GYRO_FS_15_625DPS] = { 0, 8322 },
etc

> +};

> +
> +static int inv_icm45600_gyro_write_offset(struct inv_icm45600_state *st,
> +					  struct iio_chan_spec const *chan,
> +					  int val, int val2)
> +{

> +
> +	/* clamp value limited to 14 bits signed */

Use clamp() then! :)

> +	if (offset < -8192)
> +		offset = -8192;
> +	else if (offset > 8191)
> +		offset = 8191;
> +
> +	st->buffer.u16 = cpu_to_le16(offset & INV_ICM45600_GYRO_OFFUSER_MASK);
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &st->lock)
> +		ret = regmap_bulk_write(st->map, reg, &st->buffer.u16, sizeof(st->buffer.u16));
> +
> +	pm_runtime_put_autosuspend(dev);
> +	return ret;
> +}


> +static int inv_icm45600_gyro_hwfifo_flush(struct iio_dev *indio_dev,
> +					  unsigned int count)
> +{
> +	struct inv_icm45600_state *st = iio_device_get_drvdata(indio_dev);
> +	int ret;
> +
> +	if (count == 0)
> +		return 0;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = inv_icm45600_buffer_hwfifo_flush(st, count);
> +	if (!ret)
> +		ret = st->fifo.nb.gyro;
I'd prefer keeping errors out of line.
	if (ret)
		return ret;

	return st->fifo.mb.gyro;

Such error formatting helps a bit when people are reading a lot of code
because it is very common.

> +
> +	return ret;
> +}

> +int inv_icm45600_gyro_parse_fifo(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm45600_state *st = iio_device_get_drvdata(indio_dev);
> +	struct inv_icm45600_sensor_state *gyro_st = iio_priv(indio_dev);
> +	struct inv_sensors_timestamp *ts = &gyro_st->ts;
> +	ssize_t i, size;
> +	unsigned int no;
> +	const struct inv_icm45600_fifo_sensor_data *accel, *gyro;
> +	const __le16 *timestamp;
> +	const s8 *temp;
> +	unsigned int odr;
> +	s64 ts_val;
> +	struct inv_icm45600_gyro_buffer buffer;

Can we give some sort of order to these local variables. Pretty much anything is
fine. Failing anything else, reverse xmas tree. Maybe also push any
that are local to the loop in there?

> +
> +	/* parse all fifo packets */
> +	for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {
> +		size = inv_icm45600_fifo_decode_packet(&st->fifo.data[i],
> +				&accel, &gyro, &temp, &timestamp, &odr);
> +		/* quit if error or FIFO is empty */
> +		if (size <= 0)
> +			return size;
> +
> +		/* skip packet if no gyro data or data is invalid */
> +		if (gyro == NULL || !inv_icm45600_fifo_is_data_valid(gyro))
> +			continue;
> +
> +		/* update odr */
> +		if (odr & INV_ICM45600_SENSOR_GYRO)
> +			inv_sensors_timestamp_apply_odr(ts, st->fifo.period,
> +							st->fifo.nb.total, no);
> +
> +		/* buffer is copied to userspace, zeroing it to avoid any data leak */
> +		memset(&buffer, 0, sizeof(buffer));
We only care about not leaking data that is sensitive.  Not stale readings or
similar so can just do
	struct inv_icm45600_gyro_buffer buffer = { };
above and skip the memset here.

> +		memcpy(&buffer.gyro, gyro, sizeof(buffer.gyro));
> +		/* convert 8 bits FIFO temperature in high resolution format */
> +		buffer.temp = temp ? (*temp * 64) : 0;
> +		ts_val = inv_sensors_timestamp_pop(ts);
> +		iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val);
> +	}
> +
> +	return 0;
> +}
>