* [PATCH 0/5] iio:bmi160: add drdy interrupt support
@ 2019-01-14 3:56 Martin Kelly
2019-01-14 3:56 ` [PATCH 1/5] iio:bmi160: add SPDX identifiers Martin Kelly
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Martin Kelly @ 2019-01-14 3:56 UTC (permalink / raw)
To: linux-iio
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Rob Herring, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/5] iio:bmi160: add SPDX identifiers 2019-01-14 3:56 [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly @ 2019-01-14 3:56 ` Martin Kelly 2019-01-19 17:03 ` Jonathan Cameron 2019-01-14 3:56 ` [PATCH 2/5] iio:bmi160: add drdy interrupt support Martin Kelly ` (4 subsequent siblings) 5 siblings, 1 reply; 12+ messages in thread From: Martin Kelly @ 2019-01-14 3:56 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/5] iio:bmi160: add SPDX identifiers 2019-01-14 3:56 ` [PATCH 1/5] iio:bmi160: add SPDX identifiers Martin Kelly @ 2019-01-19 17:03 ` Jonathan Cameron 0 siblings, 0 replies; 12+ messages in thread From: Jonathan Cameron @ 2019-01-19 17:03 UTC (permalink / raw) To: Martin Kelly Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/5] iio:bmi160: add drdy interrupt support 2019-01-14 3:56 [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly 2019-01-14 3:56 ` [PATCH 1/5] iio:bmi160: add SPDX identifiers Martin Kelly @ 2019-01-14 3:56 ` Martin Kelly 2019-01-19 17:17 ` Jonathan Cameron 2019-01-14 3:56 ` [PATCH 3/5] dt-bindings: fix incorrect bmi160 IRQ note Martin Kelly ` (3 subsequent siblings) 5 siblings, 1 reply; 12+ messages in thread From: Martin Kelly @ 2019-01-14 3:56 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/5] iio:bmi160: add drdy interrupt support 2019-01-14 3:56 ` [PATCH 2/5] iio:bmi160: add drdy interrupt support Martin Kelly @ 2019-01-19 17:17 ` Jonathan Cameron 2019-01-22 2:06 ` Martin Kelly 0 siblings, 1 reply; 12+ messages in thread From: Jonathan Cameron @ 2019-01-19 17:17 UTC (permalink / raw) To: Martin Kelly Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring 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); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/5] iio:bmi160: add drdy interrupt support 2019-01-19 17:17 ` Jonathan Cameron @ 2019-01-22 2:06 ` Martin Kelly 0 siblings, 0 replies; 12+ messages in thread From: Martin Kelly @ 2019-01-22 2:06 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/5] dt-bindings: fix incorrect bmi160 IRQ note 2019-01-14 3:56 [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly 2019-01-14 3:56 ` [PATCH 1/5] iio:bmi160: add SPDX identifiers Martin Kelly 2019-01-14 3:56 ` [PATCH 2/5] iio:bmi160: add drdy interrupt support Martin Kelly @ 2019-01-14 3:56 ` Martin Kelly 2019-01-14 3:56 ` [PATCH 4/5] dt-bindings:bmi160: add "BOTH" int pin type Martin Kelly ` (2 subsequent siblings) 5 siblings, 0 replies; 12+ messages in thread From: Martin Kelly @ 2019-01-14 3:56 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/5] dt-bindings:bmi160: add "BOTH" int pin type 2019-01-14 3:56 [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly ` (2 preceding siblings ...) 2019-01-14 3:56 ` [PATCH 3/5] dt-bindings: fix incorrect bmi160 IRQ note Martin Kelly @ 2019-01-14 3:56 ` Martin Kelly 2019-01-19 17:24 ` Jonathan Cameron 2019-01-14 3:56 ` [PATCH 5/5] iio:bmi160:snap timestamp closer to event Martin Kelly 2019-01-15 2:13 ` [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly 5 siblings, 1 reply; 12+ messages in thread From: Martin Kelly @ 2019-01-14 3:56 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 4/5] dt-bindings:bmi160: add "BOTH" int pin type 2019-01-14 3:56 ` [PATCH 4/5] dt-bindings:bmi160: add "BOTH" int pin type Martin Kelly @ 2019-01-19 17:24 ` Jonathan Cameron 0 siblings, 0 replies; 12+ messages in thread From: Jonathan Cameron @ 2019-01-19 17:24 UTC (permalink / raw) To: Martin Kelly Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, devicetree 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 > +}; ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/5] iio:bmi160:snap timestamp closer to event 2019-01-14 3:56 [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly ` (3 preceding siblings ...) 2019-01-14 3:56 ` [PATCH 4/5] dt-bindings:bmi160: add "BOTH" int pin type Martin Kelly @ 2019-01-14 3:56 ` Martin Kelly 2019-01-19 17:29 ` Jonathan Cameron 2019-01-15 2:13 ` [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly 5 siblings, 1 reply; 12+ messages in thread From: Martin Kelly @ 2019-01-14 3:56 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] iio:bmi160:snap timestamp closer to event 2019-01-14 3:56 ` [PATCH 5/5] iio:bmi160:snap timestamp closer to event Martin Kelly @ 2019-01-19 17:29 ` Jonathan Cameron 0 siblings, 0 replies; 12+ messages in thread From: Jonathan Cameron @ 2019-01-19 17:29 UTC (permalink / raw) To: Martin Kelly Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring 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; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] iio:bmi160: add drdy interrupt support 2019-01-14 3:56 [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly ` (4 preceding siblings ...) 2019-01-14 3:56 ` [PATCH 5/5] iio:bmi160:snap timestamp closer to event Martin Kelly @ 2019-01-15 2:13 ` Martin Kelly 5 siblings, 0 replies; 12+ messages in thread From: Martin Kelly @ 2019-01-15 2:13 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-01-22 2:06 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-14 3:56 [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly 2019-01-14 3:56 ` [PATCH 1/5] iio:bmi160: add SPDX identifiers Martin Kelly 2019-01-19 17:03 ` Jonathan Cameron 2019-01-14 3:56 ` [PATCH 2/5] iio:bmi160: add drdy interrupt support Martin Kelly 2019-01-19 17:17 ` Jonathan Cameron 2019-01-22 2:06 ` Martin Kelly 2019-01-14 3:56 ` [PATCH 3/5] dt-bindings: fix incorrect bmi160 IRQ note Martin Kelly 2019-01-14 3:56 ` [PATCH 4/5] dt-bindings:bmi160: add "BOTH" int pin type Martin Kelly 2019-01-19 17:24 ` Jonathan Cameron 2019-01-14 3:56 ` [PATCH 5/5] iio:bmi160:snap timestamp closer to event Martin Kelly 2019-01-19 17:29 ` Jonathan Cameron 2019-01-15 2:13 ` [PATCH 0/5] iio:bmi160: add drdy interrupt support Martin Kelly
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).