Re: [PATCH v3 2/2] iio: imu: Add support for the FXOS8700 IMU

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 10 Oct 2019 10:56:48 -0700
Robert Jones <rjones@gateworks.com> wrote:

> FXOS8700CQ is a small, low-power, 3-axis linear accelerometer and 3-axis
> magnetometer combined into a single package. The device features a
> selectable I2C or point-to-point SPI serial interface with 14-bit
> accelerometer and 16-bit magnetometer ADC resolution along with
> smart-embedded functions.
> 
> FXOS8700CQ has dynamically selectable accelerationfull-scale ranges of
> ±2 g/±4 g/±8 g and a fixed magnetic measurement range of ±1200 μT.
> Output data rates (ODR) from 1.563 Hz to 800 Hz are selectable by the user
> for each sensor. Interleaved magnetic and acceleration data is available
> at ODR rates of up to 400 Hz. FXOS8700CQ is available in a plastic QFN
> package and it is guaranteed to operate over the extended temperature
> range of –40 °C to +85 °C.
> 
> TODO: Trigger and IRQ configuration support
> 
> Datasheet:
>   http://cache.freescale.com/files/sensors/doc/data_sheet/FXOS8700CQ.pdf
> 
> Signed-off-by: Robert Jones <rjones@gateworks.com>
Mostly looks good.  A few remaining bits inline.

The endian types seems to have gotten very confused!

Jonathan

> ---
>  drivers/iio/imu/Kconfig         |  27 +++
>  drivers/iio/imu/Makefile        |   5 +
>  drivers/iio/imu/fxos8700.h      | 185 +++++++++++++++
>  drivers/iio/imu/fxos8700_core.c | 499 ++++++++++++++++++++++++++++++++++++++++
>  drivers/iio/imu/fxos8700_i2c.c  |  71 ++++++
>  drivers/iio/imu/fxos8700_spi.c  |  59 +++++
>  6 files changed, 846 insertions(+)
>  create mode 100644 drivers/iio/imu/fxos8700.h
>  create mode 100644 drivers/iio/imu/fxos8700_core.c
>  create mode 100644 drivers/iio/imu/fxos8700_i2c.c
>  create mode 100644 drivers/iio/imu/fxos8700_spi.c
> 
> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
> index f3c7282..60bb102 100644
> --- a/drivers/iio/imu/Kconfig
> +++ b/drivers/iio/imu/Kconfig
> @@ -40,6 +40,33 @@ config ADIS16480
>  
>  source "drivers/iio/imu/bmi160/Kconfig"
>  
> +config FXOS8700
> +	tristate
> +
> +config FXOS8700_I2C
> +	tristate "NXP FXOS8700 I2C driver"
> +	depends on I2C
> +	select FXOS8700
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build support for the NXP FXOS8700 m+g combo
> +	  sensor on I2C.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called fxos8700_i2c.
> +
> +config FXOS8700_SPI
> +	tristate "NXP FXOS8700 SPI driver"
> +	depends on SPI
> +	select FXOS8700
> +	select REGMAP_SPI
> +	help
> +	  Say yes here to build support for the NXP FXOS8700 m+g combo
> +	  sensor on SPI.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called fxos8700_spi.
> +
>  config KMX61
>  	tristate "Kionix KMX61 6-axis accelerometer and magnetometer"
>  	depends on I2C
> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
> index 4a69588..5237fd4 100644
> --- a/drivers/iio/imu/Makefile
> +++ b/drivers/iio/imu/Makefile
> @@ -14,6 +14,11 @@ adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
>  obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
>  
>  obj-y += bmi160/
> +
> +obj-$(CONFIG_FXOS8700) += fxos8700_core.o
> +obj-$(CONFIG_FXOS8700_I2C) += fxos8700_i2c.o
> +obj-$(CONFIG_FXOS8700_SPI) += fxos8700_spi.o
> +
>  obj-y += inv_mpu6050/
>  
>  obj-$(CONFIG_KMX61) += kmx61.o
> diff --git a/drivers/iio/imu/fxos8700.h b/drivers/iio/imu/fxos8700.h
> new file mode 100644
> index 0000000..92499a2
> --- /dev/null
> +++ b/drivers/iio/imu/fxos8700.h
> @@ -0,0 +1,185 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef FXOS8700_H_
> +#define FXOS8700_H_
> +
> +extern const struct regmap_config fxos8700_regmap_config;
> +
> +int fxos8700_core_probe(struct device *dev, struct regmap *regmap,
> +			const char *name, bool use_spi);
> +
> +/* Register Definitions */

If register definitions are only used in one c file, move them there
rather than exposing the in the header.

> +#define FXOS8700_STATUS             0x00
> +#define FXOS8700_OUT_X_MSB          0x01
> +#define FXOS8700_OUT_X_LSB          0x02
> +#define FXOS8700_OUT_Y_MSB          0x03
> +#define FXOS8700_OUT_Y_LSB          0x04
> +#define FXOS8700_OUT_Z_MSB          0x05
> +#define FXOS8700_OUT_Z_LSB          0x06
> +#define FXOS8700_F_SETUP            0x09
> +#define FXOS8700_TRIG_CFG           0x0a
> +#define FXOS8700_SYSMOD             0x0b
> +#define FXOS8700_INT_SOURCE         0x0c
> +#define FXOS8700_WHO_AM_I           0x0d
> +#define FXOS8700_XYZ_DATA_CFG       0x0e
> +#define FXOS8700_HP_FILTER_CUTOFF   0x0f
> +#define FXOS8700_PL_STATUS          0x10
> +#define FXOS8700_PL_CFG             0x11
> +#define FXOS8700_PL_COUNT           0x12
> +#define FXOS8700_PL_BF_ZCOMP        0x13
> +#define FXOS8700_PL_THS_REG         0x14
> +#define FXOS8700_A_FFMT_CFG         0x15
> +#define FXOS8700_A_FFMT_SRC         0x16
> +#define FXOS8700_A_FFMT_THS         0x17
> +#define FXOS8700_A_FFMT_COUNT       0x18
> +#define FXOS8700_TRANSIENT_CFG      0x1d
> +#define FXOS8700_TRANSIENT_SRC      0x1e
> +#define FXOS8700_TRANSIENT_THS      0x1f
> +#define FXOS8700_TRANSIENT_COUNT    0x20
> +#define FXOS8700_PULSE_CFG          0x21
> +#define FXOS8700_PULSE_SRC          0x22
> +#define FXOS8700_PULSE_THSX         0x23
> +#define FXOS8700_PULSE_THSY         0x24
> +#define FXOS8700_PULSE_THSZ         0x25
> +#define FXOS8700_PULSE_TMLT         0x26
> +#define FXOS8700_PULSE_LTCY         0x27
> +#define FXOS8700_PULSE_WIND         0x28
> +#define FXOS8700_ASLP_COUNT         0x29
> +#define FXOS8700_CTRL_REG1          0x2a
> +#define FXOS8700_CTRL_REG2          0x2b
> +#define FXOS8700_CTRL_REG3          0x2c
> +#define FXOS8700_CTRL_REG4          0x2d
> +#define FXOS8700_CTRL_REG5          0x2e
> +#define FXOS8700_OFF_X              0x2f
> +#define FXOS8700_OFF_Y              0x30
> +#define FXOS8700_OFF_Z              0x31
> +#define FXOS8700_M_DR_STATUS        0x32
> +#define FXOS8700_M_OUT_X_MSB        0x33
> +#define FXOS8700_M_OUT_X_LSB        0x34
> +#define FXOS8700_M_OUT_Y_MSB        0x35
> +#define FXOS8700_M_OUT_Y_LSB        0x36
> +#define FXOS8700_M_OUT_Z_MSB        0x37
> +#define FXOS8700_M_OUT_Z_LSB        0x38
> +#define FXOS8700_CMP_X_MSB          0x39
> +#define FXOS8700_CMP_X_LSB          0x3a
> +#define FXOS8700_CMP_Y_MSB          0x3b
> +#define FXOS8700_CMP_Y_LSB          0x3c
> +#define FXOS8700_CMP_Z_MSB          0x3d
> +#define FXOS8700_CMP_Z_LSB          0x3e
> +#define FXOS8700_M_OFF_X_MSB        0x3f
> +#define FXOS8700_M_OFF_X_LSB        0x40
> +#define FXOS8700_M_OFF_Y_MSB        0x41
> +#define FXOS8700_M_OFF_Y_LSB        0x42
> +#define FXOS8700_M_OFF_Z_MSB        0x43
> +#define FXOS8700_M_OFF_Z_LSB        0x44
> +#define FXOS8700_MAX_X_MSB          0x45
> +#define FXOS8700_MAX_X_LSB          0x46
> +#define FXOS8700_MAX_Y_MSB          0x47
> +#define FXOS8700_MAX_Y_LSB          0x48
> +#define FXOS8700_MAX_Z_MSB          0x49
> +#define FXOS8700_MAX_Z_LSB          0x4a
> +#define FXOS8700_MIN_X_MSB          0x4b
> +#define FXOS8700_MIN_X_LSB          0x4c
> +#define FXOS8700_MIN_Y_MSB          0x4d
> +#define FXOS8700_MIN_Y_LSB          0x4e
> +#define FXOS8700_MIN_Z_MSB          0x4f
> +#define FXOS8700_MIN_Z_LSB          0x50
> +#define FXOS8700_TEMP               0x51
> +#define FXOS8700_M_THS_CFG          0x52
> +#define FXOS8700_M_THS_SRC          0x53
> +#define FXOS8700_M_THS_X_MSB        0x54
> +#define FXOS8700_M_THS_X_LSB        0x55
> +#define FXOS8700_M_THS_Y_MSB        0x56
> +#define FXOS8700_M_THS_Y_LSB        0x57
> +#define FXOS8700_M_THS_Z_MSB        0x58
> +#define FXOS8700_M_THS_Z_LSB        0x59
> +#define FXOS8700_M_THS_COUNT        0x5a
> +#define FXOS8700_M_CTRL_REG1        0x5b
> +#define FXOS8700_M_CTRL_REG2        0x5c
> +#define FXOS8700_M_CTRL_REG3        0x5d
> +#define FXOS8700_M_INT_SRC          0x5e
> +#define FXOS8700_A_VECM_CFG         0x5f
> +#define FXOS8700_A_VECM_THS_MSB     0x60
> +#define FXOS8700_A_VECM_THS_LSB     0x61
> +#define FXOS8700_A_VECM_CNT         0x62
> +#define FXOS8700_A_VECM_INITX_MSB   0x63
> +#define FXOS8700_A_VECM_INITX_LSB   0x64
> +#define FXOS8700_A_VECM_INITY_MSB   0x65
> +#define FXOS8700_A_VECM_INITY_LSB   0x66
> +#define FXOS8700_A_VECM_INITZ_MSB   0x67
> +#define FXOS8700_A_VECM_INITZ_LSB   0x68
> +#define FXOS8700_M_VECM_CFG         0x69
> +#define FXOS8700_M_VECM_THS_MSB     0x6a
> +#define FXOS8700_M_VECM_THS_LSB     0x6b
> +#define FXOS8700_M_VECM_CNT         0x6c
> +#define FXOS8700_M_VECM_INITX_MSB   0x6d
> +#define FXOS8700_M_VECM_INITX_LSB   0x6e
> +#define FXOS8700_M_VECM_INITY_MSB   0x6f
> +#define FXOS8700_M_VECM_INITY_LSB   0x70
> +#define FXOS8700_M_VECM_INITZ_MSB   0x71
> +#define FXOS8700_M_VECM_INITZ_LSB   0x72
> +#define FXOS8700_A_FFMT_THS_X_MSB   0x73
> +#define FXOS8700_A_FFMT_THS_X_LSB   0x74
> +#define FXOS8700_A_FFMT_THS_Y_MSB   0x75
> +#define FXOS8700_A_FFMT_THS_Y_LSB   0x76
> +#define FXOS8700_A_FFMT_THS_Z_MSB   0x77
> +#define FXOS8700_A_FFMT_THS_Z_LSB   0x78
> +#define FXOS8700_A_TRAN_INIT_MSB    0x79
> +#define FXOS8700_A_TRAN_INIT_LSB_X  0x7a
> +#define FXOS8700_A_TRAN_INIT_LSB_Y  0x7b
> +#define FXOS8700_A_TRAN_INIT_LSB_Z  0x7d
> +#define FXOS8700_TM_NVM_LOCK        0x7e
> +#define FXOS8700_NVM_DATA0_35       0x80
> +#define FXOS8700_NVM_DATA_BNK3      0xa4
> +#define FXOS8700_NVM_DATA_BNK2      0xa5
> +#define FXOS8700_NVM_DATA_BNK1      0xa6
> +#define FXOS8700_NVM_DATA_BNK0      0xa7
> +
> +/* Bit definitions for FXOS8700_M_CTRL_REG1 */
> +#define FXOS8700_HMS_MASK           GENMASK(1, 0)
> +#define FXOS8700_OS_MASK            GENMASK(4, 2)
> +
> +/* Bit definitions for FXOS8700_M_CTRL_REG2 */
> +#define FXOS8700_MAXMIN_RST         BIT(2)
> +#define FXOS8700_MAXMIN_DIS_THS     BIT(3)
> +#define FXOS8700_MAXMIN_DIS         BIT(4)
> +
> +#define SENSOR_IOCTL_BASE           'S'
> +#define SENSOR_GET_MODEL_NAME       _IOR(SENSOR_IOCTL_BASE, 0, char *)
> +#define SENSOR_GET_POWER_STATUS     _IOR(SENSOR_IOCTL_BASE, 2, int)
> +#define SENSOR_SET_POWER_STATUS     _IOR(SENSOR_IOCTL_BASE, 3, int)
> +#define SENSOR_GET_DELAY_TIME       _IOR(SENSOR_IOCTL_BASE, 4, int)
> +#define SENSOR_SET_DELAY_TIME       _IOR(SENSOR_IOCTL_BASE, 5, int)
> +#define SENSOR_GET_RAW_DATA         _IOR(SENSOR_IOCTL_BASE, 6, short[3])

What are these doing here?

> +
> +#define FXOS8700_I2C_ADDR           0x1E
> +#define FXOS8700_DEVICE_ID          0xC7
> +#define FXOS8700_PRE_DEVICE_ID      0xC4
> +#define FXOS8700_DATA_BUF_SIZE      6
> +#define FXOS8700_DELAY_DEFAULT      200 /* msecs */
> +#define FXOS8700_POSITION_DEFAULT   1   /* msecs */
> +
> +#define FXOS8700_TYPE_ACC           0x00
> +#define FXOS8700_TYPE_MAG           0x01
> +#define FXOS8700_STANDBY            0x00
> +#define FXOS8700_ACTIVE             0x01
> +#define FXOS8700_ACTIVE_MIN_USLEEP  4000 /* from table 6 in datasheet */
> +
> +#define ABS_STATUS                  ABS_WHEEL
> +#define FXOS8700_DRIVER             "fxos8700"
> +
> +#define ABSMAX_ACC_VAL              0x01FF
> +#define ABSMIN_ACC_VAL              -(ABSMAX_ACC_VAL)
> +#define FXOS8700_POLL_INTERVAL      400
> +#define FXOS8700_POLL_MAX           800
> +#define FXOS8700_POLL_MIN           100
> +#define FXOS8700_CTRL_ODR_MSK       0x38
> +#define FXOS8700_CTRL_ODR_MAX       0x00
> +#define FXOS8700_CTRL_ODR_MIN       (0x03 << 3)
> +
> +enum {
> +	MODE_2G = 0,
> +	MODE_4G,
> +	MODE_8G,
> +};
> +
> +#endif  /* FXOS8700_H_ */
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> new file mode 100644
> index 0000000..ea2716b
> --- /dev/null
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FXOS8700 - NXP IMU (accelerometer plus magnetometer)
> + *
> + * IIO core driver for FXOS8700, with support for I2C/SPI busses
> + *
> + * TODO: Buffer, trigger, and IRQ support
> + */
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#include "fxos8700.h"
> +
> +struct fxos8700_data {
> +	struct regmap *regmap;
> +	struct iio_trigger *trig;
> +	u8 buf[FXOS8700_DATA_BUF_SIZE] ____cacheline_aligned;
> +};
> +
> +/* Regmap info */
> +static const struct regmap_range read_range[] = {
> +	{
> +		.range_min = FXOS8700_STATUS,
> +		.range_max = FXOS8700_A_FFMT_COUNT,
> +	}, {
> +		.range_min = FXOS8700_TRANSIENT_CFG,
> +		.range_max = FXOS8700_A_FFMT_THS_Z_LSB,
> +	},
> +};
> +
> +static const struct regmap_range write_range[] = {
> +	{
> +		.range_min = FXOS8700_F_SETUP,
> +		.range_max = FXOS8700_TRIG_CFG,
> +	}, {
> +		.range_min = FXOS8700_XYZ_DATA_CFG,
> +		.range_max = FXOS8700_HP_FILTER_CUTOFF,
> +	}, {
> +		.range_min = FXOS8700_PL_CFG,
> +		.range_max = FXOS8700_A_FFMT_CFG,
> +	}, {
> +		.range_min = FXOS8700_A_FFMT_THS,
> +		.range_max = FXOS8700_TRANSIENT_CFG,
> +	}, {
> +		.range_min = FXOS8700_TRANSIENT_THS,
> +		.range_max = FXOS8700_PULSE_CFG,
> +	}, {
> +		.range_min = FXOS8700_PULSE_THSX,
> +		.range_max = FXOS8700_OFF_Z,
> +	}, {
> +		.range_min = FXOS8700_M_OFF_X_MSB,
> +		.range_max = FXOS8700_M_OFF_Z_LSB,
> +	}, {
> +		.range_min = FXOS8700_M_THS_CFG,
> +		.range_max = FXOS8700_M_THS_CFG,
> +	}, {
> +		.range_min = FXOS8700_M_THS_X_MSB,
> +		.range_max = FXOS8700_M_CTRL_REG3,
> +	}, {
> +		.range_min = FXOS8700_A_VECM_CFG,
> +		.range_max = FXOS8700_A_FFMT_THS_Z_LSB,
> +	},
> +};
> +
> +static const struct regmap_access_table driver_read_table = {
> +	.yes_ranges =   read_range,
> +	.n_yes_ranges = ARRAY_SIZE(read_range),
> +};
> +
> +static const struct regmap_access_table driver_write_table = {
> +	.yes_ranges =   write_range,
> +	.n_yes_ranges = ARRAY_SIZE(write_range),
> +};
> +
> +const struct regmap_config fxos8700_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = FXOS8700_NVM_DATA_BNK0,
> +	.rd_table = &driver_read_table,
> +	.wr_table = &driver_write_table,
> +};
> +EXPORT_SYMBOL(fxos8700_regmap_config);
> +
> +#define FXOS8700_CHANNEL(_type, _axis) {			\
> +	.type = _type,						\
> +	.modified = 1,						\
> +	.channel2 = IIO_MOD_##_axis,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |  \
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
> +}
> +
> +/* scan indexes follow DATA register order */
> +enum fxos8700_scan_axis {
> +	FXOS8700_SCAN_ACCEL_X = 0,
> +	FXOS8700_SCAN_ACCEL_Y,
> +	FXOS8700_SCAN_ACCEL_Z,
> +	FXOS8700_SCAN_MAGN_X,
> +	FXOS8700_SCAN_MAGN_Y,
> +	FXOS8700_SCAN_MAGN_Z,
> +	FXOS8700_SCAN_RHALL,
> +	FXOS8700_SCAN_TIMESTAMP,
> +};
> +
> +enum fxos8700_sensor {
> +	FXOS8700_ACCEL	= 0,
> +	FXOS8700_MAGN,
> +	FXOS8700_NUM_SENSORS /* must be last */
> +};
> +
> +enum fxos8700_int_pin {
> +	FXOS8700_PIN_INT1,
> +	FXOS8700_PIN_INT2
> +};
> +
> +struct fxos8700_scale {
> +	u8 bits;
> +	int uscale;
> +};
> +
> +struct fxos8700_odr {
> +	u8 bits;
> +	int odr;
> +	int uodr;
> +};
> +
> +static const struct fxos8700_scale fxos8700_accel_scale[] = {
> +	{ MODE_2G, 244},
> +	{ MODE_4G, 488},
> +	{ MODE_8G, 976},
> +};
> +
> +/*
> + * Accellerometer and magnetometer have the same ODR options, set in the
> + * CTRL_REG1 register. ODR is halved when using both sensors at once in
> + * hybrid mode.
> + */
> +static const struct fxos8700_odr fxos8700_odr[] = {
> +	{0x00, 800, 0},
> +	{0x01, 400, 0},
> +	{0x02, 200, 0},
> +	{0x03, 100, 0},
> +	{0x04, 50, 0},
> +	{0x05, 12, 500000},
> +	{0x06, 6, 250000},
> +	{0x07, 1, 562500},
> +};
> +
> +static const struct iio_chan_spec fxos8700_channels[] = {
> +	FXOS8700_CHANNEL(IIO_ACCEL, X),
> +	FXOS8700_CHANNEL(IIO_ACCEL, Y),
> +	FXOS8700_CHANNEL(IIO_ACCEL, Z),
> +	FXOS8700_CHANNEL(IIO_MAGN, X),
> +	FXOS8700_CHANNEL(IIO_MAGN, Y),
> +	FXOS8700_CHANNEL(IIO_MAGN, Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(FXOS8700_SCAN_TIMESTAMP),
> +};
> +
> +static enum fxos8700_sensor fxos8700_to_sensor(enum iio_chan_type iio_type)
> +{
> +	switch (iio_type) {
> +	case IIO_ACCEL:
> +		return FXOS8700_ACCEL;
> +	case IIO_ANGL_VEL:
> +		return FXOS8700_MAGN;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int fxos8700_set_active_mode(struct fxos8700_data *data,
> +				    enum fxos8700_sensor t, bool mode)
> +{
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1, mode);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(FXOS8700_ACTIVE_MIN_USLEEP,
> +		     FXOS8700_ACTIVE_MIN_USLEEP + 1000);
> +
> +	return 0;
> +}
> +
> +static int fxos8700_set_scale(struct fxos8700_data *data,
> +			      enum fxos8700_sensor t, int uscale)
> +{
> +	int i;
> +	static const int scale_num = ARRAY_SIZE(fxos8700_accel_scale);
> +	struct device *dev = regmap_get_device(data->regmap);
> +
> +	if (t == FXOS8700_MAGN) {
> +		dev_err(dev, "Magnetometer scale is locked at 1200uT\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < scale_num; i++)
> +		if (fxos8700_accel_scale[i].uscale == uscale)
> +			break;
> +
> +	if (i == scale_num)
> +		return -EINVAL;
> +
> +	return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
> +			    fxos8700_accel_scale[i].bits);
> +}
> +
> +static int fxos8700_get_scale(struct fxos8700_data *data,
> +			      enum fxos8700_sensor t, int *uscale)
> +{
> +	int i, ret, val;
> +	static const int scale_num = ARRAY_SIZE(fxos8700_accel_scale);
> +
> +	if (t == FXOS8700_MAGN) {
> +		*uscale = 1200; /* Magnetometer is locked at 1200uT */
> +		return 0;
> +	}
> +
> +	ret = regmap_read(data->regmap, FXOS8700_XYZ_DATA_CFG, &val);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < scale_num; i++)
> +		if (fxos8700_accel_scale[i].bits == (val & 0x3)) {
> +			*uscale = fxos8700_accel_scale[i].uscale;
> +			return 0;
> +		}
> +
> +	return -EINVAL;
> +}
> +
> +static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
> +			     int axis, int *val)
> +{
> +	u8 base, reg;
> +	int ret;
> +	__le16 sample = 0;
> +	enum fxos8700_sensor type = fxos8700_to_sensor(chan_type);
> +
> +	base = type ? FXOS8700_OUT_X_MSB : FXOS8700_M_OUT_X_MSB;
> +
> +	/* Block read 6 bytes of device output registers to avoid data loss */
> +	ret = regmap_bulk_read(data->regmap, base, data->buf,
> +			       FXOS8700_DATA_BUF_SIZE);
> +	if (ret)
> +		return ret;
> +
> +	/* Convert axis to buffer index */
> +	reg = axis - IIO_MOD_X;
> +
> +	/* Composite individual axis MSB/LSB registers */
> +	sample = ((u16 *)data->buf)[reg];

Firstly cast to le16, then secondly don't then do a be16 to
cpu conversion?  Which one is it le16 or be16?

> +
> +	/* Convert to native endianness */
> +	*val = sign_extend32(be16_to_cpu(sample), 15);
> +
> +	return 0;
> +}
> +
> +static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
> +			    int odr, int uodr)
> +{
> +	int i, ret, val;
> +	bool active_mode;
> +	static const int odr_num = ARRAY_SIZE(fxos8700_odr);
> +
> +	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &val);
> +	if (ret)
> +		return ret;
> +
> +	active_mode = val & FXOS8700_ACTIVE;
> +
> +	if (active_mode) {
> +		/*
> +		 * The device must be in standby mode to change any of the
> +		 * other fields within CTRL_REG1
> +		 */
> +		ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> +				   val & ~FXOS8700_ACTIVE);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < odr_num; i++)
> +		if (fxos8700_odr[i].odr == odr && fxos8700_odr[i].uodr == uodr)
> +			break;
> +
> +	if (i >= odr_num)
> +		return -EINVAL;
> +
> +	return regmap_update_bits(data->regmap,
> +				  FXOS8700_CTRL_REG1,
> +				  FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
> +				  fxos8700_odr[i].bits << 3 | active_mode);
> +}
> +
> +static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
> +			    int *odr, int *uodr)
> +{
> +	int i, val, ret;
> +	static const int odr_num = ARRAY_SIZE(fxos8700_odr);
> +
> +	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= FXOS8700_CTRL_ODR_MSK;
> +
> +	for (i = 0; i < odr_num; i++)
> +		if (val == fxos8700_odr[i].bits)
> +			break;
> +
> +	if (i >= odr_num)
> +		return -EINVAL;
> +
> +	*odr = fxos8700_odr[i].odr;
> +	*uodr = fxos8700_odr[i].uodr;
> +
> +	return 0;
> +}
> +
> +static int fxos8700_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	int ret;
> +	struct fxos8700_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = fxos8700_get_data(data, chan->type, chan->channel2, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		ret = fxos8700_get_scale(data, fxos8700_to_sensor(chan->type),
> +					 val2);
> +		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = fxos8700_get_odr(data, fxos8700_to_sensor(chan->type),
> +				       val, val2);
> +		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int fxos8700_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct fxos8700_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return fxos8700_set_scale(data, fxos8700_to_sensor(chan->type),
> +					  val2);
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return fxos8700_set_odr(data, fxos8700_to_sensor(chan->type),
> +					val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static IIO_CONST_ATTR(in_accel_sampling_frequency_available,
> +		      "1.5625 6.25 12.5 50 100 200 400 800");
> +static IIO_CONST_ATTR(in_magn_sampling_frequency_available,
> +		      "1.5625 6.25 12.5 50 100 200 400 800");
> +static IIO_CONST_ATTR(in_accel_scale_available, "0.000244 0.000488 0.000976");
> +static IIO_CONST_ATTR(in_magn_scale_available, "0.000001200");
> +
> +static struct attribute *fxos8700_attrs[] = {
> +	&iio_const_attr_in_accel_sampling_frequency_available.dev_attr.attr,
> +	&iio_const_attr_in_magn_sampling_frequency_available.dev_attr.attr,
> +	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
> +	&iio_const_attr_in_magn_scale_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group fxos8700_attrs_group = {
> +	.attrs = fxos8700_attrs,
> +};
> +
> +static const struct iio_info fxos8700_info = {
> +	.read_raw = fxos8700_read_raw,
> +	.write_raw = fxos8700_write_raw,
> +	.attrs = &fxos8700_attrs_group,
> +};
> +
> +static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct device *dev = regmap_get_device(data->regmap);
> +
> +	ret = regmap_read(data->regmap, FXOS8700_WHO_AM_I, &val);
> +	if (ret) {
> +		dev_err(dev, "Error reading chip id\n");
> +		return ret;
> +	}
> +	if (val != FXOS8700_DEVICE_ID && val != FXOS8700_PRE_DEVICE_ID) {
> +		dev_err(dev, "Wrong chip id, got %x expected %x or %x\n",
> +			val, FXOS8700_DEVICE_ID, FXOS8700_PRE_DEVICE_ID);
> +		return -ENODEV;
> +	}
> +
> +	ret = fxos8700_set_active_mode(data, FXOS8700_ACCEL, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = fxos8700_set_active_mode(data, FXOS8700_MAGN, true);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The device must be in standby mode to change any of the other fields
> +	 * within CTRL_REG1
> +	 */
> +	ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1, 0x00);
> +	if (ret)
> +		return ret;
> +	/* Set max oversample ratio (OSR) and both devices active */
> +	ret = regmap_write(data->regmap, FXOS8700_M_CTRL_REG1,
> +			   FXOS8700_HMS_MASK | FXOS8700_OS_MASK);
> +	if (ret)
> +		return ret;
> +	/* Disable and rst min/max measurements & threshold */
> +	ret = regmap_write(data->regmap, FXOS8700_M_CTRL_REG2,
> +			   FXOS8700_MAXMIN_RST | FXOS8700_MAXMIN_DIS_THS |
> +			   FXOS8700_MAXMIN_DIS);
> +	if (ret)
> +		return ret;
> +	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> +	ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> +			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +	if (ret)
> +		return ret;
> +	/* Set for max full-scale range (+/-8G) */
> +	ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG, MODE_8G);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void fxos8700_chip_uninit(void *data)
> +{
> +	struct fxos8700_data *fxos8700_data = data;
> +
> +	fxos8700_set_active_mode(fxos8700_data, FXOS8700_ACCEL, false);
> +	fxos8700_set_active_mode(fxos8700_data, FXOS8700_MAGN, false);
> +}
> +
> +int fxos8700_core_probe(struct device *dev, struct regmap *regmap,
> +			const char *name, bool use_spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct fxos8700_data *data;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	dev_set_drvdata(dev, indio_dev);
> +	data->regmap = regmap;
> +
> +	ret = fxos8700_chip_init(data, use_spi);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, fxos8700_chip_uninit, data);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->channels = fxos8700_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(fxos8700_channels);
> +	indio_dev->name = name ? name : "fxos8700";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &fxos8700_info;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +EXPORT_SYMBOL_GPL(fxos8700_core_probe);
> +
> +MODULE_AUTHOR("Robert Jones <rjones@gateworks.com>");
> +MODULE_DESCRIPTION("FXOS8700 6-Axis Acc and Mag Combo Sensor driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/imu/fxos8700_i2c.c b/drivers/iio/imu/fxos8700_i2c.c
> new file mode 100644
> index 0000000..73a5f91
> --- /dev/null
> +++ b/drivers/iio/imu/fxos8700_i2c.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FXOS8700 - NXP IMU, I2C bits
> + *
> + * 7-bit I2C slave address determined by SA1 and SA0 logic level
> + * inputs represented in the following table:
> + *      SA1  |  SA0  |  Slave Address
> + *      0    |  0    |  0x1E
> + *      0    |  1    |  0x1D
> + *      1    |  0    |  0x1C
> + *      1    |  1    |  0x1F
> + */
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +#include "fxos8700.h"
> +
> +static int fxos8700_i2c_probe(struct i2c_client *client,
> +			      const struct i2c_device_id *id)
> +{
> +	struct regmap *regmap;
> +	const char *name = NULL;
> +
> +	regmap = devm_regmap_init_i2c(client, &fxos8700_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "Failed to register i2c regmap %d\n",
> +			(int)PTR_ERR(regmap));
> +		return PTR_ERR(regmap);
> +	}
> +
> +	if (id)
> +		name = id->name;
> +
> +	return fxos8700_core_probe(&client->dev, regmap, name, false);
> +}
> +
> +static const struct i2c_device_id fxos8700_i2c_id[] = {
> +	{"fxos8700", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, fxos8700_i2c_id);
> +
> +static const struct acpi_device_id fxos8700_acpi_match[] = {
> +	{"FXOS8700", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, fxos8700_acpi_match);
> +
> +static const struct of_device_id fxos8700_of_match[] = {
> +	{ .compatible = "nxp,fxos8700" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, fxos8700_of_match);
> +
> +static struct i2c_driver fxos8700_i2c_driver = {
> +	.driver = {
> +		.name                   = "fxos8700_i2c",
> +		.acpi_match_table       = ACPI_PTR(fxos8700_acpi_match),
> +		.of_match_table         = of_match_ptr(fxos8700_of_match),
> +	},
> +	.probe          = fxos8700_i2c_probe,
> +	.id_table       = fxos8700_i2c_id,
> +};
> +module_i2c_driver(fxos8700_i2c_driver);
> +
> +MODULE_AUTHOR("Robert Jones <rjones@gateworks.com>");
> +MODULE_DESCRIPTION("FXOS8700 I2C driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/imu/fxos8700_spi.c b/drivers/iio/imu/fxos8700_spi.c
> new file mode 100644
> index 0000000..412ff08
> --- /dev/null
> +++ b/drivers/iio/imu/fxos8700_spi.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FXOS8700 - NXP IMU, I2C bits
> + *
nitpick :) Trailing blank line here doesn't add anything.

> + */
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

If we are spinning again, another thread I read today pointed out that
we don't actually need the of header in these files, but rather just
mod_devicetable.h so it would be better to include that.

> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include "fxos8700.h"
> +
> +static int fxos8700_spi_probe(struct spi_device *spi)
> +{
> +	struct regmap *regmap;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	regmap = devm_regmap_init_spi(spi, &fxos8700_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&spi->dev, "Failed to register spi regmap %d\n",
> +			(int)PTR_ERR(regmap));
> +		return PTR_ERR(regmap);
> +	}
> +	return fxos8700_core_probe(&spi->dev, regmap, id->name, true);
> +}
> +
> +static const struct spi_device_id fxos8700_spi_id[] = {
> +	{"fxos8700", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, fxos8700_spi_id);
> +
> +static const struct acpi_device_id fxos8700_acpi_match[] = {
> +	{"FXOS8700", 0},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, fxos8700_acpi_match);
> +
> +static const struct of_device_id fxos8700_of_match[] = {
> +	{ .compatible = "nxp,fxos8700" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, fxos8700_of_match);
> +
> +static struct spi_driver fxos8700_spi_driver = {
> +	.probe          = fxos8700_spi_probe,
> +	.id_table       = fxos8700_spi_id,
> +	.driver = {
> +		.acpi_match_table       = ACPI_PTR(fxos8700_acpi_match),
> +		.of_match_table         = fxos8700_of_match,
> +		.name                   = "fxos8700_spi",
> +	},
> +};
> +module_spi_driver(fxos8700_spi_driver);
> +
> +MODULE_AUTHOR("Robert Jones <rjones@gateworks.com>");
> +MODULE_DESCRIPTION("FXOS8700 SPI driver");
> +MODULE_LICENSE("GPL v2");