Murata IRS-D200 is a PIR sensor for human detection. In this series we
add devicetree bindings and an IIO driver with support for triggered
buffer and events. Link to the datasheet should be added to the
devicetree bindings, when that is available.
Waqar Hameed (2):
dt-bindings: iio: proximity: Add bindings for Murata IRS-D200
iio: Add driver for Murata IRS-D200
.../iio/proximity/murata,irsd200.yaml | 54 +
drivers/iio/proximity/Kconfig | 12 +
drivers/iio/proximity/Makefile | 1 +
drivers/iio/proximity/irsd200.c | 1051 +++++++++++++++++
4 files changed, 1118 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
create mode 100644 drivers/iio/proximity/irsd200.c
base-commit: b6dad5178ceaf23f369c3711062ce1f2afc33644
--
2.30.2
Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
for communication with interrupt support. Add devicetree bindings
requiring the compatible string, I2C slave address (reg) and interrupts.
Signed-off-by: Waqar Hameed <[email protected]>
---
.../iio/proximity/murata,irsd200.yaml | 54 +++++++++++++++++++
1 file changed, 54 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
diff --git a/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
new file mode 100644
index 000000000000..d317fbe7bd50
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/proximity/murata,irsd200.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Murata IRS-D200 PIR sensor
+
+maintainers:
+ - Waqar Hameed <[email protected]>
+
+description: |
+ PIR sensor for human detection.
+
+properties:
+ compatible:
+ const: murata,irsd200
+
+ reg:
+ items:
+ - enum:
+ - 0x48
+ - 0x49
+ description: |
+ When the AD pin is connected to GND, the slave address is 0x48.
+ When the AD pin is connected to VDD, the slave address is 0x49.
+
+ interrupts:
+ maxItems: 1
+ description:
+ Type should be IRQ_TYPE_EDGE_RISING.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pir@48 {
+ compatible = "murata,irsd200";
+ reg = <0x48>;
+ interrupts = <24 IRQ_TYPE_EDGE_RISING>;
+ };
+ };
+...
--
2.30.2
Murata IRS-D200 is a PIR sensor for human detection. It has support for
raw data measurements and detection event notification.
Add a driver with support for triggered buffer and events. Map the
various settings to the `iio` framework, e.g. threshold values, sampling
frequency, filter frequencies etc.
Signed-off-by: Waqar Hameed <[email protected]>
---
drivers/iio/proximity/Kconfig | 12 +
drivers/iio/proximity/Makefile | 1 +
drivers/iio/proximity/irsd200.c | 1051 +++++++++++++++++++++++++++++++
3 files changed, 1064 insertions(+)
create mode 100644 drivers/iio/proximity/irsd200.c
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 0e5c17530b8b..2ca3b0bc5eba 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -32,6 +32,18 @@ config CROS_EC_MKBP_PROXIMITY
To compile this driver as a module, choose M here: the
module will be called cros_ec_mkbp_proximity.
+config IRSD200
+ tristate "Murata IRS-D200 PIR sensor"
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ select REGMAP_I2C
+ depends on I2C
+ help
+ Say Y here to build a driver for the Murata IRS-D200 PIR sensor.
+
+ To compile this driver as a module, choose M here: the module will be
+ called irsd200.
+
config ISL29501
tristate "Intersil ISL29501 Time Of Flight sensor"
depends on I2C
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index cc838bb5408a..f36598380446 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -6,6 +6,7 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_AS3935) += as3935.o
obj-$(CONFIG_CROS_EC_MKBP_PROXIMITY) += cros_ec_mkbp_proximity.o
+obj-$(CONFIG_IRSD200) += irsd200.o
obj-$(CONFIG_ISL29501) += isl29501.o
obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
obj-$(CONFIG_MB1232) += mb1232.o
diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
new file mode 100644
index 000000000000..699801d60295
--- /dev/null
+++ b/drivers/iio/proximity/irsd200.c
@@ -0,0 +1,1051 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for Murata IRS-D200 PIR sensor.
+ *
+ * Copyright (C) 2023 Axis Communications AB
+ */
+
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/types.h>
+
+#define IRS_DRV_NAME "irsd200"
+
+/* Registers. */
+#define IRS_REG_OP 0x00 /* Operation mode. */
+#define IRS_REG_DATA_LO 0x02 /* Sensor data LSB. */
+#define IRS_REG_DATA_HI 0x03 /* Sensor data MSB. */
+#define IRS_REG_STATUS 0x04 /* Interrupt status. */
+#define IRS_REG_COUNT 0x05 /* Count of exceeding threshold. */
+#define IRS_REG_DATA_RATE 0x06 /* Output data rate. */
+#define IRS_REG_FILTER 0x07 /* High-pass and low-pass filter. */
+#define IRS_REG_INTR 0x09 /* Interrupt mode. */
+#define IRS_REG_NR_COUNT 0x0a /* Number of counts before interrupt. */
+#define IRS_REG_THR_HI 0x0b /* Upper threshold. */
+#define IRS_REG_THR_LO 0x0c /* Lower threshold. */
+#define IRS_REG_TIMER_LO 0x0d /* Timer setting LSB. */
+#define IRS_REG_TIMER_HI 0x0e /* Timer setting MSB. */
+
+/* Interrupt status bits. */
+#define IRS_INTR_DATA 0 /* Data update. */
+#define IRS_INTR_TIMER 1 /* Timer expiration. */
+#define IRS_INTR_COUNT_THR_AND 2 /* Count "AND" threshold. */
+#define IRS_INTR_COUNT_THR_OR 3 /* Count "OR" threshold. */
+
+/*
+ * Quantization scale value for threshold. Used for conversion from/to register
+ * value.
+ */
+#define IRS_THR_QUANT_SCALE 128
+
+/*
+ * The upper 4 bits in register IRS_REG_COUNT value is the upper count value
+ * (exceeding upper threshold value). The lower 4 is the lower count value
+ * (exceeding lower threshold value).
+ */
+#define IRS_UPPER_COUNT(count) (count >> 4)
+#define IRS_LOWER_COUNT(count) (count & GENMASK(3, 0))
+
+/* Index corresponds to the value of IRS_REG_DATA_RATE register. */
+static const int irsd200_data_rates[2] = {
+ 50,
+ 100,
+};
+
+/* Index corresponds to the (field) value of IRS_REG_FILTER register. */
+static const unsigned int irsd200_lp_filter_freq[2] = {
+ 10,
+ 7,
+};
+
+/*
+ * Index corresponds to the (field) value of IRS_REG_FILTER register. Contains
+ * only the fractional part (since the integer part is 0, e.g the first value
+ * corresponds to 0.3 Hz).
+ */
+static const unsigned int irsd200_hp_filter_freq[2] = {
+ 3,
+ 5,
+};
+
+/* Register fields. */
+enum irsd200_regfield {
+ /* Data interrupt. */
+ IRS_REGF_INTR_DATA,
+ /* Timer interrupt. */
+ IRS_REGF_INTR_TIMER,
+ /* AND count threshold interrupt. */
+ IRS_REGF_INTR_COUNT_THR_AND,
+ /* OR count threshold interrupt. */
+ IRS_REGF_INTR_COUNT_THR_OR,
+
+ /* Low-pass filter frequency. */
+ IRS_REGF_LP_FILTER,
+ /* High-pass filter frequency. */
+ IRS_REGF_HP_FILTER,
+
+ /* Sentinel value. */
+ IRS_REGF_MAX
+};
+
+static const struct reg_field irsd200_regfields[] = {
+ [IRS_REGF_INTR_DATA] =
+ REG_FIELD(IRS_REG_INTR, IRS_INTR_DATA, IRS_INTR_DATA),
+ [IRS_REGF_INTR_TIMER] =
+ REG_FIELD(IRS_REG_INTR, IRS_INTR_TIMER, IRS_INTR_TIMER),
+ [IRS_REGF_INTR_COUNT_THR_AND] = REG_FIELD(
+ IRS_REG_INTR, IRS_INTR_COUNT_THR_AND, IRS_INTR_COUNT_THR_AND),
+ [IRS_REGF_INTR_COUNT_THR_OR] = REG_FIELD(
+ IRS_REG_INTR, IRS_INTR_COUNT_THR_OR, IRS_INTR_COUNT_THR_OR),
+
+ [IRS_REGF_LP_FILTER] = REG_FIELD(IRS_REG_FILTER, 1, 1),
+ [IRS_REGF_HP_FILTER] = REG_FIELD(IRS_REG_FILTER, 0, 0),
+};
+
+static const struct regmap_config irsd200_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = IRS_REG_TIMER_HI,
+};
+
+struct irsd200_data {
+ struct regmap *regmap;
+ struct regmap_field *regfields[IRS_REGF_MAX];
+ struct device *dev;
+};
+
+static int irsd200_setup(struct irsd200_data *data)
+{
+ unsigned int val;
+ int ret;
+
+ /* Disable all interrupt sources. */
+ ret = regmap_write(data->regmap, IRS_REG_INTR, 0);
+ if (ret) {
+ dev_err(data->dev, "Could not set interrupt sources (%d)\n",
+ ret);
+ return ret;
+ }
+
+ /* Set operation to active. */
+ ret = regmap_write(data->regmap, IRS_REG_OP, 0x00);
+ if (ret) {
+ dev_err(data->dev, "Could not set operation mode (%d)\n", ret);
+ return ret;
+ }
+
+ /* Clear threshold count. */
+ ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
+ if (ret) {
+ dev_err(data->dev, "Could not clear threshold count (%d)\n",
+ ret);
+ return ret;
+ }
+
+ /* Clear status. */
+ ret = regmap_write(data->regmap, IRS_REG_STATUS, 0x0f);
+ if (ret) {
+ dev_err(data->dev, "Could not clear status (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int irsd200_read_threshold(struct irsd200_data *data,
+ enum iio_event_direction dir, int *val)
+{
+ unsigned int regval;
+ unsigned int reg;
+ int scale;
+ int ret;
+
+ /* Set quantization scale. */
+ if (dir == IIO_EV_DIR_RISING) {
+ scale = IRS_THR_QUANT_SCALE;
+ reg = IRS_REG_THR_HI;
+ } else if (dir == IIO_EV_DIR_FALLING) {
+ scale = -IRS_THR_QUANT_SCALE;
+ reg = IRS_REG_THR_LO;
+ } else {
+ return -EINVAL;
+ }
+
+ ret = regmap_read(data->regmap, reg, ®val);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read threshold (%d)\n", ret);
+ return ret;
+ }
+
+ *val = ((int)regval) * scale;
+
+ return 0;
+}
+
+static int irsd200_write_threshold(struct irsd200_data *data,
+ enum iio_event_direction dir, int val)
+{
+ unsigned int regval;
+ unsigned int reg;
+ int scale;
+ int ret;
+
+ /* Set quantization scale. */
+ if (dir == IIO_EV_DIR_RISING) {
+ if (val < 0)
+ return -ERANGE;
+
+ scale = IRS_THR_QUANT_SCALE;
+ reg = IRS_REG_THR_HI;
+ } else if (dir == IIO_EV_DIR_FALLING) {
+ if (val > 0)
+ return -ERANGE;
+
+ scale = -IRS_THR_QUANT_SCALE;
+ reg = IRS_REG_THR_LO;
+ } else {
+ return -EINVAL;
+ }
+
+ regval = val / scale;
+
+ if (regval >= BIT(8))
+ return -ERANGE;
+
+ ret = regmap_write(data->regmap, reg, regval);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not write threshold (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int irsd200_read_data(struct irsd200_data *data, s16 *val)
+{
+ unsigned int tmpval;
+ int ret;
+
+ ret = regmap_read(data->regmap, IRS_REG_DATA_HI, &tmpval);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read hi data (%d)\n", ret);
+ return ret;
+ }
+
+ *val = (s16)(tmpval << 8);
+
+ ret = regmap_read(data->regmap, IRS_REG_DATA_LO, &tmpval);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read lo data (%d)\n", ret);
+ return ret;
+ }
+
+ *val |= tmpval;
+
+ return 0;
+}
+
+static int irsd200_read_data_rate(struct irsd200_data *data, int *val)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read(data->regmap, IRS_REG_DATA_RATE, ®val);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read data rate (%d)\n", ret);
+ return ret;
+ }
+
+ if (regval >= ARRAY_SIZE(irsd200_data_rates))
+ return -ERANGE;
+
+ *val = irsd200_data_rates[regval];
+
+ return 0;
+}
+
+static int irsd200_write_data_rate(struct irsd200_data *data, int val)
+{
+ size_t idx;
+ int ret;
+
+ for (idx = 0; idx < ARRAY_SIZE(irsd200_data_rates); ++idx) {
+ if (irsd200_data_rates[idx] == val)
+ break;
+ }
+
+ if (idx == ARRAY_SIZE(irsd200_data_rates))
+ return -ERANGE;
+
+ ret = regmap_write(data->regmap, IRS_REG_DATA_RATE, idx);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not write data rate (%d)\n", ret);
+ return ret;
+ }
+
+ /* Data sheet says the device needs 3 seconds of settling time. */
+ ssleep(3);
+
+ return 0;
+}
+
+static int irsd200_read_timer(struct irsd200_data *data, int *val, int *val2)
+{
+ unsigned int tmpval;
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read(data->regmap, IRS_REG_TIMER_HI, &tmpval);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read hi timer (%d)\n", ret);
+ return ret;
+ }
+
+ /* Value is 10 bits. IRS_REG_TIMER_HI is the two MSBs. */
+ regval = tmpval << 8;
+
+ ret = regmap_read(data->regmap, IRS_REG_TIMER_LO, &tmpval);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read lo timer (%d)\n", ret);
+ return ret;
+ }
+
+ regval |= tmpval;
+
+ ret = irsd200_read_data_rate(data, val2);
+ if (ret < 0)
+ return ret;
+
+ *val = regval;
+
+ return 0;
+}
+
+static int irsd200_write_timer(struct irsd200_data *data, int val, int val2)
+{
+ unsigned int regval;
+ int data_rate;
+ int ret;
+
+ if (val < 0 || val2 < 0)
+ return -ERANGE;
+
+ ret = irsd200_read_data_rate(data, &data_rate);
+ if (ret < 0)
+ return ret;
+
+ /* Quantize from seconds. */
+ regval = val * data_rate + (val2 * data_rate) / 1000000;
+
+ /* Value is 10 bits. */
+ if (regval >= BIT(10))
+ return -ERANGE;
+
+ /* IRS_REG_TIMER_HI is the 2 MSBs. */
+ ret = regmap_write(data->regmap, IRS_REG_TIMER_HI, regval >> 8);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not write hi timer (%d)\n", ret);
+ return ret;
+ }
+
+ /*
+ * IRS_REG_TIMER_LO is the 8 LSBs. Since regmap_config is setup with
+ * val_bits = 8, we don't have to mask the lower bits in regval when
+ * writing.
+ */
+ ret = regmap_write(data->regmap, IRS_REG_TIMER_LO, regval);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not write lo timer (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int irsd200_read_nr_count(struct irsd200_data *data, int *val)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read(data->regmap, IRS_REG_NR_COUNT, ®val);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read nr count (%d)\n", ret);
+ return ret;
+ }
+
+ *val = regval;
+
+ return 0;
+}
+
+static int irsd200_write_nr_count(struct irsd200_data *data, int val)
+{
+ unsigned int regval;
+ int ret;
+
+ /* A value of zero means that IRS_REG_STATUS is never set. */
+ if (val <= 0 || val >= BIT(3))
+ return -ERANGE;
+
+ regval = val;
+
+ if (regval >= 2) {
+ /*
+ * According to the data sheet, timer must be also set in this
+ * case (i.e. be non-zero). Check and enforce that.
+ */
+ ret = irsd200_read_timer(data, &val, &val);
+ if (ret < 0)
+ return ret;
+
+ if (val == 0) {
+ dev_err(data->dev,
+ "Timer must be non-zero when nr count is %u\n",
+ regval);
+ return -EPERM;
+ }
+ }
+
+ ret = regmap_write(data->regmap, IRS_REG_NR_COUNT, regval);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not write nr count (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int irsd200_read_lp_filter(struct irsd200_data *data, int *val)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_field_read(data->regfields[IRS_REGF_LP_FILTER], ®val);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read lp filter frequency (%d)\n",
+ ret);
+ return ret;
+ }
+
+ *val = irsd200_lp_filter_freq[regval];
+
+ return 0;
+}
+
+static int irsd200_write_lp_filter(struct irsd200_data *data, int val)
+{
+ size_t idx;
+ int ret;
+
+ for (idx = 0; idx < ARRAY_SIZE(irsd200_lp_filter_freq); ++idx) {
+ if (irsd200_lp_filter_freq[idx] == val)
+ break;
+ }
+
+ if (idx == ARRAY_SIZE(irsd200_lp_filter_freq))
+ return -ERANGE;
+
+ ret = regmap_field_write(data->regfields[IRS_REGF_LP_FILTER], idx);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not write lp filter frequency (%d)\n",
+ ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int irsd200_read_hp_filter(struct irsd200_data *data, int *val,
+ int *val2)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_field_read(data->regfields[IRS_REGF_HP_FILTER], ®val);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not read hp filter frequency (%d)\n",
+ ret);
+ return ret;
+ }
+
+ *val = irsd200_hp_filter_freq[regval];
+ *val2 = 10;
+
+ return 0;
+}
+
+static int irsd200_write_hp_filter(struct irsd200_data *data, int val, int val2)
+{
+ size_t idx;
+ int ret;
+
+ /* Truncate fractional part to one digit. */
+ val2 /= 100000;
+
+ for (idx = 0; idx < ARRAY_SIZE(irsd200_hp_filter_freq); ++idx) {
+ if (irsd200_hp_filter_freq[idx] == val2)
+ break;
+ }
+
+ if (idx == ARRAY_SIZE(irsd200_hp_filter_freq) || val != 0)
+ return -ERANGE;
+
+ ret = regmap_field_write(data->regfields[IRS_REGF_HP_FILTER], idx);
+ if (ret < 0) {
+ dev_err(data->dev, "Could not write hp filter frequency (%d)\n",
+ ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int irsd200_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct irsd200_data *data = iio_priv(indio_dev);
+ int ret;
+ s16 buf;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = irsd200_read_data(data, &buf);
+ if (ret < 0)
+ return ret;
+
+ *val = buf;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = irsd200_read_data_rate(data, val);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int irsd200_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *vals = irsd200_data_rates;
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(irsd200_data_rates);
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int irsd200_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct irsd200_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = irsd200_write_data_rate(data, val);
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int irsd200_read_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info, int *val, int *val2)
+{
+ struct irsd200_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = irsd200_read_threshold(data, dir, val);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_TIMEOUT:
+ ret = irsd200_read_timer(data, val, val2);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_FRACTIONAL;
+ case IIO_EV_INFO_PERIOD:
+ ret = irsd200_read_nr_count(data, val);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_LOW_PASS_FILTER_3DB:
+ ret = irsd200_read_lp_filter(data, val);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
+ ret = irsd200_read_hp_filter(data, val, val2);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int irsd200_write_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info, int val, int val2)
+{
+ struct irsd200_data *data = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return irsd200_write_threshold(data, dir, val);
+ case IIO_EV_INFO_TIMEOUT:
+ return irsd200_write_timer(data, val, val2);
+ case IIO_EV_INFO_PERIOD:
+ return irsd200_write_nr_count(data, val);
+ case IIO_EV_INFO_LOW_PASS_FILTER_3DB:
+ return irsd200_write_lp_filter(data, val);
+ case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
+ return irsd200_write_hp_filter(data, val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int irsd200_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct irsd200_data *data = iio_priv(indio_dev);
+ unsigned int val;
+ int ret;
+
+ switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ if (dir != IIO_EV_DIR_RISING && dir != IIO_EV_DIR_FALLING)
+ return -EINVAL;
+
+ ret = regmap_field_read(
+ data->regfields[IRS_REGF_INTR_COUNT_THR_OR], &val);
+ if (ret < 0)
+ return ret;
+
+ return val;
+ case IIO_EV_TYPE_CHANGE:
+ ret = regmap_field_read(
+ data->regfields[IRS_REGF_INTR_TIMER], &val);
+ if (ret < 0)
+ return ret;
+
+ return val;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int irsd200_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir, int state)
+{
+ struct irsd200_data *data = iio_priv(indio_dev);
+ unsigned int val;
+ int ret;
+
+ switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ /*
+ * There is no way to tell the hardware to only signal for a
+ * single direction. Let's just group IIO_EV_DIR_RISING and
+ * IIO_EV_DIR_FALLING together instead of doing extra
+ * (unnecessary) post-processing after an interrupt.
+ */
+
+ /* Clear the count register (by reading from it). */
+ ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
+ if (ret < 0)
+ return ret;
+
+ val = !!state;
+ ret = regmap_field_write(
+ data->regfields[IRS_REGF_INTR_COUNT_THR_OR], val);
+ if (ret < 0)
+ return ret;
+
+ return val;
+ case IIO_EV_TYPE_CHANGE:
+ val = !!state;
+ ret = regmap_field_write(data->regfields[IRS_REGF_INTR_TIMER],
+ val);
+ if (ret < 0)
+ return ret;
+
+ return val;
+ default:
+ return -EINVAL;
+ }
+}
+
+static irqreturn_t irsd200_irq_thread(int irq, void *dev_id)
+{
+ struct iio_dev *indio_dev = dev_id;
+ struct irsd200_data *data = iio_priv(indio_dev);
+ enum iio_event_direction dir;
+ unsigned int lower_count;
+ unsigned int upper_count;
+ unsigned int status = 0;
+ unsigned int source = 0;
+ unsigned int clear = 0;
+ unsigned int count = 0;
+ int ret;
+
+ ret = regmap_read(data->regmap, IRS_REG_INTR, &source);
+ if (ret) {
+ dev_err(data->dev, "Could not read interrupt source (%d)\n",
+ ret);
+ return IRQ_NONE;
+ }
+
+ ret = regmap_read(data->regmap, IRS_REG_STATUS, &status);
+ if (ret) {
+ dev_err(data->dev, "Could not acknowledge interrupt (%d)\n",
+ ret);
+ return IRQ_NONE;
+ }
+
+ if (status & BIT(IRS_INTR_DATA) && iio_buffer_enabled(indio_dev)) {
+ iio_trigger_poll_nested(indio_dev->trig);
+ clear |= BIT(IRS_INTR_DATA);
+ }
+
+ if (status & BIT(IRS_INTR_TIMER) && source & BIT(IRS_INTR_TIMER)) {
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+ IIO_EV_TYPE_CHANGE,
+ IIO_EV_DIR_NONE),
+ iio_get_time_ns(indio_dev));
+ clear |= BIT(IRS_INTR_TIMER);
+ }
+
+ if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
+ source & BIT(IRS_INTR_COUNT_THR_OR)) {
+ /*
+ * The register value resets to zero after reading. We therefore
+ * need to read once and manually extract the lower and upper
+ * count register fields.
+ */
+ ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
+ if (ret)
+ dev_err(data->dev, "Could not read count (%d)\n", ret);
+
+ upper_count = IRS_UPPER_COUNT(count);
+ lower_count = IRS_LOWER_COUNT(count);
+
+ /*
+ * We only check the OR mode to be able to push events for
+ * rising and falling thresholds. AND mode is covered when both
+ * upper and lower count is non-zero, and is signaled with
+ * IIO_EV_DIR_EITHER.
+ */
+ if (upper_count && !lower_count)
+ dir = IIO_EV_DIR_RISING;
+ else if (!upper_count && lower_count)
+ dir = IIO_EV_DIR_FALLING;
+ else
+ dir = IIO_EV_DIR_EITHER;
+
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+ IIO_EV_TYPE_THRESH, dir),
+ iio_get_time_ns(indio_dev));
+
+ /*
+ * The OR mode will always trigger when the AND mode does, but
+ * not vice versa. However, it seems like the AND bit needs to
+ * be cleared if data capture _and_ threshold count interrupts
+ * are desirable, even though it hasn't explicitly been selected
+ * (with IRS_REG_INTR). Either way, it doesn't hurt...
+ */
+ clear |= BIT(IRS_INTR_COUNT_THR_OR) |
+ BIT(IRS_INTR_COUNT_THR_AND);
+ }
+
+ if (clear) {
+ ret = regmap_write(data->regmap, IRS_REG_STATUS, clear);
+ if (ret)
+ dev_err(data->dev,
+ "Could not clear interrupt status (%d)\n", ret);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t irsd200_trigger_handler(int irq, void *pollf)
+{
+ struct iio_dev *indio_dev = ((struct iio_poll_func *)pollf)->indio_dev;
+ struct irsd200_data *data = iio_priv(indio_dev);
+ s16 buf = 0;
+ int ret;
+
+ ret = irsd200_read_data(data, &buf);
+ if (ret)
+ goto end;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &buf,
+ iio_get_time_ns(indio_dev));
+
+end:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return ret ? IRQ_NONE : IRQ_HANDLED;
+}
+
+static int irsd200_buf_postenable(struct iio_dev *indio_dev)
+{
+ struct irsd200_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = regmap_field_write(data->regfields[IRS_REGF_INTR_DATA], 1);
+ if (ret) {
+ dev_err(data->dev,
+ "Could not enable data interrupt source (%d)\n", ret);
+ }
+
+ return ret;
+}
+
+static int irsd200_buf_predisable(struct iio_dev *indio_dev)
+{
+ struct irsd200_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = regmap_field_write(data->regfields[IRS_REGF_INTR_DATA], 0);
+ if (ret) {
+ dev_err(data->dev,
+ "Could not disable data interrupt source (%d)\n", ret);
+ }
+
+ return ret;
+}
+
+static const struct iio_info irsd200_info = {
+ .read_raw = irsd200_read_raw,
+ .read_avail = irsd200_read_avail,
+ .write_raw = irsd200_write_raw,
+ .read_event_value = irsd200_read_event,
+ .write_event_value = irsd200_write_event,
+ .read_event_config = irsd200_read_event_config,
+ .write_event_config = irsd200_write_event_config,
+};
+
+static const struct iio_buffer_setup_ops irsd200_buf_ops = {
+ .postenable = &irsd200_buf_postenable,
+ .predisable = &irsd200_buf_predisable,
+};
+
+static const struct iio_trigger_ops irsd200_trigger_ops = {
+ .validate_device = iio_trigger_validate_own_device,
+};
+
+static const struct iio_event_spec irsd200_event_spec[] = {
+ /* Upper threshold value. */
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate =
+ BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
+ },
+ /* Lower threshold value. */
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate =
+ BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
+ },
+ /* Window time. */
+ {
+ .type = IIO_EV_TYPE_CHANGE,
+ .dir = IIO_EV_DIR_NONE,
+ .mask_separate =
+ BIT(IIO_EV_INFO_TIMEOUT) | BIT(IIO_EV_INFO_ENABLE),
+ },
+ /* Number of counts (exceeding thresholds) before interrupt. */
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_PERIOD),
+ },
+ /* Low-pass filter frequency. */
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_LOW_PASS_FILTER_3DB),
+ },
+ /* High-pass filter frequency. */
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
+ },
+};
+
+static const struct iio_chan_spec irsd200_channels[] = {
+ {
+ .type = IIO_PROXIMITY,
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .event_spec = irsd200_event_spec,
+ .num_event_specs = ARRAY_SIZE(irsd200_event_spec),
+ .scan_type = {
+ .sign = 's',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_CPU,
+ },
+ },
+};
+
+static int irsd200_probe(struct i2c_client *client)
+{
+ struct iio_trigger *trigger;
+ struct irsd200_data *data;
+ struct iio_dev *indio_dev;
+ struct regmap *regmap;
+ size_t i;
+ int ret;
+
+ regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(&client->dev, "Could not initialize regmap\n");
+ return PTR_ERR(regmap);
+ }
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (!indio_dev) {
+ dev_err(&client->dev, "Could not allocate iio device\n");
+ return -ENOMEM;
+ }
+
+ data = iio_priv(indio_dev);
+ data->regmap = regmap;
+ data->dev = &client->dev;
+ i2c_set_clientdata(client, indio_dev);
+
+ for (i = 0; i < IRS_REGF_MAX; ++i) {
+ data->regfields[i] = devm_regmap_field_alloc(
+ data->dev, data->regmap, irsd200_regfields[i]);
+ if (IS_ERR(data->regfields[i])) {
+ dev_err(data->dev,
+ "Could not allocate register field %zu\n", i);
+ return PTR_ERR(data->regfields[i]);
+ }
+ }
+
+ ret = irsd200_setup(data);
+ if (ret)
+ return ret;
+
+ indio_dev->info = &irsd200_info;
+ indio_dev->name = IRS_DRV_NAME;
+ indio_dev->channels = irsd200_channels;
+ indio_dev->num_channels = ARRAY_SIZE(irsd200_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ if (!client->irq) {
+ dev_err(&client->dev, "No irq available\n");
+ return -ENXIO;
+ }
+
+ ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+ irsd200_trigger_handler,
+ &irsd200_buf_ops);
+ if (ret) {
+ dev_err(&client->dev,
+ "Could not setup iio triggered buffer (%d)\n", ret);
+ return ret;
+ }
+
+ ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+ irsd200_irq_thread,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ NULL, indio_dev);
+ if (ret) {
+ return dev_err_probe(&client->dev, ret,
+ "Could not request irq (%d)\n", ret);
+ }
+
+ trigger = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
+ indio_dev->name,
+ iio_device_id(indio_dev));
+ if (!trigger) {
+ dev_err(&client->dev, "Could not allocate iio trigger\n");
+ return -ENOMEM;
+ }
+
+ trigger->ops = &irsd200_trigger_ops;
+ iio_trigger_set_drvdata(trigger, indio_dev);
+
+ ret = devm_iio_trigger_register(&client->dev, trigger);
+ if (ret) {
+ dev_err(&client->dev, "Could not register iio trigger (%d)\n",
+ ret);
+ return ret;
+ }
+
+ ret = devm_iio_device_register(&client->dev, indio_dev);
+ if (ret) {
+ dev_err(&client->dev, "Could not register iio device (%d)\n",
+ ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id irsd200_of_match[] = {
+ {
+ .compatible = "murata,irsd200",
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(of, irsd200_of_match);
+
+static struct i2c_driver irsd200_driver = {
+ .driver = {
+ .name = IRS_DRV_NAME,
+ .of_match_table = irsd200_of_match,
+ },
+ .probe = irsd200_probe,
+};
+module_i2c_driver(irsd200_driver);
+
+MODULE_AUTHOR("Waqar Hameed <[email protected]>");
+MODULE_DESCRIPTION("Murata IRS-D200 PIR sensor driver");
+MODULE_LICENSE("GPL");
--
2.30.2
On 6/16/23 08:10, Waqar Hameed wrote:
> Murata IRS-D200 is a PIR sensor for human detection. It has support for
> raw data measurements and detection event notification.
>
> Add a driver with support for triggered buffer and events. Map the
> various settings to the `iio` framework, e.g. threshold values, sampling
> frequency, filter frequencies etc.
>
> Signed-off-by: Waqar Hameed <[email protected]>
Looks very good, small minor comments.
> [...]
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index cc838bb5408a..f36598380446 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -6,6 +6,7 @@
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_AS3935) += as3935.o
> obj-$(CONFIG_CROS_EC_MKBP_PROXIMITY) += cros_ec_mkbp_proximity.o
> +obj-$(CONFIG_IRSD200) += irsd200.o
> obj-$(CONFIG_ISL29501) += isl29501.o
> obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
> obj-$(CONFIG_MB1232) += mb1232.o
> diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
> new file mode 100644
> index 000000000000..699801d60295
> --- /dev/null
> +++ b/drivers/iio/proximity/irsd200.c
> @@ -0,0 +1,1051 @@
> [...]
> +/*
> + * The upper 4 bits in register IRS_REG_COUNT value is the upper count value
> + * (exceeding upper threshold value). The lower 4 is the lower count value
> + * (exceeding lower threshold value).
> + */
> +#define IRS_UPPER_COUNT(count) (count >> 4)
> +#define IRS_LOWER_COUNT(count) (count & GENMASK(3, 0))
Usually we add parenthesis around macro arguments to avoid issues in
case the argument is a non-singular expression.
> [...]
>
> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
> +{
> + unsigned int tmpval;
> + int ret;
> +
> + ret = regmap_read(data->regmap, IRS_REG_DATA_HI, &tmpval);
> + if (ret < 0) {
> + dev_err(data->dev, "Could not read hi data (%d)\n", ret);
> + return ret;
> + }
> +
> + *val = (s16)(tmpval << 8);
> +
> + ret = regmap_read(data->regmap, IRS_REG_DATA_LO, &tmpval);
> + if (ret < 0) {
> + dev_err(data->dev, "Could not read lo data (%d)\n", ret);
> + return ret;
> + }
Is there a way to bulk read those registers in one go to avoid
inconsistent data if they change while being read?
> + *val |= tmpval;
> +
> + return 0;
> +}
> [...]
> +static int irsd200_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = irsd200_write_data_rate(data, val);
> + return ret;
Maybe just `return irsd200_write_data_rate(...)`
> + default:
> + return -EINVAL;
> + }
> +}
> +
>
> [...]
> +static int irsd200_probe(struct i2c_client *client)
> +{
> + struct iio_trigger *trigger;
> + struct irsd200_data *data;
> + struct iio_dev *indio_dev;
> + struct regmap *regmap;
> + size_t i;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(&client->dev, "Could not initialize regmap\n");
dev_err_probe() is the more modern variant for error reporting in the
probe function. Same for all the other dev_err() in this function.
> + return PTR_ERR(regmap);
> + }
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev) {
> + dev_err(&client->dev, "Could not allocate iio device\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> + data->regmap = regmap;
> + data->dev = &client->dev;
> + i2c_set_clientdata(client, indio_dev);
> +
> + for (i = 0; i < IRS_REGF_MAX; ++i) {
> + data->regfields[i] = devm_regmap_field_alloc(
> + data->dev, data->regmap, irsd200_regfields[i]);
> + if (IS_ERR(data->regfields[i])) {
> + dev_err(data->dev,
> + "Could not allocate register field %zu\n", i);
> + return PTR_ERR(data->regfields[i]);
> + }
> + }
> +
> [...]
On 16/06/2023 17:10, Waqar Hameed wrote:
> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
> for communication with interrupt support. Add devicetree bindings
> requiring the compatible string, I2C slave address (reg) and interrupts.
Thank you for your patch. There is something to discuss/improve. I have
actually only remark about DTS example, but since I expect resend two
more nits as well.
A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.
>
> Signed-off-by: Waqar Hameed <[email protected]>
> ---
> .../iio/proximity/murata,irsd200.yaml | 54 +++++++++++++++++++
> 1 file changed, 54 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> new file mode 100644
> index 000000000000..d317fbe7bd50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/murata,irsd200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Murata IRS-D200 PIR sensor
> +
> +maintainers:
> + - Waqar Hameed <[email protected]>
> +
> +description: |
Nit, do not need '|' unless you need to preserve formatting.
> + PIR sensor for human detection.
> +
> +properties:
> + compatible:
> + const: murata,irsd200
> +
> + reg:
> + items:
> + - enum:
> + - 0x48
> + - 0x49
> + description: |
> + When the AD pin is connected to GND, the slave address is 0x48.
> + When the AD pin is connected to VDD, the slave address is 0x49.
> +
> + interrupts:
> + maxItems: 1
> + description:
> + Type should be IRQ_TYPE_EDGE_RISING.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pir@48 {
Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
so I guess: proximity@?
Best regards,
Krzysztof
On Fri, 16 Jun 2023 17:10:42 +0200
Waqar Hameed <[email protected]> wrote:
> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
> for communication with interrupt support. Add devicetree bindings
> requiring the compatible string, I2C slave address (reg) and interrupts.
>
> Signed-off-by: Waqar Hameed <[email protected]>
This device will have some power supplies, so I'd expect those to be both
listed and marked as required (maybe some are optional?)
Other than that and the points in the other review one thing inline about interrupts.
Jonathan
> ---
> .../iio/proximity/murata,irsd200.yaml | 54 +++++++++++++++++++
> 1 file changed, 54 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> new file mode 100644
> index 000000000000..d317fbe7bd50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/murata,irsd200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Murata IRS-D200 PIR sensor
> +
> +maintainers:
> + - Waqar Hameed <[email protected]>
> +
> +description: |
> + PIR sensor for human detection.
> +
> +properties:
> + compatible:
> + const: murata,irsd200
> +
> + reg:
> + items:
> + - enum:
> + - 0x48
> + - 0x49
> + description: |
> + When the AD pin is connected to GND, the slave address is 0x48.
> + When the AD pin is connected to VDD, the slave address is 0x49.
> +
> + interrupts:
> + maxItems: 1
> + description:
> + Type should be IRQ_TYPE_EDGE_RISING.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
If it is possible to remove interrupts from requires - and hence have
at least a partly functional driver doing basic reading of the sensor
then that is usually a good idea. Far too many board designers seem
to decide that they don't need to wire up interrupt lines.
If it's really hard then don't worry too much.
Jonathan
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pir@48 {
> + compatible = "murata,irsd200";
> + reg = <0x48>;
> + interrupts = <24 IRQ_TYPE_EDGE_RISING>;
> + };
> + };
> +...
On Fri, 16 Jun 2023 17:10:42 +0200
Waqar Hameed <[email protected]> wrote:
> Murata IRS-D200 is a PIR sensor for human detection. It has support for
> raw data measurements and detection event notification.
>
> Add a driver with support for triggered buffer and events. Map the
> various settings to the `iio` framework, e.g. threshold values, sampling
> frequency, filter frequencies etc.
>
> Signed-off-by: Waqar Hameed <[email protected]>
Hi Waqar,
Generally looks pretty good to me.
A few comments inline to add to Lars-Peter's review.
Jonathan
> diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
> new file mode 100644
> index 000000000000..699801d60295
> --- /dev/null
> +++ b/drivers/iio/proximity/irsd200.c
> @@ -0,0 +1,1051 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for Murata IRS-D200 PIR sensor.
> + *
> + * Copyright (C) 2023 Axis Communications AB
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/types.h>
> +
> +#define IRS_DRV_NAME "irsd200"
> +
> +/* Registers. */
> +#define IRS_REG_OP 0x00 /* Operation mode. */
> +#define IRS_REG_DATA_LO 0x02 /* Sensor data LSB. */
> +#define IRS_REG_DATA_HI 0x03 /* Sensor data MSB. */
> +#define IRS_REG_STATUS 0x04 /* Interrupt status. */
> +#define IRS_REG_COUNT 0x05 /* Count of exceeding threshold. */
> +#define IRS_REG_DATA_RATE 0x06 /* Output data rate. */
> +#define IRS_REG_FILTER 0x07 /* High-pass and low-pass filter. */
> +#define IRS_REG_INTR 0x09 /* Interrupt mode. */
> +#define IRS_REG_NR_COUNT 0x0a /* Number of counts before interrupt. */
> +#define IRS_REG_THR_HI 0x0b /* Upper threshold. */
> +#define IRS_REG_THR_LO 0x0c /* Lower threshold. */
> +#define IRS_REG_TIMER_LO 0x0d /* Timer setting LSB. */
> +#define IRS_REG_TIMER_HI 0x0e /* Timer setting MSB. */
> +
> +/* Interrupt status bits. */
> +#define IRS_INTR_DATA 0 /* Data update. */
> +#define IRS_INTR_TIMER 1 /* Timer expiration. */
> +#define IRS_INTR_COUNT_THR_AND 2 /* Count "AND" threshold. */
> +#define IRS_INTR_COUNT_THR_OR 3 /* Count "OR" threshold. */
> +
> +/*
> + * Quantization scale value for threshold. Used for conversion from/to register
> + * value.
> + */
> +#define IRS_THR_QUANT_SCALE 128
> +
> +/*
> + * The upper 4 bits in register IRS_REG_COUNT value is the upper count value
> + * (exceeding upper threshold value). The lower 4 is the lower count value
> + * (exceeding lower threshold value).
Code makes this obvious. This is just a comment that might become wrong in
future (adds little) - so I'd drop it.
> + */
> +#define IRS_UPPER_COUNT(count) (count >> 4)
> +#define IRS_LOWER_COUNT(count) (count & GENMASK(3, 0))
What Lars said on this one ((count) & GENMAKS(3, 0)
or better yet FIELD_GET() with appropriately defined mask for each of these
as that is more readable in general.
> +/*
> + * Index corresponds to the (field) value of IRS_REG_FILTER register. Contains
> + * only the fractional part (since the integer part is 0, e.g the first value
> + * corresponds to 0.3 Hz).
> + */
> +static const unsigned int irsd200_hp_filter_freq[2] = {
[] probably easier. Means reviewer doesn't need to sanity the length.
> + 3,
> + 5,
> +};
> +
> +static int irsd200_setup(struct irsd200_data *data)
> +{
> + unsigned int val;
> + int ret;
> +
> + /* Disable all interrupt sources. */
I assume the device doesn't provide any means of resetting it to get a known
register state? Bit unusual to need to disable interrupts rather than be
able to assume they are already disabled.
> + ret = regmap_write(data->regmap, IRS_REG_INTR, 0);
> + if (ret) {
> + dev_err(data->dev, "Could not set interrupt sources (%d)\n",
> + ret);
> + return ret;
> + }
> +
> + /* Set operation to active. */
> + ret = regmap_write(data->regmap, IRS_REG_OP, 0x00);
That 0 value probably wants a define.
> + if (ret) {
> + dev_err(data->dev, "Could not set operation mode (%d)\n", ret);
> + return ret;
> + }
> +
> + /* Clear threshold count. */
> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
> + if (ret) {
> + dev_err(data->dev, "Could not clear threshold count (%d)\n",
> + ret);
> + return ret;
> + }
> +
> + /* Clear status. */
> + ret = regmap_write(data->regmap, IRS_REG_STATUS, 0x0f);
> + if (ret) {
> + dev_err(data->dev, "Could not clear status (%d)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +
> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
> +{
> + unsigned int tmpval;
> + int ret;
> +
> + ret = regmap_read(data->regmap, IRS_REG_DATA_HI, &tmpval);
> + if (ret < 0) {
> + dev_err(data->dev, "Could not read hi data (%d)\n", ret);
> + return ret;
> + }
> +
> + *val = (s16)(tmpval << 8);
> +
> + ret = regmap_read(data->regmap, IRS_REG_DATA_LO, &tmpval);
> + if (ret < 0) {
> + dev_err(data->dev, "Could not read lo data (%d)\n", ret);
> + return ret;
> + }
> +
> + *val |= tmpval;
As below - bulk read and endian conversion if possible.
> +
> + return 0;
> +}
> +
> +stan 0;
> +}
> +
> +static int irsd200_write_data_rate(struct irsd200_data *data, int val)
> +{
> + size_t idx;
> + int ret;
> +
> + for (idx = 0; idx < ARRAY_SIZE(irsd200_data_rates); ++idx) {
> + if (irsd200_data_rates[idx] == val)
> + break;
> + }
> +
> + if (idx == ARRAY_SIZE(irsd200_data_rates))
> + return -ERANGE;
> +
> + ret = regmap_write(data->regmap, IRS_REG_DATA_RATE, idx);
> + if (ret < 0) {
> + dev_err(data->dev, "Could not write data rate (%d)\n", ret);
> + return ret;
> + }
> +
> + /* Data sheet says the device needs 3 seconds of settling time. */
> + ssleep(3);
You aren't preventing other userspace reads / writes during that time so
this is a light protection at best.
> +
> + return 0;
> +}
> +
> +static int irsd200_read_timer(struct irsd200_data *data, int *val, int *val2)
> +{
> + unsigned int tmpval;
> + unsigned int regval;
> + int ret;
> +
> + ret = regmap_read(data->regmap, IRS_REG_TIMER_HI, &tmpval);
> + if (ret < 0) {
> + dev_err(data->dev, "Could not read hi timer (%d)\n", ret);
> + return ret;
> + }
> +
> + /* Value is 10 bits. IRS_REG_TIMER_HI is the two MSBs. */
> + regval = tmpval << 8;
> +
> + ret = regmap_read(data->regmap, IRS_REG_TIMER_LO, &tmpval);
> + if (ret < 0) {
> + dev_err(data->dev, "Could not read lo timer (%d)\n", ret);
> + return ret;
> + }
Bulk read and endian conversion preferred for this as Lars pointed out.
Help with various thing including preventing tearing of the value if someone
else is writing whilst it is being read.
In general, you may want to look at using a local lock to ensure consistent
state whenever there are multiple reads / writes in a given function that userspace
can cause to be called.
> +
> + regval |= tmpval;
> +
> + ret = irsd200_read_data_rate(data, val2);
> + if (ret < 0)
> + return ret;
> +
> + *val = regval;
> +
> + return 0;
> +}
...
> +
> +static int irsd200_write_hp_filter(struct irsd200_data *data, int val, int val2)
> +{
> + size_t idx;
> + int ret;
> +
> + /* Truncate fractional part to one digit. */
> + val2 /= 100000;
> +
> + for (idx = 0; idx < ARRAY_SIZE(irsd200_hp_filter_freq); ++idx) {
> + if (irsd200_hp_filter_freq[idx] == val2)
> + break;
> + }
> +
> + if (idx == ARRAY_SIZE(irsd200_hp_filter_freq) || val != 0)
> + return -ERANGE;
> +
> + ret = regmap_field_write(data->regfields[IRS_REGF_HP_FILTER], idx);
> + if (ret < 0) {
For regmap calls, they are (I think) all documented as returning 0 for success
and negative otherwise. That lets you both check the simpler if (ret) and ...
> + dev_err(data->dev, "Could not write hp filter frequency (%d)\n",
> + ret);
> + return ret;
drop this return ret out of the if block here.
In general being able to ignore possibility of ret > 0 simplifies handling.
> + }
> +
> + return 0;
> +}
> +
> +static int irsd200_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> + int ret;
> + s16 buf;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = irsd200_read_data(data, &buf);
> + if (ret < 0)
> + return ret;
> +
> + *val = buf;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = irsd200_read_data_rate(data, val);
> + if (ret < 0)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int irsd200_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *vals = irsd200_data_rates;
> + *type = IIO_VAL_INT;
> + *length = ARRAY_SIZE(irsd200_data_rates);
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int irsd200_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = irsd200_write_data_rate(data, val);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int irsd200_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int val, int val2)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + return irsd200_write_threshold(data, dir, val);
> + case IIO_EV_INFO_TIMEOUT:
> + return irsd200_write_timer(data, val, val2);
> + case IIO_EV_INFO_PERIOD:
> + return irsd200_write_nr_count(data, val);
> + case IIO_EV_INFO_LOW_PASS_FILTER_3DB:
> + return irsd200_write_lp_filter(data, val);
> + case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
> + return irsd200_write_hp_filter(data, val, val2);
Just to check - filtering is only on the data being used for events? Not
on the direct readback of the data? Whilst that happens, it's not all that
common - hence the check.
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int irsd200_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> + unsigned int val;
> + int ret;
> +
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + if (dir != IIO_EV_DIR_RISING && dir != IIO_EV_DIR_FALLING)
> + return -EINVAL;
> +
> + ret = regmap_field_read(
> + data->regfields[IRS_REGF_INTR_COUNT_THR_OR], &val);
> + if (ret < 0)
> + return ret;
> +
> + return val;
> + case IIO_EV_TYPE_CHANGE:
> + ret = regmap_field_read(
> + data->regfields[IRS_REGF_INTR_TIMER], &val);
> + if (ret < 0)
> + return ret;
> +
> + return val;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int irsd200_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> + unsigned int val;
> + int ret;
> +
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + /*
> + * There is no way to tell the hardware to only signal for a
> + * single direction. Let's just group IIO_EV_DIR_RISING and
> + * IIO_EV_DIR_FALLING together instead of doing extra
> + * (unnecessary) post-processing after an interrupt.
> + */
If you can't control them separately then you should use the EITHER direction
for the enable. That means you get in_proximity_thresh_en
covering both thresholds.
> +
> + /* Clear the count register (by reading from it). */
> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
> + if (ret < 0)
> + return ret;
> +
> + val = !!state;
> + ret = regmap_field_write(
> + data->regfields[IRS_REGF_INTR_COUNT_THR_OR], val);
> + if (ret < 0)
> + return ret;
> +
> + return val;
> + case IIO_EV_TYPE_CHANGE:
> + val = !!state;
> + ret = regmap_field_write(data->regfields[IRS_REGF_INTR_TIMER],
> + val);
> + if (ret < 0)
> + return ret;
> +
> + return val;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static irqreturn_t irsd200_irq_thread(int irq, void *dev_id)
> +{
> + struct iio_dev *indio_dev = dev_id;
> + struct irsd200_data *data = iio_priv(indio_dev);
> + enum iio_event_direction dir;
> + unsigned int lower_count;
> + unsigned int upper_count;
> + unsigned int status = 0;
> + unsigned int source = 0;
> + unsigned int clear = 0;
> + unsigned int count = 0;
> + int ret;
> +
> + ret = regmap_read(data->regmap, IRS_REG_INTR, &source);
> + if (ret) {
> + dev_err(data->dev, "Could not read interrupt source (%d)\n",
> + ret);
> + return IRQ_NONE;
> + }
> +
> + ret = regmap_read(data->regmap, IRS_REG_STATUS, &status);
> + if (ret) {
> + dev_err(data->dev, "Could not acknowledge interrupt (%d)\n",
> + ret);
> + return IRQ_NONE;
> + }
> +
> + if (status & BIT(IRS_INTR_DATA) && iio_buffer_enabled(indio_dev)) {
> + iio_trigger_poll_nested(indio_dev->trig);
> + clear |= BIT(IRS_INTR_DATA);
> + }
> +
> + if (status & BIT(IRS_INTR_TIMER) && source & BIT(IRS_INTR_TIMER)) {
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> + IIO_EV_TYPE_CHANGE,
> + IIO_EV_DIR_NONE),
As below, I'd like more explanation of what this is.
I can't find a datasheet to look it up in.
> + iio_get_time_ns(indio_dev));
> + clear |= BIT(IRS_INTR_TIMER);
> + }
> +
> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
> + /*
> + * The register value resets to zero after reading. We therefore
> + * need to read once and manually extract the lower and upper
> + * count register fields.
> + */
> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
> + if (ret)
> + dev_err(data->dev, "Could not read count (%d)\n", ret);
> +
> + upper_count = IRS_UPPER_COUNT(count);
> + lower_count = IRS_LOWER_COUNT(count);
> +
> + /*
> + * We only check the OR mode to be able to push events for
> + * rising and falling thresholds. AND mode is covered when both
> + * upper and lower count is non-zero, and is signaled with
> + * IIO_EV_DIR_EITHER.
Whey you say AND, you mean that both thresholds have been crossed but also that
in that configuration either being crossed would also have gotten us to here?
(as opposed to the interrupt only occurring if value is greater than rising threshold
and less than falling threshold?)
If it's the first then just report two events. Either means we don't know, rather
than we know both occurred. We don't document that well though - so something
we should improved there. whilst a bit confusing:
https://elixir.bootlin.com/linux/v6.4-rc6/source/Documentation/ABI/testing/sysfs-bus-iio#L792
talks about this.
The bracketed case is more annoying to deal with so I hope you don't mean that.
Whilst we've had sensors that support it in hardware for years, I don't think we
ever came up with a usecase that really justified describing it.
> + */
> + if (upper_count && !lower_count)
> + dir = IIO_EV_DIR_RISING;
> + else if (!upper_count && lower_count)
> + dir = IIO_EV_DIR_FALLING;
> + else
> + dir = IIO_EV_DIR_EITHER;
> +
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> + IIO_EV_TYPE_THRESH, dir),
> + iio_get_time_ns(indio_dev));
> +
> + /*
> + * The OR mode will always trigger when the AND mode does, but
> + * not vice versa. However, it seems like the AND bit needs to
> + * be cleared if data capture _and_ threshold count interrupts
> + * are desirable, even though it hasn't explicitly been selected
> + * (with IRS_REG_INTR). Either way, it doesn't hurt...
> + */
> + clear |= BIT(IRS_INTR_COUNT_THR_OR) |
> + BIT(IRS_INTR_COUNT_THR_AND);
> + }
> +
> + if (clear) {
> + ret = regmap_write(data->regmap, IRS_REG_STATUS, clear);
> + if (ret)
> + dev_err(data->dev,
> + "Could not clear interrupt status (%d)\n", ret);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t irsd200_trigger_handler(int irq, void *pollf)
> +{
> + struct iio_dev *indio_dev = ((struct iio_poll_func *)pollf)->indio_dev;
> + struct irsd200_data *data = iio_priv(indio_dev);
> + s16 buf = 0;
> + int ret;
> +
> + ret = irsd200_read_data(data, &buf);
> + if (ret)
> + goto end;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &buf,
> + iio_get_time_ns(indio_dev));
> +
> +end:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return ret ? IRQ_NONE : IRQ_HANDLED;
> +}
> +
> +static int irsd200_buf_postenable(struct iio_dev *indio_dev)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_field_write(data->regfields[IRS_REGF_INTR_DATA], 1);
This looks to be turning on the interrupt.
Whilst the abstract trigger driving capture into a buffer model doesn't always
cleanly fit a driver architecture, in this case this seems like it should be
in the set_trigger_state() callback not here.
> + if (ret) {
> + dev_err(data->dev,
> + "Could not enable data interrupt source (%d)\n", ret);
> + }
> +
> + return ret;
> +}
> +
> +static int irsd200_buf_predisable(struct iio_dev *indio_dev)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_field_write(data->regfields[IRS_REGF_INTR_DATA], 0);
> + if (ret) {
> + dev_err(data->dev,
> + "Could not disable data interrupt source (%d)\n", ret);
> + }
> +
> + return ret;
> +}
> +
> +static const struct iio_info irsd200_info = {
> + .read_raw = irsd200_read_raw,
> + .read_avail = irsd200_read_avail,
> + .write_raw = irsd200_write_raw,
> + .read_event_value = irsd200_read_event,
> + .write_event_value = irsd200_write_event,
> + .read_event_config = irsd200_read_event_config,
> + .write_event_config = irsd200_write_event_config,
> +};
> +
> +static const struct iio_buffer_setup_ops irsd200_buf_ops = {
> + .postenable = &irsd200_buf_postenable,
> + .predisable = &irsd200_buf_predisable,
> +};
> +
> +static const struct iio_trigger_ops irsd200_trigger_ops = {
> + .validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static const struct iio_event_spec irsd200_event_spec[] = {
> + /* Upper threshold value. */
As below, the code is pretty much self documenting, so I'd drop
comments that can just become out of date. Comments are good when they
add information but here I don't think they do.
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate =
> + BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
> + },
> + /* Lower threshold value. */
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate =
> + BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
> + },
> + /* Window time. */
> + {
> + .type = IIO_EV_TYPE_CHANGE,
> + .dir = IIO_EV_DIR_NONE,
> + .mask_separate =
> + BIT(IIO_EV_INFO_TIMEOUT) | BIT(IIO_EV_INFO_ENABLE),
This is unusual use of ABI. Please given some more detail on what it is.
> + },
> + /* Number of counts (exceeding thresholds) before interrupt. */
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_PERIOD),
> + },
> + /* Low-pass filter frequency. */
Documentation doesn't add anything over code, so drop it.
Also combined all these cases where type and dir match into one entry
with multiple bits set in mask_separate etc.
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_LOW_PASS_FILTER_3DB),
> + },
> + /* High-pass filter frequency. */
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
> + },
> +};
> +};
> +
> +static int irsd200_probe(struct i2c_client *client)
> +{
> + struct iio_trigger *trigger;
> + struct irsd200_data *data;
> + struct iio_dev *indio_dev;
> + struct regmap *regmap;
> + size_t i;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(&client->dev, "Could not initialize regmap\n");
> + return PTR_ERR(regmap);
> + }
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev) {
> + dev_err(&client->dev, "Could not allocate iio device\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> + data->regmap = regmap;
> + data->dev = &client->dev;
> + i2c_set_clientdata(client, indio_dev);
I'm not seeing any where this is ready back... Don't set it unless you need
it.
> +
> + for (i = 0; i < IRS_REGF_MAX; ++i) {
> + data->regfields[i] = devm_regmap_field_alloc(
> + data->dev, data->regmap, irsd200_regfields[i]);
> + if (IS_ERR(data->regfields[i])) {
> + dev_err(data->dev,
> + "Could not allocate register field %zu\n", i);
> + return PTR_ERR(data->regfields[i]);
> + }
> + }
> +
> + ret = irsd200_setup(data);
> + if (ret)
> + return ret;
> +
> + indio_dev->info = &irsd200_info;
> + indio_dev->name = IRS_DRV_NAME;
> + indio_dev->channels = irsd200_channels;
> + indio_dev->num_channels = ARRAY_SIZE(irsd200_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + if (!client->irq) {
> + dev_err(&client->dev, "No irq available\n");
> + return -ENXIO;
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> + irsd200_trigger_handler,
> + &irsd200_buf_ops);
> + if (ret) {
> + dev_err(&client->dev,
> + "Could not setup iio triggered buffer (%d)\n", ret);
> + return ret;
> + }
> +
> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + irsd200_irq_thread,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + NULL, indio_dev);
> + if (ret) {
> + return dev_err_probe(&client->dev, ret,
> + "Could not request irq (%d)\n", ret);
> + }
Brackets not needed. Doesn't matter as a one off, but if you take
a lot more of these to dev_err_probe() like here then it will shorten
your code a fair bit and make it slightly easier to review (more on the
screen at a time etc).
As noted in DT binding review, it's definitely a good thing to support
basic operation in the absence of the interrupt line being wired up.
Normally that involves not registering all the stuff that needs the interrupt
and providing alternative iio_info, channels and some polling code to
us for data ready detection. On majority of hardware you can make triggered
buffers work without interrupts (driven by an hrtimer trigger or similar)
so you keep that available for both interrupt and non interrupt cases.
> +
> + trigger = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!trigger) {
> + dev_err(&client->dev, "Could not allocate iio trigger\n");
> + return -ENOMEM;
> + }
> +
> + trigger->ops = &irsd200_trigger_ops;
> + iio_trigger_set_drvdata(trigger, indio_dev);
> +
> + ret = devm_iio_trigger_register(&client->dev, trigger);
> + if (ret) {
> + dev_err(&client->dev, "Could not register iio trigger (%d)\n",
> + ret);
> + return ret;
> + }
> +
> + ret = devm_iio_device_register(&client->dev, indio_dev);
> + if (ret) {
> + dev_err(&client->dev, "Could not register iio device (%d)\n",
> + ret);
> + return ret;
Lars pointed this out already. It is fine (and generally a good idea) to
use dev_err_probe() for paths like this in probe. Some of it's functionality
is irrelevant (the deferred handling) but it does no harm in other paths and
makes the code more compact and easier to review.
> + }
> +
> + return 0;
> +}
On Sat, Jun 17, 2023 at 13:55 +0100 Jonathan Cameron <[email protected]> wrote:
> On Fri, 16 Jun 2023 17:10:42 +0200
> Waqar Hameed <[email protected]> wrote:
>
>> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
>> for communication with interrupt support. Add devicetree bindings
>> requiring the compatible string, I2C slave address (reg) and interrupts.
>>
>> Signed-off-by: Waqar Hameed <[email protected]>
>
> This device will have some power supplies, so I'd expect those to be both
> listed and marked as required (maybe some are optional?)
Right, will add that here (and call `devm_regulator_get_enable()` in
driver's probe).
[...]
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>
> If it is possible to remove interrupts from requires - and hence have
> at least a partly functional driver doing basic reading of the sensor
> then that is usually a good idea. Far too many board designers seem
> to decide that they don't need to wire up interrupt lines.
>
> If it's really hard then don't worry too much.
I see. It would be possible, but would also require some work. Let's
leave it for now then?
On Sat, Jun 17, 2023 at 10:55 +0200 Krzysztof Kozlowski <[email protected]> wrote:
> On 16/06/2023 17:10, Waqar Hameed wrote:
>> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
>> for communication with interrupt support. Add devicetree bindings
>> requiring the compatible string, I2C slave address (reg) and interrupts.
>
> Thank you for your patch. There is something to discuss/improve. I have
> actually only remark about DTS example, but since I expect resend two
> more nits as well.
>
>
> A nit, subject: drop second/last, redundant "bindings for". The
> "dt-bindings" prefix is already stating that these are bindings.
Alright, will change to "dt-bindings: iio: proximity: Add Murata
IRS-D200".
[...]
>> +maintainers:
>> + - Waqar Hameed <[email protected]>
>> +
>> +description: |
>
> Nit, do not need '|' unless you need to preserve formatting.
Yes, will remove (a remnant from previous version that had multiple
lines... Sorry!).
[...]
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> + i2c {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + pir@48 {
>
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> so I guess: proximity@?
On that list there is "temperature-sensor". Would it make sense then to
call it "proximity-sensor"?
On 19/06/2023 12:40, Waqar Hameed wrote:
>> Node names should be generic. See also explanation and list of examples
>> in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
>> so I guess: proximity@?
>
> On that list there is "temperature-sensor". Would it make sense then to
> call it "proximity-sensor"?
Choose whatever is the most popular in existing Linux sources.
Best regards,
Krzysztof
On Sat, Jun 17, 2023 at 14:35 +0100 Jonathan Cameron <[email protected]> wrote:
> On Fri, 16 Jun 2023 17:10:42 +0200
> Waqar Hameed <[email protected]> wrote:
>
>> Murata IRS-D200 is a PIR sensor for human detection. It has support for
>> raw data measurements and detection event notification.
>>
>> Add a driver with support for triggered buffer and events. Map the
>> various settings to the `iio` framework, e.g. threshold values, sampling
>> frequency, filter frequencies etc.
>>
>> Signed-off-by: Waqar Hameed <[email protected]>
>
> Hi Waqar,
>
> Generally looks pretty good to me.
> A few comments inline to add to Lars-Peter's review.
Thank you for your thorough review!
>> diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
>> new file mode 100644
>> index 000000000000..699801d60295
>> --- /dev/null
>> +++ b/drivers/iio/proximity/irsd200.c
>> @@ -0,0 +1,1051 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Driver for Murata IRS-D200 PIR sensor.
>> + *
>> + * Copyright (C) 2023 Axis Communications AB
>> + */
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/types.h>
>> +
>> +#define IRS_DRV_NAME "irsd200"
>> +
>> +/* Registers. */
>> +#define IRS_REG_OP 0x00 /* Operation mode. */
>> +#define IRS_REG_DATA_LO 0x02 /* Sensor data LSB. */
>> +#define IRS_REG_DATA_HI 0x03 /* Sensor data MSB. */
>> +#define IRS_REG_STATUS 0x04 /* Interrupt status. */
>> +#define IRS_REG_COUNT 0x05 /* Count of exceeding threshold. */
>> +#define IRS_REG_DATA_RATE 0x06 /* Output data rate. */
>> +#define IRS_REG_FILTER 0x07 /* High-pass and low-pass filter. */
>> +#define IRS_REG_INTR 0x09 /* Interrupt mode. */
>> +#define IRS_REG_NR_COUNT 0x0a /* Number of counts before interrupt. */
>> +#define IRS_REG_THR_HI 0x0b /* Upper threshold. */
>> +#define IRS_REG_THR_LO 0x0c /* Lower threshold. */
>> +#define IRS_REG_TIMER_LO 0x0d /* Timer setting LSB. */
>> +#define IRS_REG_TIMER_HI 0x0e /* Timer setting MSB. */
>> +
>> +/* Interrupt status bits. */
>> +#define IRS_INTR_DATA 0 /* Data update. */
>> +#define IRS_INTR_TIMER 1 /* Timer expiration. */
>> +#define IRS_INTR_COUNT_THR_AND 2 /* Count "AND" threshold. */
>> +#define IRS_INTR_COUNT_THR_OR 3 /* Count "OR" threshold. */
>> +
>> +/*
>> + * Quantization scale value for threshold. Used for conversion from/to register
>> + * value.
>> + */
>> +#define IRS_THR_QUANT_SCALE 128
>> +
>> +/*
>> + * The upper 4 bits in register IRS_REG_COUNT value is the upper count value
>> + * (exceeding upper threshold value). The lower 4 is the lower count value
>> + * (exceeding lower threshold value).
>
> Code makes this obvious. This is just a comment that might become wrong in
> future (adds little) - so I'd drop it.
I understand, will do!
>> + */
>> +#define IRS_UPPER_COUNT(count) (count >> 4)
>> +#define IRS_LOWER_COUNT(count) (count & GENMASK(3, 0))
>
> What Lars said on this one ((count) & GENMAKS(3, 0)
>
> or better yet FIELD_GET() with appropriately defined mask for each of these
> as that is more readable in general.
I agree, `FIELD_GET()` is better.
>> +/*
>> + * Index corresponds to the (field) value of IRS_REG_FILTER register. Contains
>> + * only the fractional part (since the integer part is 0, e.g the first value
>> + * corresponds to 0.3 Hz).
>> + */
>> +static const unsigned int irsd200_hp_filter_freq[2] = {
>
> [] probably easier. Means reviewer doesn't need to sanity the length.
Alright.
>> + 3,
>> + 5,
>> +};
>
>> +
>> +static int irsd200_setup(struct irsd200_data *data)
>> +{
>> + unsigned int val;
>> + int ret;
>> +
>> + /* Disable all interrupt sources. */
>
> I assume the device doesn't provide any means of resetting it to get a known
> register state? Bit unusual to need to disable interrupts rather than be
> able to assume they are already disabled.
There is no dedicated reset (other than explicitly power cycle it of
course). I mean, according to the data sheet, the default value is zero
(i.e. interrupt disabled). It wouldn't hurt to be really sure here,
right?
This register and IRS_REG_OP below are the important one to be sure
about during the setup (we want to be sure that it is in _this_ state
here).
>> + ret = regmap_write(data->regmap, IRS_REG_INTR, 0);
>> + if (ret) {
>> + dev_err(data->dev, "Could not set interrupt sources (%d)\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + /* Set operation to active. */
>> + ret = regmap_write(data->regmap, IRS_REG_OP, 0x00);
>
> That 0 value probably wants a define.
Yeah, I'll add that.
>> + if (ret) {
>> + dev_err(data->dev, "Could not set operation mode (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + /* Clear threshold count. */
>> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
>> + if (ret) {
>> + dev_err(data->dev, "Could not clear threshold count (%d)\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + /* Clear status. */
>> + ret = regmap_write(data->regmap, IRS_REG_STATUS, 0x0f);
>> + if (ret) {
>> + dev_err(data->dev, "Could not clear status (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
>> +
>> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
>> +{
>> + unsigned int tmpval;
>> + int ret;
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_DATA_HI, &tmpval);
>> + if (ret < 0) {
>> + dev_err(data->dev, "Could not read hi data (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + *val = (s16)(tmpval << 8);
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_DATA_LO, &tmpval);
>> + if (ret < 0) {
>> + dev_err(data->dev, "Could not read lo data (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + *val |= tmpval;
>
> As below - bulk read and endian conversion if possible.
Yes, will use `regmap_bulk_read()`.
>> +
>> + return 0;
>> +}
>> +
>> +stan 0;
(What happened here?)
>> +}
>> +
>> +static int irsd200_write_data_rate(struct irsd200_data *data, int val)
>> +{
>> + size_t idx;
>> + int ret;
>> +
>> + for (idx = 0; idx < ARRAY_SIZE(irsd200_data_rates); ++idx) {
>> + if (irsd200_data_rates[idx] == val)
>> + break;
>> + }
>> +
>> + if (idx == ARRAY_SIZE(irsd200_data_rates))
>> + return -ERANGE;
>> +
>> + ret = regmap_write(data->regmap, IRS_REG_DATA_RATE, idx);
>> + if (ret < 0) {
>> + dev_err(data->dev, "Could not write data rate (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + /* Data sheet says the device needs 3 seconds of settling time. */
>> + ssleep(3);
> You aren't preventing other userspace reads / writes during that time so
> this is a light protection at best.
Yes, we aren't preventing anything. The hardware actually operates
without any "errors" during this period (e.g. buffer data and events
keep arriving).
This is more of a guarantee that "within 3 s, the new data rate should
be in effect". When I think about it, we should maybe just remove this
sleep?
>> +
>> + return 0;
>> +}
>> +
>> +static int irsd200_read_timer(struct irsd200_data *data, int *val, int *val2)
>> +{
>> + unsigned int tmpval;
>> + unsigned int regval;
>> + int ret;
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_TIMER_HI, &tmpval);
>> + if (ret < 0) {
>> + dev_err(data->dev, "Could not read hi timer (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + /* Value is 10 bits. IRS_REG_TIMER_HI is the two MSBs. */
>> + regval = tmpval << 8;
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_TIMER_LO, &tmpval);
>> + if (ret < 0) {
>> + dev_err(data->dev, "Could not read lo timer (%d)\n", ret);
>> + return ret;
>> + }
>
> Bulk read and endian conversion preferred for this as Lars pointed out.
> Help with various thing including preventing tearing of the value if someone
> else is writing whilst it is being read.
>
> In general, you may want to look at using a local lock to ensure consistent
> state whenever there are multiple reads / writes in a given function that userspace
> can cause to be called.
Yeah, will use bulk read here as well.
[...]
>> +static int irsd200_write_hp_filter(struct irsd200_data *data, int val, int val2)
>> +{
>> + size_t idx;
>> + int ret;
>> +
>> + /* Truncate fractional part to one digit. */
>> + val2 /= 100000;
>> +
>> + for (idx = 0; idx < ARRAY_SIZE(irsd200_hp_filter_freq); ++idx) {
>> + if (irsd200_hp_filter_freq[idx] == val2)
>> + break;
>> + }
>> +
>> + if (idx == ARRAY_SIZE(irsd200_hp_filter_freq) || val != 0)
>> + return -ERANGE;
>> +
>> + ret = regmap_field_write(data->regfields[IRS_REGF_HP_FILTER], idx);
>> + if (ret < 0) {
>
> For regmap calls, they are (I think) all documented as returning 0 for success
> and negative otherwise. That lets you both check the simpler if (ret) and ...
Maybe it _is_ too defensive here. However, I have encountered APIs where
positive values can/has been returned (due to debug/testing or just plain
slip ups), without any actual errors.
However, let's trust `regmap` then. I can change to only look for `if
(ret)` everywhere.
>
>> + dev_err(data->dev, "Could not write hp filter frequency (%d)\n",
>> + ret);
>> + return ret;
>
> drop this return ret out of the if block here.
>
> In general being able to ignore possibility of ret > 0 simplifies handling.
I try to be consistent and it also "helps" the next person potentially
adding code after the `if`-statement and forgetting about adding
`return`. We can drop the `return here, but then we should do the same
in other places with a check just before the last `return` (like
`irsd200_write_timer()`, `irsd200_read_nr_count()`,
`irsd200_write_nr_count()` and many more), right?
[...]
>> +
>> +static int irsd200_write_event(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + enum iio_event_info info, int val, int val2)
>> +{
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> +
>> + switch (info) {
>> + case IIO_EV_INFO_VALUE:
>> + return irsd200_write_threshold(data, dir, val);
>> + case IIO_EV_INFO_TIMEOUT:
>> + return irsd200_write_timer(data, val, val2);
>> + case IIO_EV_INFO_PERIOD:
>> + return irsd200_write_nr_count(data, val);
>> + case IIO_EV_INFO_LOW_PASS_FILTER_3DB:
>> + return irsd200_write_lp_filter(data, val);
>> + case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
>> + return irsd200_write_hp_filter(data, val, val2);
>
> Just to check - filtering is only on the data being used for events? Not
> on the direct readback of the data? Whilst that happens, it's not all that
> common - hence the check.
Well spotted! It actually made think here! :)
I do think it applies to the raw data (as well). Comparing that to
`IIO_CHAN_INFO_SAMP_FREQ` (which also affects the events) we should
therefore probably move the filtering to `struct iio_chan_spec
irsd200_channels`, right?
[...]
>> +static int irsd200_write_event_config(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir, int state)
>> +{
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> + unsigned int val;
>> + int ret;
>> +
>> + switch (type) {
>> + case IIO_EV_TYPE_THRESH:
>> + /*
>> + * There is no way to tell the hardware to only signal for a
>> + * single direction. Let's just group IIO_EV_DIR_RISING and
>> + * IIO_EV_DIR_FALLING together instead of doing extra
>> + * (unnecessary) post-processing after an interrupt.
>> + */
> If you can't control them separately then you should use the EITHER direction
> for the enable. That means you get in_proximity_thresh_en
> covering both thresholds.
Ah, of course! I'll change!
>> +
>> + /* Clear the count register (by reading from it). */
>> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + val = !!state;
>> + ret = regmap_field_write(
>> + data->regfields[IRS_REGF_INTR_COUNT_THR_OR], val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return val;
>> + case IIO_EV_TYPE_CHANGE:
>> + val = !!state;
>> + ret = regmap_field_write(data->regfields[IRS_REGF_INTR_TIMER],
>> + val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return val;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static irqreturn_t irsd200_irq_thread(int irq, void *dev_id)
>> +{
>> + struct iio_dev *indio_dev = dev_id;
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> + enum iio_event_direction dir;
>> + unsigned int lower_count;
>> + unsigned int upper_count;
>> + unsigned int status = 0;
>> + unsigned int source = 0;
>> + unsigned int clear = 0;
>> + unsigned int count = 0;
>> + int ret;
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_INTR, &source);
>> + if (ret) {
>> + dev_err(data->dev, "Could not read interrupt source (%d)\n",
>> + ret);
>> + return IRQ_NONE;
>> + }
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_STATUS, &status);
>> + if (ret) {
>> + dev_err(data->dev, "Could not acknowledge interrupt (%d)\n",
>> + ret);
>> + return IRQ_NONE;
>> + }
>> +
>> + if (status & BIT(IRS_INTR_DATA) && iio_buffer_enabled(indio_dev)) {
>> + iio_trigger_poll_nested(indio_dev->trig);
>> + clear |= BIT(IRS_INTR_DATA);
>> + }
>> +
>> + if (status & BIT(IRS_INTR_TIMER) && source & BIT(IRS_INTR_TIMER)) {
>> + iio_push_event(indio_dev,
>> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
>> + IIO_EV_TYPE_CHANGE,
>> + IIO_EV_DIR_NONE),
>
> As below, I'd like more explanation of what this is.
> I can't find a datasheet to look it up in.
This is a timer for the detection event window time, i.e. the signal
should pass the threshold values within this time in order to get an
interrupt (`IIO_EV_TYPE_THRESH`).
You setup the window time (`IIO_EV_INFO_TIMEOUT`), and when this timer
has expired, you get this interrupt (and thus `IIO_EV_TYPE_CHANGE`). I
couldn't find any other more fitting value in `enum iio_event_type`.
>> + iio_get_time_ns(indio_dev));
>> + clear |= BIT(IRS_INTR_TIMER);
>> + }
>> +
>> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
>> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
>> + /*
>> + * The register value resets to zero after reading. We therefore
>> + * need to read once and manually extract the lower and upper
>> + * count register fields.
>> + */
>> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
>> + if (ret)
>> + dev_err(data->dev, "Could not read count (%d)\n", ret);
>> +
>> + upper_count = IRS_UPPER_COUNT(count);
>> + lower_count = IRS_LOWER_COUNT(count);
>> +
>> + /*
>> + * We only check the OR mode to be able to push events for
>> + * rising and falling thresholds. AND mode is covered when both
>> + * upper and lower count is non-zero, and is signaled with
>> + * IIO_EV_DIR_EITHER.
>
> Whey you say AND, you mean that both thresholds have been crossed but also that
> in that configuration either being crossed would also have gotten us to here?
> (as opposed to the interrupt only occurring if value is greater than rising threshold
> and less than falling threshold?)
>
> If it's the first then just report two events. Either means we don't know, rather
> than we know both occurred. We don't document that well though - so something
> we should improved there. whilst a bit confusing:
> https://elixir.bootlin.com/linux/v6.4-rc6/source/Documentation/ABI/testing/sysfs-bus-iio#L792
> talks about this.
>
> The bracketed case is more annoying to deal with so I hope you don't mean that.
> Whilst we've had sensors that support it in hardware for years, I don't think we
> ever came up with a usecase that really justified describing it.
According to the data sheet (which will hopefully be soon publicly
available):
OR-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT)
AND-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT) &&
(UPPER_COUNT != 0) && (LOWER_COUNT != 0)
For example, consider the following situation:
___
/ \
-----------------------------3------------------- Upper threshold
___ / \
______ / \ / \___________ Data signal
\ / \ /
-------1------------2---------------------------- Lower threshold
\__/ \__/
When `NR_COUNT` is set to 3, we will get an OR-interrupt on point "3" in
the graph above. In this case `UPPER_COUNT = 1` and `LOWER_COUNT = 2`.
When `NR_COUNT` is set to 2, we will get an OR-interrupt on point "2"
instead. Here `UPPER_COUNT = 0` and `LOWER_COUNT = 2`.
>
>> + */
>> + if (upper_count && !lower_count)
>> + dir = IIO_EV_DIR_RISING;
>> + else if (!upper_count && lower_count)
>> + dir = IIO_EV_DIR_FALLING;
>> + else
>> + dir = IIO_EV_DIR_EITHER;
>> +
>> + iio_push_event(indio_dev,
>> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
>> + IIO_EV_TYPE_THRESH, dir),
>> + iio_get_time_ns(indio_dev));
>> +
>> + /*
>> + * The OR mode will always trigger when the AND mode does, but
>> + * not vice versa. However, it seems like the AND bit needs to
>> + * be cleared if data capture _and_ threshold count interrupts
>> + * are desirable, even though it hasn't explicitly been selected
>> + * (with IRS_REG_INTR). Either way, it doesn't hurt...
>> + */
>> + clear |= BIT(IRS_INTR_COUNT_THR_OR) |
>> + BIT(IRS_INTR_COUNT_THR_AND);
>> + }
>> +
>> + if (clear) {
>> + ret = regmap_write(data->regmap, IRS_REG_STATUS, clear);
>> + if (ret)
>> + dev_err(data->dev,
>> + "Could not clear interrupt status (%d)\n", ret);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t irsd200_trigger_handler(int irq, void *pollf)
>> +{
>> + struct iio_dev *indio_dev = ((struct iio_poll_func *)pollf)->indio_dev;
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> + s16 buf = 0;
>> + int ret;
>> +
>> + ret = irsd200_read_data(data, &buf);
>> + if (ret)
>> + goto end;
>> +
>> + iio_push_to_buffers_with_timestamp(indio_dev, &buf,
>> + iio_get_time_ns(indio_dev));
>> +
>> +end:
>> + iio_trigger_notify_done(indio_dev->trig);
>> +
>> + return ret ? IRQ_NONE : IRQ_HANDLED;
>> +}
>> +
>> +static int irsd200_buf_postenable(struct iio_dev *indio_dev)
>> +{
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + ret = regmap_field_write(data->regfields[IRS_REGF_INTR_DATA], 1);
>
> This looks to be turning on the interrupt.
> Whilst the abstract trigger driving capture into a buffer model doesn't always
> cleanly fit a driver architecture, in this case this seems like it should be
> in the set_trigger_state() callback not here.
Alright, I'll move them to `set_trigger_state()`.
>
>
>> + if (ret) {
>> + dev_err(data->dev,
>> + "Could not enable data interrupt source (%d)\n", ret);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int irsd200_buf_predisable(struct iio_dev *indio_dev)
>> +{
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + ret = regmap_field_write(data->regfields[IRS_REGF_INTR_DATA], 0);
>> + if (ret) {
>> + dev_err(data->dev,
>> + "Could not disable data interrupt source (%d)\n", ret);
>> + }
>> +
>> + return ret;
>> +}
>> +
[...]
>> +static const struct iio_event_spec irsd200_event_spec[] = {
>> + /* Upper threshold value. */
> As below, the code is pretty much self documenting, so I'd drop
> comments that can just become out of date. Comments are good when they
> add information but here I don't think they do.
Will remove!
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_RISING,
>> + .mask_separate =
>> + BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
>> + },
>> + /* Lower threshold value. */
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_FALLING,
>> + .mask_separate =
>> + BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
>> + },
>> + /* Window time. */
>> + {
>> + .type = IIO_EV_TYPE_CHANGE,
>> + .dir = IIO_EV_DIR_NONE,
>> + .mask_separate =
>> + BIT(IIO_EV_INFO_TIMEOUT) | BIT(IIO_EV_INFO_ENABLE),
>
> This is unusual use of ABI. Please given some more detail on what it is.
Explained above.
>
>> + },
>> + /* Number of counts (exceeding thresholds) before interrupt. */
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_EITHER,
>> + .mask_separate = BIT(IIO_EV_INFO_PERIOD),
>> + },
>> + /* Low-pass filter frequency. */
>
> Documentation doesn't add anything over code, so drop it.
> Also combined all these cases where type and dir match into one entry
> with multiple bits set in mask_separate etc.
Will do!
>
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_EITHER,
>> + .mask_separate = BIT(IIO_EV_INFO_LOW_PASS_FILTER_3DB),
>> + },
>> + /* High-pass filter frequency. */
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_EITHER,
>> + .mask_separate = BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
>> + },
>> +};
>
>> +};
>> +
>> +static int irsd200_probe(struct i2c_client *client)
>> +{
>> + struct iio_trigger *trigger;
>> + struct irsd200_data *data;
>> + struct iio_dev *indio_dev;
>> + struct regmap *regmap;
>> + size_t i;
>> + int ret;
>> +
>> + regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
>> + if (IS_ERR(regmap)) {
>> + dev_err(&client->dev, "Could not initialize regmap\n");
>> + return PTR_ERR(regmap);
>> + }
>> +
>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> + if (!indio_dev) {
>> + dev_err(&client->dev, "Could not allocate iio device\n");
>> + return -ENOMEM;
>> + }
>> +
>> + data = iio_priv(indio_dev);
>> + data->regmap = regmap;
>> + data->dev = &client->dev;
>> + i2c_set_clientdata(client, indio_dev);
>
> I'm not seeing any where this is ready back... Don't set it unless you need
> it.
You are right.
>
>> +
>> + for (i = 0; i < IRS_REGF_MAX; ++i) {
>> + data->regfields[i] = devm_regmap_field_alloc(
>> + data->dev, data->regmap, irsd200_regfields[i]);
>> + if (IS_ERR(data->regfields[i])) {
>> + dev_err(data->dev,
>> + "Could not allocate register field %zu\n", i);
>> + return PTR_ERR(data->regfields[i]);
>> + }
>> + }
>> +
>> + ret = irsd200_setup(data);
>> + if (ret)
>> + return ret;
>> +
>> + indio_dev->info = &irsd200_info;
>> + indio_dev->name = IRS_DRV_NAME;
>> + indio_dev->channels = irsd200_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(irsd200_channels);
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + if (!client->irq) {
>> + dev_err(&client->dev, "No irq available\n");
>> + return -ENXIO;
>> + }
>> +
>> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>> + irsd200_trigger_handler,
>> + &irsd200_buf_ops);
>> + if (ret) {
>> + dev_err(&client->dev,
>> + "Could not setup iio triggered buffer (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> + irsd200_irq_thread,
>> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> + NULL, indio_dev);
>> + if (ret) {
>> + return dev_err_probe(&client->dev, ret,
>> + "Could not request irq (%d)\n", ret);
>> + }
> Brackets not needed. Doesn't matter as a one off, but if you take
> a lot more of these to dev_err_probe() like here then it will shorten
> your code a fair bit and make it slightly easier to review (more on the
> screen at a time etc).
Interesting that `checkpatch.pl` missed this one. Yes, I'll change to
`dev_err_probe()`.
> As noted in DT binding review, it's definitely a good thing to support
> basic operation in the absence of the interrupt line being wired up.
> Normally that involves not registering all the stuff that needs the interrupt
> and providing alternative iio_info, channels and some polling code to
> us for data ready detection. On majority of hardware you can make triggered
> buffers work without interrupts (driven by an hrtimer trigger or similar)
> so you keep that available for both interrupt and non interrupt cases.
Answered there.
>> +
>> + trigger = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
>> + indio_dev->name,
>> + iio_device_id(indio_dev));
>> + if (!trigger) {
>> + dev_err(&client->dev, "Could not allocate iio trigger\n");
>> + return -ENOMEM;
>> + }
>> +
>> + trigger->ops = &irsd200_trigger_ops;
>> + iio_trigger_set_drvdata(trigger, indio_dev);
>> +
>> + ret = devm_iio_trigger_register(&client->dev, trigger);
>> + if (ret) {
>> + dev_err(&client->dev, "Could not register iio trigger (%d)\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + ret = devm_iio_device_register(&client->dev, indio_dev);
>> + if (ret) {
>> + dev_err(&client->dev, "Could not register iio device (%d)\n",
>> + ret);
>> + return ret;
>
> Lars pointed this out already. It is fine (and generally a good idea) to
> use dev_err_probe() for paths like this in probe. Some of it's functionality
> is irrelevant (the deferred handling) but it does no harm in other paths and
> makes the code more compact and easier to review.
Yeah, I'll change to `dev_err_probe()`.
>
>> + }
>> +
>> + return 0;
>> +}
On Fri, Jun 16, 2023 at 18:37 -0700 Lars-Peter Clausen <[email protected]> wrote:
> On 6/16/23 08:10, Waqar Hameed wrote:
>> Murata IRS-D200 is a PIR sensor for human detection. It has support for
>> raw data measurements and detection event notification.
>>
>> Add a driver with support for triggered buffer and events. Map the
>> various settings to the `iio` framework, e.g. threshold values, sampling
>> frequency, filter frequencies etc.
>>
>> Signed-off-by: Waqar Hameed <[email protected]>
>
> Looks very good, small minor comments.
Thanks!
[...]
>> index 000000000000..699801d60295
>> --- /dev/null
>> +++ b/drivers/iio/proximity/irsd200.c
>> @@ -0,0 +1,1051 @@
>> [...]
>> +/*
>> + * The upper 4 bits in register IRS_REG_COUNT value is the upper count value
>> + * (exceeding upper threshold value). The lower 4 is the lower count value
>> + * (exceeding lower threshold value).
>> + */
>> +#define IRS_UPPER_COUNT(count) (count >> 4)
>> +#define IRS_LOWER_COUNT(count) (count & GENMASK(3, 0))
>
> Usually we add parenthesis around macro arguments to avoid issues in case the
> argument is a non-singular expression.
Of course! Will use `FIELD_GET()` as Jonathan suggests.
>> [...]
>>
>> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
>> +{
>> + unsigned int tmpval;
>> + int ret;
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_DATA_HI, &tmpval);
>> + if (ret < 0) {
>> + dev_err(data->dev, "Could not read hi data (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + *val = (s16)(tmpval << 8);
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_DATA_LO, &tmpval);
>> + if (ret < 0) {
>> + dev_err(data->dev, "Could not read lo data (%d)\n", ret);
>> + return ret;
>> + }
> Is there a way to bulk read those registers in one go to avoid inconsistent data
> if they change while being read?
Yes, will use `regmap_bulk_read()`.
>> + *val |= tmpval;
>> +
>> + return 0;
>> +}
>> [...]
>> +static int irsd200_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int val,
>> + int val2, long mask)
>> +{
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + ret = irsd200_write_data_rate(data, val);
>> + return ret;
> Maybe just `return irsd200_write_data_rate(...)`
Of course! (Remnant from a refactorization... Sorry!)
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>>
>> [...]
>> +static int irsd200_probe(struct i2c_client *client)
>> +{
>> + struct iio_trigger *trigger;
>> + struct irsd200_data *data;
>> + struct iio_dev *indio_dev;
>> + struct regmap *regmap;
>> + size_t i;
>> + int ret;
>> +
>> + regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
>> + if (IS_ERR(regmap)) {
>> + dev_err(&client->dev, "Could not initialize regmap\n");
> dev_err_probe() is the more modern variant for error reporting in the probe
> function. Same for all the other dev_err() in this function.
Alright, I'll change to `dev_err_probe()`.
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +stan 0;
>
> (What happened here?)
No idea :)
> >> +}
> >> +
> >> +static int irsd200_write_data_rate(struct irsd200_data *data, int val)
> >> +{
> >> + size_t idx;
> >> + int ret;
> >> +
> >> + for (idx = 0; idx < ARRAY_SIZE(irsd200_data_rates); ++idx) {
> >> + if (irsd200_data_rates[idx] == val)
> >> + break;
> >> + }
> >> +
> >> + if (idx == ARRAY_SIZE(irsd200_data_rates))
> >> + return -ERANGE;
> >> +
> >> + ret = regmap_write(data->regmap, IRS_REG_DATA_RATE, idx);
> >> + if (ret < 0) {
> >> + dev_err(data->dev, "Could not write data rate (%d)\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + /* Data sheet says the device needs 3 seconds of settling time. */
> >> + ssleep(3);
> > You aren't preventing other userspace reads / writes during that time so
> > this is a light protection at best.
>
> Yes, we aren't preventing anything. The hardware actually operates
> without any "errors" during this period (e.g. buffer data and events
> keep arriving).
>
> This is more of a guarantee that "within 3 s, the new data rate should
> be in effect". When I think about it, we should maybe just remove this
> sleep?
Interesting corner case. I'd keep as you have it but add a little
more documentation as why. Having this sleep will make it easy for a single
thread in userspace to get what it expects.
>
> >> +
> >> + return 0;
> >> +}
...
> >> +static int irsd200_write_hp_filter(struct irsd200_data *data, int val, int val2)
> >> +{
> >> + size_t idx;
> >> + int ret;
> >> +
> >> + /* Truncate fractional part to one digit. */
> >> + val2 /= 100000;
> >> +
> >> + for (idx = 0; idx < ARRAY_SIZE(irsd200_hp_filter_freq); ++idx) {
> >> + if (irsd200_hp_filter_freq[idx] == val2)
> >> + break;
> >> + }
> >> +
> >> + if (idx == ARRAY_SIZE(irsd200_hp_filter_freq) || val != 0)
> >> + return -ERANGE;
> >> +
> >> + ret = regmap_field_write(data->regfields[IRS_REGF_HP_FILTER], idx);
> >> + if (ret < 0) {
> >
> > For regmap calls, they are (I think) all documented as returning 0 for success
> > and negative otherwise. That lets you both check the simpler if (ret) and ...
>
> Maybe it _is_ too defensive here. However, I have encountered APIs where
> positive values can/has been returned (due to debug/testing or just plain
> slip ups), without any actual errors.
>
> However, let's trust `regmap` then. I can change to only look for `if
> (ret)` everywhere.
Indeed there are interfaces where you can get positive values.
Regmap docs specifically say they won't though so we can rely on it.
>
> >
> >> + dev_err(data->dev, "Could not write hp filter frequency (%d)\n",
> >> + ret);
> >> + return ret;
> >
> > drop this return ret out of the if block here.
> >
> > In general being able to ignore possibility of ret > 0 simplifies handling.
>
> I try to be consistent and it also "helps" the next person potentially
> adding code after the `if`-statement and forgetting about adding
> `return`. We can drop the `return here, but then we should do the same
> in other places with a check just before the last `return` (like
> `irsd200_write_timer()`, `irsd200_read_nr_count()`,
> `irsd200_write_nr_count()` and many more), right?
I don't feel particulartly strongly about this, but there are scripts
that get used to scan for this pattern to simplify the code.
Sure on the other cases. I don't tend to try and label all cases of things
pointed out, just pick on one and rely on the patch author to generalise.
>
> [...]
>
> >> +
> >> +static int irsd200_write_event(struct iio_dev *indio_dev,
> >> + const struct iio_chan_spec *chan,
> >> + enum iio_event_type type,
> >> + enum iio_event_direction dir,
> >> + enum iio_event_info info, int val, int val2)
> >> +{
> >> + struct irsd200_data *data = iio_priv(indio_dev);
> >> +
> >> + switch (info) {
> >> + case IIO_EV_INFO_VALUE:
> >> + return irsd200_write_threshold(data, dir, val);
> >> + case IIO_EV_INFO_TIMEOUT:
> >> + return irsd200_write_timer(data, val, val2);
> >> + case IIO_EV_INFO_PERIOD:
> >> + return irsd200_write_nr_count(data, val);
> >> + case IIO_EV_INFO_LOW_PASS_FILTER_3DB:
> >> + return irsd200_write_lp_filter(data, val);
> >> + case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
> >> + return irsd200_write_hp_filter(data, val, val2);
> >
> > Just to check - filtering is only on the data being used for events? Not
> > on the direct readback of the data? Whilst that happens, it's not all that
> > common - hence the check.
>
> Well spotted! It actually made think here! :)
>
> I do think it applies to the raw data (as well). Comparing that to
> `IIO_CHAN_INFO_SAMP_FREQ` (which also affects the events) we should
> therefore probably move the filtering to `struct iio_chan_spec
> irsd200_channels`, right?
Yes.
> >> +static irqreturn_t irsd200_irq_thread(int irq, void *dev_id)
> >> +{
> >> + struct iio_dev *indio_dev = dev_id;
> >> + struct irsd200_data *data = iio_priv(indio_dev);
> >> + enum iio_event_direction dir;
> >> + unsigned int lower_count;
> >> + unsigned int upper_count;
> >> + unsigned int status = 0;
> >> + unsigned int source = 0;
> >> + unsigned int clear = 0;
> >> + unsigned int count = 0;
> >> + int ret;
> >> +
> >> + ret = regmap_read(data->regmap, IRS_REG_INTR, &source);
> >> + if (ret) {
> >> + dev_err(data->dev, "Could not read interrupt source (%d)\n",
> >> + ret);
> >> + return IRQ_NONE;
> >> + }
> >> +
> >> + ret = regmap_read(data->regmap, IRS_REG_STATUS, &status);
> >> + if (ret) {
> >> + dev_err(data->dev, "Could not acknowledge interrupt (%d)\n",
> >> + ret);
> >> + return IRQ_NONE;
> >> + }
> >> +
> >> + if (status & BIT(IRS_INTR_DATA) && iio_buffer_enabled(indio_dev)) {
> >> + iio_trigger_poll_nested(indio_dev->trig);
> >> + clear |= BIT(IRS_INTR_DATA);
> >> + }
> >> +
> >> + if (status & BIT(IRS_INTR_TIMER) && source & BIT(IRS_INTR_TIMER)) {
> >> + iio_push_event(indio_dev,
> >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> >> + IIO_EV_TYPE_CHANGE,
> >> + IIO_EV_DIR_NONE),
> >
> > As below, I'd like more explanation of what this is.
> > I can't find a datasheet to look it up in.
>
> This is a timer for the detection event window time, i.e. the signal
> should pass the threshold values within this time in order to get an
> interrupt (`IIO_EV_TYPE_THRESH`).
>
> You setup the window time (`IIO_EV_INFO_TIMEOUT`), and when this timer
> has expired, you get this interrupt (and thus `IIO_EV_TYPE_CHANGE`). I
> couldn't find any other more fitting value in `enum iio_event_type`.
I'm not totally following. This is some sort of watchdog? If threshold
not exceeded for N seconds an interrupt occurs?
Change is definitely not indicating that, so not appropriate ABI to use.
Timeout currently has a very specific defined meaning and it's not this
(see the ABI docs - to do with adaptive algorithm jumps - we also have
reset_timeout but that's different again).
This probably needs some new ABI defining. I'm not sure what will work
best though as it's kind of a 'event did not happen' signal if I've understood
it correctly?
>
> >> + iio_get_time_ns(indio_dev));
> >> + clear |= BIT(IRS_INTR_TIMER);
> >> + }
> >> +
> >> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
> >> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
> >> + /*
> >> + * The register value resets to zero after reading. We therefore
> >> + * need to read once and manually extract the lower and upper
> >> + * count register fields.
> >> + */
> >> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
> >> + if (ret)
> >> + dev_err(data->dev, "Could not read count (%d)\n", ret);
> >> +
> >> + upper_count = IRS_UPPER_COUNT(count);
> >> + lower_count = IRS_LOWER_COUNT(count);
> >> +
> >> + /*
> >> + * We only check the OR mode to be able to push events for
> >> + * rising and falling thresholds. AND mode is covered when both
> >> + * upper and lower count is non-zero, and is signaled with
> >> + * IIO_EV_DIR_EITHER.
> >
> > Whey you say AND, you mean that both thresholds have been crossed but also that
> > in that configuration either being crossed would also have gotten us to here?
> > (as opposed to the interrupt only occurring if value is greater than rising threshold
> > and less than falling threshold?)
> >
> > If it's the first then just report two events. Either means we don't know, rather
> > than we know both occurred. We don't document that well though - so something
> > we should improved there. whilst a bit confusing:
> > https://elixir.bootlin.com/linux/v6.4-rc6/source/Documentation/ABI/testing/sysfs-bus-iio#L792
> > talks about this.
> >
> > The bracketed case is more annoying to deal with so I hope you don't mean that.
> > Whilst we've had sensors that support it in hardware for years, I don't think we
> > ever came up with a usecase that really justified describing it.
>
> According to the data sheet (which will hopefully be soon publicly
> available):
>
> OR-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT)
>
> AND-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT) &&
> (UPPER_COUNT != 0) && (LOWER_COUNT != 0)
>
> For example, consider the following situation:
>
> ___
> / \
> -----------------------------3------------------- Upper threshold
> ___ / \
> ______ / \ / \___________ Data signal
> \ / \ /
> -------1------------2---------------------------- Lower threshold
> \__/ \__/
>
> When `NR_COUNT` is set to 3, we will get an OR-interrupt on point "3" in
> the graph above. In this case `UPPER_COUNT = 1` and `LOWER_COUNT = 2`.
>
> When `NR_COUNT` is set to 2, we will get an OR-interrupt on point "2"
> instead. Here `UPPER_COUNT = 0` and `LOWER_COUNT = 2`.
>
Thanks. That is very odd definition of AND. At least OR is close to normal
though the way count is applied is unusual. Most common thing similar to that
is what we use period for in IIO - it's same count here, but it resets once
the condition is no longer true. Here we have a running total...
Getting this into standard ABI or anything approaching it is going to be tricky.
Firstly need a concept similar to period but with out the reset. That will at least
allow us to comprehend the counts part.
Either can then be used for the OR case.
The AND case is a mess so for now I'm stuck. Will think some more on this.
Out of curiosity does the datasheet include why that particular function makes
any sense? Feels like a rough attempt to approximate something they don't have
hardware resources to actually estimate.
Jonathan
On Sun, Jun 25, 2023 at 12:06 +0100 Jonathan Cameron <[email protected]> wrote:
[...]
>> >> +static int irsd200_write_data_rate(struct irsd200_data *data, int val)
>> >> +{
>> >> + size_t idx;
>> >> + int ret;
>> >> +
>> >> + for (idx = 0; idx < ARRAY_SIZE(irsd200_data_rates); ++idx) {
>> >> + if (irsd200_data_rates[idx] == val)
>> >> + break;
>> >> + }
>> >> +
>> >> + if (idx == ARRAY_SIZE(irsd200_data_rates))
>> >> + return -ERANGE;
>> >> +
>> >> + ret = regmap_write(data->regmap, IRS_REG_DATA_RATE, idx);
>> >> + if (ret < 0) {
>> >> + dev_err(data->dev, "Could not write data rate (%d)\n", ret);
>> >> + return ret;
>> >> + }
>> >> +
>> >> + /* Data sheet says the device needs 3 seconds of settling time. */
>> >> + ssleep(3);
>> > You aren't preventing other userspace reads / writes during that time so
>> > this is a light protection at best.
>>
>> Yes, we aren't preventing anything. The hardware actually operates
>> without any "errors" during this period (e.g. buffer data and events
>> keep arriving).
>>
>> This is more of a guarantee that "within 3 s, the new data rate should
>> be in effect". When I think about it, we should maybe just remove this
>> sleep?
> Interesting corner case. I'd keep as you have it but add a little
> more documentation as why. Having this sleep will make it easy for a single
> thread in userspace to get what it expects.
Alright, let's update the comment then.
[...]
>> >> + dev_err(data->dev, "Could not write hp filter frequency (%d)\n",
>> >> + ret);
>> >> + return ret;
>> >
>> > drop this return ret out of the if block here.
>> >
>> > In general being able to ignore possibility of ret > 0 simplifies handling.
>>
>> I try to be consistent and it also "helps" the next person potentially
>> adding code after the `if`-statement and forgetting about adding
>> `return`. We can drop the `return here, but then we should do the same
>> in other places with a check just before the last `return` (like
>> `irsd200_write_timer()`, `irsd200_read_nr_count()`,
>> `irsd200_write_nr_count()` and many more), right?
>
> I don't feel particulartly strongly about this, but there are scripts
> that get used to scan for this pattern to simplify the code.
>
> Sure on the other cases. I don't tend to try and label all cases of things
> pointed out, just pick on one and rely on the patch author to generalise.
I don't have strong opinions on this either. Let's remove the `return`.
[...]
>> >> +static irqreturn_t irsd200_irq_thread(int irq, void *dev_id)
>> >> +{
>> >> + struct iio_dev *indio_dev = dev_id;
>> >> + struct irsd200_data *data = iio_priv(indio_dev);
>> >> + enum iio_event_direction dir;
>> >> + unsigned int lower_count;
>> >> + unsigned int upper_count;
>> >> + unsigned int status = 0;
>> >> + unsigned int source = 0;
>> >> + unsigned int clear = 0;
>> >> + unsigned int count = 0;
>> >> + int ret;
>> >> +
>> >> + ret = regmap_read(data->regmap, IRS_REG_INTR, &source);
>> >> + if (ret) {
>> >> + dev_err(data->dev, "Could not read interrupt source (%d)\n",
>> >> + ret);
>> >> + return IRQ_NONE;
>> >> + }
>> >> +
>> >> + ret = regmap_read(data->regmap, IRS_REG_STATUS, &status);
>> >> + if (ret) {
>> >> + dev_err(data->dev, "Could not acknowledge interrupt (%d)\n",
>> >> + ret);
>> >> + return IRQ_NONE;
>> >> + }
>> >> +
>> >> + if (status & BIT(IRS_INTR_DATA) && iio_buffer_enabled(indio_dev)) {
>> >> + iio_trigger_poll_nested(indio_dev->trig);
>> >> + clear |= BIT(IRS_INTR_DATA);
>> >> + }
>> >> +
>> >> + if (status & BIT(IRS_INTR_TIMER) && source & BIT(IRS_INTR_TIMER)) {
>> >> + iio_push_event(indio_dev,
>> >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
>> >> + IIO_EV_TYPE_CHANGE,
>> >> + IIO_EV_DIR_NONE),
>> >
>> > As below, I'd like more explanation of what this is.
>> > I can't find a datasheet to look it up in.
>>
>> This is a timer for the detection event window time, i.e. the signal
>> should pass the threshold values within this time in order to get an
>> interrupt (`IIO_EV_TYPE_THRESH`).
>>
>> You setup the window time (`IIO_EV_INFO_TIMEOUT`), and when this timer
>> has expired, you get this interrupt (and thus `IIO_EV_TYPE_CHANGE`). I
>> couldn't find any other more fitting value in `enum iio_event_type`.
>
> I'm not totally following. This is some sort of watchdog? If threshold
> not exceeded for N seconds an interrupt occurs?
Yes, exactly.
> Change is definitely not indicating that, so not appropriate ABI to use.
> Timeout currently has a very specific defined meaning and it's not this
> (see the ABI docs - to do with adaptive algorithm jumps - we also have
> reset_timeout but that's different again).
>
> This probably needs some new ABI defining. I'm not sure what will work
> best though as it's kind of a 'event did not happen' signal if I've understood
> it correctly?
Yeah, I'm not sure when this interrupt actually could be useful. Maybe
when you are testing and calibrating the device, it could help to know
that "these particular settings didn't cause the data to pass any
thresholds during the window time"?
The alternative would be to just ignore this interrupt and not signaling
any events for this. I don't think it would deteriorate the
functionality of the device (except the test/calibration situation
described above, which obviously _can_ be resolved in user space).
>> >> + iio_get_time_ns(indio_dev));
>> >> + clear |= BIT(IRS_INTR_TIMER);
>> >> + }
>> >> +
>> >> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
>> >> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
>> >> + /*
>> >> + * The register value resets to zero after reading. We therefore
>> >> + * need to read once and manually extract the lower and upper
>> >> + * count register fields.
>> >> + */
>> >> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
>> >> + if (ret)
>> >> + dev_err(data->dev, "Could not read count (%d)\n", ret);
>> >> +
>> >> + upper_count = IRS_UPPER_COUNT(count);
>> >> + lower_count = IRS_LOWER_COUNT(count);
>> >> +
>> >> + /*
>> >> + * We only check the OR mode to be able to push events for
>> >> + * rising and falling thresholds. AND mode is covered when both
>> >> + * upper and lower count is non-zero, and is signaled with
>> >> + * IIO_EV_DIR_EITHER.
>> >
>> > Whey you say AND, you mean that both thresholds have been crossed but also that
>> > in that configuration either being crossed would also have gotten us to here?
>> > (as opposed to the interrupt only occurring if value is greater than rising threshold
>> > and less than falling threshold?)
>> >
>> > If it's the first then just report two events. Either means we don't know, rather
>> > than we know both occurred. We don't document that well though - so something
>> > we should improved there. whilst a bit confusing:
>> > https://elixir.bootlin.com/linux/v6.4-rc6/source/Documentation/ABI/testing/sysfs-bus-iio#L792
>> > talks about this.
>> >
>> > The bracketed case is more annoying to deal with so I hope you don't mean that.
>> > Whilst we've had sensors that support it in hardware for years, I don't think we
>> > ever came up with a usecase that really justified describing it.
>>
>> According to the data sheet (which will hopefully be soon publicly
>> available):
>>
>> OR-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT)
>>
>> AND-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT) &&
>> (UPPER_COUNT != 0) && (LOWER_COUNT != 0)
>>
>> For example, consider the following situation:
>>
>> ___
>> / \
>> -----------------------------3------------------- Upper threshold
>> ___ / \
>> ______ / \ / \___________ Data signal
>> \ / \ /
>> -------1------------2---------------------------- Lower threshold
>> \__/ \__/
>>
>> When `NR_COUNT` is set to 3, we will get an OR-interrupt on point "3" in
>> the graph above. In this case `UPPER_COUNT = 1` and `LOWER_COUNT = 2`.
>>
>> When `NR_COUNT` is set to 2, we will get an OR-interrupt on point "2"
>> instead. Here `UPPER_COUNT = 0` and `LOWER_COUNT = 2`.
>>
>
> Thanks. That is very odd definition of AND. At least OR is close to normal
> though the way count is applied is unusual. Most common thing similar to that
> is what we use period for in IIO - it's same count here, but it resets once
> the condition is no longer true. Here we have a running total...
>
> Getting this into standard ABI or anything approaching it is going to be tricky.
>
> Firstly need a concept similar to period but with out the reset. That will at least
> allow us to comprehend the counts part.
>
> Either can then be used for the OR case.
Are you saying that the current implementation (with manually checking
the upper and lower counts only with the OR mode) wouldn't "fit" the
current ABI? It does cover the rising and falling directions correctly,
no? Could `IIO_EV_DIR_NONE` instead of `IIO_EV_DIR_EITHER` be used to
signal "both" then?
>
> The AND case is a mess so for now I'm stuck. Will think some more on this.
> Out of curiosity does the datasheet include why that particular function makes
> any sense? Feels like a rough attempt to approximate something they don't have
> hardware resources to actually estimate.
Unfortunately not. I guess there could be an application where you are
only interested if _both_ lower and upper threshold are exceeded. Maybe
in order to minimize small "false positives" movements in front of the
sensor? But as stated in the comments, one can cover this with only the
OR mode (and manually checking the upper and lower count as we do).
On Mon, 19 Jun 2023 12:41:06 +0200
Waqar Hameed <[email protected]> wrote:
> On Sat, Jun 17, 2023 at 13:55 +0100 Jonathan Cameron <[email protected]> wrote:
>
> > On Fri, 16 Jun 2023 17:10:42 +0200
> > Waqar Hameed <[email protected]> wrote:
> >
> >> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
> >> for communication with interrupt support. Add devicetree bindings
> >> requiring the compatible string, I2C slave address (reg) and interrupts.
> >>
> >> Signed-off-by: Waqar Hameed <[email protected]>
> >
> > This device will have some power supplies, so I'd expect those to be both
> > listed and marked as required (maybe some are optional?)
>
> Right, will add that here (and call `devm_regulator_get_enable()` in
> driver's probe).
>
> [...]
>
> >> +required:
> >> + - compatible
> >> + - reg
> >> + - interrupts
> >
> > If it is possible to remove interrupts from requires - and hence have
> > at least a partly functional driver doing basic reading of the sensor
> > then that is usually a good idea. Far too many board designers seem
> > to decide that they don't need to wire up interrupt liness
> > If it's really hard then don't worry too much.
>
> I see. It would be possible, but would also require some work. Let's
> leave it for now then?
Sure - as long as you review the patches when they come in :)
Jonathan
#> >> >> + if (status & BIT(IRS_INTR_TIMER) && source & BIT(IRS_INTR_TIMER)) {
> >> >> + iio_push_event(indio_dev,
> >> >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> >> >> + IIO_EV_TYPE_CHANGE,
> >> >> + IIO_EV_DIR_NONE),
> >> >
> >> > As below, I'd like more explanation of what this is.
> >> > I can't find a datasheet to look it up in.
> >>
> >> This is a timer for the detection event window time, i.e. the signal
> >> should pass the threshold values within this time in order to get an
> >> interrupt (`IIO_EV_TYPE_THRESH`).
> >>
> >> You setup the window time (`IIO_EV_INFO_TIMEOUT`), and when this timer
> >> has expired, you get this interrupt (and thus `IIO_EV_TYPE_CHANGE`). I
> >> couldn't find any other more fitting value in `enum iio_event_type`.
> >
> > I'm not totally following. This is some sort of watchdog? If threshold
> > not exceeded for N seconds an interrupt occurs?
>
> Yes, exactly.
>
> > Change is definitely not indicating that, so not appropriate ABI to use.
> > Timeout currently has a very specific defined meaning and it's not this
> > (see the ABI docs - to do with adaptive algorithm jumps - we also have
> > reset_timeout but that's different again).
> >
> > This probably needs some new ABI defining. I'm not sure what will work
> > best though as it's kind of a 'event did not happen' signal if I've understood
> > it correctly?
>
> Yeah, I'm not sure when this interrupt actually could be useful. Maybe
> when you are testing and calibrating the device, it could help to know
> that "these particular settings didn't cause the data to pass any
> thresholds during the window time"?
>
> The alternative would be to just ignore this interrupt and not signaling
> any events for this. I don't think it would deteriorate the
> functionality of the device (except the test/calibration situation
> described above, which obviously _can_ be resolved in user space).
That's probably best way to move forwards with this. Can revisit later
if it turns out there is an important usecase!
>
> >> >> + iio_get_time_ns(indio_dev));
> >> >> + clear |= BIT(IRS_INTR_TIMER);
> >> >> + }
> >> >> +
> >> >> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
> >> >> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
> >> >> + /*
> >> >> + * The register value resets to zero after reading. We therefore
> >> >> + * need to read once and manually extract the lower and upper
> >> >> + * count register fields.
> >> >> + */
> >> >> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
> >> >> + if (ret)
> >> >> + dev_err(data->dev, "Could not read count (%d)\n", ret);
> >> >> +
> >> >> + upper_count = IRS_UPPER_COUNT(count);
> >> >> + lower_count = IRS_LOWER_COUNT(count);
> >> >> +
> >> >> + /*
> >> >> + * We only check the OR mode to be able to push events for
> >> >> + * rising and falling thresholds. AND mode is covered when both
> >> >> + * upper and lower count is non-zero, and is signaled with
> >> >> + * IIO_EV_DIR_EITHER.
> >> >
> >> > Whey you say AND, you mean that both thresholds have been crossed but also that
> >> > in that configuration either being crossed would also have gotten us to here?
> >> > (as opposed to the interrupt only occurring if value is greater than rising threshold
> >> > and less than falling threshold?)
> >> >
> >> > If it's the first then just report two events. Either means we don't know, rather
> >> > than we know both occurred. We don't document that well though - so something
> >> > we should improved there. whilst a bit confusing:
> >> > https://elixir.bootlin.com/linux/v6.4-rc6/source/Documentation/ABI/testing/sysfs-bus-iio#L792
> >> > talks about this.
> >> >
> >> > The bracketed case is more annoying to deal with so I hope you don't mean that.
> >> > Whilst we've had sensors that support it in hardware for years, I don't think we
> >> > ever came up with a usecase that really justified describing it.
> >>
> >> According to the data sheet (which will hopefully be soon publicly
> >> available):
> >>
> >> OR-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT)
> >>
> >> AND-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT) &&
> >> (UPPER_COUNT != 0) && (LOWER_COUNT != 0)
> >>
> >> For example, consider the following situation:
> >>
> >> ___
> >> / \
> >> -----------------------------3------------------- Upper threshold
> >> ___ / \
> >> ______ / \ / \___________ Data signal
> >> \ / \ /
> >> -------1------------2---------------------------- Lower threshold
> >> \__/ \__/
> >>
> >> When `NR_COUNT` is set to 3, we will get an OR-interrupt on point "3" in
> >> the graph above. In this case `UPPER_COUNT = 1` and `LOWER_COUNT = 2`.
> >>
> >> When `NR_COUNT` is set to 2, we will get an OR-interrupt on point "2"
> >> instead. Here `UPPER_COUNT = 0` and `LOWER_COUNT = 2`.
> >>
> >
> > Thanks. That is very odd definition of AND. At least OR is close to normal
> > though the way count is applied is unusual. Most common thing similar to that
> > is what we use period for in IIO - it's same count here, but it resets once
> > the condition is no longer true. Here we have a running total...
> >
> > Getting this into standard ABI or anything approaching it is going to be tricky.
> >
> > Firstly need a concept similar to period but with out the reset. That will at least
> > allow us to comprehend the counts part.
> >
> > Either can then be used for the OR case.
>
> Are you saying that the current implementation (with manually checking
> the upper and lower counts only with the OR mode) wouldn't "fit" the
> current ABI? It does cover the rising and falling directions correctly,
> no? Could `IIO_EV_DIR_NONE` instead of `IIO_EV_DIR_EITHER` be used to
> signal "both" then?
The fact it's a running count (so doesn't go back to 0 when threshold is
not exceeded) is the unusual bit, not the direction.
No on none. That's for channels where there is not concept of direction.
Either is fine, but we still need to deal with the temporal element being
different from period. For that I think we need some new ABI, but
not sure exactly what it should be.
XXX_runningperiod maybe? Still measured in seconds, but not resetting
unlike _period...
>
> >
> > The AND case is a mess so for now I'm stuck. Will think some more on this.
> > Out of curiosity does the datasheet include why that particular function makes
> > any sense? Feels like a rough attempt to approximate something they don't have
> > hardware resources to actually estimate.
>
> Unfortunately not. I guess there could be an application where you are
> only interested if _both_ lower and upper threshold are exceeded. Maybe
> in order to minimize small "false positives" movements in front of the
> sensor? But as stated in the comments, one can cover this with only the
> OR mode (and manually checking the upper and lower count as we do).
On Sun, Jul 02, 2023 at 18:42 +0800 Jonathan Cameron <[email protected]> wrote:
> #> >> >> + if (status & BIT(IRS_INTR_TIMER) && source & BIT(IRS_INTR_TIMER)) {
>> >> >> + iio_push_event(indio_dev,
>> >> >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
>> >> >> + IIO_EV_TYPE_CHANGE,
>> >> >> + IIO_EV_DIR_NONE),
>> >> >
>> >> > As below, I'd like more explanation of what this is.
>> >> > I can't find a datasheet to look it up in.
>> >>
>> >> This is a timer for the detection event window time, i.e. the signal
>> >> should pass the threshold values within this time in order to get an
>> >> interrupt (`IIO_EV_TYPE_THRESH`).
>> >>
>> >> You setup the window time (`IIO_EV_INFO_TIMEOUT`), and when this timer
>> >> has expired, you get this interrupt (and thus `IIO_EV_TYPE_CHANGE`). I
>> >> couldn't find any other more fitting value in `enum iio_event_type`.
>> >
>> > I'm not totally following. This is some sort of watchdog? If threshold
>> > not exceeded for N seconds an interrupt occurs?
>>
>> Yes, exactly.
>>
>> > Change is definitely not indicating that, so not appropriate ABI to use.
>> > Timeout currently has a very specific defined meaning and it's not this
>> > (see the ABI docs - to do with adaptive algorithm jumps - we also have
>> > reset_timeout but that's different again).
>> >
>> > This probably needs some new ABI defining. I'm not sure what will work
>> > best though as it's kind of a 'event did not happen' signal if I've understood
>> > it correctly?
>>
>> Yeah, I'm not sure when this interrupt actually could be useful. Maybe
>> when you are testing and calibrating the device, it could help to know
>> that "these particular settings didn't cause the data to pass any
>> thresholds during the window time"?
>>
>> The alternative would be to just ignore this interrupt and not signaling
>> any events for this. I don't think it would deteriorate the
>> functionality of the device (except the test/calibration situation
>> described above, which obviously _can_ be resolved in user space).
>
> That's probably best way to move forwards with this. Can revisit later
> if it turns out there is an important usecase!
Alright, let's skip this interrupt. However, we still need a way for
users to specify the window time (see answer below).
>> >> >> + iio_get_time_ns(indio_dev));
>> >> >> + clear |= BIT(IRS_INTR_TIMER);
>> >> >> + }
>> >> >> +
>> >> >> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
>> >> >> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
>> >> >> + /*
>> >> >> + * The register value resets to zero after reading. We therefore
>> >> >> + * need to read once and manually extract the lower and upper
>> >> >> + * count register fields.
>> >> >> + */
>> >> >> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
>> >> >> + if (ret)
>> >> >> + dev_err(data->dev, "Could not read count (%d)\n", ret);
>> >> >> +
>> >> >> + upper_count = IRS_UPPER_COUNT(count);
>> >> >> + lower_count = IRS_LOWER_COUNT(count);
>> >> >> +
>> >> >> + /*
>> >> >> + * We only check the OR mode to be able to push events for
>> >> >> + * rising and falling thresholds. AND mode is covered when both
>> >> >> + * upper and lower count is non-zero, and is signaled with
>> >> >> + * IIO_EV_DIR_EITHER.
>> >> >
>> >> > Whey you say AND, you mean that both thresholds have been crossed but also that
>> >> > in that configuration either being crossed would also have gotten us to here?
>> >> > (as opposed to the interrupt only occurring if value is greater than rising threshold
>> >> > and less than falling threshold?)
>> >> >
>> >> > If it's the first then just report two events. Either means we don't know, rather
>> >> > than we know both occurred. We don't document that well though - so something
>> >> > we should improved there. whilst a bit confusing:
>> >> > https://elixir.bootlin.com/linux/v6.4-rc6/source/Documentation/ABI/testing/sysfs-bus-iio#L792
>> >> > talks about this.
>> >> >
>> >> > The bracketed case is more annoying to deal with so I hope you don't mean that.
>> >> > Whilst we've had sensors that support it in hardware for years, I don't think we
>> >> > ever came up with a usecase that really justified describing it.
>> >>
>> >> According to the data sheet (which will hopefully be soon publicly
>> >> available):
>> >>
>> >> OR-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT)
>> >>
>> >> AND-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT) &&
>> >> (UPPER_COUNT != 0) && (LOWER_COUNT != 0)
>> >>
>> >> For example, consider the following situation:
>> >>
>> >> ___
>> >> / \
>> >> -----------------------------3------------------- Upper threshold
>> >> ___ / \
>> >> ______ / \ / \___________ Data signal
>> >> \ / \ /
>> >> -------1------------2---------------------------- Lower threshold
>> >> \__/ \__/
>> >>
>> >> When `NR_COUNT` is set to 3, we will get an OR-interrupt on point "3" in
>> >> the graph above. In this case `UPPER_COUNT = 1` and `LOWER_COUNT = 2`.
>> >>
>> >> When `NR_COUNT` is set to 2, we will get an OR-interrupt on point "2"
>> >> instead. Here `UPPER_COUNT = 0` and `LOWER_COUNT = 2`.
>> >>
>> >
>> > Thanks. That is very odd definition of AND. At least OR is close to normal
>> > though the way count is applied is unusual. Most common thing similar to that
>> > is what we use period for in IIO - it's same count here, but it resets once
>> > the condition is no longer true. Here we have a running total...
>> >
>> > Getting this into standard ABI or anything approaching it is going to be tricky.
>> >
>> > Firstly need a concept similar to period but with out the reset. That will at least
>> > allow us to comprehend the counts part.
>> >
>> > Either can then be used for the OR case.
>>
>> Are you saying that the current implementation (with manually checking
>> the upper and lower counts only with the OR mode) wouldn't "fit" the
>> current ABI? It does cover the rising and falling directions correctly,
>> no? Could `IIO_EV_DIR_NONE` instead of `IIO_EV_DIR_EITHER` be used to
>> signal "both" then?
>
> The fact it's a running count (so doesn't go back to 0 when threshold is
> not exceeded) is the unusual bit, not the direction.
>
> No on none. That's for channels where there is not concept of direction.
> Either is fine, but we still need to deal with the temporal element being
> different from period. For that I think we need some new ABI, but
> not sure exactly what it should be.
>
> XXX_runningperiod maybe? Still measured in seconds, but not resetting
> unlike _period...
>
>> >
>> > The AND case is a mess so for now I'm stuck. Will think some more on this.
>> > Out of curiosity does the datasheet include why that particular function makes
>> > any sense? Feels like a rough attempt to approximate something they don't have
>> > hardware resources to actually estimate.
>>
>> Unfortunately not. I guess there could be an application where you are
>> only interested if _both_ lower and upper threshold are exceeded. Maybe
>> in order to minimize small "false positives" movements in front of the
>> sensor? But as stated in the comments, one can cover this with only the
>> OR mode (and manually checking the upper and lower count as we do).
Hm, I see. The "cleanest" way is probably to add some new ABIs.
Let's say we add `IIO_EV_INFO_RUNNING_PERIOD` (or something) to be able
to specify the time (in seconds) for the threshold window time
(`irsd200_read/write_timer()`), e.g. `*_thresh_runningperiod`. We then
also need an ABI for specifying the number of threshold counts in this
running period (`irsd200_read/write_nr_count()`, i.e. `NR_COUNT` from
the graph above), e.g. `*_thresh_runningcount` (or something).
This would cover the OR mode. As stated before, the AND mode is much
more complicated, and should maybe considered later (when someone
actually has a use case)? We can signal with the direction to tell if
both thresholds has been passed (either), compared to only exceeding one
of them (rising or falling) in the running period.
On Mon, 3 Jul 2023 10:59:41 +0200
Waqar Hameed <[email protected]> wrote:
> On Sun, Jul 02, 2023 at 18:42 +0800 Jonathan Cameron <[email protected]> wrote:
>
> [...]
> >> >> >
> >> >> > As below, I'd like more explanation of what this is.
> >> >> > I can't find a datasheet to look it up in.
> >> >>
> >> >> This is a timer for the detection event window time, i.e. the signal
> >> >> should pass the threshold values within this time in order to get an
> >> >> interrupt (`IIO_EV_TYPE_THRESH`).
> >> >>
> >> >> You setup the window time (`IIO_EV_INFO_TIMEOUT`), and when this timer
> >> >> has expired, you get this interrupt (and thus `IIO_EV_TYPE_CHANGE`). I
> >> >> couldn't find any other more fitting value in `enum iio_event_type`.
> >> >
> >> > I'm not totally following. This is some sort of watchdog? If threshold
> >> > not exceeded for N seconds an interrupt occurs?
> >>
> >> Yes, exactly.
> >>
> >> > Change is definitely not indicating that, so not appropriate ABI to use.
> >> > Timeout currently has a very specific defined meaning and it's not this
> >> > (see the ABI docs - to do with adaptive algorithm jumps - we also have
> >> > reset_timeout but that's different again).
> >> >
> >> > This probably needs some new ABI defining. I'm not sure what will work
> >> > best though as it's kind of a 'event did not happen' signal if I've understood
> >> > it correctly?
> >>
> >> Yeah, I'm not sure when this interrupt actually could be useful. Maybe
> >> when you are testing and calibrating the device, it could help to know
> >> that "these particular settings didn't cause the data to pass any
> >> thresholds during the window time"?
> >>
> >> The alternative would be to just ignore this interrupt and not signaling
> >> any events for this. I don't think it would deteriorate the
> >> functionality of the device (except the test/calibration situation
> >> described above, which obviously _can_ be resolved in user space).
> >
> > That's probably best way to move forwards with this. Can revisit later
> > if it turns out there is an important usecase!
>
> Alright, let's skip this interrupt. However, we still need a way for
> users to specify the window time (see answer below).
>
> >> >> >> + iio_get_time_ns(indio_dev));
> >> >> >> + clear |= BIT(IRS_INTR_TIMER);
> >> >> >> + }
> >> >> >> +
> >> >> >> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
> >> >> >> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
> >> >> >> + /*
> >> >> >> + * The register value resets to zero after reading. We therefore
> >> >> >> + * need to read once and manually extract the lower and upper
> >> >> >> + * count register fields.
> >> >> >> + */
> >> >> >> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
> >> >> >> + if (ret)
> >> >> >> + dev_err(data->dev, "Could not read count (%d)\n", ret);
> >> >> >> +
> >> >> >> + upper_count = IRS_UPPER_COUNT(count);
> >> >> >> + lower_count = IRS_LOWER_COUNT(count);
> >> >> >> +
> >> >> >> + /*
> >> >> >> + * We only check the OR mode to be able to push events for
> >> >> >> + * rising and falling thresholds. AND mode is covered when both
> >> >> >> + * upper and lower count is non-zero, and is signaled with
> >> >> >> + * IIO_EV_DIR_EITHER.
> >> >> >
> >> >> > Whey you say AND, you mean that both thresholds have been crossed but also that
> >> >> > in that configuration either being crossed would also have gotten us to here?
> >> >> > (as opposed to the interrupt only occurring if value is greater than rising threshold
> >> >> > and less than falling threshold?)
> >> >> >
> >> >> > If it's the first then just report two events. Either means we don't know, rather
> >> >> > than we know both occurred. We don't document that well though - so something
> >> >> > we should improved there. whilst a bit confusing:
> >> >> > https://elixir.bootlin.com/linux/v6.4-rc6/source/Documentation/ABI/testing/sysfs-bus-iio#L792
> >> >> > talks about this.
> >> >> >
> >> >> > The bracketed case is more annoying to deal with so I hope you don't mean that.
> >> >> > Whilst we've had sensors that support it in hardware for years, I don't think we
> >> >> > ever came up with a usecase that really justified describing it.
> >> >>
> >> >> According to the data sheet (which will hopefully be soon publicly
> >> >> available):
> >> >>
> >> >> OR-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT)
> >> >>
> >> >> AND-interrupt: (UPPER_COUNT + LOWER_COUNT >= NR_COUNT) &&
> >> >> (UPPER_COUNT != 0) && (LOWER_COUNT != 0)
> >> >>
> >> >> For example, consider the following situation:
> >> >>
> >> >> ___
> >> >> / \
> >> >> -----------------------------3------------------- Upper threshold
> >> >> ___ / \
> >> >> ______ / \ / \___________ Data signal
> >> >> \ / \ /
> >> >> -------1------------2---------------------------- Lower threshold
> >> >> \__/ \__/
> >> >>
> >> >> When `NR_COUNT` is set to 3, we will get an OR-interrupt on point "3" in
> >> >> the graph above. In this case `UPPER_COUNT = 1` and `LOWER_COUNT = 2`.
> >> >>
> >> >> When `NR_COUNT` is set to 2, we will get an OR-interrupt on point "2"
> >> >> instead. Here `UPPER_COUNT = 0` and `LOWER_COUNT = 2`.
> >> >>
> >> >
> >> > Thanks. That is very odd definition of AND. At least OR is close to normal
> >> > though the way count is applied is unusual. Most common thing similar to that
> >> > is what we use period for in IIO - it's same count here, but it resets once
> >> > the condition is no longer true. Here we have a running total...
> >> >
> >> > Getting this into standard ABI or anything approaching it is going to be tricky.
> >> >
> >> > Firstly need a concept similar to period but with out the reset. That will at least
> >> > allow us to comprehend the counts part.
> >> >
> >> > Either can then be used for the OR case.
> >>
> >> Are you saying that the current implementation (with manually checking
> >> the upper and lower counts only with the OR mode) wouldn't "fit" the
> >> current ABI? It does cover the rising and falling directions correctly,
> >> no? Could `IIO_EV_DIR_NONE` instead of `IIO_EV_DIR_EITHER` be used to
> >> signal "both" then?
> >
> > The fact it's a running count (so doesn't go back to 0 when threshold is
> > not exceeded) is the unusual bit, not the direction.
> >
> > No on none. That's for channels where there is not concept of direction.
> > Either is fine, but we still need to deal with the temporal element being
> > different from period. For that I think we need some new ABI, but
> > not sure exactly what it should be.
> >
> > XXX_runningperiod maybe? Still measured in seconds, but not resetting
> > unlike _period...
> >
> >> >
> >> > The AND case is a mess so for now I'm stuck. Will think some more on this.
> >> > Out of curiosity does the datasheet include why that particular function makes
> >> > any sense? Feels like a rough attempt to approximate something they don't have
> >> > hardware resources to actually estimate.
> >>
> >> Unfortunately not. I guess there could be an application where you are
> >> only interested if _both_ lower and upper threshold are exceeded. Maybe
> >> in order to minimize small "false positives" movements in front of the
> >> sensor? But as stated in the comments, one can cover this with only the
> >> OR mode (and manually checking the upper and lower count as we do).
>
> Hm, I see. The "cleanest" way is probably to add some new ABIs.
>
> Let's say we add `IIO_EV_INFO_RUNNING_PERIOD` (or something) to be able
> to specify the time (in seconds) for the threshold window time
> (`irsd200_read/write_timer()`), e.g. `*_thresh_runningperiod`. We then
> also need an ABI for specifying the number of threshold counts in this
> running period (`irsd200_read/write_nr_count()`, i.e. `NR_COUNT` from
> the graph above), e.g. `*_thresh_runningcount` (or something).
Agreed. The name may need some refinement, but this seems a good place
to start.
>
> This would cover the OR mode. As stated before, the AND mode is much
> more complicated, and should maybe considered later (when someone
> actually has a use case)? We can signal with the direction to tell if
> both thresholds has been passed (either), compared to only exceeding one
> of them (rising or falling) in the running period.
Using either to define that both things happened would be non intuitive.
Let's resolve that question if / when it matters.
Thanks,
Jonathan
On Sun, Jun 25, 2023 at 12:06 +0100 Jonathan Cameron <[email protected]> wrote:
[...]
>> >> + dev_err(data->dev, "Could not write hp filter frequency (%d)\n",
>> >> + ret);
>> >> + return ret;
>> >
>> > drop this return ret out of the if block here.
>> >
>> > In general being able to ignore possibility of ret > 0 simplifies handling.
>>
>> I try to be consistent and it also "helps" the next person potentially
>> adding code after the `if`-statement and forgetting about adding
>> `return`. We can drop the `return here, but then we should do the same
>> in other places with a check just before the last `return` (like
>> `irsd200_write_timer()`, `irsd200_read_nr_count()`,
>> `irsd200_write_nr_count()` and many more), right?
>
> I don't feel particulartly strongly about this, but there are scripts
> that get used to scan for this pattern to simplify the code.
>
> Sure on the other cases. I don't tend to try and label all cases of things
> pointed out, just pick on one and rely on the patch author to generalise.
I started to remove the returns but then realized that it got a little
messy. For example, in some cases we can't drop the return (side effects
after the return etc.).
Since you didn't have any strong opinions on this, I kept them in v2.
Hope that's fine!
On Wed, 12 Jul 2023 17:33:09 +0200
Waqar Hameed <[email protected]> wrote:
> On Sun, Jun 25, 2023 at 12:06 +0100 Jonathan Cameron <[email protected]> wrote:
>
> [...]
>
> >> >> + dev_err(data->dev, "Could not write hp filter frequency (%d)\n",
> >> >> + ret);
> >> >> + return ret;
> >> >
> >> > drop this return ret out of the if block here.
> >> >
> >> > In general being able to ignore possibility of ret > 0 simplifies handling.
> >>
> >> I try to be consistent and it also "helps" the next person potentially
> >> adding code after the `if`-statement and forgetting about adding
> >> `return`. We can drop the `return here, but then we should do the same
> >> in other places with a check just before the last `return` (like
> >> `irsd200_write_timer()`, `irsd200_read_nr_count()`,
> >> `irsd200_write_nr_count()` and many more), right?
> >
> > I don't feel particulartly strongly about this, but there are scripts
> > that get used to scan for this pattern to simplify the code.
> >
> > Sure on the other cases. I don't tend to try and label all cases of things
> > pointed out, just pick on one and rely on the patch author to generalise.
>
> I started to remove the returns but then realized that it got a little
> messy. For example, in some cases we can't drop the return (side effects
> after the return etc.).
>
> Since you didn't have any strong opinions on this, I kept them in v2.
> Hope that's fine!
Absolutely. I wasn't advocating removing separate returns in general, just this
cases where there was nothing after the if check.
Thanks,
Jonathan