Skip to contentSkip to conversation

Add Invensense ICM42607

PATCH Superseded Chris Morgan 5 participants 20 messages Jun 4 – Jun 9

Subsystems (4)

v10 of 21 Superseded Latest is v21 →

lore
Share & tools
lore
#0 lore raw

v10 cover Add Invensense ICM42607

From: Chris Morgan <[email protected]>

Add support for the ICM42607 IMU. This sensor shares the same functionality but a different register layout with the existing ICM42600.

This driver should work with the ICM42607 and ICM42607P over both I2C and SPI, however only the ICM42607P over I2C could be tested.

Changes Since V1:
- Instead of creating a new driver, merged with the existing inv_icm42600 driver. This necessitated adding some code to the existing driver to permit using a different register layout for the same functionality.
- Split changes up a bit more to decrease the size of the individual patches. Note that patch 0004 is still pretty hefty; if I need to split further I may need to create some temporary stub functions.
- Used guard() and PM_RUNTIME_ACQUIRE_AUTOSUSPEND() on the new functions per Jonathan's recommendations.

Changes Since V2:
- Went back to using a new driver on advice from Invensense engineer.
- Further split changes up into smaller chunks of functionality. Note still that the largest patch is approximately 900 lines, and that while the driver compiles cleanly at each commit it is not able to drive the hardware until the commit that adds the Interrupt (as it also adds the Makefile).
- Change the error to a warning when the devicetree binding does not match the hardware ID.
- Dropped the ack on the devicetree bindings, as I am creating a new file (for a new driver) instead of modifying the existing one.

Changes Since V3:
- Numerous small fixes (too many to list here). Thank you to everyone who provided feedback.
- Split power management additions into an additional commit to break things up further.
- Consolidated devicetree documentation in existing invensense,icm42600.yaml file.
- Removed most of the FIELD_PREP from header file to c files to make code easier to read.
- Changed scale values to 2D arrays for Gyro and Accelerometer.
- Removed IIO_CHAN_INFO_CALIBBIAS attribute.

Changes Since V4:
- Additional numerous small fixes, thank you again for all the feedback.
- Dropped power control API and instead run device in low noise mode.
- Split devicetree bindings into two distinct changes.
- Reordered adding of enums and structs to main header file so that they are only brought in when needed.
- Stopped using enum for driver data and instead am using pointer to device specific driver data.

Changes Since V5:
- Corrected use of "dev_warn_probe" to just "dev_warn".
- Fixed some return scenarios which would unconditionally return 0 when an error was present.
- Corrected use of max() to min() for bounds checking. max() was incorrect.
- Fixed using "st->conf.accel.odr" in the gyroscope function. It should have been "st->conf.gyro.odr" which it now is.
- Additional small fixes suggested by "sashiko.dev".
- Added a regmap cache. I used the datasheet to try and determine which registers might change without explicit writes.

Changes Since V6:
- Corrected additional errors identified by sashiko.dev, mostly fixing potential deadlocks, missing calls for pm runtime, and potential overflow issues.

Changes Since V7:
- Dropped Wake on Movement patches, since some of the functionality was only available for a device on which I cannot test.
- Dropped support for SPI 3-Wire mode, since it complicated the bus setup (and I lack the hardware to test such features anyway).
- Fixed a few additional bugs identified by sashiko.dev bot.

Changes Since V8:
- Added back IRQ dropped accidentally when dropping wake on movement patches.
- Dropped "Reviewed-By" tag on patch 2 because of substantial changes made to devicetree binding documentation.
- Additional small fixes as suggested.

Changes Since V9:
- Removed interrupts (and buffers) from the driver. I previously was unable to detect deadlocks because it turns out my IRQ was not even wired correctly in my device.
- Updated devicetree binding commits to make interrupts optional for users of the icm42607 driver.

Chris Morgan (9):
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 Temp Support in icm42607
iio: imu: inv_icm42607: Add Accelerometer for icm42607
iio: imu: inv_icm42607: Add Gyroscope to icm42607
arm64: dts: rockchip: Add icm42607p IMU for RG-DS

 .../bindings/iio/imu/invensense,icm42600.yaml |  20 +-
 .../dts/rockchip/rk3568-anbernic-rg-ds.dts    |   8 +-
 drivers/iio/imu/Kconfig                       |   1 +
 drivers/iio/imu/Makefile                      |   1 +
 drivers/iio/imu/inv_icm42607/Kconfig          |  30 +
 drivers/iio/imu/inv_icm42607/Makefile         |  13 +
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   | 421 ++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 465 +++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 539 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_gyro.c  | 437 ++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |  95 +++
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   | 107 ++++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.c  |  82 +++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.h  |  33 ++
 14 files changed, 2250 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iio/imu/inv_icm42607/Kconfig
 create mode 100644 drivers/iio/imu/inv_icm42607/Makefile
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h
Signature · 1 line
2.43.0
#1 lore raw

PATCH v10 1/9 dt-bindings: iio: imu: icm42600: Add mount-matrix to icm42600

From: Chris Morgan <[email protected]>

Add mount-matrix attribute to schema. This attribute has been supported since the first revision of this driver, but was not documented.

---
 .../devicetree/bindings/iio/imu/invensense,icm42600.yaml        | 2 ++
 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml +2 -0
index 119e28a833fd3..9b2af104f186d 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 v10 2/9 dt-bindings: iio: imu: icm42600: Add icm42607

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add the ICM42607 and ICM42607P inertial measurement unit.

This device is functionally very similar to the icm42600 series with a very different register layout. The driver does not require an interrupt for these specific chip revisions.

---
 .../bindings/iio/imu/invensense,icm42600.yaml  | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml +17 -1
index 9b2af104f186d..81b6e85decd52 100644
@@ -30,6 +30,8 @@ properties:
       - invensense,icm42600
       - invensense,icm42602
       - invensense,icm42605
+      - invensense,icm42607
+      - invensense,icm42607p
       - invensense,icm42622
       - invensense,icm42631
       - invensense,icm42686
@@ -67,10 +69,24 @@ properties:
 required:
   - compatible
   - reg
-  - interrupts
 
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - invensense,icm42600
+              - invensense,icm42602
+              - invensense,icm42605
+              - invensense,icm42622
+              - invensense,icm42631
+              - invensense,icm42686
+              - invensense,icm42688
+    then:
+      required:
+        - interrupts
 
 unevaluatedProperties: false
 
Signature · 1 line
2.43.0
Reviewed-by on v10 2/9
#3 lore raw

Re: [PATCH V10 2/9] dt-bindings: iio: imu: icm42600: Add icm42607

Quoted 4 lines · Chris Morgan

From: Chris Morgan <[email protected]>

Add the ICM42607 and ICM42607P inertial measurement unit.

Quoted 8 lines

This device is functionally very similar to the icm42600 series with a very different register layout. The driver does not require an interrupt for these specific chip revisions.

---
 .../bindings/iio/imu/invensense,icm42600.yaml  | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Best regards,
Krzysztof

#4 lore raw

PATCH v10 3/9 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   | 334 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 200 +++++++++++
 2 files changed, 534 insertions(+)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +334 -0
new file mode 100644
index 0000000000000..1fa9e4e11b2ee
@@ -0,0 +1,334 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2026 InvenSense, Inc.
+ */
+
+#ifndef INV_ICM42607_H_
+#define INV_ICM42607_H_
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+enum inv_icm42607_sensor_mode {
+	INV_ICM42607_SENSOR_MODE_OFF,
+	INV_ICM42607_SENSOR_MODE_STANDBY,
+	INV_ICM42607_SENSOR_MODE_LOW_POWER,
+	INV_ICM42607_SENSOR_MODE_LOW_NOISE,
+	INV_ICM42607_SENSOR_MODE_NB
+};
+
+/* gyroscope fullscale values */
+enum inv_icm42607_gyro_fs {
+	INV_ICM42607_GYRO_FS_2000DPS,
+	INV_ICM42607_GYRO_FS_1000DPS,
+	INV_ICM42607_GYRO_FS_500DPS,
+	INV_ICM42607_GYRO_FS_250DPS,
+	INV_ICM42607_GYRO_FS_NB
+};
+
+/* accelerometer fullscale values */
+enum inv_icm42607_accel_fs {
+	INV_ICM42607_ACCEL_FS_16G,
+	INV_ICM42607_ACCEL_FS_8G,
+	INV_ICM42607_ACCEL_FS_4G,
+	INV_ICM42607_ACCEL_FS_2G,
+	INV_ICM42607_ACCEL_FS_NB
+};
+
+/* ODR values */
+enum inv_icm42607_odr {
+	INV_ICM42607_ODR_1600HZ = 5,
+	INV_ICM42607_ODR_800HZ,
+	INV_ICM42607_ODR_400HZ,
+	INV_ICM42607_ODR_200HZ,
+	INV_ICM42607_ODR_100HZ,
+	INV_ICM42607_ODR_50HZ,
+	INV_ICM42607_ODR_25HZ,
+	INV_ICM42607_ODR_12_5HZ,
+	INV_ICM42607_ODR_6_25HZ_LP,
+	INV_ICM42607_ODR_3_125HZ_LP,
+	INV_ICM42607_ODR_1_5625HZ_LP,
+	INV_ICM42607_ODR_NB
+};
+
+enum inv_icm42607_filter_bw {
+	/* Low-Noise mode sensor data filter (bandwidth) */
+	INV_ICM42607_FILTER_BYPASS,
+	INV_ICM42607_FILTER_BW_180HZ,
+	INV_ICM42607_FILTER_BW_121HZ,
+	INV_ICM42607_FILTER_BW_73HZ,
+	INV_ICM42607_FILTER_BW_53HZ,
+	INV_ICM42607_FILTER_BW_34HZ,
+	INV_ICM42607_FILTER_BW_25HZ,
+	INV_ICM42607_FILTER_BW_16HZ,
+	INV_ICM42607_FILTER_BW_NB
+};
+
+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.
+ *  @orientation:	sensor chip orientation relative to main hardware.
+ *  @conf:		chip sensors configurations.
+ */
+struct inv_icm42607_state {
+	struct mutex lock;
+	const struct inv_icm42607_hw *hw;
+	struct regmap *map;
+	struct regulator *vddio_supply;
+	struct iio_mount_matrix orientation;
+	struct inv_icm42607_conf conf;
+};
+
+/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
+
+/* Register Map for User Bank 0 */
+#define INV_ICM42607_REG_MCLK_RDY			0x00
+
+#define INV_ICM42607_REG_DEVICE_CONFIG			0x01
+#define INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE		BIT(2)
+#define INV_ICM42607_DEVICE_CONFIG_SPI_MODE		BIT(0)
+
+#define INV_ICM42607_REG_SIGNAL_PATH_RESET		0x02
+#define INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET	BIT(4)
+#define INV_ICM42607_SIGNAL_PATH_RESET_FIFO_FLUSH	BIT(2)
+
+#define INV_ICM42607_REG_DRIVE_CONFIG1			0x03
+#define INV_ICM42607_DRIVE_CONFIG1_I3C_DDR_MASK		GENMASK(5, 3)
+#define INV_ICM42607_DRIVE_CONFIG1_I3C_SDR_MASK		GENMASK(2, 0)
+
+#define INV_ICM42607_REG_DRIVE_CONFIG2			0x04
+#define INV_ICM42607_DRIVE_CONFIG2_I2C_MASK		GENMASK(5, 3)
+#define INV_ICM42607_DRIVE_CONFIG2_ALL_MASK		GENMASK(2, 0)
+
+#define INV_ICM42607_REG_DRIVE_CONFIG3			0x05
+#define INV_ICM42607_DRIVE_CONFIG3_SPI_MASK		GENMASK(2, 0)
+
+#define INV_ICM42607_REG_INT_CONFIG			0x06
+#define INV_ICM42607_INT_CONFIG_INT2_LATCHED		BIT(5)
+#define INV_ICM42607_INT_CONFIG_INT2_PUSH_PULL		BIT(4)
+#define INV_ICM42607_INT_CONFIG_INT2_ACTIVE_HIGH	BIT(3)
+#define INV_ICM42607_INT_CONFIG_INT2_ACTIVE_LOW		0x00
+#define INV_ICM42607_INT_CONFIG_INT1_LATCHED		BIT(2)
+#define INV_ICM42607_INT_CONFIG_INT1_PUSH_PULL		BIT(1)
+#define INV_ICM42607_INT_CONFIG_INT1_ACTIVE_HIGH	BIT(0)
+#define INV_ICM42607_INT_CONFIG_INT1_ACTIVE_LOW		0x00
+
+/* all sensor data are 16 bits (2 registers wide) in big-endian */
+#define INV_ICM42607_REG_TEMP_DATA1			0x09
+#define INV_ICM42607_REG_TEMP_DATA0			0x0A
+#define INV_ICM42607_REG_ACCEL_DATA_X1			0x0B
+#define INV_ICM42607_REG_ACCEL_DATA_X0			0x0C
+#define INV_ICM42607_REG_ACCEL_DATA_Y1			0x0D
+#define INV_ICM42607_REG_ACCEL_DATA_Y0			0x0E
+#define INV_ICM42607_REG_ACCEL_DATA_Z1			0x0F
+#define INV_ICM42607_REG_ACCEL_DATA_Z0			0x10
+#define INV_ICM42607_REG_GYRO_DATA_X1			0x11
+#define INV_ICM42607_REG_GYRO_DATA_X0			0x12
+#define INV_ICM42607_REG_GYRO_DATA_Y1			0x13
+#define INV_ICM42607_REG_GYRO_DATA_Y0			0x14
+#define INV_ICM42607_REG_GYRO_DATA_Z1			0x15
+#define INV_ICM42607_REG_GYRO_DATA_Z0			0x16
+#define INV_ICM42607_DATA_INVALID			-32768
+
+#define INV_ICM42607_REG_TMST_FSYNCH			0x17
+#define INV_ICM42607_REG_TMST_FSYNCL			0x18
+
+/* APEX Data Registers */
+#define INV_ICM42607_REG_APEX_DATA0			0x31
+#define INV_ICM42607_REG_APEX_DATA1			0x32
+#define INV_ICM42607_REG_APEX_DATA2			0x33
+#define INV_ICM42607_REG_APEX_DATA3			0x34
+#define INV_ICM42607_REG_APEX_DATA4			0x1D
+#define INV_ICM42607_REG_APEX_DATA5			0x1E
+
+#define INV_ICM42607_REG_PWR_MGMT0			0x1F
+#define INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL		BIT(7)
+#define INV_ICM42607_PWR_MGMT0_IDLE			BIT(4)
+#define INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK		GENMASK(3, 2)
+#define INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK		GENMASK(1, 0)
+
+#define INV_ICM42607_REG_GYRO_CONFIG0			0x20
+#define INV_ICM42607_GYRO_CONFIG0_FS_SEL_MASK		GENMASK(6, 5)
+#define INV_ICM42607_GYRO_CONFIG0_ODR_MASK		GENMASK(3, 0)
+
+#define INV_ICM42607_REG_ACCEL_CONFIG0			0x21
+#define INV_ICM42607_ACCEL_CONFIG0_FS_SEL_MASK		GENMASK(6, 5)
+#define INV_ICM42607_ACCEL_CONFIG0_ODR_MASK		GENMASK(3, 0)
+
+#define INV_ICM42607_REG_TEMP_CONFIG0			0x22
+#define INV_ICM42607_TEMP_CONFIG0_FILTER_MASK		GENMASK(6, 4)
+
+#define INV_ICM42607_REG_GYRO_CONFIG1			0x23
+#define INV_ICM42607_GYRO_CONFIG1_FILTER_MASK		GENMASK(2, 0)
+
+#define INV_ICM42607_REG_ACCEL_CONFIG1			0x24
+#define INV_ICM42607_ACCEL_CONFIG1_AVG_MASK		GENMASK(6, 4)
+#define INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK		GENMASK(2, 0)
+
+#define INV_ICM42607_REG_APEX_CONFIG0			0x25
+#define INV_ICM42607_APEX_CONFIG0_DMP_POWER_SAVE_EN	BIT(3)
+#define INV_ICM42607_APEX_CONFIG0_DMP_INIT_EN		BIT(2)
+#define INV_ICM42607_APEX_CONFIG0_DMP_MEM_RESET_EN	BIT(0)
+
+#define INV_ICM42607_REG_APEX_CONFIG1			0x26
+#define INV_ICM42607_APEX_CONFIG1_SMD_ENABLE		BIT(6)
+#define INV_ICM42607_APEX_CONFIG1_FF_ENABLE		BIT(5)
+#define INV_ICM42607_APEX_CONFIG1_TILT_ENABLE		BIT(4)
+#define INV_ICM42607_APEX_CONFIG1_PED_ENABLE		BIT(3)
+#define INV_ICM42607_APEX_CONFIG1_DMP_ODR_MASK		GENMASK(1, 0)
+
+#define INV_ICM42607_REG_WOM_CONFIG			0x27
+#define INV_ICM42607_WOM_CONFIG_INT_DUR_MASK		GENMASK(4, 3)
+#define INV_ICM42607_WOM_CONFIG_INT_MODE		BIT(2)
+#define INV_ICM42607_WOM_CONFIG_MODE			BIT(1)
+#define INV_ICM42607_WOM_CONFIG_EN			BIT(0)
+
+#define INV_ICM42607_REG_FIFO_CONFIG1			0x28
+#define INV_ICM42607_FIFO_CONFIG1_MODE			BIT(1)
+#define INV_ICM42607_FIFO_CONFIG1_BYPASS		BIT(0)
+
+#define INV_ICM42607_REG_FIFO_CONFIG2			0x29
+#define INV_ICM42607_REG_FIFO_CONFIG3			0x2A
+#define INV_ICM42607_FIFO_WATERMARK_VAL(_wm)		\
+		cpu_to_le16((_wm) & GENMASK(11, 0))
+/* FIFO is 2048 bytes, let 12 samples for reading latency */
+#define INV_ICM42607_FIFO_WATERMARK_MAX			(2048 - 12 * 16)
+#define INV_ICM42607_FIFO_1SENSOR_PACKET_SIZE		8
+#define INV_ICM42607_FIFO_2SENSORS_PACKET_SIZE		16
+
+#define INV_ICM42607_REG_INT_SOURCE0			0x2B
+#define INV_ICM42607_INT_SOURCE0_ST_INT1_EN		BIT(7)
+#define INV_ICM42607_INT_SOURCE0_FSYNC_INT1_EN		BIT(6)
+#define INV_ICM42607_INT_SOURCE0_PLL_RDY_INT1_EN	BIT(5)
+#define INV_ICM42607_INT_SOURCE0_RESET_DONE_INT1_EN	BIT(4)
+#define INV_ICM42607_INT_SOURCE0_DRDY_INT1_EN		BIT(3)
+#define INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN	BIT(2)
+#define INV_ICM42607_INT_SOURCE0_FIFO_FULL_INT1_EN	BIT(1)
+#define INV_ICM42607_INT_SOURCE0_AGC_RDY_INT1_EN	BIT(0)
+
+#define INV_ICM42607_REG_INT_SOURCE1			0x2C
+#define INV_ICM42607_INT_SOURCE1_I3C_ERROR_INT1_EN	BIT(6)
+#define INV_ICM42607_INT_SOURCE1_SMD_INT1_EN		BIT(3)
+#define INV_ICM42607_INT_SOURCE1_WOM_INT1_EN		GENMASK(2, 0)
+
+#define INV_ICM42607_REG_INT_SOURCE3			0x2D
+#define INV_ICM42607_INT_SOURCE3_ST_INT2_EN		BIT(7)
+#define INV_ICM42607_INT_SOURCE3_FSYNC_INT2_EN		BIT(6)
+#define INV_ICM42607_INT_SOURCE3_PLL_RDY_INT2_EN	BIT(5)
+#define INV_ICM42607_INT_SOURCE3_RESET_DONE_INT2_EN	BIT(4)
+#define INV_ICM42607_INT_SOURCE3_DRDY_INT2_EN		BIT(3)
+#define INV_ICM42607_INT_SOURCE3_FIFO_THS_INT2_EN	BIT(2)
+#define INV_ICM42607_INT_SOURCE3_FIFO_FULL_INT2_EN	BIT(1)
+#define INV_ICM42607_INT_SOURCE3_AGC_RDY_INT2_EN	BIT(0)
+
+#define INV_ICM42607_REG_INT_SOURCE4			0x2E
+#define INV_ICM42607_INT_SOURCE4_I3C_ERROR_INT2_EN	BIT(6)
+#define INV_ICM42607_INT_SOURCE4_SMD_INT2_EN		BIT(3)
+#define INV_ICM42607_INT_SOURCE4_WOM_Z_INT2_EN		BIT(2)
+#define INV_ICM42607_INT_SOURCE4_WOM_Y_INT2_EN		BIT(1)
+#define INV_ICM42607_INT_SOURCE4_WOM_X_INT2_EN		BIT(0)
+
+#define INV_ICM42607_REG_FIFO_LOST_PKT0			0x2F
+#define INV_ICM42607_REG_FIFO_LOST_PKT1			0x30
+
+#define INV_ICM42607_REG_INTF_CONFIG0			0x35
+#define INV_ICM42607_INTF_CONFIG0_FIFO_COUNT_FORMAT	BIT(6)
+#define INV_ICM42607_INTF_CONFIG0_FIFO_COUNT_ENDIAN	BIT(5)
+#define INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN	BIT(4)
+#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK	GENMASK(1, 0)
+#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS	\
+	FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 2)
+#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS	\
+	FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 3)
+
+#define INV_ICM42607_REG_INTF_CONFIG1			0x36
+#define INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN		BIT(3)
+#define INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN		BIT(2)
+#define INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK		GENMASK(1, 0)
+#define INV_ICM42607_INTF_CONFIG1_CLKSEL_INT		0
+#define INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL		1
+#define INV_ICM42607_INTF_CONFIG1_CLKSEL_OFF		2
+
+#define INV_ICM42607_REG_INT_STATUS_DRDY		0x39
+#define INV_ICM42607_INT_STATUS_DRDY_DATA_RDY		BIT(0)
+
+#define INV_ICM42607_REG_INT_STATUS			0x3A
+#define INV_ICM42607_INT_STATUS_ST			BIT(7)
+#define INV_ICM42607_INT_STATUS_FSYNC			BIT(6)
+#define INV_ICM42607_INT_STATUS_PLL_RDY			BIT(5)
+#define INV_ICM42607_INT_STATUS_RESET_DONE		BIT(4)
+#define INV_ICM42607_INT_STATUS_FIFO_THS		BIT(2)
+#define INV_ICM42607_INT_STATUS_FIFO_FULL		BIT(1)
+#define INV_ICM42607_INT_STATUS_AGC_RDY			BIT(0)
+
+#define INV_ICM42607_REG_INT_STATUS2			0x3B
+#define INV_ICM42607_INT_STATUS2_SMD			BIT(3)
+#define INV_ICM42607_INT_STATUS2_WOM_INT		GENMASK(2, 0)
+
+#define INV_ICM42607_REG_INT_STATUS3			0x3C
+#define INV_ICM42607_INT_STATUS3_STEP_DET		BIT(5)
+#define INV_ICM42607_INT_STATUS3_STEP_CNT_OVF		BIT(4)
+#define INV_ICM42607_INT_STATUS3_TILT_DET		BIT(3)
+#define INV_ICM42607_INT_STATUS3_FF_DET			BIT(2)
+
+/*
+ * FIFO access registers
+ * FIFO count is 16 bits (2 registers) big-endian
+ * FIFO data is a continuous read register to read FIFO content
+ */
+#define INV_ICM42607_REG_FIFO_COUNTH			0x3D
+#define INV_ICM42607_REG_FIFO_COUNTL			0x3E
+#define INV_ICM42607_REG_FIFO_DATA			0x3F
+
+#define INV_ICM42607_REG_ACCEL_WOM_X_THR		0x4b
+#define INV_ICM42607_REG_ACCEL_WOM_Y_THR		0x4c
+#define INV_ICM42607_REG_ACCEL_WOM_Z_THR		0x4d
+
+#define INV_ICM42607_REG_WHOAMI				0x75
+#define INV_ICM42607P_WHOAMI				0x60
+#define INV_ICM42607_WHOAMI				0x67
+
+/* Timings as listed in section 3 of datasheet */
+#define INV_ICM42607_POWER_UP_TIME_US			100000
+#define INV_ICM42607_RESET_TIME_MS			1
+#define INV_ICM42607_ACCEL_STARTUP_TIME_MS		10
+#define INV_ICM42607_GYRO_STARTUP_TIME_MS		30
+#define INV_ICM42607_TEMP_STARTUP_TIME_MS		77
+
+typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
+
+extern const struct regmap_config inv_icm42607_regmap_config;
+extern const struct inv_icm42607_hw inv_icm42607_hw_data;
+extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
+
+int inv_icm42607_core_probe(struct regmap *regmap,
+			    const struct inv_icm42607_hw *hw,
+			    inv_icm42607_bus_setup bus_setup);
+
+#endif
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +200 -0

200 lines not on this page · raw message

Signature · 1 line
2.43.0
#5 lore raw

Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The driver state structure `st` is never saved to the device's drvdata, leading to a guaranteed…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver state structure st is never saved to the device's drvdata, leading to a guaranteed NULL pointer dereference in PM callbacks.
- [Low] Redundant error code argument in dev_err_probe() format string.
--

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

iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

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.

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

[ ... ]

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +13 -0
+int inv_icm42607_core_probe(struct regmap *regmap,
+			    const struct inv_icm42607_hw *hw,
+			    inv_icm42607_bus_setup bus_setup)
+{
+	struct device *dev = regmap_get_device(regmap);
+	struct inv_icm42607_state *st;
+	int ret;
+
+	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	ret = devm_mutex_init(dev, &st->lock);

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10 -0
+	if (ret)
+		return ret;
+
+	st->hw = hw;
+	st->map = regmap;
+
+	ret = iio_read_mount_matrix(dev, &st->orientation);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to retrieve mounting matrix %d\n", ret);

[Severity: Low]
This isn't a bug, but is it necessary to include the %d format specifier and ret argument in this dev_err_probe() call?

The dev_err_probe() API natively prefixes the log message with the provided error code, so passing it as an explicit format argument might result in the error code being printed twice.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5 -0
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get vdd regulator\n");

[ ... ]

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

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +7 -0
+	/* Setup chip registers (includes WHOAMI check, reset check, bus setup) */
+	ret = inv_icm42607_setup(st, bus_setup);
+	if (ret)
+		return ret;
+
+	return 0;
+}

[Severity: High]
Should this code save the driver state structure to the device's drvdata?

Because runtime PM is enabled at the end of the driver series, inv_icm42607_runtime_suspend() will automatically trigger after the autosuspend delay.

It calls dev_get_drvdata(dev) to retrieve the state structure, which will return NULL since it was never set here.

Could this lead to a NULL pointer dereference shortly after initialization when the PM callbacks try to access the state structure?

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

Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

In reply to Chris Morgan
Quoted 3 lines · Chris Morgan

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 line 10

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5 -0
+#ifndef INV_ICM42607_H_
+#define INV_ICM42607_H_
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>

I haven't found users for these two.

+ bits.h // BIT() / GENMASK()

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 14

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +4 -0
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
 
 
+#include <linux/regulator/consumer.h>

No users for this one.

+ types.h // for bool

...

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 20

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5 -0
+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,

Are those enums map 1:1 to HW bits or bitfields? If so, assign explicitly each of them.

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 21

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +1 -0
+	INV_ICM42607_SENSOR_MODE_NB

Is this a terminator like NUMBER_OF ?

Quoted 1 line

+};

...

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 44

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3 -0
+/* ODR values */
+enum inv_icm42607_odr {
+	INV_ICM42607_ODR_1600HZ = 5,

See above. This one is problematic. No one should rely on Linux/C enums when it's about HW bits. All HW related stuff has to be explicit.

Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +12 -0
+	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
+};

...

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 75

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5 -0
+struct inv_icm42607_sensor_conf {
+	int mode;
+	int fs;
+	int odr;
+	int filter;

All of them are supposed to be signed? Why?

Quoted 1 line

+};

...

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 85

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +2 -0
+struct inv_icm42607_hw {
+	uint8_t whoami;

What's wrong with u8?

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3 -0
+	const char *name;
+	const struct inv_icm42607_conf *conf;
+};

...

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

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9 -0
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>

IWYU, please.

...

Quoted 6 lines

+/**

+ *  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.

If you do kernel-doc, validate it. Return section is missing here.

Quoted 1 line

+ */

...

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

Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +12 -0
+{
+	const struct device *dev = regmap_get_device(st->map);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
+	if (ret)
+		return ret;
+
+	if (val != st->hw->whoami)
+		dev_warn(dev, "Unknown whoami %#02x expected %#02x (%s)\n",
+			 val, st->hw->whoami, st->hw->name);

dev_warn_probe() ?

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

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;
 
+	fsleep(INV_ICM42607_RESET_TIME_MS * 1000);

USEC_PER_MSEC (needs time.h)

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

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +4 -0
+	ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
+				       val, val & INV_ICM42607_INT_STATUS_RESET_DONE,
+				       INV_ICM42607_RESET_TIME_MS * 100,
+				       INV_ICM42607_RESET_TIME_MS * 10000);

These are weird, as in the first case it's actually 1/10th of _RESET_TIME_MS. Perhaps you need to reconsider what you use as that constant. Personally I prefer to see just plain values with the multipliers (to convert to µs).

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

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +11 -0
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "reset error, reset done bit not set\n");
+
+	/* Sync the regcache again after a reset. */
+	regcache_mark_dirty(st->map);
+	ret = regcache_sync(st->map);
+	if (ret)
+		return ret;
+
+	ret = bus_setup(st);

Hmm... This is bad name with potential of name collision in the future (in case driver bus code wants to have the same name for the function).

Quoted 16 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +16 -0
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
+			      INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG1,
+				 INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK,
+				 INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL);
+	if (ret)
+		return ret;
+
+	return inv_icm42607_set_conf(st, st->hw->conf);
+}

...

Quoted 14 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +14 -0
+int inv_icm42607_core_probe(struct regmap *regmap,
+			    const struct inv_icm42607_hw *hw,
+			    inv_icm42607_bus_setup bus_setup)
+{
+	struct device *dev = regmap_get_device(regmap);
+	struct inv_icm42607_state *st;
+	int ret;
+
+	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9 -0
+		return ret;
+
+	st->hw = hw;
+	st->map = regmap;
+
+	ret = iio_read_mount_matrix(dev, &st->orientation);
+	if (ret)
 
+		return dev_err_probe(dev, ret,
+				     "failed to retrieve mounting matrix %d\n", ret);

Remove duplicate ret printing.

Quoted 15 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +15 -0
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get vdd regulator\n");
+
+	st->vddio_supply = devm_regulator_get(dev, "vddio");
+	if (IS_ERR(st->vddio_supply))
+		return dev_err_probe(dev, PTR_ERR(st->vddio_supply),
+				     "Failed to get vddio regulator\n");
+
+	ret = inv_icm42607_enable_vddio_reg(st);
+	if (ret)
+		return ret;
+

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9 -0
+	ret = devm_add_action_or_reset(dev, inv_icm42607_disable_vddio_reg, st);
+	if (ret)
+		return ret;
 
+	/* Setup chip registers (includes WHOAMI check, reset check, bus setup) */
+	ret = inv_icm42607_setup(st, bus_setup);
+	if (ret)
+		return ret;
+
+	return 0;

Just

	return inv_icm42607_setup(st, bus_setup);

? Or is it going to be extended in the next changes?

Quoted 1 line

+}

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

Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

Quoted 16 lines · Andy Shevchenko
Quoted 3 lines · Chris Morgan

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 line 10

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5 -0
+#ifndef INV_ICM42607_H_
+#define INV_ICM42607_H_
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>

I haven't found users for these two.

Quoted 1 line

+ bits.h // BIT() / GENMASK()

I'll correct this.

Quoted 6 lines
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3 -0
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
Quoted 8 lines
Quoted 1 line

+#include <linux/regulator/consumer.h>

No users for this one.

+ types.h // for bool

...

And this.

Quoted 8 lines

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 20

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5 -0
+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,

Are those enums map 1:1 to HW bits or bitfields? If so, assign explicitly each of them.

I'll explicitly set every value in the enums to be clear.

Quoted 4 lines

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 21

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +1 -0
+	INV_ICM42607_SENSOR_MODE_NB

Is this a terminator like NUMBER_OF ?

This is a terminator, yes. I will not set these explicitly and keep commas off of them to keep them easily identifable as terminators.

Quoted 12 lines
Quoted 2 lines

+};

...

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 44

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3 -0
+/* ODR values */
+enum inv_icm42607_odr {
+	INV_ICM42607_ODR_1600HZ = 5,

See above. This one is problematic. No one should rely on Linux/C enums when it's about HW bits. All HW related stuff has to be explicit.

I will explicitly set the values here.

Quoted 15 lines
Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +12 -0
+	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
+};

...

Quoted 10 lines

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 75

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5 -0
+struct inv_icm42607_sensor_conf {
+	int mode;
+	int fs;
+	int odr;
+	int filter;

All of them are supposed to be signed? Why?

Quoted 1 line

+};

A later commit will use -1 for invalid values.

Quoted 6 lines

...

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 85

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +2 -0
+struct inv_icm42607_hw {
+	uint8_t whoami;

What's wrong with u8?

Nothing, I'll make the change.

Quoted 19 lines
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3 -0
+	const char *name;
+	const struct inv_icm42607_conf *conf;
+};

...

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

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9 -0
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>

IWYU, please.

Quoted 2 lines

...

This may sound like a dumb question, but what's the best way to invoke iwyu for a kernel? None of the readmes or man pages seem to be getting me anywhere...

Quoted 9 lines
Quoted 6 lines

+/**

+ *  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.

If you do kernel-doc, validate it. Return section is missing here.

I'll just nuke it, most of the stuff is fairly self explanatory anyway.

Quoted 29 lines
Quoted 1 line

+ */

...

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

Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +12 -0
+{
+	const struct device *dev = regmap_get_device(st->map);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
+	if (ret)
+		return ret;
+
+	if (val != st->hw->whoami)
+		dev_warn(dev, "Unknown whoami %#02x expected %#02x (%s)\n",
+			 val, st->hw->whoami, st->hw->name);

dev_warn_probe() ?

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

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;
 
+	fsleep(INV_ICM42607_RESET_TIME_MS * 1000);

USEC_PER_MSEC (needs time.h)

Quoted 9 lines

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

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +4 -0
+	ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
+				       val, val & INV_ICM42607_INT_STATUS_RESET_DONE,
+				       INV_ICM42607_RESET_TIME_MS * 100,
+				       INV_ICM42607_RESET_TIME_MS * 10000);

These are weird, as in the first case it's actually 1/10th of _RESET_TIME_MS. Perhaps you need to reconsider what you use as that constant. Personally I prefer to see just plain values with the multipliers (to convert to µs).

I honestly don't know what a good wait period between polling should be, so I guessed at 1/10th the timeout period. I will however change the timeout to us.

Quoted 6 lines
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +6 -0
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "reset error, reset done bit not set\n");
+
+	/* Sync the regcache again after a reset. */
+	regcache_mark_dirty(st->map);
Quoted 8 lines

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

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5 -0
+	ret = regcache_sync(st->map);
+	if (ret)
+		return ret;
+
+	ret = bus_setup(st);

Hmm... This is bad name with potential of name collision in the future (in case driver bus code wants to have the same name for the function).

Thanks, I'll rename the function.

Quoted 42 lines
Quoted 17 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +16 -0
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
+			      INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG1,
+				 INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK,
+				 INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL);
+	if (ret)
+		return ret;
+
+	return inv_icm42607_set_conf(st, st->hw->conf);
+}

...

Quoted 21 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +21 -0
+int inv_icm42607_core_probe(struct regmap *regmap,
+			    const struct inv_icm42607_hw *hw,
+			    inv_icm42607_bus_setup bus_setup)
+{
+	struct device *dev = regmap_get_device(regmap);
+	struct inv_icm42607_state *st;
+	int ret;
+
+	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	st->hw = hw;
+	st->map = regmap;
+
+	ret = iio_read_mount_matrix(dev, &st->orientation);
+	if (ret)
Quoted 5 lines

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

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +2 -0
+		return dev_err_probe(dev, ret,
+				     "failed to retrieve mounting matrix %d\n", ret);

Remove duplicate ret printing.

Understood.

Quoted 28 lines
Quoted 15 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +15 -0
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get vdd regulator\n");
+
+	st->vddio_supply = devm_regulator_get(dev, "vddio");
+	if (IS_ERR(st->vddio_supply))
+		return dev_err_probe(dev, PTR_ERR(st->vddio_supply),
+				     "Failed to get vddio regulator\n");
+
+	ret = inv_icm42607_enable_vddio_reg(st);
+	if (ret)
+		return ret;
+

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9 -0
+	ret = devm_add_action_or_reset(dev, inv_icm42607_disable_vddio_reg, st);
+	if (ret)
+		return ret;
 
+	/* Setup chip registers (includes WHOAMI check, reset check, bus setup) */
+	ret = inv_icm42607_setup(st, bus_setup);
+	if (ret)
+		return ret;
+
+	return 0;

Just

Quoted 5 lines
	return inv_icm42607_setup(st, bus_setup);

? Or is it going to be extended in the next changes?

Quoted 1 line

+}

It will get extended, so I'll keep this for now if that's okay.

Quoted 6 lines
Signature · 3 lines
-- 
With Best Regards,
Andy Shevchenko

Thank you for your feedback. And please let me know about the iwyu tool, that seems like it would be useful.

- Chris

#8 lore raw

Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

Quoted 2 lines · Chris Morgan
Quoted 1 line · Andy Shevchenko

On Thu, Jun 04, 2026 at 03:18:25PM -0500, Chris Morgan wrote:

...

Quoted 11 lines
Quoted 9 lines

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 75

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5 -0
+struct inv_icm42607_sensor_conf {
+	int mode;
+	int fs;
+	int odr;
+	int filter;

All of them are supposed to be signed? Why?

Quoted 1 line

+};

A later commit will use -1 for invalid values.

Okay, perhaps add a comment on top that negative values will mean invalid case?

...

Quoted 12 lines
Quoted 11 lines

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

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9 -0
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>

IWYU, please.

Quoted 3 lines

This may sound like a dumb question, but what's the best way to invoke iwyu for a kernel? None of the readmes or man pages seem to be getting me anywhere...

In this list we (I mean community related to IIO) discussed iwyu tool and its best configuration. Yep, it's barely good and far from ideal, but that's what we have currently. Otherwise git grep ... and cscope are your friends to answer this. Also doing review on regular basis makes this knowledge developed better.

...

Quoted 1 line

It will get extended, so I'll keep this for now if that's okay.

It's okay.

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

Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

Quoted 14 lines · Andy Shevchenko
Quoted 2 lines · Chris Morgan
Quoted 1 line · Andy Shevchenko

On Thu, Jun 04, 2026 at 03:18:25PM -0500, Chris Morgan wrote:

...

Quoted 7 lines
Quoted 7 lines

drivers/iio/imu/inv_icm42607/inv_icm42607.h line 75

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5 -0
+struct inv_icm42607_sensor_conf {
+	int mode;
+	int fs;
+	int odr;
+	int filter;

All of them are supposed to be signed? Why?

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line

+};

A later commit will use -1 for invalid values.

Okay, perhaps add a comment on top that negative values will mean invalid case?

Will do.

Quoted 19 lines

...

Quoted 12 lines
Quoted 11 lines

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

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9 -0
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>

IWYU, please.

Quoted 3 lines

This may sound like a dumb question, but what's the best way to invoke iwyu for a kernel? None of the readmes or man pages seem to be getting me anywhere...

Quoted 6 lines

In this list we (I mean community related to IIO) discussed iwyu tool and its best configuration. Yep, it's barely good and far from ideal, but that's what we have currently. Otherwise git grep ... and cscope are your friends to answer this. Also doing review on regular basis makes this knowledge developed better.

Since git grep (and elixir.bootlin.com) are the tools I know best, I'll try to sensibly pare down the includes where I can. I do see that the interrupt include isn't needed anymore so I can drop that one. If anything else jumps out let me know and I'll exclude it too. There's a few other things I'm going to pare out since sashiko.dev bot correctly pointed out some of my code is now dead since I removed buffers support (because I don't have an interrupt on this device).

Quoted 11 lines

...

Quoted 1 line

It will get extended, so I'll keep this for now if that's okay.

It's okay.

Signature · 2 lines
With Best Regards,
Andy Shevchenko

Thank you,
Chris

#10 lore raw

PATCH v10 4/9 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   |  16 +++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  57 ++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |  94 ++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   | 106 ++++++++++++++++++
 8 files changed, 315 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 7e0181c27bb69..8bab4616be208 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 13fb7846e9c98..3268dc2371aed 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 0000000000000..7ba64e6e8d806
@@ -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 0000000000000..be109102e2035
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o
+inv-icm42607-y += inv_icm42607_core.o
+
+obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o
+inv-icm42607-i2c-y += inv_icm42607_i2c.o
+
+obj-$(CONFIG_INV_ICM42607_SPI) += inv-icm42607-spi.o
+inv-icm42607-spi-y += inv_icm42607_spi.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +16 -0
index 1fa9e4e11b2ee..7928f81300ed8 100644
@@ -13,6 +13,22 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+/*
+ * Serial bus slew rates. Rates are expressed as range between the two
+ * values with the midpoint as the typical rate. For the final value of
+ * 2ns, 2ns is considered the max value with no expressed minimum or
+ * typical value.
+ */
+enum inv_icm42607_slew_rate {
+	INV_ICM42607_SLEW_RATE_20_60NS,
+	INV_ICM42607_SLEW_RATE_12_36NS,
+	INV_ICM42607_SLEW_RATE_6_19NS,
+	INV_ICM42607_SLEW_RATE_4_14NS,
+	INV_ICM42607_SLEW_RATE_2_8NS,
+	INV_ICM42607_SLEW_RATE_2NS,
+	INV_ICM42607_SLEW_RATE_NB
+};
+
 enum inv_icm42607_sensor_mode {
 	INV_ICM42607_SENSOR_MODE_OFF,
 	INV_ICM42607_SENSOR_MODE_STANDBY,
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +57 -0

63 lines not on this page · raw message

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

94 lines not on this page · raw message

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

106 lines not on this page · raw message

Signature · 1 line
2.43.0
#11 lore raw

PATCH v10 5/9 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   |  18 +++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 123 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |   1 +
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   |   1 +
 4 files changed, 143 insertions(+)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +18 -0
index 7928f81300ed8..72327381b6dbc 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>
 
@@ -103,22 +104,32 @@ struct inv_icm42607_hw {
 	const struct inv_icm42607_conf *conf;
 };
 
+struct inv_icm42607_suspended {
+	enum inv_icm42607_sensor_mode gyro;
+	enum inv_icm42607_sensor_mode accel;
+	bool temp;
+};
+
 /**
  *  struct inv_icm42607_state - driver state variables
  *  @lock:		lock for serializing multiple registers access.
  *  @hw:		Hardware specific data.
  *  @map:		regmap pointer.
  *  @vddio_supply:	I/O voltage regulator for the chip.
+ *  @vddio_en:		I/O voltage status for runtime PM.
  *  @orientation:	sensor chip orientation relative to main hardware.
  *  @conf:		chip sensors configurations.
+ *  @suspended:		suspended sensors configuration.
  */
 struct inv_icm42607_state {
 	struct mutex lock;
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;
 	struct regulator *vddio_supply;
+	bool vddio_en;
 	struct iio_mount_matrix orientation;
 	struct inv_icm42607_conf conf;
+	struct inv_icm42607_suspended suspended;
 };
 
 /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
@@ -337,11 +348,18 @@ struct inv_icm42607_state {
 #define INV_ICM42607_GYRO_STARTUP_TIME_MS		30
 #define INV_ICM42607_TEMP_STARTUP_TIME_MS		77
 
+/*
+ * Suspend delay assumed from other icm42600 series device, not
+ * documented in datasheet.
+ */
+#define INV_ICM42607_SUSPEND_DELAY_MS			2000
+
 typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
 
 extern const struct regmap_config inv_icm42607_regmap_config;
 extern const struct inv_icm42607_hw inv_icm42607_hw_data;
 extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
+extern const struct dev_pm_ops inv_icm42607_pm_ops;
 
 int inv_icm42607_core_probe(struct regmap *regmap,
 			    const struct inv_icm42607_hw *hw,
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +123 -0

164 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c +1 -0
index 22ac51108cd3b..cd08bb9514f2d 100644
@@ -82,6 +82,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 421d3ec1cded3..8a82e1610531b 100644
@@ -94,6 +94,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 V10 5/9] iio: imu: inv_icm42607: Add PM support for icm42607

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Missing `dev_set_drvdata()` causes a guaranteed kernel panic on PM transitions. - [Medium]…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing dev_set_drvdata() causes a guaranteed kernel panic on PM transitions.
- [Medium] Unnecessary start-up delays applied when disabling sensors during power management transitions. --

commit 095604e6b5564d3e2f59fa7bf159761bbf791c56
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add PM support for icm42607

Add power management support for the ICM42607 device driver.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
index 4448d63e95fd7..39620da92bd43 100644

[ ... ]

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +5 -0
@@ -72,6 +73,51 @@ 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)
+{

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c const struct inv_icm42607_hw inv_icm42607p_hw_data = { · line 111

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +9 -0
+	sleepval = 0;
+	if (temp && !oldtemp)
+		sleepval = max(sleepval, INV_ICM42607_TEMP_STARTUP_TIME_MS);
+
+	if (accel != oldaccel)
+		sleepval = max(sleepval, INV_ICM42607_ACCEL_STARTUP_TIME_MS);
+
+	if (gyro != oldgyro)
+		sleepval = max(sleepval, INV_ICM42607_GYRO_STARTUP_TIME_MS);

[Severity: Medium]
Does this unnecessarily delay suspend?

When inv_icm42607_runtime_suspend() turns the sensors off by passing INV_ICM42607_SENSOR_MODE_OFF, the state changes (accel != oldaccel and gyro != oldgyro).

Because of this, it seems the function will wait for the maximum startup time before suspending, rather than only sleeping when the sensors are actually turning on.

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +8 -0
+
+	if (sleep_ms)
+		*sleep_ms = sleepval;
+	else if (sleepval)
+		fsleep(sleepval * 1000);
+
+	return 0;
+}

[ ... ]

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

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +5 -0
+static int inv_icm42607_runtime_suspend(struct device *dev)
+{
+	struct inv_icm42607_state *st = dev_get_drvdata(dev);
+
+	guard(mutex)(&st->lock);

[Severity: Critical]
Will this cause a NULL pointer dereference?

The PM callbacks retrieve the driver state using dev_get_drvdata(), but it doesn't appear that dev_set_drvdata() is ever called during probe to associate the state with the device.

Since pm_runtime_use_autosuspend() is enabled with a 2000ms delay in probe, won't this reliably crash the kernel exactly two seconds after the device is probed?

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +10 -0
+
+	/* save sensors state */
+	st->suspended.gyro = st->conf.gyro.mode;
+	st->suspended.accel = st->conf.accel.mode;
+	st->suspended.temp = st->conf.temp_en;
+
+	return inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
+					  INV_ICM42607_SENSOR_MODE_OFF, false,
+					  NULL);
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
#13 lore raw

PATCH v10 6/9 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   |  5 ++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 18 ++++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.c  | 82 +++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.h  | 33 ++++++++
 5 files changed, 139 insertions(+)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h
drivers/iio/imu/inv_icm42607/Makefile +1 -0
index be109102e2035..c04953ed42ce9 100644
@@ -2,6 +2,7 @@
 
 obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o
 inv-icm42607-y += inv_icm42607_core.o
+inv-icm42607-y += inv_icm42607_temp.o
 
 obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o
 inv-icm42607-i2c-y += inv_icm42607_i2c.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +5 -0
index 72327381b6dbc..13a5b06e80142 100644
@@ -120,6 +120,7 @@ struct inv_icm42607_suspended {
  *  @orientation:	sensor chip orientation relative to main hardware.
  *  @conf:		chip sensors configurations.
  *  @suspended:		suspended sensors configuration.
+ *  @buffer:		data transfer buffer aligned for DMA.
  */
 struct inv_icm42607_state {
 	struct mutex lock;
@@ -130,6 +131,7 @@ struct inv_icm42607_state {
 	struct iio_mount_matrix orientation;
 	struct inv_icm42607_conf conf;
 	struct inv_icm42607_suspended suspended;
+	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
 };
 
 /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
@@ -361,6 +363,9 @@ extern const struct inv_icm42607_hw inv_icm42607_hw_data;
 extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
 extern const struct dev_pm_ops inv_icm42607_pm_ops;
 
+int inv_icm42607_set_temp_conf(struct inv_icm42607_state *st, bool enable,
+			       unsigned int *sleep_ms);
+
 int inv_icm42607_core_probe(struct regmap *regmap,
 			    const struct inv_icm42607_hw *hw,
 			    inv_icm42607_bus_setup bus_setup);
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +18 -0
index 39620da92bd43..a438e6fbe508b 100644
@@ -118,6 +118,24 @@ static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
 	return 0;
 }
 
+int inv_icm42607_set_temp_conf(struct inv_icm42607_state *st, bool enable,
+			       unsigned int *sleep_ms)
+{
+	unsigned int val;
+	int ret;
+
+	val = FIELD_PREP(INV_ICM42607_TEMP_CONFIG0_FILTER_MASK,
+			 INV_ICM42607_FILTER_BW_34HZ);
+	ret = regmap_update_bits(st->map, INV_ICM42607_REG_TEMP_CONFIG0,
+				 INV_ICM42607_TEMP_CONFIG0_FILTER_MASK, val);
+	if (ret)
+		return ret;
+
+	return inv_icm42607_set_pwr_mgmt0(st, st->conf.gyro.mode,
+					  st->conf.accel.mode, enable,
+					  sleep_ms);
+}
+
 static int inv_icm42607_set_conf(struct inv_icm42607_state *st,
 				 const struct inv_icm42607_conf *conf)
 {
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +82 -0

82 lines not on this page · raw message

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

Re: [PATCH V10 6/9] iio: imu: inv_icm42607: Add Temp Support in icm42607

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The temperature sensor is explicitly enabled for a one-shot sysfs read but is never disabled…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The temperature sensor is explicitly enabled for a one-shot sysfs read but is never disabled, resulting in a permanent 77ms latency penalty on all subsequent runtime resumes, the disabling of the low-power RC oscillator clock, and increased power consumption during active states. --

commit 4c6f897d3d27fc0ab2a068b2b3d0736751dc6150
Author: Chris Morgan <[email protected]>

    iio: imu: inv_icm42607: Add Temp Support in icm42607

This commit adds support for reading temperature sensor data to the inv_icm42607 driver. It introduces functions to configure the temperature sensor and read its raw data values.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +0 -0
new file mode 100644

[ ... ]

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +10 -0
+static int inv_icm42607_temp_read(struct inv_icm42607_state *st, s16 *temp)
+{
+	struct device *dev = regmap_get_device(st->map);
+	__be16 *raw;
+	int ret;
+
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +10 -0
+
+	guard(mutex)(&st->lock);
+
+	ret = inv_icm42607_set_temp_conf(st, true, NULL);
+	if (ret)
+		return ret;
+
+	raw = &st->buffer[0];
+	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_TEMP_DATA1,
+			       raw, sizeof(*raw));

[Severity: High]
Does this leave the temperature sensor permanently enabled?

Calling inv_icm42607_set_temp_conf() with true sets the sensor to active, but it doesn't appear to be disabled after the read completes or fails.

If the temperature sensor is left enabled, st->conf.temp_en remains true. On subsequent runtime resumes, inv_icm42607_set_pwr_mgmt0() would be called with temp = true. This appears to force a 77ms sleep (INV_ICM42607_TEMP_STARTUP_TIME_MS) on every resume, rather than the standard 10ms for accelerometer wakeups.

Additionally, since temp is permanently true, the low-power RC oscillator clock (INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL) might be prevented from being selected, leading to continuously increased power consumption during all active states.

Should there be a call to inv_icm42607_set_temp_conf(st, false, NULL) before returning to restore the previous power state?

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +9 -0
+	if (ret)
+		return ret;
+
+	*temp = be16_to_cpup(raw);
+	if (*temp == INV_ICM42607_DATA_INVALID)
+		return -EINVAL;
+
+	return 0;
+}

[ ... ]

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

PATCH v10 7/9 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   |  40 ++
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 465 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  93 ++++
 4 files changed, 599 insertions(+)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
index c04953ed42ce9..d74b23b1e1bec 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_temp.o
 
 obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +40 -0
index 13a5b06e80142..fb4bcc2b7da54 100644
@@ -14,6 +14,8 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/iio/common/inv_sensors_timestamp.h>
+
 /*
  * Serial bus slew rates. Rates are expressed as range between the two
  * values with the midpoint as the typical rate. For the final value of
@@ -85,12 +87,24 @@ enum inv_icm42607_filter_bw {
 	INV_ICM42607_FILTER_BW_NB
 };
 
+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;
 	int odr;
 	int filter;
 };
+#define INV_ICM42607_SENSOR_CONF_INIT		{ -1, -1, -1, -1 }
 
 struct inv_icm42607_conf {
 	struct inv_icm42607_sensor_conf gyro;
@@ -120,6 +134,7 @@ struct inv_icm42607_suspended {
  *  @orientation:	sensor chip orientation relative to main hardware.
  *  @conf:		chip sensors configurations.
  *  @suspended:		suspended sensors configuration.
+ *  @indio_accel:	accelerometer IIO device.
  *  @buffer:		data transfer buffer aligned for DMA.
  */
 struct inv_icm42607_state {
@@ -131,9 +146,22 @@ struct inv_icm42607_state {
 	struct iio_mount_matrix orientation;
 	struct inv_icm42607_conf conf;
 	struct inv_icm42607_suspended suspended;
+	struct iio_dev *indio_accel;
 	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
 };
 
+/**
+ * struct inv_icm42607_sensor_state - sensor state variables
+ * @power_mode:		sensor requested power mode (for common frequencies)
+ * @filter:		sensor filter.
+ * @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 */
 
 /* Register Map for User Bank 0 */
@@ -363,6 +391,16 @@ 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);
+
+const struct iio_mount_matrix *
+inv_icm42607_get_mount_matrix(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan);
+
+int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
+				struct inv_icm42607_sensor_conf *conf,
+				unsigned int *sleep_ms);
+
 int inv_icm42607_set_temp_conf(struct inv_icm42607_state *st, bool enable,
 			       unsigned int *sleep_ms);
 
@@ -370,4 +408,6 @@ int inv_icm42607_core_probe(struct regmap *regmap,
 			    const struct inv_icm42607_hw *hw,
 			    inv_icm42607_bus_setup bus_setup);
 
+struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st);
+
 #endif
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +465 -0

465 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +93 -0

111 lines not on this page · raw message

Signature · 1 line
2.43.0
#16 lore raw

Re: [PATCH V10 7/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] Incomplete IIO buffer support results in dead code and logically flawed `update_scan_mode`…

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Incomplete IIO buffer support results in dead code and logically flawed update_scan_mode callback.
- [Medium] Accelerometer initialized with conflicting power mode and filter macro.
- [Medium] Failing to configure the hardware ODR leaves the software timestamp structure in an inconsistent state.
- [Medium] Lockless reads of shared state in inv_icm42607_accel_read_scale() and inv_icm42607_accel_read_odr(). --

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

iio: imu: inv_icm42607: Add Accelerometer for icm42607

Add icm42607 accelerometer sensor for icm42607.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +0 -0
new file mode 100644
index 0000000000000..6c1f88688c3f4

[ ... ]

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

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +7 -0
+static int inv_icm42607_accel_read_scale(struct iio_dev *indio_dev,
+					 int *val, int *val2)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	unsigned int idx;
+
+	idx = st->conf.accel.fs;

[Severity: Medium]
Is it safe to read st->conf.accel.fs here without acquiring st->lock?

The write path in inv_icm42607_accel_write_scale() mutates this state while holding the lock, so concurrent accesses might result in a data race. Should this read be protected using guard(mutex)(&st->lock)?

[ ... ]

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

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +7 -0
+static int inv_icm42607_accel_read_odr(struct inv_icm42607_state *st,
+				       int *val, int *val2)
+{
+	unsigned int odr;
+	unsigned int i;
+
+	odr = st->conf.accel.odr;

[Severity: Medium]
Does this read of st->conf.accel.odr need to be protected by st->lock, similar to how it is protected during writes in inv_icm42607_accel_write_odr()?

[ ... ]

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 309

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;

[Severity: Medium]
If inv_icm42607_set_accel_conf() fails here, do we leave the software timestamp module expecting the new ODR while the hardware remains 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 441

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +2 -0
+	accel_st->power_mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
+	accel_st->filter = INV_ICM42607_FILTER_AVG_16X;

[Severity: Medium]
Is this the intended filter configuration?

We are setting the power_mode to INV_ICM42607_SENSOR_MODE_LOW_NOISE, but using an averaging filter macro (INV_ICM42607_FILTER_AVG_16X) meant for low power mode.

[ ... ]

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

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +7 -0
+	iio_device_set_drvdata(indio_dev, st);
+	indio_dev->name = name;
+	indio_dev->info = &inv_icm42607_accel_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = inv_icm42607_accel_channels;
+	indio_dev->num_channels = ARRAY_SIZE(inv_icm42607_accel_channels);
+	indio_dev->available_scan_masks = inv_icm42607_accel_scan_masks;

[Severity: Medium]
Since INDIO_DIRECT_MODE is the only mode configured and there is no buffer setup (like devm_iio_kfifo_buffer_setup), does this make the buffer-specific logic like inv_icm42607_accel_update_scan_mode, available_scan_masks, and IIO_CHAN_SOFT_TIMESTAMP dead code?

Also, if inv_icm42607_accel_update_scan_mode were to be called, it lacks a PM runtime acquisition, which could lead to accessing hardware registers on a suspended device.

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

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4 -0
+int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
+				struct inv_icm42607_sensor_conf *conf,
+				unsigned int *sleep_ms)
+{

[ ... ]

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

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +8 -0
+	if (conf->filter != oldconf->filter) {
+		if (conf->mode == INV_ICM42607_SENSOR_MODE_LOW_POWER) {
+			val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG1_AVG_MASK, conf->filter);
+			ret = regmap_update_bits(st->map, INV_ICM42607_REG_ACCEL_CONFIG1,
+						 INV_ICM42607_ACCEL_CONFIG1_AVG_MASK, val);
+		} else {
+			val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK,
+					 conf->filter);

[Severity: Medium]
Since conf->mode is initialized to LOW_NOISE, this branch will apply the filter value (3 for AVG_16X) to INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK.

Value 3 in the bandwidth enum corresponds to INV_ICM42607_FILTER_BW_73HZ. Will this silently set a 73Hz bandwidth instead of the intended 16x averaging?

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

PATCH v10 8/9 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   |   8 +
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  48 ++
 .../iio/imu/inv_icm42607/inv_icm42607_gyro.c  | 437 ++++++++++++++++++
 4 files changed, 494 insertions(+)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
index d74b23b1e1bec..7b907e0196017 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_temp.o
 
drivers/iio/imu/inv_icm42607/inv_icm42607.h +8 -0
index fb4bcc2b7da54..b2f081ec8776c 100644
@@ -134,6 +134,7 @@ struct inv_icm42607_suspended {
  *  @orientation:	sensor chip orientation relative to main hardware.
  *  @conf:		chip sensors configurations.
  *  @suspended:		suspended sensors configuration.
+ *  @indio_gyro:	gyroscope IIO device.
  *  @indio_accel:	accelerometer IIO device.
  *  @buffer:		data transfer buffer aligned for DMA.
  */
@@ -146,6 +147,7 @@ struct inv_icm42607_state {
 	struct iio_mount_matrix orientation;
 	struct inv_icm42607_conf conf;
 	struct inv_icm42607_suspended suspended;
+	struct iio_dev *indio_gyro;
 	struct iio_dev *indio_accel;
 	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
 };
@@ -401,6 +403,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);
 
@@ -408,6 +414,8 @@ int inv_icm42607_core_probe(struct regmap *regmap,
 			    const struct inv_icm42607_hw *hw,
 			    inv_icm42607_bus_setup bus_setup);
 
+struct iio_dev *inv_icm42607_gyro_init(struct inv_icm42607_state *st);
+
 struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st);
 
 #endif
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +48 -0

60 lines not on this page · raw message

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

437 lines not on this page · raw message

Signature · 1 line
2.43.0
#18 lore raw

Re: [PATCH V10 8/9] iio: imu: inv_icm42607: Add Gyroscope to icm42607

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Gyroscope is permanently left powered on in the driver's cached state after a single raw read…

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Gyroscope is permanently left powered on in the driver's cached state after a single raw read, causing a cross-sensor power leak.
- [Medium] Concurrent lockless access to shared configuration fields in sysfs handlers.
- [Low] Buffer-specific structures, channels, and callbacks are defined but completely unreachable (dead code) because the device does not instantiate an IIO buffer.
- [Low] Variable ret may trigger a -Wmaybe-uninitialized compiler warning in inv_icm42607_gyro_update_scan_mode().
--

commit f6b9c2f7324591ed8e8a04df80fed00466b4a298

Author: Chris Morgan <[email protected]>
Date:   Thu Jun 4 20:18:30 2026 +0000

iio: imu: inv_icm42607: Add Gyroscope to icm42607

Add gyroscope functions to the icm42607 driver.

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

[ ... ]

Quoted 22 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +22 -0
+/* enable gyroscope sensor */
+static int inv_icm42607_gyro_update_scan_mode(struct iio_dev *indio_dev,
+					      const unsigned long *scan_mask)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
+	unsigned int sleep_gyro = 0;
+	unsigned int sleep_temp = 0;
+	int ret;
+
+	mutex_lock(&st->lock);
+
+	if (*scan_mask & INV_ICM42607_SCAN_MASK_TEMP) {
+		/* enable temp sensor */
+		ret = inv_icm42607_set_temp_conf(st, true, &sleep_temp);
+		if (ret)
+			goto out_unlock;
+	}
+
+	if (*scan_mask & INV_ICM42607_SCAN_MASK_GYRO_3AXIS) {
+		/* enable gyro sensor */
+		conf.mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10 -0
+		ret = inv_icm42607_set_gyro_conf(st, &conf, &sleep_gyro);
+		if (ret)
+			goto out_unlock;
+	}
+
+out_unlock:
+	mutex_unlock(&st->lock);
+	/* sleep maximum required time */
+	msleep(max(sleep_gyro, sleep_temp));
+	return ret;

[Severity: Low]
Can ret be returned uninitialized here?

If *scan_mask contains neither INV_ICM42607_SCAN_MASK_TEMP nor INV_ICM42607_SCAN_MASK_GYRO_3AXIS, the code jumps directly to out_unlock and returns the uninitialized stack value.

Although the IIO subsystem guarantees the mask strictly matches one of the available_scan_masks (which all contain the 3-axis gyro mask), compilers and static analyzers evaluate the function locally. This triggers a -Wmaybe-uninitialized warning that will break builds configured with W=1 or -Werror.

Quoted 31 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +31 -0
+}
+
+static int inv_icm42607_gyro_read_sensor(struct iio_dev *indio_dev,
+					 struct iio_chan_spec const *chan,
+					 s16 *val)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	struct inv_icm42607_sensor_state *gyro_st = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(st->map);
+	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
+	unsigned int reg;
+	__be16 *data;
+	int ret;
+
+	if (chan->type != IIO_ANGL_VEL)
+		return -EINVAL;
+
+	switch (chan->channel2) {
+	case IIO_MOD_X:
+		reg = INV_ICM42607_REG_GYRO_DATA_X1;
+		break;
+	case IIO_MOD_Y:
+		reg = INV_ICM42607_REG_GYRO_DATA_Y1;
+		break;
+	case IIO_MOD_Z:
+		reg = INV_ICM42607_REG_GYRO_DATA_Z1;
+		break;
+	default:
+		return -EINVAL;
+	}
+

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10 -0
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&st->lock);
+
+	/* enable gyro sensor */
+	conf.mode = gyro_st->power_mode;
+	ret = inv_icm42607_set_gyro_conf(st, &conf, NULL);

[Severity: High]
Does this leave the gyroscope permanently powered on in the driver's cached state?

When the raw read finishes, inv_icm42607_set_gyro_conf() isn't called again to revert the state to OFF.

When PM auto-suspend triggers after the delay, it appears inv_icm42607_runtime_suspend() will erroneously save this active state into st->suspended.gyro before powering off the device.

Later, if a read is performed on another sensor like the accelerometer, inv_icm42607_runtime_resume() restores st->suspended.gyro to the hardware. Consequently, the gyroscope is incorrectly powered back on and will remain powered on during all future PM active periods, draining power unnecessarily.

Quoted 15 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +15 -0
+	if (ret)
+		return ret;
+
+	/* read gyro register data */
+	data = &st->buffer[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)
+		return -EINVAL;
+
+	return 0;
+}

[ ... ]

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

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +7 -0
+static int inv_icm42607_gyro_read_scale(struct iio_dev *indio_dev,
+					int *val, int *val2)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	unsigned int idx;
+
+	idx = st->conf.gyro.fs;

[Severity: Medium]
Is it safe to read st->conf.gyro.fs without holding st->lock here?

Concurrent sysfs writes via inv_icm42607_gyro_write_scale() lock st->lock and modify this exact field. Reading it locklessly creates a data race that is undefined behavior in C and could be flagged by KCSAN.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +5 -0
+
+	*val = inv_icm42607_gyro_scale_nano[idx][0];
+	*val2 = inv_icm42607_gyro_scale_nano[idx][1];
+	return IIO_VAL_INT_PLUS_NANO;
+}

[ ... ]

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

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +7 -0
+static int inv_icm42607_gyro_read_odr(struct inv_icm42607_state *st,
+				      int *val, int *val2)
+{
+	unsigned int odr;
+	unsigned int i;
+
+	odr = st->conf.gyro.odr;

[Severity: Medium]
Similarly, is it safe to read st->conf.gyro.odr here without holding st->lock?

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +5 -0
+
+	for (i = 5; i < ARRAY_SIZE(inv_icm42607_gyro_odr); ++i) {
+		if (i == odr)
+			break;
+	}

[ ... ]

Quoted 26 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +26 -0
+struct iio_dev *inv_icm42607_gyro_init(struct inv_icm42607_state *st)
+{
+	struct device *dev = regmap_get_device(st->map);
+	const char *name;
+	struct inv_icm42607_sensor_state *gyro_st;
+	struct inv_sensors_timestamp_chip ts_chip;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	name = devm_kasprintf(dev, GFP_KERNEL, "%s-gyro", st->hw->name);
+	if (!name)
+		return ERR_PTR(-ENOMEM);
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*gyro_st));
+	if (!indio_dev)
+		return ERR_PTR(-ENOMEM);
+	gyro_st = iio_priv(indio_dev);
+
+	gyro_st->power_mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
+
+	/*
+	 * clock period is 32kHz (31250ns)
+	 * jitter is +/- 2% (20 per mille)
+	 */
+	ts_chip.clock_period = 31250;
+	ts_chip.jitter = 20;

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10 -0
+	ts_chip.init_period = inv_icm42607_odr_to_period(st->conf.gyro.odr);
+	inv_sensors_timestamp_init(&gyro_st->ts, &ts_chip);
+
+	iio_device_set_drvdata(indio_dev, st);
+	indio_dev->name = name;
+	indio_dev->info = &inv_icm42607_gyro_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = inv_icm42607_gyro_channels;
+	indio_dev->num_channels = ARRAY_SIZE(inv_icm42607_gyro_channels);
+	indio_dev->available_scan_masks = inv_icm42607_gyro_scan_masks;

[Severity: Low]
Since the device is initialized exclusively in INDIO_DIRECT_MODE and lacks any calls to allocate or attach a triggered buffer (e.g., via devm_iio_triggered_buffer_setup), are the scan masks and buffer callbacks dead code?

Without a buffer setup, inv_icm42607_gyro_scan_masks, the INV_ICM42607_GYRO_SCAN_TIMESTAMP channel, the software tracking logic, and the update_scan_mode callback will never be executed.

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +7 -0
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return indio_dev;
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
#19 lore raw

PATCH v10 9/9 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.

---
 arch/arm64/boot/dts/rockchip/rk3568-anbernic-rg-ds.dts | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/arm64/boot/dts/rockchip/rk3568-anbernic-rg-ds.dts +7 -1
index 8d906ab02c5fb..b770bfd5268d8 100644
@@ -871,7 +871,13 @@ aw87391_pa_r: audio-codec@5b {
 		sound-name-prefix = "Right Amp";
 	};
 
-	/* invensense,icm42607p at 0x68 */
+	icm42607p: imu@68 {
+		compatible = "invensense,icm42607p";
+		reg = <0x68>;
+		mount-matrix = "-1", "0", "0",
+			       "0", "1", "0",
+			       "0", "0", "-1";
+	};
 };
 
 &i2c3 {
Signature · 1 line
2.43.0