Skip to contentSkip to conversation

Add Invensense ICM42607

PATCH Superseded Chris Morgan 3 participants 22 messages May 12 – May 14

Subsystems (4)

v6 of 21 Superseded Latest is v21 →

lore
Share & tools
lore
#0 lore raw

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

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   | 457 +++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 879 ++++++++++++++++++
 .../imu/inv_icm42607/inv_icm42607_buffer.c    | 565 +++++++++++
 .../imu/inv_icm42607/inv_icm42607_buffer.h    |  95 ++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 854 +++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_gyro.c  | 510 ++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |  90 ++
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   | 103 ++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.c  |  80 ++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.h  |  33 +
 16 files changed, 3735 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 v6 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
Reviewed-by on v6 1/11
#2 lore raw

Re: [PATCH V6 01/11] dt-bindings: iio: imu: icm42600: Add mount-matrix to icm42600

Quoted 9 lines · Chris Morgan

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(+)

Best regards,
Krzysztof

#3 lore raw

PATCH v6 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
Acked-by on v6 2/11
#4 lore raw

Re: [PATCH V6 02/11] dt-bindings: iio: imu: icm42600: Add icm42607 binding

Quoted 6 lines · 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.

Quoted 7 lines

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(+)

Best regards,
Krzysztof

#5 lore raw

PATCH v6 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   | 334 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 209 +++++++++++
 2 files changed, 543 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 000000000000..1916e0b08bca
@@ -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
+};
+
+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_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
+
+/* 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 +209 -0

209 lines not on this page · raw message

Signature · 1 line
2.43.0
#6 lore raw

Re: [PATCH V6 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: - [High] Polling for reset completion immediately after a soft reset will cause probe to fail due to a bus…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Polling for reset completion immediately after a soft reset will cause probe to fail due to a bus read error. --

commit 62a44073ad81f2d638c1665cd828465dfd85d6f7
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

This commit introduces the core driver component for the inv_icm42607 IMU, setting up initial configuration, reset handling, and register definitions.

[ ... ]

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

[ ... ]

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +3 -0
+static int inv_icm42607_setup(struct inv_icm42607_state *st,
+			      inv_icm42607_bus_setup bus_setup)
+{

[ ... ]

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

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +13 -0
+	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
+			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
+	if (ret)
+		return ret;
+
+	/*
+	 * Datasheet only specifies a reset period wait of 1ms for a
+	 * power-on reset, but not a soft reset so not waiting here.
+	 */
+	ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
+				       val, val & INV_ICM42607_INT_STATUS_RESET_DONE,
+				       INV_ICM42607_RESET_TIME_MS * 100,
+				       INV_ICM42607_RESET_TIME_MS * 1000);

Could this cause the probe to fail due to a bus read error?

When a soft reset is triggered, the device's digital core restarts, making the interface temporarily unresponsive. Since regmap_read_poll_timeout performs its first read immediately, the read will likely return a bus error.

Because regmap_read_poll_timeout evaluates its exit condition on any error returned by the read operation, it will abort immediately rather than retrying the read.

Would it be safer to insert a small sleep delay before attempting to read from the device after initiating the soft reset?

[ ... ]

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

PATCH v6 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  |  59 +++++++++-
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |  89 +++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   | 102 ++++++++++++++++++
 8 files changed, 303 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_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 1916e0b08bca..2c20e95b237a 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,
@@ -327,6 +337,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 +58 -1

71 lines not on this page · raw message

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

89 lines not on this page · raw message

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

102 lines not on this page · raw message

Signature · 1 line
2.43.0
#8 lore raw

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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Probe incorrectly returns 0 (success) when the IRQ mapping fails, leading to an uninitialized but…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Probe incorrectly returns 0 (success) when the IRQ mapping fails, leading to an uninitialized but successfully bound driver.
- [High] SPI 3-wire mode initialization will fail to probe or corrupt the device configuration register due to a read-modify-write on a mismatched bus state. --

commit cfb2b10debf46b662831cd805722f6c7939bea7c
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 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
index 0baf4763b0a7d..6119379ca6a8b 100644

[ ... ]

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

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +1 -1
@@ -154,7 +211,7 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
 	int ret;
 
 	irq = fwnode_irq_get_byname(fwnode, "INT1");
-	if (irq < 0)
+	if (irq <= 0)
 		return dev_err_probe(dev, irq, "error missing INT1 interrupt\n");

If fwnode_irq_get_byname() returns 0, could this lead to an uninitialized but successfully bound driver?

Since dev_err_probe() returns the error code it is passed, passing 0 will cause inv_icm42607_core_probe() to immediately return 0. This bypasses the driver data setup and hardware initialization, which might cause a NULL pointer dereference later during suspend when dev_get_drvdata() is called.

Would it be better to pass an explicit negative error code like -EINVAL when irq is 0?

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +0 -0
new file mode 100644
index 0000000000000..49438fa6f8673

[ ... ]

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +10 -0
+static int inv_icm42607_spi_bus_setup(struct inv_icm42607_state *st)
+{
+	const struct device *dev = regmap_get_device(st->map);
+	const struct spi_device *spi = to_spi_device(dev);
+	unsigned int val;
+	int ret;
+
+	if (spi->mode & SPI_3WIRE)
+		ret = regmap_clear_bits(st->map, INV_ICM42607_REG_DEVICE_CONFIG,
+					INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE);

Does this read-modify-write correctly handle a device that powers up in 4-wire mode when the host expects 3-wire mode?

It appears inv_icm42607_setup() performs reads (like a WHOAMI check and reset poll) before calling bus_setup(). If the chip is in 4-wire mode and outputs on SDO while the host listens on MOSI/SISO, wouldn't these reads return garbage?

If the reads fail or return garbage, wouldn't regmap_clear_bits() read that garbage and write it back, corrupting other bits in the register like SPI_MODE?

Should 3-wire mode be enabled using a direct regmap_write() prior to any reads?

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

PATCH v6 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   |  12 ++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 151 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |   1 +
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   |   1 +
 4 files changed, 165 insertions(+)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +12 -0
index 2c20e95b237a..5f37999e39a5 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,24 +97,34 @@ 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.
  *  @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;
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;
 	struct regulator *vddio_supply;
+	bool vddio_en;
 	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 */
@@ -339,6 +350,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 +151 -0

191 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c +1 -0
index 8631579a609a..a51c764bf1dc 100644
@@ -77,6 +77,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 49438fa6f867..9ba3dd0c689a 100644
@@ -90,6 +90,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
#10 lore raw

Re: [PATCH V6 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: - [Low] Misleading comment in `inv_icm42607_resume()` regarding runtime PM state. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Misleading comment in inv_icm42607_resume() regarding runtime PM state. --

commit 5e3ef42dfba709e36dacf1125d8f13cfabbf3f5c
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 6119379ca6a8b..dfd2fed7bf1e6 100644

[ ... ]

Quoted 4 lines

+/*
+ * System resume gets the system back on and restores the sensors state.
+ * Manually put runtime power management in system active state.
+ */

This isn't a bug, but the comment mentions manually putting runtime power management in the system active state.

However, the function correctly does not call any PM core APIs (such as pm_runtime_set_active()) to alter the runtime PM status. The driver checks pm_runtime_suspended() to avoid waking up hardware that was already runtime-suspended, ensuring the hardware state perfectly aligns with the PM core's state.

Could this comment be updated or removed to prevent confusion?

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +9 -0
+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;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
#11 lore raw

PATCH v6 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    | 486 ++++++++++++++++++
 .../imu/inv_icm42607/inv_icm42607_buffer.h    |  93 ++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  54 +-
 5 files changed, 660 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 5f37999e39a5..b00ee6a4d451 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;
@@ -114,6 +119,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;
@@ -125,6 +134,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 */
@@ -352,6 +377,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 +486 -0

486 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 +53 -1

84 lines not on this page · raw message

Signature · 1 line
2.43.0
#12 lore raw

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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Hardware FIFO stream corruption via inadvertent read of FIFO_DATA register - [High]…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Hardware FIFO stream corruption via inadvertent read of FIFO_DATA register
- [High] Uninitialized variable and inverted bounds check in FIFO parsing loops --

commit 1ef48549ddf09f109d55e87b2f2e81329ea2a547
Author: Chris Morgan <[email protected]>

    iio: imu: inv_icm42607: Add Buffer support for icm42607

This commit adds buffer support for the inv_icm42607 hardware by implementing FIFO parsing and reading functions.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c new file +0 -0
new file mode 100644
index 0000000000000..0919b4241241a

[ ... ]

Quoted 17 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c new file +17 -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;
+	const s8 *temp;
+	ssize_t size;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	/* reset all samples counters */
+	st->fifo.count = 0;
+	st->fifo.nb.gyro = 0;
+	st->fifo.nb.accel = 0;
+	st->fifo.nb.total = 0;
+

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c line 423

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c new file +10 -0
+	/* compute maximum FIFO read size */
+	if (max == 0)
+		max_count = sizeof(st->fifo.data);
+	else
+		max_count = min((max * inv_icm42607_get_packet_size(st->fifo.en)),
+				sizeof(st->fifo.data));
+
+	/* read FIFO count value */
+	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_FIFO_COUNTH,
+			       st->buffer, sizeof(u8) * 3);

Does reading 3 bytes here fetch the COUNTH, COUNTL, and FIFO_DATA registers?

If so, could this inadvertent read of the FIFO_DATA register automatically dequeue 1 byte from the hardware FIFO?

This could cause the subsequent regmap_noinc_read() to fetch misaligned packet headers, corrupting the decoded sensor stream. Should this be a 2-byte read instead?

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c new file +8 -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;
+

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c line 441

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c new file +10 -0
+	st->fifo.count = min(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);
+	if (ret)
+		return ret;
+
+	/* compute number of samples for each sensor */
+	for (i = 0; i < st->fifo.count && i + size > st->fifo.count; i += size) {

Is the size variable uninitialized on the first iteration of this loop?

Also, is the i + size > st->fifo.count condition logically inverted? This appears to require the packet to overflow the buffer boundaries to continue, which could cause the loop to terminate prematurely or not run at all.

Could this inverted logic allow a partial packet to bypass boundaries and cause an out-of-bounds read inside inv_icm42607_fifo_is_data_valid() later in the loop?

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c new file +13 -0
+		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
+				&accel, &gyro, &temp, &timestamp);
+		if (size <= 0)
+			break;
+		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;
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
#13 lore raw

PATCH v6 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 b00ee6a4d451..4e053281dc55 100644
@@ -379,6 +379,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 3b5d5722ae39..9b10722af3ae 100644
@@ -164,6 +164,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
#14 lore raw

Re: [PATCH V6 07/11] 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 and high-power clock are left enabled indefinitely after a one-shot read. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The temperature sensor and high-power clock are left enabled indefinitely after a one-shot read. --

commit 8d77083c202fdb58f8003d737a9eb92b429cb188
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add Temp Support in icm42607

This commit adds functions for reading temperature sensor data.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +0 -0
new file mode 100644
index 0000000000000..7e9cb072a0110

[ ... ]

Quoted 17 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +17 -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);
+	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
+		return -ENXIO;
+
+	guard(mutex)(&st->lock);
+
+	ret = inv_icm42607_set_temp_conf(st, true, NULL);
+	if (ret)
+		return ret;
+
+	raw = &st->buffer[0];

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

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

Does this leave the temperature sensor and high-power clock enabled indefinitely after a one-shot read?

It looks like inv_icm42607_set_temp_conf() is called with true before the read, which enables the sensor and disables the low-power RC oscillator, but the sensor is never disabled again before returning.

While pm_runtime_autosuspend might eventually power down the device, if another operation (like an accelerometer read) keeps the PM runtime active before the autosuspend delay expires, the autosuspend will not happen.

Because other configuration functions preserve the existing st->conf.temp_en state, the high-power clock and temperature sensor would remain permanently active, leading to an unintended power draw.

Should there be a matching call to inv_icm42607_set_temp_conf() with false after the read is complete?

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

PATCH v6 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 | 575 ++++++++++++++++++
 .../imu/inv_icm42607/inv_icm42607_buffer.c    |  63 +-
 .../imu/inv_icm42607/inv_icm42607_buffer.h    |   4 +-
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  72 +++
 6 files changed, 738 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 4e053281dc55..095f4a495e3f 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;
@@ -121,6 +132,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.
  */
@@ -136,6 +148,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);
 };
@@ -377,12 +392,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 +575 -0

575 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 +72 -0

109 lines not on this page · raw message

Signature · 1 line
2.43.0
#16 lore raw

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

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] Uninitialized variable `size` used in loop condition and incorrect bounds checking leads to…

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Uninitialized variable size used in loop condition and incorrect bounds checking leads to undefined behavior and out-of-bounds reads.
- [High] Self-deadlock on st->lock during hardware FIFO flush.
- [High] NULL pointer dereference of st->indio_gyro during hardware FIFO flush.
- [High] Missing PM runtime acquisition in sysfs callbacks.
- [High] IIO device registration race condition leading to NULL pointer dereference.
- [High] 3-byte regmap read corrupts hardware FIFO stream.
--

commit 965b05f7b7791892c50d261d2a071baceb950c47
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..06f2126af22a8

[ ... ]

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +10 -0
+static int inv_icm42607_accel_hwfifo_set_watermark(struct iio_dev *indio_dev,
+						   unsigned int val)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+
+	guard(mutex)(&st->lock);
+
+	st->fifo.watermark.accel = val;
+	return inv_icm42607_buffer_update_watermark(st);
+}

Does this callback need to acquire a power management reference (via PM_RUNTIME_ACQUIRE_AUTOSUSPEND) before updating hardware registers? Since the runtime suspend callback turns off the sensor, accessing registers here could result in bus lockups or dropped writes.

[ ... ]

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

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +13 -0
+static int inv_icm42607_accel_hwfifo_flush(struct iio_dev *indio_dev,
+					   unsigned int count)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	int ret;
+
+	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);

Similarly, should this callback acquire a power management reference before communicating with the hardware to avoid timeouts?

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

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +1 -0
+	ret = inv_icm42607_buffer_hwfifo_flush(st, count);

Will this result in a self-deadlock? The guard(mutex) above acquires st->lock, but inv_icm42607_buffer_hwfifo_flush() calls inv_icm42607_buffer_fifo_read(), which unconditionally attempts to acquire st->lock again.

[ ... ]

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 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +6 -0
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return indio_dev;
+}

Does calling devm_iio_device_register() here expose the IIO device to userspace before st->indio_accel is assigned in inv_icm42607_core_probe()?

If userspace triggers a hardware FIFO flush in this window, it seems this would lead to a NULL pointer dereference when accessing st->indio_accel.

[ ... ]

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 550

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +11 -0
+	ssize_t i, size;
+	unsigned int no;
+	const void *accel, *gyro, *timestamp;
+	const int8_t *temp;
+	unsigned int odr;
+	int64_t ts_val;
+	struct inv_icm42607_accel_buffer buffer = { };
+
+	/* parse all fifo packets */
+	for (i = 0, no = 0; i < st->fifo.count && i + size > st->fifo.count;
+	     i += size, ++no) {

Is the size variable uninitialized on the first iteration of this loop?

If the garbage value happens to be small, could the inverted loop condition (using > instead of <=) cause the loop to exit immediately or lead to out-of-bounds heap memory reads when parsing the data?

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +0 -0
index 0919b4241241a..42a3211a1dfb8 100644

[ ... ]

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

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c +1 -1
@@ -440,7 +460,7 @@ int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,
 	/* compute number of samples for each sensor */
 	for (i = 0; i < st->fifo.count && i + size > st->fifo.count; i += size) {
 		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
-				&accel, &gyro, &temp, &timestamp);
+				&accel, &gyro, &temp, &timestamp, &odr);

As mentioned above, could the uninitialized size variable and inverted loop condition cause out-of-bounds reads here as well?

Also, does reading the FIFO count earlier in this function with a 3-byte regmap bulk read at INV_ICM42607_REG_FIFO_COUNTH inadvertently pop a byte from the FIFO_DATA register? It seems this might misalign the subsequent read and corrupt the stream. I realize this wasn't introduced in this patch, but wanted to point it out.

[ ... ]

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 +2 -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 passing st->indio_gyro to iio_get_time_ns() cause a panic here?

It appears st->indio_gyro is not initialized in this patch and remains NULL. I understand this is fixed later in the patch series, but could this break git bisect?

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

PATCH v6 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   |  33 ++
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 304 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 263 ++++++++++++++-
 3 files changed, 590 insertions(+), 10 deletions(-)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +33 -0
index 095f4a495e3f..16f7929d3d0e 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.
@@ -133,6 +141,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.
  */
@@ -151,6 +160,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);
 };
@@ -376,6 +386,18 @@ struct inv_icm42607_sensor_state {
 #define INV_ICM42607P_WHOAMI				0x60
 #define INV_ICM42607_WHOAMI				0x67
 
+/* Bank 2/3 register access */
+#define INV_ICM42607_BLK_SEL_W				0x79
+#define INV_ICM42607_MADDR_W				0x7A
+#define INV_ICM42607_M_W				0x7B
+#define INV_ICM42607_BLK_SEL_R				0x7C
+#define INV_ICM42607_MADDR_R				0x7D
+#define INV_ICM42607_M_R				0x7E
+
+#define INV_ICM42607_MREG_1				0x00
+#define INV_ICM42607_MREG_2				0x28
+#define INV_ICM42607_MREG_3				0x50
+
 /* Sleep times required by the driver */
 #define INV_ICM42607_POWER_UP_TIME_US			100000
 #define INV_ICM42607_RESET_TIME_MS			1
@@ -384,6 +406,7 @@ struct inv_icm42607_sensor_state {
 #define INV_ICM42607_GYRO_STOP_TIME_MS			150
 #define INV_ICM42607_TEMP_STARTUP_TIME_MS		14
 #define INV_ICM42607_SUSPEND_DELAY_MS			2000
+#define INV_ICM42607_MREG_DELAY_US			10
 
 typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
 
@@ -392,6 +415,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_mreg_write(struct regmap *map, unsigned int bank,
+			    u8 reg, u8 data);
+
 const struct iio_mount_matrix *
 inv_icm42607_get_mount_matrix(struct iio_dev *indio_dev,
 			      const struct iio_chan_spec *chan);
@@ -405,6 +431,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);
 
@@ -412,4 +441,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 +304 -0

340 lines not on this page · raw message

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

335 lines not on this page · raw message

Signature · 1 line
2.43.0
#18 lore raw

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

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] Race condition in `inv_icm42607_accel_write_event_config` leads to PM runtime reference and state…

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Race condition in inv_icm42607_accel_write_event_config leads to PM runtime reference and state leaks.
- [High] The threaded IRQ handler self-deadlocks on the driver's mutex.
- [High] Devres initialization order causes a Use-After-Free during device removal and a potential NULL pointer dereference during probe.
- [High] The Wake-on-Motion thresholds are not configured for the Y and Z axes.
- [Medium] Integer overflow in inv_icm42607_accel_convert_roc_to_wom causes misconfiguration for large input values.
- [Medium] Error return in _inv_icm42607_accel_disable_wom prevents the accelerometer from powering off.
- [Low] Pointer type mismatch in _inv_icm42607_accel_wom_value causes a compiler warning.
--

commit cb98aa6ec3bd79c9e6c757530bb7aecf9c26e058
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 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +0 -0
index 06f2126af22a8..4628bda356370 100644
@@ -209,6 +221,183 @@ static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev,

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +4 -0
+static unsigned int inv_icm42607_accel_convert_roc_to_wom(u64 roc,
+							  int accel_hz, int accel_uhz)
+{
+	/* 1000/256mg per LSB converted in µm/s² */

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +10 -0
+	const unsigned int convert = (9807U * (MICRO / MILLI)) / 256U;
+	u64 value;
+	u64 freq_uhz;
+
+	/* return 0 only if roc is 0 */
+	if (roc == 0)
+		return 0;
+
+	freq_uhz = (u64)accel_hz * MICRO + (u64)accel_uhz;
+	value = div64_u64(roc * MICRO, freq_uhz * (u64)convert);

Does this calculation overflow for large input values?

The roc value can be up to ~2.14 * 10^15. Multiplying this by MICRO again could exceed the maximum value of a 64-bit unsigned integer, potentially resulting in an incorrectly small threshold value wrapping around.

[ ... ]

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

[ ... ]

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

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +5 -0
+	/* set accel WoM threshold for the 3 axes */
+	for (i = 0; i < 3; i++) {
+		ret = inv_icm42607_mreg_write(st->map, INV_ICM42607_MREG_2,
+					      INV_ICM42607_REG_ACCEL_WOM_X_THR,
+					      threshold);

Does this code configure the Y and Z axes?

The register address INV_ICM42607_REG_ACCEL_WOM_X_THR is hardcoded, so it looks like the X-axis register is overwritten three times while the Y and Z thresholds are left uninitialized.

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +6 -0
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}

[ ... ]

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

[ ... ]

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

Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +12 -0
+	scoped_guard(mutex, &st->lock) {
+		/*
+		 * Consider that turning off WoM is always working to avoid
+		 * blocking the chip in on mode and prevent going back to sleep.
+		 * If there is an error, the chip will anyway go back to sleep
+		 * and the feature will not work anymore.
+		 */
+		st->apex.wom.enable = false;
+		st->apex.on--;
+		ret = inv_icm42607_disable_wom(st);
+		if (ret)
+			return ret;

Is this early return intentional?

The comment states that failures should be ignored to avoid blocking the chip in the on state, but the code immediately returns if inv_icm42607_disable_wom() fails. This skips the subsequent code that actually turns off the accelerometer, potentially leaving the chip powered on.

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +8 -0
+		/* turn off accel sensor if not used */
+		if (!st->apex.on && !iio_buffer_enabled(indio_dev)) {
+			conf.mode = INV_ICM42607_SENSOR_MODE_OFF;
+			ret = inv_icm42607_set_accel_conf(st, &conf, &sleep_ms);
+			if (ret)
+				return ret;
+		}
+	}

[ ... ]

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +0 -0
@@ -467,6 +660,113 @@ static int inv_icm42607_accel_hwfifo_flush(struct iio_dev *indio_dev,

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +4 -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,

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

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

Can multiple concurrent writes from userspace bypass this lockless check?

If two threads write '1', both could pass the check, acquire PM runtime references, and increment st->apex.on repeatedly within inv_icm42607_accel_enable_wom(). A subsequent write of '0' would only decrement the counters once, potentially leaking a runtime PM reference and preventing suspend.

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

[ ... ]

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c +10 -0
+static int _inv_icm42607_accel_wom_value(struct inv_icm42607_state *st,
+					 int val, int val2)
+{
+	u64 value;
+	unsigned int accel_hz, accel_uhz;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = inv_icm42607_accel_read_odr(st, &accel_hz, &accel_uhz);

This isn't a bug, but accel_hz and accel_uhz are declared as unsigned int while inv_icm42607_accel_read_odr() expects int * arguments. This causes an incompatible pointer type warning.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
index 56ce9a51cb9c9..a69cd0a61050e 100644
@@ -347,6 +431,110 @@ static int inv_icm42607_setup(struct inv_icm42607_state *st,

[ ... ]

Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +9 -0
+static irqreturn_t inv_icm42607_irq_handler(int irq, void *_data)
+{
+	struct inv_icm42607_state *st = _data;
+	struct device *dev = regmap_get_device(st->map);
+	unsigned int status;
+	int ret;
+
+	guard(mutex)(&st->lock);
+

[ ... ]

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

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +2 -0
+	if (status & INV_ICM42607_INT_STATUS_FIFO_THS) {
+		ret = inv_icm42607_buffer_fifo_read(st, 0);

Will this cause a self-deadlock?

The threaded IRQ handler acquires st->lock via guard(mutex) and then calls inv_icm42607_buffer_fifo_read(), which also attempts to acquire the exact same non-recursive mutex:

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c:inv_icm42607_buffer_fifo_read() {
...

    guard(mutex)(&st->lock);
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4 -0
+		if (ret) {
+			dev_err(dev, "FIFO read error %d\n", ret);
+			return IRQ_HANDLED;
+		}

[ ... ]

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

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +5 -0
@@ -441,6 +634,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 interrupt handling */
+	ret = inv_icm42607_irq_init(st, irq, irq_type, open_drain);
+	if (ret)
+		return ret;
+
 	/* Initialize IIO device for Accel */
 	st->indio_accel = inv_icm42607_accel_init(st);

Is this interrupt request ordered correctly?

The IRQ is enabled via devm_request_threaded_irq in inv_icm42607_irq_init() before the IIO device is allocated and its buffers are set up. If an early interrupt fires, could it dereference a NULL st->indio_accel pointer?

Since devres frees resources in reverse order, could this also cause a use-after-free if an interrupt fires during device teardown after the IIO device has been freed?

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

PATCH v6 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  | 510 ++++++++++++++++++
 5 files changed, 597 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 16f7929d3d0e..158a0385f0b9 100644
@@ -158,6 +158,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;
@@ -428,6 +429,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);
 
@@ -437,6 +442,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 42a3211a1dfb..0d892ef97eab 100644
@@ -384,7 +384,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;
@@ -475,6 +477,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;
@@ -482,6 +485,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;
@@ -498,6 +511,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;
@@ -513,6 +527,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 +510 -0

510 lines not on this page · raw message

Signature · 1 line
2.43.0
#20 lore raw

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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Uninitialized variable 'size' and inverted logic in FIFO parsing loop condition. - [High]…

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Uninitialized variable 'size' and inverted logic in FIFO parsing loop condition.
- [High] Self-deadlock due to double acquisition of st->lock in hardware FIFO flush.
- [High] Race condition in device initialization leads to NULL pointer dereference in IRQ handler.
- [Medium] Gyroscope remains indefinitely powered on in software state after a raw read.
- [Medium] The gyroscope IIO device does not implement the read_avail API.
--

commit 90abb258df502e4c4bb56ac17141c1096a23395d
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add Gyroscope to icm42607

Add gyroscope functions to the icm42607 driver.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0 -0
index a69cd0a61050e..c8545dd68cdb0 100644

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c static irqreturn_t inv_icm42607_irq_timestamp(int irq, void *_data) · line 482

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +1 -0
@@ -435,6 +478,7 @@ static irqreturn_t inv_icm42607_irq_timestamp(int irq, void *_data)
 {
 	struct inv_icm42607_state *st = _data;
 
+	st->timestamp.gyro = iio_get_time_ns(st->indio_gyro);
 	st->timestamp.accel = iio_get_time_ns(st->indio_accel);

Could this result in a NULL pointer dereference?

In inv_icm42607_core_probe(), the interrupt is initialized and the accelerometer device is registered and exposed to userspace before st->indio_gyro is allocated:

	st->indio_accel = inv_icm42607_accel_init(st);
	if (IS_ERR(st->indio_accel))
		...
	st->indio_gyro = inv_icm42607_gyro_init(st);

If userspace enables the accelerometer immediately, the interrupt could fire while st->indio_gyro is still NULL, causing iio_get_time_ns() to dereference it.

[ ... ]

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

[ ... ]

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)
+		return -EINVAL;
+
+	return 0;
+}

Should the gyroscope be powered down before returning?

The device is explicitly powered up to INV_ICM42607_SENSOR_MODE_LOW_NOISE, but it is never restored to INV_ICM42607_SENSOR_MODE_OFF. While pm_runtime_put_autosuspend() is used, if the accelerometer is running concurrently, the device usage count won't reach zero and the gyroscope will remain powered on indefinitely.

[ ... ]

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);

Can this lead to a self-deadlock?

The st->lock mutex is acquired here using guard(mutex)(&st->lock), but inv_icm42607_buffer_hwfifo_flush() subsequently calls inv_icm42607_buffer_fifo_read(), which unconditionally attempts to acquire the exact same st->lock.

[ ... ]

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

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +8 -0
+static const struct iio_info inv_icm42607_gyro_info = {
+	.read_raw = inv_icm42607_gyro_read_raw,
+	.write_raw = inv_icm42607_gyro_write_raw,
+	.write_raw_get_fmt = inv_icm42607_gyro_write_raw_get_fmt,
+	.update_scan_mode = inv_icm42607_gyro_update_scan_mode,
+	.hwfifo_set_watermark = inv_icm42607_gyro_hwfifo_set_watermark,
+	.hwfifo_flush_to_buffer = inv_icm42607_gyro_hwfifo_flush,
+};

Is the .read_avail callback intentionally missing?

Without it, the driver does not expose supported sampling frequencies and scale factors to userspace.

[ ... ]

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +7 -0
+int inv_icm42607_gyro_parse_fifo(struct iio_dev *indio_dev)
+{
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	struct inv_icm42607_sensor_state *gyro_st = iio_priv(indio_dev);
+	struct inv_sensors_timestamp *ts = &gyro_st->ts;
+	ssize_t i, size;
+	unsigned int no;

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10 -0
+	const void *accel, *gyro, *timestamp;
+	const s8 *temp;
+	unsigned int odr;
+	s64 ts_val;
+	struct inv_icm42607_gyro_buffer buffer = { };
+
+	/* parse all fifo packets */
+	for (i = 0, no = 0; i < st->fifo.count && i + size > st->fifo.count;
+	     i += size, ++no) {
+		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],

Will this evaluate the loop condition with an uninitialized size variable?

The variable size is declared but uninitialized, so the expression i + size > st->fifo.count will use an indeterminate value on the first iteration.

Additionally, should this check verify that the packet fits within the buffer (e.g. <=) rather than exceeding it (>)?

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

PATCH v6 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