Re: [PATCH V8 08/10] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 18 May 2026 15:05:23 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add icm42607 accelerometer sensor for icm42607.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
https://sashiko.dev/#/patchset/20260518200526.458421-1-macroalpha82%40gmail.com
More feedback. Some of it looks valid. I haven't looked at all of them.


A few other bits and some comments on one or two of the sashiko ones
that stood out to me.

Thanks,

Jonathan

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> index 2542ad1bee51..c646c22b5e0e 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> @@ -82,12 +82,24 @@ enum inv_icm42607_filter_bw {
>  	INV_ICM42607_FILTER_BW_16HZ,
>  };
>  
> +enum inv_icm42607_filter_avg {
> +	/* Low-Power mode sensor data filter (averaging) */
> +	INV_ICM42607_FILTER_AVG_2X = 0,
> +	INV_ICM42607_FILTER_AVG_4X,
> +	INV_ICM42607_FILTER_AVG_8X,
> +	INV_ICM42607_FILTER_AVG_16X,
> +	INV_ICM42607_FILTER_AVG_32X,
> +	INV_ICM42607_FILTER_AVG_64X
trailing comma should be there.

> +	/* values 7 and 8 also correspond to 64x. */
> +};
> +

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> new file mode 100644
> index 000000000000..623d60704609
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> @@ -0,0 +1,587 @@

> +static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev,
> +					  struct iio_chan_spec const *chan,
> +					  s16 *val)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct inv_icm42607_sensor_state *accel_st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> +	unsigned int reg;
> +	__be16 *data;
> +	int ret;
> +
> +	if (chan->type != IIO_ACCEL)
> +		return -EINVAL;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +		reg = INV_ICM42607_REG_ACCEL_DATA_X1;
I didn't notice this until now, but you don't have anything using
chan->address.  This is what would typically go in there.
If you need multiple registers that would normally be an index into
an array of per channel structures used to look up any register.

Not that important but would move this 'code' into 'data' which would
be nice.

> +		break;
> +	case IIO_MOD_Y:
> +		reg = INV_ICM42607_REG_ACCEL_DATA_Y1;
> +		break;
> +	case IIO_MOD_Z:
> +		reg = INV_ICM42607_REG_ACCEL_DATA_Z1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> +		return -ENXIO;
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* enable accel sensor */
> +	conf.mode = accel_st->power_mode;
> +	conf.filter = accel_st->filter;
> +	ret = inv_icm42607_set_accel_conf(st, &conf, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* read accel register data */
> +	data = &st->buffer[0];
> +	ret = regmap_bulk_read(st->map, reg, data, sizeof(*data));
> +	if (ret)
> +		return ret;
> +
> +	*val = be16_to_cpup(data);
> +	if (*val == INV_ICM42607_DATA_INVALID)
> +		return -EINVAL;
> +
> +	return 0;
> +}

> +}
> +
> +static int inv_icm42607_accel_read_avail(struct iio_dev *indio_dev,
> +					 struct iio_chan_spec const *chan,
> +					 const int **vals,
> +					 int *type, int *length, long mask)
> +{
> +	if (chan->type != IIO_ACCEL)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (const int *)inv_icm42607_accel_scale_nano;
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		*length = ARRAY_SIZE(inv_icm42607_accel_scale_nano) * 2;
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = (const int *)inv_icm42607_accel_odr;
Sashiko is correctly pointing out that this is going to print a bunch
of leading 0s as the array is only useful from 5 onwards.  Start
there rather than at the beginning.

> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		*length = ARRAY_SIZE(inv_icm42607_accel_odr) * 2;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;	
> +	}
> +}
>
> +struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
> +{

> +	/* accel events are wakeup capable */
> +	ret = devm_device_init_wakeup(&indio_dev->dev);
Sashiko points out correctly that should probably be on the
physical bus device rather than this one. Mind you I think for now you
ripped that out anyway so don't set it at all.
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return indio_dev;
> +}