[PATCH V7 03/11] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Chris Morgan
macromorgan at hotmail.com
Fri May 15 18:51:01 PDT 2026
On Fri, May 15, 2026 at 07:31:02PM +0100, Jonathan Cameron wrote:
> On Fri, 15 May 2026 08:00:08 -0500
> Chris Morgan <macroalpha82 at gmail.com> wrote:
>
> > From: Chris Morgan <macromorgan at hotmail.com>
> >
> > Add the core component of a new inv_icm42607 driver. This includes
> > a few setup functions and the full register definition in the
> > header file.
> >
> > Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
>
> Sashiko led you into the weeks with irq request return values.
> It was less broken in v6 :(
>
> As to it's other comments on checking for line high/low values (0x00 / 0xFF)
> for whoami is something we don't normally bother with but you could if you like.
> I'm fairly sure we've had both those values turn up as valid in some devices
> in the past.
>
> Otherwise just trivial stuff I noticed whilst having a fresh read through.
> All stuff I might have tweaked whilst applying or just let through but
> seeing as you are going to be doing a v8, please take a look.
>
> Jonathan
I think I'll ignore that comment about the high-low values, but it does make
valid points about using the wrong call to invalidate the regmap cache and
also the wrong call to set the SPI_MODE_3 stuff.
All in all I'll try to post another version in another day or two with the
recommended fixes (as best I can) and see if it still complains. I'm expecting
a few complaints like the 0x00/0xFF or some stuff about "keeping the temp sensor
enabled" which is a non-issue per the datasheet. Hopefully I'm near the finish
line either way.
>
>
> > ---
> > drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++
> > .../iio/imu/inv_icm42607/inv_icm42607_core.c | 207 +++++++++++
> > 2 files changed, 541 insertions(+)
> > create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> >
> > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > new file mode 100644
> > index 000000000000..1916e0b08bca
> > --- /dev/null
> > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > @@ -0,0 +1,334 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2026 InvenSense, Inc.
> > + */
> > +
> > +#ifndef INV_ICM42607_H_
> > +#define INV_ICM42607_H_
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +enum inv_icm42607_sensor_mode {
> > + INV_ICM42607_SENSOR_MODE_OFF,
> > + INV_ICM42607_SENSOR_MODE_STANDBY,
> > + INV_ICM42607_SENSOR_MODE_LOW_POWER,
> > + INV_ICM42607_SENSOR_MODE_LOW_NOISE,
> > + INV_ICM42607_SENSOR_MODE_NB
> > +};
> > +
> > +/* gyroscope fullscale values */
> > +enum inv_icm42607_gyro_fs {
> > + INV_ICM42607_GYRO_FS_2000DPS,
> > + INV_ICM42607_GYRO_FS_1000DPS,
> > + INV_ICM42607_GYRO_FS_500DPS,
> > + INV_ICM42607_GYRO_FS_250DPS,
> > + INV_ICM42607_GYRO_FS_NB
> > +};
> > +
> > +/* accelerometer fullscale values */
> > +enum inv_icm42607_accel_fs {
> > + INV_ICM42607_ACCEL_FS_16G,
> > + INV_ICM42607_ACCEL_FS_8G,
> > + INV_ICM42607_ACCEL_FS_4G,
> > + INV_ICM42607_ACCEL_FS_2G,
> > + INV_ICM42607_ACCEL_FS_NB
> > +};
> > +
> > +/* ODR values */
> > +enum inv_icm42607_odr {
> > + INV_ICM42607_ODR_1600HZ = 5,
> > + INV_ICM42607_ODR_800HZ,
> > + INV_ICM42607_ODR_400HZ,
> > + INV_ICM42607_ODR_200HZ,
> > + INV_ICM42607_ODR_100HZ,
> > + INV_ICM42607_ODR_50HZ,
> > + INV_ICM42607_ODR_25HZ,
> > + INV_ICM42607_ODR_12_5HZ,
> > + INV_ICM42607_ODR_6_25HZ_LP,
> > + INV_ICM42607_ODR_3_125HZ_LP,
> > + INV_ICM42607_ODR_1_5625HZ_LP,
> > + INV_ICM42607_ODR_NB
> > +};
> > +
> > +enum inv_icm42607_filter_bw {
> > + /* Low-Noise mode sensor data filter (bandwidth) */
> > + INV_ICM42607_FILTER_BYPASS,
> > + INV_ICM42607_FILTER_BW_180HZ,
> > + INV_ICM42607_FILTER_BW_121HZ,
> > + INV_ICM42607_FILTER_BW_73HZ,
> > + INV_ICM42607_FILTER_BW_53HZ,
> > + INV_ICM42607_FILTER_BW_34HZ,
> > + INV_ICM42607_FILTER_BW_25HZ,
> > + INV_ICM42607_FILTER_BW_16HZ
>
> That isn't a terminating entry so it should have a ,
> Rules on this are a bit obscure - but basically _NB type entries
> should never have a , anything else should - even if it's
> a register field enum where we know all values are there.
>
> > +};
>
>
> > +/* Sleep times required by the driver */
> > +#define INV_ICM42607_POWER_UP_TIME_US 100000
> > +#define INV_ICM42607_RESET_TIME_MS 1
> > +#define INV_ICM42607_ACCEL_STARTUP_TIME_MS 20
> > +#define INV_ICM42607_GYRO_STARTUP_TIME_MS 60
> > +#define INV_ICM42607_GYRO_STOP_TIME_MS 150
> > +#define INV_ICM42607_TEMP_STARTUP_TIME_MS 14
> > +#define INV_ICM42607_SUSPEND_DELAY_MS 2000
>
> Can we have spec references for these? We often hit problems later
> with devices that are a little bit slow and no one is sure if it's because
> the sleeps are wrong or device is actually out of spec. Hence
> it is useful to be able to quickly check these.
I took these from the driver I basically copied wholesale and changed
the registers for according to the datasheet, but I didn't take a very
close look at these (which I am now).
The POWER_UP_TIME_US, assuming it's the same as "Supply Ramp Time" from
the datasheet is 100ms which matches. Likewise, the "Start-up time for
register read/write" listed under "Power-On Reset" shows 1ms, which
matches.
The ACCEL_STARTUP_TIME_MS should probably be 10ms, and the
GYRO_STARTUP_TIME_MS should probably be 30ms. I don't see a startup
time listed for the temperature sensor but I do see "Stabilization
Time" which lists 77us. Gyro stop time and suspend delay time I cannot
find a reference for... logically if those values are needed I think
they should likely be the same as the gyro start time and power up
time, respectively right?
The values I am referencing now appear under the electrical
characteristics section (section 3) of the datasheet for both the
icm42607p (my test devices, over i2c) and the icm42607(c).
>
> > +
> > +typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
> > +
> > +extern const struct regmap_config inv_icm42607_regmap_config;
> > +
> > +int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw *hw,
> > + inv_icm42607_bus_setup bus_setup);
> > +
> > +#endif
> > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > new file mode 100644
> > index 000000000000..9784709319b9
> > --- /dev/null
> > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2026 InvenSense, Inc.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/irq.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include "inv_icm42607.h"
> > +
> > +static int inv_icm42607_set_conf(struct inv_icm42607_state *st,
> > + const struct inv_icm42607_conf *conf)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK,
> > + conf->gyro.mode);
> > + val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK,
> > + conf->accel.mode);
>
> As below - I'm fine with these going on one line (first is 80 chars, second just over)
>
> > + /*
> > + * No temperature enable reg in datasheet, but BSP driver
> > + * selected RC oscillator clock in LP mode when temperature
> > + * was disabled.
> > + */
> /*
> * No temperature enable reg in datasheet, but BSP driver selected
> * RC oscillator clock in LP mode when temperature was disabled.
> */
>
> Basically go up to 80 chars. Here I think keeping RC oscillator on one line
> is a good idea though.
>
>
> > + if (!conf->temp_en)
> > + val |= INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL;
> > + ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
> > + if (ret)
> > + return ret;
> > +
> > + val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG0_FS_SEL_MASK,
> > + conf->gyro.fs);
> It's under 80 chars on one line so don't wrap.
>
> > + val |= FIELD_PREP(INV_ICM42607_GYRO_CONFIG0_ODR_MASK,
> > + conf->gyro.odr);
>
> As is this.
>
> > + ret = regmap_write(st->map, INV_ICM42607_REG_GYRO_CONFIG0, val);
> > + if (ret)
> > + return ret;
> > +
> > + val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG0_FS_SEL_MASK, conf->accel.fs);
> > + val |= FIELD_PREP(INV_ICM42607_ACCEL_CONFIG0_ODR_MASK, conf->accel.odr);
> > + ret = regmap_write(st->map, INV_ICM42607_REG_ACCEL_CONFIG0, val);
> > + if (ret)
> > + return ret;
> > +
> > + val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG1_FILTER_MASK,
> > + conf->gyro.filter);
>
> Whilst this one and next are a tiny bit over 80 chars, if you want to I don't
> mind those being a little long as single lines.
>
> > + ret = regmap_write(st->map, INV_ICM42607_REG_GYRO_CONFIG1, val);
> > + if (ret)
> > + return ret;
> > +
> > + val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK,
> > + conf->accel.filter);
> > + ret = regmap_write(st->map, INV_ICM42607_REG_ACCEL_CONFIG1, val);
> > + if (ret)
> > + return ret;
> > +
> > + st->conf = *conf;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * inv_icm42607_setup() - check and setup chip
> > + * @st: driver internal state
> > + * @bus_setup: callback for setting up bus specific registers
> > + *
> > + * Returns 0 on success, a negative error code otherwise.
> > + */
> > +static int inv_icm42607_setup(struct inv_icm42607_state *st,
> > + inv_icm42607_bus_setup bus_setup)
> > +{
> > + const struct device *dev = regmap_get_device(st->map);
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
> > + if (ret)
> > + return ret;
> > +
> > + if (val != st->hw->whoami)
> > + dev_warn(dev, "invalid whoami %#02x expected %#02x (%s)\n",
> > + val, st->hw->whoami, st->hw->name);
> > +
> > + ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
> > + INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
> > + if (ret)
> > + return ret;
> > +
> > + fsleep(1000);
>
> Do we need an explicit sleep here, or is the idea just to save on
> a read that can't succeed in the poll that follows? I'd be tempted
> to drop this if it's not absolutely needed just to avoid explaining it.
> This isn't fast path code. If we need to leave the device alone after
> reset for a bit then a datasheet reference is needed.
>
Data sheet only really says that the time required after a *hard* reset
is 1ms, it honestly doesn't say anything about a soft reset.
I also thought I got rid of the read_poll_timeout, since I'm already
waiting 1ms... if you think this fsleep is needed or not I'll defer
to you.
Honestly this whole path thing got messed up when we wanted to code for
3 wire SPI. For 3 wire I need to make sure I enable it as soon as the reset
completes, so it's probably not even right here. I need to add bus_setup()
before the read not after. So to support 3-wire it needs to go:
bus_setup()
whoami check
reset
wait (maybe)
bus_setup() again
check int_status (if we can after the bus_setup)
do the rest
whereas if we explicitly say no 3-wire then we can drop the first bus setup
and use regmap_read_poll_timeout to get the status.
> > +
> > + ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
> > + val, val & INV_ICM42607_INT_STATUS_RESET_DONE,
> > + INV_ICM42607_RESET_TIME_MS * 100,
> > + INV_ICM42607_RESET_TIME_MS * 1000);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "reset error, reset done bit not set\n");
> > +
> > + ret = bus_setup(st);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_set_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
> > + INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG1,
> > + INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK,
> > + INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL);
> > + if (ret)
> > + return ret;
> > +
> > + return inv_icm42607_set_conf(st, st->hw->conf);
> > +}
>
> > +int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw *hw,
> > + inv_icm42607_bus_setup bus_setup)
> > +{
> > + struct device *dev = regmap_get_device(regmap);
> > + struct fwnode_handle *fwnode = dev_fwnode(dev);
> > + struct inv_icm42607_state *st;
> > + int irq;
> > + int ret;
> > +
> > + irq = fwnode_irq_get_byname(fwnode, "INT1");
> Trivial but if fwnode is only used here (I didn't check later patches) then
> put the dev_fwnode() inline.
>
> I've never really understood why we don't have device_ variant of that - perhaps
> because it's hard to come up with a clear name.
>
> > + if (!(irq > 0))
> > + return dev_err_probe(dev, -EINVAL, "Unable to get INT1 interrupt\n");
>
> I assume this is in response to sashiko saying fwnode_irq_get_byname() can in some
> corner cases (where I suspect things are broken enough the system probably won't boot)
> return 0. If we are actually going to defend against that then you need
> to special case it. New sashiko review correctly reports this breaks probe
> deferal if say the interrupt chip driver hasn't loaded yet.
>
> if (irq < 0) {
> return dev_err_probe(dev, irq, ...);
> if (irq == 0)
> /* Odd corner case... */
> return dev_err_probe(dev, -EINVAL, ...);
>
> However I'm deeply suspicious of whether the code paths that return 0 can happen in
> practice. Given you have a suitable DT, can you try seeing if you can actually make
> it return 0 by messing around with the interrupt mappings?
>
I will mess with it a bit, but I think I'll just go back to the simple
(irq < 0) then return dev_err_probe(dev, irq). Worst case if it's
wrong I'm in good company. :-p
>
> > +
> > + st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > + if (!st)
> > + return -ENOMEM;
>
I sincerely appreciate all your help on this. Here I was thinking this would
be easy and I could focus on a joystick/LED driver next... how wrong I was!
Chris
More information about the Linux-rockchip
mailing list