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

[PATCH v3 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.

I've ran some basic tests with libiio and I've run the device tree checks. Are
there other tests that are recommended to be run for a new device?

Thanks again for the reviews!

Cheers,

 - Dan


Changes in v4:

 * Fix error in DT bindings
 * Fix typo when setting the OSR
 * Simplified the cached sample frequency
 * Fix the MODULE_LICENSE

Changes in v3:

 * Use yaml format for DT bindings
 * Remove strict dependency on OF
 * Tidy Kconfig dependencies
 * Stylistic changes
 * Do not soft-reset device on remove

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.yaml |  39 +
 drivers/iio/accel/Kconfig                     |  18 +
 drivers/iio/accel/Makefile                    |   2 +
 drivers/iio/accel/bma400.h                    |  85 ++
 drivers/iio/accel/bma400_core.c               | 796 ++++++++++++++++++
 drivers/iio/accel/bma400_i2c.c                |  60 ++
 6 files changed, 1000 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/bma400.yaml
 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 v4 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.yaml | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/bma400.yaml

diff --git a/Documentation/devicetree/bindings/iio/accel/bma400.yaml b/Documentation/devicetree/bindings/iio/accel/bma400.yaml
new file mode 100644
index 000000000000..e0a85dc7bf34
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/bma400.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/accel/bma400.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bosch BMA400 triaxial acceleration sensor
+
+maintainers:
+  - Dan Robertson <dan@dlrobertson.com>
+
+description: |
+  Acceleration and temerature iio sensors with an i2c interface
+
+  Specifications about the sensor can be found at:
+    https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMA400-DS000.pdf
+
+properties:
+  compatible:
+    enum:
+      - bosch,bma400
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      bma400@14 {
+        compatible = "bosch,bma400";
+        reg = <0x14>;
+      };
+    };

[PATCH v4 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       |  18 +
 drivers/iio/accel/Makefile      |   2 +
 drivers/iio/accel/bma400.h      |  85 ++++
 drivers/iio/accel/bma400_core.c | 796 ++++++++++++++++++++++++++++++++
 drivers/iio/accel/bma400_i2c.c  |  60 +++
 5 files changed, 961 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..a1081b902d16 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -112,6 +112,24 @@ 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
+	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..3a3860220a68
--- /dev/null
+++ b/drivers/iio/accel/bma400.h
@@ -0,0 +1,85 @@
+/* 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>
+ */
+
+#ifndef _BMA400_H_
+#define _BMA400_H_
+
+#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
+
+#define BMA400_TWO_BITS_MASK    0x03
+#define BMA400_LP_OSR_MASK      0x60
+#define BMA400_NP_OSR_MASK      0x30
+#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
+
+#define BMA400_ACC_ODR_MIN      0x05
+
+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);
+
+#endif
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
new file mode 100644
index 000000000000..80f1ee6713fa
--- /dev/null
+++ b/drivers/iio/accel/bma400_core.c
@@ -0,0 +1,796 @@
+// 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_sample_freq {
+	int hz;
+	int uhz;
+};
+
+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;
+	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), \
+	.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_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.hz = bma400_sample_freqs[2];
+		data->sample_freq.uhz = bma400_sample_freqs[3];
+		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)
+			goto error;
+
+		odr = (val & BMA400_ACC_ODR_MASK);
+		if (odr < BMA400_ACC_ODR_MIN) {
+			dev_err(data->dev, "invalid ODR=%x", odr);
+			ret = -EINVAL;
+			goto error;
+		}
+
+		idx = (odr - BMA400_ACC_ODR_MIN) * 2;
+
+		if (idx + 1 >= ARRAY_SIZE(bma400_sample_freqs)) {
+			dev_err(data->dev, "sample freq index is too high");
+			ret = -EINVAL;
+			goto error;
+		}
+
+		data->sample_freq.hz = bma400_sample_freqs[idx];
+		data->sample_freq.uhz = bma400_sample_freqs[idx + 1];
+		return 0;
+	case POWER_MODE_SLEEP:
+		data->sample_freq.hz = 0;
+		data->sample_freq.uhz = 0;
+		return 0;
+	default:
+		ret = 0;
+		goto error;
+	}
+error:
+	data->sample_freq.hz = -1;
+	data->sample_freq.uhz = -1;
+	return ret;
+}
+
+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 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)
+			break;
+	}
+
+	if (i + 1 >= ARRAY_SIZE(bma400_sample_freqs))
+		return -EINVAL;
+
+	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) | (i / 2 + BMA400_ACC_ODR_MIN);
+
+	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG, odr);
+	if (ret < 0)
+		return ret;
+
+	data->sample_freq.hz = bma400_sample_freqs[i];
+	data->sample_freq.uhz = bma400_sample_freqs[i + 1];
+	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;
+	case POWER_MODE_SLEEP:
+		data->oversampling_ratio = 0;
+		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 (ret < 0)
+			return ret;
+
+		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);
+
+	if (idx < 0)
+		return idx;
+
+	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
+			   (acc_config & ~BMA400_ACC_SCALE_MASK) |
+			   ((idx / 2) << 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 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.hz < 0)
+				return -EINVAL;
+
+			*val = data->sample_freq.hz;
+			*val2 = data->sample_freq.uhz;
+			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 = {
+	.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_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 v2");
diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
new file mode 100644
index 000000000000..d1029e8ccd1d
--- /dev/null
+++ b/drivers/iio/accel/bma400_i2c.c
@@ -0,0 +1,60 @@
+// 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/mod_devicetable.h>
+#include <linux/module.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);
+
+	if (!regmap)
+		return -ENOMEM;
+
+	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_i2c_match[] = {
+	{ .compatible = "bosch,bma400" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bma400_of_i2c_match);
+
+static struct i2c_driver bma400_i2c_driver = {
+	.driver = {
+		.name = "bma400",
+		.of_match_table = bma400_of_i2c_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 v2");

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

[Randy Dunlap]

On 10/17/19 8:18 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>

Hi,
I'm repeating some comments from my V2 review.

> ---
>  drivers/iio/accel/Kconfig       |  18 +
>  drivers/iio/accel/Makefile      |   2 +
>  drivers/iio/accel/bma400.h      |  85 ++++
>  drivers/iio/accel/bma400_core.c | 796 ++++++++++++++++++++++++++++++++
>  drivers/iio/accel/bma400_i2c.c  |  60 +++
>  5 files changed, 961 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..a1081b902d16 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -112,6 +112,24 @@ 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 BMA400 already depends on I2C, the "if (I2C)" above is
redundant so it's not needed.

> +	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
> +	select REGMAP_I2C
> +
>  config BMC150_ACCEL
>  	tristate "Bosch BMC150 Accelerometer Driver"
>  	select IIO_BUFFER


> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> new file mode 100644
> index 000000000000..80f1ee6713fa
> --- /dev/null
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -0,0 +1,796 @@
> +// 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_sample_freq {
> +	int hz;
> +	int uhz;
> +};
> +
> +struct bma400_data {
> +	struct device *dev;
> +	struct mutex mutex; /* data register lock */

needs #include <linux/mutex.h>

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

[snip]

> +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) {

needs #include <linux/kernel.h>
for ARRAY_SIZE()

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

[snip]

Thanks.

-- 
~Randy
See Documentation/process/submit-checklist.rst, especially Rule #1.

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

[Andy Shevchenko]

On Fri, Oct 18, 2019 at 6:44 AM 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.

Thanks for an update, my comments below.


> @@ -0,0 +1,85 @@

> + * bma400.h - Register constants and other forward declarations
> + *            needed by the bma400 sources.

Including file name in the file is not the best practice. Imagine if
by some reason we will need to rename it (to support more sensors, for
example, and reflect it by replacing 00 -> 0x).
So, please, remove here and everywhere else.

> + *
> + * Copyright 2019 Dan Robertson <dan@dlrobertson.com>
> + */

> +#define BMA400_TWO_BITS_MASK    0x03
> +#define BMA400_LP_OSR_MASK      0x60
> +#define BMA400_NP_OSR_MASK      0x30
> +#define BMA400_ACC_ODR_MASK     0x0f
> +#define BMA400_ACC_SCALE_MASK   0xc0

GENMASK()
(Don't forget to include bits.h for it)

> +extern const struct regmap_config bma400_regmap_config;

I'm not sure, why you need this exposed.

> + * bma400_core.c - Core IIO driver for Bosch BMA400 triaxial acceleration
> + *                 sensor. Used by bma400-i2c.

No name.

> +/*
> + * 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

Better to leave comma here. It doesn't matter for this device, but
make of use the better practices.

> +};

Also, I'm wondering why values are not exactly multiply by 2. Is in DS
of the chip any explanation for this?

> +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,
> +};

This can be replaced by a formula(s).

> +struct bma400_sample_freq {
> +       int hz;
> +       int uhz;
> +};

I'm wondering why above table is not using this struct.

> +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);

Why? And why it's not _GPL?

> +       int ret;
> +       int host_temp;
> +       unsigned int raw_temp;

Better reversed xmas tree order.

> +       ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &raw_temp);

> +

Redundant blank line..

> +       if (ret < 0)
> +               return ret;

> +               odr = (val & BMA400_ACC_ODR_MASK);

Too many parentheses.

> +               idx = (odr - BMA400_ACC_ODR_MIN) * 2;

> +

Redundant.

> +               if (idx + 1 >= ARRAY_SIZE(bma400_sample_freqs)) {

Why do you need this churn with +1 and = ?

> +                       dev_err(data->dev, "sample freq index is too high");
> +                       ret = -EINVAL;
> +                       goto error;
> +               }


> +       for (i = 0; i + 1 < ARRAY_SIZE(bma400_sample_freqs); i += 2) {

Using defined struct will guarantee you to have always 2x members in
the array. So, drop this arithmetic churn.

> +       if (i + 1 >= ARRAY_SIZE(bma400_sample_freqs))

As a bit above.

> +               return -EINVAL;

> +       ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG, &val);

> +

Redundant.

> +       if (ret < 0)
> +               return ret;

> +       idx = (((val & BMA400_ACC_SCALE_MASK) >> BMA400_SCALE_SHIFT) * 2) + 1;

Too many parentheses.

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

Why + 1 above and =  here?
You may do + 1 below.

> +       data->scale = bma400_scale_table[idx];


> +       idx = bma400_get_accel_scale_idx(data, val);

> +

Redundant

> +       if (idx < 0)
> +               return idx;


> +       /* Preserve the low-power oversample ratio etc */
> +       ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> +                          mode | (val & ~BMA400_TWO_BITS_MASK));

> +

Redundant

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

> +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);

%x for returned error code is too hackerish.

> +               return ret;

> +       } else if (val != BMA400_ID_REG_VAL) {

Redundant 'else'

> +               dev_err(data->dev, "CHIP ID MISMATCH: %x!", ret);

Hacker detected!

> +               return -ENODEV;
> +       }

> +               /*
> +                * TODO: The datasheet waits 1500us here in the example, but
> +                * lists 2/ODR as the wakeup time.
> +                */
> +               usleep_range(1500, 20000);

These range values are too sparse. Usually the second one is less than
first one * 2.
Fix it now.

> +EXPORT_SYMBOL(bma400_probe);

Why is not GPL?

> +EXPORT_SYMBOL(bma400_remove);

Ditto.

P.S. I probably missed some places with the same mistake as commented
above. Please address all places in the code where my comments are
applicable.

-- 
With Best Regards,
Andy Shevchenko

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

[Dan Robertson]

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

On Thu, Oct 17, 2019 at 09:25:37PM -0700, Randy Dunlap wrote:
> On 10/17/19 8:18 PM, Dan Robertson wrote:
> > +config BMA400
> > +	tristate "Bosch BMA400 3-Axis Accelerometer Driver"
> > +	depends on I2C
> > +	select REGMAP
> > +	select BMA400_I2C if (I2C)
> 
> Since BMA400 already depends on I2C, the "if (I2C)" above is
> redundant so it's not needed.

You are correct. I'll remove the depends on I2C.

> > +struct bma400_data {
> > +	struct device *dev;
> > +	struct mutex mutex; /* data register lock */
> 
> needs #include <linux/mutex.h>

Good point.

> > +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) {
> 
> needs #include <linux/kernel.h>
> for ARRAY_SIZE()

Thanks

Cheers,

 - Dan

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

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

[Dan Robertson]

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

On Fri, Oct 18, 2019 at 10:23:38AM +0300, Andy Shevchenko wrote:
> On Fri, Oct 18, 2019 at 6:44 AM Dan Robertson <dan@dlrobertson.com> wrote:
> > + * bma400.h - Register constants and other forward declarations
> > + *            needed by the bma400 sources.
>
> Including file name in the file is not the best practice. Imagine if
> by some reason we will need to rename it (to support more sensors, for
> example, and reflect it by replacing 00 -> 0x).
> So, please, remove here and everywhere else.

That makes sense.

> > +#define BMA400_TWO_BITS_MASK    0x03
> > +#define BMA400_LP_OSR_MASK      0x60
> > +#define BMA400_NP_OSR_MASK      0x30
> > +#define BMA400_ACC_ODR_MASK     0x0f
> > +#define BMA400_ACC_SCALE_MASK   0xc0
> 
> GENMASK()
> (Don't forget to include bits.h for it)

Thanks.

> > +static const int bma400_scale_table[] = {
> > +       0, 38344,
> > +       0, 76590,
> > +       0, 153277,
> 
> > +       0, 306457
> 
> Better to leave comma here. It doesn't matter for this device, but
> make of use the better practices.
> > +};
> 
> Also, I'm wondering why values are not exactly multiply by 2. Is in DS
> of the chip any explanation for this?

It would be a multiply by 2. I tried to follow the bma180 driver here, but I'm
starting to think that may be the wrong approach.

> > +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,
> > +};
> 
> This can be replaced by a formula(s).

Yeah I think I can implement the get, set, and read functions for sample_freq
with a formula, but the scale and sample frequency tables are needed by the
implementation of read_avail. A implementation of read_avail with a range and
a step would be ideal, but I couldn't find any documentation on implementing
read_avail where the step value of the range is a multiple. Please correct
me if I've missed something.

Note that this applies to the scale table as well.

> > +struct bma400_sample_freq {
> > +       int hz;
> > +       int uhz;
> > +};
> 
> I'm wondering why above table is not using this struct.

Originally it did, but I changed this in the second version when I added support
for iio_info read_avail to try to be a little closer to other implementations of
iio_read avail.

> > +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);
> 
> Why? And why it's not _GPL?

This is used by the bma400_i2c module.

> > +       int ret;
> > +       int host_temp;
> > +       unsigned int raw_temp;
> 
> Better reversed xmas tree order.

Sounds good.

> 
> > +               if (idx + 1 >= ARRAY_SIZE(bma400_sample_freqs)) {
> 
> Why do you need this churn with +1 and = ?

Since we've "flattened" the array of sample frequency we need to ensure that the
Hz (bma400_sample_freqs[idx]) and uHz (bma400_sample_freqs[idx + 1]) are both
valid. This will be negated in the next version as I'll switch to a formula.
Instead I'll ensure the returned ODR value is not above 0x0b.

> > +                       dev_err(data->dev, "sample freq index is too high");
> > +                       ret = -EINVAL;
> > +                       goto error;
> > +               }
> 
> 
> > +       for (i = 0; i + 1 < ARRAY_SIZE(bma400_sample_freqs); i += 2) {
> 
> Using defined struct will guarantee you to have always 2x members in
> the array. So, drop this arithmetic churn.

I should be able to figure out how to use a formula here, but I see where you're
coming from and I agree.

> > +       if (ret < 0) {
> > +               dev_err(data->dev, "Failed to read chip id register: %x!", ret);
> 
> %x for returned error code is too hackerish.

Makes sense. I'll change this in the update.

> > +               return ret;
> 
> > +       } else if (val != BMA400_ID_REG_VAL) {
> 
> Redundant 'else'

> > +               dev_err(data->dev, "CHIP ID MISMATCH: %x!", ret);
> 
> Hacker detected!

:)

> > +               return -ENODEV;
> > +       }
> 
> > +               /*
> > +                * TODO: The datasheet waits 1500us here in the example, but
> > +                * lists 2/ODR as the wakeup time.
> > +                */
> > +               usleep_range(1500, 20000);
> 
> These range values are too sparse. Usually the second one is less than
> first one * 2.
> Fix it now.

Good to know. I'll fix this in the update.

> > +EXPORT_SYMBOL(bma400_probe);
> 
> Why is not GPL?

Ah, saw in the docs "GPL" means GPL-2.0.

> 
> > +EXPORT_SYMBOL(bma400_remove);
> 
> Ditto.

This symbol is used in bma400_i2c.

> 
> P.S. I probably missed some places with the same mistake as commented
> above. Please address all places in the code where my comments are
> applicable.

Noted. Thanks for the feedback!

Cheers,

 - Dan

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

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

[Joe Perches]

On Fri, 2019-10-18 at 03:18 +0000, 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.

trivial logging note:

> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
[]
> +static int bma400_get_accel_reg(struct bma400_data *data,
> +				const struct iio_chan_spec *chan,
> +				int *val)
> +{
[]
> +		dev_err(data->dev, "invalid axis channel modifier");

All the logging should use \n terminations

		dev_err(data->dev, "invalid axis channel modifier\n");
[]
> +static int bma400_get_accel_output_data_rate(struct bma400_data *data)
> +{
[]
> +			dev_err(data->dev, "invalid ODR=%x", odr);
should be:
			dev_err(data->dev, "invalid ODR=%x\n", odr);

etc...

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

[Jonathan Cameron]

On Sat, 19 Oct 2019 02:43:51 +0000
Dan Robertson <dan@dlrobertson.com> wrote:

> On Fri, Oct 18, 2019 at 10:23:38AM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 18, 2019 at 6:44 AM Dan Robertson <dan@dlrobertson.com> wrote:  
> > > + * bma400.h - Register constants and other forward declarations
> > > + *            needed by the bma400 sources.  
> >
> > Including file name in the file is not the best practice. Imagine if
> > by some reason we will need to rename it (to support more sensors, for
> > example, and reflect it by replacing 00 -> 0x).
> > So, please, remove here and everywhere else.  
> 
> That makes sense.
> 
> > > +#define BMA400_TWO_BITS_MASK    0x03
> > > +#define BMA400_LP_OSR_MASK      0x60
> > > +#define BMA400_NP_OSR_MASK      0x30
> > > +#define BMA400_ACC_ODR_MASK     0x0f
> > > +#define BMA400_ACC_SCALE_MASK   0xc0  
> > 
> > GENMASK()
> > (Don't forget to include bits.h for it)  
> 
> Thanks.
> 
> > > +static const int bma400_scale_table[] = {
> > > +       0, 38344,
> > > +       0, 76590,
> > > +       0, 153277,  
> >   
> > > +       0, 306457  
> > 
> > Better to leave comma here. It doesn't matter for this device, but
> > make of use the better practices.  
> > > +};  
> > 
> > Also, I'm wondering why values are not exactly multiply by 2. Is in DS
> > of the chip any explanation for this?  
> 
> It would be a multiply by 2. I tried to follow the bma180 driver here, but I'm
> starting to think that may be the wrong approach.

I'll guess that, like so many accelerometers this one has documentation
in terms of 'g'.  Which is bonkers at g isn't a constant... g_0 is but no
datasheet ever seems to note that distinction.  Oh well. So these
are rounded to nearest value in m/s^2...  To be honest, the sensor
noise levels are usual such that this level of precision really
doesn't matter. 

> 
> > > +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,
> > > +};  
> > 
> > This can be replaced by a formula(s).  
> 
> Yeah I think I can implement the get, set, and read functions for sample_freq
> with a formula, but the scale and sample frequency tables are needed by the
> implementation of read_avail. A implementation of read_avail with a range and
> a step would be ideal, but I couldn't find any documentation on implementing
> read_avail where the step value of the range is a multiple. Please correct
> me if I've missed something.

Indeed. We've only defined it as being fixed intervals.
I'm not keen to expand the options for the userspace interface any
further.  

You could compute the values at startup and store it in your state structure
I think (or compute them on demand, but you'd need to have the space somewhere
non volatile).

> 
> Note that this applies to the scale table as well.
> 
> > > +struct bma400_sample_freq {
> > > +       int hz;
> > > +       int uhz;
> > > +};  
> > 
> > I'm wondering why above table is not using this struct.  
> 
> Originally it did, but I changed this in the second version when I added support
> for iio_info read_avail to try to be a little closer to other implementations of
> iio_read avail.
> 
> > > +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);  
> > 
> > Why? And why it's not _GPL?  
> 
> This is used by the bma400_i2c module.

Which is licensed as GPL v2, so I don't follow why this isn't
_GPL either.

> 
> > > +       int ret;
> > > +       int host_temp;
> > > +       unsigned int raw_temp;  
> > 
> > Better reversed xmas tree order.  
> 
> Sounds good.
> 
> >   
> > > +               if (idx + 1 >= ARRAY_SIZE(bma400_sample_freqs)) {  
> > 
> > Why do you need this churn with +1 and = ?  
> 
> Since we've "flattened" the array of sample frequency we need to ensure that the
> Hz (bma400_sample_freqs[idx]) and uHz (bma400_sample_freqs[idx + 1]) are both
> valid. This will be negated in the next version as I'll switch to a formula.
> Instead I'll ensure the returned ODR value is not above 0x0b.
> 
> > > +                       dev_err(data->dev, "sample freq index is too high");
> > > +                       ret = -EINVAL;
> > > +                       goto error;
> > > +               }  
> > 
> >   
> > > +       for (i = 0; i + 1 < ARRAY_SIZE(bma400_sample_freqs); i += 2) {  
> > 
> > Using defined struct will guarantee you to have always 2x members in
> > the array. So, drop this arithmetic churn.  
> 
> I should be able to figure out how to use a formula here, but I see where you're
> coming from and I agree.
> 
> > > +       if (ret < 0) {
> > > +               dev_err(data->dev, "Failed to read chip id register: %x!", ret);  
> > 
> > %x for returned error code is too hackerish.  
> 
> Makes sense. I'll change this in the update.
> 
> > > +               return ret;  
> >   
> > > +       } else if (val != BMA400_ID_REG_VAL) {  
> > 
> > Redundant 'else'  
> 
> > > +               dev_err(data->dev, "CHIP ID MISMATCH: %x!", ret);  
> > 
> > Hacker detected!  
> 
> :)
> 
> > > +               return -ENODEV;
> > > +       }  
> >   
> > > +               /*
> > > +                * TODO: The datasheet waits 1500us here in the example, but
> > > +                * lists 2/ODR as the wakeup time.
> > > +                */
> > > +               usleep_range(1500, 20000);  
> > 
> > These range values are too sparse. Usually the second one is less than
> > first one * 2.
> > Fix it now.  
> 
> Good to know. I'll fix this in the update.
> 
> > > +EXPORT_SYMBOL(bma400_probe);  
> > 
> > Why is not GPL?  
> 
> Ah, saw in the docs "GPL" means GPL-2.0.

So guess you are fine with making these all GPL exports then :)

> 
> >   
> > > +EXPORT_SYMBOL(bma400_remove);  
> > 
> > Ditto.  
> 
> This symbol is used in bma400_i2c.
> 
> > 
> > P.S. I probably missed some places with the same mistake as commented
> > above. Please address all places in the code where my comments are
> > applicable.  
> 
> Noted. Thanks for the feedback!
> 
> Cheers,
> 
>  - Dan
Thanks,

Jonathan.

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

[Rob Herring]

On Fri, Oct 18, 2019 at 03:18:47AM +0000, Dan Robertson wrote:
> 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.yaml | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/bma400.yaml

bosch,bma400.yaml

> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/bma400.yaml b/Documentation/devicetree/bindings/iio/accel/bma400.yaml
> new file mode 100644
> index 000000000000..e0a85dc7bf34
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/bma400.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/bma400.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bosch BMA400 triaxial acceleration sensor
> +
> +maintainers:
> +  - Dan Robertson <dan@dlrobertson.com>
> +
> +description: |
> +  Acceleration and temerature iio sensors with an i2c interface
> +
> +  Specifications about the sensor can be found at:
> +    https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMA400-DS000.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - bosch,bma400
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    i2c0 {

i2c {

(will enable i2c bus schema)

> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      bma400@14 {

accelerometer@14

> +        compatible = "bosch,bma400";
> +        reg = <0x14>;
> +      };
> +    };
> 
> 

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

[Dan Robertson]

Sorry for the incredibly late reply. Before I submit the next patchset version,
I have a question from the last set of reviews.

On Mon, Oct 21, 2019 at 04:20:16PM +0100, Jonathan Cameron wrote:
> On Sat, 19 Oct 2019 02:43:51 +0000
> Dan Robertson <dan@dlrobertson.com> wrote:
> > On Fri, Oct 18, 2019 at 10:23:38AM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 18, 2019 at 6:44 AM Dan Robertson <dan@dlrobertson.com> wrote:  
> > > > +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,
> > > > +};  
> > > 
> > > This can be replaced by a formula(s).  
> > 
> > Yeah I think I can implement the get, set, and read functions for sample_freq
> > with a formula, but the scale and sample frequency tables are needed by the
> > implementation of read_avail. A implementation of read_avail with a range and
> > a step would be ideal, but I couldn't find any documentation on implementing
> > read_avail where the step value of the range is a multiple. Please correct
> > me if I've missed something.
> 
> Indeed. We've only defined it as being fixed intervals.
> I'm not keen to expand the options for the userspace interface any
> further.  
> 
> You could compute the values at startup and store it in your state structure
> I think (or compute them on demand, but you'd need to have the space somewhere
> non volatile).
> 

I ended up writing an implementation that uses a formula for the get/set
functions of the sample frequency and scale, but uses a table for the
implementation of the read_avail function. While it does work, I worry
that this makes the driver less maintainable and would make it harder to
add support for a new hypothetical future BMA4xx device. Also, the majority
of drivers seem to use a table for the raw value to user input conversion,
so a move from this might make the code less "familiar".

If we do stick with the translation table, would it be better to have two
tables (a translation table and a read_avail table) so that we do not have
a step distance of two? This would mean we would need to maintain two
tables, but would simplify the code.

Random workflow question:

The sampling ratio, frequency, etc code seems to be the most complicated part
of the driver. Is it typically recommended to upstream a more minimal driver
that might assume the defaults?

Cheers,

 - Dan

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

[Jonathan Cameron]

On Mon, 18 Nov 2019 00:25:04 +0000
Dan Robertson <dan@dlrobertson.com> wrote:

> Sorry for the incredibly late reply. Before I submit the next patchset version,
> I have a question from the last set of reviews.
> 
> On Mon, Oct 21, 2019 at 04:20:16PM +0100, Jonathan Cameron wrote:
> > On Sat, 19 Oct 2019 02:43:51 +0000
> > Dan Robertson <dan@dlrobertson.com> wrote:  
> > > On Fri, Oct 18, 2019 at 10:23:38AM +0300, Andy Shevchenko wrote:  
> > > > On Fri, Oct 18, 2019 at 6:44 AM Dan Robertson <dan@dlrobertson.com> wrote:    
> > > > > +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,
> > > > > +};    
> > > > 
> > > > This can be replaced by a formula(s).    
> > > 
> > > Yeah I think I can implement the get, set, and read functions for sample_freq
> > > with a formula, but the scale and sample frequency tables are needed by the
> > > implementation of read_avail. A implementation of read_avail with a range and
> > > a step would be ideal, but I couldn't find any documentation on implementing
> > > read_avail where the step value of the range is a multiple. Please correct
> > > me if I've missed something.  
> > 
> > Indeed. We've only defined it as being fixed intervals.
> > I'm not keen to expand the options for the userspace interface any
> > further.  
> > 
> > You could compute the values at startup and store it in your state structure
> > I think (or compute them on demand, but you'd need to have the space somewhere
> > non volatile).
> >   
> 
> I ended up writing an implementation that uses a formula for the get/set
> functions of the sample frequency and scale, but uses a table for the
> implementation of the read_avail function. While it does work, I worry
> that this makes the driver less maintainable and would make it harder to
> add support for a new hypothetical future BMA4xx device. Also, the majority
> of drivers seem to use a table for the raw value to user input conversion,
> so a move from this might make the code less "familiar".
> 
> If we do stick with the translation table, would it be better to have two
> tables (a translation table and a read_avail table) so that we do not have
> a step distance of two? This would mean we would need to maintain two
> tables, but would simplify the code.

If a function is your preferred route you could also just use it to compute
the values for the available table at startup?

Otherwise, its fine to just use a table for both.

> 
> Random workflow question:
> 
> The sampling ratio, frequency, etc code seems to be the most complicated part
> of the driver. Is it typically recommended to upstream a more minimal driver
> that might assume the defaults?

Often people upstream a first version that just uses defaults, then follow
up (if they care) with later series adding the more fiddly elements.

Sometimes those more fiddly bits never come as a particular author
never needed them.  That's absolutely fine.  It's a rare driver
that supports all the features on a non trivial device!

Thanks,

Jonathan

> 
> Cheers,
> 
>  - Dan
> 

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

[Dan Robertson]

On Sat, Nov 23, 2019 at 12:51:35PM +0000, Jonathan Cameron wrote:
> If a function is your preferred route you could also just use it to compute
> the values for the available table at startup?

Yeah that makes sense. I'll add that in the next patchset version.

> > The sampling ratio, frequency, etc code seems to be the most complicated part
> > of the driver. Is it typically recommended to upstream a more minimal driver
> > that might assume the defaults?
> 
> Often people upstream a first version that just uses defaults, then follow
> up (if they care) with later series adding the more fiddly elements.
> 
> Sometimes those more fiddly bits never come as a particular author
> never needed them.  That's absolutely fine.  It's a rare driver
> that supports all the features on a non trivial device!

Makes sense. I'll likely add some extra bits in a follow-up patchset, so I can
learn a bit more.

Cheers,

 - Dan