* [PATCH v2 1/6] iio:bmi160: add SPDX identifiers @ 2019-01-22 2:04 Martin Kelly 2019-01-22 2:04 ` [PATCH v2 2/6] iio:bmi160: add drdy interrupt support Martin Kelly ` (4 more replies) 0 siblings, 5 replies; 13+ messages in thread From: Martin Kelly @ 2019-01-22 2:04 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree, 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] 13+ messages in thread
* [PATCH v2 2/6] iio:bmi160: add drdy interrupt support 2019-01-22 2:04 [PATCH v2 1/6] iio:bmi160: add SPDX identifiers Martin Kelly @ 2019-01-22 2:04 ` Martin Kelly 2019-01-26 20:17 ` Jonathan Cameron 2019-01-22 2:04 ` [PATCH v2 3/6] dt-bindings: fix incorrect bmi160 IRQ note Martin Kelly ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Martin Kelly @ 2019-01-22 2:04 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/6] iio:bmi160: add drdy interrupt support 2019-01-22 2:04 ` [PATCH v2 2/6] iio:bmi160: add drdy interrupt support Martin Kelly @ 2019-01-26 20:17 ` Jonathan Cameron 2019-01-26 23:39 ` Martin Kelly 2019-01-27 20:43 ` Martin Kelly 0 siblings, 2 replies; 13+ messages in thread From: Jonathan Cameron @ 2019-01-26 20:17 UTC (permalink / raw) To: Martin Kelly Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/6] iio:bmi160: add drdy interrupt support 2019-01-26 20:17 ` Jonathan Cameron @ 2019-01-26 23:39 ` Martin Kelly 2019-01-27 15:07 ` Jonathan Cameron 2019-01-27 20:43 ` Martin Kelly 1 sibling, 1 reply; 13+ messages in thread From: Martin Kelly @ 2019-01-26 23:39 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree 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 >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/6] iio:bmi160: add drdy interrupt support 2019-01-26 23:39 ` Martin Kelly @ 2019-01-27 15:07 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2019-01-27 15:07 UTC (permalink / raw) To: Martin Kelly Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree 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; > >> +} > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/6] iio:bmi160: add drdy interrupt support 2019-01-26 20:17 ` Jonathan Cameron 2019-01-26 23:39 ` Martin Kelly @ 2019-01-27 20:43 ` Martin Kelly 1 sibling, 0 replies; 13+ messages in thread From: Martin Kelly @ 2019-01-27 20:43 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/6] dt-bindings: fix incorrect bmi160 IRQ note 2019-01-22 2:04 [PATCH v2 1/6] iio:bmi160: add SPDX identifiers Martin Kelly 2019-01-22 2:04 ` [PATCH v2 2/6] iio:bmi160: add drdy interrupt support Martin Kelly @ 2019-01-22 2:04 ` Martin Kelly 2019-01-22 2:04 ` [PATCH v2 4/6] dt-bindings: document open-drain property Martin Kelly ` (2 subsequent siblings) 4 siblings, 0 replies; 13+ messages in thread From: Martin Kelly @ 2019-01-22 2:04 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree, 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] 13+ messages in thread
* [PATCH v2 4/6] dt-bindings: document open-drain property 2019-01-22 2:04 [PATCH v2 1/6] iio:bmi160: add SPDX identifiers Martin Kelly 2019-01-22 2:04 ` [PATCH v2 2/6] iio:bmi160: add drdy interrupt support Martin Kelly 2019-01-22 2:04 ` [PATCH v2 3/6] dt-bindings: fix incorrect bmi160 IRQ note Martin Kelly @ 2019-01-22 2:04 ` Martin Kelly 2019-01-26 20:19 ` Jonathan Cameron 2019-01-22 2:04 ` [PATCH v2 5/6] iio:bmi160: use iio_pollfunc_store_time Martin Kelly 2019-01-22 2:04 ` [PATCH v2 6/6] iio:bmi160: use if (ret) instead of if (ret < 0) Martin Kelly 4 siblings, 1 reply; 13+ messages in thread From: Martin Kelly @ 2019-01-22 2:04 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/6] dt-bindings: document open-drain property 2019-01-22 2:04 ` [PATCH v2 4/6] dt-bindings: document open-drain property Martin Kelly @ 2019-01-26 20:19 ` Jonathan Cameron 2019-01-26 23:31 ` Martin Kelly 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2019-01-26 20:19 UTC (permalink / raw) To: Martin Kelly Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree, Rob Herring 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: > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/6] dt-bindings: document open-drain property 2019-01-26 20:19 ` Jonathan Cameron @ 2019-01-26 23:31 ` Martin Kelly 0 siblings, 0 replies; 13+ messages in thread From: Martin Kelly @ 2019-01-26 23:31 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree 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: >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 5/6] iio:bmi160: use iio_pollfunc_store_time 2019-01-22 2:04 [PATCH v2 1/6] iio:bmi160: add SPDX identifiers Martin Kelly ` (2 preceding siblings ...) 2019-01-22 2:04 ` [PATCH v2 4/6] dt-bindings: document open-drain property Martin Kelly @ 2019-01-22 2:04 ` Martin Kelly 2019-01-22 2:04 ` [PATCH v2 6/6] iio:bmi160: use if (ret) instead of if (ret < 0) Martin Kelly 4 siblings, 0 replies; 13+ messages in thread From: Martin Kelly @ 2019-01-22 2:04 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 6/6] iio:bmi160: use if (ret) instead of if (ret < 0) 2019-01-22 2:04 [PATCH v2 1/6] iio:bmi160: add SPDX identifiers Martin Kelly ` (3 preceding siblings ...) 2019-01-22 2:04 ` [PATCH v2 5/6] iio:bmi160: use iio_pollfunc_store_time Martin Kelly @ 2019-01-22 2:04 ` Martin Kelly 2019-01-26 20:22 ` Jonathan Cameron 4 siblings, 1 reply; 13+ messages in thread From: Martin Kelly @ 2019-01-22 2:04 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 6/6] iio:bmi160: use if (ret) instead of if (ret < 0) 2019-01-22 2:04 ` [PATCH v2 6/6] iio:bmi160: use if (ret) instead of if (ret < 0) Martin Kelly @ 2019-01-26 20:22 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2019-01-26 20:22 UTC (permalink / raw) To: Martin Kelly Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring, Daniel Baluta, devicetree 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-01-27 20:43 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-22 2:04 [PATCH v2 1/6] iio:bmi160: add SPDX identifiers Martin Kelly 2019-01-22 2:04 ` [PATCH v2 2/6] iio:bmi160: add drdy interrupt support Martin Kelly 2019-01-26 20:17 ` Jonathan Cameron 2019-01-26 23:39 ` Martin Kelly 2019-01-27 15:07 ` Jonathan Cameron 2019-01-27 20:43 ` Martin Kelly 2019-01-22 2:04 ` [PATCH v2 3/6] dt-bindings: fix incorrect bmi160 IRQ note Martin Kelly 2019-01-22 2:04 ` [PATCH v2 4/6] dt-bindings: document open-drain property Martin Kelly 2019-01-26 20:19 ` Jonathan Cameron 2019-01-26 23:31 ` Martin Kelly 2019-01-22 2:04 ` [PATCH v2 5/6] iio:bmi160: use iio_pollfunc_store_time Martin Kelly 2019-01-22 2:04 ` [PATCH v2 6/6] iio:bmi160: use if (ret) instead of if (ret < 0) Martin Kelly 2019-01-26 20:22 ` Jonathan Cameron
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).