[PATCH v2 0/3] Add driver for Murata IRS-D200

[PATCH v2 0/3] Add driver for Murata IRS-D200

[Waqar Hameed]

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.

Changes in v2:

[dt-bindings]
* Remove "bindings for" in commit subject.
* Remove superfluous yaml block style indicator ('|') for
  `description:`.
* Change node name in example from `pir` to `proximity`.
* Add required `vdd-supply` property.

[iio]
* Add event enums for running period and count.
* Use `set_trigger_state` callback instead of `iio_buffer_setup_ops`'s
  `predisable` and `postenable`.
* Use `regmap_bulk_read()` in `irsd200_read_data()` and
  `irsd200_read_timer()`.
* Use `regmap_bulk_write()` in `irsd200_write_timer()`.
* Remove comment for macro `IRS_UPPER_COUNT()`.
* Move `IIO_EV_INFO_LOW/HIGH_PASS_FILTER_3DB` from `iio_event_spec` to
  `iio_chan_spec`.
* Ignore timer (`IRS_INTR_TIMER`) interrupts.
* Clarify comment on `ssleep(3)` in `irsd200_write_data_rate()`.
* Only check for non-zero return values from `regmap` functions (as
  opposed to `ret < 0`).
* Add macro defines for operation states.
* Remove fix size in static const array declarations.
* Remove unnecessary `ret` variable in `irsd200_write_raw()`.
* Remove comments in `irsd200_event_spec[]`.
* Remove unnecessary call to `i2c_set_clientdata()` in probe.
* Use `dev_err_probe()` everywhere in probe.
* Remove unnecessary braces around if statement in probe.
* Get and enable regulator in probe.

Link to v1: https://lore.kernel.org/lkml/cover.1686926857.git.waqarh@axis.com/

Waqar Hameed (3):
  dt-bindings: iio: proximity: Add Murata IRS-D200
  iio: Add event enums for running period and count
  iio: Add driver for Murata IRS-D200

 Documentation/ABI/testing/sysfs-bus-iio       |  16 +
 .../iio/proximity/murata,irsd200.yaml         |  60 ++
 drivers/iio/industrialio-event.c              |   2 +
 drivers/iio/proximity/Kconfig                 |  12 +
 drivers/iio/proximity/Makefile                |   1 +
 drivers/iio/proximity/irsd200.c               | 982 ++++++++++++++++++
 include/linux/iio/types.h                     |   2 +
 7 files changed, 1075 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
 create mode 100644 drivers/iio/proximity/irsd200.c


base-commit: 3f01e9fed8454dcd89727016c3e5b2fbb8f8e50c
-- 
2.30.2

[PATCH v2 2/3] iio: Add event enums for running period and count

[Waqar Hameed]

There are devices (such as Murata IRS-D200 PIR proximity sensor) that
check the data signal with a running period. I.e. for a specified time,
they count the number of conditions that have occurred, and then signal
if that is more than a specified amount.

`IIO_EV_INFO_PERIOD` resets when the condition no longer is true and is
therefore not suitable for these devices. Add a new `iio_event_info`
`IIO_EV_INFO_RUNNING_PERIOD` that can be used as a running period. Also
add a new `IIO_EV_INFO_RUNNING_COUNT` that can be used to specify the
number of conditions that must occur during this running period.

Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 16 ++++++++++++++++
 drivers/iio/industrialio-event.c        |  2 ++
 include/linux/iio/types.h               |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 7140e8e7313f..a2854dc9a839 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2163,3 +2163,19 @@ Contact:	linux-iio@vger.kernel.org
 Description:
 		An example format is 16-bytes, 2-digits-per-byte, HEX-string
 		representing the sensor unique ID number.
+
+What:		/sys/.../events/in_proximity_thresh_either_runningperiod
+KernelVersion:	6.6
+Contact:	linux-iio@vger.kernel.org
+Description:
+		A running period of time (in seconds) for which
+		in_proximity_thresh_either_runningcount amount of conditions
+		must occur before an event is generated. If direction is not
+		specified then this period applies to both directions.
+
+What:		/sys/.../events/in_proximity_thresh_either_runningcount
+KernelVersion:	6.6
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Number of conditions that must occur, during a running
+		period, before an event is generated.
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index f77ce49d4c36..19f7a91157ee 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -252,6 +252,8 @@ static const char * const iio_ev_info_text[] = {
 	[IIO_EV_INFO_TIMEOUT] = "timeout",
 	[IIO_EV_INFO_RESET_TIMEOUT] = "reset_timeout",
 	[IIO_EV_INFO_TAP2_MIN_DELAY] = "tap2_min_delay",
+	[IIO_EV_INFO_RUNNING_PERIOD] = "runningperiod",
+	[IIO_EV_INFO_RUNNING_COUNT] = "runningcount",
 };
 
 static enum iio_event_direction iio_ev_attr_dir(struct iio_dev_attr *attr)
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 82faa98c719a..117bde7d6ad7 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -19,6 +19,8 @@ enum iio_event_info {
 	IIO_EV_INFO_TIMEOUT,
 	IIO_EV_INFO_RESET_TIMEOUT,
 	IIO_EV_INFO_TAP2_MIN_DELAY,
+	IIO_EV_INFO_RUNNING_PERIOD,
+	IIO_EV_INFO_RUNNING_COUNT,
 };
 
 #define IIO_VAL_INT 1
-- 
2.30.2

[PATCH v2 1/3] dt-bindings: iio: proximity: Add Murata IRS-D200

[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), power supply
and interrupts.

Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
 .../iio/proximity/murata,irsd200.yaml         | 60 +++++++++++++++++++
 1 file changed, 60 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..67f5389ece67
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
@@ -0,0 +1,60 @@
+# 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 <waqar.hameed@axis.com>
+
+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.
+
+  vdd-supply:
+    description:
+      3.3 V supply voltage.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        proximity@48 {
+            compatible = "murata,irsd200";
+            reg = <0x48>;
+            interrupts = <24 IRQ_TYPE_EDGE_RISING>;
+            vdd-supply = <&regulator_3v3>;
+        };
+    };
+...
-- 
2.30.2

[PATCH v2 3/3] iio: Add driver for Murata IRS-D200

[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 <waqar.hameed@axis.com>
---
 drivers/iio/proximity/Kconfig   |  12 +
 drivers/iio/proximity/Makefile  |   1 +
 drivers/iio/proximity/irsd200.c | 982 ++++++++++++++++++++++++++++++++
 3 files changed, 995 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..e6b9eca1bf8e
--- /dev/null
+++ b/drivers/iio/proximity/irsd200.c
@@ -0,0 +1,982 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for Murata IRS-D200 PIR sensor.
+ *
+ * Copyright (C) 2023 Axis Communications AB
+ */
+
+#include <linux/bitfield.h>
+#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. */
+
+/* Operation states. */
+#define IRS_OP_ACTIVE		0x00
+#define IRS_OP_SLEEP		0x01
+
+/*
+ * Quantization scale value for threshold. Used for conversion from/to register
+ * value.
+ */
+#define IRS_THR_QUANT_SCALE	128
+
+#define IRS_UPPER_COUNT(count)	FIELD_GET(GENMASK(7, 4), count)
+#define IRS_LOWER_COUNT(count)	FIELD_GET(GENMASK(3, 0), count)
+
+/* Index corresponds to the value of IRS_REG_DATA_RATE register. */
+static const int irsd200_data_rates[] = {
+	50,
+	100,
+};
+
+/* Index corresponds to the (field) value of IRS_REG_FILTER register. */
+static const unsigned int irsd200_lp_filter_freq[] = {
+	10,
+	7,
+};
+
+/*
+ * Index corresponds to the (field) value of IRS_REG_FILTER register. Note that
+ * this represents a fractional value (e.g the first value corresponds to 3 / 10
+ * = 0.3 Hz).
+ */
+static const unsigned int irsd200_hp_filter_freq[][2] = {
+	{3, 10},
+	{5, 10},
+};
+
+/* 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;
+	struct regulator *regulator;
+};
+
+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, IRS_OP_ACTIVE);
+	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 void irsd200_regulator_disable(void *data)
+{
+	regulator_disable((struct regulator *)data);
+}
+
+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, &regval);
+	if (ret) {
+		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) {
+		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)
+{
+	u8 buf[2];
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, IRS_REG_DATA_LO, buf,
+			       ARRAY_SIZE(buf));
+	if (ret) {
+		dev_err(data->dev, "Could not bulk read data (%d)\n", ret);
+		return ret;
+	}
+
+	*val = (buf[1] << 8) | buf[0];
+
+	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, &regval);
+	if (ret) {
+		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) {
+		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. The
+	 * device operates normally during this period though. This is more of a
+	 * "guarantee" than trying to prevent other user space reads/writes.
+	 */
+	ssleep(3);
+
+	return 0;
+}
+
+static int irsd200_read_timer(struct irsd200_data *data, int *val, int *val2)
+{
+	u8 buf[2];
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, IRS_REG_TIMER_LO, buf,
+			       ARRAY_SIZE(buf));
+	if (ret) {
+		dev_err(data->dev, "Could not bulk read timer (%d)\n", ret);
+		return ret;
+	}
+
+	ret = irsd200_read_data_rate(data, val2);
+	if (ret)
+		return ret;
+
+	/* Value is 10 bits. IRS_REG_TIMER_HI is the two MSBs. */
+	*val = (buf[1] << 8) | buf[0];
+
+	return 0;
+}
+
+static int irsd200_write_timer(struct irsd200_data *data, int val, int val2)
+{
+	unsigned int regval;
+	int data_rate;
+	u8 buf[2];
+	int ret;
+
+	if (val < 0 || val2 < 0)
+		return -ERANGE;
+
+	ret = irsd200_read_data_rate(data, &data_rate);
+	if (ret)
+		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_LO is the 8 LSBs and IRS_REG_TIMER_HI is the 2 MSBs. */
+	buf[0] = FIELD_GET(GENMASK(7, 0), regval);
+	buf[1] = FIELD_GET(GENMASK(9, 8), regval);
+
+	ret = regmap_bulk_write(data->regmap, IRS_REG_TIMER_LO, buf,
+				ARRAY_SIZE(buf));
+	if (ret) {
+		dev_err(data->dev, "Could not bulk write 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, &regval);
+	if (ret) {
+		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)
+			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) {
+		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], &regval);
+	if (ret) {
+		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) {
+		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], &regval);
+	if (ret) {
+		dev_err(data->dev, "Could not read hp filter frequency (%d)\n",
+			ret);
+		return ret;
+	}
+
+	*val = irsd200_hp_filter_freq[regval][0];
+	*val2 = irsd200_hp_filter_freq[regval][1];
+
+	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][0] == 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) {
+		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)
+			return ret;
+
+		*val = buf;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = irsd200_read_data_rate(data, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = irsd200_read_lp_filter(data, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		ret = irsd200_read_hp_filter(data, val, val2);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_FRACTIONAL;
+	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;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		*vals = irsd200_lp_filter_freq;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(irsd200_lp_filter_freq);
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		*vals = (int *)irsd200_hp_filter_freq;
+		*type = IIO_VAL_FRACTIONAL;
+		*length = 2 * ARRAY_SIZE(irsd200_hp_filter_freq);
+		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);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return irsd200_write_data_rate(data, val);
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return irsd200_write_lp_filter(data, val);
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		return irsd200_write_hp_filter(data, val, val2);
+	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)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_EV_INFO_RUNNING_PERIOD:
+		ret = irsd200_read_timer(data, val, val2);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_FRACTIONAL;
+	case IIO_EV_INFO_RUNNING_COUNT:
+		ret = irsd200_read_nr_count(data, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	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_RUNNING_PERIOD:
+		return irsd200_write_timer(data, val, val2);
+	case IIO_EV_INFO_RUNNING_COUNT:
+		return irsd200_write_nr_count(data, val);
+	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:
+		ret = regmap_field_read(
+			data->regfields[IRS_REGF_INTR_COUNT_THR_OR], &val);
+		if (ret)
+			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:
+		/* Clear the count register (by reading from it). */
+		ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
+		if (ret)
+			return ret;
+
+		val = !!state;
+		ret = regmap_field_write(
+			data->regfields[IRS_REGF_INTR_COUNT_THR_OR], val);
+		if (ret)
+			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_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_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct irsd200_data *data = iio_trigger_get_drvdata(trig);
+	int ret;
+
+	ret = regmap_field_write(data->regfields[IRS_REGF_INTR_DATA], state);
+	if (ret) {
+		dev_err(data->dev, "Could not %s data interrupt source (%d)\n",
+			state ? "enable" : "disable", 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_trigger_ops irsd200_trigger_ops = {
+	.set_trigger_state = irsd200_set_trigger_state,
+	.validate_device = iio_trigger_validate_own_device,
+};
+
+static const struct iio_event_spec irsd200_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate =
+			BIT(IIO_EV_INFO_RUNNING_PERIOD) |
+			BIT(IIO_EV_INFO_RUNNING_COUNT) |
+			BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+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) |
+			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |
+			BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),
+		.info_mask_separate_available =
+			BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |
+			BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),
+		.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;
+	size_t i;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return dev_err_probe(&client->dev, -ENOMEM,
+				     "Could not allocate iio device\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = &client->dev;
+
+	data->regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(data->dev, PTR_ERR(data->regmap),
+				     "Could not initialize regmap\n");
+
+	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]))
+			return dev_err_probe(
+				data->dev, PTR_ERR(data->regfields[i]),
+				"Could not allocate register field %zu\n", i);
+	}
+
+	data->regulator = devm_regulator_get(data->dev, "vdd");
+	if (IS_ERR(data->regulator))
+		return dev_err_probe(data->dev, PTR_ERR(data->regulator),
+				     "Could not get regulator\n");
+
+	ret = regulator_enable(data->regulator);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not enable regulator (%d)\n", ret);
+
+	ret = devm_add_action_or_reset(data->dev, irsd200_regulator_disable,
+				       data->regulator);
+	if (ret)
+		return dev_err_probe(
+			data->dev, ret,
+			"Could not add regulator disable devres action (%d)\n",
+			ret);
+
+	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)
+		return dev_err_probe(data->dev, -ENXIO, "No irq available\n");
+
+	ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev, NULL,
+					      irsd200_trigger_handler, NULL);
+	if (ret)
+		return dev_err_probe(
+			data->dev, ret,
+			"Could not setup iio triggered buffer (%d)\n", ret);
+
+	ret = devm_request_threaded_irq(data->dev, client->irq, NULL,
+					irsd200_irq_thread,
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					NULL, indio_dev);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not request irq (%d)\n", ret);
+
+	trigger = devm_iio_trigger_alloc(data->dev, "%s-dev%d", indio_dev->name,
+					 iio_device_id(indio_dev));
+	if (!trigger)
+		return dev_err_probe(data->dev, -ENOMEM,
+				     "Could not allocate iio trigger\n");
+
+	trigger->ops = &irsd200_trigger_ops;
+	iio_trigger_set_drvdata(trigger, data);
+
+	ret = devm_iio_trigger_register(data->dev, trigger);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not register iio trigger (%d)\n",
+				     ret);
+
+	ret = devm_iio_device_register(data->dev, indio_dev);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not register iio device (%d)\n",
+				     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 <waqar.hameed@axis.com>");
+MODULE_DESCRIPTION("Murata IRS-D200 PIR sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH v2 1/3] dt-bindings: iio: proximity: Add Murata IRS-D200

[Krzysztof Kozlowski]

On 12/07/2023 17:12, 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), power supply
> and interrupts.
> 
> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 3/3] iio: Add driver for Murata IRS-D200

[Jonathan Cameron]

On Wed, 12 Jul 2023 17:12:17 +0200
Waqar Hameed <waqar.hameed@axis.com> 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 <waqar.hameed@axis.com>
Hi Waqar,

Some additional comments inline.

Jonathan

> ---
>  drivers/iio/proximity/Kconfig   |  12 +
>  drivers/iio/proximity/Makefile  |   1 +
>  drivers/iio/proximity/irsd200.c | 982 ++++++++++++++++++++++++++++++++
>  3 files changed, 995 insertions(+)
>  create mode 100644 drivers/iio/proximity/irsd200.c

> diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
> new file mode 100644
> index 000000000000..e6b9eca1bf8e
> --- /dev/null
> +++ b/drivers/iio/proximity/irsd200.c
> @@ -0,0 +1,982 @@



> +
> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
> +{
> +	u8 buf[2];

__le16

> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, IRS_REG_DATA_LO, buf,
> +			       ARRAY_SIZE(buf));
> +	if (ret) {
> +		dev_err(data->dev, "Could not bulk read data (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	*val = (buf[1] << 8) | buf[0];


If you make buf a __le16 then you can use aligned conversions otherwise
get_unaligned_le16(buf) 

> +
> +	return 0;
> +}
> +

...

> +
> +static int irsd200_read_timer(struct irsd200_data *data, int *val, int *val2)
> +{
> +	u8 buf[2];
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, IRS_REG_TIMER_LO, buf,
> +			       ARRAY_SIZE(buf));
> +	if (ret) {
> +		dev_err(data->dev, "Could not bulk read timer (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = irsd200_read_data_rate(data, val2);
> +	if (ret)
> +		return ret;
> +
> +	/* Value is 10 bits. IRS_REG_TIMER_HI is the two MSBs. */
> +	*val = (buf[1] << 8) | buf[0];

Another one where type should be __le16 and appropriate conversion functions
used.

> +
> +	return 0;
> +}
> +
> +static int irsd200_write_timer(struct irsd200_data *data, int val, int val2)
> +{
> +	unsigned int regval;
> +	int data_rate;
> +	u8 buf[2];

__le16 might be more appropriate.

> +	int ret;
> +
> +	if (val < 0 || val2 < 0)
> +		return -ERANGE;
> +
> +	ret = irsd200_read_data_rate(data, &data_rate);
> +	if (ret)
> +		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_LO is the 8 LSBs and IRS_REG_TIMER_HI is the 2 MSBs. */
> +	buf[0] = FIELD_GET(GENMASK(7, 0), regval);
> +	buf[1] = FIELD_GET(GENMASK(9, 8), regval);

I think I'd rather see this as appropriate put_unaligned_le16() or similar with
the input masked.

> +
> +	ret = regmap_bulk_write(data->regmap, IRS_REG_TIMER_LO, buf,
> +				ARRAY_SIZE(buf));
> +	if (ret) {
> +		dev_err(data->dev, "Could not bulk write timer (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
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))

BIT(3) is an unusual representation... Here, 8 is probably clearer unless
it really has something to do with that bit being set.

> +		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)
> +			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) {
> +		dev_err(data->dev, "Could not write nr count (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 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:
> +		/* Clear the count register (by reading from it). */
> +		ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
I'd use a different variable than val, preferably named to indicate
we don't care about the contents.  Using val here and below is a little
confusing.

> +		if (ret)
> +			return ret;
> +
> +		val = !!state;
> +		ret = regmap_field_write(
> +			data->regfields[IRS_REGF_INTR_COUNT_THR_OR], val);
> +		if (ret)
> +			return ret;
> +
> +		return val;

Why return val?  Should probably be 0 if this write succeeded.

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

As below.  IRQ_NONE does not normally mean error, it means we are sure it is not
our interrupt.  Even in error cases, return IRQ_HANDLED.

If you confirm via status bits that it isn't out interrupt, that's when you
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_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) {

if (!clear) then it's not our interrupt (I think) and you should return IRQ_NONE.

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

An error in a read is not reported as IRQ_NONE.  The only time we should report
that is if we know it definitely wasn't our interrupt.  Otherwise we will
fire the spurious interrupt handling disable logic for the wrong reason.

> +}
> +


> +
> +static int irsd200_probe(struct i2c_client *client)
> +{
> +	struct iio_trigger *trigger;
> +	struct irsd200_data *data;
> +	struct iio_dev *indio_dev;
> +	size_t i;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +				     "Could not allocate iio device\n");
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = &client->dev;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(data->dev, PTR_ERR(data->regmap),
> +				     "Could not initialize regmap\n");
> +
> +	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]))
> +			return dev_err_probe(
> +				data->dev, PTR_ERR(data->regfields[i]),
> +				"Could not allocate register field %zu\n", i);
> +	}
> +
> +	data->regulator = devm_regulator_get(data->dev, "vdd");

devm_regulator_get_enable()

> +	if (IS_ERR(data->regulator))
> +		return dev_err_probe(data->dev, PTR_ERR(data->regulator),
> +				     "Could not get regulator\n");
> +
> +	ret = regulator_enable(data->regulator);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Could not enable regulator (%d)\n", ret);
> +
> +	ret = devm_add_action_or_reset(data->dev, irsd200_regulator_disable,
> +				       data->regulator);
> +	if (ret)
> +		return dev_err_probe(
> +			data->dev, ret,
> +			"Could not add regulator disable devres action (%d)\n",
> +			ret);

...

Re: [PATCH v2 3/3] iio: Add driver for Murata IRS-D200

[Waqar Hameed]

On Sun, Jul 16, 2023 at 16:11 +0100 Jonathan Cameron <jic23@kernel.org> wrote:

[...]

>> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
>> +{
>> +	u8 buf[2];
>
> __le16
>
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(data->regmap, IRS_REG_DATA_LO, buf,
>> +			       ARRAY_SIZE(buf));
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not bulk read data (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	*val = (buf[1] << 8) | buf[0];
>
>
> If you make buf a __le16 then you can use aligned conversions otherwise
> get_unaligned_le16(buf) 
>
>> +
>> +	return 0;
>> +}
>> +
>
> ...
>
>> +
>> +static int irsd200_read_timer(struct irsd200_data *data, int *val, int *val2)
>> +{
>> +	u8 buf[2];
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(data->regmap, IRS_REG_TIMER_LO, buf,
>> +			       ARRAY_SIZE(buf));
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not bulk read timer (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = irsd200_read_data_rate(data, val2);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Value is 10 bits. IRS_REG_TIMER_HI is the two MSBs. */
>> +	*val = (buf[1] << 8) | buf[0];
>
> Another one where type should be __le16 and appropriate conversion functions
> used.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int irsd200_write_timer(struct irsd200_data *data, int val, int val2)
>> +{
>> +	unsigned int regval;
>> +	int data_rate;
>> +	u8 buf[2];
>
> __le16 might be more appropriate.
>
>> +	int ret;
>> +
>> +	if (val < 0 || val2 < 0)
>> +		return -ERANGE;
>> +
>> +	ret = irsd200_read_data_rate(data, &data_rate);
>> +	if (ret)
>> +		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_LO is the 8 LSBs and IRS_REG_TIMER_HI is the 2 MSBs. */
>> +	buf[0] = FIELD_GET(GENMASK(7, 0), regval);
>> +	buf[1] = FIELD_GET(GENMASK(9, 8), regval);
>
> I think I'd rather see this as appropriate put_unaligned_le16() or similar with
> the input masked.

I was choosing between these functions or manually doing things. I'll
update to use the endian conversions!

>> +
>> +	ret = regmap_bulk_write(data->regmap, IRS_REG_TIMER_LO, buf,
>> +				ARRAY_SIZE(buf));
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not bulk write timer (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 0;

(The mystery continues! :) )

>> +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))
>
> BIT(3) is an unusual representation... Here, 8 is probably clearer unless
> it really has something to do with that bit being set.

Yeah, I agree.

[...]

>> +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:
>> +		/* Clear the count register (by reading from it). */
>> +		ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
> I'd use a different variable than val, preferably named to indicate
> we don't care about the contents.  Using val here and below is a little
> confusing.

I get that. Let's use `tmp`.

>
>> +		if (ret)
>> +			return ret;
>> +
>> +		val = !!state;
>> +		ret = regmap_field_write(
>> +			data->regfields[IRS_REGF_INTR_COUNT_THR_OR], val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return val;
>
> Why return val?  Should probably be 0 if this write succeeded.

Yes, will fix! (copy-pasting is dangerous...)

>
>> +	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;
>
> As below.  IRQ_NONE does not normally mean error, it means we are sure it is not
> our interrupt.  Even in error cases, return IRQ_HANDLED.
>
> If you confirm via status bits that it isn't out interrupt, that's when you
> return IRQ_NONE.

Alright, let's do that then (and in `irsd200_trigger_handler()`).

>> +	}
>> +
>> +	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_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) {
>
> if (!clear) then it's not our interrupt (I think) and you should return IRQ_NONE.

Yeah, this is probably the only place we can be sure it isn't our
interrupt.

[...]

>> +static int irsd200_probe(struct i2c_client *client)
>> +{
>> +	struct iio_trigger *trigger;
>> +	struct irsd200_data *data;
>> +	struct iio_dev *indio_dev;
>> +	size_t i;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return dev_err_probe(&client->dev, -ENOMEM,
>> +				     "Could not allocate iio device\n");
>> +
>> +	data = iio_priv(indio_dev);
>> +	data->dev = &client->dev;
>> +
>> +	data->regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
>> +	if (IS_ERR(data->regmap))
>> +		return dev_err_probe(data->dev, PTR_ERR(data->regmap),
>> +				     "Could not initialize regmap\n");
>> +
>> +	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]))
>> +			return dev_err_probe(
>> +				data->dev, PTR_ERR(data->regfields[i]),
>> +				"Could not allocate register field %zu\n", i);
>> +	}
>> +
>> +	data->regulator = devm_regulator_get(data->dev, "vdd");
>
> devm_regulator_get_enable()

I suggested it, but didn't use it... (because I was testing with
hardware on an older tree where it was not present...). Sorry!