Skip to contentSkip to conversation

Add Invensense ICM42607

PATCH Superseded Chris Morgan 5 participants 25 messages May 9 – May 12

Subsystems (4)

v5 of 21 Superseded Latest is v21 →

lore
Share & tools
lore
#0 lore raw

v5 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.

Chris Morgan (11):
dt-bindings: iio: imu: icm42600: Add mount-matrix to icm42600
dt-bindings: iio: imu: icm42600: Add icm42607 binding
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 Buffer 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 Wake on Movement to icm42607
iio: imu: inv_icm42607: Add Gyroscope to icm42607
arm64: dts: rockchip: Add icm42607p IMU for RG-DS

 .../bindings/iio/imu/invensense,icm42600.yaml |   4 +
 .../dts/rockchip/rk3568-anbernic-rg-ds.dts    |  20 +-
 drivers/iio/imu/Kconfig                       |   1 +
 drivers/iio/imu/Makefile                      |   1 +
 drivers/iio/imu/inv_icm42607/Kconfig          |  30 +
 drivers/iio/imu/inv_icm42607/Makefile         |  14 +
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   | 437 +++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 875 ++++++++++++++++++
 .../imu/inv_icm42607/inv_icm42607_buffer.c    | 552 +++++++++++
 .../imu/inv_icm42607/inv_icm42607_buffer.h    |  95 ++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 795 ++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_gyro.c  | 509 ++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |  85 ++
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   |  92 ++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.c  |  80 ++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.h  |  33 +
 16 files changed, 3622 insertions(+), 1 deletion(-)
 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_buffer.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h
 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
#1 lore raw

PATCH v5 1/11 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
index 119e28a833fd..9b2af104f186 100644
@@ -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
#2 lore raw

PATCH v5 2/11 dt-bindings: iio: imu: icm42600: Add icm42607 binding

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add devicetree binding for the Invensense ICM42607 and Invensense ICM42607P inertial measurement unit. This unit is a combined accelerometer, gyroscope, and thermometer available via I2C or SPI.

This device is functionally very similar to the icm42600 series with a very different register layout.

---
 .../devicetree/bindings/iio/imu/invensense,icm42600.yaml        | 2 ++
 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml +2 -0
index 9b2af104f186..b69c6bbb6f6b 100644
@@ -30,6 +30,8 @@ properties:
       - invensense,icm42600
       - invensense,icm42602
       - invensense,icm42605
+      - invensense,icm42607
+      - invensense,icm42607p
       - invensense,icm42622
       - invensense,icm42631
       - invensense,icm42686
Signature · 1 line
2.43.0
#3 lore raw

PATCH v5 3/11 iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

In reply to Chris Morgan

From: 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   | 332 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 210 +++++++++++
 2 files changed, 542 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 +332 -0
new file mode 100644
index 000000000000..10dcd3473e9c
@@ -0,0 +1,332 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2026 InvenSense, Inc.
+ */
+
+#ifndef INV_ICM42607_H_
+#define INV_ICM42607_H_
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+enum inv_icm42607_sensor_mode {
+	INV_ICM42607_SENSOR_MODE_OFF,
+	INV_ICM42607_SENSOR_MODE_STANDBY,
+	INV_ICM42607_SENSOR_MODE_LOW_POWER,
+	INV_ICM42607_SENSOR_MODE_LOW_NOISE,
+	INV_ICM42607_SENSOR_MODE_NB
+};
+
+/* gyroscope fullscale values */
+enum inv_icm42607_gyro_fs {
+	INV_ICM42607_GYRO_FS_2000DPS,
+	INV_ICM42607_GYRO_FS_1000DPS,
+	INV_ICM42607_GYRO_FS_500DPS,
+	INV_ICM42607_GYRO_FS_250DPS,
+	INV_ICM42607_GYRO_FS_NB
+};
+
+/* accelerometer fullscale values */
+enum inv_icm42607_accel_fs {
+	INV_ICM42607_ACCEL_FS_16G,
+	INV_ICM42607_ACCEL_FS_8G,
+	INV_ICM42607_ACCEL_FS_4G,
+	INV_ICM42607_ACCEL_FS_2G,
+	INV_ICM42607_ACCEL_FS_NB
+};
+
+/* ODR values */
+enum inv_icm42607_odr {
+	INV_ICM42607_ODR_1600HZ = 5,
+	INV_ICM42607_ODR_800HZ,
+	INV_ICM42607_ODR_400HZ,
+	INV_ICM42607_ODR_200HZ,
+	INV_ICM42607_ODR_100HZ,
+	INV_ICM42607_ODR_50HZ,
+	INV_ICM42607_ODR_25HZ,
+	INV_ICM42607_ODR_12_5HZ,
+	INV_ICM42607_ODR_6_25HZ_LP,
+	INV_ICM42607_ODR_3_125HZ_LP,
+	INV_ICM42607_ODR_1_5625HZ_LP,
+	INV_ICM42607_ODR_NB
+};
+
+enum inv_icm42607_filter_bw {
+	/* Low-Noise mode sensor data filter (bandwidth) */
+	INV_ICM42607_FILTER_BYPASS,
+	INV_ICM42607_FILTER_BW_180HZ,
+	INV_ICM42607_FILTER_BW_121HZ,
+	INV_ICM42607_FILTER_BW_73HZ,
+	INV_ICM42607_FILTER_BW_53HZ,
+	INV_ICM42607_FILTER_BW_34HZ,
+	INV_ICM42607_FILTER_BW_25HZ,
+	INV_ICM42607_FILTER_BW_16HZ
+};
+
+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 {
+	uint8_t 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.
+ *  @irq:		chip irq, required to enable/disable and set wakeup
+ *  @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;
+	int irq;
+	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_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
+
+/* Sleep times required by the driver */
+#define INV_ICM42607_POWER_UP_TIME_US			100000
+#define INV_ICM42607_RESET_TIME_MS			1
+#define INV_ICM42607_ACCEL_STARTUP_TIME_MS		20
+#define INV_ICM42607_GYRO_STARTUP_TIME_MS		60
+#define INV_ICM42607_GYRO_STOP_TIME_MS			150
+#define INV_ICM42607_TEMP_STARTUP_TIME_MS		14
+#define INV_ICM42607_SUSPEND_DELAY_MS			2000
+
+typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
+
+extern const struct regmap_config inv_icm42607_regmap_config;
+
+int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw *hw,
+			    inv_icm42607_bus_setup bus_setup);
+
+#endif
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +210 -0

210 lines not on this page · raw message

Signature · 1 line
2.43.0
#4 lore raw

Re: [PATCH V5 03/11] 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] The return value of `dev_warn_probe()` is discarded when a WHOAMI mismatch occurs. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The return value of dev_warn_probe() is discarded when a WHOAMI mismatch occurs. --

commit 6349b0a7b12b07b3aed999d0609cbcf30437d378
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, including setup functions and full register definitions in the header file.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +0 -0
new file mode 100644
index 0000000000000..e84e84416dab5

[ ... ]

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +8 -0
+static int inv_icm42607_setup(struct inv_icm42607_state *st,
+			      inv_icm42607_bus_setup bus_setup)
+{
+	const struct device *dev = regmap_get_device(st->map);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);

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
+	if (ret)
+		return ret;
+
+	if (val != st->hw->whoami)
+		dev_warn_probe(dev, -ENODEV,
+			       "invalid whoami %#02x expected %#02x (%s)\n",
+			       val, st->hw->whoami, st->hw->name);
+
+	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
+			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);

Was the return keyword accidentally omitted here?

The return value of dev_warn_probe() is silently discarded. If the chip's WHOAMI register does not match the expected value, the driver will proceed to perform a soft reset and configure what is potentially an unsupported or unresponsive device.

If the intent was to abort the probe on a mismatch, it should probably return dev_warn_probe(...). If the intent was merely to log a non-fatal warning and continue, would a standard dev_warn() be more appropriate?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
maintainer
#5 lore raw

Re: [PATCH V5 03/11] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

In reply to Chris Morgan
Quoted 7 lines · On Sat, 9 May 2026 14:18:57 -0500 Chris Morgan <[email protected]> wrote:

From: 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.

Hi Chris

The sashiko bot has taken a look and had some comments and one in this patch is valid.

https://sashiko.dev/#/patchset/20260509191907.24734-1-macroalpha82%40gmail.com

It's minor though so if everything else looks good I might just tweak whilst applying.

Quoted 28 lines
---
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   | 332 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 210 +++++++++++
 2 files changed, 542 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 +0 -0
new file mode 100644
index 000000000000..10dcd3473e9c
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +12 -1
new file mode 100644
index 000000000000..e84e84416dab
--- /dev/null
+/**
+ *  inv_icm42607_setup() - check and setup chip
+ *  @st:	driver internal state
+ *  @bus_setup:	callback for setting up bus specific registers
+ *
+ *  Returns 0 on success, a negative error code otherwise.
+ */
+static int inv_icm42607_setup(struct inv_icm42607_state *st,
+			      inv_icm42607_bus_setup bus_setup)
+{
+	const struct device *dev = regmap_get_device(st->map);
+	unsigned int val;

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 91

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10 -0
+	int ret;
+
+	ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
+	if (ret)
+		return ret;
+
+	if (val != st->hw->whoami)
+		dev_warn_probe(dev, -ENODEV,
+			       "invalid whoami %#02x expected %#02x (%s)\n",
+			       val, st->hw->whoami, st->hw->name);

Sashiko correctly called this out as a bit nonsensical. dev_warn_probe() is useful for dealing with deferral or that can't happen with allowing a return dev_warn_probe() type call.

In this particular case it brings us nothing over dev_warn() so better to just use that.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5 -0
+
+	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
+			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
+	if (ret)
+		return ret;
#6 lore raw

Re: [PATCH V5 03/11] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

Quoted 12 lines · Jonathan Cameron
Quoted 7 lines · On Sat, 9 May 2026 14:18:57 -0500 Chris Morgan <[email protected]> wrote:

From: 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.

Hi Chris

Quoted 7 lines

The sashiko bot has taken a look and had some comments and one in this patch is valid.

https://sashiko.dev/#/patchset/20260509191907.24734-1-macroalpha82%40gmail.com

It's minor though so if everything else looks good I might just tweak whilst applying.

I see a few things in the bot's comments that seem valid, so I'll try to address them an do another revision. I think the only one I disagreed with was the stuff about "keeping the temperature sensor enabled", because if I read the data sheet right the temperature sensor really isn't doing anything unless the gyro or accelerometer is also enabled.

Quoted 44 lines
Quoted 30 lines

---
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   | 332 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 210 +++++++++++
 2 files changed, 542 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 +0 -0
new file mode 100644
index 000000000000..10dcd3473e9c
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +12 -1
new file mode 100644
index 000000000000..e84e84416dab
--- /dev/null
+/**
+ *  inv_icm42607_setup() - check and setup chip
+ *  @st:	driver internal state
+ *  @bus_setup:	callback for setting up bus specific registers
+ *
+ *  Returns 0 on success, a negative error code otherwise.
+ */
+static int inv_icm42607_setup(struct inv_icm42607_state *st,
+			      inv_icm42607_bus_setup bus_setup)
+{
+	const struct device *dev = regmap_get_device(st->map);
+	unsigned int val;

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 91

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10 -0
+	int ret;
+
+	ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
+	if (ret)
+		return ret;
+
+	if (val != st->hw->whoami)
+		dev_warn_probe(dev, -ENODEV,
+			       "invalid whoami %#02x expected %#02x (%s)\n",
+			       val, st->hw->whoami, st->hw->name);

Sashiko correctly called this out as a bit nonsensical. dev_warn_probe() is useful for dealing with deferral or that can't happen with allowing a return dev_warn_probe() type call.

Quoted 2 lines

In this particular case it brings us nothing over dev_warn() so better to just use that.

Yeah, I think I called that twice in various places but didn't return from it, so I can change them to just "dev_warn" which makes more sense.

Quoted 8 lines

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 96

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5 -0
+
+	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
+			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
+	if (ret)
+		return ret;

Thank you,
Chris

#7 lore raw

PATCH v5 4/11 iio: imu: inv_icm42607: Add I2C and SPI For icm42607

In reply to Chris Morgan

From: 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   | 12 +++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 51 +++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   | 84 +++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   | 91 +++++++++++++++++++
 8 files changed, 280 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
index 7e0181c27bb6..8bab4616be20 100644
@@ -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
index 13fb7846e9c9..3268dc2371ae 100644
@@ -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
new file mode 100644
index 000000000000..7ba64e6e8d80
@@ -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
new file mode 100644
index 000000000000..be109102e203
@@ -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 +12 -0
index 10dcd3473e9c..3ebc06025051 100644
@@ -13,6 +13,16 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+/* serial bus slew rates */
+enum inv_icm42607_slew_rate {
+	INV_ICM42607_SLEW_RATE_20_60NS,
+	INV_ICM42607_SLEW_RATE_12_36NS,
+	INV_ICM42607_SLEW_RATE_6_18NS,
+	INV_ICM42607_SLEW_RATE_4_12NS,
+	INV_ICM42607_SLEW_RATE_2_6NS,
+	INV_ICM42607_SLEW_RATE_INF_2NS
+};
+
 enum inv_icm42607_sensor_mode {
 	INV_ICM42607_SENSOR_MODE_OFF,
 	INV_ICM42607_SENSOR_MODE_STANDBY,
@@ -325,6 +335,8 @@ struct inv_icm42607_state {
 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);
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +51 -0

57 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +84 -0

84 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +91 -0

91 lines not on this page · raw message

Signature · 1 line
2.43.0
#8 lore raw

Re: [PATCH V5 04/11] iio: imu: inv_icm42607: Add I2C and SPI For icm42607

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Missing NULL check for match data leads to kernel panic. - [Medium] Incorrect regmap ranges…

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing NULL check for match data leads to kernel panic.
- [Medium] Incorrect regmap ranges configuration forces an unnecessary dummy read of register 0 before every access.
- [Medium] The driver ignores a mismatched WHOAMI value due to a missing return statement.
- [Medium] The SPI bus setup unconditionally forces 4-wire SPI mode, breaking 3-wire configurations.
- [Low] IRQ mapping failures are treated as valid interrupts.
--

commit 52263283b912417c0c4ca749220464cd92114b9c
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 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
index e84e84416dab5..8ff51711ab1fe 100644
@@ -15,6 +15,57 @@
 
 #include "inv_icm42607.h"

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 26

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +9 -0
 
+static const struct regmap_range_cfg inv_icm42607_regmap_ranges[] = {
+	{
+		.name = "user bank",
+		.range_min = 0x0000,
+		.range_max = 0x00FF,
+		.window_start = 0,
+		.window_len = 0x0100,
+	},
+};

Is this regmap range configuration missing a selector register?

Because selector_reg and selector_mask default to 0, and max_register is 0xFF, paging might be unnecessary here. During regmap access to registers other than 0, the page check could evaluate to true and trigger a dummy read on register 0 before every transaction.

[ ... ]

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +0 -0
new file mode 100644
index 0000000000000..3859517f88c94
@@ -0,0 +1,84 @@

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c line 41

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +4 -0
+static int inv_icm42607_probe(struct i2c_client *client)
+{
+	const struct inv_icm42607_hw *hw = i2c_get_match_data(client);
+	struct regmap *regmap;

If the device is bound dynamically via sysfs without a matching device ID, can i2c_get_match_data() return NULL? If so, could this lead to a kernel panic when hw is unconditionally dereferenced later in inv_icm42607_core_probe()?

[ ... ]

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +0 -0
new file mode 100644
index 0000000000000..4a38a11f2baef
@@ -0,0 +1,91 @@

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c line 21

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +7 -0
+static int inv_icm42607_spi_bus_setup(struct inv_icm42607_state *st)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_set_bits(st->map, INV_ICM42607_REG_DEVICE_CONFIG,
+			      INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE);

Does this unconditionally force 4-wire SPI mode, even if the SPI controller is configured for 3-wire mode?

If the hardware is physically connected using 3-wire SPI and configured with spi-3wire in the device tree, will forcing the 4-wire bit break communication? Should this check for SPI_3WIRE first?

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +2 -0
+	if (ret)
+		return ret;

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c line 46

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +4 -0
+static int inv_icm42607_probe(struct spi_device *spi)
+{
+	const struct inv_icm42607_hw *hw = spi_get_device_match_data(spi);
+	struct regmap *regmap;

Similarly, can spi_get_device_match_data() return NULL here and cause a kernel panic during probe if the device is bound dynamically?

[ ... ]

Could the WHOAMI mismatch warning be silently ignored?

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c:inv_icm42607_setup() {
    ...
	if (val != st->hw->whoami)
		dev_warn_probe(dev, -ENODEV,
			       "invalid whoami %#02x expected %#02x (%s)\n",
			       val, st->hw->whoami, st->hw->name);

	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
    ...
}

It looks like the driver attempts to abort by calling dev_warn_probe(), but fails to return the result. Does this cause the -ENODEV error to be discarded, allowing the driver to proceed configuring an unrecognized device?

Are IRQ mapping failures handled correctly during probe?

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c:inv_icm42607_core_probe() {
    ...
	irq = fwnode_irq_get_byname(fwnode, "INT1");
	if (irq < 0)
		return dev_err_probe(dev, irq, "error missing INT1 interrupt\n");
    ...
}

fwnode_irq_get_byname() can return 0 if the interrupt exists but fails to be mapped by the irq domain. By only checking if irq < 0, does this allow 0 to pass as a valid IRQ, potentially failing later when requesting the threaded IRQ?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
maintainer
#9 lore raw

Re: [PATCH V5 04/11] iio: imu: inv_icm42607: Add I2C and SPI For icm42607

In reply to Chris Morgan
Quoted 7 lines · On Sat, 9 May 2026 14:18:58 -0500 Chris Morgan <[email protected]> wrote:

From: 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.

Sashiko got busy and found something that I now realize I didn't understand at all :(

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
index e84e84416dab..8ff51711ab1f 100644

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 24

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +7 -0
@@ -15,6 +15,57 @@
 
 #include "inv_icm42607.h"
 
+static const struct regmap_range_cfg inv_icm42607_regmap_ranges[] = {
+	{
+		.name = "user bank",
+		.range_min = 0x0000,
+		.range_max = 0x00FF,
+		.window_start = 0,
+		.window_len = 0x0100,

I'd never really looked at this bit of regmap before but sashiko did suggest a possible issue and from the documentation it appears it might be right.
* Registers, mapped to this virtual range, are accessed in two steps:
* 1. page selector register update;
* 2. access through data window registers.

So whether or not we want one the page selector infrastructure is used.

So Sashiko suggested we'll get a bank setting write to register 0 with value 0.

Having dug around I found a 42607-c datasheet:
https://www.lcsc.com/datasheet/C2879807.pdf?spm=wm.sxq.inf.ggs&lcsc_vid=EVNXBlQAR1RZAwIHTwMLUVcEFgVcAQdQQgVeBlJSQAIxVlNRT1RcVldeQlZeVzsOAxUeFF5JWBYZEEoBGA4JCwFIFA4DSA%3D%3D

That suggests there is a bank select register but at 0x7c which isn't documented for the 42607P :(

It also doesn't correspond to conventional window / bank arrangement so this probably doesn't apply anyway.

Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +12 -0
+	},
+};
+
+const struct regmap_config inv_icm42607_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x00FF,
+	.ranges = inv_icm42607_regmap_ranges,
+	.num_ranges = ARRAY_SIZE(inv_icm42607_regmap_ranges),
+	.cache_type = REGCACHE_NONE,
+};
+EXPORT_SYMBOL_NS_GPL(inv_icm42607_regmap_config, "IIO_ICM42607");
#10 lore raw

Re: [PATCH V5 04/11] iio: imu: inv_icm42607: Add I2C and SPI For icm42607

Quoted 2 lines · Jonathan Cameron

On Sat, 9 May 2026 14:18:58 -0500
Chris Morgan <[email protected]> wrote:

...

Quoted 5 lines

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 21

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4 -0
+static const struct regmap_range_cfg inv_icm42607_regmap_ranges[] = {
+	{
+		.name = "user bank",
 
+		.range_min = 0x0000,

range_min == 0 is a bug for sure. Yet it may work with current regmap implementation, it has to be fixed. See, for example, this pending patch:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/ci.git/commit/?h=regmap-warn-range-overlap&id=062bb97410835055036490c96cf77898f90d34ea

Quoted 3 lines

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 24

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +3 -0
+		.range_max = 0x00FF,
+		.window_start = 0,
+		.window_len = 0x0100,

This makes no sense. If you have a page enabled the range max has to be PAGES * window_len value. This suggests that we have a single page, means no paging is used. Very confusing.

...

Quoted 8 lines

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 34

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +7 -0
+const struct regmap_config inv_icm42607_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x00FF,
+	.ranges = inv_icm42607_regmap_ranges,
+	.num_ranges = ARRAY_SIZE(inv_icm42607_regmap_ranges),
 
+	.cache_type = REGCACHE_NONE,

Why no cache?

Quoted 1 line
Quoted 1 line

+};

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#11 lore raw

PATCH v5 5/11 iio: imu: inv_icm42607: Add PM support for icm42607

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add power management support for the ICM42607 device driver.

---
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   |  10 ++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 148 +++++++++++++++++-
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |   1 +
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   |   1 +
 4 files changed, 159 insertions(+), 1 deletion(-)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +10 -0
index 3ebc06025051..623ff3936b4b 100644
@@ -10,6 +10,7 @@
 #include <linux/bitops.h>
 #include <linux/iio/iio.h>
 #include <linux/mutex.h>
+#include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -96,6 +97,12 @@ 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.
@@ -105,6 +112,7 @@ struct inv_icm42607_hw {
  *  @irq:		chip irq, required to enable/disable and set wakeup
  *  @orientation:	sensor chip orientation relative to main hardware.
  *  @conf:		chip sensors configurations.
+ *  @suspended:		suspended sensors configuration.
  */
 struct inv_icm42607_state {
 	struct mutex lock;
@@ -114,6 +122,7 @@ struct inv_icm42607_state {
 	int irq;
 	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,6 +346,7 @@ 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,
 			    inv_icm42607_bus_setup bus_setup);
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +147 -1

177 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c +1 -0
index 3859517f88c9..d4fab3d12753 100644
@@ -72,6 +72,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
index 4a38a11f2bae..23204194eca5 100644
@@ -79,6 +79,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
#12 lore raw

Re: [PATCH V5 05/11] iio: imu: inv_icm42607: Add PM support for icm42607

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Unbalanced regulator disable upon driver unbind due to incorrect devres ordering and PM status…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbalanced regulator disable upon driver unbind due to incorrect devres ordering and PM status check. --

commit bd7f7b2e2bfc8853252ca783dc08bb6dfb4d8df7
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add PM support for icm42607

This commit adds power management support for the ICM42607 device driver.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
index 8ff51711ab1fe..047de1b405507 100644

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st) · line 256

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +3 -1
@@ -192,8 +249,10 @@ static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st)
 static void inv_icm42607_disable_vddio_reg(void *_data)
 {
 	struct inv_icm42607_state *st = _data;
+	struct device *dev = regmap_get_device(st->map);
 
-	regulator_disable(st->vddio_supply);
+	if (!pm_runtime_suspended(dev))
+		regulator_disable(st->vddio_supply);
 }

Can this lead to an unbalanced regulator disable upon driver unbind?

During driver unbind, devres actions execute in reverse order. The PM runtime cleanup added by devm_pm_runtime_set_active_enabled() runs first, calling pm_runtime_disable() which increments disable_depth, and pm_runtime_set_suspended() which overwrites runtime_status to RPM_SUSPENDED.

Next, this devm action executes and checks pm_runtime_suspended(dev). Because pm_runtime_suspended() requires !dev->power.disable_depth, it will unconditionally return false here, causing the action to always call regulator_disable().

If the device was already runtime-suspended prior to unbind, its regulator was already disabled by inv_icm42607_runtime_suspend(), resulting in an unbalanced double-disable that can shut off power to shared devices.

Simply changing the check to pm_runtime_status_suspended() will not work either, because pm_runtime_set_suspended() has already forced the status to RPM_SUSPENDED, which would cause the regulator to leak if the device was active.

Does the driver need to ensure the device is resumed before unbind, or use a custom remove function, to correctly balance the regulator?

[ ... ]

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +7 -0
@@ -251,10 +310,97 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
 	if (ret)
 		return ret;
 
+	ret = devm_pm_runtime_set_active_enabled(dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
+
 	return 0;
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
maintainer
#13 lore raw

Re: [PATCH V5 05/11] iio: imu: inv_icm42607: Add PM support for icm42607

In reply to Chris Morgan
Quoted 5 lines · On Sat, 9 May 2026 14:18:59 -0500 Chris Morgan <[email protected]> wrote:

From: Chris Morgan <[email protected]>

Add power management support for the ICM42607 device driver.

More stuff in here from sashikio.

The runtime pm comment in particular feeds into a couple of other ongoing threads if true:

https://sashiko.dev/#/patchset/20260509191907.24734-1-macroalpha82%40gmail.com

[PATCH 3/8] iio: magnetometer: ak8975: switch to using managed resources
is the other thread where we have a similar but subtly different setup.

One other thing inline that is related to this. I just left the whole thing here so we have the code to discuss ways around this runtime_pm devm mess :(

Fairly sure this runs into the other problem we had which is that the devm_ regulator cleanup can run before we have any valid state for runtime pm.

I don't have anything setup to hammer these corners of runtime PM. I can probably get something up in a few days. Overall I think we really need to find and test a pattern that actually works and gets the reference counting right for this devm regulator + runtime suspend regulator combination. We have gotten it wrong too often :(

Jonathan

Quoted 132 lines
---
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   |  10 ++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 148 +++++++++++++++++-
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |   1 +
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   |   1 +
 4 files changed, 159 insertions(+), 1 deletion(-)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +10 -0
index 3ebc06025051..623ff3936b4b 100644
@@ -10,6 +10,7 @@
 #include <linux/bitops.h>
 #include <linux/iio/iio.h>
 #include <linux/mutex.h>
+#include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -96,6 +97,12 @@ 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.
@@ -105,6 +112,7 @@ struct inv_icm42607_hw {
  *  @irq:		chip irq, required to enable/disable and set wakeup
  *  @orientation:	sensor chip orientation relative to main hardware.
  *  @conf:		chip sensors configurations.
+ *  @suspended:		suspended sensors configuration.
  */
 struct inv_icm42607_state {
 	struct mutex lock;
@@ -114,6 +122,7 @@ struct inv_icm42607_state {
 	int irq;
 	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,6 +346,7 @@ 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,
 			    inv_icm42607_bus_setup bus_setup);
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +57 -0
index 8ff51711ab1f..047de1b40550 100644
@@ -9,6 +9,7 @@
 #include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -66,6 +67,62 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = {
 };
 EXPORT_SYMBOL_NS_GPL(inv_icm42607p_hw_data, "IIO_ICM42607");
 
+static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
+				      enum inv_icm42607_sensor_mode gyro,
+				      enum inv_icm42607_sensor_mode accel,
+				      bool temp, unsigned int *sleep_ms)
+{
+	enum inv_icm42607_sensor_mode oldgyro = st->conf.gyro.mode;
+	enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
+	bool oldtemp = st->conf.temp_en;
+	unsigned int sleepval;
+	unsigned int val;
+	int ret;
+
+	if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
+		return 0;
+
+	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
+	val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
+	if (!temp)
+		val |= INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL;
+	ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
+	if (ret)
+		return ret;
+
+	st->conf.gyro.mode = gyro;
+	st->conf.accel.mode = accel;
+	st->conf.temp_en = temp;
+
+	sleepval = 0;
+	if (temp && !oldtemp) {
+		if (sleepval < INV_ICM42607_TEMP_STARTUP_TIME_MS)
+			sleepval = INV_ICM42607_TEMP_STARTUP_TIME_MS;
+	}
+	if (accel != oldaccel && oldaccel == INV_ICM42607_SENSOR_MODE_OFF) {
+		usleep_range(200, 300);
+		if (sleepval < INV_ICM42607_ACCEL_STARTUP_TIME_MS)
+			sleepval = INV_ICM42607_ACCEL_STARTUP_TIME_MS;
+	}
+	if (gyro != oldgyro) {
+		if (oldgyro == INV_ICM42607_SENSOR_MODE_OFF) {
+			usleep_range(200, 300);
+			if (sleepval < INV_ICM42607_GYRO_STARTUP_TIME_MS)
+				sleepval = INV_ICM42607_GYRO_STARTUP_TIME_MS;
+		} else if (gyro == INV_ICM42607_SENSOR_MODE_OFF) {
+			if (sleepval < INV_ICM42607_GYRO_STOP_TIME_MS)
+				sleepval = INV_ICM42607_GYRO_STOP_TIME_MS;
+		}
+	}
+
+	if (sleep_ms)
+		*sleep_ms = sleepval;
+	else if (sleepval)
+		msleep(sleepval);
+
+	return 0;
+}
+
 static int inv_icm42607_set_conf(struct inv_icm42607_state *st,
 				 const struct inv_icm42607_conf *conf)
 {

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st) · line 254

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +2 -1
@@ -192,8 +249,10 @@ static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st)
 static void inv_icm42607_disable_vddio_reg(void *_data)
 {
 	struct inv_icm42607_state *st = _data;
+	struct device *dev = regmap_get_device(st->map);
 
-	regulator_disable(st->vddio_supply);
+	if (!pm_runtime_suspended(dev))

Can this run before we've set the state up? We had some previous issues with needing to set active earlier just so the status check would not indicate it was suspended.

Quoted 126 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +88 -0
+		regulator_disable(st->vddio_supply);
 }
 
 int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw *hw,
@@ -251,10 +310,97 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
 	if (ret)
 		return ret;
 
+	ret = devm_pm_runtime_set_active_enabled(dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
+
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");
 
+/*
+ * Suspend saves sensors state and turns everything off.
+ * Check first if runtime suspend has not already done the job.
+ */
+static int inv_icm42607_suspend(struct device *dev)
+{
+	struct inv_icm42607_state *st = dev_get_drvdata(dev);
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	if (pm_runtime_suspended(dev))
+		return 0;
+
+	ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
+					 INV_ICM42607_SENSOR_MODE_OFF,
+					 false, NULL);
+	if (ret)
+		return ret;
+	regulator_disable(st->vddio_supply);
+
+	return 0;
+}
+
+/*
+ * System resume gets the system back on and restores the sensors state.
+ * Manually put runtime power management in system active state.
+ */
+static int inv_icm42607_resume(struct device *dev)
+{
+	struct inv_icm42607_state *st = dev_get_drvdata(dev);
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	if (pm_runtime_suspended(dev))
+		return 0;
+
+	ret = inv_icm42607_enable_vddio_reg(st);
+	if (ret)
+		return ret;
+
+	/* Nothing else to restore at this time. */
+
+	return 0;
+}
+
+static int inv_icm42607_runtime_suspend(struct device *dev)
+{
+	struct inv_icm42607_state *st = dev_get_drvdata(dev);
+	int ret = 0;
+
+	guard(mutex)(&st->lock);
+
+	ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
+					 INV_ICM42607_SENSOR_MODE_OFF, false,
+					 NULL);
+	if (ret)
+		return ret;
+
+	regulator_disable(st->vddio_supply);
+
+	return 0;
+}
+
+static int inv_icm42607_runtime_resume(struct device *dev)
+{
+	struct inv_icm42607_state *st = dev_get_drvdata(dev);
+
+	guard(mutex)(&st->lock);
+
+	return inv_icm42607_enable_vddio_reg(st);
+}
+
+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)
+};
+
 MODULE_AUTHOR("InvenSense, Inc.");
 MODULE_DESCRIPTION("InvenSense ICM-42607x device driver");
 MODULE_LICENSE("GPL");
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c +1 -0
index 3859517f88c9..d4fab3d12753 100644
@@ -72,6 +72,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
index 4a38a11f2bae..23204194eca5 100644
@@ -79,6 +79,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,
#14 lore raw

PATCH v5 6/11 iio: imu: inv_icm42607: Add Buffer support for icm42607

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add all FIFO parsing and reading functions to support inv_icm42607 hardware.

---
 drivers/iio/imu/inv_icm42607/Makefile         |   1 +
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   |  27 +
 .../imu/inv_icm42607/inv_icm42607_buffer.c    | 473 ++++++++++++++++++
 .../imu/inv_icm42607/inv_icm42607_buffer.h    |  93 ++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  53 +-
 5 files changed, 646 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h
drivers/iio/imu/inv_icm42607/Makefile +1 -0
index be109102e203..3c9d08509793 100644
@@ -2,6 +2,7 @@
 
 obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o
 inv-icm42607-y += inv_icm42607_core.o
+inv-icm42607-y += inv_icm42607_buffer.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 +27 -0
index 623ff3936b4b..6511567a456f 100644
@@ -14,6 +14,10 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/iio/common/inv_sensors_timestamp.h>
+
+#include "inv_icm42607_buffer.h"
+
 /* serial bus slew rates */
 enum inv_icm42607_slew_rate {
 	INV_ICM42607_SLEW_RATE_20_60NS,
@@ -84,6 +88,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;
@@ -113,6 +118,10 @@ 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.
+ *  @fifo:		FIFO management structure.
+ *  @buffer:		data transfer buffer aligned for DMA.
  */
 struct inv_icm42607_state {
 	struct mutex lock;
@@ -123,6 +132,22 @@ 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;
+	struct inv_icm42607_fifo fifo;
+	__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.
+ * @ts:			timestamp module states.
+ */
+struct inv_icm42607_sensor_state {
+	enum inv_icm42607_sensor_mode power_mode;
+	int filter;
+	struct inv_sensors_timestamp ts;
 };
 
 /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
@@ -348,6 +373,8 @@ 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;
 
+u32 inv_icm42607_odr_to_period(enum inv_icm42607_odr odr);
+
 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_buffer.c new file +473 -0

473 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h new file +93 -0

93 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +52 -1

85 lines not on this page · raw message

Signature · 1 line
2.43.0
#15 lore raw

Re: [PATCH V5 06/11] iio: imu: inv_icm42607: Add Buffer support for icm42607

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [Critical] Uninitialized variable usage and kernel heap buffer overflow during FIFO read. - [Critical]…

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Uninitialized variable usage and kernel heap buffer overflow during FIFO read.
- [Critical] Uninitialized pointer dereference and infinite loop vulnerability due to integer type mismatch.
- [High] Hardware FIFO stream corruption due to oversized register read.
- [High] Logic inversion completely breaks watermark truncation.
- [High] Integer underflow on internal state variable permanently disables the device on error rollback.
- [High] Race condition due to missing lock in hwfifo_flush callback.
- [High] Data corruption due to missing bounds check on partial hardware FIFO packets.
- [Medium] Missing hardware delays during buffer postdisable.
- [Medium] Watermark interrupt is leaked/permanently disabled on regmap error path.
- [Medium] Ignored return value leads to silent failure during system resume.
--

commit bdccadc0097311be50ac67b485f5dccd2b087eff
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add Buffer support for icm42607

This commit adds FIFO parsing and reading functions to support buffer transfers on the inv_icm42607 hardware.

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c new file +0 -0
@@ -148,15 +148,12 @@
 
 [ ... ]
 

Review

Quoted 7 lines
(quoted diff) +7 -0
+static unsigned int inv_icm42607_wm_truncate(unsigned int watermark,
+					     size_t packet_size)
+{
+	size_t wm_size;
+
+	wm_size = watermark * packet_size;
+	wm_size = max(wm_size, INV_ICM42607_FIFO_WATERMARK_MAX);

Is this max() intentional? It forces the watermark to always be greater than or equal to the maximum hardware limit. Should this be min() instead?

Quoted 6 lines
(quoted diff) +6 -0
+
+	return wm_size / packet_size;
+}
+
+/**
+ * inv_icm42607_buffer_update_watermark - update watermark FIFO threshold

[ ... ]

Quoted 5 lines
(quoted diff) +5 -0
+	/* changing FIFO watermark requires to turn off watermark interrupt */
+	ret = regmap_update_bits_check(st->map, INV_ICM42607_REG_INT_SOURCE0,
+				       INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN,
+				       0, &restore);
+	if (ret)

Review

Quoted 10 lines
(quoted diff) +10 -0
+		return ret;
+
+	/* compute watermark value in bytes */
+	wm_size = watermark * packet_size;
+	raw_wm = INV_ICM42607_FIFO_WATERMARK_VAL(wm_size);
+	memcpy(st->buffer, &raw_wm, sizeof(raw_wm));
+	ret = regmap_bulk_write(st->map, INV_ICM42607_REG_FIFO_CONFIG2,
+				st->buffer, sizeof(raw_wm));
+	if (ret)
+		return ret;

If this regmap_bulk_write() fails, the function returns immediately without restoring the interrupt. Could this leave the watermark interrupt permanently disabled?

Quoted 12 lines
(quoted diff) +12 -0
+
+	/* restore watermark interrupt */
+	if (restore) {
+		ret = regmap_update_bits(st->map, INV_ICM42607_REG_INT_SOURCE0,
+					 INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN,
+					 INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}

[ ... ]

Quoted 21 lines
(quoted diff) +21 -0
+static int inv_icm42607_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	if (st->fifo.on > 1) {
+		st->fifo.on--;
+		return 0;
+	}
+
+	/* set FIFO in bypass mode */
+	ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
+			   INV_ICM42607_FIFO_CONFIG1_BYPASS);
+	if (ret)
+		return ret;
+
+	/* flush FIFO data */
+	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
+			   INV_ICM42607_SIGNAL_PATH_RESET_FIFO_FLUSH);

Review

Quoted 10 lines
(quoted diff) +10 -0
+	if (ret)
+		return ret;
+
+	/* disable FIFO threshold interrupt */
+	ret = regmap_update_bits(st->map, INV_ICM42607_REG_INT_SOURCE0,
+				 INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN, 0);
+	if (ret)
+		return ret;
+
+	st->fifo.on--;

If inv_icm42607_buffer_postenable() fails early before incrementing st->fifo.on, and the IIO core rolls back by calling this predisable function, st->fifo.on would be 0 here. Since it is unsigned, could this decrement cause it to underflow to UINT_MAX, leaving the device permanently disabled for future calls?

Quoted 14 lines
(quoted diff) +14 -0
+
+	return 0;
+}
+
+static int inv_icm42607_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	struct device *dev = regmap_get_device(st->map);
+	unsigned int sensor;
+	unsigned int *watermark;
+	unsigned int sleep_temp = 0;
+	unsigned int sleep_sensor = 0;
+	unsigned int sleep;
+	int ret;

[ ... ]

Review

Quoted 12 lines
(quoted diff) +12 -0
+	*watermark = 0;
+	ret = inv_icm42607_buffer_update_watermark(st);
+	if (ret)
+		goto out_unlock;
+
+out_unlock:
+	mutex_unlock(&st->lock);
+
+	/* sleep maximum required time */
+	sleep = max(sleep_sensor, sleep_temp);
+	if (sleep)
+		msleep(sleep);

Both sleep_temp and sleep_sensor are initialized to 0 and never modified before this check. Does this mean the required hardware shutdown power-transition delays are skipped completely?

[ ... ]

Quoted 11 lines
(quoted diff) +11 -0
+int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,
+				  unsigned int max)
+{
+	const void *accel, *gyro, *timestamp;
+	size_t i, max_count, size;
+	const s8 *temp;
+	int ret;
+
+	/* reset all samples counters */
+	st->fifo.count = 0;
+	st->fifo.nb.gyro = 0;

Review

Quoted 10 lines
(quoted diff) +10 -0
+	st->fifo.nb.accel = 0;
+	st->fifo.nb.total = 0;
+
+	/* compute maximum FIFO read size */
+	if (max == 0)
+		max_count = sizeof(st->fifo.data);
+	else
+		max_count = clamp(max_count,
+				  (max * inv_icm42607_get_packet_size(st->fifo.en)),
+				  sizeof(st->fifo.data));

If max != 0, max_count is passed uninitialized to clamp(). Could this lead to undefined behavior?

Review

Quoted 4 lines
(quoted diff) +4 -0
+
+	/* read FIFO count value */
+	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_FIFO_COUNTH,
+			       st->buffer, sizeof(st->buffer));

Is it safe to use sizeof(st->buffer) here? st->buffer is declared as an array of 3 __be16 elements (6 bytes). The hardware FIFO count register is 2 bytes, so a 6-byte read would spill over into the adjacent FIFO_DATA registers, unintentionally popping 4 bytes of data and breaking alignment.

Review

Quoted 9 lines
(quoted diff) +9 -0
+	if (ret)
+		return ret;
+	st->fifo.count = be16_to_cpup(st->buffer);
+
+	/* check and clamp FIFO count value */
+	if (st->fifo.count == 0)
+		return 0;
+
+	st->fifo.count = max(st->fifo.count, max_count);

Should this be min() instead of max()? Using max() forces the count to be at least the potentially uninitialized max_count value. This could cause regmap_noinc_read() to read past the hardware count and overflow the st->fifo.data buffer.

Review

Quoted 13 lines
(quoted diff) +13 -0
+
+	/* read all FIFO data in internal buffer */
+	ret = regmap_noinc_read(st->map, INV_ICM42607_REG_FIFO_DATA,
+				st->fifo.data, st->fifo.count);
+	if (ret)
+		return ret;
+
+	/* compute number of samples for each sensor */
+	for (i = 0; i < st->fifo.count; i += size) {
+		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
+				&accel, &gyro, &temp, &timestamp);
+		if (size <= 0)
+			break;

inv_icm42607_fifo_decode_packet() returns a ssize_t which can be -EINVAL. Since size is of type size_t, the negative error code becomes a huge unsigned integer. The size <= 0 check will evaluate to false, skipping error handling.

Since decode_packet() does not initialize accel and gyro on error, could this result in dereferencing uninitialized pointers below?

Also, if the hardware returns a byte count that ends with a partial packet, could this loop access memory beyond what was populated from the hardware since there is no check for i + size > st->fifo.count?

Quoted 8 lines
(quoted diff) +8 -0
+		if (gyro != NULL && inv_icm42607_fifo_is_data_valid(gyro))
+			st->fifo.nb.gyro++;
+		if (accel != NULL && inv_icm42607_fifo_is_data_valid(accel))
+			st->fifo.nb.accel++;
+		st->fifo.nb.total++;
+	}
+
+	return 0;

Review

Quoted 10 lines
(quoted diff) +10 -0
+}
+
+int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
+				     unsigned int count)
+{
+	s64 gyro_ts, accel_ts;
+	int ret;
+
+	gyro_ts = iio_get_time_ns(st->indio_gyro);
+	accel_ts = iio_get_time_ns(st->indio_accel);

These timestamps appear to be unused. Should they be passed to the buffer logic or are they just dead code?

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+	ret = inv_icm42607_buffer_fifo_read(st, count);

Does this need to be protected by mutex_lock(&st->lock)? Since this callback can be triggered via sysfs concurrently with the normal interrupt handler, it seems possible to have interleaved regmap reads or corrupted internal FIFO sample counters.

Review

Quoted 3 lines
(quoted diff) +3 -0
+
+	return ret;
+}
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4 -1
@@ -404,8 +404,11 @@
 	if (ret)
 		return ret;
 
-	/* Nothing else to restore at this time. */
+	if (st->fifo.on) {
+		ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
+				   INV_ICM42607_FIFO_CONFIG1_MODE);
+	}
 
 	return 0;
 }
 

Is the return value from regmap_write() meant to be checked? Currently, ret is assigned but not used, so the function unconditionally returns 0 even if the write fails.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
maintainer
#16 lore raw

Re: [PATCH V5 06/11] iio: imu: inv_icm42607: Add Buffer support for icm42607

In reply to Chris Morgan
Quoted 6 lines · On Sat, 9 May 2026 14:19:00 -0500 Chris Morgan <[email protected]> wrote:

From: Chris Morgan <[email protected]>

Add all FIFO parsing and reading functions to support inv_icm42607 hardware.

Sashiko was busy on this one as well.

Not all necessary valid but at least some are.
Given I'm way behind with reviews and it's getting late I'm going to stop look at this series for now. Please take a close look at:
https://sashiko.dev/#/patchset/20260509191907.24734-1-macroalpha82%40gmail.com

If there are things you disagree with that's fine but it has a bunch of things in there that are definitely correct.

Thanks

J

#17 lore raw

PATCH v5 7/11 iio: imu: inv_icm42607: Add Temp Support in icm42607

In reply to Chris Morgan

From: 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   |  3 +
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 18 +++++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.c  | 80 +++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.h  | 33 ++++++++
 5 files changed, 135 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
index 3c9d08509793..ccb8e007cdeb 100644
@@ -3,6 +3,7 @@
 obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o
 inv-icm42607-y += inv_icm42607_core.o
 inv-icm42607-y += inv_icm42607_buffer.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 +3 -0
index 6511567a456f..06deebae5230 100644
@@ -375,6 +375,9 @@ extern const struct dev_pm_ops inv_icm42607_pm_ops;
 
 u32 inv_icm42607_odr_to_period(enum inv_icm42607_odr odr);
 
+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
index c956bf02da47..90b3db1de8a5 100644
@@ -159,6 +159,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 +80 -0

80 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h new file +33 -0
new file mode 100644
index 000000000000..e03924e30866
@@ -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
#18 lore raw

PATCH v5 8/11 iio: imu: inv_icm42607: Add Accelerometer for icm42607

In reply to Chris Morgan

From: 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   |  27 +
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 574 ++++++++++++++++++
 .../imu/inv_icm42607/inv_icm42607_buffer.c    |  63 +-
 .../imu/inv_icm42607/inv_icm42607_buffer.h    |   4 +-
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  74 +++
 6 files changed, 739 insertions(+), 4 deletions(-)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
index ccb8e007cdeb..e908d77c4219 100644
@@ -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_buffer.o
 inv-icm42607-y += inv_icm42607_temp.o
 
drivers/iio/imu/inv_icm42607/inv_icm42607.h +27 -0
index 06deebae5230..cfda60905d71 100644
@@ -82,6 +82,17 @@ enum inv_icm42607_filter_bw {
 	INV_ICM42607_FILTER_BW_16HZ
 };
 
+enum inv_icm42607_filter_avg {
+	/* Low-Power mode sensor data filter (averaging) */
+	INV_ICM42607_FILTER_AVG_2X = 0,
+	INV_ICM42607_FILTER_AVG_4X,
+	INV_ICM42607_FILTER_AVG_8X,
+	INV_ICM42607_FILTER_AVG_16X,
+	INV_ICM42607_FILTER_AVG_32X,
+	INV_ICM42607_FILTER_AVG_64X
+	/* values 7 and 8 also correspond to 64x. */
+};
+
 struct inv_icm42607_sensor_conf {
 	int mode;
 	int fs;
@@ -120,6 +131,7 @@ struct inv_icm42607_suspended {
  *  @suspended:		suspended sensors configuration.
  *  @indio_gyro:	gyroscope IIO device.
  *  @indio_accel:	accelerometer IIO device.
+ *  @timestamp:         interrupt timestamps.
  *  @fifo:		FIFO management structure.
  *  @buffer:		data transfer buffer aligned for DMA.
  */
@@ -134,6 +146,9 @@ struct inv_icm42607_state {
 	struct inv_icm42607_suspended suspended;
 	struct iio_dev *indio_gyro;
 	struct iio_dev *indio_accel;
+	struct {
+		s64 accel;
+	} timestamp;
 	struct inv_icm42607_fifo fifo;
 	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
 };
@@ -373,12 +388,24 @@ 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);
+
 u32 inv_icm42607_odr_to_period(enum inv_icm42607_odr odr);
 
+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);
 
 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);
+
+int inv_icm42607_accel_parse_fifo(struct iio_dev *indio_dev);
+
 #endif
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +574 -0

574 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +60 -3

118 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h +3 -1
index b77deb66f8bd..4160076bbf77 100644
@@ -71,7 +71,7 @@ inv_icm42607_fifo_is_data_valid(const struct inv_icm42607_fifo_sensor_data *s)
 
 ssize_t inv_icm42607_fifo_decode_packet(const void *packet, const void **accel,
 					const void **gyro, const s8 **temp,
-					const void **timestamp);
+					const void **timestamp, unsigned int *odr);
 
 extern const struct iio_buffer_setup_ops inv_icm42607_buffer_ops;
 
@@ -87,6 +87,8 @@ int inv_icm42607_buffer_update_watermark(struct inv_icm42607_state *st);
 int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,
 				  unsigned int max);
 
+int inv_icm42607_buffer_fifo_parse(struct inv_icm42607_state *st);
+
 int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
 				     unsigned int count);
 
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +74 -0

113 lines not on this page · raw message

Signature · 1 line
2.43.0
#19 lore raw

Re: [PATCH V5 08/11] iio: imu: inv_icm42607: Add Accelerometer for icm42607

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [High] Uninitialized `st->indio_gyro` dereference in `hwfifo_flush`. - [High] Uninitialized variable and…

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Uninitialized st->indio_gyro dereference in hwfifo_flush.
- [High] Uninitialized variable and buffer overflow in inv_icm42607_buffer_fifo_read.
- [High] Out-of-bounds read in inv_icm42607_accel_parse_fifo due to missing boundary check.
- [High] Torn read and data race on st->timestamp.accel.
- [Medium] Ignored error code in inv_icm42607_accel_read_sensor.
- [Medium] Missing rollback of temperature sensor configuration on error.
- [Medium] Software timestamp state is not rolled back if hardware ODR configuration fails.
- [Medium] IIO device registration race with wakeup configuration.
- [Medium] Invalid Array Hole / Reserved State Exposure in inv_icm42607_accel_odr. --

commit c0c9f30a70b1f8b6ae97565d96f6f70b49377e9d
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add Accelerometer for icm42607

Add icm42607 accelerometer sensor for icm42607.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +0 -0
index 0000000000000..f90ba46bd7e48

[ ... ]

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +3 -0
+static int inv_icm42607_accel_update_scan_mode(struct iio_dev *indio_dev,
+					       const unsigned long *scan_mask)
+{

[ ... ]

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +5 -0
+	if (*scan_mask & INV_ICM42607_SCAN_MASK_TEMP) {
+		/* enable temp sensor */
+		ret = inv_icm42607_set_temp_conf(st, true, &sleep_temp);
+		if (ret)
+			goto out_unlock;

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 139

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +10 -0
+		fifo_en |= INV_ICM42607_SENSOR_TEMP;
+	}
+
+	if (*scan_mask & INV_ICM42607_SCAN_MASK_ACCEL_3AXIS) {
+		/* enable accel sensor */
+		conf.mode = accel_st->power_mode;
+		conf.filter = accel_st->filter;
+		ret = inv_icm42607_set_accel_conf(st, &conf, &sleep_accel);
+		if (ret)
+			goto out_unlock;

If inv_icm42607_set_accel_conf() fails here, the function jumps to out_unlock and returns an error.

Does this leave the temperature sensor actively consuming power indefinitely, since it is not disabled on the error path?

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +4 -0
+static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev,
+					  struct iio_chan_spec const *chan,
+					  s16 *val)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 210

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +6 -0
+	*val = be16_to_cpup(data);
+	if (*val == INV_ICM42607_DATA_INVALID)
+		ret = -EINVAL;
+
+	return 0;
+}

Should this return ret instead of 0?

If *val == INV_ICM42607_DATA_INVALID, ret is set to -EINVAL but then discarded. This regression would silently leak the invalid data value (-32768) to user-space.

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 273

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +13 -0
+static const int inv_icm42607_accel_odr[][2] = {
+	[INV_ICM42607_ODR_1_5625HZ_LP] = { 1, 562500 },
+	[INV_ICM42607_ODR_3_125HZ_LP] = { 3, 125000 },
+	[INV_ICM42607_ODR_6_25HZ_LP] = { 6, 250000 },
+	[INV_ICM42607_ODR_12_5HZ] = { 12, 500000 },
+	[INV_ICM42607_ODR_25HZ] = { 25, 0 },
+	[INV_ICM42607_ODR_50HZ] = { 50, 0 },
+	[INV_ICM42607_ODR_100HZ] = { 100, 0 },
+	[INV_ICM42607_ODR_200HZ] = { 200, 0 },
+	[INV_ICM42607_ODR_400HZ] = { 400, 0 },
+	[INV_ICM42607_ODR_800HZ] = { 800, 0 },
+	[INV_ICM42607_ODR_1600HZ] = { 1600, 0 }
+};

Does this array leave indices 0 through 4 zero-initialized?

Since inv_icm42607_accel_read_avail() exposes ARRAY_SIZE elements, this regression could allow user-space to configure the hardware with a zeroed/reserved ODR state.

[ ... ]

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +3 -0
+static int inv_icm42607_accel_write_odr(struct iio_dev *indio_dev,
+					int val, int val2)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 330

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +8 -0
+	ret = inv_sensors_timestamp_update_odr(ts, inv_icm42607_odr_to_period(conf.odr),
+					       iio_buffer_enabled(indio_dev));
+	if (ret)
+		return ret;
+
+	ret = inv_icm42607_set_accel_conf(st, &conf, NULL);
+	if (ret)
+		return ret;

If inv_icm42607_set_accel_conf() fails here, the software timestamping state has already been updated to the new ODR.

Will this regression break software timestamp tracking for all future samples because the hardware continues running at the old ODR?

[ ... ]

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +2 -0
+struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 533

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +11 -0
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/* accel events are wakeup capable */
+	ret = devm_device_init_wakeup(&indio_dev->dev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return indio_dev;
+}

Could registering the IIO device before initializing its wakeup capabilities cause a race condition?

Once devm_iio_device_register() succeeds, the device is visible to user-space, so udev rules might attempt to access the power/wakeup sysfs attributes before they are configured.

[ ... ]

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +2 -0
+int inv_icm42607_accel_parse_fifo(struct iio_dev *indio_dev)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 554

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +7 -0
+	/* parse all fifo packets */
+	for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {
+		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
+				&accel, &gyro, &temp, &timestamp, &odr);
+		/* quit if error or FIFO is empty */
+		if (size <= 0)
+			return size;

Is it possible for this to read past the end of st->fifo.data if a packet header is near the end of the buffer?

If the remaining bytes are fewer than size, there is no boundary check against st->fifo.count before the data is parsed and copied. Could this leak adjacent kernel memory?

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +0 -0
index d8784ccf26728..eb629b86446b0 100644

[ ... ]

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +5 -0
+int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,
+				  unsigned int max)
+{
+	const void *accel, *gyro, *timestamp;
+	size_t i, max_count, size;

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +7 -0
+	/* compute maximum FIFO read size */
+	if (max == 0)
+		max_count = sizeof(st->fifo.data);
+	else
+		max_count = clamp(max_count,
+				  (max * inv_icm42607_get_packet_size(st->fifo.en)),
+				  sizeof(st->fifo.data));

Does this use an uninitialized value for max_count?

The clamp() macro evaluates its first parameter, which hasn't been initialized here.

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +9 -0
+	/* check and clamp FIFO count value */
+	if (st->fifo.count == 0)
+		return 0;
+
+	st->fifo.count = max(st->fifo.count, max_count);
+
+	/* read all FIFO data in internal buffer */
+	ret = regmap_noinc_read(st->map, INV_ICM42607_REG_FIFO_DATA,
+				st->fifo.data, st->fifo.count);

Should this use min() instead of max()?

If the hardware returns an erroneously large count, using max() could cause st->fifo.count to exceed the size of st->fifo.data, leading to a buffer overflow during regmap_noinc_read().

[ ... ]

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +2 -0
+int inv_icm42607_buffer_fifo_parse(struct inv_icm42607_state *st)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st, · line 476

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +5 -0
+	/* handle accelerometer timestamp and FIFO data parsing */
+	if (st->fifo.nb.accel > 0) {
+		ts = &accel_st->ts;
+		inv_sensors_timestamp_interrupt(ts, st->fifo.watermark.eff_accel,
+						st->timestamp.accel);

Can st->timestamp.accel be overwritten by a new hardirq while it is being read here?

Since st->timestamp.accel is a 64-bit value updated in the hardirq handler without locks, reading it in the threaded irq handler could cause a torn read on 32-bit architectures, or associate the parsed FIFO data with the timestamp of a newer interrupt.

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +9 -0
+int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
+				     unsigned int count)
+{
+	struct inv_icm42607_sensor_state *accel_st = iio_priv(st->indio_accel);
+	struct inv_sensors_timestamp *ts;
+	s64 gyro_ts, accel_ts;
+	int ret;
+
+	gyro_ts = iio_get_time_ns(st->indio_gyro);

Will this crash if st->indio_gyro is NULL?

Since gyroscope support is not yet implemented in this patch, st->indio_gyro remains uninitialized. Calling iio_get_time_ns() evaluates to_iio_dev_opaque(), which could compute a negative offset from the NULL pointer and cause a kernel oops.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
#20 lore raw

PATCH v5 9/11 iio: imu: inv_icm42607: Add Wake on Movement to icm42607

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add support for wake on movement for the icm42607 driver.

---
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   |  17 +
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 301 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 213 ++++++++++++-
 3 files changed, 521 insertions(+), 10 deletions(-)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +17 -0
index cfda60905d71..ab79a056f28c 100644
@@ -119,6 +119,14 @@ struct inv_icm42607_suspended {
 	bool temp;
 };
 
+struct inv_icm42607_apex {
+	unsigned int on;
+	struct {
+		u64 value;
+		bool enable;
+	} wom;
+};
+
 /**
  *  struct inv_icm42607_state - driver state variables
  *  @lock:		lock for serializing multiple registers access.
@@ -132,6 +140,7 @@ struct inv_icm42607_suspended {
  *  @indio_gyro:	gyroscope IIO device.
  *  @indio_accel:	accelerometer IIO device.
  *  @timestamp:         interrupt timestamps.
+ *  @apex:		APEX (Advanced Pedometer and Event detection) management
  *  @fifo:		FIFO management structure.
  *  @buffer:		data transfer buffer aligned for DMA.
  */
@@ -149,6 +158,7 @@ struct inv_icm42607_state {
 	struct {
 		s64 accel;
 	} timestamp;
+	struct inv_icm42607_apex apex;
 	struct inv_icm42607_fifo fifo;
 	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
 };
@@ -401,6 +411,9 @@ int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
 int inv_icm42607_set_temp_conf(struct inv_icm42607_state *st, bool enable,
 			       unsigned int *sleep_ms);
 
+int inv_icm42607_enable_wom(struct inv_icm42607_state *st);
+int inv_icm42607_disable_wom(struct inv_icm42607_state *st);
+
 int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw *hw,
 			    inv_icm42607_bus_setup bus_setup);
 
@@ -408,4 +421,8 @@ struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st);
 
 int inv_icm42607_accel_parse_fifo(struct iio_dev *indio_dev);
 
+void inv_icm42607_accel_handle_events(struct iio_dev *indio_dev,
+				      unsigned int status2, unsigned int status3,
+				      s64 timestamp);
+
 #endif
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +301 -0

337 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +203 -10

273 lines not on this page · raw message

Signature · 1 line
2.43.0
#21 lore raw

Re: [PATCH V5 09/11] iio: imu: inv_icm42607: Add Wake on Movement to icm42607

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] Self-deadlock in inv_icm42607_accel_write_event_config() due to nested mutex locking. - [High]…

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Self-deadlock in inv_icm42607_accel_write_event_config() due to nested mutex locking.
- [High] ABBA deadlock in inv_icm42607_suspend() between the driver mutex and disable_irq().
- [High] Data corruption and uninitialized memory access due to using a __be16 array for 8-bit regmap I/O.
- [High] TOCTOU race condition in inv_icm42607_accel_write_event_config() causes PM usage counter imbalances.
- [Medium] Integer overflow in Wake-on-Motion threshold calculation.
- [Medium] IIO device is registered before its interrupt handler is fully initialized.
--

commit c90077c0769c3ddde8afd25e1494bccd1e0f18d4
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add Wake on Movement to icm42607

Add support for wake on movement for the icm42607 driver.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +0 -0
index f90ba46bd7e48..99aaa370bd124 100644

[ ... ]

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +0 -0
@@ -209,6 +221,178 @@ static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev,

[ ... ]

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +3 -0
+static unsigned int inv_icm42607_accel_convert_roc_to_wom(u64 roc,
+							  int accel_hz, int accel_uhz)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev, · line 237

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +2 -0
+	freq_uhz = (u64)accel_hz * MICRO + (u64)accel_uhz;
+	value = div64_u64(roc * MICRO, freq_uhz * (u64)convert);

Is there a risk of integer overflow when multiplying roc by MICRO?

If the user-supplied rate of change exceeds approximately 18,446, roc * MICRO will exceed U64_MAX. Could this wrap-around lead to applying a miscalculated threshold to the sensor?

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +4 -0
+static int inv_icm42607_accel_set_wom_threshold(struct inv_icm42607_state *st,
+						u64 value,
+						int accel_hz, int accel_uhz)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev, · line 275

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +5 -0
+	/* set accel WoM threshold for the 3 axes */
+	st->buffer[0] = threshold;
+	st->buffer[1] = threshold;
+	st->buffer[2] = threshold;
+	ret = regmap_bulk_write(st->map, INV_ICM42607_REG_ACCEL_WOM_X_THR, st->buffer, 3);

Can this write corrupted data?

st->buffer is declared as __be16 buffer[3]. Assigning an 8-bit threshold to st->buffer[0], st->buffer[1], and st->buffer[2] writes 16-bit values.

A 3-byte regmap_bulk_write() then reads only the first 3 bytes from memory, which writes the first byte of st->buffer[0], the second byte of st->buffer[0], and the first byte of st->buffer[1]. Could this write 0 and misaligned data to the WoM threshold registers?

[ ... ]

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +8 -0
+static int inv_icm42607_accel_write_event_config(struct iio_dev *indio_dev,
+						 const struct iio_chan_spec *chan,
+						 enum iio_event_type type,
+						 enum iio_event_direction dir,
+						 bool state)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c static int inv_icm42607_accel_hwfifo_flush(struct iio_dev *indio_dev, · line 691

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +10 -0
+	/* handle only WoM (roc rising) event */
+	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
+		return -EINVAL;
+
+	scoped_guard(mutex, &st->lock) {
+		if (st->apex.wom.enable == state)
+			return 0;
+
+	if (state)
+		return inv_icm42607_accel_enable_wom(indio_dev);

Will this sequence cause a self-deadlock?

This function acquires st->lock via scoped_guard. It then calls inv_icm42607_accel_enable_wom(), which calls pm_runtime_resume_and_get().

If the device is suspended, this synchronously invokes inv_icm42607_resume(), which also attempts to acquire st->lock. Additionally, _inv_icm42607_accel_enable_wom() explicitly acquires st->lock again.

Since kernel mutexes are not recursive, does this guarantee a deadlock?

drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c static int inv_icm42607_accel_hwfifo_flush(struct iio_dev *indio_dev, · line 694

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +3 -0
+	}
+
+	return inv_icm42607_accel_disable_wom(indio_dev);

Does this drop the lock before disabling WoM, opening a race condition?

If multiple threads write 0 to the sysfs node simultaneously, they could all pass the st->apex.wom.enable == state check inside the lock, then sequentially call inv_icm42607_accel_disable_wom() outside the lock.

Could this cause unbalanced pm_runtime_put_autosuspend() calls and underflow st->apex.on?

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +1 -0
+}
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
index ce59f9337ed9b..e5d031ca631d6 100644

[ ... ]

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
@@ -343,6 +375,112 @@ static int inv_icm42607_setup(struct inv_icm42607_state *st,

[ ... ]

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +2 -0
+static irqreturn_t inv_icm42607_irq_handler(int irq, void *_data)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c static int inv_icm42607_setup(struct inv_icm42607_state *st, · line 407

Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +12 -0
+	if (st->apex.on) {
+		unsigned int status2, status3;
+
+		/* read INT_STATUS2 and INT_STATUS3 in 1 operation */
+		ret = regmap_bulk_read(st->map, INV_ICM42607_REG_INT_STATUS2, st->buffer, 2);
+		if (ret) {
+			dev_err(dev, "Interrupt status read error %d\n", ret);
+			return IRQ_HANDLED;
+		}
+
+		status2 = st->buffer[0];
+		status3 = st->buffer[1];

Can this read uninitialized memory?

st->buffer is declared as __be16 buffer[3], so its elements are 16 bits. A 2-byte regmap_bulk_read() populates only the first two bytes of the array.

Assigning status2 = st->buffer[0] reads both bytes as a single 16-bit integer, and status3 = st->buffer[1] reads bytes 2 and 3, which contain uninitialized memory.

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw · line 588

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +5 -0
@@ -437,6 +580,11 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
 	if (IS_ERR(st->indio_accel))
 		return PTR_ERR(st->indio_accel);
 
+	/* Initialize interrupt handling */
+	ret = inv_icm42607_irq_init(st, irq, irq_type, open_drain);
+	if (ret)
+		return ret;
+
 	return 0;

Is the IIO device registered before its interrupt handler is fully initialized?

st->indio_accel is initialized, which calls devm_iio_device_register(), before inv_icm42607_irq_init() is called.

Could userspace open the device and enable events or buffers before the hardware interrupt is ready?

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
@@ -448,6 +596,9 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");
 static int inv_icm42607_suspend(struct device *dev)
 {
 	struct inv_icm42607_state *st = dev_get_drvdata(dev);

[ ... ]

Quoted 1 line

guard(mutex)(&st->lock);

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -4
-	ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
-					 INV_ICM42607_SENSOR_MODE_OFF,
-					 false, NULL);
-	if (ret)

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c static int inv_icm42607_suspend(struct device *dev) · line 625

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +8 -2
-		return ret;
-	regulator_disable(st->vddio_supply);
+	/* keep chip on and wake-up capable if APEX and wakeup on */
+	accel_dev = &st->indio_accel->dev;
+	wakeup = st->apex.on && device_may_wakeup(accel_dev);
+	if (wakeup) {
+		/* keep accel on and setup irq for wakeup */
+		accel_conf = st->conf.accel.mode;
+		enable_irq_wake(st->irq);
+		disable_irq(st->irq);

Could this cause a deadlock between the driver mutex and the interrupt handler?

inv_icm42607_suspend() takes st->lock via guard(mutex) and then calls disable_irq().

Since disable_irq() waits for any currently executing interrupt handlers to finish, and the threaded handler inv_icm42607_irq_handler() also attempts to acquire st->lock, will this deadlock if the handler is running during suspend?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
#22 lore raw

PATCH v5 10/11 iio: imu: inv_icm42607: Add Gyroscope to icm42607

In reply to Chris Morgan

From: 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   |   9 +
 .../imu/inv_icm42607/inv_icm42607_buffer.c    |  24 +-
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  56 +-
 .../iio/imu/inv_icm42607/inv_icm42607_gyro.c  | 509 ++++++++++++++++++
 5 files changed, 596 insertions(+), 3 deletions(-)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
index e908d77c4219..fc66e580fe99 100644
@@ -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_buffer.o
 inv-icm42607-y += inv_icm42607_temp.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +9 -0
index ab79a056f28c..2c4376191148 100644
@@ -156,6 +156,7 @@ struct inv_icm42607_state {
 	struct iio_dev *indio_gyro;
 	struct iio_dev *indio_accel;
 	struct {
+		s64 gyro;
 		s64 accel;
 	} timestamp;
 	struct inv_icm42607_apex apex;
@@ -408,6 +409,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);
 
@@ -417,6 +422,10 @@ int inv_icm42607_disable_wom(struct inv_icm42607_state *st);
 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);
+
+int inv_icm42607_gyro_parse_fifo(struct iio_dev *indio_dev);
+
 struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st);
 
 int inv_icm42607_accel_parse_fifo(struct iio_dev *indio_dev);
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +23 -1
index eb629b86446b..33abc8a0a990 100644
@@ -373,7 +373,9 @@ static int inv_icm42607_buffer_postdisable(struct iio_dev *indio_dev)
 		goto out_unlock;
 
 	conf.mode = INV_ICM42607_SENSOR_MODE_OFF;
-	if (sensor != INV_ICM42607_SENSOR_GYRO)
+	if (sensor == INV_ICM42607_SENSOR_GYRO)
+		ret = inv_icm42607_set_gyro_conf(st, &conf, &sleep_sensor);
+	else
 		ret = inv_icm42607_set_accel_conf(st, &conf, &sleep_sensor);
 	if (ret)
 		goto out_unlock;
@@ -462,6 +464,7 @@ int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,
 
 int inv_icm42607_buffer_fifo_parse(struct inv_icm42607_state *st)
 {
+	struct inv_icm42607_sensor_state *gyro_st = iio_priv(st->indio_gyro);
 	struct inv_icm42607_sensor_state *accel_st = iio_priv(st->indio_accel);
 	struct inv_sensors_timestamp *ts;
 	int ret;
@@ -469,6 +472,16 @@ int inv_icm42607_buffer_fifo_parse(struct inv_icm42607_state *st)
 	if (st->fifo.nb.total == 0)
 		return 0;
 
+	/* handle gyroscope timestamp and FIFO data parsing */
+	if (st->fifo.nb.gyro > 0) {
+		ts = &gyro_st->ts;
+		inv_sensors_timestamp_interrupt(ts, st->fifo.watermark.eff_gyro,
+						st->timestamp.gyro);
+		ret = inv_icm42607_gyro_parse_fifo(st->indio_gyro);
+		if (ret)
+			return ret;
+	}
+
 	/* handle accelerometer timestamp and FIFO data parsing */
 	if (st->fifo.nb.accel > 0) {
 		ts = &accel_st->ts;
@@ -485,6 +498,7 @@ int inv_icm42607_buffer_fifo_parse(struct inv_icm42607_state *st)
 int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
 				     unsigned int count)
 {
+	struct inv_icm42607_sensor_state *gyro_st = iio_priv(st->indio_gyro);
 	struct inv_icm42607_sensor_state *accel_st = iio_priv(st->indio_accel);
 	struct inv_sensors_timestamp *ts;
 	s64 gyro_ts, accel_ts;
@@ -500,6 +514,14 @@ int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
 	if (st->fifo.nb.total == 0)
 		return 0;
 
+	if (st->fifo.nb.gyro > 0) {
+		ts = &gyro_st->ts;
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
+		ret = inv_icm42607_gyro_parse_fifo(st->indio_gyro);
+		if (ret)
+			return ret;
+	}
+
 	if (st->fifo.nb.accel > 0) {
 		ts = &accel_st->ts;
 		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +54 -2

98 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +509 -0

509 lines not on this page · raw message

Signature · 1 line
2.43.0
#23 lore raw

Re: [PATCH V5 10/11] iio: imu: inv_icm42607: Add Gyroscope to icm42607

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [Critical] Missing PM runtime wake in `inv_icm42607_gyro_hwfifo_flush` triggers a heap buffer overflow.…

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Missing PM runtime wake in inv_icm42607_gyro_hwfifo_flush triggers a heap buffer overflow.
- [High] Missing hardware wait, ignored error code, and power state leak in inv_icm42607_gyro_read_sensor.
- [High] Software state desynchronization causes unintended hardware shutdown.
- [High] Out-of-bounds kernel memory read in inv_icm42607_gyro_parse_fifo.
- [High] Standalone temperature scanning is completely broken due to parser logic.
- [Medium] Type mismatch and invalid boundary units in inv_icm42607_gyro_hwfifo_flush.
- [Medium] Uninitialized padding in inv_icm42607_gyro_odr array bypasses validation and writes reserved hardware state.
- [Medium] Gyroscope timestamp initialized using the accelerometer's ODR.
- [Medium] Premature IIO device registration causes race condition during probe.
--

commit c1f5aefdb4a5a29bea5b6cae45743d275de06e35
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add Gyroscope to icm42607

Add gyroscope functions to the icm42607 driver.

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0

[ ... ]

Quoted 14 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +10 -0
@@ -215,6 +215,49 @@ int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
 					  st->conf.temp_en, sleep_ms);
 }
 
+int inv_icm42607_set_gyro_conf(struct inv_icm42607_state *st,
+			       struct inv_icm42607_sensor_conf *conf,
+			       unsigned int *sleep_ms)
+{
+	struct inv_icm42607_sensor_conf *oldconf = &st->conf.gyro;
+	unsigned int val;
+	int ret;
+
+	if (conf->mode < 0)
+		conf->mode = oldconf->mode;

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st, · line 259

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +13 -0
+	if (conf->filter != oldconf->filter) {
+		val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG1_FILTER_MASK,
+				 conf->filter);
+		ret = regmap_update_bits(st->map, INV_ICM42607_REG_GYRO_CONFIG1,
+					 INV_ICM42607_GYRO_CONFIG1_FILTER_MASK, val);
+		if (ret)
+			return ret;
+		oldconf->filter = conf->filter;
+	}
+
+	return inv_icm42607_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode,
+					  st->conf.temp_en, sleep_ms);
+}

Does this code need to assign conf->mode to oldconf->mode? By not updating the cached state, is it possible that subsequent operations will save an incorrect power state during suspend, causing an unintended hardware shutdown on resume?

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw · line 628

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +5 -0
@@ -575,6 +619,11 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
 	pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(dev);
 
+	/* Initialize IIO device for Gyro */
+	st->indio_gyro = inv_icm42607_gyro_init(st);
+	if (IS_ERR(st->indio_gyro))
+		return PTR_ERR(st->indio_gyro);
+
 	/* Initialize IIO device for Accel */
 	st->indio_accel = inv_icm42607_accel_init(st);

Since inv_icm42607_irq_init() is called later in inv_icm42607_core_probe(), could this expose the IIO device to userspace before the interrupt handler is ready, allowing a race condition if the buffer is enabled immediately?

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +0 -0

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +4 -0
+static int inv_icm42607_gyro_read_sensor(struct inv_icm42607_state *st,
+					 struct iio_chan_spec const *chan,
+					 s16 *val)
+{

[ ... ]

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +8 -0
+	/* enable gyro sensor */
+	conf.mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
+	ret = inv_icm42607_set_gyro_conf(st, &conf, NULL);
+	if (ret)
+		return ret;
+
+	/* read gyro register data */
+	data = &st->buffer[0];

drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 182

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10 -0
+	ret = regmap_bulk_read(st->map, reg, data, sizeof(*data));
+	if (ret)
+		return ret;
+
+	*val = be16_to_cpup(data);
+	if (*val == INV_ICM42607_DATA_INVALID)
+		ret = -EINVAL;
+
+	return 0;
+}

Are there a few potential issues in this function?

First, by passing NULL for sleep_ms to inv_icm42607_set_gyro_conf(), does this bypass the required sleep for the hardware to settle, resulting in a read of stale registers?

Second, when the data is invalid and ret is set to -EINVAL, the function unconditionally returns 0. Will this expose the invalid value to userspace instead of returning the error?

Finally, does the function need to restore the sensor power mode to an off state before returning? As written, does it leave the gyroscope permanently powered on after a single sysfs read?

[ ... ]

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +3 -0
+static const int inv_icm42607_gyro_odr[][2] = {
+	[INV_ICM42607_ODR_12_5HZ] = { 12, 500000 },
+	[INV_ICM42607_ODR_25HZ] = { 25, 0 },

[ ... ]

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +3 -0
+static int inv_icm42607_gyro_write_odr(struct iio_dev *indio_dev,
+				       int val, int val2)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 283

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +9 -0
+	for (idx = 0; idx < ARRAY_SIZE(inv_icm42607_gyro_odr); ++idx) {
+		if (val == inv_icm42607_gyro_odr[idx][0] &&
+		    val2 == inv_icm42607_gyro_odr[idx][1])
+			break;
+	}
+	if (idx >= ARRAY_SIZE(inv_icm42607_gyro_odr))
+		return -EINVAL;
+
+	conf.odr = idx;

Since the inv_icm42607_gyro_odr array uses designated initializers starting at index 5 (INV_ICM42607_ODR_1600HZ), are indices 0-4 implicitly initialized to {0, 0}? If userspace writes a frequency of 0, will this bypass validation and write a reserved value of 0 into the hardware register?

[ ... ]

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +5 -0
+static int inv_icm42607_gyro_hwfifo_flush(struct iio_dev *indio_dev,
+					  unsigned int count)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	int ret;

drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 406

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10 -0
+
+	if (count == 0)
+		return 0;
+
+	/* Make sure we don't try to count more than the buffer can hold. */
+	count = min(count, sizeof(st->fifo.data));
+
+	guard(mutex)(&st->lock);
+
+	ret = inv_icm42607_buffer_hwfifo_flush(st, count);

Should this function call PM_RUNTIME_ACQUIRE_AUTOSUSPEND() before accessing device registers via inv_icm42607_buffer_hwfifo_flush()? If called while suspended, could it read 0xFFFF from the hardware, which might lead to a buffer overflow downstream when max() is used with this value?

Additionally, does the min() check mix unsigned int and size_t, which might cause a compiler warning? Also, since count is a number of samples, does checking it against the byte capacity of the buffer (sizeof) permit requesting far more samples than the buffer can hold?

[ ... ]

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +2 -0
+struct iio_dev *inv_icm42607_gyro_init(struct inv_icm42607_state *st)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 447

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +8 -0
+	/*
+	 * clock period is 32kHz (31250ns)
+	 * jitter is +/- 2% (20 per mille)
+	 */
+	ts_chip.clock_period = 31250;
+	ts_chip.jitter = 20;
+	ts_chip.init_period = inv_icm42607_odr_to_period(st->conf.accel.odr);
+	inv_sensors_timestamp_init(&gyro_st->ts, &ts_chip);

Should this use st->conf.gyro.odr instead of st->conf.accel.odr? If the default output data rates differ, could this cause the software IIO timestamps for the gyroscope to start out skewed?

[ ... ]

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +2 -0
+int inv_icm42607_gyro_parse_fifo(struct iio_dev *indio_dev)
+{

[ ... ]

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +8 -0
+	/* parse all fifo packets */
+	for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {
+		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
+				&accel, &gyro, &temp, &timestamp, &odr);
+		/* quit if error or FIFO is empty */
+		if (size <= 0)
+			return size;
+

drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 500

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10 -0
+		/* skip packet if no gyro data or data is invalid */
+		if (gyro == NULL || !inv_icm42607_fifo_is_data_valid(gyro))
+			continue;
+
+		/* update odr */
+		if (odr & INV_ICM42607_SENSOR_GYRO)
+			inv_sensors_timestamp_apply_odr(ts, st->fifo.period,
+							st->fifo.nb.total, no);
+
+		memcpy(&buffer.gyro, gyro, sizeof(buffer.gyro));

Does the loop need a bounds check to ensure i + size <= st->fifo.count? Without it, if a partial packet is present, could this copy out-of-bounds kernel memory into the IIO buffer?

Also, if only the temperature channel is enabled, will gyro always be NULL? Since this loop continues when gyro is NULL, will temperature packets be silently discarded, leaving userspace readers without data?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
#24 lore raw

PATCH v5 11/11 arm64: dts: rockchip: Add icm42607p IMU for RG-DS

In reply to Chris Morgan

From: 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.

---
 .../dts/rockchip/rk3568-anbernic-rg-ds.dts    | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
arch/arm64/boot/dts/rockchip/rk3568-anbernic-rg-ds.dts +19 -1
index 8d906ab02c5f..875ca884deca 100644
@@ -871,7 +871,18 @@ aw87391_pa_r: audio-codec@5b {
 		sound-name-prefix = "Right Amp";
 	};
 
-	/* invensense,icm42607p at 0x68 */
+	icm42607p: imu@68 {
+		compatible = "invensense,icm42607p";
+		reg = <0x68>;
+		interrupt-names = "INT1";
+		interrupt-parent = <&gpio0>;
+		interrupts = <RK_PD6 IRQ_TYPE_EDGE_FALLING>;
+		mount-matrix = "-1", "0", "0",
+			       "0", "1", "0",
+			       "0", "0", "-1";
+		pinctrl-0 = <&accel_irq>;
+		pinctrl-names = "default";
+	};
 };
 
 &i2c3 {
@@ -932,6 +943,13 @@ &i2s1_8ch {
 };
 
 &pinctrl {
+	accel {
+		accel_irq: accel-irq {
+			rockchip,pins =
+				<0 RK_PD6 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+	};
+
 	gpio-keys {
 		vol_keys_l: vol-keys_l {
 			rockchip,pins =
Signature · 1 line
2.43.0