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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] The accelerometer offset calibration read and write logic is fundamentally broken, containing missing reads, swapped registers, uninitialized memory aliasing, and ignored error codes.
- [High] Unbalanced `pm_runtime_put_sync()` in the device cleanup action causes a PM runtime usage counter underflow.
- [High] The driver systematically ignores the return value of `pm_runtime_get_sync()` before accessing hardware registers.
- [High] The interrupt handler accesses hardware registers without ensuring the device is in an active PM state.
- [High] The `vdd_supply` regulator is enabled but never disabled, and the `vddio_supply` is acquired but never enabled, leading to resource leaks and potential unpowered hardware.
- [High] Incorrect scale calculation in `inv_icm42370_accel_scale` results in returned IIO values being twice as large as their true physical scale.
- [High] Race condition and teardown order inversion caused by calling `devm_iio_device_register` before IRQ and PM initialization.
- [High] The DMA buffer shares a CPU cacheline with other frequently modified fields, risking memory corruption.
- [Medium] Incorrect fixed-point math for negative IIO values breaks calibration bias limit checks and user input parsing.
- [Low] The I2C device ID array is initialized without C99 named initializers, violating subsystem guidelines.
- [Low] Compilation failure: `struct inv_icm42370_fifo` is an incomplete type.
--

--- Patch [2]: [PATCH 2/3] iio: accel: Add support for ICM42370P ---
commit 428e7955fe02229e2d71685724240ab27a044dd8
Author: Kanak Shilledar <kanak.shilledar@axis.com>
Date:   Thu Aug 6 12:46:28 2026 +0000

    iio: accel: Add support for ICM42370P
    
    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.

> diff --git a/drivers/iio/accel/inv_icm42370.h b/drivers/iio/accel/inv_icm42370.h
> index 0000000000000..9866a5e970dcd
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370.h
[ ... ]
> @@ -0,0 +1,365 @@
> +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;
> +	struct inv_icm42370_sensor_state *sensor_state;
> +	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);

[Severity: High]
Does aligning the start of the buffer guarantee the end is padded? If the
buffer shares a cacheline with frequently modified fields like timestamp,
could a DMA cache invalidation corrupt concurrent CPU writes to those
adjacent fields?

> +	s16 accel_calibbias[3];
> +	struct inv_icm42370_fifo fifo;

[Severity: Low]
Is struct inv_icm42370_fifo defined anywhere? It seems to be an incomplete
type which might cause compilation failures.

> +	s64 timestamp;
> +	enum inv_icm42370_chip chip;
> +	struct inv_icm42370_conf conf;
> +};
[ ... ]
> diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c
> index 0000000000000..9f6c302e6f331
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370_core.c
[ ... ]
> @@ -0,0 +1,1251 @@
> +/* IIO format int + nano */
> +static const int inv_icm42370_accel_scale[] = {
> +	/* +/- 16G => 2*16*9.80665 / (2**15) m/s-2 */
> +	[2 * INV_ICM42370_ACCEL_FS_16G] = 0,
> +	[2 * INV_ICM42370_ACCEL_FS_16G + 1] = 9576807,

[Severity: High]
Does this scale calculation double the true physical scale? Since a total 32G
span (+/-16G) fits in a signed 16-bit integer, shouldn't the multiplier be
16 * 9.80665 / 32768 rather than 2 * 16 * 9.80665 / 32768?

[ ... ]
> +static void inv_icm42370_disable_pm(void *_data)
> +{
> +	struct device *dev = _data;
> +
> +	pm_runtime_put_sync(dev);

[Severity: High]
Will this unconditionally drop the PM usage counter to -1 during unbind or
probe error? Since inv_icm42370_core_probe() balances the get and put, the
counter should already be 0 when this devm cleanup action runs.

> +	pm_runtime_disable(dev);
> +}
[ ... ]
> +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);
> +
> +	ret = regmap_read(dev_data->map, INV_ICM42370_REG_INT_STATUS, &status);

[Severity: High]
Should this threaded interrupt handler ensure the device is in an active PM
state before accessing hardware registers? If the device is suspended, could
this read fail or cause an I2C bus lockup?

> +	if (ret)
> +		goto out_unlock;
[ ... ]
> +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);

[Severity: High]
What happens if the device is suspended and pm_runtime_get_sync() fails?
Should the return value be checked before proceeding to read the temperature,
to avoid reading garbage data or locking the bus?

> +	mutex_lock(&dev_data->lock);
> +
> +	raw = (__be16 *)&dev_data->buffer[0];
> +	ret = regmap_bulk_read(dev_data->map, INV_ICM42370_REG_TEMP_DATA1, raw,
> +			       sizeof(*raw));
[ ... ]
> +static int inv_icm42370_accel_read_offset(struct inv_icm42370_data *dev_data,
> +					  struct iio_chan_spec const *chan,
> +					  int *val, int *val2)
> +{
[ ... ]
> +	pm_runtime_get_sync(dev);
> +	mutex_lock(&dev_data->lock);
> +
> +	ret = inv_icm42370_mreg_read(dev_data->map, INV_ICM42370_MREG1, reg,
> +				     dev_data->buffer);
> +	memcpy(data, dev_data->buffer, sizeof(data));

[Severity: High]
Is it possible that the second byte of the offset remains uninitialized? Since
inv_icm42370_mreg_read() only reads one byte into buffer[0], copying into data
will include uninitialized memory.

[ ... ]
> +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 +
> +	      inv_icm42370_accel_calibbias[1];
> +	max = inv_icm42370_accel_calibbias[4] * 1000000L +
> +	      inv_icm42370_accel_calibbias[5];
> +	val64 = (s64)val * 1000000LL + (s64)val2;

[Severity: Medium]
Will this fixed-point math yield incorrect results for negative inputs? For
instance, if val is -10 and val2 is 42010, val64 will be -9957990 instead of
-10042010.

> +	if (val64 < min || val64 > max)
> +		return -EINVAL;
[ ... ]
> +	pm_runtime_get_sync(dev);

[Severity: High]
Should this check the return value of pm_runtime_get_sync() before writing
offsets? A failure here could lead to writing to a suspended device.

> +	mutex_lock(&dev_data->lock);
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
[ ... ]
> +	case IIO_MOD_Y:
> +		/* OFFSET_USER7 register is shared */
> +		ret = inv_icm42370_mreg_read(dev_data->map, INV_ICM42370_MREG1,
> +					     INV_ICM42370_REG_OFFSET_USER7,
> +					     (u8 *)&regval);

[Severity: High]
Does this Y-axis logic swap the upper and lower registers (USER6 and USER7)?
Also, could casting the uninitialized unsigned int regval to a u8 pointer
cause uninitialized stack memory to be written back? Lastly, the Z-axis case
below seems to ignore the mreg_read return value completely.

> +		if (ret)
> +			goto out_unlock;
[ ... ]
> +static int inv_icm42370_accel_read_sensor(struct iio_dev *indio_dev,
> +					  struct iio_chan_spec const *chan,
> +					  s16 *val)
> +{
[ ... ]
> +	pm_runtime_get_sync(dev);

[Severity: High]
Should this check the return value of pm_runtime_get_sync() before proceeding
to read the accelerometer sensor data?

> +	mutex_lock(&data->lock);
> +
> +	/* read accel register data */
> +	value = (__be16 *)&data->buffer[0];
[ ... ]
> +int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq,
> +			    inv_icm42370_bus_setup bus_setup)
> +{
[ ... ]
> +	data->vdd_supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(data->vdd_supply))
> +		return PTR_ERR(data->vdd_supply);
> +
> +	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);

[Severity: High]
Is data->vddio_supply supposed to be enabled here too? Additionally, is there
a missing devm action or remove callback to disable data->vdd_supply? It looks
like it will leak across unbinds.

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

[Severity: High]
Does calling devm_iio_device_register() (inside inv_icm42370_accel_init())
before initializing IRQs and PM runtime introduce a race condition? Userspace
could access sysfs attributes before the hardware and IRQs are fully ready.

> +	if (ret)
> +		return ret;
[ ... ]
> diff --git a/drivers/iio/accel/inv_icm42370_i2c.c b/drivers/iio/accel/inv_icm42370_i2c.c
> index 0000000000000..57e97d66329a1
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370_i2c.c
[ ... ]
> @@ -0,0 +1,103 @@
> +static const struct i2c_device_id inv_icm42370_id[] = { { "icm42370",
> +							  INV_CHIP_ICM42370 },
> +							{} };

[Severity: Low]
Should this array of struct i2c_device_id use C99 named initializers?

> +MODULE_DEVICE_TABLE(i2c, inv_icm42370_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-b4-inv_icm42370p-v1-0-670837f5842f@axis.com?part=2