On Thu, 7 May 2020 16:42:11 +0200
Jean-Baptiste Maneyrol <jmaney...@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 <jmaney...@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");


Reply via email to