* [PATCH 1/2] iio: bmi160: Support hardware fifo
@ 2016-11-03 11:25 ` Marcin Niestroj
0 siblings, 0 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-03 11:25 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Marcin Niestroj
This patch was developed primarily based on bmc150_accel hardware fifo
implementation.
IRQ handler was added, which for now is responsible only for handling
watermark interrupts. The BMI160 chip has two interrupt outputs. By
default INT is considered to be connected. If INT2 is used instead, the
interrupt-names device-tree property can be used to specify that.
Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
---
drivers/iio/imu/bmi160/bmi160.h | 3 +-
drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
4 files changed, 618 insertions(+), 28 deletions(-)
diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
index d2ae6ed..4a7c10e 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -4,7 +4,8 @@
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, int irq,
+ bool use_spi, bool block_supported);
void bmi160_core_remove(struct device *dev);
#endif /* BMI160_H_ */
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index e0251b8..153734c 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -2,6 +2,7 @@
* BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
*
* Copyright (c) 2016, Intel Corporation.
+ * Copyright (c) 2016, Grinn
*
* 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
@@ -9,7 +10,7 @@
*
* IIO core driver for BMI160, with support for I2C/SPI busses
*
- * TODO: magnetometer, interrupts, hardware FIFO
+ * TODO: magnetometer, interrupts
*/
#include <linux/module.h>
#include <linux/regmap.h>
@@ -22,8 +23,12 @@
#include <linux/iio/buffer.h>
#include <linux/iio/sysfs.h>
+#include <linux/of_irq.h>
+
#include "bmi160.h"
+#define BMI160_IRQ_NAME "bmi160_event"
+
#define BMI160_REG_CHIP_ID 0x00
#define BMI160_CHIP_ID_VAL 0xD1
@@ -34,6 +39,21 @@
#define BMI160_REG_DATA_GYRO_XOUT_L 0x0C
#define BMI160_REG_DATA_ACCEL_XOUT_L 0x12
+#define BMI160_REG_STATUS 0x1B
+#define BMI160_STATUS_MAG_MAN_OP BIT(2)
+
+#define BMI160_REG_INT_STATUS0 0x1C
+
+#define BMI160_REG_INT_STATUS1 0x1D
+#define BMI160_INT_STATUS_FWM BIT(6)
+
+#define BMI160_REG_INT_STATUS2 0x1E
+
+#define BMI160_REG_INT_STATUS3 0x1F
+
+#define BMI160_REG_FIFO_LENGTH 0x22
+#define BMI160_REG_FIFO_DATA 0x24
+
#define BMI160_REG_ACCEL_CONFIG 0x40
#define BMI160_ACCEL_CONFIG_ODR_MASK GENMASK(3, 0)
#define BMI160_ACCEL_CONFIG_BWP_MASK GENMASK(6, 4)
@@ -55,6 +75,36 @@
#define BMI160_GYRO_RANGE_250DPS 0x03
#define BMI160_GYRO_RANGE_125DPS 0x04
+#define BMI160_REG_FIFO_CONFIG_0 0x46
+
+#define BMI160_REG_FIFO_CONFIG_1 0x47
+#define BMI160_FIFO_GYRO_EN BIT(7)
+#define BMI160_FIFO_ACCEL_EN BIT(6)
+#define BMI160_FIFO_MAGN_EN BIT(5)
+#define BMI160_FIFO_HEADER_EN BIT(4)
+#define BMI160_FIFO_TAG_INT1_EN BIT(3)
+#define BMI160_FIFO_TAG_INT2_EN BIT(2)
+#define BMI160_FIFO_TIME_EN BIT(1)
+
+#define BMI160_REG_INT_EN_1 0x51
+#define BMI160_INT_FWM_EN BIT(6)
+#define BMI160_INT_FFULL_EN BIT(5)
+#define BMI160_INT_DRDY_EN BIT(4)
+
+#define BMI160_REG_INT_OUT_CTRL 0x53
+#define BMI160_INT2_OUTPUT_EN BIT(7)
+#define BMI160_INT1_OUTPUT_EN BIT(3)
+
+#define BMI160_REG_INT_LATCH 0x54
+
+#define BMI160_REG_INT_MAP_1 0x56
+#define BMI160_INT1_MAP_DRDY BIT(7)
+#define BMI160_INT1_MAP_FWM BIT(6)
+#define BMI160_INT1_MAP_FFULL BIT(5)
+#define BMI160_INT2_MAP_DRDY BIT(3)
+#define BMI160_INT2_MAP_FWM BIT(2)
+#define BMI160_INT2_MAP_FFULL BIT(1)
+
#define BMI160_REG_CMD 0x7E
#define BMI160_CMD_ACCEL_PM_SUSPEND 0x10
#define BMI160_CMD_ACCEL_PM_NORMAL 0x11
@@ -66,6 +116,8 @@
#define BMI160_REG_DUMMY 0x7F
+#define BMI160_FIFO_LENGTH 1024
+
#define BMI160_ACCEL_PMU_MIN_USLEEP 3200
#define BMI160_ACCEL_PMU_MAX_USLEEP 3800
#define BMI160_GYRO_PMU_MIN_USLEEP 55000
@@ -110,8 +162,33 @@ enum bmi160_sensor_type {
BMI160_NUM_SENSORS /* must be last */
};
+struct bmi160_irq_data {
+ unsigned int map_fwm;
+ unsigned int output_en;
+};
+
+static const struct bmi160_irq_data bmi160_irq1_data = {
+ .map_fwm = BMI160_INT1_MAP_FWM,
+ .output_en = BMI160_INT1_OUTPUT_EN,
+};
+
+static const struct bmi160_irq_data bmi160_irq2_data = {
+ .map_fwm = BMI160_INT2_MAP_FWM,
+ .output_en = BMI160_INT2_OUTPUT_EN,
+};
+
struct bmi160_data {
struct regmap *regmap;
+ struct mutex mutex;
+ const struct bmi160_irq_data *irq_data;
+ int irq;
+ int64_t timestamp;
+ int64_t fifo_sample_period;
+ bool fifo_enabled;
+ unsigned int fifo_config;
+ unsigned int fifo_sample_size;
+ u8 *fifo_buffer;
+ unsigned int watermark;
};
const struct regmap_config bmi160_regmap_config = {
@@ -159,11 +236,11 @@ struct bmi160_pmu_time {
static struct bmi160_pmu_time bmi160_pmu_time[] = {
[BMI160_ACCEL] = {
.min = BMI160_ACCEL_PMU_MIN_USLEEP,
- .max = BMI160_ACCEL_PMU_MAX_USLEEP
+ .max = BMI160_ACCEL_PMU_MAX_USLEEP,
},
[BMI160_GYRO] = {
.min = BMI160_GYRO_PMU_MIN_USLEEP,
- .max = BMI160_GYRO_PMU_MIN_USLEEP,
+ .max = BMI160_GYRO_PMU_MAX_USLEEP,
},
};
@@ -285,7 +362,9 @@ int bmi160_set_mode(struct bmi160_data *data, enum bmi160_sensor_type t,
else
cmd = bmi160_regs[t].pmu_cmd_suspend;
+ mutex_lock(&data->mutex);
ret = regmap_write(data->regmap, BMI160_REG_CMD, cmd);
+ mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
@@ -298,6 +377,7 @@ static
int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
int uscale)
{
+ int ret;
int i;
for (i = 0; i < bmi160_scale_table[t].num; i++)
@@ -307,8 +387,12 @@ int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
if (i == bmi160_scale_table[t].num)
return -EINVAL;
- return regmap_write(data->regmap, bmi160_regs[t].range,
- bmi160_scale_table[t].tbl[i].bits);
+ mutex_lock(&data->mutex);
+ ret = regmap_write(data->regmap, bmi160_regs[t].range,
+ bmi160_scale_table[t].tbl[i].bits);
+ mutex_unlock(&data->mutex);
+
+ return ret;
}
static
@@ -317,7 +401,9 @@ int bmi160_get_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
{
int i, ret, val;
+ mutex_lock(&data->mutex);
ret = regmap_read(data->regmap, bmi160_regs[t].range, &val);
+ mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
@@ -340,7 +426,9 @@ static int bmi160_get_data(struct bmi160_data *data, int chan_type,
reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(__le16);
+ mutex_lock(&data->mutex);
ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(__le16));
+ mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
@@ -353,6 +441,7 @@ static
int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
int odr, int uodr)
{
+ int ret;
int i;
for (i = 0; i < bmi160_odr_table[t].num; i++)
@@ -363,20 +452,30 @@ int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
if (i >= bmi160_odr_table[t].num)
return -EINVAL;
- return regmap_update_bits(data->regmap,
- bmi160_regs[t].config,
- bmi160_regs[t].config_odr_mask,
- bmi160_odr_table[t].tbl[i].bits);
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap,
+ bmi160_regs[t].config,
+ bmi160_regs[t].config_odr_mask,
+ bmi160_odr_table[t].tbl[i].bits);
+ mutex_unlock(&data->mutex);
+
+ return ret;
}
-static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
- int *odr, int *uodr)
+static int64_t bmi160_frequency_to_period(int odr, int uodr)
{
- int i, val, ret;
+ uint64_t period = 1000000000000000;
+ int64_t frequency = (int64_t) odr * 1000000 + uodr;
- ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
- if (ret < 0)
- return ret;
+ do_div(period, frequency);
+
+ return period;
+}
+
+static const struct bmi160_odr *bmi160_reg_to_odr(enum bmi160_sensor_type t,
+ unsigned int val)
+{
+ int i;
val &= bmi160_regs[t].config_odr_mask;
@@ -385,10 +484,52 @@ static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
break;
if (i >= bmi160_odr_table[t].num)
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
+
+ return &bmi160_odr_table[t].tbl[i];
+}
+
+static int bmi160_get_sample_period(struct bmi160_data *data,
+ enum bmi160_sensor_type t,
+ int64_t *sample_period)
+{
+ const struct bmi160_odr *odr_entry;
+ int ret;
+ unsigned int val;
+
+ ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
+ if (ret < 0)
+ return ret;
- *odr = bmi160_odr_table[t].tbl[i].odr;
- *uodr = bmi160_odr_table[t].tbl[i].uodr;
+ odr_entry = bmi160_reg_to_odr(t, val);
+ if (IS_ERR(odr_entry))
+ return PTR_ERR(odr_entry);
+
+ *sample_period = bmi160_frequency_to_period(odr_entry->odr,
+ odr_entry->uodr);
+
+ return 0;
+}
+
+static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
+ int *odr, int *uodr)
+{
+ const struct bmi160_odr *odr_entry;
+ int ret;
+ unsigned int val;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
+ mutex_unlock(&data->mutex);
+ if (ret < 0)
+ return ret;
+
+ odr_entry = bmi160_reg_to_odr(t, val);
+ if (IS_ERR(odr_entry))
+ return PTR_ERR(odr_entry);
+
+ *odr = odr_entry->odr;
+ *uodr = odr_entry->uodr;
return 0;
}
@@ -402,14 +543,18 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
__le16 sample;
+ mutex_lock(&data->mutex);
for_each_set_bit(i, indio_dev->active_scan_mask,
indio_dev->masklength) {
ret = regmap_bulk_read(data->regmap, base + i * sizeof(__le16),
&sample, sizeof(__le16));
- if (ret < 0)
+ if (ret < 0) {
+ mutex_unlock(&data->mutex);
goto done;
+ }
buf[j++] = sample;
}
+ mutex_unlock(&data->mutex);
iio_push_to_buffers_with_timestamp(indio_dev, buf,
iio_get_time_ns(indio_dev));
@@ -493,11 +638,364 @@ static const struct attribute_group bmi160_attrs_group = {
.attrs = bmi160_attrs,
};
+static int bmi160_update_sample_period(struct bmi160_data *data,
+ enum bmi160_sensor_type sensor_type)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int64_t sample_period;
+ int ret;
+
+ ret = bmi160_get_sample_period(data, sensor_type, &sample_period);
+ if (ret < 0)
+ return ret;
+
+ if (data->fifo_sample_period) {
+ if (data->fifo_sample_period != sample_period) {
+ dev_warn(dev, "Enabled sensors have unequal ODR values\n");
+ return -EINVAL;
+ }
+ } else {
+ data->fifo_sample_period = sample_period;
+ }
+
+ return 0;
+}
+
+static int bmi160_fifo_enable(struct iio_dev *indio_dev,
+ struct bmi160_data *data)
+{
+ struct regmap *regmap = data->regmap;
+ struct device *dev = regmap_get_device(regmap);
+ int ret;
+ int i;
+ unsigned int val;
+ unsigned int fifo_config = 0;
+
+ /* Set fifo sample size and period */
+ for_each_set_bit(i, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ if (i <= BMI160_SCAN_GYRO_Z)
+ fifo_config |= BMI160_FIFO_GYRO_EN;
+ else if (i <= BMI160_SCAN_ACCEL_Z)
+ fifo_config |= BMI160_FIFO_ACCEL_EN;
+ }
+
+ data->fifo_sample_period = 0;
+ data->fifo_sample_size = 0;
+ if (fifo_config & BMI160_FIFO_GYRO_EN) {
+ data->fifo_sample_size += 6;
+ ret = bmi160_update_sample_period(data, BMI160_GYRO);
+ if (ret < 0)
+ return ret;
+ }
+ if (fifo_config & BMI160_FIFO_ACCEL_EN) {
+ data->fifo_sample_size += 6;
+ ret = bmi160_update_sample_period(data, BMI160_ACCEL);
+ if (ret < 0)
+ return ret;
+ }
+
+ /*
+ * Set watermark level and write real value back, as it will be used
+ * in timestamp calculation.
+ */
+ val = data->watermark * data->fifo_sample_size;
+ if (val > BMI160_FIFO_LENGTH - 1) {
+ val = BMI160_FIFO_LENGTH - 1;
+ data->watermark = val / data->fifo_sample_size;
+ }
+ val = data->watermark * data->fifo_sample_size / 4;
+
+ ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_0, val);
+ if (ret < 0) {
+ dev_err(dev, "Failed to set watermark\n");
+ return ret;
+ }
+
+ /* Enable FIFO channels */
+ ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1,
+ fifo_config);
+ if (ret < 0) {
+ dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
+ return ret;
+ }
+
+ data->fifo_config = fifo_config;
+ data->fifo_enabled = true;
+
+ return 0;
+}
+
+static int bmi160_fifo_disable(struct bmi160_data *data)
+{
+ struct regmap *regmap = data->regmap;
+ struct device *dev = regmap_get_device(regmap);
+ int ret;
+
+ /* Disable all FIFO channels */
+ ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1, 0);
+ if (ret < 0) {
+ dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
+ return ret;
+ }
+
+ data->fifo_enabled = false;
+
+ return 0;
+}
+
+static int bmi160_buffer_postenable(struct iio_dev *indio_dev)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+ int ret;
+
+ if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+ return iio_triggered_buffer_postenable(indio_dev);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, BMI160_REG_INT_MAP_1,
+ data->irq_data->map_fwm, data->irq_data->map_fwm);
+ if (ret < 0)
+ goto unlock;
+
+ ret = regmap_update_bits(data->regmap, BMI160_REG_INT_EN_1,
+ BMI160_INT_FWM_EN, BMI160_INT_FWM_EN);
+ if (ret < 0)
+ goto unlock;
+
+ ret = bmi160_fifo_enable(indio_dev, data);
+
+unlock:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bmi160_buffer_predisable(struct iio_dev *indio_dev)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret = 0;
+
+ if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+ return iio_triggered_buffer_predisable(indio_dev);
+
+ mutex_lock(&data->mutex);
+
+ ret = regmap_update_bits(regmap, BMI160_REG_INT_EN_1,
+ BMI160_INT_FWM_EN, 0);
+ if (ret < 0)
+ goto unlock;
+
+ ret = bmi160_fifo_disable(data);
+
+unlock:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static const struct iio_buffer_setup_ops bmi160_buffer_ops = {
+ .postenable = bmi160_buffer_postenable,
+ .predisable = bmi160_buffer_predisable,
+};
+
+static ssize_t bmi160_get_fifo_state(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct bmi160_data *data = iio_priv(indio_dev);
+ bool state;
+
+ mutex_lock(&data->mutex);
+ state = data->fifo_enabled;
+ mutex_unlock(&data->mutex);
+
+ return sprintf(buf, "%d\n", (int) state);
+}
+
+static ssize_t bmi160_get_fifo_watermark(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct bmi160_data *data = iio_priv(indio_dev);
+ int wm;
+
+ mutex_lock(&data->mutex);
+ wm = data->watermark;
+ mutex_unlock(&data->mutex);
+
+ return sprintf(buf, "%d\n", wm);
+}
+
+static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
+static IIO_CONST_ATTR(hwfifo_watermark_max,
+ __stringify(BMI160_FIFO_LENGTH));
+static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
+ bmi160_get_fifo_state, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
+ bmi160_get_fifo_watermark, NULL, 0);
+
+static const struct attribute *bmi160_fifo_attributes[] = {
+ &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
+ &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+ &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+ &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+ NULL,
+};
+
+static int bmi160_set_watermark(struct iio_dev *indio_dev, unsigned int val)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+
+ if (val > BMI160_FIFO_LENGTH)
+ val = BMI160_FIFO_LENGTH;
+
+ data->watermark = val;
+
+ return 0;
+}
+
+static int bmi160_fifo_transfer(struct bmi160_data *data,
+ char *buffer, int num_bytes)
+{
+ struct regmap *regmap = data->regmap;
+ struct device *dev = regmap_get_device(regmap);
+ size_t step = regmap_get_raw_read_max(regmap);
+ int ret = 0;
+ int i;
+
+ if (!step || step > num_bytes)
+ step = num_bytes;
+ else if (step < num_bytes)
+ step = data->fifo_sample_size;
+
+ for (i = 0; i < num_bytes; i += step) {
+ ret = regmap_raw_read(regmap, BMI160_REG_FIFO_DATA,
+ &buffer[i], step);
+
+ if (ret)
+ break;
+ }
+
+ if (ret)
+ dev_err(dev,
+ "Error transferring data from fifo in single steps of %zu\n",
+ step);
+
+ return ret;
+}
+
+static int __bmi160_fifo_flush(struct iio_dev *indio_dev,
+ unsigned int samples, bool irq)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ struct device *dev = regmap_get_device(regmap);
+ int ret;
+ __le16 fifo_length;
+ unsigned int fifo_samples;
+ unsigned int fifo_bytes;
+ u8 *buffer = data->fifo_buffer;
+ u8 *buffer_iter;
+ int64_t last_timestamp, timestamp;
+ unsigned int last_samples;
+ unsigned int i;
+
+ /* Get the current FIFO length */
+ ret = regmap_bulk_read(regmap, BMI160_REG_FIFO_LENGTH,
+ &fifo_length, sizeof(__le16));
+ if (ret < 0) {
+ dev_err(dev, "Error reading FIFO_LENGTH\n");
+ return ret;
+ }
+
+ fifo_bytes = le16_to_cpu(fifo_length);
+ fifo_samples = fifo_bytes / data->fifo_sample_size;
+
+ if (fifo_bytes % data->fifo_sample_size)
+ dev_warn(dev, "fifo_bytes %u is not dividable by %u\n",
+ fifo_bytes, data->fifo_sample_size);
+
+ if (!fifo_samples)
+ return 0;
+
+ if (samples && fifo_samples > samples) {
+ fifo_samples = samples;
+ fifo_bytes = fifo_samples * data->fifo_sample_size;
+ }
+
+ /*
+ * If we are not called from IRQ, it means that we are flushing data
+ * on demand. In that case we do not have latest timestamp saved in
+ * data->timestamp. Get the time now instead.
+ *
+ * In case of IRQ flush, saved timestamp shows the time when number
+ * of samples configured by watermark were ready. Currently there might
+ * be more samples already.
+ * If we are not called from IRQ, than we are getting the current fifo
+ * length, as we are setting timestamp just after getting it.
+ */
+ if (!irq) {
+ last_timestamp = iio_get_time_ns(indio_dev);
+ last_samples = fifo_samples;
+ } else {
+ last_timestamp = data->timestamp;
+ last_samples = data->watermark;
+ }
+
+ /* Get all measurements */
+ ret = bmi160_fifo_transfer(data, buffer, fifo_bytes);
+ if (ret)
+ return ret;
+
+ /* Handle demux */
+ timestamp = last_timestamp - (last_samples * data->fifo_sample_period);
+ buffer_iter = buffer;
+ for (i = 0; i < fifo_samples; i++) {
+ u8 tmp_buf[indio_dev->scan_bytes];
+
+ memcpy(tmp_buf, buffer_iter, data->fifo_sample_size);
+
+ timestamp += data->fifo_sample_period;
+ iio_push_to_buffers_with_timestamp(indio_dev,
+ tmp_buf,
+ timestamp);
+
+ buffer_iter += data->fifo_sample_size;
+ }
+
+ return fifo_samples;
+}
+
+static int bmi160_fifo_flush(struct iio_dev *indio_dev, unsigned int samples)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = __bmi160_fifo_flush(indio_dev, samples, false);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
static const struct iio_info bmi160_info = {
- .driver_module = THIS_MODULE,
- .read_raw = bmi160_read_raw,
- .write_raw = bmi160_write_raw,
- .attrs = &bmi160_attrs_group,
+ .driver_module = THIS_MODULE,
+ .read_raw = bmi160_read_raw,
+ .write_raw = bmi160_write_raw,
+ .attrs = &bmi160_attrs_group,
+};
+
+static const struct iio_info bmi160_info_fifo = {
+ .driver_module = THIS_MODULE,
+ .read_raw = bmi160_read_raw,
+ .write_raw = bmi160_write_raw,
+ .attrs = &bmi160_attrs_group,
+ .hwfifo_set_watermark = bmi160_set_watermark,
+ .hwfifo_flush_to_buffer = bmi160_fifo_flush,
};
static const char *bmi160_match_acpi_device(struct device *dev)
@@ -561,12 +1059,54 @@ static void bmi160_chip_uninit(struct bmi160_data *data)
bmi160_set_mode(data, BMI160_ACCEL, false);
}
+static int bmi160_enable_irq(struct bmi160_data *data)
+{
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, BMI160_REG_INT_OUT_CTRL,
+ data->irq_data->output_en,
+ data->irq_data->output_en);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static irqreturn_t bmi160_irq_thread_handler(int irq, void *p)
+{
+ struct iio_dev *indio_dev = p;
+ struct bmi160_data *data = iio_priv(indio_dev);
+ struct device *dev = regmap_get_device(data->regmap);
+
+ mutex_lock(&data->mutex);
+ if (data->fifo_enabled)
+ __bmi160_fifo_flush(indio_dev, BMI160_FIFO_LENGTH, true);
+ else
+ dev_warn(dev,
+ "IRQ has been triggered, but FIFO is not enabled.\n");
+ mutex_unlock(&data->mutex);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t bmi160_irq_handler(int irq, void *p)
+{
+ struct iio_dev *indio_dev = p;
+ struct bmi160_data *data = iio_priv(indio_dev);
+
+ data->timestamp = iio_get_time_ns(indio_dev);
+
+ return IRQ_WAKE_THREAD;
+}
+
int bmi160_core_probe(struct device *dev, struct regmap *regmap,
- const char *name, bool use_spi)
+ const char *name, int irq,
+ bool use_spi, bool block_supported)
{
struct iio_dev *indio_dev;
struct bmi160_data *data;
int ret;
+ int irq2;
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
@@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
data = iio_priv(indio_dev);
dev_set_drvdata(dev, indio_dev);
+ data->irq = irq;
data->regmap = regmap;
+ mutex_init(&data->mutex);
+
ret = bmi160_chip_init(data, use_spi);
if (ret < 0)
return ret;
@@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
indio_dev->info = &bmi160_info;
ret = iio_triggered_buffer_setup(indio_dev, NULL,
- bmi160_trigger_handler, NULL);
+ bmi160_trigger_handler,
+ &bmi160_buffer_ops);
if (ret < 0)
goto uninit;
+ if (data->irq > 0) {
+ /* Check which interrupt pin is connected to our board */
+ irq2 = of_irq_get_byname(dev->of_node, "INT2");
+ if (irq2 == data->irq) {
+ dev_dbg(dev, "Using interrupt line INT2\n");
+ data->irq_data = &bmi160_irq2_data;
+ } else {
+ dev_dbg(dev, "Using interrupt line INT1\n");
+ data->irq_data = &bmi160_irq1_data;
+ }
+
+ ret = devm_request_threaded_irq(dev,
+ data->irq,
+ bmi160_irq_handler,
+ bmi160_irq_thread_handler,
+ IRQF_ONESHOT,
+ BMI160_IRQ_NAME,
+ indio_dev);
+ if (ret)
+ return ret;
+
+ ret = bmi160_enable_irq(data);
+ if (ret < 0)
+ goto buffer_cleanup;
+
+ if (block_supported) {
+ indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
+ indio_dev->info = &bmi160_info_fifo;
+ indio_dev->buffer->attrs = bmi160_fifo_attributes;
+ data->fifo_buffer = devm_kmalloc(dev,
+ BMI160_FIFO_LENGTH,
+ GFP_KERNEL);
+ if (!data->fifo_buffer) {
+ ret = -ENOMEM;
+ goto buffer_cleanup;
+ }
+ }
+ }
+
ret = iio_device_register(indio_dev);
if (ret < 0)
goto buffer_cleanup;
diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
index 07a179d..aa63f89 100644
--- a/drivers/iio/imu/bmi160/bmi160_i2c.c
+++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
@@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client *client,
{
struct regmap *regmap;
const char *name = NULL;
+ bool block_supported =
+ i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
+ i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_I2C_BLOCK);
regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
if (IS_ERR(regmap)) {
@@ -34,7 +38,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, client->irq,
+ false, block_supported);
}
static int bmi160_i2c_remove(struct i2c_client *client)
diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
index 1ec8b12..9b57fbe 100644
--- a/drivers/iio/imu/bmi160/bmi160_spi.c
+++ b/drivers/iio/imu/bmi160/bmi160_spi.c
@@ -25,7 +25,8 @@ 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, spi->irq,
+ true, true);
}
static int bmi160_spi_remove(struct spi_device *spi)
--
2.10.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 1/2] iio: bmi160: Support hardware fifo
@ 2016-11-03 11:25 ` Marcin Niestroj
0 siblings, 0 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-03 11:25 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio, devicetree, Marcin Niestroj
This patch was developed primarily based on bmc150_accel hardware fifo
implementation.
IRQ handler was added, which for now is responsible only for handling
watermark interrupts. The BMI160 chip has two interrupt outputs. By
default INT is considered to be connected. If INT2 is used instead, the
interrupt-names device-tree property can be used to specify that.
Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
---
drivers/iio/imu/bmi160/bmi160.h | 3 +-
drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
4 files changed, 618 insertions(+), 28 deletions(-)
diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
index d2ae6ed..4a7c10e 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -4,7 +4,8 @@
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, int irq,
+ bool use_spi, bool block_supported);
void bmi160_core_remove(struct device *dev);
#endif /* BMI160_H_ */
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index e0251b8..153734c 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -2,6 +2,7 @@
* BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
*
* Copyright (c) 2016, Intel Corporation.
+ * Copyright (c) 2016, Grinn
*
* 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
@@ -9,7 +10,7 @@
*
* IIO core driver for BMI160, with support for I2C/SPI busses
*
- * TODO: magnetometer, interrupts, hardware FIFO
+ * TODO: magnetometer, interrupts
*/
#include <linux/module.h>
#include <linux/regmap.h>
@@ -22,8 +23,12 @@
#include <linux/iio/buffer.h>
#include <linux/iio/sysfs.h>
+#include <linux/of_irq.h>
+
#include "bmi160.h"
+#define BMI160_IRQ_NAME "bmi160_event"
+
#define BMI160_REG_CHIP_ID 0x00
#define BMI160_CHIP_ID_VAL 0xD1
@@ -34,6 +39,21 @@
#define BMI160_REG_DATA_GYRO_XOUT_L 0x0C
#define BMI160_REG_DATA_ACCEL_XOUT_L 0x12
+#define BMI160_REG_STATUS 0x1B
+#define BMI160_STATUS_MAG_MAN_OP BIT(2)
+
+#define BMI160_REG_INT_STATUS0 0x1C
+
+#define BMI160_REG_INT_STATUS1 0x1D
+#define BMI160_INT_STATUS_FWM BIT(6)
+
+#define BMI160_REG_INT_STATUS2 0x1E
+
+#define BMI160_REG_INT_STATUS3 0x1F
+
+#define BMI160_REG_FIFO_LENGTH 0x22
+#define BMI160_REG_FIFO_DATA 0x24
+
#define BMI160_REG_ACCEL_CONFIG 0x40
#define BMI160_ACCEL_CONFIG_ODR_MASK GENMASK(3, 0)
#define BMI160_ACCEL_CONFIG_BWP_MASK GENMASK(6, 4)
@@ -55,6 +75,36 @@
#define BMI160_GYRO_RANGE_250DPS 0x03
#define BMI160_GYRO_RANGE_125DPS 0x04
+#define BMI160_REG_FIFO_CONFIG_0 0x46
+
+#define BMI160_REG_FIFO_CONFIG_1 0x47
+#define BMI160_FIFO_GYRO_EN BIT(7)
+#define BMI160_FIFO_ACCEL_EN BIT(6)
+#define BMI160_FIFO_MAGN_EN BIT(5)
+#define BMI160_FIFO_HEADER_EN BIT(4)
+#define BMI160_FIFO_TAG_INT1_EN BIT(3)
+#define BMI160_FIFO_TAG_INT2_EN BIT(2)
+#define BMI160_FIFO_TIME_EN BIT(1)
+
+#define BMI160_REG_INT_EN_1 0x51
+#define BMI160_INT_FWM_EN BIT(6)
+#define BMI160_INT_FFULL_EN BIT(5)
+#define BMI160_INT_DRDY_EN BIT(4)
+
+#define BMI160_REG_INT_OUT_CTRL 0x53
+#define BMI160_INT2_OUTPUT_EN BIT(7)
+#define BMI160_INT1_OUTPUT_EN BIT(3)
+
+#define BMI160_REG_INT_LATCH 0x54
+
+#define BMI160_REG_INT_MAP_1 0x56
+#define BMI160_INT1_MAP_DRDY BIT(7)
+#define BMI160_INT1_MAP_FWM BIT(6)
+#define BMI160_INT1_MAP_FFULL BIT(5)
+#define BMI160_INT2_MAP_DRDY BIT(3)
+#define BMI160_INT2_MAP_FWM BIT(2)
+#define BMI160_INT2_MAP_FFULL BIT(1)
+
#define BMI160_REG_CMD 0x7E
#define BMI160_CMD_ACCEL_PM_SUSPEND 0x10
#define BMI160_CMD_ACCEL_PM_NORMAL 0x11
@@ -66,6 +116,8 @@
#define BMI160_REG_DUMMY 0x7F
+#define BMI160_FIFO_LENGTH 1024
+
#define BMI160_ACCEL_PMU_MIN_USLEEP 3200
#define BMI160_ACCEL_PMU_MAX_USLEEP 3800
#define BMI160_GYRO_PMU_MIN_USLEEP 55000
@@ -110,8 +162,33 @@ enum bmi160_sensor_type {
BMI160_NUM_SENSORS /* must be last */
};
+struct bmi160_irq_data {
+ unsigned int map_fwm;
+ unsigned int output_en;
+};
+
+static const struct bmi160_irq_data bmi160_irq1_data = {
+ .map_fwm = BMI160_INT1_MAP_FWM,
+ .output_en = BMI160_INT1_OUTPUT_EN,
+};
+
+static const struct bmi160_irq_data bmi160_irq2_data = {
+ .map_fwm = BMI160_INT2_MAP_FWM,
+ .output_en = BMI160_INT2_OUTPUT_EN,
+};
+
struct bmi160_data {
struct regmap *regmap;
+ struct mutex mutex;
+ const struct bmi160_irq_data *irq_data;
+ int irq;
+ int64_t timestamp;
+ int64_t fifo_sample_period;
+ bool fifo_enabled;
+ unsigned int fifo_config;
+ unsigned int fifo_sample_size;
+ u8 *fifo_buffer;
+ unsigned int watermark;
};
const struct regmap_config bmi160_regmap_config = {
@@ -159,11 +236,11 @@ struct bmi160_pmu_time {
static struct bmi160_pmu_time bmi160_pmu_time[] = {
[BMI160_ACCEL] = {
.min = BMI160_ACCEL_PMU_MIN_USLEEP,
- .max = BMI160_ACCEL_PMU_MAX_USLEEP
+ .max = BMI160_ACCEL_PMU_MAX_USLEEP,
},
[BMI160_GYRO] = {
.min = BMI160_GYRO_PMU_MIN_USLEEP,
- .max = BMI160_GYRO_PMU_MIN_USLEEP,
+ .max = BMI160_GYRO_PMU_MAX_USLEEP,
},
};
@@ -285,7 +362,9 @@ int bmi160_set_mode(struct bmi160_data *data, enum bmi160_sensor_type t,
else
cmd = bmi160_regs[t].pmu_cmd_suspend;
+ mutex_lock(&data->mutex);
ret = regmap_write(data->regmap, BMI160_REG_CMD, cmd);
+ mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
@@ -298,6 +377,7 @@ static
int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
int uscale)
{
+ int ret;
int i;
for (i = 0; i < bmi160_scale_table[t].num; i++)
@@ -307,8 +387,12 @@ int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
if (i == bmi160_scale_table[t].num)
return -EINVAL;
- return regmap_write(data->regmap, bmi160_regs[t].range,
- bmi160_scale_table[t].tbl[i].bits);
+ mutex_lock(&data->mutex);
+ ret = regmap_write(data->regmap, bmi160_regs[t].range,
+ bmi160_scale_table[t].tbl[i].bits);
+ mutex_unlock(&data->mutex);
+
+ return ret;
}
static
@@ -317,7 +401,9 @@ int bmi160_get_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
{
int i, ret, val;
+ mutex_lock(&data->mutex);
ret = regmap_read(data->regmap, bmi160_regs[t].range, &val);
+ mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
@@ -340,7 +426,9 @@ static int bmi160_get_data(struct bmi160_data *data, int chan_type,
reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(__le16);
+ mutex_lock(&data->mutex);
ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(__le16));
+ mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
@@ -353,6 +441,7 @@ static
int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
int odr, int uodr)
{
+ int ret;
int i;
for (i = 0; i < bmi160_odr_table[t].num; i++)
@@ -363,20 +452,30 @@ int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
if (i >= bmi160_odr_table[t].num)
return -EINVAL;
- return regmap_update_bits(data->regmap,
- bmi160_regs[t].config,
- bmi160_regs[t].config_odr_mask,
- bmi160_odr_table[t].tbl[i].bits);
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap,
+ bmi160_regs[t].config,
+ bmi160_regs[t].config_odr_mask,
+ bmi160_odr_table[t].tbl[i].bits);
+ mutex_unlock(&data->mutex);
+
+ return ret;
}
-static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
- int *odr, int *uodr)
+static int64_t bmi160_frequency_to_period(int odr, int uodr)
{
- int i, val, ret;
+ uint64_t period = 1000000000000000;
+ int64_t frequency = (int64_t) odr * 1000000 + uodr;
- ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
- if (ret < 0)
- return ret;
+ do_div(period, frequency);
+
+ return period;
+}
+
+static const struct bmi160_odr *bmi160_reg_to_odr(enum bmi160_sensor_type t,
+ unsigned int val)
+{
+ int i;
val &= bmi160_regs[t].config_odr_mask;
@@ -385,10 +484,52 @@ static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
break;
if (i >= bmi160_odr_table[t].num)
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
+
+ return &bmi160_odr_table[t].tbl[i];
+}
+
+static int bmi160_get_sample_period(struct bmi160_data *data,
+ enum bmi160_sensor_type t,
+ int64_t *sample_period)
+{
+ const struct bmi160_odr *odr_entry;
+ int ret;
+ unsigned int val;
+
+ ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
+ if (ret < 0)
+ return ret;
- *odr = bmi160_odr_table[t].tbl[i].odr;
- *uodr = bmi160_odr_table[t].tbl[i].uodr;
+ odr_entry = bmi160_reg_to_odr(t, val);
+ if (IS_ERR(odr_entry))
+ return PTR_ERR(odr_entry);
+
+ *sample_period = bmi160_frequency_to_period(odr_entry->odr,
+ odr_entry->uodr);
+
+ return 0;
+}
+
+static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
+ int *odr, int *uodr)
+{
+ const struct bmi160_odr *odr_entry;
+ int ret;
+ unsigned int val;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
+ mutex_unlock(&data->mutex);
+ if (ret < 0)
+ return ret;
+
+ odr_entry = bmi160_reg_to_odr(t, val);
+ if (IS_ERR(odr_entry))
+ return PTR_ERR(odr_entry);
+
+ *odr = odr_entry->odr;
+ *uodr = odr_entry->uodr;
return 0;
}
@@ -402,14 +543,18 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
__le16 sample;
+ mutex_lock(&data->mutex);
for_each_set_bit(i, indio_dev->active_scan_mask,
indio_dev->masklength) {
ret = regmap_bulk_read(data->regmap, base + i * sizeof(__le16),
&sample, sizeof(__le16));
- if (ret < 0)
+ if (ret < 0) {
+ mutex_unlock(&data->mutex);
goto done;
+ }
buf[j++] = sample;
}
+ mutex_unlock(&data->mutex);
iio_push_to_buffers_with_timestamp(indio_dev, buf,
iio_get_time_ns(indio_dev));
@@ -493,11 +638,364 @@ static const struct attribute_group bmi160_attrs_group = {
.attrs = bmi160_attrs,
};
+static int bmi160_update_sample_period(struct bmi160_data *data,
+ enum bmi160_sensor_type sensor_type)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int64_t sample_period;
+ int ret;
+
+ ret = bmi160_get_sample_period(data, sensor_type, &sample_period);
+ if (ret < 0)
+ return ret;
+
+ if (data->fifo_sample_period) {
+ if (data->fifo_sample_period != sample_period) {
+ dev_warn(dev, "Enabled sensors have unequal ODR values\n");
+ return -EINVAL;
+ }
+ } else {
+ data->fifo_sample_period = sample_period;
+ }
+
+ return 0;
+}
+
+static int bmi160_fifo_enable(struct iio_dev *indio_dev,
+ struct bmi160_data *data)
+{
+ struct regmap *regmap = data->regmap;
+ struct device *dev = regmap_get_device(regmap);
+ int ret;
+ int i;
+ unsigned int val;
+ unsigned int fifo_config = 0;
+
+ /* Set fifo sample size and period */
+ for_each_set_bit(i, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ if (i <= BMI160_SCAN_GYRO_Z)
+ fifo_config |= BMI160_FIFO_GYRO_EN;
+ else if (i <= BMI160_SCAN_ACCEL_Z)
+ fifo_config |= BMI160_FIFO_ACCEL_EN;
+ }
+
+ data->fifo_sample_period = 0;
+ data->fifo_sample_size = 0;
+ if (fifo_config & BMI160_FIFO_GYRO_EN) {
+ data->fifo_sample_size += 6;
+ ret = bmi160_update_sample_period(data, BMI160_GYRO);
+ if (ret < 0)
+ return ret;
+ }
+ if (fifo_config & BMI160_FIFO_ACCEL_EN) {
+ data->fifo_sample_size += 6;
+ ret = bmi160_update_sample_period(data, BMI160_ACCEL);
+ if (ret < 0)
+ return ret;
+ }
+
+ /*
+ * Set watermark level and write real value back, as it will be used
+ * in timestamp calculation.
+ */
+ val = data->watermark * data->fifo_sample_size;
+ if (val > BMI160_FIFO_LENGTH - 1) {
+ val = BMI160_FIFO_LENGTH - 1;
+ data->watermark = val / data->fifo_sample_size;
+ }
+ val = data->watermark * data->fifo_sample_size / 4;
+
+ ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_0, val);
+ if (ret < 0) {
+ dev_err(dev, "Failed to set watermark\n");
+ return ret;
+ }
+
+ /* Enable FIFO channels */
+ ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1,
+ fifo_config);
+ if (ret < 0) {
+ dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
+ return ret;
+ }
+
+ data->fifo_config = fifo_config;
+ data->fifo_enabled = true;
+
+ return 0;
+}
+
+static int bmi160_fifo_disable(struct bmi160_data *data)
+{
+ struct regmap *regmap = data->regmap;
+ struct device *dev = regmap_get_device(regmap);
+ int ret;
+
+ /* Disable all FIFO channels */
+ ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1, 0);
+ if (ret < 0) {
+ dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
+ return ret;
+ }
+
+ data->fifo_enabled = false;
+
+ return 0;
+}
+
+static int bmi160_buffer_postenable(struct iio_dev *indio_dev)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+ int ret;
+
+ if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+ return iio_triggered_buffer_postenable(indio_dev);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, BMI160_REG_INT_MAP_1,
+ data->irq_data->map_fwm, data->irq_data->map_fwm);
+ if (ret < 0)
+ goto unlock;
+
+ ret = regmap_update_bits(data->regmap, BMI160_REG_INT_EN_1,
+ BMI160_INT_FWM_EN, BMI160_INT_FWM_EN);
+ if (ret < 0)
+ goto unlock;
+
+ ret = bmi160_fifo_enable(indio_dev, data);
+
+unlock:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bmi160_buffer_predisable(struct iio_dev *indio_dev)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret = 0;
+
+ if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+ return iio_triggered_buffer_predisable(indio_dev);
+
+ mutex_lock(&data->mutex);
+
+ ret = regmap_update_bits(regmap, BMI160_REG_INT_EN_1,
+ BMI160_INT_FWM_EN, 0);
+ if (ret < 0)
+ goto unlock;
+
+ ret = bmi160_fifo_disable(data);
+
+unlock:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static const struct iio_buffer_setup_ops bmi160_buffer_ops = {
+ .postenable = bmi160_buffer_postenable,
+ .predisable = bmi160_buffer_predisable,
+};
+
+static ssize_t bmi160_get_fifo_state(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct bmi160_data *data = iio_priv(indio_dev);
+ bool state;
+
+ mutex_lock(&data->mutex);
+ state = data->fifo_enabled;
+ mutex_unlock(&data->mutex);
+
+ return sprintf(buf, "%d\n", (int) state);
+}
+
+static ssize_t bmi160_get_fifo_watermark(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct bmi160_data *data = iio_priv(indio_dev);
+ int wm;
+
+ mutex_lock(&data->mutex);
+ wm = data->watermark;
+ mutex_unlock(&data->mutex);
+
+ return sprintf(buf, "%d\n", wm);
+}
+
+static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
+static IIO_CONST_ATTR(hwfifo_watermark_max,
+ __stringify(BMI160_FIFO_LENGTH));
+static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
+ bmi160_get_fifo_state, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
+ bmi160_get_fifo_watermark, NULL, 0);
+
+static const struct attribute *bmi160_fifo_attributes[] = {
+ &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
+ &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+ &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+ &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+ NULL,
+};
+
+static int bmi160_set_watermark(struct iio_dev *indio_dev, unsigned int val)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+
+ if (val > BMI160_FIFO_LENGTH)
+ val = BMI160_FIFO_LENGTH;
+
+ data->watermark = val;
+
+ return 0;
+}
+
+static int bmi160_fifo_transfer(struct bmi160_data *data,
+ char *buffer, int num_bytes)
+{
+ struct regmap *regmap = data->regmap;
+ struct device *dev = regmap_get_device(regmap);
+ size_t step = regmap_get_raw_read_max(regmap);
+ int ret = 0;
+ int i;
+
+ if (!step || step > num_bytes)
+ step = num_bytes;
+ else if (step < num_bytes)
+ step = data->fifo_sample_size;
+
+ for (i = 0; i < num_bytes; i += step) {
+ ret = regmap_raw_read(regmap, BMI160_REG_FIFO_DATA,
+ &buffer[i], step);
+
+ if (ret)
+ break;
+ }
+
+ if (ret)
+ dev_err(dev,
+ "Error transferring data from fifo in single steps of %zu\n",
+ step);
+
+ return ret;
+}
+
+static int __bmi160_fifo_flush(struct iio_dev *indio_dev,
+ unsigned int samples, bool irq)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ struct device *dev = regmap_get_device(regmap);
+ int ret;
+ __le16 fifo_length;
+ unsigned int fifo_samples;
+ unsigned int fifo_bytes;
+ u8 *buffer = data->fifo_buffer;
+ u8 *buffer_iter;
+ int64_t last_timestamp, timestamp;
+ unsigned int last_samples;
+ unsigned int i;
+
+ /* Get the current FIFO length */
+ ret = regmap_bulk_read(regmap, BMI160_REG_FIFO_LENGTH,
+ &fifo_length, sizeof(__le16));
+ if (ret < 0) {
+ dev_err(dev, "Error reading FIFO_LENGTH\n");
+ return ret;
+ }
+
+ fifo_bytes = le16_to_cpu(fifo_length);
+ fifo_samples = fifo_bytes / data->fifo_sample_size;
+
+ if (fifo_bytes % data->fifo_sample_size)
+ dev_warn(dev, "fifo_bytes %u is not dividable by %u\n",
+ fifo_bytes, data->fifo_sample_size);
+
+ if (!fifo_samples)
+ return 0;
+
+ if (samples && fifo_samples > samples) {
+ fifo_samples = samples;
+ fifo_bytes = fifo_samples * data->fifo_sample_size;
+ }
+
+ /*
+ * If we are not called from IRQ, it means that we are flushing data
+ * on demand. In that case we do not have latest timestamp saved in
+ * data->timestamp. Get the time now instead.
+ *
+ * In case of IRQ flush, saved timestamp shows the time when number
+ * of samples configured by watermark were ready. Currently there might
+ * be more samples already.
+ * If we are not called from IRQ, than we are getting the current fifo
+ * length, as we are setting timestamp just after getting it.
+ */
+ if (!irq) {
+ last_timestamp = iio_get_time_ns(indio_dev);
+ last_samples = fifo_samples;
+ } else {
+ last_timestamp = data->timestamp;
+ last_samples = data->watermark;
+ }
+
+ /* Get all measurements */
+ ret = bmi160_fifo_transfer(data, buffer, fifo_bytes);
+ if (ret)
+ return ret;
+
+ /* Handle demux */
+ timestamp = last_timestamp - (last_samples * data->fifo_sample_period);
+ buffer_iter = buffer;
+ for (i = 0; i < fifo_samples; i++) {
+ u8 tmp_buf[indio_dev->scan_bytes];
+
+ memcpy(tmp_buf, buffer_iter, data->fifo_sample_size);
+
+ timestamp += data->fifo_sample_period;
+ iio_push_to_buffers_with_timestamp(indio_dev,
+ tmp_buf,
+ timestamp);
+
+ buffer_iter += data->fifo_sample_size;
+ }
+
+ return fifo_samples;
+}
+
+static int bmi160_fifo_flush(struct iio_dev *indio_dev, unsigned int samples)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = __bmi160_fifo_flush(indio_dev, samples, false);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
static const struct iio_info bmi160_info = {
- .driver_module = THIS_MODULE,
- .read_raw = bmi160_read_raw,
- .write_raw = bmi160_write_raw,
- .attrs = &bmi160_attrs_group,
+ .driver_module = THIS_MODULE,
+ .read_raw = bmi160_read_raw,
+ .write_raw = bmi160_write_raw,
+ .attrs = &bmi160_attrs_group,
+};
+
+static const struct iio_info bmi160_info_fifo = {
+ .driver_module = THIS_MODULE,
+ .read_raw = bmi160_read_raw,
+ .write_raw = bmi160_write_raw,
+ .attrs = &bmi160_attrs_group,
+ .hwfifo_set_watermark = bmi160_set_watermark,
+ .hwfifo_flush_to_buffer = bmi160_fifo_flush,
};
static const char *bmi160_match_acpi_device(struct device *dev)
@@ -561,12 +1059,54 @@ static void bmi160_chip_uninit(struct bmi160_data *data)
bmi160_set_mode(data, BMI160_ACCEL, false);
}
+static int bmi160_enable_irq(struct bmi160_data *data)
+{
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, BMI160_REG_INT_OUT_CTRL,
+ data->irq_data->output_en,
+ data->irq_data->output_en);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static irqreturn_t bmi160_irq_thread_handler(int irq, void *p)
+{
+ struct iio_dev *indio_dev = p;
+ struct bmi160_data *data = iio_priv(indio_dev);
+ struct device *dev = regmap_get_device(data->regmap);
+
+ mutex_lock(&data->mutex);
+ if (data->fifo_enabled)
+ __bmi160_fifo_flush(indio_dev, BMI160_FIFO_LENGTH, true);
+ else
+ dev_warn(dev,
+ "IRQ has been triggered, but FIFO is not enabled.\n");
+ mutex_unlock(&data->mutex);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t bmi160_irq_handler(int irq, void *p)
+{
+ struct iio_dev *indio_dev = p;
+ struct bmi160_data *data = iio_priv(indio_dev);
+
+ data->timestamp = iio_get_time_ns(indio_dev);
+
+ return IRQ_WAKE_THREAD;
+}
+
int bmi160_core_probe(struct device *dev, struct regmap *regmap,
- const char *name, bool use_spi)
+ const char *name, int irq,
+ bool use_spi, bool block_supported)
{
struct iio_dev *indio_dev;
struct bmi160_data *data;
int ret;
+ int irq2;
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
@@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
data = iio_priv(indio_dev);
dev_set_drvdata(dev, indio_dev);
+ data->irq = irq;
data->regmap = regmap;
+ mutex_init(&data->mutex);
+
ret = bmi160_chip_init(data, use_spi);
if (ret < 0)
return ret;
@@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
indio_dev->info = &bmi160_info;
ret = iio_triggered_buffer_setup(indio_dev, NULL,
- bmi160_trigger_handler, NULL);
+ bmi160_trigger_handler,
+ &bmi160_buffer_ops);
if (ret < 0)
goto uninit;
+ if (data->irq > 0) {
+ /* Check which interrupt pin is connected to our board */
+ irq2 = of_irq_get_byname(dev->of_node, "INT2");
+ if (irq2 == data->irq) {
+ dev_dbg(dev, "Using interrupt line INT2\n");
+ data->irq_data = &bmi160_irq2_data;
+ } else {
+ dev_dbg(dev, "Using interrupt line INT1\n");
+ data->irq_data = &bmi160_irq1_data;
+ }
+
+ ret = devm_request_threaded_irq(dev,
+ data->irq,
+ bmi160_irq_handler,
+ bmi160_irq_thread_handler,
+ IRQF_ONESHOT,
+ BMI160_IRQ_NAME,
+ indio_dev);
+ if (ret)
+ return ret;
+
+ ret = bmi160_enable_irq(data);
+ if (ret < 0)
+ goto buffer_cleanup;
+
+ if (block_supported) {
+ indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
+ indio_dev->info = &bmi160_info_fifo;
+ indio_dev->buffer->attrs = bmi160_fifo_attributes;
+ data->fifo_buffer = devm_kmalloc(dev,
+ BMI160_FIFO_LENGTH,
+ GFP_KERNEL);
+ if (!data->fifo_buffer) {
+ ret = -ENOMEM;
+ goto buffer_cleanup;
+ }
+ }
+ }
+
ret = iio_device_register(indio_dev);
if (ret < 0)
goto buffer_cleanup;
diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
index 07a179d..aa63f89 100644
--- a/drivers/iio/imu/bmi160/bmi160_i2c.c
+++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
@@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client *client,
{
struct regmap *regmap;
const char *name = NULL;
+ bool block_supported =
+ i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
+ i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_I2C_BLOCK);
regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
if (IS_ERR(regmap)) {
@@ -34,7 +38,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, client->irq,
+ false, block_supported);
}
static int bmi160_i2c_remove(struct i2c_client *client)
diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
index 1ec8b12..9b57fbe 100644
--- a/drivers/iio/imu/bmi160/bmi160_spi.c
+++ b/drivers/iio/imu/bmi160/bmi160_spi.c
@@ -25,7 +25,8 @@ 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, spi->irq,
+ true, true);
}
static int bmi160_spi_remove(struct spi_device *spi)
--
2.10.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
2016-11-03 11:25 ` Marcin Niestroj
@ 2016-11-03 11:25 ` Marcin Niestroj
-1 siblings, 0 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-03 11:25 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Marcin Niestroj
This adds documentation for Bosch BMI160 Inertial Measurement Unit
device-tree bindings.
Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
---
.../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
new file mode 100644
index 0000000..b02ef3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
@@ -0,0 +1,34 @@
+Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
+and externally connectable Magnetometer
+
+https://www.bosch-sensortec.com/bst/products/all_products/bmi160
+
+Required properties:
+ - compatible : should be "bosch,bmi160"
+ - reg : the I2C address or SPI chip select number of the sensor
+ - spi-max-frequency : set maximum clock frequency (only for SPI)
+
+Optional properties:
+ - interrupt-parent : should be the phandle of the interrupt controller
+ - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
+ - interrupt-names : set to "INT2" if using INT2 pin
+
+Examples:
+
+bmi160@68 {
+ compatible = "bosch,bmi160";
+ reg = <0x68>;
+
+ interrupt-parent = <&gpio4>;
+ interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+};
+
+bmi160@0 {
+ compatible = "bosch,bmi160";
+ reg = <0>;
+ spi-max-frequency = <10000000>;
+
+ interrupt-parent = <&gpio2>;
+ interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-names = "INT2";
+};
--
2.10.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
@ 2016-11-03 11:25 ` Marcin Niestroj
0 siblings, 0 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-03 11:25 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio, devicetree, Marcin Niestroj
This adds documentation for Bosch BMI160 Inertial Measurement Unit
device-tree bindings.
Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
---
.../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
new file mode 100644
index 0000000..b02ef3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
@@ -0,0 +1,34 @@
+Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
+and externally connectable Magnetometer
+
+https://www.bosch-sensortec.com/bst/products/all_products/bmi160
+
+Required properties:
+ - compatible : should be "bosch,bmi160"
+ - reg : the I2C address or SPI chip select number of the sensor
+ - spi-max-frequency : set maximum clock frequency (only for SPI)
+
+Optional properties:
+ - interrupt-parent : should be the phandle of the interrupt controller
+ - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
+ - interrupt-names : set to "INT2" if using INT2 pin
+
+Examples:
+
+bmi160@68 {
+ compatible = "bosch,bmi160";
+ reg = <0x68>;
+
+ interrupt-parent = <&gpio4>;
+ interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+};
+
+bmi160@0 {
+ compatible = "bosch,bmi160";
+ reg = <0>;
+ spi-max-frequency = <10000000>;
+
+ interrupt-parent = <&gpio2>;
+ interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-names = "INT2";
+};
--
2.10.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-03 11:25 ` Marcin Niestroj
(?)
@ 2016-11-03 12:09 ` Peter Meerwald-Stadler
2016-11-09 14:16 ` Marcin Niestroj
-1 siblings, 1 reply; 25+ messages in thread
From: Peter Meerwald-Stadler @ 2016-11-03 12:09 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
linux-iio
> This patch was developed primarily based on bmc150_accel hardware fifo
> implementation.
parts of the patch are cleanup and bugfixing; should be separate?
more comments below
> IRQ handler was added, which for now is responsible only for handling
> watermark interrupts. The BMI160 chip has two interrupt outputs. By
> default INT is considered to be connected. If INT2 is used instead, the
> interrupt-names device-tree property can be used to specify that.
>
> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
> ---
> drivers/iio/imu/bmi160/bmi160.h | 3 +-
> drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
> drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
> drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
> 4 files changed, 618 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index d2ae6ed..4a7c10e 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -4,7 +4,8 @@
> 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, int irq,
> + bool use_spi, bool block_supported);
> void bmi160_core_remove(struct device *dev);
>
> #endif /* BMI160_H_ */
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index e0251b8..153734c 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -2,6 +2,7 @@
> * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
> *
> * Copyright (c) 2016, Intel Corporation.
> + * Copyright (c) 2016, Grinn
> *
> * 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
> @@ -9,7 +10,7 @@
> *
> * IIO core driver for BMI160, with support for I2C/SPI busses
> *
> - * TODO: magnetometer, interrupts, hardware FIFO
> + * TODO: magnetometer, interrupts
> */
> #include <linux/module.h>
> #include <linux/regmap.h>
> @@ -22,8 +23,12 @@
> #include <linux/iio/buffer.h>
> #include <linux/iio/sysfs.h>
>
> +#include <linux/of_irq.h>
> +
> #include "bmi160.h"
>
> +#define BMI160_IRQ_NAME "bmi160_event"
> +
> #define BMI160_REG_CHIP_ID 0x00
> #define BMI160_CHIP_ID_VAL 0xD1
>
> @@ -34,6 +39,21 @@
> #define BMI160_REG_DATA_GYRO_XOUT_L 0x0C
> #define BMI160_REG_DATA_ACCEL_XOUT_L 0x12
>
> +#define BMI160_REG_STATUS 0x1B
> +#define BMI160_STATUS_MAG_MAN_OP BIT(2)
> +
> +#define BMI160_REG_INT_STATUS0 0x1C
> +
> +#define BMI160_REG_INT_STATUS1 0x1D
> +#define BMI160_INT_STATUS_FWM BIT(6)
> +
> +#define BMI160_REG_INT_STATUS2 0x1E
> +
> +#define BMI160_REG_INT_STATUS3 0x1F
> +
> +#define BMI160_REG_FIFO_LENGTH 0x22
> +#define BMI160_REG_FIFO_DATA 0x24
> +
> #define BMI160_REG_ACCEL_CONFIG 0x40
> #define BMI160_ACCEL_CONFIG_ODR_MASK GENMASK(3, 0)
> #define BMI160_ACCEL_CONFIG_BWP_MASK GENMASK(6, 4)
> @@ -55,6 +75,36 @@
> #define BMI160_GYRO_RANGE_250DPS 0x03
> #define BMI160_GYRO_RANGE_125DPS 0x04
>
> +#define BMI160_REG_FIFO_CONFIG_0 0x46
> +
> +#define BMI160_REG_FIFO_CONFIG_1 0x47
> +#define BMI160_FIFO_GYRO_EN BIT(7)
> +#define BMI160_FIFO_ACCEL_EN BIT(6)
> +#define BMI160_FIFO_MAGN_EN BIT(5)
> +#define BMI160_FIFO_HEADER_EN BIT(4)
> +#define BMI160_FIFO_TAG_INT1_EN BIT(3)
> +#define BMI160_FIFO_TAG_INT2_EN BIT(2)
> +#define BMI160_FIFO_TIME_EN BIT(1)
> +
> +#define BMI160_REG_INT_EN_1 0x51
> +#define BMI160_INT_FWM_EN BIT(6)
> +#define BMI160_INT_FFULL_EN BIT(5)
> +#define BMI160_INT_DRDY_EN BIT(4)
> +
> +#define BMI160_REG_INT_OUT_CTRL 0x53
> +#define BMI160_INT2_OUTPUT_EN BIT(7)
> +#define BMI160_INT1_OUTPUT_EN BIT(3)
> +
> +#define BMI160_REG_INT_LATCH 0x54
> +
> +#define BMI160_REG_INT_MAP_1 0x56
> +#define BMI160_INT1_MAP_DRDY BIT(7)
> +#define BMI160_INT1_MAP_FWM BIT(6)
> +#define BMI160_INT1_MAP_FFULL BIT(5)
> +#define BMI160_INT2_MAP_DRDY BIT(3)
> +#define BMI160_INT2_MAP_FWM BIT(2)
> +#define BMI160_INT2_MAP_FFULL BIT(1)
> +
> #define BMI160_REG_CMD 0x7E
> #define BMI160_CMD_ACCEL_PM_SUSPEND 0x10
> #define BMI160_CMD_ACCEL_PM_NORMAL 0x11
> @@ -66,6 +116,8 @@
>
> #define BMI160_REG_DUMMY 0x7F
>
> +#define BMI160_FIFO_LENGTH 1024
> +
> #define BMI160_ACCEL_PMU_MIN_USLEEP 3200
> #define BMI160_ACCEL_PMU_MAX_USLEEP 3800
> #define BMI160_GYRO_PMU_MIN_USLEEP 55000
> @@ -110,8 +162,33 @@ enum bmi160_sensor_type {
> BMI160_NUM_SENSORS /* must be last */
> };
>
> +struct bmi160_irq_data {
> + unsigned int map_fwm;
> + unsigned int output_en;
> +};
> +
> +static const struct bmi160_irq_data bmi160_irq1_data = {
> + .map_fwm = BMI160_INT1_MAP_FWM,
> + .output_en = BMI160_INT1_OUTPUT_EN,
> +};
> +
> +static const struct bmi160_irq_data bmi160_irq2_data = {
> + .map_fwm = BMI160_INT2_MAP_FWM,
> + .output_en = BMI160_INT2_OUTPUT_EN,
> +};
> +
> struct bmi160_data {
> struct regmap *regmap;
> + struct mutex mutex;
> + const struct bmi160_irq_data *irq_data;
> + int irq;
> + int64_t timestamp;
> + int64_t fifo_sample_period;
> + bool fifo_enabled;
> + unsigned int fifo_config;
> + unsigned int fifo_sample_size;
> + u8 *fifo_buffer;
> + unsigned int watermark;
> };
>
> const struct regmap_config bmi160_regmap_config = {
> @@ -159,11 +236,11 @@ struct bmi160_pmu_time {
> static struct bmi160_pmu_time bmi160_pmu_time[] = {
> [BMI160_ACCEL] = {
> .min = BMI160_ACCEL_PMU_MIN_USLEEP,
> - .max = BMI160_ACCEL_PMU_MAX_USLEEP
> + .max = BMI160_ACCEL_PMU_MAX_USLEEP,
this is cosmetic cleanup
> },
> [BMI160_GYRO] = {
> .min = BMI160_GYRO_PMU_MIN_USLEEP,
> - .max = BMI160_GYRO_PMU_MIN_USLEEP,
this looks like bug fixing
> + .max = BMI160_GYRO_PMU_MAX_USLEEP,
> },
> };
>
> @@ -285,7 +362,9 @@ int bmi160_set_mode(struct bmi160_data *data, enum bmi160_sensor_type t,
> else
> cmd = bmi160_regs[t].pmu_cmd_suspend;
>
> + mutex_lock(&data->mutex);
what does the mutex protect?
is it also needed without the fifo/irq support?
probably split out as a separate patch
> ret = regmap_write(data->regmap, BMI160_REG_CMD, cmd);
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -298,6 +377,7 @@ static
> int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> int uscale)
> {
> + int ret;
> int i;
>
> for (i = 0; i < bmi160_scale_table[t].num; i++)
> @@ -307,8 +387,12 @@ int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> if (i == bmi160_scale_table[t].num)
> return -EINVAL;
>
> - return regmap_write(data->regmap, bmi160_regs[t].range,
> - bmi160_scale_table[t].tbl[i].bits);
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, bmi160_regs[t].range,
> + bmi160_scale_table[t].tbl[i].bits);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> }
>
> static
> @@ -317,7 +401,9 @@ int bmi160_get_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> {
> int i, ret, val;
>
> + mutex_lock(&data->mutex);
> ret = regmap_read(data->regmap, bmi160_regs[t].range, &val);
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -340,7 +426,9 @@ static int bmi160_get_data(struct bmi160_data *data, int chan_type,
>
> reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(__le16);
>
> + mutex_lock(&data->mutex);
> ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(__le16));
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -353,6 +441,7 @@ static
> int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> int odr, int uodr)
> {
> + int ret;
> int i;
>
> for (i = 0; i < bmi160_odr_table[t].num; i++)
> @@ -363,20 +452,30 @@ int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> if (i >= bmi160_odr_table[t].num)
> return -EINVAL;
>
> - return regmap_update_bits(data->regmap,
> - bmi160_regs[t].config,
> - bmi160_regs[t].config_odr_mask,
> - bmi160_odr_table[t].tbl[i].bits);
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap,
> + bmi160_regs[t].config,
> + bmi160_regs[t].config_odr_mask,
> + bmi160_odr_table[t].tbl[i].bits);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> }
>
> -static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> - int *odr, int *uodr)
> +static int64_t bmi160_frequency_to_period(int odr, int uodr)
> {
> - int i, val, ret;
> + uint64_t period = 1000000000000000;
> + int64_t frequency = (int64_t) odr * 1000000 + uodr;
>
> - ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> - if (ret < 0)
> - return ret;
> + do_div(period, frequency);
> +
> + return period;
> +}
> +
> +static const struct bmi160_odr *bmi160_reg_to_odr(enum bmi160_sensor_type t,
> + unsigned int val)
> +{
> + int i;
>
> val &= bmi160_regs[t].config_odr_mask;
>
> @@ -385,10 +484,52 @@ static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> break;
>
> if (i >= bmi160_odr_table[t].num)
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
> +
> + return &bmi160_odr_table[t].tbl[i];
> +}
> +
> +static int bmi160_get_sample_period(struct bmi160_data *data,
> + enum bmi160_sensor_type t,
> + int64_t *sample_period)
> +{
> + const struct bmi160_odr *odr_entry;
> + int ret;
> + unsigned int val;
> +
no mutex here?
> + ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> + if (ret < 0)
> + return ret;
>
> - *odr = bmi160_odr_table[t].tbl[i].odr;
> - *uodr = bmi160_odr_table[t].tbl[i].uodr;
> + odr_entry = bmi160_reg_to_odr(t, val);
> + if (IS_ERR(odr_entry))
> + return PTR_ERR(odr_entry);
> +
> + *sample_period = bmi160_frequency_to_period(odr_entry->odr,
> + odr_entry->uodr);
> +
> + return 0;
> +}
> +
> +static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> + int *odr, int *uodr)
> +{
> + const struct bmi160_odr *odr_entry;
> + int ret;
> + unsigned int val;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> + mutex_unlock(&data->mutex);
> + if (ret < 0)
> + return ret;
> +
> + odr_entry = bmi160_reg_to_odr(t, val);
> + if (IS_ERR(odr_entry))
> + return PTR_ERR(odr_entry);
> +
> + *odr = odr_entry->odr;
> + *uodr = odr_entry->uodr;
>
> return 0;
> }
> @@ -402,14 +543,18 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
> int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
> __le16 sample;
>
> + mutex_lock(&data->mutex);
> for_each_set_bit(i, indio_dev->active_scan_mask,
> indio_dev->masklength) {
> ret = regmap_bulk_read(data->regmap, base + i * sizeof(__le16),
> &sample, sizeof(__le16));
> - if (ret < 0)
> + if (ret < 0) {
> + mutex_unlock(&data->mutex);
> goto done;
> + }
> buf[j++] = sample;
> }
> + mutex_unlock(&data->mutex);
>
> iio_push_to_buffers_with_timestamp(indio_dev, buf,
> iio_get_time_ns(indio_dev));
> @@ -493,11 +638,364 @@ static const struct attribute_group bmi160_attrs_group = {
> .attrs = bmi160_attrs,
> };
>
> +static int bmi160_update_sample_period(struct bmi160_data *data,
> + enum bmi160_sensor_type sensor_type)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int64_t sample_period;
> + int ret;
> +
> + ret = bmi160_get_sample_period(data, sensor_type, &sample_period);
> + if (ret < 0)
> + return ret;
> +
> + if (data->fifo_sample_period) {
> + if (data->fifo_sample_period != sample_period) {
> + dev_warn(dev, "Enabled sensors have unequal ODR values\n");
> + return -EINVAL;
> + }
> + } else {
> + data->fifo_sample_period = sample_period;
brackets not needed
> + }
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_enable(struct iio_dev *indio_dev,
> + struct bmi160_data *data)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> + int i;
> + unsigned int val;
> + unsigned int fifo_config = 0;
> +
> + /* Set fifo sample size and period */
> + for_each_set_bit(i, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + if (i <= BMI160_SCAN_GYRO_Z)
> + fifo_config |= BMI160_FIFO_GYRO_EN;
> + else if (i <= BMI160_SCAN_ACCEL_Z)
> + fifo_config |= BMI160_FIFO_ACCEL_EN;
> + }
> +
> + data->fifo_sample_period = 0;
> + data->fifo_sample_size = 0;
> + if (fifo_config & BMI160_FIFO_GYRO_EN) {
> + data->fifo_sample_size += 6;
> + ret = bmi160_update_sample_period(data, BMI160_GYRO);
> + if (ret < 0)
> + return ret;
> + }
> + if (fifo_config & BMI160_FIFO_ACCEL_EN) {
> + data->fifo_sample_size += 6;
> + ret = bmi160_update_sample_period(data, BMI160_ACCEL);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /*
> + * Set watermark level and write real value back, as it will be used
> + * in timestamp calculation.
> + */
> + val = data->watermark * data->fifo_sample_size;
> + if (val > BMI160_FIFO_LENGTH - 1) {
> + val = BMI160_FIFO_LENGTH - 1;
> + data->watermark = val / data->fifo_sample_size;
> + }
> + val = data->watermark * data->fifo_sample_size / 4;
> +
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_0, val);
> + if (ret < 0) {
> + dev_err(dev, "Failed to set watermark\n");
> + return ret;
> + }
> +
> + /* Enable FIFO channels */
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1,
> + fifo_config);
> + if (ret < 0) {
> + dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
> + return ret;
> + }
> +
> + data->fifo_config = fifo_config;
> + data->fifo_enabled = true;
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_disable(struct bmi160_data *data)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> +
> + /* Disable all FIFO channels */
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1, 0);
> + if (ret < 0) {
> + dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
> + return ret;
> + }
> +
> + data->fifo_enabled = false;
> +
> + return 0;
> +}
> +
> +static int bmi160_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> + return iio_triggered_buffer_postenable(indio_dev);
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_MAP_1,
> + data->irq_data->map_fwm, data->irq_data->map_fwm);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_EN_1,
> + BMI160_INT_FWM_EN, BMI160_INT_FWM_EN);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = bmi160_fifo_enable(indio_dev, data);
> +
> +unlock:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static int bmi160_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct regmap *regmap = data->regmap;
> + int ret = 0;
> +
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> + return iio_triggered_buffer_predisable(indio_dev);
> +
> + mutex_lock(&data->mutex);
> +
> + ret = regmap_update_bits(regmap, BMI160_REG_INT_EN_1,
> + BMI160_INT_FWM_EN, 0);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = bmi160_fifo_disable(data);
> +
> +unlock:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops bmi160_buffer_ops = {
> + .postenable = bmi160_buffer_postenable,
> + .predisable = bmi160_buffer_predisable,
> +};
> +
> +static ssize_t bmi160_get_fifo_state(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct bmi160_data *data = iio_priv(indio_dev);
> + bool state;
> +
> + mutex_lock(&data->mutex);
> + state = data->fifo_enabled;
> + mutex_unlock(&data->mutex);
> +
> + return sprintf(buf, "%d\n", (int) state);
> +}
> +
> +static ssize_t bmi160_get_fifo_watermark(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int wm;
> +
> + mutex_lock(&data->mutex);
> + wm = data->watermark;
> + mutex_unlock(&data->mutex);
> +
> + return sprintf(buf, "%d\n", wm);
> +}
> +
> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
> +static IIO_CONST_ATTR(hwfifo_watermark_max,
> + __stringify(BMI160_FIFO_LENGTH));
> +static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
> + bmi160_get_fifo_state, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
> + bmi160_get_fifo_watermark, NULL, 0);
> +
> +static const struct attribute *bmi160_fifo_attributes[] = {
> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> + NULL,
> +};
> +
> +static int bmi160_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> +
> + if (val > BMI160_FIFO_LENGTH)
> + val = BMI160_FIFO_LENGTH;
> +
> + data->watermark = val;
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_transfer(struct bmi160_data *data,
> + char *buffer, int num_bytes)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + size_t step = regmap_get_raw_read_max(regmap);
> + int ret = 0;
> + int i;
> +
> + if (!step || step > num_bytes)
> + step = num_bytes;
> + else if (step < num_bytes)
> + step = data->fifo_sample_size;
> +
> + for (i = 0; i < num_bytes; i += step) {
> + ret = regmap_raw_read(regmap, BMI160_REG_FIFO_DATA,
> + &buffer[i], step);
> +
> + if (ret)
> + break;
> + }
> +
> + if (ret)
> + dev_err(dev,
> + "Error transferring data from fifo in single steps of %zu\n",
> + step);
> +
> + return ret;
> +}
> +
> +static int __bmi160_fifo_flush(struct iio_dev *indio_dev,
> + unsigned int samples, bool irq)
what does the __ prefix denote?
likely that the function is supposed to be called with mutex held --
maybe use that convention elsewhere as well?
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> + __le16 fifo_length;
> + unsigned int fifo_samples;
> + unsigned int fifo_bytes;
> + u8 *buffer = data->fifo_buffer;
> + u8 *buffer_iter;
> + int64_t last_timestamp, timestamp;
> + unsigned int last_samples;
> + unsigned int i;
> +
> + /* Get the current FIFO length */
> + ret = regmap_bulk_read(regmap, BMI160_REG_FIFO_LENGTH,
> + &fifo_length, sizeof(__le16));
> + if (ret < 0) {
> + dev_err(dev, "Error reading FIFO_LENGTH\n");
> + return ret;
> + }
> +
> + fifo_bytes = le16_to_cpu(fifo_length);
> + fifo_samples = fifo_bytes / data->fifo_sample_size;
> +
> + if (fifo_bytes % data->fifo_sample_size)
> + dev_warn(dev, "fifo_bytes %u is not dividable by %u\n",
> + fifo_bytes, data->fifo_sample_size);
> +
> + if (!fifo_samples)
> + return 0;
> +
> + if (samples && fifo_samples > samples) {
> + fifo_samples = samples;
> + fifo_bytes = fifo_samples * data->fifo_sample_size;
> + }
> +
> + /*
> + * If we are not called from IRQ, it means that we are flushing data
> + * on demand. In that case we do not have latest timestamp saved in
> + * data->timestamp. Get the time now instead.
> + *
> + * In case of IRQ flush, saved timestamp shows the time when number
> + * of samples configured by watermark were ready. Currently there might
> + * be more samples already.
> + * If we are not called from IRQ, than we are getting the current fifo
> + * length, as we are setting timestamp just after getting it.
> + */
> + if (!irq) {
> + last_timestamp = iio_get_time_ns(indio_dev);
> + last_samples = fifo_samples;
> + } else {
> + last_timestamp = data->timestamp;
> + last_samples = data->watermark;
> + }
> +
> + /* Get all measurements */
> + ret = bmi160_fifo_transfer(data, buffer, fifo_bytes);
> + if (ret)
> + return ret;
> +
> + /* Handle demux */
> + timestamp = last_timestamp - (last_samples * data->fifo_sample_period);
> + buffer_iter = buffer;
> + for (i = 0; i < fifo_samples; i++) {
> + u8 tmp_buf[indio_dev->scan_bytes];
non-constant array size, is this allowed these days?
> +
> + memcpy(tmp_buf, buffer_iter, data->fifo_sample_size);
> +
> + timestamp += data->fifo_sample_period;
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + tmp_buf,
> + timestamp);
> +
> + buffer_iter += data->fifo_sample_size;
> + }
> +
> + return fifo_samples;
> +}
> +
> +static int bmi160_fifo_flush(struct iio_dev *indio_dev, unsigned int samples)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = __bmi160_fifo_flush(indio_dev, samples, false);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> static const struct iio_info bmi160_info = {
> - .driver_module = THIS_MODULE,
> - .read_raw = bmi160_read_raw,
> - .write_raw = bmi160_write_raw,
> - .attrs = &bmi160_attrs_group,
> + .driver_module = THIS_MODULE,
> + .read_raw = bmi160_read_raw,
> + .write_raw = bmi160_write_raw,
> + .attrs = &bmi160_attrs_group,
> +};
> +
> +static const struct iio_info bmi160_info_fifo = {
> + .driver_module = THIS_MODULE,
> + .read_raw = bmi160_read_raw,
> + .write_raw = bmi160_write_raw,
> + .attrs = &bmi160_attrs_group,
> + .hwfifo_set_watermark = bmi160_set_watermark,
> + .hwfifo_flush_to_buffer = bmi160_fifo_flush,
> };
>
> static const char *bmi160_match_acpi_device(struct device *dev)
> @@ -561,12 +1059,54 @@ static void bmi160_chip_uninit(struct bmi160_data *data)
> bmi160_set_mode(data, BMI160_ACCEL, false);
> }
>
> +static int bmi160_enable_irq(struct bmi160_data *data)
> +{
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_OUT_CTRL,
> + data->irq_data->output_en,
> + data->irq_data->output_en);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static irqreturn_t bmi160_irq_thread_handler(int irq, void *p)
> +{
> + struct iio_dev *indio_dev = p;
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct device *dev = regmap_get_device(data->regmap);
> +
> + mutex_lock(&data->mutex);
> + if (data->fifo_enabled)
> + __bmi160_fifo_flush(indio_dev, BMI160_FIFO_LENGTH, true);
> + else
> + dev_warn(dev,
> + "IRQ has been triggered, but FIFO is not enabled.\n");
> + mutex_unlock(&data->mutex);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t bmi160_irq_handler(int irq, void *p)
> +{
> + struct iio_dev *indio_dev = p;
> + struct bmi160_data *data = iio_priv(indio_dev);
> +
> + data->timestamp = iio_get_time_ns(indio_dev);
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> - const char *name, bool use_spi)
> + const char *name, int irq,
> + bool use_spi, bool block_supported)
> {
> struct iio_dev *indio_dev;
> struct bmi160_data *data;
> int ret;
> + int irq2;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>
> data = iio_priv(indio_dev);
> dev_set_drvdata(dev, indio_dev);
> + data->irq = irq;
> data->regmap = regmap;
>
> + mutex_init(&data->mutex);
> +
> ret = bmi160_chip_init(data, use_spi);
> if (ret < 0)
> return ret;
> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> indio_dev->info = &bmi160_info;
>
> ret = iio_triggered_buffer_setup(indio_dev, NULL,
> - bmi160_trigger_handler, NULL);
> + bmi160_trigger_handler,
> + &bmi160_buffer_ops);
> if (ret < 0)
> goto uninit;
>
> + if (data->irq > 0) {
> + /* Check which interrupt pin is connected to our board */
> + irq2 = of_irq_get_byname(dev->of_node, "INT2");
> + if (irq2 == data->irq) {
> + dev_dbg(dev, "Using interrupt line INT2\n");
> + data->irq_data = &bmi160_irq2_data;
> + } else {
> + dev_dbg(dev, "Using interrupt line INT1\n");
> + data->irq_data = &bmi160_irq1_data;
> + }
> +
> + ret = devm_request_threaded_irq(dev,
> + data->irq,
> + bmi160_irq_handler,
> + bmi160_irq_thread_handler,
> + IRQF_ONESHOT,
> + BMI160_IRQ_NAME,
> + indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = bmi160_enable_irq(data);
> + if (ret < 0)
> + goto buffer_cleanup;
> +
> + if (block_supported) {
> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> + indio_dev->info = &bmi160_info_fifo;
> + indio_dev->buffer->attrs = bmi160_fifo_attributes;
> + data->fifo_buffer = devm_kmalloc(dev,
> + BMI160_FIFO_LENGTH,
> + GFP_KERNEL);
> + if (!data->fifo_buffer) {
> + ret = -ENOMEM;
need to disable irq on failure?
> + goto buffer_cleanup;
> + }
> + }
> + }
> +
> ret = iio_device_register(indio_dev);
> if (ret < 0)
> goto buffer_cleanup;
> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
> index 07a179d..aa63f89 100644
> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client *client,
> {
> struct regmap *regmap;
> const char *name = NULL;
> + bool block_supported =
> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
> + i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>
> regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
> if (IS_ERR(regmap)) {
> @@ -34,7 +38,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, client->irq,
> + false, block_supported);
> }
>
> static int bmi160_i2c_remove(struct i2c_client *client)
> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
> index 1ec8b12..9b57fbe 100644
> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
> @@ -25,7 +25,8 @@ 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, spi->irq,
> + true, true);
> }
>
> static int bmi160_spi_remove(struct spi_device *spi)
>
--
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-03 11:25 ` Marcin Niestroj
@ 2016-11-06 12:35 ` Jonathan Cameron
-1 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-06 12:35 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 03/11/16 11:25, Marcin Niestroj wrote:
> This patch was developed primarily based on bmc150_accel hardware fifo
> implementation.
>
> IRQ handler was added, which for now is responsible only for handling
> watermark interrupts. The BMI160 chip has two interrupt outputs. By
> default INT is considered to be connected. If INT2 is used instead, the
> interrupt-names device-tree property can be used to specify that.
>
> Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
I agree with Peter that there should have been a few precursor patches
to this one doing various cleanups and reworking bits that you have
in here. Would have made it easier to review (always a good thing :)
In general the resulting code looks good to me. A few little
additional comments inline from me. Mostly about small code ordering things
and function rename suggestions that would make the code more 'obviously'
correct.
Thanks,
Jonathan
> ---
> drivers/iio/imu/bmi160/bmi160.h | 3 +-
> drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
> drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
> drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
> 4 files changed, 618 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index d2ae6ed..4a7c10e 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -4,7 +4,8 @@
> 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, int irq,
> + bool use_spi, bool block_supported);
> void bmi160_core_remove(struct device *dev);
>
> #endif /* BMI160_H_ */
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index e0251b8..153734c 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -2,6 +2,7 @@
> * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
> *
> * Copyright (c) 2016, Intel Corporation.
> + * Copyright (c) 2016, Grinn
> *
> * 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
> @@ -9,7 +10,7 @@
> *
> * IIO core driver for BMI160, with support for I2C/SPI busses
> *
> - * TODO: magnetometer, interrupts, hardware FIFO
> + * TODO: magnetometer, interrupts
> */
> #include <linux/module.h>
> #include <linux/regmap.h>
> @@ -22,8 +23,12 @@
> #include <linux/iio/buffer.h>
> #include <linux/iio/sysfs.h>
>
> +#include <linux/of_irq.h>
> +
> #include "bmi160.h"
>
> +#define BMI160_IRQ_NAME "bmi160_event"
> +
> #define BMI160_REG_CHIP_ID 0x00
> #define BMI160_CHIP_ID_VAL 0xD1
>
> @@ -34,6 +39,21 @@
> #define BMI160_REG_DATA_GYRO_XOUT_L 0x0C
> #define BMI160_REG_DATA_ACCEL_XOUT_L 0x12
>
> +#define BMI160_REG_STATUS 0x1B
> +#define BMI160_STATUS_MAG_MAN_OP BIT(2)
> +
> +#define BMI160_REG_INT_STATUS0 0x1C
> +
> +#define BMI160_REG_INT_STATUS1 0x1D
> +#define BMI160_INT_STATUS_FWM BIT(6)
> +
> +#define BMI160_REG_INT_STATUS2 0x1E
> +
> +#define BMI160_REG_INT_STATUS3 0x1F
> +
> +#define BMI160_REG_FIFO_LENGTH 0x22
> +#define BMI160_REG_FIFO_DATA 0x24
> +
> #define BMI160_REG_ACCEL_CONFIG 0x40
> #define BMI160_ACCEL_CONFIG_ODR_MASK GENMASK(3, 0)
> #define BMI160_ACCEL_CONFIG_BWP_MASK GENMASK(6, 4)
> @@ -55,6 +75,36 @@
> #define BMI160_GYRO_RANGE_250DPS 0x03
> #define BMI160_GYRO_RANGE_125DPS 0x04
>
> +#define BMI160_REG_FIFO_CONFIG_0 0x46
> +
> +#define BMI160_REG_FIFO_CONFIG_1 0x47
> +#define BMI160_FIFO_GYRO_EN BIT(7)
> +#define BMI160_FIFO_ACCEL_EN BIT(6)
> +#define BMI160_FIFO_MAGN_EN BIT(5)
> +#define BMI160_FIFO_HEADER_EN BIT(4)
> +#define BMI160_FIFO_TAG_INT1_EN BIT(3)
> +#define BMI160_FIFO_TAG_INT2_EN BIT(2)
> +#define BMI160_FIFO_TIME_EN BIT(1)
> +
> +#define BMI160_REG_INT_EN_1 0x51
> +#define BMI160_INT_FWM_EN BIT(6)
> +#define BMI160_INT_FFULL_EN BIT(5)
> +#define BMI160_INT_DRDY_EN BIT(4)
> +
> +#define BMI160_REG_INT_OUT_CTRL 0x53
> +#define BMI160_INT2_OUTPUT_EN BIT(7)
> +#define BMI160_INT1_OUTPUT_EN BIT(3)
> +
> +#define BMI160_REG_INT_LATCH 0x54
> +
> +#define BMI160_REG_INT_MAP_1 0x56
> +#define BMI160_INT1_MAP_DRDY BIT(7)
> +#define BMI160_INT1_MAP_FWM BIT(6)
> +#define BMI160_INT1_MAP_FFULL BIT(5)
> +#define BMI160_INT2_MAP_DRDY BIT(3)
> +#define BMI160_INT2_MAP_FWM BIT(2)
> +#define BMI160_INT2_MAP_FFULL BIT(1)
> +
> #define BMI160_REG_CMD 0x7E
> #define BMI160_CMD_ACCEL_PM_SUSPEND 0x10
> #define BMI160_CMD_ACCEL_PM_NORMAL 0x11
> @@ -66,6 +116,8 @@
>
> #define BMI160_REG_DUMMY 0x7F
>
> +#define BMI160_FIFO_LENGTH 1024
> +
> #define BMI160_ACCEL_PMU_MIN_USLEEP 3200
> #define BMI160_ACCEL_PMU_MAX_USLEEP 3800
> #define BMI160_GYRO_PMU_MIN_USLEEP 55000
> @@ -110,8 +162,33 @@ enum bmi160_sensor_type {
> BMI160_NUM_SENSORS /* must be last */
> };
>
> +struct bmi160_irq_data {
> + unsigned int map_fwm;
> + unsigned int output_en;
> +};
> +
> +static const struct bmi160_irq_data bmi160_irq1_data = {
> + .map_fwm = BMI160_INT1_MAP_FWM,
> + .output_en = BMI160_INT1_OUTPUT_EN,
> +};
> +
> +static const struct bmi160_irq_data bmi160_irq2_data = {
> + .map_fwm = BMI160_INT2_MAP_FWM,
> + .output_en = BMI160_INT2_OUTPUT_EN,
> +};
> +
> struct bmi160_data {
> struct regmap *regmap;
> + struct mutex mutex;
> + const struct bmi160_irq_data *irq_data;
> + int irq;
> + int64_t timestamp;
> + int64_t fifo_sample_period;
> + bool fifo_enabled;
> + unsigned int fifo_config;
> + unsigned int fifo_sample_size;
> + u8 *fifo_buffer;
> + unsigned int watermark;
> };
>
> const struct regmap_config bmi160_regmap_config = {
> @@ -159,11 +236,11 @@ struct bmi160_pmu_time {
> static struct bmi160_pmu_time bmi160_pmu_time[] = {
> [BMI160_ACCEL] = {
> .min = BMI160_ACCEL_PMU_MIN_USLEEP,
> - .max = BMI160_ACCEL_PMU_MAX_USLEEP
> + .max = BMI160_ACCEL_PMU_MAX_USLEEP,
> },
> [BMI160_GYRO] = {
> .min = BMI160_GYRO_PMU_MIN_USLEEP,
> - .max = BMI160_GYRO_PMU_MIN_USLEEP,
Guessing this one is a bug!
> + .max = BMI160_GYRO_PMU_MAX_USLEEP,
> },
> };
>
> @@ -285,7 +362,9 @@ int bmi160_set_mode(struct bmi160_data *data, enum bmi160_sensor_type t,
> else
> cmd = bmi160_regs[t].pmu_cmd_suspend;
>
> + mutex_lock(&data->mutex);
> ret = regmap_write(data->regmap, BMI160_REG_CMD, cmd);
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -298,6 +377,7 @@ static
> int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> int uscale)
> {
> + int ret;
> int i;
>
> for (i = 0; i < bmi160_scale_table[t].num; i++)
> @@ -307,8 +387,12 @@ int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> if (i == bmi160_scale_table[t].num)
> return -EINVAL;
>
> - return regmap_write(data->regmap, bmi160_regs[t].range,
> - bmi160_scale_table[t].tbl[i].bits);
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, bmi160_regs[t].range,
> + bmi160_scale_table[t].tbl[i].bits);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> }
>
> static
> @@ -317,7 +401,9 @@ int bmi160_get_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> {
> int i, ret, val;
>
> + mutex_lock(&data->mutex);
> ret = regmap_read(data->regmap, bmi160_regs[t].range, &val);
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -340,7 +426,9 @@ static int bmi160_get_data(struct bmi160_data *data, int chan_type,
>
> reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(__le16);
>
> + mutex_lock(&data->mutex);
> ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(__le16));
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -353,6 +441,7 @@ static
> int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> int odr, int uodr)
> {
> + int ret;
> int i;
>
> for (i = 0; i < bmi160_odr_table[t].num; i++)
> @@ -363,20 +452,30 @@ int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> if (i >= bmi160_odr_table[t].num)
> return -EINVAL;
>
> - return regmap_update_bits(data->regmap,
> - bmi160_regs[t].config,
> - bmi160_regs[t].config_odr_mask,
> - bmi160_odr_table[t].tbl[i].bits);
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap,
> + bmi160_regs[t].config,
> + bmi160_regs[t].config_odr_mask,
> + bmi160_odr_table[t].tbl[i].bits);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> }
>
> -static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> - int *odr, int *uodr)
> +static int64_t bmi160_frequency_to_period(int odr, int uodr)
> {
> - int i, val, ret;
> + uint64_t period = 1000000000000000;
> + int64_t frequency = (int64_t) odr * 1000000 + uodr;
>
> - ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> - if (ret < 0)
> - return ret;
> + do_div(period, frequency);
> +
> + return period;
> +}
> +
> +static const struct bmi160_odr *bmi160_reg_to_odr(enum bmi160_sensor_type t,
> + unsigned int val)
> +{
> + int i;
>
> val &= bmi160_regs[t].config_odr_mask;
>
> @@ -385,10 +484,52 @@ static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> break;
>
> if (i >= bmi160_odr_table[t].num)
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
> +
> + return &bmi160_odr_table[t].tbl[i];
> +}
> +
> +static int bmi160_get_sample_period(struct bmi160_data *data,
> + enum bmi160_sensor_type t,
> + int64_t *sample_period)
> +{
> + const struct bmi160_odr *odr_entry;
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> + if (ret < 0)
> + return ret;
>
> - *odr = bmi160_odr_table[t].tbl[i].odr;
> - *uodr = bmi160_odr_table[t].tbl[i].uodr;
> + odr_entry = bmi160_reg_to_odr(t, val);
> + if (IS_ERR(odr_entry))
> + return PTR_ERR(odr_entry);
> +
> + *sample_period = bmi160_frequency_to_period(odr_entry->odr,
> + odr_entry->uodr);
> +
> + return 0;
> +}
> +
> +static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> + int *odr, int *uodr)
> +{
> + const struct bmi160_odr *odr_entry;
> + int ret;
> + unsigned int val;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> + mutex_unlock(&data->mutex);
> + if (ret < 0)
> + return ret;
> +
> + odr_entry = bmi160_reg_to_odr(t, val);
> + if (IS_ERR(odr_entry))
> + return PTR_ERR(odr_entry);
> +
> + *odr = odr_entry->odr;
> + *uodr = odr_entry->uodr;
>
> return 0;
> }
> @@ -402,14 +543,18 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
> int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
> __le16 sample;
>
> + mutex_lock(&data->mutex);
> for_each_set_bit(i, indio_dev->active_scan_mask,
> indio_dev->masklength) {
> ret = regmap_bulk_read(data->regmap, base + i * sizeof(__le16),
> &sample, sizeof(__le16));
> - if (ret < 0)
> + if (ret < 0) {
> + mutex_unlock(&data->mutex);
> goto done;
> + }
> buf[j++] = sample;
> }
> + mutex_unlock(&data->mutex);
>
> iio_push_to_buffers_with_timestamp(indio_dev, buf,
> iio_get_time_ns(indio_dev));
> @@ -493,11 +638,364 @@ static const struct attribute_group bmi160_attrs_group = {
> .attrs = bmi160_attrs,
> };
>
Naming to my mind would imply we are causing the period to be updated
rather than reading the updated value that hte hardware is giving us.
I'd rename as simple _read_sample_period
> +static int bmi160_update_sample_period(struct bmi160_data *data,
> + enum bmi160_sensor_type sensor_type)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int64_t sample_period;
> + int ret;
> +
> + ret = bmi160_get_sample_period(data, sensor_type, &sample_period);
> + if (ret < 0)
> + return ret;
> +
> + if (data->fifo_sample_period) {
> + if (data->fifo_sample_period != sample_period) {
> + dev_warn(dev, "Enabled sensors have unequal ODR values\n");
> + return -EINVAL;
> + }
> + } else {
> + data->fifo_sample_period = sample_period;
> + }
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_enable(struct iio_dev *indio_dev,
> + struct bmi160_data *data)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> + int i;
> + unsigned int val;
> + unsigned int fifo_config = 0;
> +
> + /* Set fifo sample size and period */
> + for_each_set_bit(i, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + if (i <= BMI160_SCAN_GYRO_Z)
> + fifo_config |= BMI160_FIFO_GYRO_EN;
> + else if (i <= BMI160_SCAN_ACCEL_Z)
> + fifo_config |= BMI160_FIFO_ACCEL_EN;
> + }
> +
> + data->fifo_sample_period = 0;
> + data->fifo_sample_size = 0;
> + if (fifo_config & BMI160_FIFO_GYRO_EN) {
> + data->fifo_sample_size += 6;
> + ret = bmi160_update_sample_period(data, BMI160_GYRO);
> + if (ret < 0)
> + return ret;
> + }
> + if (fifo_config & BMI160_FIFO_ACCEL_EN) {
> + data->fifo_sample_size += 6;
> + ret = bmi160_update_sample_period(data, BMI160_ACCEL);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /*
> + * Set watermark level and write real value back, as it will be used
> + * in timestamp calculation.
> + */
> + val = data->watermark * data->fifo_sample_size;
> + if (val > BMI160_FIFO_LENGTH - 1) {
> + val = BMI160_FIFO_LENGTH - 1;
> + data->watermark = val / data->fifo_sample_size;
> + }
> + val = data->watermark * data->fifo_sample_size / 4;
> +
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_0, val);
> + if (ret < 0) {
> + dev_err(dev, "Failed to set watermark\n");
> + return ret;
> + }
> +
> + /* Enable FIFO channels */
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1,
> + fifo_config);
> + if (ret < 0) {
> + dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
> + return ret;
> + }
> +
> + data->fifo_config = fifo_config;
> + data->fifo_enabled = true;
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_disable(struct bmi160_data *data)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> +
> + /* Disable all FIFO channels */
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1, 0);
> + if (ret < 0) {
> + dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
> + return ret;
> + }
> +
> + data->fifo_enabled = false;
> +
> + return 0;
> +}
> +
> +static int bmi160_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> + return iio_triggered_buffer_postenable(indio_dev);
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_MAP_1,
> + data->irq_data->map_fwm, data->irq_data->map_fwm);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_EN_1,
> + BMI160_INT_FWM_EN, BMI160_INT_FWM_EN);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = bmi160_fifo_enable(indio_dev, data);
> +
> +unlock:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static int bmi160_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct regmap *regmap = data->regmap;
> + int ret = 0;
> +
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> + return iio_triggered_buffer_predisable(indio_dev);
> +
> + mutex_lock(&data->mutex);
> +
I would rather expect this unwind to reverse the order of the setup above.
Either that is obviously correct, or there should be some comments in here
indicating why not.
> + ret = regmap_update_bits(regmap, BMI160_REG_INT_EN_1,
> + BMI160_INT_FWM_EN, 0);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = bmi160_fifo_disable(data);
> +
> +unlock:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops bmi160_buffer_ops = {
> + .postenable = bmi160_buffer_postenable,
> + .predisable = bmi160_buffer_predisable,
> +};
> +
> +static ssize_t bmi160_get_fifo_state(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct bmi160_data *data = iio_priv(indio_dev);
> + bool state;
> +
> + mutex_lock(&data->mutex);
> + state = data->fifo_enabled;
> + mutex_unlock(&data->mutex);
> +
> + return sprintf(buf, "%d\n", (int) state);
> +}
> +
> +static ssize_t bmi160_get_fifo_watermark(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int wm;
> +
> + mutex_lock(&data->mutex);
> + wm = data->watermark;
> + mutex_unlock(&data->mutex);
> +
> + return sprintf(buf, "%d\n", wm);
> +}
> +
> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
> +static IIO_CONST_ATTR(hwfifo_watermark_max,
> + __stringify(BMI160_FIFO_LENGTH));
> +static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
> + bmi160_get_fifo_state, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
> + bmi160_get_fifo_watermark, NULL, 0);
> +
> +static const struct attribute *bmi160_fifo_attributes[] = {
> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
There are enough of these drivers now that sometimes soon we should
revisit the question of pulling these into the core. Can certainly
concieve of downstream consumer devices (in particular the iio_input
bridge when that finally resurfaces - my fault) wanting to be able to
manipulate or at least have visibilty of these.
> + NULL,
> +};
> +
> +static int bmi160_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> +
> + if (val > BMI160_FIFO_LENGTH)
> + val = BMI160_FIFO_LENGTH;
> +
> + data->watermark = val;
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_transfer(struct bmi160_data *data,
> + char *buffer, int num_bytes)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + size_t step = regmap_get_raw_read_max(regmap);
> + int ret = 0;
> + int i;
> +
> + if (!step || step > num_bytes)
> + step = num_bytes;
> + else if (step < num_bytes)
> + step = data->fifo_sample_size;
> +
> + for (i = 0; i < num_bytes; i += step) {
> + ret = regmap_raw_read(regmap, BMI160_REG_FIFO_DATA,
> + &buffer[i], step);
> +
> + if (ret)
> + break;
> + }
> +
> + if (ret)
> + dev_err(dev,
> + "Error transferring data from fifo in single steps of %zu\n",
> + step);
> +
> + return ret;
> +}
> +
> +static int __bmi160_fifo_flush(struct iio_dev *indio_dev,
> + unsigned int samples, bool irq)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> + __le16 fifo_length;
> + unsigned int fifo_samples;
> + unsigned int fifo_bytes;
> + u8 *buffer = data->fifo_buffer;
> + u8 *buffer_iter;
> + int64_t last_timestamp, timestamp;
> + unsigned int last_samples;
> + unsigned int i;
> +
> + /* Get the current FIFO length */
> + ret = regmap_bulk_read(regmap, BMI160_REG_FIFO_LENGTH,
> + &fifo_length, sizeof(__le16));
> + if (ret < 0) {
> + dev_err(dev, "Error reading FIFO_LENGTH\n");
> + return ret;
> + }
> +
> + fifo_bytes = le16_to_cpu(fifo_length);
> + fifo_samples = fifo_bytes / data->fifo_sample_size;
> +
> + if (fifo_bytes % data->fifo_sample_size)
> + dev_warn(dev, "fifo_bytes %u is not dividable by %u\n",
> + fifo_bytes, data->fifo_sample_size);
> +
> + if (!fifo_samples)
> + return 0;
> +
> + if (samples && fifo_samples > samples) {
> + fifo_samples = samples;
> + fifo_bytes = fifo_samples * data->fifo_sample_size;
> + }
> +
> + /*
> + * If we are not called from IRQ, it means that we are flushing data
> + * on demand. In that case we do not have latest timestamp saved in
> + * data->timestamp. Get the time now instead.
> + *
> + * In case of IRQ flush, saved timestamp shows the time when number
> + * of samples configured by watermark were ready. Currently there might
> + * be more samples already.
> + * If we are not called from IRQ, than we are getting the current fifo
> + * length, as we are setting timestamp just after getting it.
> + */
It's a real pain that people almost always want timestamps with IMU data :(
Ah well, this is about the best that can be done...
> + if (!irq) {
> + last_timestamp = iio_get_time_ns(indio_dev);
> + last_samples = fifo_samples;
> + } else {
> + last_timestamp = data->timestamp;
> + last_samples = data->watermark;
> + }
> +
> + /* Get all measurements */
> + ret = bmi160_fifo_transfer(data, buffer, fifo_bytes);
> + if (ret)
> + return ret;
> +
> + /* Handle demux */
> + timestamp = last_timestamp - (last_samples * data->fifo_sample_period);
> + buffer_iter = buffer;
> + for (i = 0; i < fifo_samples; i++) {
> + u8 tmp_buf[indio_dev->scan_bytes];
> +
> + memcpy(tmp_buf, buffer_iter, data->fifo_sample_size);
> +
> + timestamp += data->fifo_sample_period;
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + tmp_buf,
> + timestamp);
Not a comment on your code which is correct as it stands, but...
We should really get a useful utility functions in place:
iio_push_to_buffer_with_timestamp_safe(struct iio_dev *, void *data, void *working, timestamp);
Which would use the working space to do your little data shuffle as here - if
the timestamp is enabled, and skip it if not.
We've been meaning to add a multiple element push to buffer, so we'd need
a little helper to do the data shuffling for that as well (probably just
looping on the _safe version of the local push.)
> +
> + buffer_iter += data->fifo_sample_size;
> + }
> +
> + return fifo_samples;
> +}
> +
> +static int bmi160_fifo_flush(struct iio_dev *indio_dev, unsigned int samples)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = __bmi160_fifo_flush(indio_dev, samples, false);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> static const struct iio_info bmi160_info = {
> - .driver_module = THIS_MODULE,
> - .read_raw = bmi160_read_raw,
> - .write_raw = bmi160_write_raw,
> - .attrs = &bmi160_attrs_group,
> + .driver_module = THIS_MODULE,
> + .read_raw = bmi160_read_raw,
> + .write_raw = bmi160_write_raw,
> + .attrs = &bmi160_attrs_group,
> +};
The realignment adds nothing but noise - so I wouldn't do it. These always
seem very nice and pretty but then we get another addition that requires
futher indenting and suddenly we have another pointless 15 lines of patch.
The only exception in my mind is things like filling in numerical arrays
where readability is really enhanced.
(I wouldn't have minded enough to comment if this hadn't been modifying
existing code but just introducing new indented code).
> +
> +static const struct iio_info bmi160_info_fifo = {
> + .driver_module = THIS_MODULE,
> + .read_raw = bmi160_read_raw,
> + .write_raw = bmi160_write_raw,
> + .attrs = &bmi160_attrs_group,
> + .hwfifo_set_watermark = bmi160_set_watermark,
> + .hwfifo_flush_to_buffer = bmi160_fifo_flush,
> };
>
> static const char *bmi160_match_acpi_device(struct device *dev)
> @@ -561,12 +1059,54 @@ static void bmi160_chip_uninit(struct bmi160_data *data)
> bmi160_set_mode(data, BMI160_ACCEL, false);
> }
>
> +static int bmi160_enable_irq(struct bmi160_data *data)
> +{
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_OUT_CTRL,
> + data->irq_data->output_en,
> + data->irq_data->output_en);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static irqreturn_t bmi160_irq_thread_handler(int irq, void *p)
> +{
> + struct iio_dev *indio_dev = p;
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct device *dev = regmap_get_device(data->regmap);
> +
> + mutex_lock(&data->mutex);
> + if (data->fifo_enabled)
> + __bmi160_fifo_flush(indio_dev, BMI160_FIFO_LENGTH, true);
> + else
> + dev_warn(dev,
> + "IRQ has been triggered, but FIFO is not enabled.\n");
> + mutex_unlock(&data->mutex);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t bmi160_irq_handler(int irq, void *p)
> +{
> + struct iio_dev *indio_dev = p;
> + struct bmi160_data *data = iio_priv(indio_dev);
> +
> + data->timestamp = iio_get_time_ns(indio_dev);
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> - const char *name, bool use_spi)
> + const char *name, int irq,
> + bool use_spi, bool block_supported)
> {
> struct iio_dev *indio_dev;
> struct bmi160_data *data;
> int ret;
> + int irq2;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>
> data = iio_priv(indio_dev);
> dev_set_drvdata(dev, indio_dev);
> + data->irq = irq;
> data->regmap = regmap;
>
> + mutex_init(&data->mutex);
> +
> ret = bmi160_chip_init(data, use_spi);
> if (ret < 0)
> return ret;
> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> indio_dev->info = &bmi160_info;
>
> ret = iio_triggered_buffer_setup(indio_dev, NULL,
> - bmi160_trigger_handler, NULL);
> + bmi160_trigger_handler,
> + &bmi160_buffer_ops);
> if (ret < 0)
> goto uninit;
>
> + if (data->irq > 0) {
> + /* Check which interrupt pin is connected to our board */
> + irq2 = of_irq_get_byname(dev->of_node, "INT2");
> + if (irq2 == data->irq) {
> + dev_dbg(dev, "Using interrupt line INT2\n");
> + data->irq_data = &bmi160_irq2_data;
> + } else {
> + dev_dbg(dev, "Using interrupt line INT1\n");
> + data->irq_data = &bmi160_irq1_data;
> + }
> +
> + ret = devm_request_threaded_irq(dev,
> + data->irq,
> + bmi160_irq_handler,
> + bmi160_irq_thread_handler,
> + IRQF_ONESHOT,
> + BMI160_IRQ_NAME,
> + indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = bmi160_enable_irq(data);
> + if (ret < 0)
> + goto buffer_cleanup;
> +
> + if (block_supported) {
> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> + indio_dev->info = &bmi160_info_fifo;
> + indio_dev->buffer->attrs = bmi160_fifo_attributes;
> + data->fifo_buffer = devm_kmalloc(dev,
> + BMI160_FIFO_LENGTH,
> + GFP_KERNEL);
> + if (!data->fifo_buffer) {
> + ret = -ENOMEM;
> + goto buffer_cleanup;
> + }
> + }
> + }
> +
> ret = iio_device_register(indio_dev);
> if (ret < 0)
> goto buffer_cleanup;
> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
> index 07a179d..aa63f89 100644
> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client *client,
> {
> struct regmap *regmap;
> const char *name = NULL;
> + bool block_supported =
> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
> + i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>
> regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
> if (IS_ERR(regmap)) {
> @@ -34,7 +38,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, client->irq,
> + false, block_supported);
> }
>
> static int bmi160_i2c_remove(struct i2c_client *client)
> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
> index 1ec8b12..9b57fbe 100644
> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
> @@ -25,7 +25,8 @@ 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, spi->irq,
> + true, true);
> }
>
> static int bmi160_spi_remove(struct spi_device *spi)
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
@ 2016-11-06 12:35 ` Jonathan Cameron
0 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-06 12:35 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio, devicetree
On 03/11/16 11:25, Marcin Niestroj wrote:
> This patch was developed primarily based on bmc150_accel hardware fifo
> implementation.
>
> IRQ handler was added, which for now is responsible only for handling
> watermark interrupts. The BMI160 chip has two interrupt outputs. By
> default INT is considered to be connected. If INT2 is used instead, the
> interrupt-names device-tree property can be used to specify that.
>
> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
I agree with Peter that there should have been a few precursor patches
to this one doing various cleanups and reworking bits that you have
in here. Would have made it easier to review (always a good thing :)
In general the resulting code looks good to me. A few little
additional comments inline from me. Mostly about small code ordering things
and function rename suggestions that would make the code more 'obviously'
correct.
Thanks,
Jonathan
> ---
> drivers/iio/imu/bmi160/bmi160.h | 3 +-
> drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
> drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
> drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
> 4 files changed, 618 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index d2ae6ed..4a7c10e 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -4,7 +4,8 @@
> 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, int irq,
> + bool use_spi, bool block_supported);
> void bmi160_core_remove(struct device *dev);
>
> #endif /* BMI160_H_ */
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index e0251b8..153734c 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -2,6 +2,7 @@
> * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
> *
> * Copyright (c) 2016, Intel Corporation.
> + * Copyright (c) 2016, Grinn
> *
> * 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
> @@ -9,7 +10,7 @@
> *
> * IIO core driver for BMI160, with support for I2C/SPI busses
> *
> - * TODO: magnetometer, interrupts, hardware FIFO
> + * TODO: magnetometer, interrupts
> */
> #include <linux/module.h>
> #include <linux/regmap.h>
> @@ -22,8 +23,12 @@
> #include <linux/iio/buffer.h>
> #include <linux/iio/sysfs.h>
>
> +#include <linux/of_irq.h>
> +
> #include "bmi160.h"
>
> +#define BMI160_IRQ_NAME "bmi160_event"
> +
> #define BMI160_REG_CHIP_ID 0x00
> #define BMI160_CHIP_ID_VAL 0xD1
>
> @@ -34,6 +39,21 @@
> #define BMI160_REG_DATA_GYRO_XOUT_L 0x0C
> #define BMI160_REG_DATA_ACCEL_XOUT_L 0x12
>
> +#define BMI160_REG_STATUS 0x1B
> +#define BMI160_STATUS_MAG_MAN_OP BIT(2)
> +
> +#define BMI160_REG_INT_STATUS0 0x1C
> +
> +#define BMI160_REG_INT_STATUS1 0x1D
> +#define BMI160_INT_STATUS_FWM BIT(6)
> +
> +#define BMI160_REG_INT_STATUS2 0x1E
> +
> +#define BMI160_REG_INT_STATUS3 0x1F
> +
> +#define BMI160_REG_FIFO_LENGTH 0x22
> +#define BMI160_REG_FIFO_DATA 0x24
> +
> #define BMI160_REG_ACCEL_CONFIG 0x40
> #define BMI160_ACCEL_CONFIG_ODR_MASK GENMASK(3, 0)
> #define BMI160_ACCEL_CONFIG_BWP_MASK GENMASK(6, 4)
> @@ -55,6 +75,36 @@
> #define BMI160_GYRO_RANGE_250DPS 0x03
> #define BMI160_GYRO_RANGE_125DPS 0x04
>
> +#define BMI160_REG_FIFO_CONFIG_0 0x46
> +
> +#define BMI160_REG_FIFO_CONFIG_1 0x47
> +#define BMI160_FIFO_GYRO_EN BIT(7)
> +#define BMI160_FIFO_ACCEL_EN BIT(6)
> +#define BMI160_FIFO_MAGN_EN BIT(5)
> +#define BMI160_FIFO_HEADER_EN BIT(4)
> +#define BMI160_FIFO_TAG_INT1_EN BIT(3)
> +#define BMI160_FIFO_TAG_INT2_EN BIT(2)
> +#define BMI160_FIFO_TIME_EN BIT(1)
> +
> +#define BMI160_REG_INT_EN_1 0x51
> +#define BMI160_INT_FWM_EN BIT(6)
> +#define BMI160_INT_FFULL_EN BIT(5)
> +#define BMI160_INT_DRDY_EN BIT(4)
> +
> +#define BMI160_REG_INT_OUT_CTRL 0x53
> +#define BMI160_INT2_OUTPUT_EN BIT(7)
> +#define BMI160_INT1_OUTPUT_EN BIT(3)
> +
> +#define BMI160_REG_INT_LATCH 0x54
> +
> +#define BMI160_REG_INT_MAP_1 0x56
> +#define BMI160_INT1_MAP_DRDY BIT(7)
> +#define BMI160_INT1_MAP_FWM BIT(6)
> +#define BMI160_INT1_MAP_FFULL BIT(5)
> +#define BMI160_INT2_MAP_DRDY BIT(3)
> +#define BMI160_INT2_MAP_FWM BIT(2)
> +#define BMI160_INT2_MAP_FFULL BIT(1)
> +
> #define BMI160_REG_CMD 0x7E
> #define BMI160_CMD_ACCEL_PM_SUSPEND 0x10
> #define BMI160_CMD_ACCEL_PM_NORMAL 0x11
> @@ -66,6 +116,8 @@
>
> #define BMI160_REG_DUMMY 0x7F
>
> +#define BMI160_FIFO_LENGTH 1024
> +
> #define BMI160_ACCEL_PMU_MIN_USLEEP 3200
> #define BMI160_ACCEL_PMU_MAX_USLEEP 3800
> #define BMI160_GYRO_PMU_MIN_USLEEP 55000
> @@ -110,8 +162,33 @@ enum bmi160_sensor_type {
> BMI160_NUM_SENSORS /* must be last */
> };
>
> +struct bmi160_irq_data {
> + unsigned int map_fwm;
> + unsigned int output_en;
> +};
> +
> +static const struct bmi160_irq_data bmi160_irq1_data = {
> + .map_fwm = BMI160_INT1_MAP_FWM,
> + .output_en = BMI160_INT1_OUTPUT_EN,
> +};
> +
> +static const struct bmi160_irq_data bmi160_irq2_data = {
> + .map_fwm = BMI160_INT2_MAP_FWM,
> + .output_en = BMI160_INT2_OUTPUT_EN,
> +};
> +
> struct bmi160_data {
> struct regmap *regmap;
> + struct mutex mutex;
> + const struct bmi160_irq_data *irq_data;
> + int irq;
> + int64_t timestamp;
> + int64_t fifo_sample_period;
> + bool fifo_enabled;
> + unsigned int fifo_config;
> + unsigned int fifo_sample_size;
> + u8 *fifo_buffer;
> + unsigned int watermark;
> };
>
> const struct regmap_config bmi160_regmap_config = {
> @@ -159,11 +236,11 @@ struct bmi160_pmu_time {
> static struct bmi160_pmu_time bmi160_pmu_time[] = {
> [BMI160_ACCEL] = {
> .min = BMI160_ACCEL_PMU_MIN_USLEEP,
> - .max = BMI160_ACCEL_PMU_MAX_USLEEP
> + .max = BMI160_ACCEL_PMU_MAX_USLEEP,
> },
> [BMI160_GYRO] = {
> .min = BMI160_GYRO_PMU_MIN_USLEEP,
> - .max = BMI160_GYRO_PMU_MIN_USLEEP,
Guessing this one is a bug!
> + .max = BMI160_GYRO_PMU_MAX_USLEEP,
> },
> };
>
> @@ -285,7 +362,9 @@ int bmi160_set_mode(struct bmi160_data *data, enum bmi160_sensor_type t,
> else
> cmd = bmi160_regs[t].pmu_cmd_suspend;
>
> + mutex_lock(&data->mutex);
> ret = regmap_write(data->regmap, BMI160_REG_CMD, cmd);
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -298,6 +377,7 @@ static
> int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> int uscale)
> {
> + int ret;
> int i;
>
> for (i = 0; i < bmi160_scale_table[t].num; i++)
> @@ -307,8 +387,12 @@ int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> if (i == bmi160_scale_table[t].num)
> return -EINVAL;
>
> - return regmap_write(data->regmap, bmi160_regs[t].range,
> - bmi160_scale_table[t].tbl[i].bits);
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, bmi160_regs[t].range,
> + bmi160_scale_table[t].tbl[i].bits);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> }
>
> static
> @@ -317,7 +401,9 @@ int bmi160_get_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
> {
> int i, ret, val;
>
> + mutex_lock(&data->mutex);
> ret = regmap_read(data->regmap, bmi160_regs[t].range, &val);
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -340,7 +426,9 @@ static int bmi160_get_data(struct bmi160_data *data, int chan_type,
>
> reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(__le16);
>
> + mutex_lock(&data->mutex);
> ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(__le16));
> + mutex_unlock(&data->mutex);
> if (ret < 0)
> return ret;
>
> @@ -353,6 +441,7 @@ static
> int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> int odr, int uodr)
> {
> + int ret;
> int i;
>
> for (i = 0; i < bmi160_odr_table[t].num; i++)
> @@ -363,20 +452,30 @@ int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> if (i >= bmi160_odr_table[t].num)
> return -EINVAL;
>
> - return regmap_update_bits(data->regmap,
> - bmi160_regs[t].config,
> - bmi160_regs[t].config_odr_mask,
> - bmi160_odr_table[t].tbl[i].bits);
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap,
> + bmi160_regs[t].config,
> + bmi160_regs[t].config_odr_mask,
> + bmi160_odr_table[t].tbl[i].bits);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> }
>
> -static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> - int *odr, int *uodr)
> +static int64_t bmi160_frequency_to_period(int odr, int uodr)
> {
> - int i, val, ret;
> + uint64_t period = 1000000000000000;
> + int64_t frequency = (int64_t) odr * 1000000 + uodr;
>
> - ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> - if (ret < 0)
> - return ret;
> + do_div(period, frequency);
> +
> + return period;
> +}
> +
> +static const struct bmi160_odr *bmi160_reg_to_odr(enum bmi160_sensor_type t,
> + unsigned int val)
> +{
> + int i;
>
> val &= bmi160_regs[t].config_odr_mask;
>
> @@ -385,10 +484,52 @@ static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> break;
>
> if (i >= bmi160_odr_table[t].num)
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
> +
> + return &bmi160_odr_table[t].tbl[i];
> +}
> +
> +static int bmi160_get_sample_period(struct bmi160_data *data,
> + enum bmi160_sensor_type t,
> + int64_t *sample_period)
> +{
> + const struct bmi160_odr *odr_entry;
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> + if (ret < 0)
> + return ret;
>
> - *odr = bmi160_odr_table[t].tbl[i].odr;
> - *uodr = bmi160_odr_table[t].tbl[i].uodr;
> + odr_entry = bmi160_reg_to_odr(t, val);
> + if (IS_ERR(odr_entry))
> + return PTR_ERR(odr_entry);
> +
> + *sample_period = bmi160_frequency_to_period(odr_entry->odr,
> + odr_entry->uodr);
> +
> + return 0;
> +}
> +
> +static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
> + int *odr, int *uodr)
> +{
> + const struct bmi160_odr *odr_entry;
> + int ret;
> + unsigned int val;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
> + mutex_unlock(&data->mutex);
> + if (ret < 0)
> + return ret;
> +
> + odr_entry = bmi160_reg_to_odr(t, val);
> + if (IS_ERR(odr_entry))
> + return PTR_ERR(odr_entry);
> +
> + *odr = odr_entry->odr;
> + *uodr = odr_entry->uodr;
>
> return 0;
> }
> @@ -402,14 +543,18 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
> int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
> __le16 sample;
>
> + mutex_lock(&data->mutex);
> for_each_set_bit(i, indio_dev->active_scan_mask,
> indio_dev->masklength) {
> ret = regmap_bulk_read(data->regmap, base + i * sizeof(__le16),
> &sample, sizeof(__le16));
> - if (ret < 0)
> + if (ret < 0) {
> + mutex_unlock(&data->mutex);
> goto done;
> + }
> buf[j++] = sample;
> }
> + mutex_unlock(&data->mutex);
>
> iio_push_to_buffers_with_timestamp(indio_dev, buf,
> iio_get_time_ns(indio_dev));
> @@ -493,11 +638,364 @@ static const struct attribute_group bmi160_attrs_group = {
> .attrs = bmi160_attrs,
> };
>
Naming to my mind would imply we are causing the period to be updated
rather than reading the updated value that hte hardware is giving us.
I'd rename as simple _read_sample_period
> +static int bmi160_update_sample_period(struct bmi160_data *data,
> + enum bmi160_sensor_type sensor_type)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int64_t sample_period;
> + int ret;
> +
> + ret = bmi160_get_sample_period(data, sensor_type, &sample_period);
> + if (ret < 0)
> + return ret;
> +
> + if (data->fifo_sample_period) {
> + if (data->fifo_sample_period != sample_period) {
> + dev_warn(dev, "Enabled sensors have unequal ODR values\n");
> + return -EINVAL;
> + }
> + } else {
> + data->fifo_sample_period = sample_period;
> + }
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_enable(struct iio_dev *indio_dev,
> + struct bmi160_data *data)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> + int i;
> + unsigned int val;
> + unsigned int fifo_config = 0;
> +
> + /* Set fifo sample size and period */
> + for_each_set_bit(i, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + if (i <= BMI160_SCAN_GYRO_Z)
> + fifo_config |= BMI160_FIFO_GYRO_EN;
> + else if (i <= BMI160_SCAN_ACCEL_Z)
> + fifo_config |= BMI160_FIFO_ACCEL_EN;
> + }
> +
> + data->fifo_sample_period = 0;
> + data->fifo_sample_size = 0;
> + if (fifo_config & BMI160_FIFO_GYRO_EN) {
> + data->fifo_sample_size += 6;
> + ret = bmi160_update_sample_period(data, BMI160_GYRO);
> + if (ret < 0)
> + return ret;
> + }
> + if (fifo_config & BMI160_FIFO_ACCEL_EN) {
> + data->fifo_sample_size += 6;
> + ret = bmi160_update_sample_period(data, BMI160_ACCEL);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /*
> + * Set watermark level and write real value back, as it will be used
> + * in timestamp calculation.
> + */
> + val = data->watermark * data->fifo_sample_size;
> + if (val > BMI160_FIFO_LENGTH - 1) {
> + val = BMI160_FIFO_LENGTH - 1;
> + data->watermark = val / data->fifo_sample_size;
> + }
> + val = data->watermark * data->fifo_sample_size / 4;
> +
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_0, val);
> + if (ret < 0) {
> + dev_err(dev, "Failed to set watermark\n");
> + return ret;
> + }
> +
> + /* Enable FIFO channels */
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1,
> + fifo_config);
> + if (ret < 0) {
> + dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
> + return ret;
> + }
> +
> + data->fifo_config = fifo_config;
> + data->fifo_enabled = true;
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_disable(struct bmi160_data *data)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> +
> + /* Disable all FIFO channels */
> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1, 0);
> + if (ret < 0) {
> + dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
> + return ret;
> + }
> +
> + data->fifo_enabled = false;
> +
> + return 0;
> +}
> +
> +static int bmi160_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> + return iio_triggered_buffer_postenable(indio_dev);
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_MAP_1,
> + data->irq_data->map_fwm, data->irq_data->map_fwm);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_EN_1,
> + BMI160_INT_FWM_EN, BMI160_INT_FWM_EN);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = bmi160_fifo_enable(indio_dev, data);
> +
> +unlock:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static int bmi160_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct regmap *regmap = data->regmap;
> + int ret = 0;
> +
> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> + return iio_triggered_buffer_predisable(indio_dev);
> +
> + mutex_lock(&data->mutex);
> +
I would rather expect this unwind to reverse the order of the setup above.
Either that is obviously correct, or there should be some comments in here
indicating why not.
> + ret = regmap_update_bits(regmap, BMI160_REG_INT_EN_1,
> + BMI160_INT_FWM_EN, 0);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = bmi160_fifo_disable(data);
> +
> +unlock:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops bmi160_buffer_ops = {
> + .postenable = bmi160_buffer_postenable,
> + .predisable = bmi160_buffer_predisable,
> +};
> +
> +static ssize_t bmi160_get_fifo_state(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct bmi160_data *data = iio_priv(indio_dev);
> + bool state;
> +
> + mutex_lock(&data->mutex);
> + state = data->fifo_enabled;
> + mutex_unlock(&data->mutex);
> +
> + return sprintf(buf, "%d\n", (int) state);
> +}
> +
> +static ssize_t bmi160_get_fifo_watermark(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int wm;
> +
> + mutex_lock(&data->mutex);
> + wm = data->watermark;
> + mutex_unlock(&data->mutex);
> +
> + return sprintf(buf, "%d\n", wm);
> +}
> +
> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
> +static IIO_CONST_ATTR(hwfifo_watermark_max,
> + __stringify(BMI160_FIFO_LENGTH));
> +static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
> + bmi160_get_fifo_state, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
> + bmi160_get_fifo_watermark, NULL, 0);
> +
> +static const struct attribute *bmi160_fifo_attributes[] = {
> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
There are enough of these drivers now that sometimes soon we should
revisit the question of pulling these into the core. Can certainly
concieve of downstream consumer devices (in particular the iio_input
bridge when that finally resurfaces - my fault) wanting to be able to
manipulate or at least have visibilty of these.
> + NULL,
> +};
> +
> +static int bmi160_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> +
> + if (val > BMI160_FIFO_LENGTH)
> + val = BMI160_FIFO_LENGTH;
> +
> + data->watermark = val;
> +
> + return 0;
> +}
> +
> +static int bmi160_fifo_transfer(struct bmi160_data *data,
> + char *buffer, int num_bytes)
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + size_t step = regmap_get_raw_read_max(regmap);
> + int ret = 0;
> + int i;
> +
> + if (!step || step > num_bytes)
> + step = num_bytes;
> + else if (step < num_bytes)
> + step = data->fifo_sample_size;
> +
> + for (i = 0; i < num_bytes; i += step) {
> + ret = regmap_raw_read(regmap, BMI160_REG_FIFO_DATA,
> + &buffer[i], step);
> +
> + if (ret)
> + break;
> + }
> +
> + if (ret)
> + dev_err(dev,
> + "Error transferring data from fifo in single steps of %zu\n",
> + step);
> +
> + return ret;
> +}
> +
> +static int __bmi160_fifo_flush(struct iio_dev *indio_dev,
> + unsigned int samples, bool irq)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct regmap *regmap = data->regmap;
> + struct device *dev = regmap_get_device(regmap);
> + int ret;
> + __le16 fifo_length;
> + unsigned int fifo_samples;
> + unsigned int fifo_bytes;
> + u8 *buffer = data->fifo_buffer;
> + u8 *buffer_iter;
> + int64_t last_timestamp, timestamp;
> + unsigned int last_samples;
> + unsigned int i;
> +
> + /* Get the current FIFO length */
> + ret = regmap_bulk_read(regmap, BMI160_REG_FIFO_LENGTH,
> + &fifo_length, sizeof(__le16));
> + if (ret < 0) {
> + dev_err(dev, "Error reading FIFO_LENGTH\n");
> + return ret;
> + }
> +
> + fifo_bytes = le16_to_cpu(fifo_length);
> + fifo_samples = fifo_bytes / data->fifo_sample_size;
> +
> + if (fifo_bytes % data->fifo_sample_size)
> + dev_warn(dev, "fifo_bytes %u is not dividable by %u\n",
> + fifo_bytes, data->fifo_sample_size);
> +
> + if (!fifo_samples)
> + return 0;
> +
> + if (samples && fifo_samples > samples) {
> + fifo_samples = samples;
> + fifo_bytes = fifo_samples * data->fifo_sample_size;
> + }
> +
> + /*
> + * If we are not called from IRQ, it means that we are flushing data
> + * on demand. In that case we do not have latest timestamp saved in
> + * data->timestamp. Get the time now instead.
> + *
> + * In case of IRQ flush, saved timestamp shows the time when number
> + * of samples configured by watermark were ready. Currently there might
> + * be more samples already.
> + * If we are not called from IRQ, than we are getting the current fifo
> + * length, as we are setting timestamp just after getting it.
> + */
It's a real pain that people almost always want timestamps with IMU data :(
Ah well, this is about the best that can be done...
> + if (!irq) {
> + last_timestamp = iio_get_time_ns(indio_dev);
> + last_samples = fifo_samples;
> + } else {
> + last_timestamp = data->timestamp;
> + last_samples = data->watermark;
> + }
> +
> + /* Get all measurements */
> + ret = bmi160_fifo_transfer(data, buffer, fifo_bytes);
> + if (ret)
> + return ret;
> +
> + /* Handle demux */
> + timestamp = last_timestamp - (last_samples * data->fifo_sample_period);
> + buffer_iter = buffer;
> + for (i = 0; i < fifo_samples; i++) {
> + u8 tmp_buf[indio_dev->scan_bytes];
> +
> + memcpy(tmp_buf, buffer_iter, data->fifo_sample_size);
> +
> + timestamp += data->fifo_sample_period;
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + tmp_buf,
> + timestamp);
Not a comment on your code which is correct as it stands, but...
We should really get a useful utility functions in place:
iio_push_to_buffer_with_timestamp_safe(struct iio_dev *, void *data, void *working, timestamp);
Which would use the working space to do your little data shuffle as here - if
the timestamp is enabled, and skip it if not.
We've been meaning to add a multiple element push to buffer, so we'd need
a little helper to do the data shuffling for that as well (probably just
looping on the _safe version of the local push.)
> +
> + buffer_iter += data->fifo_sample_size;
> + }
> +
> + return fifo_samples;
> +}
> +
> +static int bmi160_fifo_flush(struct iio_dev *indio_dev, unsigned int samples)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = __bmi160_fifo_flush(indio_dev, samples, false);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> static const struct iio_info bmi160_info = {
> - .driver_module = THIS_MODULE,
> - .read_raw = bmi160_read_raw,
> - .write_raw = bmi160_write_raw,
> - .attrs = &bmi160_attrs_group,
> + .driver_module = THIS_MODULE,
> + .read_raw = bmi160_read_raw,
> + .write_raw = bmi160_write_raw,
> + .attrs = &bmi160_attrs_group,
> +};
The realignment adds nothing but noise - so I wouldn't do it. These always
seem very nice and pretty but then we get another addition that requires
futher indenting and suddenly we have another pointless 15 lines of patch.
The only exception in my mind is things like filling in numerical arrays
where readability is really enhanced.
(I wouldn't have minded enough to comment if this hadn't been modifying
existing code but just introducing new indented code).
> +
> +static const struct iio_info bmi160_info_fifo = {
> + .driver_module = THIS_MODULE,
> + .read_raw = bmi160_read_raw,
> + .write_raw = bmi160_write_raw,
> + .attrs = &bmi160_attrs_group,
> + .hwfifo_set_watermark = bmi160_set_watermark,
> + .hwfifo_flush_to_buffer = bmi160_fifo_flush,
> };
>
> static const char *bmi160_match_acpi_device(struct device *dev)
> @@ -561,12 +1059,54 @@ static void bmi160_chip_uninit(struct bmi160_data *data)
> bmi160_set_mode(data, BMI160_ACCEL, false);
> }
>
> +static int bmi160_enable_irq(struct bmi160_data *data)
> +{
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_OUT_CTRL,
> + data->irq_data->output_en,
> + data->irq_data->output_en);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static irqreturn_t bmi160_irq_thread_handler(int irq, void *p)
> +{
> + struct iio_dev *indio_dev = p;
> + struct bmi160_data *data = iio_priv(indio_dev);
> + struct device *dev = regmap_get_device(data->regmap);
> +
> + mutex_lock(&data->mutex);
> + if (data->fifo_enabled)
> + __bmi160_fifo_flush(indio_dev, BMI160_FIFO_LENGTH, true);
> + else
> + dev_warn(dev,
> + "IRQ has been triggered, but FIFO is not enabled.\n");
> + mutex_unlock(&data->mutex);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t bmi160_irq_handler(int irq, void *p)
> +{
> + struct iio_dev *indio_dev = p;
> + struct bmi160_data *data = iio_priv(indio_dev);
> +
> + data->timestamp = iio_get_time_ns(indio_dev);
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> - const char *name, bool use_spi)
> + const char *name, int irq,
> + bool use_spi, bool block_supported)
> {
> struct iio_dev *indio_dev;
> struct bmi160_data *data;
> int ret;
> + int irq2;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>
> data = iio_priv(indio_dev);
> dev_set_drvdata(dev, indio_dev);
> + data->irq = irq;
> data->regmap = regmap;
>
> + mutex_init(&data->mutex);
> +
> ret = bmi160_chip_init(data, use_spi);
> if (ret < 0)
> return ret;
> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> indio_dev->info = &bmi160_info;
>
> ret = iio_triggered_buffer_setup(indio_dev, NULL,
> - bmi160_trigger_handler, NULL);
> + bmi160_trigger_handler,
> + &bmi160_buffer_ops);
> if (ret < 0)
> goto uninit;
>
> + if (data->irq > 0) {
> + /* Check which interrupt pin is connected to our board */
> + irq2 = of_irq_get_byname(dev->of_node, "INT2");
> + if (irq2 == data->irq) {
> + dev_dbg(dev, "Using interrupt line INT2\n");
> + data->irq_data = &bmi160_irq2_data;
> + } else {
> + dev_dbg(dev, "Using interrupt line INT1\n");
> + data->irq_data = &bmi160_irq1_data;
> + }
> +
> + ret = devm_request_threaded_irq(dev,
> + data->irq,
> + bmi160_irq_handler,
> + bmi160_irq_thread_handler,
> + IRQF_ONESHOT,
> + BMI160_IRQ_NAME,
> + indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = bmi160_enable_irq(data);
> + if (ret < 0)
> + goto buffer_cleanup;
> +
> + if (block_supported) {
> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> + indio_dev->info = &bmi160_info_fifo;
> + indio_dev->buffer->attrs = bmi160_fifo_attributes;
> + data->fifo_buffer = devm_kmalloc(dev,
> + BMI160_FIFO_LENGTH,
> + GFP_KERNEL);
> + if (!data->fifo_buffer) {
> + ret = -ENOMEM;
> + goto buffer_cleanup;
> + }
> + }
> + }
> +
> ret = iio_device_register(indio_dev);
> if (ret < 0)
> goto buffer_cleanup;
> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
> index 07a179d..aa63f89 100644
> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client *client,
> {
> struct regmap *regmap;
> const char *name = NULL;
> + bool block_supported =
> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
> + i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>
> regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
> if (IS_ERR(regmap)) {
> @@ -34,7 +38,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, client->irq,
> + false, block_supported);
> }
>
> static int bmi160_i2c_remove(struct i2c_client *client)
> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
> index 1ec8b12..9b57fbe 100644
> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
> @@ -25,7 +25,8 @@ 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, spi->irq,
> + true, true);
> }
>
> static int bmi160_spi_remove(struct spi_device *spi)
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
2016-11-03 11:25 ` Marcin Niestroj
@ 2016-11-06 12:41 ` Jonathan Cameron
-1 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-06 12:41 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 03/11/16 11:25, Marcin Niestroj wrote:
> This adds documentation for Bosch BMI160 Inertial Measurement Unit
> device-tree bindings.
>
> Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
Unless I missed it in the previous patch we should also have of tables
added to the i2c and spi files (which is why the various tests haven't
been screaming at me that this device doesn't have documented bindings).
Otherwise, the use of interrupt names to indicate which pin on the chip
is a little unusual (if you cribbed this from somewhere I've forgotten
about then do say so!), so will want a devicetree bindings maintainer
input on this.
Thanks,
Jonathan
> ---
> .../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> new file mode 100644
> index 0000000..b02ef3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> @@ -0,0 +1,34 @@
> +Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
> +and externally connectable Magnetometer
> +
> +https://www.bosch-sensortec.com/bst/products/all_products/bmi160
> +
> +Required properties:
> + - compatible : should be "bosch,bmi160"
> + - reg : the I2C address or SPI chip select number of the sensor
> + - spi-max-frequency : set maximum clock frequency (only for SPI)
> +
> +Optional properties:
> + - interrupt-parent : should be the phandle of the interrupt controller
> + - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
> + - interrupt-names : set to "INT2" if using INT2 pin
> +
> +Examples:
> +
> +bmi160@68 {
> + compatible = "bosch,bmi160";
> + reg = <0x68>;
> +
> + interrupt-parent = <&gpio4>;
> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
> +};
> +
> +bmi160@0 {
> + compatible = "bosch,bmi160";
> + reg = <0>;
> + spi-max-frequency = <10000000>;
> +
> + interrupt-parent = <&gpio2>;
> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-names = "INT2";
> +};
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
@ 2016-11-06 12:41 ` Jonathan Cameron
0 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-06 12:41 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio, devicetree
On 03/11/16 11:25, Marcin Niestroj wrote:
> This adds documentation for Bosch BMI160 Inertial Measurement Unit
> device-tree bindings.
>
> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
Unless I missed it in the previous patch we should also have of tables
added to the i2c and spi files (which is why the various tests haven't
been screaming at me that this device doesn't have documented bindings).
Otherwise, the use of interrupt names to indicate which pin on the chip
is a little unusual (if you cribbed this from somewhere I've forgotten
about then do say so!), so will want a devicetree bindings maintainer
input on this.
Thanks,
Jonathan
> ---
> .../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> new file mode 100644
> index 0000000..b02ef3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> @@ -0,0 +1,34 @@
> +Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
> +and externally connectable Magnetometer
> +
> +https://www.bosch-sensortec.com/bst/products/all_products/bmi160
> +
> +Required properties:
> + - compatible : should be "bosch,bmi160"
> + - reg : the I2C address or SPI chip select number of the sensor
> + - spi-max-frequency : set maximum clock frequency (only for SPI)
> +
> +Optional properties:
> + - interrupt-parent : should be the phandle of the interrupt controller
> + - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
> + - interrupt-names : set to "INT2" if using INT2 pin
> +
> +Examples:
> +
> +bmi160@68 {
> + compatible = "bosch,bmi160";
> + reg = <0x68>;
> +
> + interrupt-parent = <&gpio4>;
> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
> +};
> +
> +bmi160@0 {
> + compatible = "bosch,bmi160";
> + reg = <0>;
> + spi-max-frequency = <10000000>;
> +
> + interrupt-parent = <&gpio2>;
> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-names = "INT2";
> +};
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-03 12:09 ` Peter Meerwald-Stadler
@ 2016-11-09 14:16 ` Marcin Niestroj
2016-11-09 17:16 ` Marcin Niestroj
2016-11-12 15:53 ` Jonathan Cameron
0 siblings, 2 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-09 14:16 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
linux-iio
Hi,
Thanks for review, below are my comments.
On 03.11.2016 13:09, Peter Meerwald-Stadler wrote:
>
>> This patch was developed primarily based on bmc150_accel hardware fifo
>> implementation.
>
> parts of the patch are cleanup and bugfixing; should be separate?
>
> more comments below
>
>> IRQ handler was added, which for now is responsible only for handling
>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>> default INT is considered to be connected. If INT2 is used instead, the
>> interrupt-names device-tree property can be used to specify that.
>>
>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>> ---
>> drivers/iio/imu/bmi160/bmi160.h | 3 +-
>> drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
>> drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
>> drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
>> 4 files changed, 618 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
>> index d2ae6ed..4a7c10e 100644
>> --- a/drivers/iio/imu/bmi160/bmi160.h
>> +++ b/drivers/iio/imu/bmi160/bmi160.h
>> @@ -4,7 +4,8 @@
>> 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, int irq,
>> + bool use_spi, bool block_supported);
>> void bmi160_core_remove(struct device *dev);
>>
>> #endif /* BMI160_H_ */
>> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
>> index e0251b8..153734c 100644
>> --- a/drivers/iio/imu/bmi160/bmi160_core.c
>> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
>> @@ -2,6 +2,7 @@
>> * BMI160 - Bosch IMU (accel, gyro plus external magnetometer)
>> *
>> * Copyright (c) 2016, Intel Corporation.
>> + * Copyright (c) 2016, Grinn
>> *
>> * 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
>> @@ -9,7 +10,7 @@
>> *
>> * IIO core driver for BMI160, with support for I2C/SPI busses
>> *
>> - * TODO: magnetometer, interrupts, hardware FIFO
>> + * TODO: magnetometer, interrupts
>> */
>> #include <linux/module.h>
>> #include <linux/regmap.h>
>> @@ -22,8 +23,12 @@
>> #include <linux/iio/buffer.h>
>> #include <linux/iio/sysfs.h>
>>
>> +#include <linux/of_irq.h>
>> +
>> #include "bmi160.h"
>>
>> +#define BMI160_IRQ_NAME "bmi160_event"
>> +
>> #define BMI160_REG_CHIP_ID 0x00
>> #define BMI160_CHIP_ID_VAL 0xD1
>>
>> @@ -34,6 +39,21 @@
>> #define BMI160_REG_DATA_GYRO_XOUT_L 0x0C
>> #define BMI160_REG_DATA_ACCEL_XOUT_L 0x12
>>
>> +#define BMI160_REG_STATUS 0x1B
>> +#define BMI160_STATUS_MAG_MAN_OP BIT(2)
>> +
>> +#define BMI160_REG_INT_STATUS0 0x1C
>> +
>> +#define BMI160_REG_INT_STATUS1 0x1D
>> +#define BMI160_INT_STATUS_FWM BIT(6)
>> +
>> +#define BMI160_REG_INT_STATUS2 0x1E
>> +
>> +#define BMI160_REG_INT_STATUS3 0x1F
>> +
>> +#define BMI160_REG_FIFO_LENGTH 0x22
>> +#define BMI160_REG_FIFO_DATA 0x24
>> +
>> #define BMI160_REG_ACCEL_CONFIG 0x40
>> #define BMI160_ACCEL_CONFIG_ODR_MASK GENMASK(3, 0)
>> #define BMI160_ACCEL_CONFIG_BWP_MASK GENMASK(6, 4)
>> @@ -55,6 +75,36 @@
>> #define BMI160_GYRO_RANGE_250DPS 0x03
>> #define BMI160_GYRO_RANGE_125DPS 0x04
>>
>> +#define BMI160_REG_FIFO_CONFIG_0 0x46
>> +
>> +#define BMI160_REG_FIFO_CONFIG_1 0x47
>> +#define BMI160_FIFO_GYRO_EN BIT(7)
>> +#define BMI160_FIFO_ACCEL_EN BIT(6)
>> +#define BMI160_FIFO_MAGN_EN BIT(5)
>> +#define BMI160_FIFO_HEADER_EN BIT(4)
>> +#define BMI160_FIFO_TAG_INT1_EN BIT(3)
>> +#define BMI160_FIFO_TAG_INT2_EN BIT(2)
>> +#define BMI160_FIFO_TIME_EN BIT(1)
>> +
>> +#define BMI160_REG_INT_EN_1 0x51
>> +#define BMI160_INT_FWM_EN BIT(6)
>> +#define BMI160_INT_FFULL_EN BIT(5)
>> +#define BMI160_INT_DRDY_EN BIT(4)
>> +
>> +#define BMI160_REG_INT_OUT_CTRL 0x53
>> +#define BMI160_INT2_OUTPUT_EN BIT(7)
>> +#define BMI160_INT1_OUTPUT_EN BIT(3)
>> +
>> +#define BMI160_REG_INT_LATCH 0x54
>> +
>> +#define BMI160_REG_INT_MAP_1 0x56
>> +#define BMI160_INT1_MAP_DRDY BIT(7)
>> +#define BMI160_INT1_MAP_FWM BIT(6)
>> +#define BMI160_INT1_MAP_FFULL BIT(5)
>> +#define BMI160_INT2_MAP_DRDY BIT(3)
>> +#define BMI160_INT2_MAP_FWM BIT(2)
>> +#define BMI160_INT2_MAP_FFULL BIT(1)
>> +
>> #define BMI160_REG_CMD 0x7E
>> #define BMI160_CMD_ACCEL_PM_SUSPEND 0x10
>> #define BMI160_CMD_ACCEL_PM_NORMAL 0x11
>> @@ -66,6 +116,8 @@
>>
>> #define BMI160_REG_DUMMY 0x7F
>>
>> +#define BMI160_FIFO_LENGTH 1024
>> +
>> #define BMI160_ACCEL_PMU_MIN_USLEEP 3200
>> #define BMI160_ACCEL_PMU_MAX_USLEEP 3800
>> #define BMI160_GYRO_PMU_MIN_USLEEP 55000
>> @@ -110,8 +162,33 @@ enum bmi160_sensor_type {
>> BMI160_NUM_SENSORS /* must be last */
>> };
>>
>> +struct bmi160_irq_data {
>> + unsigned int map_fwm;
>> + unsigned int output_en;
>> +};
>> +
>> +static const struct bmi160_irq_data bmi160_irq1_data = {
>> + .map_fwm = BMI160_INT1_MAP_FWM,
>> + .output_en = BMI160_INT1_OUTPUT_EN,
>> +};
>> +
>> +static const struct bmi160_irq_data bmi160_irq2_data = {
>> + .map_fwm = BMI160_INT2_MAP_FWM,
>> + .output_en = BMI160_INT2_OUTPUT_EN,
>> +};
>> +
>> struct bmi160_data {
>> struct regmap *regmap;
>> + struct mutex mutex;
>> + const struct bmi160_irq_data *irq_data;
>> + int irq;
>> + int64_t timestamp;
>> + int64_t fifo_sample_period;
>> + bool fifo_enabled;
>> + unsigned int fifo_config;
>> + unsigned int fifo_sample_size;
>> + u8 *fifo_buffer;
>> + unsigned int watermark;
>> };
>>
>> const struct regmap_config bmi160_regmap_config = {
>> @@ -159,11 +236,11 @@ struct bmi160_pmu_time {
>> static struct bmi160_pmu_time bmi160_pmu_time[] = {
>> [BMI160_ACCEL] = {
>> .min = BMI160_ACCEL_PMU_MIN_USLEEP,
>> - .max = BMI160_ACCEL_PMU_MAX_USLEEP
>> + .max = BMI160_ACCEL_PMU_MAX_USLEEP,
>
> this is cosmetic cleanup
>
>> },
>> [BMI160_GYRO] = {
>> .min = BMI160_GYRO_PMU_MIN_USLEEP,
>> - .max = BMI160_GYRO_PMU_MIN_USLEEP,
>
> this looks like bug fixing
I agree this should be a separate patch.
I looked more deeply into the datasheet (2.11.38 Register 0x7E CMD).
What we have specified in code as MIN value is in datasheet specified as
typical time the command is executed. MAX value in the code corresponds
to datasheet's maximum time the command takes place.
According to that I think we should wait *at least* the maximum time
specified in datasheet. Please correct me if I am wrong.
>
>> + .max = BMI160_GYRO_PMU_MAX_USLEEP,
>> },
>> };
>>
>> @@ -285,7 +362,9 @@ int bmi160_set_mode(struct bmi160_data *data, enum bmi160_sensor_type t,
>> else
>> cmd = bmi160_regs[t].pmu_cmd_suspend;
>>
>> + mutex_lock(&data->mutex);
>
> what does the mutex protect?
> is it also needed without the fifo/irq support?
> probably split out as a separate patch
It protects data transmission on regmap_* functions. They are used from
sysfs access functions as well as from bmi160_trigger_handler. I will
create a separate bugfix patch for it.
Additionally fifo_enabled and fifo_buffer are protected by mutex, after
adding fifo support.
>
>> ret = regmap_write(data->regmap, BMI160_REG_CMD, cmd);
>> + mutex_unlock(&data->mutex);
>> if (ret < 0)
>> return ret;
>>
>> @@ -298,6 +377,7 @@ static
>> int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
>> int uscale)
>> {
>> + int ret;
>> int i;
>>
>> for (i = 0; i < bmi160_scale_table[t].num; i++)
>> @@ -307,8 +387,12 @@ int bmi160_set_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
>> if (i == bmi160_scale_table[t].num)
>> return -EINVAL;
>>
>> - return regmap_write(data->regmap, bmi160_regs[t].range,
>> - bmi160_scale_table[t].tbl[i].bits);
>> + mutex_lock(&data->mutex);
>> + ret = regmap_write(data->regmap, bmi160_regs[t].range,
>> + bmi160_scale_table[t].tbl[i].bits);
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
>> }
>>
>> static
>> @@ -317,7 +401,9 @@ int bmi160_get_scale(struct bmi160_data *data, enum bmi160_sensor_type t,
>> {
>> int i, ret, val;
>>
>> + mutex_lock(&data->mutex);
>> ret = regmap_read(data->regmap, bmi160_regs[t].range, &val);
>> + mutex_unlock(&data->mutex);
>> if (ret < 0)
>> return ret;
>>
>> @@ -340,7 +426,9 @@ static int bmi160_get_data(struct bmi160_data *data, int chan_type,
>>
>> reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(__le16);
>>
>> + mutex_lock(&data->mutex);
>> ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(__le16));
>> + mutex_unlock(&data->mutex);
>> if (ret < 0)
>> return ret;
>>
>> @@ -353,6 +441,7 @@ static
>> int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
>> int odr, int uodr)
>> {
>> + int ret;
>> int i;
>>
>> for (i = 0; i < bmi160_odr_table[t].num; i++)
>> @@ -363,20 +452,30 @@ int bmi160_set_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
>> if (i >= bmi160_odr_table[t].num)
>> return -EINVAL;
>>
>> - return regmap_update_bits(data->regmap,
>> - bmi160_regs[t].config,
>> - bmi160_regs[t].config_odr_mask,
>> - bmi160_odr_table[t].tbl[i].bits);
>> + mutex_lock(&data->mutex);
>> + ret = regmap_update_bits(data->regmap,
>> + bmi160_regs[t].config,
>> + bmi160_regs[t].config_odr_mask,
>> + bmi160_odr_table[t].tbl[i].bits);
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
>> }
>>
>> -static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
>> - int *odr, int *uodr)
>> +static int64_t bmi160_frequency_to_period(int odr, int uodr)
>> {
>> - int i, val, ret;
>> + uint64_t period = 1000000000000000;
>> + int64_t frequency = (int64_t) odr * 1000000 + uodr;
>>
>> - ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
>> - if (ret < 0)
>> - return ret;
>> + do_div(period, frequency);
>> +
>> + return period;
>> +}
>> +
>> +static const struct bmi160_odr *bmi160_reg_to_odr(enum bmi160_sensor_type t,
>> + unsigned int val)
>> +{
>> + int i;
>>
>> val &= bmi160_regs[t].config_odr_mask;
>>
>> @@ -385,10 +484,52 @@ static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
>> break;
>>
>> if (i >= bmi160_odr_table[t].num)
>> - return -EINVAL;
>> + return ERR_PTR(-EINVAL);
>> +
>> + return &bmi160_odr_table[t].tbl[i];
>> +}
>> +
>> +static int bmi160_get_sample_period(struct bmi160_data *data,
>> + enum bmi160_sensor_type t,
>> + int64_t *sample_period)
>> +{
>> + const struct bmi160_odr *odr_entry;
>> + int ret;
>> + unsigned int val;
>> +
>
> no mutex here?
This function should be called when mutex is already acquired. Should be
propably renamed to __bmi160_*.
>
>> + ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
>> + if (ret < 0)
>> + return ret;
>>
>> - *odr = bmi160_odr_table[t].tbl[i].odr;
>> - *uodr = bmi160_odr_table[t].tbl[i].uodr;
>> + odr_entry = bmi160_reg_to_odr(t, val);
>> + if (IS_ERR(odr_entry))
>> + return PTR_ERR(odr_entry);
>> +
>> + *sample_period = bmi160_frequency_to_period(odr_entry->odr,
>> + odr_entry->uodr);
>> +
>> + return 0;
>> +}
>> +
>> +static int bmi160_get_odr(struct bmi160_data *data, enum bmi160_sensor_type t,
>> + int *odr, int *uodr)
>> +{
>> + const struct bmi160_odr *odr_entry;
>> + int ret;
>> + unsigned int val;
>> +
>> + mutex_lock(&data->mutex);
>> + ret = regmap_read(data->regmap, bmi160_regs[t].config, &val);
>> + mutex_unlock(&data->mutex);
>> + if (ret < 0)
>> + return ret;
>> +
>> + odr_entry = bmi160_reg_to_odr(t, val);
>> + if (IS_ERR(odr_entry))
>> + return PTR_ERR(odr_entry);
>> +
>> + *odr = odr_entry->odr;
>> + *uodr = odr_entry->uodr;
>>
>> return 0;
>> }
>> @@ -402,14 +543,18 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
>> int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
>> __le16 sample;
>>
>> + mutex_lock(&data->mutex);
>> for_each_set_bit(i, indio_dev->active_scan_mask,
>> indio_dev->masklength) {
>> ret = regmap_bulk_read(data->regmap, base + i * sizeof(__le16),
>> &sample, sizeof(__le16));
>> - if (ret < 0)
>> + if (ret < 0) {
>> + mutex_unlock(&data->mutex);
>> goto done;
>> + }
>> buf[j++] = sample;
>> }
>> + mutex_unlock(&data->mutex);
>>
>> iio_push_to_buffers_with_timestamp(indio_dev, buf,
>> iio_get_time_ns(indio_dev));
>> @@ -493,11 +638,364 @@ static const struct attribute_group bmi160_attrs_group = {
>> .attrs = bmi160_attrs,
>> };
>>
>> +static int bmi160_update_sample_period(struct bmi160_data *data,
>> + enum bmi160_sensor_type sensor_type)
>> +{
>> + struct device *dev = regmap_get_device(data->regmap);
>> + int64_t sample_period;
>> + int ret;
>> +
>> + ret = bmi160_get_sample_period(data, sensor_type, &sample_period);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (data->fifo_sample_period) {
>> + if (data->fifo_sample_period != sample_period) {
>> + dev_warn(dev, "Enabled sensors have unequal ODR values\n");
>> + return -EINVAL;
>> + }
>> + } else {
>> + data->fifo_sample_period = sample_period;
>
> brackets not needed
Sure.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int bmi160_fifo_enable(struct iio_dev *indio_dev,
>> + struct bmi160_data *data)
>> +{
>> + struct regmap *regmap = data->regmap;
>> + struct device *dev = regmap_get_device(regmap);
>> + int ret;
>> + int i;
>> + unsigned int val;
>> + unsigned int fifo_config = 0;
>> +
>> + /* Set fifo sample size and period */
>> + for_each_set_bit(i, indio_dev->active_scan_mask,
>> + indio_dev->masklength) {
>> + if (i <= BMI160_SCAN_GYRO_Z)
>> + fifo_config |= BMI160_FIFO_GYRO_EN;
>> + else if (i <= BMI160_SCAN_ACCEL_Z)
>> + fifo_config |= BMI160_FIFO_ACCEL_EN;
>> + }
>> +
>> + data->fifo_sample_period = 0;
>> + data->fifo_sample_size = 0;
>> + if (fifo_config & BMI160_FIFO_GYRO_EN) {
>> + data->fifo_sample_size += 6;
>> + ret = bmi160_update_sample_period(data, BMI160_GYRO);
>> + if (ret < 0)
>> + return ret;
>> + }
>> + if (fifo_config & BMI160_FIFO_ACCEL_EN) {
>> + data->fifo_sample_size += 6;
>> + ret = bmi160_update_sample_period(data, BMI160_ACCEL);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + /*
>> + * Set watermark level and write real value back, as it will be used
>> + * in timestamp calculation.
>> + */
>> + val = data->watermark * data->fifo_sample_size;
>> + if (val > BMI160_FIFO_LENGTH - 1) {
>> + val = BMI160_FIFO_LENGTH - 1;
>> + data->watermark = val / data->fifo_sample_size;
>> + }
>> + val = data->watermark * data->fifo_sample_size / 4;
>> +
>> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_0, val);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to set watermark\n");
>> + return ret;
>> + }
>> +
>> + /* Enable FIFO channels */
>> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1,
>> + fifo_config);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
>> + return ret;
>> + }
>> +
>> + data->fifo_config = fifo_config;
>> + data->fifo_enabled = true;
>> +
>> + return 0;
>> +}
>> +
>> +static int bmi160_fifo_disable(struct bmi160_data *data)
>> +{
>> + struct regmap *regmap = data->regmap;
>> + struct device *dev = regmap_get_device(regmap);
>> + int ret;
>> +
>> + /* Disable all FIFO channels */
>> + ret = regmap_write(regmap, BMI160_REG_FIFO_CONFIG_1, 0);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to write FIFO_CONFIG_1\n");
>> + return ret;
>> + }
>> +
>> + data->fifo_enabled = false;
>> +
>> + return 0;
>> +}
>> +
>> +static int bmi160_buffer_postenable(struct iio_dev *indio_dev)
>> +{
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
>> + return iio_triggered_buffer_postenable(indio_dev);
>> +
>> + mutex_lock(&data->mutex);
>> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_MAP_1,
>> + data->irq_data->map_fwm, data->irq_data->map_fwm);
>> + if (ret < 0)
>> + goto unlock;
>> +
>> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_EN_1,
>> + BMI160_INT_FWM_EN, BMI160_INT_FWM_EN);
>> + if (ret < 0)
>> + goto unlock;
>> +
>> + ret = bmi160_fifo_enable(indio_dev, data);
>> +
>> +unlock:
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static int bmi160_buffer_predisable(struct iio_dev *indio_dev)
>> +{
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> + struct regmap *regmap = data->regmap;
>> + int ret = 0;
>> +
>> + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
>> + return iio_triggered_buffer_predisable(indio_dev);
>> +
>> + mutex_lock(&data->mutex);
>> +
>> + ret = regmap_update_bits(regmap, BMI160_REG_INT_EN_1,
>> + BMI160_INT_FWM_EN, 0);
>> + if (ret < 0)
>> + goto unlock;
>> +
>> + ret = bmi160_fifo_disable(data);
>> +
>> +unlock:
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct iio_buffer_setup_ops bmi160_buffer_ops = {
>> + .postenable = bmi160_buffer_postenable,
>> + .predisable = bmi160_buffer_predisable,
>> +};
>> +
>> +static ssize_t bmi160_get_fifo_state(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> + bool state;
>> +
>> + mutex_lock(&data->mutex);
>> + state = data->fifo_enabled;
>> + mutex_unlock(&data->mutex);
>> +
>> + return sprintf(buf, "%d\n", (int) state);
>> +}
>> +
>> +static ssize_t bmi160_get_fifo_watermark(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> + int wm;
>> +
>> + mutex_lock(&data->mutex);
>> + wm = data->watermark;
>> + mutex_unlock(&data->mutex);
>> +
>> + return sprintf(buf, "%d\n", wm);
>> +}
>> +
>> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
>> +static IIO_CONST_ATTR(hwfifo_watermark_max,
>> + __stringify(BMI160_FIFO_LENGTH));
>> +static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
>> + bmi160_get_fifo_state, NULL, 0);
>> +static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
>> + bmi160_get_fifo_watermark, NULL, 0);
>> +
>> +static const struct attribute *bmi160_fifo_attributes[] = {
>> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
>> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
>> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
>> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
>> + NULL,
>> +};
>> +
>> +static int bmi160_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>> +{
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> +
>> + if (val > BMI160_FIFO_LENGTH)
>> + val = BMI160_FIFO_LENGTH;
>> +
>> + data->watermark = val;
>> +
>> + return 0;
>> +}
>> +
>> +static int bmi160_fifo_transfer(struct bmi160_data *data,
>> + char *buffer, int num_bytes)
>> +{
>> + struct regmap *regmap = data->regmap;
>> + struct device *dev = regmap_get_device(regmap);
>> + size_t step = regmap_get_raw_read_max(regmap);
>> + int ret = 0;
>> + int i;
>> +
>> + if (!step || step > num_bytes)
>> + step = num_bytes;
>> + else if (step < num_bytes)
>> + step = data->fifo_sample_size;
>> +
>> + for (i = 0; i < num_bytes; i += step) {
>> + ret = regmap_raw_read(regmap, BMI160_REG_FIFO_DATA,
>> + &buffer[i], step);
>> +
>> + if (ret)
>> + break;
>> + }
>> +
>> + if (ret)
>> + dev_err(dev,
>> + "Error transferring data from fifo in single steps of %zu\n",
>> + step);
>> +
>> + return ret;
>> +}
>> +
>> +static int __bmi160_fifo_flush(struct iio_dev *indio_dev,
>> + unsigned int samples, bool irq)
>
> what does the __ prefix denote?
> likely that the function is supposed to be called with mutex held --
> maybe use that convention elsewhere as well?
I will rename it together with other functions that assume mutex already
acquired.
>
>> +{
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> + struct regmap *regmap = data->regmap;
>> + struct device *dev = regmap_get_device(regmap);
>> + int ret;
>> + __le16 fifo_length;
>> + unsigned int fifo_samples;
>> + unsigned int fifo_bytes;
>> + u8 *buffer = data->fifo_buffer;
>> + u8 *buffer_iter;
>> + int64_t last_timestamp, timestamp;
>> + unsigned int last_samples;
>> + unsigned int i;
>> +
>> + /* Get the current FIFO length */
>> + ret = regmap_bulk_read(regmap, BMI160_REG_FIFO_LENGTH,
>> + &fifo_length, sizeof(__le16));
>> + if (ret < 0) {
>> + dev_err(dev, "Error reading FIFO_LENGTH\n");
>> + return ret;
>> + }
>> +
>> + fifo_bytes = le16_to_cpu(fifo_length);
>> + fifo_samples = fifo_bytes / data->fifo_sample_size;
>> +
>> + if (fifo_bytes % data->fifo_sample_size)
>> + dev_warn(dev, "fifo_bytes %u is not dividable by %u\n",
>> + fifo_bytes, data->fifo_sample_size);
>> +
>> + if (!fifo_samples)
>> + return 0;
>> +
>> + if (samples && fifo_samples > samples) {
>> + fifo_samples = samples;
>> + fifo_bytes = fifo_samples * data->fifo_sample_size;
>> + }
>> +
>> + /*
>> + * If we are not called from IRQ, it means that we are flushing data
>> + * on demand. In that case we do not have latest timestamp saved in
>> + * data->timestamp. Get the time now instead.
>> + *
>> + * In case of IRQ flush, saved timestamp shows the time when number
>> + * of samples configured by watermark were ready. Currently there might
>> + * be more samples already.
>> + * If we are not called from IRQ, than we are getting the current fifo
>> + * length, as we are setting timestamp just after getting it.
>> + */
>> + if (!irq) {
>> + last_timestamp = iio_get_time_ns(indio_dev);
>> + last_samples = fifo_samples;
>> + } else {
>> + last_timestamp = data->timestamp;
>> + last_samples = data->watermark;
>> + }
>> +
>> + /* Get all measurements */
>> + ret = bmi160_fifo_transfer(data, buffer, fifo_bytes);
>> + if (ret)
>> + return ret;
>> +
>> + /* Handle demux */
>> + timestamp = last_timestamp - (last_samples * data->fifo_sample_period);
>> + buffer_iter = buffer;
>> + for (i = 0; i < fifo_samples; i++) {
>> + u8 tmp_buf[indio_dev->scan_bytes];
>
> non-constant array size, is this allowed these days?
I am not sure if this is permitted in kernel. [1] says that ISO C99 adds
arrays of variable length.
The reason I have used it is that iio_push_to_buffers_with_timestamp
uses indio_dev->scan_bytes to write timestamp to last element in array.
One of the possiblities to get rid of non-constant array size is to
determine maximum size. Is that what I should do?
[1] https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html
>
>> +
>> + memcpy(tmp_buf, buffer_iter, data->fifo_sample_size);
>> +
>> + timestamp += data->fifo_sample_period;
>> + iio_push_to_buffers_with_timestamp(indio_dev,
>> + tmp_buf,
>> + timestamp);
>> +
>> + buffer_iter += data->fifo_sample_size;
>> + }
>> +
>> + return fifo_samples;
>> +}
>> +
>> +static int bmi160_fifo_flush(struct iio_dev *indio_dev, unsigned int samples)
>> +{
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + mutex_lock(&data->mutex);
>> + ret = __bmi160_fifo_flush(indio_dev, samples, false);
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
>> +}
>> +
>> static const struct iio_info bmi160_info = {
>> - .driver_module = THIS_MODULE,
>> - .read_raw = bmi160_read_raw,
>> - .write_raw = bmi160_write_raw,
>> - .attrs = &bmi160_attrs_group,
>> + .driver_module = THIS_MODULE,
>> + .read_raw = bmi160_read_raw,
>> + .write_raw = bmi160_write_raw,
>> + .attrs = &bmi160_attrs_group,
>> +};
>> +
>> +static const struct iio_info bmi160_info_fifo = {
>> + .driver_module = THIS_MODULE,
>> + .read_raw = bmi160_read_raw,
>> + .write_raw = bmi160_write_raw,
>> + .attrs = &bmi160_attrs_group,
>> + .hwfifo_set_watermark = bmi160_set_watermark,
>> + .hwfifo_flush_to_buffer = bmi160_fifo_flush,
>> };
>>
>> static const char *bmi160_match_acpi_device(struct device *dev)
>> @@ -561,12 +1059,54 @@ static void bmi160_chip_uninit(struct bmi160_data *data)
>> bmi160_set_mode(data, BMI160_ACCEL, false);
>> }
>>
>> +static int bmi160_enable_irq(struct bmi160_data *data)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&data->mutex);
>> + ret = regmap_update_bits(data->regmap, BMI160_REG_INT_OUT_CTRL,
>> + data->irq_data->output_en,
>> + data->irq_data->output_en);
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static irqreturn_t bmi160_irq_thread_handler(int irq, void *p)
>> +{
>> + struct iio_dev *indio_dev = p;
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> + struct device *dev = regmap_get_device(data->regmap);
>> +
>> + mutex_lock(&data->mutex);
>> + if (data->fifo_enabled)
>> + __bmi160_fifo_flush(indio_dev, BMI160_FIFO_LENGTH, true);
>> + else
>> + dev_warn(dev,
>> + "IRQ has been triggered, but FIFO is not enabled.\n");
>> + mutex_unlock(&data->mutex);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t bmi160_irq_handler(int irq, void *p)
>> +{
>> + struct iio_dev *indio_dev = p;
>> + struct bmi160_data *data = iio_priv(indio_dev);
>> +
>> + data->timestamp = iio_get_time_ns(indio_dev);
>> +
>> + return IRQ_WAKE_THREAD;
>> +}
>> +
>> int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>> - const char *name, bool use_spi)
>> + const char *name, int irq,
>> + bool use_spi, bool block_supported)
>> {
>> struct iio_dev *indio_dev;
>> struct bmi160_data *data;
>> int ret;
>> + int irq2;
>>
>> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> if (!indio_dev)
>> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>>
>> data = iio_priv(indio_dev);
>> dev_set_drvdata(dev, indio_dev);
>> + data->irq = irq;
>> data->regmap = regmap;
>>
>> + mutex_init(&data->mutex);
>> +
>> ret = bmi160_chip_init(data, use_spi);
>> if (ret < 0)
>> return ret;
>> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>> indio_dev->info = &bmi160_info;
>>
>> ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> - bmi160_trigger_handler, NULL);
>> + bmi160_trigger_handler,
>> + &bmi160_buffer_ops);
>> if (ret < 0)
>> goto uninit;
>>
>> + if (data->irq > 0) {
>> + /* Check which interrupt pin is connected to our board */
>> + irq2 = of_irq_get_byname(dev->of_node, "INT2");
>> + if (irq2 == data->irq) {
>> + dev_dbg(dev, "Using interrupt line INT2\n");
>> + data->irq_data = &bmi160_irq2_data;
>> + } else {
>> + dev_dbg(dev, "Using interrupt line INT1\n");
>> + data->irq_data = &bmi160_irq1_data;
>> + }
>> +
>> + ret = devm_request_threaded_irq(dev,
>> + data->irq,
>> + bmi160_irq_handler,
>> + bmi160_irq_thread_handler,
>> + IRQF_ONESHOT,
>> + BMI160_IRQ_NAME,
>> + indio_dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = bmi160_enable_irq(data);
>> + if (ret < 0)
>> + goto buffer_cleanup;
>> +
>> + if (block_supported) {
>> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
>> + indio_dev->info = &bmi160_info_fifo;
>> + indio_dev->buffer->attrs = bmi160_fifo_attributes;
>> + data->fifo_buffer = devm_kmalloc(dev,
>> + BMI160_FIFO_LENGTH,
>> + GFP_KERNEL);
>> + if (!data->fifo_buffer) {
>> + ret = -ENOMEM;
>
> need to disable irq on failure?
Yes, I missed that.
>
>> + goto buffer_cleanup;
>> + }
>> + }
>> + }
>> +
>> ret = iio_device_register(indio_dev);
>> if (ret < 0)
>> goto buffer_cleanup;
>> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c
>> index 07a179d..aa63f89 100644
>> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
>> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
>> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client *client,
>> {
>> struct regmap *regmap;
>> const char *name = NULL;
>> + bool block_supported =
>> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
>> + i2c_check_functionality(client->adapter,
>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>>
>> regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
>> if (IS_ERR(regmap)) {
>> @@ -34,7 +38,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, client->irq,
>> + false, block_supported);
>> }
>>
>> static int bmi160_i2c_remove(struct i2c_client *client)
>> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
>> index 1ec8b12..9b57fbe 100644
>> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
>> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
>> @@ -25,7 +25,8 @@ 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, spi->irq,
>> + true, true);
>> }
>>
>> static int bmi160_spi_remove(struct spi_device *spi)
>>
>
--
Marcin Niestroj
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-06 12:35 ` Jonathan Cameron
@ 2016-11-09 14:52 ` Marcin Niestroj
-1 siblings, 0 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-09 14:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Hi,
Thanks for review. I agree with all of the comments and will fix these
in next patch version. Below is just a comment on hwfifo_* sysfs access.
On 06.11.2016 13:35, Jonathan Cameron wrote:
> On 03/11/16 11:25, Marcin Niestroj wrote:
>> This patch was developed primarily based on bmc150_accel hardware fifo
>> implementation.
>>
>> IRQ handler was added, which for now is responsible only for handling
>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>> default INT is considered to be connected. If INT2 is used instead, the
>> interrupt-names device-tree property can be used to specify that.
>>
>> Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
> I agree with Peter that there should have been a few precursor patches
> to this one doing various cleanups and reworking bits that you have
> in here. Would have made it easier to review (always a good thing :)
>
> In general the resulting code looks good to me. A few little
> additional comments inline from me. Mostly about small code ordering things
> and function rename suggestions that would make the code more 'obviously'
> correct.
>
> Thanks,
>
> Jonathan
>> ---
>> drivers/iio/imu/bmi160/bmi160.h | 3 +-
>> drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
>> drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
>> drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
>> 4 files changed, 618 insertions(+), 28 deletions(-)
<snip>
>> +
>> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
>> +static IIO_CONST_ATTR(hwfifo_watermark_max,
>> + __stringify(BMI160_FIFO_LENGTH));
>> +static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
>> + bmi160_get_fifo_state, NULL, 0);
>> +static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
>> + bmi160_get_fifo_watermark, NULL, 0);
>> +
>> +static const struct attribute *bmi160_fifo_attributes[] = {
>> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
>> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
>> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
>> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> There are enough of these drivers now that sometimes soon we should
> revisit the question of pulling these into the core. Can certainly
> concieve of downstream consumer devices (in particular the iio_input
> bridge when that finally resurfaces - my fault) wanting to be able to
> manipulate or at least have visibilty of these.
One more thing to consider is setting hwfifo_watermark to other value
than "userspace" watermark. It would be nice to set hwfifo_watermark to
a *safe* value to be able to get all data from hardware to kfifo. By
safe I mean that the chance of hardware fifo overflow will be small. On
the other hand there might be no reason to have such small watermark for
userspace application, so we can save scheduler cycles.
<snip>
--
Marcin Niestroj
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
@ 2016-11-09 14:52 ` Marcin Niestroj
0 siblings, 0 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-09 14:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio, devicetree
Hi,
Thanks for review. I agree with all of the comments and will fix these
in next patch version. Below is just a comment on hwfifo_* sysfs access.
On 06.11.2016 13:35, Jonathan Cameron wrote:
> On 03/11/16 11:25, Marcin Niestroj wrote:
>> This patch was developed primarily based on bmc150_accel hardware fifo
>> implementation.
>>
>> IRQ handler was added, which for now is responsible only for handling
>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>> default INT is considered to be connected. If INT2 is used instead, the
>> interrupt-names device-tree property can be used to specify that.
>>
>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
> I agree with Peter that there should have been a few precursor patches
> to this one doing various cleanups and reworking bits that you have
> in here. Would have made it easier to review (always a good thing :)
>
> In general the resulting code looks good to me. A few little
> additional comments inline from me. Mostly about small code ordering things
> and function rename suggestions that would make the code more 'obviously'
> correct.
>
> Thanks,
>
> Jonathan
>> ---
>> drivers/iio/imu/bmi160/bmi160.h | 3 +-
>> drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
>> drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
>> drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
>> 4 files changed, 618 insertions(+), 28 deletions(-)
<snip>
>> +
>> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
>> +static IIO_CONST_ATTR(hwfifo_watermark_max,
>> + __stringify(BMI160_FIFO_LENGTH));
>> +static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
>> + bmi160_get_fifo_state, NULL, 0);
>> +static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
>> + bmi160_get_fifo_watermark, NULL, 0);
>> +
>> +static const struct attribute *bmi160_fifo_attributes[] = {
>> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
>> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
>> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
>> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> There are enough of these drivers now that sometimes soon we should
> revisit the question of pulling these into the core. Can certainly
> concieve of downstream consumer devices (in particular the iio_input
> bridge when that finally resurfaces - my fault) wanting to be able to
> manipulate or at least have visibilty of these.
One more thing to consider is setting hwfifo_watermark to other value
than "userspace" watermark. It would be nice to set hwfifo_watermark to
a *safe* value to be able to get all data from hardware to kfifo. By
safe I mean that the chance of hardware fifo overflow will be small. On
the other hand there might be no reason to have such small watermark for
userspace application, so we can save scheduler cycles.
<snip>
--
Marcin Niestroj
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
2016-11-06 12:41 ` Jonathan Cameron
@ 2016-11-09 15:18 ` Marcin Niestroj
-1 siblings, 0 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-09 15:18 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 06.11.2016 13:41, Jonathan Cameron wrote:
> On 03/11/16 11:25, Marcin Niestroj wrote:
>> This adds documentation for Bosch BMI160 Inertial Measurement Unit
>> device-tree bindings.
>>
>> Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
> Unless I missed it in the previous patch we should also have of tables
> added to the i2c and spi files (which is why the various tests haven't
> been screaming at me that this device doesn't have documented bindings).
Ok, I will add them.
>
> Otherwise, the use of interrupt names to indicate which pin on the chip
> is a little unusual (if you cribbed this from somewhere I've forgotten
> about then do say so!), so will want a devicetree bindings maintainer
> input on this.
I have used interrupt names similar as in other driver. Please see [1]
for it's DT documentation and [2] for implementation.
[1] Documentation/devicetree/bindings/iio/accel/mma8452.txt
[2] drivers/iio/accel/mma8452.c
>
> Thanks,
>
> Jonathan
>> ---
>> .../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>> new file mode 100644
>> index 0000000..b02ef3e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>> @@ -0,0 +1,34 @@
>> +Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
>> +and externally connectable Magnetometer
>> +
>> +https://www.bosch-sensortec.com/bst/products/all_products/bmi160
>> +
>> +Required properties:
>> + - compatible : should be "bosch,bmi160"
>> + - reg : the I2C address or SPI chip select number of the sensor
>> + - spi-max-frequency : set maximum clock frequency (only for SPI)
>> +
>> +Optional properties:
>> + - interrupt-parent : should be the phandle of the interrupt controller
>> + - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
>> + - interrupt-names : set to "INT2" if using INT2 pin
>> +
>> +Examples:
>> +
>> +bmi160@68 {
>> + compatible = "bosch,bmi160";
>> + reg = <0x68>;
>> +
>> + interrupt-parent = <&gpio4>;
>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>> +};
>> +
>> +bmi160@0 {
>> + compatible = "bosch,bmi160";
>> + reg = <0>;
>> + spi-max-frequency = <10000000>;
>> +
>> + interrupt-parent = <&gpio2>;
>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>> + interrupt-names = "INT2";
>> +};
>>
>
--
Marcin Niestroj
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
@ 2016-11-09 15:18 ` Marcin Niestroj
0 siblings, 0 replies; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-09 15:18 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio, devicetree
On 06.11.2016 13:41, Jonathan Cameron wrote:
> On 03/11/16 11:25, Marcin Niestroj wrote:
>> This adds documentation for Bosch BMI160 Inertial Measurement Unit
>> device-tree bindings.
>>
>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
> Unless I missed it in the previous patch we should also have of tables
> added to the i2c and spi files (which is why the various tests haven't
> been screaming at me that this device doesn't have documented bindings).
Ok, I will add them.
>
> Otherwise, the use of interrupt names to indicate which pin on the chip
> is a little unusual (if you cribbed this from somewhere I've forgotten
> about then do say so!), so will want a devicetree bindings maintainer
> input on this.
I have used interrupt names similar as in other driver. Please see [1]
for it's DT documentation and [2] for implementation.
[1] Documentation/devicetree/bindings/iio/accel/mma8452.txt
[2] drivers/iio/accel/mma8452.c
>
> Thanks,
>
> Jonathan
>> ---
>> .../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>> new file mode 100644
>> index 0000000..b02ef3e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>> @@ -0,0 +1,34 @@
>> +Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
>> +and externally connectable Magnetometer
>> +
>> +https://www.bosch-sensortec.com/bst/products/all_products/bmi160
>> +
>> +Required properties:
>> + - compatible : should be "bosch,bmi160"
>> + - reg : the I2C address or SPI chip select number of the sensor
>> + - spi-max-frequency : set maximum clock frequency (only for SPI)
>> +
>> +Optional properties:
>> + - interrupt-parent : should be the phandle of the interrupt controller
>> + - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
>> + - interrupt-names : set to "INT2" if using INT2 pin
>> +
>> +Examples:
>> +
>> +bmi160@68 {
>> + compatible = "bosch,bmi160";
>> + reg = <0x68>;
>> +
>> + interrupt-parent = <&gpio4>;
>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>> +};
>> +
>> +bmi160@0 {
>> + compatible = "bosch,bmi160";
>> + reg = <0>;
>> + spi-max-frequency = <10000000>;
>> +
>> + interrupt-parent = <&gpio2>;
>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>> + interrupt-names = "INT2";
>> +};
>>
>
--
Marcin Niestroj
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-09 14:16 ` Marcin Niestroj
@ 2016-11-09 17:16 ` Marcin Niestroj
2016-11-12 15:55 ` Jonathan Cameron
2016-11-12 15:53 ` Jonathan Cameron
1 sibling, 1 reply; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-09 17:16 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
linux-iio
On 09.11.2016 15:16, Marcin Niestroj wrote:
> Hi,
> Thanks for review, below are my comments.
>
> On 03.11.2016 13:09, Peter Meerwald-Stadler wrote:
>>
>>> This patch was developed primarily based on bmc150_accel hardware fifo
>>> implementation.
>>
>> parts of the patch are cleanup and bugfixing; should be separate?
>>
>> more comments below
>>
>>> IRQ handler was added, which for now is responsible only for handling
>>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>>> default INT is considered to be connected. If INT2 is used instead, the
>>> interrupt-names device-tree property can be used to specify that.
>>>
>>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
<snip>
>>> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev,
>>> struct regmap *regmap,
>>>
>>> data = iio_priv(indio_dev);
>>> dev_set_drvdata(dev, indio_dev);
>>> + data->irq = irq;
>>> data->regmap = regmap;
>>>
>>> + mutex_init(&data->mutex);
>>> +
>>> ret = bmi160_chip_init(data, use_spi);
>>> if (ret < 0)
>>> return ret;
>>> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev,
>>> struct regmap *regmap,
>>> indio_dev->info = &bmi160_info;
>>>
>>> ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> - bmi160_trigger_handler, NULL);
>>> + bmi160_trigger_handler,
>>> + &bmi160_buffer_ops);
>>> if (ret < 0)
>>> goto uninit;
>>>
>>> + if (data->irq > 0) {
>>> + /* Check which interrupt pin is connected to our board */
>>> + irq2 = of_irq_get_byname(dev->of_node, "INT2");
>>> + if (irq2 == data->irq) {
>>> + dev_dbg(dev, "Using interrupt line INT2\n");
>>> + data->irq_data = &bmi160_irq2_data;
>>> + } else {
>>> + dev_dbg(dev, "Using interrupt line INT1\n");
>>> + data->irq_data = &bmi160_irq1_data;
>>> + }
>>> +
>>> + ret = devm_request_threaded_irq(dev,
>>> + data->irq,
>>> + bmi160_irq_handler,
>>> + bmi160_irq_thread_handler,
>>> + IRQF_ONESHOT,
>>> + BMI160_IRQ_NAME,
>>> + indio_dev);
>>> + if (ret)
>>> + return ret;
I just noticed, that there should be a "goto buffer_cleanup" instead.
>>> +
>>> + ret = bmi160_enable_irq(data);
>>> + if (ret < 0)
>>> + goto buffer_cleanup;
>>> +
>>> + if (block_supported) {
>>> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
>>> + indio_dev->info = &bmi160_info_fifo;
>>> + indio_dev->buffer->attrs = bmi160_fifo_attributes;
>>> + data->fifo_buffer = devm_kmalloc(dev,
>>> + BMI160_FIFO_LENGTH,
>>> + GFP_KERNEL);
>>> + if (!data->fifo_buffer) {
>>> + ret = -ENOMEM;
>>
>> need to disable irq on failure?
>
> Yes, I missed that.
I am just wondering now if it is really neccessary. bmi160_enable_irq()
is just enabling IRQ output on INT1 or INT2 depending on device-tree.
This alone will not trigger interrupts, as all should be masked at this
stage.
>
>>
>>> + goto buffer_cleanup;
>>> + }
>>> + }
>>> + }
>>> +
>>> ret = iio_device_register(indio_dev);
>>> if (ret < 0)
>>> goto buffer_cleanup;
>>> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>> b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>> index 07a179d..aa63f89 100644
>>> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client
>>> *client,
>>> {
>>> struct regmap *regmap;
>>> const char *name = NULL;
>>> + bool block_supported =
>>> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
>>> + i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>>>
>>> regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
>>> if (IS_ERR(regmap)) {
>>> @@ -34,7 +38,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, client->irq,
>>> + false, block_supported);
>>> }
>>>
>>> static int bmi160_i2c_remove(struct i2c_client *client)
>>> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c
>>> b/drivers/iio/imu/bmi160/bmi160_spi.c
>>> index 1ec8b12..9b57fbe 100644
>>> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
>>> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
>>> @@ -25,7 +25,8 @@ 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, spi->irq,
>>> + true, true);
>>> }
>>>
>>> static int bmi160_spi_remove(struct spi_device *spi)
>>>
>>
>
--
Marcin Niestroj
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
2016-11-03 11:25 ` Marcin Niestroj
@ 2016-11-10 18:55 ` Rob Herring
-1 siblings, 0 replies; 25+ messages in thread
From: Rob Herring @ 2016-11-10 18:55 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Daniel Baluta, Gregor Boirie,
Sanchayan Maity, Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On Thu, Nov 03, 2016 at 12:25:27PM +0100, Marcin Niestroj wrote:
> This adds documentation for Bosch BMI160 Inertial Measurement Unit
> device-tree bindings.
>
> Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
> ---
> .../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> new file mode 100644
> index 0000000..b02ef3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> @@ -0,0 +1,34 @@
> +Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
> +and externally connectable Magnetometer
> +
> +https://www.bosch-sensortec.com/bst/products/all_products/bmi160
> +
> +Required properties:
> + - compatible : should be "bosch,bmi160"
> + - reg : the I2C address or SPI chip select number of the sensor
> + - spi-max-frequency : set maximum clock frequency (only for SPI)
> +
> +Optional properties:
> + - interrupt-parent : should be the phandle of the interrupt controller
> + - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
The fact that a GPIO is typically used is outside the scope of this doc.
> + - interrupt-names : set to "INT2" if using INT2 pin
Normally there's no point to have names property when there is a single
interrupt. However, it seems this could be either INT1 or INT2 or both
connected. You need to specify all the options.
Rob
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
@ 2016-11-10 18:55 ` Rob Herring
0 siblings, 0 replies; 25+ messages in thread
From: Rob Herring @ 2016-11-10 18:55 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Daniel Baluta, Gregor Boirie,
Sanchayan Maity, Mark Rutland, linux-iio, devicetree
On Thu, Nov 03, 2016 at 12:25:27PM +0100, Marcin Niestroj wrote:
> This adds documentation for Bosch BMI160 Inertial Measurement Unit
> device-tree bindings.
>
> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
> ---
> .../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> new file mode 100644
> index 0000000..b02ef3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
> @@ -0,0 +1,34 @@
> +Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
> +and externally connectable Magnetometer
> +
> +https://www.bosch-sensortec.com/bst/products/all_products/bmi160
> +
> +Required properties:
> + - compatible : should be "bosch,bmi160"
> + - reg : the I2C address or SPI chip select number of the sensor
> + - spi-max-frequency : set maximum clock frequency (only for SPI)
> +
> +Optional properties:
> + - interrupt-parent : should be the phandle of the interrupt controller
> + - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
The fact that a GPIO is typically used is outside the scope of this doc.
> + - interrupt-names : set to "INT2" if using INT2 pin
Normally there's no point to have names property when there is a single
interrupt. However, it seems this could be either INT1 or INT2 or both
connected. You need to specify all the options.
Rob
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-09 14:52 ` Marcin Niestroj
@ 2016-11-12 13:13 ` Jonathan Cameron
-1 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-12 13:13 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 09/11/16 14:52, Marcin Niestroj wrote:
> Hi,
> Thanks for review. I agree with all of the comments and will fix these in next patch version. Below is just a comment on hwfifo_* sysfs access.
>
> On 06.11.2016 13:35, Jonathan Cameron wrote:
>> On 03/11/16 11:25, Marcin Niestroj wrote:
>>> This patch was developed primarily based on bmc150_accel hardware fifo
>>> implementation.
>>>
>>> IRQ handler was added, which for now is responsible only for handling
>>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>>> default INT is considered to be connected. If INT2 is used instead, the
>>> interrupt-names device-tree property can be used to specify that.
>>>
>>> Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
>> I agree with Peter that there should have been a few precursor patches
>> to this one doing various cleanups and reworking bits that you have
>> in here. Would have made it easier to review (always a good thing :)
>>
>> In general the resulting code looks good to me. A few little
>> additional comments inline from me. Mostly about small code ordering things
>> and function rename suggestions that would make the code more 'obviously'
>> correct.
>>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> drivers/iio/imu/bmi160/bmi160.h | 3 +-
>>> drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
>>> drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
>>> drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
>>> 4 files changed, 618 insertions(+), 28 deletions(-)
>
> <snip>
>
>>> +
>>> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
>>> +static IIO_CONST_ATTR(hwfifo_watermark_max,
>>> + __stringify(BMI160_FIFO_LENGTH));
>>> +static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
>>> + bmi160_get_fifo_state, NULL, 0);
>>> +static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
>>> + bmi160_get_fifo_watermark, NULL, 0);
>>> +
>>> +static const struct attribute *bmi160_fifo_attributes[] = {
>>> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
>>> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
>>> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
>>> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
>> There are enough of these drivers now that sometimes soon we should
>> revisit the question of pulling these into the core. Can certainly
>> concieve of downstream consumer devices (in particular the iio_input
>> bridge when that finally resurfaces - my fault) wanting to be able to
>> manipulate or at least have visibilty of these.
>
> One more thing to consider is setting hwfifo_watermark to other value
> than "userspace" watermark. It would be nice to set hwfifo_watermark
> to a *safe* value to be able to get all data from hardware to kfifo.
> By safe I mean that the chance of hardware fifo overflow will be
> small. On the other hand there might be no reason to have such small
> watermark for userspace application, so we can save scheduler
> cycles.
>
This may be worth doing, but I worry we'll end up with too many knobs
around this feature. Ultimately there is no point in setting the hardware
fifo as smaller than the userspace one if the userspace one is small enough.
What counts as a safe level is going to be very hardware / load dependant.
We 'could' try and put this in the devicetree so that a particular setup
could set it at a level that is 'reasonable'. It's kind of a hardware
feature, but will also depend on software constraints (like whether the
bus controller driver can support dma for example).
Non obvious unfortunately.
Jonathan
> <snip>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
@ 2016-11-12 13:13 ` Jonathan Cameron
0 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-12 13:13 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio, devicetree
On 09/11/16 14:52, Marcin Niestroj wrote:
> Hi,
> Thanks for review. I agree with all of the comments and will fix these in next patch version. Below is just a comment on hwfifo_* sysfs access.
>
> On 06.11.2016 13:35, Jonathan Cameron wrote:
>> On 03/11/16 11:25, Marcin Niestroj wrote:
>>> This patch was developed primarily based on bmc150_accel hardware fifo
>>> implementation.
>>>
>>> IRQ handler was added, which for now is responsible only for handling
>>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>>> default INT is considered to be connected. If INT2 is used instead, the
>>> interrupt-names device-tree property can be used to specify that.
>>>
>>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>> I agree with Peter that there should have been a few precursor patches
>> to this one doing various cleanups and reworking bits that you have
>> in here. Would have made it easier to review (always a good thing :)
>>
>> In general the resulting code looks good to me. A few little
>> additional comments inline from me. Mostly about small code ordering things
>> and function rename suggestions that would make the code more 'obviously'
>> correct.
>>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> drivers/iio/imu/bmi160/bmi160.h | 3 +-
>>> drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
>>> drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
>>> drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
>>> 4 files changed, 618 insertions(+), 28 deletions(-)
>
> <snip>
>
>>> +
>>> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
>>> +static IIO_CONST_ATTR(hwfifo_watermark_max,
>>> + __stringify(BMI160_FIFO_LENGTH));
>>> +static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
>>> + bmi160_get_fifo_state, NULL, 0);
>>> +static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
>>> + bmi160_get_fifo_watermark, NULL, 0);
>>> +
>>> +static const struct attribute *bmi160_fifo_attributes[] = {
>>> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
>>> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
>>> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
>>> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
>> There are enough of these drivers now that sometimes soon we should
>> revisit the question of pulling these into the core. Can certainly
>> concieve of downstream consumer devices (in particular the iio_input
>> bridge when that finally resurfaces - my fault) wanting to be able to
>> manipulate or at least have visibilty of these.
>
> One more thing to consider is setting hwfifo_watermark to other value
> than "userspace" watermark. It would be nice to set hwfifo_watermark
> to a *safe* value to be able to get all data from hardware to kfifo.
> By safe I mean that the chance of hardware fifo overflow will be
> small. On the other hand there might be no reason to have such small
> watermark for userspace application, so we can save scheduler
> cycles.
>
This may be worth doing, but I worry we'll end up with too many knobs
around this feature. Ultimately there is no point in setting the hardware
fifo as smaller than the userspace one if the userspace one is small enough.
What counts as a safe level is going to be very hardware / load dependant.
We 'could' try and put this in the devicetree so that a particular setup
could set it at a level that is 'reasonable'. It's kind of a hardware
feature, but will also depend on software constraints (like whether the
bus controller driver can support dma for example).
Non obvious unfortunately.
Jonathan
> <snip>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
2016-11-09 15:18 ` Marcin Niestroj
@ 2016-11-12 13:15 ` Jonathan Cameron
-1 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-12 13:15 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 09/11/16 15:18, Marcin Niestroj wrote:
> On 06.11.2016 13:41, Jonathan Cameron wrote:
>> On 03/11/16 11:25, Marcin Niestroj wrote:
>>> This adds documentation for Bosch BMI160 Inertial Measurement Unit
>>> device-tree bindings.
>>>
>>> Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
>> Unless I missed it in the previous patch we should also have of tables
>> added to the i2c and spi files (which is why the various tests haven't
>> been screaming at me that this device doesn't have documented bindings).
>
> Ok, I will add them.
>
>>
>> Otherwise, the use of interrupt names to indicate which pin on the chip
>> is a little unusual (if you cribbed this from somewhere I've forgotten
>> about then do say so!), so will want a devicetree bindings maintainer
>> input on this.
>
> I have used interrupt names similar as in other driver. Please see
> [1] for it's DT documentation and [2] for implementation.
> [1] Documentation/devicetree/bindings/iio/accel/mma8452.txt
> [2] drivers/iio/accel/mma8452.c
Fair enough. Just put the INT1 option in the docs as requested and this
should be fine.
Jonathan
>
>>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> .../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
>>> 1 file changed, 34 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>>> new file mode 100644
>>> index 0000000..b02ef3e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>>> @@ -0,0 +1,34 @@
>>> +Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
>>> +and externally connectable Magnetometer
>>> +
>>> +https://www.bosch-sensortec.com/bst/products/all_products/bmi160
>>> +
>>> +Required properties:
>>> + - compatible : should be "bosch,bmi160"
>>> + - reg : the I2C address or SPI chip select number of the sensor
>>> + - spi-max-frequency : set maximum clock frequency (only for SPI)
>>> +
>>> +Optional properties:
>>> + - interrupt-parent : should be the phandle of the interrupt controller
>>> + - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
>>> + - interrupt-names : set to "INT2" if using INT2 pin
>>> +
>>> +Examples:
>>> +
>>> +bmi160@68 {
>>> + compatible = "bosch,bmi160";
>>> + reg = <0x68>;
>>> +
>>> + interrupt-parent = <&gpio4>;
>>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>>> +};
>>> +
>>> +bmi160@0 {
>>> + compatible = "bosch,bmi160";
>>> + reg = <0>;
>>> + spi-max-frequency = <10000000>;
>>> +
>>> + interrupt-parent = <&gpio2>;
>>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>>> + interrupt-names = "INT2";
>>> +};
>>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: Add bmi160 imu binding
@ 2016-11-12 13:15 ` Jonathan Cameron
0 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-12 13:15 UTC (permalink / raw)
To: Marcin Niestroj
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Daniel Baluta, Gregor Boirie, Sanchayan Maity, Rob Herring,
Mark Rutland, linux-iio, devicetree
On 09/11/16 15:18, Marcin Niestroj wrote:
> On 06.11.2016 13:41, Jonathan Cameron wrote:
>> On 03/11/16 11:25, Marcin Niestroj wrote:
>>> This adds documentation for Bosch BMI160 Inertial Measurement Unit
>>> device-tree bindings.
>>>
>>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>> Unless I missed it in the previous patch we should also have of tables
>> added to the i2c and spi files (which is why the various tests haven't
>> been screaming at me that this device doesn't have documented bindings).
>
> Ok, I will add them.
>
>>
>> Otherwise, the use of interrupt names to indicate which pin on the chip
>> is a little unusual (if you cribbed this from somewhere I've forgotten
>> about then do say so!), so will want a devicetree bindings maintainer
>> input on this.
>
> I have used interrupt names similar as in other driver. Please see
> [1] for it's DT documentation and [2] for implementation.
> [1] Documentation/devicetree/bindings/iio/accel/mma8452.txt
> [2] drivers/iio/accel/mma8452.c
Fair enough. Just put the INT1 option in the docs as requested and this
should be fine.
Jonathan
>
>>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> .../devicetree/bindings/iio/imu/bmi160.txt | 34 ++++++++++++++++++++++
>>> 1 file changed, 34 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.txt b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>>> new file mode 100644
>>> index 0000000..b02ef3e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.txt
>>> @@ -0,0 +1,34 @@
>>> +Bosch BMI160 - Inertial Measurement Unit with Accelerometer, Gyroscope
>>> +and externally connectable Magnetometer
>>> +
>>> +https://www.bosch-sensortec.com/bst/products/all_products/bmi160
>>> +
>>> +Required properties:
>>> + - compatible : should be "bosch,bmi160"
>>> + - reg : the I2C address or SPI chip select number of the sensor
>>> + - spi-max-frequency : set maximum clock frequency (only for SPI)
>>> +
>>> +Optional properties:
>>> + - interrupt-parent : should be the phandle of the interrupt controller
>>> + - interrupts : interrupt mapping for GPIO IRQ, must be IRQ_TYPE_LEVEL_LOW
>>> + - interrupt-names : set to "INT2" if using INT2 pin
>>> +
>>> +Examples:
>>> +
>>> +bmi160@68 {
>>> + compatible = "bosch,bmi160";
>>> + reg = <0x68>;
>>> +
>>> + interrupt-parent = <&gpio4>;
>>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>>> +};
>>> +
>>> +bmi160@0 {
>>> + compatible = "bosch,bmi160";
>>> + reg = <0>;
>>> + spi-max-frequency = <10000000>;
>>> +
>>> + interrupt-parent = <&gpio2>;
>>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>>> + interrupt-names = "INT2";
>>> +};
>>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-09 14:16 ` Marcin Niestroj
2016-11-09 17:16 ` Marcin Niestroj
@ 2016-11-12 15:53 ` Jonathan Cameron
1 sibling, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-12 15:53 UTC (permalink / raw)
To: Marcin Niestroj, Peter Meerwald-Stadler
Cc: Hartmut Knaack, Lars-Peter Clausen, Daniel Baluta, Gregor Boirie,
Sanchayan Maity, Rob Herring, linux-iio
On 09/11/16 14:16, Marcin Niestroj wrote:
> Hi,
> Thanks for review, below are my comments.
>
> On 03.11.2016 13:09, Peter Meerwald-Stadler wrote:
>>
>>> This patch was developed primarily based on bmc150_accel hardware fifo
>>> implementation.
>>
>> parts of the patch are cleanup and bugfixing; should be separate?
>>
>> more comments below
>>
>>> IRQ handler was added, which for now is responsible only for handling
>>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>>> default INT is considered to be connected. If INT2 is used instead, the
>>> interrupt-names device-tree property can be used to specify that.
>>>
>>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>>> ---
<snip>
>>> + /* Handle demux */
>>> + timestamp = last_timestamp - (last_samples * data->fifo_sample_period);
>>> + buffer_iter = buffer;
>>> + for (i = 0; i < fifo_samples; i++) {
>>> + u8 tmp_buf[indio_dev->scan_bytes];
>>
>> non-constant array size, is this allowed these days?
>
> I am not sure if this is permitted in kernel. [1] says that ISO C99 adds arrays of variable length.
>
> The reason I have used it is that iio_push_to_buffers_with_timestamp uses indio_dev->scan_bytes to write timestamp to last element in array.
> One of the possiblities to get rid of non-constant array size is to determine maximum size. Is that what I should do?
Yes.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html
>
>>
<snip>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-09 17:16 ` Marcin Niestroj
@ 2016-11-12 15:55 ` Jonathan Cameron
2016-11-14 17:30 ` Marcin Niestroj
0 siblings, 1 reply; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-12 15:55 UTC (permalink / raw)
To: Marcin Niestroj, Peter Meerwald-Stadler
Cc: Hartmut Knaack, Lars-Peter Clausen, Daniel Baluta, Gregor Boirie,
Sanchayan Maity, Rob Herring, linux-iio
On 09/11/16 17:16, Marcin Niestroj wrote:
> On 09.11.2016 15:16, Marcin Niestroj wrote:
>> Hi,
>> Thanks for review, below are my comments.
>>
>> On 03.11.2016 13:09, Peter Meerwald-Stadler wrote:
>>>
>>>> This patch was developed primarily based on bmc150_accel hardware fifo
>>>> implementation.
>>>
>>> parts of the patch are cleanup and bugfixing; should be separate?
>>>
>>> more comments below
>>>
>>>> IRQ handler was added, which for now is responsible only for handling
>>>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>>>> default INT is considered to be connected. If INT2 is used instead, the
>>>> interrupt-names device-tree property can be used to specify that.
>>>>
>>>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>
> <snip>
>
>>>> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev,
>>>> struct regmap *regmap,
>>>>
>>>> data = iio_priv(indio_dev);
>>>> dev_set_drvdata(dev, indio_dev);
>>>> + data->irq = irq;
>>>> data->regmap = regmap;
>>>>
>>>> + mutex_init(&data->mutex);
>>>> +
>>>> ret = bmi160_chip_init(data, use_spi);
>>>> if (ret < 0)
>>>> return ret;
>>>> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev,
>>>> struct regmap *regmap,
>>>> indio_dev->info = &bmi160_info;
>>>>
>>>> ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>> - bmi160_trigger_handler, NULL);
>>>> + bmi160_trigger_handler,
>>>> + &bmi160_buffer_ops);
>>>> if (ret < 0)
>>>> goto uninit;
>>>>
>>>> + if (data->irq > 0) {
>>>> + /* Check which interrupt pin is connected to our board */
>>>> + irq2 = of_irq_get_byname(dev->of_node, "INT2");
>>>> + if (irq2 == data->irq) {
>>>> + dev_dbg(dev, "Using interrupt line INT2\n");
>>>> + data->irq_data = &bmi160_irq2_data;
>>>> + } else {
>>>> + dev_dbg(dev, "Using interrupt line INT1\n");
>>>> + data->irq_data = &bmi160_irq1_data;
>>>> + }
>>>> +
>>>> + ret = devm_request_threaded_irq(dev,
>>>> + data->irq,
>>>> + bmi160_irq_handler,
>>>> + bmi160_irq_thread_handler,
>>>> + IRQF_ONESHOT,
>>>> + BMI160_IRQ_NAME,
>>>> + indio_dev);
>>>> + if (ret)
>>>> + return ret;
>
> I just noticed, that there should be a "goto buffer_cleanup" instead.
>
>>>> +
>>>> + ret = bmi160_enable_irq(data);
>>>> + if (ret < 0)
>>>> + goto buffer_cleanup;
>>>> +
>>>> + if (block_supported) {
>>>> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
>>>> + indio_dev->info = &bmi160_info_fifo;
>>>> + indio_dev->buffer->attrs = bmi160_fifo_attributes;
>>>> + data->fifo_buffer = devm_kmalloc(dev,
>>>> + BMI160_FIFO_LENGTH,
>>>> + GFP_KERNEL);
>>>> + if (!data->fifo_buffer) {
>>>> + ret = -ENOMEM;
>>>
>>> need to disable irq on failure?
>>
>> Yes, I missed that.
>
> I am just wondering now if it is really neccessary. bmi160_enable_irq()
> is just enabling IRQ output on INT1 or INT2 depending on device-tree.
> This alone will not trigger interrupts, as all should be masked at this
> stage.
You should always unwind everything in error paths as it makes them
'obviously' correct rather than subject to subtle bugs.
>
>>
>>>
>>>> + goto buffer_cleanup;
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> ret = iio_device_register(indio_dev);
>>>> if (ret < 0)
>>>> goto buffer_cleanup;
>>>> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>> b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>> index 07a179d..aa63f89 100644
>>>> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client
>>>> *client,
>>>> {
>>>> struct regmap *regmap;
>>>> const char *name = NULL;
>>>> + bool block_supported =
>>>> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
>>>> + i2c_check_functionality(client->adapter,
>>>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>>>>
>>>> regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
>>>> if (IS_ERR(regmap)) {
>>>> @@ -34,7 +38,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, client->irq,
>>>> + false, block_supported);
>>>> }
>>>>
>>>> static int bmi160_i2c_remove(struct i2c_client *client)
>>>> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c
>>>> b/drivers/iio/imu/bmi160/bmi160_spi.c
>>>> index 1ec8b12..9b57fbe 100644
>>>> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
>>>> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
>>>> @@ -25,7 +25,8 @@ 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, spi->irq,
>>>> + true, true);
>>>> }
>>>>
>>>> static int bmi160_spi_remove(struct spi_device *spi)
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-12 15:55 ` Jonathan Cameron
@ 2016-11-14 17:30 ` Marcin Niestroj
2016-11-19 12:43 ` Jonathan Cameron
0 siblings, 1 reply; 25+ messages in thread
From: Marcin Niestroj @ 2016-11-14 17:30 UTC (permalink / raw)
To: Jonathan Cameron, Peter Meerwald-Stadler
Cc: Hartmut Knaack, Lars-Peter Clausen, Daniel Baluta, Gregor Boirie,
Sanchayan Maity, Rob Herring, linux-iio
On 12.11.2016 16:55, Jonathan Cameron wrote:
> On 09/11/16 17:16, Marcin Niestroj wrote:
>> On 09.11.2016 15:16, Marcin Niestroj wrote:
>>> Hi,
>>> Thanks for review, below are my comments.
>>>
>>> On 03.11.2016 13:09, Peter Meerwald-Stadler wrote:
>>>>
>>>>> This patch was developed primarily based on bmc150_accel hardware fifo
>>>>> implementation.
>>>>
>>>> parts of the patch are cleanup and bugfixing; should be separate?
>>>>
>>>> more comments below
>>>>
>>>>> IRQ handler was added, which for now is responsible only for handling
>>>>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>>>>> default INT is considered to be connected. If INT2 is used instead, the
>>>>> interrupt-names device-tree property can be used to specify that.
>>>>>
>>>>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>>
>> <snip>
>>
>>>>> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev,
>>>>> struct regmap *regmap,
>>>>>
>>>>> data = iio_priv(indio_dev);
>>>>> dev_set_drvdata(dev, indio_dev);
>>>>> + data->irq = irq;
>>>>> data->regmap = regmap;
>>>>>
>>>>> + mutex_init(&data->mutex);
>>>>> +
>>>>> ret = bmi160_chip_init(data, use_spi);
>>>>> if (ret < 0)
>>>>> return ret;
>>>>> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev,
>>>>> struct regmap *regmap,
>>>>> indio_dev->info = &bmi160_info;
>>>>>
>>>>> ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>> - bmi160_trigger_handler, NULL);
>>>>> + bmi160_trigger_handler,
>>>>> + &bmi160_buffer_ops);
>>>>> if (ret < 0)
>>>>> goto uninit;
>>>>>
>>>>> + if (data->irq > 0) {
>>>>> + /* Check which interrupt pin is connected to our board */
>>>>> + irq2 = of_irq_get_byname(dev->of_node, "INT2");
>>>>> + if (irq2 == data->irq) {
>>>>> + dev_dbg(dev, "Using interrupt line INT2\n");
>>>>> + data->irq_data = &bmi160_irq2_data;
>>>>> + } else {
>>>>> + dev_dbg(dev, "Using interrupt line INT1\n");
>>>>> + data->irq_data = &bmi160_irq1_data;
>>>>> + }
>>>>> +
>>>>> + ret = devm_request_threaded_irq(dev,
>>>>> + data->irq,
>>>>> + bmi160_irq_handler,
>>>>> + bmi160_irq_thread_handler,
>>>>> + IRQF_ONESHOT,
>>>>> + BMI160_IRQ_NAME,
>>>>> + indio_dev);
>>>>> + if (ret)
>>>>> + return ret;
>>
>> I just noticed, that there should be a "goto buffer_cleanup" instead.
>>
>>>>> +
>>>>> + ret = bmi160_enable_irq(data);
>>>>> + if (ret < 0)
>>>>> + goto buffer_cleanup;
>>>>> +
>>>>> + if (block_supported) {
>>>>> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
>>>>> + indio_dev->info = &bmi160_info_fifo;
>>>>> + indio_dev->buffer->attrs = bmi160_fifo_attributes;
>>>>> + data->fifo_buffer = devm_kmalloc(dev,
>>>>> + BMI160_FIFO_LENGTH,
>>>>> + GFP_KERNEL);
>>>>> + if (!data->fifo_buffer) {
>>>>> + ret = -ENOMEM;
>>>>
>>>> need to disable irq on failure?
>>>
>>> Yes, I missed that.
>>
>> I am just wondering now if it is really neccessary. bmi160_enable_irq()
>> is just enabling IRQ output on INT1 or INT2 depending on device-tree.
>> This alone will not trigger interrupts, as all should be masked at this
>> stage.
> You should always unwind everything in error paths as it makes them
> 'obviously' correct rather than subject to subtle bugs.
Ok. And what about issuing a softreset of the device? This should
revert all registers to defaults.
>>
>>>
>>>>
>>>>> + goto buffer_cleanup;
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> +
>>>>> ret = iio_device_register(indio_dev);
>>>>> if (ret < 0)
>>>>> goto buffer_cleanup;
>>>>> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>> b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>> index 07a179d..aa63f89 100644
>>>>> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client
>>>>> *client,
>>>>> {
>>>>> struct regmap *regmap;
>>>>> const char *name = NULL;
>>>>> + bool block_supported =
>>>>> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
>>>>> + i2c_check_functionality(client->adapter,
>>>>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>>>>>
>>>>> regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
>>>>> if (IS_ERR(regmap)) {
>>>>> @@ -34,7 +38,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, client->irq,
>>>>> + false, block_supported);
>>>>> }
>>>>>
>>>>> static int bmi160_i2c_remove(struct i2c_client *client)
>>>>> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>> b/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>> index 1ec8b12..9b57fbe 100644
>>>>> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>> @@ -25,7 +25,8 @@ 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, spi->irq,
>>>>> + true, true);
>>>>> }
>>>>>
>>>>> static int bmi160_spi_remove(struct spi_device *spi)
>>>>>
>>>>
>>>
>>
>
--
Marcin Niestroj
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] iio: bmi160: Support hardware fifo
2016-11-14 17:30 ` Marcin Niestroj
@ 2016-11-19 12:43 ` Jonathan Cameron
0 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2016-11-19 12:43 UTC (permalink / raw)
To: Marcin Niestroj, Peter Meerwald-Stadler
Cc: Hartmut Knaack, Lars-Peter Clausen, Daniel Baluta, Gregor Boirie,
Sanchayan Maity, Rob Herring, linux-iio
On 14/11/16 17:30, Marcin Niestroj wrote:
> On 12.11.2016 16:55, Jonathan Cameron wrote:
>> On 09/11/16 17:16, Marcin Niestroj wrote:
>>> On 09.11.2016 15:16, Marcin Niestroj wrote:
>>>> Hi,
>>>> Thanks for review, below are my comments.
>>>>
>>>> On 03.11.2016 13:09, Peter Meerwald-Stadler wrote:
>>>>>
>>>>>> This patch was developed primarily based on bmc150_accel hardware fifo
>>>>>> implementation.
>>>>>
>>>>> parts of the patch are cleanup and bugfixing; should be separate?
>>>>>
>>>>> more comments below
>>>>>
>>>>>> IRQ handler was added, which for now is responsible only for handling
>>>>>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>>>>>> default INT is considered to be connected. If INT2 is used instead, the
>>>>>> interrupt-names device-tree property can be used to specify that.
>>>>>>
>>>>>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>>>
>>> <snip>
>>>
>>>>>> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev,
>>>>>> struct regmap *regmap,
>>>>>>
>>>>>> data = iio_priv(indio_dev);
>>>>>> dev_set_drvdata(dev, indio_dev);
>>>>>> + data->irq = irq;
>>>>>> data->regmap = regmap;
>>>>>>
>>>>>> + mutex_init(&data->mutex);
>>>>>> +
>>>>>> ret = bmi160_chip_init(data, use_spi);
>>>>>> if (ret < 0)
>>>>>> return ret;
>>>>>> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev,
>>>>>> struct regmap *regmap,
>>>>>> indio_dev->info = &bmi160_info;
>>>>>>
>>>>>> ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>>> - bmi160_trigger_handler, NULL);
>>>>>> + bmi160_trigger_handler,
>>>>>> + &bmi160_buffer_ops);
>>>>>> if (ret < 0)
>>>>>> goto uninit;
>>>>>>
>>>>>> + if (data->irq > 0) {
>>>>>> + /* Check which interrupt pin is connected to our board */
>>>>>> + irq2 = of_irq_get_byname(dev->of_node, "INT2");
>>>>>> + if (irq2 == data->irq) {
>>>>>> + dev_dbg(dev, "Using interrupt line INT2\n");
>>>>>> + data->irq_data = &bmi160_irq2_data;
>>>>>> + } else {
>>>>>> + dev_dbg(dev, "Using interrupt line INT1\n");
>>>>>> + data->irq_data = &bmi160_irq1_data;
>>>>>> + }
>>>>>> +
>>>>>> + ret = devm_request_threaded_irq(dev,
>>>>>> + data->irq,
>>>>>> + bmi160_irq_handler,
>>>>>> + bmi160_irq_thread_handler,
>>>>>> + IRQF_ONESHOT,
>>>>>> + BMI160_IRQ_NAME,
>>>>>> + indio_dev);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>
>>> I just noticed, that there should be a "goto buffer_cleanup" instead.
>>>
>>>>>> +
>>>>>> + ret = bmi160_enable_irq(data);
>>>>>> + if (ret < 0)
>>>>>> + goto buffer_cleanup;
>>>>>> +
>>>>>> + if (block_supported) {
>>>>>> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
>>>>>> + indio_dev->info = &bmi160_info_fifo;
>>>>>> + indio_dev->buffer->attrs = bmi160_fifo_attributes;
>>>>>> + data->fifo_buffer = devm_kmalloc(dev,
>>>>>> + BMI160_FIFO_LENGTH,
>>>>>> + GFP_KERNEL);
>>>>>> + if (!data->fifo_buffer) {
>>>>>> + ret = -ENOMEM;
>>>>>
>>>>> need to disable irq on failure?
>>>>
>>>> Yes, I missed that.
>>>
>>> I am just wondering now if it is really neccessary. bmi160_enable_irq()
>>> is just enabling IRQ output on INT1 or INT2 depending on device-tree.
>>> This alone will not trigger interrupts, as all should be masked at this
>>> stage.
>> You should always unwind everything in error paths as it makes them
>> 'obviously' correct rather than subject to subtle bugs.
>
> Ok. And what about issuing a softreset of the device? This should
> revert all registers to defaults.
That's fine, but best to add a comment saying what it's effects are if they
can be specifically listed (such as 'will disable the interrupt'.)
Again makes it 'obviously correct' which I like as a reviewer!
Jonathan
>
>>>
>>>>
>>>>>
>>>>>> + goto buffer_cleanup;
>>>>>> + }
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> ret = iio_device_register(indio_dev);
>>>>>> if (ret < 0)
>>>>>> goto buffer_cleanup;
>>>>>> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>>> b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>>> index 07a179d..aa63f89 100644
>>>>>> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>>> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>>> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client
>>>>>> *client,
>>>>>> {
>>>>>> struct regmap *regmap;
>>>>>> const char *name = NULL;
>>>>>> + bool block_supported =
>>>>>> + i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
>>>>>> + i2c_check_functionality(client->adapter,
>>>>>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>>>>>>
>>>>>> regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
>>>>>> if (IS_ERR(regmap)) {
>>>>>> @@ -34,7 +38,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, client->irq,
>>>>>> + false, block_supported);
>>>>>> }
>>>>>>
>>>>>> static int bmi160_i2c_remove(struct i2c_client *client)
>>>>>> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>>> b/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>>> index 1ec8b12..9b57fbe 100644
>>>>>> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>>> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>>> @@ -25,7 +25,8 @@ 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, spi->irq,
>>>>>> + true, true);
>>>>>> }
>>>>>>
>>>>>> static int bmi160_spi_remove(struct spi_device *spi)
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2016-11-19 12:43 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-03 11:25 [PATCH 1/2] iio: bmi160: Support hardware fifo Marcin Niestroj
2016-11-03 11:25 ` Marcin Niestroj
2016-11-03 12:09 ` Peter Meerwald-Stadler
2016-11-09 14:16 ` Marcin Niestroj
2016-11-09 17:16 ` Marcin Niestroj
2016-11-12 15:55 ` Jonathan Cameron
2016-11-14 17:30 ` Marcin Niestroj
2016-11-19 12:43 ` Jonathan Cameron
2016-11-12 15:53 ` Jonathan Cameron
[not found] ` <20161103112527.29623-1-m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
2016-11-03 11:25 ` [PATCH 2/2] Documentation: DT: Add bmi160 imu binding Marcin Niestroj
2016-11-03 11:25 ` Marcin Niestroj
[not found] ` <20161103112527.29623-2-m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
2016-11-06 12:41 ` Jonathan Cameron
2016-11-06 12:41 ` Jonathan Cameron
[not found] ` <e46a4b5e-a347-f268-80f4-0ee1edc77aac-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2016-11-09 15:18 ` Marcin Niestroj
2016-11-09 15:18 ` Marcin Niestroj
[not found] ` <d07474fc-e6c9-d529-6a06-0549f3dfad52-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
2016-11-12 13:15 ` Jonathan Cameron
2016-11-12 13:15 ` Jonathan Cameron
2016-11-10 18:55 ` Rob Herring
2016-11-10 18:55 ` Rob Herring
2016-11-06 12:35 ` [PATCH 1/2] iio: bmi160: Support hardware fifo Jonathan Cameron
2016-11-06 12:35 ` Jonathan Cameron
[not found] ` <b7dd5694-b757-c0d1-3400-46e92b8deb58-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2016-11-09 14:52 ` Marcin Niestroj
2016-11-09 14:52 ` Marcin Niestroj
[not found] ` <1d50ebf1-2f14-2e3f-9b8e-b34a7ebca4c5-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
2016-11-12 13:13 ` Jonathan Cameron
2016-11-12 13:13 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.