[PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings

[PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings

[Mike Looijmans]

This adds the device-tree bindings for the Bosch Sensortec BMI088 IMU,
the accelerometer part.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

Changes in v6:
I't been almost a year since the last commit, sorry...
Fixed the yaml errors
Add interrupt, vdd and vddio properties

Changes in v5:
submit together with driver code as patch series

Changes in v2:
convert to yaml format

 .../bindings/iio/accel/bosch,bmi088.yaml      | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml

diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
new file mode 100644
index 000000000000..459b9969fd12
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/accel/bosch,bmi088.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bosch BMI088 IMU accelerometer part
+
+maintainers:
+  - Mike Looijmans <mike.looijmans@topic.nl>
+
+description: |
+  Acceleration part of the IMU sensor with an SPI interface
+  Specifications about the sensor can be found at:
+    https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi088-ds001.pdf
+
+properties:
+  compatible:
+    enum:
+      - bosch,bmi088_accel
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  vddio-supply: true
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+    description: |
+      Type should be either IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_LEVEL_LOW.
+      The first interrupt listed must be the one connected to the INT1 pin, the
+      second must be the one connected to the INT2 pin.
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      bmi088_accel@1 {
+        compatible = "bosch,bmi088_accel";
+        reg = <1>;
+        spi-max-frequency = <10000000>;
+        interrupt-parent = <&gpio6>;
+        interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
+      };
+    };
+...
-- 
2.17.1

[PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Mike Looijmans]

The BMI088 is a combined module with both accelerometer and gyroscope.
This adds the accelerometer driver support for the SPI interface.
The gyroscope part is already supported by the BMG160 driver.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

Changes in v6:
Hope you have good memory - v5 was almost a year ago now
Remove superfluous *val=0
Make sample_frequency selection into read_avail list

Changes in v5:
Add includes and forward defines in header
BIT(7) instead of 0x80
Reset already sets defaults, do not set them again
Remove now unused bmi088_accel_set_bw
Remove unused AXIS_MAX
Use MASK define for ODR setting
Explain buffer use and alignment
Split bmi088_accel_set_power_state into "on" and "off" parts
Cosmetic changes to improve readability

Changes in v4:
Remove unused #include directives
Remove unused #defines for event and irq
Replace (ret < 0) with (ret) for all regmap calls
Consistent checking of IO errors in probe and init
Removed #ifdef CONFIG_PM guard
Use bitops for set_frequency instead of loop with shift
s/__s16/s16/g
Remove excess blank lines
Don't return -EAGAIN in pm_runtime

Changes in v3:
Processed comments from Jonathan Cameron and Lars-Peter Clausen
implement runtime PM (tested by code tracing) and sleep
fix scale and offset factors for accel and temperature and
return raw values instead of pre-scaled ones
Use iio_device_{claim,release}_direct_mode
Remove unused code and structs
Use a cache-aligned buffer for bulk read
Configure and enable caching register values

Changes in v2:
Remove unused typedefs and variables
Fix error return when iio_device_register fails

 drivers/iio/accel/Kconfig             |  18 +
 drivers/iio/accel/Makefile            |   2 +
 drivers/iio/accel/bmi088-accel-core.c | 630 ++++++++++++++++++++++++++
 drivers/iio/accel/bmi088-accel-spi.c  |  85 ++++
 drivers/iio/accel/bmi088-accel.h      |  18 +
 5 files changed, 753 insertions(+)
 create mode 100644 drivers/iio/accel/bmi088-accel-core.c
 create mode 100644 drivers/iio/accel/bmi088-accel-spi.c
 create mode 100644 drivers/iio/accel/bmi088-accel.h

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 2e0c62c39155..cceda3cecbcf 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -157,6 +157,24 @@ config BMC150_ACCEL_SPI
 	tristate
 	select REGMAP_SPI
 
+config BMI088_ACCEL
+	tristate "Bosch BMI088 Accelerometer Driver"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	select REGMAP
+	select BMI088_ACCEL_SPI
+	help
+	  Say yes here to build support for the Bosch BMI088 accelerometer.
+
+	  This is a combo module with both accelerometer and gyroscope. This
+	  driver only implements the accelerometer part, which has its own
+	  address and register map. BMG160 provides the gyroscope driver.
+
+config BMI088_ACCEL_SPI
+	tristate
+	select REGMAP_SPI
+
 config DA280
 	tristate "MiraMEMS DA280 3-axis 14-bit digital accelerometer driver"
 	depends on I2C
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 4f6c1ebe13b0..32cd1342a31a 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -20,6 +20,8 @@ obj-$(CONFIG_BMA400_SPI) += bma400_spi.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
+obj-$(CONFIG_BMI088_ACCEL) += bmi088-accel-core.o
+obj-$(CONFIG_BMI088_ACCEL_SPI) += bmi088-accel-spi.o
 obj-$(CONFIG_DA280)	+= da280.o
 obj-$(CONFIG_DA311)	+= da311.o
 obj-$(CONFIG_DMARD06)	+= dmard06.o
diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
new file mode 100644
index 000000000000..788e54ed0728
--- /dev/null
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -0,0 +1,630 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
+ *  - BMI088
+ *
+ * Copyright (c) 2018-2020, Topic Embedded Products
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/acpi.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/regmap.h>
+#include <asm/unaligned.h>
+
+#include "bmi088-accel.h"
+
+#define BMI088_ACCEL_REG_CHIP_ID			0x00
+#define BMI088_ACCEL_REG_ERROR				0x02
+
+#define BMI088_ACCEL_REG_INT_STATUS			0x1D
+#define BMI088_ACCEL_INT_STATUS_BIT_DRDY		BIT(7)
+
+#define BMI088_ACCEL_REG_RESET				0x7E
+#define BMI088_ACCEL_RESET_VAL				0xB6
+
+#define BMI088_ACCEL_REG_PWR_CTRL			0x7D
+#define BMI088_ACCEL_REG_PWR_CONF			0x7C
+
+#define BMI088_ACCEL_REG_INT_MAP_DATA			0x58
+#define BMI088_ACCEL_INT_MAP_DATA_BIT_INT1_DRDY		BIT(2)
+#define BMI088_ACCEL_INT_MAP_DATA_BIT_INT2_FWM		BIT(5)
+
+#define BMI088_ACCEL_REG_INT1_IO_CONF			0x53
+#define BMI088_ACCEL_INT1_IO_CONF_BIT_ENABLE_OUT	BIT(3)
+#define BMI088_ACCEL_INT1_IO_CONF_BIT_LVL		BIT(1)
+
+#define BMI088_ACCEL_REG_INT2_IO_CONF			0x54
+#define BMI088_ACCEL_INT2_IO_CONF_BIT_ENABLE_OUT	BIT(3)
+#define BMI088_ACCEL_INT2_IO_CONF_BIT_LVL		BIT(1)
+
+#define BMI088_ACCEL_REG_ACC_CONF			0x40
+#define BMI088_ACCEL_MODE_ODR_MASK			0x0f
+
+#define BMI088_ACCEL_REG_ACC_RANGE			0x41
+#define BMI088_ACCEL_RANGE_3G				0x00
+#define BMI088_ACCEL_RANGE_6G				0x01
+#define BMI088_ACCEL_RANGE_12G				0x02
+#define BMI088_ACCEL_RANGE_24G				0x03
+
+#define BMI088_ACCEL_REG_TEMP				0x22
+#define BMI088_ACCEL_REG_TEMP_SHIFT			5
+#define BMI088_ACCEL_TEMP_UNIT				125
+#define BMI088_ACCEL_TEMP_OFFSET			23000
+
+#define BMI088_ACCEL_REG_XOUT_L				0x12
+#define BMI088_ACCEL_AXIS_TO_REG(axis) \
+	(BMI088_ACCEL_REG_XOUT_L + (axis * 2))
+
+#define BMI088_ACCEL_MAX_STARTUP_TIME_US		1000
+#define BMI088_AUTO_SUSPEND_DELAY_MS			2000
+
+#define BMI088_ACCEL_REG_FIFO_STATUS			0x0E
+#define BMI088_ACCEL_REG_FIFO_CONFIG0			0x48
+#define BMI088_ACCEL_REG_FIFO_CONFIG1			0x49
+#define BMI088_ACCEL_REG_FIFO_DATA			0x3F
+#define BMI088_ACCEL_FIFO_LENGTH			100
+
+#define BMI088_ACCEL_FIFO_MODE_FIFO			0x40
+#define BMI088_ACCEL_FIFO_MODE_STREAM			0x80
+
+enum bmi088_accel_axis {
+	AXIS_X,
+	AXIS_Y,
+	AXIS_Z,
+};
+
+enum bmi088_power_modes {
+	BMI088_ACCEL_MODE_ACTIVE,
+	BMI088_ACCEL_MODE_SUSPEND,
+};
+
+static const int bmi088_sample_freqs[] = {
+	12, 500000,
+	25, 0,
+	50, 0,
+	100, 0,
+	200, 0,
+	400, 0,
+	800, 0,
+	1600, 0,
+};
+
+/* Available OSR (over sampling rate) sets the 3dB cut-off frequency */
+enum bmi088_osr_modes {
+	BMI088_ACCEL_MODE_OSR_NORMAL = 0xA,
+	BMI088_ACCEL_MODE_OSR_2 = 0x9,
+	BMI088_ACCEL_MODE_OSR_4 = 0x8,
+};
+
+/* Available ODR (output data rates) in Hz */
+enum bmi088_odr_modes {
+	BMI088_ACCEL_MODE_ODR_12_5 = 0x5,
+	BMI088_ACCEL_MODE_ODR_25 = 0x6,
+	BMI088_ACCEL_MODE_ODR_50 = 0x7,
+	BMI088_ACCEL_MODE_ODR_100 = 0x8,
+	BMI088_ACCEL_MODE_ODR_200 = 0x9,
+	BMI088_ACCEL_MODE_ODR_400 = 0xa,
+	BMI088_ACCEL_MODE_ODR_800 = 0xb,
+	BMI088_ACCEL_MODE_ODR_1600 = 0xc,
+};
+
+struct bmi088_scale_info {
+	int scale;
+	u8 reg_range;
+};
+
+struct bmi088_accel_chip_info {
+	const char *name;
+	u8 chip_id;
+	const struct iio_chan_spec *channels;
+	int num_channels;
+};
+
+struct bmi088_accel_data {
+	struct regmap *regmap;
+	struct mutex mutex;
+	const struct bmi088_accel_chip_info *chip_info;
+	u8 buffer[2] ____cacheline_aligned; /* shared DMA safe buffer */
+};
+
+static const struct regmap_range bmi088_volatile_ranges[] = {
+	/* All registers below 0x40 are volatile, except the CHIP ID. */
+	regmap_reg_range(BMI088_ACCEL_REG_ERROR, 0x3f),
+	/* Mark the RESET as volatile too, it is self-clearing */
+	regmap_reg_range(BMI088_ACCEL_REG_RESET, BMI088_ACCEL_REG_RESET),
+};
+
+static const struct regmap_access_table bmi088_volatile_table = {
+	.yes_ranges	= bmi088_volatile_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(bmi088_volatile_ranges),
+};
+
+const struct regmap_config bmi088_regmap_conf = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x7E,
+	.volatile_table = &bmi088_volatile_table,
+	.cache_type = REGCACHE_RBTREE,
+};
+EXPORT_SYMBOL_GPL(bmi088_regmap_conf);
+
+static int bmi088_accel_set_power_state_on(struct bmi088_accel_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmi088_accel_set_power_state_off(struct bmi088_accel_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	pm_runtime_mark_last_busy(dev);
+	ret = pm_runtime_put_autosuspend(dev);
+
+	return ret < 0 ? ret : 0;
+}
+
+/*
+ * The register ACC_PWR_CTRL enables and disables the accelerometer and the
+ * temperature sensor.
+ */
+static int bmi088_accel_enable(struct bmi088_accel_data *data, bool on_off)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	unsigned int val = on_off ? 0x4 : 0x0;
+	int ret;
+
+	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CTRL, val);
+	if (ret) {
+		dev_err(dev, "Error writing ACC_PWR_CTRL reg\n");
+		return ret;
+	}
+	/* Datasheet recommends to wait at least 5ms before communication */
+	usleep_range(5000, 6000);
+
+	return 0;
+}
+
+/* In suspend mode, only the accelerometer is powered down. */
+static int bmi088_accel_set_mode(struct bmi088_accel_data *data,
+				enum bmi088_power_modes mode)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	unsigned int val = mode == BMI088_ACCEL_MODE_SUSPEND ? 0x3 : 0x0;
+	int ret;
+
+	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CONF, val);
+	if (ret) {
+		dev_err(dev, "Error writing ACCEL_PWR_CONF reg\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmi088_accel_get_sample_freq(struct bmi088_accel_data *data,
+					int *val, int *val2)
+{
+	unsigned int value;
+	int ret;
+
+	ret = regmap_read(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
+			  &value);
+	if (ret)
+		return ret;
+
+	value &= BMI088_ACCEL_MODE_ODR_MASK;
+	value -= BMI088_ACCEL_MODE_ODR_12_5;
+	value <<= 1;
+
+	if (value >= ARRAY_SIZE(bmi088_sample_freqs) - 1)
+		return -EINVAL;
+
+	*val = bmi088_sample_freqs[value];
+	*val2 = bmi088_sample_freqs[value + 1];
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int bmi088_accel_set_sample_freq(struct bmi088_accel_data *data, int val)
+{
+	unsigned int regval;
+	int index = 0;
+
+	while (index < ARRAY_SIZE(bmi088_sample_freqs) &&
+	       bmi088_sample_freqs[index] != val)
+		index += 2;
+
+	if (index >= ARRAY_SIZE(bmi088_sample_freqs))
+		return -EINVAL;
+
+	regval = (index >> 1) + BMI088_ACCEL_MODE_ODR_12_5;
+
+	return regmap_update_bits(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
+				  BMI088_ACCEL_MODE_ODR_MASK, regval);
+}
+
+static int bmi088_accel_get_temp(struct bmi088_accel_data *data, int *val)
+{
+	int ret;
+	s16 temp;
+
+	mutex_lock(&data->mutex);
+
+	ret = regmap_bulk_read(data->regmap, BMI088_ACCEL_REG_TEMP,
+			       &data->buffer, sizeof(__be16));
+	/* data->buffer is cacheline aligned */
+	temp = be16_to_cpu(*(__be16 *)data->buffer);
+
+	mutex_unlock(&data->mutex);
+
+	if (ret)
+		return ret;
+
+	*val = temp >> BMI088_ACCEL_REG_TEMP_SHIFT;
+
+	return IIO_VAL_INT;
+}
+
+static int bmi088_accel_get_axis(struct bmi088_accel_data *data,
+				 struct iio_chan_spec const *chan,
+				 int *val)
+{
+	int ret;
+	s16 raw_val;
+
+	mutex_lock(&data->mutex);
+
+	ret = bmi088_accel_set_power_state_on(data);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap,
+			       BMI088_ACCEL_AXIS_TO_REG(chan->scan_index),
+			       data->buffer, sizeof(__le16));
+	raw_val = le16_to_cpu(*(__le16 *)data->buffer);
+
+	bmi088_accel_set_power_state_off(data);
+
+	mutex_unlock(&data->mutex);
+
+	if (ret)
+		return ret;
+
+	*val = raw_val;
+
+	return IIO_VAL_INT;
+}
+
+static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int *val, int *val2, long mask)
+{
+	struct bmi088_accel_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_TEMP:
+			return bmi088_accel_get_temp(data, val);
+		case IIO_ACCEL:
+			ret = iio_device_claim_direct_mode(indio_dev);
+			if (ret)
+				return ret;
+
+			ret = bmi088_accel_get_axis(data, chan, val);
+			iio_device_release_direct_mode(indio_dev);
+			if (ret)
+				return ret;
+
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		switch (chan->type) {
+		case IIO_TEMP:
+			/* Offset applies before scale */
+			*val = BMI088_ACCEL_TEMP_OFFSET/BMI088_ACCEL_TEMP_UNIT;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_TEMP:
+			/* 0.125 degrees per LSB */
+			*val = BMI088_ACCEL_TEMP_UNIT;
+			return IIO_VAL_INT;
+		case IIO_ACCEL:
+			ret = regmap_read(data->regmap,
+					  BMI088_ACCEL_REG_ACC_RANGE, val);
+			if (ret)
+				return ret;
+
+			*val2 = 15 - (*val & 0x3);
+			*val = 3 * 980;
+
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&data->mutex);
+		ret = bmi088_accel_get_sample_freq(data, val, val2);
+		mutex_unlock(&data->mutex);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi088_accel_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_SAMP_FREQ:
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*vals = bmi088_sample_freqs;
+		*length = ARRAY_SIZE(bmi088_sample_freqs);
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  int val, int val2, long mask)
+{
+	struct bmi088_accel_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&data->mutex);
+		ret = bmi088_accel_set_sample_freq(data, val);
+		mutex_unlock(&data->mutex);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+#define BMI088_ACCEL_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_SCALE) | \
+				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+	.scan_index = AXIS_##_axis, \
+}
+
+static const struct iio_chan_spec bmi088_accel_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.scan_index = -1,
+	},
+	BMI088_ACCEL_CHANNEL(X),
+	BMI088_ACCEL_CHANNEL(Y),
+	BMI088_ACCEL_CHANNEL(Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
+	[0] = {
+		.name = "bmi088a",
+		.chip_id = 0x1E,
+		.channels = bmi088_accel_channels,
+		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
+	},
+};
+
+static const struct iio_info bmi088_accel_info = {
+	.read_raw	= bmi088_accel_read_raw,
+	.write_raw	= bmi088_accel_write_raw,
+	.read_avail	= bmi088_accel_read_avail,
+};
+
+static const unsigned long bmi088_accel_scan_masks[] = {
+	BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
+	0
+};
+
+static int bmi088_accel_chip_init(struct bmi088_accel_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret, i;
+	unsigned int val;
+
+	/* Do a dummy read to enable SPI interface, won't harm I2C */
+	regmap_read(data->regmap, BMI088_ACCEL_REG_INT_STATUS, &val);
+
+	/*
+	 * Reset chip to get it in a known good state. A delay of 1ms after
+	 * reset is required according to the data sheet
+	 */
+	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_RESET,
+			   BMI088_ACCEL_RESET_VAL);
+	if (ret)
+		return ret;
+
+	usleep_range(1000, 2000);
+
+	/* Do a dummy read again after a reset to enable the SPI interface */
+	regmap_read(data->regmap, BMI088_ACCEL_REG_INT_STATUS, &val);
+
+	/* Read chip ID */
+	ret = regmap_read(data->regmap, BMI088_ACCEL_REG_CHIP_ID, &val);
+	if (ret) {
+		dev_err(dev, "Error: Reading chip id\n");
+		return ret;
+	}
+
+	/* Validate chip ID */
+	for (i = 0; i < ARRAY_SIZE(bmi088_accel_chip_info_tbl); i++) {
+		if (bmi088_accel_chip_info_tbl[i].chip_id == val) {
+			data->chip_info = &bmi088_accel_chip_info_tbl[i];
+			break;
+		}
+	}
+	if (i == ARRAY_SIZE(bmi088_accel_chip_info_tbl)) {
+		dev_err(dev, "Invalid chip %x\n", val);
+		return -ENODEV;
+	}
+
+	/* Enable accelerometer after reset */
+	return bmi088_accel_enable(data, true);
+}
+
+int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
+	int irq, const char *name, bool block_supported)
+{
+	struct bmi088_accel_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	dev_set_drvdata(dev, indio_dev);
+
+	data->regmap = regmap;
+
+	ret = bmi088_accel_chip_init(data);
+	if (ret)
+		return ret;
+
+	mutex_init(&data->mutex);
+
+	indio_dev->dev.parent = dev;
+	indio_dev->channels = data->chip_info->channels;
+	indio_dev->num_channels = data->chip_info->num_channels;
+	indio_dev->name = name ? name : data->chip_info->name;
+	indio_dev->available_scan_masks = bmi088_accel_scan_masks;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &bmi088_accel_info;
+
+	ret = pm_runtime_set_active(dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, BMI088_AUTO_SUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		dev_err(dev, "Unable to register iio device\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bmi088_accel_core_probe);
+
+int bmi088_accel_core_remove(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct bmi088_accel_data *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_put_noidle(dev);
+
+	mutex_lock(&data->mutex);
+	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bmi088_accel_core_remove);
+
+/* When going into system sleep, put the chip in power down */
+static int __maybe_unused bmi088_accel_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct bmi088_accel_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->mutex);
+	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
+	bmi088_accel_enable(data, false);
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static int __maybe_unused bmi088_accel_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct bmi088_accel_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->mutex);
+	bmi088_accel_enable(data, true);
+	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_ACTIVE);
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+/* For runtime PM put the chip in suspend mode */
+static int __maybe_unused bmi088_accel_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct bmi088_accel_data *data = iio_priv(indio_dev);
+
+	return bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
+}
+
+static int __maybe_unused bmi088_accel_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct bmi088_accel_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_ACTIVE);
+	if (ret)
+		return ret;
+
+	usleep_range(BMI088_ACCEL_MAX_STARTUP_TIME_US,
+		     BMI088_ACCEL_MAX_STARTUP_TIME_US * 2);
+
+	return 0;
+}
+
+const struct dev_pm_ops bmi088_accel_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(bmi088_accel_suspend, bmi088_accel_resume)
+	SET_RUNTIME_PM_OPS(bmi088_accel_runtime_suspend,
+			   bmi088_accel_runtime_resume, NULL)
+};
+EXPORT_SYMBOL_GPL(bmi088_accel_pm_ops);
+
+MODULE_AUTHOR("Niek van Agt <niek.van.agt@topicproducts.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("BMI088 accelerometer driver (core)");
diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
new file mode 100644
index 000000000000..7794090b8e6d
--- /dev/null
+++ b/drivers/iio/accel/bmi088-accel-spi.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
+ *  - BMI088
+ *
+ * Copyright (c) 2018-2020, Topic Embedded Products
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/regmap.h>
+
+#include "bmi088-accel.h"
+
+int bmi088_regmap_spi_write(void *context, const void *data, size_t count)
+{
+	struct spi_device *spi = context;
+
+	/* Write register is same as generic SPI */
+	return spi_write(spi, data, count);
+}
+
+int bmi088_regmap_spi_read(void *context, const void *reg,
+				size_t reg_size, void *val, size_t val_size)
+{
+	struct spi_device *spi = context;
+	u8 addr[2];
+
+	addr[0] = *(u8 *)reg;
+	addr[0] |= BIT(7); /* Set RW = '1' */
+	addr[1] = 0; /* Read requires a dummy byte transfer */
+
+	return spi_write_then_read(spi, addr, sizeof(addr), val, val_size);
+}
+
+static struct regmap_bus bmi088_regmap_bus = {
+	.write = bmi088_regmap_spi_write,
+	.read = bmi088_regmap_spi_read,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG,
+};
+
+static int bmi088_accel_probe(struct spi_device *spi)
+{
+	struct regmap *regmap;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+
+	regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus,
+			spi, &bmi088_regmap_conf);
+
+	if (IS_ERR(regmap)) {
+		dev_err(&spi->dev, "Failed to initialize spi regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, id->name,
+				       true);
+}
+
+static int bmi088_accel_remove(struct spi_device *spi)
+{
+	return bmi088_accel_core_remove(&spi->dev);
+}
+
+static const struct spi_device_id bmi088_accel_id[] = {
+	{"bmi088_accel", },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, bmi088_accel_id);
+
+static struct spi_driver bmi088_accel_driver = {
+	.driver = {
+		.name	= "bmi088_accel_spi",
+		.pm	= &bmi088_accel_pm_ops,
+	},
+	.probe		= bmi088_accel_probe,
+	.remove		= bmi088_accel_remove,
+	.id_table	= bmi088_accel_id,
+};
+module_spi_driver(bmi088_accel_driver);
+
+MODULE_AUTHOR("Niek van Agt <niek.van.agt@topicproducts.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("BMI088 accelerometer driver (SPI)");
diff --git a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h
new file mode 100644
index 000000000000..5c25f16b672c
--- /dev/null
+++ b/drivers/iio/accel/bmi088-accel.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef BMI088_ACCEL_H
+#define BMI088_ACCEL_H
+
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+struct device;
+
+extern const struct regmap_config bmi088_regmap_conf;
+extern const struct dev_pm_ops bmi088_accel_pm_ops;
+
+int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
+			    const char *name, bool block_supported);
+int bmi088_accel_core_remove(struct device *dev);
+
+#endif /* BMI088_ACCEL_H */
-- 
2.17.1

Re: [PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings

[Rob Herring]

On Tue, 19 Jan 2021 13:46:21 +0100, Mike Looijmans wrote:
> This adds the device-tree bindings for the Bosch Sensortec BMI088 IMU,
> the accelerometer part.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
> Changes in v6:
> I't been almost a year since the last commit, sorry...
> Fixed the yaml errors
> Add interrupt, vdd and vddio properties
> 
> Changes in v5:
> submit together with driver code as patch series
> 
> Changes in v2:
> convert to yaml format
> 
>  .../bindings/iio/accel/bosch,bmi088.yaml      | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml: 'additionalProperties' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml: ignoring, error in schema: 
warning: no schema found in file: ./Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml

See https://patchwork.ozlabs.org/patch/1428668

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings

[Rob Herring]

On Tue, Jan 19, 2021 at 01:46:21PM +0100, Mike Looijmans wrote:
> This adds the device-tree bindings for the Bosch Sensortec BMI088 IMU,
> the accelerometer part.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
> Changes in v6:
> I't been almost a year since the last commit, sorry...
> Fixed the yaml errors
> Add interrupt, vdd and vddio properties
> 
> Changes in v5:
> submit together with driver code as patch series
> 
> Changes in v2:
> convert to yaml format
> 
>  .../bindings/iio/accel/bosch,bmi088.yaml      | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> new file mode 100644
> index 000000000000..459b9969fd12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/bosch,bmi088.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bosch BMI088 IMU accelerometer part
> +
> +maintainers:
> +  - Mike Looijmans <mike.looijmans@topic.nl>
> +
> +description: |
> +  Acceleration part of the IMU sensor with an SPI interface
> +  Specifications about the sensor can be found at:
> +    https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi088-ds001.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - bosch,bmi088_accel

bosch,bmi088-accel

> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  vddio-supply: true
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2
> +    description: |
> +      Type should be either IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_LEVEL_LOW.
> +      The first interrupt listed must be the one connected to the INT1 pin, the
> +      second must be the one connected to the INT2 pin.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      bmi088_accel@1 {
> +        compatible = "bosch,bmi088_accel";
> +        reg = <1>;
> +        spi-max-frequency = <10000000>;
> +        interrupt-parent = <&gpio6>;
> +        interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> +      };
> +    };
> +...
> -- 
> 2.17.1
> 

Re: [PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings

[Mike Looijmans]

Wow, fast response, thanks.

Updated my dtschema, fixed the match name as requested and v7 is ready 
to go. I'm awaiting feedback from the iio people so I can send a new 
patch set.

M.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 20-01-2021 02:31, Rob Herring wrote:
> On Tue, Jan 19, 2021 at 01:46:21PM +0100, Mike Looijmans wrote:
>> This adds the device-tree bindings for the Bosch Sensortec BMI088 IMU,
>> the accelerometer part.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>
>> ---
>>
>> Changes in v6:
>> I't been almost a year since the last commit, sorry...
>> Fixed the yaml errors
>> Add interrupt, vdd and vddio properties
>>
>> Changes in v5:
>> submit together with driver code as patch series
>>
>> Changes in v2:
>> convert to yaml format
>>
>>   .../bindings/iio/accel/bosch,bmi088.yaml      | 55 +++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
>> new file mode 100644
>> index 000000000000..459b9969fd12
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
>> @@ -0,0 +1,55 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/accel/bosch,bmi088.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Bosch BMI088 IMU accelerometer part
>> +
>> +maintainers:
>> +  - Mike Looijmans <mike.looijmans@topic.nl>
>> +
>> +description: |
>> +  Acceleration part of the IMU sensor with an SPI interface
>> +  Specifications about the sensor can be found at:
>> +    https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi088-ds001.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - bosch,bmi088_accel
> bosch,bmi088-accel
>
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  vdd-supply: true
>> +
>> +  vddio-supply: true
>> +
>> +  interrupts:
>> +    minItems: 1
>> +    maxItems: 2
>> +    description: |
>> +      Type should be either IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_LEVEL_LOW.
>> +      The first interrupt listed must be the one connected to the INT1 pin, the
>> +      second must be the one connected to the INT2 pin.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    spi {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      bmi088_accel@1 {
>> +        compatible = "bosch,bmi088_accel";
>> +        reg = <1>;
>> +        spi-max-frequency = <10000000>;
>> +        interrupt-parent = <&gpio6>;
>> +        interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
>> +      };
>> +    };
>> +...
>> -- 
>> 2.17.1
>>

-- 
Mike Looijmans

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[kernel test robot]

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

Hi Mike,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on v5.11-rc4 next-20210120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mike-Looijmans/dt-bindings-iio-accel-Add-bmi088-accelerometer-bindings/20210120-152728
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f17da22f54858ae666a714de19463d6c29dbdfc6
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mike-Looijmans/dt-bindings-iio-accel-Add-bmi088-accelerometer-bindings/20210120-152728
        git checkout f17da22f54858ae666a714de19463d6c29dbdfc6
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

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

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:10,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/iio/accel/bmi088-accel-spi.c:9:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_mm.h:174:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     174 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:137:2: note: in expansion of macro 'BUG_ON'
     137 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:137:10: note: in expansion of macro 'virt_addr_valid'
     137 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   drivers/iio/accel/bmi088-accel-spi.c: At top level:
>> drivers/iio/accel/bmi088-accel-spi.c:16:5: warning: no previous prototype for 'bmi088_regmap_spi_write' [-Wmissing-prototypes]
      16 | int bmi088_regmap_spi_write(void *context, const void *data, size_t count)
         |     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/accel/bmi088-accel-spi.c:24:5: warning: no previous prototype for 'bmi088_regmap_spi_read' [-Wmissing-prototypes]
      24 | int bmi088_regmap_spi_read(void *context, const void *reg,
         |     ^~~~~~~~~~~~~~~~~~~~~~


vim +/bmi088_regmap_spi_write +16 drivers/iio/accel/bmi088-accel-spi.c

    15	
  > 16	int bmi088_regmap_spi_write(void *context, const void *data, size_t count)
    17	{
    18		struct spi_device *spi = context;
    19	
    20		/* Write register is same as generic SPI */
    21		return spi_write(spi, data, count);
    22	}
    23	
  > 24	int bmi088_regmap_spi_read(void *context, const void *reg,
    25					size_t reg_size, void *val, size_t val_size)
    26	{
    27		struct spi_device *spi = context;
    28		u8 addr[2];
    29	
    30		addr[0] = *(u8 *)reg;
    31		addr[0] |= BIT(7); /* Set RW = '1' */
    32		addr[1] = 0; /* Read requires a dummy byte transfer */
    33	
    34		return spi_write_then_read(spi, addr, sizeof(addr), val, val_size);
    35	}
    36	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59608 bytes --]

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[kernel test robot]

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

Hi Mike,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on v5.11-rc4 next-20210120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mike-Looijmans/dt-bindings-iio-accel-Add-bmi088-accelerometer-bindings/20210120-152728
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f17da22f54858ae666a714de19463d6c29dbdfc6
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mike-Looijmans/dt-bindings-iio-accel-Add-bmi088-accelerometer-bindings/20210120-152728
        git checkout f17da22f54858ae666a714de19463d6c29dbdfc6
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

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

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:10,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/iio/accel/bmi088-accel-spi.c:9:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_mm.h:174:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     174 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:137:2: note: in expansion of macro 'BUG_ON'
     137 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:137:10: note: in expansion of macro 'virt_addr_valid'
     137 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   drivers/iio/accel/bmi088-accel-spi.c: At top level:
>> drivers/iio/accel/bmi088-accel-spi.c:16:5: warning: no previous prototype for 'bmi088_regmap_spi_write' [-Wmissing-prototypes]
      16 | int bmi088_regmap_spi_write(void *context, const void *data, size_t count)
         |     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/accel/bmi088-accel-spi.c:24:5: warning: no previous prototype for 'bmi088_regmap_spi_read' [-Wmissing-prototypes]
      24 | int bmi088_regmap_spi_read(void *context, const void *reg,
         |     ^~~~~~~~~~~~~~~~~~~~~~


vim +/bmi088_regmap_spi_write +16 drivers/iio/accel/bmi088-accel-spi.c

    15	
  > 16	int bmi088_regmap_spi_write(void *context, const void *data, size_t count)
    17	{
    18		struct spi_device *spi = context;
    19	
    20		/* Write register is same as generic SPI */
    21		return spi_write(spi, data, count);
    22	}
    23	
  > 24	int bmi088_regmap_spi_read(void *context, const void *reg,
    25					size_t reg_size, void *val, size_t val_size)
    26	{
    27		struct spi_device *spi = context;
    28		u8 addr[2];
    29	
    30		addr[0] = *(u8 *)reg;
    31		addr[0] |= BIT(7); /* Set RW = '1' */
    32		addr[1] = 0; /* Read requires a dummy byte transfer */
    33	
    34		return spi_write_then_read(spi, addr, sizeof(addr), val, val_size);
    35	}
    36	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 59608 bytes --]

Re: [PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings

[Jonathan Cameron]

On Tue, 19 Jan 2021 13:46:21 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> This adds the device-tree bindings for the Bosch Sensortec BMI088 IMU,
> the accelerometer part.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
> Changes in v6:
> I't been almost a year since the last commit, sorry...
No problem. Happens to us all sometimes!

One thing inline below.

Thanks,

Jonathan

> Fixed the yaml errors
> Add interrupt, vdd and vddio properties
> 
> Changes in v5:
> submit together with driver code as patch series
> 
> Changes in v2:
> convert to yaml format
> 
>  .../bindings/iio/accel/bosch,bmi088.yaml      | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> new file mode 100644
> index 000000000000..459b9969fd12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/bosch,bmi088.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bosch BMI088 IMU accelerometer part
> +
> +maintainers:
> +  - Mike Looijmans <mike.looijmans@topic.nl>
> +
> +description: |
> +  Acceleration part of the IMU sensor with an SPI interface
> +  Specifications about the sensor can be found at:
> +    https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi088-ds001.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - bosch,bmi088_accel
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  vddio-supply: true
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2
> +    description: |
> +      Type should be either IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_LEVEL_LOW.
> +      The first interrupt listed must be the one connected to the INT1 pin, the
> +      second must be the one connected to the INT2 pin.

What if the board only has the INT2 pin connected?
That's looks to be a valid hardware configuration.

I'd suggest using interrupt-names to cover this.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      bmi088_accel@1 {
> +        compatible = "bosch,bmi088_accel";
> +        reg = <1>;
> +        spi-max-frequency = <10000000>;
> +        interrupt-parent = <&gpio6>;
> +        interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> +      };
> +    };
> +...

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Jonathan Cameron]

On Tue, 19 Jan 2021 13:46:22 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> The BMI088 is a combined module with both accelerometer and gyroscope.
> This adds the accelerometer driver support for the SPI interface.
> The gyroscope part is already supported by the BMG160 driver.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
> Changes in v6:
> Hope you have good memory - v5 was almost a year ago now
*laughs* fresh review so probably disagree with what I said back then on
something :)


A few really small comments inline seeing as you are respinning anyway.

> Remove superfluous *val=0
> Make sample_frequency selection into read_avail list
> 
> Changes in v5:
> Add includes and forward defines in header
> BIT(7) instead of 0x80
> Reset already sets defaults, do not set them again
> Remove now unused bmi088_accel_set_bw
> Remove unused AXIS_MAX
> Use MASK define for ODR setting
> Explain buffer use and alignment
> Split bmi088_accel_set_power_state into "on" and "off" parts
> Cosmetic changes to improve readability
> 
> Changes in v4:
> Remove unused #include directives
> Remove unused #defines for event and irq
> Replace (ret < 0) with (ret) for all regmap calls
> Consistent checking of IO errors in probe and init
> Removed #ifdef CONFIG_PM guard
> Use bitops for set_frequency instead of loop with shift
> s/__s16/s16/g
> Remove excess blank lines
> Don't return -EAGAIN in pm_runtime
> 
> Changes in v3:
> Processed comments from Jonathan Cameron and Lars-Peter Clausen
> implement runtime PM (tested by code tracing) and sleep
> fix scale and offset factors for accel and temperature and
> return raw values instead of pre-scaled ones
> Use iio_device_{claim,release}_direct_mode
> Remove unused code and structs
> Use a cache-aligned buffer for bulk read
> Configure and enable caching register values
> 
> Changes in v2:
> Remove unused typedefs and variables
> Fix error return when iio_device_register fails
> 
>  drivers/iio/accel/Kconfig             |  18 +
>  drivers/iio/accel/Makefile            |   2 +
>  drivers/iio/accel/bmi088-accel-core.c | 630 ++++++++++++++++++++++++++
>  drivers/iio/accel/bmi088-accel-spi.c  |  85 ++++
>  drivers/iio/accel/bmi088-accel.h      |  18 +
>  5 files changed, 753 insertions(+)
>  create mode 100644 drivers/iio/accel/bmi088-accel-core.c
>  create mode 100644 drivers/iio/accel/bmi088-accel-spi.c
>  create mode 100644 drivers/iio/accel/bmi088-accel.h
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 2e0c62c39155..cceda3cecbcf 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -157,6 +157,24 @@ config BMC150_ACCEL_SPI
>  	tristate
>  	select REGMAP_SPI
>  
> +config BMI088_ACCEL
> +	tristate "Bosch BMI088 Accelerometer Driver"
> +	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	select REGMAP
> +	select BMI088_ACCEL_SPI
> +	help
> +	  Say yes here to build support for the Bosch BMI088 accelerometer.
> +
> +	  This is a combo module with both accelerometer and gyroscope. This
> +	  driver only implements the accelerometer part, which has its own
> +	  address and register map. BMG160 provides the gyroscope driver.
> +
> +config BMI088_ACCEL_SPI
> +	tristate
> +	select REGMAP_SPI
> +
>  config DA280
>  	tristate "MiraMEMS DA280 3-axis 14-bit digital accelerometer driver"
>  	depends on I2C
> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> index 4f6c1ebe13b0..32cd1342a31a 100644
> --- a/drivers/iio/accel/Makefile
> +++ b/drivers/iio/accel/Makefile
> @@ -20,6 +20,8 @@ obj-$(CONFIG_BMA400_SPI) += bma400_spi.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
> +obj-$(CONFIG_BMI088_ACCEL) += bmi088-accel-core.o
> +obj-$(CONFIG_BMI088_ACCEL_SPI) += bmi088-accel-spi.o
>  obj-$(CONFIG_DA280)	+= da280.o
>  obj-$(CONFIG_DA311)	+= da311.o
>  obj-$(CONFIG_DMARD06)	+= dmard06.o
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> new file mode 100644
> index 000000000000..788e54ed0728
> --- /dev/null
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -0,0 +1,630 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
> + *  - BMI088
> + *
> + * Copyright (c) 2018-2020, Topic Embedded Products
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
As below. Alphabetical ordering preferred.

> +#include <linux/slab.h>
> +#include <linux/acpi.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regmap.h>
> +#include <asm/unaligned.h>
> +
> +#include "bmi088-accel.h"
...

> +static int bmi088_accel_chip_init(struct bmi088_accel_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret, i;
> +	unsigned int val;
> +
> +	/* Do a dummy read to enable SPI interface, won't harm I2C */
> +	regmap_read(data->regmap, BMI088_ACCEL_REG_INT_STATUS, &val);
> +
> +	/*
> +	 * Reset chip to get it in a known good state. A delay of 1ms after
> +	 * reset is required according to the data sheet
> +	 */
> +	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_RESET,
> +			   BMI088_ACCEL_RESET_VAL);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(1000, 2000);
> +
> +	/* Do a dummy read again after a reset to enable the SPI interface */
> +	regmap_read(data->regmap, BMI088_ACCEL_REG_INT_STATUS, &val);
> +
> +	/* Read chip ID */
> +	ret = regmap_read(data->regmap, BMI088_ACCEL_REG_CHIP_ID, &val);
> +	if (ret) {
> +		dev_err(dev, "Error: Reading chip id\n");
> +		return ret;
> +	}
> +
> +	/* Validate chip ID */
> +	for (i = 0; i < ARRAY_SIZE(bmi088_accel_chip_info_tbl); i++) {
> +		if (bmi088_accel_chip_info_tbl[i].chip_id == val) {
> +			data->chip_info = &bmi088_accel_chip_info_tbl[i];
> +			break;
> +		}
> +	}
> +	if (i == ARRAY_SIZE(bmi088_accel_chip_info_tbl)) {
> +		dev_err(dev, "Invalid chip %x\n", val);
> +		return -ENODEV;
> +	}
> +
> +	/* Enable accelerometer after reset */
> +	return bmi088_accel_enable(data, true);

We bring the device up here, but I'm not seeing it turned off again
in either error paths of probe or remove.
Am I missing it somewhere?

> +}
> +
> +int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
> +	int irq, const char *name, bool block_supported)
> +{
> +	struct bmi088_accel_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	dev_set_drvdata(dev, indio_dev);
> +
> +	data->regmap = regmap;
> +
> +	ret = bmi088_accel_chip_init(data);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&data->mutex);
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->channels = data->chip_info->channels;
> +	indio_dev->num_channels = data->chip_info->num_channels;
> +	indio_dev->name = name ? name : data->chip_info->name;
> +	indio_dev->available_scan_masks = bmi088_accel_scan_masks;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &bmi088_accel_info;
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, BMI088_AUTO_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		dev_err(dev, "Unable to register iio device\n");
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(bmi088_accel_core_probe);
> +
> +int bmi088_accel_core_remove(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_put_noidle(dev);
> +
> +	mutex_lock(&data->mutex);
> +	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
> +	mutex_unlock(&data->mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(bmi088_accel_core_remove);
> +
> +/* When going into system sleep, put the chip in power down */
> +static int __maybe_unused bmi088_accel_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
> +	bmi088_accel_enable(data, false);
> +	mutex_unlock(&data->mutex);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused bmi088_accel_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +	bmi088_accel_enable(data, true);
> +	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_ACTIVE);
> +	mutex_unlock(&data->mutex);
> +
> +	return 0;
> +}
> +
> +/* For runtime PM put the chip in suspend mode */
> +static int __maybe_unused bmi088_accel_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
> +
> +	return bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
> +}
> +
> +static int __maybe_unused bmi088_accel_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_ACTIVE);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(BMI088_ACCEL_MAX_STARTUP_TIME_US,
> +		     BMI088_ACCEL_MAX_STARTUP_TIME_US * 2);
> +
> +	return 0;
> +}
> +
> +const struct dev_pm_ops bmi088_accel_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(bmi088_accel_suspend, bmi088_accel_resume)
> +	SET_RUNTIME_PM_OPS(bmi088_accel_runtime_suspend,
> +			   bmi088_accel_runtime_resume, NULL)
> +};
> +EXPORT_SYMBOL_GPL(bmi088_accel_pm_ops);
> +
> +MODULE_AUTHOR("Niek van Agt <niek.van.agt@topicproducts.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("BMI088 accelerometer driver (core)");
> diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
> new file mode 100644
> index 000000000000..7794090b8e6d
> --- /dev/null
> +++ b/drivers/iio/accel/bmi088-accel-spi.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
> + *  - BMI088
> + *
> + * Copyright (c) 2018-2020, Topic Embedded Products
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regmap.h>

If no other reason for ordering ever so slight preference for
alphabetical order.

> +
> +#include "bmi088-accel.h"
> +
> +int bmi088_regmap_spi_write(void *context, const void *data, size_t count)
> +{
> +	struct spi_device *spi = context;
> +
> +	/* Write register is same as generic SPI */
> +	return spi_write(spi, data, count);
> +}
> +
> +int bmi088_regmap_spi_read(void *context, const void *reg,
> +				size_t reg_size, void *val, size_t val_size)
> +{
> +	struct spi_device *spi = context;
> +	u8 addr[2];
> +
> +	addr[0] = *(u8 *)reg;
> +	addr[0] |= BIT(7); /* Set RW = '1' */
> +	addr[1] = 0; /* Read requires a dummy byte transfer */
> +
> +	return spi_write_then_read(spi, addr, sizeof(addr), val, val_size);
> +}
> +
> +static struct regmap_bus bmi088_regmap_bus = {
> +	.write = bmi088_regmap_spi_write,
> +	.read = bmi088_regmap_spi_read,
> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,

Aren't these both 8 bits, making endian rather irrelevant?

> +};
...

Re: [PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings

[Mike Looijmans]

Comments inlined below.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 20-01-2021 19:50, Jonathan Cameron wrote:
> On Tue, 19 Jan 2021 13:46:21 +0100
> Mike Looijmans <mike.looijmans@topic.nl> wrote:
> 
>> This adds the device-tree bindings for the Bosch Sensortec BMI088 IMU,
>> the accelerometer part.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>
>> ---
>>
>> Changes in v6:
>> I't been almost a year since the last commit, sorry...
> No problem. Happens to us all sometimes!
> 
> One thing inline below.
> 
> Thanks,
> 
> Jonathan
> 
>> Fixed the yaml errors
>> Add interrupt, vdd and vddio properties
>>
>> Changes in v5:
>> submit together with driver code as patch series
>>
>> Changes in v2:
>> convert to yaml format
>>
>>   .../bindings/iio/accel/bosch,bmi088.yaml      | 55 +++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
>> new file mode 100644
>> index 000000000000..459b9969fd12
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
>> @@ -0,0 +1,55 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/accel/bosch,bmi088.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Bosch BMI088 IMU accelerometer part
>> +
>> +maintainers:
>> +  - Mike Looijmans <mike.looijmans@topic.nl>
>> +
>> +description: |
>> +  Acceleration part of the IMU sensor with an SPI interface
>> +  Specifications about the sensor can be found at:
>> +    https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi088-ds001.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - bosch,bmi088_accel
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  vdd-supply: true
>> +
>> +  vddio-supply: true
>> +
>> +  interrupts:
>> +    minItems: 1
>> +    maxItems: 2
>> +    description: |
>> +      Type should be either IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_LEVEL_LOW.
>> +      The first interrupt listed must be the one connected to the INT1 pin, the
>> +      second must be the one connected to the INT2 pin.
> 
> What if the board only has the INT2 pin connected?
> That's looks to be a valid hardware configuration.
> 
> I'd suggest using interrupt-names to cover this.

Yeah makes sense. The pins have the same functionality, so either one will do.

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    spi {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      bmi088_accel@1 {
>> +        compatible = "bosch,bmi088_accel";
>> +        reg = <1>;
>> +        spi-max-frequency = <10000000>;
>> +        interrupt-parent = <&gpio6>;
>> +        interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
>> +      };
>> +    };
>> +...
> 

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Mike Looijmans]

Comments inlined below.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 20-01-2021 21:22, Jonathan Cameron wrote:
> On Tue, 19 Jan 2021 13:46:22 +0100
> Mike Looijmans <mike.looijmans@topic.nl> wrote:
> 
>> The BMI088 is a combined module with both accelerometer and gyroscope.
>> This adds the accelerometer driver support for the SPI interface.
>> The gyroscope part is already supported by the BMG160 driver.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>
>> Changes in v6:
>> Hope you have good memory - v5 was almost a year ago now
> *laughs* fresh review so probably disagree with what I said back then on
> something :)
> 
> 
> A few really small comments inline seeing as you are respinning anyway.
> 
>> Remove superfluous *val=0
>> Make sample_frequency selection into read_avail list
>>
>> Changes in v5:
>> Add includes and forward defines in header
>> BIT(7) instead of 0x80
>> Reset already sets defaults, do not set them again
>> Remove now unused bmi088_accel_set_bw
>> Remove unused AXIS_MAX
>> Use MASK define for ODR setting
>> Explain buffer use and alignment
>> Split bmi088_accel_set_power_state into "on" and "off" parts
>> Cosmetic changes to improve readability
>>
>> Changes in v4:
>> Remove unused #include directives
>> Remove unused #defines for event and irq
>> Replace (ret < 0) with (ret) for all regmap calls
>> Consistent checking of IO errors in probe and init
>> Removed #ifdef CONFIG_PM guard
>> Use bitops for set_frequency instead of loop with shift
>> s/__s16/s16/g
>> Remove excess blank lines
>> Don't return -EAGAIN in pm_runtime
>>
>> Changes in v3:
>> Processed comments from Jonathan Cameron and Lars-Peter Clausen
>> implement runtime PM (tested by code tracing) and sleep
>> fix scale and offset factors for accel and temperature and
>> return raw values instead of pre-scaled ones
>> Use iio_device_{claim,release}_direct_mode
>> Remove unused code and structs
>> Use a cache-aligned buffer for bulk read
>> Configure and enable caching register values
>>
>> Changes in v2:
>> Remove unused typedefs and variables
>> Fix error return when iio_device_register fails
>>
>>   drivers/iio/accel/Kconfig             |  18 +
>>   drivers/iio/accel/Makefile            |   2 +
>>   drivers/iio/accel/bmi088-accel-core.c | 630 ++++++++++++++++++++++++++
>>   drivers/iio/accel/bmi088-accel-spi.c  |  85 ++++
>>   drivers/iio/accel/bmi088-accel.h      |  18 +
>>   5 files changed, 753 insertions(+)
>>   create mode 100644 drivers/iio/accel/bmi088-accel-core.c
>>   create mode 100644 drivers/iio/accel/bmi088-accel-spi.c
>>   create mode 100644 drivers/iio/accel/bmi088-accel.h
>>
>> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
>> index 2e0c62c39155..cceda3cecbcf 100644
>> --- a/drivers/iio/accel/Kconfig
>> +++ b/drivers/iio/accel/Kconfig
>> @@ -157,6 +157,24 @@ config BMC150_ACCEL_SPI
>>   	tristate
>>   	select REGMAP_SPI
>>   
>> +config BMI088_ACCEL
>> +	tristate "Bosch BMI088 Accelerometer Driver"
>> +	depends on SPI
>> +	select IIO_BUFFER
>> +	select IIO_TRIGGERED_BUFFER
>> +	select REGMAP
>> +	select BMI088_ACCEL_SPI
>> +	help
>> +	  Say yes here to build support for the Bosch BMI088 accelerometer.
>> +
>> +	  This is a combo module with both accelerometer and gyroscope. This
>> +	  driver only implements the accelerometer part, which has its own
>> +	  address and register map. BMG160 provides the gyroscope driver.
>> +
>> +config BMI088_ACCEL_SPI
>> +	tristate
>> +	select REGMAP_SPI
>> +
>>   config DA280
>>   	tristate "MiraMEMS DA280 3-axis 14-bit digital accelerometer driver"
>>   	depends on I2C
>> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
>> index 4f6c1ebe13b0..32cd1342a31a 100644
>> --- a/drivers/iio/accel/Makefile
>> +++ b/drivers/iio/accel/Makefile
>> @@ -20,6 +20,8 @@ obj-$(CONFIG_BMA400_SPI) += bma400_spi.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
>> +obj-$(CONFIG_BMI088_ACCEL) += bmi088-accel-core.o
>> +obj-$(CONFIG_BMI088_ACCEL_SPI) += bmi088-accel-spi.o
>>   obj-$(CONFIG_DA280)	+= da280.o
>>   obj-$(CONFIG_DA311)	+= da311.o
>>   obj-$(CONFIG_DMARD06)	+= dmard06.o
>> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
>> new file mode 100644
>> index 000000000000..788e54ed0728
>> --- /dev/null
>> +++ b/drivers/iio/accel/bmi088-accel-core.c
>> @@ -0,0 +1,630 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
>> + *  - BMI088
>> + *
>> + * Copyright (c) 2018-2020, Topic Embedded Products
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/delay.h>
> As below. Alphabetical ordering preferred.

Will do.

> 
>> +#include <linux/slab.h>
>> +#include <linux/acpi.h>
>> +#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/regmap.h>
>> +#include <asm/unaligned.h>
>> +
>> +#include "bmi088-accel.h"
> ...
> 
>> +static int bmi088_accel_chip_init(struct bmi088_accel_data *data)
>> +{
>> +	struct device *dev = regmap_get_device(data->regmap);
>> +	int ret, i;
>> +	unsigned int val;
>> +
>> +	/* Do a dummy read to enable SPI interface, won't harm I2C */
>> +	regmap_read(data->regmap, BMI088_ACCEL_REG_INT_STATUS, &val);
>> +
>> +	/*
>> +	 * Reset chip to get it in a known good state. A delay of 1ms after
>> +	 * reset is required according to the data sheet
>> +	 */
>> +	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_RESET,
>> +			   BMI088_ACCEL_RESET_VAL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	usleep_range(1000, 2000);
>> +
>> +	/* Do a dummy read again after a reset to enable the SPI interface */
>> +	regmap_read(data->regmap, BMI088_ACCEL_REG_INT_STATUS, &val);
>> +
>> +	/* Read chip ID */
>> +	ret = regmap_read(data->regmap, BMI088_ACCEL_REG_CHIP_ID, &val);
>> +	if (ret) {
>> +		dev_err(dev, "Error: Reading chip id\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Validate chip ID */
>> +	for (i = 0; i < ARRAY_SIZE(bmi088_accel_chip_info_tbl); i++) {
>> +		if (bmi088_accel_chip_info_tbl[i].chip_id == val) {
>> +			data->chip_info = &bmi088_accel_chip_info_tbl[i];
>> +			break;
>> +		}
>> +	}
>> +	if (i == ARRAY_SIZE(bmi088_accel_chip_info_tbl)) {
>> +		dev_err(dev, "Invalid chip %x\n", val);
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Enable accelerometer after reset */
>> +	return bmi088_accel_enable(data, true);
> 
> We bring the device up here, but I'm not seeing it turned off again
> in either error paths of probe or remove.
> Am I missing it somewhere?

Nah makes sense to put it back in the disabled state at removal.
I'll re-use the "suspend" code in the "remove" call so it'll be less code in 
total and things will be symetric.

> 
>> +}
>> +
>> +int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
>> +	int irq, const char *name, bool block_supported)
>> +{
>> +	struct bmi088_accel_data *data;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	data = iio_priv(indio_dev);
>> +	dev_set_drvdata(dev, indio_dev);
>> +
>> +	data->regmap = regmap;
>> +
>> +	ret = bmi088_accel_chip_init(data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mutex_init(&data->mutex);
>> +
>> +	indio_dev->dev.parent = dev;
>> +	indio_dev->channels = data->chip_info->channels;
>> +	indio_dev->num_channels = data->chip_info->num_channels;
>> +	indio_dev->name = name ? name : data->chip_info->name;
>> +	indio_dev->available_scan_masks = bmi088_accel_scan_masks;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->info = &bmi088_accel_info;
>> +
>> +	ret = pm_runtime_set_active(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pm_runtime_enable(dev);
>> +	pm_runtime_set_autosuspend_delay(dev, BMI088_AUTO_SUSPEND_DELAY_MS);
>> +	pm_runtime_use_autosuspend(dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret)
>> +		dev_err(dev, "Unable to register iio device\n");
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(bmi088_accel_core_probe);
>> +
>> +int bmi088_accel_core_remove(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
>> +
>> +	iio_device_unregister(indio_dev);
>> +
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_set_suspended(dev);
>> +	pm_runtime_put_noidle(dev);
>> +
>> +	mutex_lock(&data->mutex);
>> +	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(bmi088_accel_core_remove);
>> +
>> +/* When going into system sleep, put the chip in power down */
>> +static int __maybe_unused bmi088_accel_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
>> +
>> +	mutex_lock(&data->mutex);
>> +	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
>> +	bmi088_accel_enable(data, false);
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused bmi088_accel_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
>> +
>> +	mutex_lock(&data->mutex);
>> +	bmi088_accel_enable(data, true);
>> +	bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_ACTIVE);
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return 0;
>> +}
>> +
>> +/* For runtime PM put the chip in suspend mode */
>> +static int __maybe_unused bmi088_accel_runtime_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
>> +
>> +	return bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
>> +}
>> +
>> +static int __maybe_unused bmi088_accel_runtime_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	ret = bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_ACTIVE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	usleep_range(BMI088_ACCEL_MAX_STARTUP_TIME_US,
>> +		     BMI088_ACCEL_MAX_STARTUP_TIME_US * 2);
>> +
>> +	return 0;
>> +}
>> +
>> +const struct dev_pm_ops bmi088_accel_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(bmi088_accel_suspend, bmi088_accel_resume)
>> +	SET_RUNTIME_PM_OPS(bmi088_accel_runtime_suspend,
>> +			   bmi088_accel_runtime_resume, NULL)
>> +};
>> +EXPORT_SYMBOL_GPL(bmi088_accel_pm_ops);
>> +
>> +MODULE_AUTHOR("Niek van Agt <niek.van.agt@topicproducts.com>");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("BMI088 accelerometer driver (core)");
>> diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
>> new file mode 100644
>> index 000000000000..7794090b8e6d
>> --- /dev/null
>> +++ b/drivers/iio/accel/bmi088-accel-spi.c
>> @@ -0,0 +1,85 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
>> + *  - BMI088
>> + *
>> + * Copyright (c) 2018-2020, Topic Embedded Products
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/regmap.h>
> 
> If no other reason for ordering ever so slight preference for
> alphabetical order.

Will do

> 
>> +
>> +#include "bmi088-accel.h"
>> +
>> +int bmi088_regmap_spi_write(void *context, const void *data, size_t count)

This should have been a static (as reported by build bot)

>> +{
>> +	struct spi_device *spi = context;
>> +
>> +	/* Write register is same as generic SPI */
>> +	return spi_write(spi, data, count);
>> +}
>> +
>> +int bmi088_regmap_spi_read(void *context, const void *reg,
>> +				size_t reg_size, void *val, size_t val_size)
>> +{
>> +	struct spi_device *spi = context;
>> +	u8 addr[2];
>> +
>> +	addr[0] = *(u8 *)reg;
>> +	addr[0] |= BIT(7); /* Set RW = '1' */
>> +	addr[1] = 0; /* Read requires a dummy byte transfer */
>> +
>> +	return spi_write_then_read(spi, addr, sizeof(addr), val, val_size);
>> +}
>> +
>> +static struct regmap_bus bmi088_regmap_bus = {
>> +	.write = bmi088_regmap_spi_write,
>> +	.read = bmi088_regmap_spi_read,
>> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
>> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,
> 
> Aren't these both 8 bits, making endian rather irrelevant?

Indeed, all registers are accessed as 8-bit only. Where bulk-read is involved, 
the byte order in interpreted in code.

> 
>> +};
> ...
> 

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Linus Walleij]

Hi Mike,

thanks for your patch!

I have a comment about PM:

On Tue, Jan 19, 2021 at 1:46 PM Mike Looijmans <mike.looijmans@topic.nl> wrote:

> The BMI088 is a combined module with both accelerometer and gyroscope.
> This adds the accelerometer driver support for the SPI interface.
> The gyroscope part is already supported by the BMG160 driver.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
(...)

> +static int bmi088_accel_set_power_state_on(struct bmi088_accel_data *data)
> +{
> +       struct device *dev = regmap_get_device(data->regmap);
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(dev);
> +       if (ret < 0) {
> +               pm_runtime_put_noidle(dev);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int bmi088_accel_set_power_state_off(struct bmi088_accel_data *data)
> +{
> +       struct device *dev = regmap_get_device(data->regmap);
> +       int ret;
> +
> +       pm_runtime_mark_last_busy(dev);
> +       ret = pm_runtime_put_autosuspend(dev);
> +
> +       return ret < 0 ? ret : 0;
> +}

I'm not sure you should wrap the pm_runtime calls like this.
I think it is better to inline them.

Next, I think it is better to let suspend/resume, i.e. system PM
reuse runtime PM since you're implementing that. This is why
we invented PM runtime force resume and force suspend.

Here are some drivers that I implemented using that model:
drivers/iio/gyro/mpu3050-core.c
drivers/iio/accel/kxsd9.c
drivers/iio/magnetometer/ak8974.c

The short summary is:
- Only implement runtime suspend/resume.
- SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
   pm_runtime_force_resume)
- In probe() enable runtime PM with autosuspend:
  pm_runtime_get_noresume(dev);
  pm_runtime_set_active(dev);
  pm_runtime_enable(dev);
  pm_runtime_set_autosuspend_delay(dev, NNNN);
  pm_runtime_use_autosuspend(dev);
  pm_runtime_put(dev);
- In remove() disable it like this:
  pm_runtime_get_sync(dev);
  pm_runtime_put_noidle(dev);
  pm_runtime_disable(dev);
- Any time the driver needs the hardware, call:
  pm_runtime_get_sync(dev);
- As soon as you're done using the hardware call:
  pm_runtime_mark_last_busy(dev);
  pm_runtime_put_autosuspend(dev);

The system PM will just hook into the same callbacks and suspend
the hardware using the existing runtime PM hooks.

This works fine in my drivers and saves some complexity and avoids
bugs.

Yours,
Linus Walleij

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Jonathan Cameron]

On Fri, 22 Jan 2021 23:38:48 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> Hi Mike,
> 
> thanks for your patch!
> 
> I have a comment about PM:
> 
> On Tue, Jan 19, 2021 at 1:46 PM Mike Looijmans <mike.looijmans@topic.nl> wrote:
> 
> > The BMI088 is a combined module with both accelerometer and gyroscope.
> > This adds the accelerometer driver support for the SPI interface.
> > The gyroscope part is already supported by the BMG160 driver.
> >
> > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>  
> (...)
> 
> > +static int bmi088_accel_set_power_state_on(struct bmi088_accel_data *data)
> > +{
> > +       struct device *dev = regmap_get_device(data->regmap);
> > +       int ret;
> > +
> > +       ret = pm_runtime_get_sync(dev);
> > +       if (ret < 0) {
> > +               pm_runtime_put_noidle(dev);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int bmi088_accel_set_power_state_off(struct bmi088_accel_data *data)
> > +{
> > +       struct device *dev = regmap_get_device(data->regmap);
> > +       int ret;
> > +
> > +       pm_runtime_mark_last_busy(dev);
> > +       ret = pm_runtime_put_autosuspend(dev);
> > +
> > +       return ret < 0 ? ret : 0;
> > +}  
> 
> I'm not sure you should wrap the pm_runtime calls like this.
> I think it is better to inline them.
> 
> Next, I think it is better to let suspend/resume, i.e. system PM
> reuse runtime PM since you're implementing that. This is why
> we invented PM runtime force resume and force suspend.

Here the driver is turning more off for full suspend than in the
runtime path.  If that results in significant extra delay then
it's not appropriate to have that in the runtime suspend path.

Maybe the simplification of not doing the deeper power saving
mode is worth the extra power cost or extra delay, but
I'm not yet convinced.

I'll hold off on applying v7 though whilst we discuss this.

J

> 
> Here are some drivers that I implemented using that model:
> drivers/iio/gyro/mpu3050-core.c
> drivers/iio/accel/kxsd9.c
> drivers/iio/magnetometer/ak8974.c
> 
> The short summary is:
> - Only implement runtime suspend/resume.
> - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>    pm_runtime_force_resume)
> - In probe() enable runtime PM with autosuspend:
>   pm_runtime_get_noresume(dev);
>   pm_runtime_set_active(dev);
>   pm_runtime_enable(dev);
>   pm_runtime_set_autosuspend_delay(dev, NNNN);
>   pm_runtime_use_autosuspend(dev);
>   pm_runtime_put(dev);
> - In remove() disable it like this:
>   pm_runtime_get_sync(dev);
>   pm_runtime_put_noidle(dev);
>   pm_runtime_disable(dev);
> - Any time the driver needs the hardware, call:
>   pm_runtime_get_sync(dev);
> - As soon as you're done using the hardware call:
>   pm_runtime_mark_last_busy(dev);
>   pm_runtime_put_autosuspend(dev);
> 
> The system PM will just hook into the same callbacks and suspend
> the hardware using the existing runtime PM hooks.
> 
> This works fine in my drivers and saves some complexity and avoids
> bugs.
> 
> Yours,
> Linus Walleij

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Linus Walleij]

On Sat, Jan 23, 2021 at 4:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
> [Me]
> > Next, I think it is better to let suspend/resume, i.e. system PM
> > reuse runtime PM since you're implementing that. This is why
> > we invented PM runtime force resume and force suspend.
>
> Here the driver is turning more off for full suspend than in the
> runtime path.  If that results in significant extra delay then
> it's not appropriate to have that in the runtime suspend path.

I see the point.

The resume path calls bmi088_accel_enable() which incurs
a 5ms delay.

The runtime resume path incurs a 1 ms delay.

The runtime autosuspend kicks in after 2 ms.

> Maybe the simplification of not doing the deeper power saving
> mode is worth the extra power cost or extra delay, but
> I'm not yet convinced.

I would personally set the autosuspend to ~20ms and just use
one path and take a hit of 5 ms whenever we go down between
measures if it is a system that is for human interaction, but for
control systems this more complex set-up may be better for
response latencies.

The current approach may be better tuned to perfection and
we are all perfectionists :D

I'm just worrying a little about bugs and maintainability.

Yours,
Linus Walleij

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Jonathan Cameron]

On Sun, 24 Jan 2021 00:21:13 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sat, Jan 23, 2021 at 4:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > [Me]  
> > > Next, I think it is better to let suspend/resume, i.e. system PM
> > > reuse runtime PM since you're implementing that. This is why
> > > we invented PM runtime force resume and force suspend.  
> >
> > Here the driver is turning more off for full suspend than in the
> > runtime path.  If that results in significant extra delay then
> > it's not appropriate to have that in the runtime suspend path.  
> 
> I see the point.
> 
> The resume path calls bmi088_accel_enable() which incurs
> a 5ms delay.
> 
> The runtime resume path incurs a 1 ms delay.
> 
> The runtime autosuspend kicks in after 2 ms.
> 
> > Maybe the simplification of not doing the deeper power saving
> > mode is worth the extra power cost or extra delay, but
> > I'm not yet convinced.  
> 
> I would personally set the autosuspend to ~20ms and just use
> one path and take a hit of 5 ms whenever we go down between
> measures if it is a system that is for human interaction, but for
> control systems this more complex set-up may be better for
> response latencies.
> 
> The current approach may be better tuned to perfection and
> we are all perfectionists :D
> 
> I'm just worrying a little about bugs and maintainability.
Fully understood.  Though for things like this I like to leave
it at the discretion of the driver author as fairly safe they
are a user of the device.  

May well make sense to go with the longer times as you
suggest though!  Over to you Mike :)

Jonathan

> 
> Yours,
> Linus Walleij

Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Mike Looijmans]

See below


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 24-01-2021 14:23, Jonathan Cameron wrote:
> On Sun, 24 Jan 2021 00:21:13 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> On Sat, Jan 23, 2021 at 4:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
>>> [Me]
>>>> Next, I think it is better to let suspend/resume, i.e. system PM
>>>> reuse runtime PM since you're implementing that. This is why
>>>> we invented PM runtime force resume and force suspend.
>>> Here the driver is turning more off for full suspend than in the
>>> runtime path.  If that results in significant extra delay then
>>> it's not appropriate to have that in the runtime suspend path.
>> I see the point.
>>
>> The resume path calls bmi088_accel_enable() which incurs
>> a 5ms delay.
>>
>> The runtime resume path incurs a 1 ms delay.
>>
>> The runtime autosuspend kicks in after 2 ms.

It's set to 2 seconds as I understand it. This to support reading a 
single value every second or so.

>>
>>> Maybe the simplification of not doing the deeper power saving
>>> mode is worth the extra power cost or extra delay, but
>>> I'm not yet convinced.
>> I would personally set the autosuspend to ~20ms and just use
>> one path and take a hit of 5 ms whenever we go down between
>> measures if it is a system that is for human interaction, but for
>> control systems this more complex set-up may be better for
>> response latencies.
>>
>> The current approach may be better tuned to perfection and
>> we are all perfectionists :D
>>
>> I'm just worrying a little about bugs and maintainability.
> Fully understood.  Though for things like this I like to leave
> it at the discretion of the driver author as fairly safe they
> are a user of the device.
>
> May well make sense to go with the longer times as you
> suggest though!  Over to you Mike :)

I've been digging in the datasheet and it's really unclear how you're 
supposed to control the two power registers.

I think it's best to just put both control values into on/off state at 
the same time. I also prefer the simplicity of Linus' suggestion. I'll 
do some testing to see if the device behaves properly.

-- 
Mike Looijmans