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

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

[Dan Robertson]

This patchset adds 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.

A huge thanks to the reviewers of the first version of the patchet!

Cheers,

 - Dan

Changes in v2:

 * Implemented iio_info -> read_avail
 * Stylistic changes
 * Implemented devicetree bindings

Dan Robertson (2):
  dt-bindings: iio: accel: bma400: add bindings
  iio: (bma400) add driver for the BMA400

 .../devicetree/bindings/iio/accel/bma400.txt  |  16 +
 drivers/iio/accel/Kconfig                     |  19 +
 drivers/iio/accel/Makefile                    |   2 +
 drivers/iio/accel/bma400.h                    |  86 ++
 drivers/iio/accel/bma400_core.c               | 839 ++++++++++++++++++
 drivers/iio/accel/bma400_i2c.c                |  58 ++
 6 files changed, 1020 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/bma400.txt
 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 v2 1/2] dt-bindings: iio: accel: bma400: add bindings

[Dan Robertson]

Add devicetree binding for the Bosch BMA400 3-axes ultra-low power
accelerometer sensor.

Signed-off-by: Dan Robertson <dan@dlrobertson.com>
---
 .../devicetree/bindings/iio/accel/bma400.txt     | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/bma400.txt

diff --git a/Documentation/devicetree/bindings/iio/accel/bma400.txt b/Documentation/devicetree/bindings/iio/accel/bma400.txt
new file mode 100644
index 000000000000..abba4f104941
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/bma400.txt
@@ -0,0 +1,16 @@
+
+* Bosch BMA400 triaxial acceleration sensor
+
+https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMA400-DS000.pdf
+
+Required properties:
+
+  - compatible : should be "bosch,bma400"
+  - reg : the I2C address of the sensor
+
+Example:
+
+bma400@14 {
+	compatible = "bosch,bma400";
+	reg = <0x14>;
+};

[PATCH v2 2/2] 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      |  86 ++++
 drivers/iio/accel/bma400_core.c | 839 ++++++++++++++++++++++++++++++++
 drivers/iio/accel/bma400_i2c.c  |  58 +++
 5 files changed, 1004 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..fa6e8dd598db
--- /dev/null
+++ b/drivers/iio/accel/bma400.h
@@ -0,0 +1,86 @@
+/* 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
+
+/* Sensor 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
+
+/*
+ * Commands accepted by the command register
+ */
+#define BMA400_SOFTRESET_CMD    0xb6
+
+#define BMA400_TWO_BITS_MASK    0x03
+#define BMA400_LP_OSR_MASK      0x60
+#define BMA400_NP_OSR_MASK      0x30
+#define BMA400_CMD_RDY_MASK     0x10
+#define BMA400_ACC_ODR_MASK     0x0f
+#define BMA400_ACC_SCALE_MASK   0xc0
+
+#define BMA400_LP_OSR_SHIFT     0x05
+#define BMA400_NP_OSR_SHIFT     0x04
+#define BMA400_SCALE_SHIFT      0x06
+
+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..5b3cb8919c47
--- /dev/null
+++ b/drivers/iio/accel/bma400_core.c
@@ -0,0 +1,839 @@
+// 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.
+ */
+static const int bma400_scale_table[] = {
+	0, 38344,
+	0, 76590,
+	0, 153277,
+	0, 306457
+};
+
+static const int bma400_osr_table[] = { 0, 1, 3 };
+
+/* See the ACC_CONFIG1 section of the datasheet */
+static const int bma400_sample_freqs[] = {
+	12,  500000,
+	25,  0,
+	50,  0,
+	100, 0,
+	200, 0,
+	400, 0,
+	800, 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 int *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), \
+	.info_mask_shared_by_type_available = 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 < 0)
+		return 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;
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int bma400_get_accel_reg(struct bma400_data *data,
+				const struct iio_chan_spec *chan,
+				int *val)
+{
+	int ret;
+	int lsb_reg;
+	__le16 raw_accel;
+
+	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,
+			       sizeof(raw_accel));
+	if (ret < 0)
+		return ret;
+
+	*val = sign_extend32(le16_to_cpu(raw_accel), 11);
+	return IIO_VAL_INT;
+}
+
+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)
+		return 0;
+
+	return (val & BMA400_CMD_RDY_MASK) ? 1 : 0;
+}
+
+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 < 0)
+		return 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[1];
+	return 0;
+}
+
+static int bma400_get_accel_output_data_rate(struct bma400_data *data)
+{
+	int ret;
+	unsigned int val;
+	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[2];
+		return 0;
+	case POWER_MODE_NORMAL:
+		/*
+		 * In normal mode the ODR can be found in the ACC_CONFIG1
+		 * register.
+		 */
+		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
+		if (ret < 0) {
+			data->sample_freq = NULL;
+			return ret;
+		}
+
+		odr = (val & BMA400_ACC_ODR_MASK);
+		if (odr < 0x05) {
+			dev_err(data->dev, "invalid ODR=%x", odr);
+			data->sample_freq = NULL;
+			return -EINVAL;
+		}
+
+		idx = (odr - 0x05) * 2;
+
+		if (idx + 1 >= ARRAY_SIZE(bma400_sample_freqs)) {
+			dev_err(data->dev, "sample freq index is too high");
+			return -EINVAL;
+		}
+
+		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;
+
+	for (i = 0; i + 1 < ARRAY_SIZE(bma400_sample_freqs); i += 2) {
+		if (bma400_sample_freqs[i] == hz &&
+		    bma400_sample_freqs[i + 1] == uhz)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int bma400_set_accel_output_data_rate(struct bma400_data *data,
+					     int hz, int uhz)
+{
+	int ret;
+	unsigned int odr;
+	unsigned int val;
+	int idx;
+
+	idx = bma400_get_accel_output_data_rate_idx(data, hz, uhz);
+
+	if (idx < 0)
+		return idx;
+
+	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
+
+	if (ret < 0)
+		return ret;
+
+	/* preserve the range and normal mode osr */
+	odr = (~BMA400_ACC_ODR_MASK & val) | (idx / 2 + 0x5);
+
+	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG, odr);
+	if (ret < 0)
+		return ret;
+
+	data->sample_freq = &bma400_sample_freqs[idx];
+	return 0;
+
+}
+
+static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
+{
+	unsigned int val;
+	unsigned int osr;
+	int ret;
+
+	/*
+	 * 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:
+		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG, &val);
+		if (ret < 0) {
+			data->oversampling_ratio = -1;
+			return ret;
+		}
+
+		osr = (val & BMA400_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
+
+		data->oversampling_ratio = osr;
+		return 0;
+	case POWER_MODE_NORMAL:
+		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
+		if (ret < 0) {
+			data->oversampling_ratio = -1;
+			return ret;
+		}
+
+		osr = (val & BMA400_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
+
+		data->oversampling_ratio = osr;
+		return 0;
+	default:
+		data->oversampling_ratio = -1;
+		return 0;
+	}
+}
+
+static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
+					       int val)
+{
+	int ret;
+	unsigned int acc_config;
+
+	if (val & ~BMA400_TWO_BITS_MASK)
+		return -EINVAL;
+
+	/*
+	 * The oversampling ratio is stored in a different register
+	 * based on the power-mode.
+	 */
+	switch (data->power_mode) {
+	case POWER_MODE_LOW:
+		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
+				  &acc_config);
+		if (acc_config < 0)
+			return acc_config;
+
+		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
+				   (acc_config & ~BMA400_LP_OSR_MASK) |
+				   (val << BMA400_LP_OSR_SHIFT));
+		if (ret < 0) {
+			dev_err(data->dev, "Failed to write out OSR");
+			return ret;
+		}
+
+		data->oversampling_ratio = val;
+		return 0;
+	case POWER_MODE_NORMAL:
+		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG,
+				  &acc_config);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
+				   (acc_config & ~BMA400_NP_OSR_MASK) |
+				   (val << BMA400_NP_OSR_SHIFT));
+		if (ret < 0) {
+			dev_err(data->dev, "Failed to write out OSR");
+			return ret;
+		}
+
+		data->oversampling_ratio = val;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+	return ret;
+}
+
+static int bma400_get_accel_scale(struct bma400_data *data)
+{
+	int idx;
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
+	if (ret < 0)
+		return ret;
+
+	idx = (((val & BMA400_ACC_SCALE_MASK) >> BMA400_SCALE_SHIFT) * 2) + 1;
+	if (idx >= ARRAY_SIZE(bma400_scale_table))
+		return -EINVAL;
+
+	data->scale = bma400_scale_table[idx];
+
+	return 0;
+}
+
+static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
+{
+	int i;
+
+	for (i = 1; i < ARRAY_SIZE(bma400_scale_table); i += 2) {
+		if (bma400_scale_table[i] == val)
+			return i - 1;
+	}
+	return -EINVAL;
+}
+
+static int bma400_set_accel_scale(struct bma400_data *data, unsigned int val)
+{
+	int ret;
+	int idx;
+	unsigned int acc_config;
+
+	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &acc_config);
+	if (ret < 0)
+		return ret;
+
+	idx = bma400_get_accel_scale_idx(data, val) / 2;
+
+	if (idx < 0)
+		return idx;
+
+	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
+			   (acc_config & ~BMA400_ACC_SCALE_MASK) |
+			   (idx << BMA400_SCALE_SHIFT));
+	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) & BMA400_TWO_BITS_MASK;
+
+	return 0;
+}
+
+static int bma400_set_power_mode(struct bma400_data *data,
+				 enum bma400_power_mode mode)
+{
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG, &val);
+	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 | (val & ~BMA400_TWO_BITS_MASK));
+
+	if (ret < 0) {
+		dev_err(data->dev, "Failed to write to power-mode");
+		return ret;
+	}
+
+	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 struct attribute *bma400_attributes[] = {
+	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[0];
+			*val2 = data->sample_freq[1];
+			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_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*vals = bma400_scale_table;
+		*length = ARRAY_SIZE(bma400_scale_table);
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*type = IIO_VAL_INT;
+		*vals = bma400_osr_table;
+		*length = ARRAY_SIZE(bma400_osr_table);
+		return IIO_AVAIL_RANGE;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*vals = bma400_sample_freqs;
+		*length = ARRAY_SIZE(bma400_sample_freqs);
+		return IIO_AVAIL_LIST;
+	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,
+	.read_avail        = bma400_read_avail,
+	.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);
+
+	return iio_device_register(indio_dev);
+}
+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..9d3a4a089a0a
--- /dev/null
+++ b/drivers/iio/accel/bma400_i2c.c
@@ -0,0 +1,58 @@
+// 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/i2c.h>
+#include <linux/module.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 const struct of_device_id bma400_of_match[] = {
+	{ .compatible = "bosch,bma400" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bma400_of_match);
+
+static struct i2c_driver bma400_i2c_driver = {
+	.driver = {
+		.name = "bma400",
+		.of_match_table = bma400_of_match,
+	},
+	.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 v2 2/2] iio: (bma400) add driver for the BMA400

[Randy Dunlap]

On 10/11/19 8:54 PM, Dan Robertson 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>
> ---
>  drivers/iio/accel/Kconfig       |  19 +
>  drivers/iio/accel/Makefile      |   2 +
>  drivers/iio/accel/bma400.h      |  86 ++++
>  drivers/iio/accel/bma400_core.c | 839 ++++++++++++++++++++++++++++++++
>  drivers/iio/accel/bma400_i2c.c  |  58 +++
>  5 files changed, 1004 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)

Since this already has "depends on I2C", the "if (I2C)" above is not needed.
Or maybe BMA400 alone does not depend on 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

	Add ending '.'.

> +
> +config BMA400_I2C
> +	tristate
> +	depends on BMA400
> +	depends on I2C
> +	select REGMAP_I2C
> +

The bma400_i2c driver seems to use some OF interfaces.
Should it also depend on OF?

>  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_core.c b/drivers/iio/accel/bma400_core.c
> new file mode 100644
> index 000000000000..5b3cb8919c47
> --- /dev/null
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -0,0 +1,839 @@
> +// 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"
> +

[snip]

> +
> +struct bma400_data {
> +	struct device *dev;
> +	struct mutex mutex; /* data register lock */

#include <linux/mutex.h>

> +	struct iio_mount_matrix orientation;
> +	struct regmap *regmap;
> +	enum bma400_power_mode power_mode;
> +	const int *sample_freq;
> +	int oversampling_ratio;
> +	int scale;
> +};

[snip]

> +
> +static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
> +{
> +	unsigned int val;
> +	unsigned int osr;
> +	int ret;
> +
> +	/*
> +	 * 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:
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG, &val);
> +		if (ret < 0) {
> +			data->oversampling_ratio = -1;
> +			return ret;
> +		}
> +
> +		osr = (val & BMA400_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
> +
> +		data->oversampling_ratio = osr;
> +		return 0;
> +	case POWER_MODE_NORMAL:
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
> +		if (ret < 0) {
> +			data->oversampling_ratio = -1;
> +			return ret;
> +		}
> +
> +		osr = (val & BMA400_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
> +
> +		data->oversampling_ratio = osr;
> +		return 0;
> +	default:
> +		data->oversampling_ratio = -1;
> +		return 0;
> +	}
> +}
> +
> +static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
> +					       int val)
> +{
> +	int ret;
> +	unsigned int acc_config;
> +
> +	if (val & ~BMA400_TWO_BITS_MASK)
> +		return -EINVAL;
> +
> +	/*
> +	 * The oversampling ratio is stored in a different register
> +	 * based on the power-mode.
> +	 */
> +	switch (data->power_mode) {
> +	case POWER_MODE_LOW:
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
> +				  &acc_config);
> +		if (acc_config < 0)
> +			return acc_config;
> +
> +		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> +				   (acc_config & ~BMA400_LP_OSR_MASK) |
> +				   (val << BMA400_LP_OSR_SHIFT));
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to write out OSR");
> +			return ret;
> +		}
> +
> +		data->oversampling_ratio = val;
> +		return 0;
> +	case POWER_MODE_NORMAL:
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG,
> +				  &acc_config);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> +				   (acc_config & ~BMA400_NP_OSR_MASK) |
> +				   (val << BMA400_NP_OSR_SHIFT));
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to write out OSR");
> +			return ret;
> +		}
> +
> +		data->oversampling_ratio = val;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;
> +}
> +
> +static int bma400_get_accel_scale(struct bma400_data *data)
> +{
> +	int idx;
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	idx = (((val & BMA400_ACC_SCALE_MASK) >> BMA400_SCALE_SHIFT) * 2) + 1;
> +	if (idx >= ARRAY_SIZE(bma400_scale_table))
> +		return -EINVAL;
> +
> +	data->scale = bma400_scale_table[idx];
> +
> +	return 0;
> +}
> +
> +static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
> +{
> +	int i;
> +
> +	for (i = 1; i < ARRAY_SIZE(bma400_scale_table); i += 2) {

#include <linux/kernel.h>

> +		if (bma400_scale_table[i] == val)
> +			return i - 1;
> +	}
> +	return -EINVAL;
> +}


[snip]


-- 
~Randy

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

[Andy Shevchenko]

On Sat, Oct 12, 2019 at 10:07 AM Randy Dunlap <rdunlap@infradead.org> wrote:
> On 10/11/19 8:54 PM, Dan Robertson wrote:

> > +config BMA400_I2C
> > +     tristate
> > +     depends on BMA400
> > +     depends on I2C
> > +     select REGMAP_I2C
> > +
>
> The bma400_i2c driver seems to use some OF interfaces.
> Should it also depend on OF?

Please, avoid unnecessary and strict dependencies when it's limiting
the area of use the driver.
The driver does not depend to OF. Why to stick with OF?

The actual change has to be done is to switch from

> #include <linux/of.h>

to

#include <linux/mod_devicetable.h>

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Sat, 12 Oct 2019 10:39:54 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Oct 12, 2019 at 10:07 AM Randy Dunlap <rdunlap@infradead.org> wrote:
> > On 10/11/19 8:54 PM, Dan Robertson wrote:  
> 
> > > +config BMA400_I2C
> > > +     tristate
> > > +     depends on BMA400
> > > +     depends on I2C
> > > +     select REGMAP_I2C
> > > +  
> >
> > The bma400_i2c driver seems to use some OF interfaces.
> > Should it also depend on OF?  
> 
> Please, avoid unnecessary and strict dependencies when it's limiting
> the area of use the driver.
> The driver does not depend to OF. Why to stick with OF?
> 
> The actual change has to be done is to switch from
> 
> > #include <linux/of.h>  
> 
> to
> 
> #include <linux/mod_devicetable.h>
> 

Good point Andy.  There are numerous drivers in IIO that do the same
over enthusiastic including of linux/of.h just to get of_device_id
definition.  Would be good to clean those up to just include this
instead.

This is going to cause very little noise so if there are any new
contributors reading this it would be a nice sensible cleanup to get
started with!

Thanks,

Jonathan

Re: [PATCH v2 1/2] dt-bindings: iio: accel: bma400: add bindings

[Jonathan Cameron]

On Sat, 12 Oct 2019 03:54:19 +0000
Dan Robertson <dan@dlrobertson.com> wrote:

> Add devicetree binding for the Bosch BMA400 3-axes ultra-low power
> accelerometer sensor.
> 
> Signed-off-by: Dan Robertson <dan@dlrobertson.com>
Hi Dan,

Apologies for not pointing this out in V1 but all new IIO bindings need
to be in the yaml format rather than plain text.

Please convert this one over for v3.  Content wise it's fine btw!

Jonathan


> ---
>  .../devicetree/bindings/iio/accel/bma400.txt     | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/bma400.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/bma400.txt b/Documentation/devicetree/bindings/iio/accel/bma400.txt
> new file mode 100644
> index 000000000000..abba4f104941
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/bma400.txt
> @@ -0,0 +1,16 @@
> +
> +* Bosch BMA400 triaxial acceleration sensor
> +
> +https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMA400-DS000.pdf
> +
> +Required properties:
> +
> +  - compatible : should be "bosch,bma400"
> +  - reg : the I2C address of the sensor
> +
> +Example:
> +
> +bma400@14 {
> +	compatible = "bosch,bma400";
> +	reg = <0x14>;
> +};
> 
> 

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

[Jonathan Cameron]

On Sat, 12 Oct 2019 03:54:20 +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,

A few minor bits I missed on v1.

> ---
>  drivers/iio/accel/Kconfig       |  19 +
>  drivers/iio/accel/Makefile      |   2 +
>  drivers/iio/accel/bma400.h      |  86 ++++
>  drivers/iio/accel/bma400_core.c | 839 ++++++++++++++++++++++++++++++++
>  drivers/iio/accel/bma400_i2c.c  |  58 +++
>  5 files changed, 1004 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..fa6e8dd598db
> --- /dev/null
> +++ b/drivers/iio/accel/bma400.h
> @@ -0,0 +1,86 @@
> +/* 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
> +
> +/* Sensor 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
> +
> +/*
> + * Commands accepted by the command register
> + */
> +#define BMA400_SOFTRESET_CMD    0xb6
> +
> +#define BMA400_TWO_BITS_MASK    0x03
> +#define BMA400_LP_OSR_MASK      0x60
> +#define BMA400_NP_OSR_MASK      0x30
> +#define BMA400_CMD_RDY_MASK     0x10
> +#define BMA400_ACC_ODR_MASK     0x0f
> +#define BMA400_ACC_SCALE_MASK   0xc0
> +
> +#define BMA400_LP_OSR_SHIFT     0x05
> +#define BMA400_NP_OSR_SHIFT     0x04
> +#define BMA400_SCALE_SHIFT      0x06
> +
> +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..5b3cb8919c47
> --- /dev/null
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -0,0 +1,839 @@
> +// 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.
> + */
> +static const int bma400_scale_table[] = {
> +	0, 38344,
> +	0, 76590,
> +	0, 153277,
> +	0, 306457
> +};
> +
> +static const int bma400_osr_table[] = { 0, 1, 3 };
> +
> +/* See the ACC_CONFIG1 section of the datasheet */
> +static const int bma400_sample_freqs[] = {
> +	12,  500000,
> +	25,  0,
> +	50,  0,
> +	100, 0,
> +	200, 0,
> +	400, 0,
> +	800, 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 int *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), \
> +	.info_mask_shared_by_type_available = 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 < 0)
> +		return 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;
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int bma400_get_accel_reg(struct bma400_data *data,
> +				const struct iio_chan_spec *chan,
> +				int *val)
> +{
> +	int ret;
> +	int lsb_reg;
> +	__le16 raw_accel;
> +
> +	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,
> +			       sizeof(raw_accel));
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = sign_extend32(le16_to_cpu(raw_accel), 11);
> +	return IIO_VAL_INT;
> +}
> +
> +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)
> +		return 0;
> +
> +	return (val & BMA400_CMD_RDY_MASK) ? 1 : 0;
> +}
> +
> +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 < 0)
> +		return 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[1];
> +	return 0;
> +}
> +
> +static int bma400_get_accel_output_data_rate(struct bma400_data *data)
> +{
> +	int ret;
> +	unsigned int val;
> +	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[2];
> +		return 0;
> +	case POWER_MODE_NORMAL:
> +		/*
> +		 * In normal mode the ODR can be found in the ACC_CONFIG1
> +		 * register.
> +		 */
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
> +		if (ret < 0) {
> +			data->sample_freq = NULL;
> +			return ret;
> +		}
> +
> +		odr = (val & BMA400_ACC_ODR_MASK);
> +		if (odr < 0x05) {
> +			dev_err(data->dev, "invalid ODR=%x", odr);
> +			data->sample_freq = NULL;
> +			return -EINVAL;
> +		}
> +
> +		idx = (odr - 0x05) * 2;
> +
> +		if (idx + 1 >= ARRAY_SIZE(bma400_sample_freqs)) {
> +			dev_err(data->dev, "sample freq index is too high");
> +			return -EINVAL;
> +		}
> +
> +		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;
> +
> +	for (i = 0; i + 1 < ARRAY_SIZE(bma400_sample_freqs); i += 2) {
> +		if (bma400_sample_freqs[i] == hz &&
> +		    bma400_sample_freqs[i + 1] == uhz)
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int bma400_set_accel_output_data_rate(struct bma400_data *data,
> +					     int hz, int uhz)
> +{
> +	int ret;
> +	unsigned int odr;
> +	unsigned int val;
> +	int idx;
> +
> +	idx = bma400_get_accel_output_data_rate_idx(data, hz, uhz);
> +
> +	if (idx < 0)
> +		return idx;
> +
> +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/* preserve the range and normal mode osr */
> +	odr = (~BMA400_ACC_ODR_MASK & val) | (idx / 2 + 0x5);
> +
> +	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG, odr);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->sample_freq = &bma400_sample_freqs[idx];
> +	return 0;
> +
> +}
> +
> +static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
> +{
> +	unsigned int val;
> +	unsigned int osr;
> +	int ret;
> +
> +	/*
> +	 * 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:
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG, &val);
> +		if (ret < 0) {
> +			data->oversampling_ratio = -1;
> +			return ret;
> +		}
> +
> +		osr = (val & BMA400_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
> +
> +		data->oversampling_ratio = osr;
> +		return 0;
> +	case POWER_MODE_NORMAL:
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
> +		if (ret < 0) {
> +			data->oversampling_ratio = -1;
> +			return ret;
> +		}
> +
> +		osr = (val & BMA400_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
> +
> +		data->oversampling_ratio = osr;
> +		return 0;
> +	default:
> +		data->oversampling_ratio = -1;
> +		return 0;
> +	}
> +}
> +
> +static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
> +					       int val)
> +{
> +	int ret;
> +	unsigned int acc_config;
> +
> +	if (val & ~BMA400_TWO_BITS_MASK)
> +		return -EINVAL;
> +
> +	/*
> +	 * The oversampling ratio is stored in a different register
> +	 * based on the power-mode.
> +	 */
> +	switch (data->power_mode) {
> +	case POWER_MODE_LOW:
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
> +				  &acc_config);
> +		if (acc_config < 0)
> +			return acc_config;
> +
> +		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> +				   (acc_config & ~BMA400_LP_OSR_MASK) |
> +				   (val << BMA400_LP_OSR_SHIFT));
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to write out OSR");
> +			return ret;
> +		}
> +
> +		data->oversampling_ratio = val;
> +		return 0;
> +	case POWER_MODE_NORMAL:
> +		ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG,
> +				  &acc_config);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> +				   (acc_config & ~BMA400_NP_OSR_MASK) |
> +				   (val << BMA400_NP_OSR_SHIFT));
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to write out OSR");
> +			return ret;
> +		}
> +
> +		data->oversampling_ratio = val;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;
> +}
> +
> +static int bma400_get_accel_scale(struct bma400_data *data)
> +{
> +	int idx;
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	idx = (((val & BMA400_ACC_SCALE_MASK) >> BMA400_SCALE_SHIFT) * 2) + 1;
> +	if (idx >= ARRAY_SIZE(bma400_scale_table))
> +		return -EINVAL;
> +
> +	data->scale = bma400_scale_table[idx];
> +
> +	return 0;
> +}
> +
> +static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
> +{
> +	int i;
> +
> +	for (i = 1; i < ARRAY_SIZE(bma400_scale_table); i += 2) {
> +		if (bma400_scale_table[i] == val)
> +			return i - 1;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int bma400_set_accel_scale(struct bma400_data *data, unsigned int val)
> +{
> +	int ret;
> +	int idx;
> +	unsigned int acc_config;
> +
> +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &acc_config);
> +	if (ret < 0)
> +		return ret;
> +
> +	idx = bma400_get_accel_scale_idx(data, val) / 2;
> +
> +	if (idx < 0)
> +		return idx;
> +
> +	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> +			   (acc_config & ~BMA400_ACC_SCALE_MASK) |
> +			   (idx << BMA400_SCALE_SHIFT));
> +	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) & BMA400_TWO_BITS_MASK;
> +
> +	return 0;
> +}
> +
> +static int bma400_set_power_mode(struct bma400_data *data,
> +				 enum bma400_power_mode mode)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG, &val);
> +	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 | (val & ~BMA400_TWO_BITS_MASK));
> +
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed to write to power-mode");
> +		return ret;
> +	}
> +
> +	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 struct attribute *bma400_attributes[] = {
> +	NULL,
> +};
> +
> +static const struct attribute_group bma400_attrs_group = {
> +	.attrs = bma400_attributes,

No need to supply any attrs at all if the core is doing everything
for you, so get rid of these.

> +};
> +
> +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[0];
> +			*val2 = data->sample_freq[1];
> +			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_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		*vals = bma400_scale_table;
> +		*length = ARRAY_SIZE(bma400_scale_table);
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*type = IIO_VAL_INT;
> +		*vals = bma400_osr_table;
> +		*length = ARRAY_SIZE(bma400_osr_table);
> +		return IIO_AVAIL_RANGE;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		*vals = bma400_sample_freqs;
> +		*length = ARRAY_SIZE(bma400_sample_freqs);
> +		return IIO_AVAIL_LIST;
> +	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,
> +	.read_avail        = bma400_read_avail,
> +	.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)

Try to avoid unnecessary line breaks.  There are stilly only
so many lines on a computer monitor :)

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);
> +
> +	return iio_device_register(indio_dev);
> +}
> +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.
> +		 */

Silly question.  Why is soft_reset preferred to sleep mode?

> +		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..9d3a4a089a0a
> --- /dev/null
> +++ b/drivers/iio/accel/bma400_i2c.c
> @@ -0,0 +1,58 @@
> +// 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/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

As per Andy's suggestion, tighten that header to include
on what is used.

> +#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);
	regmap = devm_regmap_init_i2c(client, &bma400_regmap_config);

Is well under the 80 char limit by the look of it. Please avoid
line breaks where not needed.

It can also fail so you need to check if (regmap) and return
-ENOMEM otherwise.

> +
> +	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 const struct of_device_id bma400_of_match[] = {
> +	{ .compatible = "bosch,bma400" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, bma400_of_match);
> +
> +static struct i2c_driver bma400_i2c_driver = {
> +	.driver = {
> +		.name = "bma400",
> +		.of_match_table = bma400_of_match,
> +	},
> +	.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 v2 2/2] iio: (bma400) add driver for the BMA400

[Dan Robertson]

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

Thanks again everyone for the informative feedback!

On Sat, Oct 12, 2019 at 10:31:07AM +0100, Jonathan Cameron wrote:
> Apologies for not pointing this out in V1 but all new IIO bindings need
> to be in the yaml format rather than plain text.

No problem. I'll use the yaml format in v3.

On Fri, Oct 11, 2019 at 11:11:31PM -0700, Randy Dunlap wrote:
> > +config BMA400
> > +	tristate "Bosch BMA400 3-Axis Accelerometer Driver"
> > +	depends on I2C
> > +	select REGMAP
> > +	select BMA400_I2C if (I2C)
>
> Since this already has "depends on I2C", the "if (I2C)" above is not needed.
> Or maybe BMA400 alone does not depend on I2C?

Good catch. I'll remove the I2C depends from BMA400_I2C.

On Sat, Oct 12, 2019 at 10:39:54AM +0300, Andy Shevchenko wrote:
> On Sat, Oct 12, 2019 at 10:07 AM Randy Dunlap <rdunlap@infradead.org> wrote:
> > On 10/11/19 8:54 PM, Dan Robertson wrote:
>
> > > +config BMA400_I2C
> > > +     tristate
> > > +     depends on BMA400
> > > +     depends on I2C
> > > +     select REGMAP_I2C
> > > +
> >
> > The bma400_i2c driver seems to use some OF interfaces.
> > Should it also depend on OF?
>
> Please, avoid unnecessary and strict dependencies when it's limiting
> the area of use the driver.
> The driver does not depend to OF. Why to stick with OF?
>
> The actual change has to be done is to switch from
>
> > #include <linux/of.h>
>
> to
>
> #include <linux/mod_devicetable.h>

Ah! Did not know about mod_devicetable.h. I'll make this change in the next
version.

On Sat, Oct 12, 2019 at 10:40:33AM +0100, Jonathan Cameron wrote:
> > +static const struct attribute_group bma400_attrs_group = {
> > +	.attrs = bma400_attributes,
>
> No need to supply any attrs at all if the core is doing everything
> for you, so get rid of these.

Good catch.

> > +int bma400_probe(struct device *dev,
> > +		 struct regmap *regmap,
> > +		 const char *name)
>
> Try to avoid unnecessary line breaks.  There are stilly only
> so many lines on a computer monitor :)

Will do. I'll do a quick audit of the patchset for other short lines, but
please don't hesitate to point out others you may find.

> > +		/*
> > +		 * If the softreset failed, try to put the device in
> > +		 * sleep mode, but still report the error.
> > +		 */
>
> Silly question.  Why is soft_reset preferred to sleep mode?

Not a silly question. I actually debated this when initially implementing the
driver. The datasheet describes soft-reset behavior in section 4.9, the
following snippet from the datasheet is particularly relevant:

> The softreset performs a fundamental reset to the device which is largely
> equivalent to a power cycle. Following a delay, all user configuration
> settings are overwritten with their default state...

Sleep mode is the default power-mode, so the only real difference would be that
the oversampling ratio, sampling frequency, and scale would all be reset to
their defaults with a soft-reset. If we just set the power-mode to sleep mode,
the user configuration registers would be preserved.

I can add a comment about the behavior of a softreset in bma400_remove.

Cheers,

 - Dan

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

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

[Jonathan Cameron]

On Sat, 12 Oct 2019 15:35:56 +0000
Dan Robertson <dan@dlrobertson.com> wrote:

> Thanks again everyone for the informative feedback!
> 
> On Sat, Oct 12, 2019 at 10:31:07AM +0100, Jonathan Cameron wrote:
> > Apologies for not pointing this out in V1 but all new IIO bindings need
> > to be in the yaml format rather than plain text.  
> 
> No problem. I'll use the yaml format in v3.
> 
> On Fri, Oct 11, 2019 at 11:11:31PM -0700, Randy Dunlap wrote:
> > > +config BMA400
> > > +	tristate "Bosch BMA400 3-Axis Accelerometer Driver"
> > > +	depends on I2C
> > > +	select REGMAP
> > > +	select BMA400_I2C if (I2C)  
> >
> > Since this already has "depends on I2C", the "if (I2C)" above is not needed.
> > Or maybe BMA400 alone does not depend on I2C?  
> 
> Good catch. I'll remove the I2C depends from BMA400_I2C.
> 
> On Sat, Oct 12, 2019 at 10:39:54AM +0300, Andy Shevchenko wrote:
> > On Sat, Oct 12, 2019 at 10:07 AM Randy Dunlap <rdunlap@infradead.org> wrote:  
> > > On 10/11/19 8:54 PM, Dan Robertson wrote:  
> >  
> > > > +config BMA400_I2C
> > > > +     tristate
> > > > +     depends on BMA400
> > > > +     depends on I2C
> > > > +     select REGMAP_I2C
> > > > +  
> > >
> > > The bma400_i2c driver seems to use some OF interfaces.
> > > Should it also depend on OF?  
> >
> > Please, avoid unnecessary and strict dependencies when it's limiting
> > the area of use the driver.
> > The driver does not depend to OF. Why to stick with OF?
> >
> > The actual change has to be done is to switch from
> >  
> > > #include <linux/of.h>  
> >
> > to
> >
> > #include <linux/mod_devicetable.h>  
> 
> Ah! Did not know about mod_devicetable.h. I'll make this change in the next
> version.
> 
> On Sat, Oct 12, 2019 at 10:40:33AM +0100, Jonathan Cameron wrote:
> > > +static const struct attribute_group bma400_attrs_group = {
> > > +	.attrs = bma400_attributes,  
> >
> > No need to supply any attrs at all if the core is doing everything
> > for you, so get rid of these.  
> 
> Good catch.
> 
> > > +int bma400_probe(struct device *dev,
> > > +		 struct regmap *regmap,
> > > +		 const char *name)  
> >
> > Try to avoid unnecessary line breaks.  There are stilly only
> > so many lines on a computer monitor :)  
> 
> Will do. I'll do a quick audit of the patchset for other short lines, but
> please don't hesitate to point out others you may find.
> 
> > > +		/*
> > > +		 * If the softreset failed, try to put the device in
> > > +		 * sleep mode, but still report the error.
> > > +		 */  
> >
> > Silly question.  Why is soft_reset preferred to sleep mode?  
> 
> Not a silly question. I actually debated this when initially implementing the
> driver. The datasheet describes soft-reset behavior in section 4.9, the
> following snippet from the datasheet is particularly relevant:
> 
> > The softreset performs a fundamental reset to the device which is largely
> > equivalent to a power cycle. Following a delay, all user configuration
> > settings are overwritten with their default state...  
> 
> Sleep mode is the default power-mode, so the only real difference would be that
> the oversampling ratio, sampling frequency, and scale would all be reset to
> their defaults with a soft-reset. If we just set the power-mode to sleep mode,
> the user configuration registers would be preserved.
> 
> I can add a comment about the behavior of a softreset in bma400_remove.
I'd just not do it.  That way there is only the putting it to sleep path.
We normally assume that we don't know the state during startup and aren't
as careful on remove to make sure things are in a default state.

That way you only need a single path in the remove function. If sleep mode
fails you are probably broken anyway.

Jonathan

> 
> Cheers,
> 
>  - Dan

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

[Dan Robertson]

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

On Sat, Oct 12, 2019 at 05:33:44PM +0100, Jonathan Cameron wrote:
> On Sat, 12 Oct 2019 15:35:56 +0000
> Dan Robertson <dan@dlrobertson.com> wrote:
> > On Sat, Oct 12, 2019 at 10:40:33AM +0100, Jonathan Cameron wrote:
> > >
> > > Silly question.  Why is soft_reset preferred to sleep mode?  
> > 
> > Not a silly question. I actually debated this when initially implementing the
> > driver. The datasheet describes soft-reset behavior in section 4.9, the
> > following snippet from the datasheet is particularly relevant:
> > 
> > > The softreset performs a fundamental reset to the device which is largely
> > > equivalent to a power cycle. Following a delay, all user configuration
> > > settings are overwritten with their default state...  
> > 
> > Sleep mode is the default power-mode, so the only real difference would be that
> > the oversampling ratio, sampling frequency, and scale would all be reset to
> > their defaults with a soft-reset. If we just set the power-mode to sleep mode,
> > the user configuration registers would be preserved.
> > 
> > I can add a comment about the behavior of a softreset in bma400_remove.
> I'd just not do it.  That way there is only the putting it to sleep path.
> We normally assume that we don't know the state during startup and aren't
> as careful on remove to make sure things are in a default state.
> 
> That way you only need a single path in the remove function. If sleep mode
> fails you are probably broken anyway.

Sounds good. That does make remove much easier if the device is not ready for a
command. I'll remove this in the next patchset version.

Cheers,

 - Dan

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

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

[kbuild test robot]

Hi Dan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Robertson/dt-bindings-iio-accel-bma400-add-bindings/20191014-034052
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

smatch warnings:
drivers/iio/accel/bma400_core.c:422 bma400_set_accel_oversampling_ratio() warn: unsigned 'acc_config' is never less than zero.

vim +/acc_config +422 drivers/iio/accel/bma400_core.c

   404	
   405	static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
   406						       int val)
   407	{
   408		int ret;
   409		unsigned int acc_config;
   410	
   411		if (val & ~BMA400_TWO_BITS_MASK)
   412			return -EINVAL;
   413	
   414		/*
   415		 * The oversampling ratio is stored in a different register
   416		 * based on the power-mode.
   417		 */
   418		switch (data->power_mode) {
   419		case POWER_MODE_LOW:
   420			ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
   421					  &acc_config);
 > 422			if (acc_config < 0)
   423				return acc_config;
   424	
   425			ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
   426					   (acc_config & ~BMA400_LP_OSR_MASK) |
   427					   (val << BMA400_LP_OSR_SHIFT));
   428			if (ret < 0) {
   429				dev_err(data->dev, "Failed to write out OSR");
   430				return ret;
   431			}
   432	
   433			data->oversampling_ratio = val;
   434			return 0;
   435		case POWER_MODE_NORMAL:
   436			ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG,
   437					  &acc_config);
   438			if (ret < 0)
   439				return ret;
   440	
   441			ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
   442					   (acc_config & ~BMA400_NP_OSR_MASK) |
   443					   (val << BMA400_NP_OSR_SHIFT));
   444			if (ret < 0) {
   445				dev_err(data->dev, "Failed to write out OSR");
   446				return ret;
   447			}
   448	
   449			data->oversampling_ratio = val;
   450			return 0;
   451		default:
   452			return -EINVAL;
   453		}
   454		return ret;
   455	}
   456	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[Andy Shevchenko]

On Tue, Oct 15, 2019 at 12:25 AM kbuild test robot <lkp@intel.com> wrote:

> smatch warnings:
> drivers/iio/accel/bma400_core.c:422 bma400_set_accel_oversampling_ratio() warn: unsigned 'acc_config' is never less than zero.

>    420                  ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
>    421                                    &acc_config);
>  > 422                  if (acc_config < 0)
>    423                          return acc_config;

Obvious typo, has to be if (ret < 0)

>    424
>    425                  ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
>    426                                     (acc_config & ~BMA400_LP_OSR_MASK) |
>    427                                     (val << BMA400_LP_OSR_SHIFT));
>    428                  if (ret < 0) {
>    429                          dev_err(data->dev, "Failed to write out OSR");
>    430                          return ret;
>    431                  }



-- 
With Best Regards,
Andy Shevchenko