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

[Kanak Shilledar <Kanak.Shilledar@axis.com>]

[-- Attachment #1: Type: text/plain, Size: 42711 bytes --]

Hi Joshua,

On Fri, 2026-08-07 at 12:09 +0200, Joshua Crofts wrote:
> [You don't often get email from joshua.crofts1@gmail.com. Learn why
> this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Thu, 6 Aug 2026 14:46:28 +0200
> Kanak Shilledar <kanak.shilledar@axis.com> 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 <kanak.shilledar@axis.com>
> > ---
> 
> 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?

> 
> > +     struct inv_icm42370_sensor_state *sensor_state;
> > +     u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
> 
> You shouldn't have a DMA buffer in the middle of your struct, move it
> to
> the end to ensure the buffer gets its own cacheline to prevent the
> CPU
> overwriting the DMA area on accident.

Will move the buffer property at the end.

> 
> > +     s16 accel_calibbias[3];
> > +     struct inv_icm42370_fifo fifo;
> 
> You add buffer support in patch 3, yet this is in patch 2, causing
> build
> failures if you compile without buffer support. Move this to patch 3.

Will move this to patch 3.

> 
> > +     s64 timestamp;
> > +     enum inv_icm42370_chip chip;
> > +     struct inv_icm42370_conf conf;
> > +};
> > +
[snip]
> > +
> > +/* Temperature sensor filters */
> > +enum inv_icm42370_temp_filter {
> > +     INV_ICM42370_TEMP_FILT_BW_DLPF_BYPASS,
> > +     INV_ICM42370_TEMP_FILT_BW_DLPF_180HZ,
> > +     INV_ICM42370_TEMP_FILT_BW_DLPF_72HZ,
> > +     INV_ICM42370_TEMP_FILT_BW_DLPF_34HZ,
> > +     INV_ICM42370_TEMP_FILT_BW_DLPF_16HZ,
> > +     INV_ICM42370_TEMP_FILT_BW_DLPF_8HZ,
> > +     INV_ICM42370_TEMP_FILT_BW_DLPF_4HZ,
> > +     INV_ICM42370_TEMP_FILT_BW_DLPF_NB,
> > +};
> > +
> > +/* IIO format int + micro */
> 
> No need for this comment.
> 
> > +static const int inv_icm42370_accel_odr[] = {
> > +     /* 1.5625Hz */
> 
> No need for these comments.

Will remove the comments.

> 
> > +     1, 562500,
> > +     /* 3.125Hz */
> > +     3, 125000,
> > +     /* 6.25Hz */
> > +     6, 250000,
> > +     /* 12.5Hz */
> > +     12, 500000,
> > +     /* 25Hz */
> > +     25, 0,
> > +     /* 50Hz */
> > +     50, 0,
> > +     /* 100Hz */
> > +     100, 0,
> > +     /* 200Hz */
> > +     200, 0,
> > +     /* 400Hz */
> > +     400, 0,
> > +     /* 800Hz */
> > +     800, 0,
> > +     /* 1.6kHz */
> > +     1600, 0,
> > +};
> > +
> > +#define INV_ICM42370_SENSOR_CONF_INIT { -1, -1, -1, -1 }
> > +
> > 
[snip]
> > diff --git a/drivers/iio/accel/inv_icm42370_core.c
> > b/drivers/iio/accel/inv_icm42370_core.c
> > new file mode 100644
> > index 0000000000000..9f6c302e6f331
> > --- /dev/null
> > +++ b/drivers/iio/accel/inv_icm42370_core.c
> > @@ -0,0 +1,1251 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2020 Invensense, Inc.
> > + * Copyright (C) 2026 Axis Communications AB
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/irq.h>
> > +#include <linux/slab.h>
> > +#include <linux/mod_devicetable.h>
> 
> As Uwe said in his email, remove this.
> 
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +
> 
> You're missing types.h, bitops.h, err.h, array_size.h and
> <asm/byteorder.h>.

Will sort the includes, add the missing headers and remove the
<linux/mod_devicetable.h>.

> 
> > +#include <linux/iio/common/inv_sensors_timestamp.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#include "inv_icm42370.h"
> > +
> > +const struct regmap_config inv_icm42370_regmap_config = {
> > +     .name = "inv_icm42370",
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .max_register = 0x7E,
> > +};
> > +EXPORT_SYMBOL_NS_GPL(inv_icm42370_regmap_config, "IIO_ICM42370");
> > +
> > 
[snip]
> > +/**
> > + *  inv_icm42370_odr_to_period() - map ODR to Period
> > + *  @odr - enum of ODR value
> > + *
> > + * Returns the period in nanoseconds
> > + */
> > +u32 inv_icm42370_odr_to_period(enum inv_icm42370_odr odr)
> > +{
> > +     static u32 odr_periods[INV_ICM42370_ODR_NB] = {
> > +             /* reserved values */
> > +             0,
> > +             0,
> > +             0,
> > +             0,
> > +             0,
> > +             /* 1.6kHz */
> 
> Maybe a personal preference, but wouldn't it be better to have
> the comment on the same line as the value? I find this harder
> to read.

Will reformat this array to be more cleaner.

> 
> > +             625000,
> > +             /* 800Hz */
> > +             1250000,
> > +             /* 400Hz */
> > +             2500000,
> > +             /* 200Hz */
> > +             5000000,
> > +             /* 100Hz */
> > +             10000000,
> > +             /* 50Hz */
> > +             20000000,
> > +             /* 25Hz */
> > +             40000000,
> > +             /* 12.5Hz */
> > +             80000000,
> > +             /* 6.25Hz */
> > +             160000000,
> > +             /* 3.125Hz */
> > +             320000000,
> > +             /* 1.5625Hz */
> > +             640000000,
> > +     };
> > +
> > +     return odr_periods[odr];
> > +}
> > +
> > +
[snip]
> > +/**
> > + * inv_icm42370_mreg_read() - routine for reading from other bank
> > registers
> > + *
> > + * @map: regmap of the device
> > + * @bank: register bank being accessed
> > + * @addr: address of the register being accessed
> > + * @val: pointer to store the register's data
> > + *
> > + * Returns 0 on success, negative errno on error
> > + */
> > +static int inv_icm42370_mreg_read(struct regmap *map, u8 bank, u8
> > addr, u8 *val)
> > +{
> > +     int ret;
> > +     unsigned int read_val;
> > +
> > +     ret = inv_icm42370_mreg_check(map);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(map, INV_ICM42370_REG_BLK_SEL_R, bank);
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     ret = regmap_write(map, INV_ICM42370_REG_MADDR_R, addr);
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     usleep_range(10, 20);
> 
> fsleep() will guarantee at least 10ms of sleep and it chooses
> the optimal way of achieving this.

Will convert all instances of usleep_range() to fsleep().

> 
> > +     ret = regmap_read(map, INV_ICM42370_REG_M_R, &read_val);
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     usleep_range(10, 20);
> > +     *val = (u8)read_val;
> > +
> > +     return regmap_write(map, INV_ICM42370_REG_BLK_SEL_R, 0x00);
> > +}
> > +
> > +/**
> > + * inv_icm42370_set_conf() - set sensor configuration
> > + *
> > + * @data: pointer to struct containing the sensor data
> > + * @conf: pointer to configuration data
> > + *
> > + * Returns 0 on success, negative errno on error
> > + */
> > +static int inv_icm42370_set_conf(struct inv_icm42370_data *data,
> > +                              const struct inv_icm42370_conf
> > *conf)
> > +{
> > +     unsigned int val;
> > +     int ret;
> > +
> > +     /* set PWR_MGMT0 register (accel sensor mode, temp enabled)
> > */
> > +     val = INV_ICM42370_PWR_MGMT0(conf->mode);
> > +     ret = regmap_write(data->map, INV_ICM42370_REG_PWR_MGMT0,
> > val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     msleep(200);
> > +     /* set ACCEL_CONFIG0 register (accel fullscale & odr) */
> > +     val = INV_ICM42370_ACCEL_CONFIG0_FS(conf->fs) |
> > +           INV_ICM42370_ACCEL_CONFIG0_ODR(conf->odr);
> > +     ret = regmap_write(data->map, INV_ICM42370_REG_ACCEL_CONFIG0,
> > val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     msleep(200);
> > +
> > +     /* update internal conf */
> 
> Redundant comment.

Will remove this.

> 
> > +     data->conf = *conf;
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * inv_icm42370_set_pwr_mgmt0() - set the PWR_MGMT0 register for
> > sensor
> > + *
> > + * @dev_data: pointer to struct containing the sensor data
> > + * @accel: enum of sensor power mode
> > + * @sleep_ms: pointer to check how long the sensor is in sleep
> > mode
> > + *
> > + * Returns 0 on success, negative errno on error
> > + */
> > +static int inv_icm42370_set_pwr_mgmt0(struct inv_icm42370_data
> > *dev_data,
> > +                                   enum inv_icm42370_sensor_mode
> > accel,
> > +                                   unsigned int *sleep_ms)
> > +{
> > +     enum inv_icm42370_sensor_mode oldaccel = dev_data->conf.mode;
> > +     unsigned int sleepval;
> > +     unsigned int val;
> > +     int ret;
> > +
> > +     /* if nothing changed, exit */
> > +     if (accel == oldaccel)
> > +             return 0;
> > +
> > +     val = INV_ICM42370_PWR_MGMT0(accel);
> > +     ret = regmap_write(dev_data->map, INV_ICM42370_REG_PWR_MGMT0,
> > val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     dev_data->conf.mode = accel;
> > +     dev_data->sensor_state->power_mode = accel;
> > +
> > +     /* compute required wait time for sensors to stabilize */
> > +     sleepval = 0;
> > +     /* accel startup time */
> > +     if (accel != oldaccel && oldaccel ==
> > INV_ICM42370_SENSOR_MODE_OFF) {
> > +             /* block any register write for at least 200 µs */
> > +             usleep_range(200, 300);
> 
> Use fsleep() here as well.
> 
> > +             if (sleepval < INV_ICM42370_ACCEL_STARTUP_TIME_MS)
> > +                     sleepval =
> > INV_ICM42370_ACCEL_STARTUP_TIME_MS;
> > +     }
> > +
> > +     /* deferred sleep value if sleep pointer is provided or
> > direct sleep */
> > +     if (sleep_ms)
> > +             *sleep_ms = sleepval;
> > +     else if (sleepval)
> > +             msleep(sleepval);
> > +
> > +     return 0;
> > +}
> > +
> > +static void inv_icm42370_disable_pm(void *_data)
> > +{
> > +     struct device *dev = _data;
> > +
> > +     pm_runtime_put_sync(dev);
> > +     pm_runtime_disable(dev);
> > +}
> > +
> > +/**
> > + * inv_icm42370_set_accel_conf() - set configuration data for
> > accelerometer
> > + *
> > + * @dev_data: pointer to struct containing the sensor data
> > + * @conf: pointer to configuration data
> > + * @sleep_ms: pointer to check how long the sensor is in sleep
> > mode
> > + *
> > + * Returns 0 on success, negative errno on error
> > + */
> > +int inv_icm42370_set_accel_conf(struct inv_icm42370_data
> > *dev_data,
> > +                             struct inv_icm42370_conf *conf,
> > +                             unsigned int *sleep_ms)
> > +{
> > +     struct inv_icm42370_conf *oldconf = &dev_data->conf;
> > +     unsigned int val;
> > +     int ret;
> > +
> > +     /* sanitize missing values with current values */
> > +     if (conf->mode < 0)
> > +             conf->mode = oldconf->mode;
> > +     if (conf->fs < 0)
> > +             conf->fs = oldconf->fs;
> > +     if (conf->odr < 0)
> > +             conf->odr = oldconf->odr;
> > +     if (conf->filter < 0)
> > +             conf->filter = oldconf->filter;
> > +
> > +     /* force power mode against ODR when sensor is on */
> > +     switch (conf->mode) {
> > +     case INV_ICM42370_SENSOR_MODE_LOW_POWER:
> > +     case INV_ICM42370_SENSOR_MODE_LOW_NOISE:
> > +             if (conf->odr <= INV_ICM42370_ODR_800HZ_LN) {
> > +                     conf->mode =
> > INV_ICM42370_SENSOR_MODE_LOW_NOISE;
> > +                     conf->filter =
> > +                            
> > INV_ICM42370_UI_FILT_BW_LP_FILTER_BYPASSED;
> > +             } else if (conf->odr == INV_ICM42370_ODR_400HZ) {
> > +                     if (conf->filter ==
> > INV_ICM42370_FILTER_AVG_16X ||
> > +                         conf->filter ==
> > INV_ICM42370_FILTER_AVG_32X ||
> > +                         conf->filter ==
> > INV_ICM42370_FILTER_AVG_64X) {
> > +                             conf->mode =
> > INV_ICM42370_SENSOR_MODE_LOW_NOISE;
> > +                     } else {
> > +                             conf->mode =
> > INV_ICM42370_SENSOR_MODE_LOW_POWER;
> > +                     }
> > +             } else if (conf->odr == INV_ICM42370_ODR_200HZ &&
> > +                        conf->filter ==
> > INV_ICM42370_FILTER_AVG_64X) {
> > +                     conf->mode =
> > INV_ICM42370_SENSOR_MODE_LOW_NOISE;
> > +                     conf->filter =
> > +                            
> > INV_ICM42370_UI_FILT_BW_LP_FILTER_BYPASSED;
> > +             } else if (conf->odr >= INV_ICM42370_ODR_6_25HZ_LP) {
> > +                     conf->mode =
> > INV_ICM42370_SENSOR_MODE_LOW_POWER;
> > +                     conf->filter = INV_ICM42370_FILTER_AVG_16X;
> > +             }
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     /* set ACCEL_CONFIG0 register (accel fullscale & odr) */
> > +     if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> > +             val = INV_ICM42370_ACCEL_CONFIG0_FS(conf->fs) |
> > +                   INV_ICM42370_ACCEL_CONFIG0_ODR(conf->odr);
> > +             ret = regmap_write(dev_data->map,
> > +                                INV_ICM42370_REG_ACCEL_CONFIG0,
> > val);
> > +             if (ret)
> > +                     return ret;
> 
> Maybe a personal opinion as well, but readability is an issue
> sometimes.
> Adding blank lines to separate blocks of code is fine! (like here for
> example)

Will reformat to make it a bit cleaner by adding spaces.
> 
> > +             oldconf->fs = conf->fs;
> > +             oldconf->odr = conf->odr;
> > +     }
> > +
> > +     /* set PWR_MGMT0 register (accel sensor mode) */
> > +     return inv_icm42370_set_pwr_mgmt0(dev_data, dev_data-
> > >conf.mode,
> > +                                       sleep_ms);
> > +}
> > +
> > 
[snip]
> > +static irqreturn_t inv_icm42370_irq_timestamp(int irq, void
> > *_data)
> > +{
> > +     struct inv_icm42370_data *dev_data = _data;
> > +
> > +     dev_data->timestamp = iio_get_time_ns(dev_data->indio_accel);
> > +
> > +     return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data)
> > +{
> > +     struct inv_icm42370_data *dev_data = _data;
> > +     unsigned int status;
> > +     int ret;
> > +
> > +     mutex_lock(&dev_data->lock);
> 
> Use guard(mutex) from cleanup.h, it eliminates the need to use gotos
> and
> labels for cleaning up functions (a lot of examples in IIO for this).

Will convert all instances of mutex operations with a guard(mutex).

> > +
> > +     ret = regmap_read(dev_data->map, INV_ICM42370_REG_INT_STATUS,
> > &status);
> > +     if (ret)
> > +             goto out_unlock;
> > +
> > +out_unlock:
> > +     mutex_unlock(&dev_data->lock);
> > +     return IRQ_HANDLED;
> 
> Not sure, but is it okay to always return IRQ_HANDLED, even on regmap
> failure?

From some reference drivers it seems to be common, but we will
investigate this.

> 
> > +}
> > +
> > +/**
> > + * inv_icm42370_irq_init() - initialize int pin and interrupt
> > handler
> > + * @data:            driver internal state
> > + * @irq:     irq number
> > + * @irq_type:        irq trigger type
> > + * @open_drain:      true if irq is open drain, false for push-
> > pull
> > + *
> > + * Returns 0 on success, a negative error code otherwise.
> > + */
> > +static int inv_icm42370_irq_init(struct inv_icm42370_data *data,
> > int irq,
> > +                              int irq_type, bool open_drain)
> > +{
> > +     struct device *dev = regmap_get_device(data->map);
> > +     u8 val;
> > +     int ret;
> > +
> > +     /* configure INT1 interrupt: default is active low on edge */
> > +     switch (irq_type) {
> > +     case IRQF_TRIGGER_RISING:
> > +     case IRQF_TRIGGER_HIGH:
> > +             val = INV_ICM42370_INT_CONFIG_INT1_ACTIVE_HIGH;
> > +             break;
> > +     default:
> > +             val = INV_ICM42370_INT_CONFIG_INT1_ACTIVE_LOW;
> > +             break;
> > +     }
> > +
> > +     switch (irq_type) {
> > +     case IRQF_TRIGGER_LOW:
> > +     case IRQF_TRIGGER_HIGH:
> > +             val |= INV_ICM42370_INT_CONFIG_INT1_LATCHED;
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     if (!open_drain)
> > +             val |= INV_ICM42370_INT_CONFIG_INT1_PUSH_PULL;
> > +
> > +     ret = regmap_write(data->map, INV_ICM42370_REG_INT_CONFIG,
> > val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Deassert async reset for proper INT pin operation (cf
> > datasheet) */
> > +     ret = inv_icm42370_mreg_read(data->map, INV_ICM42370_MREG1,
> > +                                  INV_ICM42370_REG_INT_CONFIG1,
> > &val);
> > +     if (ret)
> > +             return ret;
> 
> Blank line.

Will add here.

> 
> > +     val &= ~INV_ICM42370_INT_CONFIG1_ASYNC_RESET;
> > +
> > +     ret = inv_icm42370_mreg_write(data->map, INV_ICM42370_MREG1,
> > +                                   INV_ICM42370_REG_INT_CONFIG1,
> > val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     irq_type |= IRQF_ONESHOT;
> > +     return devm_request_threaded_irq(dev, irq,
> > inv_icm42370_irq_timestamp,
> > +                                      inv_icm42370_irq_handler,
> > irq_type,
> > +                                      "inv_icm42370", data);
> > +}
> > +
> > +/*
> > + * Calibration bias values, IIO range format int + micro.
> > + * Value is limited to +/-1g coded on 12 bits signed. Step is
> > 0.5mg.
> > + */
> > +static int inv_icm42370_accel_calibbias[] = {
> > +     -10, 42010, /* min: -2^12 * 0.0005 * 9.80665 = -10.042010
> > m/s² */
> > +     0,   4903, /* step: 0.5 * 0.00980655 = 0.004903 m/s² */
> > +     10,  37106, /* max: (2^12 - 1) * 0.0005 * 9.80665 = 10.037106
> > m/s² */
> > +};
> > +
> > +/**
> > + * inv_icm42370_temp_read() - internal function to access
> > temperature sensor registers
> > + *
> > + * @dev_data: pointer to struct containing the sensor data
> > + * @temp: pointer containing the temperature data in s16 format
> > + *
> > + * Return 0 on success, negative errno on error
> > + */
> > +static int inv_icm42370_temp_read(struct inv_icm42370_data
> > *dev_data, s16 *temp)
> > +{
> > +     struct device *dev = regmap_get_device(dev_data->map);
> > +     __be16 *raw;
> > +     int ret;
> > +
> > +     pm_runtime_get_sync(dev);
> 
> Please check out PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND, it
> utilizes the cleanup.h
> magic as guard(mutex).
> 
> > +     mutex_lock(&dev_data->lock);
> 
> guard(mutex) combined with the macro above removes the need for gotos
> and the exit
> label.

Will convert the pm_runtime_get_sync() callbacks to use the newer
methods.

> 
> > +
> > +     raw = (__be16 *)&dev_data->buffer[0];
> > +     ret = regmap_bulk_read(dev_data->map,
> > INV_ICM42370_REG_TEMP_DATA1, raw,
> > +                            sizeof(*raw));
> > +     if (ret)
> > +             goto exit;
> > +
> > +     *temp = (s16)be16_to_cpup(raw);
> > +
> > +     /*
> > +      * Temperature data is invalid if both accel and gyro are
> > off.
> > +      * Return -EBUSY in this case.
> > +      */
> > +     if (*temp == INV_ICM42370_DATA_INVALID)
> > +             ret = -EBUSY;
> > +
> > 

[snip]

> > +
> > +     /*
> > +      * convert raw offset to g then to m/s²
> > +      * 12 bits signed raw step 0.5mg to g: 5 / 10000
> > +      * g to m/s²: 9.806650
> > +      * result in micro (1000000)
> > +      * (offset * 5 * 9.806650 * 1000000) / 10000
> > +      */
> > +     val64 = (s64)offset * 5LL * 9806650LL;
> > +     /* for rounding, add + or - divisor (10000) divided by 2 */
> > +     if (val64 >= 0)
> > +             val64 += 10000LL / 2LL;
> > +     else
> > +             val64 -= 10000LL / 2LL;
> > +     bias = div_s64(val64, 10000L);
> 
> This is probably the last time I'll mention a blank line in this
> review, but
> there are more places in this patch where they should be added.

Will do our best to add more empty lines to make it more readable.

> > +     *val = bias / 1000000L;
> > +     *val2 = bias % 1000000L;
> > +
> > +     return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +/**
> > + * inv_icm42370_accel_write_offset() - write offset values to the
> > accelerometer
> > + *
> > + * @dev_data: pointer to struct containing the sensor data
> > + * @chan: pointer to iio channel specification
> > + * @val: integer part of the value
> > + * @val2: decimal part of the value
> > + *
> > + * Returns 0 on success, negative errno on error
> > + */
> > +static int inv_icm42370_accel_write_offset(struct
> > inv_icm42370_data *dev_data,
> > +                                        struct iio_chan_spec const
> > *chan,
> > +                                        int val, int val2)
> > +{
> > +     struct device *dev = regmap_get_device(dev_data->map);
> > +     s64 val64;
> > +     s32 min, max;
> > +     unsigned int regval;
> > +     s16 offset;
> > +     int ret;
> > +
> > +     if (chan->type != IIO_ACCEL)
> > +             return -EINVAL;
> > +
> > +     /* inv_icm42370_accel_calibbias: min - step - max in micro */
> > +     min = inv_icm42370_accel_calibbias[0] * 1000000L +
> 
> This would benefit from using the macros from units.h.

Will use values from units.h

> 
> > +           inv_icm42370_accel_calibbias[1];
> > +     max = inv_icm42370_accel_calibbias[4] * 1000000L +
> > +           inv_icm42370_accel_calibbias[5];
> > +     val64 = (s64)val * 1000000LL + (s64)val2;
> > +     if (val64 < min || val64 > max)
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * convert m/s² to g then to raw value
> > +      * m/s² to g: 1 / 9.806650
> > +      * g to raw 12 bits signed, step 0.5mg: 10000 / 5
> > +      * val in micro (1000000)
> > +      * val * 10000 / (9.806650 * 1000000 * 5)
> > +      */
> > +     val64 = val64 * 10000LL;
> > +     /* for rounding, add + or - divisor (9806650 * 5) divided by
> > 2 */
> > +     if (val64 >= 0)
> > +             val64 += 9806650 * 5 / 2;
> > +     else
> > +             val64 -= 9806650 * 5 / 2;
> > +     offset = div_s64(val64, 9806650 * 5);
> > +
> > +     /* clamp value limited to 12 bits signed */
> > +     if (offset < -2048)
> > +             offset = -2048;
> > +     else if (offset > 2047)
> > +             offset = 2047;
> > +
> > +     pm_runtime_get_sync(dev);
> > +     mutex_lock(&dev_data->lock);
> 
> The pm_runtime and guard_macros would clean this up nicely.

Will do here.

> 
> > +
> > +     switch (chan->channel2) {
> > +     case IIO_MOD_X:
> > +             /* OFFSET_USER4 register is shared */
> > +             ret = inv_icm42370_mreg_read(dev_data->map,
> > INV_ICM42370_MREG1,
> > +                                         
> > INV_ICM42370_REG_OFFSET_USER4,
> > +                                          (u8 *)&regval);
> > +             if (ret)
> > +                     goto out_unlock;
> > +             dev_data->buffer[0] = ((offset & 0xF00) >> 4) |
> > (regval & 0x0F);
> > +             dev_data->buffer[1] = offset & 0xFF;
> > +
> > +             ret = inv_icm42370_mreg_write(dev_data->map,
> > INV_ICM42370_MREG1,
> > +                                          
> > INV_ICM42370_REG_OFFSET_USER4,
> > +                                           dev_data->buffer[0]);
> > +             if (ret)
> > +                     goto out_unlock;
> > 

[snip]

> > +}
> > +
> > +/**
> > + * inv_icm42370_accel_read_sensor() - internal function to read
> > accelerometer sensor registers
> > + *
> > + * @indio_dev: pointer to the industrial io struct
> 
> I/O.

Will use the abbrevation.

> > + * @chan: pointer to iio channel specification
> > + * @val: pointer containing accelerometer data in s16 format
> > + *
> > + * Return 0 on success, negative errno on error
> > + */
> > +static int inv_icm42370_accel_read_sensor(struct iio_dev
> > *indio_dev,
> > +                                       struct iio_chan_spec const
> > *chan,
> > +                                       s16 *val)
> > 

[snip]

> > +      * clock period is 32kHz (31250ns)
> > +      * jitter is +/- 2% (20 per mille)
> > +      */
> > +     ts_chip.clock_period = 31250;
> > +     ts_chip.jitter = 20;
> > +     ts_chip.init_period = inv_icm42370_odr_to_period(data-
> > >conf.odr);
> > +     inv_sensors_timestamp_init(&data->sensor_state->ts,
> > &ts_chip);
> > +
> > +     indio_dev->name = "inv_icm42370";
> > +     indio_dev->info = &inv_icm42370_info;
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +     indio_dev->channels = inv_icm42370_accel_channels;
> > +     indio_dev->num_channels =
> > ARRAY_SIZE(inv_icm42370_accel_channels);
> > +
> > +     ret = devm_iio_device_register(dev, indio_dev);
> 
> You register the device before initializing IRQs and PM runtime,
> causing potential
> race conditions as userspace could already interact with the device
> before probe()
> has finished.

Will go through the setup to fix the potential race condition.
> 
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     return indio_dev;
> > +}
> > +
> > +/**
> > + * inv_icm42370_core_probe() - initialize and register the ICM-
> > 42370 device
> > + * @regmap:  register map for accessing the device's registers.
> > + * @chip:    chip identifier, must be %INV_CHIP_ICM42370.
> > + * @irq:     interrupt number for the device's data-ready signal.
> > + * @bus_setup:       callback to configure bus-specific settings
> > (e.g. I2C).
> > + *
> > + * Returns 0 on success, a negative error code otherwise.
> > + */
> > +int inv_icm42370_core_probe(struct regmap *regmap, int chip, int
> > irq,
> > +                         inv_icm42370_bus_setup bus_setup)
> > +{
> > +     struct device *dev = regmap_get_device(regmap);
> > +     struct inv_icm42370_data *data;
> > +     struct iio_dev *indio_dev;
> > +     struct irq_data *irq_desc;
> > +     int irq_type;
> > +     bool open_drain;
> > +     int ret;
> > +
> > +     if (chip != INV_CHIP_ICM42370) {
> > +             dev_err(dev, "invalid chip = %d\n", chip);
> > +             return -ENODEV;
> > +     }
> > +
> > +     /* get irq properties, set trigger falling by default */
> > +     irq_desc = irq_get_irq_data(irq);
> > +     if (!irq_desc) {
> > +             dev_err(dev, "could not find IRQ %d\n", irq);
> 
> dev_err_probe() would be better.

Will convert it to dev_err_probe()

> 
> > +             return -EINVAL;
> > +     }
> > +
> > +     irq_type = irqd_get_trigger_type(irq_desc);
> > +     if (!irq_type)
> > +             irq_type = IRQF_TRIGGER_FALLING;
> > +
> > +     open_drain = device_property_read_bool(dev, "drive-open-
> > drain");
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     data = iio_priv(indio_dev);
> > +     data->sensor_state =
> > +             devm_kzalloc(dev, sizeof(*data->sensor_state),
> > GFP_KERNEL);
> > +     if (!data->sensor_state)
> > +             return -ENOMEM;
> > +
> > +     mutex_init(&data->lock);
> 
> devm_mutex_init() and check the return value.

Will convert it to devm_mutex_init() and check for the return value.

> 
> > +     data->chip = chip;
> > +     data->map = regmap;
> > +
> > +     data->vdd_supply = devm_regulator_get(dev, "vdd");
> 
> devm_regulator_get_enable will also handle the disabling on unbind.
> Same
> for data->vddio.

Will convert it to use devm_regulator_get_enable().
> 
> > +     if (IS_ERR(data->vdd_supply))
> > +             return PTR_ERR(data->vdd_supply);
> 
> dev_error_probe() here and below as well (if you want to print an
> error message)

Will add the error message with dev_error_probe()

> 
> > +
> > +     data->vddio_supply = devm_regulator_get(dev, "vddio");
> > +     if (IS_ERR(data->vddio_supply))
> > +             return PTR_ERR(data->vddio_supply);
> > +
> > +     ret = regulator_enable(data->vdd_supply);
> 
> Use devm_regulator_get_enable() as mentioned above, you wouldn't need
> this then.

Will fix this.

> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = inv_icm42370_setup(data, bus_setup);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "Setup failed\n");
> > +
> > +     data->indio_accel = inv_icm42370_accel_init(indio_dev, data);
> > +     if (IS_ERR(data->indio_accel))
> > +             return PTR_ERR(data->indio_accel);
> > +
> > +     ret = inv_icm42370_irq_init(data, irq, irq_type, open_drain);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* setup runtime power management */
> > +     ret = pm_runtime_set_active(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     pm_runtime_get_noresume(dev);
> > +     pm_runtime_enable(dev);
> > +     pm_runtime_use_autosuspend(dev);
> > +     pm_runtime_put(dev);
> > +
> > +     return devm_add_action_or_reset(dev, inv_icm42370_disable_pm,
> > dev);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(inv_icm42370_core_probe, "IIO_ICM42370");
> > +
> > +MODULE_AUTHOR("Kanak Shilledar <kanak.shilledar@axis.com>");
> > +MODULE_AUTHOR("Henrik Grimler <henrik.grimler@axis.com>");
> > +MODULE_DESCRIPTION("Invensense device ICM42370 driver");
> 
> No point in having the word "device" in the description.

Will remove it from the description.

> 
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS("IIO_INV_SENSORS_TIMESTAMP");
> > diff --git a/drivers/iio/accel/inv_icm42370_i2c.c
> > b/drivers/iio/accel/inv_icm42370_i2c.c
> > new file mode 100644
> > index 0000000000000..57e97d66329a1
> > --- /dev/null
> > +++ b/drivers/iio/accel/inv_icm42370_i2c.c
> > @@ -0,0 +1,103 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2020 InvenSense, Inc.
> > + * Copyright (C) 2026 Axis Communications AB
> > + */
> > +
> > +#include <linux/kernel.h>
> 
> Don't include this in new drivers.
> 
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> 
> Remove this.

Will remove the suggested includes and sort them alphabetically.
> 
> > +#include <linux/i2c.h>
> > +#include <linux/regmap.h>
> > +#include <linux/property.h>
> > +
> > +#include "inv_icm42370.h"
> > +
> > +/**
> > + * inv_icm42370_i2c_bus_setup() - I2C bus setup for icm42370
> > + *
> > + * @data: pointer to struct containing the sensor data
> > + *
> > + * Returns 0 on success, negative errno on error
> > + */
> > +static int inv_icm42370_i2c_bus_setup(struct inv_icm42370_data
> > *data)
> > +{
> > +     unsigned int mask, val;
> > +     int ret;
> > +
> > +     /* set slew rates for I2C */
> > +     mask = INV_ICM42370_DRIVE_CONFIG2_I2C_MASK;
> > +     val =
> > INV_ICM42370_DRIVE_CONFIG2_I2C(INV_ICM42370_SLEW_RATE_12_36NS);
> > +     ret = regmap_update_bits(data->map,
> > INV_ICM42370_REG_DRIVE_CONFIG2,
> > +                              mask, val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* set slew rates for SPI */
> > +     mask = INV_ICM42370_DRIVE_CONFIG3_SPI_MASK;
> > +     val =
> > INV_ICM42370_DRIVE_CONFIG3_SPI(INV_ICM42370_SLEW_RATE_12_36NS);
> > +     ret = regmap_update_bits(data->map,
> > INV_ICM42370_REG_DRIVE_CONFIG3,
> > +                              mask, val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +static int inv_icm42370_probe(struct i2c_client *client)
> > +{
> > +     const void *match;
> > +     enum inv_icm42370_chip chip;
> > +     struct regmap *regmap;
> > +
> > +     if (!i2c_check_functionality(client->adapter,
> > I2C_FUNC_SMBUS_I2C_BLOCK))
> > +             return -EOPNOTSUPP;
> > +
> > +     match = device_get_match_data(&client->dev);
> > +     if (!match)
> > +             return -EINVAL;
> > +     chip = (uintptr_t)match;
> > +
> > +     regmap = devm_regmap_init_i2c(client,
> > &inv_icm42370_regmap_config);
> > +     if (IS_ERR(regmap))
> > +             return PTR_ERR(regmap);
> > +
> > +     return inv_icm42370_core_probe(regmap, chip, client->irq,
> > +                                    inv_icm42370_i2c_bus_setup);
> > +}
> > +
> > +/**
> > + * device id table is used to identify what device can be
> > supported by this driver
> > + */
> > +static const struct i2c_device_id inv_icm42370_id[] = { {
> > "icm42370",
> > +                                                      
> > INV_CHIP_ICM42370 },
> > +                                                     {} };
> 
> The formatting is awful, fix it to something like:
> static const struct i2c_device_id inv_icm42370_id[] = {
>         { .compatible = "icm42370", .data = INV_CHIP_ICM42370 },
>         { }
> };
> 
> Also the use of named initializers is preferred.

Will fix it.

> > +MODULE_DEVICE_TABLE(i2c, inv_icm42370_id);
> > +
> > +/**
> > + * inv_icm42370_of_matches - struct for all the compatibe strings
> > + *
> > + */
> > +static const struct of_device_id inv_icm42370_of_matches[] = {
> > +     {
> > +             .compatible = "invensense,icm42370",
> > +             .data = (void *)INV_CHIP_ICM42370,
> > +     },
> 
> You can put the two on one line.

Will fix it.

> 
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, inv_icm42370_of_matches);
> > +
> > +static struct i2c_driver inv_icm42370_driver = {
> > +     .driver = {
> > +             .name = "inv-icm42370-i2c",
> > +             .of_match_table = inv_icm42370_of_matches,
> > +     },
> > +     .probe = inv_icm42370_probe,
> > +};
> > +module_i2c_driver(inv_icm42370_driver);
> > +
> > +MODULE_AUTHOR("Kanak Shilledar <kanak.shilledar@axis.com>");
> > +MODULE_AUTHOR("Henrik Grimler <henrik.grimler@axis.com>");
> > +MODULE_DESCRIPTION("InvenSense ICM-42370P I2C driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS("IIO_ICM42370");
> > 
> 
> 
> 
> --
> Kind regards,
> Joshua Crofts

Thanks and Regards,
Kanak Shilledar


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]