[PATCH 0/1] iio: add driver for Bosch BMA400 accelerometer

[PATCH 0/1] iio: add driver for Bosch BMA400 accelerometer

[Dan Robertson]

Add a IIO driver for the Bosch BMA400 3-axes ultra low-power accelerometer.
The initial implementation of the driver adds read support for the
acceleration and temperature data registers. The driver also has support for
reading and writing to the output data rate, oversampling ratio, and scale
configuration registers.

Comments and feedback are very much welcomed :)

Cheers,

 - Dan

Dan Robertson (1):
  iio: (bma400) add driver for the BMA400

 drivers/iio/accel/Kconfig       |  19 +
 drivers/iio/accel/Makefile      |   2 +
 drivers/iio/accel/bma400.h      |  74 +++
 drivers/iio/accel/bma400_core.c | 862 ++++++++++++++++++++++++++++++++
 drivers/iio/accel/bma400_i2c.c  |  54 ++
 5 files changed, 1011 insertions(+)
 create mode 100644 drivers/iio/accel/bma400.h
 create mode 100644 drivers/iio/accel/bma400_core.c
 create mode 100644 drivers/iio/accel/bma400_i2c.c

[PATCH 1/1] iio: (bma400) add driver for the BMA400

[Dan Robertson]

Add a IIO driver for the Bosch BMA400 3-axes ultra-low power accelerometer.
The driver supports reading from the acceleration and temperature
registers. The driver also supports reading and configuring the output data
rate, oversampling ratio, and scale.

Signed-off-by: Dan Robertson <dan@dlrobertson.com>
---
 drivers/iio/accel/Kconfig       |  19 +
 drivers/iio/accel/Makefile      |   2 +
 drivers/iio/accel/bma400.h      |  74 +++
 drivers/iio/accel/bma400_core.c | 862 ++++++++++++++++++++++++++++++++
 drivers/iio/accel/bma400_i2c.c  |  54 ++
 5 files changed, 1011 insertions(+)
 create mode 100644 drivers/iio/accel/bma400.h
 create mode 100644 drivers/iio/accel/bma400_core.c
 create mode 100644 drivers/iio/accel/bma400_i2c.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 9b9656ce37e6..cca6727e037e 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -112,6 +112,25 @@ config BMA220
 	  To compile this driver as a module, choose M here: the
 	  module will be called bma220_spi.
 
+config BMA400
+	tristate "Bosch BMA400 3-Axis Accelerometer Driver"
+	depends on I2C
+	select REGMAP
+	select BMA400_I2C if (I2C)
+	help
+	  Say Y here if you want to build a driver for the Bosch BMA400
+	  triaxial acceleration sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called bma400_core and you will also get
+	  bma400_i2c for I2C
+
+config BMA400_I2C
+	tristate
+	depends on BMA400
+	depends on I2C
+	select REGMAP_I2C
+
 config BMC150_ACCEL
 	tristate "Bosch BMC150 Accelerometer Driver"
 	select IIO_BUFFER
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 56bd0215e0d4..3a051cf37f40 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -14,6 +14,8 @@ obj-$(CONFIG_ADXL372_I2C) += adxl372_i2c.o
 obj-$(CONFIG_ADXL372_SPI) += adxl372_spi.o
 obj-$(CONFIG_BMA180) += bma180.o
 obj-$(CONFIG_BMA220) += bma220_spi.o
+obj-$(CONFIG_BMA400) += bma400_core.o
+obj-$(CONFIG_BMA400_I2C) += bma400_i2c.o
 obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
 obj-$(CONFIG_BMC150_ACCEL_I2C) += bmc150-accel-i2c.o
 obj-$(CONFIG_BMC150_ACCEL_SPI) += bmc150-accel-spi.o
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
new file mode 100644
index 000000000000..7fa92bc457f6
--- /dev/null
+++ b/drivers/iio/accel/bma400.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * bma400.h - Register constants and other forward declarations
+ *            needed by the bma400 sources.
+ *
+ * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
+ *
+ */
+
+#include <linux/regmap.h>
+
+/*
+ * Read-Only Registers
+ */
+
+/* Status and ID registers */
+#define BMA400_CHIP_ID_REG      0x00
+#define BMA400_ERR_REG          0x02
+#define BMA400_STATUS_REG       0x03
+
+/* Acceleration registers */
+#define BMA400_X_AXIS_LSB_REG   0x04
+#define BMA400_X_AXIS_MSB_REG   0x05
+#define BMA400_Y_AXIS_LSB_REG   0x06
+#define BMA400_Y_AXIS_MSB_REG   0x07
+#define BMA400_Z_AXIS_LSB_REG   0x08
+#define BMA400_Z_AXIS_MSB_REG   0x09
+
+/* Sensort time registers */
+#define BMA400_SENSOR_TIME0     0x0a
+#define BMA400_SENSOR_TIME1     0x0b
+#define BMA400_SENSOR_TIME2     0x0c
+
+/* Event and interrupt registers */
+#define BMA400_EVENT_REG        0x0d
+#define BMA400_INT_STAT0_REG    0x0e
+#define BMA400_INT_STAT1_REG    0x0f
+#define BMA400_INT_STAT2_REG    0x10
+
+/* Temperature register */
+#define BMA400_TEMP_DATA_REG    0x11
+
+/* FIFO length and data registers */
+#define BMA400_FIFO_LENGTH0_REG 0x12
+#define BMA400_FIFO_LENGTH1_REG 0x13
+#define BMA400_FIFO_DATA_REG    0x14
+
+/* Step count registers */
+#define BMA400_STEP_CNT0_REG    0x15
+#define BMA400_STEP_CNT1_REG    0x16
+#define BMA400_STEP_CNT3_REG    0x17
+#define BMA400_STEP_STAT_REG    0x18
+
+/*
+ * Read-write configuration registers
+ */
+#define BMA400_ACC_CONFIG0_REG  0x19
+#define BMA400_ACC_CONFIG1_REG  0x1a
+#define BMA400_ACC_CONFIG2_REG  0x1b
+#define BMA400_CMD_REG          0x7e
+
+/* Chip ID of BMA 400 devices found in the chip ID register. */
+#define BMA400_ID_REG_VAL       0x90
+
+/* The softreset command */
+#define BMA400_SOFTRESET_CMD    0xb6
+
+extern const struct regmap_config bma400_regmap_config;
+
+int bma400_probe(struct device *dev,
+		 struct regmap *regmap,
+		 const char *name);
+
+int bma400_remove(struct device *dev);
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
new file mode 100644
index 000000000000..55fe2f220c30
--- /dev/null
+++ b/drivers/iio/accel/bma400_core.c
@@ -0,0 +1,862 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * bma400-core.c - Core IIO driver for Bosch BMA400 triaxial acceleration
+ *                 sensor. Used by bma400-i2c.
+ *
+ * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
+ *
+ * TODO:
+ *  - Support for power management
+ *  - Support events and interrupts
+ *  - Create channel the step count
+ *  - Create channel for sensor time
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/bitops.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include "bma400.h"
+
+/*
+ * The G-range selection may be one of 2g, 4g, 8, or 16g. The scale may
+ * be selected with the acc_range bits of the ACC_CONFIG1 register.
+ */
+const int bma400_scale_table[] = { 38344, 76590, 153277, 306457 };
+
+struct bma400_sample_freq {
+	int hz;
+	int micro_hz;
+};
+
+/* See the ACC_CONFIG1 section of the datasheet */
+const struct bma400_sample_freq bma400_sample_freqs[] = {
+	{ .hz = 12, .micro_hz = 500000 },
+	{ .hz = 25, .micro_hz = 0 },
+	{ .hz = 50, .micro_hz = 0 },
+	{ .hz = 100, .micro_hz = 0 },
+	{ .hz = 200, .micro_hz = 0 },
+	{ .hz = 400, .micro_hz = 0 },
+	{ .hz = 800, .micro_hz = 0 },
+};
+
+/* See the ACC_CONFIG0 section of the datasheet */
+enum bma400_power_mode {
+	POWER_MODE_SLEEP   = 0x00,
+	POWER_MODE_LOW     = 0x01,
+	POWER_MODE_NORMAL  = 0x02,
+	POWER_MODE_INVALID = 0x03,
+};
+
+struct bma400_data {
+	struct device *dev;
+	struct mutex mutex; /* data register lock */
+	struct iio_mount_matrix orientation;
+	struct regmap *regmap;
+	enum bma400_power_mode power_mode;
+	const struct bma400_sample_freq *sample_freq;
+	int oversampling_ratio;
+	int scale;
+};
+
+static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case BMA400_CHIP_ID_REG:
+	case BMA400_ERR_REG:
+	case BMA400_STATUS_REG:
+	case BMA400_X_AXIS_LSB_REG:
+	case BMA400_X_AXIS_MSB_REG:
+	case BMA400_Y_AXIS_LSB_REG:
+	case BMA400_Y_AXIS_MSB_REG:
+	case BMA400_Z_AXIS_LSB_REG:
+	case BMA400_Z_AXIS_MSB_REG:
+	case BMA400_SENSOR_TIME0:
+	case BMA400_SENSOR_TIME1:
+	case BMA400_SENSOR_TIME2:
+	case BMA400_EVENT_REG:
+	case BMA400_INT_STAT0_REG:
+	case BMA400_INT_STAT1_REG:
+	case BMA400_INT_STAT2_REG:
+	case BMA400_TEMP_DATA_REG:
+	case BMA400_FIFO_LENGTH0_REG:
+	case BMA400_FIFO_LENGTH1_REG:
+	case BMA400_FIFO_DATA_REG:
+	case BMA400_STEP_CNT0_REG:
+	case BMA400_STEP_CNT1_REG:
+	case BMA400_STEP_CNT3_REG:
+	case BMA400_STEP_STAT_REG:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static bool bma400_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case BMA400_ERR_REG:
+	case BMA400_STATUS_REG:
+	case BMA400_X_AXIS_LSB_REG:
+	case BMA400_X_AXIS_MSB_REG:
+	case BMA400_Y_AXIS_LSB_REG:
+	case BMA400_Y_AXIS_MSB_REG:
+	case BMA400_Z_AXIS_LSB_REG:
+	case BMA400_Z_AXIS_MSB_REG:
+	case BMA400_SENSOR_TIME0:
+	case BMA400_SENSOR_TIME1:
+	case BMA400_SENSOR_TIME2:
+	case BMA400_EVENT_REG:
+	case BMA400_INT_STAT0_REG:
+	case BMA400_INT_STAT1_REG:
+	case BMA400_INT_STAT2_REG:
+	case BMA400_TEMP_DATA_REG:
+	case BMA400_FIFO_LENGTH0_REG:
+	case BMA400_FIFO_LENGTH1_REG:
+	case BMA400_FIFO_DATA_REG:
+	case BMA400_STEP_CNT0_REG:
+	case BMA400_STEP_CNT1_REG:
+	case BMA400_STEP_CNT3_REG:
+	case BMA400_STEP_STAT_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+const struct regmap_config bma400_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = BMA400_CMD_REG,
+	.cache_type = REGCACHE_RBTREE,
+	.writeable_reg = bma400_is_writable_reg,
+	.volatile_reg = bma400_is_volatile_reg,
+};
+EXPORT_SYMBOL(bma400_regmap_config);
+
+static const struct iio_mount_matrix *
+bma400_accel_get_mount_matrix(const struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan)
+{
+	struct bma400_data *data = iio_priv(indio_dev);
+
+	return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bma400_accel_get_mount_matrix),
+	{ }
+};
+
+#define BMA400_ACC_CHANNEL(_axis) { \
+	.type = IIO_ACCEL, \
+	.modified = 1, \
+	.channel2 = IIO_MOD_##_axis, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+		BIT(IIO_CHAN_INFO_SCALE) | \
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+	.ext_info = bma400_ext_info \
+}
+
+static const struct iio_chan_spec bma400_channels[] = {
+	BMA400_ACC_CHANNEL(X),
+	BMA400_ACC_CHANNEL(Y),
+	BMA400_ACC_CHANNEL(Z),
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	},
+};
+
+static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
+{
+	int ret;
+	int host_temp;
+	unsigned int raw_temp;
+
+	if (data->power_mode == POWER_MODE_SLEEP)
+		return -EBUSY;
+
+	ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &raw_temp);
+
+	if (!ret) {
+		host_temp = sign_extend32(raw_temp, 7);
+		/*
+		 * The formula for the TEMP_DATA register in the datasheet
+		 * is: x * 0.5 + 23
+		 */
+		*val = (host_temp >> 1) + 23;
+		*val2 = (host_temp & 0x1) * 500000;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+	}
+	return ret;
+}
+
+static int bma400_get_accel_reg(struct bma400_data *data,
+				const struct iio_chan_spec *chan,
+				int *val)
+{
+	int ret;
+	u16 lsb_reg;
+	__le16 raw_accel = 0;
+
+	if (data->power_mode == POWER_MODE_SLEEP)
+		return -EBUSY;
+
+	switch (chan->channel2) {
+	case IIO_MOD_X:
+		lsb_reg = BMA400_X_AXIS_LSB_REG;
+		break;
+	case IIO_MOD_Y:
+		lsb_reg = BMA400_Y_AXIS_LSB_REG;
+		break;
+	case IIO_MOD_Z:
+		lsb_reg = BMA400_Z_AXIS_LSB_REG;
+		break;
+	default:
+		dev_err(data->dev, "invalid axis channel modifier");
+		return -EINVAL;
+	}
+
+	/* bulk read two registers, with the base being the LSB register */
+	ret = regmap_bulk_read(data->regmap, lsb_reg, &raw_accel, 2);
+	if (!ret) {
+		*val = sign_extend32(le16_to_cpu(raw_accel), 11);
+		ret = IIO_VAL_INT;
+	}
+
+	return ret;
+}
+
+static int bma400_ready_for_cmd(struct bma400_data *data)
+{
+	unsigned int val;
+	int ret = regmap_read(data->regmap, BMA400_STATUS_REG, &val);
+
+	if (ret < 0 || !(val & 0x10))
+		return 0;
+
+	return 1;
+}
+
+static int bma400_softreset(struct bma400_data *data)
+{
+	int ret;
+
+	if (!bma400_ready_for_cmd(data))
+		return -EAGAIN;
+
+	ret = regmap_write(data->regmap, BMA400_CMD_REG,
+			   BMA400_SOFTRESET_CMD);
+	if (!ret) {
+		/* a softreset restores registers to their defaults */
+		data->power_mode = POWER_MODE_SLEEP;
+		data->sample_freq = NULL;
+		data->oversampling_ratio = -1;
+		data->scale = bma400_scale_table[0];
+	}
+	return ret;
+}
+
+static int bma400_get_acc_config0_reg(struct bma400_data *data)
+{
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
+			  &val);
+	if (ret < 0) {
+		dev_err(data->dev, "Failed read acc_config0");
+		return ret;
+	}
+
+	return val;
+}
+
+static int bma400_get_acc_config1_reg(struct bma400_data *data)
+{
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG,
+			  &val);
+	if (ret < 0) {
+		dev_err(data->dev, "Failed read acc_config1");
+		return ret;
+	}
+
+	return val;
+}
+
+static int bma400_get_accel_output_data_rate(struct bma400_data *data)
+{
+	int acc_config1;
+	unsigned int odr;
+	int idx;
+
+	switch (data->power_mode) {
+	case POWER_MODE_LOW:
+		/*
+		 * Runs at a fixed rate in low-power mode. See section 4.3
+		 * in the datasheet.
+		 */
+		data->sample_freq = &bma400_sample_freqs[1];
+		return 0;
+	case POWER_MODE_NORMAL:
+		/*
+		 * In normal mode the ODR can be found in the ACC_CONFIG1
+		 * register.
+		 */
+		acc_config1 = bma400_get_acc_config1_reg(data);
+		if (acc_config1 < 0) {
+			data->sample_freq = NULL;
+			return acc_config1;
+		}
+
+		odr = (acc_config1 & 0x0f);
+		if (odr < 0x05 || odr > 0x0b) {
+			dev_err(data->dev, "invalid ODR=%x", odr);
+			data->sample_freq = NULL;
+			return -EINVAL;
+		}
+		idx = odr - 0x05;
+
+		data->sample_freq = &bma400_sample_freqs[idx];
+		return 0;
+	default:
+		data->sample_freq = NULL;
+		return 0;
+	}
+}
+
+static int bma400_get_accel_output_data_rate_idx(struct bma400_data *data,
+						 int hz, int uhz)
+{
+	int i;
+	const struct bma400_sample_freq *sample_freq;
+
+	for (i = 0; i < ARRAY_SIZE(bma400_sample_freqs); ++i) {
+		sample_freq = &bma400_sample_freqs[i];
+		if (sample_freq->hz == hz &&
+		    sample_freq->micro_hz == uhz)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int bma400_set_accel_output_data_rate(struct bma400_data *data,
+					     int hz, int uhz)
+{
+	int acc_config1;
+	unsigned int odr;
+	int idx;
+	int ret;
+
+	idx = bma400_get_accel_output_data_rate_idx(data, hz, uhz);
+
+	if (idx < 0)
+		return idx;
+
+	acc_config1 = bma400_get_acc_config1_reg(data);
+
+	if (acc_config1 < 0)
+		return acc_config1;
+
+	/* preserve the range and normal mode osr */
+	odr = (0xf0 & acc_config1) | (idx + 0x5);
+
+	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG, odr);
+	if (!ret)
+		data->sample_freq = &bma400_sample_freqs[idx];
+
+	return ret;
+}
+
+static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
+{
+	int acc_config;
+
+	/*
+	 * The oversampling ratio is stored in a different register
+	 * based on the power-mode. In normal mode the OSR is stored
+	 * in ACC_CONFIG1. In low-power mode it is stored in
+	 * ACC_CONFIG0.
+	 */
+	switch (data->power_mode) {
+	case POWER_MODE_LOW:
+		acc_config = bma400_get_acc_config0_reg(data);
+		if (acc_config < 0) {
+			data->oversampling_ratio = -1;
+			return acc_config;
+		}
+
+		data->oversampling_ratio = (acc_config & 0x60) >> 5;
+		return 0;
+	case POWER_MODE_NORMAL:
+		acc_config = bma400_get_acc_config1_reg(data);
+		if (acc_config < 0) {
+			data->oversampling_ratio = -1;
+			return acc_config;
+		}
+
+		data->oversampling_ratio = (acc_config & 0x30) >> 4;
+		return 0;
+	default:
+		data->oversampling_ratio = -1;
+		return 0;
+	}
+}
+
+static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
+					       int val)
+{
+	int ret;
+	int acc_config;
+
+	/* The oversampling ratio is a two bit field */
+	if (val & ~0x3)
+		return -EINVAL;
+
+	/* The oversampling ratio is stored in a different register
+	 * based on the power-mode.
+	 */
+	switch (data->power_mode) {
+	case POWER_MODE_LOW:
+		acc_config = bma400_get_acc_config0_reg(data);
+		if (acc_config < 0)
+			return acc_config;
+
+		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
+				   (acc_config & 0x9f) | (val << 5));
+		if (ret < 0)
+			dev_err(data->dev, "Failed to write out OSR");
+		else
+			data->oversampling_ratio = val;
+		break;
+	case POWER_MODE_NORMAL:
+		acc_config = bma400_get_acc_config1_reg(data);
+		if (acc_config < 0)
+			return acc_config;
+
+		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
+				   (acc_config & 0xcf) | (val << 4));
+		if (ret < 0)
+			dev_err(data->dev, "Failed to write out OSR");
+		else
+			data->oversampling_ratio = val;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return ret;
+}
+
+static int bma400_get_accel_scale(struct bma400_data *data)
+{
+	int idx;
+	int acc_config1 = bma400_get_acc_config1_reg(data);
+
+	if (acc_config1 < 0)
+		return acc_config1;
+
+	idx = (acc_config1 & 0xc0) >> 6;
+	if (idx < ARRAY_SIZE(bma400_scale_table)) {
+		data->scale = bma400_scale_table[idx];
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(bma400_scale_table); ++i) {
+		if (bma400_scale_table[i] == val)
+			return i;
+	}
+	return -EINVAL;
+}
+
+static int bma400_set_accel_scale(struct bma400_data *data, unsigned int val)
+{
+	int ret;
+	int idx;
+	int acc_config1 = bma400_get_acc_config1_reg(data);
+
+	if (acc_config1 < 0)
+		return acc_config1;
+
+	idx = bma400_get_accel_scale_idx(data, val);
+
+	if (idx < 0)
+		return idx;
+
+	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
+			   (acc_config1 & 0x3f) | (idx << 6));
+	if (ret < 0)
+		return ret;
+
+	data->scale = val;
+	return 0;
+}
+
+static int bma400_get_power_mode(struct bma400_data *data)
+{
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(data->regmap, BMA400_STATUS_REG, &val);
+	if (ret < 0) {
+		dev_err(data->dev, "Failed to read status register");
+		return ret;
+	}
+
+	data->power_mode = (val >> 1) & 0x03;
+	return 0;
+}
+
+static int bma400_set_power_mode(struct bma400_data *data,
+				 enum bma400_power_mode mode)
+{
+	int ret;
+
+	ret = bma400_get_acc_config0_reg(data);
+
+	if (ret < 0)
+		return ret;
+
+	if (data->power_mode == mode)
+		return 0;
+
+	if (mode == POWER_MODE_INVALID)
+		return -EINVAL;
+
+	/* Preserve the low-power oversample ratio etc */
+	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
+			   mode | (ret & 0xfc));
+
+	if (ret < 0) {
+		dev_err(data->dev, "Failed to write to power-mode");
+		return ret;
+	}
+
+	/* Update our cached power-mode */
+	data->power_mode = mode;
+
+	/*
+	 * Update our cached osr and odr based on the new
+	 * power-mode.
+	 */
+	bma400_get_accel_output_data_rate(data);
+	bma400_get_accel_oversampling_ratio(data);
+
+	return 0;
+}
+
+static int bma400_init(struct bma400_data *data)
+{
+	int ret;
+	unsigned int val;
+
+	/* Try to read chip_id register. It must return 0x90. */
+	ret = regmap_read(data->regmap, BMA400_CHIP_ID_REG, &val);
+
+	if (ret < 0) {
+		dev_err(data->dev, "Failed to read chip id register: %x!", ret);
+		return ret;
+	} else if (val != BMA400_ID_REG_VAL) {
+		dev_err(data->dev, "CHIP ID MISMATCH: %x!", ret);
+		return -ENODEV;
+	}
+
+	ret = bma400_get_power_mode(data);
+	if (ret < 0) {
+		dev_err(data->dev, "Failed to get the initial power-mode!");
+		return ret;
+	}
+
+	if (data->power_mode != POWER_MODE_NORMAL) {
+		ret = bma400_set_power_mode(data, POWER_MODE_NORMAL);
+		if (ret < 0) {
+			dev_err(data->dev, "Failed to wake up the device!");
+			return ret;
+		}
+		/*
+		 * TODO: The datasheet waits 1500us here in the example, but
+		 * lists 2/ODR as the wakeup time.
+		 */
+		usleep_range(1500, 20000);
+	}
+
+	ret = bma400_get_accel_output_data_rate(data);
+	if (ret < 0)
+		return ret;
+
+	ret = bma400_get_accel_oversampling_ratio(data);
+	if (ret < 0)
+		return ret;
+
+	ret = bma400_get_accel_scale(data);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Once the interrupt engine is supported we might use the
+	 * data_src_reg, but for now ensure this is set to the
+	 * variable ODR filter selectable by the sample frequency
+	 * channel.
+	 */
+	return regmap_write(data->regmap, BMA400_ACC_CONFIG2_REG, 0x00);
+}
+
+static ssize_t bma400_show_samp_freq_avail(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	int i;
+	int len = 0;
+	const struct bma400_sample_freq *sample_freq;
+
+	for (i = 0; i < ARRAY_SIZE(bma400_sample_freqs); ++i) {
+		sample_freq = &bma400_sample_freqs[i];
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				 sample_freq->hz, sample_freq->micro_hz);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t bma400_show_scale_avail(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	int i;
+	int len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(bma400_scale_table); ++i)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06d ",
+				 bma400_scale_table[i]);
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(bma400_show_samp_freq_avail);
+
+static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
+		       bma400_show_scale_avail, NULL, 0);
+
+static struct attribute *bma400_attributes[] = {
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group bma400_attrs_group = {
+	.attrs = bma400_attributes,
+};
+
+static int bma400_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	struct bma400_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		mutex_lock(&data->mutex);
+		ret = bma400_get_temp_reg(data, val, val2);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&data->mutex);
+		ret = bma400_get_accel_reg(data, chan, val);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			if (!data->sample_freq)
+				return -EINVAL;
+
+			*val = data->sample_freq->hz;
+			if (!data->sample_freq->micro_hz)
+				return IIO_VAL_INT;
+
+			*val2 = data->sample_freq->micro_hz;
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_TEMP:
+			/*
+			 * Runs at a fixed sampling frequency. See Section 4.4
+			 * of the datasheet.
+			 */
+			*val = 6;
+			*val2 = 250000;
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		*val2 = data->scale;
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		/*
+		 * TODO: We could avoid this logic and returning -EINVAL here if
+		 * we set both the low-power and normal mode OSR registers when
+		 * we configure the device.
+		 */
+		if (data->oversampling_ratio < 0)
+			return -EINVAL;
+
+		*val = data->oversampling_ratio;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bma400_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val, int val2,
+			    long mask)
+{
+	int ret;
+	struct bma400_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		/*
+		 * The sample frequency is readonly for the temperature
+		 * register and a fixed value in low-power mode.
+		 */
+		if (chan->type != IIO_ACCEL)
+			return -EINVAL;
+
+		mutex_lock(&data->mutex);
+		ret = bma400_set_accel_output_data_rate(data, val, val2);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_CHAN_INFO_SCALE:
+		if (val != 0)
+			return -EINVAL;
+
+		mutex_lock(&data->mutex);
+		ret = bma400_set_accel_scale(data, val2);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		mutex_lock(&data->mutex);
+		ret = bma400_set_accel_oversampling_ratio(data, val);
+		mutex_unlock(&data->mutex);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan,
+				    long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info bma400_info = {
+	.attrs             = &bma400_attrs_group,
+	.read_raw          = bma400_read_raw,
+	.write_raw         = bma400_write_raw,
+	.write_raw_get_fmt = bma400_write_raw_get_fmt,
+};
+
+int bma400_probe(struct device *dev,
+		 struct regmap *regmap,
+		 const char *name)
+{
+	int ret;
+	struct bma400_data *data;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->regmap = regmap;
+	data->dev = dev;
+
+	ret = bma400_init(data);
+	if (ret < 0)
+		return ret;
+
+	ret = iio_read_mount_matrix(dev, "mount-matrix",
+				    &data->orientation);
+	if (ret)
+		return ret;
+
+	mutex_init(&data->mutex);
+	indio_dev->dev.parent = dev;
+	indio_dev->name = name;
+	indio_dev->info = &bma400_info;
+	indio_dev->channels = bma400_channels;
+	indio_dev->num_channels = ARRAY_SIZE(bma400_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	dev_set_drvdata(dev, indio_dev);
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(dev, "unable to register iio device\n");
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(bma400_probe);
+
+int bma400_remove(struct device *dev)
+{
+	int ret;
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct bma400_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->mutex);
+	ret = bma400_softreset(data);
+	if (ret < 0) {
+		/*
+		 * If the softreset failed, try to put the device in
+		 * sleep mode, but still report the error.
+		 */
+		dev_err(data->dev, "Failed to reset the device");
+		bma400_set_power_mode(data, POWER_MODE_SLEEP);
+	}
+	mutex_unlock(&data->mutex);
+
+	iio_device_unregister(indio_dev);
+
+	return ret;
+}
+EXPORT_SYMBOL(bma400_remove);
+
+MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
+MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
new file mode 100644
index 000000000000..227012a32e13
--- /dev/null
+++ b/drivers/iio/accel/bma400_i2c.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * bma400-i2c.c - I2C IIO driver for Bosch BMA400 triaxial acceleration sensor.
+ *
+ * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
+ *
+ * I2C address is either 0x14 or 0x15 depending on SDO
+ *
+ */
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/acpi.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "bma400.h"
+
+static int bma400_i2c_probe(struct i2c_client *client,
+			    const struct i2c_device_id *id)
+{
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i2c(client,
+				      &bma400_regmap_config);
+
+	return bma400_probe(&client->dev, regmap, id->name);
+}
+
+static int bma400_i2c_remove(struct i2c_client *client)
+{
+	return bma400_remove(&client->dev);
+}
+
+static const struct i2c_device_id bma400_i2c_ids[] = {
+	{ "bma400", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, bma400_i2c_ids);
+
+static struct i2c_driver bma400_i2c_driver = {
+	.driver = {
+		.name = "bma400",
+	},
+	.probe    = bma400_i2c_probe,
+	.remove   = bma400_i2c_remove,
+	.id_table = bma400_i2c_ids,
+};
+
+module_i2c_driver(bma400_i2c_driver);
+
+MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
+MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor");
+MODULE_LICENSE("GPL");

Re: [PATCH 1/1] iio: (bma400) add driver for the BMA400

[Jonathan Cameron]

On Wed, 25 Sep 2019 01:39:41 +0000
Dan Robertson <dan@dlrobertson.com> wrote:

> Add a IIO driver for the Bosch BMA400 3-axes ultra-low power accelerometer.
> The driver supports reading from the acceleration and temperature
> registers. The driver also supports reading and configuring the output data
> rate, oversampling ratio, and scale.
> 
> Signed-off-by: Dan Robertson <dan@dlrobertson.com>
Hi Dan,

So my first thought was that Bosch have been moderately good at keeping their
interfaces consistent across generations.   Seems not this time from
a quick look!

Minor stuff inline.  Quite a bit of it is stylistic rather than functional.
I'm a lazy maintainer and tend to end up reading a lot of code so want to
think as little as possible ;)

I'd also expect a modern driver to have DT bindings.  Might let that go if
it was being used with ACPI but neither is true here.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/Kconfig       |  19 +
>  drivers/iio/accel/Makefile      |   2 +
>  drivers/iio/accel/bma400.h      |  74 +++
>  drivers/iio/accel/bma400_core.c | 862 ++++++++++++++++++++++++++++++++
>  drivers/iio/accel/bma400_i2c.c  |  54 ++
>  5 files changed, 1011 insertions(+)
>  create mode 100644 drivers/iio/accel/bma400.h
>  create mode 100644 drivers/iio/accel/bma400_core.c
>  create mode 100644 drivers/iio/accel/bma400_i2c.c
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 9b9656ce37e6..cca6727e037e 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -112,6 +112,25 @@ config BMA220
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bma220_spi.
>  
> +config BMA400
> +	tristate "Bosch BMA400 3-Axis Accelerometer Driver"
> +	depends on I2C
> +	select REGMAP
> +	select BMA400_I2C if (I2C)
> +	help
> +	  Say Y here if you want to build a driver for the Bosch BMA400
> +	  triaxial acceleration sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called bma400_core and you will also get
> +	  bma400_i2c for I2C
> +
> +config BMA400_I2C
> +	tristate
> +	depends on BMA400
> +	depends on I2C
> +	select REGMAP_I2C
> +
>  config BMC150_ACCEL
>  	tristate "Bosch BMC150 Accelerometer Driver"
>  	select IIO_BUFFER
> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> index 56bd0215e0d4..3a051cf37f40 100644
> --- a/drivers/iio/accel/Makefile
> +++ b/drivers/iio/accel/Makefile
> @@ -14,6 +14,8 @@ obj-$(CONFIG_ADXL372_I2C) += adxl372_i2c.o
>  obj-$(CONFIG_ADXL372_SPI) += adxl372_spi.o
>  obj-$(CONFIG_BMA180) += bma180.o
>  obj-$(CONFIG_BMA220) += bma220_spi.o
> +obj-$(CONFIG_BMA400) += bma400_core.o
> +obj-$(CONFIG_BMA400_I2C) += bma400_i2c.o
>  obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
>  obj-$(CONFIG_BMC150_ACCEL_I2C) += bmc150-accel-i2c.o
>  obj-$(CONFIG_BMC150_ACCEL_SPI) += bmc150-accel-spi.o
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> new file mode 100644
> index 000000000000..7fa92bc457f6
> --- /dev/null
> +++ b/drivers/iio/accel/bma400.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * bma400.h - Register constants and other forward declarations
> + *            needed by the bma400 sources.
> + *
> + * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
> + *

Blank line not needed.

> + */
> +
> +#include <linux/regmap.h>
> +
> +/*
> + * Read-Only Registers
> + */
> +
> +/* Status and ID registers */
> +#define BMA400_CHIP_ID_REG      0x00
> +#define BMA400_ERR_REG          0x02
> +#define BMA400_STATUS_REG       0x03
> +
> +/* Acceleration registers */
> +#define BMA400_X_AXIS_LSB_REG   0x04
> +#define BMA400_X_AXIS_MSB_REG   0x05
> +#define BMA400_Y_AXIS_LSB_REG   0x06
> +#define BMA400_Y_AXIS_MSB_REG   0x07
> +#define BMA400_Z_AXIS_LSB_REG   0x08
> +#define BMA400_Z_AXIS_MSB_REG   0x09
> +
> +/* Sensort time registers */
> +#define BMA400_SENSOR_TIME0     0x0a
> +#define BMA400_SENSOR_TIME1     0x0b
> +#define BMA400_SENSOR_TIME2     0x0c
> +
> +/* Event and interrupt registers */
> +#define BMA400_EVENT_REG        0x0d
> +#define BMA400_INT_STAT0_REG    0x0e
> +#define BMA400_INT_STAT1_REG    0x0f
> +#define BMA400_INT_STAT2_REG    0x10
> +
> +/* Temperature register */
> +#define BMA400_TEMP_DATA_REG    0x11
> +
> +/* FIFO length and data registers */
> +#define BMA400_FIFO_LENGTH0_REG 0x12
> +#define BMA400_FIFO_LENGTH1_REG 0x13
> +#define BMA400_FIFO_DATA_REG    0x14
> +
> +/* Step count registers */
> +#define BMA400_STEP_CNT0_REG    0x15
> +#define BMA400_STEP_CNT1_REG    0x16
> +#define BMA400_STEP_CNT3_REG    0x17
> +#define BMA400_STEP_STAT_REG    0x18
> +
> +/*
> + * Read-write configuration registers
> + */
> +#define BMA400_ACC_CONFIG0_REG  0x19
> +#define BMA400_ACC_CONFIG1_REG  0x1a
> +#define BMA400_ACC_CONFIG2_REG  0x1b
> +#define BMA400_CMD_REG          0x7e
> +
> +/* Chip ID of BMA 400 devices found in the chip ID register. */
> +#define BMA400_ID_REG_VAL       0x90
> +
> +/* The softreset command */
> +#define BMA400_SOFTRESET_CMD    0xb6
> +
> +extern const struct regmap_config bma400_regmap_config;
> +
> +int bma400_probe(struct device *dev,
> +		 struct regmap *regmap,
> +		 const char *name);
> +
> +int bma400_remove(struct device *dev);
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> new file mode 100644
> index 000000000000..55fe2f220c30
> --- /dev/null
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -0,0 +1,862 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * bma400-core.c - Core IIO driver for Bosch BMA400 triaxial acceleration
> + *                 sensor. Used by bma400-i2c.
> + *
> + * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
> + *
> + * TODO:
> + *  - Support for power management
> + *  - Support events and interrupts
> + *  - Create channel the step count
> + *  - Create channel for sensor time
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/bitops.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#include "bma400.h"
> +
> +/*
> + * The G-range selection may be one of 2g, 4g, 8, or 16g. The scale may
> + * be selected with the acc_range bits of the ACC_CONFIG1 register.
> + */
> +const int bma400_scale_table[] = { 38344, 76590, 153277, 306457 };
> +
> +struct bma400_sample_freq {
> +	int hz;
> +	int micro_hz;
> +};
> +
> +/* See the ACC_CONFIG1 section of the datasheet */
> +const struct bma400_sample_freq bma400_sample_freqs[] = {
> +	{ .hz = 12, .micro_hz = 500000 },
> +	{ .hz = 25, .micro_hz = 0 },
> +	{ .hz = 50, .micro_hz = 0 },
> +	{ .hz = 100, .micro_hz = 0 },
> +	{ .hz = 200, .micro_hz = 0 },
> +	{ .hz = 400, .micro_hz = 0 },
> +	{ .hz = 800, .micro_hz = 0 },
> +};
> +
> +/* See the ACC_CONFIG0 section of the datasheet */
> +enum bma400_power_mode {
> +	POWER_MODE_SLEEP   = 0x00,
> +	POWER_MODE_LOW     = 0x01,
> +	POWER_MODE_NORMAL  = 0x02,
> +	POWER_MODE_INVALID = 0x03,
> +};
> +
> +struct bma400_data {
> +	struct device *dev;
> +	struct mutex mutex; /* data register lock */
> +	struct iio_mount_matrix orientation;
> +	struct regmap *regmap;
> +	enum bma400_power_mode power_mode;
> +	const struct bma400_sample_freq *sample_freq;
> +	int oversampling_ratio;
> +	int scale;
> +};
> +
> +static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case BMA400_CHIP_ID_REG:
> +	case BMA400_ERR_REG:
> +	case BMA400_STATUS_REG:
> +	case BMA400_X_AXIS_LSB_REG:
> +	case BMA400_X_AXIS_MSB_REG:
> +	case BMA400_Y_AXIS_LSB_REG:
> +	case BMA400_Y_AXIS_MSB_REG:
> +	case BMA400_Z_AXIS_LSB_REG:
> +	case BMA400_Z_AXIS_MSB_REG:
> +	case BMA400_SENSOR_TIME0:
> +	case BMA400_SENSOR_TIME1:
> +	case BMA400_SENSOR_TIME2:
> +	case BMA400_EVENT_REG:
> +	case BMA400_INT_STAT0_REG:
> +	case BMA400_INT_STAT1_REG:
> +	case BMA400_INT_STAT2_REG:
> +	case BMA400_TEMP_DATA_REG:
> +	case BMA400_FIFO_LENGTH0_REG:
> +	case BMA400_FIFO_LENGTH1_REG:
> +	case BMA400_FIFO_DATA_REG:
> +	case BMA400_STEP_CNT0_REG:
> +	case BMA400_STEP_CNT1_REG:
> +	case BMA400_STEP_CNT3_REG:
> +	case BMA400_STEP_STAT_REG:
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
> +static bool bma400_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case BMA400_ERR_REG:
> +	case BMA400_STATUS_REG:
> +	case BMA400_X_AXIS_LSB_REG:
> +	case BMA400_X_AXIS_MSB_REG:
> +	case BMA400_Y_AXIS_LSB_REG:
> +	case BMA400_Y_AXIS_MSB_REG:
> +	case BMA400_Z_AXIS_LSB_REG:
> +	case BMA400_Z_AXIS_MSB_REG:
> +	case BMA400_SENSOR_TIME0:
> +	case BMA400_SENSOR_TIME1:
> +	case BMA400_SENSOR_TIME2:
> +	case BMA400_EVENT_REG:
> +	case BMA400_INT_STAT0_REG:
> +	case BMA400_INT_STAT1_REG:
> +	case BMA400_INT_STAT2_REG:
> +	case BMA400_TEMP_DATA_REG:
> +	case BMA400_FIFO_LENGTH0_REG:
> +	case BMA400_FIFO_LENGTH1_REG:
> +	case BMA400_FIFO_DATA_REG:
> +	case BMA400_STEP_CNT0_REG:
> +	case BMA400_STEP_CNT1_REG:
> +	case BMA400_STEP_CNT3_REG:
> +	case BMA400_STEP_STAT_REG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +const struct regmap_config bma400_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = BMA400_CMD_REG,
> +	.cache_type = REGCACHE_RBTREE,
> +	.writeable_reg = bma400_is_writable_reg,
> +	.volatile_reg = bma400_is_volatile_reg,
> +};
> +EXPORT_SYMBOL(bma400_regmap_config);
> +
> +static const struct iio_mount_matrix *
> +bma400_accel_get_mount_matrix(const struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan)
> +{
> +	struct bma400_data *data = iio_priv(indio_dev);
> +
> +	return &data->orientation;
> +}
> +
> +static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bma400_accel_get_mount_matrix),
> +	{ }
> +};
> +
> +#define BMA400_ACC_CHANNEL(_axis) { \
> +	.type = IIO_ACCEL, \
> +	.modified = 1, \
> +	.channel2 = IIO_MOD_##_axis, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +		BIT(IIO_CHAN_INFO_SCALE) | \
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> +	.ext_info = bma400_ext_info \
> +}
> +
> +static const struct iio_chan_spec bma400_channels[] = {
> +	BMA400_ACC_CHANNEL(X),
> +	BMA400_ACC_CHANNEL(Y),
> +	BMA400_ACC_CHANNEL(Z),
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	},
> +};
> +
> +static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> +{
> +	int ret;
> +	int host_temp;
> +	unsigned int raw_temp;
> +
> +	if (data->power_mode == POWER_MODE_SLEEP)
> +		return -EBUSY;
> +
> +	ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &raw_temp);
> +
> +	if (!ret) {
Error path out of line, not the good one. See below.

> +		host_temp = sign_extend32(raw_temp, 7);
> +		/*
> +		 * The formula for the TEMP_DATA register in the datasheet
> +		 * is: x * 0.5 + 23
> +		 */
> +		*val = (host_temp >> 1) + 23;
> +		*val2 = (host_temp & 0x1) * 500000;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	return ret;
> +}
> +
> +static int bma400_get_accel_reg(struct bma400_data *data,
> +				const struct iio_chan_spec *chan,
> +				int *val)
> +{
> +	int ret;
> +	u16 lsb_reg;
> +	__le16 raw_accel = 0;

I don't think we need the assignment... Is there a compiler that
can't figure that out?

> +
> +	if (data->power_mode == POWER_MODE_SLEEP)
> +		return -EBUSY;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +		lsb_reg = BMA400_X_AXIS_LSB_REG;
> +		break;
> +	case IIO_MOD_Y:
> +		lsb_reg = BMA400_Y_AXIS_LSB_REG;
> +		break;
> +	case IIO_MOD_Z:
> +		lsb_reg = BMA400_Z_AXIS_LSB_REG;
> +		break;
> +	default:
> +		dev_err(data->dev, "invalid axis channel modifier");
> +		return -EINVAL;
> +	}
> +
> +	/* bulk read two registers, with the base being the LSB register */
> +	ret = regmap_bulk_read(data->regmap, lsb_reg, &raw_accel, 2);
> +	if (!ret) {

Same point as below on error path out of line.

> +		*val = sign_extend32(le16_to_cpu(raw_accel), 11);
> +		ret = IIO_VAL_INT;
> +	}
> +
> +	return ret;
> +}
> +
> +static int bma400_ready_for_cmd(struct bma400_data *data)
> +{
> +	unsigned int val;
> +	int ret = regmap_read(data->regmap, BMA400_STATUS_REG, &val);
> +
> +	if (ret < 0 || !(val & 0x10))
> +		return 0;
> +
> +	return 1;

Maybe... (up to you on this one as I'm not convinced this is better :)

	if (ret < 0)
		return 0;

	return (val & 0x10) ? 1 : 0;

Separates the error case from checking the parameter.
> +}
> +
> +static int bma400_softreset(struct bma400_data *data)
> +{
> +	int ret;
> +
> +	if (!bma400_ready_for_cmd(data))
> +		return -EAGAIN;
> +
> +	ret = regmap_write(data->regmap, BMA400_CMD_REG,
> +			   BMA400_SOFTRESET_CMD);
> +	if (!ret) {
As below, prefer the error path indented even if it costs a line or two of
code.
	if (ret)
		return ret;

	data->...
> +		/* a softreset restores registers to their defaults */
> +		data->power_mode = POWER_MODE_SLEEP;
> +		data->sample_freq = NULL;
> +		data->oversampling_ratio = -1;
> +		data->scale = bma400_scale_table[0];
> +	}
> +	return ret;
> +}
> +
> +static int bma400_get_acc_config0_reg(struct bma400_data *data)
> +{

I'm unconvinced these wrappers add anything. Just do the regmap_read
inline.

> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
> +			  &val);
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed read acc_config0");
> +		return ret;
> +	}
> +
> +	return val;
> +}
> +
> +static int bma400_get_acc_config1_reg(struct bma400_data *data)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG,
> +			  &val);
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed read acc_config1");
> +		return ret;
> +	}
> +
> +	return val;
> +}
> +
> +static int bma400_get_accel_output_data_rate(struct bma400_data *data)
> +{
> +	int acc_config1;
> +	unsigned int odr;
> +	int idx;
> +
> +	switch (data->power_mode) {
> +	case POWER_MODE_LOW:
> +		/*
> +		 * Runs at a fixed rate in low-power mode. See section 4.3
> +		 * in the datasheet.
> +		 */
> +		data->sample_freq = &bma400_sample_freqs[1];
> +		return 0;
> +	case POWER_MODE_NORMAL:
> +		/*
> +		 * In normal mode the ODR can be found in the ACC_CONFIG1
> +		 * register.
> +		 */
> +		acc_config1 = bma400_get_acc_config1_reg(data);
> +		if (acc_config1 < 0) {
> +			data->sample_freq = NULL;
> +			return acc_config1;
> +		}
> +
> +		odr = (acc_config1 & 0x0f);
> +		if (odr < 0x05 || odr > 0x0b) {
> +			dev_err(data->dev, "invalid ODR=%x", odr);
> +			data->sample_freq = NULL;
> +			return -EINVAL;
> +		}
> +		idx = odr - 0x05;
> +
> +		data->sample_freq = &bma400_sample_freqs[idx];
> +		return 0;
> +	default:
> +		data->sample_freq = NULL;
> +		return 0;
> +	}
> +}
> +
> +static int bma400_get_accel_output_data_rate_idx(struct bma400_data *data,
> +						 int hz, int uhz)
> +{
> +	int i;
> +	const struct bma400_sample_freq *sample_freq;
> +
> +	for (i = 0; i < ARRAY_SIZE(bma400_sample_freqs); ++i) {
> +		sample_freq = &bma400_sample_freqs[i];
> +		if (sample_freq->hz == hz &&
> +		    sample_freq->micro_hz == uhz)
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int bma400_set_accel_output_data_rate(struct bma400_data *data,
> +					     int hz, int uhz)
> +{
> +	int acc_config1;
> +	unsigned int odr;
> +	int idx;
> +	int ret;
> +
> +	idx = bma400_get_accel_output_data_rate_idx(data, hz, uhz);
> +
> +	if (idx < 0)
> +		return idx;
> +
> +	acc_config1 = bma400_get_acc_config1_reg(data);
> +
> +	if (acc_config1 < 0)
> +		return acc_config1;
> +
> +	/* preserve the range and normal mode osr */
> +	odr = (0xf0 & acc_config1) | (idx + 0x5);
> +
> +	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG, odr);
> +	if (!ret)
> +		data->sample_freq = &bma400_sample_freqs[idx];

I'd rather we kept error flow indented even if a bit longer. It means
these fine error checks are handled the same as earlier ones and makes
reviewing a little bit easier.

	if (ret)
		return ret;

	data->...

> +
> +	return ret;
> +}
> +
> +static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
> +{
> +	int acc_config;
> +
> +	/*
> +	 * The oversampling ratio is stored in a different register
> +	 * based on the power-mode. In normal mode the OSR is stored
> +	 * in ACC_CONFIG1. In low-power mode it is stored in
> +	 * ACC_CONFIG0.
> +	 */
> +	switch (data->power_mode) {
> +	case POWER_MODE_LOW:
> +		acc_config = bma400_get_acc_config0_reg(data);
> +		if (acc_config < 0) {
> +			data->oversampling_ratio = -1;
> +			return acc_config;
> +		}
> +
> +		data->oversampling_ratio = (acc_config & 0x60) >> 5;
> +		return 0;
> +	case POWER_MODE_NORMAL:
> +		acc_config = bma400_get_acc_config1_reg(data);
> +		if (acc_config < 0) {
> +			data->oversampling_ratio = -1;
> +			return acc_config;
> +		}
> +
> +		data->oversampling_ratio = (acc_config & 0x30) >> 4;
> +		return 0;
> +	default:
> +		data->oversampling_ratio = -1;
> +		return 0;
> +	}
> +}
> +
> +static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
> +					       int val)
> +{
> +	int ret;
> +	int acc_config;
> +
> +	/* The oversampling ratio is a two bit field */
> +	if (val & ~0x3)

Use a define for that mask. Then we don't need the comment.

> +		return -EINVAL;
> +
> +	/* The oversampling ratio is stored in a different register

Most of the kernel, including iio, use
/*
 * The...
 * ..
 */

> +	 * based on the power-mode.
> +	 */
> +	switch (data->power_mode) {
> +	case POWER_MODE_LOW:
> +		acc_config = bma400_get_acc_config0_reg(data);
> +		if (acc_config < 0)
> +			return acc_config;
> +
> +		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> +				   (acc_config & 0x9f) | (val << 5));

Use defines for these masks.

> +		if (ret < 0)
> +			dev_err(data->dev, "Failed to write out OSR");
> +		else
> +			data->oversampling_ratio = val;
> +		break;
> +	case POWER_MODE_NORMAL:
> +		acc_config = bma400_get_acc_config1_reg(data);
> +		if (acc_config < 0)
> +			return acc_config;
> +
> +		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> +				   (acc_config & 0xcf) | (val << 4));
> +		if (ret < 0)
> +			dev_err(data->dev, "Failed to write out OSR");
> +		else
> +			data->oversampling_ratio = val;
Nicer (even if a little longer) to read as:

		if (ret < 0) {
			dev_err(data->dev, "Failed to write out OSR");
			return ret;
		}

		data->oversampling_ration = val;

		return 0;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;


> +}
> +
> +static int bma400_get_accel_scale(struct bma400_data *data)
> +{
> +	int idx;
> +	int acc_config1 = bma400_get_acc_config1_reg(data);
> +
> +	if (acc_config1 < 0)
> +		return acc_config1;
> +
> +	idx = (acc_config1 & 0xc0) >> 6;
> +	if (idx < ARRAY_SIZE(bma400_scale_table)) {
> +		data->scale = bma400_scale_table[idx];
> +		return 0;

Preference for error path indented in cases where it doesn't
make a substantial difference in code length.

	if (idx >= ARRAY_SIZE(bma400_scale_table)
		return -EINVAL;

	data->scale = bma400_scale_table[idx];

	return 0;

> +	}
> +	return -EINVAL;
> +}
> +
> +static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(bma400_scale_table); ++i) {
> +		if (bma400_scale_table[i] == val)
> +			return i;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int bma400_set_accel_scale(struct bma400_data *data, unsigned int val)
> +{
> +	int ret;
> +	int idx;
> +	int acc_config1 = bma400_get_acc_config1_reg(data);
> +
> +	if (acc_config1 < 0)
> +		return acc_config1;
> +
> +	idx = bma400_get_accel_scale_idx(data, val);
> +
> +	if (idx < 0)
> +		return idx;
> +
> +	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> +			   (acc_config1 & 0x3f) | (idx << 6));
> +	if (ret < 0)
> +		return ret;
> +
> +	data->scale = val;
> +	return 0;
> +}
> +
> +static int bma400_get_power_mode(struct bma400_data *data)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(data->regmap, BMA400_STATUS_REG, &val);
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed to read status register");
> +		return ret;
> +	}
> +
> +	data->power_mode = (val >> 1) & 0x03;

Blank line here and in similar cases helps readability (a tiny bit).

> +	return 0;
> +}
> +
> +static int bma400_set_power_mode(struct bma400_data *data,
> +				 enum bma400_power_mode mode)
> +{
> +	int ret;
> +
> +	ret = bma400_get_acc_config0_reg(data);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (data->power_mode == mode)
> +		return 0;
> +
> +	if (mode == POWER_MODE_INVALID)
> +		return -EINVAL;
> +
> +	/* Preserve the low-power oversample ratio etc */
> +	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> +			   mode | (ret & 0xfc));
> +
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed to write to power-mode");
> +		return ret;
> +	}
> +
> +	/* Update our cached power-mode */

Bit marginal on whether that comment adds anything not obvious from
the code.  From a maintenance point of view, excess comments are
just opportunities for rot. Hence we are generally only in favour
of those that add significant value.

> +	data->power_mode = mode;
> +
> +	/*
> +	 * Update our cached osr and odr based on the new
> +	 * power-mode.
> +	 */
> +	bma400_get_accel_output_data_rate(data);
> +	bma400_get_accel_oversampling_ratio(data);
> +
> +	return 0;
> +}
> +
> +static int bma400_init(struct bma400_data *data)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	/* Try to read chip_id register. It must return 0x90. */
> +	ret = regmap_read(data->regmap, BMA400_CHIP_ID_REG, &val);
> +
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed to read chip id register: %x!", ret);
> +		return ret;
> +	} else if (val != BMA400_ID_REG_VAL) {
> +		dev_err(data->dev, "CHIP ID MISMATCH: %x!", ret);
> +		return -ENODEV;
> +	}
> +
> +	ret = bma400_get_power_mode(data);
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed to get the initial power-mode!");
> +		return ret;
> +	}
> +
> +	if (data->power_mode != POWER_MODE_NORMAL) {
> +		ret = bma400_set_power_mode(data, POWER_MODE_NORMAL);
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to wake up the device!");
> +			return ret;
> +		}
> +		/*
> +		 * TODO: The datasheet waits 1500us here in the example, but
> +		 * lists 2/ODR as the wakeup time.
> +		 */
> +		usleep_range(1500, 20000);
> +	}
> +
> +	ret = bma400_get_accel_output_data_rate(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = bma400_get_accel_oversampling_ratio(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = bma400_get_accel_scale(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Once the interrupt engine is supported we might use the
> +	 * data_src_reg, but for now ensure this is set to the
> +	 * variable ODR filter selectable by the sample frequency
> +	 * channel.
> +	 */
> +	return regmap_write(data->regmap, BMA400_ACC_CONFIG2_REG, 0x00);
> +}
> +
> +static ssize_t bma400_show_samp_freq_avail(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	int i;
> +	int len = 0;
> +	const struct bma400_sample_freq *sample_freq;
> +
> +	for (i = 0; i < ARRAY_SIZE(bma400_sample_freqs); ++i) {
> +		sample_freq = &bma400_sample_freqs[i];
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
> +				 sample_freq->hz, sample_freq->micro_hz);
> +	}
> +
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static ssize_t bma400_show_scale_avail(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
As you are providing functions for it anyway, perhaps use the core
functions to do this?
See the docs for struct iio_info -> read_avail.

Not that many users yet as we added this for a few specific cases
that needed to query these from in kernel users.


> +{
> +	int i;
> +	int len = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(bma400_scale_table); ++i)
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06d ",
> +				 bma400_scale_table[i]);
> +
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(bma400_show_samp_freq_avail);
> +
> +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> +		       bma400_show_scale_avail, NULL, 0);
> +
> +static struct attribute *bma400_attributes[] = {
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> +	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group bma400_attrs_group = {
> +	.attrs = bma400_attributes,
> +};
> +
> +static int bma400_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	struct bma400_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		mutex_lock(&data->mutex);
> +		ret = bma400_get_temp_reg(data, val, val2);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&data->mutex);
> +		ret = bma400_get_accel_reg(data, chan, val);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			if (!data->sample_freq)
> +				return -EINVAL;
> +
> +			*val = data->sample_freq->hz;
> +			if (!data->sample_freq->micro_hz)
> +				return IIO_VAL_INT;
> +
> +			*val2 = data->sample_freq->micro_hz;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_TEMP:
> +			/*
> +			 * Runs at a fixed sampling frequency. See Section 4.4
> +			 * of the datasheet.
> +			 */
> +			*val = 6;
> +			*val2 = 250000;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		*val2 = data->scale;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		/*
> +		 * TODO: We could avoid this logic and returning -EINVAL here if
> +		 * we set both the low-power and normal mode OSR registers when
> +		 * we configure the device.
> +		 */
> +		if (data->oversampling_ratio < 0)
> +			return -EINVAL;
> +
> +		*val = data->oversampling_ratio;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bma400_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long mask)
> +{
> +	int ret;
> +	struct bma400_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		/*
> +		 * The sample frequency is readonly for the temperature
> +		 * register and a fixed value in low-power mode.
> +		 */
> +		if (chan->type != IIO_ACCEL)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->mutex);
> +		ret = bma400_set_accel_output_data_rate(data, val, val2);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->mutex);
> +		ret = bma400_set_accel_scale(data, val2);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		mutex_lock(&data->mutex);
> +		ret = bma400_set_accel_oversampling_ratio(data, val);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan,
> +				    long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SCALE:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info bma400_info = {
> +	.attrs             = &bma400_attrs_group,
> +	.read_raw          = bma400_read_raw,
> +	.write_raw         = bma400_write_raw,
> +	.write_raw_get_fmt = bma400_write_raw_get_fmt,
> +};
> +
> +int bma400_probe(struct device *dev,
> +		 struct regmap *regmap,
> +		 const char *name)
> +{
> +	int ret;
> +	struct bma400_data *data;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->regmap = regmap;
> +	data->dev = dev;
> +
> +	ret = bma400_init(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = iio_read_mount_matrix(dev, "mount-matrix",
> +				    &data->orientation);

Looks like the above will fit on one line..

> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&data->mutex);
> +	indio_dev->dev.parent = dev;
> +	indio_dev->name = name;
> +	indio_dev->info = &bma400_info;
> +	indio_dev->channels = bma400_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(bma400_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	dev_set_drvdata(dev, indio_dev);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to register iio device\n");
> +		return ret;

Drop the return out of the if statement and no need for the return 0 below.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(bma400_probe);
> +
> +int bma400_remove(struct device *dev)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct bma400_data *data = iio_priv(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +	ret = bma400_softreset(data);
> +	if (ret < 0) {
> +		/*
> +		 * If the softreset failed, try to put the device in
> +		 * sleep mode, but still report the error.
> +		 */
> +		dev_err(data->dev, "Failed to reset the device");
> +		bma400_set_power_mode(data, POWER_MODE_SLEEP);
> +	}
> +	mutex_unlock(&data->mutex);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(bma400_remove);
> +
> +MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
> +MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
> new file mode 100644
> index 000000000000..227012a32e13
> --- /dev/null
> +++ b/drivers/iio/accel/bma400_i2c.c
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * bma400-i2c.c - I2C IIO driver for Bosch BMA400 triaxial acceleration sensor.
> + *
> + * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
> + *
> + * I2C address is either 0x14 or 0x15 depending on SDO
> + *

This blank line doesn't add anything (nitpick)

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

Slight preference for alphabetical order.  Also why ACPI?

> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +#include "bma400.h"
> +
> +static int bma400_i2c_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *id)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i2c(client,
> +				      &bma400_regmap_config);
> +
> +	return bma400_probe(&client->dev, regmap, id->name);
> +}
> +
> +static int bma400_i2c_remove(struct i2c_client *client)
> +{
> +	return bma400_remove(&client->dev);
> +}
> +
> +static const struct i2c_device_id bma400_i2c_ids[] = {
> +	{ "bma400", 0 },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, bma400_i2c_ids);
> +

Good to have a of_device_id table as well from the start.
There is a general (but slow) move to stop using the fallback
to the i2c_device_id table.

> +static struct i2c_driver bma400_i2c_driver = {
> +	.driver = {
> +		.name = "bma400",
> +	},
> +	.probe    = bma400_i2c_probe,
> +	.remove   = bma400_i2c_remove,
> +	.id_table = bma400_i2c_ids,
> +};
> +
> +module_i2c_driver(bma400_i2c_driver);
> +
> +MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
> +MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor");
> +MODULE_LICENSE("GPL");
> 
> 

Re: [PATCH 1/1] iio: (bma400) add driver for the BMA400

[Dan Robertson]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

Jonathan,

Thanks for the review! I _think_ I've made all the changes you mentioned
locally. I'll run some more tests and clean it up a bit before submitting the
next patchset version.

> So my first thought was that Bosch have been moderately good at keeping their
> interfaces consistent across generations.  Seems not this time from
> a quick look!

Originally I had hoped to extend the bma180 driver, but there did seem to be a
reasonably large change in the interfaces. I also hope to add support for some
of the newer features and SPI. I think it would be possible to merge the two
drivers, but I think it would require pretty close to a complete overhaul of
the bma180 driver. If this is something you'd like to explore, I can probably
order a breakout board for the BMA180 and BMA220 and work on a way to create a
driver that supports all three.

> I'd also expect a modern driver to have DT bindings.  Might let that go if
> it was being used with ACPI but neither is true here.

Good catch. The DT bindings will be very simple in the initial version of the
driver. I'll add the bindings and some documentation.

> See the docs for struct iio_info -> read_avail.

Nice! I missed this. This fits exactly what I needed.

Cheers,

Dan

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/1] iio: (bma400) add driver for the BMA400

[Peter Meerwald-Stadler]

On Sun, 6 Oct 2019, Jonathan Cameron wrote:

> On Wed, 25 Sep 2019 01:39:41 +0000
> Dan Robertson <dan@dlrobertson.com> wrote:
> 
> > Add a IIO driver for the Bosch BMA400 3-axes ultra-low power accelerometer.
> > The driver supports reading from the acceleration and temperature
> > registers. The driver also supports reading and configuring the output data
> > rate, oversampling ratio, and scale.

some more comments (on top of Jonathan's)
 
> > Signed-off-by: Dan Robertson <dan@dlrobertson.com>
> Hi Dan,
> 
> So my first thought was that Bosch have been moderately good at keeping their
> interfaces consistent across generations.   Seems not this time from
> a quick look!
> 
> Minor stuff inline.  Quite a bit of it is stylistic rather than functional.
> I'm a lazy maintainer and tend to end up reading a lot of code so want to
> think as little as possible ;)
> 
> I'd also expect a modern driver to have DT bindings.  Might let that go if
> it was being used with ACPI but neither is true here.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/accel/Kconfig       |  19 +
> >  drivers/iio/accel/Makefile      |   2 +
> >  drivers/iio/accel/bma400.h      |  74 +++
> >  drivers/iio/accel/bma400_core.c | 862 ++++++++++++++++++++++++++++++++
> >  drivers/iio/accel/bma400_i2c.c  |  54 ++
> >  5 files changed, 1011 insertions(+)
> >  create mode 100644 drivers/iio/accel/bma400.h
> >  create mode 100644 drivers/iio/accel/bma400_core.c
> >  create mode 100644 drivers/iio/accel/bma400_i2c.c
> > 
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index 9b9656ce37e6..cca6727e037e 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -112,6 +112,25 @@ config BMA220
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called bma220_spi.
> >  
> > +config BMA400
> > +	tristate "Bosch BMA400 3-Axis Accelerometer Driver"
> > +	depends on I2C
> > +	select REGMAP
> > +	select BMA400_I2C if (I2C)
> > +	help
> > +	  Say Y here if you want to build a driver for the Bosch BMA400
> > +	  triaxial acceleration sensor.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called bma400_core and you will also get
> > +	  bma400_i2c for I2C
> > +
> > +config BMA400_I2C
> > +	tristate
> > +	depends on BMA400
> > +	depends on I2C
> > +	select REGMAP_I2C
> > +
> >  config BMC150_ACCEL
> >  	tristate "Bosch BMC150 Accelerometer Driver"
> >  	select IIO_BUFFER
> > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > index 56bd0215e0d4..3a051cf37f40 100644
> > --- a/drivers/iio/accel/Makefile
> > +++ b/drivers/iio/accel/Makefile
> > @@ -14,6 +14,8 @@ obj-$(CONFIG_ADXL372_I2C) += adxl372_i2c.o
> >  obj-$(CONFIG_ADXL372_SPI) += adxl372_spi.o
> >  obj-$(CONFIG_BMA180) += bma180.o
> >  obj-$(CONFIG_BMA220) += bma220_spi.o
> > +obj-$(CONFIG_BMA400) += bma400_core.o
> > +obj-$(CONFIG_BMA400_I2C) += bma400_i2c.o
> >  obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
> >  obj-$(CONFIG_BMC150_ACCEL_I2C) += bmc150-accel-i2c.o
> >  obj-$(CONFIG_BMC150_ACCEL_SPI) += bmc150-accel-spi.o
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > new file mode 100644
> > index 000000000000..7fa92bc457f6
> > --- /dev/null
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -0,0 +1,74 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * bma400.h - Register constants and other forward declarations
> > + *            needed by the bma400 sources.
> > + *
> > + * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
> > + *
> 
> Blank line not needed.
> 
> > + */
> > +
> > +#include <linux/regmap.h>
> > +
> > +/*
> > + * Read-Only Registers
> > + */
> > +
> > +/* Status and ID registers */
> > +#define BMA400_CHIP_ID_REG      0x00
> > +#define BMA400_ERR_REG          0x02
> > +#define BMA400_STATUS_REG       0x03
> > +
> > +/* Acceleration registers */
> > +#define BMA400_X_AXIS_LSB_REG   0x04
> > +#define BMA400_X_AXIS_MSB_REG   0x05
> > +#define BMA400_Y_AXIS_LSB_REG   0x06
> > +#define BMA400_Y_AXIS_MSB_REG   0x07
> > +#define BMA400_Z_AXIS_LSB_REG   0x08
> > +#define BMA400_Z_AXIS_MSB_REG   0x09
> > +
> > +/* Sensort time registers */

Sensor time

> > +#define BMA400_SENSOR_TIME0     0x0a
> > +#define BMA400_SENSOR_TIME1     0x0b
> > +#define BMA400_SENSOR_TIME2     0x0c
> > +
> > +/* Event and interrupt registers */
> > +#define BMA400_EVENT_REG        0x0d
> > +#define BMA400_INT_STAT0_REG    0x0e
> > +#define BMA400_INT_STAT1_REG    0x0f
> > +#define BMA400_INT_STAT2_REG    0x10
> > +
> > +/* Temperature register */
> > +#define BMA400_TEMP_DATA_REG    0x11
> > +
> > +/* FIFO length and data registers */
> > +#define BMA400_FIFO_LENGTH0_REG 0x12
> > +#define BMA400_FIFO_LENGTH1_REG 0x13
> > +#define BMA400_FIFO_DATA_REG    0x14
> > +
> > +/* Step count registers */
> > +#define BMA400_STEP_CNT0_REG    0x15
> > +#define BMA400_STEP_CNT1_REG    0x16
> > +#define BMA400_STEP_CNT3_REG    0x17
> > +#define BMA400_STEP_STAT_REG    0x18
> > +
> > +/*
> > + * Read-write configuration registers
> > + */
> > +#define BMA400_ACC_CONFIG0_REG  0x19
> > +#define BMA400_ACC_CONFIG1_REG  0x1a
> > +#define BMA400_ACC_CONFIG2_REG  0x1b
> > +#define BMA400_CMD_REG          0x7e
> > +
> > +/* Chip ID of BMA 400 devices found in the chip ID register. */
> > +#define BMA400_ID_REG_VAL       0x90
> > +
> > +/* The softreset command */
> > +#define BMA400_SOFTRESET_CMD    0xb6
> > +
> > +extern const struct regmap_config bma400_regmap_config;
> > +
> > +int bma400_probe(struct device *dev,
> > +		 struct regmap *regmap,
> > +		 const char *name);
> > +
> > +int bma400_remove(struct device *dev);
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > new file mode 100644
> > index 000000000000..55fe2f220c30
> > --- /dev/null
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -0,0 +1,862 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * bma400-core.c - Core IIO driver for Bosch BMA400 triaxial acceleration

bma400_core.c (underscore)

> > + *                 sensor. Used by bma400-i2c.
> > + *
> > + * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
> > + *
> > + * TODO:
> > + *  - Support for power management
> > + *  - Support events and interrupts
> > + *  - Create channel the step count
> > + *  - Create channel for sensor time
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/bitops.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#include "bma400.h"
> > +
> > +/*
> > + * The G-range selection may be one of 2g, 4g, 8, or 16g. The scale may
> > + * be selected with the acc_range bits of the ACC_CONFIG1 register.
> > + */
> > +const int bma400_scale_table[] = { 38344, 76590, 153277, 306457 };

static?

> > +
> > +struct bma400_sample_freq {
> > +	int hz;
> > +	int micro_hz;
> > +};
> > +
> > +/* See the ACC_CONFIG1 section of the datasheet */
> > +const struct bma400_sample_freq bma400_sample_freqs[] = {

static?

> > +	{ .hz = 12, .micro_hz = 500000 },
> > +	{ .hz = 25, .micro_hz = 0 },
> > +	{ .hz = 50, .micro_hz = 0 },
> > +	{ .hz = 100, .micro_hz = 0 },
> > +	{ .hz = 200, .micro_hz = 0 },
> > +	{ .hz = 400, .micro_hz = 0 },
> > +	{ .hz = 800, .micro_hz = 0 },
> > +};
> > +
> > +/* See the ACC_CONFIG0 section of the datasheet */
> > +enum bma400_power_mode {
> > +	POWER_MODE_SLEEP   = 0x00,
> > +	POWER_MODE_LOW     = 0x01,
> > +	POWER_MODE_NORMAL  = 0x02,
> > +	POWER_MODE_INVALID = 0x03,
> > +};
> > +
> > +struct bma400_data {
> > +	struct device *dev;
> > +	struct mutex mutex; /* data register lock */
> > +	struct iio_mount_matrix orientation;
> > +	struct regmap *regmap;
> > +	enum bma400_power_mode power_mode;
> > +	const struct bma400_sample_freq *sample_freq;
> > +	int oversampling_ratio;
> > +	int scale;
> > +};
> > +
> > +static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case BMA400_CHIP_ID_REG:
> > +	case BMA400_ERR_REG:
> > +	case BMA400_STATUS_REG:
> > +	case BMA400_X_AXIS_LSB_REG:
> > +	case BMA400_X_AXIS_MSB_REG:
> > +	case BMA400_Y_AXIS_LSB_REG:
> > +	case BMA400_Y_AXIS_MSB_REG:
> > +	case BMA400_Z_AXIS_LSB_REG:
> > +	case BMA400_Z_AXIS_MSB_REG:
> > +	case BMA400_SENSOR_TIME0:
> > +	case BMA400_SENSOR_TIME1:
> > +	case BMA400_SENSOR_TIME2:
> > +	case BMA400_EVENT_REG:
> > +	case BMA400_INT_STAT0_REG:
> > +	case BMA400_INT_STAT1_REG:
> > +	case BMA400_INT_STAT2_REG:
> > +	case BMA400_TEMP_DATA_REG:
> > +	case BMA400_FIFO_LENGTH0_REG:
> > +	case BMA400_FIFO_LENGTH1_REG:
> > +	case BMA400_FIFO_DATA_REG:
> > +	case BMA400_STEP_CNT0_REG:
> > +	case BMA400_STEP_CNT1_REG:
> > +	case BMA400_STEP_CNT3_REG:
> > +	case BMA400_STEP_STAT_REG:
> > +		return false;
> > +	default:
> > +		return true;
> > +	}
> > +}
> > +
> > +static bool bma400_is_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case BMA400_ERR_REG:
> > +	case BMA400_STATUS_REG:
> > +	case BMA400_X_AXIS_LSB_REG:
> > +	case BMA400_X_AXIS_MSB_REG:
> > +	case BMA400_Y_AXIS_LSB_REG:
> > +	case BMA400_Y_AXIS_MSB_REG:
> > +	case BMA400_Z_AXIS_LSB_REG:
> > +	case BMA400_Z_AXIS_MSB_REG:
> > +	case BMA400_SENSOR_TIME0:
> > +	case BMA400_SENSOR_TIME1:
> > +	case BMA400_SENSOR_TIME2:
> > +	case BMA400_EVENT_REG:
> > +	case BMA400_INT_STAT0_REG:
> > +	case BMA400_INT_STAT1_REG:
> > +	case BMA400_INT_STAT2_REG:
> > +	case BMA400_TEMP_DATA_REG:
> > +	case BMA400_FIFO_LENGTH0_REG:
> > +	case BMA400_FIFO_LENGTH1_REG:
> > +	case BMA400_FIFO_DATA_REG:
> > +	case BMA400_STEP_CNT0_REG:
> > +	case BMA400_STEP_CNT1_REG:
> > +	case BMA400_STEP_CNT3_REG:
> > +	case BMA400_STEP_STAT_REG:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +const struct regmap_config bma400_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = BMA400_CMD_REG,
> > +	.cache_type = REGCACHE_RBTREE,
> > +	.writeable_reg = bma400_is_writable_reg,
> > +	.volatile_reg = bma400_is_volatile_reg,
> > +};
> > +EXPORT_SYMBOL(bma400_regmap_config);
> > +
> > +static const struct iio_mount_matrix *
> > +bma400_accel_get_mount_matrix(const struct iio_dev *indio_dev,
> > +			      const struct iio_chan_spec *chan)
> > +{
> > +	struct bma400_data *data = iio_priv(indio_dev);
> > +
> > +	return &data->orientation;
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bma400_accel_get_mount_matrix),
> > +	{ }
> > +};
> > +
> > +#define BMA400_ACC_CHANNEL(_axis) { \
> > +	.type = IIO_ACCEL, \
> > +	.modified = 1, \
> > +	.channel2 = IIO_MOD_##_axis, \
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > +		BIT(IIO_CHAN_INFO_SCALE) | \
> > +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > +	.ext_info = bma400_ext_info \

end with ,

> > +}
> > +
> > +static const struct iio_chan_spec bma400_channels[] = {
> > +	BMA400_ACC_CHANNEL(X),
> > +	BMA400_ACC_CHANNEL(Y),
> > +	BMA400_ACC_CHANNEL(Z),
> > +	{
> > +		.type = IIO_TEMP,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > +	},
> > +};
> > +
> > +static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> > +{
> > +	int ret;
> > +	int host_temp;
> > +	unsigned int raw_temp;
> > +
> > +	if (data->power_mode == POWER_MODE_SLEEP)
> > +		return -EBUSY;
> > +
> > +	ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &raw_temp);
> > +
> > +	if (!ret) {
> Error path out of line, not the good one. See below.
> 
> > +		host_temp = sign_extend32(raw_temp, 7);
> > +		/*
> > +		 * The formula for the TEMP_DATA register in the datasheet
> > +		 * is: x * 0.5 + 23
> > +		 */
> > +		*val = (host_temp >> 1) + 23;
> > +		*val2 = (host_temp & 0x1) * 500000;
> > +		ret = IIO_VAL_INT_PLUS_MICRO;
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int bma400_get_accel_reg(struct bma400_data *data,
> > +				const struct iio_chan_spec *chan,
> > +				int *val)
> > +{
> > +	int ret;
> > +	u16 lsb_reg;

the BMA400 seems to use 8-bit register addresses, u16 doesn't make sense 
here (either u8 or just int)

> > +	__le16 raw_accel = 0;
> 
> I don't think we need the assignment... Is there a compiler that
> can't figure that out?
> 
> > +
> > +	if (data->power_mode == POWER_MODE_SLEEP)
> > +		return -EBUSY;
> > +
> > +	switch (chan->channel2) {
> > +	case IIO_MOD_X:
> > +		lsb_reg = BMA400_X_AXIS_LSB_REG;
> > +		break;
> > +	case IIO_MOD_Y:
> > +		lsb_reg = BMA400_Y_AXIS_LSB_REG;
> > +		break;
> > +	case IIO_MOD_Z:
> > +		lsb_reg = BMA400_Z_AXIS_LSB_REG;
> > +		break;
> > +	default:
> > +		dev_err(data->dev, "invalid axis channel modifier");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* bulk read two registers, with the base being the LSB register */
> > +	ret = regmap_bulk_read(data->regmap, lsb_reg, &raw_accel, 2);

2 == sizeof(rawaccel)

> > +	if (!ret) {
> 
> Same point as below on error path out of line.
> 
> > +		*val = sign_extend32(le16_to_cpu(raw_accel), 11);
> > +		ret = IIO_VAL_INT;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int bma400_ready_for_cmd(struct bma400_data *data)
> > +{
> > +	unsigned int val;
> > +	int ret = regmap_read(data->regmap, BMA400_STATUS_REG, &val);
> > +
> > +	if (ret < 0 || !(val & 0x10))
> > +		return 0;
> > +
> > +	return 1;
> 
> Maybe... (up to you on this one as I'm not convinced this is better :)
> 
> 	if (ret < 0)
> 		return 0;
> 
> 	return (val & 0x10) ? 1 : 0;
> 
> Separates the error case from checking the parameter.

the magic 0x10 would benefit from a #define

> > +}
> > +
> > +static int bma400_softreset(struct bma400_data *data)
> > +{
> > +	int ret;
> > +
> > +	if (!bma400_ready_for_cmd(data))
> > +		return -EAGAIN;
> > +
> > +	ret = regmap_write(data->regmap, BMA400_CMD_REG,
> > +			   BMA400_SOFTRESET_CMD);
> > +	if (!ret) {
> As below, prefer the error path indented even if it costs a line or two of
> code.
> 	if (ret)
> 		return ret;
> 
> 	data->...
> > +		/* a softreset restores registers to their defaults */
> > +		data->power_mode = POWER_MODE_SLEEP;
> > +		data->sample_freq = NULL;
> > +		data->oversampling_ratio = -1;
> > +		data->scale = bma400_scale_table[0];
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int bma400_get_acc_config0_reg(struct bma400_data *data)
> > +{
> 
> I'm unconvinced these wrappers add anything. Just do the regmap_read
> inline.
> 
> > +	int ret;
> > +	unsigned int val;
> > +
> > +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
> > +			  &val);
> > +	if (ret < 0) {
> > +		dev_err(data->dev, "Failed read acc_config0");
> > +		return ret;
> > +	}
> > +
> > +	return val;
> > +}
> > +
> > +static int bma400_get_acc_config1_reg(struct bma400_data *data)
> > +{
> > +	int ret;
> > +	unsigned int val;
> > +
> > +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG,
> > +			  &val);
> > +	if (ret < 0) {
> > +		dev_err(data->dev, "Failed read acc_config1");
> > +		return ret;
> > +	}
> > +
> > +	return val;
> > +}
> > +
> > +static int bma400_get_accel_output_data_rate(struct bma400_data *data)
> > +{
> > +	int acc_config1;
> > +	unsigned int odr;
> > +	int idx;
> > +
> > +	switch (data->power_mode) {
> > +	case POWER_MODE_LOW:
> > +		/*
> > +		 * Runs at a fixed rate in low-power mode. See section 4.3
> > +		 * in the datasheet.
> > +		 */
> > +		data->sample_freq = &bma400_sample_freqs[1];
> > +		return 0;
> > +	case POWER_MODE_NORMAL:
> > +		/*
> > +		 * In normal mode the ODR can be found in the ACC_CONFIG1
> > +		 * register.
> > +		 */
> > +		acc_config1 = bma400_get_acc_config1_reg(data);
> > +		if (acc_config1 < 0) {
> > +			data->sample_freq = NULL;
> > +			return acc_config1;
> > +		}
> > +
> > +		odr = (acc_config1 & 0x0f);
> > +		if (odr < 0x05 || odr > 0x0b) {
> > +			dev_err(data->dev, "invalid ODR=%x", odr);
> > +			data->sample_freq = NULL;
> > +			return -EINVAL;
> > +		}
> > +		idx = odr - 0x05;
> > +
> > +		data->sample_freq = &bma400_sample_freqs[idx];
> > +		return 0;
> > +	default:
> > +		data->sample_freq = NULL;
> > +		return 0;
> > +	}
> > +}
> > +
> > +static int bma400_get_accel_output_data_rate_idx(struct bma400_data *data,
> > +						 int hz, int uhz)
> > +{
> > +	int i;
> > +	const struct bma400_sample_freq *sample_freq;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(bma400_sample_freqs); ++i) {
> > +		sample_freq = &bma400_sample_freqs[i];
> > +		if (sample_freq->hz == hz &&
> > +		    sample_freq->micro_hz == uhz)
> > +			return i;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int bma400_set_accel_output_data_rate(struct bma400_data *data,
> > +					     int hz, int uhz)
> > +{
> > +	int acc_config1;
> > +	unsigned int odr;
> > +	int idx;
> > +	int ret;
> > +
> > +	idx = bma400_get_accel_output_data_rate_idx(data, hz, uhz);
> > +
> > +	if (idx < 0)
> > +		return idx;
> > +
> > +	acc_config1 = bma400_get_acc_config1_reg(data);
> > +
> > +	if (acc_config1 < 0)
> > +		return acc_config1;
> > +
> > +	/* preserve the range and normal mode osr */
> > +	odr = (0xf0 & acc_config1) | (idx + 0x5);
> > +
> > +	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG, odr);
> > +	if (!ret)
> > +		data->sample_freq = &bma400_sample_freqs[idx];
> 
> I'd rather we kept error flow indented even if a bit longer. It means
> these fine error checks are handled the same as earlier ones and makes
> reviewing a little bit easier.
> 
> 	if (ret)
> 		return ret;
> 
> 	data->...
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
> > +{
> > +	int acc_config;
> > +
> > +	/*
> > +	 * The oversampling ratio is stored in a different register
> > +	 * based on the power-mode. In normal mode the OSR is stored
> > +	 * in ACC_CONFIG1. In low-power mode it is stored in
> > +	 * ACC_CONFIG0.
> > +	 */
> > +	switch (data->power_mode) {
> > +	case POWER_MODE_LOW:
> > +		acc_config = bma400_get_acc_config0_reg(data);
> > +		if (acc_config < 0) {
> > +			data->oversampling_ratio = -1;
> > +			return acc_config;
> > +		}
> > +
> > +		data->oversampling_ratio = (acc_config & 0x60) >> 5;
> > +		return 0;
> > +	case POWER_MODE_NORMAL:
> > +		acc_config = bma400_get_acc_config1_reg(data);
> > +		if (acc_config < 0) {
> > +			data->oversampling_ratio = -1;
> > +			return acc_config;
> > +		}
> > +
> > +		data->oversampling_ratio = (acc_config & 0x30) >> 4;
> > +		return 0;
> > +	default:
> > +		data->oversampling_ratio = -1;
> > +		return 0;
> > +	}
> > +}
> > +
> > +static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
> > +					       int val)
> > +{
> > +	int ret;
> > +	int acc_config;
> > +
> > +	/* The oversampling ratio is a two bit field */
> > +	if (val & ~0x3)
> 
> Use a define for that mask. Then we don't need the comment.
> 
> > +		return -EINVAL;
> > +
> > +	/* The oversampling ratio is stored in a different register
> 
> Most of the kernel, including iio, use
> /*
>  * The...
>  * ..
>  */
> 
> > +	 * based on the power-mode.
> > +	 */
> > +	switch (data->power_mode) {
> > +	case POWER_MODE_LOW:
> > +		acc_config = bma400_get_acc_config0_reg(data);
> > +		if (acc_config < 0)
> > +			return acc_config;
> > +
> > +		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> > +				   (acc_config & 0x9f) | (val << 5));
> 
> Use defines for these masks.
> 
> > +		if (ret < 0)
> > +			dev_err(data->dev, "Failed to write out OSR");
> > +		else
> > +			data->oversampling_ratio = val;
> > +		break;
> > +	case POWER_MODE_NORMAL:
> > +		acc_config = bma400_get_acc_config1_reg(data);
> > +		if (acc_config < 0)
> > +			return acc_config;
> > +
> > +		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> > +				   (acc_config & 0xcf) | (val << 4));
> > +		if (ret < 0)
> > +			dev_err(data->dev, "Failed to write out OSR");
> > +		else
> > +			data->oversampling_ratio = val;
> Nicer (even if a little longer) to read as:
> 
> 		if (ret < 0) {
> 			dev_err(data->dev, "Failed to write out OSR");
> 			return ret;
> 		}
> 
> 		data->oversampling_ration = val;
> 
> 		return 0;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return ret;
> 
> 
> > +}
> > +
> > +static int bma400_get_accel_scale(struct bma400_data *data)
> > +{
> > +	int idx;
> > +	int acc_config1 = bma400_get_acc_config1_reg(data);
> > +
> > +	if (acc_config1 < 0)
> > +		return acc_config1;
> > +
> > +	idx = (acc_config1 & 0xc0) >> 6;
> > +	if (idx < ARRAY_SIZE(bma400_scale_table)) {
> > +		data->scale = bma400_scale_table[idx];
> > +		return 0;
> 
> Preference for error path indented in cases where it doesn't
> make a substantial difference in code length.
> 
> 	if (idx >= ARRAY_SIZE(bma400_scale_table)
> 		return -EINVAL;
> 
> 	data->scale = bma400_scale_table[idx];
> 
> 	return 0;
> 
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(bma400_scale_table); ++i) {
> > +		if (bma400_scale_table[i] == val)
> > +			return i;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +static int bma400_set_accel_scale(struct bma400_data *data, unsigned int val)
> > +{
> > +	int ret;
> > +	int idx;
> > +	int acc_config1 = bma400_get_acc_config1_reg(data);
> > +
> > +	if (acc_config1 < 0)
> > +		return acc_config1;
> > +
> > +	idx = bma400_get_accel_scale_idx(data, val);
> > +
> > +	if (idx < 0)
> > +		return idx;
> > +
> > +	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> > +			   (acc_config1 & 0x3f) | (idx << 6));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	data->scale = val;
> > +	return 0;
> > +}
> > +
> > +static int bma400_get_power_mode(struct bma400_data *data)
> > +{
> > +	int ret;
> > +	unsigned int val;
> > +
> > +	ret = regmap_read(data->regmap, BMA400_STATUS_REG, &val);
> > +	if (ret < 0) {
> > +		dev_err(data->dev, "Failed to read status register");
> > +		return ret;
> > +	}
> > +
> > +	data->power_mode = (val >> 1) & 0x03;
> 
> Blank line here and in similar cases helps readability (a tiny bit).
> 
> > +	return 0;
> > +}
> > +
> > +static int bma400_set_power_mode(struct bma400_data *data,
> > +				 enum bma400_power_mode mode)
> > +{
> > +	int ret;
> > +
> > +	ret = bma400_get_acc_config0_reg(data);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (data->power_mode == mode)
> > +		return 0;
> > +
> > +	if (mode == POWER_MODE_INVALID)
> > +		return -EINVAL;
> > +
> > +	/* Preserve the low-power oversample ratio etc */
> > +	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> > +			   mode | (ret & 0xfc));
> > +
> > +	if (ret < 0) {
> > +		dev_err(data->dev, "Failed to write to power-mode");
> > +		return ret;
> > +	}
> > +
> > +	/* Update our cached power-mode */
> 
> Bit marginal on whether that comment adds anything not obvious from
> the code.  From a maintenance point of view, excess comments are
> just opportunities for rot. Hence we are generally only in favour
> of those that add significant value.
> 
> > +	data->power_mode = mode;
> > +
> > +	/*
> > +	 * Update our cached osr and odr based on the new
> > +	 * power-mode.
> > +	 */
> > +	bma400_get_accel_output_data_rate(data);
> > +	bma400_get_accel_oversampling_ratio(data);
> > +
> > +	return 0;
> > +}
> > +
> > +static int bma400_init(struct bma400_data *data)
> > +{
> > +	int ret;
> > +	unsigned int val;
> > +
> > +	/* Try to read chip_id register. It must return 0x90. */
> > +	ret = regmap_read(data->regmap, BMA400_CHIP_ID_REG, &val);
> > +
> > +	if (ret < 0) {
> > +		dev_err(data->dev, "Failed to read chip id register: %x!", ret);
> > +		return ret;
> > +	} else if (val != BMA400_ID_REG_VAL) {
> > +		dev_err(data->dev, "CHIP ID MISMATCH: %x!", ret);
> > +		return -ENODEV;
> > +	}
> > +
> > +	ret = bma400_get_power_mode(data);
> > +	if (ret < 0) {
> > +		dev_err(data->dev, "Failed to get the initial power-mode!");
> > +		return ret;
> > +	}
> > +
> > +	if (data->power_mode != POWER_MODE_NORMAL) {
> > +		ret = bma400_set_power_mode(data, POWER_MODE_NORMAL);
> > +		if (ret < 0) {
> > +			dev_err(data->dev, "Failed to wake up the device!");
> > +			return ret;
> > +		}
> > +		/*
> > +		 * TODO: The datasheet waits 1500us here in the example, but
> > +		 * lists 2/ODR as the wakeup time.
> > +		 */
> > +		usleep_range(1500, 20000);
> > +	}
> > +
> > +	ret = bma400_get_accel_output_data_rate(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = bma400_get_accel_oversampling_ratio(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = bma400_get_accel_scale(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * Once the interrupt engine is supported we might use the
> > +	 * data_src_reg, but for now ensure this is set to the
> > +	 * variable ODR filter selectable by the sample frequency
> > +	 * channel.
> > +	 */
> > +	return regmap_write(data->regmap, BMA400_ACC_CONFIG2_REG, 0x00);
> > +}
> > +
> > +static ssize_t bma400_show_samp_freq_avail(struct device *dev,
> > +					   struct device_attribute *attr,
> > +					   char *buf)
> > +{
> > +	int i;
> > +	int len = 0;
> > +	const struct bma400_sample_freq *sample_freq;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(bma400_sample_freqs); ++i) {
> > +		sample_freq = &bma400_sample_freqs[i];
> > +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
> > +				 sample_freq->hz, sample_freq->micro_hz);
> > +	}
> > +
> > +	buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static ssize_t bma400_show_scale_avail(struct device *dev,
> > +				       struct device_attribute *attr,
> > +				       char *buf)
> As you are providing functions for it anyway, perhaps use the core
> functions to do this?
> See the docs for struct iio_info -> read_avail.
> 
> Not that many users yet as we added this for a few specific cases
> that needed to query these from in kernel users.
> 
> 
> > +{
> > +	int i;
> > +	int len = 0;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(bma400_scale_table); ++i)
> > +		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06d ",
> > +				 bma400_scale_table[i]);
> > +
> > +	buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(bma400_show_samp_freq_avail);
> > +
> > +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> > +		       bma400_show_scale_avail, NULL, 0);
> > +
> > +static struct attribute *bma400_attributes[] = {
> > +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > +	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group bma400_attrs_group = {
> > +	.attrs = bma400_attributes,
> > +};
> > +
> > +static int bma400_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan, int *val,
> > +			   int *val2, long mask)
> > +{
> > +	struct bma400_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		mutex_lock(&data->mutex);
> > +		ret = bma400_get_temp_reg(data, val, val2);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&data->mutex);
> > +		ret = bma400_get_accel_reg(data, chan, val);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		switch (chan->type) {
> > +		case IIO_ACCEL:
> > +			if (!data->sample_freq)
> > +				return -EINVAL;
> > +
> > +			*val = data->sample_freq->hz;
> > +			if (!data->sample_freq->micro_hz)
> > +				return IIO_VAL_INT;
> > +
> > +			*val2 = data->sample_freq->micro_hz;
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		case IIO_TEMP:
> > +			/*
> > +			 * Runs at a fixed sampling frequency. See Section 4.4
> > +			 * of the datasheet.
> > +			 */
> > +			*val = 6;
> > +			*val2 = 250000;
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = 0;
> > +		*val2 = data->scale;
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		/*
> > +		 * TODO: We could avoid this logic and returning -EINVAL here if
> > +		 * we set both the low-power and normal mode OSR registers when
> > +		 * we configure the device.
> > +		 */
> > +		if (data->oversampling_ratio < 0)
> > +			return -EINVAL;
> > +
> > +		*val = data->oversampling_ratio;
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int bma400_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int val, int val2,
> > +			    long mask)
> > +{
> > +	int ret;
> > +	struct bma400_data *data = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		/*
> > +		 * The sample frequency is readonly for the temperature
> > +		 * register and a fixed value in low-power mode.
> > +		 */
> > +		if (chan->type != IIO_ACCEL)
> > +			return -EINVAL;
> > +
> > +		mutex_lock(&data->mutex);
> > +		ret = bma400_set_accel_output_data_rate(data, val, val2);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (val != 0)
> > +			return -EINVAL;
> > +
> > +		mutex_lock(&data->mutex);
> > +		ret = bma400_set_accel_scale(data, val2);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		mutex_lock(&data->mutex);
> > +		ret = bma400_set_accel_oversampling_ratio(data, val);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> > +				    struct iio_chan_spec const *chan,
> > +				    long mask)
> > +{
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info bma400_info = {
> > +	.attrs             = &bma400_attrs_group,
> > +	.read_raw          = bma400_read_raw,
> > +	.write_raw         = bma400_write_raw,
> > +	.write_raw_get_fmt = bma400_write_raw_get_fmt,
> > +};
> > +
> > +int bma400_probe(struct device *dev,
> > +		 struct regmap *regmap,
> > +		 const char *name)
> > +{
> > +	int ret;
> > +	struct bma400_data *data;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->regmap = regmap;
> > +	data->dev = dev;
> > +
> > +	ret = bma400_init(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = iio_read_mount_matrix(dev, "mount-matrix",
> > +				    &data->orientation);
> 
> Looks like the above will fit on one line..
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_init(&data->mutex);
> > +	indio_dev->dev.parent = dev;
> > +	indio_dev->name = name;
> > +	indio_dev->info = &bma400_info;
> > +	indio_dev->channels = bma400_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(bma400_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	dev_set_drvdata(dev, indio_dev);
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "unable to register iio device\n");
> > +		return ret;
> 
> Drop the return out of the if statement and no need for the return 0 below.

I'd also avoid the error message altogether

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(bma400_probe);
> > +
> > +int bma400_remove(struct device *dev)
> > +{
> > +	int ret;
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct bma400_data *data = iio_priv(indio_dev);
> > +
> > +	mutex_lock(&data->mutex);
> > +	ret = bma400_softreset(data);
> > +	if (ret < 0) {
> > +		/*
> > +		 * If the softreset failed, try to put the device in
> > +		 * sleep mode, but still report the error.
> > +		 */
> > +		dev_err(data->dev, "Failed to reset the device");
> > +		bma400_set_power_mode(data, POWER_MODE_SLEEP);
> > +	}
> > +	mutex_unlock(&data->mutex);
> > +
> > +	iio_device_unregister(indio_dev);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(bma400_remove);
> > +
> > +MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
> > +MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
> > new file mode 100644
> > index 000000000000..227012a32e13
> > --- /dev/null
> > +++ b/drivers/iio/accel/bma400_i2c.c
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * bma400-i2c.c - I2C IIO driver for Bosch BMA400 triaxial acceleration sensor.
> > + *
> > + * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
> > + *
> > + * I2C address is either 0x14 or 0x15 depending on SDO
> > + *
> 
> This blank line doesn't add anything (nitpick)
> 
> > + */
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/acpi.h>
> 
> Slight preference for alphabetical order.  Also why ACPI?
> 
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +
> > +#include "bma400.h"
> > +
> > +static int bma400_i2c_probe(struct i2c_client *client,
> > +			    const struct i2c_device_id *id)
> > +{
> > +	struct regmap *regmap;
> > +
> > +	regmap = devm_regmap_init_i2c(client,
> > +				      &bma400_regmap_config);
> > +
> > +	return bma400_probe(&client->dev, regmap, id->name);
> > +}
> > +
> > +static int bma400_i2c_remove(struct i2c_client *client)
> > +{
> > +	return bma400_remove(&client->dev);
> > +}
> > +
> > +static const struct i2c_device_id bma400_i2c_ids[] = {
> > +	{ "bma400", 0 },
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, bma400_i2c_ids);
> > +
> 
> Good to have a of_device_id table as well from the start.
> There is a general (but slow) move to stop using the fallback
> to the i2c_device_id table.
> 
> > +static struct i2c_driver bma400_i2c_driver = {
> > +	.driver = {
> > +		.name = "bma400",
> > +	},
> > +	.probe    = bma400_i2c_probe,
> > +	.remove   = bma400_i2c_remove,
> > +	.id_table = bma400_i2c_ids,
> > +};
> > +
> > +module_i2c_driver(bma400_i2c_driver);
> > +
> > +MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
> > +MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor");
> > +MODULE_LICENSE("GPL");
> > 
> > 
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418