Re: [PATCH 2/3] iio: accel: Add support for ICM42370P

On Fri, 7 Aug 2026 13:41:04 +0000
Kanak Shilledar <[email protected]> wrote:

> Hi Joshua,
> 
> On Fri, 2026-08-07 at 12:09 +0200, Joshua Crofts wrote:
> > [You don't often get email from [email protected]. Learn why
> > this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > On Thu, 6 Aug 2026 14:46:28 +0200
> > Kanak Shilledar <[email protected]> wrote:
> >   
> > > Add support for the Invensense ICM42370P MEMS MotionTracking 3-axis
> > > accelerometer with a built-in temperature sensor. Compared to other
> > > sensors from the same vendor ICM42370 uses a different way of
> > > handling
> > > register banks. Although the device supports I2C, SPI, and I3C,
> > > implement only I2C support.  Provide basic support for raw sensor
> > > reads and a sysfs interface for setting the calibration bias. Keep
> > > the
> > > embedded temperature sensor enabled because the device design does
> > > not
> > > allow it to be turned off.
> > > 
> > > Signed-off-by: Kanak Shilledar <[email protected]>
> > > ---  
> > 
> > Hi Kanak,
> > 
> > my comments inline. This is an large driver and I've probably missed
> > something. Additionally, Sashiko had some pretty good remarks about
> > the scaling math etc. so please check out those:  
> 
> 
> Thanks for the detailed review.
> 
> > 
> > https://sashiko.dev/#/patchset/20260806-b4-inv_icm42370p-v1-0-670837f5842f%40axis.com  
> 
> I am going through the sashiko's comments and incorporating them in my
> v2.
> 
> > Josh
> >   
> > > +config INV_ICM42370
> > > +     tristate
> > > +     select IIO_BUFFER
> > > +     select IIO_INV_SENSORS_TIMESTAMP
> > > +
> > > +config INV_ICM42370_I2C
> > > +     tristate "InvenSense ICM-42370 I2C driver"
> > > +     depends on I2C
> > > +     select INV_ICM42370
> > > +     select REGMAP_I2C
> > > +     help
> > > +       This driver supports the InvenSense ICM-42730 motion
> > > tracking
> > > +       devices over I2C.
> > > +
> > > +       This driver can be built as a module. The module will be
> > > called
> > > +       inv_icm42370_i2c.
> > > +
> > >  config KXSD9
> > >       tristate "Kionix KXSD9 Accelerometer Driver"
> > >       select IIO_BUFFER
> > > diff --git a/drivers/iio/accel/Makefile
> > > b/drivers/iio/accel/Makefile
> > > index fa440a8592839..6750b03edf518 100644
> > > --- a/drivers/iio/accel/Makefile
> > > +++ b/drivers/iio/accel/Makefile
> > > @@ -49,6 +49,11 @@ obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-
> > > accel-3d.o
> > >  obj-$(CONFIG_IIO_KX022A)     += kionix-kx022a.o
> > >  obj-$(CONFIG_IIO_KX022A_I2C) += kionix-kx022a-i2c.o
> > >  obj-$(CONFIG_IIO_KX022A_SPI) += kionix-kx022a-spi.o
> > > +
> > > +obj-$(CONFIG_INV_ICM42370) += inv-icm42370.o
> > > +inv-icm42370-y += inv_icm42370_core.o
> > > +obj-$(CONFIG_INV_ICM42370_I2C) += inv_icm42370_i2c.o
> > > +
> > >  obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o
> > >  obj-$(CONFIG_KXSD9)  += kxsd9.o
> > >  obj-$(CONFIG_KXSD9_SPI)      += kxsd9-spi.o
> > > diff --git a/drivers/iio/accel/inv_icm42370.h
> > > b/drivers/iio/accel/inv_icm42370.h
> > > new file mode 100644
> > > index 0000000000000..9866a5e970dcd
> > > --- /dev/null
> > > +++ b/drivers/iio/accel/inv_icm42370.h
> > > @@ -0,0 +1,365 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2020 Invensense, Inc.
> > > + * Copyright (C) 2026 Axis Communications AB
> > > + */
> > > +
> > > +#ifndef INV_ICM42370_H_
> > > +#define INV_ICM42370_H_
> > > +
> > > +#include <linux/bits.h>
> > > +#include <linux/bitfield.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regulator/consumer.h>  
> > 
> > Sort these headers alphabetically. Also you're missing types.h.
> >   
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/common/inv_sensors_timestamp.h>  
> > 
> > Group the <linux/iio/*> headers separately (check other drivers in
> > IIO
> > for reference).  
> 
> Will sort and group the headers as per the convention.
> 
> > > +
> > > +enum inv_icm42370_chip {
> > > +     INV_CHIP_INVALID,
> > > +     INV_CHIP_ICM42370,
> > > +     INV_CHIP_NB,
> > > +};
> > > +
> > > +/* sensor configuration struct */
> > > +struct inv_icm42370_conf {
> > > +     int mode;
> > > +     int fs;
> > > +     int odr;
> > > +     int filter;
> > > +};
> > > +
> > > +/**
> > > + * struct inv_icm42370_data - driver state variables
> > > + * @lock:            lock for serializing multiple register
> > > access.
> > > + * @name:            chip name.
> > > + * @map:             regmap pointer.
> > > + * @vdd_supply:              VDD voltage regulator for the chip.
> > > + * @vddio_supply:    I/O voltage regulator for the chip.
> > > + * @indio_accel:     accelerometer IIO device.
> > > + * @sensor_state:    per-sensor state tracking (e.g. power, ODR).
> > > + * @buffer:          buffer for reading data registers, aligned
> > > for DMA.
> > > + * @accel_calibbias: accelerometer calibration bias for X, Y, and
> > > Z axes.
> > > + * @fifo:            FIFO state and configuration.
> > > + * @timestamp:               interrupt timestamp.
> > > + * @chip:            chip identifier.
> > > + * @conf:            chip sensors configurations.
> > > + */
> > > +struct inv_icm42370_data {
> > > +     struct mutex lock;
> > > +     const char *name;
> > > +     struct regmap *map;
> > > +     struct regulator *vdd_supply;
> > > +     struct regulator *vddio_supply;
> > > +     struct iio_dev *indio_accel;  
> > 
> > You probably don't need this.  
> Can you please clarify this comment? As I am using `*indio_accel` in
> other places inside inv_icm42370_core.c and in many places in
> inv_icm42370_buffer.c. Or are you perhaps referring to the
> *sensor_state struct below?

Yes, apologies, I meant to come back to this comment but forgot (given the
size of the driver :)). Reading the cover letter I see that you've
taken inspiration from the icm42600, which is an accelerometer and gyroscope,
which would warrant two different iio_dev structs. However, this chip is
an accelerometer only, meaning you can just pass the one iio_dev struct
that you initialize in probe to the *_irq_init() function you have and forget
about it later.

-- 
Kind regards,
Joshua Crofts