Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 8 May 2020 14:28:20 +0100
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
CC:     <jic23@kernel.org>, <robh+dt@kernel.org>, <robh@kernel.org>,
        <mchehab+huawei@kernel.org>, <davem@davemloft.net>,
        <gregkh@linuxfoundation.org>, <linux-iio@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/12] iio: imu: inv_icm42600: add core of new
 inv_icm42600 driver
Message-ID: <20200508142820.000057f8@Huawei.com>
In-Reply-To: <20200507144222.20989-2-jmaneyrol@invensense.com>
References: <20200507144222.20989-1-jmaneyrol@invensense.com>
        <20200507144222.20989-2-jmaneyrol@invensense.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 7 May 2020 16:42:11 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Core component of a new driver for InvenSense ICM-426xx devices.
> It includes registers definition, main probe/setup, and device
> utility functions.
> 
> ICM-426xx devices are latest generation of 6-axis IMU,
> gyroscope+accelerometer and temperature sensor. This device
> includes a 2K FIFO, supports I2C/I3C/SPI, and provides
> intelligent motion features like pedometer, tilt detection,
> and tap detection.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

Hi Jean-Baptiste,

A few minor things inline.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h   | 372 +++++++++++
>  .../iio/imu/inv_icm42600/inv_icm42600_core.c  | 618 ++++++++++++++++++
>  2 files changed, 990 insertions(+)
>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600.h
>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> new file mode 100644
> index 000000000000..8da4c8249aed
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -0,0 +1,372 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#ifndef INV_ICM42600_H_
> +#define INV_ICM42600_H_
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/regmap.h>
> +#include <linux/mutex.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/pm.h>
> +#include <linux/iio/iio.h>
> +
> +enum inv_icm42600_chip {
> +	INV_CHIP_ICM42600,
> +	INV_CHIP_ICM42602,
> +	INV_CHIP_ICM42605,
> +	INV_CHIP_ICM42622,
> +	INV_CHIP_NB,
> +};
> +
> +/* serial bus slew rates */
> +enum inv_icm42600_slew_rate {
> +	INV_ICM42600_SLEW_RATE_20_60NS,
> +	INV_ICM42600_SLEW_RATE_12_36NS,
> +	INV_ICM42600_SLEW_RATE_6_18NS,
> +	INV_ICM42600_SLEW_RATE_4_12NS,
> +	INV_ICM42600_SLEW_RATE_2_6NS,
> +	INV_ICM42600_SLEW_RATE_INF_2NS,
> +};
> +
> +enum inv_icm42600_sensor_mode {
> +	INV_ICM42600_SENSOR_MODE_OFF,
> +	INV_ICM42600_SENSOR_MODE_STANDBY,
> +	INV_ICM42600_SENSOR_MODE_LOW_POWER,
> +	INV_ICM42600_SENSOR_MODE_LOW_NOISE,
> +	INV_ICM42600_SENSOR_MODE_NB,
> +};
> +
> +/* gyroscope fullscale values */
> +enum inv_icm42600_gyro_fs {
> +	INV_ICM42600_GYRO_FS_2000DPS,
> +	INV_ICM42600_GYRO_FS_1000DPS,
> +	INV_ICM42600_GYRO_FS_500DPS,
> +	INV_ICM42600_GYRO_FS_250DPS,
> +	INV_ICM42600_GYRO_FS_125DPS,
> +	INV_ICM42600_GYRO_FS_62_5DPS,
> +	INV_ICM42600_GYRO_FS_31_25DPS,
> +	INV_ICM42600_GYRO_FS_15_625DPS,
> +	INV_ICM42600_GYRO_FS_NB,
> +};
> +
> +/* accelerometer fullscale values */
> +enum inv_icm42600_accel_fs {
> +	INV_ICM42600_ACCEL_FS_16G,
> +	INV_ICM42600_ACCEL_FS_8G,
> +	INV_ICM42600_ACCEL_FS_4G,
> +	INV_ICM42600_ACCEL_FS_2G,
> +	INV_ICM42600_ACCEL_FS_NB,
> +};
> +
> +/* ODR suffixed by LN or LP are Low-Noise or Low-Power mode only */
> +enum inv_icm42600_odr {
> +	INV_ICM42600_ODR_8KHZ_LN = 3,
> +	INV_ICM42600_ODR_4KHZ_LN,
> +	INV_ICM42600_ODR_2KHZ_LN,
> +	INV_ICM42600_ODR_1KHZ_LN,
> +	INV_ICM42600_ODR_200HZ,
> +	INV_ICM42600_ODR_100HZ,
> +	INV_ICM42600_ODR_50HZ,
> +	INV_ICM42600_ODR_25HZ,
> +	INV_ICM42600_ODR_12_5HZ,
> +	INV_ICM42600_ODR_6_25HZ_LP,
> +	INV_ICM42600_ODR_3_125HZ_LP,
> +	INV_ICM42600_ODR_1_5625HZ_LP,
> +	INV_ICM42600_ODR_500HZ,
> +	INV_ICM42600_ODR_NB,
> +};
> +
> +enum inv_icm42600_filter {
> +	/* Low-Noise mode sensor data filter (3rd order filter by default) */
> +	INV_ICM42600_FILTER_BW_ODR_DIV_2,
> +
> +	/* Low-Power mode sensor data filter (averaging) */
> +	INV_ICM42600_FILTER_AVG_1X = 1,
> +	INV_ICM42600_FILTER_AVG_16X = 6,
> +};
> +
> +struct inv_icm42600_sensor_conf {
> +	int mode;
> +	int fs;
> +	int odr;
> +	int filter;
> +};
> +#define INV_ICM42600_SENSOR_CONF_INIT		{-1, -1, -1, -1}
> +
> +struct inv_icm42600_conf {
> +	struct inv_icm42600_sensor_conf gyro;
> +	struct inv_icm42600_sensor_conf accel;
> +	bool temp_en;
> +};
> +
> +struct inv_icm42600_suspended {
> +	enum inv_icm42600_sensor_mode gyro;
> +	enum inv_icm42600_sensor_mode accel;
> +	bool temp;
> +};
> +
> +/*
/**

It's valid kernel doc so lets mark it as such.

> + *  struct inv_icm42600_state - driver state variables
> + *  @lock:		chip access lock.

Nice to be a bit more specific on that.  What about the chip needs
a lock at this level as opposed to bus locks etc?

> + *  @chip:		chip identifier.
> + *  @name:		chip name.
> + *  @map:		regmap pointer.
> + *  @vdd_supply:	VDD voltage regulator for the chip.
> + *  @vddio_supply:	I/O voltage regulator for the chip.
> + *  @orientation:	sensor chip orientation relative to main hardware.
> + *  @conf:		chip sensors configurations.
> + *  @suspended:		suspended sensors configuration.
> + */
> +struct inv_icm42600_state {
> +	struct mutex lock;
> +	enum inv_icm42600_chip chip;
> +	const char *name;
> +	struct regmap *map;
> +	struct regulator *vdd_supply;
> +	struct regulator *vddio_supply;
> +	struct iio_mount_matrix orientation;
> +	struct inv_icm42600_conf conf;
> +	struct inv_icm42600_suspended suspended;
> +};
> +
> +/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
> +
> +/* Bank selection register, available in all banks */
> +#define INV_ICM42600_REG_BANK_SEL			0x76
> +#define INV_ICM42600_BANK_SEL_MASK			GENMASK(2, 0)
> +
> +/* User bank 0 (MSB 0x00) */
> +#define INV_ICM42600_REG_DEVICE_CONFIG			0x0011
> +#define INV_ICM42600_DEVICE_CONFIG_SOFT_RESET		BIT(0)
> +
> +#define INV_ICM42600_REG_DRIVE_CONFIG			0x0013
> +#define INV_ICM42600_DRIVE_CONFIG_I2C_MASK		GENMASK(5, 3)
> +#define INV_ICM42600_DRIVE_CONFIG_I2C(_rate)		\
> +		FIELD_PREP(INV_ICM42600_DRIVE_CONFIG_I2C_MASK, (_rate))
> +#define INV_ICM42600_DRIVE_CONFIG_SPI_MASK		GENMASK(2, 0)
> +#define INV_ICM42600_DRIVE_CONFIG_SPI(_rate)		\
> +		FIELD_PREP(INV_ICM42600_DRIVE_CONFIG_SPI_MASK, (_rate))
> +
> +#define INV_ICM42600_REG_INT_CONFIG			0x0014
> +#define INV_ICM42600_INT_CONFIG_INT2_LATCHED		BIT(5)
> +#define INV_ICM42600_INT_CONFIG_INT2_PUSH_PULL		BIT(4)
> +#define INV_ICM42600_INT_CONFIG_INT2_ACTIVE_HIGH	BIT(3)
> +#define INV_ICM42600_INT_CONFIG_INT2_ACTIVE_LOW		0x00
> +#define INV_ICM42600_INT_CONFIG_INT1_LATCHED		BIT(2)
> +#define INV_ICM42600_INT_CONFIG_INT1_PUSH_PULL		BIT(1)
> +#define INV_ICM42600_INT_CONFIG_INT1_ACTIVE_HIGH	BIT(0)
> +#define INV_ICM42600_INT_CONFIG_INT1_ACTIVE_LOW		0x00
> +
> +#define INV_ICM42600_REG_FIFO_CONFIG			0x0016
> +#define INV_ICM42600_FIFO_CONFIG_MASK			GENMASK(7, 6)
> +#define INV_ICM42600_FIFO_CONFIG_BYPASS			\
> +		FIELD_PREP(INV_ICM42600_FIFO_CONFIG_MASK, 0)
> +#define INV_ICM42600_FIFO_CONFIG_STREAM			\
> +		FIELD_PREP(INV_ICM42600_FIFO_CONFIG_MASK, 1)
> +#define INV_ICM42600_FIFO_CONFIG_STOP_ON_FULL		\
> +		FIELD_PREP(INV_ICM42600_FIFO_CONFIG_MASK, 2)
> +
> +/* all sensor data are 16 bits (2 registers wide) in big-endian */
> +#define INV_ICM42600_REG_TEMP_DATA			0x001D
> +#define INV_ICM42600_REG_ACCEL_DATA_X			0x001F
> +#define INV_ICM42600_REG_ACCEL_DATA_Y			0x0021
> +#define INV_ICM42600_REG_ACCEL_DATA_Z			0x0023
> +#define INV_ICM42600_REG_GYRO_DATA_X			0x0025
> +#define INV_ICM42600_REG_GYRO_DATA_Y			0x0027
> +#define INV_ICM42600_REG_GYRO_DATA_Z			0x0029
> +#define INV_ICM42600_DATA_INVALID			-32768
> +
> +#define INV_ICM42600_REG_INT_STATUS			0x002D
> +#define INV_ICM42600_INT_STATUS_UI_FSYNC		BIT(6)
> +#define INV_ICM42600_INT_STATUS_PLL_RDY			BIT(5)
> +#define INV_ICM42600_INT_STATUS_RESET_DONE		BIT(4)
> +#define INV_ICM42600_INT_STATUS_DATA_RDY		BIT(3)
> +#define INV_ICM42600_INT_STATUS_FIFO_THS		BIT(2)
> +#define INV_ICM42600_INT_STATUS_FIFO_FULL		BIT(1)
> +#define INV_ICM42600_INT_STATUS_AGC_RDY			BIT(0)
> +
> +/*
> + * FIFO access registers
> + * FIFO count is 16 bits (2 registers) big-endian
> + * FIFO data is a continuous read register to read FIFO content
> + */
> +#define INV_ICM42600_REG_FIFO_COUNT			0x002E
> +#define INV_ICM42600_REG_FIFO_DATA			0x0030
> +
> +#define INV_ICM42600_REG_SIGNAL_PATH_RESET		0x004B
> +#define INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN	BIT(6)
> +#define INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET	BIT(5)
> +#define INV_ICM42600_SIGNAL_PATH_RESET_RESET		BIT(3)
> +#define INV_ICM42600_SIGNAL_PATH_RESET_TMST_STROBE	BIT(2)
> +#define INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH	BIT(1)
> +
> +/* default configuration: all data big-endian and fifo count in bytes */
> +#define INV_ICM42600_REG_INTF_CONFIG0			0x004C
> +#define INV_ICM42600_INTF_CONFIG0_FIFO_HOLD_LAST_DATA	BIT(7)
> +#define INV_ICM42600_INTF_CONFIG0_FIFO_COUNT_REC	BIT(6)
> +#define INV_ICM42600_INTF_CONFIG0_FIFO_COUNT_ENDIAN	BIT(5)
> +#define INV_ICM42600_INTF_CONFIG0_SENSOR_DATA_ENDIAN	BIT(4)
> +#define INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK	GENMASK(1, 0)
> +#define INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS	\
> +		FIELD_PREP(INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK, 2)
> +#define INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_I2C_DIS	\
> +		FIELD_PREP(INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK, 3)
> +
> +#define INV_ICM42600_REG_INTF_CONFIG1			0x004D
> +#define INV_ICM42600_INTF_CONFIG1_ACCEL_LP_CLK_RC	BIT(3)
> +
> +#define INV_ICM42600_REG_PWR_MGMT0			0x004E
> +#define INV_ICM42600_PWR_MGMT0_TEMP_DIS			BIT(5)
> +#define INV_ICM42600_PWR_MGMT0_IDLE			BIT(4)
> +#define INV_ICM42600_PWR_MGMT0_GYRO(_mode)		\
> +		FIELD_PREP(GENMASK(3, 2), (_mode))
> +#define INV_ICM42600_PWR_MGMT0_ACCEL(_mode)		\
> +		FIELD_PREP(GENMASK(1, 0), (_mode))
> +
> +#define INV_ICM42600_REG_GYRO_CONFIG0			0x004F
> +#define INV_ICM42600_GYRO_CONFIG0_FS(_fs)		\
> +		FIELD_PREP(GENMASK(7, 5), (_fs))
> +#define INV_ICM42600_GYRO_CONFIG0_ODR(_odr)		\
> +		FIELD_PREP(GENMASK(3, 0), (_odr))
> +
> +#define INV_ICM42600_REG_ACCEL_CONFIG0			0x0050
> +#define INV_ICM42600_ACCEL_CONFIG0_FS(_fs)		\
> +		FIELD_PREP(GENMASK(7, 5), (_fs))
> +#define INV_ICM42600_ACCEL_CONFIG0_ODR(_odr)		\
> +		FIELD_PREP(GENMASK(3, 0), (_odr))
> +
> +#define INV_ICM42600_REG_GYRO_ACCEL_CONFIG0		0x0052
> +#define INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(_f)	\
> +		FIELD_PREP(GENMASK(7, 4), (_f))
> +#define INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(_f)	\
> +		FIELD_PREP(GENMASK(3, 0), (_f))
> +
> +#define INV_ICM42600_REG_TMST_CONFIG			0x0054
> +#define INV_ICM42600_TMST_CONFIG_MASK			GENMASK(4, 0)
> +#define INV_ICM42600_TMST_CONFIG_TMST_TO_REGS_EN	BIT(4)
> +#define INV_ICM42600_TMST_CONFIG_TMST_RES_16US		BIT(3)
> +#define INV_ICM42600_TMST_CONFIG_TMST_DELTA_EN		BIT(2)
> +#define INV_ICM42600_TMST_CONFIG_TMST_FSYNC_EN		BIT(1)
> +#define INV_ICM42600_TMST_CONFIG_TMST_EN		BIT(0)
> +
> +#define INV_ICM42600_REG_FIFO_CONFIG1			0x005F
> +#define INV_ICM42600_FIFO_CONFIG1_RESUME_PARTIAL_RD	BIT(6)
> +#define INV_ICM42600_FIFO_CONFIG1_WM_GT_TH		BIT(5)
> +#define INV_ICM42600_FIFO_CONFIG1_TMST_FSYNC_EN		BIT(3)
> +#define INV_ICM42600_FIFO_CONFIG1_TEMP_EN		BIT(2)
> +#define INV_ICM42600_FIFO_CONFIG1_GYRO_EN		BIT(1)
> +#define INV_ICM42600_FIFO_CONFIG1_ACCEL_EN		BIT(0)
> +
> +/* FIFO watermark is 16 bits (2 registers wide) in little-endian */
> +#define INV_ICM42600_REG_FIFO_WATERMARK			0x0060
> +#define INV_ICM42600_FIFO_WATERMARK_VAL(_wm)		\
> +		cpu_to_le16((_wm) & GENMASK(11, 0))
> +/* FIFO is 2048 bytes, let 12 samples for reading latency */
> +#define INV_ICM42600_FIFO_WATERMARK_MAX			(2048 - 12 * 16)
> +
> +#define INV_ICM42600_REG_INT_CONFIG1			0x0064
> +#define INV_ICM42600_INT_CONFIG1_TPULSE_DURATION	BIT(6)
> +#define INV_ICM42600_INT_CONFIG1_TDEASSERT_DISABLE	BIT(5)
> +#define INV_ICM42600_INT_CONFIG1_ASYNC_RESET		BIT(4)
> +
> +#define INV_ICM42600_REG_INT_SOURCE0			0x0065
> +#define INV_ICM42600_INT_SOURCE0_UI_FSYNC_INT1_EN	BIT(6)
> +#define INV_ICM42600_INT_SOURCE0_PLL_RDY_INT1_EN	BIT(5)
> +#define INV_ICM42600_INT_SOURCE0_RESET_DONE_INT1_EN	BIT(4)
> +#define INV_ICM42600_INT_SOURCE0_UI_DRDY_INT1_EN	BIT(3)
> +#define INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN	BIT(2)
> +#define INV_ICM42600_INT_SOURCE0_FIFO_FULL_INT1_EN	BIT(1)
> +#define INV_ICM42600_INT_SOURCE0_UI_AGC_RDY_INT1_EN	BIT(0)
> +
> +#define INV_ICM42600_REG_WHOAMI				0x0075
> +#define INV_ICM42600_WHOAMI_ICM42600			0x40
> +#define INV_ICM42600_WHOAMI_ICM42602			0x41
> +#define INV_ICM42600_WHOAMI_ICM42605			0x42
> +#define INV_ICM42600_WHOAMI_ICM42622			0x46
> +
> +/* User bank 1 (MSB 0x10) */
> +#define INV_ICM42600_REG_SENSOR_CONFIG0			0x1003
> +#define INV_ICM42600_SENSOR_CONFIG0_ZG_DISABLE		BIT(5)
> +#define INV_ICM42600_SENSOR_CONFIG0_YG_DISABLE		BIT(4)
> +#define INV_ICM42600_SENSOR_CONFIG0_XG_DISABLE		BIT(3)
> +#define INV_ICM42600_SENSOR_CONFIG0_ZA_DISABLE		BIT(2)
> +#define INV_ICM42600_SENSOR_CONFIG0_YA_DISABLE		BIT(1)
> +#define INV_ICM42600_SENSOR_CONFIG0_XA_DISABLE		BIT(0)
> +
> +/* Timestamp value is 20 bits (3 registers) in little-endian */
> +#define INV_ICM42600_REG_TMSTVAL			0x1062
> +#define INV_ICM42600_TMSTVAL_MASK			GENMASK(19, 0)
> +
> +#define INV_ICM42600_REG_INTF_CONFIG4			0x107A
> +#define INV_ICM42600_INTF_CONFIG4_I3C_BUS_ONLY		BIT(6)
> +#define INV_ICM42600_INTF_CONFIG4_SPI_AP_4WIRE		BIT(1)
> +
> +#define INV_ICM42600_REG_INTF_CONFIG6			0x107C
> +#define INV_ICM42600_INTF_CONFIG6_MASK			GENMASK(4, 0)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_EN		BIT(4)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_IBI_BYTE_EN	BIT(3)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_IBI_EN		BIT(2)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_DDR_EN		BIT(1)
> +#define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN		BIT(0)
> +
> +/* User bank 4 (MSB 0x40) */
> +#define INV_ICM42600_REG_INT_SOURCE8			0x404F
> +#define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN		BIT(5)
> +#define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN		BIT(4)
> +#define INV_ICM42600_INT_SOURCE8_UI_DRDY_IBI_EN		BIT(3)
> +#define INV_ICM42600_INT_SOURCE8_FIFO_THS_IBI_EN	BIT(2)
> +#define INV_ICM42600_INT_SOURCE8_FIFO_FULL_IBI_EN	BIT(1)
> +#define INV_ICM42600_INT_SOURCE8_AGC_RDY_IBI_EN		BIT(0)
> +
> +#define INV_ICM42600_REG_OFFSET_USER0			0x4077
> +#define INV_ICM42600_REG_OFFSET_USER1			0x4078
> +#define INV_ICM42600_REG_OFFSET_USER2			0x4079
> +#define INV_ICM42600_REG_OFFSET_USER3			0x407A
> +#define INV_ICM42600_REG_OFFSET_USER4			0x407B
> +#define INV_ICM42600_REG_OFFSET_USER5			0x407C
> +#define INV_ICM42600_REG_OFFSET_USER6			0x407D
> +#define INV_ICM42600_REG_OFFSET_USER7			0x407E
> +#define INV_ICM42600_REG_OFFSET_USER8			0x407F
> +
> +/* Sleep times required by the driver */
> +#define INV_ICM42600_POWER_UP_TIME_MS		100
> +#define INV_ICM42600_RESET_TIME_MS		1
> +#define INV_ICM42600_ACCEL_STARTUP_TIME_MS	20
> +#define INV_ICM42600_GYRO_STARTUP_TIME_MS	60
> +#define INV_ICM42600_GYRO_STOP_TIME_MS		150
> +#define INV_ICM42600_TEMP_STARTUP_TIME_MS	14
> +#define INV_ICM42600_SUSPEND_DELAY_MS		2000
> +
> +typedef int (*inv_icm42600_bus_setup)(struct inv_icm42600_state *);
> +
> +extern const struct regmap_config inv_icm42600_regmap_config;
> +extern const struct dev_pm_ops inv_icm42600_pm_ops;
> +
> +const struct iio_mount_matrix *
> +inv_icm42600_get_mount_matrix(const struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan);
> +
> +uint32_t inv_icm42600_odr_to_period(enum inv_icm42600_odr odr);
> +
> +int inv_icm42600_set_accel_conf(struct inv_icm42600_state *st,
> +				struct inv_icm42600_sensor_conf *conf,
> +				unsigned int *sleep);
> +
> +int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
> +			       struct inv_icm42600_sensor_conf *conf,
> +			       unsigned int *sleep);
> +
> +int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
> +			       unsigned int *sleep);
> +
> +int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> +			     unsigned int writeval, unsigned int *readval);
> +
> +int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> +			    inv_icm42600_bus_setup bus_setup);
> +
> +#endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> new file mode 100644
> index 000000000000..35bdf4f9d31e
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -0,0 +1,618 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +
> +#include "inv_icm42600.h"
> +
> +static const struct regmap_range_cfg inv_icm42600_regmap_ranges[] = {
> +	{
> +		.name = "user banks",
> +		.range_min = 0x0000,
> +		.range_max = 0x4FFF,
> +		.selector_reg = INV_ICM42600_REG_BANK_SEL,
> +		.selector_mask = INV_ICM42600_BANK_SEL_MASK,
> +		.selector_shift = 0,
> +		.window_start = 0,
> +		.window_len = 0x1000,
> +	},
> +};
> +
> +const struct regmap_config inv_icm42600_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x4FFF,
> +	.ranges = inv_icm42600_regmap_ranges,
> +	.num_ranges = ARRAY_SIZE(inv_icm42600_regmap_ranges),
> +};
> +EXPORT_SYMBOL_GPL(inv_icm42600_regmap_config);
> +
> +struct inv_icm42600_hw {
> +	uint8_t whoami;
> +	const char *name;
> +	const struct inv_icm42600_conf *conf;
> +};
> +
> +/* chip initial default configuration */
> +static const struct inv_icm42600_conf inv_icm42600_default_conf = {
> +	.gyro = {
> +		.mode = INV_ICM42600_SENSOR_MODE_OFF,
> +		.fs = INV_ICM42600_GYRO_FS_2000DPS,
> +		.odr = INV_ICM42600_ODR_50HZ,
> +		.filter = INV_ICM42600_FILTER_BW_ODR_DIV_2,
> +	},
> +	.accel = {
> +		.mode = INV_ICM42600_SENSOR_MODE_OFF,
> +		.fs = INV_ICM42600_ACCEL_FS_16G,
> +		.odr = INV_ICM42600_ODR_50HZ,
> +		.filter = INV_ICM42600_FILTER_BW_ODR_DIV_2,
> +	},
> +	.temp_en = false,
> +};
> +
> +static const struct inv_icm42600_hw inv_icm42600_hw[] = {
> +	[INV_CHIP_ICM42600] = {
> +		.whoami = INV_ICM42600_WHOAMI_ICM42600,
> +		.name = "icm42600",
> +		.conf = &inv_icm42600_default_conf,
> +	},
> +	[INV_CHIP_ICM42602] = {
> +		.whoami = INV_ICM42600_WHOAMI_ICM42602,
> +		.name = "icm42602",
> +		.conf = &inv_icm42600_default_conf,
> +	},
> +	[INV_CHIP_ICM42605] = {
> +		.whoami = INV_ICM42600_WHOAMI_ICM42605,
> +		.name = "icm42605",
> +		.conf = &inv_icm42600_default_conf,
> +	},
> +	[INV_CHIP_ICM42622] = {
> +		.whoami = INV_ICM42600_WHOAMI_ICM42622,
> +		.name = "icm42622",
> +		.conf = &inv_icm42600_default_conf,
> +	},
> +};
> +
> +const struct iio_mount_matrix *
> +inv_icm42600_get_mount_matrix(const struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan)
> +{
> +	const struct inv_icm42600_state *st =
> +			iio_device_get_drvdata((struct iio_dev *)indio_dev);

Interesting... iio_device_get_drvdata is never going to modify
the struct iio_dev.  Should we just change that to take a
const struct iio_dev * ?

> +
> +	return &st->orientation;
> +}
> +
> +uint32_t inv_icm42600_odr_to_period(enum inv_icm42600_odr odr)
> +{
> +	static uint32_t odr_periods[INV_ICM42600_ODR_NB] = {
> +		/* reserved values */
> +		0, 0, 0,
> +		/* 8kHz */
> +		125000,
> +		/* 4kHz */
> +		250000,
> +		/* 2kHz */
> +		500000,
> +		/* 1kHz */
> +		1000000,
> +		/* 200Hz */
> +		5000000,
> +		/* 100Hz */
> +		10000000,
> +		/* 50Hz */
> +		20000000,
> +		/* 25Hz */
> +		40000000,
> +		/* 12.5Hz */
> +		80000000,
> +		/* 6.25Hz */
> +		160000000,
> +		/* 3.125Hz */
> +		320000000,
> +		/* 1.5625Hz */
> +		640000000,
> +		/* 500Hz */
> +		2000000,
> +	};
> +
> +	return odr_periods[odr];
> +}
> +
> +static int inv_icm42600_set_pwr_mgmt0(struct inv_icm42600_state *st,
> +				      enum inv_icm42600_sensor_mode gyro,
> +				      enum inv_icm42600_sensor_mode accel,
> +				      bool temp, unsigned int *sleep)

msleep or similar that indicates the units of the sleep time.

> +{
> +	enum inv_icm42600_sensor_mode oldgyro = st->conf.gyro.mode;
> +	enum inv_icm42600_sensor_mode oldaccel = st->conf.accel.mode;
> +	bool oldtemp = st->conf.temp_en;
> +	unsigned int sleepval;
> +	unsigned int val;
> +	int ret;
> +
> +	/* if nothing changed, exit */
> +	if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
> +		return 0;
> +
> +	val = INV_ICM42600_PWR_MGMT0_GYRO(gyro) |
> +	      INV_ICM42600_PWR_MGMT0_ACCEL(accel);
> +	if (!temp)
> +		val |= INV_ICM42600_PWR_MGMT0_TEMP_DIS;
> +	dev_dbg(regmap_get_device(st->map), "pwr_mgmt0: %#02x\n", val);

I wonder if you have a little too much in the way of debug prints.
These are internal to the code and so could only be wrong due to a local
bug.  Once you've finished writing the driver I'd hope we won't need these!

> +	ret = regmap_write(st->map, INV_ICM42600_REG_PWR_MGMT0, val);
> +	if (ret)
> +		return ret;
> +
> +	st->conf.gyro.mode = gyro;
> +	st->conf.accel.mode = accel;
> +	st->conf.temp_en = temp;
> +
> +	/* compute required wait time for sensors to stabilize */
> +	sleepval = 0;
> +	/* temperature stabilization time */
> +	if (temp && !oldtemp) {
> +		if (sleepval < INV_ICM42600_TEMP_STARTUP_TIME_MS)
> +			sleepval = INV_ICM42600_TEMP_STARTUP_TIME_MS;
> +	}
> +	/* accel startup time */
> +	if (accel != oldaccel && oldaccel == INV_ICM42600_SENSOR_MODE_OFF) {
> +		/* block any register write for at least 200 ?s */
> +		usleep_range(200, 300);
> +		if (sleepval < INV_ICM42600_ACCEL_STARTUP_TIME_MS)
> +			sleepval = INV_ICM42600_ACCEL_STARTUP_TIME_MS;
> +	}
> +	if (gyro != oldgyro) {
> +		/* gyro startup time */
> +		if (oldgyro == INV_ICM42600_SENSOR_MODE_OFF) {
> +			/* block any register write for at least 200 ?s */
> +			usleep_range(200, 300);
> +			if (sleepval < INV_ICM42600_GYRO_STARTUP_TIME_MS)
> +				sleepval = INV_ICM42600_GYRO_STARTUP_TIME_MS;
> +		/* gyro stop time */
> +		} else if (gyro == INV_ICM42600_SENSOR_MODE_OFF) {
> +			if (sleepval < INV_ICM42600_GYRO_STOP_TIME_MS)
> +				sleepval =  INV_ICM42600_GYRO_STOP_TIME_MS;
> +		}
> +	}
> +
> +	/* deferred sleep value if sleep pointer is provided or direct sleep */
> +	if (sleep)
> +		*sleep = sleepval;
> +	else if (sleepval)
> +		msleep(sleepval);
> +
> +	return 0;
> +}
> +
> +int inv_icm42600_set_accel_conf(struct inv_icm42600_state *st,
> +				struct inv_icm42600_sensor_conf *conf,
> +				unsigned int *sleep)
> +{
> +	struct inv_icm42600_sensor_conf *oldconf = &st->conf.accel;
> +	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;
> +
> +	/* set ACCEL_CONFIG0 register (accel fullscale & odr) */
> +	if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> +		val = INV_ICM42600_ACCEL_CONFIG0_FS(conf->fs) |
> +		      INV_ICM42600_ACCEL_CONFIG0_ODR(conf->odr);
> +		dev_dbg(regmap_get_device(st->map), "accel_config0: %#02x\n", val);
> +		ret = regmap_write(st->map, INV_ICM42600_REG_ACCEL_CONFIG0, val);
> +		if (ret)
> +			return ret;
> +		oldconf->fs = conf->fs;
> +		oldconf->odr = conf->odr;
> +	}
> +
> +	/* set GYRO_ACCEL_CONFIG0 register (accel filter) */
> +	if (conf->filter != oldconf->filter) {
> +		val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(conf->filter) |
> +		      INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(st->conf.gyro.filter);
> +		dev_dbg(regmap_get_device(st->map), "gyro_accel_config0: %#02x\n", val);
> +		ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_ACCEL_CONFIG0, val);
> +		if (ret)
> +			return ret;
> +		oldconf->filter = conf->filter;
> +	}
> +
> +	/* set PWR_MGMT0 register (accel sensor mode) */
> +	return inv_icm42600_set_pwr_mgmt0(st, st->conf.gyro.mode, conf->mode,
> +					  st->conf.temp_en, sleep);
> +}
> +
> +int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
> +			       struct inv_icm42600_sensor_conf *conf,
> +			       unsigned int *sleep)
> +{
> +	struct inv_icm42600_sensor_conf *oldconf = &st->conf.gyro;
> +	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;
> +
> +	/* set GYRO_CONFIG0 register (gyro fullscale & odr) */
> +	if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> +		val = INV_ICM42600_GYRO_CONFIG0_FS(conf->fs) |
> +		      INV_ICM42600_GYRO_CONFIG0_ODR(conf->odr);
> +		dev_dbg(regmap_get_device(st->map), "gyro_config0: %#02x\n", val);
> +		ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_CONFIG0, val);
> +		if (ret)
> +			return ret;
> +		oldconf->fs = conf->fs;
> +		oldconf->odr = conf->odr;
> +	}
> +
> +	/* set GYRO_ACCEL_CONFIG0 register (gyro filter) */
> +	if (conf->filter != oldconf->filter) {
> +		val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(st->conf.accel.filter) |
> +		      INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(conf->filter);
> +		dev_dbg(regmap_get_device(st->map), "gyro_accel_config0: %#02x\n", val);
> +		ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_ACCEL_CONFIG0, val);
> +		if (ret)
> +			return ret;
> +		oldconf->filter = conf->filter;
> +	}
> +
> +	/* set PWR_MGMT0 register (gyro sensor mode) */
> +	return inv_icm42600_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode,
> +					  st->conf.temp_en, sleep);
> +
> +	return 0;
> +}
> +
> +int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
> +			       unsigned int *sleep)
> +{
> +	return inv_icm42600_set_pwr_mgmt0(st, st->conf.gyro.mode,
> +					  st->conf.accel.mode, enable,
> +					  sleep);
> +}
> +
> +int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> +			     unsigned int writeval, unsigned int *readval)
> +{
> +	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	if (readval)
> +		ret = regmap_read(st->map, reg, readval);
> +	else
> +		ret = regmap_write(st->map, reg, writeval);
> +
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int inv_icm42600_set_conf(struct inv_icm42600_state *st,
> +				 const struct inv_icm42600_conf *conf)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	/* set PWR_MGMT0 register (gyro & accel sensor mode, temp enabled) */
> +	val = INV_ICM42600_PWR_MGMT0_GYRO(conf->gyro.mode) |
> +	      INV_ICM42600_PWR_MGMT0_ACCEL(conf->accel.mode);
> +	if (!conf->temp_en)
> +		val |= INV_ICM42600_PWR_MGMT0_TEMP_DIS;
> +	ret = regmap_write(st->map, INV_ICM42600_REG_PWR_MGMT0, val);
> +	if (ret)
> +		return ret;
> +
> +	/* set GYRO_CONFIG0 register (gyro fullscale & odr) */
> +	val = INV_ICM42600_GYRO_CONFIG0_FS(conf->gyro.fs) |
> +	      INV_ICM42600_GYRO_CONFIG0_ODR(conf->gyro.odr);
> +	ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_CONFIG0, val);
> +	if (ret)
> +		return ret;
> +
> +	/* set ACCEL_CONFIG0 register (accel fullscale & odr) */
> +	val = INV_ICM42600_ACCEL_CONFIG0_FS(conf->accel.fs) |
> +	      INV_ICM42600_ACCEL_CONFIG0_ODR(conf->accel.odr);
> +	ret = regmap_write(st->map, INV_ICM42600_REG_ACCEL_CONFIG0, val);
> +	if (ret)
> +		return ret;
> +
> +	/* set GYRO_ACCEL_CONFIG0 register (gyro & accel filters) */
> +	val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(conf->accel.filter) |
> +	      INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(conf->gyro.filter);
> +	ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_ACCEL_CONFIG0, val);
> +	if (ret)
> +		return ret;
> +
> +	/* update internal conf */
> +	st->conf = *conf;
> +
> +	return 0;
> +}
> +
> +/**
> + *  inv_icm42600_setup() - check and setup chip.

If doing kernel-doc (which is good) you should do it all.
So document the parameters as well.
It's worth running the kernel-doc script over any file where
you put some and fixing up any warnings / errors.

> + */
> +static int inv_icm42600_setup(struct inv_icm42600_state *st,
> +			      inv_icm42600_bus_setup bus_setup)
> +{
> +	const struct inv_icm42600_hw *hw = &inv_icm42600_hw[st->chip];
> +	const struct device *dev = regmap_get_device(st->map);
> +	unsigned int mask, val;
> +	int ret;
> +
> +	/* check chip self-identification value */
> +	ret = regmap_read(st->map, INV_ICM42600_REG_WHOAMI, &val);
> +	if (ret)
> +		return ret;
> +	if (val != hw->whoami) {
> +		dev_err(dev, "invalid whoami %#02x expected %#02x (%s)\n",
> +			val, hw->whoami, hw->name);
> +		return -ENODEV;
> +	}
> +	dev_info(dev, "found %s (%#02x)\n", hw->name, hw->whoami);

Hmm. I'm never that keen on this sort of log noise.  Why do you need it
except for initial debug?

> +	st->name = hw->name;
> +
> +	/* reset to make sure previous state are not there */
> +	ret = regmap_write(st->map, INV_ICM42600_REG_DEVICE_CONFIG,
> +			   INV_ICM42600_DEVICE_CONFIG_SOFT_RESET);
> +	if (ret)
> +		return ret;
> +	msleep(INV_ICM42600_RESET_TIME_MS);

blank line here to separate two logical blocks of code.
Slightly helps readability.

> +	ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &val);
> +	if (ret)
> +		return ret;
> +	if (!(val & INV_ICM42600_INT_STATUS_RESET_DONE)) {
> +		dev_err(dev, "reset error, reset done bit not set\n");
> +		return -ENODEV;
> +	}
> +
> +	/* set chip bus configuration */
> +	ret = bus_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	/* sensor data in big-endian (default) */
> +	mask = INV_ICM42600_INTF_CONFIG0_SENSOR_DATA_ENDIAN;
> +	val = INV_ICM42600_INTF_CONFIG0_SENSOR_DATA_ENDIAN;
> +	ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG0,
> +				 mask, val);

Long line, but I'd rather you just didn't bother will local variables
in cases like this where you just set them to a constant.
Take the 80 chars thing as guidance not a rule :)

> +	if (ret)
> +		return ret;
> +
> +	return inv_icm42600_set_conf(st, hw->conf);
> +}
> +
> +static int inv_icm42600_enable_regulator_vddio(struct inv_icm42600_state *st)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(st->vddio_supply);
> +	if (ret)
> +		return ret;
> +
> +	/* wait a little for supply ramp */
> +	usleep_range(3000, 4000);
> +
> +	return 0;
> +}
> +
> +static void inv_icm42600_disable_regulators(void *_data)
> +{
> +	struct inv_icm42600_state *st = _data;
> +	const struct device *dev = regmap_get_device(st->map);
> +	int ret;
> +
> +	ret = regulator_disable(st->vddio_supply);
> +	if (ret)
> +		dev_err(dev, "failed to disable vddio error %d\n", ret);
> +
> +	ret = regulator_disable(st->vdd_supply);
> +	if (ret)
> +		dev_err(dev, "failed to disable vdd error %d\n", ret);
> +}
> +
> +static void inv_icm42600_disable_pm(void *_data)
> +{
> +	struct device *dev = _data;
> +
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +}
> +
> +int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> +			    inv_icm42600_bus_setup bus_setup)
> +{
> +	struct device *dev = regmap_get_device(regmap);
> +	struct inv_icm42600_state *st;
> +	int ret;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(inv_icm42600_hw) != INV_CHIP_NB);

Why not just give the array an explicit size when you define it above?
I guess it would in theory be possible to not instantiate all of the array
but relying on different size of a variable length array seems less than
ideal.

> +	if (chip < 0 || chip >= INV_CHIP_NB) {
> +		dev_err(dev, "invalid chip = %d\n", chip);
> +		return -ENODEV;
> +	}
> +
> +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
nitpick: blank line here.

> +	dev_set_drvdata(dev, st);
> +	mutex_init(&st->lock);
> +	st->chip = chip;
> +	st->map = regmap;
> +
> +	ret = iio_read_mount_matrix(dev, "mount-matrix", &st->orientation);
> +	if (ret) {
> +		dev_err(dev, "failed to retrieve mounting matrix %d\n", ret);
> +		return ret;
> +	}
> +
> +	st->vdd_supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(st->vdd_supply))
> +		return PTR_ERR(st->vdd_supply);
> +
> +	st->vddio_supply = devm_regulator_get(dev, "vddio");
> +	if (IS_ERR(st->vddio_supply))
> +		return PTR_ERR(st->vddio_supply);
> +
> +	ret = regulator_enable(st->vdd_supply);
> +	if (ret)
> +		return ret;
> +	msleep(INV_ICM42600_POWER_UP_TIME_MS);
> +
> +	ret = inv_icm42600_enable_regulator_vddio(st);
> +	if (ret) {
> +		regulator_disable(st->vdd_supply);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_regulators,
> +				       st);

I'd prefer to see two devm_add_action_or_reset calls. One for each regulator.
That means you don't have to do the extra disable logic above which is
a bit fragile in amongst a whole load of device managed calls.

> +	if (ret)
> +		return ret;
> +
> +	/* setup chip registers */
> +	ret = inv_icm42600_setup(st, bus_setup);
> +	if (ret)
> +		return ret;
> +
> +	/* setup runtime power management */
> +	ret = pm_runtime_set_active(dev);
> +	if (ret)
> +		return ret;
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put(dev);
> +
> +	return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
> +}
> +EXPORT_SYMBOL_GPL(inv_icm42600_core_probe);
> +
> +static int __maybe_unused inv_icm42600_suspend(struct device *dev)
> +{
> +	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	st->suspended.gyro = st->conf.gyro.mode;
> +	st->suspended.accel = st->conf.accel.mode;
> +	st->suspended.temp = st->conf.temp_en;
> +	if (pm_runtime_suspended(dev)) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
> +
> +	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> +					 INV_ICM42600_SENSOR_MODE_OFF, false,
> +					 NULL);
> +	if (ret)
> +		goto out_unlock;
> +
> +	regulator_disable(st->vddio_supply);
> +
> +out_unlock:
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +static int __maybe_unused inv_icm42600_resume(struct device *dev)
> +{
> +	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = inv_icm42600_enable_regulator_vddio(st);
> +	if (ret)
> +		goto out_unlock;
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	/* restore sensors state */
> +	ret = inv_icm42600_set_pwr_mgmt0(st, st->suspended.gyro,
> +					 st->suspended.accel,
> +					 st->suspended.temp, NULL);
> +	if (ret)
> +		goto out_unlock;

You may need this later, but for now it's a bit comic so ideally introduce
it only when needed.

> +
> +out_unlock:
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +static int __maybe_unused inv_icm42600_runtime_suspend(struct device *dev)
> +{
> +	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	/* disable all sensors */
> +	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> +					 INV_ICM42600_SENSOR_MODE_OFF, false,
> +					 NULL);
> +	if (ret)
> +		goto error_unlock;
> +
> +	regulator_disable(st->vddio_supply);
> +
> +error_unlock:
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +static int __maybe_unused inv_icm42600_runtime_resume(struct device *dev)
> +{
> +	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +

Why don't we need to reenable all the sensors we disabled in runtime suspend?
I can guess why we might not, but a comment here to explain would save on
possible confusion..

> +	ret = inv_icm42600_enable_regulator_vddio(st);
> +
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +const struct dev_pm_ops inv_icm42600_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(inv_icm42600_suspend, inv_icm42600_resume)
> +	SET_RUNTIME_PM_OPS(inv_icm42600_runtime_suspend,
> +			   inv_icm42600_runtime_resume, NULL)
> +};
> +EXPORT_SYMBOL_GPL(inv_icm42600_pm_ops);
> +
> +MODULE_AUTHOR("InvenSense, Inc.");
> +MODULE_DESCRIPTION("InvenSense ICM-426xx device driver");
> +MODULE_LICENSE("GPL");