Add Invensense ICM42607
Subsystems (4)
v11 cover Add Invensense ICM42607
From: Chris Morgan <[email protected]>
Add support for the ICM42607 IMU. This sensor shares the same functionality but a different register layout with the existing ICM42600.
This driver should work with the ICM42607 and ICM42607P over both I2C and SPI, however only the ICM42607P over I2C could be tested.
Changes Since V1:
- Instead of creating a new driver, merged with the existing inv_icm42600 driver. This necessitated adding some code to the existing driver to permit using a different register layout for the same functionality.
- Split changes up a bit more to decrease the size of the individual patches. Note that patch 0004 is still pretty hefty; if I need to split further I may need to create some temporary stub functions.
- Used guard() and PM_RUNTIME_ACQUIRE_AUTOSUSPEND() on the new functions per Jonathan's recommendations.
Changes Since V2:
- Went back to using a new driver on advice from Invensense engineer.
- Further split changes up into smaller chunks of functionality. Note still that the largest patch is approximately 900 lines, and that while the driver compiles cleanly at each commit it is not able to drive the hardware until the commit that adds the Interrupt (as it also adds the Makefile).
- Change the error to a warning when the devicetree binding does not match the hardware ID.
- Dropped the ack on the devicetree bindings, as I am creating a new file (for a new driver) instead of modifying the existing one.
Changes Since V3:
- Numerous small fixes (too many to list here). Thank you to everyone who provided feedback.
- Split power management additions into an additional commit to break things up further.
- Consolidated devicetree documentation in existing invensense,icm42600.yaml file.
- Removed most of the FIELD_PREP from header file to c files to make code easier to read.
- Changed scale values to 2D arrays for Gyro and Accelerometer.
- Removed IIO_CHAN_INFO_CALIBBIAS attribute.
Changes Since V4:
- Additional numerous small fixes, thank you again for all the feedback.
- Dropped power control API and instead run device in low noise mode.
- Split devicetree bindings into two distinct changes.
- Reordered adding of enums and structs to main header file so that they are only brought in when needed.
- Stopped using enum for driver data and instead am using pointer to device specific driver data.
Changes Since V5:
- Corrected use of "dev_warn_probe" to just "dev_warn".
- Fixed some return scenarios which would unconditionally return 0 when an error was present.
- Corrected use of max() to min() for bounds checking. max() was incorrect.
- Fixed using "st->conf.accel.odr" in the gyroscope function. It should have been "st->conf.gyro.odr" which it now is.
- Additional small fixes suggested by "sashiko.dev".
- Added a regmap cache. I used the datasheet to try and determine which registers might change without explicit writes.
Changes Since V6:
- Corrected additional errors identified by sashiko.dev, mostly fixing potential deadlocks, missing calls for pm runtime, and potential overflow issues.
Changes Since V7:
- Dropped Wake on Movement patches, since some of the functionality was only available for a device on which I cannot test.
- Dropped support for SPI 3-Wire mode, since it complicated the bus setup (and I lack the hardware to test such features anyway).
- Fixed a few additional bugs identified by sashiko.dev bot.
Changes Since V8:
- Added back IRQ dropped accidentally when dropping wake on movement patches.
- Dropped "Reviewed-By" tag on patch 2 because of substantial changes made to devicetree binding documentation.
- Additional small fixes as suggested.
Changes Since V9:
- Removed interrupts (and buffers) from the driver. I previously was unable to detect deadlocks because it turns out my IRQ was not even wired correctly in my device.
- Updated devicetree binding commits to make interrupts optional for users of the icm42607 driver.
Changes Since V10:
- Explicitly specified enum values in header file.
- Removed additional dead code for buffer handling.
- Cleaned up headers.
- Added additional locks as requested by sashiko.dev bot.
Chris Morgan (9):
dt-bindings: iio: imu: icm42600: Add mount-matrix to icm42600
dt-bindings: iio: imu: icm42600: Add icm42607
iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
iio: imu: inv_icm42607: Add I2C and SPI For icm42607
iio: imu: inv_icm42607: Add PM support for icm42607
iio: imu: inv_icm42607: Add Temp Support in icm42607
iio: imu: inv_icm42607: Add Accelerometer for icm42607
iio: imu: inv_icm42607: Add Gyroscope to icm42607
arm64: dts: rockchip: Add icm42607p IMU for RG-DS
.../bindings/iio/imu/invensense,icm42600.yaml | 20 +- .../dts/rockchip/rk3568-anbernic-rg-ds.dts | 8 +- drivers/iio/imu/Kconfig | 1 + drivers/iio/imu/Makefile | 1 + drivers/iio/imu/inv_icm42607/Kconfig | 30 ++ drivers/iio/imu/inv_icm42607/Makefile | 13 + drivers/iio/imu/inv_icm42607/inv_icm42607.h | 415 ++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 379 +++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 506 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_gyro.c | 369 +++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 93 ++++ .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 105 ++++ .../iio/imu/inv_icm42607/inv_icm42607_temp.c | 82 +++ .../iio/imu/inv_icm42607/inv_icm42607_temp.h | 33 ++ 14 files changed, 2053 insertions(+), 2 deletions(-) create mode 100644 drivers/iio/imu/inv_icm42607/Kconfig create mode 100644 drivers/iio/imu/inv_icm42607/Makefile create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h
Signature · 1 line
2.43.0
PATCH v11 1/9 dt-bindings: iio: imu: icm42600: Add mount-matrix to icm42600
From: Chris Morgan <[email protected]>
Add mount-matrix attribute to schema. This attribute has been supported since the first revision of this driver, but was not documented.
--- .../devicetree/bindings/iio/imu/invensense,icm42600.yaml | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml +2 -0
@@ -53,6 +53,8 @@ properties: drive-open-drain: type: boolean + mount-matrix: true + vdd-supply: description: Regulator that provides power to the sensor
Signature · 1 line
2.43.0
PATCH v11 2/9 dt-bindings: iio: imu: icm42600: Add icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add the ICM42607 and ICM42607P inertial measurement unit.
This device is functionally very similar to the icm42600 series with a very different register layout. The driver does not require an interrupt for these specific chip revisions.
--- .../bindings/iio/imu/invensense,icm42600.yaml | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml +17 -1
@@ -30,6 +30,8 @@ properties: - invensense,icm42600 - invensense,icm42602 - invensense,icm42605 + - invensense,icm42607 + - invensense,icm42607p - invensense,icm42622 - invensense,icm42631 - invensense,icm42686
@@ -67,10 +69,24 @@ properties: required: - compatible - reg - - interrupts allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml# + - if: + properties: + compatible: + contains: + enum: + - invensense,icm42600 + - invensense,icm42602 + - invensense,icm42605 + - invensense,icm42622 + - invensense,icm42631 + - invensense,icm42686 + - invensense,icm42688 + then: + required: + - interrupts unevaluatedProperties: false
Signature · 1 line
2.43.0
PATCH v11 3/9 iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
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.
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 197 +++++++++++ 2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +334 -0
@@ -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/bits.h> +#include <linux/iio/iio.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/types.h> + +enum inv_icm42607_sensor_mode { +INV_ICM42607_SENSOR_MODE_OFF =0 , +INV_ICM42607_SENSOR_MODE_STANDBY =1 , +INV_ICM42607_SENSOR_MODE_LOW_POWER =2 , +INV_ICM42607_SENSOR_MODE_LOW_NOISE =3 , +INV_ICM42607_SENSOR_MODE_NB +}; + +/* gyroscope fullscale values */ +enum inv_icm42607_gyro_fs { +INV_ICM42607_GYRO_FS_2000DPS =0 , +INV_ICM42607_GYRO_FS_1000DPS =1 , +INV_ICM42607_GYRO_FS_500DPS =2 , +INV_ICM42607_GYRO_FS_250DPS =3 , +INV_ICM42607_GYRO_FS_NB +}; + +/* accelerometer fullscale values */ +enum inv_icm42607_accel_fs { +INV_ICM42607_ACCEL_FS_16G =0 , +INV_ICM42607_ACCEL_FS_8G =1 , +INV_ICM42607_ACCEL_FS_4G =2 , +INV_ICM42607_ACCEL_FS_2G =3 , +INV_ICM42607_ACCEL_FS_NB +}; + +/* ODR values - Note Gyro does not support ODR less than 12.5Hz */ +enum inv_icm42607_odr { +INV_ICM42607_ODR_1600HZ =5 , +INV_ICM42607_ODR_800HZ =6 , +INV_ICM42607_ODR_400HZ =7 , +INV_ICM42607_ODR_200HZ =8 , +INV_ICM42607_ODR_100HZ =9 , +INV_ICM42607_ODR_50HZ =10 , +INV_ICM42607_ODR_25HZ =11 , +INV_ICM42607_ODR_12_5HZ =12 , +INV_ICM42607_ODR_6_25HZ_LP =13 , +INV_ICM42607_ODR_3_125HZ_LP =14 , +INV_ICM42607_ODR_1_5625HZ_LP =15 , +INV_ICM42607_ODR_NB +}; + +/* Low-Noise mode sensor data filter (bandwidth) */ +enum inv_icm42607_filter_bw { +INV_ICM42607_FILTER_BYPASS =0 , +INV_ICM42607_FILTER_BW_180HZ =1 , +INV_ICM42607_FILTER_BW_121HZ =2 , +INV_ICM42607_FILTER_BW_73HZ =3 , +INV_ICM42607_FILTER_BW_53HZ =4 , +INV_ICM42607_FILTER_BW_34HZ =5 , +INV_ICM42607_FILTER_BW_25HZ =6 , +INV_ICM42607_FILTER_BW_16HZ =7 , +INV_ICM42607_FILTER_BW_NB +}; + +/* Signed so that negative values can signify an invalid condition. */ +struct inv_icm42607_sensor_conf { +int mode; +int fs; +int odr; +int filter; +}; + +struct inv_icm42607_conf { +struct inv_icm42607_sensor_conf gyro; +struct inv_icm42607_sensor_conf accel; +bool temp_en; +}; + +struct inv_icm42607_hw { +u8 whoami; +const char *name; +const struct inv_icm42607_conf *conf; +}; + +/** +* struct inv_icm42607_state - driver state variables +* @lock: lock for serializing multiple registers access. +* @hw: Hardware specific data. +* @map: regmap pointer. +* @vddio_supply: I/O voltage regulator for the chip. +* @orientation: sensor chip orientation relative to main hardware. +* @conf: chip sensors configurations. +*/ +struct inv_icm42607_state { +struct mutex lock; +const struct inv_icm42607_hw *hw; +struct regmap *map; +struct regulator *vddio_supply; +struct iio_mount_matrix orientation; +struct inv_icm42607_conf conf; +}; + +/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */ + +/* Register Map for User Bank 0 */ +#define INV_ICM42607_REG_MCLK_RDY 0x00 + +#define INV_ICM42607_REG_DEVICE_CONFIG 0x01 +#define INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE BIT(2) +#define INV_ICM42607_DEVICE_CONFIG_SPI_MODE BIT(0) + +#define INV_ICM42607_REG_SIGNAL_PATH_RESET 0x02 +#define INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET BIT(4) +#define INV_ICM42607_SIGNAL_PATH_RESET_FIFO_FLUSH BIT(2) + +#define INV_ICM42607_REG_DRIVE_CONFIG1 0x03 +#define INV_ICM42607_DRIVE_CONFIG1_I3C_DDR_MASK GENMASK(5, 3) +#define INV_ICM42607_DRIVE_CONFIG1_I3C_SDR_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_DRIVE_CONFIG2 0x04 +#define INV_ICM42607_DRIVE_CONFIG2_I2C_MASK GENMASK(5, 3) +#define INV_ICM42607_DRIVE_CONFIG2_ALL_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_DRIVE_CONFIG3 0x05 +#define INV_ICM42607_DRIVE_CONFIG3_SPI_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_INT_CONFIG 0x06 +#define INV_ICM42607_INT_CONFIG_INT2_LATCHED BIT(5) +#define INV_ICM42607_INT_CONFIG_INT2_PUSH_PULL BIT(4) +#define INV_ICM42607_INT_CONFIG_INT2_ACTIVE_HIGH BIT(3) +#define INV_ICM42607_INT_CONFIG_INT2_ACTIVE_LOW 0x00 +#define INV_ICM42607_INT_CONFIG_INT1_LATCHED BIT(2) +#define INV_ICM42607_INT_CONFIG_INT1_PUSH_PULL BIT(1) +#define INV_ICM42607_INT_CONFIG_INT1_ACTIVE_HIGH BIT(0) +#define INV_ICM42607_INT_CONFIG_INT1_ACTIVE_LOW 0x00 + +/* all sensor data are 16 bits (2 registers wide) in big-endian */ +#define INV_ICM42607_REG_TEMP_DATA1 0x09 +#define INV_ICM42607_REG_TEMP_DATA0 0x0A +#define INV_ICM42607_REG_ACCEL_DATA_X1 0x0B +#define INV_ICM42607_REG_ACCEL_DATA_X0 0x0C +#define INV_ICM42607_REG_ACCEL_DATA_Y1 0x0D +#define INV_ICM42607_REG_ACCEL_DATA_Y0 0x0E +#define INV_ICM42607_REG_ACCEL_DATA_Z1 0x0F +#define INV_ICM42607_REG_ACCEL_DATA_Z0 0x10 +#define INV_ICM42607_REG_GYRO_DATA_X1 0x11 +#define INV_ICM42607_REG_GYRO_DATA_X0 0x12 +#define INV_ICM42607_REG_GYRO_DATA_Y1 0x13 +#define INV_ICM42607_REG_GYRO_DATA_Y0 0x14 +#define INV_ICM42607_REG_GYRO_DATA_Z1 0x15 +#define INV_ICM42607_REG_GYRO_DATA_Z0 0x16 +#define INV_ICM42607_DATA_INVALID -32768 + +#define INV_ICM42607_REG_TMST_FSYNCH 0x17 +#define INV_ICM42607_REG_TMST_FSYNCL 0x18 + +/* APEX Data Registers */ +#define INV_ICM42607_REG_APEX_DATA0 0x31 +#define INV_ICM42607_REG_APEX_DATA1 0x32 +#define INV_ICM42607_REG_APEX_DATA2 0x33 +#define INV_ICM42607_REG_APEX_DATA3 0x34 +#define INV_ICM42607_REG_APEX_DATA4 0x1D +#define INV_ICM42607_REG_APEX_DATA5 0x1E + +#define INV_ICM42607_REG_PWR_MGMT0 0x1F +#define INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL BIT(7) +#define INV_ICM42607_PWR_MGMT0_IDLE BIT(4) +#define INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK GENMASK(3, 2) +#define INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK GENMASK(1, 0) + +#define INV_ICM42607_REG_GYRO_CONFIG0 0x20 +#define INV_ICM42607_GYRO_CONFIG0_FS_SEL_MASK GENMASK(6, 5) +#define INV_ICM42607_GYRO_CONFIG0_ODR_MASK GENMASK(3, 0) + +#define INV_ICM42607_REG_ACCEL_CONFIG0 0x21 +#define INV_ICM42607_ACCEL_CONFIG0_FS_SEL_MASK GENMASK(6, 5) +#define INV_ICM42607_ACCEL_CONFIG0_ODR_MASK GENMASK(3, 0) + +#define INV_ICM42607_REG_TEMP_CONFIG0 0x22 +#define INV_ICM42607_TEMP_CONFIG0_FILTER_MASK GENMASK(6, 4) + +#define INV_ICM42607_REG_GYRO_CONFIG1 0x23 +#define INV_ICM42607_GYRO_CONFIG1_FILTER_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_ACCEL_CONFIG1 0x24 +#define INV_ICM42607_ACCEL_CONFIG1_AVG_MASK GENMASK(6, 4) +#define INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_APEX_CONFIG0 0x25 +#define INV_ICM42607_APEX_CONFIG0_DMP_POWER_SAVE_EN BIT(3) +#define INV_ICM42607_APEX_CONFIG0_DMP_INIT_EN BIT(2) +#define INV_ICM42607_APEX_CONFIG0_DMP_MEM_RESET_EN BIT(0) + +#define INV_ICM42607_REG_APEX_CONFIG1 0x26 +#define INV_ICM42607_APEX_CONFIG1_SMD_ENABLE BIT(6) +#define INV_ICM42607_APEX_CONFIG1_FF_ENABLE BIT(5) +#define INV_ICM42607_APEX_CONFIG1_TILT_ENABLE BIT(4) +#define INV_ICM42607_APEX_CONFIG1_PED_ENABLE BIT(3) +#define INV_ICM42607_APEX_CONFIG1_DMP_ODR_MASK GENMASK(1, 0) + +#define INV_ICM42607_REG_WOM_CONFIG 0x27 +#define INV_ICM42607_WOM_CONFIG_INT_DUR_MASK GENMASK(4, 3) +#define INV_ICM42607_WOM_CONFIG_INT_MODE BIT(2) +#define INV_ICM42607_WOM_CONFIG_MODE BIT(1) +#define INV_ICM42607_WOM_CONFIG_EN BIT(0) + +#define INV_ICM42607_REG_FIFO_CONFIG1 0x28 +#define INV_ICM42607_FIFO_CONFIG1_MODE BIT(1) +#define INV_ICM42607_FIFO_CONFIG1_BYPASS BIT(0) + +#define INV_ICM42607_REG_FIFO_CONFIG2 0x29 +#define INV_ICM42607_REG_FIFO_CONFIG3 0x2A +#define INV_ICM42607_FIFO_WATERMARK_VAL (_wm) \ + cpu_to_le16((_wm) & GENMASK(11, 0)) +/* FIFO is 2048 bytes, let 12 samples for reading latency */ +#define INV_ICM42607_FIFO_WATERMARK_MAX (2048 - 12 * 16) +#define INV_ICM42607_FIFO_1SENSOR_PACKET_SIZE 8 +#define INV_ICM42607_FIFO_2SENSORS_PACKET_SIZE 16 + +#define INV_ICM42607_REG_INT_SOURCE0 0x2B +#define INV_ICM42607_INT_SOURCE0_ST_INT1_EN BIT(7) +#define INV_ICM42607_INT_SOURCE0_FSYNC_INT1_EN BIT(6) +#define INV_ICM42607_INT_SOURCE0_PLL_RDY_INT1_EN BIT(5) +#define INV_ICM42607_INT_SOURCE0_RESET_DONE_INT1_EN BIT(4) +#define INV_ICM42607_INT_SOURCE0_DRDY_INT1_EN BIT(3) +#define INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN BIT(2) +#define INV_ICM42607_INT_SOURCE0_FIFO_FULL_INT1_EN BIT(1) +#define INV_ICM42607_INT_SOURCE0_AGC_RDY_INT1_EN BIT(0) + +#define INV_ICM42607_REG_INT_SOURCE1 0x2C +#define INV_ICM42607_INT_SOURCE1_I3C_ERROR_INT1_EN BIT(6) +#define INV_ICM42607_INT_SOURCE1_SMD_INT1_EN BIT(3) +#define INV_ICM42607_INT_SOURCE1_WOM_INT1_EN GENMASK(2, 0) + +#define INV_ICM42607_REG_INT_SOURCE3 0x2D +#define INV_ICM42607_INT_SOURCE3_ST_INT2_EN BIT(7) +#define INV_ICM42607_INT_SOURCE3_FSYNC_INT2_EN BIT(6) +#define INV_ICM42607_INT_SOURCE3_PLL_RDY_INT2_EN BIT(5) +#define INV_ICM42607_INT_SOURCE3_RESET_DONE_INT2_EN BIT(4) +#define INV_ICM42607_INT_SOURCE3_DRDY_INT2_EN BIT(3) +#define INV_ICM42607_INT_SOURCE3_FIFO_THS_INT2_EN BIT(2) +#define INV_ICM42607_INT_SOURCE3_FIFO_FULL_INT2_EN BIT(1) +#define INV_ICM42607_INT_SOURCE3_AGC_RDY_INT2_EN BIT(0) + +#define INV_ICM42607_REG_INT_SOURCE4 0x2E +#define INV_ICM42607_INT_SOURCE4_I3C_ERROR_INT2_EN BIT(6) +#define INV_ICM42607_INT_SOURCE4_SMD_INT2_EN BIT(3) +#define INV_ICM42607_INT_SOURCE4_WOM_Z_INT2_EN BIT(2) +#define INV_ICM42607_INT_SOURCE4_WOM_Y_INT2_EN BIT(1) +#define INV_ICM42607_INT_SOURCE4_WOM_X_INT2_EN BIT(0) + +#define INV_ICM42607_REG_FIFO_LOST_PKT0 0x2F +#define INV_ICM42607_REG_FIFO_LOST_PKT1 0x30 + +#define INV_ICM42607_REG_INTF_CONFIG0 0x35 +#define INV_ICM42607_INTF_CONFIG0_FIFO_COUNT_FORMAT BIT(6) +#define INV_ICM42607_INTF_CONFIG0_FIFO_COUNT_ENDIAN BIT(5) +#define INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN BIT(4) +#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK GENMASK(1, 0) +#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS \ + FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 2) +#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS \ + FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 3) + +#define INV_ICM42607_REG_INTF_CONFIG1 0x36 +#define INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN BIT(3) +#define INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN BIT(2) +#define INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK GENMASK(1, 0) +#define INV_ICM42607_INTF_CONFIG1_CLKSEL_INT 0 +#define INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL 1 +#define INV_ICM42607_INTF_CONFIG1_CLKSEL_OFF 2 + +#define INV_ICM42607_REG_INT_STATUS_DRDY 0x39 +#define INV_ICM42607_INT_STATUS_DRDY_DATA_RDY BIT(0) + +#define INV_ICM42607_REG_INT_STATUS 0x3A +#define INV_ICM42607_INT_STATUS_ST BIT(7) +#define INV_ICM42607_INT_STATUS_FSYNC BIT(6) +#define INV_ICM42607_INT_STATUS_PLL_RDY BIT(5) +#define INV_ICM42607_INT_STATUS_RESET_DONE BIT(4) +#define INV_ICM42607_INT_STATUS_FIFO_THS BIT(2) +#define INV_ICM42607_INT_STATUS_FIFO_FULL BIT(1) +#define INV_ICM42607_INT_STATUS_AGC_RDY BIT(0) + +#define INV_ICM42607_REG_INT_STATUS2 0x3B +#define INV_ICM42607_INT_STATUS2_SMD BIT(3) +#define INV_ICM42607_INT_STATUS2_WOM_INT GENMASK(2, 0) + +#define INV_ICM42607_REG_INT_STATUS3 0x3C +#define INV_ICM42607_INT_STATUS3_STEP_DET BIT(5) +#define INV_ICM42607_INT_STATUS3_STEP_CNT_OVF BIT(4) +#define INV_ICM42607_INT_STATUS3_TILT_DET BIT(3) +#define INV_ICM42607_INT_STATUS3_FF_DET BIT(2) + +/* +* FIFO access registers +* FIFO count is 16 bits (2 registers) big-endian +* FIFO data is a continuous read register to read FIFO content +*/ +#define INV_ICM42607_REG_FIFO_COUNTH 0x3D +#define INV_ICM42607_REG_FIFO_COUNTL 0x3E +#define INV_ICM42607_REG_FIFO_DATA 0x3F + +#define INV_ICM42607_REG_ACCEL_WOM_X_THR 0x4b +#define INV_ICM42607_REG_ACCEL_WOM_Y_THR 0x4c +#define INV_ICM42607_REG_ACCEL_WOM_Z_THR 0x4d + +#define INV_ICM42607_REG_WHOAMI 0x75 +#define INV_ICM42607P_WHOAMI 0x60 +#define INV_ICM42607_WHOAMI 0x67 + +/* Timings as listed in section 3 of datasheet */ +#define INV_ICM42607_POWER_UP_TIME_US 100000 +#define INV_ICM42607_RESET_TIME_MS 1 +#define INV_ICM42607_ACCEL_STARTUP_TIME_MS 10 +#define INV_ICM42607_GYRO_STARTUP_TIME_MS 30 +#define INV_ICM42607_TEMP_STARTUP_TIME_MS 77 + +typedef int (*inv_icm42607_bus_setup )(struct inv_icm42607_state *); + +extern const struct regmap_config inv_icm42607_regmap_config; +extern const struct inv_icm42607_hw inv_icm42607_hw_data; +extern const struct inv_icm42607_hw inv_icm42607p_hw_data; + +int inv_icm42607_core_probe (struct regmap *regmap, +const struct inv_icm42607_hw *hw, +inv_icm42607_bus_setup bus_setup); + +#endif
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +197 -0
197 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Incorrect use of `regcache_sync()` in probe writes to the read-only WHOAMI register and executes…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incorrect use of regcache_sync() in probe writes to the read-only WHOAMI register and executes before bus setup. --
commit 40cc0df3cdc00863249296cfd5a6b946f44cff13
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
This commit adds the core component of a new inv_icm42607 driver. It includes the initial setup functions and full register definitions for the device.
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +0
-0
[ ... ]
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10
-0+static int inv_icm42607_setup (struct inv_icm42607_state *st, +inv_icm42607_bus_setup inv_icm42607_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;
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 103
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5
-0+/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map); + ret =regcache_sync (st->map); +if (ret) +return ret;
[Severity: Medium]
Will regcache_sync() attempt to write the cached value back to the read-only WHOAMI register here?
Since WHOAMI is read earlier in this function and is not marked as volatile in inv_icm42607_regmap_config, its value is cached. Because the regmap_config lacks a writeable_reg or wr_table definition, regmap assumes all non-volatile registers are writable.
Could this unintended write to a read-only register cause a bus NACK or unexpected hardware behavior, especially since it occurs before inv_icm42607_bus_setup() configures bus-specific settings?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
In reply to Chris MorganQuoted 3 lines · Chris Morgan
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file.
...
Quoted 3 lines
2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
I'm not sure this is correct split by files. These files are not:
- being build
- being mentioned in MAINTAINERS
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 11
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +6
-0+#ifndef INV_ICM42607_H_ +#define INV_ICM42607_H_ + +#include <linux/bits.h> +#include <linux/iio/iio.h> +#include <linux/mutex.h>
mutex_types.h ?
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 13
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +2
-0+#include <linux/regmap.h> +#include <linux/types.h>
Do you need forward declaration for regulator data type?
...
Quoted 1 line
+#define
INV_ICM42607_POWER_UP_TIME_US100000
100 * USEC_PER_MSEC (will require time.h)?
...
Quoted 1 line
+++
b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
+ bitfields.h
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 7
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +2
-0+#include <linux/delay.h> +#include <linux/dev_printk.h>
+ device/devres.h
+ err.h
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 13
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +6
-0+#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/time.h>
Also check if you need types.h.
...
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 94
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10
-0+fsleep (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ); + +/* +* No polling interval specified in datasheet, so use reset time as +* polling interval and 10x reset time as timeout period. +*/ + 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 *USEC_PER_MSEC ), + (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC *10 ));
Besides too many parentheses, this can be switched to regular patter of "num * what". Also, TBH, the plain values would be better here
fsleep(1 * USEC_PER_MSEC); /* * No polling interval specified in datasheet, so use reset time as * polling interval and 10x reset time as timeout period. */ ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS, val, val & INV_ICM42607_INT_STATUS_RESET_DONE, 1 * USEC_PER_MSEC, 10 * USEC_PER_MSEC);
And in the similar way in other fsleep() / _read_poll_timeout() cases.
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3
-0+if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" );
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 8 lines · Andy Shevchenko
Quoted 3 lines · Chris Morgan
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file....
Quoted 7 lines
Quoted 3 lines
2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.cI'm not sure this is correct split by files. These files are not:
- being build
- being mentioned inMAINTAINERS
I can merge patches v3 and v4. Early on they were pretty big but now that they've been paired down some I think it may be manageable.
Early on I was told to try and keep each patch around ~750 lines. I can add these to together and get it to about ~850 lines, but it will be compileable at that commit rather than waiting on a second one.
Quoted 12 lines
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 11
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +6
-0+#ifndef INV_ICM42607_H_ +#define INV_ICM42607_H_ + +#include <linux/bits.h> +#include <linux/iio/iio.h> +#include <linux/mutex.h>
mutex_types.h?
I'm not sure I understand this, can you elaborate why I'd need it?
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 13
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +2
-0+#include <linux/regmap.h> +#include <linux/types.h> Do you need forward declaration for regulator data type?
I think earlier I had regulator-consumer and stripped it out, but I might have cut a bit too deep so I can add that back.
Quoted 6 lines
...
Quoted 1 line
+#define
INV_ICM42607_POWER_UP_TIME_US100000100 *
USEC_PER_MSEC(will requiretime.h)?
Understood.
Quoted 6 lines
...
Quoted 1 line
+++
b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c+
bitfields.h
Understood.
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 7
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +2
-0+#include <linux/delay.h> +#include <linux/dev_printk.h> +
device/devres.h
+err.h
Understood.
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 13
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +6
-0+#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/time.h> Also check if you need
types.h.
I'll add it, I'm honestly not sure. I never got iwyu working it seems.
Quoted 15 lines
...
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 94
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10
-0+fsleep (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ); + +/* +* No polling interval specified in datasheet, so use reset time as +* polling interval and 10x reset time as timeout period. +*/ + 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 *USEC_PER_MSEC ), + (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC *10 ));Besides too many parentheses, this can be switched to regular patter of "num * what". Also, TBH, the plain values would be better here
I'll switch to plain numbers and let the comment above do the explaining.
Quoted 11 lines
fsleep(1 * USEC_PER_MSEC); /* * No polling interval specified in datasheet, so use reset time as * polling interval and 10x reset time as timeout period. */ ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS, val, val & INV_ICM42607_INT_STATUS_RESET_DONE, 1 * USEC_PER_MSEC, 10 * USEC_PER_MSEC);
Quoted 2 lines
And in the similar way in other
fsleep()/_read_poll_timeout()cases.
Plain numbers, gotcha.
Quoted 9 lines
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3
-0+if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" );Signature · 2 lines
With Best Regards, Andy Shevchenko
Thank you,
Chris
Jonathan Cameron
maintainerRe: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
In reply to Chris MorganQuoted 7 lines · On Wed, 10 Jun 2026 12:54:47 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file.Signed-off-by Chris Morgan <[email protected]>
Hi Chris,
A couple more minor things from a fresh read.
Thanks
Jonathan
Quoted 48 lines
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 197 +++++++++++ 2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.cdrivers/iio/imu/inv_icm42607/inv_icm42607.h new file +0
-0drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +29
-0+ +static int inv_icm42607_setup (struct inv_icm42607_state *st, +inv_icm42607_bus_setup inv_icm42607_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; + +/* Warn, but don't fail. */ +if (val != st->hw->whoami) +dev_warn (dev,"Unknown 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 (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ); + +/* +* No polling interval specified in datasheet, so use reset time as +* polling interval and 10x reset time as timeout period. +*/ + ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 101
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10
-0+ val, val &INV_ICM42607_INT_STATUS_RESET_DONE , + (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ), + (INV_ICM42607_RESET_TIME_MS * USEC_PER_MSEC * 10)); +if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" ); + +/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map); + ret =regcache_sync (st->map);
Sashiko raised the point that you don't have a writeable register list for the regmap and so potentially we at very least write a bunch of stuff that isn't needed. I doubt it's actually a problem or you would have seen it, but nice little optimization to reduce what is written.
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5
-0+if (ret) +return ret; + + ret =inv_icm42607_bus_setup (st); +if (ret)
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 116
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10
-0+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 );
That should have a FIELD_PREP() to save us having to got check that the MASK includes the LSB.
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 195
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +8
-0+if (ret) +return ret; + +return inv_icm42607_set_conf (st, st->hw->conf); +} + +MODULE_AUTHOR ("InvenSense, Inc." ); +MODULE_DESCRIPTION ("InvenSense ICM-42607x device driver" );
Why does it have a trailing x? Whilst a wild card is less harmful here than in many places I'd still drop it.
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 197
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +2
-0+MODULE_LICENSE ("GPL" ); +MODULE_IMPORT_NS ("IIO_INV_SENSORS_TIMESTAMP" );
Do we need this yet? I think it only gets used later, in which case move it to the first patch that needs this.
Re: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 73 lines · Jonathan Cameron
Quoted 7 lines · On Wed, 10 Jun 2026 12:54:47 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file.Signed-off-by Chris Morgan <[email protected]>Hi Chris,
A couple more minor things from a fresh read.
Thanks
Jonathan
Quoted 55 lines
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 197 +++++++++++ 2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.cdrivers/iio/imu/inv_icm42607/inv_icm42607.h new file +0
-0drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +36
-0+ +static int inv_icm42607_setup (struct inv_icm42607_state *st, +inv_icm42607_bus_setup inv_icm42607_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; + +/* Warn, but don't fail. */ +if (val != st->hw->whoami) +dev_warn (dev,"Unknown 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 (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ); + +/* +* No polling interval specified in datasheet, so use reset time as +* polling interval and 10x reset time as timeout period. +*/ + 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 *USEC_PER_MSEC ), + (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC *10 )); +if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" ); +
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 101
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3
-0+/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map); + ret =regcache_sync (st->map);Sashiko raised the point that you don't have a writeable register list for the regmap and so potentially we at very least write a bunch of stuff that isn't needed. I doubt it's actually a problem or you would have seen it, but nice little optimization to reduce what is written.
I will add such a list, I'd put it in patch 4 unless you think I should merge patch 3 and 4 to make the driver compileable at the first commit of code (will push the number of lines to the ~850/900 range though).
Quoted 11 lines
Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10
-0+if (ret) +return ret; + + ret =inv_icm42607_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)
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 116
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5
-0+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 );That should have a
FIELD_PREP()to save us having to got check that the MASK includes the LSB.
Okay.
Quoted 7 lines
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5
-0+if (ret) +return ret; + +return inv_icm42607_set_conf (st, st->hw->conf); +}
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 195
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3
-0+ +MODULE_AUTHOR ("InvenSense, Inc." ); +MODULE_DESCRIPTION ("InvenSense ICM-42607x device driver" );Why does it have a trailing x? Whilst a wild card is less harmful here than in many places I'd still drop it.
I think I missed it from the very beginning. I'll drop.
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 197
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +2
-0+MODULE_LICENSE ("GPL" ); +MODULE_IMPORT_NS ("IIO_INV_SENSORS_TIMESTAMP" );Do we need this yet? I think it only gets used later, in which case move it to the first patch that needs this.
No, we don't need it at all anymore. I think I can safely drop this now that I'm no longer calling that code. Was a vestige from the buffers code.
Thank you,
Chris
Jonathan Cameron
maintainerRe: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 83 lines · On Thu, 11 Jun 2026 09:32:38 -0500 Chris Morgan <[email protected]> wrote:
Quoted 73 lines · Jonathan Cameron
Quoted 7 lines · On Wed, 10 Jun 2026 12:54:47 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file.Signed-off-by Chris Morgan <[email protected]>Hi Chris,
A couple more minor things from a fresh read.
Thanks
Jonathan
Quoted 55 lines
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 197 +++++++++++ 2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.cdrivers/iio/imu/inv_icm42607/inv_icm42607.h new file +0
-0drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +36
-0+ +static int inv_icm42607_setup (struct inv_icm42607_state *st, +inv_icm42607_bus_setup inv_icm42607_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; + +/* Warn, but don't fail. */ +if (val != st->hw->whoami) +dev_warn (dev,"Unknown 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 (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ); + +/* +* No polling interval specified in datasheet, so use reset time as +* polling interval and 10x reset time as timeout period. +*/ + 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 *USEC_PER_MSEC ), + (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC *10 )); +if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" ); +Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 101
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3
-0+/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map); + ret =regcache_sync (st->map);Sashiko raised the point that you don't have a writeable register list for the regmap and so potentially we at very least write a bunch of stuff that isn't needed. I doubt it's actually a problem or you would have seen it, but nice little optimization to reduce what is written.
Quoted 4 lines
I will add such a list, I'd put it in patch 4 unless you think I should merge patch 3 and 4 to make the driver compileable at the first commit of code (will push the number of lines to the ~850/900 range though).
I'm fine with them split but don't mind if Andy prefers them combined. His comment on adding a COMPILE_TEST option would allow that to be built, but it is a bit odd to do it just for that purpose.
Jonathan
Re: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 90 lines · Jonathan Cameron
Quoted 83 lines · On Thu, 11 Jun 2026 09:32:38 -0500 Chris Morgan <[email protected]> wrote:
Quoted 73 lines · Jonathan Cameron
Quoted 7 lines · On Wed, 10 Jun 2026 12:54:47 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file.Signed-off-by Chris Morgan <[email protected]>Hi Chris,
A couple more minor things from a fresh read.
Thanks
Jonathan
Quoted 55 lines
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 197 +++++++++++ 2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.cdrivers/iio/imu/inv_icm42607/inv_icm42607.h new file +0
-0drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +36
-0+ +static int inv_icm42607_setup (struct inv_icm42607_state *st, +inv_icm42607_bus_setup inv_icm42607_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; + +/* Warn, but don't fail. */ +if (val != st->hw->whoami) +dev_warn (dev,"Unknown 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 (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ); + +/* +* No polling interval specified in datasheet, so use reset time as +* polling interval and 10x reset time as timeout period. +*/ + 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 *USEC_PER_MSEC ), + (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC *10 )); +if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" ); +Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 101
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3
-0+/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map); + ret =regcache_sync (st->map);Sashiko raised the point that you don't have a writeable register list for the regmap and so potentially we at very least write a bunch of stuff that isn't needed. I doubt it's actually a problem or you would have seen it, but nice little optimization to reduce what is written.
Quoted 4 lines
I will add such a list, I'd put it in patch 4 unless you think I should merge patch 3 and 4 to make the driver compileable at the first commit of code (will push the number of lines to the ~850/900 range though).
Quoted 6 lines
I'm fine with them split but don't mind if Andy prefers them combined. His comment on adding a
COMPILE_TESToption would allow that to be built, but it is a bit odd to do it just for that purpose.Jonathan
I'm going to add the i2c stuff to this series, and then do the SPI on the next patch. That should add only a little bit to this one but still ensure the very first (code) commit has something to compile. I'm going to avoid doing COMPILE_TEST unless collectively you all think I should.
I'm still racing to try and get this one done before the merge window opens up, but I'm pretty sure it's already too late at this point. I'll refrain from submitting updates once the window opens though.
Thank you,
Chris
Jonathan Cameron
maintainerRe: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 104 lines · On Thu, 11 Jun 2026 11:31:09 -0500 Chris Morgan <[email protected]> wrote:
Quoted 90 lines · Jonathan Cameron
Quoted 83 lines · On Thu, 11 Jun 2026 09:32:38 -0500 Chris Morgan <[email protected]> wrote:
Quoted 73 lines · Jonathan Cameron
Quoted 7 lines · On Wed, 10 Jun 2026 12:54:47 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file.Signed-off-by Chris Morgan <[email protected]>Hi Chris,
A couple more minor things from a fresh read.
Thanks
Jonathan
Quoted 55 lines
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 197 +++++++++++ 2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.cdrivers/iio/imu/inv_icm42607/inv_icm42607.h new file +0
-0drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +36
-0+ +static int inv_icm42607_setup (struct inv_icm42607_state *st, +inv_icm42607_bus_setup inv_icm42607_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; + +/* Warn, but don't fail. */ +if (val != st->hw->whoami) +dev_warn (dev,"Unknown 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 (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ); + +/* +* No polling interval specified in datasheet, so use reset time as +* polling interval and 10x reset time as timeout period. +*/ + 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 *USEC_PER_MSEC ), + (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC *10 )); +if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" ); +Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 101
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3
-0+/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map); + ret =regcache_sync (st->map);Sashiko raised the point that you don't have a writeable register list for the regmap and so potentially we at very least write a bunch of stuff that isn't needed. I doubt it's actually a problem or you would have seen it, but nice little optimization to reduce what is written.
Quoted 4 lines
I will add such a list, I'd put it in patch 4 unless you think I should merge patch 3 and 4 to make the driver compileable at the first commit of code (will push the number of lines to the ~850/900 range though).
Quoted 6 lines
I'm fine with them split but don't mind if Andy prefers them combined. His comment on adding a
COMPILE_TESToption would allow that to be built, but it is a bit odd to do it just for that purpose.Jonathan
I'm going to add the i2c stuff to this series, and then do the SPI on the next patch. That should add only a little bit to this one but still ensure the very first (code) commit has something to compile. I'm going to avoid doing
COMPILE_TESTunless collectively you all think I should.
Quoted 4 lines
I'm still racing to try and get this one done before the merge window opens up, but I'm pretty sure it's already too late at this point. I'll refrain from submitting updates once the window opens though.
Unfortunately you are correct. IIO in practice closes around 10 days before the merge window to give time to both expose new things to testing in linux-next and to allow time for Greg KH to merge my pull request into char-misc (which is the path IIO takes to upstream)
Sorry about that, and whilst no need to rush as a result, we also don't stop merging IIO stuff in the merge window, it just sits in the testing branch until I can rebase that on rc1 and then push it out for linux-next to pick up.
Thanks,
Jonathan
Quoted 4 lines
Thank you,
Chris
Re: [PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 116 lines · Jonathan Cameron
Quoted 104 lines · On Thu, 11 Jun 2026 11:31:09 -0500 Chris Morgan <[email protected]> wrote:
Quoted 90 lines · Jonathan Cameron
Quoted 83 lines · On Thu, 11 Jun 2026 09:32:38 -0500 Chris Morgan <[email protected]> wrote:
Quoted 73 lines · Jonathan Cameron
Quoted 7 lines · On Wed, 10 Jun 2026 12:54:47 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file.Signed-off-by Chris Morgan <[email protected]>Hi Chris,
A couple more minor things from a fresh read.
Thanks
Jonathan
Quoted 55 lines
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 197 +++++++++++ 2 files changed, 531 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.cdrivers/iio/imu/inv_icm42607/inv_icm42607.h new file +0
-0drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +36
-0+ +static int inv_icm42607_setup (struct inv_icm42607_state *st, +inv_icm42607_bus_setup inv_icm42607_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; + +/* Warn, but don't fail. */ +if (val != st->hw->whoami) +dev_warn (dev,"Unknown 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 (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC ); + +/* +* No polling interval specified in datasheet, so use reset time as +* polling interval and 10x reset time as timeout period. +*/ + 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 *USEC_PER_MSEC ), + (INV_ICM42607_RESET_TIME_MS *USEC_PER_MSEC *10 )); +if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" ); +Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 101
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3
-0+/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map); + ret =regcache_sync (st->map);Sashiko raised the point that you don't have a writeable register list for the regmap and so potentially we at very least write a bunch of stuff that isn't needed. I doubt it's actually a problem or you would have seen it, but nice little optimization to reduce what is written.
Quoted 4 lines
I will add such a list, I'd put it in patch 4 unless you think I should merge patch 3 and 4 to make the driver compileable at the first commit of code (will push the number of lines to the ~850/900 range though).
Quoted 6 lines
I'm fine with them split but don't mind if Andy prefers them combined. His comment on adding a
COMPILE_TESToption would allow that to be built, but it is a bit odd to do it just for that purpose.Jonathan
I'm going to add the i2c stuff to this series, and then do the SPI on the next patch. That should add only a little bit to this one but still ensure the very first (code) commit has something to compile. I'm going to avoid doing
COMPILE_TESTunless collectively you all think I should.Quoted 4 lines
I'm still racing to try and get this one done before the merge window opens up, but I'm pretty sure it's already too late at this point. I'll refrain from submitting updates once the window opens though.
Unfortunately you are correct. IIO in practice closes around 10 days before the merge window to give time to both expose new things to testing in linux-next and to allow time for Greg KH to merge my pull request into char-misc (which is the path IIO takes to upstream)
Quoted 6 lines
Sorry about that, and whilst no need to rush as a result, we also don't stop merging IIO stuff in the merge window, it just sits in the testing branch until I can rebase that on rc1 and then push it out for linux-next to pick up.
Thanks,
No worries. I think I'm down to 2 bugs with sashiko that I'll try to quash, then I can move on to other stuff. Note that when it complains about the temperature sensor and power management that's a false positive. I tried to put a note there to give it a hint but it doesn't seem to want to take it.
If I haven't communicated it previously, thank you again for your help with this. I thought adding an accelerometer to my handheld would be easy, but I do appreciate everything you and the other maintainers have helped me with.
Chris
Quoted 7 lines
Jonathan
Quoted 4 lines
Thank you,
Chris
PATCH v11 4/9 iio: imu: inv_icm42607: Add I2C and SPI For icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add I2C and SPI driver support for InvenSense ICM-42607 devices. Add necessary Kconfig and Makefile to allow building of (incomplete) driver.
--- drivers/iio/imu/Kconfig | 1 + drivers/iio/imu/Makefile | 1 + drivers/iio/imu/inv_icm42607/Kconfig | 30 +++++ drivers/iio/imu/inv_icm42607/Makefile | 10 ++ drivers/iio/imu/inv_icm42607/inv_icm42607.h | 16 +++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 57 ++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 92 ++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 104 ++++++++++++++++++ 8 files changed, 311 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/Kconfig create mode 100644 drivers/iio/imu/inv_icm42607/Makefile create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c
drivers/iio/imu/Kconfig +1 -0
@@ -109,6 +109,7 @@ config KMX61 be called kmx61. source "drivers/iio/imu/inv_icm42600/Kconfig" +source "drivers/iio/imu/inv_icm42607/Kconfig" source "drivers/iio/imu/inv_icm45600/Kconfig" source "drivers/iio/imu/inv_mpu6050/Kconfig"
drivers/iio/imu/Makefile +1 -0
@@ -25,6 +25,7 @@ obj-$(CONFIG_FXOS8700_I2C) += fxos8700_i2c.o obj-$(CONFIG_FXOS8700_SPI) += fxos8700_spi.o obj-y += inv_icm42600/ +obj-y += inv_icm42607/ obj-y += inv_icm45600/ obj-y += inv_mpu6050/
drivers/iio/imu/inv_icm42607/Kconfig new file +30 -0
@@ -0,0 +1,30 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +config INV_ICM42607 + tristate + select IIO_BUFFER + select IIO_INV_SENSORS_TIMESTAMP + +config INV_ICM42607_I2C + tristate "InvenSense ICM-42607X I2C driver" + depends on I2C + select INV_ICM42607 + select REGMAP_I2C + help + This driver supports the InvenSense ICM-42607 motion tracking + device over I2C. + + This driver can be built as a module. The module will be called + inv-icm42607-i2c. + +config INV_ICM42607_SPI + tristate "InvenSense ICM-42607X SPI driver" + depends on SPI_MASTER + select INV_ICM42607 + select REGMAP_SPI + help + This driver supports the InvenSense ICM-42607 motion tracking + device over SPI. + + This driver can be built as a module. The module will be called + inv-icm42607-spi.
drivers/iio/imu/inv_icm42607/Makefile new file +10 -0
@@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o +inv-icm42607-y += inv_icm42607_core.o + +obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o +inv-icm42607-i2c-y += inv_icm42607_i2c.o + +obj-$(CONFIG_INV_ICM42607_SPI) += inv-icm42607-spi.o +inv-icm42607-spi-y += inv_icm42607_spi.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +16 -0
@@ -12,6 +12,22 @@#include <linux/regmap.h> #include <linux/types.h> +/* +* Serial bus slew rates. Rates are expressed as range between the two +* values with the midpoint as the typical rate. For the final value of +* 2ns, 2ns is considered the max value with no expressed minimum or +* typical value. +*/ +enum inv_icm42607_slew_rate { +INV_ICM42607_SLEW_RATE_20_60NS =0 , +INV_ICM42607_SLEW_RATE_12_36NS =1 , +INV_ICM42607_SLEW_RATE_6_19NS =2 , +INV_ICM42607_SLEW_RATE_4_14NS =3 , +INV_ICM42607_SLEW_RATE_2_6NS =4 , +INV_ICM42607_SLEW_RATE_2NS =5 , +INV_ICM42607_SLEW_RATE_NB +}; +enum inv_icm42607_sensor_mode {INV_ICM42607_SENSOR_MODE_OFF =0 ,INV_ICM42607_SENSOR_MODE_STANDBY =1 ,
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +57 -0
63 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +92 -0
92 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +104 -0
104 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V11 4/9] iio: imu: inv_icm42607: Add I2C and SPI For icm42607
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Missing `<linux/bitfield.h>` include for the `FIELD_PREP` macro. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing <linux/bitfield.h> include for the FIELD_PREP macro.
--
commit 20b53f9fe7967725a7d83e31eaa40543016b6759
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add I2C and SPI For icm42607
Add I2C and SPI driver support for InvenSense ICM-42607 devices. Add necessary Kconfig and Makefile to allow building of (incomplete) driver.
Quoted 22 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +16
-0@@ -0,0 +1,92 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/regmap.h> + +#include "inv_icm42607.h" + +static int inv_icm42607_i2c_bus_setup (struct inv_icm42607_state *st) +{ +unsigned int val; +int ret;
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c line 25
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +8
-0+ ret =regmap_clear_bits (st->map,INV_ICM42607_REG_INTF_CONFIG1 , +INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN | +INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN ); +if (ret) +return ret; + + val =FIELD_PREP (INV_ICM42607_DRIVE_CONFIG2_I2C_MASK , +INV_ICM42607_SLEW_RATE_12_36NS );
[Severity: Low]
Does this code require including <linux/bitfield.h>?
This driver, along with inv_icm42607_spi.c, inv_icm42607_core.c, and macro definitions in inv_icm42607.h heavily use FIELD_PREP(), but the bitfield header isn't included here or in the shared header. Will this result in implicit declaration or undefined macro errors during compilation?
[ ... ]
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +0
-0
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c line 31
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +8
-0+ ret =regmap_clear_bits (st->map,INV_ICM42607_REG_INTF_CONFIG1 , +INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN | +INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN ); +if (ret) +return ret; + + val =FIELD_PREP (INV_ICM42607_DRIVE_CONFIG3_SPI_MASK , +INV_ICM42607_SLEW_RATE_2NS );
[Severity: Low]
Similar to the I2C driver, should <linux/bitfield.h> be included for this FIELD_PREP() usage?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH V11 4/9] iio: imu: inv_icm42607: Add I2C and SPI For icm42607
In reply to Chris MorganQuoted 3 lines · Chris Morgan
Add I2C and SPI driver support for InvenSense ICM-42607 devices. Add necessary
KconfigandMakefileto allow building of (incomplete) driver.
...
Quoted 4 lines
drivers/iio/imu/Kconfig | 1 + drivers/iio/imu/Makefile | 1 + drivers/iio/imu/inv_icm42607/Kconfig | 30 +++++ drivers/iio/imu/inv_icm42607/Makefile | 10 ++
These should be part of the previous patch.
...
drivers/iio/imu/inv_icm42607/Kconfig line 4
Quoted 2 lines
drivers/iio/imu/inv_icm42607/Kconfig new file +2
-0+config INV_ICM42607 + tristate
To make it compilable w.o. SPI/I2C make this as
tristate "Inven... Core driver" if COMPILE_TEST
Quoted 2 lines
drivers/iio/imu/inv_icm42607/Kconfig new file +2
-0+ select IIO_BUFFER + select IIO_INV_SENSORS_TIMESTAMP
...
drivers/iio/imu/inv_icm42607/Makefile line 10
Quoted 5 lines
drivers/iio/imu/inv_icm42607/Makefile new file +5
-0+obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o +inv-icm42607-i2c-y += inv_icm42607_i2c.o + +obj-$(CONFIG_INV_ICM42607_SPI) += inv-icm42607-spi.o +inv-icm42607-spi-y += inv_icm42607_spi.o
Add each of them in a separate patch.
...
Quoted 1 line
+#include <
linux/device.h>
Why?
And a lot of missing headers (bitfield.h, dev_printk.h, mod_devicetable.h, err.h)
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +3
-0+#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/regmap.h>
...
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c line 44
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +9
-0+static int inv_icm42607_probe (struct i2c_client *client) +{ +const struct inv_icm42607_hw *hw; +struct regmap *regmap; + + hw =i2c_get_match_data (client); +if (!hw) +return dev_err_probe (&client->dev, -ENODEV , +"Failed to get i2c data\n" );
Add
struct device *dev = &client->dev;
and modify this and other eligible pieces accordingly.
return dev_err_probe(dev, -ENODEV, "Failed to get i2c data\n");
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c line 49
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +4
-0+ regmap =devm_regmap_init_i2c (client, &inv_icm42607_regmap_config); +if (IS_ERR (regmap)) + return dev_err_probe(&client->dev, PTR_ERR(regmap), + "Failed to register i2c regmap %ld\n",
Do not duplicate a printing of an error code.
Review
Quoted 1 line
(quoted diff) +1
-0+ PTR_ERR(regmap));
return dev_err_probe(dev, PTR_ERR(regmap), "Failed to register i2c regmap\n");
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +3
-0+return inv_icm42607_core_probe (regmap, hw, inv_icm42607_i2c_bus_setup); +} +
...
Quoted 1 line
+++
b/drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c
As per above.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH V11 4/9] iio: imu: inv_icm42607: Add I2C and SPI For icm42607
Quoted 15 lines · On Thu, 11 Jun 2026 10:49:41 +0300 Andy Shevchenko…
Quoted 3 lines · Chris Morgan
Add I2C and SPI driver support for InvenSense ICM-42607 devices. Add necessary
KconfigandMakefileto allow building of (incomplete) driver....
Quoted 4 lines
drivers/iio/imu/Kconfig | 1 + drivers/iio/imu/Makefile | 1 + drivers/iio/imu/inv_icm42607/Kconfig | 30 +++++ drivers/iio/imu/inv_icm42607/Makefile | 10 ++These should be part of the previous patch.
Quoted 8 lines
...
drivers/iio/imu/inv_icm42607/Kconfig line 4
Quoted 2 lines
drivers/iio/imu/inv_icm42607/Kconfig new file +2
-0+config INV_ICM42607 + tristateTo make it compilable w.o. SPI/I2C make this as
tristate "Inven... Core driver" if
COMPILE_TEST
Hi Andy,
Is it worth it? I'm not convinced - we will get pretty good build coverage with a dependence on either SPI or I2C. Plus as someone who always builds with COMPILE_TEST this seems overly noisy to me.
Jonathan
Jonathan Cameron
maintainerRe: [PATCH V11 4/9] iio: imu: inv_icm42607: Add I2C and SPI For icm42607
In reply to Chris MorganQuoted 7 lines · On Wed, 10 Jun 2026 12:54:48 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add I2C and SPI driver support for InvenSense ICM-42607 devices. Add necessary
KconfigandMakefileto allow building of (incomplete) driver.Signed-off-by Chris Morgan <[email protected]>
Some similar missing FIELD_PREP() comments. Not a bug, but a readability issue that is easy to resolve.
Quoted 34 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0
-0drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +23
-0@@ -0,0 +1,92 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/regmap.h> + +#include "inv_icm42607.h" + +static int inv_icm42607_i2c_bus_setup (struct inv_icm42607_state *st) +{ +unsigned int val; +int ret; + + ret =regmap_clear_bits (st->map,INV_ICM42607_REG_INTF_CONFIG1 , +INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN | +INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN ); +if (ret) +return ret; +
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c line 33
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +10
-0+ val =FIELD_PREP (INV_ICM42607_DRIVE_CONFIG2_I2C_MASK , +INV_ICM42607_SLEW_RATE_12_36NS ); + ret =regmap_update_bits (st->map,INV_ICM42607_REG_DRIVE_CONFIG2 , +INV_ICM42607_DRIVE_CONFIG2_I2C_MASK , val); +if (ret) +return ret; + +return regmap_update_bits (st->map,INV_ICM42607_REG_INTF_CONFIG0 , +INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK , +INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS );
FIELD_PREP() (see below)
Quoted 37 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +1
-0+}drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +29
-0@@ -0,0 +1,104 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#include <linux/device.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/spi/spi.h> + +#include "inv_icm42607.h" + +static int inv_icm42607_spi_bus_setup (struct inv_icm42607_state *st) +{ +unsigned int val; +int ret; + +/* Only support 4-wire mode for now. */ + ret =regmap_set_bits (st->map,INV_ICM42607_REG_DEVICE_CONFIG , +INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE ); +if (ret) +return ret; + + ret =regmap_clear_bits (st->map,INV_ICM42607_REG_INTF_CONFIG1 , +INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN | +INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN ); +if (ret) +return ret; +
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c line 39
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +10
-0+ val =FIELD_PREP (INV_ICM42607_DRIVE_CONFIG3_SPI_MASK , +INV_ICM42607_SLEW_RATE_2NS ); + ret =regmap_update_bits (st->map,INV_ICM42607_REG_DRIVE_CONFIG3 , +INV_ICM42607_DRIVE_CONFIG3_SPI_MASK , val); +if (ret) +return ret; + +return regmap_update_bits (st->map,INV_ICM42607_REG_INTF_CONFIG0 , +INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK , +INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS );
Should be a FIELD_PREP() for readability (so reviewer doesn't need to check this field includes bit 0)
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +1
-0+}
Re: [PATCH V11 4/9] iio: imu: inv_icm42607: Add I2C and SPI For icm42607
In reply to Chris MorganHello,
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c line 76
Quoted 11 lines · Chris Morgan
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +11
-0+static const struct spi_device_id inv_icm42607_spi_id_table[] = { + { + .name ="icm42607" , + .driver_data = (kernel_ulong_t)&inv_icm42607_hw_data + }, { + .name ="icm42607p" , + .driver_data = (kernel_ulong_t)&inv_icm42607p_hw_data + }, + { } +}; +MODULE_DEVICE_TABLE (spi, inv_icm42607_spi_id_table);
Missing comma after the .driver_data lines.
Best regards
Uwe
PATCH v11 5/9 iio: imu: inv_icm42607: Add PM support for icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add power management support for the ICM42607 device driver.
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 18 +++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 125 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 1 + .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 1 + 4 files changed, 145 insertions(+)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +18 -0
@@ -9,6 +9,7 @@#include <linux/bits.h> #include <linux/iio/iio.h> #include <linux/mutex.h> +#include <linux/pm.h> #include <linux/regmap.h> #include <linux/types.h>
@@ -103,22 +104,32 @@ struct inv_icm42607_hw {const struct inv_icm42607_conf *conf; }; +struct inv_icm42607_suspended { +enum inv_icm42607_sensor_mode gyro; +enum inv_icm42607_sensor_mode accel; +bool temp; +}; +/** * struct inv_icm42607_state - driver state variables * @lock: lock for serializing multiple registers access. * @hw: Hardware specific data. * @map: regmap pointer. * @vddio_supply: I/O voltage regulator for the chip. +* @vddio_en: I/O voltage status for runtime PM. * @orientation: sensor chip orientation relative to main hardware. * @conf: chip sensors configurations. +* @suspended: suspended sensors configuration. */ struct inv_icm42607_state {struct mutex lock;const struct inv_icm42607_hw *hw;struct regmap *map;struct regulator *vddio_supply; +bool vddio_en;struct iio_mount_matrix orientation;struct inv_icm42607_conf conf; +struct inv_icm42607_suspended suspended; };/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
@@ -337,11 +348,18 @@ struct inv_icm42607_state {#define INV_ICM42607_GYRO_STARTUP_TIME_MS 30#define INV_ICM42607_TEMP_STARTUP_TIME_MS 77 +/* +* Suspend delay assumed from other icm42600 series device, not +* documented in datasheet. +*/ +#define INV_ICM42607_SUSPEND_DELAY_MS 2000 +typedef int (*inv_icm42607_bus_setup )(struct inv_icm42607_state *);extern const struct regmap_config inv_icm42607_regmap_config;extern const struct inv_icm42607_hw inv_icm42607_hw_data;extern const struct inv_icm42607_hw inv_icm42607p_hw_data; +extern const struct dev_pm_ops inv_icm42607_pm_ops; int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw *hw,
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +125 -0
172 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c +1 -0
@@ -80,6 +80,7 @@ static struct i2c_driver inv_icm42607_driver = { .driver = { .name = "inv-icm42607-i2c", .of_match_table = inv_icm42607_of_matches, + .pm = pm_ptr(&inv_icm42607_pm_ops), }, .id_table = inv_icm42607_id, .probe = inv_icm42607_probe,
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c +1 -0
@@ -92,6 +92,7 @@ static struct spi_driver inv_icm42607_driver = { .driver = { .name = "inv-icm42607-spi", .of_match_table = inv_icm42607_of_matches, + .pm = pm_ptr(&inv_icm42607_pm_ops), }, .id_table = inv_icm42607_spi_id_table, .probe = inv_icm42607_probe,
Signature · 1 line
2.43.0
Re: [PATCH V11 5/9] iio: imu: inv_icm42607: Add PM support for icm42607
Quoted 1 line · Chris Morgan
Add power management support for the ICM42607 device driver.
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h struct inv_icm42607_hw { · line 133
Quoted 10 lines
struct
inv_icm42607_state{drivers/iio/imu/inv_icm42607/inv_icm42607.h +2
-0struct mutex lock;const struct inv_icm42607_hw *hw;struct regmap *map;struct regulator *vddio_supply; +bool vddio_en;struct iio_mount_matrix orientation;struct inv_icm42607_conf conf; +struct inv_icm42607_suspended suspended; };
When adding a new member to the data structure always consult with pahole tool to see if the layout is the best fit.
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h struct inv_icm42607_state { · line 355
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h +5
-0+/* +* Suspend delay assumed from other icm42600 series device, not +* documented in datasheet. +*/ +#define INV_ICM42607_SUSPEND_DELAY_MS 2000
2 * USEC_PER_MSEC
...
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4
-0+ sleepval =0 ; +if (temp && !oldtemp) + sleepval =max (sleepval,INV_ICM42607_TEMP_STARTUP_TIME_MS ); +
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c const struct inv_icm42607_hw inv_icm42607p_hw_data = { · line 115
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +10
-0+if (accel != oldaccel) + sleepval =max (sleepval,INV_ICM42607_ACCEL_STARTUP_TIME_MS ); + +if (gyro != oldgyro) + sleepval =max (sleepval,INV_ICM42607_GYRO_STARTUP_TIME_MS ); + +if (sleep_ms) + *sleep_ms = sleepval; +else if (sleepval) +fsleep (sleepval *USEC_PER_MSEC );
Also good to have a _ms suffis in sleepval.
...
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c int inv_icm42607_core_probe(struct regmap *regmap, · line 268
Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +1
-0+dev_set_drvdata (dev, st);
This what requires device.h. So, you can start with it whenever you first need dev_printk.h, devres/*.h, but it doesn't mean you are supposed to drop other non-device.h related headers, such as err.h.
...
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c int inv_icm42607_core_probe(struct regmap *regmap, · line 373
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4
-0+EXPORT_NS_GPL_DEV_PM_OPS(inv_icm42607_pm_ops, IIO_ICM42607) = { + SYSTEM_SLEEP_PM_OPS(inv_icm42607_suspend, inv_icm42607_resume) +RUNTIME_PM_OPS (inv_icm42607_runtime_suspend, + inv_icm42607_runtime_resume,NULL )
Keep it logically split, either as
RUNTIME_PM_OPS(inv_icm42607_runtime_suspend, inv_icm42607_runtime_resume, NULL)
or as
RUNTIME_PM_OPS(inv_icm42607_runtime_suspend, inv_icm42607_runtime_resume, NULL)
Quoted 1 line
+};
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v11 6/9 iio: imu: inv_icm42607: Add Temp Support in icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add functions for reading temperature sensor data.
--- drivers/iio/imu/inv_icm42607/Makefile | 1 + drivers/iio/imu/inv_icm42607/inv_icm42607.h | 5 ++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 18 ++++ .../iio/imu/inv_icm42607/inv_icm42607_temp.c | 82 +++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_temp.h | 33 ++++++++ 5 files changed, 139 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h
drivers/iio/imu/inv_icm42607/Makefile +1 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o inv-icm42607-y += inv_icm42607_core.o +inv-icm42607-y += inv_icm42607_temp.o obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o inv-icm42607-i2c-y += inv_icm42607_i2c.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +5 -0
@@ -120,6 +120,7 @@ struct inv_icm42607_suspended {* @orientation: sensor chip orientation relative to main hardware. * @conf: chip sensors configurations. * @suspended: suspended sensors configuration. +* @buffer: data transfer buffer aligned for DMA. */ struct inv_icm42607_state {struct mutex lock;
@@ -130,6 +131,7 @@ struct inv_icm42607_state {struct iio_mount_matrix orientation;struct inv_icm42607_conf conf;struct inv_icm42607_suspended suspended; +__be16 buffer[3 ]__aligned(IIO_DMA_MINALIGN) ; };/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
@@ -361,6 +363,9 @@ extern const struct inv_icm42607_hw inv_icm42607_hw_data;extern const struct inv_icm42607_hw inv_icm42607p_hw_data;extern const struct dev_pm_ops inv_icm42607_pm_ops; +int inv_icm42607_set_temp_conf (struct inv_icm42607_state *st,bool enable, +unsigned int *sleep_ms); +int inv_icm42607_core_probe (struct regmap *regmap,const struct inv_icm42607_hw *hw,inv_icm42607_bus_setup bus_setup);
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +18 -0
@@ -117,6 +117,24 @@ static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,return 0 ; } +int inv_icm42607_set_temp_conf (struct inv_icm42607_state *st,bool enable, +unsigned int *sleep_ms) +{ +unsigned int val; +int ret; + + val =FIELD_PREP (INV_ICM42607_TEMP_CONFIG0_FILTER_MASK , +INV_ICM42607_FILTER_BW_34HZ ); + ret =regmap_update_bits (st->map,INV_ICM42607_REG_TEMP_CONFIG0 , +INV_ICM42607_TEMP_CONFIG0_FILTER_MASK , val); +if (ret) +return ret; + +return inv_icm42607_set_pwr_mgmt0 (st, st->conf.gyro.mode, + st->conf.accel.mode, enable, + sleep_ms); +} +static int inv_icm42607_set_conf (struct inv_icm42607_state *st,const struct inv_icm42607_conf *conf) {
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +82 -0
82 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h new file +33 -0
@@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#ifndef INV_ICM42607_TEMP_H_ +#define INV_ICM42607_TEMP_H_ + +#include <linux/bitops.h> + +struct iio_dev ; +struct iio_chan_spec ; + +#define INV_ICM42607_TEMP_CHAN (_index) \ +{ \ + .type = IIO_TEMP, \ + .info_mask_separate = \ + BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_OFFSET) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .scan_index = _index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 16, \ + .storagebits = 16, \ + }, \ +} + +int inv_icm42607_temp_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask); + +#endif
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V11 6/9] iio: imu: inv_icm42607: Add Temp Support in icm42607
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The temperature sensor is permanently enabled after the first read, causing continuous power…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The temperature sensor is permanently enabled after the first read, causing continuous power drain, preventing low-power accelerometer clocks, and forcing unnecessary startup delays on subsequent sensor reads. --
commit 910deb05735ec6ed5869092361616349ba5cc0f7
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add Temp Support in icm42607
Add functions for reading temperature sensor data.
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +0
-0
[ ... ]
Quoted 19 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +19
-0+static int inv_icm42607_temp_read (struct inv_icm42607_state *st,s16 *temp) +{ +struct device *dev =regmap_get_device (st->map); +__be16 *raw; +int ret; + +PM_RUNTIME_ACQUIRE_AUTOSUSPEND (dev, pm); + ret =PM_RUNTIME_ACQUIRE_ERR (&pm); +if (ret) +return ret; + +guard (mutex)(&st->lock); + + ret =inv_icm42607_set_temp_conf (st, true,NULL ); +if (ret) +return ret; + + raw = &st->buffer[0 ]; + ret = regmap_bulk_read(st->map, INV_ICM42607_REG_TEMP_DATA1,
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c line 43
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +10
-0+ raw, sizeof(*raw)); +if (ret) +return ret; + + *temp =be16_to_cpup (raw); +if (*temp ==INV_ICM42607_DATA_INVALID ) +return -EINVAL ; + +return 0 ; +}
[Severity: Medium]
Does this leave the temperature sensor permanently enabled?
Looking at inv_icm42607_temp_read(), the sensor is enabled with inv_icm42607_set_temp_conf(st, true, NULL) but is never disabled before the function returns on either the success or error paths.
Because st->conf.temp_en remains true, it seems this would prevent the setting of the INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL bit in inv_icm42607_set_pwr_mgmt0(), permanently disabling the accelerometer low-power clock.
Would this also force a 77ms startup delay on every subsequent pm_runtime resume?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Jonathan Cameron
maintainerRe: [PATCH V11 6/9] iio: imu: inv_icm42607: Add Temp Support in icm42607
In reply to Chris MorganQuoted 5 lines · On Wed, 10 Jun 2026 12:54:50 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add functions for reading temperature sensor data.
Signed-off-by Chris Morgan <[email protected]>
Hi Chris,
A few consistency things around the channel definitions and some left over stuff I think from you ripping out the interrupt / buffered support.
Thanks,
Jonathan
Quoted 20 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +13
-0@@ -0,0 +1,82 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#include <linux/device.h> +#include <linux/iio/iio.h> +#include <linux/mutex.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> + +#include "inv_icm42607.h" +#include "inv_icm42607_temp.h"
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c line 54
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +10
-0+int inv_icm42607_temp_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 temp; +int ret; + +if (chan->type !=IIO_TEMP ) +return -EINVAL ;
This feels little over defensive given the function name. Any bug that called it anyway would be pretty bad!
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c line 57
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +3
-0+ +switch (mask) { +case IIO_CHAN_INFO_RAW :
The lack of having the shared_by_all stuff in here makes things a little fragile (once you've added them to the bitmap below) as which chan->type we get depends on channel registration ordering.
So avoid this being called for those mask elements. I'll reply to next patch to call that out in a few mins.
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c line 59
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +2
-0+if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ;
A driver that isn't yet doing buffered support should not be claiming modes (as it's always in direct mode). If this is serializing for some reason other than avoiding mode transitions, it should be using the local lock. Seems it is doing that anyway so just drop this layer of protection.
Quoted 39 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +23
-0+ ret =inv_icm42607_temp_read (st, &temp); +iio_device_release_direct (indio_dev); +if (ret) +return ret; + *val = temp; +return IIO_VAL_INT ; +/* +* T°C = (temp / 128) + 25 +* Tm°C = 1000 * ((temp * 100 / 12800) + 25) +* scale: 100000 / 12800 ~= 7.8125 +* offset: 3200 +*/ +case IIO_CHAN_INFO_SCALE : + *val =7 ; + *val2 =812500000 ; +return IIO_VAL_INT_PLUS_NANO ; +case IIO_CHAN_INFO_OFFSET : + *val =3200 ; +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +}drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h new file +10
-0@@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#ifndef INV_ICM42607_TEMP_H_ +#define INV_ICM42607_TEMP_H_ + +#include <linux/bitops.h> +
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h line 20
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h new file +10
-0+struct iio_dev ; +struct iio_chan_spec ; + +#define INV_ICM42607_TEMP_CHAN (_index) \ +{ \ + .type = IIO_TEMP, \ + .info_mask_separate = \ + BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_OFFSET) | \ + BIT(IIO_CHAN_INFO_SCALE), \
Whilst it makes no difference to exposure of sysfs attributes, this should include the shared_by_all stuff form the other channels. That is supposed to be in every channel so that we can easily see what affects each channel.
Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h new file +13
-0+ .scan_index = _index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 16, \ + .storagebits = 16, \ + }, \ +} + +int inv_icm42607_temp_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask); + +#endif
PATCH v11 7/9 iio: imu: inv_icm42607: Add Accelerometer for icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add icm42607 accelerometer sensor for icm42607.
--- drivers/iio/imu/inv_icm42607/Makefile | 1 + drivers/iio/imu/inv_icm42607/inv_icm42607.h | 34 ++ .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 379 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 61 +++ 4 files changed, 475 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o inv-icm42607-y += inv_icm42607_core.o +inv-icm42607-y += inv_icm42607_accel.o inv-icm42607-y += inv_icm42607_temp.o obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +34 -0
@@ -84,6 +84,17 @@ enum inv_icm42607_filter_bw {INV_ICM42607_FILTER_BW_NB }; +/* Low-Power mode sensor data filter (averaging) */ +enum inv_icm42607_filter_avg { +INV_ICM42607_FILTER_AVG_2X =0 , +INV_ICM42607_FILTER_AVG_4X =1 , +INV_ICM42607_FILTER_AVG_8X =2 , +INV_ICM42607_FILTER_AVG_16X =3 , +INV_ICM42607_FILTER_AVG_32X =4 , +INV_ICM42607_FILTER_AVG_64X =5 , +/* values 6 and 7 also correspond to 64x. */ +}; +/* Signed so that negative values can signify an invalid condition. */ struct inv_icm42607_sensor_conf {int mode;
@@ -91,6 +102,7 @@ struct inv_icm42607_sensor_conf {int odr;int filter; }; +#define INV_ICM42607_SENSOR_CONF_INIT { -1, -1, -1, -1 }struct inv_icm42607_conf {struct inv_icm42607_sensor_conf gyro;
@@ -120,6 +132,7 @@ struct inv_icm42607_suspended {* @orientation: sensor chip orientation relative to main hardware. * @conf: chip sensors configurations. * @suspended: suspended sensors configuration. +* @indio_accel: accelerometer IIO device. * @buffer: data transfer buffer aligned for DMA. */ struct inv_icm42607_state {
@@ -131,9 +144,20 @@ struct inv_icm42607_state {struct iio_mount_matrix orientation;struct inv_icm42607_conf conf;struct inv_icm42607_suspended suspended; +struct iio_dev *indio_accel;__be16 buffer[3 ]__aligned(IIO_DMA_MINALIGN) ; }; +/** +* struct inv_icm42607_sensor_state - sensor state variables +* @power_mode: sensor requested power mode (for common frequencies) +* @filter: sensor filter. +*/ +struct inv_icm42607_sensor_state { +enum inv_icm42607_sensor_mode power_mode; +int filter; +}; +/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */ /* Register Map for User Bank 0 */
@@ -363,6 +387,14 @@ extern const struct inv_icm42607_hw inv_icm42607_hw_data;extern const struct inv_icm42607_hw inv_icm42607p_hw_data;extern const struct dev_pm_ops inv_icm42607_pm_ops; +const struct iio_mount_matrix * +inv_icm42607_get_mount_matrix (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan); + +int inv_icm42607_set_accel_conf (struct inv_icm42607_state *st, +struct inv_icm42607_sensor_conf *conf, +unsigned int *sleep_ms); +int inv_icm42607_set_temp_conf (struct inv_icm42607_state *st,bool enable,unsigned int *sleep_ms);
@@ -370,4 +402,6 @@ int inv_icm42607_core_probe(struct regmap *regmap,const struct inv_icm42607_hw *hw, inv_icm42607_bus_setup bus_setup); +struct iio_dev *inv_icm42607_accel_init (struct inv_icm42607_state *st); +#endif
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +379 -0
379 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +61 -0
79 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V11 7/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Missing `iio_device_claim_direct()` protection when changing the sampling frequency (ODR) in…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing iio_device_claim_direct() protection when changing the sampling frequency (ODR) in inv_icm42607_accel_write_raw(). --
commit 09acc28167411b3db95fd7b8d7918637d445b62b
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add Accelerometer for icm42607
This commit adds support for the accelerometer sensor to the icm42607 driver.
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +0
-0
[ ... ]
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +8
-0+static int inv_icm42607_accel_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2,long mask) +{ +int ret; + +if (chan->type !=IIO_ACCEL ) +return -EINVAL ;
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 317
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +10
-0+ +switch (mask) { +case IIO_CHAN_INFO_SCALE : +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + ret =inv_icm42607_accel_write_scale (indio_dev, val, val2); +iio_device_release_direct (indio_dev); +return ret; +case IIO_CHAN_INFO_SAMP_FREQ : +return inv_icm42607_accel_write_odr (indio_dev, val, val2);
[Severity: Medium]
Should this IIO_CHAN_INFO_SAMP_FREQ case be protected by iio_device_claim_direct() and iio_device_release_direct() similar to the IIO_CHAN_INFO_SCALE case above it?
While the driver currently only supports INDIO_DIRECT_MODE, omitting the lock here leaves the state management inconsistent. If buffered mode is added in the future, modifying the sampling frequency while the buffer is actively streaming could lead to stream breakage or corrupted timestamps.
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +4
-0+default : +return -EINVAL ; + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
Jonathan Cameron
maintainerRe: [PATCH V11 7/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607
In reply to Chris MorganQuoted 5 lines · On Wed, 10 Jun 2026 12:54:51 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add icm42607 accelerometer sensor for icm42607.
Signed-off-by Chris Morgan <[email protected]>
Minor things inline.
Quoted 24 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +18
-0@@ -0,0 +1,379 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#include <linux/iio/iio.h> +#include <linux/mutex.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> + +#include "inv_icm42607.h" +#include "inv_icm42607_temp.h" + +#define INV_ICM42607_ACCEL_CHAN (_modifier, _index, _ext_info) \ +{ \ + .type = IIO_ACCEL, \ + .modified = 1, \ + .channel2 = _modifier, \
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 28
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +10
-0+ .info_mask_separate = \ + BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = \ + BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_type_available = \ + BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_all = \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .info_mask_shared_by_all_available = \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
See other comments on this is is 'all' and perhaps applies to temperature. If it doesn't apply to temperature then it may need splitting up.
Little less clear cut for mount matrix, so I don't mind that one just
Quoted 32 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +30
-0+ .scan_index = _index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 16, \ + .storagebits = 16, \ + .endianness = IIO_BE, \ + }, \ + .ext_info = _ext_info, \ +} +static int inv_icm42607_accel_write_odr (struct iio_dev *indio_dev, +int val,int val2) +{ +struct inv_icm42607_state *st =iio_device_get_drvdata (indio_dev); +struct device *dev =regmap_get_device (st->map); +unsigned int idx; +struct inv_icm42607_sensor_conf conf =INV_ICM42607_SENSOR_CONF_INIT ; +int ret; + +for (idx =5 ; idx <ARRAY_SIZE (inv_icm42607_accel_odr); ++idx) { +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 ; + + conf.odr = idx; + +PM_RUNTIME_ACQUIRE_AUTOSUSPEND (dev, pm); + ret =PM_RUNTIME_ACQUIRE_ERR (&pm);
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 237
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +10
-0+if (ret) +return ret; + +guard (mutex)(&st->lock); + + ret =inv_icm42607_set_accel_conf (st, &conf,NULL ); +if (ret) +return ret; + +return 0 ;
If this isn't getting more complex later,
return inv_...
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +5
-0+} + +static int inv_icm42607_accel_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask)
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 252
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +10
-0+{ +struct inv_icm42607_state *st =iio_device_get_drvdata (indio_dev); +s16 data; +int ret; + +switch (chan->type) { +case IIO_ACCEL : +break ; +case IIO_TEMP : +return inv_icm42607_temp_read_raw (indio_dev, chan, val, val2, mask);
I commented on this in previous patch, but once the shared_by_all is added to the temp channel, this needs modifying to ensure we only call the temp handler for cases that aren't shared_by_all.
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 260
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +8
-0+default : +return -EINVAL ; + } + +switch (mask) { +case IIO_CHAN_INFO_RAW : +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ;
As below - drop these mode claims as for now there is only one mode.
Quoted 41 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +41
-0+ ret =inv_icm42607_accel_read_sensor (indio_dev, chan, &data); +iio_device_release_direct (indio_dev); +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 ; + +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 ; + } +} + +static int inv_icm42607_accel_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan,
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 311
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +10
-0+ int val, int val2, long mask) +{ + int ret; + +if (chan->type !=IIO_ACCEL ) +return -EINVAL ; + +switch (mask) { +case IIO_CHAN_INFO_SCALE : +if (!iio_device_claim_direct (indio_dev))
As in previous, this stuff doesn't belong in a driver that is always in direct mode.
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +4
-0+return -EBUSY ; + ret =inv_icm42607_accel_write_scale (indio_dev, val, val2); +iio_device_release_direct (indio_dev); +return ret;
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 350
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +9
-0+case IIO_CHAN_INFO_SAMP_FREQ : +return inv_icm42607_accel_write_odr (indio_dev, val, val2); +default : +return -EINVAL ; + } +struct iio_dev *inv_icm42607_accel_init (struct inv_icm42607_state *st) +{ +struct device *dev =regmap_get_device (st->map); +const char *name;
where no other strong reason for ordering, reverse xmas tree.
Quoted 29 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +29
-0+struct inv_icm42607_sensor_state *accel_st; +struct iio_dev *indio_dev; +int ret; + + name =devm_kasprintf (dev,GFP_KERNEL ,"%s-accel" , st->hw->name); +if (!name) +return ERR_PTR (-ENOMEM ); + + indio_dev =devm_iio_device_alloc (dev,sizeof (*accel_st)); +if (!indio_dev) +return ERR_PTR (-ENOMEM ); + accel_st =iio_priv (indio_dev); + + accel_st->power_mode =INV_ICM42607_SENSOR_MODE_LOW_NOISE ; + accel_st->filter =INV_ICM42607_FILTER_BW_73HZ ; + +iio_device_set_drvdata (indio_dev, st); + indio_dev->name = name; + indio_dev->info = &inv_icm42607_accel_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = inv_icm42607_accel_channels; + indio_dev->num_channels =ARRAY_SIZE (inv_icm42607_accel_channels); + + ret =devm_iio_device_register (dev, indio_dev); +if (ret) +return ERR_PTR (ret); + +return indio_dev; +}
PATCH v11 8/9 iio: imu: inv_icm42607: Add Gyroscope to icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add gyroscope functions to the icm42607 driver.
--- drivers/iio/imu/inv_icm42607/Makefile | 1 + drivers/iio/imu/inv_icm42607/inv_icm42607.h | 8 + .../iio/imu/inv_icm42607/inv_icm42607_core.c | 48 +++ .../iio/imu/inv_icm42607/inv_icm42607_gyro.c | 369 ++++++++++++++++++ 4 files changed, 426 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o inv-icm42607-y += inv_icm42607_core.o +inv-icm42607-y += inv_icm42607_gyro.o inv-icm42607-y += inv_icm42607_accel.o inv-icm42607-y += inv_icm42607_temp.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +8 -0
@@ -132,6 +132,7 @@ struct inv_icm42607_suspended {* @orientation: sensor chip orientation relative to main hardware. * @conf: chip sensors configurations. * @suspended: suspended sensors configuration. +* @indio_gyro: gyroscope IIO device. * @indio_accel: accelerometer IIO device. * @buffer: data transfer buffer aligned for DMA. */
@@ -144,6 +145,7 @@ struct inv_icm42607_state {struct iio_mount_matrix orientation;struct inv_icm42607_conf conf;struct inv_icm42607_suspended suspended; +struct iio_dev *indio_gyro;struct iio_dev *indio_accel;__be16 buffer[3 ]__aligned(IIO_DMA_MINALIGN) ; };
@@ -395,6 +397,10 @@ int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,struct inv_icm42607_sensor_conf *conf, unsigned int *sleep_ms); +int inv_icm42607_set_gyro_conf (struct inv_icm42607_state *st, +struct inv_icm42607_sensor_conf *conf, +unsigned int *sleep_ms); +int inv_icm42607_set_temp_conf (struct inv_icm42607_state *st,bool enable,unsigned int *sleep_ms);
@@ -402,6 +408,8 @@ int inv_icm42607_core_probe(struct regmap *regmap,const struct inv_icm42607_hw *hw, inv_icm42607_bus_setup bus_setup); +struct iio_dev *inv_icm42607_gyro_init (struct inv_icm42607_state *st); +struct iio_dev *inv_icm42607_accel_init (struct inv_icm42607_state *st);#endif
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +48 -0
60 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +369 -0
369 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V11 8/9] iio: imu: inv_icm42607: Add Gyroscope to icm42607
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Missing `_available` info masks in `INV_ICM42607_GYRO_CHAN` makes `read_avail` dead code and…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing _available info masks in INV_ICM42607_GYRO_CHAN makes read_avail dead code and hides capabilities from userspace.
- [Medium] Missing iio_device_claim_direct() protection when writing sampling frequency.
--
commit 85b10db1767bd36f30119bab0e0a4f4f63966fc3
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add Gyroscope to icm42607
Add gyroscope functions to the icm42607 driver.
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +0
-0@@ -0,0 +1,369 @@
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 24
Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +11
-0+#define INV_ICM42607_GYRO_CHAN (_modifier, _index, _ext_info) \ +{ \ + .type = IIO_ANGL_VEL, \ + .modified = 1, \ + .channel2 = _modifier, \ + .info_mask_separate = \ + BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = \ + BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_all = \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
[Severity: Medium]
Will userspace be able to discover the available scales and frequencies?
Since .info_mask_shared_by_type_available and .info_mask_shared_by_all_available are not included in this macro, it appears the IIO core will never query inv_icm42607_gyro_read_avail(), leaving that function as dead code.
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +2
-0+ .scan_index = _index, \ + .scan_type = { \
[ ... ]
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +8
-0+static int inv_icm42607_gyro_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2,long mask) +{ +int ret; + +if (chan->type !=IIO_ANGL_VEL ) +return -EINVAL ;
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 308
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10
-0+ +switch (mask) { +case IIO_CHAN_INFO_SCALE : +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + ret =inv_icm42607_gyro_write_scale (indio_dev, val, val2); +iio_device_release_direct (indio_dev); +return ret; +case IIO_CHAN_INFO_SAMP_FREQ : +return inv_icm42607_gyro_write_odr (indio_dev, val, val2);
[Severity: Medium]
Is it safe to write the sampling frequency without claiming direct mode here?
It looks like IIO_CHAN_INFO_SAMP_FREQ bypasses iio_device_claim_direct(), meaning it could potentially be modified concurrently with active IIO data buffering.
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +4
-0+default : +return -EINVAL ; + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
PATCH v11 9/9 arm64: dts: rockchip: Add icm42607p IMU for RG-DS
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add the Invensense ICM42607P IMU for the Anbernic RG-DS. Mount-matrix was tested with iio-sensor-proxy and reports correct orientation.
--- arch/arm64/boot/dts/rockchip/rk3568-anbernic-rg-ds.dts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
arch/arm64/boot/dts/rockchip/rk3568-anbernic-rg-ds.dts +7 -1
@@ -871,7 +871,13 @@ aw87391_pa_r: audio-codec@5b { sound-name-prefix = "Right Amp"; }; - /* invensense,icm42607p at 0x68 */ + icm42607p: imu@68 { + compatible = "invensense,icm42607p"; + reg = <0x68>; + mount-matrix = "-1", "0", "0", + "0", "1", "0", + "0", "0", "-1"; + }; }; &i2c3 {
Signature · 1 line
2.43.0
Jonathan Cameron
maintainerQuoted 8 lines · On Wed, 10 Jun 2026 12:54:44 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add support for the ICM42607 IMU. This sensor shares the same functionality but a different register layout with the existing ICM42600.
This driver should work with the ICM42607 and ICM42607P over both I2C and SPI, however only the ICM42607P over I2C could be tested.
Hi Chris,
Just a quick reminder to check if there is useful feedback at:
https://sashiko.dev/#/patchset/20260610175455.19006-1-macroalpha82%40gmail.com
I didn't look into most of the but the last one about no avail mask bits being set for gyro channels was easy to verify as correct and suggests a testing gap.
Jonathan
Quoted 13 lines · Jonathan Cameron
Quoted 8 lines · On Wed, 10 Jun 2026 12:54:44 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add support for the ICM42607 IMU. This sensor shares the same functionality but a different register layout with the existing ICM42600.
This driver should work with the ICM42607 and ICM42607P over both I2C and SPI, however only the ICM42607P over I2C could be tested.
Hi Chris,
Quoted 9 lines
Just a quick reminder to check if there is useful feedback at:
https://sashiko.dev/#/patchset/20260610175455.19006-1-macroalpha82%40gmail.comI didn't look into most of the but the last one about no avail mask bits being set for gyro channels was easy to verify as correct and suggests a testing gap.
Jonathan
Thanks, I've been checking sashiko on every patch and trying to address what it says to the best of my ability. Honestly I wish there was a way to simply push to that bot first and *then* push to the mailing list once I've resolved all of its comments.
Thank you,
Chris
Jonathan Cameron
maintainerQuoted 21 lines · On Thu, 11 Jun 2026 09:36:08 -0500 Chris Morgan <[email protected]> wrote:
Quoted 20 lines · Jonathan Cameron
Quoted 8 lines · On Wed, 10 Jun 2026 12:54:44 -0500 Chris Morgan <[email protected]> wrote:
From: Chris Morgan <[email protected]>
Add support for the ICM42607 IMU. This sensor shares the same functionality but a different register layout with the existing ICM42600.
This driver should work with the ICM42607 and ICM42607P over both I2C and SPI, however only the ICM42607P over I2C could be tested.
Hi Chris,
Just a quick reminder to check if there is useful feedback at:
https://sashiko.dev/#/patchset/20260610175455.19006-1-macroalpha82%40gmail.comI didn't look into most of the but the last one about no avail mask bits being set for gyro channels was easy to verify as correct and suggests a testing gap.
Quoted 7 lines
Quoted 2 lines
Jonathan
Thanks, I've been checking sashiko on every patch and trying to address what it says to the best of my ability. Honestly I wish there was a way to simply push to that bot first and *then* push to the mailing list once I've resolved all of its comments.
Agreed - that would be helpful. I believe it is possible to run locally against a bunch of LLMs - I recall seeing something saying there
is a git repo with scripts. No idea if that is possible with a free level though.
Jonathan
Quoted 3 lines
Thank you,
Chris