Skip to contentSkip to conversation

Add Invensense ICM42607

PATCH Superseded Chris Morgan 5 participants 27 messages Jun 11 – Jun 16

Subsystems (4)

v12 of 21 Superseded Latest is v21 →

lore
Share & tools
lore
#0 lore raw

v12 cover Add Invensense ICM42607

From: Chris Morgan <[email protected]>

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

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

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

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

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

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

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

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

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

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

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

Changes Since V10:
- Explicitly specified enum values in header file.
- Removed additional dead code for buffer handling.
- Cleaned up headers.
- Added additional locks as requested by sashiko.dev bot.

Changes Since V11:
- Since driver has shrunk in size considerably, moved i2c bits into first code commit. This ensures that the very first commit with code can now be compiled. The commit after that adds SPI support as it was in the previous versions.
- Used pahole to optimize inv_icm42607_state. Reordering elements reduced size in memory from 384 bytes to 256 bytes.
- Added a map of all readable registers and all writeable registers according to the datasheet.
- Added back some missing headers pointed out by the maintainers.
- Added FIELD_PREP in a few more places to make the code more obvious on what it's doing.
- Added a comment to the power management code to note that temperature sensor being enabled doesn't matter as the clocks are off by default when the gyro and accel channels are off.
- Removed iio_device_claim_direct() calls since it was no longer needed.
- Fixed shared_by_all attributes for temperature sensor.
- Additional miscellanous fixes as requested.

Chris Morgan (9):
dt-bindings: iio: imu: icm42600: Add mount-matrix to icm42600
dt-bindings: iio: imu: icm42600: Add icm42607
iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
iio: imu: inv_icm42607: Add 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   | 417 ++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 376 ++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 543 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_gyro.c  | 370 ++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |  98 ++++
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   | 108 ++++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.c  |  76 +++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.h  |  37 ++
 14 files changed, 2096 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 v12 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 119e28a833fd..9b2af104f186 100644
@@ -53,6 +53,8 @@ properties:
   drive-open-drain:
     type: boolean
 
+  mount-matrix: true
+
   vdd-supply:
     description: Regulator that provides power to the sensor
 
Signature · 1 line
2.43.0
#2 lore raw

PATCH v12 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 9b2af104f186..81b6e85decd5 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
#3 lore raw

PATCH v12 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, as well as the bits necessary to compile and probe the device when used on an i2c bus.

---
 drivers/iio/imu/Kconfig                       |   1 +
 drivers/iio/imu/Makefile                      |   1 +
 drivers/iio/imu/inv_icm42607/Kconfig          |  18 +
 drivers/iio/imu/inv_icm42607/Makefile         |   7 +
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   | 352 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 285 ++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |  97 +++++
 7 files changed, 761 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.h
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.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 +18 -0
new file mode 100644
index 000000000000..083c212087ab
@@ -0,0 +1,18 @@
+# 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-42607 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.
drivers/iio/imu/inv_icm42607/Makefile new file +7 -0
new file mode 100644
index 000000000000..32046e2727d7
@@ -0,0 +1,7 @@
+# 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
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +352 -0

352 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +285 -0

285 lines not on this page · raw message

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

97 lines not on this page · raw message

Signature · 1 line
2.43.0
#4 lore raw

Re: [PATCH V12 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: - [Low] Double application of FIELD_PREP on INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS. - [Low]…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Double application of FIELD_PREP on INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS.
- [Low] Wake-on-Motion threshold registers are omitted from regmap read/write permission access checks. --

commit db0995d0ef167d77bbd7b68753044eb2b93adf1c
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, as well as the bits necessary to compile and probe the device when used on an i2c bus.

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +0 -0
index 0000000000000..5d40f1ee53d6d
@@ -0,0 +1,285 @@

[ ... ]

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

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +13 -0
+static bool inv_icm42607_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case INV_ICM42607_REG_MCLK_RDY ... INV_ICM42607_REG_INT_CONFIG:
+	case INV_ICM42607_REG_TEMP_DATA1 ... INV_ICM42607_REG_TMST_FSYNCL:
+	case INV_ICM42607_REG_APEX_DATA4 ... INV_ICM42607_REG_INTF_CONFIG1:
+	case INV_ICM42607_REG_INT_STATUS_DRDY ... INV_ICM42607_REG_FIFO_DATA:
+	case INV_ICM42607_REG_WHOAMI:
+		return true;
+	}
+
+	return false;
+}

[Severity: Low]
Are the Wake-on-Motion threshold registers (0x4b - 0x4d) missing from this allowed list in inv_icm42607_is_readable_reg()?

Registers like INV_ICM42607_REG_ACCEL_WOM_X_THR are defined in the header, but since they are omitted here, any future attempt to read them will be rejected by the regmap core with an -EIO error.

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

Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +12 -0
+
+static bool inv_icm42607_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case INV_ICM42607_REG_DEVICE_CONFIG ... INV_ICM42607_REG_INT_CONFIG:
+	case INV_ICM42607_REG_PWR_MGMT0 ... INV_ICM42607_REG_INT_SOURCE4:
+	case INV_ICM42607_REG_INTF_CONFIG0 ... INV_ICM42607_REG_INTF_CONFIG1:
+		return true;
+	}
+
+	return false;
+}

[Severity: Low]
Similarly, are the Wake-on-Motion threshold registers missing from the switch statement in inv_icm42607_is_writeable_reg()?

[ ... ]

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +0 -0
index 0000000000000..a1484f32c5d94
@@ -0,0 +1,97 @@

[ ... ]

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +10 -0
+static int inv_icm42607_i2c_bus_setup(struct inv_icm42607_state *st)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_clear_bits(st->map, INV_ICM42607_REG_INTF_CONFIG1,
+				INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN |
+				INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN);
+	if (ret)
+		return ret;

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +10 -0
+
+	val = FIELD_PREP(INV_ICM42607_DRIVE_CONFIG2_I2C_MASK,
+			 INV_ICM42607_SLEW_RATE_12_36NS);
+	ret = regmap_update_bits(st->map, INV_ICM42607_REG_DRIVE_CONFIG2,
+				 INV_ICM42607_DRIVE_CONFIG2_I2C_MASK, val);
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK,
+			 INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS);

[Severity: Low]
Is this double-application of FIELD_PREP intentional?

The macro INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS is already defined using FIELD_PREP in inv_icm42607.h:

#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS \
    FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 2)

Passing it as the value argument to another FIELD_PREP in inv_icm42607_i2c_bus_setup() is conceptually incorrect. It only evaluates correctly here by coincidence because the bitmask GENMASK(1, 0) has a shift of 0.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +4 -0
+	return regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
+				  INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK,
+				  val);
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
maintainer
#5 lore raw

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

In reply to Chris Morgan
Quoted 8 lines · On Thu, 11 Jun 2026 15:26:00 -0500 Chris Morgan <[email protected]> wrote:

From: Chris Morgan <[email protected]>

Add the core component of a new inv_icm42607 driver. This includes a few setup functions and the full register definition in the header file, as well as the bits necessary to compile and probe the device when used on an i2c bus.

Hi Chris,

Given this is nearly ready to merge I took a look at Sashiko and seems it has found a few more things.

Other than those, looks good to me.

Jonathan

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

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +7 -0
new file mode 100644
index 000000000000..c85d3b74166f
 
+#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 this as simply 2.

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

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +2 -0
+#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS	\
+	FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 3)

and 3
not the FIELD_PREPified version

Sashiko correctly called out that it is field_prepped again at the callsite. As the mask includes lowest bit this is will 'work' but definitely isn't what you intended!

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

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +7 -0
new file mode 100644
index 000000000000..5d40f1ee53d6
 
+static bool inv_icm42607_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case INV_ICM42607_REG_MCLK_RDY ... INV_ICM42607_REG_INT_CONFIG:
+	case INV_ICM42607_REG_TEMP_DATA1 ... INV_ICM42607_REG_TMST_FSYNCL:
+	case INV_ICM42607_REG_APEX_DATA4 ... INV_ICM42607_REG_INTF_CONFIG1:
+	case INV_ICM42607_REG_INT_STATUS_DRDY ... INV_ICM42607_REG_FIFO_DATA:

Sashiko pointed out that the WOM_ registers, 4b to 4d are in the defines, but not readable or writeable which seems like an omission.

So far only matters for debug, but perhaps better to add them from the start.

Quoted 18 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +18 -0
+	case INV_ICM42607_REG_WHOAMI:
+		return true;
+	}
+
+	return false;
+}
+
+static bool inv_icm42607_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case INV_ICM42607_REG_DEVICE_CONFIG ... INV_ICM42607_REG_INT_CONFIG:
+	case INV_ICM42607_REG_PWR_MGMT0 ... INV_ICM42607_REG_INT_SOURCE4:
+	case INV_ICM42607_REG_INTF_CONFIG0 ... INV_ICM42607_REG_INTF_CONFIG1:
+		return true;
+	}
+
+	return false;
+}
#6 lore raw

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

Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 11 Jun 2026 15:26:00 -0500 Chris Morgan <[email protected]> wrote:

From: Chris Morgan <[email protected]>

Add the core component of a new inv_icm42607 driver. This includes a few setup functions and the full register definition in the header file, as well as the bits necessary to compile and probe the device when used on an i2c bus.

Quoted 8 lines

Hi Chris,

Given this is nearly ready to merge I took a look at Sashiko and seems it has found a few more things.

Other than those, looks good to me.

Jonathan

Thanks. I'm going to resubmit again probably tomorrow (even though it's merge window, again unless you don't want it).

I've removed the buffer from the main st struct, since it's really no longer needed given the lack of buffer support. Additionally I've removed the registers completely that sashiko was complaining about, since it only applies to the 42607c and isn't used by this driver right now anyway (only needed for APEX/WoM). I also altered the power management logic a bit to prevent it from delaying on shutdown, except for when the gyro changes state which the datasheet says is needed. If Sashiko continues to complain about power management for the temp sensor that can be ignored, as I've confirmed so long as both the gyro and accel are off the whole chip is "off". Along with making a change from 42607x to just 42607 in a few places those are the only major changes.

Chris

Quoted 62 lines
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +0 -0
new file mode 100644
index 000000000000..c85d3b74166f

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

Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +7 -0
 
+#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 this as simply 2.

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

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +2 -0
+#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS	\
+	FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 3)

and 3
not the FIELD_PREPified version

Sashiko correctly called out that it is field_prepped again at the callsite. As the mask includes lowest bit this is will 'work' but definitely isn't what you intended!

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

Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +7 -0
new file mode 100644
index 000000000000..5d40f1ee53d6
 
+static bool inv_icm42607_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case INV_ICM42607_REG_MCLK_RDY ... INV_ICM42607_REG_INT_CONFIG:
+	case INV_ICM42607_REG_TEMP_DATA1 ... INV_ICM42607_REG_TMST_FSYNCL:
+	case INV_ICM42607_REG_APEX_DATA4 ... INV_ICM42607_REG_INTF_CONFIG1:
+	case INV_ICM42607_REG_INT_STATUS_DRDY ... INV_ICM42607_REG_FIFO_DATA:

Sashiko pointed out that the WOM_ registers, 4b to 4d are in the defines, but not readable or writeable which seems like an omission.

So far only matters for debug, but perhaps better to add them from the start.

Quoted 18 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +18 -0
+	case INV_ICM42607_REG_WHOAMI:
+		return true;
+	}
+
+	return false;
+}
+
+static bool inv_icm42607_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case INV_ICM42607_REG_DEVICE_CONFIG ... INV_ICM42607_REG_INT_CONFIG:
+	case INV_ICM42607_REG_PWR_MGMT0 ... INV_ICM42607_REG_INT_SOURCE4:
+	case INV_ICM42607_REG_INTF_CONFIG0 ... INV_ICM42607_REG_INTF_CONFIG1:
+		return true;
+	}
+
+	return false;
+}
#7 lore raw

PATCH v12 4/9 iio: imu: inv_icm42607: Add SPI For icm42607

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add SPI driver support for InvenSense ICM-42607 devices.

---
 drivers/iio/imu/inv_icm42607/Kconfig          |  12 ++
 drivers/iio/imu/inv_icm42607/Makefile         |   3 +
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   | 107 ++++++++++++++++++
 3 files changed, 122 insertions(+)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c
drivers/iio/imu/inv_icm42607/Kconfig +12 -0
index 083c212087ab..23f461f57afc 100644
@@ -16,3 +16,15 @@ config INV_ICM42607_I2C
 
 	  This driver can be built as a module. The module will be called
 	  inv-icm42607-i2c.
+
+config INV_ICM42607_SPI
+	tristate "InvenSense ICM-42607 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 +3 -0
index 32046e2727d7..be109102e203 100644
@@ -5,3 +5,6 @@ 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_spi.c new file +107 -0

107 lines not on this page · raw message

Signature · 1 line
2.43.0
#8 lore raw

Re: [PATCH V12 4/9] iio: imu: inv_icm42607: Add SPI For icm42607

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Double application of FIELD_PREP macro on an already prepped value. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Double application of FIELD_PREP macro on an already prepped value. --

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

iio: imu: inv_icm42607: Add SPI For icm42607

Add SPI driver support for InvenSense ICM-42607 devices.

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

[ ... ]

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +0 -0
@@ -0,0 +1,107 @@

[ ... ]

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +4 -0
+static int inv_icm42607_spi_bus_setup(struct inv_icm42607_state *st)
+{
+	unsigned int val;
+	int ret;

[ ... ]

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

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +2 -0
+	val = FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK,
+			 INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS);

[Severity: Low]
Does this code apply the FIELD_PREP() macro twice?

INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS is already defined using FIELD_PREP() in the header. Applying it again here works only because the mask shift happens to be zero. If the mask had a non-zero shift, this double preparation would trigger a compile-time error.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +4 -0
+	return regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
+				  INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK,
+				  val);
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
#9 lore raw

Re: [PATCH V12 4/9] iio: imu: inv_icm42607: Add SPI For icm42607

In reply to Chris Morgan
Quoted 1 line · Chris Morgan

Add SPI driver support for InvenSense ICM-42607 devices.

...

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +7 -0
+#include <linux/bitfield.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>

...

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

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +7 -0
+static const struct spi_device_id inv_icm42607_spi_id_table[] = {
+	{
+		.name = "icm42607",
+		.driver_data = (kernel_ulong_t)&inv_icm42607_hw_data
+	}, {
+		.name = "icm42607p",
+		.driver_data = (kernel_ulong_t)&inv_icm42607p_hw_data

I believe I have commented on this already. But anyway, please leave a trailing comma for .driver_data assignments as well. Same for I²C driver.

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +3 -0
+	},
+	{ }
+};
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#10 lore raw

PATCH v12 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  | 131 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |   1 +
 .../iio/imu/inv_icm42607/inv_icm42607_spi.c   |   1 +
 4 files changed, 151 insertions(+)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +18 -0
index c85d3b74166f..28edc12d5373 100644
@@ -9,6 +9,7 @@
 #include <linux/bits.h>
 #include <linux/iio/iio.h>
 #include <linux/mutex.h>
+#include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/time.h>
@@ -105,11 +106,19 @@ struct inv_icm42607_hw {
 	u8 whoami;
 };
 
+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
  *  @hw:		Hardware specific data.
  *  @map:		regmap pointer.
  *  @vddio_supply:	I/O voltage regulator for the chip.
+ *  @suspended:		suspended sensors configuration.
+ *  @vddio_en:		I/O voltage status for runtime PM.
  *  @lock:		lock for serializing multiple registers access.
  *  @conf:		chip sensors configurations.
  *  @orientation:	sensor chip orientation relative to main hardware.
@@ -118,6 +127,8 @@ struct inv_icm42607_state {
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;
 	struct regulator *vddio_supply;
+	struct inv_icm42607_suspended suspended;
+	bool vddio_en;
 	struct mutex lock;
 	struct inv_icm42607_conf conf;
 	struct iio_mount_matrix orientation;
@@ -339,11 +350,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			(2 * USEC_PER_MSEC)
+
 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 +131 -0

178 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c +1 -0
index a1484f32c5d9..6e7310db5598 100644
@@ -85,6 +85,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 0c8deb1f33e2..e361a692a32f 100644
@@ -95,6 +95,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
#11 lore raw

Re: [PATCH V12 5/9] iio: imu: inv_icm42607: Add PM support for icm42607

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The driver incorrectly waits for the sensor's startup stabilization time (up to 30ms) when…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver incorrectly waits for the sensor's startup stabilization time (up to 30ms) when powering OFF the sensor, adding unnecessary latency to the runtime suspend path. --

commit f784e6048473d95cc560f84d8b8134e476cbc4ad
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 5d40f1ee53d6d..8073317088ec0 100644

[ ... ]

Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +5 -0
+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)
+{

[ ... ]

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

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +10 -0
+	if (gyro != oldgyro)
+		sleepval_ms = max(sleepval_ms, INV_ICM42607_GYRO_STARTUP_TIME_MS);
+
+	if (sleep_ms)
+		*sleep_ms = sleepval_ms;
+	else if (sleepval_ms)
+		fsleep(sleepval_ms * USEC_PER_MSEC);
+
+	return 0;
+}

[Severity: Medium]
Does this unintentionally add a delay when powering off the sensors?

While the temperature check correctly verifies if the sensor is being turned on (temp && !oldtemp), the accel and gyro checks only verify that the state has changed (accel != oldaccel).

When inv_icm42607_runtime_suspend() calls this function to power down the sensors:

[ ... ]

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

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

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

The accel != oldaccel and gyro != oldgyro conditions will evaluate to true if the sensors were previously active. This appears to cause the runtime suspend path to needlessly sleep for the startup stabilization time (up to 30ms) while powering down.

Could the delay logic be updated to only sleep when the new mode is not INV_ICM42607_SENSOR_MODE_OFF?

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

Re: [PATCH V12 5/9] iio: imu: inv_icm42607: Add PM support for icm42607

In reply to Chris Morgan
Quoted 1 line · Chris Morgan

Add power management support for the ICM42607 device driver.

...

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4 -0
+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 119

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +10 -0
+{
+	enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
+	enum inv_icm42607_sensor_mode oldgyro = st->conf.gyro.mode;
+	bool oldtemp = st->conf.temp_en;
+	unsigned int sleepval_ms;
+	unsigned int val;
+	int ret;
+
+	if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
+		return 0;

This validation seems weak, see below why.

Quoted 16 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +16 -0
+	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
+	val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
+	/*
+	 * Note that temp being enabled here doesn't affect PM since
+	 * per 10.25 of the datasheet the clock will be off by default
+	 * if both the gyro and accel modes are off.
+	 */
+	if (!temp)
+		val |= INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL;
+	ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
+	if (ret)
+		return ret;
+
+	st->conf.gyro.mode = gyro;
+	st->conf.accel.mode = accel;
+	st->conf.temp_en = temp;

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

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

Does it mean that it might be more than a single conditional to become true? If so, the last code wins, which is most likely buggy approach. Can you put a comment, and possible convert this to if-else-if, to clarify what's going on here?

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +7 -0
+	if (sleep_ms)
+		*sleep_ms = sleepval_ms;
+	else if (sleepval_ms)
+		fsleep(sleepval_ms * USEC_PER_MSEC);
+
+	return 0;
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#13 lore raw

Re: [PATCH V12 5/9] iio: imu: inv_icm42607: Add PM support for icm42607

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

Add power management support for the ICM42607 device driver.

...

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4 -0
+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 119

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +10 -0
+{
+	enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
+	enum inv_icm42607_sensor_mode oldgyro = st->conf.gyro.mode;
+	bool oldtemp = st->conf.temp_en;
+	unsigned int sleepval_ms;
+	unsigned int val;
+	int ret;
+
+	if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
+		return 0;

This validation seems weak, see below why.

Quoted 16 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +16 -0
+	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
+	val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
+	/*
+	 * Note that temp being enabled here doesn't affect PM since
+	 * per 10.25 of the datasheet the clock will be off by default
+	 * if both the gyro and accel modes are off.
+	 */
+	if (!temp)
+		val |= INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL;
+	ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
+	if (ret)
+		return ret;
+
+	st->conf.gyro.mode = gyro;
+	st->conf.accel.mode = accel;
+	st->conf.temp_en = temp;
Quoted 14 lines

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

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

Does it mean that it might be more than a single conditional to become true? If so, the last code wins, which is most likely buggy approach. Can you put a comment, and possible convert this to if-else-if, to clarify what's going on here?

I've changed this logic in the next version, but basically what needs to be done is ensure if any sensor changes from off to on we wait the startup delay amount of time for that sensor. If more than one sensor changes from off to on, we wait the maximum of the two sensors delay.

I'm going to change this instead to just (sensor && !oldsensor) for each of the three. Note that I also have to insert a shutdown delay for the gyro to ensure if the state goes from on to off the sensor is on a minimum of 45ms per the datasheet.

So I do expect it to be possible for more than one condition to be true here, and when it is I want to select the maximum value for the sleep. If the sleep_ms is specified however, we will use that value.

Quoted 14 lines
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +7 -0
+	if (sleep_ms)
+		*sleep_ms = sleepval_ms;
+	else if (sleepval_ms)
+		fsleep(sleepval_ms * USEC_PER_MSEC);
+
+	return 0;
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Thank you,
Chris

#14 lore raw

PATCH v12 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  | 76 +++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.h  | 37 +++++++++
 5 files changed, 137 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 be109102e203..c04953ed42ce 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 28edc12d5373..53dd23509a53 100644
@@ -114,6 +114,7 @@ struct inv_icm42607_suspended {
 
 /**
  *  struct inv_icm42607_state - driver state variables
+ *  @buffer:		data transfer buffer aligned for DMA.
  *  @hw:		Hardware specific data.
  *  @map:		regmap pointer.
  *  @vddio_supply:	I/O voltage regulator for the chip.
@@ -124,6 +125,7 @@ struct inv_icm42607_suspended {
  *  @orientation:	sensor chip orientation relative to main hardware.
  */
 struct inv_icm42607_state {
+	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;
 	struct regulator *vddio_supply;
@@ -363,6 +365,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 8073317088ec..549fe248e600 100644
@@ -153,6 +153,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 +76 -0

76 lines not on this page · raw message

drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h new file +37 -0
new file mode 100644
index 000000000000..cb7b460ffb44
@@ -0,0 +1,37 @@
+/* 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),			\
+	.info_mask_shared_by_all =				\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
+	.info_mask_shared_by_all_available =			\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
+	.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
#15 lore raw

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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The DMA buffer `buffer` is placed at the beginning of `struct inv_icm42607_state`, sharing a…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The DMA buffer buffer is placed at the beginning of struct inv_icm42607_state, sharing a cacheline with subsequent members like hw, map, and lock. This can lead to memory corruption and deadlocks during DMA operations.
- [High] The temperature sensor is permanently left enabled after a single raw read, continuously wasting power and adding unnecessary latency to subsequent runtime resumes.
--

commit 18abfa4e34a17ed9cef9a8a6e5d8c222ba930a49
Author: Chris Morgan <[email protected]>

iio: imu: inv_icm42607: Add Temp Support in icm42607

Add functions for reading temperature sensor data.

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h +0 -0
index 35c9210ca86fe..5416754907913 100644

[ ... ]

drivers/iio/imu/inv_icm42607/inv_icm42607.h struct inv_icm42607_suspended { · line 130

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h +1 -0
@@ -124,6 +125,7 @@ struct inv_icm42607_suspended {
  *  @orientation:	sensor chip orientation relative to main hardware.
  */
 struct inv_icm42607_state {
+	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;

[Severity: High]
Does this DMA buffer alignment ensure the end of the buffer is also padded to the cacheline boundary?

Since the buffer is only 6 bytes, fields following it like hw, map, and the mutex lock will share the same cacheline. When performing a DMA transfer, the controller might invalidate the entire cacheline, which could destroy concurrent writes to the lock or other shared state and lead to memory corruption or deadlocks.

Usually, buffers used for DMA need to occupy their own cacheline. Placing them at the end of the struct with __aligned(IIO_DMA_MINALIGN) can prevent this cacheline sharing.

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607.h +0 -0
 	struct regulator *vddio_supply;

[ ... ]

Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +0 -0
new file mode 100644
index 0000000000000..b259fc9c9fd29
@@ -0,0 +1,76 @@

[ ... ]

Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +2 -0
+static int inv_icm42607_temp_read(struct inv_icm42607_state *st, s16 *temp)
+{

[ ... ]

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

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

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

[Severity: High]
Will the temperature sensor remain permanently enabled after this read completes?

Since inv_icm42607_set_temp_conf(st, true, NULL) is called but never disabled before returning, st->conf.temp_en will stay true.

Because the driver saves and restores this state during runtime PM suspend and resume, the temperature sensor will stay powered on whenever the chip is active, consuming extra power.

Additionally, this leaked state might force an artificial 77ms sleep delay on every subsequent runtime PM resume. When the device resumes, inv_icm42607_set_pwr_mgmt0() will see the sensor transitioning from off to on:

inv_icm42607_set_pwr_mgmt0() {
...

	st->conf.temp_en = temp;

	sleepval_ms = 0;
	if (temp && !oldtemp)
		sleepval_ms = max(sleepval_ms, INV_ICM42607_TEMP_STARTUP_TIME_MS);

...
}

Should the temperature sensor configuration be disabled before returning from inv_icm42607_temp_read()?

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

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

In reply to Chris Morgan
Quoted 5 lines · On Thu, 11 Jun 2026 15:26:03 -0500 Chris Morgan <[email protected]> wrote:

From: Chris Morgan <[email protected]>

Add functions for reading temperature sensor data.

The buffer isn't safe for use for DMA. See below.
I'm not sure I'd have noticed. This used to be a common bug but not seen it that often recently for some reason!

Jonathan

Quoted 35 lines
---
 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  | 76 +++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_temp.h  | 37 +++++++++
 5 files changed, 137 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 be109102e203..c04953ed42ce 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 +1 -0
index 28edc12d5373..53dd23509a53 100644
@@ -114,6 +114,7 @@ struct inv_icm42607_suspended {
 
 /**
  *  struct inv_icm42607_state - driver state variables
+ *  @buffer:		data transfer buffer aligned for DMA.
  *  @hw:		Hardware specific data.
  *  @map:		regmap pointer.
  *  @vddio_supply:	I/O voltage regulator for the chip.

drivers/iio/imu/inv_icm42607/inv_icm42607.h struct inv_icm42607_suspended { · line 128

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h +1 -0
@@ -124,6 +125,7 @@ struct inv_icm42607_suspended {
  *  @orientation:	sensor chip orientation relative to main hardware.
  */
 struct inv_icm42607_state {
+	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);

Look in more depth at what this does. Key thing is there is a reason
this should always be at the end of the iio_priv() structure.

(Sashiko caught this btw)

Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h +0 -0
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;
 	struct regulator *vddio_supply;
@@ -363,6 +365,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;
 
#17 lore raw

PATCH v12 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   |  34 ++
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 376 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  61 +++
 4 files changed, 472 insertions(+)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
index c04953ed42ce..d74b23b1e1be 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 +34 -0
index 53dd23509a53..e236e5aa61cb 100644
@@ -86,6 +86,17 @@ enum inv_icm42607_filter_bw {
 	INV_ICM42607_FILTER_BW_NB
 };
 
+/* Low-Power mode sensor data filter (averaging) */
+enum inv_icm42607_filter_avg {
+	INV_ICM42607_FILTER_AVG_2X = 0,
+	INV_ICM42607_FILTER_AVG_4X = 1,
+	INV_ICM42607_FILTER_AVG_8X = 2,
+	INV_ICM42607_FILTER_AVG_16X = 3,
+	INV_ICM42607_FILTER_AVG_32X = 4,
+	INV_ICM42607_FILTER_AVG_64X = 5,
+	/* values 6 and 7 also correspond to 64x. */
+};
+
 /* Signed so that negative values can signify an invalid condition. */
 struct inv_icm42607_sensor_conf {
 	int mode;
@@ -93,6 +104,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;
@@ -117,6 +129,7 @@ struct inv_icm42607_suspended {
  *  @buffer:		data transfer buffer aligned for DMA.
  *  @hw:		Hardware specific data.
  *  @map:		regmap pointer.
+ *  @indio_accel:	accelerometer IIO device.
  *  @vddio_supply:	I/O voltage regulator for the chip.
  *  @suspended:		suspended sensors configuration.
  *  @vddio_en:		I/O voltage status for runtime PM.
@@ -128,6 +141,7 @@ struct inv_icm42607_state {
 	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;
+	struct iio_dev *indio_accel;
 	struct regulator *vddio_supply;
 	struct inv_icm42607_suspended suspended;
 	bool vddio_en;
@@ -136,6 +150,16 @@ struct inv_icm42607_state {
 	struct iio_mount_matrix orientation;
 };
 
+/**
+ * struct inv_icm42607_sensor_state - sensor state variables
+ * @power_mode:		sensor requested power mode (for common frequencies)
+ * @filter:		sensor filter.
+ */
+struct inv_icm42607_sensor_state {
+	enum inv_icm42607_sensor_mode power_mode;
+	int filter;
+};
+
 /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
 
 /* Register Map for User Bank 0 */
@@ -365,6 +389,14 @@ extern const struct inv_icm42607_hw inv_icm42607_hw_data;
 extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
 extern const struct dev_pm_ops inv_icm42607_pm_ops;
 
+const struct iio_mount_matrix *
+inv_icm42607_get_mount_matrix(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan);
+
+int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
+				struct inv_icm42607_sensor_conf *conf,
+				unsigned int *sleep_ms);
+
 int inv_icm42607_set_temp_conf(struct inv_icm42607_state *st, bool enable,
 			       unsigned int *sleep_ms);
 
@@ -372,4 +404,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 +376 -0

376 lines not on this page · raw message

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

79 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer
#18 lore raw

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

Quoted 5 lines · On Thu, 11 Jun 2026 15:26:04 -0500 Chris Morgan <[email protected]> wrote:

From: Chris Morgan <[email protected]>

Add icm42607 accelerometer sensor for icm42607.

Really trivial stuff that I'd not have mentioned if you weren't doing a v13 for other reasons.

Quoted 32 lines
---
 drivers/iio/imu/inv_icm42607/Makefile         |   1 +
 drivers/iio/imu/inv_icm42607/inv_icm42607.h   |  34 ++
 .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 376 ++++++++++++++++++
 .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  61 +++
 4 files changed, 472 insertions(+)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +18 -0
new file mode 100644
index 000000000000..5e260ddad641
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2026 InvenSense, Inc.
+ */
+
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include "inv_icm42607.h"
+#include "inv_icm42607_temp.h"
+
+#define INV_ICM42607_ACCEL_CHAN(_modifier, _index, _ext_info)		\
+{									\
+	.type = IIO_ACCEL,						\
+	.modified = 1,							\
+	.channel2 = _modifier,						\

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +10 -0
+	.info_mask_separate =						\
+		BIT(IIO_CHAN_INFO_RAW),					\
+	.info_mask_shared_by_type =					\
+		BIT(IIO_CHAN_INFO_SCALE),				\
+	.info_mask_shared_by_type_available =				\
+		BIT(IIO_CHAN_INFO_SCALE),				\
+	.info_mask_shared_by_all =					\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.info_mask_shared_by_all_available =				\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\

Maybe for these, just go a bit long on line length (so 81 chars). I don't really care either way!

Quoted 31 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +29 -0
+	.scan_index = _index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_BE,					\
+	},								\
+	.ext_info = _ext_info,						\
+}
 
 
+
+static int inv_icm42607_accel_write_odr(struct iio_dev *indio_dev,
+					int val, int val2)
+{
+	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
+	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
+	struct device *dev = regmap_get_device(st->map);
+	unsigned int idx;
+	int ret;
+
+	for (idx = 5; idx < ARRAY_SIZE(inv_icm42607_accel_odr); ++idx) {
+		if (val == inv_icm42607_accel_odr[idx][0] &&
+		    val2 == inv_icm42607_accel_odr[idx][1])
+			break;
+	}
+	if (idx >= ARRAY_SIZE(inv_icm42607_accel_odr))
+		return -EINVAL;
+
+	conf.odr = idx;
+

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

Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.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);
+
+	ret = inv_icm42607_set_accel_conf(st, &conf, NULL);
+	if (ret)
+		return ret;

return inv_icm....

Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +3 -0
+
+	return 0;
+}
#19 lore raw

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

In reply to Chris Morgan
Quoted 1 line · Chris Morgan

Add icm42607 accelerometer sensor for icm42607.

...

Quoted 1 line

+++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

Please, please, use IWYU! So many headers are missing... (Same comment to all files in this series.)

+ array_size.h
+ bits.h // BIT()
+ cleanup.h // guard()()
+ device/devres.h // devm_kasprintf()
+ err.h // -EINVAL, IS_ERR()

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

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +4 -0
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>

+ types.h // s16, __be16

Also you need to have

asm/byteorder.h // be16_to_cpup()

...

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

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +11 -0
+static int inv_icm42607_accel_read_odr(struct inv_icm42607_state *st,
+				       int *val, int *val2)
+{
+	unsigned int odr;
+	unsigned int i;
+
+	guard(mutex)(&st->lock);
+
+	odr = st->conf.accel.odr;
+
+	for (i = 5; i < ARRAY_SIZE(inv_icm42607_accel_odr); ++i) {

Why pre-increment? Same for all other cases.

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +11 -0
+		if (i == odr)
+			break;
+	}
+	if (i >= ARRAY_SIZE(inv_icm42607_accel_odr))
+		return -EINVAL;
+
+	*val = inv_icm42607_accel_odr[i][0];
+	*val2 = inv_icm42607_accel_odr[i][1];
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#20 lore raw

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

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

Add icm42607 accelerometer sensor for icm42607.

...

Quoted 1 line

+++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

Please, please, use IWYU! So many headers are missing... (Same comment to all files in this series.)

+ array_size.h
+ bits.h // BIT()
+ cleanup.h // guard()()
+ device/devres.h // devm_kasprintf()
+ err.h // -EINVAL, IS_ERR()

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

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +4 -0
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>

+ types.h // s16, __be16

Quoted 3 lines

Also you need to have

asm/byteorder.h // be16_to_cpup()

How are you running IWYU against the builds? So far I've tried but I can't seem to get it to run properly.

Quoted 16 lines

...

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

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +11 -0
+static int inv_icm42607_accel_read_odr(struct inv_icm42607_state *st,
+				       int *val, int *val2)
+{
+	unsigned int odr;
+	unsigned int i;
+
+	guard(mutex)(&st->lock);
+
+	odr = st->conf.accel.odr;
+
+	for (i = 5; i < ARRAY_SIZE(inv_icm42607_accel_odr); ++i) {

Why pre-increment? Same for all other cases.

The register starts at 5 and all values below 5 are invalid. Starting this increment at 5 ensures we don't expose invalid values to userspace.

Quoted 18 lines
Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +11 -0
+		if (i == odr)
+			break;
+	}
+	if (i >= ARRAY_SIZE(inv_icm42607_accel_odr))
+		return -EINVAL;
+
+	*val = inv_icm42607_accel_odr[i][0];
+	*val2 = inv_icm42607_accel_odr[i][1];
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Thank you,
Chris

#21 lore raw

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

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

On Thu, Jun 11, 2026 at 03:26:04PM -0500, Chris Morgan wrote:

...

Quoted 16 lines
Quoted 16 lines

Please, please, use IWYU! So many headers are missing... (Same comment to all files in this series.)

+ array_size.h
+ bits.h // BIT()
+ cleanup.h // guard()()
+ device/devres.h // devm_kasprintf()
+ err.h // -EINVAL, IS_ERR()

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

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +4 -0
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>

+ types.h // s16, __be16

Quoted 6 lines
Quoted 3 lines

Also you need to have

asm/byteorder.h // be16_to_cpup()

How are you running IWYU against the builds? So far I've tried but I can't seem to get it to run properly.

Sorry, I meant "use IWYU principle". I don't run the tool, I just looked into the code.

...

Quoted 7 lines
Quoted 3 lines

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

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +1 -0
+	for (i = 5; i < ARRAY_SIZE(inv_icm42607_accel_odr); ++i) {

Why pre-increment? Same for all other cases.

The register starts at 5 and all values below 5 are invalid. Starting this increment at 5 ensures we don't expose invalid values to userspace.

It doesn't explain pre-increment. Post-increment should work as is.

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +3 -0
+		if (i == odr)
+			break;
+	}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#22 lore raw

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

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

On Thu, Jun 11, 2026 at 03:26:04PM -0500, Chris Morgan wrote:

...

Quoted 18 lines
Quoted 18 lines

Please, please, use IWYU! So many headers are missing... (Same comment to all files in this series.)

+ array_size.h
+ bits.h // BIT()
+ cleanup.h // guard()()
+ device/devres.h // devm_kasprintf()
+ err.h // -EINVAL, IS_ERR()

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

Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +4 -0
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>

+ types.h // s16, __be16

Also you need to have

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

asm/byteorder.h // be16_to_cpup()

How are you running IWYU against the builds? So far I've tried but I can't seem to get it to run properly.

Sorry, I meant "use IWYU principle". I don't run the tool, I just looked into the code.

I've done what I can then, I have added these headers where I am using them. I'm not going to add asm/byteorder.h though because I am dropping that in the next revision and replacing it with get_unaligned_be16() to further simplify things. Note that I am adding linux/unaligned.h for the files where I use that function.

The additional headers will be added in the next revision. If you see any other obvious ones missing let me know but for now I *think* it's correct.

Quoted 7 lines

...

Quoted 4 lines
Quoted 3 lines

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

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +1 -0
+	for (i = 5; i < ARRAY_SIZE(inv_icm42607_accel_odr); ++i) {

Why pre-increment? Same for all other cases.

Quoted 5 lines
Quoted 3 lines

The register starts at 5 and all values below 5 are invalid. Starting this increment at 5 ensures we don't expose invalid values to userspace.

It doesn't explain pre-increment. Post-increment should work as is.

The array this references starts at 5, because those correspond to the values written to the odr register. That said, I do see a bug because the odr register is from highest to smallest and this array is backwards in the accel and gyro code. I'll fix that.

Quoted 10 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +3 -0
+		if (i == odr)
+			break;
+	}
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Thank you,
Chris

#23 lore raw

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

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

On Thu, Jun 11, 2026 at 03:26:04PM -0500, Chris Morgan wrote:

...

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines

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

Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +1 -0
+	for (i = 5; i < ARRAY_SIZE(inv_icm42607_accel_odr); ++i) {

Why pre-increment? Same for all other cases.

The register starts at 5 and all values below 5 are invalid. Starting this increment at 5 ensures we don't expose invalid values to userspace.

Quoted 6 lines
Quoted 1 line

It doesn't explain pre-increment. Post-increment should work as is.

The array this references starts at 5, because those correspond to the values written to the odr register. That said, I do see a bug because the odr register is from highest to smallest and this array is backwards in the accel and gyro code. I'll fix that.

Good we even managed to find a bug, but what I initially pointed out is simply as ++i versus i++. When I see ++i, it rises a flag: "this code is special", but looking closer, I see nothing special about it.

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +3 -0
+		if (i == odr)
+			break;
+	}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#24 lore raw

PATCH v12 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  | 370 ++++++++++++++++++
 4 files changed, 427 insertions(+)
 create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
index d74b23b1e1be..7b907e019601 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 e236e5aa61cb..203271b9ef7a 100644
@@ -130,6 +130,7 @@ struct inv_icm42607_suspended {
  *  @hw:		Hardware specific data.
  *  @map:		regmap pointer.
  *  @indio_accel:	accelerometer IIO device.
+ *  @indio_gyro:	gyroscope IIO device.
  *  @vddio_supply:	I/O voltage regulator for the chip.
  *  @suspended:		suspended sensors configuration.
  *  @vddio_en:		I/O voltage status for runtime PM.
@@ -142,6 +143,7 @@ struct inv_icm42607_state {
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;
 	struct iio_dev *indio_accel;
+	struct iio_dev *indio_gyro;
 	struct regulator *vddio_supply;
 	struct inv_icm42607_suspended suspended;
 	bool vddio_en;
@@ -397,6 +399,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);
 
@@ -404,6 +410,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 +370 -0

370 lines not on this page · raw message

Signature · 1 line
2.43.0
#25 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] DMA cacheline sharing vulnerability due to `buffer` placement without trailing padding. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] DMA cacheline sharing vulnerability due to buffer placement without trailing padding. --

commit ffab596572628e0ee5392f57ffbac77ad2c58f08
Author: Chris Morgan <[email protected]>
Subject: iio: imu: inv_icm42607: Add Gyroscope to icm42607

This commit adds gyroscope functions and IIO device registration to the inv_icm42607 driver.

drivers/iio/imu/inv_icm42607/inv_icm42607.h struct inv_icm42607_state { · line 148

Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h +1 -0
index 91ac75bb719e2..ddb354fc57abe 100644
@@ -142,6 +143,7 @@ struct inv_icm42607_state {
 	const struct inv_icm42607_hw *hw;
 	struct regmap *map;
 	struct iio_dev *indio_accel;
+	struct iio_dev *indio_gyro;
 	struct regulator *vddio_supply;
 	struct inv_icm42607_suspended suspended;

[Severity: High]
Could the structure layout here cause a DMA cacheline sharing regression?

The DMA-mapped buffer array is defined at the beginning of struct inv_icm42607_state. While its start is aligned to IIO_DMA_MINALIGN, its size is only 6 bytes, and no padding separates it from subsequent members. Consequently, the buffer shares a CPU cacheline with other struct members (such as hw, map, and lock), depending on the architecture's cacheline size.

When regmap_bulk_read() triggers an SPI DMA transfer, the SPI core maps the buffer with DMA_FROM_DEVICE. If another thread attempts to acquire st->lock while the DMA transfer is ongoing (for instance, reading from the other sensor exposed by the driver), it will modify the mutex wait queue, dirtying the shared cacheline in the CPU cache.

When the DMA transfer completes, dma_unmap_single() invalidates the cacheline, silently discarding the concurrent writes to the mutex. This can corrupt the mutex state and cause blocked threads to hang forever, leading to a system deadlock.

Would it be safer to move the DMA buffer array to the end of the structure so that the IIO_DMA_MINALIGN alignment guarantees it occupies its own cacheline without trailing members sharing the space?

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

PATCH v12 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 8d906ab02c5f..b770bfd5268d 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