[PATCH v2 1/6] iio:bmi160: add SPDX identifiers

[PATCH v2 1/6] 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

[PATCH v2 2/6] 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>
---
v2:
- Drop "BOTH" interrupt setting.
- Change to "if (ret)" instead of "if (ret < 0)".
- Stylistic changes suggested by Jonathan Cameron.
- Fix bogus return check after iio_trigger_get.

 arch/arm/boot/dts/Makefile           |   1 +
 drivers/iio/imu/bmi160/bmi160.h      |  13 +-
 drivers/iio/imu/bmi160/bmi160_core.c | 294 ++++++++++++++++++++++++++++++++++-
 drivers/iio/imu/bmi160/bmi160_i2c.c  |   3 +-
 drivers/iio/imu/bmi160/bmi160_spi.c  |   2 +-
 5 files changed, 303 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index b0e966d625b9..df68910fc2c1 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -701,6 +701,7 @@ dtb-$(CONFIG_SOC_AM33XX) += \
 	am335x-base0033.dtb \
 	am335x-bone.dtb \
 	am335x-boneblack.dtb \
+	am335x-boneblack-bmi160-i2c1.dtb \
 	am335x-boneblack-wireless.dtb \
 	am335x-boneblue.dtb \
 	am335x-bonegreen.dtb \
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..c848fc1bce61 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -3,21 +3,25 @@
  * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
  *
  * Copyright (c) 2016, Intel Corporation.
+ * Copyright (c) 2019, Martin Kelly.
  *
  * 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>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>

 #include "bmi160.h"

@@ -61,8 +65,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 +133,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 +524,209 @@ 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)
+		return ret;
+
+	val = (val & ~mask) | bits;
+
+	ret = regmap_write(regmap, reg, val);
+	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 = 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)
+		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)
+		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)
+		return ret;
+
+	return 0;
+}
+
+int bmi160_enable_irq(struct regmap *regmap, bool enable)
+{
+	unsigned int enable_bit = 0;
+
+	if (enable)
+		enable_bit = BMI160_DRDY_INT_EN;
+
+	return bmi160_write_conf_reg(regmap, BMI160_REG_INT_EN,
+				     BMI160_DRDY_INT_EN, enable_bit,
+				     BMI160_NORMAL_WRITE_USLEEP);
+}
+EXPORT_SYMBOL(bmi160_enable_irq);
+
+static bool bmi160_parse_irqname(struct device_node *of_node, int irq,
+				 enum bmi160_int_pin *pin)
+{
+	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) {
+		*pin = BMI160_PIN_INT1;
+		return true;
+	}
+
+	ret = of_irq_get_byname(of_node, "INT2");
+	if (ret == irq) {
+		*pin = BMI160_PIN_INT2;
+		return true;
+	}
+
+	return false;
+}
+
+static int bmi160_config_device_irq(struct iio_dev *indio_dev,
+				    int irq, int irq_type)
+{
+	int ret;
+	bool success;
+	enum bmi160_int_pin int_pin;
+	bool open_drain;
+	const char *pin_name;
+	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;
+	}
+
+	success = bmi160_parse_irqname(dev->of_node, irq, &int_pin);
+	if (!success) {
+		dev_err(&indio_dev->dev,
+			"interrupt-names for IRQ %d must be set to either \"INT1\" or \"INT2\"",
+			irq);
+		return -EINVAL;
+	}
+
+	open_drain = of_property_read_bool(dev->of_node, "bmi160,open-drain");
+
+	ret = bmi160_config_pin(data->regmap, int_pin, open_drain, irq_mask,
+				BMI160_NORMAL_WRITE_USLEEP);
+	if (ret) {
+		switch (int_pin) {
+		case BMI160_PIN_INT1:
+			pin_name = "INT1";
+			break;
+		case BMI160_PIN_INT2:
+			pin_name = "INT2";
+			break;
+		}
+		dev_err(&indio_dev->dev, "Failed to configure %s IRQ pin",
+			pin_name);
+		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)
+		return ret;
+
+	ret = bmi160_probe_trigger(indio_dev, irq, irq_type);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
 {
 	int ret;
 	unsigned int val;
@@ -518,7 +749,7 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
 	}

 	ret = regmap_read(data->regmap, BMI160_REG_CHIP_ID, &val);
-	if (ret < 0) {
+	if (ret) {
 		dev_err(dev, "Error reading chip id\n");
 		return ret;
 	}
@@ -539,6 +770,49 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
 	return 0;
 }

+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)
+		return ret;
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}
+
 static void bmi160_chip_uninit(void *data)
 {
 	struct bmi160_data *bmi_data = data;
@@ -548,7 +822,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;
@@ -562,7 +836,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
 	dev_set_drvdata(dev, indio_dev);
 	data->regmap = regmap;

-	ret = bmi160_chip_init(data, use_spi);
+	ret = bmi160_chip_init(data, use_spi, irq);
 	if (ret < 0)
 		return ret;

@@ -585,6 +859,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)
+			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[] = {
--
2.11.0

[PATCH v2 3/6] 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 v2 4/6] dt-bindings: document open-drain property

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

We have added an optional boolean property for configuring a BMI160
interrupt pin as open-drain, as opposed to the default push-pull, so
document this.

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

diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
index 1aec19997fb5..7eb4b6016404 100644
--- a/Documentation/devicetree/bindings/iio/imu/bmi160.txt
+++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
@@ -12,6 +12,8 @@ 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
+- bmi160,open-drain : set if the specified interrupt pin should be configured as
+  open drain. If not set, defaults to push-pull.
 
 Examples:
 
-- 
2.11.0

[PATCH v2 5/6] iio:bmi160: use iio_pollfunc_store_time

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

Do this by using iio_pollfunc_store_time as other drivers do.

Signed-off-by: Martin Kelly <martin@martingkelly.com>
---
v2:
- Use iio_pollfunc_store_time.

 drivers/iio/imu/bmi160/bmi160_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index c848fc1bce61..dca53be066e1 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -425,8 +425,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, pf->timestamp);
 done:
 	iio_trigger_notify_done(indio_dev->trig);
 	return IRQ_HANDLED;
@@ -854,7 +853,8 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmi160_info;

-	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
 					      bmi160_trigger_handler, NULL);
 	if (ret < 0)
 		return ret;
--
2.11.0

[PATCH v2 6/6] iio:bmi160: use if (ret) instead of if (ret < 0)

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

We are using "if (ret < 0)" in many places in which the function returns 0
on success. Use "if (ret)" instead for better clarity and correctness.

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

diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index dca53be066e1..0ec9ded975e2 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -299,7 +299,7 @@ int bmi160_set_mode(struct bmi160_data *data, enum bmi160_sensor_type t,
 		cmd = bmi160_regs[t].pmu_cmd_suspend;
 
 	ret = regmap_write(data->regmap, BMI160_REG_CMD, cmd);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	usleep_range(bmi160_pmu_time[t], bmi160_pmu_time[t] + 1000);
@@ -331,7 +331,7 @@ int bmi160_get_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
 	int i, ret, val;
 
 	ret = regmap_read(data->regmap, bmi160_regs[t].range, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	for (i = 0; i < bmi160_scale_table[t].num; i++)
@@ -354,7 +354,7 @@ static int bmi160_get_data(struct bmi160_data *data, int chan_type,
 	reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(sample);
 
 	ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(sample));
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = sign_extend32(le16_to_cpu(sample), 15);
@@ -388,7 +388,7 @@ static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
 	int i, val, ret;
 
 	ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	val &= bmi160_regs[t].config_odr_mask;
@@ -420,7 +420,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
 			 indio_dev->masklength) {
 		ret = regmap_bulk_read(data->regmap, base + i * sizeof(sample),
 				       &sample, sizeof(sample));
-		if (ret < 0)
+		if (ret)
 			goto done;
 		buf[j++] = sample;
 	}
@@ -441,18 +441,18 @@ static int bmi160_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		ret = bmi160_get_data(data, chan->type, chan->channel2, val);
-		if (ret < 0)
+		if (ret)
 			return ret;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 0;
 		ret = bmi160_get_scale(data,
 				       bmi160_to_sensor(chan->type), val2);
-		return ret < 0 ? ret : IIO_VAL_INT_PLUS_MICRO;
+		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		ret = bmi160_get_odr(data, bmi160_to_sensor(chan->type),
 				     val, val2);
-		return ret < 0 ? ret : IIO_VAL_INT_PLUS_MICRO;
+		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
@@ -732,7 +732,7 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
 	struct device *dev = regmap_get_device(data->regmap);
 
 	ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	usleep_range(BMI160_SOFTRESET_USLEEP, BMI160_SOFTRESET_USLEEP + 1);
@@ -743,7 +743,7 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
 	 */
 	if (use_spi) {
 		ret = regmap_read(data->regmap, BMI160_REG_DUMMY, &val);
-		if (ret < 0)
+		if (ret)
 			return ret;
 	}
 
@@ -759,11 +759,11 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
 	}
 
 	ret = bmi160_set_mode(data, BMI160_ACCEL, true);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = bmi160_set_mode(data, BMI160_GYRO, true);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	return 0;
@@ -796,7 +796,7 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
 	ret = devm_request_irq(&indio_dev->dev, irq,
 			       &iio_trigger_generic_data_rdy_poll,
 			       irq_type, "bmi160", data->trig);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	data->trig->dev.parent = regmap_get_device(data->regmap);
@@ -836,11 +836,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
 	data->regmap = regmap;
 
 	ret = bmi160_chip_init(data, use_spi, irq);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = devm_add_action_or_reset(dev, bmi160_chip_uninit, data);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	if (!name && ACPI_HANDLE(dev))
@@ -856,7 +856,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
 					      iio_pollfunc_store_time,
 					      bmi160_trigger_handler, NULL);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	if (irq) {
@@ -866,7 +866,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
 	}
 
 	ret = devm_iio_device_register(dev, indio_dev);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	return 0;
-- 
2.11.0

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

[Jonathan Cameron]

On Mon, 21 Jan 2019 18:04:27 -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.
> 
> Signed-off-by: Martin Kelly <martin@martingkelly.com>

Various minor bits inline.  I would also, if possible like Daniel to
take a glance at this series before we apply it. I don't know this hardware
at all well!

Jonathan

> ---
> v2:
> - Drop "BOTH" interrupt setting.
> - Change to "if (ret)" instead of "if (ret < 0)".
> - Stylistic changes suggested by Jonathan Cameron.
> - Fix bogus return check after iio_trigger_get.
> 
>  arch/arm/boot/dts/Makefile           |   1 +
>  drivers/iio/imu/bmi160/bmi160.h      |  13 +-
>  drivers/iio/imu/bmi160/bmi160_core.c | 294 ++++++++++++++++++++++++++++++++++-
>  drivers/iio/imu/bmi160/bmi160_i2c.c  |   3 +-
>  drivers/iio/imu/bmi160/bmi160_spi.c  |   2 +-
>  5 files changed, 303 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index b0e966d625b9..df68910fc2c1 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -701,6 +701,7 @@ dtb-$(CONFIG_SOC_AM33XX) += \
>  	am335x-base0033.dtb \
>  	am335x-bone.dtb \
>  	am335x-boneblack.dtb \
> +	am335x-boneblack-bmi160-i2c1.dtb \

Separate patch for this.

>  	am335x-boneblack-wireless.dtb \
>  	am335x-boneblue.dtb \
>  	am335x-bonegreen.dtb \
> 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..c848fc1bce61 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -3,21 +3,25 @@
>   * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
>   *
>   * Copyright (c) 2016, Intel Corporation.
> + * Copyright (c) 2019, Martin Kelly.
>   *
>   * 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>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> 
>  #include "bmi160.h"
> 
> @@ -61,8 +65,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 +133,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 +524,209 @@ 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)
> +		return ret;
> +
> +	val = (val & ~mask) | bits;
> +
> +	ret = regmap_write(regmap, reg, val);
> +	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 = 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)
> +		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)
> +		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)
> +		return ret;
return bmi160...
> +
> +	return 0;
> +}
> +
> +int bmi160_enable_irq(struct regmap *regmap, bool enable)
> +{
> +	unsigned int enable_bit = 0;
> +
> +	if (enable)
> +		enable_bit = BMI160_DRDY_INT_EN;
> +
> +	return bmi160_write_conf_reg(regmap, BMI160_REG_INT_EN,
> +				     BMI160_DRDY_INT_EN, enable_bit,
> +				     BMI160_NORMAL_WRITE_USLEEP);
> +}
> +EXPORT_SYMBOL(bmi160_enable_irq);
> +
> +static bool bmi160_parse_irqname(struct device_node *of_node, int irq,
> +				 enum bmi160_int_pin *pin)
> +{
> +	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) {
> +		*pin = BMI160_PIN_INT1;
> +		return true;
> +	}

Given both could be provided, and we are implying a preference,
why not just get INT1 first by name and if it's not there try for
INT2?  No need for this separate does the name match query.

> +
> +	ret = of_irq_get_byname(of_node, "INT2");
> +	if (ret == irq) {
> +		*pin = BMI160_PIN_INT2;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int bmi160_config_device_irq(struct iio_dev *indio_dev,
> +				    int irq, int irq_type)
> +{
> +	int ret;
> +	bool success;
> +	enum bmi160_int_pin int_pin;
> +	bool open_drain;
> +	const char *pin_name;
> +	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;

That seems unlikely.  Why on a rising trigger are we setting a flag
that is named to imply it sets level triggering?

> +	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;
> +	}
> +
> +	success = bmi160_parse_irqname(dev->of_node, irq, &int_pin);
> +	if (!success) {
> +		dev_err(&indio_dev->dev,
> +			"interrupt-names for IRQ %d must be set to either \"INT1\" or \"INT2\"",
> +			irq);
> +		return -EINVAL;
> +	}
> +
> +	open_drain = of_property_read_bool(dev->of_node, "bmi160,open-drain");
> +
> +	ret = bmi160_config_pin(data->regmap, int_pin, open_drain, irq_mask,
> +				BMI160_NORMAL_WRITE_USLEEP);
> +	if (ret) {
> +		switch (int_pin) {
> +		case BMI160_PIN_INT1:
> +			pin_name = "INT1";
> +			break;
> +		case BMI160_PIN_INT2:
> +			pin_name = "INT2";
> +			break;
> +		}
> +		dev_err(&indio_dev->dev, "Failed to configure %s IRQ pin",
> +			pin_name);

Why not do that within bmi160_config_pin?  Then we have
return bmi160_config_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)
> +		return ret;
> +
> +	ret = bmi160_probe_trigger(indio_dev, irq, irq_type);
> +	if (ret)
> +		return ret;

return bmi160_probe_trigger...  It's either zero or it's not.
Either way this gives the same as you have here.

> +
> +	return 0;
> +}
> +
> +static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
>  {
>  	int ret;
>  	unsigned int val;
> @@ -518,7 +749,7 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
>  	}
> 
>  	ret = regmap_read(data->regmap, BMI160_REG_CHIP_ID, &val);
> -	if (ret < 0) {
> +	if (ret) {

This should be a separate patch, not rolled in here.  It's
a good cleanup but within this patch it's noise.

>  		dev_err(dev, "Error reading chip id\n");
>  		return ret;
>  	}
> @@ -539,6 +770,49 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
>  	return 0;
>  }
> 
> +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)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(data->trig);
> +
> +	return 0;
> +}
> +
>  static void bmi160_chip_uninit(void *data)
>  {
>  	struct bmi160_data *bmi_data = data;
> @@ -548,7 +822,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;
> @@ -562,7 +836,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>  	dev_set_drvdata(dev, indio_dev);
>  	data->regmap = regmap;
> 
> -	ret = bmi160_chip_init(data, use_spi);
> +	ret = bmi160_chip_init(data, use_spi, irq);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -585,6 +859,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)
> +			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[] = {
> --
> 2.11.0
> 

Re: [PATCH v2 4/6] dt-bindings: document open-drain property

[Jonathan Cameron]

On Mon, 21 Jan 2019 18:04:29 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> From: Martin Kelly <martin@martingkelly.com>
> 
> We have added an optional boolean property for configuring a BMI160
> interrupt pin as open-drain, as opposed to the default push-pull, so
> document this.
> 
> Signed-off-by: Martin Kelly <martin@martingkelly.com>
> ---
>  Documentation/devicetree/bindings/iio/imu/bmi160.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> index 1aec19997fb5..7eb4b6016404 100644
> --- a/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> @@ -12,6 +12,8 @@ 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
> +- bmi160,open-drain : set if the specified interrupt pin should be configured as
> +  open drain. If not set, defaults to push-pull.
I missed this before, but normally we prefix with a manufacturer rather than
a particular part number.  Maybe that changed when I wasn't looking though!

Anyhow, these DT binding changes all need devicetree maintainer reviews as they
aren't totally trivial.

Thanks,

Jonathan

>  
>  Examples:
>  

Re: [PATCH v2 6/6] iio:bmi160: use if (ret) instead of if (ret < 0)

[Jonathan Cameron]

On Mon, 21 Jan 2019 18:04:31 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> From: Martin Kelly <martin@martingkelly.com>
> 
> We are using "if (ret < 0)" in many places in which the function returns 0
> on success. Use "if (ret)" instead for better clarity and correctness.
> 
> Signed-off-by: Martin Kelly <martin@martingkelly.com>
The rest of the series that I haven't commented on looks fine to me.
I think there is one of these changes in an earlier patch but otherwise
this looks good for v3.

One small tidy up inline though.

> ---
>  drivers/iio/imu/bmi160/bmi160_core.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index dca53be066e1..0ec9ded975e2 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -299,7 +299,7 @@ int bmi160_set_mode(struct bmi160_data *data, enum bmi160_sensor_type t,
>  		cmd = bmi160_regs[t].pmu_cmd_suspend;
>  
>  	ret = regmap_write(data->regmap, BMI160_REG_CMD, cmd);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	usleep_range(bmi160_pmu_time[t], bmi160_pmu_time[t] + 1000);
> @@ -331,7 +331,7 @@ int bmi160_get_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
>  	int i, ret, val;
>  
>  	ret = regmap_read(data->regmap, bmi160_regs[t].range, &val);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	for (i = 0; i < bmi160_scale_table[t].num; i++)
> @@ -354,7 +354,7 @@ static int bmi160_get_data(struct bmi160_data *data, int chan_type,
>  	reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(sample);
>  
>  	ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(sample));
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = sign_extend32(le16_to_cpu(sample), 15);
> @@ -388,7 +388,7 @@ static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
>  	int i, val, ret;
>  
>  	ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	val &= bmi160_regs[t].config_odr_mask;
> @@ -420,7 +420,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
>  			 indio_dev->masklength) {
>  		ret = regmap_bulk_read(data->regmap, base + i * sizeof(sample),
>  				       &sample, sizeof(sample));
> -		if (ret < 0)
> +		if (ret)
>  			goto done;
>  		buf[j++] = sample;
>  	}
> @@ -441,18 +441,18 @@ static int bmi160_read_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		ret = bmi160_get_data(data, chan->type, chan->channel2, val);
> -		if (ret < 0)
> +		if (ret)
>  			return ret;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 0;
>  		ret = bmi160_get_scale(data,
>  				       bmi160_to_sensor(chan->type), val2);
> -		return ret < 0 ? ret : IIO_VAL_INT_PLUS_MICRO;
> +		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = bmi160_get_odr(data, bmi160_to_sensor(chan->type),
>  				     val, val2);
> -		return ret < 0 ? ret : IIO_VAL_INT_PLUS_MICRO;
> +		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -732,7 +732,7 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
>  	struct device *dev = regmap_get_device(data->regmap);
>  
>  	ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	usleep_range(BMI160_SOFTRESET_USLEEP, BMI160_SOFTRESET_USLEEP + 1);
> @@ -743,7 +743,7 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
>  	 */
>  	if (use_spi) {
>  		ret = regmap_read(data->regmap, BMI160_REG_DUMMY, &val);
> -		if (ret < 0)
> +		if (ret)
>  			return ret;
>  	}
>  
> @@ -759,11 +759,11 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
>  	}
>  
>  	ret = bmi160_set_mode(data, BMI160_ACCEL, true);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = bmi160_set_mode(data, BMI160_GYRO, true);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	return 0;
> @@ -796,7 +796,7 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
>  	ret = devm_request_irq(&indio_dev->dev, irq,
>  			       &iio_trigger_generic_data_rdy_poll,
>  			       irq_type, "bmi160", data->trig);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	data->trig->dev.parent = regmap_get_device(data->regmap);
> @@ -836,11 +836,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>  	data->regmap = regmap;
>  
>  	ret = bmi160_chip_init(data, use_spi, irq);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = devm_add_action_or_reset(dev, bmi160_chip_uninit, data);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	if (!name && ACPI_HANDLE(dev))
> @@ -856,7 +856,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  					      iio_pollfunc_store_time,
>  					      bmi160_trigger_handler, NULL);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	if (irq) {
> @@ -866,7 +866,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>  	}
>  
>  	ret = devm_iio_device_register(dev, indio_dev);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
return devm_iio_device_register.

>  
>  	return 0;

Re: [PATCH v2 4/6] dt-bindings: document open-drain property

[Martin Kelly]

On 1/26/19 12:19 PM, Jonathan Cameron wrote:
> On Mon, 21 Jan 2019 18:04:29 -0800
> Martin Kelly <martin@martingkelly.com> wrote:
> 
>> From: Martin Kelly <martin@martingkelly.com>
>>
>> We have added an optional boolean property for configuring a BMI160
>> interrupt pin as open-drain, as opposed to the default push-pull, so
>> document this.
>>
>> Signed-off-by: Martin Kelly <martin@martingkelly.com>
>> ---
>>   Documentation/devicetree/bindings/iio/imu/bmi160.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>> index 1aec19997fb5..7eb4b6016404 100644
>> --- a/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>> @@ -12,6 +12,8 @@ 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
>> +- bmi160,open-drain : set if the specified interrupt pin should be configured as
>> +  open drain. If not set, defaults to push-pull.
> I missed this before, but normally we prefix with a manufacturer rather than
> a particular part number.  Maybe that changed when I wasn't looking though!
> 
> Anyhow, these DT binding changes all need devicetree maintainer reviews as they
> aren't totally trivial.
> 

Yep, devicetree is on this thread. Rob, should I prefix with device name 
("bmi160") or manufacturer ("bosch")?

> Thanks,
> 
> Jonathan
> 
>>   
>>   Examples:
>>   
> 

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

[Martin Kelly]

On 1/26/19 12:17 PM, Jonathan Cameron wrote:
> On Mon, 21 Jan 2019 18:04:27 -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.
>>
>> Signed-off-by: Martin Kelly <martin@martingkelly.com>
> 
> Various minor bits inline.  I would also, if possible like Daniel to
> take a glance at this series before we apply it. I don't know this hardware
> at all well!
> 
> Jonathan
> 
>> ---
>> v2:
>> - Drop "BOTH" interrupt setting.
>> - Change to "if (ret)" instead of "if (ret < 0)".
>> - Stylistic changes suggested by Jonathan Cameron.
>> - Fix bogus return check after iio_trigger_get.
>>
>>   arch/arm/boot/dts/Makefile           |   1 +
>>   drivers/iio/imu/bmi160/bmi160.h      |  13 +-
>>   drivers/iio/imu/bmi160/bmi160_core.c | 294 ++++++++++++++++++++++++++++++++++-
>>   drivers/iio/imu/bmi160/bmi160_i2c.c  |   3 +-
>>   drivers/iio/imu/bmi160/bmi160_spi.c  |   2 +-
>>   5 files changed, 303 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index b0e966d625b9..df68910fc2c1 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -701,6 +701,7 @@ dtb-$(CONFIG_SOC_AM33XX) += \
>>   	am335x-base0033.dtb \
>>   	am335x-bone.dtb \
>>   	am335x-boneblack.dtb \
>> +	am335x-boneblack-bmi160-i2c1.dtb \
> 
> Separate patch for this.
> 

Oops, this wasn't intended to be there at all; it's an accidental 
carry-over from my testing that should not be checked in. I'll remove it.

>>   	am335x-boneblack-wireless.dtb \
>>   	am335x-boneblue.dtb \
>>   	am335x-bonegreen.dtb \
>> 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..c848fc1bce61 100644
>> --- a/drivers/iio/imu/bmi160/bmi160_core.c
>> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
>> @@ -3,21 +3,25 @@
>>    * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
>>    *
>>    * Copyright (c) 2016, Intel Corporation.
>> + * Copyright (c) 2019, Martin Kelly.
>>    *
>>    * 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>
>>   #include <linux/iio/trigger_consumer.h>
>>   #include <linux/iio/buffer.h>
>>   #include <linux/iio/sysfs.h>
>> +#include <linux/iio/trigger.h>
>>
>>   #include "bmi160.h"
>>
>> @@ -61,8 +65,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 +133,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 +524,209 @@ 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)
>> +		return ret;
>> +
>> +	val = (val & ~mask) | bits;
>> +
>> +	ret = regmap_write(regmap, reg, val);
>> +	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 = 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)
>> +		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)
>> +		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)
>> +		return ret;
> return bmi160...
>> +
>> +	return 0;
>> +}
>> +
>> +int bmi160_enable_irq(struct regmap *regmap, bool enable)
>> +{
>> +	unsigned int enable_bit = 0;
>> +
>> +	if (enable)
>> +		enable_bit = BMI160_DRDY_INT_EN;
>> +
>> +	return bmi160_write_conf_reg(regmap, BMI160_REG_INT_EN,
>> +				     BMI160_DRDY_INT_EN, enable_bit,
>> +				     BMI160_NORMAL_WRITE_USLEEP);
>> +}
>> +EXPORT_SYMBOL(bmi160_enable_irq);
>> +
>> +static bool bmi160_parse_irqname(struct device_node *of_node, int irq,
>> +				 enum bmi160_int_pin *pin)
>> +{
>> +	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) {
>> +		*pin = BMI160_PIN_INT1;
>> +		return true;
>> +	}
> 
> Given both could be provided, and we are implying a preference,
> why not just get INT1 first by name and if it's not there try for
> INT2?  No need for this separate does the name match query.
> 

I actually didn't mean to imply a preference; I just figured that a 
given IRQ can have only one name, and therefore at most one of the names 
"INT1" and "INT2" will match the passed-in IRQ. Is this a bad assumption?

>> +
>> +	ret = of_irq_get_byname(of_node, "INT2");
>> +	if (ret == irq) {
>> +		*pin = BMI160_PIN_INT2;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +static int bmi160_config_device_irq(struct iio_dev *indio_dev,
>> +				    int irq, int irq_type)
>> +{
>> +	int ret;
>> +	bool success;
>> +	enum bmi160_int_pin int_pin;
>> +	bool open_drain;
>> +	const char *pin_name;
>> +	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;
> 
> That seems unlikely.  Why on a rising trigger are we setting a flag
> that is named to imply it sets level triggering?
> 

I will double-check the datasheet, but I think this is a mistake and I 
should swap the RISING and HIGH cases.

>> +	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;
>> +	}
>> +
>> +	success = bmi160_parse_irqname(dev->of_node, irq, &int_pin);
>> +	if (!success) {
>> +		dev_err(&indio_dev->dev,
>> +			"interrupt-names for IRQ %d must be set to either \"INT1\" or \"INT2\"",
>> +			irq);
>> +		return -EINVAL;
>> +	}
>> +
>> +	open_drain = of_property_read_bool(dev->of_node, "bmi160,open-drain");
>> +
>> +	ret = bmi160_config_pin(data->regmap, int_pin, open_drain, irq_mask,
>> +				BMI160_NORMAL_WRITE_USLEEP);
>> +	if (ret) {
>> +		switch (int_pin) {
>> +		case BMI160_PIN_INT1:
>> +			pin_name = "INT1";
>> +			break;
>> +		case BMI160_PIN_INT2:
>> +			pin_name = "INT2";
>> +			break;
>> +		}
>> +		dev_err(&indio_dev->dev, "Failed to configure %s IRQ pin",
>> +			pin_name);
> 
> Why not do that within bmi160_config_pin?  Then we have
> return bmi160_config_pin..
> 

Yeah, I'll move this into bmi160_config_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)
>> +		return ret;
>> +
>> +	ret = bmi160_probe_trigger(indio_dev, irq, irq_type);
>> +	if (ret)
>> +		return ret;
> 
> return bmi160_probe_trigger...  It's either zero or it's not.
> Either way this gives the same as you have here.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int bmi160_chip_init(struct bmi160_data *data, bool use_spi, int irq)
>>   {
>>   	int ret;
>>   	unsigned int val;
>> @@ -518,7 +749,7 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
>>   	}
>>
>>   	ret = regmap_read(data->regmap, BMI160_REG_CHIP_ID, &val);
>> -	if (ret < 0) {
>> +	if (ret) {
> 
> This should be a separate patch, not rolled in here.  It's
> a good cleanup but within this patch it's noise.
> 

Good catch, I tried to separate the cleanup changes but I missed this 
one. I will move it.

>>   		dev_err(dev, "Error reading chip id\n");
>>   		return ret;
>>   	}
>> @@ -539,6 +770,49 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
>>   	return 0;
>>   }
>>
>> +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)
>> +		return ret;
>> +
>> +	indio_dev->trig = iio_trigger_get(data->trig);
>> +
>> +	return 0;
>> +}
>> +
>>   static void bmi160_chip_uninit(void *data)
>>   {
>>   	struct bmi160_data *bmi_data = data;
>> @@ -548,7 +822,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;
>> @@ -562,7 +836,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>>   	dev_set_drvdata(dev, indio_dev);
>>   	data->regmap = regmap;
>>
>> -	ret = bmi160_chip_init(data, use_spi);
>> +	ret = bmi160_chip_init(data, use_spi, irq);
>>   	if (ret < 0)
>>   		return ret;
>>
>> @@ -585,6 +859,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)
>> +			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[] = {
>> --
>> 2.11.0
>>
> 

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

[Jonathan Cameron]

On Sat, 26 Jan 2019 15:39:16 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> On 1/26/19 12:17 PM, Jonathan Cameron wrote:
> > On Mon, 21 Jan 2019 18:04:27 -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.
> >>
> >> Signed-off-by: Martin Kelly <martin@martingkelly.com>  
> > 
> > Various minor bits inline.  I would also, if possible like Daniel to
> > take a glance at this series before we apply it. I don't know this hardware
> > at all well!
> > 
> > Jonathan
> >   
...

> >> +
> >> +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 = 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)
> >> +		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)
> >> +		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)
> >> +		return ret;  
> > return bmi160...  
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int bmi160_enable_irq(struct regmap *regmap, bool enable)
> >> +{
> >> +	unsigned int enable_bit = 0;
> >> +
> >> +	if (enable)
> >> +		enable_bit = BMI160_DRDY_INT_EN;
> >> +
> >> +	return bmi160_write_conf_reg(regmap, BMI160_REG_INT_EN,
> >> +				     BMI160_DRDY_INT_EN, enable_bit,
> >> +				     BMI160_NORMAL_WRITE_USLEEP);
> >> +}
> >> +EXPORT_SYMBOL(bmi160_enable_irq);
> >> +
> >> +static bool bmi160_parse_irqname(struct device_node *of_node, int irq,
> >> +				 enum bmi160_int_pin *pin)
> >> +{
> >> +	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) {
> >> +		*pin = BMI160_PIN_INT1;
> >> +		return true;
> >> +	}  
> > 
> > Given both could be provided, and we are implying a preference,
> > why not just get INT1 first by name and if it's not there try for
> > INT2?  No need for this separate does the name match query.
> >   
> 
> I actually didn't mean to imply a preference; I just figured that a 
> given IRQ can have only one name, and therefore at most one of the names 
> "INT1" and "INT2" will match the passed-in IRQ. Is this a bad assumption?

I'm saying that you should express a preference. It makes
things predictable.  But not by matching the 'first' one (which is
currently what happens) but rather just asking for INT1 by name
(don't use the spi->irq at all) and use that if present.

> 
> >> +
> >> +	ret = of_irq_get_byname(of_node, "INT2");
> >> +	if (ret == irq) {
> >> +		*pin = BMI160_PIN_INT2;
> >> +		return true;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> 

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

[Martin Kelly]

On 1/26/19 12:17 PM, Jonathan Cameron wrote:
> On Mon, 21 Jan 2019 18:04:27 -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.
>>
>> Signed-off-by: Martin Kelly <martin@martingkelly.com>
[snip]

> 
> That seems unlikely.  Why on a rising trigger are we setting a flag
> that is named to imply it sets level triggering?
> 

Looking again, I see that the code is right but the names/comment were 
wrong. In v3, I renamed BMI160_LEVEL_TRIGGERED to BMI160_EDGE_TRIGGERED, 
as the default is actually level-triggered active-low. The datasheet (p 
63) has a sentence:

‘1’ (‘0’) is edge (level) triggered for INT1 pin

which I read backwards.