Re: [PATCH V14 0/9] Add Invensense ICM42607

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 24 Jun 2026 16:18:23 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:

> > Changes Since V13:
> >  - Refactored inv_icm42607_set_accel_conf() and
> >    inv_icm42607_set_gyro_conf() into a single function.
> >  - Refactored inv_icm42607_accel_read_sensor() and
> >    inv_icm42607_gyro_read_sensor() into a single function.
> >  - Merged inv_icm42607_set_temp_conf() into initial init function
> >    since it only really needs to be called once.
> >  - Saved adding temp sensor for last and updated
> >    inv_icm42607_temp_read() to either confirm other sensors are already
> >    enabled or enable the accelerometer so it can get a reading.
> >  - Updated inv_icm42607_set_pwr_mgmt0() so that it does not update the
> >    sensor mode and forcibly keep the sensor enabled.
> >  - Added inv_icm42607_temp_filter_bw enums since it appears to use
> >    different values than the accel or gyro sensor.
> >  - Set the temp startup time from 77ms to 77us, as I previously misread
> >    the datasheet.
> >  - Additional minor fixes.
> > 
> > Chris Morgan (9):
> >   dt-bindings: iio: imu: icm42600: Add mount-matrix
> >   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 Accelerometer for icm42607
> >   iio: imu: inv_icm42607: Add Gyroscope to icm42607
> >   iio: imu: inv_icm42607: Add Temp Support in 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   | 423 ++++++++++++
> >  .../iio/imu/inv_icm42607/inv_icm42607_accel.c | 317 +++++++++
> >  .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 606 ++++++++++++++++++
> >  .../iio/imu/inv_icm42607/inv_icm42607_gyro.c  | 313 +++++++++
> >  .../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  |  99 +++
> >  .../iio/imu/inv_icm42607/inv_icm42607_temp.h  |  37 ++
> >  14 files changed, 2072 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
> > 
> > -- 
> > 2.43.0
> >   
> 
> I got sashiko.dev's review, and agree with everything except 2 points.
> 
> 1) It asks "Is it appropriate to omit the interrupts property based on
> driver behavior rather than hardware wiring?" I would say no, but in
> my case the hardware is not wired for an interrupt so it's moot.


I think the key thing it gets wrong is 'required'.  Obviously an interrupt
isn't required if it isn't wired ;)  My only question is why is
it required for the other devices?  That makes no sense - so I think
you can get rid of that change to require it in the binding (I'll reply to v15)


> 
> 2) It asks "Does this code update the software state cache for the
> sensor mode?" No, it doesn't. The goal is for the device to stay
> powered up for up to 2 seconds until runtime PM turns it off if it
> isn't active, but I don't mind something turning around and shutting
> the hardware down if it's not in use before then. Anything that needs
> to read the specific sensor in question will be holding the mutex so
> that there shouldn't be a risk of another thread turning stuff off
> while I'm trying to use it, I don't think. What I want to avoid is
> calling to turn the hardware on for a one-shot read and then
> immediately turning it off once I'm done. I can wait the two seconds
> for runtime PM to do it when the alternative is to call the shutdown
> routine every single time for one-shot reads and wait for the startup
> and in the case of the gyro shutdown times otherwise.
> 
> Thoughts?
> 
As long as the locks are doing their jobs, this seems correct.

Jonathan

> I can fix the remaining issues it points out and resubmit.
> 
> Thank you,
> Chris