[PATCH V15 6/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607
Jonathan Cameron
jic23 at kernel.org
Wed Jul 1 12:24:41 PDT 2026
On Fri, 26 Jun 2026 11:12:27 -0500
Chris Morgan <macroalpha82 at gmail.com> wrote:
> From: Chris Morgan <macromorgan at hotmail.com>
>
> Add icm42607 accelerometer sensor for icm42607.
>
> Signed-off-by: Chris Morgan <macromorgan at 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);
> +}
More information about the Linux-rockchip
mailing list