Add driver for Murata IRS-D200

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/Makefile +1 -0

index cc838bb5408a..f36598380446 100644
@@ -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

drivers/iio/proximity/irsd200.c new file +0 -0

new file mode 100644
index 000000000000..699801d60295

[...]

drivers/iio/proximity/irsd200.c new file +8 -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);

drivers/iio/proximity/irsd200.c new file +5 -0

+	*val |= tmpval;
+
+	return 0;
+}
[...]
+static int irsd200_write_raw(struct iio_dev *indio_dev,

drivers/iio/proximity/irsd200.c new file +7 -0

+	default:
+		return -EINVAL;
+	}
+}
+
 
[...]
+static int irsd200_probe(struct i2c_client *client)
+{

drivers/iio/proximity/irsd200.c new file +24 -0

+		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]);
+		}
+	}
+
[...]

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

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml new file +2 -0

new file mode 100644
index 000000000000..d317fbe7bd50
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml new file +26 -0

+  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

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

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml new file +26 -0

new file mode 100644
index 000000000000..d317fbe7bd50
@@ -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.

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml new file +18 -0

+
+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>;
+        };
+    };
+...

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/irsd200.c new file +43 -0

new file mode 100644
index 000000000000..699801d60295
@@ -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. */
+

drivers/iio/proximity/irsd200.c new file +34 -0

+	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;
+	}

drivers/iio/proximity/irsd200.c new file +18 -0

+
+	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))

drivers/iio/proximity/irsd200.c new file +14 -0

+
+	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;

drivers/iio/proximity/irsd200.c new file +11 -0

+
+	regval |= tmpval;
+
+	ret = irsd200_read_data_rate(data, val2);
+	if (ret < 0)
+		return ret;
+
+	*val = regval;
+
+	return 0;
+}

drivers/iio/proximity/irsd200.c new file +9 -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;
+

drivers/iio/proximity/irsd200.c new file +73 -0

+	}
+
+	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) {

drivers/iio/proximity/irsd200.c new file +44 -0

+	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;

drivers/iio/proximity/irsd200.c new file +53 -0

+
+		/* 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;
+	}
+

drivers/iio/proximity/irsd200.c new file +13 -0

+			       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)

drivers/iio/proximity/irsd200.c new file +50 -0

+		 */
+		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);

drivers/iio/proximity/irsd200.c new file +33 -0

+	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 = {

drivers/iio/proximity/irsd200.c new file +9 -0

+	{
+		.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,

drivers/iio/proximity/irsd200.c new file +29 -0

+	{
+		.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);
+	}
+

drivers/iio/proximity/irsd200.c new file +33 -0

+
+	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;

drivers/iio/proximity/irsd200.c new file +14 -0

+
+	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) {

drivers/iio/proximity/irsd200.c new file +4 -0

+	}
+
+	return 0;
+}

Quoted 3 lines · Waqar Hameed

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.

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml line 12

Quoted 4 lines

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml new file +4 -0

+maintainers:
+  - Waqar Hameed <[email protected]>
+
+description: |

Nit, do not need '|' unless you need to preserve formatting.

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml line 48

Quoted 9 lines

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml new file +9 -0

+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@?

Quoted 5 lines · 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?)

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml line 36

Quoted 4 lines

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml new file +4 -0

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

Quoted 5 lines

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"?

Quoted 8 lines · Waqar Hameed

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.

Quoted 7 lines

index 000000000000..699801d60295

drivers/iio/proximity/irsd200.c new file +2 -0

@@ -0,0 +1,1051 @@
[...]
+/*
+ * The upper 4 bits in register IRS_REG_COUNT value is the upper count value

drivers/iio/proximity/irsd200.c line 56

Quoted 5 lines

+ * (exceeding upper threshold value). The lower 4 is the lower count value
+ * (exceeding lower threshold value).
+ */

drivers/iio/proximity/irsd200.c new file +2 -0

+#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.

Quoted 14 lines

[...]

drivers/iio/proximity/irsd200.c new file +12 -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);

drivers/iio/proximity/irsd200.c line 250

Quoted 6 lines

drivers/iio/proximity/irsd200.c new file +6 -0

+
+	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?

Quoted 9 lines

drivers/iio/proximity/irsd200.c new file +8 -0

+	*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)
+{

drivers/iio/proximity/irsd200.c line 565

Quoted 7 lines

drivers/iio/proximity/irsd200.c new file +7 -0

+	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(...)

Quoted 6 lines

drivers/iio/proximity/irsd200.c new file +5 -0

+	default:
+		return -EINVAL;
+	}
+}
+

drivers/iio/proximity/irsd200.c line 946

Quoted 13 lines

[...]

drivers/iio/proximity/irsd200.c new file +12 -0

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

Quoted 7 lines · 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.

Quoted 1 line

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.

Quoted 54 lines

drivers/iio/proximity/irsd200.c new file +48 -0

new file mode 100644
index 000000000000..699801d60295
@@ -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

drivers/iio/proximity/irsd200.c line 53

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

+
+/*
+ * 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.

drivers/iio/proximity/irsd200.c line 56

Quoted 3 lines

drivers/iio/proximity/irsd200.c new file +3 -0

+ */
+#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.

drivers/iio/proximity/irsd200.c line 75

Quoted 6 lines

drivers/iio/proximity/irsd200.c new file +6 -0

+/*
+ * 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.

Quoted 3 lines

drivers/iio/proximity/irsd200.c new file +3 -0

+	3,
+	5,
+};

drivers/iio/proximity/irsd200.c line 131

Quoted 7 lines

drivers/iio/proximity/irsd200.c new file +7 -0

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

drivers/iio/proximity/irsd200.c line 140

Quoted 9 lines

drivers/iio/proximity/irsd200.c new file +9 -0

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

Quoted 39 lines

drivers/iio/proximity/irsd200.c new file +38 -0

+	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);

drivers/iio/proximity/irsd200.c line 252

Quoted 6 lines

drivers/iio/proximity/irsd200.c new file +6 -0

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

drivers/iio/proximity/irsd200.c

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

+
+	return 0;
+}
+
+stan 0;

Quoted 17 lines

drivers/iio/proximity/irsd200.c new file +17 -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) {

drivers/iio/proximity/irsd200.c line 296

Quoted 6 lines

drivers/iio/proximity/irsd200.c new file +6 -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.

Quoted 14 lines

drivers/iio/proximity/irsd200.c new file +14 -0

+
+	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;

drivers/iio/proximity/irsd200.c line 320

Quoted 10 lines

drivers/iio/proximity/irsd200.c new file +10 -0

+	}
+
+	/* 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.

Quoted 13 lines

drivers/iio/proximity/irsd200.c new file +13 -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;
+	}
+

drivers/iio/proximity/irsd200.c line 503

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

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

drivers/iio/proximity/irsd200.c line 506

Quoted 4 lines

drivers/iio/proximity/irsd200.c new file +3 -0

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

Quoted 16 lines

drivers/iio/proximity/irsd200.c new file +16 -0

+
+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);

drivers/iio/proximity/irsd200.c line 634

Quoted 4 lines

drivers/iio/proximity/irsd200.c new file +4 -0

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

Quoted 12 lines

drivers/iio/proximity/irsd200.c new file +12 -0

+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:
+		/*

drivers/iio/proximity/irsd200.c line 688

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

+		 * 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.

Quoted 58 lines

drivers/iio/proximity/irsd200.c new file +58 -0

+
+		/* 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);
+	}
+

drivers/iio/proximity/irsd200.c line 751

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

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

Quoted 13 lines

drivers/iio/proximity/irsd200.c new file +13 -0

+			       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)

drivers/iio/proximity/irsd200.c line 774

Quoted 10 lines

drivers/iio/proximity/irsd200.c new file +10 -0

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

Quoted 58 lines

drivers/iio/proximity/irsd200.c new file +57 -0

+		 */
+		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);

drivers/iio/proximity/irsd200.c line 834

Quoted 3 lines

drivers/iio/proximity/irsd200.c new file +3 -0

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

Quoted 24 lines

drivers/iio/proximity/irsd200.c new file +22 -0

+	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;
+}
+

drivers/iio/proximity/irsd200.c line 877

Quoted 2 lines

drivers/iio/proximity/irsd200.c new file +2 -0

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

Quoted 13 lines

drivers/iio/proximity/irsd200.c new file +13 -0

+	{
+		.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),
+	},

drivers/iio/proximity/irsd200.c line 896

Quoted 6 lines

drivers/iio/proximity/irsd200.c new file +6 -0

+	/* 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.

drivers/iio/proximity/irsd200.c line 904

Quoted 9 lines

drivers/iio/proximity/irsd200.c new file +8 -0

+	},
+	/* 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.

Quoted 36 lines

drivers/iio/proximity/irsd200.c new file +34 -0

+	{
+		.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;
+	}

drivers/iio/proximity/irsd200.c line 959

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

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

Quoted 40 lines

drivers/iio/proximity/irsd200.c new file +39 -0

+
+	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);

drivers/iio/proximity/irsd200.c line 1002

Quoted 4 lines

drivers/iio/proximity/irsd200.c new file +4 -0

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

Quoted 21 lines

drivers/iio/proximity/irsd200.c new file +21 -0

+
+	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) {

drivers/iio/proximity/irsd200.c line 1026

Quoted 3 lines

drivers/iio/proximity/irsd200.c new file +3 -0

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

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +4 -0

+	}
+
+	return 0;
+}

Quoted 5 lines

drivers/iio/proximity/irsd200.c

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

+
+	return 0;
+}
+
+stan 0;  

(What happened here?)

Quoted 17 lines

Quoted 17 lines

drivers/iio/proximity/irsd200.c new file +17 -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) {

Quoted 8 lines

drivers/iio/proximity/irsd200.c line 296

Quoted 6 lines

drivers/iio/proximity/irsd200.c new file +6 -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?

Quoted 4 lines

Quoted 4 lines

drivers/iio/proximity/irsd200.c new file +3 -0

+
+	return 0;
+}

Quoted 13 lines

Quoted 13 lines

drivers/iio/proximity/irsd200.c new file +13 -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;
+	}
+

Quoted 8 lines

drivers/iio/proximity/irsd200.c line 503

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

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

Quoted 8 lines

drivers/iio/proximity/irsd200.c line 506

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +3 -0

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

Quoted 1 line

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?

[...]

Quoted 16 lines

Quoted 16 lines

drivers/iio/proximity/irsd200.c new file +16 -0

+
+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);

Quoted 8 lines

drivers/iio/proximity/irsd200.c line 634

Quoted 4 lines

drivers/iio/proximity/irsd200.c new file +4 -0

+	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?

Quoted 32 lines

Quoted 32 lines

drivers/iio/proximity/irsd200.c new file +32 -0

+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);
+	}
+

Quoted 8 lines

drivers/iio/proximity/irsd200.c line 751

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

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

Quoted 36 lines

Quoted 14 lines

drivers/iio/proximity/irsd200.c new file +13 -0

+			       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)

drivers/iio/proximity/irsd200.c line 774

Quoted 10 lines

drivers/iio/proximity/irsd200.c new file +10 -0

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

Quoted 3 lines

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.

Quoted 28 lines

Quoted 15 lines

Quoted 15 lines

drivers/iio/proximity/irsd200.c new file +15 -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) {

Quoted 8 lines

drivers/iio/proximity/irsd200.c line 296

Quoted 6 lines

drivers/iio/proximity/irsd200.c new file +6 -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).

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

drivers/iio/proximity/irsd200.c line 506

Quoted 3 lines

drivers/iio/proximity/irsd200.c new file +3 -0

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

Quoted 8 lines

Quoted 1 line

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.

Quoted 45 lines

Quoted 32 lines

Quoted 32 lines

drivers/iio/proximity/irsd200.c new file +32 -0

+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);
+	}
+

Quoted 8 lines

drivers/iio/proximity/irsd200.c line 751

Quoted 5 lines

drivers/iio/proximity/irsd200.c new file +5 -0

+	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).

Quoted 3 lines

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?

Quoted 58 lines

Quoted 35 lines

Quoted 13 lines

drivers/iio/proximity/irsd200.c new file +13 -0

+			       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)

drivers/iio/proximity/irsd200.c line 774

Quoted 10 lines

drivers/iio/proximity/irsd200.c new file +10 -0

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

Quoted 3 lines

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
        \__/         \__/

Quoted 6 lines

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.

Quoted 11 lines · On Sat, Jun 17, 2023 at 13:55 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 5 lines · 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).

[...]

Quoted 5 lines

Quoted 4 lines

Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml new file +4 -0

+required:
+  - compatible
+  - reg
+  - interrupts  

Quoted 5 lines

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?

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

drivers/iio/proximity/irsd200.c line 751

Quoted 4 lines

drivers/iio/proximity/irsd200.c new file +4 -0

+		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).

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

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

Quoted 76 lines

Quoted 59 lines

Quoted 36 lines

Quoted 14 lines

drivers/iio/proximity/irsd200.c new file +13 -0

+			       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)

drivers/iio/proximity/irsd200.c line 774

Quoted 10 lines

drivers/iio/proximity/irsd200.c new file +10 -0

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

Quoted 3 lines

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
        \__/         \__/

Quoted 6 lines

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.

Quoted 1 line

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?

Quoted 6 lines

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

#> >> >> + if (status & BIT(IRS_INTR_TIMER) && source & BIT(IRS_INTR_TIMER)) {

Quoted 35 lines

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

drivers/iio/proximity/irsd200.c line 751

Quoted 4 lines

drivers/iio/proximity/irsd200.c new file +4 -0

+		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).

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

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"?

Quoted 4 lines

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!

Quoted 75 lines

Quoted 75 lines

Quoted 58 lines

Quoted 35 lines

Quoted 13 lines

drivers/iio/proximity/irsd200.c new file +13 -0

+			       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)

drivers/iio/proximity/irsd200.c line 774

Quoted 10 lines

drivers/iio/proximity/irsd200.c new file +10 -0

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

Quoted 3 lines

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
        \__/         \__/

Quoted 6 lines

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.

Quoted 7 lines

Quoted 1 line

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

Quoted 11 lines

Quoted 5 lines

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:

[...]

Quoted 31 lines

Quoted 31 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

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"?

Quoted 7 lines

Quoted 4 lines

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

Quoted 86 lines

Quoted 75 lines

Quoted 75 lines

Quoted 58 lines

Quoted 35 lines

Quoted 13 lines

(quoted diff) +13 -0

+			       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)

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 3 lines

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
        \__/         \__/

Quoted 6 lines

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.

Quoted 7 lines

Quoted 1 line

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.

Quoted 19 lines

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

Quoted 11 lines

Quoted 5 lines

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.

Quoted 6 lines

Quoted 6 lines

drivers/iio/proximity/irsd200.c line 506

Quoted 3 lines

drivers/iio/proximity/irsd200.c new file +3 -0

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

Quoted 8 lines

Quoted 1 line

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.

On Sun, Jun 25, 2023 at 12:06 +0100 Jonathan Cameron <[email protected]> wrote:

[...]

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

drivers/iio/proximity/irsd200.c line 506

Quoted 3 lines

drivers/iio/proximity/irsd200.c new file +3 -0

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

Quoted 8 lines

Quoted 1 line

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?

Quoted 5 lines

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!