* [PATCH 0/3] Add driver for Invensense ICM42370P accelerometer
@ 2026-08-06 12:46 Kanak Shilledar
2026-08-06 12:46 ` [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Kanak Shilledar @ 2026-08-06 12:46 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Grimler,
Jean-Baptiste Maneyrol
Cc: linux-iio, devicetree, linux-kernel, kernel, Kanak Shilledar
Invensense ICM42370P is a high performance MEMS MotionTracking 3-axis
accelerometer. It supports I2C, I3C and SPI protocols. It has a 2.25kB
FIFO and two programmable interrupts with support for ultra-low-power
wake-on-motion support. It has a built-in temperature sensor. This
patch series adds basic support for the sensor with functionality of
performing raw reads and writes along with buffered reads via the I2C
interface.
This device contains 4 register banks for configuring the device called
MREG0, MREG1, MREG2 and MREG3. Unlike other devices from the same
vendor, this contains a very different way of accessing the register
banks apart from the default user bank 0 (MREG0). The register bank access
procedure is mentioned in the datasheet [1] Section 13. Due to this
specific reason and that it doesn't have a gyro sensor, it is implemented
as a standalone driver.
This device has 3 formats for the FIFO structure (datasheet [1] Section
6), currently only FIFO packet format 1 is supported, others
formats can be added as part of future work.
The device supports EDGE and LEVEL triggered interrupts, refer the dt-binding
for example of how the devicetree should be setup.
The driver is inspired from iio/imu/inv_icm42600.
[1] https://uat.invensense.com/en-us/products/3-axis/icm-42370-p
Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com>
---
Kanak Shilledar (3):
dt-bindings: Add InvenSense ICM-42370-p accelerometer
iio: accel: Add support for ICM42370P
iio: accel: icm42370: Add FIFO buffer functionality
.../bindings/iio/accel/invensense,icm42370.yaml | 65 +
MAINTAINERS | 9 +
drivers/iio/accel/Kconfig | 18 +
drivers/iio/accel/Makefile | 6 +
drivers/iio/accel/inv_icm42370.h | 371 +++++
drivers/iio/accel/inv_icm42370_buffer.c | 504 +++++++
drivers/iio/accel/inv_icm42370_buffer.h | 97 ++
drivers/iio/accel/inv_icm42370_core.c | 1454 ++++++++++++++++++++
drivers/iio/accel/inv_icm42370_i2c.c | 103 ++
9 files changed, 2627 insertions(+)
---
base-commit: 350d1fb9204b13c5f95e511e98b8bcb47574d425
change-id: 20260629-b4-inv_icm42370p-ccd671066bcf
Best regards,
--
Kanak Shilledar <kanak.shilledar@axis.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer 2026-08-06 12:46 [PATCH 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar @ 2026-08-06 12:46 ` Kanak Shilledar 2026-08-06 12:53 ` sashiko-bot 2026-08-07 10:16 ` Joshua Crofts 2026-08-06 12:46 ` [PATCH 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar 2026-08-06 12:46 ` [PATCH 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar 2 siblings, 2 replies; 14+ messages in thread From: Kanak Shilledar @ 2026-08-06 12:46 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Grimler, Jean-Baptiste Maneyrol Cc: linux-iio, devicetree, linux-kernel, kernel, Kanak Shilledar ICM42370P is a 3-axis accelerometer. The device can support I2C, SPI and I3C. Add the supporting devicetree documentation for I2C as we have only tested using I2C protocol and leave the reset for future work. The device supports VDD and VDDIO operating range of 1.71V to 3.6V. Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com> --- .../bindings/iio/accel/invensense,icm42370.yaml | 65 ++++++++++++++++++++++ MAINTAINERS | 8 +++ 2 files changed, 73 insertions(+) diff --git a/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml new file mode 100644 index 0000000000000..561e798b911ba --- /dev/null +++ b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml @@ -0,0 +1,65 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/accel/invensense,icm42370.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: InvenSense ICM-42370 Accelerometer + +maintainers: + - Kanak Shilledar <kanak.shilledar@axis.com> + - Henrik Grimler <henrik.grimler@axis.com> + +description: | + 3-axis accelerometer MotionTracking device. + + It supports I3C, I2C and SPI serial communication, has a 2.25kB FIFO + and 2 programmable interrupts with low-power wake-on-motion support. + + It also has programmable filters and an embedded temperature sensor. + + https://uat.invensense.com/en-us/products/3-axis/icm-42370-p + +properties: + compatible: + const: invensense,icm42370 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + drive-open-drain: + type: boolean + + vdd-supply: + description: Regulator operating range between 1.71V to 3.6V. + + vddio-supply: + description: Regulator operating range between 1.71V to 3.6V. + +required: + - compatible + - reg + - interrupts + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + icm42370@69 { + compatible = "invensense,icm42370"; + reg = <0x69>; + interrupt-parent = <&gpio1>; + interrupts = <7 IRQ_TYPE_EDGE_FALLING>; + vdd-supply = <&vdd>; + vddio-supply = <&vddio>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 04fa5322d9f74..f7039df58bb0f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13561,6 +13561,14 @@ S: Maintained F: Documentation/devicetree/bindings/media/i2c/isil,isl79987.yaml F: drivers/media/i2c/isl7998x.c +INVENSENSE ICM-42370P ACCELEROMETER +M: Kanak Shilledar <kanak.shilledar@axis.com> +M: Henrik Grimler <henrik.grimler@axis.com> +L: linux-iio@vger.kernel.org +S: Maintained +W: https://invensense.tdk.com/ +F: Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml + INVENSENSE ICM-426xx IMU DRIVER M: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com> L: linux-iio@vger.kernel.org -- 2.43.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer 2026-08-06 12:46 ` [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar @ 2026-08-06 12:53 ` sashiko-bot 2026-08-07 10:16 ` Joshua Crofts 1 sibling, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-06 12:53 UTC (permalink / raw) To: Kanak Shilledar; +Cc: robh, conor+dt, devicetree Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Medium] The binding intentionally omits support for SPI and I3C protocols, breaking schema validation for valid hardware configurations. - [Medium] The `interrupts` property is restricted to a single item, contradicting the device's 2 programmable interrupts. - [Medium] The schema is missing the `mount-matrix` property, which is essential for 3-axis accelerometers. - [Low] The binding includes a fragile link to a staging (UAT) website instead of a production URL. - [Low] The compatible string drops the 'P' suffix from the part number, risking future naming collisions. -- --- Patch [1]: [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer --- commit 12d3935884516d76927755afe69d0ed970ee7417 Author: Kanak Shilledar <kanak.shilledar@axis.com> dt-bindings: Add InvenSense ICM-42370-p accelerometer Adds devicetree documentation for the ICM-42370-P 3-axis accelerometer. Support is currently only described for I2C, leaving out SPI and I3C and reset for future work. > diff --git a/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml > new file mode 100644 > index 0000000000000..561e798b911ba > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml [ ... ] > +description: | > + 3-axis accelerometer MotionTracking device. > + > + It supports I3C, I2C and SPI serial communication, has a 2.25kB FIFO > + and 2 programmable interrupts with low-power wake-on-motion support. > + > + It also has programmable filters and an embedded temperature sensor. > + > + https://uat.invensense.com/en-us/products/3-axis/icm-42370-p [Severity: Low] This isn't a bug, but this documentation link points to a staging (UAT) website instead of a production URL. Could this be updated to the production URL? > + > +properties: > + compatible: > + const: invensense,icm42370 [Severity: Low] This isn't a bug, but the compatible string drops the 'P' suffix from the part number. Does this risk future naming collisions if a non-P variant exists or is released? Could the compatible string be updated to strictly match the part number invensense,icm42370p? > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 [Severity: Medium] Will this limitation of maxItems to 1 conflict with valid hardware designs that route both of the device's interrupt lines to the host? Should the schema allow up to 2 items for this property and define interrupt-names? > + > + drive-open-drain: > + type: boolean > + > + vdd-supply: > + description: Regulator operating range between 1.71V to 3.6V. > + > + vddio-supply: > + description: Regulator operating range between 1.71V to 3.6V. > + > +required: > + - compatible > + - reg > + - interrupts > + > +unevaluatedProperties: false [Severity: Medium] By omitting support for SPI and I3C protocols and setting unevaluatedProperties to false without including SPI schema references, won't this cause schema validation errors for valid hardware configurations? Standard properties like spi-max-frequency or spi-cpol will be incorrectly rejected. Should the binding describe all hardware capabilities rather than just the tested protocols? [Severity: Medium] Does the omission of the mount-matrix property, combined with unevaluatedProperties being set to false, mean that device trees describing the sensor's physical orientation on the PCB will also fail validation? Could mount-matrix be explicitly declared as true? > + > +examples: > + - | [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/20260806-b4-inv_icm42370p-v1-0-670837f5842f@axis.com?part=1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer 2026-08-06 12:46 ` [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar 2026-08-06 12:53 ` sashiko-bot @ 2026-08-07 10:16 ` Joshua Crofts 2026-08-07 13:15 ` Kanak Shilledar 1 sibling, 1 reply; 14+ messages in thread From: Joshua Crofts @ 2026-08-07 10:16 UTC (permalink / raw) To: Kanak Shilledar Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Grimler, Jean-Baptiste Maneyrol, linux-iio, devicetree, linux-kernel, kernel On Thu, 6 Aug 2026 14:46:27 +0200 Kanak Shilledar <kanak.shilledar@axis.com> wrote: > + vdd-supply: > + description: Regulator operating range between 1.71V to 3.6V. > + > + vddio-supply: > + description: Regulator operating range between 1.71V to 3.6V. > + > +required: > + - compatible > + - reg > + - interrupts vdd-supply and vddio-supply are definitely required. ... > +INVENSENSE ICM-42370P ACCELEROMETER > +M: Kanak Shilledar <kanak.shilledar@axis.com> > +M: Henrik Grimler <henrik.grimler@axis.com> > +L: linux-iio@vger.kernel.org No need to add the IIO list as get_maintainer will automatically find it based on the file path. However I did notice that you have a company kernel mailing list, perhaps add that here? -- Kind regards, Joshua Crofts ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer 2026-08-07 10:16 ` Joshua Crofts @ 2026-08-07 13:15 ` Kanak Shilledar 0 siblings, 0 replies; 14+ messages in thread From: Kanak Shilledar @ 2026-08-07 13:15 UTC (permalink / raw) To: joshua.crofts1@gmail.com Cc: dlechner@baylibre.com, Henrik Grimler, nuno.sa@analog.com, jean-baptiste.maneyrol@tdk.com, robh@kernel.org, jic23@kernel.org, andy@kernel.org, krzk+dt@kernel.org, linux-iio@vger.kernel.org, conor+dt@kernel.org, Kernel, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 1364 bytes --] Hi, Thanks for reviewing the code. On Fri, 2026-08-07 at 12:16 +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:27 +0200 > Kanak Shilledar <kanak.shilledar@axis.com> wrote: > > > + vdd-supply: > > + description: Regulator operating range between 1.71V to 3.6V. > > + > > + vddio-supply: > > + description: Regulator operating range between 1.71V to 3.6V. > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > vdd-supply and vddio-supply are definitely required. Will add them in the dt-binding as well as other properties suggested by sashiko. > ... > > > +INVENSENSE ICM-42370P ACCELEROMETER > > +M: Kanak Shilledar <kanak.shilledar@axis.com> > > +M: Henrik Grimler <henrik.grimler@axis.com> > > +L: linux-iio@vger.kernel.org > > No need to add the IIO list as get_maintainer will automatically find > it based on the file path. However I did notice that you have a > company kernel mailing list, perhaps add that here? Thanks for pointing that out. We will update it to our company kernel mailing list (i.e., kernel@axis.com). > -- > 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] iio: accel: Add support for ICM42370P 2026-08-06 12:46 [PATCH 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar 2026-08-06 12:46 ` [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar @ 2026-08-06 12:46 ` Kanak Shilledar 2026-08-06 13:02 ` sashiko-bot ` (2 more replies) 2026-08-06 12:46 ` [PATCH 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar 2 siblings, 3 replies; 14+ messages in thread From: Kanak Shilledar @ 2026-08-06 12:46 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Grimler, Jean-Baptiste Maneyrol Cc: linux-iio, devicetree, linux-kernel, kernel, Kanak Shilledar 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> --- MAINTAINERS | 1 + drivers/iio/accel/Kconfig | 17 + drivers/iio/accel/Makefile | 5 + drivers/iio/accel/inv_icm42370.h | 365 ++++++++++ drivers/iio/accel/inv_icm42370_core.c | 1251 +++++++++++++++++++++++++++++++++ drivers/iio/accel/inv_icm42370_i2c.c | 103 +++ 6 files changed, 1742 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index f7039df58bb0f..adcd8a92e26a4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13568,6 +13568,7 @@ L: linux-iio@vger.kernel.org S: Maintained W: https://invensense.tdk.com/ F: Documentation/devicetree/bindings/iio/accel/invensense,icm42370.yaml +F: drivers/iio/accel/inv_icm42370* INVENSENSE ICM-426xx IMU DRIVER M: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index 4094299e2ed81..cef3462e4c79a 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -487,6 +487,23 @@ config IIO_KX022A_I2C connected to I2C interface. Supported devices are: KX022A, KX132-1211, KX132ACR-LBZ +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> +#include <linux/iio/iio.h> +#include <linux/iio/common/inv_sensors_timestamp.h> + +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; + struct inv_icm42370_sensor_state *sensor_state; + u8 buffer[2] __aligned(IIO_DMA_MINALIGN); + s16 accel_calibbias[3]; + struct inv_icm42370_fifo fifo; + s64 timestamp; + enum inv_icm42370_chip chip; + struct inv_icm42370_conf conf; +}; + +#define INV_ICM42370_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, \ + }, \ + } + +#define INV_ICM42370_ACCEL_CHAN(_modifier, _index) \ + { \ + .type = IIO_ACCEL, \ + .modified = 1, \ + .channel2 = _modifier, \ + .info_mask_separate = \ + BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS), \ + .info_mask_shared_by_type = \ + BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_type_available = \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS), \ + .info_mask_shared_by_all = \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .info_mask_shared_by_all_available = \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .scan_index = _index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 16, \ + .storagebits = 16, \ + .endianness = IIO_BE, \ + }, \ + } + +enum inv_icm42370_accel_scan { + INV_ICM42370_ACCEL_SCAN_X, + INV_ICM42370_ACCEL_SCAN_Y, + INV_ICM42370_ACCEL_SCAN_Z, + INV_ICM42370_ACCEL_SCAN_TEMP, +}; + +enum inv_icm42370_sensor_mode { + INV_ICM42370_SENSOR_MODE_OFF, + INV_ICM42370_SENSOR_MODE_STANDBY, + INV_ICM42370_SENSOR_MODE_LOW_POWER, + INV_ICM42370_SENSOR_MODE_LOW_NOISE, + INV_ICM42370_SENSOR_MODE_NB, +}; + +enum inv_icm42370_filter { + /* Low-Noise mode sensor data filter bandwidth*/ + INV_ICM42370_UI_FILT_BW_LP_FILTER_BYPASSED, + /* Low-Power mode sensor data filter (averaging) */ + INV_ICM42370_FILTER_AVG_2X, + INV_ICM42370_FILTER_AVG_4X, + INV_ICM42370_FILTER_AVG_8X, + INV_ICM42370_FILTER_AVG_16X, + INV_ICM42370_FILTER_AVG_32X, + INV_ICM42370_FILTER_AVG_64X, + INV_ICM42370_FILTER_AVG_NB, +}; + +/** + * struct inv_icm42370_sensor_state - sensor state variables + * @scales: table of scales. + * @scales_len: length (nb of items) of the scales table. + * @power_mode: sensor requested power mode (for common frequencies) + * @filter: sensor filter. + * @ts: timestamp module states. + */ +struct inv_icm42370_sensor_state { + const int *scales; + size_t scales_len; + enum inv_icm42370_sensor_mode power_mode; + enum inv_icm42370_filter filter; + struct inv_sensors_timestamp ts; +}; + +/* serial bus slew rates */ +enum inv_icm42370_slew_rate { + INV_ICM42370_SLEW_RATE_20_60NS, + INV_ICM42370_SLEW_RATE_12_36NS, + INV_ICM42370_SLEW_RATE_6_19NS, + INV_ICM42370_SLEW_RATE_4_14NS, + INV_ICM42370_SLEW_RATE_2_8NS, + INV_ICM42370_SLEW_RATE_INF_2NS, +}; + +/* accelerometer fullscale values */ +enum inv_icm42370_accel_fs { + INV_ICM42370_ACCEL_FS_16G, + INV_ICM42370_ACCEL_FS_8G, + INV_ICM42370_ACCEL_FS_4G, + INV_ICM42370_ACCEL_FS_2G, + INV_ICM42370_ACCEL_FS_NB, +}; + +/* ODR suffixed by LN or LP are Low-Noise or Low-Power mode only */ +enum inv_icm42370_odr { + INV_ICM42370_ODR_1_6KHZ_LN = 5, + INV_ICM42370_ODR_800HZ_LN, + INV_ICM42370_ODR_400HZ, + INV_ICM42370_ODR_200HZ, + INV_ICM42370_ODR_100HZ, + INV_ICM42370_ODR_50HZ, + INV_ICM42370_ODR_25HZ, + INV_ICM42370_ODR_12_5HZ, + INV_ICM42370_ODR_6_25HZ_LP, + INV_ICM42370_ODR_3_125HZ_LP, + INV_ICM42370_ODR_1_5625HZ_LP, + INV_ICM42370_ODR_NB, +}; + +enum inv_icm42370_mregs { + INV_ICM42370_MREG1, + INV_ICM42370_MREG2 = 0x28, + INV_ICM42370_MREG3 = 0x50, +}; + +/* 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 */ +static const int inv_icm42370_accel_odr[] = { + /* 1.5625Hz */ + 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 } + +/* Registers in USER BANK 1 */ +#define INV_ICM42370_REG_MCLK_RDY 0x0 +#define INV_ICM42370_REG_DEVICE_CONFIG 0x01 +#define INV_ICM42370_REG_SIGNAL_PATH_RESET 0x02 +#define INV_ICM42370_REG_DRIVE_CONFIG1 0x03 +#define INV_ICM42370_REG_DRIVE_CONFIG2 0x04 +#define INV_ICM42370_REG_DRIVE_CONFIG3 0x05 +#define INV_ICM42370_REG_INT_CONFIG 0x06 +#define INV_ICM42370_REG_TEMP_DATA1 0x09 +#define INV_ICM42370_REG_TEMP_DATA0 0x0A +#define INV_ICM42370_REG_ACCEL_DATA_X1 0x0B +#define INV_ICM42370_REG_ACCEL_DATA_X0 0x0C +#define INV_ICM42370_REG_ACCEL_DATA_Y1 0x0D +#define INV_ICM42370_REG_ACCEL_DATA_Y0 0x0E +#define INV_ICM42370_REG_ACCEL_DATA_Z1 0x0F +#define INV_ICM42370_REG_ACCEL_DATA_Z0 0x10 +#define INV_ICM42370_REG_ACCEL_CONFIG0 0x21 +#define INV_ICM42370_REG_PWR_MGMT0 0x1F +#define INV_ICM42370_REG_INTF_CONFIG6 0x23 +#define INV_ICM42370_REG_FIFO_CONFIG1 0x28 +#define INV_ICM42370_REG_FIFO_WATERMARK 0x29 +#define INV_ICM42370_REG_INT_SOURCE0 0x2B +#define INV_ICM42370_REG_INT_STATUS 0x3A +#define INV_ICM42370_REG_TEMP_CONFIG0 0x34 +#define INV_ICM42370_REG_INTF_CONFIG0 0x35 +#define INV_ICM42370_REG_WHO_AM_I 0x75 +#define INV_ICM42370_REG_BLK_SEL_W 0x79 +#define INV_ICM42370_REG_MADDR_W 0x7A +#define INV_ICM42370_REG_M_W 0x7B +#define INV_ICM42370_REG_BLK_SEL_R 0x7C +#define INV_ICM42370_REG_MADDR_R 0x7D +#define INV_ICM42370_REG_M_R 0x7E + +#define INV_ICM42370_DRIVE_CONFIG1_I3C_DDR_MASK GENMASK(5, 3) +#define INV_ICM42370_DRIVE_CONFIG1_I3C_DDR(_rate) \ + FIELD_PREP(INV_ICM42370_DRIVE_CONFIG1_I3C_DDR_MASK, (_rate)) + +#define INV_ICM42370_DRIVE_CONFIG1_I3C_SDR_MASK GENMASK(2, 0) +#define INV_ICM42370_DRIVE_CONFIG1_I3C_SDR(_rate) \ + FIELD_PREP(INV_ICM42370_DRIVE_CONFIG1_I3C_SDR_MASK, (_rate)) +#define INV_ICM42370_DRIVE_CONFIG2_I2C_MASK GENMASK(5, 3) +#define INV_ICM42370_DRIVE_CONFIG2_I2C(_rate) \ + FIELD_PREP(INV_ICM42370_DRIVE_CONFIG2_I2C_MASK, (_rate)) +#define INV_ICM42370_DRIVE_CONFIG3_SPI_MASK GENMASK(2, 0) +#define INV_ICM42370_DRIVE_CONFIG3_SPI(_rate) \ + FIELD_PREP(INV_ICM42370_DRIVE_CONFIG3_SPI_MASK, (_rate)) + +#define INV_ICM42370_SIGNAL_PATH_RESET_FIFO_FLUSH BIT(2) +#define INV_ICM42370_FIFO_CONFIG_MODE_MASK BIT(0) +#define INV_ICM42370_FIFO_CONFIG_BYPASS_MASK BIT(1) +#define INV_ICM42370_FIFO_CONFIG_STREAM \ + FIELD_PREP(INV_ICM42370_FIFO_CONFIG_MODE_MASK, 0) +#define INV_ICM42370_FIFO_CONFIG_STOP_ON_FULL \ + FIELD_PREP(INV_ICM42370_FIFO_CONFIG_MODE_MASK, 1) +#define INV_ICM42370_FIFO_CONFIG_BYPASS \ + FIELD_PREP(INV_ICM42370_FIFO_CONFIG_BYPASS_MASK, 1) + +#define INV_ICM42370_INT_CONFIG_INT2_LATCHED BIT(5) +#define INV_ICM42370_INT_CONFIG_INT2_PUSH_PULL BIT(4) +#define INV_ICM42370_INT_CONFIG_INT2_ACTIVE_HIGH BIT(3) +#define INV_ICM42370_INT_CONFIG_INT2_ACTIVE_LOW 0x00 +#define INV_ICM42370_INT_CONFIG_INT1_LATCHED BIT(2) +#define INV_ICM42370_INT_CONFIG_INT1_PUSH_PULL BIT(1) +#define INV_ICM42370_INT_CONFIG_INT1_ACTIVE_HIGH BIT(0) +#define INV_ICM42370_INT_CONFIG_INT1_ACTIVE_LOW 0x00 + +#define INV_ICM42370_INT_STATUS_FIFO_THS BIT(2) +#define INV_ICM42370_INT_STATUS_FIFO_FULL BIT(1) + +#define INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN BIT(2) + +/* Registers in MREG1 USER BANK 1*/ +#define INV_ICM42370_REG_TMST_CONFIG1 0x00 +#define INV_ICM42370_REG_FIFO_CONFIG5 0x01 +#define INV_ICM42370_REG_FIFO_CONFIG6 0x02 +#define INV_ICM42370_REG_INT_CONFIG1 0x05 +#define INV_ICM42370_REG_OFFSET_USER4 0x52 +#define INV_ICM42370_REG_OFFSET_USER5 0x53 +#define INV_ICM42370_REG_OFFSET_USER6 0x54 +#define INV_ICM42370_REG_OFFSET_USER7 0x55 +#define INV_ICM42370_REG_OFFSET_USER8 0x56 + +#define INV_ICM42370_TMST_CONFIG_TMST_DELTA_EN BIT(2) +#define INV_ICM42370_TMST_CONFIG_TMST_EN BIT(0) + +#define INV_ICM42370_FIFO_CONFIG5_WM_GT_TH BIT(5) +#define INV_ICM42370_FIFO_CONFIG5_RESUME_PARTIAL_RD BIT(4) +#define INV_ICM42370_FIFO_CONFIG5_ACCEL_EN BIT(0) + +#define INV_ICM42370_INT_CONFIG1_ASYNC_RESET BIT(4) + +#define INV_ICM42370_WHOAMI_VALUE 0x0D + +#define INV_ICM42370_FIFO_FLUSH_BIT_MASK BIT(2) + +#define INV_ICM42370_INTF_CONFIG0_FIFO_COUNT_ENDIAN BIT(5) +#define INV_ICM42370_INTF_CONFIG0_SENSOR_DATA_ENDIAN BIT(4) + +#define INV_ICM42370_FIFO_WATERMARK_VAL(_wm) cpu_to_le16((_wm) & GENMASK(11, 0)) +/* FIFO is 2048 bytes, let 12 samples for reading latency */ +#define INV_ICM42370_FIFO_WATERMARK_MAX (2048 - 12 * 16) + +#define INV_ICM42370_PWR_MGMT0(_mode) FIELD_PREP(GENMASK(1, 0), (_mode)) + +#define INV_ICM42370_ACCEL_CONFIG0_FS(_fs) FIELD_PREP(GENMASK(6, 5), (_fs)) +#define INV_ICM42370_ACCEL_CONFIG0_ODR(_odr) FIELD_PREP(GENMASK(3, 0), (_odr)) + +#define INV_ICM42370_TEMP_FILT_BW_DLPF(_dlpf) FIELD_PREP(GENMASK(6, 4), (_dlpf)) + +#define INV_ICM42370_MCLK_RDY_BIT BIT(3) +#define INV_ICM42370_SOFT_RESET_BIT BIT(4) +#define INV_ICM42370_ACCEL_MODE_LN 0x03 + +#define INV_ICM42370_DATA_INVALID -32768 +#define INV_ICM42370_ACCEL_STARTUP_TIME_MS 10 + +typedef int (*inv_icm42370_bus_setup)(struct inv_icm42370_data *); +extern const struct regmap_config inv_icm42370_regmap_config; + +int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq, + inv_icm42370_bus_setup bus_setup); +u32 inv_icm42370_odr_to_period(enum inv_icm42370_odr odr); + +int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq, + inv_icm42370_bus_setup bus_setup); + +struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, + struct inv_icm42370_data *data); + +int inv_icm42370_set_accel_conf(struct inv_icm42370_data *dev_data, + struct inv_icm42370_conf *conf, + unsigned int *sleep_ms); + +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev); + +#endif 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> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <linux/property.h> +#include <linux/regmap.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"); + +static const struct inv_icm42370_conf inv_icm42370_default_conf = { + .mode = INV_ICM42370_SENSOR_MODE_LOW_NOISE, + .fs = INV_ICM42370_ACCEL_FS_16G, + .odr = INV_ICM42370_ODR_400HZ, + .filter = INV_ICM42370_FILTER_AVG_16X, +}; + +static const struct iio_chan_spec inv_icm42370_accel_channels[] = { + INV_ICM42370_ACCEL_CHAN(IIO_MOD_X, INV_ICM42370_ACCEL_SCAN_X), + INV_ICM42370_ACCEL_CHAN(IIO_MOD_Y, INV_ICM42370_ACCEL_SCAN_Y), + INV_ICM42370_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42370_ACCEL_SCAN_Z), + INV_ICM42370_TEMP_CHAN(INV_ICM42370_ACCEL_SCAN_TEMP), +}; + +/* 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, + /* +/- 8G => 2*8*9.80665 / (2**15) m/s-2 */ + [2 * INV_ICM42370_ACCEL_FS_8G] = 0, + [2 * INV_ICM42370_ACCEL_FS_8G + 1] = 4788403, + /* +/- 4G => 2*4*9.80665 / (2**15) m/s-2 */ + [2 * INV_ICM42370_ACCEL_FS_4G] = 0, + [2 * INV_ICM42370_ACCEL_FS_4G + 1] = 2394202, + /* +/- 2G => 2*2*9.80665 / (2**15) m/s-2 */ + [2 * INV_ICM42370_ACCEL_FS_2G] = 0, + [2 * INV_ICM42370_ACCEL_FS_2G + 1] = 1197101, +}; + +/** + * 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 */ + 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]; +} + +/* ODR suffixed by LN or LP are Low-Noise or Low-Power mode only */ +static const int inv_icm42370_accel_odr_conv[] = { + INV_ICM42370_ODR_1_6KHZ_LN, INV_ICM42370_ODR_800HZ_LN, + INV_ICM42370_ODR_400HZ, INV_ICM42370_ODR_200HZ, + INV_ICM42370_ODR_100HZ, INV_ICM42370_ODR_50HZ, + INV_ICM42370_ODR_25HZ, INV_ICM42370_ODR_12_5HZ, + INV_ICM42370_ODR_6_25HZ_LP, INV_ICM42370_ODR_3_125HZ_LP, + INV_ICM42370_ODR_1_5625HZ_LP, INV_ICM42370_ODR_NB, +}; + +/** + * inv_icm42370_mreg_check() - check registers before accessing the registers in other banks + * + * @map: regmap of the device + * + * Returns 0 on success, negative errno on error + */ +static int inv_icm42370_mreg_check(struct regmap *map) +{ + int tmp; + + regmap_read(map, INV_ICM42370_REG_MCLK_RDY, &tmp); + if (!(tmp & INV_ICM42370_MCLK_RDY_BIT)) + return -EINVAL; + regmap_read(map, INV_ICM42370_REG_PWR_MGMT0, &tmp); + if (tmp != INV_ICM42370_PWR_MGMT0(INV_ICM42370_SENSOR_MODE_LOW_NOISE)) + return -EINVAL; + + return 0; +} + +/** + * inv_icm42370_mreg_write() - routine for writing to other bank registers + * + * @map: regmap of the device + * @bank: register bank being accessed + * @addr: address of the register being accessed + * @val: value written to the register + * + * Returns 0 on success, negative errno on error + */ +static int inv_icm42370_mreg_write(struct regmap *map, u8 bank, u8 addr, u8 val) +{ + int ret; + + ret = inv_icm42370_mreg_check(map); + if (ret) + return ret; + + ret = regmap_write(map, INV_ICM42370_REG_BLK_SEL_W, bank); + if (ret) + return -EINVAL; + + ret = regmap_write(map, INV_ICM42370_REG_MADDR_W, addr); + if (ret) + return -EINVAL; + + ret = regmap_write(map, INV_ICM42370_REG_M_W, val); + if (ret) + return -EINVAL; + + usleep_range(10, 20); + return regmap_write(map, INV_ICM42370_REG_BLK_SEL_W, 0x00); +} + +/** + * 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); + 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 */ + 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); + 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; + 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); +} + +/** + * inv_icm42370_setup() - check and setup chip + * + * @data: pointer to struct containing the sensor data + * + * Returns 0 on success, a negative error code otherwise. + */ +static int inv_icm42370_setup(struct inv_icm42370_data *data, + inv_icm42370_bus_setup bus_setup) +{ + const struct device *dev = regmap_get_device(data->map); + unsigned int whoami; + int ret; + + /* check chip self-identification value */ + ret = regmap_read(data->map, INV_ICM42370_REG_WHO_AM_I, &whoami); + if (ret) + return ret; + + if (whoami != INV_ICM42370_WHOAMI_VALUE) { + dev_err(dev, "Wrong WHO_AM_I: %d (want 0x%02X)\n", + whoami, INV_ICM42370_WHOAMI_VALUE); + return -ENODEV; + } + + data->name = "inv_icm42370"; + + /* set chip bus configuration */ + ret = bus_setup(data); + if (ret) + return ret; + + /* sensor data in big-endian (default) */ + ret = regmap_set_bits(data->map, INV_ICM42370_REG_INTF_CONFIG0, + INV_ICM42370_INTF_CONFIG0_SENSOR_DATA_ENDIAN); + if (ret) + return ret; + + return inv_icm42370_set_conf(data, &inv_icm42370_default_conf); +} + +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); + + 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; +} + +/** + * 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; + 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); + 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)); + 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; + +exit: + mutex_unlock(&dev_data->lock); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + + return ret; +} + +/** + * inv_icm42370_temp_read_raw() - read data from the temperature sensor + * + * @indio_dev: pointer to the industrial io struct + * @chan: pointer to the iio channel specification + * @val: integer part of the value + * @val2: decimal part of the value + * @mask: mask to differentiate between channel info + * + * Returns 0 on success, negative errno on error + */ +static int inv_icm42370_temp_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); + s16 temp; + int ret; + + if (chan->type != IIO_TEMP) + return -EINVAL; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + if (!iio_device_claim_direct(indio_dev)) + return -EBUSY; + ret = inv_icm42370_temp_read(dev_data, &temp); + iio_device_release_direct(indio_dev); + if (ret) + return ret; + *val = temp; + return IIO_VAL_INT; + /* + * T°C = (temp / 128) + 25 + * Tm°C = 1000 * ((temp / 128) + 25) + * Tm°C = 7.8125 * temp + 25000 + * Tm°C = (temp + 3200) * 7.8125 + * scale: 1000 / 128 ~= 7.8125 + * offset: 3200 + */ + case IIO_CHAN_INFO_SCALE: + *val = 7; + *val2 = 812500; + return IIO_VAL_INT_PLUS_MICRO; + case IIO_CHAN_INFO_OFFSET: + *val = 3200; + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +/** + * inv_icm42370_accel_read_offset() - read offset values from 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_read_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 bias; + unsigned int reg; + s16 offset; + u8 data[2]; + int ret; + + if (chan->type != IIO_ACCEL) + return -EINVAL; + + switch (chan->channel2) { + case IIO_MOD_X: + reg = INV_ICM42370_REG_OFFSET_USER4; + break; + case IIO_MOD_Y: + reg = INV_ICM42370_REG_OFFSET_USER6; + break; + case IIO_MOD_Z: + reg = INV_ICM42370_REG_OFFSET_USER7; + break; + default: + return -EINVAL; + } + + 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)); + + mutex_unlock(&dev_data->lock); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + if (ret) + return ret; + + /* 12 bits signed value */ + switch (chan->channel2) { + case IIO_MOD_X: + offset = sign_extend32(((data[0] & 0xF0) << 4) | data[1], 11); + break; + case IIO_MOD_Y: + offset = sign_extend32(((data[1] & 0x0F) << 8) | data[0], 11); + break; + case IIO_MOD_Z: + offset = sign_extend32(((data[0] & 0xF0) << 4) | data[1], 11); + break; + default: + return -EINVAL; + } + + /* + * 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); + *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 + + 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); + + 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 *)®val); + 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; + + ret = inv_icm42370_mreg_write(dev_data->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER5, + dev_data->buffer[1]); + if (ret) + goto out_unlock; + break; + 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 *)®val); + if (ret) + goto out_unlock; + dev_data->buffer[0] = offset & 0xFF; + dev_data->buffer[1] = ((offset & 0xF00) >> 8) | (regval & 0xF0); + + ret = inv_icm42370_mreg_write(dev_data->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER7, + dev_data->buffer[0]); + if (ret) + goto out_unlock; + + ret = inv_icm42370_mreg_write(dev_data->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER6, + dev_data->buffer[1]); + if (ret) + goto out_unlock; + + break; + case IIO_MOD_Z: + /* OFFSET_USER7 register is shared */ + inv_icm42370_mreg_read(dev_data->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER7, + (u8 *)®val); + + 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_USER7, + dev_data->buffer[0]); + if (ret) + goto out_unlock; + + ret = inv_icm42370_mreg_write(dev_data->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER8, + dev_data->buffer[1]); + if (ret) + goto out_unlock; + break; + default: + ret = -EINVAL; + goto out_unlock; + } + +out_unlock: + mutex_unlock(&dev_data->lock); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + return ret; +} + +/** + * inv_icm42370_accel_read_scale() - read scaling data from the accelerometer + * + * @indio_dev: pointer to the industrial io struct + * @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_read_scale(struct iio_dev *indio_dev, int *val, + int *val2) +{ + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); + unsigned int idx; + + idx = dev_data->conf.fs; + + *val = dev_data->sensor_state->scales[2 * idx]; + *val2 = dev_data->sensor_state->scales[2 * idx + 1]; + return IIO_VAL_INT_PLUS_NANO; +} + +/** + * inv_icm42370_accel_write_scale() - write scaling data to the accelerometer + * + * @indio_dev: pointer to the industrial io struct + * @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_scale(struct iio_dev *indio_dev, int val, + int val2) +{ + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); + struct device *dev = regmap_get_device(dev_data->map); + unsigned int idx; + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; + int ret; + + for (idx = 0; idx < dev_data->sensor_state->scales_len; idx += 2) { + if (val == dev_data->sensor_state->scales[idx] && + val2 == dev_data->sensor_state->scales[idx + 1]) + break; + } + + if (idx >= dev_data->sensor_state->scales_len) + return -EINVAL; + + conf.fs = idx / 2; + + pm_runtime_get_sync(dev); + mutex_lock(&dev_data->lock); + + ret = inv_icm42370_set_accel_conf(dev_data, &conf, NULL); + + mutex_unlock(&dev_data->lock); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + + return ret; +} + +/** + * inv_icm42370_accel_read_odr() - read ODR data from the accelerometer + * + * @dev_data: pointer to struct containing the sensor data + * @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_read_odr(struct inv_icm42370_data *dev_data, + int *val, int *val2) +{ + unsigned int odr; + unsigned int i; + + odr = dev_data->conf.odr; + + for (i = 0; i < ARRAY_SIZE(inv_icm42370_accel_odr_conv); ++i) { + if (inv_icm42370_accel_odr_conv[i] == odr) + break; + } + if (i >= ARRAY_SIZE(inv_icm42370_accel_odr_conv)) + return -EINVAL; + + *val = inv_icm42370_accel_odr[2 * i]; + *val2 = inv_icm42370_accel_odr[2 * i + 1]; + + return IIO_VAL_INT_PLUS_MICRO; +} + +/** + * inv_icm42370_accel_write_odr() - write ODR data to the accelerometer + * + * @indio_dev: pointer to struct containing the sensor data + * @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_odr(struct iio_dev *indio_dev, int val, + int val2) +{ + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); + struct inv_sensors_timestamp *ts = &dev_data->sensor_state->ts; + struct device *dev = regmap_get_device(dev_data->map); + unsigned int idx; + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; + int ret; + + for (idx = 0; idx < ARRAY_SIZE(inv_icm42370_accel_odr); idx += 2) { + if (val == inv_icm42370_accel_odr[idx] && + val2 == inv_icm42370_accel_odr[idx + 1]) + break; + } + if (idx >= ARRAY_SIZE(inv_icm42370_accel_odr)) + return -EINVAL; + + conf.odr = inv_icm42370_accel_odr_conv[idx / 2]; + + pm_runtime_get_sync(dev); + mutex_lock(&dev_data->lock); + + ret = inv_sensors_timestamp_update_odr( + ts, inv_icm42370_odr_to_period(conf.odr), + iio_buffer_enabled(indio_dev)); + if (ret) + goto out_unlock; + + ret = inv_icm42370_set_accel_conf(dev_data, &conf, NULL); + if (ret) + goto out_unlock; + +out_unlock: + mutex_unlock(&dev_data->lock); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + + return ret; +} + +/** + * inv_icm42370_accel_write_raw() - write raw attribute values to the accelerometer + * @indio_dev: pointer to the IIO device structure. + * @chan: pointer to the IIO channel specification. + * @val: integer part of the value to write. + * @val2: fractional part of the value to write. + * @mask: bitmask specifying which attribute to write. + * + * Returns 0 on success, negative errno on error. + */ +static int inv_icm42370_accel_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); + int ret; + + if (chan->type != IIO_ACCEL) + return -EINVAL; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + if (!iio_device_claim_direct(indio_dev)) + return -EBUSY; + ret = inv_icm42370_accel_write_scale(indio_dev, val, val2); + iio_device_release_direct(indio_dev); + return ret; + case IIO_CHAN_INFO_SAMP_FREQ: + return inv_icm42370_accel_write_odr(indio_dev, val, val2); + case IIO_CHAN_INFO_CALIBBIAS: + if (!iio_device_claim_direct(indio_dev)) + return -EBUSY; + ret = inv_icm42370_accel_write_offset(dev_data, chan, val, + val2); + iio_device_release_direct(indio_dev); + return ret; + + default: + return -EINVAL; + } +} + +/** + * inv_icm42370_accel_read_sensor() - internal function to read accelerometer sensor registers + * + * @indio_dev: pointer to the industrial io struct + * @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) +{ + struct inv_icm42370_data *data = iio_priv(indio_dev); + struct device *dev = regmap_get_device(data->map); + unsigned int reg; + __be16 *value; + int ret; + + if (chan->type != IIO_ACCEL) + return -EINVAL; + + switch (chan->channel2) { + case IIO_MOD_X: + reg = INV_ICM42370_REG_ACCEL_DATA_X1; + break; + case IIO_MOD_Y: + reg = INV_ICM42370_REG_ACCEL_DATA_Y1; + break; + case IIO_MOD_Z: + reg = INV_ICM42370_REG_ACCEL_DATA_Z1; + break; + default: + return -EINVAL; + } + + pm_runtime_get_sync(dev); + mutex_lock(&data->lock); + + /* read accel register data */ + value = (__be16 *)&data->buffer[0]; + ret = regmap_bulk_read(data->map, reg, value, sizeof(*value)); + if (ret) + goto exit; + + *val = (s16)be16_to_cpup(value); + + if (*val == INV_ICM42370_DATA_INVALID) + ret = -EINVAL; + +exit: + mutex_unlock(&data->lock); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + return ret; +} + +/** + * inv_icm42370_accel_read_raw() - read data from the accelerometer sensor + * + * @indio_dev: pointer to the industrial io struct + * @chan: pointer to the iio channel specification + * @val: integer part of the value + * @val2: decimal part of the value + * @mask: mask to differentiate between channel info + * + * Returns 0 on success, negative errno on error + */ +static int inv_icm42370_accel_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); + s16 value; + int ret; + + switch (chan->type) { + case IIO_ACCEL: + break; + case IIO_TEMP: + return inv_icm42370_temp_read_raw(indio_dev, chan, val, val2, + mask); + default: + return -EINVAL; + } + + switch (mask) { + case IIO_CHAN_INFO_RAW: + if (!iio_device_claim_direct(indio_dev)) + return -EBUSY; + ret = inv_icm42370_accel_read_sensor(indio_dev, chan, &value); + iio_device_release_direct(indio_dev); + if (ret) + return ret; + *val = value; + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + return inv_icm42370_accel_read_scale(indio_dev, val, val2); + case IIO_CHAN_INFO_SAMP_FREQ: + return inv_icm42370_accel_read_odr(dev_data, val, val2); + case IIO_CHAN_INFO_CALIBBIAS: + return inv_icm42370_accel_read_offset(dev_data, chan, val, + val2); + default: + return -EINVAL; + } +} + +static const struct iio_info inv_icm42370_info = { + .read_raw = inv_icm42370_accel_read_raw, + .write_raw = inv_icm42370_accel_write_raw, +}; + +struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, + struct inv_icm42370_data *data) +{ + struct device *dev = regmap_get_device(data->map); + struct inv_sensors_timestamp_chip ts_chip; + int ret; + + data->sensor_state->scales = inv_icm42370_accel_scale; + data->sensor_state->scales_len = ARRAY_SIZE(inv_icm42370_accel_scale); + + /* + * 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); + 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); + 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); + data->chip = chip; + data->map = regmap; + + 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); + 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"); +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> +#include <linux/device.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#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 }, + {} }; +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, + }, + {} +}; +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"); -- 2.43.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] iio: accel: Add support for ICM42370P 2026-08-06 12:46 ` [PATCH 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar @ 2026-08-06 13:02 ` sashiko-bot 2026-08-07 6:51 ` Uwe Kleine-König 2026-08-07 10:09 ` Joshua Crofts 2 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-06 13:02 UTC (permalink / raw) To: Kanak Shilledar; +Cc: robh, conor+dt, devicetree 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 *)®val); [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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] iio: accel: Add support for ICM42370P 2026-08-06 12:46 ` [PATCH 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar 2026-08-06 13:02 ` sashiko-bot @ 2026-08-07 6:51 ` Uwe Kleine-König 2026-08-07 13:11 ` Kanak Shilledar 2026-08-07 10:09 ` Joshua Crofts 2 siblings, 1 reply; 14+ messages in thread From: Uwe Kleine-König @ 2026-08-07 6:51 UTC (permalink / raw) To: Kanak Shilledar Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Grimler, Jean-Baptiste Maneyrol, linux-iio, devicetree, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 2298 bytes --] Hello, On Thu, Aug 06, 2026 at 02:46:28PM +0200, Kanak Shilledar wrote: > 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> <linux/i2c.h> already provides structs i2c_device_id and of_device_id. So please drop the explicit include for <linux/mod_devicetable.h>, as this will go away soon. > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > + > +#include <linux/iio/common/inv_sensors_timestamp.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > + > [...] > +static const struct i2c_device_id inv_icm42370_id[] = { { "icm42370", > + INV_CHIP_ICM42370 }, > + {} }; Please use named initializers and saner code style. I suggest: static const struct i2c_device_id inv_icm42370_id[] = { { .name = "icm42370", .driver_data = INV_CHIP_ICM42370 }, { } }; > +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, > + }, > + {} { } please > +}; > +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, .id_table = inv_icm42370_id, > +}; > +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"); Best regards Uwe [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] iio: accel: Add support for ICM42370P 2026-08-07 6:51 ` Uwe Kleine-König @ 2026-08-07 13:11 ` Kanak Shilledar 0 siblings, 0 replies; 14+ messages in thread From: Kanak Shilledar @ 2026-08-07 13:11 UTC (permalink / raw) To: u.kleine-koenig@baylibre.com Cc: dlechner@baylibre.com, Henrik Grimler, nuno.sa@analog.com, jean-baptiste.maneyrol@tdk.com, robh@kernel.org, jic23@kernel.org, andy@kernel.org, krzk+dt@kernel.org, linux-iio@vger.kernel.org, conor+dt@kernel.org, Kernel, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 2812 bytes --] Hello! Thanks for reviewing the code. On Fri, 2026-08-07 at 08:51 +0200, Uwe Kleine-König wrote: > Hello, > > On Thu, Aug 06, 2026 at 02:46:28PM +0200, Kanak Shilledar wrote: > > 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> > > <linux/i2c.h> already provides structs i2c_device_id and > of_device_id. > So please drop the explicit include for <linux/mod_devicetable.h>, as > this will go away soon. Will fix them. > > +#include <linux/module.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/property.h> > > +#include <linux/regmap.h> > > + > > +#include <linux/iio/common/inv_sensors_timestamp.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > + > > [...] > > +static const struct i2c_device_id inv_icm42370_id[] = { { > > "icm42370", > > + INV_CHIP_ICM42370 }, > > + {} }; > > Please use named initializers and saner code style. I suggest: > > static const struct i2c_device_id inv_icm42370_id[] = { > { .name = "icm42370", .driver_data = INV_CHIP_ICM42370 }, > { } > }; Thanks for the suggestion. Will fix the formatting. > > +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, > > + }, > > + {} > > { } please Here as well. > > > +}; > > +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, > > .id_table = inv_icm42370_id, Will add the property. > > +}; > > +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"); > > Best regards > Uwe Thanks and Regards, Kanak Shilledar [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] iio: accel: Add support for ICM42370P 2026-08-06 12:46 ` [PATCH 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar 2026-08-06 13:02 ` sashiko-bot 2026-08-07 6:51 ` Uwe Kleine-König @ 2026-08-07 10:09 ` Joshua Crofts 2026-08-07 13:41 ` Kanak Shilledar 2 siblings, 1 reply; 14+ messages in thread From: Joshua Crofts @ 2026-08-07 10:09 UTC (permalink / raw) To: Kanak Shilledar Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Grimler, Jean-Baptiste Maneyrol, linux-iio, devicetree, linux-kernel, kernel 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: https://sashiko.dev/#/patchset/20260806-b4-inv_icm42370p-v1-0-670837f5842f%40axis.com 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). > + > +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. > + 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. > + 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. > + s64 timestamp; > + enum inv_icm42370_chip chip; > + struct inv_icm42370_conf conf; > +}; > + > +#define INV_ICM42370_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, \ > + }, \ > + } > + > +#define INV_ICM42370_ACCEL_CHAN(_modifier, _index) \ > + { \ > + .type = IIO_ACCEL, \ > + .modified = 1, \ > + .channel2 = _modifier, \ > + .info_mask_separate = \ > + BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_CALIBBIAS), \ > + .info_mask_shared_by_type = \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .info_mask_shared_by_type_available = \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_CALIBBIAS), \ > + .info_mask_shared_by_all = \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .info_mask_shared_by_all_available = \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .scan_index = _index, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + }, \ > + } > + > +enum inv_icm42370_accel_scan { > + INV_ICM42370_ACCEL_SCAN_X, > + INV_ICM42370_ACCEL_SCAN_Y, > + INV_ICM42370_ACCEL_SCAN_Z, > + INV_ICM42370_ACCEL_SCAN_TEMP, > +}; > + > +enum inv_icm42370_sensor_mode { > + INV_ICM42370_SENSOR_MODE_OFF, > + INV_ICM42370_SENSOR_MODE_STANDBY, > + INV_ICM42370_SENSOR_MODE_LOW_POWER, > + INV_ICM42370_SENSOR_MODE_LOW_NOISE, > + INV_ICM42370_SENSOR_MODE_NB, > +}; > + > +enum inv_icm42370_filter { > + /* Low-Noise mode sensor data filter bandwidth*/ > + INV_ICM42370_UI_FILT_BW_LP_FILTER_BYPASSED, > + /* Low-Power mode sensor data filter (averaging) */ > + INV_ICM42370_FILTER_AVG_2X, > + INV_ICM42370_FILTER_AVG_4X, > + INV_ICM42370_FILTER_AVG_8X, > + INV_ICM42370_FILTER_AVG_16X, > + INV_ICM42370_FILTER_AVG_32X, > + INV_ICM42370_FILTER_AVG_64X, > + INV_ICM42370_FILTER_AVG_NB, > +}; > + > +/** > + * struct inv_icm42370_sensor_state - sensor state variables > + * @scales: table of scales. > + * @scales_len: length (nb of items) of the scales table. > + * @power_mode: sensor requested power mode (for common frequencies) > + * @filter: sensor filter. > + * @ts: timestamp module states. > + */ > +struct inv_icm42370_sensor_state { > + const int *scales; > + size_t scales_len; > + enum inv_icm42370_sensor_mode power_mode; > + enum inv_icm42370_filter filter; > + struct inv_sensors_timestamp ts; > +}; > + > +/* serial bus slew rates */ > +enum inv_icm42370_slew_rate { > + INV_ICM42370_SLEW_RATE_20_60NS, > + INV_ICM42370_SLEW_RATE_12_36NS, > + INV_ICM42370_SLEW_RATE_6_19NS, > + INV_ICM42370_SLEW_RATE_4_14NS, > + INV_ICM42370_SLEW_RATE_2_8NS, > + INV_ICM42370_SLEW_RATE_INF_2NS, > +}; > + > +/* accelerometer fullscale values */ > +enum inv_icm42370_accel_fs { > + INV_ICM42370_ACCEL_FS_16G, > + INV_ICM42370_ACCEL_FS_8G, > + INV_ICM42370_ACCEL_FS_4G, > + INV_ICM42370_ACCEL_FS_2G, > + INV_ICM42370_ACCEL_FS_NB, > +}; > + > +/* ODR suffixed by LN or LP are Low-Noise or Low-Power mode only */ > +enum inv_icm42370_odr { > + INV_ICM42370_ODR_1_6KHZ_LN = 5, > + INV_ICM42370_ODR_800HZ_LN, > + INV_ICM42370_ODR_400HZ, > + INV_ICM42370_ODR_200HZ, > + INV_ICM42370_ODR_100HZ, > + INV_ICM42370_ODR_50HZ, > + INV_ICM42370_ODR_25HZ, > + INV_ICM42370_ODR_12_5HZ, > + INV_ICM42370_ODR_6_25HZ_LP, > + INV_ICM42370_ODR_3_125HZ_LP, > + INV_ICM42370_ODR_1_5625HZ_LP, > + INV_ICM42370_ODR_NB, > +}; > + > +enum inv_icm42370_mregs { > + INV_ICM42370_MREG1, > + INV_ICM42370_MREG2 = 0x28, > + INV_ICM42370_MREG3 = 0x50, > +}; > + > +/* 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. > + 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 } > + > +/* Registers in USER BANK 1 */ > +#define INV_ICM42370_REG_MCLK_RDY 0x0 > +#define INV_ICM42370_REG_DEVICE_CONFIG 0x01 > +#define INV_ICM42370_REG_SIGNAL_PATH_RESET 0x02 > +#define INV_ICM42370_REG_DRIVE_CONFIG1 0x03 > +#define INV_ICM42370_REG_DRIVE_CONFIG2 0x04 > +#define INV_ICM42370_REG_DRIVE_CONFIG3 0x05 > +#define INV_ICM42370_REG_INT_CONFIG 0x06 > +#define INV_ICM42370_REG_TEMP_DATA1 0x09 > +#define INV_ICM42370_REG_TEMP_DATA0 0x0A > +#define INV_ICM42370_REG_ACCEL_DATA_X1 0x0B > +#define INV_ICM42370_REG_ACCEL_DATA_X0 0x0C > +#define INV_ICM42370_REG_ACCEL_DATA_Y1 0x0D > +#define INV_ICM42370_REG_ACCEL_DATA_Y0 0x0E > +#define INV_ICM42370_REG_ACCEL_DATA_Z1 0x0F > +#define INV_ICM42370_REG_ACCEL_DATA_Z0 0x10 > +#define INV_ICM42370_REG_ACCEL_CONFIG0 0x21 > +#define INV_ICM42370_REG_PWR_MGMT0 0x1F > +#define INV_ICM42370_REG_INTF_CONFIG6 0x23 > +#define INV_ICM42370_REG_FIFO_CONFIG1 0x28 > +#define INV_ICM42370_REG_FIFO_WATERMARK 0x29 > +#define INV_ICM42370_REG_INT_SOURCE0 0x2B > +#define INV_ICM42370_REG_INT_STATUS 0x3A > +#define INV_ICM42370_REG_TEMP_CONFIG0 0x34 > +#define INV_ICM42370_REG_INTF_CONFIG0 0x35 > +#define INV_ICM42370_REG_WHO_AM_I 0x75 > +#define INV_ICM42370_REG_BLK_SEL_W 0x79 > +#define INV_ICM42370_REG_MADDR_W 0x7A > +#define INV_ICM42370_REG_M_W 0x7B > +#define INV_ICM42370_REG_BLK_SEL_R 0x7C > +#define INV_ICM42370_REG_MADDR_R 0x7D > +#define INV_ICM42370_REG_M_R 0x7E > + > +#define INV_ICM42370_DRIVE_CONFIG1_I3C_DDR_MASK GENMASK(5, 3) > +#define INV_ICM42370_DRIVE_CONFIG1_I3C_DDR(_rate) \ > + FIELD_PREP(INV_ICM42370_DRIVE_CONFIG1_I3C_DDR_MASK, (_rate)) > + > +#define INV_ICM42370_DRIVE_CONFIG1_I3C_SDR_MASK GENMASK(2, 0) > +#define INV_ICM42370_DRIVE_CONFIG1_I3C_SDR(_rate) \ > + FIELD_PREP(INV_ICM42370_DRIVE_CONFIG1_I3C_SDR_MASK, (_rate)) > +#define INV_ICM42370_DRIVE_CONFIG2_I2C_MASK GENMASK(5, 3) > +#define INV_ICM42370_DRIVE_CONFIG2_I2C(_rate) \ > + FIELD_PREP(INV_ICM42370_DRIVE_CONFIG2_I2C_MASK, (_rate)) > +#define INV_ICM42370_DRIVE_CONFIG3_SPI_MASK GENMASK(2, 0) > +#define INV_ICM42370_DRIVE_CONFIG3_SPI(_rate) \ > + FIELD_PREP(INV_ICM42370_DRIVE_CONFIG3_SPI_MASK, (_rate)) > + > +#define INV_ICM42370_SIGNAL_PATH_RESET_FIFO_FLUSH BIT(2) > +#define INV_ICM42370_FIFO_CONFIG_MODE_MASK BIT(0) > +#define INV_ICM42370_FIFO_CONFIG_BYPASS_MASK BIT(1) > +#define INV_ICM42370_FIFO_CONFIG_STREAM \ > + FIELD_PREP(INV_ICM42370_FIFO_CONFIG_MODE_MASK, 0) > +#define INV_ICM42370_FIFO_CONFIG_STOP_ON_FULL \ > + FIELD_PREP(INV_ICM42370_FIFO_CONFIG_MODE_MASK, 1) > +#define INV_ICM42370_FIFO_CONFIG_BYPASS \ > + FIELD_PREP(INV_ICM42370_FIFO_CONFIG_BYPASS_MASK, 1) > + > +#define INV_ICM42370_INT_CONFIG_INT2_LATCHED BIT(5) > +#define INV_ICM42370_INT_CONFIG_INT2_PUSH_PULL BIT(4) > +#define INV_ICM42370_INT_CONFIG_INT2_ACTIVE_HIGH BIT(3) > +#define INV_ICM42370_INT_CONFIG_INT2_ACTIVE_LOW 0x00 > +#define INV_ICM42370_INT_CONFIG_INT1_LATCHED BIT(2) > +#define INV_ICM42370_INT_CONFIG_INT1_PUSH_PULL BIT(1) > +#define INV_ICM42370_INT_CONFIG_INT1_ACTIVE_HIGH BIT(0) > +#define INV_ICM42370_INT_CONFIG_INT1_ACTIVE_LOW 0x00 > + > +#define INV_ICM42370_INT_STATUS_FIFO_THS BIT(2) > +#define INV_ICM42370_INT_STATUS_FIFO_FULL BIT(1) > + > +#define INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN BIT(2) > + > +/* Registers in MREG1 USER BANK 1*/ > +#define INV_ICM42370_REG_TMST_CONFIG1 0x00 > +#define INV_ICM42370_REG_FIFO_CONFIG5 0x01 > +#define INV_ICM42370_REG_FIFO_CONFIG6 0x02 > +#define INV_ICM42370_REG_INT_CONFIG1 0x05 > +#define INV_ICM42370_REG_OFFSET_USER4 0x52 > +#define INV_ICM42370_REG_OFFSET_USER5 0x53 > +#define INV_ICM42370_REG_OFFSET_USER6 0x54 > +#define INV_ICM42370_REG_OFFSET_USER7 0x55 > +#define INV_ICM42370_REG_OFFSET_USER8 0x56 > + > +#define INV_ICM42370_TMST_CONFIG_TMST_DELTA_EN BIT(2) > +#define INV_ICM42370_TMST_CONFIG_TMST_EN BIT(0) > + > +#define INV_ICM42370_FIFO_CONFIG5_WM_GT_TH BIT(5) > +#define INV_ICM42370_FIFO_CONFIG5_RESUME_PARTIAL_RD BIT(4) > +#define INV_ICM42370_FIFO_CONFIG5_ACCEL_EN BIT(0) > + > +#define INV_ICM42370_INT_CONFIG1_ASYNC_RESET BIT(4) > + > +#define INV_ICM42370_WHOAMI_VALUE 0x0D > + > +#define INV_ICM42370_FIFO_FLUSH_BIT_MASK BIT(2) > + > +#define INV_ICM42370_INTF_CONFIG0_FIFO_COUNT_ENDIAN BIT(5) > +#define INV_ICM42370_INTF_CONFIG0_SENSOR_DATA_ENDIAN BIT(4) > + > +#define INV_ICM42370_FIFO_WATERMARK_VAL(_wm) cpu_to_le16((_wm) & GENMASK(11, 0)) > +/* FIFO is 2048 bytes, let 12 samples for reading latency */ > +#define INV_ICM42370_FIFO_WATERMARK_MAX (2048 - 12 * 16) > + > +#define INV_ICM42370_PWR_MGMT0(_mode) FIELD_PREP(GENMASK(1, 0), (_mode)) > + > +#define INV_ICM42370_ACCEL_CONFIG0_FS(_fs) FIELD_PREP(GENMASK(6, 5), (_fs)) > +#define INV_ICM42370_ACCEL_CONFIG0_ODR(_odr) FIELD_PREP(GENMASK(3, 0), (_odr)) > + > +#define INV_ICM42370_TEMP_FILT_BW_DLPF(_dlpf) FIELD_PREP(GENMASK(6, 4), (_dlpf)) > + > +#define INV_ICM42370_MCLK_RDY_BIT BIT(3) > +#define INV_ICM42370_SOFT_RESET_BIT BIT(4) > +#define INV_ICM42370_ACCEL_MODE_LN 0x03 > + > +#define INV_ICM42370_DATA_INVALID -32768 > +#define INV_ICM42370_ACCEL_STARTUP_TIME_MS 10 > + > +typedef int (*inv_icm42370_bus_setup)(struct inv_icm42370_data *); > +extern const struct regmap_config inv_icm42370_regmap_config; > + > +int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq, > + inv_icm42370_bus_setup bus_setup); > +u32 inv_icm42370_odr_to_period(enum inv_icm42370_odr odr); > + > +int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq, > + inv_icm42370_bus_setup bus_setup); > + > +struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, > + struct inv_icm42370_data *data); > + > +int inv_icm42370_set_accel_conf(struct inv_icm42370_data *dev_data, > + struct inv_icm42370_conf *conf, > + unsigned int *sleep_ms); > + > +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev); > + > +#endif > 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>. > +#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"); > + > +static const struct inv_icm42370_conf inv_icm42370_default_conf = { > + .mode = INV_ICM42370_SENSOR_MODE_LOW_NOISE, > + .fs = INV_ICM42370_ACCEL_FS_16G, > + .odr = INV_ICM42370_ODR_400HZ, > + .filter = INV_ICM42370_FILTER_AVG_16X, > +}; > + > +static const struct iio_chan_spec inv_icm42370_accel_channels[] = { > + INV_ICM42370_ACCEL_CHAN(IIO_MOD_X, INV_ICM42370_ACCEL_SCAN_X), > + INV_ICM42370_ACCEL_CHAN(IIO_MOD_Y, INV_ICM42370_ACCEL_SCAN_Y), > + INV_ICM42370_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42370_ACCEL_SCAN_Z), > + INV_ICM42370_TEMP_CHAN(INV_ICM42370_ACCEL_SCAN_TEMP), > +}; > + > +/* 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, > + /* +/- 8G => 2*8*9.80665 / (2**15) m/s-2 */ > + [2 * INV_ICM42370_ACCEL_FS_8G] = 0, > + [2 * INV_ICM42370_ACCEL_FS_8G + 1] = 4788403, > + /* +/- 4G => 2*4*9.80665 / (2**15) m/s-2 */ > + [2 * INV_ICM42370_ACCEL_FS_4G] = 0, > + [2 * INV_ICM42370_ACCEL_FS_4G + 1] = 2394202, > + /* +/- 2G => 2*2*9.80665 / (2**15) m/s-2 */ > + [2 * INV_ICM42370_ACCEL_FS_2G] = 0, > + [2 * INV_ICM42370_ACCEL_FS_2G + 1] = 1197101, > +}; > + > +/** > + * 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. > + 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]; > +} > + > +/* ODR suffixed by LN or LP are Low-Noise or Low-Power mode only */ > +static const int inv_icm42370_accel_odr_conv[] = { > + INV_ICM42370_ODR_1_6KHZ_LN, INV_ICM42370_ODR_800HZ_LN, > + INV_ICM42370_ODR_400HZ, INV_ICM42370_ODR_200HZ, > + INV_ICM42370_ODR_100HZ, INV_ICM42370_ODR_50HZ, > + INV_ICM42370_ODR_25HZ, INV_ICM42370_ODR_12_5HZ, > + INV_ICM42370_ODR_6_25HZ_LP, INV_ICM42370_ODR_3_125HZ_LP, > + INV_ICM42370_ODR_1_5625HZ_LP, INV_ICM42370_ODR_NB, > +}; > + > +/** > + * inv_icm42370_mreg_check() - check registers before accessing the registers in other banks > + * > + * @map: regmap of the device > + * > + * Returns 0 on success, negative errno on error > + */ > +static int inv_icm42370_mreg_check(struct regmap *map) > +{ > + int tmp; > + > + regmap_read(map, INV_ICM42370_REG_MCLK_RDY, &tmp); > + if (!(tmp & INV_ICM42370_MCLK_RDY_BIT)) > + return -EINVAL; > + regmap_read(map, INV_ICM42370_REG_PWR_MGMT0, &tmp); > + if (tmp != INV_ICM42370_PWR_MGMT0(INV_ICM42370_SENSOR_MODE_LOW_NOISE)) > + return -EINVAL; > + > + return 0; > +} > + > +/** > + * inv_icm42370_mreg_write() - routine for writing to other bank registers > + * > + * @map: regmap of the device > + * @bank: register bank being accessed > + * @addr: address of the register being accessed > + * @val: value written to the register > + * > + * Returns 0 on success, negative errno on error > + */ > +static int inv_icm42370_mreg_write(struct regmap *map, u8 bank, u8 addr, u8 val) > +{ > + int ret; > + > + ret = inv_icm42370_mreg_check(map); > + if (ret) > + return ret; > + > + ret = regmap_write(map, INV_ICM42370_REG_BLK_SEL_W, bank); > + if (ret) > + return -EINVAL; > + > + ret = regmap_write(map, INV_ICM42370_REG_MADDR_W, addr); > + if (ret) > + return -EINVAL; > + > + ret = regmap_write(map, INV_ICM42370_REG_M_W, val); > + if (ret) > + return -EINVAL; > + > + usleep_range(10, 20); > + return regmap_write(map, INV_ICM42370_REG_BLK_SEL_W, 0x00); > +} > + > +/** > + * 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. > + 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. > + 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) > + 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); > +} > + > +/** > + * inv_icm42370_setup() - check and setup chip > + * > + * @data: pointer to struct containing the sensor data > + * > + * Returns 0 on success, a negative error code otherwise. > + */ > +static int inv_icm42370_setup(struct inv_icm42370_data *data, > + inv_icm42370_bus_setup bus_setup) > +{ > + const struct device *dev = regmap_get_device(data->map); > + unsigned int whoami; > + int ret; > + > + /* check chip self-identification value */ > + ret = regmap_read(data->map, INV_ICM42370_REG_WHO_AM_I, &whoami); > + if (ret) > + return ret; > + > + if (whoami != INV_ICM42370_WHOAMI_VALUE) { > + dev_err(dev, "Wrong WHO_AM_I: %d (want 0x%02X)\n", > + whoami, INV_ICM42370_WHOAMI_VALUE); > + return -ENODEV; > + } > + > + data->name = "inv_icm42370"; > + > + /* set chip bus configuration */ > + ret = bus_setup(data); > + if (ret) > + return ret; > + > + /* sensor data in big-endian (default) */ > + ret = regmap_set_bits(data->map, INV_ICM42370_REG_INTF_CONFIG0, > + INV_ICM42370_INTF_CONFIG0_SENSOR_DATA_ENDIAN); > + if (ret) > + return ret; > + > + return inv_icm42370_set_conf(data, &inv_icm42370_default_conf); > +} > + > +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). > + > + 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? > +} > + > +/** > + * 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. > + 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. > + > + 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; > + > +exit: > + mutex_unlock(&dev_data->lock); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + > + return ret; > +} > + > +/** > + * inv_icm42370_temp_read_raw() - read data from the temperature sensor > + * > + * @indio_dev: pointer to the industrial io struct > + * @chan: pointer to the iio channel specification > + * @val: integer part of the value > + * @val2: decimal part of the value > + * @mask: mask to differentiate between channel info > + * > + * Returns 0 on success, negative errno on error > + */ > +static int inv_icm42370_temp_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); > + s16 temp; > + int ret; > + > + if (chan->type != IIO_TEMP) > + return -EINVAL; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (!iio_device_claim_direct(indio_dev)) > + return -EBUSY; > + ret = inv_icm42370_temp_read(dev_data, &temp); > + iio_device_release_direct(indio_dev); > + if (ret) > + return ret; > + *val = temp; > + return IIO_VAL_INT; > + /* > + * T°C = (temp / 128) + 25 > + * Tm°C = 1000 * ((temp / 128) + 25) > + * Tm°C = 7.8125 * temp + 25000 > + * Tm°C = (temp + 3200) * 7.8125 > + * scale: 1000 / 128 ~= 7.8125 > + * offset: 3200 > + */ > + case IIO_CHAN_INFO_SCALE: > + *val = 7; > + *val2 = 812500; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_OFFSET: > + *val = 3200; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +/** > + * inv_icm42370_accel_read_offset() - read offset values from 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_read_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 bias; > + unsigned int reg; > + s16 offset; > + u8 data[2]; > + int ret; > + > + if (chan->type != IIO_ACCEL) > + return -EINVAL; > + > + switch (chan->channel2) { > + case IIO_MOD_X: > + reg = INV_ICM42370_REG_OFFSET_USER4; > + break; > + case IIO_MOD_Y: > + reg = INV_ICM42370_REG_OFFSET_USER6; > + break; > + case IIO_MOD_Z: > + reg = INV_ICM42370_REG_OFFSET_USER7; > + break; > + default: > + return -EINVAL; > + } > + > + 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)); > + > + mutex_unlock(&dev_data->lock); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + if (ret) > + return ret; > + > + /* 12 bits signed value */ > + switch (chan->channel2) { > + case IIO_MOD_X: > + offset = sign_extend32(((data[0] & 0xF0) << 4) | data[1], 11); > + break; > + case IIO_MOD_Y: > + offset = sign_extend32(((data[1] & 0x0F) << 8) | data[0], 11); > + break; > + case IIO_MOD_Z: > + offset = sign_extend32(((data[0] & 0xF0) << 4) | data[1], 11); > + break; > + default: > + return -EINVAL; > + } > + > + /* > + * 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. > + *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. > + 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. > + > + 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 *)®val); > + 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; > + > + ret = inv_icm42370_mreg_write(dev_data->map, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER5, > + dev_data->buffer[1]); > + if (ret) > + goto out_unlock; > + break; > + 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 *)®val); > + if (ret) > + goto out_unlock; > + dev_data->buffer[0] = offset & 0xFF; > + dev_data->buffer[1] = ((offset & 0xF00) >> 8) | (regval & 0xF0); > + > + ret = inv_icm42370_mreg_write(dev_data->map, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER7, > + dev_data->buffer[0]); > + if (ret) > + goto out_unlock; > + > + ret = inv_icm42370_mreg_write(dev_data->map, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER6, > + dev_data->buffer[1]); > + if (ret) > + goto out_unlock; > + > + break; > + case IIO_MOD_Z: > + /* OFFSET_USER7 register is shared */ > + inv_icm42370_mreg_read(dev_data->map, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER7, > + (u8 *)®val); > + > + 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_USER7, > + dev_data->buffer[0]); > + if (ret) > + goto out_unlock; > + > + ret = inv_icm42370_mreg_write(dev_data->map, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER8, > + dev_data->buffer[1]); > + if (ret) > + goto out_unlock; > + break; > + default: > + ret = -EINVAL; > + goto out_unlock; > + } > + > +out_unlock: > + mutex_unlock(&dev_data->lock); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + return ret; > +} > + > +/** > + * inv_icm42370_accel_read_scale() - read scaling data from the accelerometer > + * > + * @indio_dev: pointer to the industrial io struct > + * @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_read_scale(struct iio_dev *indio_dev, int *val, > + int *val2) > +{ > + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); > + unsigned int idx; > + > + idx = dev_data->conf.fs; > + > + *val = dev_data->sensor_state->scales[2 * idx]; > + *val2 = dev_data->sensor_state->scales[2 * idx + 1]; > + return IIO_VAL_INT_PLUS_NANO; > +} > + > +/** > + * inv_icm42370_accel_write_scale() - write scaling data to the accelerometer > + * > + * @indio_dev: pointer to the industrial io struct > + * @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_scale(struct iio_dev *indio_dev, int val, > + int val2) > +{ > + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); > + struct device *dev = regmap_get_device(dev_data->map); > + unsigned int idx; > + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; > + int ret; > + > + for (idx = 0; idx < dev_data->sensor_state->scales_len; idx += 2) { > + if (val == dev_data->sensor_state->scales[idx] && > + val2 == dev_data->sensor_state->scales[idx + 1]) > + break; > + } > + > + if (idx >= dev_data->sensor_state->scales_len) > + return -EINVAL; > + > + conf.fs = idx / 2; > + > + pm_runtime_get_sync(dev); > + mutex_lock(&dev_data->lock); > + > + ret = inv_icm42370_set_accel_conf(dev_data, &conf, NULL); > + > + mutex_unlock(&dev_data->lock); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + > + return ret; > +} > + > +/** > + * inv_icm42370_accel_read_odr() - read ODR data from the accelerometer > + * > + * @dev_data: pointer to struct containing the sensor data > + * @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_read_odr(struct inv_icm42370_data *dev_data, > + int *val, int *val2) > +{ > + unsigned int odr; > + unsigned int i; > + > + odr = dev_data->conf.odr; > + > + for (i = 0; i < ARRAY_SIZE(inv_icm42370_accel_odr_conv); ++i) { > + if (inv_icm42370_accel_odr_conv[i] == odr) > + break; > + } > + if (i >= ARRAY_SIZE(inv_icm42370_accel_odr_conv)) > + return -EINVAL; > + > + *val = inv_icm42370_accel_odr[2 * i]; > + *val2 = inv_icm42370_accel_odr[2 * i + 1]; > + > + return IIO_VAL_INT_PLUS_MICRO; > +} > + > +/** > + * inv_icm42370_accel_write_odr() - write ODR data to the accelerometer > + * > + * @indio_dev: pointer to struct containing the sensor data > + * @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_odr(struct iio_dev *indio_dev, int val, > + int val2) > +{ > + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); > + struct inv_sensors_timestamp *ts = &dev_data->sensor_state->ts; > + struct device *dev = regmap_get_device(dev_data->map); > + unsigned int idx; > + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; > + int ret; > + > + for (idx = 0; idx < ARRAY_SIZE(inv_icm42370_accel_odr); idx += 2) { > + if (val == inv_icm42370_accel_odr[idx] && > + val2 == inv_icm42370_accel_odr[idx + 1]) > + break; > + } > + if (idx >= ARRAY_SIZE(inv_icm42370_accel_odr)) > + return -EINVAL; > + > + conf.odr = inv_icm42370_accel_odr_conv[idx / 2]; > + > + pm_runtime_get_sync(dev); > + mutex_lock(&dev_data->lock); > + > + ret = inv_sensors_timestamp_update_odr( > + ts, inv_icm42370_odr_to_period(conf.odr), > + iio_buffer_enabled(indio_dev)); > + if (ret) > + goto out_unlock; > + > + ret = inv_icm42370_set_accel_conf(dev_data, &conf, NULL); > + if (ret) > + goto out_unlock; > + > +out_unlock: > + mutex_unlock(&dev_data->lock); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + > + return ret; > +} > + > +/** > + * inv_icm42370_accel_write_raw() - write raw attribute values to the accelerometer > + * @indio_dev: pointer to the IIO device structure. > + * @chan: pointer to the IIO channel specification. > + * @val: integer part of the value to write. > + * @val2: fractional part of the value to write. > + * @mask: bitmask specifying which attribute to write. > + * > + * Returns 0 on success, negative errno on error. > + */ > +static int inv_icm42370_accel_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); > + int ret; > + > + if (chan->type != IIO_ACCEL) > + return -EINVAL; > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + if (!iio_device_claim_direct(indio_dev)) > + return -EBUSY; > + ret = inv_icm42370_accel_write_scale(indio_dev, val, val2); > + iio_device_release_direct(indio_dev); > + return ret; > + case IIO_CHAN_INFO_SAMP_FREQ: > + return inv_icm42370_accel_write_odr(indio_dev, val, val2); > + case IIO_CHAN_INFO_CALIBBIAS: > + if (!iio_device_claim_direct(indio_dev)) > + return -EBUSY; > + ret = inv_icm42370_accel_write_offset(dev_data, chan, val, > + val2); > + iio_device_release_direct(indio_dev); > + return ret; > + > + default: > + return -EINVAL; > + } > +} > + > +/** > + * inv_icm42370_accel_read_sensor() - internal function to read accelerometer sensor registers > + * > + * @indio_dev: pointer to the industrial io struct I/O. > + * @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) > +{ > + struct inv_icm42370_data *data = iio_priv(indio_dev); > + struct device *dev = regmap_get_device(data->map); > + unsigned int reg; > + __be16 *value; > + int ret; > + > + if (chan->type != IIO_ACCEL) > + return -EINVAL; > + > + switch (chan->channel2) { > + case IIO_MOD_X: > + reg = INV_ICM42370_REG_ACCEL_DATA_X1; > + break; > + case IIO_MOD_Y: > + reg = INV_ICM42370_REG_ACCEL_DATA_Y1; > + break; > + case IIO_MOD_Z: > + reg = INV_ICM42370_REG_ACCEL_DATA_Z1; > + break; > + default: > + return -EINVAL; > + } > + > + pm_runtime_get_sync(dev); > + mutex_lock(&data->lock); > + > + /* read accel register data */ > + value = (__be16 *)&data->buffer[0]; > + ret = regmap_bulk_read(data->map, reg, value, sizeof(*value)); > + if (ret) > + goto exit; > + > + *val = (s16)be16_to_cpup(value); > + > + if (*val == INV_ICM42370_DATA_INVALID) > + ret = -EINVAL; > + > +exit: > + mutex_unlock(&data->lock); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + return ret; > +} > + > +/** > + * inv_icm42370_accel_read_raw() - read data from the accelerometer sensor > + * > + * @indio_dev: pointer to the industrial io struct > + * @chan: pointer to the iio channel specification > + * @val: integer part of the value > + * @val2: decimal part of the value > + * @mask: mask to differentiate between channel info > + * > + * Returns 0 on success, negative errno on error > + */ > +static int inv_icm42370_accel_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct inv_icm42370_data *dev_data = iio_priv(indio_dev); > + s16 value; > + int ret; > + > + switch (chan->type) { > + case IIO_ACCEL: > + break; > + case IIO_TEMP: > + return inv_icm42370_temp_read_raw(indio_dev, chan, val, val2, > + mask); > + default: > + return -EINVAL; > + } > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (!iio_device_claim_direct(indio_dev)) > + return -EBUSY; > + ret = inv_icm42370_accel_read_sensor(indio_dev, chan, &value); > + iio_device_release_direct(indio_dev); > + if (ret) > + return ret; > + *val = value; > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + return inv_icm42370_accel_read_scale(indio_dev, val, val2); > + case IIO_CHAN_INFO_SAMP_FREQ: > + return inv_icm42370_accel_read_odr(dev_data, val, val2); > + case IIO_CHAN_INFO_CALIBBIAS: > + return inv_icm42370_accel_read_offset(dev_data, chan, val, > + val2); > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info inv_icm42370_info = { > + .read_raw = inv_icm42370_accel_read_raw, > + .write_raw = inv_icm42370_accel_write_raw, > +}; > + > +struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, > + struct inv_icm42370_data *data) > +{ > + struct device *dev = regmap_get_device(data->map); > + struct inv_sensors_timestamp_chip ts_chip; > + int ret; > + > + data->sensor_state->scales = inv_icm42370_accel_scale; > + data->sensor_state->scales_len = ARRAY_SIZE(inv_icm42370_accel_scale); > + > + /* > + * 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. > + 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. > + 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. > + 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. > + 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) > + > + 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. > + 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. > +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. > +#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. > +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. > + {} > +}; > +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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] iio: accel: Add support for ICM42370P 2026-08-07 10:09 ` Joshua Crofts @ 2026-08-07 13:41 ` Kanak Shilledar 2026-08-07 14:13 ` Joshua Crofts 0 siblings, 1 reply; 14+ messages in thread From: Kanak Shilledar @ 2026-08-07 13:41 UTC (permalink / raw) To: joshua.crofts1@gmail.com Cc: dlechner@baylibre.com, Henrik Grimler, nuno.sa@analog.com, jean-baptiste.maneyrol@tdk.com, robh@kernel.org, jic23@kernel.org, andy@kernel.org, krzk+dt@kernel.org, linux-iio@vger.kernel.org, conor+dt@kernel.org, Kernel, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org [-- 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 *)®val); > > + 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] iio: accel: Add support for ICM42370P 2026-08-07 13:41 ` Kanak Shilledar @ 2026-08-07 14:13 ` Joshua Crofts 0 siblings, 0 replies; 14+ messages in thread From: Joshua Crofts @ 2026-08-07 14:13 UTC (permalink / raw) To: Kanak Shilledar Cc: dlechner@baylibre.com, Henrik Grimler, nuno.sa@analog.com, jean-baptiste.maneyrol@tdk.com, robh@kernel.org, jic23@kernel.org, andy@kernel.org, krzk+dt@kernel.org, linux-iio@vger.kernel.org, conor+dt@kernel.org, Kernel, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org On Fri, 7 Aug 2026 13:41:04 +0000 Kanak Shilledar <Kanak.Shilledar@axis.com> wrote: > 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? 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] iio: accel: icm42370: Add FIFO buffer functionality 2026-08-06 12:46 [PATCH 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar 2026-08-06 12:46 ` [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar 2026-08-06 12:46 ` [PATCH 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar @ 2026-08-06 12:46 ` Kanak Shilledar 2026-08-06 13:03 ` sashiko-bot 2 siblings, 1 reply; 14+ messages in thread From: Kanak Shilledar @ 2026-08-06 12:46 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Grimler, Jean-Baptiste Maneyrol Cc: linux-iio, devicetree, linux-kernel, kernel, Kanak Shilledar Add support for the hardware FIFO buffer to support high data rate capture. This includes necessary IIO buffersetup ops, watermark management and FIFO packet decoding. The accelerometer has 2.25kB FIFO size. The buffer can be handled via sysfs. Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com> --- drivers/iio/accel/Kconfig | 1 + drivers/iio/accel/Makefile | 1 + drivers/iio/accel/inv_icm42370.h | 10 +- drivers/iio/accel/inv_icm42370_buffer.c | 504 ++++++++++++++++++++++++++++++++ drivers/iio/accel/inv_icm42370_buffer.h | 97 ++++++ drivers/iio/accel/inv_icm42370_core.c | 209 ++++++++++++- 6 files changed, 817 insertions(+), 5 deletions(-) diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index cef3462e4c79a..ea037134a3ce1 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -491,6 +491,7 @@ config INV_ICM42370 tristate select IIO_BUFFER select IIO_INV_SENSORS_TIMESTAMP + select IIO_KFIFO_BUF config INV_ICM42370_I2C tristate "InvenSense ICM-42370 I2C driver" diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile index 6750b03edf518..939a23793735e 100644 --- a/drivers/iio/accel/Makefile +++ b/drivers/iio/accel/Makefile @@ -52,6 +52,7 @@ obj-$(CONFIG_IIO_KX022A_SPI) += kionix-kx022a-spi.o obj-$(CONFIG_INV_ICM42370) += inv-icm42370.o inv-icm42370-y += inv_icm42370_core.o +inv-icm42370-y += inv_icm42370_buffer.o obj-$(CONFIG_INV_ICM42370_I2C) += inv_icm42370_i2c.o obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o diff --git a/drivers/iio/accel/inv_icm42370.h b/drivers/iio/accel/inv_icm42370.h index 9866a5e970dcd..bd512ebecc3df 100644 --- a/drivers/iio/accel/inv_icm42370.h +++ b/drivers/iio/accel/inv_icm42370.h @@ -15,6 +15,8 @@ #include <linux/iio/iio.h> #include <linux/iio/common/inv_sensors_timestamp.h> +#include "inv_icm42370_buffer.h" + enum inv_icm42370_chip { INV_CHIP_INVALID, INV_CHIP_ICM42370, @@ -252,6 +254,8 @@ static const int inv_icm42370_accel_odr[] = { #define INV_ICM42370_REG_INT_STATUS 0x3A #define INV_ICM42370_REG_TEMP_CONFIG0 0x34 #define INV_ICM42370_REG_INTF_CONFIG0 0x35 +#define INV_ICM42370_REG_FIFO_COUNT 0x3D +#define INV_ICM42370_REG_FIFO_DATA 0x3F #define INV_ICM42370_REG_WHO_AM_I 0x75 #define INV_ICM42370_REG_BLK_SEL_W 0x79 #define INV_ICM42370_REG_MADDR_W 0x7A @@ -275,8 +279,8 @@ static const int inv_icm42370_accel_odr[] = { FIELD_PREP(INV_ICM42370_DRIVE_CONFIG3_SPI_MASK, (_rate)) #define INV_ICM42370_SIGNAL_PATH_RESET_FIFO_FLUSH BIT(2) -#define INV_ICM42370_FIFO_CONFIG_MODE_MASK BIT(0) -#define INV_ICM42370_FIFO_CONFIG_BYPASS_MASK BIT(1) +#define INV_ICM42370_FIFO_CONFIG_MODE_MASK BIT(1) +#define INV_ICM42370_FIFO_CONFIG_BYPASS_MASK BIT(0) #define INV_ICM42370_FIFO_CONFIG_STREAM \ FIELD_PREP(INV_ICM42370_FIFO_CONFIG_MODE_MASK, 0) #define INV_ICM42370_FIFO_CONFIG_STOP_ON_FULL \ @@ -361,5 +365,7 @@ int inv_icm42370_set_accel_conf(struct inv_icm42370_data *dev_data, unsigned int *sleep_ms); int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev); +int inv_icm42370_mreg_write(struct regmap *map, u8 bank, u8 addr, u8 val); +int inv_icm42370_mreg_read(struct regmap *map, u8 bank, u8 addr, u8 *val); #endif diff --git a/drivers/iio/accel/inv_icm42370_buffer.c b/drivers/iio/accel/inv_icm42370_buffer.c new file mode 100644 index 0000000000000..a39b8aa4dd9bf --- /dev/null +++ b/drivers/iio/accel/inv_icm42370_buffer.c @@ -0,0 +1,504 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 Invensense, Inc. + * Copyright (C) 2026 Axis Communications AB + */ + +#include <linux/kernel.h> +#include <linux/device.h> +#include <linux/minmax.h> +#include <linux/mutex.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/delay.h> + +#include <linux/iio/buffer.h> +#include <linux/iio/common/inv_sensors_timestamp.h> +#include <linux/iio/iio.h> + +#include "inv_icm42370.h" +#include "inv_icm42370_buffer.h" + +/* FIFO header: 1 byte */ +#define INV_ICM42370_FIFO_HEADER_MSG BIT(7) +#define INV_ICM42370_FIFO_HEADER_ACCEL BIT(6) +#define INV_ICM42370_FIFO_HEADER_ODR_ACCEL BIT(1) + +struct inv_icm42370_fifo_packet_1 { + u8 header; + struct inv_icm42370_fifo_sensor_data data; + s8 temp; +} __packed; + +#define INV_ICM42370_FIFO_PACKET_1_SIZE 8 + +ssize_t inv_icm42370_fifo_decode_packet(const void *packet, const void **accel, + const s8 **temp, const void **timestamp, + unsigned int *odr) +{ + const struct inv_icm42370_fifo_packet_1 *pack1 = packet; + u8 header = *((const u8 *)packet); + + /* FIFO empty */ + if (header & INV_ICM42370_FIFO_HEADER_MSG) { + *accel = NULL; + *temp = NULL; + *timestamp = NULL; + *odr = 0; + return 0; + } + + /* ODR change flag */ + *odr = 0; + if (header & INV_ICM42370_FIFO_HEADER_ODR_ACCEL) + *odr |= INV_ICM42370_SENSOR_ACCEL; + + /* With TMST_EN disabled, all packets are Packet 1 (8 bytes) */ + if (header & INV_ICM42370_FIFO_HEADER_ACCEL) { + *accel = &pack1->data; + *temp = &pack1->temp; + *timestamp = NULL; + return INV_ICM42370_FIFO_PACKET_1_SIZE; + } + + /* invalid or unsupported packet format */ + return -EINVAL; +} + +void inv_icm42370_buffer_update_fifo_period(struct inv_icm42370_data *st) +{ + u32 period_accel; + + if (st->fifo.en & INV_ICM42370_SENSOR_ACCEL) + period_accel = inv_icm42370_odr_to_period(st->conf.odr); + else + period_accel = U32_MAX; + + st->fifo.period = period_accel; +} + +int inv_icm42370_buffer_set_fifo_en(struct inv_icm42370_data *st, + unsigned int fifo_en) +{ + u8 mask, val, regval; + int ret; + + /* update only FIFO EN bits */ + mask = INV_ICM42370_FIFO_CONFIG5_ACCEL_EN; + + val = 0; + if (fifo_en & INV_ICM42370_SENSOR_ACCEL) + val |= INV_ICM42370_FIFO_CONFIG5_ACCEL_EN; + + ret = inv_icm42370_mreg_read(st->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_FIFO_CONFIG5, ®val); + if (ret) + return ret; + + /* clear the mask bits and set the new values */ + regval &= ~mask; + regval |= val; + + ret = inv_icm42370_mreg_write(st->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_FIFO_CONFIG5, regval); + if (ret) + return ret; + + st->fifo.en = fifo_en; + inv_icm42370_buffer_update_fifo_period(st); + + return 0; +} + +static size_t inv_icm42370_get_packet_size(unsigned int fifo_en) +{ + /* + * With TMST_EN disabled, the device always produces Packet 1 + * (8 bytes: 1 header + 6 accel + 1 temp). + */ + return INV_ICM42370_FIFO_PACKET_1_SIZE; +} + +static unsigned int inv_icm42370_wm_truncate(unsigned int watermark, + size_t packet_size) +{ + size_t wm_size; + unsigned int wm; + + wm_size = watermark * packet_size; + if (wm_size > INV_ICM42370_FIFO_WATERMARK_MAX) + wm_size = INV_ICM42370_FIFO_WATERMARK_MAX; + + wm = wm_size / packet_size; + + return wm; +} + +/** + * inv_icm42370_buffer_update_watermark - update watermark FIFO threshold + * @st: driver internal state + * + * Returns 0 on success, a negative error code otherwise. + * + * FIFO watermark threshold is computed based on the required + * watermark values set for accel sensor. + */ +int inv_icm42370_buffer_update_watermark(struct inv_icm42370_data *st) +{ + size_t packet_size, wm_size; + unsigned int wm_accel, watermark; + bool restore; + __le16 raw_wm; + int ret; + + packet_size = inv_icm42370_get_packet_size(st->fifo.en); + + /* compute sensors latency, depending on sensor watermark and odr */ + wm_accel = + inv_icm42370_wm_truncate(st->fifo.watermark.accel, packet_size); + + /* 0 value for watermark means that the sensor is turned off */ + if (wm_accel == 0) + return 0; + + watermark = wm_accel; + st->fifo.watermark.eff_accel = wm_accel; + + /* compute watermark value in bytes */ + wm_size = watermark * packet_size; + + /* changing FIFO watermark requires to turn off watermark interrupt */ + ret = regmap_update_bits_check( + st->map, INV_ICM42370_REG_INT_SOURCE0, + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN, 0, &restore); + if (ret) + return ret; + + raw_wm = INV_ICM42370_FIFO_WATERMARK_VAL(wm_size); + memcpy(st->buffer, &raw_wm, sizeof(raw_wm)); + ret = regmap_bulk_write(st->map, INV_ICM42370_REG_FIFO_WATERMARK, + st->buffer, sizeof(raw_wm)); + if (ret) + return ret; + + /* restore watermark interrupt */ + if (restore) { + ret = regmap_set_bits( + st->map, INV_ICM42370_REG_INT_SOURCE0, + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN); + if (ret) + return ret; + } + + return 0; +} + +static int inv_icm42370_buffer_preenable(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *st = iio_priv(indio_dev); + struct device *dev = regmap_get_device(st->map); + struct inv_icm42370_sensor_state *sensor_st = st->sensor_state; + struct inv_sensors_timestamp *ts = &sensor_st->ts; + + pm_runtime_get_sync(dev); + + guard(mutex) + (&st->lock); + inv_sensors_timestamp_reset(ts); + + return 0; +} + +/** + * update_scan_mode callback - turn sensor on and set data FIFO enable bits + * @indio_dev: pointer to the industrial io struct + * + * Return 0 on success, negative errno on error + */ +static int inv_icm42370_buffer_postenable(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *st = iio_priv(indio_dev); + int ret; + + guard(mutex) + (&st->lock); + + if (st->fifo.on) { + st->fifo.on++; + return 0; + } + + ret = regmap_write(st->map, INV_ICM42370_REG_SIGNAL_PATH_RESET, + INV_ICM42370_SIGNAL_PATH_RESET_FIFO_FLUSH); + if (ret) + return ret; + + ret = regmap_write(st->map, INV_ICM42370_REG_FIFO_CONFIG1, + INV_ICM42370_FIFO_CONFIG_STREAM); + if (ret) + return ret; + + ret = regmap_bulk_read(st->map, INV_ICM42370_REG_FIFO_COUNT, st->buffer, + 2); + if (ret) + return ret; + + ret = regmap_set_bits(st->map, INV_ICM42370_REG_INT_SOURCE0, + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN); + if (ret) + return ret; + + st->fifo.on++; + + return 0; +} + +static int inv_icm42370_buffer_predisable(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *st = iio_priv(indio_dev); + int ret; + + guard(mutex) + (&st->lock); + + if (st->fifo.on > 1) { + st->fifo.on--; + return 0; + } + + /* set FIFO in bypass mode */ + ret = regmap_write(st->map, INV_ICM42370_REG_FIFO_CONFIG1, + INV_ICM42370_FIFO_CONFIG_BYPASS); + if (ret) + return ret; + + /* flush FIFO data */ + ret = regmap_write(st->map, INV_ICM42370_REG_SIGNAL_PATH_RESET, + INV_ICM42370_SIGNAL_PATH_RESET_FIFO_FLUSH); + if (ret) + return ret; + + /* disable FIFO threshold interrupt */ + ret = regmap_clear_bits(st->map, INV_ICM42370_REG_INT_SOURCE0, + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN); + if (ret) + return ret; + + st->fifo.on--; + + return 0; +} + +static int inv_icm42370_buffer_postdisable(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *st = iio_priv(indio_dev); + struct inv_icm42370_sensor_state *sensor_st = st->sensor_state; + struct inv_sensors_timestamp *ts = &sensor_st->ts; + struct device *dev = regmap_get_device(st->map); + unsigned int sensor; + unsigned int *watermark; + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; + unsigned int sleep_temp = 0; + unsigned int sleep_sensor = 0; + unsigned int sleep; + int ret; + + if (indio_dev == st->indio_accel) { + sensor = INV_ICM42370_SENSOR_ACCEL; + watermark = &st->fifo.watermark.accel; + } else { + return -EINVAL; + } + + mutex_lock(&st->lock); + + inv_sensors_timestamp_apply_odr(ts, 0, 0, 0); + + ret = inv_icm42370_buffer_set_fifo_en(st, st->fifo.en & ~sensor); + if (ret) + goto out_unlock; + + *watermark = 0; + ret = inv_icm42370_buffer_update_watermark(st); + if (ret) + goto out_unlock; + + conf.mode = INV_ICM42370_SENSOR_MODE_OFF; + ret = inv_icm42370_set_accel_conf(st, &conf, &sleep_sensor); + if (ret) + goto out_unlock; + +out_unlock: + mutex_unlock(&st->lock); + + /* sleep maximum required time */ + sleep = max(sleep_sensor, sleep_temp); + if (sleep) + msleep(sleep); + + pm_runtime_put_autosuspend(dev); + + return ret; +} + +const struct iio_buffer_setup_ops inv_icm42370_buffer_ops = { + .preenable = inv_icm42370_buffer_preenable, + .postenable = inv_icm42370_buffer_postenable, + .predisable = inv_icm42370_buffer_predisable, + .postdisable = inv_icm42370_buffer_postdisable, +}; + +static int inv_icm42370_fifo_read_data(struct inv_icm42370_data *st, + size_t count) +{ + return regmap_noinc_read(st->map, INV_ICM42370_REG_FIFO_DATA, + st->fifo.data, count); +} + +int inv_icm42370_buffer_fifo_read(struct inv_icm42370_data *st, + unsigned int max) +{ + size_t max_count; + __be16 *raw_fifo_count; + ssize_t i, size; + const void *accel, *timestamp; + const s8 *temp; + unsigned int odr; + int ret; + + /* reset all samples counters */ + st->fifo.count = 0; + st->fifo.nb.accel = 0; + st->fifo.nb.total = 0; + + /* compute maximum FIFO read size */ + if (max == 0) + max_count = sizeof(st->fifo.data); + else + max_count = max * inv_icm42370_get_packet_size(st->fifo.en); + + /* read FIFO count value */ + raw_fifo_count = (__be16 *)st->buffer; + ret = regmap_bulk_read(st->map, INV_ICM42370_REG_FIFO_COUNT, + raw_fifo_count, sizeof(*raw_fifo_count)); + if (ret) + return ret; + st->fifo.count = be16_to_cpup(raw_fifo_count); + + /* check and clamp FIFO count value */ + if (st->fifo.count == 0) + return 0; + if (st->fifo.count > max_count) + st->fifo.count = max_count; + + /* read all FIFO data in internal buffer */ + ret = inv_icm42370_fifo_read_data(st, st->fifo.count); + if (ret) + return ret; + + /* compute number of samples for each sensor */ + for (i = 0; i < st->fifo.count; i += size) { + size = inv_icm42370_fifo_decode_packet( + &st->fifo.data[i], &accel, &temp, ×tamp, &odr); + if (size <= 0) + break; + if (accel != NULL && inv_icm42370_fifo_is_data_valid(accel)) + st->fifo.nb.accel++; + st->fifo.nb.total++; + } + + return 0; +} + +int inv_icm42370_buffer_fifo_parse(struct inv_icm42370_data *st) +{ + struct inv_icm42370_sensor_state *accel_st = st->sensor_state; + struct inv_sensors_timestamp *ts; + int ret; + + if (st->fifo.nb.total == 0) + return 0; + + /* handle accelerometer timestamp and FIFO data parsing */ + if (st->fifo.nb.accel > 0) { + ts = &accel_st->ts; + inv_sensors_timestamp_interrupt( + ts, st->fifo.watermark.eff_accel, st->timestamp); + ret = inv_icm42370_accel_parse_fifo(st->indio_accel); + if (ret) + return ret; + } + + return 0; +} + +int inv_icm42370_buffer_hwfifo_flush(struct inv_icm42370_data *st, + unsigned int count) +{ + struct inv_icm42370_sensor_state *accel_st = st->sensor_state; + struct inv_sensors_timestamp *ts; + s64 accel_ts; + int ret; + + accel_ts = iio_get_time_ns(st->indio_accel); + + ret = inv_icm42370_buffer_fifo_read(st, count); + if (ret) + return ret; + + if (st->fifo.nb.total == 0) + return 0; + + if (st->fifo.nb.accel > 0) { + ts = &accel_st->ts; + inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, + accel_ts); + ret = inv_icm42370_accel_parse_fifo(st->indio_accel); + if (ret) + return ret; + } + + return 0; +} + +int inv_icm42370_buffer_init(struct inv_icm42370_data *st) +{ + unsigned int val; + u8 regval; + int ret; + + st->fifo.watermark.eff_accel = 1; + + /* watermark should be set to a non-zero value before enabling interrupts */ + st->fifo.watermark.accel = 1; + ret = inv_icm42370_buffer_update_watermark(st); + if (ret) + return ret; + + /* + * Default FIFO configuration (bits 6 to 5) + * - FIFO count in bytes + * - FIFO count in big endian + */ + val = INV_ICM42370_INTF_CONFIG0_FIFO_COUNT_ENDIAN; + ret = regmap_update_bits(st->map, INV_ICM42370_REG_INTF_CONFIG0, + GENMASK(6, 5), val); + if (ret) + return ret; + + /* + * Enable FIFO partial read interrupt. + * Disable all FIFO EN bits. + */ + ret = inv_icm42370_mreg_read(st->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_FIFO_CONFIG5, ®val); + if (ret) + return ret; + + regval &= ~(GENMASK(6, 5) | GENMASK(3, 0)); + regval |= INV_ICM42370_FIFO_CONFIG5_WM_GT_TH; + regval |= INV_ICM42370_FIFO_CONFIG5_RESUME_PARTIAL_RD; + + return inv_icm42370_mreg_write(st->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_FIFO_CONFIG5, regval); +} diff --git a/drivers/iio/accel/inv_icm42370_buffer.h b/drivers/iio/accel/inv_icm42370_buffer.h new file mode 100644 index 0000000000000..fb4e002c0067f --- /dev/null +++ b/drivers/iio/accel/inv_icm42370_buffer.h @@ -0,0 +1,97 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2020 Invensense, Inc. + * Copyright (C) 2026 Axis Communications AB + */ + +#ifndef INV_ICM42370_BUFFER_H_ +#define INV_ICM42370_BUFFER_H_ + +#include <linux/kernel.h> +#include <linux/bits.h> + +struct inv_icm42370_data; + +#define INV_ICM42370_SENSOR_ACCEL BIT(1) +#define INV_ICM42370_SENSOR_TEMP BIT(2) + +/** + * struct inv_icm42370_fifo - FIFO state variables + * @on: reference counter for FIFO on. + * @en: bits field of INV_ICM42370_SENSOR_* for FIFO EN bits. + * @period: FIFO internal period. + * @watermark: watermark configuration values for accel. + * @count: number of bytes in the FIFO data buffer. + * @nb: accel and total samples in the FIFO data buffer. + * @data: FIFO data buffer aligned for DMA (2kB + 32 bytes of read cache). + */ +struct inv_icm42370_fifo { + struct { + size_t accel; + size_t total; + } nb; + struct { + unsigned int accel; + unsigned int eff_accel; + } watermark; + unsigned int on; + unsigned int en; + size_t count; + u8 data[2080] __aligned(IIO_DMA_MINALIGN); + u32 period; +}; + +/* FIFO data packet */ +struct inv_icm42370_fifo_sensor_data { + __be16 x; + __be16 y; + __be16 z; +} __packed; +#define INV_ICM42370_FIFO_DATA_INVALID -32768 + +static inline s16 inv_icm42370_fifo_get_sensor_data(__be16 d) +{ + return be16_to_cpu(d); +} + +static inline bool +inv_icm42370_fifo_is_data_valid(const struct inv_icm42370_fifo_sensor_data *s) +{ + s16 x, y, z; + + x = inv_icm42370_fifo_get_sensor_data(s->x); + y = inv_icm42370_fifo_get_sensor_data(s->y); + z = inv_icm42370_fifo_get_sensor_data(s->z); + + if (x == INV_ICM42370_FIFO_DATA_INVALID && + y == INV_ICM42370_FIFO_DATA_INVALID && + z == INV_ICM42370_FIFO_DATA_INVALID) + return false; + + return true; +} + +ssize_t inv_icm42370_fifo_decode_packet(const void *packet, const void **accel, + const s8 **temp, const void **timestamp, + unsigned int *odr); + +extern const struct iio_buffer_setup_ops inv_icm42370_buffer_ops; + +int inv_icm42370_buffer_init(struct inv_icm42370_data *st); + +void inv_icm42370_buffer_update_fifo_period(struct inv_icm42370_data *st); + +int inv_icm42370_buffer_set_fifo_en(struct inv_icm42370_data *st, + unsigned int fifo_en); + +int inv_icm42370_buffer_update_watermark(struct inv_icm42370_data *st); + +int inv_icm42370_buffer_fifo_read(struct inv_icm42370_data *st, + unsigned int max); + +int inv_icm42370_buffer_fifo_parse(struct inv_icm42370_data *st); + +int inv_icm42370_buffer_hwfifo_flush(struct inv_icm42370_data *st, + unsigned int count); + +#endif diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c index 9f6c302e6f331..7107fc7a682dd 100644 --- a/drivers/iio/accel/inv_icm42370_core.c +++ b/drivers/iio/accel/inv_icm42370_core.c @@ -15,17 +15,33 @@ #include <linux/property.h> #include <linux/regmap.h> +#include <linux/iio/buffer.h> #include <linux/iio/common/inv_sensors_timestamp.h> #include <linux/iio/iio.h> +#include <linux/iio/kfifo_buf.h> #include <linux/iio/sysfs.h> #include "inv_icm42370.h" +#include "inv_icm42370_buffer.h" + +#define INV_ICM42370_SCAN_MASK_ACCEL_3AXIS \ + (BIT(INV_ICM42370_ACCEL_SCAN_X) | \ + BIT(INV_ICM42370_ACCEL_SCAN_Y) | \ + BIT(INV_ICM42370_ACCEL_SCAN_Z)) + +#define INV_ICM42370_SCAN_MASK_TEMP BIT(INV_ICM42370_ACCEL_SCAN_TEMP) + +static bool inv_icm42370_is_noinc_reg(struct device *dev, unsigned int reg) +{ + return reg == INV_ICM42370_REG_FIFO_DATA; +} const struct regmap_config inv_icm42370_regmap_config = { .name = "inv_icm42370", .reg_bits = 8, .val_bits = 8, .max_register = 0x7E, + .readable_noinc_reg = inv_icm42370_is_noinc_reg, }; EXPORT_SYMBOL_NS_GPL(inv_icm42370_regmap_config, "IIO_ICM42370"); @@ -59,6 +75,18 @@ static const int inv_icm42370_accel_scale[] = { [2 * INV_ICM42370_ACCEL_FS_2G + 1] = 1197101, }; +/* + * IIO buffer layout: must match channel scan types. + * Accel: 3 x s16 BE (6 bytes), Temp: 1 x s16 native (2 bytes) = 8 bytes data. + * Timestamp: s64 at 8-byte aligned offset. + */ +struct inv_icm42370_accel_buffer { + struct inv_icm42370_fifo_sensor_data accel; + s16 temp; + + s64 timestamp __aligned(8); +}; + /** * inv_icm42370_odr_to_period() - map ODR to Period * @odr - enum of ODR value @@ -142,7 +170,7 @@ static int inv_icm42370_mreg_check(struct regmap *map) * * Returns 0 on success, negative errno on error */ -static int inv_icm42370_mreg_write(struct regmap *map, u8 bank, u8 addr, u8 val) +int inv_icm42370_mreg_write(struct regmap *map, u8 bank, u8 addr, u8 val) { int ret; @@ -176,7 +204,7 @@ static int inv_icm42370_mreg_write(struct regmap *map, u8 bank, u8 addr, u8 val) * * Returns 0 on success, negative errno on error */ -static int inv_icm42370_mreg_read(struct regmap *map, u8 bank, u8 addr, u8 *val) +int inv_icm42370_mreg_read(struct regmap *map, u8 bank, u8 addr, u8 *val) { int ret; unsigned int read_val; @@ -424,6 +452,7 @@ static irqreturn_t inv_icm42370_irq_timestamp(int irq, void *_data) static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data) { struct inv_icm42370_data *dev_data = _data; + struct device *dev = regmap_get_device(dev_data->map); unsigned int status; int ret; @@ -433,6 +462,21 @@ static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data) if (ret) goto out_unlock; + if (status & INV_ICM42370_INT_STATUS_FIFO_FULL) + dev_warn_ratelimited(dev, "FIFO full data lost!\n"); + + if (status & (INV_ICM42370_INT_STATUS_FIFO_THS | + INV_ICM42370_INT_STATUS_FIFO_FULL)) { + ret = inv_icm42370_buffer_fifo_read(dev_data, 0); + if (ret) { + dev_err_ratelimited(dev, "FIFO read error %d\n", ret); + goto out_unlock; + } + ret = inv_icm42370_buffer_fifo_parse(dev_data); + if (ret) + dev_err_ratelimited(dev, "FIFO parsing error %d\n", ret); + } + out_unlock: mutex_unlock(&dev_data->lock); return IRQ_HANDLED; @@ -499,6 +543,27 @@ static int inv_icm42370_irq_init(struct inv_icm42370_data *data, int irq, "inv_icm42370", data); } +static int inv_icm42370_timestamp_setup(struct inv_icm42370_data *data) +{ + u8 val; + int ret; + + ret = inv_icm42370_mreg_read(data->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_TMST_CONFIG1, &val); + if (ret) + return ret; + + /* + * Disable FIFO timestamp to produce 8-byte Packet 1. + * Host-side timestamps are interpolated from the IRQ timestamp + * and ODR period via inv_sensors_timestamp. + */ + val &= ~INV_ICM42370_TMST_CONFIG_TMST_EN; + + return inv_icm42370_mreg_write(data->map, INV_ICM42370_MREG1, + INV_ICM42370_REG_TMST_CONFIG1, val); +} + /* * Calibration bias values, IIO range format int + micro. * Value is limited to +/-1g coded on 12 bits signed. Step is 0.5mg. @@ -1009,6 +1074,43 @@ static int inv_icm42370_accel_write_raw(struct iio_dev *indio_dev, } } +/* enable accelerometer sensor and FIFO write */ +static int inv_icm42370_accel_update_scan_mode(struct iio_dev *indio_dev, + const unsigned long *scan_mask) +{ + struct inv_icm42370_data *st = iio_priv(indio_dev); + struct inv_icm42370_sensor_state *accel_st = st->sensor_state; + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; + unsigned int fifo_en = 0; + unsigned int sleep_temp = 0; + unsigned int sleep_accel = 0; + unsigned int sleep; + int ret; + + mutex_lock(&st->lock); + + if (*scan_mask & INV_ICM42370_SCAN_MASK_ACCEL_3AXIS) { + /* enable accel sensor */ + conf.mode = st->conf.mode; + conf.filter = accel_st->filter; + ret = inv_icm42370_set_accel_conf(st, &conf, &sleep_accel); + if (ret) + goto out_unlock; + fifo_en |= INV_ICM42370_SENSOR_ACCEL; + } + + /* update data FIFO write */ + ret = inv_icm42370_buffer_set_fifo_en(st, fifo_en | st->fifo.en); + +out_unlock: + mutex_unlock(&st->lock); + /* sleep maximum required time */ + sleep = max(sleep_accel, sleep_temp); + if (sleep) + msleep(sleep); + return ret; +} + /** * inv_icm42370_accel_read_sensor() - internal function to read accelerometer sensor registers * @@ -1117,9 +1219,48 @@ static int inv_icm42370_accel_read_raw(struct iio_dev *indio_dev, } } +static int inv_icm42370_accel_hwfifo_set_watermark(struct iio_dev *indio_dev, + unsigned int val) +{ + struct inv_icm42370_data *st = iio_priv(indio_dev); + int ret; + + mutex_lock(&st->lock); + + st->fifo.watermark.accel = val; + ret = inv_icm42370_buffer_update_watermark(st); + + mutex_unlock(&st->lock); + + return ret; +} + +static int inv_icm42370_accel_hwfifo_flush(struct iio_dev *indio_dev, + unsigned int count) +{ + struct inv_icm42370_data *st = iio_priv(indio_dev); + int ret; + + if (count == 0) + return 0; + + mutex_lock(&st->lock); + + ret = inv_icm42370_buffer_hwfifo_flush(st, count); + if (!ret) + ret = st->fifo.nb.accel; + + mutex_unlock(&st->lock); + + return ret; +} + static const struct iio_info inv_icm42370_info = { .read_raw = inv_icm42370_accel_read_raw, .write_raw = inv_icm42370_accel_write_raw, + .update_scan_mode = inv_icm42370_accel_update_scan_mode, + .hwfifo_set_watermark = inv_icm42370_accel_hwfifo_set_watermark, + .hwfifo_flush_to_buffer = inv_icm42370_accel_hwfifo_flush, }; struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, @@ -1131,6 +1272,8 @@ struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, data->sensor_state->scales = inv_icm42370_accel_scale; data->sensor_state->scales_len = ARRAY_SIZE(inv_icm42370_accel_scale); + data->sensor_state->filter = data->conf.filter; + data->sensor_state->power_mode = data->conf.mode; /* * clock period is 32kHz (31250ns) @@ -1143,10 +1286,15 @@ struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, indio_dev->name = "inv_icm42370"; indio_dev->info = &inv_icm42370_info; - indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_ALL_BUFFER_MODES; indio_dev->channels = inv_icm42370_accel_channels; indio_dev->num_channels = ARRAY_SIZE(inv_icm42370_accel_channels); + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, + &inv_icm42370_buffer_ops); + if (ret) + return ERR_PTR(ret); + ret = devm_iio_device_register(dev, indio_dev); if (ret) return ERR_PTR(ret); @@ -1154,6 +1302,53 @@ struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, return indio_dev; } +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *st = iio_priv(indio_dev); + struct inv_icm42370_sensor_state *accel_st = st->sensor_state; + struct inv_sensors_timestamp *ts = &accel_st->ts; + ssize_t i, size; + unsigned int no; + const void *accel, *timestamp; + const s8 *temp; + unsigned int odr; + s64 ts_val; + struct inv_icm42370_accel_buffer buffer = {}; + + for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) { + size = inv_icm42370_fifo_decode_packet(&st->fifo.data[i], + &accel, &temp, ×tamp, &odr); + if (size <= 0) + return size; + + if (accel == NULL || !inv_icm42370_fifo_is_data_valid(accel)) + continue; + + if (odr & INV_ICM42370_SENSOR_ACCEL) + inv_sensors_timestamp_apply_odr(ts, st->fifo.period, + st->fifo.nb.total, no); + + memcpy(&buffer.accel, accel, sizeof(buffer.accel)); + + /* + * FIFO 8-bit temp has sensitivity ~2 LSB/°C. + * Register 16-bit temp has sensitivity 128 LSB/°C. + * Scale factor: 128 / 2 = 64. + * This lets the IIO scale (1000/128) and offset (3200) work + * correctly for both register reads and FIFO data. + */ + if (temp) + buffer.temp = (s16)*temp * 64; + else + buffer.temp = (s16)INV_ICM42370_DATA_INVALID; + + ts_val = inv_sensors_timestamp_pop(ts); + iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val); + } + + return 0; +} + /** * inv_icm42370_core_probe() - initialize and register the ICM-42370 device * @regmap: register map for accessing the device's registers. @@ -1222,6 +1417,14 @@ int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq, if (ret) return dev_err_probe(dev, ret, "Setup failed\n"); + ret = inv_icm42370_timestamp_setup(data); + if (ret) + return ret; + + ret = inv_icm42370_buffer_init(data); + if (ret) + return ret; + data->indio_accel = inv_icm42370_accel_init(indio_dev, data); if (IS_ERR(data->indio_accel)) return PTR_ERR(data->indio_accel); -- 2.43.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] iio: accel: icm42370: Add FIFO buffer functionality 2026-08-06 12:46 ` [PATCH 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar @ 2026-08-06 13:03 ` sashiko-bot 0 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-06 13:03 UTC (permalink / raw) To: Kanak Shilledar; +Cc: conor+dt, devicetree, robh Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider: New issues: - [Critical] Heap buffer overflow in `inv_icm42370_buffer_fifo_read()` when flushing the hardware FIFO via sysfs. - [High] The return value of `pm_runtime_get_sync()` is ignored in `inv_icm42370_buffer_preenable()`, potentially causing hardware accesses on a suspended device. - [High] The FIFO parsing loop reads out-of-bounds data if the hardware reports a partial packet size. - [Medium] If `regmap_bulk_write()` fails in `inv_icm42370_buffer_update_watermark()`, the function returns early without restoring the FIFO watermark interrupt. - [Medium] If hardware writes fail in `inv_icm42370_buffer_predisable()`, the internal reference counter `st->fifo.on` is not decremented, permanently breaking subsequent buffer enablements. - [Medium] `inv_icm42370_buffer_postdisable` incorrectly erases the user-configured watermark by setting `st->fifo.watermark.accel = 0`. - [Medium] Hardware FIFO sysfs callbacks (`hwfifo_set_watermark`, `hwfifo_flush`) access registers without PM runtime protection. - [Medium] Stale `st->fifo.period` cache corrupts timestamps when ODR is changed on the fly. - [Medium] The driver adds IIO buffer support but forgets to include `IIO_CHAN_SOFT_TIMESTAMP` in its channel array. - [Medium] The driver allows the temperature channel to be enabled without the accelerometer, but fails to turn on the sensor in this case. Pre-existing issues: - [High] `inv_icm42370_set_accel_conf` ignores the requested sensor mode, preventing the device from being turned off when the buffer is disabled. - [High] The IIO device is registered before PM runtime and IRQ initialization, exposing the device prematurely and breaking devres teardown ordering. - [High] The driver suffers from a PM runtime usage counter underflow due to unbalanced puts during probe and removal. - [Medium] The threaded IRQ handler accesses the hardware interrupt status register without verifying if the device is active. -- --- Patch [3]: [PATCH 3/3] iio: accel: icm42370: Add FIFO buffer functionality --- commit 2096dc9aae100b3082c95d260e2d65e0fdf275c9 Author: Kanak Shilledar <kanak.shilledar@axis.com> Date: Thu Aug 6 12:46:29 2026 +0000 iio: accel: icm42370: Add FIFO buffer functionality Add support for the hardware FIFO buffer to support high data rate capture. This includes necessary IIO buffersetup ops, watermark management and FIFO packet decoding. The accelerometer has 2.25kB FIFO size. The buffer can be handled via sysfs. > diff --git a/drivers/iio/accel/inv_icm42370_buffer.c b/drivers/iio/accel/inv_icm42370_buffer.c > index 0000000000000..a39b8aa4dd9bf > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370_buffer.c [ ... ] > +void inv_icm42370_buffer_update_fifo_period(struct inv_icm42370_data *st) > +{ > + u32 period_accel; > + > + if (st->fifo.en & INV_ICM42370_SENSOR_ACCEL) > + period_accel = inv_icm42370_odr_to_period(st->conf.odr); > + else > + period_accel = U32_MAX; > + > + st->fifo.period = period_accel; > +} [Severity: Medium] When a user changes the ODR sysfs property while the buffer is enabled, inv_icm42370_accel_write_odr() handles the change but it does not appear to update st->fifo.period. Could this stale st->fifo.period cache cause the FIFO parser to generate incorrect timestamps? [ ... ] > +int inv_icm42370_buffer_update_watermark(struct inv_icm42370_data *st) > +{ > + size_t packet_size, wm_size; > + unsigned int wm_accel, watermark; > + bool restore; > + __le16 raw_wm; > + int ret; > + > + packet_size = inv_icm42370_get_packet_size(st->fifo.en); > + > + /* compute sensors latency, depending on sensor watermark and odr */ > + wm_accel = > + inv_icm42370_wm_truncate(st->fifo.watermark.accel, packet_size); > + > + /* 0 value for watermark means that the sensor is turned off */ > + if (wm_accel == 0) > + return 0; > + > + watermark = wm_accel; > + st->fifo.watermark.eff_accel = wm_accel; > + > + /* compute watermark value in bytes */ > + wm_size = watermark * packet_size; > + > + /* changing FIFO watermark requires to turn off watermark interrupt */ > + ret = regmap_update_bits_check( > + st->map, INV_ICM42370_REG_INT_SOURCE0, > + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN, 0, &restore); > + if (ret) > + return ret; > + > + raw_wm = INV_ICM42370_FIFO_WATERMARK_VAL(wm_size); > + memcpy(st->buffer, &raw_wm, sizeof(raw_wm)); > + ret = regmap_bulk_write(st->map, INV_ICM42370_REG_FIFO_WATERMARK, > + st->buffer, sizeof(raw_wm)); > + if (ret) > + return ret; [Severity: Medium] If this regmap_bulk_write() fails, we return early and bypass the restoration of the watermark interrupt. Does this permanently disable the watermark interrupt on a transient I2C/SPI bus error, breaking future data capture? > + > + /* restore watermark interrupt */ > + if (restore) { > + ret = regmap_set_bits( > + st->map, INV_ICM42370_REG_INT_SOURCE0, > + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int inv_icm42370_buffer_preenable(struct iio_dev *indio_dev) > +{ > + struct inv_icm42370_data *st = iio_priv(indio_dev); > + struct device *dev = regmap_get_device(st->map); > + struct inv_icm42370_sensor_state *sensor_st = st->sensor_state; > + struct inv_sensors_timestamp *ts = &sensor_st->ts; > + > + pm_runtime_get_sync(dev); [Severity: High] Is it intentional to ignore the return value of pm_runtime_get_sync() here? A failure to wake the device might lead to bus timeouts or crashes when postenable attempts to write to hardware registers. It also leaves the PM usage counter incremented on failure, causing a reference leak. Could we use pm_runtime_resume_and_get() instead and propagate the error? > + > + guard(mutex) > + (&st->lock); > + inv_sensors_timestamp_reset(ts); > + > + return 0; > +} [ ... ] > +static int inv_icm42370_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct inv_icm42370_data *st = iio_priv(indio_dev); > + int ret; > + > + guard(mutex) > + (&st->lock); > + > + if (st->fifo.on > 1) { > + st->fifo.on--; > + return 0; > + } > + > + /* set FIFO in bypass mode */ > + ret = regmap_write(st->map, INV_ICM42370_REG_FIFO_CONFIG1, > + INV_ICM42370_FIFO_CONFIG_BYPASS); > + if (ret) > + return ret; > + > + /* flush FIFO data */ > + ret = regmap_write(st->map, INV_ICM42370_REG_SIGNAL_PATH_RESET, > + INV_ICM42370_SIGNAL_PATH_RESET_FIFO_FLUSH); > + if (ret) > + return ret; > + > + /* disable FIFO threshold interrupt */ > + ret = regmap_clear_bits(st->map, INV_ICM42370_REG_INT_SOURCE0, > + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN); > + if (ret) > + return ret; [Severity: Medium] If any hardware writes fail in inv_icm42370_buffer_predisable(), we return early and never decrement st->fifo.on. Does this permanently break subsequent buffer enablements, as postenable will see st->fifo.on > 0 and skip hardware initialization? > + > + st->fifo.on--; > + > + return 0; > +} > + > +static int inv_icm42370_buffer_postdisable(struct iio_dev *indio_dev) > +{ > + struct inv_icm42370_data *st = iio_priv(indio_dev); > + struct inv_icm42370_sensor_state *sensor_st = st->sensor_state; > + struct inv_sensors_timestamp *ts = &sensor_st->ts; > + struct device *dev = regmap_get_device(st->map); > + unsigned int sensor; > + unsigned int *watermark; > + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; > + unsigned int sleep_temp = 0; > + unsigned int sleep_sensor = 0; > + unsigned int sleep; > + int ret; > + > + if (indio_dev == st->indio_accel) { > + sensor = INV_ICM42370_SENSOR_ACCEL; > + watermark = &st->fifo.watermark.accel; > + } else { > + return -EINVAL; > + } > + > + mutex_lock(&st->lock); > + > + inv_sensors_timestamp_apply_odr(ts, 0, 0, 0); > + > + ret = inv_icm42370_buffer_set_fifo_en(st, st->fifo.en & ~sensor); > + if (ret) > + goto out_unlock; > + > + *watermark = 0; > + ret = inv_icm42370_buffer_update_watermark(st); [Severity: Medium] By setting *watermark = 0 here, does the driver incorrectly erase the user's sysfs-configured watermark? When the buffer is later re-enabled, it seems the watermark would remain 0. > + if (ret) > + goto out_unlock; [ ... ] > +int inv_icm42370_buffer_fifo_read(struct inv_icm42370_data *st, > + unsigned int max) > +{ > + size_t max_count; > + __be16 *raw_fifo_count; > + ssize_t i, size; > + const void *accel, *timestamp; > + const s8 *temp; > + unsigned int odr; > + int ret; > + > + /* reset all samples counters */ > + st->fifo.count = 0; > + st->fifo.nb.accel = 0; > + st->fifo.nb.total = 0; > + > + /* compute maximum FIFO read size */ > + if (max == 0) > + max_count = sizeof(st->fifo.data); > + else > + max_count = max * inv_icm42370_get_packet_size(st->fifo.en); [Severity: Critical] When max is greater than 0, does this else branch fail to clamp max_count against sizeof(st->fifo.data)? If userspace triggers a hardware FIFO flush with a large count, max_count could be computed to a value greater than the fixed size of st->fifo.data (2080 bytes), allowing up to the hardware FIFO size (2304 bytes) to be read blindly into the heap array. > + > + /* read FIFO count value */ > + raw_fifo_count = (__be16 *)st->buffer; > + ret = regmap_bulk_read(st->map, INV_ICM42370_REG_FIFO_COUNT, > + raw_fifo_count, sizeof(*raw_fifo_count)); > + if (ret) > + return ret; > + st->fifo.count = be16_to_cpup(raw_fifo_count); > + > + /* check and clamp FIFO count value */ > + if (st->fifo.count == 0) > + return 0; > + if (st->fifo.count > max_count) > + st->fifo.count = max_count; > + > + /* read all FIFO data in internal buffer */ > + ret = inv_icm42370_fifo_read_data(st, st->fifo.count); > + if (ret) > + return ret; > + > + /* compute number of samples for each sensor */ > + for (i = 0; i < st->fifo.count; i += size) { [Severity: High] Does this loop read out-of-bounds data if the hardware reports a partial packet size? Because it relies on i < st->fifo.count instead of ensuring i + packet_size <= st->fifo.count, inv_icm42370_fifo_decode_packet might read past the valid bytes returned by the hardware. > + size = inv_icm42370_fifo_decode_packet( > + &st->fifo.data[i], &accel, &temp, ×tamp, &odr); > + if (size <= 0) > + break; > + if (accel != NULL && inv_icm42370_fifo_is_data_valid(accel)) > + st->fifo.nb.accel++; > + st->fifo.nb.total++; > + } > + > + return 0; > +} [ ... ] > diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c > index 9f6c302e6f331..7107fc7a682dd 100644 > --- a/drivers/iio/accel/inv_icm42370_core.c > +++ b/drivers/iio/accel/inv_icm42370_core.c [ ... ] > @@ -424,6 +452,7 @@ static irqreturn_t inv_icm42370_irq_timestamp(int irq, void *_data) > static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data) > { > struct inv_icm42370_data *dev_data = _data; > + struct device *dev = regmap_get_device(dev_data->map); > unsigned int status; > int ret; > > @@ -433,6 +462,21 @@ static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data) > if (ret) > goto out_unlock; [Severity: Medium] This is a pre-existing issue, but should the IRQ handler use pm_runtime_get_if_active() before accessing the interrupt status register? If the interrupt fires spuriously or races with autosuspend, it could attempt to access a powered-down device. > > + if (status & INV_ICM42370_INT_STATUS_FIFO_FULL) > + dev_warn_ratelimited(dev, "FIFO full data lost!\n"); > + > + if (status & (INV_ICM42370_INT_STATUS_FIFO_THS | > + INV_ICM42370_INT_STATUS_FIFO_FULL)) { > + ret = inv_icm42370_buffer_fifo_read(dev_data, 0); > + if (ret) { > + dev_err_ratelimited(dev, "FIFO read error %d\n", ret); > + goto out_unlock; > + } > + ret = inv_icm42370_buffer_fifo_parse(dev_data); > + if (ret) > + dev_err_ratelimited(dev, "FIFO parsing error %d\n", ret); > + } > + > out_unlock: > mutex_unlock(&dev_data->lock); > return IRQ_HANDLED; [ ... ] > +/* enable accelerometer sensor and FIFO write */ > +static int inv_icm42370_accel_update_scan_mode(struct iio_dev *indio_dev, > + const unsigned long *scan_mask) > +{ > + struct inv_icm42370_data *st = iio_priv(indio_dev); > + struct inv_icm42370_sensor_state *accel_st = st->sensor_state; > + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; > + unsigned int fifo_en = 0; > + unsigned int sleep_temp = 0; > + unsigned int sleep_accel = 0; > + unsigned int sleep; > + int ret; > + > + mutex_lock(&st->lock); > + > + if (*scan_mask & INV_ICM42370_SCAN_MASK_ACCEL_3AXIS) { [Severity: Medium] Is there a missing check for the temperature channel? Userspace might enable only the temperature channel in the scan mask, but this check only looks for the accelerometer. In that case, the buffer would start but the sensor would remain off. > + /* enable accel sensor */ > + conf.mode = st->conf.mode; > + conf.filter = accel_st->filter; > + ret = inv_icm42370_set_accel_conf(st, &conf, &sleep_accel); [Severity: High] This is a pre-existing issue, but inv_icm42370_set_accel_conf() appears to ignore the requested sensor mode. It receives the new mode via conf->mode but never assigns it to dev_data->conf.mode, and passes the unmodified old mode to inv_icm42370_set_pwr_mgmt0(). When called here (or in postdisable), does this prevent the sensor from actually changing power state? > + if (ret) > + goto out_unlock; > + fifo_en |= INV_ICM42370_SENSOR_ACCEL; > + } > + > + /* update data FIFO write */ > + ret = inv_icm42370_buffer_set_fifo_en(st, fifo_en | st->fifo.en); [ ... ] > +static int inv_icm42370_accel_hwfifo_set_watermark(struct iio_dev *indio_dev, > + unsigned int val) > +{ > + struct inv_icm42370_data *st = iio_priv(indio_dev); > + int ret; > + > + mutex_lock(&st->lock); > + > + st->fifo.watermark.accel = val; > + ret = inv_icm42370_buffer_update_watermark(st); [Severity: Medium] Does inv_icm42370_buffer_update_watermark() access hardware registers without ensuring the device is active? If userspace writes to this sysfs attribute while the buffer is disabled and the device is powered off, this might lead to a bus timeout. Should pm_runtime_resume_and_get() be used here? > + > + mutex_unlock(&st->lock); > + > + return ret; > +} > + > +static int inv_icm42370_accel_hwfifo_flush(struct iio_dev *indio_dev, > + unsigned int count) > +{ > + struct inv_icm42370_data *st = iio_priv(indio_dev); > + int ret; > + > + if (count == 0) > + return 0; > + > + mutex_lock(&st->lock); > + > + ret = inv_icm42370_buffer_hwfifo_flush(st, count); [Severity: Medium] Similarly, does inv_icm42370_buffer_hwfifo_flush() access hardware registers without PM runtime protection? > + if (!ret) > + ret = st->fifo.nb.accel; > + > + mutex_unlock(&st->lock); > + > + return ret; > +} [ ... ] > @@ -1143,10 +1286,15 @@ struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, > > indio_dev->name = "inv_icm42370"; > indio_dev->info = &inv_icm42370_info; > - indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_ALL_BUFFER_MODES; > indio_dev->channels = inv_icm42370_accel_channels; > indio_dev->num_channels = ARRAY_SIZE(inv_icm42370_accel_channels); [Severity: Medium] With buffer modes added, should IIO_CHAN_SOFT_TIMESTAMP be included in the inv_icm42370_accel_channels array? Without it, userspace cannot enable the timestamp channel in the scan mask. > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, > + &inv_icm42370_buffer_ops); > + if (ret) > + return ERR_PTR(ret); > + > ret = devm_iio_device_register(dev, indio_dev); > if (ret) > return ERR_PTR(ret); [ ... ] > @@ -1222,6 +1417,14 @@ int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq, > if (ret) > return dev_err_probe(dev, ret, "Setup failed\n"); > > + ret = inv_icm42370_timestamp_setup(data); > + if (ret) > + return ret; > + > + ret = inv_icm42370_buffer_init(data); > + if (ret) > + return ret; > + > data->indio_accel = inv_icm42370_accel_init(indio_dev, data); > if (IS_ERR(data->indio_accel)) > return PTR_ERR(data->indio_accel); [Severity: High] This isn't a bug introduced by this patch, but registering the IIO device before PM runtime and IRQ initialization exposes the device prematurely. Furthermore, because devres unwinds in LIFO order, PM runtime is disabled before the IIO device is unregistered. Does this force the buffer teardown (postdisable) to run with PM runtime already disabled during driver removal? Also, as a pre-existing issue, the devres action inv_icm42370_disable_pm registered at the end of probe calls pm_runtime_put_sync(). Since the probe function balances the counter to 0 with a pm_runtime_put() before returning, does this second put on driver removal underflow the PM usage counter below zero? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260806-b4-inv_icm42370p-v1-0-670837f5842f@axis.com?part=3 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-08-07 14:13 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-06 12:46 [PATCH 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar 2026-08-06 12:46 ` [PATCH 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar 2026-08-06 12:53 ` sashiko-bot 2026-08-07 10:16 ` Joshua Crofts 2026-08-07 13:15 ` Kanak Shilledar 2026-08-06 12:46 ` [PATCH 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar 2026-08-06 13:02 ` sashiko-bot 2026-08-07 6:51 ` Uwe Kleine-König 2026-08-07 13:11 ` Kanak Shilledar 2026-08-07 10:09 ` Joshua Crofts 2026-08-07 13:41 ` Kanak Shilledar 2026-08-07 14:13 ` Joshua Crofts 2026-08-06 12:46 ` [PATCH 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar 2026-08-06 13:03 ` sashiko-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox