* [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer
@ 2026-08-13 12:26 Kanak Shilledar
2026-08-13 12:26 ` [PATCH v2 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Kanak Shilledar @ 2026-08-13 12:26 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.
Thanks to the reviewers for all the time you spent going through the
code and the comments. The changes for v2 are quite a lot and I hope
that every important change is documented in the changelog. After
implementing the suggested changes, the driver is in a far better state
than v1.
[1] https://uat.invensense.com/en-us/products/3-axis/icm-42370-p
Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com>
---
Changes in v2:
* Changes across all files
- Update MAINTAINERS with company mailing list
- Sort/Cleanup of includes
- Use `guard(mutex)` and newer `pm_runtime` APIs
- Fix code formatting and add empty lines
- Be consistent in inv_icm42370_data variable name
- Fix MODULE_DESCRIPTION
- Drop secondary state struct and merge it's properties in
`inv_icm42370_data` struct
- Update mreg_read/write function calls
- Change the compatible and filename to `icm42370p`
* Changes to dt-binding
- Add dependencies property
- Made vdd and vddio supply as required
- Add description to drive-open-drain property
- Add mount-matrix property
- Add interrupt-names property
* Changes to `inv_icm42370.h` and `inv_icm42370_buffer.h`:
- Resturcture the file according to @Marcelo's advice
- Move struct __aligned properties to the end
* Changes to `inv_icm42370_core.c`:
- Fix _accel_scale[] values
- Add IIO_TIMESTAMP to channel spec
- Update mreg_read/write to fix bank access
- Replace usleep_range() with fsleep()
- Use constants from linux/units.h
- Call `_update_fifo_period()` after updating the ODR values
- Fix mathematical error in offset calculation
- Implement handling of mount matrix
- Implement handling of named interrupts
- Use devm_regulator_get_enable for the vdd/vddio regulators
- Use better error handling
- Move iio device registration after performing IRQ init
* Changes to `inv_icm42370_i2c.c`
- Change compatible string as per the binding
- Use named identifiers
- Add `id_table` to the i2c_driver struct
* Changes to `inv_icm42370_buffer.c`
- Update FIFO enable/disable logic
- Update FIFO buffer to match the specification and handle increased
size dynamically.
- Link to v1: https://patch.msgid.link/20260806-b4-inv_icm42370p-v1-0-670837f5842f@axis.com
To: Kanak Shilledar <kanak.shilledar@axis.com>
To: Henrik Grimler <henrik.grimler@axis.com>
To: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
To: Nuno Sá <nuno.sa@analog.com>
To: Andy Shevchenko <andy@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: kernel@axis.com
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
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,icm42370p.yaml | 84 ++
MAINTAINERS | 9 +
drivers/iio/accel/Kconfig | 18 +
drivers/iio/accel/Makefile | 6 +
drivers/iio/accel/inv_icm42370.h | 352 +++++
drivers/iio/accel/inv_icm42370_buffer.c | 496 +++++++
drivers/iio/accel/inv_icm42370_buffer.h | 100 ++
drivers/iio/accel/inv_icm42370_core.c | 1502 ++++++++++++++++++++
drivers/iio/accel/inv_icm42370_i2c.c | 93 ++
9 files changed, 2660 insertions(+)
---
base-commit: 350d1fb9204b13c5f95e511e98b8bcb47574d425
change-id: 20260629-b4-inv_icm42370p-ccd671066bcf
Best regards,
--
Kanak Shilledar <kanak.shilledar@axis.com>
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer 2026-08-13 12:26 [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar @ 2026-08-13 12:26 ` Kanak Shilledar 2026-08-13 12:34 ` sashiko-bot 2026-08-13 12:26 ` [PATCH v2 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Kanak Shilledar @ 2026-08-13 12:26 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,icm42370p.yaml | 84 ++++++++++++++++++++++ MAINTAINERS | 8 +++ 2 files changed, 92 insertions(+) diff --git a/Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.yaml b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.yaml new file mode 100644 index 0000000000000..df23406921a51 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.yaml @@ -0,0 +1,84 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/accel/invensense,icm42370p.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: InvenSense ICM-42370-P 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://www.invensense.tdk.com/en-us/products/3-axis/icm-42370-p + +properties: + compatible: + const: invensense,icm42370p + + reg: + maxItems: 1 + + interrupts: + minItems: 1 + maxItems: 2 + + interrupt-names: + minItems: 1 + maxItems: 2 + items: + enum: + - INT1 + - INT2 + + drive-open-drain: + type: boolean + description: + Whether irq is in open-drain mode. False means push-pull mode. + + mount-matrix: true + + vdd-supply: + description: Regulator operating range between 1.71V to 3.6V. + + vddio-supply: + description: Regulator operating range between 1.71V to 3.6V. + +dependencies: + interrupt-names: [ interrupts ] + +required: + - compatible + - reg + - interrupts + - vdd-supply + - vddio-supply + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + accelerometer@69 { + compatible = "invensense,icm42370p"; + reg = <0x69>; + interrupt-parent = <&gpio1>; + interrupts = <7 IRQ_TYPE_EDGE_FALLING>; + interrupt-names = "INT1"; + vdd-supply = <&vdd>; + vddio-supply = <&vddio>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 04fa5322d9f74..e4764489ea971 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-42370-P ACCELEROMETER +M: Kanak Shilledar <kanak.shilledar@axis.com> +M: Henrik Grimler <henrik.grimler@axis.com> +L: kernel@axis.com +S: Maintained +W: https://invensense.tdk.com/ +F: Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.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] 11+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer 2026-08-13 12:26 ` [PATCH v2 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar @ 2026-08-13 12:34 ` sashiko-bot 0 siblings, 0 replies; 11+ messages in thread From: sashiko-bot @ 2026-08-13 12:34 UTC (permalink / raw) To: Kanak Shilledar; +Cc: conor+dt, robh, devicetree Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] Incomplete device tree binding schema rejects valid hardware configurations for SPI, I3C, and reset lines. - [Low] Missing IIO subsystem mailing list in MAINTAINERS entry. - [Low] Contradictory schema constraints for the `interrupts` property. -- --- Patch [1]: [PATCH v2 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer --- commit 580d5dd4de723a5224325d048b11cddaf0d9a2b1 Author: Kanak Shilledar <kanak.shilledar@axis.com> dt-bindings: Add InvenSense ICM-42370-p accelerometer This commit adds device tree binding documentation for the InvenSense ICM-42370-P 3-axis accelerometer. The device supports I2C, SPI, and I3C, but this initial binding is geared towards I2C. The commit updates the MAINTAINERS file with the new binding. > diff --git a/Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.yaml b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.yaml > new file mode 100644 > index 0000000000000..df23406921a51 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.yaml [ ... ] > +dependencies: > + interrupt-names: [ interrupts ] > + > +required: > + - compatible > + - reg > + - interrupts [Severity: Low] Does this schema unnecessarily enforce interrupts as a strict requirement? Since interrupts is listed in the required array, the dependency check for interrupt-names: [ interrupts ] above is dead logic. Also, since IIO sensors typically support polling, making interrupts strictly mandatory might prevent valid polling-based hardware designs from passing validation. > + - vdd-supply > + - vddio-supply > + > +unevaluatedProperties: false [Severity: Medium] Will this strict constraint actively reject valid hardware descriptions for SPI, I3C, or reset lines? The commit message mentions support for SPI, I3C, and resets, but since standard SPI/I3C peripheral property $ref inclusions and the resets property are missing, unevaluatedProperties: false will cause validation to fail when standard properties like spi-max-frequency or resets are provided in the device tree. [ ... ] > diff --git a/MAINTAINERS b/MAINTAINERS > index 04fa5322d9f74..e4764489ea971 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS [ ... ] > +INVENSENSE ICM-42370-P ACCELEROMETER > +M: Kanak Shilledar <kanak.shilledar@axis.com> > +M: Henrik Grimler <henrik.grimler@axis.com> > +L: kernel@axis.com [Severity: Low] Should this MAINTAINERS entry include the standard IIO subsystem mailing list? Currently, it only lists a corporate mailing list, omitting linux-iio@vger.kernel.org which is typically mandatory for IIO entries. This means patches modifying this binding will not be automatically CC'd to the subsystem list by get_maintainer.pl. > +S: Maintained > +W: https://invensense.tdk.com/ > +F: Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.yaml -- Sashiko AI review · https://sashiko.dev/#/patchset/20260813-b4-inv_icm42370p-v2-0-11aedfdf76d3@axis.com?part=1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] iio: accel: Add support for ICM42370P 2026-08-13 12:26 [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar 2026-08-13 12:26 ` [PATCH v2 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar @ 2026-08-13 12:26 ` Kanak Shilledar 2026-08-13 12:41 ` sashiko-bot 2026-08-16 2:59 ` Jonathan Cameron 2026-08-13 12:26 ` [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar 2026-08-17 7:19 ` [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer Andy Shevchenko 3 siblings, 2 replies; 11+ messages in thread From: Kanak Shilledar @ 2026-08-13 12:26 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 | 346 +++++++++ drivers/iio/accel/inv_icm42370_core.c | 1298 +++++++++++++++++++++++++++++++++ drivers/iio/accel/inv_icm42370_i2c.c | 93 +++ 6 files changed, 1760 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index e4764489ea971..3aa023b645689 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13568,6 +13568,7 @@ L: kernel@axis.com S: Maintained W: https://invensense.tdk.com/ F: Documentation/devicetree/bindings/iio/accel/invensense,icm42370p.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..3ac2534c1c76d 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-P I2C driver" + depends on I2C + select INV_ICM42370 + select REGMAP_I2C + help + This driver supports the InvenSense ICM-42730-P 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..a4a822f04355f --- /dev/null +++ b/drivers/iio/accel/inv_icm42370.h @@ -0,0 +1,346 @@ +/* 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/bitfield.h> +#include <linux/bits.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/types.h> +#include <linux/regulator/consumer.h> + +#include <linux/iio/common/inv_sensors_timestamp.h> +#include <linux/iio/iio.h> + + +#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(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 \ + 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_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_WHOAMI_VALUE 0x0D +#define INV_ICM42370_ACCEL_MODE_LN 0x03 +#define INV_ICM42370_DATA_INVALID -32768 +#define INV_ICM42370_ACCEL_STARTUP_TIME_MS 10 + +#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, _ext_info) \ + { \ + .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, \ + }, \ + .ext_info = _ext_info, \ + } + +static const int inv_icm42370_accel_odr[] = { + 1, 562500, + 3, 125000, + 6, 250000, + 12, 500000, + 25, 0, + 50, 0, + 100, 0, + 200, 0, + 400, 0, + 800, 0, + 1600, 0, +}; + +enum inv_icm42370_chip { + INV_CHIP_INVALID, + INV_CHIP_ICM42370, + INV_CHIP_NB, +}; + +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, +}; + +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, +}; + +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, +}; + +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, +}; + +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, +}; + +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. + * @indio_accel: accelerometer IIO device. + * @timestamp: interrupt timestamp. + * @orientation: sensor chip orientation relative to main hardware. + * @chip: chip identifier. + * @conf: chip sensors configurations. + * @filter: sensor filter. + * @scales: table of scales. + * @scales_len: length (nb of items) of the scales table. + * @power_mode: sensor requested power mode (for common frequencies). + * @accel_calibbias: accelerometer calibration bias for X, Y, and Z axes. + * @ts: timestamp module states. + * @buffer: buffer for reading data registers, aligned for DMA. + */ +struct inv_icm42370_data { + struct mutex lock; + const char *name; + struct regmap *map; + struct iio_dev *indio_accel; + s64 timestamp; + struct iio_mount_matrix orientation; + enum inv_icm42370_chip chip; + struct inv_icm42370_conf conf; + enum inv_icm42370_filter filter; + const int *scales; + size_t scales_len; + enum inv_icm42370_sensor_mode power_mode; + s16 accel_calibbias[3]; + struct inv_sensors_timestamp ts; + u8 buffer[2] __aligned(IIO_DMA_MINALIGN); +}; + +static 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, +}; + +typedef int (*inv_icm42370_bus_setup)(struct inv_icm42370_data *); +extern const struct regmap_config inv_icm42370_regmap_config; + +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 *data, + struct inv_icm42370_conf *conf, + unsigned int *sleep_ms); + +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev); +int inv_icm42370_mreg_write(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 val); +int inv_icm42370_mreg_read(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 *val); +int inv_icm42370_set_pwr_mgmt0(struct inv_icm42370_data *data, + enum inv_icm42370_sensor_mode accel, + unsigned int *sleep_ms); + +#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..6266362e83f6a --- /dev/null +++ b/drivers/iio/accel/inv_icm42370_core.c @@ -0,0 +1,1298 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 Invensense, Inc. + * Copyright (C) 2026 Axis Communications AB + */ + +#include <linux/array_size.h> +#include <linux/bitops.h> +#include <linux/cleanup.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/irq.h> +#include <linux/linkage.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/units.h> + +#include <linux/iio/common/inv_sensors_timestamp.h> +#include <linux/iio/iio.h> + +#include "inv_icm42370.h" + +#define INV_ICM42370_SUSPEND_DELAY_MS 2000 + +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 iio_mount_matrix * +inv_icm42370_get_mount_matrix(const struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + const struct inv_icm42370_data *data = iio_priv(indio_dev); + + return &data->orientation; +} + +static const struct iio_chan_spec_ext_info inv_icm42370_accel_ext_infos[] = { + IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, inv_icm42370_get_mount_matrix), + { } +}; + +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_ext_infos), + INV_ICM42370_ACCEL_CHAN(IIO_MOD_Y, INV_ICM42370_ACCEL_SCAN_Y, + inv_icm42370_accel_ext_infos), + INV_ICM42370_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42370_ACCEL_SCAN_Z, + inv_icm42370_accel_ext_infos), + 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] = 4788403, + /* +/- 8G => 2*8*9.80665 / (2**15) m/s-2 */ + [2 * INV_ICM42370_ACCEL_FS_8G] = 0, + [2 * INV_ICM42370_ACCEL_FS_8G + 1] = 2394202, + /* +/- 4G => 2*4*9.80665 / (2**15) m/s-2 */ + [2 * INV_ICM42370_ACCEL_FS_4G] = 0, + [2 * INV_ICM42370_ACCEL_FS_4G + 1] = 1197101, + /* +/- 2G => 2*2*9.80665 / (2**15) m/s-2 */ + [2 * INV_ICM42370_ACCEL_FS_2G] = 0, + [2 * INV_ICM42370_ACCEL_FS_2G + 1] = 598550, +}; + +/** + * 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] = { + 0, 0, 0, 0, 0, /* Reserved */ + 625000, /* 1.6kHz */ + 1250000, /* 800Hz */ + 2500000, /* 400Hz */ + 5000000, /* 200Hz */ + 10000000, /* 100Hz */ + 20000000, /* 50Hz */ + 40000000, /* 25Hz */ + 80000000, /* 12.5Hz */ + 160000000, /* 6.25Hz */ + 320000000, /* 3.125Hz */ + 640000000, /* 1.5625Hz */ + }; + + 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 + * + * @data: pointer to struct containing the sensor data + * @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 + */ +int inv_icm42370_mreg_write(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 val) +{ + int ret; + unsigned int sleep; + + /* set default conf to ensure mreg access */ + ret = inv_icm42370_set_accel_conf(data, &inv_icm42370_default_conf, &sleep); + if (ret) + return ret; + + ret = inv_icm42370_mreg_check(data->map); + if (ret) + return ret; + + ret = regmap_write(data->map, INV_ICM42370_REG_BLK_SEL_W, bank); + if (ret) + return -EINVAL; + + ret = regmap_write(data->map, INV_ICM42370_REG_MADDR_W, addr); + if (ret) + return -EINVAL; + + ret = regmap_write(data->map, INV_ICM42370_REG_M_W, val); + if (ret) + return -EINVAL; + + fsleep(10); + return regmap_write(data->map, INV_ICM42370_REG_BLK_SEL_W, 0x00); +} + +/** + * inv_icm42370_mreg_read() - routine for reading from other bank registers + * + * @data: pointer to struct containing the sensor data + * @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 + */ +int inv_icm42370_mreg_read(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 *val) +{ + int ret; + unsigned int sleep, read_val; + + /* set default conf to ensure mreg access */ + ret = inv_icm42370_set_accel_conf(data, &inv_icm42370_default_conf, &sleep); + if (ret) + return ret; + + ret = inv_icm42370_mreg_check(data->map); + if (ret) + return ret; + + ret = regmap_write(data->map, INV_ICM42370_REG_BLK_SEL_R, bank); + if (ret) + return -EINVAL; + + ret = regmap_write(data->map, INV_ICM42370_REG_MADDR_R, addr); + if (ret) + return -EINVAL; + + fsleep(10); + ret = regmap_read(data->map, INV_ICM42370_REG_M_R, &read_val); + if (ret) + return -EINVAL; + + fsleep(10); + *val = (u8)read_val; + + return regmap_write(data->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); + + data->conf = *conf; + + return 0; +} + +/** + * inv_icm42370_set_pwr_mgmt0() - set the PWR_MGMT0 register for sensor + * + * @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 + */ +int inv_icm42370_set_pwr_mgmt0(struct inv_icm42370_data *data, + enum inv_icm42370_sensor_mode accel, + unsigned int *sleep_ms) +{ + enum inv_icm42370_sensor_mode oldaccel = 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(data->map, INV_ICM42370_REG_PWR_MGMT0, val); + if (ret) + return ret; + + data->conf.mode = accel; + data->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 */ + fsleep(100); + 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; +} + +/** + * inv_icm42370_set_accel_conf() - set configuration data for accelerometer + * + * @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 *data, + struct inv_icm42370_conf *conf, + unsigned int *sleep_ms) +{ + struct inv_icm42370_conf *oldconf = &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(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(data, conf->mode, sleep_ms); +} + +/** + * inv_icm42370_setup() - check and setup chip + * + * @data: pointer to struct containing the sensor data + * @bus_setup: callback to configure bus-specific settings (e.g. I2C) + * + * 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 *data = _data; + + data->timestamp = iio_get_time_ns(data->indio_accel); + + return IRQ_WAKE_THREAD; +} + +static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data) +{ + struct inv_icm42370_data *data = _data; + unsigned int status; + int ret; + + guard(mutex)(&data->lock); + + ret = regmap_read(data->map, INV_ICM42370_REG_INT_STATUS, &status); + if (ret) + return IRQ_HANDLED; + + 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, 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, 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 + * + * @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 *data, s16 *temp) +{ + struct device *dev = regmap_get_device(data->map); + __be16 *raw; + int ret; + + PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); + if (ret) + return ret; + + guard(mutex)(&data->lock); + + raw = (__be16 *)&data->buffer[0]; + ret = regmap_bulk_read(data->map, INV_ICM42370_REG_TEMP_DATA1, raw, + sizeof(*raw)); + if (ret) + return ret; + + *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; + + return 0; +} + +/** + * 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 *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(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 + * + * @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 *data, + struct iio_chan_spec const *chan, + int *val, int *val2) +{ + struct device *dev = regmap_get_device(data->map); + s64 val64; + s32 bias; + unsigned int reg; + s16 offset; + u8 buffer_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_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); + if (ret) + return ret; + + guard(mutex)(&data->lock); + + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, reg, + &data->buffer[0]); + if (ret) + return ret; + + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, reg + 1, + &data->buffer[1]); + if (ret) + return ret; + + memcpy(buffer_data, data->buffer, sizeof(buffer_data)); + + /* 12 bits signed value */ + switch (chan->channel2) { + case IIO_MOD_X: + offset = sign_extend32(((buffer_data[0] & 0xF0) << 4) | buffer_data[1], 11); + break; + case IIO_MOD_Y: + offset = sign_extend32(((buffer_data[1] & 0x0F) << 8) | buffer_data[0], 11); + break; + case IIO_MOD_Z: + offset = sign_extend32(((buffer_data[0] & 0xF0) << 4) | buffer_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 / (long)MEGA; + *val2 = bias % (long)MEGA; + + return IIO_VAL_INT_PLUS_MICRO; +} + +/** + * inv_icm42370_accel_write_offset() - write offset values to the accelerometer + * + * @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 *data, + struct iio_chan_spec const *chan, + int val, int val2) +{ + struct device *dev = regmap_get_device(data->map); + s64 val64; + s32 min, max; + u8 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] * (long)MEGA + + inv_icm42370_accel_calibbias[1]; + max = inv_icm42370_accel_calibbias[4] * (long)MEGA + + inv_icm42370_accel_calibbias[5]; + + val64 = (s64)val * (s64)MEGA; + if (val >= 0) + val64 += (s64)val2; + else + val64 -= (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_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); + if (ret) + return ret; + + guard(mutex)(&data->lock); + + switch (chan->channel2) { + case IIO_MOD_X: + /* OFFSET_USER4 register is shared */ + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER4, + ®val); + if (ret) + return ret; + + data->buffer[0] = ((offset & 0xF00) >> 4) | (regval & 0x0F); + data->buffer[1] = offset & 0xFF; + + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER4, + data->buffer[0]); + if (ret) + return ret; + + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER5, + data->buffer[1]); + + if (ret) + return ret; + break; + + case IIO_MOD_Y: + /* OFFSET_USER7 register is shared */ + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER7, + ®val); + if (ret) + return ret; + + data->buffer[0] = offset & 0xFF; + data->buffer[1] = ((offset & 0xF00) >> 8) | (regval & 0xF0); + + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER7, + data->buffer[1]); + + if (ret) + return ret; + + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER6, + data->buffer[0]); + if (ret) + return ret; + + break; + + case IIO_MOD_Z: + /* OFFSET_USER7 register is shared */ + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER7, + ®val); + if (ret) + return ret; + + data->buffer[0] = ((offset & 0xF00) >> 4) | (regval & 0x0F); + data->buffer[1] = offset & 0xFF; + + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER7, + data->buffer[0]); + if (ret) + return ret; + + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_OFFSET_USER8, + data->buffer[1]); + if (ret) + return ret; + break; + + default: + return -EINVAL; + } + + return 0; +} + +/** + * 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 *data = iio_priv(indio_dev); + unsigned int idx; + + idx = data->conf.fs; + + *val = data->scales[2 * idx]; + *val2 = data->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 *data = iio_priv(indio_dev); + struct device *dev = regmap_get_device(data->map); + unsigned int idx; + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; + int ret; + + for (idx = 0; idx < data->scales_len; idx += 2) { + if (val == data->scales[idx] && + val2 == data->scales[idx + 1]) + break; + } + + if (idx >= data->scales_len) + return -EINVAL; + + conf.fs = idx / 2; + + PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); + if (ret) + return ret; + + guard(mutex)(&data->lock); + + ret = inv_icm42370_set_accel_conf(data, &conf, NULL); + + return ret; +} + +/** + * inv_icm42370_accel_read_odr() - read ODR data from the accelerometer + * + * @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 *data, + int *val, int *val2) +{ + unsigned int odr; + unsigned int i; + + odr = 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 *data = iio_priv(indio_dev); + struct inv_sensors_timestamp *ts = &data->ts; + struct device *dev = regmap_get_device(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_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); + if (ret) + return ret; + + guard(mutex)(&data->lock); + + ret = inv_sensors_timestamp_update_odr( + ts, inv_icm42370_odr_to_period(conf.odr), + iio_buffer_enabled(indio_dev)); + if (ret) + return ret; + + ret = inv_icm42370_set_accel_conf(data, &conf, NULL); + if (ret) + return ret; + + return 0; +} + +/** + * 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 *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(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 I/O 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); + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; + 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_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); + if (ret) + return ret; + + guard(mutex)(&data->lock); + + /* enable accel sensor */ + conf.mode = data->power_mode; + conf.filter = data->filter; + ret = inv_icm42370_set_accel_conf(data, &conf, NULL); + if (ret) + return ret; + + /* read accel register data */ + value = (__be16 *)&data->buffer[0]; + ret = regmap_bulk_read(data->map, reg, value, sizeof(*value)); + if (ret) + return ret; + + *val = (s16)be16_to_cpup(value); + + if (*val == INV_ICM42370_DATA_INVALID) + ret = -EINVAL; + + 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 *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(data, val, val2); + + case IIO_CHAN_INFO_CALIBBIAS: + return inv_icm42370_accel_read_offset(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 inv_sensors_timestamp_chip ts_chip; + + data->scales = inv_icm42370_accel_scale; + data->scales_len = ARRAY_SIZE(inv_icm42370_accel_scale); + data->filter = data->conf.filter; + data->power_mode = data->conf.mode; + + /* + * 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->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); + + 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 fwnode_handle *fwnode = dev_fwnode(dev); + struct inv_icm42370_data *data; + struct iio_dev *indio_dev; + int irq_type; + bool open_drain; + int ret; + + if (chip != INV_CHIP_ICM42370) { + dev_err(dev, "invalid chip = %d\n", chip); + return -ENODEV; + } + + /* get INT1 only supported interrupt or fallback to first interrupt */ + irq = fwnode_irq_get_byname(fwnode, "INT1"); + if (irq < 0 && irq != -EPROBE_DEFER) { + dev_info(dev, + "no INT1 interrupt defined, fallback to first interrupt\n"); + irq = fwnode_irq_get(fwnode, 0); + } + if (irq < 0) + return dev_err_probe(dev, irq, + "error missing INT1 interrupt\n"); + + irq_type = irq_get_trigger_type(irq); + 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); + + ret = devm_mutex_init(dev, &data->lock); + if (ret) + return dev_err_probe(dev, ret, "unable to initialize mutex\n"); + + data->chip = chip; + data->map = regmap; + + ret = devm_regulator_get_enable(dev, "vdd"); + if (ret) + return dev_err_probe(dev, ret, "failed to enable vdd regulator\n"); + + ret = devm_regulator_get_enable(dev, "vddio"); + if (ret) + return dev_err_probe(dev, ret, "failed to enable vddio regulator\n"); + + ret = inv_icm42370_setup(data, bus_setup); + if (ret) + return dev_err_probe(dev, ret, "setup failed\n"); + + ret = iio_read_mount_matrix(dev, &data->orientation); + if (ret) { + dev_err(dev, "failed to retrieve mounting matrix %d\n", 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); + + ret = inv_icm42370_irq_init(data, irq, irq_type, open_drain); + if (ret) + return ret; + + /* setup runtime power management */ + ret = devm_pm_runtime_set_active_enabled(dev); + if (ret) + return ret; + + pm_runtime_set_autosuspend_delay(dev, INV_ICM42370_SUSPEND_DELAY_MS); + pm_runtime_use_autosuspend(dev); + + ret = devm_iio_device_register(dev, indio_dev); + if (ret) + return dev_err_probe(dev, ret, "unable to register iio device\n"); + + return 0; +} +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 ICM-42370-P 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..c4533671051c4 --- /dev/null +++ b/drivers/iio/accel/inv_icm42370_i2c.c @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 InvenSense, Inc. + * Copyright (C) 2026 Axis Communications AB + */ + +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/regmap.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); +} + +static const struct i2c_device_id inv_icm42370_id[] = { + { .name = "icm42370p", .driver_data = INV_CHIP_ICM42370 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, inv_icm42370_id); + +static const struct of_device_id inv_icm42370_of_matches[] = { + { .compatible = "invensense,icm42370p", .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, + .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-42370-P I2C driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS("IIO_ICM42370"); -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] iio: accel: Add support for ICM42370P 2026-08-13 12:26 ` [PATCH v2 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar @ 2026-08-13 12:41 ` sashiko-bot 2026-08-16 2:59 ` Jonathan Cameron 1 sibling, 0 replies; 11+ messages in thread From: sashiko-bot @ 2026-08-13 12:41 UTC (permalink / raw) To: Kanak Shilledar; +Cc: robh, conor+dt, devicetree Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider: - [High] Accessing MREG registers mutates a global configuration struct and permanently overwrites the active device configuration without restoring it. - [High] The required hardware stabilization delay is bypassed, leading to premature MREG register access. - [High] ODR mapping arrays are inverted, misconfiguring sensor sampling frequencies. - [High] Runtime PM setup exists without implementing dev_pm_ops callbacks. - [High] Missing iio_device_claim_direct causes concurrency race on calibbias read. - [Medium] Incorrect negative minimum calculation in calibbias bounds check rejects valid offsets. - [Medium] Missing `.read_avail` callback breaks `_available` sysfs attributes. - [Medium] Unbalanced pm_runtime_use_autosuspend leaks device reference. - [Medium] MREG error paths fail to restore block selection register. - [Medium] IRQ handler accesses hardware registers without PM runtime guard. - [Medium] Endianness missing in IIO_TEMP scan_type definition (Speculative). - [Medium] Lockless reads of `data->conf` fields in sysfs read handlers. -- --- Patch [2]: [PATCH v2 2/3] iio: accel: Add support for ICM42370P --- commit 4a22a747a5f16a585b80aa50c0ea6c3944149d0e Author: Kanak Shilledar <kanak.shilledar@axis.com> Subject: iio: accel: Add support for ICM42370P This commit adds support for the Invensense ICM42370P MEMS MotionTracking accelerometer over I2C. It implements raw reads, basic sysfs calibration interfaces, and mandatory temperature sensor enablement. > diff --git a/drivers/iio/accel/inv_icm42370.h b/drivers/iio/accel/inv_icm42370.h > index 0000000000000..a4a822f04355f > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370.h [ ... ] > +#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, \ > + }, \ > + } [Severity: Medium] Does this missing endianness definition cause issues on little-endian architectures? The accelerometer channel specifies IIO_BE, but this temperature channel omits it. Since the sensor outputs big-endian data, userspace reading the temperature from the IIO buffer might decode mangled values. > diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c > index 0000000000000..6266362e83f6a > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370_core.c [ ... ] > +int inv_icm42370_mreg_write(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 val) > +{ > + int ret; > + unsigned int sleep; > + > + /* set default conf to ensure mreg access */ > + ret = inv_icm42370_set_accel_conf(data, &inv_icm42370_default_conf, &sleep); > + if (ret) > + return ret; > + > + ret = inv_icm42370_mreg_check(data->map); > + if (ret) > + return ret; > + > + ret = regmap_write(data->map, INV_ICM42370_REG_BLK_SEL_W, bank); > + if (ret) > + return -EINVAL; > + > + ret = regmap_write(data->map, INV_ICM42370_REG_MADDR_W, addr); > + if (ret) > + return -EINVAL; > + > + ret = regmap_write(data->map, INV_ICM42370_REG_M_W, val); > + if (ret) > + return -EINVAL; > + > + fsleep(10); > + return regmap_write(data->map, INV_ICM42370_REG_BLK_SEL_W, 0x00); > +} [Severity: Medium] If regmap_write fails at INV_ICM42370_REG_MADDR_W or INV_ICM42370_REG_M_W, the function returns immediately. Does this leave the hardware bank selection permanently altered since we never write 0x00 to INV_ICM42370_REG_BLK_SEL_W on the error paths? This might cause subsequent normal data reads to fetch garbage from the wrong bank. [ ... ] > +int inv_icm42370_mreg_read(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 *val) > +{ > + int ret; > + unsigned int sleep, read_val; > + > + /* set default conf to ensure mreg access */ > + ret = inv_icm42370_set_accel_conf(data, &inv_icm42370_default_conf, &sleep); > + if (ret) > + return ret; > + > + ret = inv_icm42370_mreg_check(data->map); [Severity: High] Is it intentional to pass the global inv_icm42370_default_conf struct directly to inv_icm42370_set_accel_conf()? Looking at inv_icm42370_set_accel_conf(), it mutates the passed configuration: oldconf->fs = conf->fs; oldconf->odr = conf->odr; This will permanently overwrite the active device configuration in data->conf and mutate the shared global struct. [Severity: High] Additionally, inv_icm42370_set_accel_conf() populates the sleep variable with the required hardware stabilization time, but the code proceeds directly to inv_icm42370_mreg_check() without actually sleeping. Can this cause hardware communication failures due to bypassing the startup delay? [ ... ] > +static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data) > +{ > + struct inv_icm42370_data *data = _data; > + unsigned int status; > + int ret; > + > + guard(mutex)(&data->lock); > + > + ret = regmap_read(data->map, INV_ICM42370_REG_INT_STATUS, &status); [Severity: Medium] Should this interrupt handler use pm_runtime_get_if_active() before accessing the hardware? If a spurious interrupt fires while the device is in a runtime-suspended state, attempting to read INV_ICM42370_REG_INT_STATUS without waking the hardware could lead to I2C bus transfer errors. [ ... ] > +static int inv_icm42370_accel_write_offset(struct inv_icm42370_data *data, > + struct iio_chan_spec const *chan, > + int val, int val2) > +{ > + struct device *dev = regmap_get_device(data->map); > + s64 val64; > + s32 min, max; > + u8 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] * (long)MEGA + > + inv_icm42370_accel_calibbias[1]; > + max = inv_icm42370_accel_calibbias[4] * (long)MEGA + > + inv_icm42370_accel_calibbias[5]; [Severity: Medium] Is the calculation for the negative minimum boundary correct here? Because inv_icm42370_accel_calibbias[0] is negative (-10), adding the fractional part (42010) instead of subtracting it raises the minimum floor (e.g., -9957990 instead of -10042010). Will this reject valid extreme negative calibration offsets? [ ... ] > +static int inv_icm42370_accel_read_scale(struct iio_dev *indio_dev, int *val, > + int *val2) > +{ > + struct inv_icm42370_data *data = iio_priv(indio_dev); > + unsigned int idx; > + > + idx = data->conf.fs; [Severity: Medium] Should data->lock be held when reading data->conf.fs here and data->conf.odr in inv_icm42370_accel_read_odr()? Concurrently writing to the scale or sampling frequency sysfs nodes might result in stale reads or data races. [ ... ] > +static int inv_icm42370_accel_write_odr(struct iio_dev *indio_dev, int val, > + int val2) > +{ > + struct inv_icm42370_data *data = iio_priv(indio_dev); > + struct inv_sensors_timestamp *ts = &data->ts; > + struct device *dev = regmap_get_device(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]; [Severity: High] Can this map the wrong frequency to the sensor? The inv_icm42370_accel_odr array defines frequencies in ascending order, but inv_icm42370_accel_odr_conv maps the hardware enums in descending order. Requesting 1.5625 Hz uses index 0, which appears to configure the hardware to 1.6 kHz. [ ... ] > +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 *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(data, val, val2); > + > + case IIO_CHAN_INFO_CALIBBIAS: > + return inv_icm42370_accel_read_offset(data, chan, val, val2); [Severity: High] Should iio_device_claim_direct() be called before reading the calibration bias? The MREG reads underneath inv_icm42370_accel_read_offset() silently alter the hardware's scale and ODR settings. If this happens while the IIO buffer is actively streaming, it could disrupt the active sampling state. [ ... ] > +static const struct iio_info inv_icm42370_info = { > + .read_raw = inv_icm42370_accel_read_raw, > + .write_raw = inv_icm42370_accel_write_raw, > +}; [Severity: Medium] Does this missing .read_avail callback break the _available sysfs attributes? The channel definitions set info_mask_shared_by_type_available and info_mask_shared_by_all_available, which causes the IIO core to expose those sysfs entries. Without a callback, reads will unconditionally return -EINVAL. [ ... ] > + /* setup runtime power management */ > + ret = devm_pm_runtime_set_active_enabled(dev); > + if (ret) > + return ret; > + > + pm_runtime_set_autosuspend_delay(dev, INV_ICM42370_SUSPEND_DELAY_MS); > + pm_runtime_use_autosuspend(dev); > + > + ret = devm_iio_device_register(dev, indio_dev); [Severity: Medium] Is there a missing cleanup action for pm_runtime_use_autosuspend(dev)? If the driver is unbound or the probe fails after this point, the device will remain marked for autosuspend indefinitely, leaving an unbalanced usage counter. > diff --git a/drivers/iio/accel/inv_icm42370_i2c.c b/drivers/iio/accel/inv_icm42370_i2c.c > index 0000000000000..c4533671051c4 > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370_i2c.c [ ... ] > +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, > +}; [Severity: High] The core probe enables runtime PM and autosuspend, but this I2C driver structure lacks a .pm operations pointer. Without assigning dev_pm_ops and implementing runtime_suspend, does the hardware ever actually power down when idle? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260813-b4-inv_icm42370p-v2-0-11aedfdf76d3@axis.com?part=2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] iio: accel: Add support for ICM42370P 2026-08-13 12:26 ` [PATCH v2 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar 2026-08-13 12:41 ` sashiko-bot @ 2026-08-16 2:59 ` Jonathan Cameron 1 sibling, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2026-08-16 2:59 UTC (permalink / raw) To: Kanak Shilledar Cc: 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, 13 Aug 2026 14:26:11 +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 Welcome to IIO. Various comments below. > diff --git a/drivers/iio/accel/inv_icm42370.h b/drivers/iio/accel/inv_icm42370.h > new file mode 100644 > index 0000000000000..a4a822f04355f > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370.h ... > + > +#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)) What is readability benefit of this macro over just having FIELD_PREP() inline where we can it matches with the mask that is likely being used right next to it. This sort of things is fine if there is significant complexity but not in simple cases like this. ... > +/* 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) There are various ways of formatting register defines to make it easier to see how things fit together. My personal favourite is along the lines of #define INV_ICM42370_REG_TMST_CONFIG1 0x00 #define INV_ICM42370_TMST_CONFIG_TMST_DELTA_EN BIT(2) #define INV_ICM42370_TMST_CONFIG_TMST_EN BIT(0) #define INV_ICM42370_REG_FIFO_CONFIG5 0x01 Where there are masks and values defined just use more indentation for the values. > + > +#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 the masks then use FIELD_PREP() inline. Tends to make it much easier to see what is going on. > + > +#define INV_ICM42370_TEMP_CHAN(_index) \ As below - this might not be needed at all. > + { \ > + .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, _ext_info) \ This doesn't belong in a header. Push it down near where it is used in core.c > + { \ > + .type = IIO_ACCEL, \ > + .modified = 1, \ > +enum inv_icm42370_chip { As below. An enum of chip identifiers is a lot messier to maintain and tends to end up with stuff that should be data as constants. So drop this - you don't need any of this for now anyway as only one supported chip. > + INV_CHIP_INVALID, > + INV_CHIP_ICM42370, > + INV_CHIP_NB, > +}; > + > +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, Drop trailing commas on entries that terminate an enum > +}; ... > + > +typedef int (*inv_icm42370_bus_setup)(struct inv_icm42370_data *); > +extern const struct regmap_config inv_icm42370_regmap_config; > + > +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 *data, > + struct inv_icm42370_conf *conf, > + unsigned int *sleep_ms); > + > +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev); No such function yet. Bring it in when the function is added. > +int inv_icm42370_mreg_write(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 val); > +int inv_icm42370_mreg_read(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 *val); > +int inv_icm42370_set_pwr_mgmt0(struct inv_icm42370_data *data, > + enum inv_icm42370_sensor_mode accel, > + unsigned int *sleep_ms); Will comment on this in next patch perhaps but generally the divide into core and buffer is part of some ancient history - I'd be tempted to squash both into the core.c file for this device. That will let you remove a lot of this stuff. > + > +#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..6266362e83f6a > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370_core.c ... > + > +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_ext_infos), Why have this last parameter as it is same for all ACCEL_CHAN? Just encode it in the macro. > + INV_ICM42370_ACCEL_CHAN(IIO_MOD_Y, INV_ICM42370_ACCEL_SCAN_Y, > + inv_icm42370_accel_ext_infos), > + INV_ICM42370_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42370_ACCEL_SCAN_Z, > + inv_icm42370_accel_ext_infos), > + INV_ICM42370_TEMP_CHAN(INV_ICM42370_ACCEL_SCAN_TEMP), Not sure the macro adds anything given only one useage. I'd define the temp iio_chan_spec directly here. > +}; > + > +/* IIO format int + nano */ > +static const int inv_icm42370_accel_scale[] = { Use a 2 dimensional array and cast it for the read_avail() usage They end up much easier to read. [INV_ICM4370_ACCEL_FS_16] = { 0, 4799403 }, etc Lots of examples of this in tree. > + /* +/- 16G => 2*16*9.80665 / (2**15) m/s-2 */ > + [2 * INV_ICM42370_ACCEL_FS_16G] = 0, > + [2 * INV_ICM42370_ACCEL_FS_16G + 1] = 4788403, > + /* +/- 8G => 2*8*9.80665 / (2**15) m/s-2 */ > + [2 * INV_ICM42370_ACCEL_FS_8G] = 0, > + [2 * INV_ICM42370_ACCEL_FS_8G + 1] = 2394202, > + /* +/- 4G => 2*4*9.80665 / (2**15) m/s-2 */ > + [2 * INV_ICM42370_ACCEL_FS_4G] = 0, > + [2 * INV_ICM42370_ACCEL_FS_4G + 1] = 1197101, > + /* +/- 2G => 2*2*9.80665 / (2**15) m/s-2 */ > + [2 * INV_ICM42370_ACCEL_FS_2G] = 0, > + [2 * INV_ICM42370_ACCEL_FS_2G + 1] = 598550, > +}; > + > +/** > + * 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] = { > + 0, 0, 0, 0, 0, /* Reserved */ > + 625000, /* 1.6kHz */ [INV_ICM42370_ODR_1_6KHZ_LN] = 625000, etc and no comments as they become self documenting. > + 1250000, /* 800Hz */ > + 2500000, /* 400Hz */ > + 5000000, /* 200Hz */ > + 10000000, /* 100Hz */ > + 20000000, /* 50Hz */ > + 40000000, /* 25Hz */ > + 80000000, /* 12.5Hz */ > + 160000000, /* 6.25Hz */ > + 320000000, /* 3.125Hz */ > + 640000000, /* 1.5625Hz */ > + }; > + > + 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[] = { Feels like this would be better done with a bit of maths than an aray of the numbers 5,6 etc to account for that set of reserved low values. > + 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, > +}; That NB is an entry that will always be last. As such, drop the trailing , > + > +/** > + * inv_icm42370_mreg_read() - routine for reading from other bank registers > + * > + * @data: pointer to struct containing the sensor data > + * @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 > + */ > +int inv_icm42370_mreg_read(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 *val) > +{ > + int ret; > + unsigned int sleep, read_val; > + > + /* set default conf to ensure mreg access */ > + ret = inv_icm42370_set_accel_conf(data, &inv_icm42370_default_conf, &sleep); > + if (ret) > + return ret; > + > + ret = inv_icm42370_mreg_check(data->map); > + if (ret) > + return ret; > + > + ret = regmap_write(data->map, INV_ICM42370_REG_BLK_SEL_R, bank); > + if (ret) > + return -EINVAL; > + > + ret = regmap_write(data->map, INV_ICM42370_REG_MADDR_R, addr); > + if (ret) > + return -EINVAL; > + > + fsleep(10); All of these sleeps need docs. > + ret = regmap_read(data->map, INV_ICM42370_REG_M_R, &read_val); > + if (ret) > + return -EINVAL; > + > + fsleep(10); Why would you sleep before saving the value to *val? > + *val = (u8)read_val; > + > + return regmap_write(data->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); Why? Any sleep really needs a spec reference to say why it has that value. > + > + data->conf = *conf; > + > + return 0; > +} > + > +/** > + * inv_icm42370_set_pwr_mgmt0() - set the PWR_MGMT0 register for sensor > + * > + * @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 > + */ > +int inv_icm42370_set_pwr_mgmt0(struct inv_icm42370_data *data, > + enum inv_icm42370_sensor_mode accel, > + unsigned int *sleep_ms) > +{ > + enum inv_icm42370_sensor_mode oldaccel = 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(data->map, INV_ICM42370_REG_PWR_MGMT0, val); > + if (ret) > + return ret; > + > + data->conf.mode = accel; > + data->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 */ > + fsleep(100); Curious why this fsleep()? > + 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; > +} > + > +/** > + * inv_icm42370_set_accel_conf() - set configuration data for accelerometer > + * > + * @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 *data, > + struct inv_icm42370_conf *conf, > + unsigned int *sleep_ms) > +{ > + struct inv_icm42370_conf *oldconf = &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(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) */ This comment doesn't add much I think. Plus if you did want it don't have a blank line before the thing it is talking about. > + > + return inv_icm42370_set_pwr_mgmt0(data, conf->mode, sleep_ms); > +} > + > +/** > + * inv_icm42370_setup() - check and setup chip > + * > + * @data: pointer to struct containing the sensor data > + * @bus_setup: callback to configure bus-specific settings (e.g. I2C) > + * > + * 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) { We don't fail on this any more (some drivers still need fixing) as it breaks the concept of fallback compatibles in device tree. At most dev_info() or maybe dev_dbg() then carry on anyway. > + dev_err(dev, "Wrong WHO_AM_I: %d (want 0x%02X)\n", whoami, > + INV_ICM42370_WHOAMI_VALUE); > + return -ENODEV; > + } > + > + data->name = "inv_icm42370"; What is this for? > + > + /* 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); Why? Given I doubt you are running this on a big endian host, maybe little endian would save a bit of work for most users? > + 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 *data = _data; > + > + data->timestamp = iio_get_time_ns(data->indio_accel); Maybe it makes sense later, but for now I can't see why this is useful vs the standard handler iio_pollfunc_store_timestamp I think it would be better to bring in the interrupt handling only when there is something to do with it (so probably next patch?) > + > + return IRQ_WAKE_THREAD; > +} > + > +static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data) > +{ > + struct inv_icm42370_data *data = _data; > + unsigned int status; > + int ret; > + > + guard(mutex)(&data->lock); > + > + ret = regmap_read(data->map, INV_ICM42370_REG_INT_STATUS, &status); > + if (ret) > + return IRQ_HANDLED; Hmm. This is rather odd at the moment. Does reading status clear the interrupt? Even if you are going to add proper support later, you should check if there was an interrupt there and return IRQ_NONE if not. > + > + 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; Where no other strong reason for ordering reverse xmas preferred. > + > + /* 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, 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, INV_ICM42370_MREG1, > + INV_ICM42370_REG_INT_CONFIG1, val); > + if (ret) > + return ret; > + > + irq_type |= IRQF_ONESHOT; After dropping the default below, you can just skip the |= and pass IRQF_ONESHOT in directly as the irq_type, with the rest picked up from firmware > + return devm_request_threaded_irq(dev, irq, inv_icm42370_irq_timestamp, > + inv_icm42370_irq_handler, irq_type, > + "inv_icm42370", data); > +} > +/** > + * inv_icm42370_temp_read() - internal function to access temperature sensor registers > + * > + * @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 *data, s16 *temp) > +{ > + struct device *dev = regmap_get_device(data->map); > + __be16 *raw; > + int ret; > + > + PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); > + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); > + if (ret) > + return ret; > + > + guard(mutex)(&data->lock); > + > + raw = (__be16 *)&data->buffer[0]; > + ret = regmap_bulk_read(data->map, INV_ICM42370_REG_TEMP_DATA1, raw, > + sizeof(*raw)); > + if (ret) > + return ret; > + > + *temp = (s16)be16_to_cpup(raw); Hmm. You are forcing alignment to at least 4 (maybe 8 I can't remember the minimum) so this is valid, but none the less I'd just do *temp = get_unaligned_be16(data->buffer); so that we don't need to think about that complexity or cast the types. > + > + /* > + * Temperature data is invalid if both accel and gyro are off. > + * Return -EBUSY in this case. Not particularly helpful and currently wrong. The return bit is obvious once code fixed so I'd drop that second comment line. > + */ > + if (*temp == INV_ICM42370_DATA_INVALID) > + ret = -EBUSY; return -EBUSY; > + > + return 0; > +} > + > +/** > + * 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 *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(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 > + * > + * @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 *data, > + struct iio_chan_spec const *chan, > + int *val, int *val2) > +{ > + struct device *dev = regmap_get_device(data->map); > + s64 val64; > + s32 bias; > + unsigned int reg; > + s16 offset; > + u8 buffer_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_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); > + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); > + if (ret) > + return ret; > + > + guard(mutex)(&data->lock); > + > + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, reg, > + &data->buffer[0]); > + if (ret) > + return ret; > + > + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, reg + 1, > + &data->buffer[1]); > + if (ret) > + return ret; > + > + memcpy(buffer_data, data->buffer, sizeof(buffer_data)); > + > + /* 12 bits signed value */ > + switch (chan->channel2) { > + case IIO_MOD_X: > + offset = sign_extend32(((buffer_data[0] & 0xF0) << 4) | buffer_data[1], 11); > + break; > + case IIO_MOD_Y: > + offset = sign_extend32(((buffer_data[1] & 0x0F) << 8) | buffer_data[0], 11); > + break; > + case IIO_MOD_Z: > + offset = sign_extend32(((buffer_data[0] & 0xF0) << 4) | buffer_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 / (long)MEGA; > + *val2 = bias % (long)MEGA; > + > + return IIO_VAL_INT_PLUS_MICRO; > +} > + > +/** > + * inv_icm42370_accel_write_offset() - write offset values to the accelerometer > + * > + * @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 *data, > + struct iio_chan_spec const *chan, > + int val, int val2) > +{ > + struct device *dev = regmap_get_device(data->map); > + s64 val64; > + s32 min, max; > + u8 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] * (long)MEGA + > + inv_icm42370_accel_calibbias[1]; > + max = inv_icm42370_accel_calibbias[4] * (long)MEGA + > + inv_icm42370_accel_calibbias[5]; > + > + val64 = (s64)val * (s64)MEGA; > + if (val >= 0) > + val64 += (s64)val2; > + else > + val64 -= (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_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); > + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); > + if (ret) > + return ret; > + > + guard(mutex)(&data->lock); > + > + switch (chan->channel2) { > + case IIO_MOD_X: > + /* OFFSET_USER4 register is shared */ > + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER4, > + ®val); > + if (ret) > + return ret; > + > + data->buffer[0] = ((offset & 0xF00) >> 4) | (regval & 0x0F); > + data->buffer[1] = offset & 0xFF; > + > + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER4, > + data->buffer[0]); > + if (ret) > + return ret; > + > + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER5, > + data->buffer[1]); > + > + if (ret) > + return ret; > + break; > + > + case IIO_MOD_Y: > + /* OFFSET_USER7 register is shared */ > + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER7, > + ®val); > + if (ret) > + return ret; > + > + data->buffer[0] = offset & 0xFF; > + data->buffer[1] = ((offset & 0xF00) >> 8) | (regval & 0xF0); > + > + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER7, > + data->buffer[1]); > + > + if (ret) > + return ret; > + > + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER6, > + data->buffer[0]); > + if (ret) > + return ret; > + > + break; > + > + case IIO_MOD_Z: > + /* OFFSET_USER7 register is shared */ > + ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER7, > + ®val); > + if (ret) > + return ret; > + > + data->buffer[0] = ((offset & 0xF00) >> 4) | (regval & 0x0F); > + data->buffer[1] = offset & 0xFF; > + > + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER7, > + data->buffer[0]); > + if (ret) > + return ret; > + > + ret = inv_icm42370_mreg_write(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_OFFSET_USER8, > + data->buffer[1]); > + if (ret) > + return ret; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +/** > + * 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 *data = iio_priv(indio_dev); > + unsigned int idx; > + > + idx = data->conf.fs; > + > + *val = data->scales[2 * idx]; > + *val2 = data->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 *data = iio_priv(indio_dev); > + struct device *dev = regmap_get_device(data->map); > + unsigned int idx; > + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; > + int ret; > + > + for (idx = 0; idx < data->scales_len; idx += 2) { > + if (val == data->scales[idx] && > + val2 == data->scales[idx + 1]) > + break; > + } > + > + if (idx >= data->scales_len) > + return -EINVAL; > + > + conf.fs = idx / 2; > + > + PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); > + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); > + if (ret) > + return ret; > + > + guard(mutex)(&data->lock); > + > + ret = inv_icm42370_set_accel_conf(data, &conf, NULL); > + > + return ret; > +} > + > +/** > + * inv_icm42370_accel_read_odr() - read ODR data from the accelerometer > + * > + * @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 *data, > + int *val, int *val2) > +{ > + unsigned int odr; > + unsigned int i; > + > + odr = 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 *data = iio_priv(indio_dev); > + struct inv_sensors_timestamp *ts = &data->ts; > + struct device *dev = regmap_get_device(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_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); > + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); > + if (ret) > + return ret; > + > + guard(mutex)(&data->lock); > + > + ret = inv_sensors_timestamp_update_odr( > + ts, inv_icm42370_odr_to_period(conf.odr), > + iio_buffer_enabled(indio_dev)); > + if (ret) > + return ret; > + > + ret = inv_icm42370_set_accel_conf(data, &conf, NULL); > + if (ret) > + return ret; > + > + return 0; > +} > + > +/** > + * 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 *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(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 I/O 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); > + struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT; > + 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_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm); > + ret = PM_RUNTIME_ACQUIRE_ERR(&pm); > + if (ret) > + return ret; > + > + guard(mutex)(&data->lock); > + > + /* enable accel sensor */ > + conf.mode = data->power_mode; > + conf.filter = data->filter; > + ret = inv_icm42370_set_accel_conf(data, &conf, NULL); > + if (ret) > + return ret; > + > + /* read accel register data */ > + value = (__be16 *)&data->buffer[0]; > + ret = regmap_bulk_read(data->map, reg, value, sizeof(*value)); > + if (ret) > + return ret; > + > + *val = (s16)be16_to_cpup(value); > + > + if (*val == INV_ICM42370_DATA_INVALID) > + ret = -EINVAL; return -EINVAL; return 0; as it makes it easier to spot bad vs good paths. > + > + return ret; > +} > + > +/** > + * inv_icm42370_accel_read_raw() - read data from the accelerometer sensor As below, we'd not normally have kernel-doc for this sort of standard function callback unless there was something unusual to talk about. So in the interests of brevity I'd drop it. > + * > + * @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 iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, > + struct inv_icm42370_data *data) Given this is setting up a bunch of things in different structures I'm not sure I see the advantage of factoring it out of probe. I'd just put this code inline there instead. > +{ > + struct inv_sensors_timestamp_chip ts_chip; > + > + data->scales = inv_icm42370_accel_scale; > + data->scales_len = ARRAY_SIZE(inv_icm42370_accel_scale); > + data->filter = data->conf.filter; > + data->power_mode = data->conf.mode; > + > + /* > + * 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->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); > + > + 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 fwnode_handle *fwnode = dev_fwnode(dev); > + struct inv_icm42370_data *data; > + struct iio_dev *indio_dev; > + int irq_type; > + bool open_drain; > + int ret; > + > + if (chip != INV_CHIP_ICM42370) { Pass in a pointer to device specific data and if it is NULL fail out here. That lets you use the bus specific code to get firmware data like i2c_get_match_data() that will get it from whatever firmware we have. > + dev_err(dev, "invalid chip = %d\n", chip); > + return -ENODEV; > + } > + > + /* get INT1 only supported interrupt or fallback to first interrupt */ > + irq = fwnode_irq_get_byname(fwnode, "INT1"); > + if (irq < 0 && irq != -EPROBE_DEFER) { > + dev_info(dev, > + "no INT1 interrupt defined, fallback to first interrupt\n"); > + irq = fwnode_irq_get(fwnode, 0); If we don't know it is int1 fail probe. We do this sort of hack only when we missed in the original driver review that there was a need for interrupt-names. In theory we could have a default in the dt-binding but I don't think it is useful and you don't have one documented. > + } > + if (irq < 0) > + return dev_err_probe(dev, irq, > + "error missing INT1 interrupt\n"); > + > + irq_type = irq_get_trigger_type(irq); > + if (!irq_type) > + irq_type = IRQF_TRIGGER_FALLING; This is papering over broken firmware / old mistakes in drivers that we can't fix now without risking regressions. Don't do it for a new driver. If firmware doesn't provide the trigger type then they'll find out fast when the interrupt doesn't do anything. > + > + 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); > + > + ret = devm_mutex_init(dev, &data->lock); > + if (ret) > + return dev_err_probe(dev, ret, "unable to initialize mutex\n"); > + > + data->chip = chip; > + data->map = regmap; > + > + ret = devm_regulator_get_enable(dev, "vdd"); > + if (ret) > + return dev_err_probe(dev, ret, "failed to enable vdd regulator\n"); > + > + ret = devm_regulator_get_enable(dev, "vddio"); > + if (ret) > + return dev_err_probe(dev, ret, "failed to enable vddio regulator\n"); > + > + ret = inv_icm42370_setup(data, bus_setup); > + if (ret) > + return dev_err_probe(dev, ret, "setup failed\n"); > + > + ret = iio_read_mount_matrix(dev, &data->orientation); > + if (ret) { > + dev_err(dev, "failed to retrieve mounting matrix %d\n", ret); > + return ret; return dev_err_probe() for all errors in probe() and functions only called from probe. Minor but it gives nicer looking code and consistent formatting. Deferred handling obviously not relevant for this one! > + } > + > + 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 = devm_pm_runtime_set_active_enabled(dev); > + if (ret) > + return ret; > + > + pm_runtime_set_autosuspend_delay(dev, INV_ICM42370_SUSPEND_DELAY_MS); > + pm_runtime_use_autosuspend(dev); > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "unable to register iio device\n"); > + > + return 0; > +} > +EXPORT_SYMBOL_NS_GPL(inv_icm42370_core_probe, "IIO_ICM42370"); > diff --git a/drivers/iio/accel/inv_icm42370_i2c.c b/drivers/iio/accel/inv_icm42370_i2c.c > new file mode 100644 > index 0000000000000..c4533671051c4 > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370_i2c.c > @@ -0,0 +1,93 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2020 InvenSense, Inc. > + * Copyright (C) 2026 Axis Communications AB > + */ > + > +#include <linux/device.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > + > +#include "inv_icm42370.h" > + > +/** > + * inv_icm42370_i2c_bus_setup() - I2C bus setup for icm42370 Kind of obvious stuff in this documentation given function and parameter naming. I'd drop it and save documentation for where it provides value or where a function is exported. > + * > + * @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); As noted above, I'd have FIELD_PREP() here so that we can see it is the relevant mask being used directly. Don't worry about going a little over 80 chars to do that. it is far from obvious to em that a field called CONFIG2_I2C would have anything to do with slew rate. So rename macros as needed to make it obvious what the field is alongside the value being written. > + ret = regmap_update_bits(data->map, INV_ICM42370_REG_DRIVE_CONFIG2, > + mask, val); > + if (ret) > + return ret; > + > + /* set slew rates for SPI */ In an i2c probe? this needs an explanation of why we care. > + 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; Unless this gets more complex in next patch return regmap_update_bits() > +} One blank line between functions. > +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); As elsewhere if you were doing this (no point yet) then i2c_get_match_data() to cleanly handle various possible firmware types. > + 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); > +} > + > +static const struct i2c_device_id inv_icm42370_id[] = { > + { .name = "icm42370p", .driver_data = INV_CHIP_ICM42370 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, inv_icm42370_id); > + > +static const struct of_device_id inv_icm42370_of_matches[] = { > + { .compatible = "invensense,icm42370p", .data = (void *)INV_CHIP_ICM42370 }, You are only supporting one device, so for now no .data makes more sense. When it is added, we very strongly prefer that it be a pointer to a structure with the device type specific features described. For now rip out the check on valid data above. We don't really care that much about later breaking forced device probing from userspace as that is only really a debug tool and when we have broken it in the past no one has noticed. That to me is better than making this driver more complex to support multiple parts when it is very common for only one to ever turn up! If you have other parts you plan to support in the near future, then that is a different question - add a note on that if so to the cover letter and this patches description. > + { } > +}; > +MODULE_DEVICE_TABLE(of, inv_icm42370_of_matches); ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality 2026-08-13 12:26 [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar 2026-08-13 12:26 ` [PATCH v2 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar 2026-08-13 12:26 ` [PATCH v2 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar @ 2026-08-13 12:26 ` Kanak Shilledar 2026-08-13 12:40 ` sashiko-bot ` (2 more replies) 2026-08-17 7:19 ` [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer Andy Shevchenko 3 siblings, 3 replies; 11+ messages in thread From: Kanak Shilledar @ 2026-08-13 12:26 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 buffer setup 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 | 6 + drivers/iio/accel/inv_icm42370_buffer.c | 496 ++++++++++++++++++++++++++++++++ drivers/iio/accel/inv_icm42370_buffer.h | 100 +++++++ drivers/iio/accel/inv_icm42370_core.c | 206 ++++++++++++- 6 files changed, 809 insertions(+), 1 deletion(-) diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index 3ac2534c1c76d..a8dc5692db9d9 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-P 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 a4a822f04355f..5df5876169cc9 100644 --- a/drivers/iio/accel/inv_icm42370.h +++ b/drivers/iio/accel/inv_icm42370.h @@ -17,6 +17,7 @@ #include <linux/iio/common/inv_sensors_timestamp.h> #include <linux/iio/iio.h> +#include "inv_icm42370_buffer.h" #define INV_ICM42370_SENSOR_CONF_INIT { -1, -1, -1, -1 } @@ -45,6 +46,8 @@ #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 @@ -199,6 +202,7 @@ enum inv_icm42370_accel_scan { INV_ICM42370_ACCEL_SCAN_Y, INV_ICM42370_ACCEL_SCAN_Z, INV_ICM42370_ACCEL_SCAN_TEMP, + INV_ICM42370_ACCEL_SCAN_TIMESTAMP, }; enum inv_icm42370_sensor_mode { @@ -286,6 +290,7 @@ struct inv_icm42370_conf { * @indio_accel: accelerometer IIO device. * @timestamp: interrupt timestamp. * @orientation: sensor chip orientation relative to main hardware. + * @fifo: FIFO state and configuration. * @chip: chip identifier. * @conf: chip sensors configurations. * @filter: sensor filter. @@ -303,6 +308,7 @@ struct inv_icm42370_data { struct iio_dev *indio_accel; s64 timestamp; struct iio_mount_matrix orientation; + struct inv_icm42370_fifo fifo; enum inv_icm42370_chip chip; struct inv_icm42370_conf conf; enum inv_icm42370_filter filter; diff --git a/drivers/iio/accel/inv_icm42370_buffer.c b/drivers/iio/accel/inv_icm42370_buffer.c new file mode 100644 index 0000000000000..c6a4d313c92c5 --- /dev/null +++ b/drivers/iio/accel/inv_icm42370_buffer.c @@ -0,0 +1,496 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 Invensense, Inc. + * Copyright (C) 2026 Axis Communications AB + */ + +#include <linux/minmax.h> +#include <linux/mutex.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/delay.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 *data) +{ + u32 period_accel; + + if (data->fifo.en & INV_ICM42370_SENSOR_ACCEL) + period_accel = inv_icm42370_odr_to_period(data->conf.odr); + else + period_accel = U32_MAX; + + data->fifo.period = period_accel; +} + +int inv_icm42370_buffer_set_fifo_en(struct inv_icm42370_data *data, + 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(data, 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(data, INV_ICM42370_MREG1, + INV_ICM42370_REG_FIFO_CONFIG5, regval); + if (ret) + return ret; + + data->fifo.en = fifo_en; + inv_icm42370_buffer_update_fifo_period(data); + + 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 + * @data: 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 *data) +{ + size_t packet_size, wm_size; + unsigned int wm, watermark; + bool restore; + __le16 raw_wm; + int ret; + + packet_size = inv_icm42370_get_packet_size(data->fifo.en); + + /* compute sensors latency, depending on sensor watermark and odr */ + wm = inv_icm42370_wm_truncate(data->fifo.watermark.accel, packet_size); + + /* 0 value for watermark means that the sensor is turned off */ + if (wm == 0) + return 0; + + watermark = wm; + data->fifo.watermark.eff_accel = wm; + + /* compute watermark value in bytes */ + wm_size = watermark * packet_size; + + /* changing FIFO watermark requires to turn off watermark interrupt */ + ret = regmap_update_bits_check( + data->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(data->buffer, &raw_wm, sizeof(raw_wm)); + ret = regmap_bulk_write(data->map, INV_ICM42370_REG_FIFO_WATERMARK, + data->buffer, sizeof(raw_wm)); + if (ret) + return ret; + + /* restore watermark interrupt */ + if (restore) { + ret = regmap_set_bits( + data->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 *data = iio_priv(indio_dev); + struct device *dev = regmap_get_device(data->map); + struct inv_sensors_timestamp *ts = &data->ts; + int ret; + + ret = pm_runtime_resume_and_get(dev); + if (ret) + return ret; + + guard(mutex)(&data->lock); + + inv_sensors_timestamp_reset(ts); + + return 0; +} + +static int inv_icm42370_buffer_postenable(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *data = iio_priv(indio_dev); + int ret; + + guard(mutex)(&data->lock); + + /* Exit if FIFO is already on. */ + if (data->fifo.on) { + data->fifo.on++; + return 0; + } + + ret = regmap_write(data->map, INV_ICM42370_REG_SIGNAL_PATH_RESET, + INV_ICM42370_SIGNAL_PATH_RESET_FIFO_FLUSH); + if (ret) + return ret; + + ret = regmap_write(data->map, INV_ICM42370_REG_FIFO_CONFIG1, + INV_ICM42370_FIFO_CONFIG_STREAM); + if (ret) + return ret; + + /* when FIFO_CONFIG_STREAM bit is set FIFO is enabled, so + * increase the count + */ + data->fifo.on++; + + ret = regmap_bulk_read(data->map, INV_ICM42370_REG_FIFO_COUNT, data->buffer, + 2); + if (ret) + return ret; + + ret = regmap_set_bits(data->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_predisable(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *data = iio_priv(indio_dev); + int ret; + + guard(mutex)(&data->lock); + + /* Exit if there are several sensors using the FIFO. */ + if (data->fifo.on > 1) { + data->fifo.on--; + return 0; + } + + /* set FIFO in bypass mode */ + ret = regmap_write(data->map, INV_ICM42370_REG_FIFO_CONFIG1, + INV_ICM42370_FIFO_CONFIG_BYPASS); + if (ret) + return ret; + + /* when FIFO is bypassed it gets disabled, so reduce the + * count + */ + data->fifo.on--; + + /* flush FIFO data */ + ret = regmap_write(data->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(data->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_postdisable(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *data = iio_priv(indio_dev); + struct inv_sensors_timestamp *ts = &data->ts; + struct device *dev = regmap_get_device(data->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 == data->indio_accel) { + sensor = INV_ICM42370_SENSOR_ACCEL; + watermark = &data->fifo.watermark.accel; + } else { + return -EINVAL; + } + + guard(mutex)(&data->lock); + + inv_sensors_timestamp_apply_odr(ts, 0, 0, 0); + + ret = inv_icm42370_buffer_set_fifo_en(data, data->fifo.en & ~sensor); + if (ret) + goto out_unlock; + + *watermark = 0; + ret = inv_icm42370_buffer_update_watermark(data); + if (ret) + goto out_unlock; + + conf.mode = INV_ICM42370_SENSOR_MODE_OFF; + ret = inv_icm42370_set_accel_conf(data, &conf, &sleep_sensor); + if (ret) + goto out_unlock; + +out_unlock: + /* 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, +}; + +int inv_icm42370_buffer_fifo_read(struct inv_icm42370_data *data, + unsigned int max) +{ + const ssize_t packet_size = sizeof(struct inv_icm42370_fifo_packet_1); + __be16 *raw_fifo_count; + size_t fifo_nb, i; + ssize_t size; + const void *accel, *timestamp; + const s8 *temp; + unsigned int odr; + int ret; + + /* reset all samples counters */ + data->fifo.count = 0; + data->fifo.nb.accel = 0; + data->fifo.nb.total = 0; + + raw_fifo_count = (__be16 *)data->buffer; + ret = regmap_bulk_read(data->map, INV_ICM42370_REG_FIFO_COUNT, + raw_fifo_count, sizeof(*raw_fifo_count)); + if (ret) + return ret; + + /* Check and limit number of samples if requested. */ + fifo_nb = le16_to_cpup(raw_fifo_count); + if (fifo_nb == 0) + return 0; + if (max > 0 && fifo_nb > max) + fifo_nb = max; + + /* + * Read all FIFO data into the internal buffer, clamping the + * device-reported count to the buffer capacity. + */ + data->fifo.count = min(fifo_nb * packet_size, INV_ICM42370_FIFO_SIZE_MAX); + ret = regmap_noinc_read(data->map, INV_ICM42370_REG_FIFO_DATA, + data->fifo.data, data->fifo.count); + if (ret == -EOPNOTSUPP || ret == -EFBIG) { + /* Read full fifo is not supported, read samples one by one. */ + ret = 0; + for (i = 0; i < data->fifo.count && ret == 0; i += packet_size) + ret = regmap_noinc_read(data->map, INV_ICM42370_REG_FIFO_DATA, + &data->fifo.data[i], packet_size); + } + if (ret) + return ret; + + for (i = 0; i < data->fifo.count; i += size) { + size = inv_icm42370_fifo_decode_packet( + &data->fifo.data[i], &accel, &temp, ×tamp, &odr); + if (size <= 0) + /* No more sample in buffer */ + break; + if (accel && inv_icm42370_fifo_is_data_valid(accel)) + data->fifo.nb.accel++; + data->fifo.nb.total++; + } + + return 0; +} + +int inv_icm42370_buffer_fifo_parse(struct inv_icm42370_data *data) +{ + struct inv_sensors_timestamp *ts; + int ret; + + if (data->fifo.nb.total == 0) + return 0; + + /* handle accelerometer timestamp and FIFO data parsing */ + if (data->fifo.nb.accel > 0) { + ts = &data->ts; + inv_sensors_timestamp_interrupt( + ts, data->fifo.watermark.eff_accel, data->timestamp); + ret = inv_icm42370_accel_parse_fifo(data->indio_accel); + if (ret) + return ret; + } + + return 0; +} + +int inv_icm42370_buffer_hwfifo_flush(struct inv_icm42370_data *data, + unsigned int count) +{ + struct inv_sensors_timestamp *ts; + s64 accel_ts; + int ret; + + accel_ts = iio_get_time_ns(data->indio_accel); + + ret = inv_icm42370_buffer_fifo_read(data, count); + if (ret) + return ret; + + if (data->fifo.nb.total == 0) + return 0; + + if (data->fifo.nb.accel > 0) { + ts = &data->ts; + inv_sensors_timestamp_interrupt(ts, data->fifo.nb.accel, + accel_ts); + ret = inv_icm42370_accel_parse_fifo(data->indio_accel); + if (ret) + return ret; + } + + return 0; +} + +int inv_icm42370_buffer_init(struct inv_icm42370_data *data) +{ + unsigned int val; + u8 regval; + int ret; + + data->fifo.watermark.eff_accel = 1; + + /* watermark should be set to a non-zero value before enabling interrupts */ + data->fifo.watermark.accel = 1; + ret = inv_icm42370_buffer_update_watermark(data); + 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(data->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(data, 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(data, 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..d43a4209a5640 --- /dev/null +++ b/drivers/iio/accel/inv_icm42370_buffer.h @@ -0,0 +1,100 @@ +/* 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> + +#define INV_ICM42370_SENSOR_ACCEL BIT(1) +#define INV_ICM42370_SENSOR_TEMP BIT(2) + +/* 2.25kB buffer size */ +#define INV_ICM42370_FIFO_SIZE_MAX 0x900 +#define INV_ICM42370_FIFO_DATA_INVALID -32768 + +struct inv_icm42370_data; + +/** + * 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. + */ +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; + u32 period; + u8 *data __aligned(IIO_DMA_MINALIGN); +}; + +/* FIFO data packet */ +struct inv_icm42370_fifo_sensor_data { + __be16 x; + __be16 y; + __be16 z; +} __packed; + +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 *data); + +void inv_icm42370_buffer_update_fifo_period(struct inv_icm42370_data *data); + +int inv_icm42370_buffer_set_fifo_en(struct inv_icm42370_data *data, + unsigned int fifo_en); + +int inv_icm42370_buffer_update_watermark(struct inv_icm42370_data *data); + +int inv_icm42370_buffer_fifo_read(struct inv_icm42370_data *data, + unsigned int max); + +int inv_icm42370_buffer_fifo_parse(struct inv_icm42370_data *data); + +int inv_icm42370_buffer_hwfifo_flush(struct inv_icm42370_data *data, + unsigned int count); + +#endif diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c index 6266362e83f6a..b1713a2f533c8 100644 --- a/drivers/iio/accel/inv_icm42370_core.c +++ b/drivers/iio/accel/inv_icm42370_core.c @@ -20,10 +20,25 @@ #include <linux/types.h> #include <linux/units.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 "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; +} #define INV_ICM42370_SUSPEND_DELAY_MS 2000 @@ -32,6 +47,7 @@ const struct regmap_config inv_icm42370_regmap_config = { .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"); @@ -57,6 +73,7 @@ static const struct iio_chan_spec inv_icm42370_accel_channels[] = { INV_ICM42370_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42370_ACCEL_SCAN_Z, inv_icm42370_accel_ext_infos), INV_ICM42370_TEMP_CHAN(INV_ICM42370_ACCEL_SCAN_TEMP), + IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42370_ACCEL_SCAN_TIMESTAMP), }; /* IIO format int + nano */ @@ -75,6 +92,18 @@ static const int inv_icm42370_accel_scale[] = { [2 * INV_ICM42370_ACCEL_FS_2G + 1] = 598550, }; +/* + * 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 * @@ -431,6 +460,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 *data = _data; + struct device *dev = regmap_get_device(data->map); unsigned int status; int ret; @@ -440,6 +470,21 @@ static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data) if (ret) return IRQ_HANDLED; + 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(data, 0); + if (ret) { + dev_err_ratelimited(dev, "FIFO read error %d\n", ret); + return IRQ_HANDLED; + } + ret = inv_icm42370_buffer_fifo_parse(data); + if (ret) + dev_err_ratelimited(dev, "FIFO parsing error %d\n", ret); + } + return IRQ_HANDLED; } @@ -505,6 +550,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, 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, 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. @@ -990,6 +1056,8 @@ static int inv_icm42370_accel_write_odr(struct iio_dev *indio_dev, int val, if (ret) return ret; + inv_icm42370_buffer_update_fifo_period(data); + return 0; } @@ -1039,6 +1107,40 @@ 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_conf conf = INV_ICM42370_SENSOR_CONF_INIT; + unsigned int fifo_en = 0; + unsigned int sleep_accel = 0; + int ret; + + guard(mutex)(&st->lock); + + if (*scan_mask & INV_ICM42370_SCAN_MASK_ACCEL_3AXIS) { + /* enable accel sensor */ + conf.mode = st->conf.mode; + conf.filter = st->filter; + + ret = inv_icm42370_set_accel_conf(st, &conf, &sleep_accel); + if (ret) + goto out; + + fifo_en |= INV_ICM42370_SENSOR_ACCEL; + } + + /* update data FIFO write */ + ret = inv_icm42370_buffer_set_fifo_en(st, fifo_en | st->fifo.en); + +out: + if (sleep_accel) + msleep(sleep_accel); + + return ret; +} + /** * inv_icm42370_accel_read_sensor() - internal function to read accelerometer sensor registers * @@ -1161,15 +1263,54 @@ 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; + + guard(mutex)(&st->lock); + + st->fifo.watermark.accel = val; + ret = inv_icm42370_buffer_update_watermark(st); + + 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, struct inv_icm42370_data *data) { + struct device *dev = regmap_get_device(data->map); struct inv_sensors_timestamp_chip ts_chip; + int ret; data->scales = inv_icm42370_accel_scale; data->scales_len = ARRAY_SIZE(inv_icm42370_accel_scale); @@ -1187,13 +1328,64 @@ 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); + return indio_dev; } +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev) +{ + struct inv_icm42370_data *data = iio_priv(indio_dev); + struct inv_sensors_timestamp *ts = &data->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 < data->fifo.count; i += size, ++no) { + size = inv_icm42370_fifo_decode_packet(&data->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, data->fifo.period, + data->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. @@ -1242,6 +1434,10 @@ int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq, data = iio_priv(indio_dev); + data->fifo.data = devm_kzalloc(dev, INV_ICM42370_FIFO_SIZE_MAX, GFP_KERNEL); + if (!data->fifo.data) + return -ENOMEM; + ret = devm_mutex_init(dev, &data->lock); if (ret) return dev_err_probe(dev, ret, "unable to initialize mutex\n"); @@ -1267,6 +1463,14 @@ int inv_icm42370_core_probe(struct regmap *regmap, int chip, int irq, return ret; } + 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] 11+ messages in thread
* Re: [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality 2026-08-13 12:26 ` [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar @ 2026-08-13 12:40 ` sashiko-bot 2026-08-16 3:12 ` Jonathan Cameron 2026-08-17 9:48 ` Joshua Crofts 2 siblings, 0 replies; 11+ messages in thread From: sashiko-bot @ 2026-08-13 12:40 UTC (permalink / raw) To: Kanak Shilledar; +Cc: robh, conor+dt, devicetree Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [High] Missing `available_scan_masks` causes IIO buffer data corruption when channels are disabled. - [High] Incorrect endianness parsing and scaling of the hardware `FIFO_COUNT` register. - [High] Enabling only the TEMP channel causes the IIO buffer to hang indefinitely. - [High] Hardware registers are accessed without acquiring PM runtime in sysfs and IIO core callbacks. - [Medium] Mixing `goto` error handling with `guard(mutex)` causes device lock to be held during sleeps. - [Medium] The internal FIFO state tracker `data->fifo.on` leaks when hardware configuration fails. - [Medium] The sensor hardware is left powered on indefinitely if configuration steps fail during buffer state transitions. - [Medium] The FIFO watermark interrupt is permanently disabled if updating the watermark register fails. -- --- Patch [3]: [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality --- commit 04755a7b27e94b2f885caf4cb6fed3815002934a Author: Kanak Shilledar <kanak.shilledar@axis.com> Subject: iio: accel: icm42370: Add FIFO buffer functionality Add support for the hardware FIFO buffer to support high data rate capture. This includes necessary IIO buffer setup ops, watermark management and FIFO packet decoding. The accelerometer has 2.25kB FIFO size. > diff --git a/drivers/iio/accel/inv_icm42370_buffer.c b/drivers/iio/accel/inv_icm42370_buffer.c > new file mode 100644 > index 0000000000000..c6a4d313c92c5 > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370_buffer.c [ ... ] > +int inv_icm42370_buffer_update_watermark(struct inv_icm42370_data *data) > +{ [ ... ] > + /* changing FIFO watermark requires to turn off watermark interrupt */ > + ret = regmap_update_bits_check( > + data->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(data->buffer, &raw_wm, sizeof(raw_wm)); > + ret = regmap_bulk_write(data->map, INV_ICM42370_REG_FIFO_WATERMARK, > + data->buffer, sizeof(raw_wm)); > + if (ret) > + return ret; ^^^^^^^^^^ [Severity: Medium] Will this return leave the watermark interrupt permanently disabled? If regmap_bulk_write() fails, the function exits before the restore block is reached, which might prevent future watermark interrupts. > + > + /* restore watermark interrupt */ > + if (restore) { > + ret = regmap_set_bits( > + data->map, INV_ICM42370_REG_INT_SOURCE0, > + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN); [ ... ] > +static int inv_icm42370_buffer_postenable(struct iio_dev *indio_dev) > +{ [ ... ] > + /* when FIFO_CONFIG_STREAM bit is set FIFO is enabled, so > + * increase the count > + */ > + data->fifo.on++; > + > + ret = regmap_bulk_read(data->map, INV_ICM42370_REG_FIFO_COUNT, data->buffer, > + 2); > + if (ret) > + return ret; ^^^^^^^^^^ [Severity: Medium] Does this error path leak the internal FIFO state tracker? The data->fifo.on counter is incremented, but subsequent failures exit the function without rolling it back. This might lead to silent failures when enabling data capture later. > + > + ret = regmap_set_bits(data->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_postdisable(struct iio_dev *indio_dev) > +{ [ ... ] > + guard(mutex)(&data->lock); > + > + inv_sensors_timestamp_apply_odr(ts, 0, 0, 0); > + > + ret = inv_icm42370_buffer_set_fifo_en(data, data->fifo.en & ~sensor); > + if (ret) > + goto out_unlock; > + > + *watermark = 0; > + ret = inv_icm42370_buffer_update_watermark(data); > + if (ret) > + goto out_unlock; ^^^^^^^^^^^^^^ [Severity: Medium] If the watermark update fails, could this leave the sensor hardware powered on indefinitely? The failure skips the INV_ICM42370_SENSOR_MODE_OFF configuration below, which might cause a power leak. > + > + conf.mode = INV_ICM42370_SENSOR_MODE_OFF; > + ret = inv_icm42370_set_accel_conf(data, &conf, &sleep_sensor); > + if (ret) > + goto out_unlock; > + > +out_unlock: > + /* sleep maximum required time */ > + sleep = max(sleep_sensor, sleep_temp); > + if (sleep) > + msleep(sleep); > + > + pm_runtime_put_autosuspend(dev); > + > + return ret; ^^^^^^^^^^ [Severity: Medium] Are we holding the mutex during the sleep and autosuspend operations? Since guard() is used for locking, but error handling relies on goto out_unlock, the lock is not released at the label but held until the function exits. > +} [ ... ] > +int inv_icm42370_buffer_fifo_read(struct inv_icm42370_data *data, > + unsigned int max) > +{ [ ... ] > + /* Check and limit number of samples if requested. */ > + fifo_nb = le16_to_cpup(raw_fifo_count); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Since the FIFO_COUNT is configured to use Big Endian in buffer_init(), is it correct to parse it using le16_to_cpup() here? This might cause a massive miscalculation of the count on little-endian systems. > + if (fifo_nb == 0) > + return 0; > + if (max > 0 && fifo_nb > max) > + fifo_nb = max; > + > + /* > + * Read all FIFO data into the internal buffer, clamping the > + * device-reported count to the buffer capacity. > + */ > + data->fifo.count = min(fifo_nb * packet_size, INV_ICM42370_FIFO_SIZE_MAX); ^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this inadvertently inflate the read length? If the hardware count is configured to report bytes instead of samples, multiplying it by packet_size might result in reading vastly more bytes than the FIFO contains. > + ret = regmap_noinc_read(data->map, INV_ICM42370_REG_FIFO_DATA, > + data->fifo.data, data->fifo.count); [ ... ] > +int inv_icm42370_buffer_init(struct inv_icm42370_data *data) > +{ [ ... ] > + /* > + * 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(data->map, INV_ICM42370_REG_INTF_CONFIG0, > + GENMASK(6, 5), val); [ ... ] > diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c > index 6266362e83f6a..b1713a2f533c8 100644 > --- a/drivers/iio/accel/inv_icm42370_core.c > +++ b/drivers/iio/accel/inv_icm42370_core.c [ ... ] > +/* 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_conf conf = INV_ICM42370_SENSOR_CONF_INIT; > + unsigned int fifo_en = 0; > + unsigned int sleep_accel = 0; > + int ret; > + > + guard(mutex)(&st->lock); ^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Is it possible this function gets called when the device is asleep? The IIO core might invoke update_scan_mode before preenable. Missing a pm_runtime_resume_and_get() here could lead to hardware accesses on a suspended device. > + > + if (*scan_mask & INV_ICM42370_SCAN_MASK_ACCEL_3AXIS) { > + /* enable accel sensor */ > + conf.mode = st->conf.mode; > + conf.filter = st->filter; > + > + ret = inv_icm42370_set_accel_conf(st, &conf, &sleep_accel); > + if (ret) > + goto out; > + > + fifo_en |= INV_ICM42370_SENSOR_ACCEL; > + } > + > + /* update data FIFO write */ > + ret = inv_icm42370_buffer_set_fifo_en(st, fifo_en | st->fifo.en); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] What happens if userspace requests only the TEMP channel? In that case, fifo_en evaluates to 0, meaning the sensor isn't powered on and the FIFO won't fill. This might cause the IIO buffer to hang indefinitely since watermark interrupts would never be generated. [Severity: Medium] Also, if inv_icm42370_buffer_set_fifo_en() fails here, the function returns without rolling back the sensor to its OFF state. Could this leave the sensor hardware powered on indefinitely and drain power? > + > +out: > + if (sleep_accel) > + msleep(sleep_accel); > + > + return ret; ^^^^^^^^^^ [Severity: Medium] Similar to the postdisable function, does using goto out with guard() cause the mutex to be held during the msleep() call, stalling concurrent driver operations? > +} [ ... ] > +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; > + > + guard(mutex)(&st->lock); > + > + st->fifo.watermark.accel = val; > + ret = inv_icm42370_buffer_update_watermark(st); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this interact with hardware registers without acquiring PM runtime? As a sysfs callback, this might be executed while the device is in auto-suspend, causing writes to a sleeping device. > + > + 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: High] Is it possible this flush sequence executes on a suspended device? There doesn't appear to be a pm_runtime lock here before accessing the hardware registers. > + if (!ret) > + ret = st->fifo.nb.accel; > + > + mutex_unlock(&st->lock); > + > + return ret; > +} [ ... ] > struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev, > struct inv_icm42370_data *data) > { [ ... ] > 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: High] Could the absence of indio_dev->available_scan_masks here cause data corruption? Without it, if a user enables only a subset of channels, the IIO core will expect the buffer to be packed densely. > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, > + &inv_icm42370_buffer_ops); > + if (ret) > + return ERR_PTR(ret); > + > return indio_dev; > } > > +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev) > +{ [ ... ] > + 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); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Because the driver unconditionally pushes a fixed-layout buffer without manual repacking, if available_scan_masks is missing, the IIO core will read the Y-axis bytes as Z-axis data when certain channels are disabled. Is there a plan to provide the available scan masks? > + } > + > + return 0; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/20260813-b4-inv_icm42370p-v2-0-11aedfdf76d3@axis.com?part=3 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality 2026-08-13 12:26 ` [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar 2026-08-13 12:40 ` sashiko-bot @ 2026-08-16 3:12 ` Jonathan Cameron 2026-08-17 9:48 ` Joshua Crofts 2 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2026-08-16 3:12 UTC (permalink / raw) To: Kanak Shilledar Cc: 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, 13 Aug 2026 14:26:12 +0200 Kanak Shilledar <kanak.shilledar@axis.com> wrote: > Add support for the hardware FIFO buffer to support high data rate > capture. This includes necessary IIO buffer setup 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> Hi Kanak, I'm out of time for today so this is a little superficial. I'll take a closer look at the buffer handling in v3. Thanks, Jonathan > diff --git a/drivers/iio/accel/inv_icm42370_buffer.c b/drivers/iio/accel/inv_icm42370_buffer.c > new file mode 100644 > index 0000000000000..c6a4d313c92c5 > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370_buffer.c > @@ -0,0 +1,496 @@ > + > +static int inv_icm42370_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct inv_icm42370_data *data = iio_priv(indio_dev); > + int ret; > + > + guard(mutex)(&data->lock); > + > + /* Exit if there are several sensors using the FIFO. */ > + if (data->fifo.on > 1) { > + data->fifo.on--; > + return 0; > + } > + > + /* set FIFO in bypass mode */ > + ret = regmap_write(data->map, INV_ICM42370_REG_FIFO_CONFIG1, > + INV_ICM42370_FIFO_CONFIG_BYPASS); > + if (ret) > + return ret; > + > + /* when FIFO is bypassed it gets disabled, so reduce the > + * count > + */ > + data->fifo.on--; > + > + /* flush FIFO data */ > + ret = regmap_write(data->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(data->map, INV_ICM42370_REG_INT_SOURCE0, > + INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN); > + if (ret) > + return ret; > + > + return 0; return regmap_clear_bits() > +} > + > +static int inv_icm42370_buffer_postdisable(struct iio_dev *indio_dev) > +{ > + struct inv_icm42370_data *data = iio_priv(indio_dev); > + struct inv_sensors_timestamp *ts = &data->ts; > + struct device *dev = regmap_get_device(data->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 == data->indio_accel) { > + sensor = INV_ICM42370_SENSOR_ACCEL; > + watermark = &data->fifo.watermark.accel; > + } else { > + return -EINVAL; > + } > + > + guard(mutex)(&data->lock); Read the documentation in cleanup.h. Functions with stuff from that and gotos are a non starter. This one isn't buggy but it is fragile to later code movement. > + > + inv_sensors_timestamp_apply_odr(ts, 0, 0, 0); > + > + ret = inv_icm42370_buffer_set_fifo_en(data, data->fifo.en & ~sensor); > + if (ret) > + goto out_unlock; > + > + *watermark = 0; > + ret = inv_icm42370_buffer_update_watermark(data); > + if (ret) > + goto out_unlock; > + > + conf.mode = INV_ICM42370_SENSOR_MODE_OFF; > + ret = inv_icm42370_set_accel_conf(data, &conf, &sleep_sensor); > + if (ret) > + goto out_unlock; > + > +out_unlock: No lock, so reanme that. > + /* sleep maximum required time */ > + sleep = max(sleep_sensor, sleep_temp); > + if (sleep) > + msleep(sleep); > + > + pm_runtime_put_autosuspend(dev); > + > + return ret; > +} > +int inv_icm42370_buffer_fifo_parse(struct inv_icm42370_data *data) > +{ > + struct inv_sensors_timestamp *ts; > + int ret; > + > + if (data->fifo.nb.total == 0) > + return 0; > + > + /* handle accelerometer timestamp and FIFO data parsing */ > + if (data->fifo.nb.accel > 0) { > + ts = &data->ts; > + inv_sensors_timestamp_interrupt( > + ts, data->fifo.watermark.eff_accel, data->timestamp); Go long on line to format that more nicely > + ret = inv_icm42370_accel_parse_fifo(data->indio_accel); > + if (ret) > + return ret; > + } > + > + return 0; > +} > +int inv_icm42370_buffer_init(struct inv_icm42370_data *data) > +{ > + unsigned int val; > + u8 regval; > + int ret; > + > + data->fifo.watermark.eff_accel = 1; > + > + /* watermark should be set to a non-zero value before enabling interrupts */ > + data->fifo.watermark.accel = 1; > + ret = inv_icm42370_buffer_update_watermark(data); > + 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(data->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(data, INV_ICM42370_MREG1, > + INV_ICM42370_REG_FIFO_CONFIG5, ®val); > + if (ret) > + return ret; > + > + regval &= ~(GENMASK(6, 5) | GENMASK(3, 0)); Those need defines so we know what they are. > + regval |= INV_ICM42370_FIFO_CONFIG5_WM_GT_TH; > + regval |= INV_ICM42370_FIFO_CONFIG5_RESUME_PARTIAL_RD; > + > + return inv_icm42370_mreg_write(data, 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..d43a4209a5640 > --- /dev/null > +++ b/drivers/iio/accel/inv_icm42370_buffer.h ... > + > +/** > + * 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. > + */ > +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; > + u32 period; > + u8 *data __aligned(IIO_DMA_MINALIGN); What do you think that does? It doesn't do anything useful > +}; > diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c > index 6266362e83f6a..b1713a2f533c8 100644 > --- a/drivers/iio/accel/inv_icm42370_core.c > +++ b/drivers/iio/accel/inv_icm42370_core.c > @@ -20,10 +20,25 @@ > #include <linux/types.h> > #include <linux/units.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 "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)) > + Align after ( > @@ -75,6 +92,18 @@ static const int inv_icm42370_accel_scale[] = { > [2 * INV_ICM42370_ACCEL_FS_2G + 1] = 598550, > }; > > +/* > + * 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); aligned_s64 timestamp; > +}; > @@ -1161,15 +1263,54 @@ 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; > + > + guard(mutex)(&st->lock); > + > + st->fifo.watermark.accel = val; > + ret = inv_icm42370_buffer_update_watermark(st); > + > + return ret; return inv_icm... > +} > + > +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); guard(mutex)(&st->lock); > + > + ret = inv_icm42370_buffer_hwfifo_flush(st, count); > + if (!ret) > + ret = st->fifo.nb.accel; if (ret) return ret; return st->fifo.nb.accel; > + > + mutex_unlock(&st->lock); > + > + return ret; > +} > > 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->scales = inv_icm42370_accel_scale; > data->scales_len = ARRAY_SIZE(inv_icm42370_accel_scale); > @@ -1187,13 +1328,64 @@ 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; That's very unlikely as there are a bunch of those modes. No driver should ever set 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); > + > return indio_dev; > } > > +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev) > +{ > + struct inv_icm42370_data *data = iio_priv(indio_dev); > + struct inv_sensors_timestamp *ts = &data->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 = {}; { }; preferred style in IIO (I picked randomly a few years ago!) Reverse xmas tree. > + > + for (i = 0, no = 0; i < data->fifo.count; i += size, ++no) { > + size = inv_icm42370_fifo_decode_packet(&data->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, data->fifo.period, > + data->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; Why put anything at all in there if no temp? > + > + ts_val = inv_sensors_timestamp_pop(ts); > + iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val); iio_push_to_buffers_with_ts() for new code. > + } > + > + return 0; > +} ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality 2026-08-13 12:26 ` [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar 2026-08-13 12:40 ` sashiko-bot 2026-08-16 3:12 ` Jonathan Cameron @ 2026-08-17 9:48 ` Joshua Crofts 2 siblings, 0 replies; 11+ messages in thread From: Joshua Crofts @ 2026-08-17 9:48 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, 13 Aug 2026 14:26:12 +0200 Kanak Shilledar <kanak.shilledar@axis.com> wrote: ... > +static int inv_icm42370_buffer_postdisable(struct iio_dev *indio_dev) > +{ > + struct inv_icm42370_data *data = iio_priv(indio_dev); > + struct inv_sensors_timestamp *ts = &data->ts; > + struct device *dev = regmap_get_device(data->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 == data->indio_accel) { > + sensor = INV_ICM42370_SENSOR_ACCEL; > + watermark = &data->fifo.watermark.accel; You don't need this check, since the ICM42370 is an accelerometer only. (I assume this is still an artifact from the ICM42600 driver you copied from). > + } else { > + return -EINVAL; > + } > + -- Kind regards, Joshua Crofts ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer 2026-08-13 12:26 [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar ` (2 preceding siblings ...) 2026-08-13 12:26 ` [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar @ 2026-08-17 7:19 ` Andy Shevchenko 3 siblings, 0 replies; 11+ messages in thread From: Andy Shevchenko @ 2026-08-17 7:19 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, Aug 13, 2026 at 02:26:09PM +0200, Kanak Shilledar wrote: > 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. > > Thanks to the reviewers for all the time you spent going through the > code and the comments. The changes for v2 are quite a lot and I hope > that every important change is documented in the changelog. After > implementing the suggested changes, the driver is in a far better state > than v1. > [1] https://uat.invensense.com/en-us/products/3-axis/icm-42370-p Can be added as a Datasheet tag here (and be spread by `b4` to all patches in the series, which I think is fine). Can you split the first patch to be based on the features implemented? Something like - patch 0: DT binding - patch 1: very basic (but working) driver - patch 2...n: Features 1...(n-1) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-08-17 9:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-13 12:26 [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer Kanak Shilledar 2026-08-13 12:26 ` [PATCH v2 1/3] dt-bindings: Add InvenSense ICM-42370-p accelerometer Kanak Shilledar 2026-08-13 12:34 ` sashiko-bot 2026-08-13 12:26 ` [PATCH v2 2/3] iio: accel: Add support for ICM42370P Kanak Shilledar 2026-08-13 12:41 ` sashiko-bot 2026-08-16 2:59 ` Jonathan Cameron 2026-08-13 12:26 ` [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality Kanak Shilledar 2026-08-13 12:40 ` sashiko-bot 2026-08-16 3:12 ` Jonathan Cameron 2026-08-17 9:48 ` Joshua Crofts 2026-08-17 7:19 ` [PATCH v2 0/3] Add driver for Invensense ICM42370P accelerometer Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.