[PATCH 0/5] iio:bmi160: add drdy interrupt support

[PATCH 0/5] iio:bmi160: add drdy interrupt support

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

This patch series adds drdy interrupt support to the BMI160 IMU. It also updates
the relevant DT bindings and does a little bit of related cleanup.

Martin Kelly (5):
  iio:bmi160: add SPDX identifiers
  iio:bmi160: add drdy interrupt support
  dt-bindings: fix incorrect bmi160 IRQ note
  dt-bindings:bmi160: add "BOTH" int pin type
  iio:bmi160:snap timestamp closer to event

 .../devicetree/bindings/iio/imu/bmi160.txt         |  17 +-
 drivers/iio/imu/bmi160/Makefile                    |   1 +
 drivers/iio/imu/bmi160/bmi160.h                    |  13 +-
 drivers/iio/imu/bmi160/bmi160_core.c               | 280 ++++++++++++++++++++-
 drivers/iio/imu/bmi160/bmi160_i2c.c                |   8 +-
 drivers/iio/imu/bmi160/bmi160_spi.c                |   6 +-
 drivers/iio/imu/bmi160/bmi160_trigger.c            |  60 +++++
 7 files changed, 360 insertions(+), 25 deletions(-)
 create mode 100644 drivers/iio/imu/bmi160/bmi160_trigger.c

--
2.11.0

[PATCH 1/5] iio:bmi160: add SPDX identifiers

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

Add SPDX identifiers (GPL 2) for the BMI160 driver. bmi160.h had an
identifier, but the other files did not.

Signed-off-by: Martin Kelly <martin@martingkelly.com>
---
 drivers/iio/imu/bmi160/bmi160_core.c | 5 +----
 drivers/iio/imu/bmi160/bmi160_i2c.c  | 5 +----
 drivers/iio/imu/bmi160/bmi160_spi.c  | 4 +---
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index b10330b0f93f..ce61026d84c3 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -1,12 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
  *
  * Copyright (c) 2016, Intel Corporation.
  *
- * This file is subject to the terms and conditions of version 2 of
- * the GNU General Public License.  See the file COPYING in the main
- * directory of this archive for more details.
- *
  * IIO core driver for BMI160, with support for I2C/SPI busses
  *
  * TODO: magnetometer, interrupts, hardware FIFO
diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
index 5b1f7e6af651..e36f5e82d400 100644
--- a/drivers/iio/imu/bmi160/bmi160_i2c.c
+++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
@@ -1,12 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * BMI160 - Bosch IMU, I2C bits
  *
  * Copyright (c) 2016, Intel Corporation.
  *
- * This file is subject to the terms and conditions of version 2 of
- * the GNU General Public License.  See the file COPYING in the main
- * directory of this archive for more details.
- *
  * 7-bit I2C slave address is:
  *      - 0x68 if SDO is pulled to GND
  *      - 0x69 if SDO is pulled to VDDIO
diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
index e521ad14eeac..c19e3df35559 100644
--- a/drivers/iio/imu/bmi160/bmi160_spi.c
+++ b/drivers/iio/imu/bmi160/bmi160_spi.c
@@ -1,11 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * BMI160 - Bosch IMU, SPI bits
  *
  * Copyright (c) 2016, Intel Corporation.
  *
- * This file is subject to the terms and conditions of version 2 of
- * the GNU General Public License.  See the file COPYING in the main
- * directory of this archive for more details.
  */
 #include <linux/acpi.h>
 #include <linux/module.h>
-- 
2.11.0

Re: [PATCH 1/5] iio:bmi160: add SPDX identifiers

[Jonathan Cameron]

On Sun, 13 Jan 2019 19:56:17 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> From: Martin Kelly <martin@martingkelly.com>
> 
> Add SPDX identifiers (GPL 2) for the BMI160 driver. bmi160.h had an
> identifier, but the other files did not.
> 
> Signed-off-by: Martin Kelly <martin@martingkelly.com>

This is fine, but for anything (even SPDX) that touches licenses
I ideally like to get an Ack from the main author.  
cc'd Daniel.

> ---
>  drivers/iio/imu/bmi160/bmi160_core.c | 5 +----
>  drivers/iio/imu/bmi160/bmi160_i2c.c  | 5 +----
>  drivers/iio/imu/bmi160/bmi160_spi.c  | 4 +---
>  3 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index b10330b0f93f..ce61026d84c3 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -1,12 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
>   *
>   * Copyright (c) 2016, Intel Corporation.
>   *
> - * This file is subject to the terms and conditions of version 2 of
> - * the GNU General Public License.  See the file COPYING in the main
> - * directory of this archive for more details.
> - *
>   * IIO core driver for BMI160, with support for I2C/SPI busses
>   *
>   * TODO: magnetometer, interrupts, hardware FIFO
> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
> index 5b1f7e6af651..e36f5e82d400 100644
> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
> @@ -1,12 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * BMI160 - Bosch IMU, I2C bits
>   *
>   * Copyright (c) 2016, Intel Corporation.
>   *
> - * This file is subject to the terms and conditions of version 2 of
> - * the GNU General Public License.  See the file COPYING in the main
> - * directory of this archive for more details.
> - *
>   * 7-bit I2C slave address is:
>   *      - 0x68 if SDO is pulled to GND
>   *      - 0x69 if SDO is pulled to VDDIO
> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
> index e521ad14eeac..c19e3df35559 100644
> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
> @@ -1,11 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * BMI160 - Bosch IMU, SPI bits
>   *
>   * Copyright (c) 2016, Intel Corporation.
>   *
> - * This file is subject to the terms and conditions of version 2 of
> - * the GNU General Public License.  See the file COPYING in the main
> - * directory of this archive for more details.
>   */
>  #include <linux/acpi.h>
>  #include <linux/module.h>

[PATCH 2/5] iio:bmi160: add drdy interrupt support

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

Add interrupt support for the data ready signal on the BMI160, which fires
an interrupt whenever new accelerometer/gyroscope data is ready to read.

Signed-off-by: Martin Kelly <martin@martingkelly.com>
---
 drivers/iio/imu/bmi160/Makefile         |   1 +
 drivers/iio/imu/bmi160/bmi160.h         |  13 +-
 drivers/iio/imu/bmi160/bmi160_core.c    | 271 +++++++++++++++++++++++++++++++-
 drivers/iio/imu/bmi160/bmi160_i2c.c     |   3 +-
 drivers/iio/imu/bmi160/bmi160_spi.c     |   2 +-
 drivers/iio/imu/bmi160/bmi160_trigger.c |  60 +++++++
 6 files changed, 341 insertions(+), 9 deletions(-)
 create mode 100644 drivers/iio/imu/bmi160/bmi160_trigger.c

diff --git a/drivers/iio/imu/bmi160/Makefile b/drivers/iio/imu/bmi160/Makefile
index 10365e493ae2..19e1720c6fe9 100644
--- a/drivers/iio/imu/bmi160/Makefile
+++ b/drivers/iio/imu/bmi160/Makefile
@@ -2,5 +2,6 @@
 # Makefile for Bosch BMI160 IMU
 #
 obj-$(CONFIG_BMI160) += bmi160_core.o
+obj-$(CONFIG_BMI160) += bmi160_trigger.o
 obj-$(CONFIG_BMI160_I2C) += bmi160_i2c.o
 obj-$(CONFIG_BMI160_SPI) += bmi160_spi.o
diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
index 2351049d930b..0c5e67e0d35b 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -2,9 +2,20 @@
 #ifndef BMI160_H_
 #define BMI160_H_
 
+#include <linux/iio/iio.h>
+
+struct bmi160_data {
+	struct regmap *regmap;
+	struct iio_trigger *trig;
+};
+
 extern const struct regmap_config bmi160_regmap_config;
 
 int bmi160_core_probe(struct device *dev, struct regmap *regmap,
-		      const char *name, bool use_spi);
+		      const char *name, bool use_spi, int irq);
+
+int bmi160_enable_irq(struct regmap *regmap, bool enable);
+
+int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type);
 
 #endif  /* BMI160_H_ */
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index ce61026d84c3..e119965e64a3 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -6,12 +6,14 @@
  *
  * IIO core driver for BMI160, with support for I2C/SPI busses
  *
- * TODO: magnetometer, interrupts, hardware FIFO
+ * TODO: magnetometer, hardware FIFO
  */
 #include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/acpi.h>
 #include <linux/delay.h>
+#include <linux/irq.h>
+#include <linux/of_irq.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/triggered_buffer.h>
@@ -61,8 +63,32 @@
 #define BMI160_CMD_GYRO_PM_FAST_STARTUP	0x17
 #define BMI160_CMD_SOFTRESET		0xB6
 
+#define BMI160_REG_INT_EN		0x51
+#define BMI160_DRDY_INT_EN		BIT(4)
+
+#define BMI160_REG_INT_OUT_CTRL		0x53
+#define BMI160_INT_OUT_CTRL_MASK	0x0f
+#define BMI160_INT1_OUT_CTRL_SHIFT	0
+#define BMI160_INT2_OUT_CTRL_SHIFT	4
+#define BMI160_LEVEL_TRIGGERED		BIT(0)
+#define BMI160_ACTIVE_HIGH		BIT(1)
+#define BMI160_OPEN_DRAIN		BIT(2)
+#define BMI160_OUTPUT_EN		BIT(3)
+
+#define BMI160_REG_INT_LATCH		0x54
+#define BMI160_INT1_LATCH_MASK		BIT(4)
+#define BMI160_INT2_LATCH_MASK		BIT(5)
+
+/* INT1 and INT2 are in the opposite order as in INT_OUT_CTRL! */
+#define BMI160_REG_INT_MAP		0x56
+#define BMI160_INT1_MAP_DRDY_EN		0x80
+#define BMI160_INT2_MAP_DRDY_EN		0x08
+
 #define BMI160_REG_DUMMY		0x7F
 
+#define BMI160_NORMAL_WRITE_USLEEP	2
+#define BMI160_SUSPENDED_WRITE_USLEEP	450
+
 #define BMI160_ACCEL_PMU_MIN_USLEEP	3800
 #define BMI160_GYRO_PMU_MIN_USLEEP	80000
 #define BMI160_SOFTRESET_USLEEP		1000
@@ -105,8 +131,9 @@ enum bmi160_sensor_type {
 	BMI160_NUM_SENSORS /* must be last */
 };
 
-struct bmi160_data {
-	struct regmap *regmap;
+enum bmi160_int_pin {
+	BMI160_PIN_INT1,
+	BMI160_PIN_INT2
 };
 
 const struct regmap_config bmi160_regmap_config = {
@@ -495,7 +522,232 @@ static const char *bmi160_match_acpi_device(struct device *dev)
 	return dev_name(dev);
 }
 
-static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
+static int bmi160_write_conf_reg(struct regmap *regmap, unsigned int reg,
+				 unsigned int mask, unsigned int bits,
+				 unsigned int write_usleep)
+{
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(regmap, reg, &val);
+	if (ret < 0)
+		return ret;
+
+	val = (val & ~mask) | bits;
+
+	ret = regmap_write(regmap, reg, val);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * We need to wait after writing before we can write again. See the
+	 * datasheet, page 93.
+	 */
+	usleep_range(write_usleep, write_usleep + 1000);
+
+	return 0;
+}
+
+static int bmi160_config_pin(struct regmap *regmap, enum bmi160_int_pin pin,
+				bool open_drain, u8 irq_mask,
+				unsigned long write_usleep)
+{
+	int ret;
+	u8 int_out_ctrl_shift;
+	u8 int_latch_mask;
+	u8 int_map_mask;
+	u8 int_out_ctrl_mask;
+	u8 int_out_ctrl_bits;
+
+	switch (pin) {
+	case BMI160_PIN_INT1:
+		int_out_ctrl_shift = BMI160_INT1_OUT_CTRL_SHIFT;
+		int_latch_mask = BMI160_INT1_LATCH_MASK;
+		int_map_mask = BMI160_INT1_MAP_DRDY_EN;
+		break;
+	case BMI160_PIN_INT2:
+		int_out_ctrl_shift = BMI160_INT2_OUT_CTRL_SHIFT;
+		int_latch_mask = BMI160_INT2_LATCH_MASK;
+		int_map_mask = BMI160_INT2_MAP_DRDY_EN;
+		break;
+	}
+	int_out_ctrl_mask = BMI160_INT_OUT_CTRL_MASK << int_out_ctrl_shift;
+
+	/*
+	 * Enable the requested pin with the right settings:
+	 * - Push-pull/open-drain
+	 * - Active low/high
+	 * - Edge/level triggered
+	 */
+	int_out_ctrl_bits = 0;
+	int_out_ctrl_bits |= BMI160_OUTPUT_EN;
+	if (open_drain)
+		/* Default is push-pull. */
+		int_out_ctrl_bits |= BMI160_OPEN_DRAIN;
+	int_out_ctrl_bits |= irq_mask;
+	int_out_ctrl_bits <<= int_out_ctrl_shift;
+
+	ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_OUT_CTRL,
+				    int_out_ctrl_mask, int_out_ctrl_bits,
+				    write_usleep);
+	if (ret < 0)
+		return ret;
+
+	/* Set the pin to input mode with no latching. */
+	ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_LATCH,
+				    int_latch_mask, int_latch_mask,
+				    write_usleep);
+	if (ret < 0)
+		return ret;
+
+	/* Map interrupts to the requested pin. */
+	ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_MAP,
+				    int_map_mask, int_map_mask,
+				    write_usleep);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+int bmi160_enable_irq(struct regmap *regmap, bool enable)
+{
+	int ret;
+	unsigned int enable_bit;
+
+	if (enable)
+		enable_bit = BMI160_DRDY_INT_EN;
+	else
+		enable_bit = 0;
+
+	ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_EN,
+				    BMI160_DRDY_INT_EN, enable_bit,
+				    BMI160_NORMAL_WRITE_USLEEP);
+
+	return ret;
+}
+EXPORT_SYMBOL(bmi160_enable_irq);
+
+static void bmi160_parse_irqname(struct device_node *of_node, int irq,
+				 bool *int1_enabled, bool *int2_enabled)
+{
+	int ret;
+
+	/* of_irq_get_byname returns the IRQ number if the entry is found. */
+	ret = of_irq_get_byname(of_node, "INT1");
+	if (ret == irq) {
+		*int1_enabled = true;
+		*int2_enabled = false;
+		return;
+	}
+
+	ret = of_irq_get_byname(of_node, "INT2");
+	if (ret == irq) {
+		*int1_enabled = false;
+		*int2_enabled = true;
+		return;
+	}
+
+	ret = of_irq_get_byname(of_node, "BOTH");
+	if (ret == irq) {
+		*int1_enabled = true;
+		*int2_enabled = true;
+		return;
+	}
+
+	/* No interrupt name entries found. */
+	*int1_enabled = false;
+	*int2_enabled = false;
+}
+
+static int bmi160_config_device_irq(struct iio_dev *indio_dev,
+				    int irq, int irq_type)
+{
+	int ret;
+	bool int1_enabled;
+	bool int1_open_drain;
+	bool int2_enabled;
+	bool int2_open_drain;
+	u8 irq_mask;
+	struct bmi160_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
+
+	/* Edge-triggered, active-low is the default if we set all zeroes. */
+	if (irq_type == IRQF_TRIGGER_RISING)
+		irq_mask = BMI160_ACTIVE_HIGH | BMI160_LEVEL_TRIGGERED;
+	else if (irq_type == IRQF_TRIGGER_FALLING)
+		irq_mask = BMI160_LEVEL_TRIGGERED;
+	else if (irq_type == IRQF_TRIGGER_HIGH)
+		irq_mask = BMI160_ACTIVE_HIGH;
+	else if (irq_type == IRQF_TRIGGER_LOW)
+		irq_mask = 0;
+	else {
+		dev_err(&indio_dev->dev,
+			"Invalid interrupt type 0x%x specified\n", irq_type);
+		return -EINVAL;
+	}
+
+	bmi160_parse_irqname(dev->of_node, irq, &int1_enabled, &int2_enabled);
+
+	if (!int1_enabled && !int2_enabled) {
+		dev_err(&indio_dev->dev, "IRQ specified but not routed anywhere. bmi160,int1-enabled or bmi160,int2-enabled must be set.");
+		return -EINVAL;
+	}
+
+	int1_open_drain = of_property_read_bool(dev->of_node,
+						"bmi160,int1-open-drain");
+	int2_open_drain = of_property_read_bool(dev->of_node,
+						"bmi160,int2-open-drain");
+
+	if (int1_enabled) {
+		ret = bmi160_config_pin(data->regmap, BMI160_PIN_INT1,
+					int1_open_drain, irq_mask,
+					BMI160_NORMAL_WRITE_USLEEP);
+		if (ret < 0) {
+			dev_err(&indio_dev->dev, "Failed to configure INT1 IRQ pin");
+			return ret;
+		}
+	}
+
+	if (int2_enabled) {
+		ret = bmi160_config_pin(data->regmap, BMI160_PIN_INT2,
+					int2_open_drain, irq_mask,
+					BMI160_NORMAL_WRITE_USLEEP);
+		if (ret < 0) {
+			dev_err(&indio_dev->dev, "Failed to configure INT2 IRQ pin");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int bmi160_setup_irq(struct iio_dev *indio_dev, int irq)
+{
+	struct irq_data *desc;
+	u32 irq_type;
+	int ret;
+
+	desc = irq_get_irq_data(irq);
+	if (!desc) {
+		dev_warn(&indio_dev->dev, "Could not find IRQ %d\n", irq);
+		return -EINVAL;
+	}
+
+	irq_type = irqd_get_trigger_type(desc);
+
+	ret = bmi160_config_device_irq(indio_dev, irq, irq_type);
+	if (ret < 0)
+		return ret;
+
+	ret = bmi160_probe_trigger(indio_dev, irq, irq_type);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
 {
 	int ret;
 	unsigned int val;
@@ -548,7 +800,7 @@ static void bmi160_chip_uninit(void *data)
 }
 
 int bmi160_core_probe(struct device *dev, struct regmap *regmap,
-		      const char *name, bool use_spi)
+		      const char *name, bool use_spi, int irq)
 {
 	struct iio_dev *indio_dev;
 	struct bmi160_data *data;
@@ -561,8 +813,9 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
 	data = iio_priv(indio_dev);
 	dev_set_drvdata(dev, indio_dev);
 	data->regmap = regmap;
+	data->trig = NULL;
 
-	ret = bmi160_chip_init(data, use_spi);
+	ret = bmi160_chip_init(data, use_spi, irq);
 	if (ret < 0)
 		return ret;
 
@@ -585,6 +838,12 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret < 0)
 		return ret;
 
+	if (irq) {
+		ret = bmi160_setup_irq(indio_dev, irq);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
index e36f5e82d400..98467d73c887 100644
--- a/drivers/iio/imu/bmi160/bmi160_i2c.c
+++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
@@ -32,7 +32,8 @@ static int bmi160_i2c_probe(struct i2c_client *client,
 	if (id)
 		name = id->name;
 
-	return bmi160_core_probe(&client->dev, regmap, name, false);
+	return bmi160_core_probe(&client->dev, regmap,
+				 name, false, client->irq);
 }
 
 static const struct i2c_device_id bmi160_i2c_id[] = {
diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
index c19e3df35559..23e323518873 100644
--- a/drivers/iio/imu/bmi160/bmi160_spi.c
+++ b/drivers/iio/imu/bmi160/bmi160_spi.c
@@ -24,7 +24,7 @@ static int bmi160_spi_probe(struct spi_device *spi)
 			(int)PTR_ERR(regmap));
 		return PTR_ERR(regmap);
 	}
-	return bmi160_core_probe(&spi->dev, regmap, id->name, true);
+	return bmi160_core_probe(&spi->dev, regmap, id->name, true, spi->irq);
 }
 
 static const struct spi_device_id bmi160_spi_id[] = {
diff --git a/drivers/iio/imu/bmi160/bmi160_trigger.c b/drivers/iio/imu/bmi160/bmi160_trigger.c
new file mode 100644
index 000000000000..96e254eb2f19
--- /dev/null
+++ b/drivers/iio/imu/bmi160/bmi160_trigger.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * BMI160 - Bosch IMU, trigger/IRQ bits
+ *
+ * Copyright (c) 2019, Martin Kelly.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+
+#include "bmi160.h"
+
+static int bmi160_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					     bool enable)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct bmi160_data *data = iio_priv(indio_dev);
+
+	return bmi160_enable_irq(data->regmap, enable);
+}
+
+static const struct iio_trigger_ops bmi160_trigger_ops = {
+	.set_trigger_state = &bmi160_data_rdy_trigger_set_state,
+};
+
+int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
+{
+	struct bmi160_data *data = iio_priv(indio_dev);
+	int ret;
+
+	data->trig = devm_iio_trigger_alloc(&indio_dev->dev, "%s-dev%d",
+					    indio_dev->name, indio_dev->id);
+
+	if (data->trig == NULL)
+		return -ENOMEM;
+
+	ret = devm_request_irq(&indio_dev->dev, irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       irq_type, "bmi160", data->trig);
+	if (ret < 0)
+		return ret;
+
+	data->trig->dev.parent = regmap_get_device(data->regmap);
+	data->trig->ops = &bmi160_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, indio_dev);
+
+	ret = devm_iio_trigger_register(&indio_dev->dev, data->trig);
+	if (ret < 0)
+		return ret;
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bmi160_probe_trigger);
-- 
2.11.0

Re: [PATCH 2/5] iio:bmi160: add drdy interrupt support

[Jonathan Cameron]

On Sun, 13 Jan 2019 19:56:18 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> From: Martin Kelly <martin@martingkelly.com>
> 
> Add interrupt support for the data ready signal on the BMI160, which fires
> an interrupt whenever new accelerometer/gyroscope data is ready to read.

Various minor comments inline.  Mostly fine, but one big 'oddity'.

I think we only ever request 1 irq, but we have support in here for
configuring the chip to generate two at the same time.  Doesn't make
much sense as it stands... Am I missing something?

Jonathan

> 
> Signed-off-by: Martin Kelly <martin@martingkelly.com>
> ---
>  drivers/iio/imu/bmi160/Makefile         |   1 +
>  drivers/iio/imu/bmi160/bmi160.h         |  13 +-
>  drivers/iio/imu/bmi160/bmi160_core.c    | 271 +++++++++++++++++++++++++++++++-
>  drivers/iio/imu/bmi160/bmi160_i2c.c     |   3 +-
>  drivers/iio/imu/bmi160/bmi160_spi.c     |   2 +-
>  drivers/iio/imu/bmi160/bmi160_trigger.c |  60 +++++++
>  6 files changed, 341 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/iio/imu/bmi160/bmi160_trigger.c
> 
> diff --git a/drivers/iio/imu/bmi160/Makefile b/drivers/iio/imu/bmi160/Makefile
> index 10365e493ae2..19e1720c6fe9 100644
> --- a/drivers/iio/imu/bmi160/Makefile
> +++ b/drivers/iio/imu/bmi160/Makefile
> @@ -2,5 +2,6 @@
>  # Makefile for Bosch BMI160 IMU
>  #
>  obj-$(CONFIG_BMI160) += bmi160_core.o
> +obj-$(CONFIG_BMI160) += bmi160_trigger.o
>  obj-$(CONFIG_BMI160_I2C) += bmi160_i2c.o
>  obj-$(CONFIG_BMI160_SPI) += bmi160_spi.o
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index 2351049d930b..0c5e67e0d35b 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -2,9 +2,20 @@
>  #ifndef BMI160_H_
>  #define BMI160_H_
>  
> +#include <linux/iio/iio.h>
> +
> +struct bmi160_data {
> +	struct regmap *regmap;
> +	struct iio_trigger *trig;
> +};
> +
>  extern const struct regmap_config bmi160_regmap_config;
>  
>  int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> -		      const char *name, bool use_spi);
> +		      const char *name, bool use_spi, int irq);
> +
> +int bmi160_enable_irq(struct regmap *regmap, bool enable);
> +
> +int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type);
>  
>  #endif  /* BMI160_H_ */
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index ce61026d84c3..e119965e64a3 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -6,12 +6,14 @@
>   *
>   * IIO core driver for BMI160, with support for I2C/SPI busses
>   *
> - * TODO: magnetometer, interrupts, hardware FIFO
> + * TODO: magnetometer, hardware FIFO
>   */
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  #include <linux/acpi.h>
>  #include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <linux/of_irq.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/triggered_buffer.h>
> @@ -61,8 +63,32 @@
>  #define BMI160_CMD_GYRO_PM_FAST_STARTUP	0x17
>  #define BMI160_CMD_SOFTRESET		0xB6
>  
> +#define BMI160_REG_INT_EN		0x51
> +#define BMI160_DRDY_INT_EN		BIT(4)
> +
> +#define BMI160_REG_INT_OUT_CTRL		0x53
> +#define BMI160_INT_OUT_CTRL_MASK	0x0f
> +#define BMI160_INT1_OUT_CTRL_SHIFT	0
> +#define BMI160_INT2_OUT_CTRL_SHIFT	4
> +#define BMI160_LEVEL_TRIGGERED		BIT(0)
> +#define BMI160_ACTIVE_HIGH		BIT(1)
> +#define BMI160_OPEN_DRAIN		BIT(2)
> +#define BMI160_OUTPUT_EN		BIT(3)
> +
> +#define BMI160_REG_INT_LATCH		0x54
> +#define BMI160_INT1_LATCH_MASK		BIT(4)
> +#define BMI160_INT2_LATCH_MASK		BIT(5)
> +
> +/* INT1 and INT2 are in the opposite order as in INT_OUT_CTRL! */
> +#define BMI160_REG_INT_MAP		0x56
> +#define BMI160_INT1_MAP_DRDY_EN		0x80
> +#define BMI160_INT2_MAP_DRDY_EN		0x08
> +
>  #define BMI160_REG_DUMMY		0x7F
>  
> +#define BMI160_NORMAL_WRITE_USLEEP	2
> +#define BMI160_SUSPENDED_WRITE_USLEEP	450
> +
>  #define BMI160_ACCEL_PMU_MIN_USLEEP	3800
>  #define BMI160_GYRO_PMU_MIN_USLEEP	80000
>  #define BMI160_SOFTRESET_USLEEP		1000
> @@ -105,8 +131,9 @@ enum bmi160_sensor_type {
>  	BMI160_NUM_SENSORS /* must be last */
>  };
>  
> -struct bmi160_data {
> -	struct regmap *regmap;
> +enum bmi160_int_pin {
> +	BMI160_PIN_INT1,
> +	BMI160_PIN_INT2
>  };
>  
>  const struct regmap_config bmi160_regmap_config = {
> @@ -495,7 +522,232 @@ static const char *bmi160_match_acpi_device(struct device *dev)
>  	return dev_name(dev);
>  }
>  
> -static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
> +static int bmi160_write_conf_reg(struct regmap *regmap, unsigned int reg,
> +				 unsigned int mask, unsigned int bits,
> +				 unsigned int write_usleep)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(regmap, reg, &val);
> +	if (ret < 0)
> +		return ret;
regmap is 0 on success, so
if (ret) preferred.

Makes the question on whether this can return a positive number
that I raise below more obvious.
> +
> +	val = (val & ~mask) | bits;
> +
> +	ret = regmap_write(regmap, reg, val);
> +	if (ret < 0)
if (ret)
> +		return ret;
> +
> +	/*
> +	 * We need to wait after writing before we can write again. See the
> +	 * datasheet, page 93.
> +	 */
> +	usleep_range(write_usleep, write_usleep + 1000);
> +
> +	return 0;
> +}
> +
> +static int bmi160_config_pin(struct regmap *regmap, enum bmi160_int_pin pin,
> +				bool open_drain, u8 irq_mask,
> +				unsigned long write_usleep)
> +{
> +	int ret;
> +	u8 int_out_ctrl_shift;
> +	u8 int_latch_mask;
> +	u8 int_map_mask;
> +	u8 int_out_ctrl_mask;
> +	u8 int_out_ctrl_bits;
> +
> +	switch (pin) {
> +	case BMI160_PIN_INT1:
> +		int_out_ctrl_shift = BMI160_INT1_OUT_CTRL_SHIFT;
> +		int_latch_mask = BMI160_INT1_LATCH_MASK;
> +		int_map_mask = BMI160_INT1_MAP_DRDY_EN;
> +		break;
> +	case BMI160_PIN_INT2:
> +		int_out_ctrl_shift = BMI160_INT2_OUT_CTRL_SHIFT;
> +		int_latch_mask = BMI160_INT2_LATCH_MASK;
> +		int_map_mask = BMI160_INT2_MAP_DRDY_EN;
> +		break;
> +	}
> +	int_out_ctrl_mask = BMI160_INT_OUT_CTRL_MASK << int_out_ctrl_shift;
> +
> +	/*
> +	 * Enable the requested pin with the right settings:
> +	 * - Push-pull/open-drain
> +	 * - Active low/high
> +	 * - Edge/level triggered
> +	 */
> +	int_out_ctrl_bits = 0;
> +	int_out_ctrl_bits |= BMI160_OUTPUT_EN;

Combine the two lines above.

> +	if (open_drain)
> +		/* Default is push-pull. */
> +		int_out_ctrl_bits |= BMI160_OPEN_DRAIN;
> +	int_out_ctrl_bits |= irq_mask;
> +	int_out_ctrl_bits <<= int_out_ctrl_shift;
> +
> +	ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_OUT_CTRL,
> +				    int_out_ctrl_mask, int_out_ctrl_bits,
> +				    write_usleep);
> +	if (ret < 0)
if (ret) See both above an below!
> +		return ret;
> +
> +	/* Set the pin to input mode with no latching. */
> +	ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_LATCH,
> +				    int_latch_mask, int_latch_mask,
> +				    write_usleep);
> +	if (ret < 0)
> +		return ret;
if (ret)
> +
> +	/* Map interrupts to the requested pin. */
> +	ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_MAP,
> +				    int_map_mask, int_map_mask,
> +				    write_usleep);
> +	if (ret < 0)
> +		return ret;
Can ret be greater than 0?  I checked, nope.
return bmi160_write_conf_reg.  (note btw that I review backwards
as code makes more sense that way to me!)
> +
> +	return 0;
> +}
> +
> +int bmi160_enable_irq(struct regmap *regmap, bool enable)
> +{
> +	int ret;
> +	unsigned int enable_bit;
= 0;

> +
> +	if (enable)
> +		enable_bit = BMI160_DRDY_INT_EN;
> +	else
> +		enable_bit = 0;
Then drop the else.
> +
> +	ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_EN,
> +				    BMI160_DRDY_INT_EN, enable_bit,
> +				    BMI160_NORMAL_WRITE_USLEEP);
> +

return bmi160...

> +	return ret;
> +}
> +EXPORT_SYMBOL(bmi160_enable_irq);
> +
> +static void bmi160_parse_irqname(struct device_node *of_node, int irq,
> +				 bool *int1_enabled, bool *int2_enabled)
> +{
> +	int ret;
> +
> +	/* of_irq_get_byname returns the IRQ number if the entry is found. */
> +	ret = of_irq_get_byname(of_node, "INT1");
> +	if (ret == irq) {
> +		*int1_enabled = true;
> +		*int2_enabled = false;
> +		return;
> +	}
> +
> +	ret = of_irq_get_byname(of_node, "INT2");
> +	if (ret == irq) {
> +		*int1_enabled = false;
> +		*int2_enabled = true;
> +		return;
> +	}
> +
> +	ret = of_irq_get_byname(of_node, "BOTH");
?  I'm not following this.  Hopefully the DT docs will make it clear.

> +	if (ret == irq) {
> +		*int1_enabled = true;
> +		*int2_enabled = true;
> +		return;
> +	}
> +
> +	/* No interrupt name entries found. */
> +	*int1_enabled = false;
> +	*int2_enabled = false;
> +}
> +
> +static int bmi160_config_device_irq(struct iio_dev *indio_dev,
> +				    int irq, int irq_type)
> +{
> +	int ret;
> +	bool int1_enabled;
> +	bool int1_open_drain;
> +	bool int2_enabled;
> +	bool int2_open_drain;
> +	u8 irq_mask;
> +	struct bmi160_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
> +
> +	/* Edge-triggered, active-low is the default if we set all zeroes. */
> +	if (irq_type == IRQF_TRIGGER_RISING)
> +		irq_mask = BMI160_ACTIVE_HIGH | BMI160_LEVEL_TRIGGERED;
> +	else if (irq_type == IRQF_TRIGGER_FALLING)
> +		irq_mask = BMI160_LEVEL_TRIGGERED;
> +	else if (irq_type == IRQF_TRIGGER_HIGH)
> +		irq_mask = BMI160_ACTIVE_HIGH;
> +	else if (irq_type == IRQF_TRIGGER_LOW)
> +		irq_mask = 0;
> +	else {
> +		dev_err(&indio_dev->dev,
> +			"Invalid interrupt type 0x%x specified\n", irq_type);
> +		return -EINVAL;
> +	}
> +
> +	bmi160_parse_irqname(dev->of_node, irq, &int1_enabled, &int2_enabled);
> +
> +	if (!int1_enabled && !int2_enabled) {
> +		dev_err(&indio_dev->dev, "IRQ specified but not routed anywhere. bmi160,int1-enabled or bmi160,int2-enabled must be set.");
> +		return -EINVAL;
> +	}
> +
> +	int1_open_drain = of_property_read_bool(dev->of_node,
> +						"bmi160,int1-open-drain");
> +	int2_open_drain = of_property_read_bool(dev->of_node,
> +						"bmi160,int2-open-drain");
> +

This is fine for this section, but a little confusing as we don't actually
register for two interrupts.

> +	if (int1_enabled) {
> +		ret = bmi160_config_pin(data->regmap, BMI160_PIN_INT1,
> +					int1_open_drain, irq_mask,
> +					BMI160_NORMAL_WRITE_USLEEP);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Failed to configure INT1 IRQ pin");
> +			return ret;
> +		}
> +	}
> +
> +	if (int2_enabled) {
> +		ret = bmi160_config_pin(data->regmap, BMI160_PIN_INT2,
> +					int2_open_drain, irq_mask,
> +					BMI160_NORMAL_WRITE_USLEEP);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Failed to configure INT2 IRQ pin");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int bmi160_setup_irq(struct iio_dev *indio_dev, int irq)
> +{
> +	struct irq_data *desc;
> +	u32 irq_type;
> +	int ret;
> +
> +	desc = irq_get_irq_data(irq);
> +	if (!desc) {
> +		dev_warn(&indio_dev->dev, "Could not find IRQ %d\n", irq);
> +		return -EINVAL;
> +	}
> +
> +	irq_type = irqd_get_trigger_type(desc);
> +
> +	ret = bmi160_config_device_irq(indio_dev, irq, irq_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = bmi160_probe_trigger(indio_dev, irq, irq_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
>  {
>  	int ret;
>  	unsigned int val;
> @@ -548,7 +800,7 @@ static void bmi160_chip_uninit(void *data)
>  }
>  
>  int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> -		      const char *name, bool use_spi)
> +		      const char *name, bool use_spi, int irq)
>  {
>  	struct iio_dev *indio_dev;
>  	struct bmi160_data *data;
> @@ -561,8 +813,9 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>  	data = iio_priv(indio_dev);
>  	dev_set_drvdata(dev, indio_dev);
>  	data->regmap = regmap;
> +	data->trig = NULL;

Given that data is allocated as part of the iio_device_alloc, its already
set to null.  No need to do so again as it's a fairly obvious default
for a pointer (hence no 'documentation' benefit here).

>  
> -	ret = bmi160_chip_init(data, use_spi);
> +	ret = bmi160_chip_init(data, use_spi, irq);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -585,6 +838,12 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (irq) {
> +		ret = bmi160_setup_irq(indio_dev, irq);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
> index e36f5e82d400..98467d73c887 100644
> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
> @@ -32,7 +32,8 @@ static int bmi160_i2c_probe(struct i2c_client *client,
>  	if (id)
>  		name = id->name;
>  
> -	return bmi160_core_probe(&client->dev, regmap, name, false);
> +	return bmi160_core_probe(&client->dev, regmap,
> +				 name, false, client->irq);
>  }
>  
>  static const struct i2c_device_id bmi160_i2c_id[] = {
> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
> index c19e3df35559..23e323518873 100644
> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
> @@ -24,7 +24,7 @@ static int bmi160_spi_probe(struct spi_device *spi)
>  			(int)PTR_ERR(regmap));
>  		return PTR_ERR(regmap);
>  	}
> -	return bmi160_core_probe(&spi->dev, regmap, id->name, true);
> +	return bmi160_core_probe(&spi->dev, regmap, id->name, true, spi->irq);
>  }
>  
>  static const struct spi_device_id bmi160_spi_id[] = {
> diff --git a/drivers/iio/imu/bmi160/bmi160_trigger.c b/drivers/iio/imu/bmi160/bmi160_trigger.c
> new file mode 100644
> index 000000000000..96e254eb2f19
> --- /dev/null
> +++ b/drivers/iio/imu/bmi160/bmi160_trigger.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * BMI160 - Bosch IMU, trigger/IRQ bits
> + *
> + * Copyright (c) 2019, Martin Kelly.
> + */

It's nice to break up code into relevant files etc, but this one
is rather short.  Perhaps just put it in the core file?
Feel free to add a second copyright notice to that file whilst
doing so to reflect this non-trivial addition.

> +
> +#include <linux/interrupt.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +
> +#include "bmi160.h"
> +
> +static int bmi160_data_rdy_trigger_set_state(struct iio_trigger *trig,
> +					     bool enable)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct bmi160_data *data = iio_priv(indio_dev);
> +
> +	return bmi160_enable_irq(data->regmap, enable);
> +}
> +
> +static const struct iio_trigger_ops bmi160_trigger_ops = {
> +	.set_trigger_state = &bmi160_data_rdy_trigger_set_state,
> +};
> +
> +int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
> +{
> +	struct bmi160_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	data->trig = devm_iio_trigger_alloc(&indio_dev->dev, "%s-dev%d",
> +					    indio_dev->name, indio_dev->id);
> +
> +	if (data->trig == NULL)
> +		return -ENOMEM;
> +
> +	ret = devm_request_irq(&indio_dev->dev, irq,
> +			       &iio_trigger_generic_data_rdy_poll,
> +			       irq_type, "bmi160", data->trig);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->trig->dev.parent = regmap_get_device(data->regmap);
> +	data->trig->ops = &bmi160_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> +	ret = devm_iio_trigger_register(&indio_dev->dev, data->trig);
> +	if (ret < 0)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(data->trig);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(bmi160_probe_trigger);

Re: [PATCH 2/5] iio:bmi160: add drdy interrupt support

[Martin Kelly]

On 1/19/19 9:17 AM, Jonathan Cameron wrote:
> On Sun, 13 Jan 2019 19:56:18 -0800
> Martin Kelly <martin@martingkelly.com> wrote:
> 
>> From: Martin Kelly <martin@martingkelly.com>
>>
>> Add interrupt support for the data ready signal on the BMI160, which fires
>> an interrupt whenever new accelerometer/gyroscope data is ready to read.
> 
> Various minor comments inline.  Mostly fine, but one big 'oddity'.
> 
> I think we only ever request 1 irq, but we have support in here for
> configuring the chip to generate two at the same time.  Doesn't make
> much sense as it stands... Am I missing something?
> 
> Jonathan
> 

Yeah, I initially wrote it not allowing for a "BOTH" mapping but added 
it afterwards in order to support what the hardware allows. However, 
thinking about it more, the hardware allows this so that you can map 
different types of interrupts onto the two pins. So, I agree with you 
that with just a DRDY interrupt, there's no reason for this mapping, so 
I've dropped it.

Thanks for the other comments; I have made all the changes you suggested 
and sent a v2. I also added a cleanup patch in the v2 to change the 
other invocations of "if (ret < 0)" to "if (ret)", as there were a 
number of them in the driver prior to this change.

[PATCH 3/5] dt-bindings: fix incorrect bmi160 IRQ note

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

The bmi160 bindings say that the BMI160 requires level-triggered,
active-low interrupts, but it actually supports all interrupt types, so fix
the note to reflect that.

Signed-off-by: Martin Kelly <martin@martingkelly.com>
---
 Documentation/devicetree/bindings/iio/imu/bmi160.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
index 0c1c105fb503..1aec19997fb5 100644
--- a/Documentation/devicetree/bindings/iio/imu/bmi160.txt
+++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
@@ -9,7 +9,7 @@ Required properties:
  - spi-max-frequency : set maximum clock frequency (only for SPI)
 
 Optional properties:
- - interrupts : interrupt mapping for IRQ, must be IRQ_TYPE_LEVEL_LOW
+ - interrupts : interrupt mapping for IRQ
  - interrupt-names : set to "INT1" if INT1 pin should be used as interrupt
    input, set to "INT2" if INT2 pin should be used instead
 
@@ -20,7 +20,7 @@ bmi160@68 {
 	reg = <0x68>;
 
 	interrupt-parent = <&gpio4>;
-	interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+	interrupts = <12 IRQ_TYPE_EDGE_RISING>;
 	interrupt-names = "INT1";
 };
 
-- 
2.11.0

[PATCH 4/5] dt-bindings:bmi160: add "BOTH" int pin type

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

The bindings currently allow for interrupts routing to INT1 or INT2, but
the hardware allows for routing to both, so add "BOTH" as an int pin entry.

Signed-off-by: Martin Kelly <martin@martingkelly.com>
---
 Documentation/devicetree/bindings/iio/imu/bmi160.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
index 1aec19997fb5..a56d377dabf3 100644
--- a/Documentation/devicetree/bindings/iio/imu/bmi160.txt
+++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
@@ -11,7 +11,8 @@ Required properties:
 Optional properties:
  - interrupts : interrupt mapping for IRQ
  - interrupt-names : set to "INT1" if INT1 pin should be used as interrupt
-   input, set to "INT2" if INT2 pin should be used instead
+   input, set to "INT2" if INT2 pin should be used instead, set to "BOTH" if
+   both should be used.
 
 Examples:
 
@@ -33,3 +34,13 @@ bmi160@0 {
 	interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
 	interrupt-names = "INT2";
 };
+
+bmi160@0 {
+	compatible = "bosch,bmi160";
+	reg = <0>;
+	spi-max-frequency = <10000000>;
+
+	interrupt-parent = <&gpio2>;
+	interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
+	interrupt-names = "BOTH";
+};
-- 
2.11.0

Re: [PATCH 4/5] dt-bindings:bmi160: add "BOTH" int pin type

[Jonathan Cameron]

On Sun, 13 Jan 2019 19:56:20 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> From: Martin Kelly <martin@martingkelly.com>
> 
> The bindings currently allow for interrupts routing to INT1 or INT2, but
> the hardware allows for routing to both, so add "BOTH" as an int pin entry.
> 

DT binding changes should be sent to the dt list as well as Rob.

> Signed-off-by: Martin Kelly <martin@martingkelly.com>
> ---
>  Documentation/devicetree/bindings/iio/imu/bmi160.txt | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> index 1aec19997fb5..a56d377dabf3 100644
> --- a/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> @@ -11,7 +11,8 @@ Required properties:
>  Optional properties:
>   - interrupts : interrupt mapping for IRQ
>   - interrupt-names : set to "INT1" if INT1 pin should be used as interrupt
> -   input, set to "INT2" if INT2 pin should be used instead
> +   input, set to "INT2" if INT2 pin should be used instead, set to "BOTH" if
> +   both should be used.
>  
>  Examples:
>  
> @@ -33,3 +34,13 @@ bmi160@0 {
>  	interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>  	interrupt-names = "INT2";
>  };
> +
> +bmi160@0 {
> +	compatible = "bosch,bmi160";
> +	reg = <0>;
> +	spi-max-frequency = <10000000>;
> +
> +	interrupt-parent = <&gpio2>;
> +	interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
> +	interrupt-names = "BOTH";

Just have two entries, not BOTH.  Routing both internal interrupts to the
same pin on the host doesn't make any sense, so we should only see both
along with a pair of entries for the interrupts.

Jonathan
> +};

[PATCH 5/5] iio:bmi160:snap timestamp closer to event

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

Currently, we snap the timestamp after reading from the buffer and
processing the event. When the IIO poll function is triggered by an
interrupt,  we can get a slightly more accurate timestamp by snapping it
prior to reading the data, since the data was already generated prior to
entering the trigger handler. This is not going to make a huge difference,
but we might as well improve slightly.

Signed-off-by: Martin Kelly <martin@martingkelly.com>
---
 drivers/iio/imu/bmi160/bmi160_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index e119965e64a3..4b4456af0971 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -408,6 +408,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
+	s64 ts = iio_get_time_ns(indio_dev);
 	struct bmi160_data *data = iio_priv(indio_dev);
 	__le16 buf[16];
 	/* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */
@@ -423,8 +424,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
 		buf[j++] = sample;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
-					   iio_get_time_ns(indio_dev));
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
 done:
 	iio_trigger_notify_done(indio_dev->trig);
 	return IRQ_HANDLED;
-- 
2.11.0

Re: [PATCH 5/5] iio:bmi160:snap timestamp closer to event

[Jonathan Cameron]

On Sun, 13 Jan 2019 19:56:21 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> From: Martin Kelly <martin@martingkelly.com>
> 
> Currently, we snap the timestamp after reading from the buffer and
> processing the event. When the IIO poll function is triggered by an
> interrupt,  we can get a slightly more accurate timestamp by snapping it
> prior to reading the data, since the data was already generated prior to
> entering the trigger handler. This is not going to make a huge difference,
> but we might as well improve slightly.
> 
> Signed-off-by: Martin Kelly <martin@martingkelly.com>
Could do better by using the standard iio_pollfunc_store_time
to grab the timestamp in the interrupt handler itself rather than in
it's thread.

There is some argument that we should only do that if we know it's
'our' trigger. If we are triggered by someone else, we might report
a time before when the sample we then read was actually captured.

Still most of the time people will use the devices own trigger, or
be running slowly enough they won't care.

Anyhow, if we are going to argue this makes any sense use the
iio_pollfunc_store_time approach please.

As you will probably guess, this isn't the first device with a
dataready interrupt!
 :)

Jonathan

> ---
>  drivers/iio/imu/bmi160/bmi160_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index e119965e64a3..4b4456af0971 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -408,6 +408,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
> +	s64 ts = iio_get_time_ns(indio_dev);
>  	struct bmi160_data *data = iio_priv(indio_dev);
>  	__le16 buf[16];
>  	/* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */
> @@ -423,8 +424,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
>  		buf[j++] = sample;
>  	}
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> -					   iio_get_time_ns(indio_dev));
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
>  done:
>  	iio_trigger_notify_done(indio_dev->trig);
>  	return IRQ_HANDLED;

Re: [PATCH 0/5] iio:bmi160: add drdy interrupt support

[Martin Kelly]

On 1/13/19 7:56 PM, Martin Kelly wrote:
> From: Martin Kelly <martin@martingkelly.com>
> 
> This patch series adds drdy interrupt support to the BMI160 IMU. It also updates
> the relevant DT bindings and does a little bit of related cleanup.
> 
> Martin Kelly (5):
>    iio:bmi160: add SPDX identifiers
>    iio:bmi160: add drdy interrupt support
>    dt-bindings: fix incorrect bmi160 IRQ note
>    dt-bindings:bmi160: add "BOTH" int pin type
>    iio:bmi160:snap timestamp closer to event
> 
>   .../devicetree/bindings/iio/imu/bmi160.txt         |  17 +-
>   drivers/iio/imu/bmi160/Makefile                    |   1 +
>   drivers/iio/imu/bmi160/bmi160.h                    |  13 +-
>   drivers/iio/imu/bmi160/bmi160_core.c               | 280 ++++++++++++++++++++-
>   drivers/iio/imu/bmi160/bmi160_i2c.c                |   8 +-
>   drivers/iio/imu/bmi160/bmi160_spi.c                |   6 +-
>   drivers/iio/imu/bmi160/bmi160_trigger.c            |  60 +++++
>   7 files changed, 360 insertions(+), 25 deletions(-)
>   create mode 100644 drivers/iio/imu/bmi160/bmi160_trigger.c
> 
> --
> 2.11.0
> 

I realized this patch series does not contain updated binding 
documentation for the added bmi160,int1-open-drain and 
bmi160,int2-open-drain keys. I assume there will be other review 
feedback, so I will add a patch for this in a v2 series.