[PATCH V3 6/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607
Chris Morgan
macromorgan at hotmail.com
Tue Apr 28 07:01:12 PDT 2026
On Fri, Apr 10, 2026 at 05:59:05PM -0500, David Lechner wrote:
> On 3/30/26 2:58 PM, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan at hotmail.com>
> >
> > Add icm42607 accelerometer sensor for icm42607.
> >
>
> ...
>
> > +static const unsigned long inv_icm42607_accel_scan_masks[] = {
> > + /* 3-axis accel + temperature */
> > + INV_ICM42607_SCAN_MASK_ACCEL_3AXIS | INV_ICM42607_SCAN_MASK_TEMP,
>
> This is going to make it so that the temperature channel is always read
> even if it isn't enabled and additional work is needed when pushing to
> buffers to remove it again.
>
> It looks like it is possible to read accel and temp separatly, so
> there shuold be two more lines here,
>
> INV_ICM42607_SCAN_MASK_ACCEL_3AXIS,
> INV_ICM42607_SCAN_MASK_TEMP,
>
> I forget what the correct order is though.
>
> > + 0,
> > +};
> > +
> > +/* enable accelerometer sensor and FIFO write */
> > +static int inv_icm42607_accel_update_scan_mode(struct iio_dev *indio_dev,
> > + const unsigned long *scan_mask)
> > +{
> > + struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> > + struct inv_icm42607_sensor_state *accel_st = iio_priv(indio_dev);
> > + struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> > + unsigned int fifo_en = 0;
> > + unsigned int sleep_temp = 0;
> > + unsigned int sleep_accel = 0;
> > + unsigned int sleep;
> > + int ret;
> > +
> > + mutex_lock(&st->lock);
> > +
> > + if (*scan_mask & INV_ICM42607_SCAN_MASK_TEMP) {
> > + /* enable temp sensor */
> > + ret = inv_icm42607_set_temp_conf(st, true, &sleep_temp);
> > + if (ret)
> > + goto out_unlock;
> > + fifo_en |= INV_ICM42607_SENSOR_TEMP;
> > + }
> > +
> > + if (*scan_mask & INV_ICM42607_SCAN_MASK_ACCEL_3AXIS) {
> > + /* enable accel sensor */
> > + conf.mode = accel_st->power_mode;
> > + conf.filter = accel_st->filter;
> > + ret = inv_icm42607_set_accel_conf(st, &conf, &sleep_accel);
> > + if (ret)
> > + goto out_unlock;
> > + fifo_en |= INV_ICM42607_SENSOR_ACCEL;
> > + }
> > +
> > + /* update data FIFO write */
> > + ret = inv_icm42607_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
> > +
> > +out_unlock:
> > + mutex_unlock(&st->lock);
> > + /* sleep maximum required time */
>
> Would be better if the comment explain _why_ we need to sleep.
>
> The code is pretty obvious that it does what the comment says, so
> it doesn't add much.
>
> > + sleep = max(sleep_accel, sleep_temp);
> > + if (sleep)
>
> Probably don't need the if here as msleep() should handle 0 without actually
> sleeping.
>
> > + msleep(sleep);
> > + return ret;
> > +}
> > +
> > +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;
> > + 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 = (__be16 *)&st->buffer[0];
> > + ret = regmap_bulk_read(st->map, reg, data, sizeof(*data));
> > + if (ret)
> > + return ret;
> > +
> > + *val = (int16_t)be16_to_cpup(data);
>
> We don't use int16_t in the kernel (ideally). Stick with s16.
>
> Although cast isn't needed here since val is already s16.
>
> > + if (*val == INV_ICM42607_DATA_INVALID)
> > + ret = -EINVAL;
> > +
> > + return ret;
> > +}
> > +
> > +/* IIO format int + nano */
>
> Usually we make these 2-D arrays for readability and then cast to int * if needed.
>
> > +static const int inv_icm42607_accel_scale[] = {
> > + /* +/- 16G => 0.004788403 m/s-2 */
> > + [2 * INV_ICM42607_ACCEL_FS_16G] = 0,
> > + [2 * INV_ICM42607_ACCEL_FS_16G + 1] = 4788403,
> > + /* +/- 8G => 0.002394202 m/s-2 */
> > + [2 * INV_ICM42607_ACCEL_FS_8G] = 0,
> > + [2 * INV_ICM42607_ACCEL_FS_8G + 1] = 2394202,
> > + /* +/- 4G => 0.001197101 m/s-2 */
> > + [2 * INV_ICM42607_ACCEL_FS_4G] = 0,
> > + [2 * INV_ICM42607_ACCEL_FS_4G + 1] = 1197101,
> > + /* +/- 2G => 0.000598550 m/s-2 */
> > + [2 * INV_ICM42607_ACCEL_FS_2G] = 0,
> > + [2 * INV_ICM42607_ACCEL_FS_2G + 1] = 598550,
> > +};
> > +
I've gone through and implemented all of the changes everyone suggested, though
this is one of the few on which I had a question. Obviously this driver was
cobbled together from 2 different sources and checked to the best of my ability
and tested/validated against the data sheet, but there are a few bits I'm not
fully clear on such as this.
What's the correct way to represent this data? Since it looks like one of the
values is always 0, should I just assume it's always 0 and only represent the
values that change in this scale?
>
> ...
>
> > +static int inv_icm42607_accel_read_calibbias(struct inv_icm42607_state *st,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2)
> > +{
> > + /* Not actually supported in the ICM-42607P registers */
> > + return -EOPNOTSUPP;
> > +}
>
> Can we just not create the attribute instead of returning an error?
>
>
> > +static int inv_icm42607_accel_write_raw_get_fmt(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + long mask)
> > +{
> > + if (chan->type != IIO_ACCEL)
> > + return -EINVAL;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + return IIO_VAL_INT_PLUS_NANO;
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_CALIBBIAS:
> > + return IIO_VAL_INT_PLUS_MICRO;
>
> Can write this as:
>
> case IIO_CHAN_INFO_SAMP_FREQ:
> case IIO_CHAN_INFO_CALIBBIAS:
> return IIO_VAL_INT_PLUS_MICRO;
>
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
>
> ...
>
> > +int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
> > + struct inv_icm42607_sensor_conf *conf,
> > + unsigned int *sleep_ms)
> > +{
> > + struct inv_icm42607_sensor_conf *oldconf = &st->conf.accel;
> > + unsigned int val;
> > + int ret;
> > +
> > + if (conf->mode < 0)
> > + conf->mode = oldconf->mode;
> > + if (conf->fs < 0)
> > + conf->fs = oldconf->fs;
> > + if (conf->odr < 0)
> > + conf->odr = oldconf->odr;
> > + if (conf->filter < 0)
> > + conf->filter = oldconf->filter;
> > +
> > + if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
>
> We could use the regmap cache feature to avoid having to manual keep
> track of old values. Or just always write the same values anyway. I
> find that is nice when debugging hardware with a logic analyzer. Unless
> there is some measureable performance improvlment here?
This is another one I had a question on. I'm not entirely clear from the
datasheet which reg values are volatile and which ones are safe to cache.
Performance wise the 42607 series appears to be the *least* performant
in their lineup, so I don't imagine we care much either way. Should I just
not worry about the old values and always write? Do you think that would
work?
>
> > + val = INV_ICM42607_ACCEL_CONFIG0_FS_SEL(conf->fs) |
> > + INV_ICM42607_ACCEL_CONFIG0_ODR(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 (conf->filter != oldconf->filter) {
> > + if (conf->mode == INV_ICM42607_SENSOR_MODE_LOW_POWER) {
> > + val = INV_ICM42607_ACCEL_CONFIG1_AVG(conf->filter);
> > + ret = regmap_update_bits(st->map, INV_ICM42607_REG_ACCEL_CONFIG1,
> > + INV_ICM42607_ACCEL_CONFIG1_AVG_MASK, val);
> > + } else {
> > + val = INV_ICM42607_ACCEL_CONFIG1_FILTER(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;
> > + }
> > +
> > + return inv_icm42607_set_pwr_mgmt0(st, st->conf.gyro.mode, conf->mode,
> > + st->conf.temp_en, sleep_ms);
> > +}
> > +
Thank you otherwise for all your valuable feedback. I've implemented
it to the best of my ability and plan on resubmitting this series
soon. Thank you.
Chris
More information about the Linux-rockchip
mailing list