Re: [PATCH V15 6/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 26 Jun 2026 11:12:27 -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>
Hi Chris,

A few things in here.

Thanks,

Jonathan

> 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..8ef9fdae1bc8
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

> +
> +/* IIO format int + micro , values 0-5 reserved. */

As sashiko notes, 5 is set, so probably 0-4 reserved?

> +static const int inv_icm42607_accel_odr[][2] = {
> +	[INV_ICM42607_ODR_1600HZ] = { 1600, 0 },
> +	[INV_ICM42607_ODR_800HZ] = { 800, 0 },
> +	[INV_ICM42607_ODR_400HZ] = { 400, 0 },
> +	[INV_ICM42607_ODR_200HZ] = { 200, 0 },
> +	[INV_ICM42607_ODR_100HZ] = { 100, 0 },
> +	[INV_ICM42607_ODR_50HZ] = { 50, 0 },
> +	[INV_ICM42607_ODR_25HZ] = { 25, 0 },
> +	[INV_ICM42607_ODR_12_5HZ] = { 12, 500000 },
> +	[INV_ICM42607_ODR_6_25HZ_LP] = { 6, 250000 },
> +	[INV_ICM42607_ODR_3_125HZ_LP] = { 3, 125000 },
> +	[INV_ICM42607_ODR_1_5625HZ_LP] = { 1, 562500 },
> +};
> +
> +static int inv_icm42607_accel_read_odr(struct inv_icm42607_state *st,
> +				       int *val, int *val2)
> +{
> +	unsigned int odr;
> +	unsigned int i;
> +
> +	guard(mutex)(&st->lock);
> +
> +	odr = st->conf.accel.odr;
> +
> +	for (i = 5; i < ARRAY_SIZE(inv_icm42607_accel_odr); ++i) {

As below. Also, why the preincrement?  Common practice in kernel
is postincrement unless it matters.

> +		if (i == odr)
> +			break;
> +	}
> +	if (i >= ARRAY_SIZE(inv_icm42607_accel_odr))
> +		return -EINVAL;
> +
> +	*val = inv_icm42607_accel_odr[i][0];
> +	*val2 = inv_icm42607_accel_odr[i][1];
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int inv_icm42607_accel_write_odr(struct iio_dev *indio_dev,
> +					int val, int val2)
> +{
> +	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int idx;
> +	int ret;
> +
> +	for (idx = 5; idx < ARRAY_SIZE(inv_icm42607_accel_odr); ++idx) {

Maybe use the define rather than 5 for the start point?
As it stands that 5 looks a bit magic :)
Similar to above on the preincrement

> +		if (val == inv_icm42607_accel_odr[idx][0] &&
> +		    val2 == inv_icm42607_accel_odr[idx][1])
> +			break;
> +	}
> +	if (idx >= ARRAY_SIZE(inv_icm42607_accel_odr))
> +		return -EINVAL;

> +
> +static int inv_icm42607_accel_read_raw(struct iio_dev *indio_dev,
> +				       struct iio_chan_spec const *chan,
> +				       int *val, int *val2, long mask)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	s16 data;
> +	int ret;
> +
> +	switch (chan->type) {
> +	case IIO_ACCEL:

Same issue as for avail for samp_freq as it is shared_by_all and later
you add the temperature channel. (I admit we cheat in a similar way
for timestamp channels which strictly speaking should have all the
shared_by_all values set).  Still nice to do better here and not
add ordering constraints to channel definitions!


> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = inv_icm42607_read_sensor(indio_dev, chan, &data);
> +		if (ret)
> +			return ret;
> +		*val = data;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		return inv_icm42607_accel_read_scale(indio_dev, val, val2);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return inv_icm42607_accel_read_odr(st, val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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;

Not sure why sashiko only moaned on the gyro, as same issue here. 
After temperature support is added we have that as a possible channel for
read_avail for the sampling frequency but that patch doesn't change this
check.  So it works because of channel ordering and that a different channel
is associated with that sysfs attribute. That is messy so this should
handle temperature as well. Perhaps push this check into the IIO_CHAN_INFO_SCALE
case only?
 
> +
> +	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[5];
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		*length = (ARRAY_SIZE(inv_icm42607_accel_odr) - 5) * 2;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 738970ed5c66..300c583aba81 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

> +int inv_icm42607_set_sensor_conf(struct inv_icm42607_state *st,
> +				 struct inv_icm42607_sensor_conf *conf,
> +				 enum iio_chan_type chan_type)
> +{
> +	struct inv_icm42607_sensor_conf *oldconf;
> +	bool config0, config1;
> +	unsigned int val;
> +	int ret;
> +
> +	switch (chan_type) {
> +	case IIO_ACCEL:
> +		oldconf = &st->conf.accel;
> +		break;
> +	case IIO_ANGL_VEL:
> +		oldconf = &st->conf.gyro;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	inv_icm42607_update_config(conf, oldconf, &config0, &config1);
> +
> +	if (config0) {
> +		if (chan_type == IIO_ANGL_VEL) {
> +			val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG0_FS_SEL_MASK, conf->fs);
> +			val |= FIELD_PREP(INV_ICM42607_GYRO_CONFIG0_ODR_MASK, conf->odr);

Looking again at this code, it would be fairly easy to stash masks and registers in
a pair of structs that we pick between based on channel type, but maybe it's not worth
the effort. Up to you.

> +			ret = regmap_write(st->map, INV_ICM42607_REG_GYRO_CONFIG0, val);
> +		} else {
> +			val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG0_FS_SEL_MASK, conf->fs);
> +			val |= FIELD_PREP(INV_ICM42607_ACCEL_CONFIG0_ODR_MASK, conf->odr);
> +			ret = regmap_write(st->map, INV_ICM42607_REG_ACCEL_CONFIG0, val);
> +		}
> +		if (ret)
> +			return ret;
> +
> +		oldconf->fs = conf->fs;
> +		oldconf->odr = conf->odr;
> +	}
> +
> +	if (config1) {
> +		if (chan_type == IIO_ANGL_VEL) {
> +			val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG1_FILTER_MASK,
> +					 conf->filter);
> +			ret = regmap_update_bits(st->map, INV_ICM42607_REG_GYRO_CONFIG1,
> +						 INV_ICM42607_GYRO_CONFIG1_FILTER_MASK, val);
> +		} else {
> +			val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK,
> +					 conf->filter);
> +			ret = regmap_update_bits(st->map, INV_ICM42607_REG_ACCEL_CONFIG1,
> +						 INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK, val);
> +		}
> +		if (ret)
> +			return ret;
> +
> +		oldconf->filter = conf->filter;
> +	}
> +
> +	if (chan_type == IIO_ANGL_VEL)

I'd be tempted to do this with a switch just to match the style for the similar
per channel type block above.

> +		return inv_icm42607_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode);
> +
> +	return inv_icm42607_set_pwr_mgmt0(st, st->conf.gyro.mode, conf->mode);
> +}

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip