Add Invensense ICM42607
Subsystems (4)
v10 cover Add Invensense ICM42607
From: Chris Morgan <[email protected]>
Add support for the ICM42607 IMU. This sensor shares the same functionality but a different register layout with the existing ICM42600.
This driver should work with the ICM42607 and ICM42607P over both I2C and SPI, however only the ICM42607P over I2C could be tested.
Changes Since V1:
- Instead of creating a new driver, merged with the existing inv_icm42600 driver. This necessitated adding some code to the existing driver to permit using a different register layout for the same functionality.
- Split changes up a bit more to decrease the size of the individual patches. Note that patch 0004 is still pretty hefty; if I need to split further I may need to create some temporary stub functions.
- Used guard() and PM_RUNTIME_ACQUIRE_AUTOSUSPEND() on the new functions per Jonathan's recommendations.
Changes Since V2:
- Went back to using a new driver on advice from Invensense engineer.
- Further split changes up into smaller chunks of functionality. Note still that the largest patch is approximately 900 lines, and that while the driver compiles cleanly at each commit it is not able to drive the hardware until the commit that adds the Interrupt (as it also adds the Makefile).
- Change the error to a warning when the devicetree binding does not match the hardware ID.
- Dropped the ack on the devicetree bindings, as I am creating a new file (for a new driver) instead of modifying the existing one.
Changes Since V3:
- Numerous small fixes (too many to list here). Thank you to everyone who provided feedback.
- Split power management additions into an additional commit to break things up further.
- Consolidated devicetree documentation in existing invensense,icm42600.yaml file.
- Removed most of the FIELD_PREP from header file to c files to make code easier to read.
- Changed scale values to 2D arrays for Gyro and Accelerometer.
- Removed IIO_CHAN_INFO_CALIBBIAS attribute.
Changes Since V4:
- Additional numerous small fixes, thank you again for all the feedback.
- Dropped power control API and instead run device in low noise mode.
- Split devicetree bindings into two distinct changes.
- Reordered adding of enums and structs to main header file so that they are only brought in when needed.
- Stopped using enum for driver data and instead am using pointer to device specific driver data.
Changes Since V5:
- Corrected use of "dev_warn_probe" to just "dev_warn".
- Fixed some return scenarios which would unconditionally return 0 when an error was present.
- Corrected use of max() to min() for bounds checking. max() was incorrect.
- Fixed using "st->conf.accel.odr" in the gyroscope function. It should have been "st->conf.gyro.odr" which it now is.
- Additional small fixes suggested by "sashiko.dev".
- Added a regmap cache. I used the datasheet to try and determine which registers might change without explicit writes.
Changes Since V6:
- Corrected additional errors identified by sashiko.dev, mostly fixing potential deadlocks, missing calls for pm runtime, and potential overflow issues.
Changes Since V7:
- Dropped Wake on Movement patches, since some of the functionality was only available for a device on which I cannot test.
- Dropped support for SPI 3-Wire mode, since it complicated the bus setup (and I lack the hardware to test such features anyway).
- Fixed a few additional bugs identified by sashiko.dev bot.
Changes Since V8:
- Added back IRQ dropped accidentally when dropping wake on movement patches.
- Dropped "Reviewed-By" tag on patch 2 because of substantial changes made to devicetree binding documentation.
- Additional small fixes as suggested.
Changes Since V9:
- Removed interrupts (and buffers) from the driver. I previously was unable to detect deadlocks because it turns out my IRQ was not even wired correctly in my device.
- Updated devicetree binding commits to make interrupts optional for users of the icm42607 driver.
Chris Morgan (9):
dt-bindings: iio: imu: icm42600: Add mount-matrix to icm42600
dt-bindings: iio: imu: icm42600: Add icm42607 binding
iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
iio: imu: inv_icm42607: Add I2C and SPI For icm42607
iio: imu: inv_icm42607: Add PM support for icm42607
iio: imu: inv_icm42607: Add Temp Support in icm42607
iio: imu: inv_icm42607: Add Accelerometer for icm42607
iio: imu: inv_icm42607: Add Gyroscope to icm42607
arm64: dts: rockchip: Add icm42607p IMU for RG-DS
.../bindings/iio/imu/invensense,icm42600.yaml | 20 +- .../dts/rockchip/rk3568-anbernic-rg-ds.dts | 8 +- drivers/iio/imu/Kconfig | 1 + drivers/iio/imu/Makefile | 1 + drivers/iio/imu/inv_icm42607/Kconfig | 30 + drivers/iio/imu/inv_icm42607/Makefile | 13 + drivers/iio/imu/inv_icm42607/inv_icm42607.h | 421 ++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 465 +++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 539 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_gyro.c | 437 ++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 95 +++ .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 107 ++++ .../iio/imu/inv_icm42607/inv_icm42607_temp.c | 82 +++ .../iio/imu/inv_icm42607/inv_icm42607_temp.h | 33 ++ 14 files changed, 2250 insertions(+), 2 deletions(-) create mode 100644 drivers/iio/imu/inv_icm42607/Kconfig create mode 100644 drivers/iio/imu/inv_icm42607/Makefile create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h
Signature · 1 line
2.43.0
PATCH v10 1/9 dt-bindings: iio: imu: icm42600: Add mount-matrix to icm42600
From: Chris Morgan <[email protected]>
Add mount-matrix attribute to schema. This attribute has been supported since the first revision of this driver, but was not documented.
--- .../devicetree/bindings/iio/imu/invensense,icm42600.yaml | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml +2 -0
@@ -53,6 +53,8 @@ properties: drive-open-drain: type: boolean + mount-matrix: true + vdd-supply: description: Regulator that provides power to the sensor
Signature · 1 line
2.43.0
PATCH v10 2/9 dt-bindings: iio: imu: icm42600: Add icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add the ICM42607 and ICM42607P inertial measurement unit.
This device is functionally very similar to the icm42600 series with a very different register layout. The driver does not require an interrupt for these specific chip revisions.
--- .../bindings/iio/imu/invensense,icm42600.yaml | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml +17 -1
@@ -30,6 +30,8 @@ properties: - invensense,icm42600 - invensense,icm42602 - invensense,icm42605 + - invensense,icm42607 + - invensense,icm42607p - invensense,icm42622 - invensense,icm42631 - invensense,icm42686
@@ -67,10 +69,24 @@ properties: required: - compatible - reg - - interrupts allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml# + - if: + properties: + compatible: + contains: + enum: + - invensense,icm42600 + - invensense,icm42602 + - invensense,icm42605 + - invensense,icm42622 + - invensense,icm42631 + - invensense,icm42686 + - invensense,icm42688 + then: + required: + - interrupts unevaluatedProperties: false
Signature · 1 line
2.43.0
Re: [PATCH V10 2/9] dt-bindings: iio: imu: icm42600: Add icm42607
Quoted 4 lines · Chris Morgan
From: Chris Morgan <[email protected]>
Add the ICM42607 and ICM42607P inertial measurement unit.
Quoted 8 lines
This device is functionally very similar to the icm42600 series with a very different register layout. The driver does not require an interrupt for these specific chip revisions.
Signed-off-by Chris Morgan <[email protected]>--- .../bindings/iio/imu/invensense,icm42600.yaml | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Best regards,
Krzysztof
PATCH v10 3/9 iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add the core component of a new inv_icm42607 driver. This includes a few setup functions and the full register definition in the header file.
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 334 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 200 +++++++++++ 2 files changed, 534 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +334 -0
@@ -0,0 +1,334 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#ifndef INV_ICM42607_H_ +#define INV_ICM42607_H_ + +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <linux/iio/iio.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> + +enum inv_icm42607_sensor_mode { +INV_ICM42607_SENSOR_MODE_OFF , +INV_ICM42607_SENSOR_MODE_STANDBY , +INV_ICM42607_SENSOR_MODE_LOW_POWER , +INV_ICM42607_SENSOR_MODE_LOW_NOISE , +INV_ICM42607_SENSOR_MODE_NB +}; + +/* gyroscope fullscale values */ +enum inv_icm42607_gyro_fs { +INV_ICM42607_GYRO_FS_2000DPS , +INV_ICM42607_GYRO_FS_1000DPS , +INV_ICM42607_GYRO_FS_500DPS , +INV_ICM42607_GYRO_FS_250DPS , +INV_ICM42607_GYRO_FS_NB +}; + +/* accelerometer fullscale values */ +enum inv_icm42607_accel_fs { +INV_ICM42607_ACCEL_FS_16G , +INV_ICM42607_ACCEL_FS_8G , +INV_ICM42607_ACCEL_FS_4G , +INV_ICM42607_ACCEL_FS_2G , +INV_ICM42607_ACCEL_FS_NB +}; + +/* ODR values */ +enum inv_icm42607_odr { +INV_ICM42607_ODR_1600HZ =5 , +INV_ICM42607_ODR_800HZ , +INV_ICM42607_ODR_400HZ , +INV_ICM42607_ODR_200HZ , +INV_ICM42607_ODR_100HZ , +INV_ICM42607_ODR_50HZ , +INV_ICM42607_ODR_25HZ , +INV_ICM42607_ODR_12_5HZ , +INV_ICM42607_ODR_6_25HZ_LP , +INV_ICM42607_ODR_3_125HZ_LP , +INV_ICM42607_ODR_1_5625HZ_LP , +INV_ICM42607_ODR_NB +}; + +enum inv_icm42607_filter_bw { +/* Low-Noise mode sensor data filter (bandwidth) */ +INV_ICM42607_FILTER_BYPASS , +INV_ICM42607_FILTER_BW_180HZ , +INV_ICM42607_FILTER_BW_121HZ , +INV_ICM42607_FILTER_BW_73HZ , +INV_ICM42607_FILTER_BW_53HZ , +INV_ICM42607_FILTER_BW_34HZ , +INV_ICM42607_FILTER_BW_25HZ , +INV_ICM42607_FILTER_BW_16HZ , +INV_ICM42607_FILTER_BW_NB +}; + +struct inv_icm42607_sensor_conf { +int mode; +int fs; +int odr; +int filter; +}; + +struct inv_icm42607_conf { +struct inv_icm42607_sensor_conf gyro; +struct inv_icm42607_sensor_conf accel; +bool temp_en; +}; + +struct inv_icm42607_hw { +uint8_t whoami; +const char *name; +const struct inv_icm42607_conf *conf; +}; + +/** +* struct inv_icm42607_state - driver state variables +* @lock: lock for serializing multiple registers access. +* @hw: Hardware specific data. +* @map: regmap pointer. +* @vddio_supply: I/O voltage regulator for the chip. +* @orientation: sensor chip orientation relative to main hardware. +* @conf: chip sensors configurations. +*/ +struct inv_icm42607_state { +struct mutex lock; +const struct inv_icm42607_hw *hw; +struct regmap *map; +struct regulator *vddio_supply; +struct iio_mount_matrix orientation; +struct inv_icm42607_conf conf; +}; + +/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */ + +/* Register Map for User Bank 0 */ +#define INV_ICM42607_REG_MCLK_RDY 0x00 + +#define INV_ICM42607_REG_DEVICE_CONFIG 0x01 +#define INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE BIT(2) +#define INV_ICM42607_DEVICE_CONFIG_SPI_MODE BIT(0) + +#define INV_ICM42607_REG_SIGNAL_PATH_RESET 0x02 +#define INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET BIT(4) +#define INV_ICM42607_SIGNAL_PATH_RESET_FIFO_FLUSH BIT(2) + +#define INV_ICM42607_REG_DRIVE_CONFIG1 0x03 +#define INV_ICM42607_DRIVE_CONFIG1_I3C_DDR_MASK GENMASK(5, 3) +#define INV_ICM42607_DRIVE_CONFIG1_I3C_SDR_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_DRIVE_CONFIG2 0x04 +#define INV_ICM42607_DRIVE_CONFIG2_I2C_MASK GENMASK(5, 3) +#define INV_ICM42607_DRIVE_CONFIG2_ALL_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_DRIVE_CONFIG3 0x05 +#define INV_ICM42607_DRIVE_CONFIG3_SPI_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_INT_CONFIG 0x06 +#define INV_ICM42607_INT_CONFIG_INT2_LATCHED BIT(5) +#define INV_ICM42607_INT_CONFIG_INT2_PUSH_PULL BIT(4) +#define INV_ICM42607_INT_CONFIG_INT2_ACTIVE_HIGH BIT(3) +#define INV_ICM42607_INT_CONFIG_INT2_ACTIVE_LOW 0x00 +#define INV_ICM42607_INT_CONFIG_INT1_LATCHED BIT(2) +#define INV_ICM42607_INT_CONFIG_INT1_PUSH_PULL BIT(1) +#define INV_ICM42607_INT_CONFIG_INT1_ACTIVE_HIGH BIT(0) +#define INV_ICM42607_INT_CONFIG_INT1_ACTIVE_LOW 0x00 + +/* all sensor data are 16 bits (2 registers wide) in big-endian */ +#define INV_ICM42607_REG_TEMP_DATA1 0x09 +#define INV_ICM42607_REG_TEMP_DATA0 0x0A +#define INV_ICM42607_REG_ACCEL_DATA_X1 0x0B +#define INV_ICM42607_REG_ACCEL_DATA_X0 0x0C +#define INV_ICM42607_REG_ACCEL_DATA_Y1 0x0D +#define INV_ICM42607_REG_ACCEL_DATA_Y0 0x0E +#define INV_ICM42607_REG_ACCEL_DATA_Z1 0x0F +#define INV_ICM42607_REG_ACCEL_DATA_Z0 0x10 +#define INV_ICM42607_REG_GYRO_DATA_X1 0x11 +#define INV_ICM42607_REG_GYRO_DATA_X0 0x12 +#define INV_ICM42607_REG_GYRO_DATA_Y1 0x13 +#define INV_ICM42607_REG_GYRO_DATA_Y0 0x14 +#define INV_ICM42607_REG_GYRO_DATA_Z1 0x15 +#define INV_ICM42607_REG_GYRO_DATA_Z0 0x16 +#define INV_ICM42607_DATA_INVALID -32768 + +#define INV_ICM42607_REG_TMST_FSYNCH 0x17 +#define INV_ICM42607_REG_TMST_FSYNCL 0x18 + +/* APEX Data Registers */ +#define INV_ICM42607_REG_APEX_DATA0 0x31 +#define INV_ICM42607_REG_APEX_DATA1 0x32 +#define INV_ICM42607_REG_APEX_DATA2 0x33 +#define INV_ICM42607_REG_APEX_DATA3 0x34 +#define INV_ICM42607_REG_APEX_DATA4 0x1D +#define INV_ICM42607_REG_APEX_DATA5 0x1E + +#define INV_ICM42607_REG_PWR_MGMT0 0x1F +#define INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL BIT(7) +#define INV_ICM42607_PWR_MGMT0_IDLE BIT(4) +#define INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK GENMASK(3, 2) +#define INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK GENMASK(1, 0) + +#define INV_ICM42607_REG_GYRO_CONFIG0 0x20 +#define INV_ICM42607_GYRO_CONFIG0_FS_SEL_MASK GENMASK(6, 5) +#define INV_ICM42607_GYRO_CONFIG0_ODR_MASK GENMASK(3, 0) + +#define INV_ICM42607_REG_ACCEL_CONFIG0 0x21 +#define INV_ICM42607_ACCEL_CONFIG0_FS_SEL_MASK GENMASK(6, 5) +#define INV_ICM42607_ACCEL_CONFIG0_ODR_MASK GENMASK(3, 0) + +#define INV_ICM42607_REG_TEMP_CONFIG0 0x22 +#define INV_ICM42607_TEMP_CONFIG0_FILTER_MASK GENMASK(6, 4) + +#define INV_ICM42607_REG_GYRO_CONFIG1 0x23 +#define INV_ICM42607_GYRO_CONFIG1_FILTER_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_ACCEL_CONFIG1 0x24 +#define INV_ICM42607_ACCEL_CONFIG1_AVG_MASK GENMASK(6, 4) +#define INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK GENMASK(2, 0) + +#define INV_ICM42607_REG_APEX_CONFIG0 0x25 +#define INV_ICM42607_APEX_CONFIG0_DMP_POWER_SAVE_EN BIT(3) +#define INV_ICM42607_APEX_CONFIG0_DMP_INIT_EN BIT(2) +#define INV_ICM42607_APEX_CONFIG0_DMP_MEM_RESET_EN BIT(0) + +#define INV_ICM42607_REG_APEX_CONFIG1 0x26 +#define INV_ICM42607_APEX_CONFIG1_SMD_ENABLE BIT(6) +#define INV_ICM42607_APEX_CONFIG1_FF_ENABLE BIT(5) +#define INV_ICM42607_APEX_CONFIG1_TILT_ENABLE BIT(4) +#define INV_ICM42607_APEX_CONFIG1_PED_ENABLE BIT(3) +#define INV_ICM42607_APEX_CONFIG1_DMP_ODR_MASK GENMASK(1, 0) + +#define INV_ICM42607_REG_WOM_CONFIG 0x27 +#define INV_ICM42607_WOM_CONFIG_INT_DUR_MASK GENMASK(4, 3) +#define INV_ICM42607_WOM_CONFIG_INT_MODE BIT(2) +#define INV_ICM42607_WOM_CONFIG_MODE BIT(1) +#define INV_ICM42607_WOM_CONFIG_EN BIT(0) + +#define INV_ICM42607_REG_FIFO_CONFIG1 0x28 +#define INV_ICM42607_FIFO_CONFIG1_MODE BIT(1) +#define INV_ICM42607_FIFO_CONFIG1_BYPASS BIT(0) + +#define INV_ICM42607_REG_FIFO_CONFIG2 0x29 +#define INV_ICM42607_REG_FIFO_CONFIG3 0x2A +#define INV_ICM42607_FIFO_WATERMARK_VAL (_wm) \ + cpu_to_le16((_wm) & GENMASK(11, 0)) +/* FIFO is 2048 bytes, let 12 samples for reading latency */ +#define INV_ICM42607_FIFO_WATERMARK_MAX (2048 - 12 * 16) +#define INV_ICM42607_FIFO_1SENSOR_PACKET_SIZE 8 +#define INV_ICM42607_FIFO_2SENSORS_PACKET_SIZE 16 + +#define INV_ICM42607_REG_INT_SOURCE0 0x2B +#define INV_ICM42607_INT_SOURCE0_ST_INT1_EN BIT(7) +#define INV_ICM42607_INT_SOURCE0_FSYNC_INT1_EN BIT(6) +#define INV_ICM42607_INT_SOURCE0_PLL_RDY_INT1_EN BIT(5) +#define INV_ICM42607_INT_SOURCE0_RESET_DONE_INT1_EN BIT(4) +#define INV_ICM42607_INT_SOURCE0_DRDY_INT1_EN BIT(3) +#define INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN BIT(2) +#define INV_ICM42607_INT_SOURCE0_FIFO_FULL_INT1_EN BIT(1) +#define INV_ICM42607_INT_SOURCE0_AGC_RDY_INT1_EN BIT(0) + +#define INV_ICM42607_REG_INT_SOURCE1 0x2C +#define INV_ICM42607_INT_SOURCE1_I3C_ERROR_INT1_EN BIT(6) +#define INV_ICM42607_INT_SOURCE1_SMD_INT1_EN BIT(3) +#define INV_ICM42607_INT_SOURCE1_WOM_INT1_EN GENMASK(2, 0) + +#define INV_ICM42607_REG_INT_SOURCE3 0x2D +#define INV_ICM42607_INT_SOURCE3_ST_INT2_EN BIT(7) +#define INV_ICM42607_INT_SOURCE3_FSYNC_INT2_EN BIT(6) +#define INV_ICM42607_INT_SOURCE3_PLL_RDY_INT2_EN BIT(5) +#define INV_ICM42607_INT_SOURCE3_RESET_DONE_INT2_EN BIT(4) +#define INV_ICM42607_INT_SOURCE3_DRDY_INT2_EN BIT(3) +#define INV_ICM42607_INT_SOURCE3_FIFO_THS_INT2_EN BIT(2) +#define INV_ICM42607_INT_SOURCE3_FIFO_FULL_INT2_EN BIT(1) +#define INV_ICM42607_INT_SOURCE3_AGC_RDY_INT2_EN BIT(0) + +#define INV_ICM42607_REG_INT_SOURCE4 0x2E +#define INV_ICM42607_INT_SOURCE4_I3C_ERROR_INT2_EN BIT(6) +#define INV_ICM42607_INT_SOURCE4_SMD_INT2_EN BIT(3) +#define INV_ICM42607_INT_SOURCE4_WOM_Z_INT2_EN BIT(2) +#define INV_ICM42607_INT_SOURCE4_WOM_Y_INT2_EN BIT(1) +#define INV_ICM42607_INT_SOURCE4_WOM_X_INT2_EN BIT(0) + +#define INV_ICM42607_REG_FIFO_LOST_PKT0 0x2F +#define INV_ICM42607_REG_FIFO_LOST_PKT1 0x30 + +#define INV_ICM42607_REG_INTF_CONFIG0 0x35 +#define INV_ICM42607_INTF_CONFIG0_FIFO_COUNT_FORMAT BIT(6) +#define INV_ICM42607_INTF_CONFIG0_FIFO_COUNT_ENDIAN BIT(5) +#define INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN BIT(4) +#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK GENMASK(1, 0) +#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS \ + FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 2) +#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS \ + FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 3) + +#define INV_ICM42607_REG_INTF_CONFIG1 0x36 +#define INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN BIT(3) +#define INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN BIT(2) +#define INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK GENMASK(1, 0) +#define INV_ICM42607_INTF_CONFIG1_CLKSEL_INT 0 +#define INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL 1 +#define INV_ICM42607_INTF_CONFIG1_CLKSEL_OFF 2 + +#define INV_ICM42607_REG_INT_STATUS_DRDY 0x39 +#define INV_ICM42607_INT_STATUS_DRDY_DATA_RDY BIT(0) + +#define INV_ICM42607_REG_INT_STATUS 0x3A +#define INV_ICM42607_INT_STATUS_ST BIT(7) +#define INV_ICM42607_INT_STATUS_FSYNC BIT(6) +#define INV_ICM42607_INT_STATUS_PLL_RDY BIT(5) +#define INV_ICM42607_INT_STATUS_RESET_DONE BIT(4) +#define INV_ICM42607_INT_STATUS_FIFO_THS BIT(2) +#define INV_ICM42607_INT_STATUS_FIFO_FULL BIT(1) +#define INV_ICM42607_INT_STATUS_AGC_RDY BIT(0) + +#define INV_ICM42607_REG_INT_STATUS2 0x3B +#define INV_ICM42607_INT_STATUS2_SMD BIT(3) +#define INV_ICM42607_INT_STATUS2_WOM_INT GENMASK(2, 0) + +#define INV_ICM42607_REG_INT_STATUS3 0x3C +#define INV_ICM42607_INT_STATUS3_STEP_DET BIT(5) +#define INV_ICM42607_INT_STATUS3_STEP_CNT_OVF BIT(4) +#define INV_ICM42607_INT_STATUS3_TILT_DET BIT(3) +#define INV_ICM42607_INT_STATUS3_FF_DET BIT(2) + +/* +* FIFO access registers +* FIFO count is 16 bits (2 registers) big-endian +* FIFO data is a continuous read register to read FIFO content +*/ +#define INV_ICM42607_REG_FIFO_COUNTH 0x3D +#define INV_ICM42607_REG_FIFO_COUNTL 0x3E +#define INV_ICM42607_REG_FIFO_DATA 0x3F + +#define INV_ICM42607_REG_ACCEL_WOM_X_THR 0x4b +#define INV_ICM42607_REG_ACCEL_WOM_Y_THR 0x4c +#define INV_ICM42607_REG_ACCEL_WOM_Z_THR 0x4d + +#define INV_ICM42607_REG_WHOAMI 0x75 +#define INV_ICM42607P_WHOAMI 0x60 +#define INV_ICM42607_WHOAMI 0x67 + +/* Timings as listed in section 3 of datasheet */ +#define INV_ICM42607_POWER_UP_TIME_US 100000 +#define INV_ICM42607_RESET_TIME_MS 1 +#define INV_ICM42607_ACCEL_STARTUP_TIME_MS 10 +#define INV_ICM42607_GYRO_STARTUP_TIME_MS 30 +#define INV_ICM42607_TEMP_STARTUP_TIME_MS 77 + +typedef int (*inv_icm42607_bus_setup )(struct inv_icm42607_state *); + +extern const struct regmap_config inv_icm42607_regmap_config; +extern const struct inv_icm42607_hw inv_icm42607_hw_data; +extern const struct inv_icm42607_hw inv_icm42607p_hw_data; + +int inv_icm42607_core_probe (struct regmap *regmap, +const struct inv_icm42607_hw *hw, +inv_icm42607_bus_setup bus_setup); + +#endif
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +200 -0
200 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The driver state structure `st` is never saved to the device's drvdata, leading to a guaranteed…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver state structure st is never saved to the device's drvdata, leading to a guaranteed NULL pointer dereference in PM callbacks.
- [Low] Redundant error code argument in dev_err_probe() format string.
--
commit d1ce5988d50518ee3fa99e6870abfe673fb42169
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Add the core component of a new inv_icm42607 driver. This includes a few setup functions and the full register definition in the header file.
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +0
-0
[ ... ]
Quoted 13 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +13
-0+int inv_icm42607_core_probe (struct regmap *regmap, +const struct inv_icm42607_hw *hw, +inv_icm42607_bus_setup bus_setup) +{ +struct device *dev =regmap_get_device (regmap); +struct inv_icm42607_state *st; +int ret; + + st =devm_kzalloc (dev,sizeof (*st),GFP_KERNEL ); +if (!st) +return -ENOMEM ; + + ret =devm_mutex_init (dev, &st->lock);
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 168
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +10
-0+if (ret) +return ret; + + st->hw = hw; + st->map = regmap; + + ret =iio_read_mount_matrix (dev, &st->orientation); +if (ret) +return dev_err_probe (dev, ret, +"failed to retrieve mounting matrix %d\n" , ret);
[Severity: Low]
This isn't a bug, but is it necessary to include the %d format specifier and ret argument in this dev_err_probe() call?
The dev_err_probe() API natively prefixes the log message with the provided error code, so passing it as an explicit format argument might result in the error code being printed twice.
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5
-0+ + ret =devm_regulator_get_enable (dev,"vdd" ); +if (ret) +return dev_err_probe (dev, ret, +"Failed to get vdd regulator\n" );
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 194
Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +7
-0+/* Setup chip registers (includes WHOAMI check, reset check, bus setup) */ + ret =inv_icm42607_setup (st, bus_setup); +if (ret) +return ret; + +return 0 ; +}
[Severity: High]
Should this code save the driver state structure to the device's drvdata?
Because runtime PM is enabled at the end of the driver series, inv_icm42607_runtime_suspend() will automatically trigger after the autosuspend delay.
It calls dev_get_drvdata(dev) to retrieve the state structure, which will return NULL since it was never set here.
Could this lead to a NULL pointer dereference shortly after initialization when the PM callbacks try to access the state structure?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
In reply to Chris MorganQuoted 3 lines · Chris Morgan
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file.
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 10
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5
-0+#ifndef INV_ICM42607_H_ +#define INV_ICM42607_H_ + +#include <linux/bitfield.h> +#include <linux/bitops.h>
I haven't found users for these two.
+ bits.h // BIT() / GENMASK()
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 14
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +4
-0+#include <linux/iio/iio.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h>
No users for this one.
+ types.h // for bool
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 20
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5
-0+enum inv_icm42607_sensor_mode { +INV_ICM42607_SENSOR_MODE_OFF , +INV_ICM42607_SENSOR_MODE_STANDBY , +INV_ICM42607_SENSOR_MODE_LOW_POWER , +INV_ICM42607_SENSOR_MODE_LOW_NOISE ,
Are those enums map 1:1 to HW bits or bitfields? If so, assign explicitly each of them.
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 21
Quoted 1 line
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +1
-0+INV_ICM42607_SENSOR_MODE_NB
Is this a terminator like NUMBER_OF ?
Quoted 1 line
+};
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 44
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3
-0+/* ODR values */ +enum inv_icm42607_odr { +INV_ICM42607_ODR_1600HZ =5 ,
See above. This one is problematic. No one should rely on Linux/C enums when it's about HW bits. All HW related stuff has to be explicit.
Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +12
-0+INV_ICM42607_ODR_800HZ , +INV_ICM42607_ODR_400HZ , +INV_ICM42607_ODR_200HZ , +INV_ICM42607_ODR_100HZ , +INV_ICM42607_ODR_50HZ , +INV_ICM42607_ODR_25HZ , +INV_ICM42607_ODR_12_5HZ , +INV_ICM42607_ODR_6_25HZ_LP , +INV_ICM42607_ODR_3_125HZ_LP , +INV_ICM42607_ODR_1_5625HZ_LP , +INV_ICM42607_ODR_NB +};
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 75
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5
-0+struct inv_icm42607_sensor_conf { +int mode; +int fs; +int odr; +int filter;
All of them are supposed to be signed? Why?
Quoted 1 line
+};
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 85
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +2
-0+struct inv_icm42607_hw { +uint8_t whoami;
What's wrong with u8?
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3
-0+const char *name; +const struct inv_icm42607_conf *conf; +};
...
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 14
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9
-0+#include <linux/delay.h> +#include <linux/dev_printk.h> +#include <linux/interrupt.h> +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h>
IWYU, please.
...
Quoted 6 lines
+/**
+ * inv_icm42607_setup() - check and setup chip + * @st: driver internal state + * @bus_setup: callback for setting up bus specific registers+ *
+ * Returns 0 on success, a negative error code otherwise.
If you do kernel-doc, validate it. Return section is missing here.
Quoted 1 line
+ */
...
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 85
Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +12
-0+{ +const struct device *dev =regmap_get_device (st->map); +unsigned int val; +int ret; + + ret =regmap_read (st->map,INV_ICM42607_REG_WHOAMI , &val); +if (ret) +return ret; + +if (val != st->hw->whoami) +dev_warn (dev,"Unknown whoami %#02x expected %#02x (%s)\n" , + val, st->hw->whoami, st->hw->name);
dev_warn_probe() ?
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 92
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5
-0+ ret =regmap_write (st->map,INV_ICM42607_REG_SIGNAL_PATH_RESET , +INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET ); +if (ret) +return ret; +fsleep (INV_ICM42607_RESET_TIME_MS *1000 );
USEC_PER_MSEC (needs time.h)
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 97
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +4
-0+ ret =regmap_read_poll_timeout (st->map,INV_ICM42607_REG_INT_STATUS , + val, val &INV_ICM42607_INT_STATUS_RESET_DONE , +INV_ICM42607_RESET_TIME_MS *100 , +INV_ICM42607_RESET_TIME_MS *10000 );
These are weird, as in the first case it's actually 1/10th of _RESET_TIME_MS. Perhaps you need to reconsider what you use as that constant. Personally I prefer to see just plain values with the multipliers (to convert to µs).
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 108
Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +11
-0+if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" ); + +/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map); + ret =regcache_sync (st->map); +if (ret) +return ret; + + ret =bus_setup (st);
Hmm... This is bad name with potential of name collision in the future (in case driver bus code wants to have the same name for the function).
Quoted 16 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +16
-0+if (ret) +return ret; + + ret =regmap_set_bits (st->map,INV_ICM42607_REG_INTF_CONFIG0 , +INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN ); +if (ret) +return ret; + + ret =regmap_update_bits (st->map,INV_ICM42607_REG_INTF_CONFIG1 , +INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK , +INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL ); +if (ret) +return ret; + +return inv_icm42607_set_conf (st, st->hw->conf); +}
...
Quoted 14 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +14
-0+int inv_icm42607_core_probe (struct regmap *regmap, +const struct inv_icm42607_hw *hw, +inv_icm42607_bus_setup bus_setup) +{ +struct device *dev =regmap_get_device (regmap); +struct inv_icm42607_state *st; +int ret; + + st =devm_kzalloc (dev,sizeof (*st),GFP_KERNEL ); +if (!st) +return -ENOMEM ; + + ret =devm_mutex_init (dev, &st->lock); +if (ret)
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 168
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9
-0+return ret; + + st->hw = hw; + st->map = regmap; + + ret =iio_read_mount_matrix (dev, &st->orientation); +if (ret) +return dev_err_probe (dev, ret, +"failed to retrieve mounting matrix %d\n" , ret);
Remove duplicate ret printing.
Quoted 15 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +15
-0+ + ret =devm_regulator_get_enable (dev,"vdd" ); +if (ret) +return dev_err_probe (dev, ret, +"Failed to get vdd regulator\n" ); + + st->vddio_supply =devm_regulator_get (dev,"vddio" ); +if (IS_ERR (st->vddio_supply)) +return dev_err_probe (dev,PTR_ERR (st->vddio_supply), +"Failed to get vddio regulator\n" ); + + ret =inv_icm42607_enable_vddio_reg (st); +if (ret) +return ret; +
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 193
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9
-0+ ret =devm_add_action_or_reset (dev, inv_icm42607_disable_vddio_reg, st); +if (ret) +return ret; +/* Setup chip registers (includes WHOAMI check, reset check, bus setup) */ + ret =inv_icm42607_setup (st, bus_setup); +if (ret) +return ret; + +return 0 ;
Just
return inv_icm42607_setup(st, bus_setup);
? Or is it going to be extended in the next changes?
Quoted 1 line
+}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 16 lines · Andy Shevchenko
Quoted 3 lines · Chris Morgan
Add the core component of a new
inv_icm42607driver. This includes a few setup functions and the full register definition in the header file....
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 10
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5
-0+#ifndef INV_ICM42607_H_ +#define INV_ICM42607_H_ + +#include <linux/bitfield.h> +#include <linux/bitops.h> I haven't found users for these two.
Quoted 1 line
+
bits.h//BIT()/GENMASK()
I'll correct this.
Quoted 6 lines
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3
-0+#include <linux/iio/iio.h> +#include <linux/mutex.h> +#include <linux/regmap.h>
Quoted 8 lines
Quoted 1 line
+#include <
linux/regulator/consumer.h>No users for this one.
+
types.h// for bool...
And this.
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 20
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5
-0+enum inv_icm42607_sensor_mode { +INV_ICM42607_SENSOR_MODE_OFF , +INV_ICM42607_SENSOR_MODE_STANDBY , +INV_ICM42607_SENSOR_MODE_LOW_POWER , +INV_ICM42607_SENSOR_MODE_LOW_NOISE ,Are those enums map 1:1 to HW bits or bitfields? If so, assign explicitly each of them.
I'll explicitly set every value in the enums to be clear.
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 21
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +1
-0+INV_ICM42607_SENSOR_MODE_NB Is this a terminator like
NUMBER_OF?
This is a terminator, yes. I will not set these explicitly and keep commas off of them to keep them easily identifable as terminators.
Quoted 12 lines
Quoted 2 lines
+};
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 44
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3
-0+/* ODR values */ +enum inv_icm42607_odr { +INV_ICM42607_ODR_1600HZ =5 ,See above. This one is problematic. No one should rely on Linux/C enums when it's about HW bits. All HW related stuff has to be explicit.
I will explicitly set the values here.
Quoted 15 lines
Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +12
-0+INV_ICM42607_ODR_800HZ , +INV_ICM42607_ODR_400HZ , +INV_ICM42607_ODR_200HZ , +INV_ICM42607_ODR_100HZ , +INV_ICM42607_ODR_50HZ , +INV_ICM42607_ODR_25HZ , +INV_ICM42607_ODR_12_5HZ , +INV_ICM42607_ODR_6_25HZ_LP , +INV_ICM42607_ODR_3_125HZ_LP , +INV_ICM42607_ODR_1_5625HZ_LP , +INV_ICM42607_ODR_NB +};...
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 75
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5
-0+struct inv_icm42607_sensor_conf { +int mode; +int fs; +int odr; +int filter;All of them are supposed to be signed? Why?
Quoted 1 line
+};
A later commit will use -1 for invalid values.
Quoted 6 lines
...
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 85
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +2
-0+struct inv_icm42607_hw { +uint8_t whoami;What's wrong with u8?
Nothing, I'll make the change.
Quoted 19 lines
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +3
-0+const char *name; +const struct inv_icm42607_conf *conf; +};...
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 14
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9
-0+#include <linux/delay.h> +#include <linux/dev_printk.h> +#include <linux/interrupt.h> +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> IWYU, please.
Quoted 2 lines
...
This may sound like a dumb question, but what's the best way to invoke iwyu for a kernel? None of the readmes or man pages seem to be getting me anywhere...
Quoted 9 lines
Quoted 6 lines
+/**
+ * inv_icm42607_setup() - check and setup chip + * @st: driver internal state + * @bus_setup: callback for setting up bus specific registers+ *
+ * Returns 0 on success, a negative error code otherwise.If you do kernel-doc, validate it. Return section is missing here.
I'll just nuke it, most of the stuff is fairly self explanatory anyway.
Quoted 29 lines
Quoted 1 line
+ */
...
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 85
Quoted 12 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +12
-0+{ +const struct device *dev =regmap_get_device (st->map); +unsigned int val; +int ret; + + ret =regmap_read (st->map,INV_ICM42607_REG_WHOAMI , &val); +if (ret) +return ret; + +if (val != st->hw->whoami) +dev_warn (dev,"Unknown whoami %#02x expected %#02x (%s)\n" , + val, st->hw->whoami, st->hw->name);
dev_warn_probe()?drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 92
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5
-0+ ret =regmap_write (st->map,INV_ICM42607_REG_SIGNAL_PATH_RESET , +INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET ); +if (ret) +return ret; +fsleep (INV_ICM42607_RESET_TIME_MS *1000 );
USEC_PER_MSEC(needstime.h)
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 97
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +4
-0+ ret =regmap_read_poll_timeout (st->map,INV_ICM42607_REG_INT_STATUS , + val, val &INV_ICM42607_INT_STATUS_RESET_DONE , +INV_ICM42607_RESET_TIME_MS *100 , +INV_ICM42607_RESET_TIME_MS *10000 );These are weird, as in the first case it's actually 1/10th of
_RESET_TIME_MS. Perhaps you need to reconsider what you use as that constant. Personally I prefer to see just plain values with the multipliers (to convert to µs).
I honestly don't know what a good wait period between polling should be, so I guessed at 1/10th the timeout period. I will however change the timeout to us.
Quoted 6 lines
Quoted 6 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +6
-0+if (ret) +return dev_err_probe (dev, ret, +"reset error, reset done bit not set\n" ); + +/* Sync the regcache again after a reset. */ +regcache_mark_dirty (st->map);
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 108
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +5
-0+ ret =regcache_sync (st->map); +if (ret) +return ret; + + ret =bus_setup (st);Hmm... This is bad name with potential of name collision in the future (in case driver bus code wants to have the same name for the function).
Thanks, I'll rename the function.
Quoted 42 lines
Quoted 17 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +16
-0+if (ret) +return ret; + + ret =regmap_set_bits (st->map,INV_ICM42607_REG_INTF_CONFIG0 , +INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN ); +if (ret) +return ret; + + ret =regmap_update_bits (st->map,INV_ICM42607_REG_INTF_CONFIG1 , +INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK , +INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL ); +if (ret) +return ret; + +return inv_icm42607_set_conf (st, st->hw->conf); +}...
Quoted 21 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +21
-0+int inv_icm42607_core_probe (struct regmap *regmap, +const struct inv_icm42607_hw *hw, +inv_icm42607_bus_setup bus_setup) +{ +struct device *dev =regmap_get_device (regmap); +struct inv_icm42607_state *st; +int ret; + + st =devm_kzalloc (dev,sizeof (*st),GFP_KERNEL ); +if (!st) +return -ENOMEM ; + + ret =devm_mutex_init (dev, &st->lock); +if (ret) +return ret; + + st->hw = hw; + st->map = regmap; + + ret =iio_read_mount_matrix (dev, &st->orientation); +if (ret)
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 168
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +2
-0+return dev_err_probe (dev, ret, +"failed to retrieve mounting matrix %d\n" , ret);Remove duplicate ret printing.
Understood.
Quoted 28 lines
Quoted 15 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +15
-0+ + ret =devm_regulator_get_enable (dev,"vdd" ); +if (ret) +return dev_err_probe (dev, ret, +"Failed to get vdd regulator\n" ); + + st->vddio_supply =devm_regulator_get (dev,"vddio" ); +if (IS_ERR (st->vddio_supply)) +return dev_err_probe (dev,PTR_ERR (st->vddio_supply), +"Failed to get vddio regulator\n" ); + + ret =inv_icm42607_enable_vddio_reg (st); +if (ret) +return ret; +drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 193
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9
-0+ ret =devm_add_action_or_reset (dev, inv_icm42607_disable_vddio_reg, st); +if (ret) +return ret; +/* Setup chip registers (includes WHOAMI check, reset check, bus setup) */ + ret =inv_icm42607_setup (st, bus_setup); +if (ret) +return ret; + +return 0 ;Just
Quoted 5 lines
return inv_icm42607_setup(st, bus_setup);? Or is it going to be extended in the next changes?
Quoted 1 line
+}
It will get extended, so I'll keep this for now if that's okay.
Quoted 6 lines
Signature · 3 lines
-- With Best Regards, Andy Shevchenko
Thank you for your feedback. And please let me know about the iwyu tool, that seems like it would be useful.
- Chris
Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 2 lines · Chris Morgan
Quoted 1 line · Andy Shevchenko
On Thu, Jun 04, 2026 at 03:18:25PM -0500, Chris Morgan wrote:
...
Quoted 11 lines
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 75
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5
-0+struct inv_icm42607_sensor_conf { +int mode; +int fs; +int odr; +int filter;All of them are supposed to be signed? Why?
Quoted 1 line
+};
A later commit will use -1 for invalid values.
Okay, perhaps add a comment on top that negative values will mean invalid case?
...
Quoted 12 lines
Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 14
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9
-0+#include <linux/delay.h> +#include <linux/dev_printk.h> +#include <linux/interrupt.h> +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> IWYU, please.
Quoted 3 lines
This may sound like a dumb question, but what's the best way to invoke iwyu for a kernel? None of the readmes or man pages seem to be getting me anywhere...
In this list we (I mean community related to IIO) discussed iwyu tool and its best configuration. Yep, it's barely good and far from ideal, but that's what we have currently. Otherwise git grep ... and cscope are your friends to answer this. Also doing review on regular basis makes this knowledge developed better.
...
Quoted 1 line
It will get extended, so I'll keep this for now if that's okay.
It's okay.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Quoted 14 lines · Andy Shevchenko
Quoted 2 lines · Chris Morgan
Quoted 1 line · Andy Shevchenko
On Thu, Jun 04, 2026 at 03:18:25PM -0500, Chris Morgan wrote:
...
Quoted 7 lines
Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h line 75
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607.h new file +5
-0+struct inv_icm42607_sensor_conf { +int mode; +int fs; +int odr; +int filter;All of them are supposed to be signed? Why?
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
+};
A later commit will use -1 for invalid values.
Okay, perhaps add a comment on top that negative values will mean invalid case?
Will do.
Quoted 19 lines
...
Quoted 12 lines
Quoted 11 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c line 14
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c new file +9
-0+#include <linux/delay.h> +#include <linux/dev_printk.h> +#include <linux/interrupt.h> +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> IWYU, please.
Quoted 3 lines
This may sound like a dumb question, but what's the best way to invoke iwyu for a kernel? None of the readmes or man pages seem to be getting me anywhere...
Quoted 6 lines
In this list we (I mean community related to IIO) discussed iwyu tool and its best configuration. Yep, it's barely good and far from ideal, but that's what we have currently. Otherwise
git grep ...and cscope are your friends to answer this. Also doing review on regular basis makes this knowledge developed better.
Since git grep (and elixir.bootlin.com) are the tools I know best, I'll try to sensibly pare down the includes where I can. I do see that the interrupt include isn't needed anymore so I can drop that one. If anything else jumps out let me know and I'll exclude it too. There's a few other things I'm going to pare out since sashiko.dev bot correctly pointed out some of my code is now dead since I removed buffers support (because I don't have an interrupt on this device).
Quoted 11 lines
...
Quoted 1 line
It will get extended, so I'll keep this for now if that's okay.
It's okay.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Thank you,
Chris
PATCH v10 4/9 iio: imu: inv_icm42607: Add I2C and SPI For icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add I2C and SPI driver support for InvenSense ICM-42607 devices. Add necessary Kconfig and Makefile to allow building of (incomplete) driver.
--- drivers/iio/imu/Kconfig | 1 + drivers/iio/imu/Makefile | 1 + drivers/iio/imu/inv_icm42607/Kconfig | 30 +++++ drivers/iio/imu/inv_icm42607/Makefile | 10 ++ drivers/iio/imu/inv_icm42607/inv_icm42607.h | 16 +++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 57 ++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 94 ++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 106 ++++++++++++++++++ 8 files changed, 315 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/Kconfig create mode 100644 drivers/iio/imu/inv_icm42607/Makefile create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c
drivers/iio/imu/Kconfig +1 -0
@@ -109,6 +109,7 @@ config KMX61 be called kmx61. source "drivers/iio/imu/inv_icm42600/Kconfig" +source "drivers/iio/imu/inv_icm42607/Kconfig" source "drivers/iio/imu/inv_icm45600/Kconfig" source "drivers/iio/imu/inv_mpu6050/Kconfig"
drivers/iio/imu/Makefile +1 -0
@@ -25,6 +25,7 @@ obj-$(CONFIG_FXOS8700_I2C) += fxos8700_i2c.o obj-$(CONFIG_FXOS8700_SPI) += fxos8700_spi.o obj-y += inv_icm42600/ +obj-y += inv_icm42607/ obj-y += inv_icm45600/ obj-y += inv_mpu6050/
drivers/iio/imu/inv_icm42607/Kconfig new file +30 -0
@@ -0,0 +1,30 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +config INV_ICM42607 + tristate + select IIO_BUFFER + select IIO_INV_SENSORS_TIMESTAMP + +config INV_ICM42607_I2C + tristate "InvenSense ICM-42607X I2C driver" + depends on I2C + select INV_ICM42607 + select REGMAP_I2C + help + This driver supports the InvenSense ICM-42607 motion tracking + device over I2C. + + This driver can be built as a module. The module will be called + inv-icm42607-i2c. + +config INV_ICM42607_SPI + tristate "InvenSense ICM-42607X SPI driver" + depends on SPI_MASTER + select INV_ICM42607 + select REGMAP_SPI + help + This driver supports the InvenSense ICM-42607 motion tracking + device over SPI. + + This driver can be built as a module. The module will be called + inv-icm42607-spi.
drivers/iio/imu/inv_icm42607/Makefile new file +10 -0
@@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o +inv-icm42607-y += inv_icm42607_core.o + +obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o +inv-icm42607-i2c-y += inv_icm42607_i2c.o + +obj-$(CONFIG_INV_ICM42607_SPI) += inv-icm42607-spi.o +inv-icm42607-spi-y += inv_icm42607_spi.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +16 -0
@@ -13,6 +13,22 @@#include <linux/regmap.h> #include <linux/regulator/consumer.h> +/* +* Serial bus slew rates. Rates are expressed as range between the two +* values with the midpoint as the typical rate. For the final value of +* 2ns, 2ns is considered the max value with no expressed minimum or +* typical value. +*/ +enum inv_icm42607_slew_rate { +INV_ICM42607_SLEW_RATE_20_60NS , +INV_ICM42607_SLEW_RATE_12_36NS , +INV_ICM42607_SLEW_RATE_6_19NS , +INV_ICM42607_SLEW_RATE_4_14NS , +INV_ICM42607_SLEW_RATE_2_8NS , +INV_ICM42607_SLEW_RATE_2NS , +INV_ICM42607_SLEW_RATE_NB +}; +enum inv_icm42607_sensor_mode {INV_ICM42607_SENSOR_MODE_OFF ,INV_ICM42607_SENSOR_MODE_STANDBY ,
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +57 -0
63 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c new file +94 -0
94 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c new file +106 -0
106 lines not on this page · raw message
Signature · 1 line
2.43.0
PATCH v10 5/9 iio: imu: inv_icm42607: Add PM support for icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add power management support for the ICM42607 device driver.
--- drivers/iio/imu/inv_icm42607/inv_icm42607.h | 18 +++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 123 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 1 + .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 1 + 4 files changed, 143 insertions(+)
drivers/iio/imu/inv_icm42607/inv_icm42607.h +18 -0
@@ -10,6 +10,7 @@#include <linux/bitops.h> #include <linux/iio/iio.h> #include <linux/mutex.h> +#include <linux/pm.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h>
@@ -103,22 +104,32 @@ struct inv_icm42607_hw {const struct inv_icm42607_conf *conf; }; +struct inv_icm42607_suspended { +enum inv_icm42607_sensor_mode gyro; +enum inv_icm42607_sensor_mode accel; +bool temp; +}; +/** * struct inv_icm42607_state - driver state variables * @lock: lock for serializing multiple registers access. * @hw: Hardware specific data. * @map: regmap pointer. * @vddio_supply: I/O voltage regulator for the chip. +* @vddio_en: I/O voltage status for runtime PM. * @orientation: sensor chip orientation relative to main hardware. * @conf: chip sensors configurations. +* @suspended: suspended sensors configuration. */ struct inv_icm42607_state {struct mutex lock;const struct inv_icm42607_hw *hw;struct regmap *map;struct regulator *vddio_supply; +bool vddio_en;struct iio_mount_matrix orientation;struct inv_icm42607_conf conf; +struct inv_icm42607_suspended suspended; };/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
@@ -337,11 +348,18 @@ struct inv_icm42607_state {#define INV_ICM42607_GYRO_STARTUP_TIME_MS 30#define INV_ICM42607_TEMP_STARTUP_TIME_MS 77 +/* +* Suspend delay assumed from other icm42600 series device, not +* documented in datasheet. +*/ +#define INV_ICM42607_SUSPEND_DELAY_MS 2000 +typedef int (*inv_icm42607_bus_setup )(struct inv_icm42607_state *);extern const struct regmap_config inv_icm42607_regmap_config;extern const struct inv_icm42607_hw inv_icm42607_hw_data;extern const struct inv_icm42607_hw inv_icm42607p_hw_data; +extern const struct dev_pm_ops inv_icm42607_pm_ops; int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw *hw,
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +123 -0
164 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c +1 -0
@@ -82,6 +82,7 @@ static struct i2c_driver inv_icm42607_driver = { .driver = { .name = "inv-icm42607-i2c", .of_match_table = inv_icm42607_of_matches, + .pm = pm_ptr(&inv_icm42607_pm_ops), }, .id_table = inv_icm42607_id, .probe = inv_icm42607_probe,
drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c +1 -0
@@ -94,6 +94,7 @@ static struct spi_driver inv_icm42607_driver = { .driver = { .name = "inv-icm42607-spi", .of_match_table = inv_icm42607_of_matches, + .pm = pm_ptr(&inv_icm42607_pm_ops), }, .id_table = inv_icm42607_spi_id_table, .probe = inv_icm42607_probe,
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V10 5/9] iio: imu: inv_icm42607: Add PM support for icm42607
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Missing `dev_set_drvdata()` causes a guaranteed kernel panic on PM transitions. - [Medium]…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing dev_set_drvdata() causes a guaranteed kernel panic on PM transitions.
- [Medium] Unnecessary start-up delays applied when disabling sensors during power management transitions. --
commit 095604e6b5564d3e2f59fa7bf159761bbf791c56
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add PM support for icm42607
Add power management support for the ICM42607 device driver.
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0
-0
[ ... ]
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +5
-0@@ -72,6 +73,51 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = { };EXPORT_SYMBOL_NS_GPL (inv_icm42607p_hw_data,"IIO_ICM42607" ); +static int inv_icm42607_set_pwr_mgmt0 (struct inv_icm42607_state *st, +enum inv_icm42607_sensor_mode gyro, +enum inv_icm42607_sensor_mode accel, +bool temp,unsigned int *sleep_ms) +{
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c const struct inv_icm42607_hw inv_icm42607p_hw_data = { · line 111
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +9
-0+ sleepval =0 ; +if (temp && !oldtemp) + sleepval =max (sleepval,INV_ICM42607_TEMP_STARTUP_TIME_MS ); + +if (accel != oldaccel) + sleepval =max (sleepval,INV_ICM42607_ACCEL_STARTUP_TIME_MS ); + +if (gyro != oldgyro) + sleepval =max (sleepval,INV_ICM42607_GYRO_STARTUP_TIME_MS );
[Severity: Medium]
Does this unnecessarily delay suspend?
When inv_icm42607_runtime_suspend() turns the sensors off by passing INV_ICM42607_SENSOR_MODE_OFF, the state changes (accel != oldaccel and gyro != oldgyro).
Because of this, it seems the function will wait for the maximum startup time before suspending, rather than only sleeping when the sensors are actually turning on.
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +8
-0+ +if (sleep_ms) + *sleep_ms = sleepval; +else if (sleepval) +fsleep (sleepval *1000 ); + +return 0 ; +}
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c int inv_icm42607_core_probe(struct regmap *regmap, · line 347
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +5
-0+static int inv_icm42607_runtime_suspend (struct device *dev) +{ +struct inv_icm42607_state *st =dev_get_drvdata (dev); + +guard (mutex)(&st->lock);
[Severity: Critical]
Will this cause a NULL pointer dereference?
The PM callbacks retrieve the driver state using dev_get_drvdata(), but it doesn't appear that dev_set_drvdata() is ever called during probe to associate the state with the device.
Since pm_runtime_use_autosuspend() is enabled with a 2000ms delay in probe, won't this reliably crash the kernel exactly two seconds after the device is probed?
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +10
-0+ +/* save sensors state */ + st->suspended.gyro = st->conf.gyro.mode; + st->suspended.accel = st->conf.accel.mode; + st->suspended.temp = st->conf.temp_en; + +return inv_icm42607_set_pwr_mgmt0 (st,INV_ICM42607_SENSOR_MODE_OFF , +INV_ICM42607_SENSOR_MODE_OFF , false, +NULL ); +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
PATCH v10 6/9 iio: imu: inv_icm42607: Add Temp Support in icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add functions for reading temperature sensor data.
--- drivers/iio/imu/inv_icm42607/Makefile | 1 + drivers/iio/imu/inv_icm42607/inv_icm42607.h | 5 ++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 18 ++++ .../iio/imu/inv_icm42607/inv_icm42607_temp.c | 82 +++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_temp.h | 33 ++++++++ 5 files changed, 139 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h
drivers/iio/imu/inv_icm42607/Makefile +1 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o inv-icm42607-y += inv_icm42607_core.o +inv-icm42607-y += inv_icm42607_temp.o obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o inv-icm42607-i2c-y += inv_icm42607_i2c.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +5 -0
@@ -120,6 +120,7 @@ struct inv_icm42607_suspended {* @orientation: sensor chip orientation relative to main hardware. * @conf: chip sensors configurations. * @suspended: suspended sensors configuration. +* @buffer: data transfer buffer aligned for DMA. */ struct inv_icm42607_state {struct mutex lock;
@@ -130,6 +131,7 @@ struct inv_icm42607_state {struct iio_mount_matrix orientation;struct inv_icm42607_conf conf;struct inv_icm42607_suspended suspended; +__be16 buffer[3 ]__aligned(IIO_DMA_MINALIGN) ; };/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
@@ -361,6 +363,9 @@ extern const struct inv_icm42607_hw inv_icm42607_hw_data;extern const struct inv_icm42607_hw inv_icm42607p_hw_data;extern const struct dev_pm_ops inv_icm42607_pm_ops; +int inv_icm42607_set_temp_conf (struct inv_icm42607_state *st,bool enable, +unsigned int *sleep_ms); +int inv_icm42607_core_probe (struct regmap *regmap,const struct inv_icm42607_hw *hw,inv_icm42607_bus_setup bus_setup);
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +18 -0
@@ -118,6 +118,24 @@ static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,return 0 ; } +int inv_icm42607_set_temp_conf (struct inv_icm42607_state *st,bool enable, +unsigned int *sleep_ms) +{ +unsigned int val; +int ret; + + val =FIELD_PREP (INV_ICM42607_TEMP_CONFIG0_FILTER_MASK , +INV_ICM42607_FILTER_BW_34HZ ); + ret =regmap_update_bits (st->map,INV_ICM42607_REG_TEMP_CONFIG0 , +INV_ICM42607_TEMP_CONFIG0_FILTER_MASK , val); +if (ret) +return ret; + +return inv_icm42607_set_pwr_mgmt0 (st, st->conf.gyro.mode, + st->conf.accel.mode, enable, + sleep_ms); +} +static int inv_icm42607_set_conf (struct inv_icm42607_state *st,const struct inv_icm42607_conf *conf) {
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +82 -0
82 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.h new file +33 -0
@@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* +* Copyright (C) 2026 InvenSense, Inc. +*/ + +#ifndef INV_ICM42607_TEMP_H_ +#define INV_ICM42607_TEMP_H_ + +#include <linux/bitops.h> + +struct iio_dev ; +struct iio_chan_spec ; + +#define INV_ICM42607_TEMP_CHAN (_index) \ +{ \ + .type = IIO_TEMP, \ + .info_mask_separate = \ + BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_OFFSET) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .scan_index = _index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 16, \ + .storagebits = 16, \ + }, \ +} + +int inv_icm42607_temp_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask); + +#endif
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V10 6/9] iio: imu: inv_icm42607: Add Temp Support in icm42607
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The temperature sensor is explicitly enabled for a one-shot sysfs read but is never disabled…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The temperature sensor is explicitly enabled for a one-shot sysfs read but is never disabled, resulting in a permanent 77ms latency penalty on all subsequent runtime resumes, the disabling of the low-power RC oscillator clock, and increased power consumption during active states. --
commit 4c6f897d3d27fc0ab2a068b2b3d0736751dc6150
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add Temp Support in icm42607
This commit adds support for reading temperature sensor data to the inv_icm42607 driver. It introduces functions to configure the temperature sensor and read its raw data values.
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +0
-0
[ ... ]
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +10
-0+static int inv_icm42607_temp_read (struct inv_icm42607_state *st,s16 *temp) +{ +struct device *dev =regmap_get_device (st->map); +__be16 *raw; +int ret; + +PM_RUNTIME_ACQUIRE_AUTOSUSPEND (dev, pm); + ret =PM_RUNTIME_ACQUIRE_ERR (&pm); +if (ret) +return ret;
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c line 34
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +10
-0+ +guard (mutex)(&st->lock); + + ret =inv_icm42607_set_temp_conf (st, true,NULL ); +if (ret) +return ret; + + raw = &st->buffer[0 ]; + ret =regmap_bulk_read (st->map,INV_ICM42607_REG_TEMP_DATA1 , + raw,sizeof (*raw));
[Severity: High]
Does this leave the temperature sensor permanently enabled?
Calling inv_icm42607_set_temp_conf() with true sets the sensor to active, but it doesn't appear to be disabled after the read completes or fails.
If the temperature sensor is left enabled, st->conf.temp_en remains true. On subsequent runtime resumes, inv_icm42607_set_pwr_mgmt0() would be called with temp = true. This appears to force a 77ms sleep (INV_ICM42607_TEMP_STARTUP_TIME_MS) on every resume, rather than the standard 10ms for accelerometer wakeups.
Additionally, since temp is permanently true, the low-power RC oscillator clock (INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL) might be prevented from being selected, leading to continuously increased power consumption during all active states.
Should there be a call to inv_icm42607_set_temp_conf(st, false, NULL) before returning to restore the previous power state?
Quoted 9 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c new file +9
-0+if (ret) +return ret; + + *temp =be16_to_cpup (raw); +if (*temp ==INV_ICM42607_DATA_INVALID ) +return -EINVAL ; + +return 0 ; +}
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
PATCH v10 7/9 iio: imu: inv_icm42607: Add Accelerometer for icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add icm42607 accelerometer sensor for icm42607.
--- drivers/iio/imu/inv_icm42607/Makefile | 1 + drivers/iio/imu/inv_icm42607/inv_icm42607.h | 40 ++ .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 465 ++++++++++++++++++ .../iio/imu/inv_icm42607/inv_icm42607_core.c | 93 ++++ 4 files changed, 599 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o inv-icm42607-y += inv_icm42607_core.o +inv-icm42607-y += inv_icm42607_accel.o inv-icm42607-y += inv_icm42607_temp.o obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o
drivers/iio/imu/inv_icm42607/inv_icm42607.h +40 -0
@@ -14,6 +14,8 @@#include <linux/regmap.h> #include <linux/regulator/consumer.h> +#include <linux/iio/common/inv_sensors_timestamp.h> +/* * Serial bus slew rates. Rates are expressed as range between the two * values with the midpoint as the typical rate. For the final value of
@@ -85,12 +87,24 @@ enum inv_icm42607_filter_bw {INV_ICM42607_FILTER_BW_NB }; +enum inv_icm42607_filter_avg { +/* Low-Power mode sensor data filter (averaging) */ +INV_ICM42607_FILTER_AVG_2X =0 , +INV_ICM42607_FILTER_AVG_4X , +INV_ICM42607_FILTER_AVG_8X , +INV_ICM42607_FILTER_AVG_16X , +INV_ICM42607_FILTER_AVG_32X , +INV_ICM42607_FILTER_AVG_64X , +/* values 7 and 8 also correspond to 64x. */ +}; +struct inv_icm42607_sensor_conf {int mode;int fs;int odr;int filter; }; +#define INV_ICM42607_SENSOR_CONF_INIT { -1, -1, -1, -1 }struct inv_icm42607_conf {struct inv_icm42607_sensor_conf gyro;
@@ -120,6 +134,7 @@ struct inv_icm42607_suspended {* @orientation: sensor chip orientation relative to main hardware. * @conf: chip sensors configurations. * @suspended: suspended sensors configuration. +* @indio_accel: accelerometer IIO device. * @buffer: data transfer buffer aligned for DMA. */ struct inv_icm42607_state {
@@ -131,9 +146,22 @@ struct inv_icm42607_state {struct iio_mount_matrix orientation;struct inv_icm42607_conf conf;struct inv_icm42607_suspended suspended; +struct iio_dev *indio_accel;__be16 buffer[3 ]__aligned(IIO_DMA_MINALIGN) ; }; +/** +* struct inv_icm42607_sensor_state - sensor state variables +* @power_mode: sensor requested power mode (for common frequencies) +* @filter: sensor filter. +* @ts: timestamp module states. +*/ +struct inv_icm42607_sensor_state { +enum inv_icm42607_sensor_mode power_mode; +int filter; +struct inv_sensors_timestamp ts; +}; +/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */ /* Register Map for User Bank 0 */
@@ -363,6 +391,16 @@ extern const struct inv_icm42607_hw inv_icm42607_hw_data;extern const struct inv_icm42607_hw inv_icm42607p_hw_data;extern const struct dev_pm_ops inv_icm42607_pm_ops; +u32 inv_icm42607_odr_to_period (enum inv_icm42607_odr odr); + +const struct iio_mount_matrix * +inv_icm42607_get_mount_matrix (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan); + +int inv_icm42607_set_accel_conf (struct inv_icm42607_state *st, +struct inv_icm42607_sensor_conf *conf, +unsigned int *sleep_ms); +int inv_icm42607_set_temp_conf (struct inv_icm42607_state *st,bool enable,unsigned int *sleep_ms);
@@ -370,4 +408,6 @@ int inv_icm42607_core_probe(struct regmap *regmap,const struct inv_icm42607_hw *hw, inv_icm42607_bus_setup bus_setup); +struct iio_dev *inv_icm42607_accel_init (struct inv_icm42607_state *st); +#endif
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +465 -0
465 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +93 -0
111 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V10 7/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] Incomplete IIO buffer support results in dead code and logically flawed `update_scan_mode`…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Incomplete IIO buffer support results in dead code and logically flawed update_scan_mode callback.
- [Medium] Accelerometer initialized with conflicting power mode and filter macro.
- [Medium] Failing to configure the hardware ODR leaves the software timestamp structure in an inconsistent state.
- [Medium] Lockless reads of shared state in inv_icm42607_accel_read_scale() and inv_icm42607_accel_read_odr(). --
commit aa64241d5631fbdd46e4e9a9ec9bb1ee1fabadb5
Author: Chris Morgan <[email protected]>
iio: imu: inv_icm42607: Add Accelerometer for icm42607
Add icm42607 accelerometer sensor for icm42607.
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +0
-0
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 201
Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +7
-0+static int inv_icm42607_accel_read_scale (struct iio_dev *indio_dev, +int *val,int *val2) +{ +struct inv_icm42607_state *st =iio_device_get_drvdata (indio_dev); +unsigned int idx; + + idx = st->conf.accel.fs;
[Severity: Medium]
Is it safe to read st->conf.accel.fs here without acquiring st->lock?
The write path in inv_icm42607_accel_write_scale() mutates this state while holding the lock, so concurrent accesses might result in a data race. Should this read be protected using guard(mutex)(&st->lock)?
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 259
Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +7
-0+static int inv_icm42607_accel_read_odr (struct inv_icm42607_state *st, +int *val,int *val2) +{ +unsigned int odr; +unsigned int i; + + odr = st->conf.accel.odr;
[Severity: Medium]
Does this read of st->conf.accel.odr need to be protected by st->lock, similar to how it is protected during writes in inv_icm42607_accel_write_odr()?
[ ... ]
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +3
-0+static int inv_icm42607_accel_write_odr (struct iio_dev *indio_dev, +int val,int val2) +{
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 309
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +8
-0+ ret =inv_sensors_timestamp_update_odr (ts,inv_icm42607_odr_to_period (conf.odr), +iio_buffer_enabled (indio_dev)); +if (ret) +return ret; + + ret =inv_icm42607_set_accel_conf (st, &conf,NULL ); +if (ret) +return ret;
[Severity: Medium]
If inv_icm42607_set_accel_conf() fails here, do we leave the software timestamp module expecting the new ODR while the hardware remains at the old ODR?
[ ... ]
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +2
-0+struct iio_dev *inv_icm42607_accel_init (struct inv_icm42607_state *st) +{
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 441
Quoted 2 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +2
-0+ accel_st->power_mode =INV_ICM42607_SENSOR_MODE_LOW_NOISE ; + accel_st->filter =INV_ICM42607_FILTER_AVG_16X ;
[Severity: Medium]
Is this the intended filter configuration?
We are setting the power_mode to INV_ICM42607_SENSOR_MODE_LOW_NOISE, but using an averaging filter macro (INV_ICM42607_FILTER_AVG_16X) meant for low power mode.
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c line 458
Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c new file +7
-0+iio_device_set_drvdata (indio_dev, st); + indio_dev->name = name; + indio_dev->info = &inv_icm42607_accel_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = inv_icm42607_accel_channels; + indio_dev->num_channels =ARRAY_SIZE (inv_icm42607_accel_channels); + indio_dev->available_scan_masks = inv_icm42607_accel_scan_masks;
[Severity: Medium]
Since INDIO_DIRECT_MODE is the only mode configured and there is no buffer setup (like devm_iio_kfifo_buffer_setup), does this make the buffer-specific logic like inv_icm42607_accel_update_scan_mode, available_scan_masks, and IIO_CHAN_SOFT_TIMESTAMP dead code?
Also, if inv_icm42607_accel_update_scan_mode were to be called, it lacks a PM runtime acquisition, which could lead to accessing hardware registers on a suspended device.
Quoted 3 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +0
-0
[ ... ]
Quoted 4 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +4
-0+int inv_icm42607_set_accel_conf (struct inv_icm42607_state *st, +struct inv_icm42607_sensor_conf *conf, +unsigned int *sleep_ms) +{
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st, · line 196
Quoted 8 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +8
-0+if (conf->filter != oldconf->filter) { +if (conf->mode ==INV_ICM42607_SENSOR_MODE_LOW_POWER ) { + val =FIELD_PREP (INV_ICM42607_ACCEL_CONFIG1_AVG_MASK , conf->filter); + ret =regmap_update_bits (st->map,INV_ICM42607_REG_ACCEL_CONFIG1 , +INV_ICM42607_ACCEL_CONFIG1_AVG_MASK , val); + }else { + val =FIELD_PREP (INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK , + conf->filter);
[Severity: Medium]
Since conf->mode is initialized to LOW_NOISE, this branch will apply the filter value (3 for AVG_16X) to INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK.
Value 3 in the bandwidth enum corresponds to INV_ICM42607_FILTER_BW_73HZ. Will this silently set a 73Hz bandwidth instead of the intended 16x averaging?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v10 8/9 iio: imu: inv_icm42607: Add Gyroscope to icm42607
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add gyroscope functions to the icm42607 driver.
--- drivers/iio/imu/inv_icm42607/Makefile | 1 + drivers/iio/imu/inv_icm42607/inv_icm42607.h | 8 + .../iio/imu/inv_icm42607/inv_icm42607_core.c | 48 ++ .../iio/imu/inv_icm42607/inv_icm42607_gyro.c | 437 ++++++++++++++++++ 4 files changed, 494 insertions(+) create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
drivers/iio/imu/inv_icm42607/Makefile +1 -0
@@ -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
@@ -134,6 +134,7 @@ struct inv_icm42607_suspended {* @orientation: sensor chip orientation relative to main hardware. * @conf: chip sensors configurations. * @suspended: suspended sensors configuration. +* @indio_gyro: gyroscope IIO device. * @indio_accel: accelerometer IIO device. * @buffer: data transfer buffer aligned for DMA. */
@@ -146,6 +147,7 @@ struct inv_icm42607_state {struct iio_mount_matrix orientation;struct inv_icm42607_conf conf;struct inv_icm42607_suspended suspended; +struct iio_dev *indio_gyro;struct iio_dev *indio_accel;__be16 buffer[3 ]__aligned(IIO_DMA_MINALIGN) ; };
@@ -401,6 +403,10 @@ int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,struct inv_icm42607_sensor_conf *conf, unsigned int *sleep_ms); +int inv_icm42607_set_gyro_conf (struct inv_icm42607_state *st, +struct inv_icm42607_sensor_conf *conf, +unsigned int *sleep_ms); +int inv_icm42607_set_temp_conf (struct inv_icm42607_state *st,bool enable,unsigned int *sleep_ms);
@@ -408,6 +414,8 @@ int inv_icm42607_core_probe(struct regmap *regmap,const struct inv_icm42607_hw *hw, inv_icm42607_bus_setup bus_setup); +struct iio_dev *inv_icm42607_gyro_init (struct inv_icm42607_state *st); +struct iio_dev *inv_icm42607_accel_init (struct inv_icm42607_state *st);#endif
drivers/iio/imu/inv_icm42607/inv_icm42607_core.c +48 -0
60 lines not on this page · raw message
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +437 -0
437 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V10 8/9] iio: imu: inv_icm42607: Add Gyroscope to icm42607
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Gyroscope is permanently left powered on in the driver's cached state after a single raw read…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Gyroscope is permanently left powered on in the driver's cached state after a single raw read, causing a cross-sensor power leak.
- [Medium] Concurrent lockless access to shared configuration fields in sysfs handlers.
- [Low] Buffer-specific structures, channels, and callbacks are defined but completely unreachable (dead code) because the device does not instantiate an IIO buffer.
- [Low] Variable ret may trigger a -Wmaybe-uninitialized compiler warning in inv_icm42607_gyro_update_scan_mode().
--
commit f6b9c2f7324591ed8e8a04df80fed00466b4a298
Author: Chris Morgan <[email protected]> Date: Thu Jun 4 20:18:30 2026 +0000
iio: imu: inv_icm42607: Add Gyroscope to icm42607
Add gyroscope functions to the icm42607 driver.
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +0
-0
[ ... ]
Quoted 22 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +22
-0+/* enable gyroscope sensor */ +static int inv_icm42607_gyro_update_scan_mode (struct iio_dev *indio_dev, +const unsigned long *scan_mask) +{ +struct inv_icm42607_state *st =iio_device_get_drvdata (indio_dev); +struct inv_icm42607_sensor_conf conf =INV_ICM42607_SENSOR_CONF_INIT ; +unsigned int sleep_gyro =0 ; +unsigned int sleep_temp =0 ; +int ret; + +mutex_lock (&st->lock); + +if (*scan_mask &INV_ICM42607_SCAN_MASK_TEMP ) { +/* enable temp sensor */ + ret =inv_icm42607_set_temp_conf (st, true, &sleep_temp); +if (ret) + goto out_unlock; + } + +if (*scan_mask &INV_ICM42607_SCAN_MASK_GYRO_3AXIS ) { +/* enable gyro sensor */ + conf.mode =INV_ICM42607_SENSOR_MODE_LOW_NOISE ;
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 107
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10
-0+ ret =inv_icm42607_set_gyro_conf (st, &conf, &sleep_gyro); +if (ret) + goto out_unlock; + } + +out_unlock: +mutex_unlock (&st->lock); +/* sleep maximum required time */ +msleep (max (sleep_gyro, sleep_temp)); +return ret;
[Severity: Low]
Can ret be returned uninitialized here?
If *scan_mask contains neither INV_ICM42607_SCAN_MASK_TEMP nor INV_ICM42607_SCAN_MASK_GYRO_3AXIS, the code jumps directly to out_unlock and returns the uninitialized stack value.
Although the IIO subsystem guarantees the mask strictly matches one of the available_scan_masks (which all contain the 3-axis gyro mask), compilers and static analyzers evaluate the function locally. This triggers a -Wmaybe-uninitialized warning that will break builds configured with W=1 or -Werror.
Quoted 31 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +31
-0+} + +static int inv_icm42607_gyro_read_sensor (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +s16 *val) +{ +struct inv_icm42607_state *st =iio_device_get_drvdata (indio_dev); +struct inv_icm42607_sensor_state *gyro_st =iio_priv (indio_dev); +struct device *dev =regmap_get_device (st->map); +struct inv_icm42607_sensor_conf conf =INV_ICM42607_SENSOR_CONF_INIT ; +unsigned int reg; +__be16 *data; +int ret; + +if (chan->type !=IIO_ANGL_VEL ) +return -EINVAL ; + +switch (chan->channel2) { +case IIO_MOD_X : + reg =INV_ICM42607_REG_GYRO_DATA_X1 ; +break ; +case IIO_MOD_Y : + reg =INV_ICM42607_REG_GYRO_DATA_Y1 ; +break ; +case IIO_MOD_Z : + reg =INV_ICM42607_REG_GYRO_DATA_Z1 ; +break ; +default : +return -EINVAL ; + } +
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 148
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10
-0+PM_RUNTIME_ACQUIRE_AUTOSUSPEND (dev, pm); + ret =PM_RUNTIME_ACQUIRE_ERR (&pm); +if (ret) +return ret; + +guard (mutex)(&st->lock); + +/* enable gyro sensor */ + conf.mode = gyro_st->power_mode; + ret =inv_icm42607_set_gyro_conf (st, &conf,NULL );
[Severity: High]
Does this leave the gyroscope permanently powered on in the driver's cached state?
When the raw read finishes, inv_icm42607_set_gyro_conf() isn't called again to revert the state to OFF.
When PM auto-suspend triggers after the delay, it appears inv_icm42607_runtime_suspend() will erroneously save this active state into st->suspended.gyro before powering off the device.
Later, if a read is performed on another sensor like the accelerometer, inv_icm42607_runtime_resume() restores st->suspended.gyro to the hardware. Consequently, the gyroscope is incorrectly powered back on and will remain powered on during all future PM active periods, draining power unnecessarily.
Quoted 15 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +15
-0+if (ret) +return ret; + +/* read gyro register data */ + data = &st->buffer[0 ]; + ret =regmap_bulk_read (st->map, reg, data,sizeof (*data)); +if (ret) +return ret; + + *val =be16_to_cpup (data); +if (*val ==INV_ICM42607_DATA_INVALID ) +return -EINVAL ; + +return 0 ; +}
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 178
Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +7
-0+static int inv_icm42607_gyro_read_scale (struct iio_dev *indio_dev, +int *val,int *val2) +{ +struct inv_icm42607_state *st =iio_device_get_drvdata (indio_dev); +unsigned int idx; + + idx = st->conf.gyro.fs;
[Severity: Medium]
Is it safe to read st->conf.gyro.fs without holding st->lock here?
Concurrent sysfs writes via inv_icm42607_gyro_write_scale() lock st->lock and modify this exact field. Reading it locklessly creates a data race that is undefined behavior in C and could be flagged by KCSAN.
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +5
-0+ + *val = inv_icm42607_gyro_scale_nano[idx][0 ]; + *val2 = inv_icm42607_gyro_scale_nano[idx][1 ]; +return IIO_VAL_INT_PLUS_NANO ; +}
[ ... ]
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 232
Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +7
-0+static int inv_icm42607_gyro_read_odr (struct inv_icm42607_state *st, +int *val,int *val2) +{ +unsigned int odr; +unsigned int i; + + odr = st->conf.gyro.odr;
[Severity: Medium]
Similarly, is it safe to read st->conf.gyro.odr here without holding st->lock?
Quoted 5 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +5
-0+ +for (i =5 ; i <ARRAY_SIZE (inv_icm42607_gyro_odr); ++i) { +if (i == odr) +break ; + }
[ ... ]
Quoted 26 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +26
-0+struct iio_dev *inv_icm42607_gyro_init (struct inv_icm42607_state *st) +{ +struct device *dev =regmap_get_device (st->map); +const char *name; +struct inv_icm42607_sensor_state *gyro_st; +struct inv_sensors_timestamp_chip ts_chip; +struct iio_dev *indio_dev; +int ret; + + name =devm_kasprintf (dev,GFP_KERNEL ,"%s-gyro" , st->hw->name); +if (!name) +return ERR_PTR (-ENOMEM ); + + indio_dev =devm_iio_device_alloc (dev,sizeof (*gyro_st)); +if (!indio_dev) +return ERR_PTR (-ENOMEM ); + gyro_st =iio_priv (indio_dev); + + gyro_st->power_mode =INV_ICM42607_SENSOR_MODE_LOW_NOISE ; + +/* +* clock period is 32kHz (31250ns) +* jitter is +/- 2% (20 per mille) +*/ + ts_chip.clock_period =31250 ; + ts_chip.jitter =20 ;
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c line 430
Quoted 10 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +10
-0+ ts_chip.init_period =inv_icm42607_odr_to_period (st->conf.gyro.odr); +inv_sensors_timestamp_init (&gyro_st->ts, &ts_chip); + +iio_device_set_drvdata (indio_dev, st); + indio_dev->name = name; + indio_dev->info = &inv_icm42607_gyro_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = inv_icm42607_gyro_channels; + indio_dev->num_channels =ARRAY_SIZE (inv_icm42607_gyro_channels); + indio_dev->available_scan_masks = inv_icm42607_gyro_scan_masks;
[Severity: Low]
Since the device is initialized exclusively in INDIO_DIRECT_MODE and lacks any calls to allocate or attach a triggered buffer (e.g., via devm_iio_triggered_buffer_setup), are the scan masks and buffer callbacks dead code?
Without a buffer setup, inv_icm42607_gyro_scan_masks, the INV_ICM42607_GYRO_SCAN_TIMESTAMP channel, the software tracking logic, and the update_scan_mode callback will never be executed.
Quoted 7 lines
drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c new file +7
-0+ + ret =devm_iio_device_register (dev, indio_dev); +if (ret) +return ERR_PTR (ret); + +return indio_dev; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
PATCH v10 9/9 arm64: dts: rockchip: Add icm42607p IMU for RG-DS
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add the Invensense ICM42607P IMU for the Anbernic RG-DS. Mount-matrix was tested with iio-sensor-proxy and reports correct orientation.
--- arch/arm64/boot/dts/rockchip/rk3568-anbernic-rg-ds.dts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
arch/arm64/boot/dts/rockchip/rk3568-anbernic-rg-ds.dts +7 -1
@@ -871,7 +871,13 @@ aw87391_pa_r: audio-codec@5b { sound-name-prefix = "Right Amp"; }; - /* invensense,icm42607p at 0x68 */ + icm42607p: imu@68 { + compatible = "invensense,icm42607p"; + reg = <0x68>; + mount-matrix = "-1", "0", "0", + "0", "1", "0", + "0", "0", "-1"; + }; }; &i2c3 {
Signature · 1 line
2.43.0