Add driver for Nicera D3-323-AA PIR sensor

Nicera D3-323-AA is a PIR sensor for human detection. It has support for raw data measurements and detection notification. The communication protocol is custom made and therefore needs to be GPIO bit banged.

Add devicetree bindings requiring the compatible string and the various GPIOs needed for operation. Some of the GPIOs have multiple use-cases depending on device state. Describe these thoroughly.

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +9 -0

+    maxItems: 1
+    description:
+      GPIO for data reading and writing.
+      During configuration, configuration data will be written and read back
+      with bit banging (together with clk-vout-gpio as clock).
+      After this, during operational mode, the device will output serial data on
+      this GPIO. However, the driver currently doesn't read this.
+
+required:

+ };
+...

Quoted 7 lines · Waqar Hameed

Nicera D3-323-AA is a PIR sensor for human detection. It has support for raw data measurements and detection notification. The communication protocol is custom made and therefore needs to be GPIO bit banged.

Add devicetree bindings requiring the compatible string and the various GPIOs needed for operation. Some of the GPIOs have multiple use-cases depending on device state. Describe these thoroughly.

Drop redundant parts of description. Describe the hardware.

Entire last paragraph is pretty pointless.

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 23

Quoted 10 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +9 -0

+
+properties:
+  compatible:
+    const: nicera,d3323aa
+
+  vdd-gpio:
+    maxItems: 1
+    description:
+      GPIO for supply voltage (1.8 to 5.5 V).

This is not how pins are represented in the kernel. Either you have here regulator (supply) or reset gpios.

Plus 'gpio' suffix is not valid, btw.

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 27

Quoted 4 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +4 -0

+      This GPIO will be driven low by the driver in order to cut the supply and
+      reset the device (driving it then back to high to power it on).
+
+  clk-vout-gpio:

No, for the similar reasons. Which pin is this?

Quoted 4 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +3 -0

+    maxItems: 1
+    description:
+      GPIO for clock and detection.

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 40

Quoted 10 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +10 -0

+      After reset, the device signals with two falling edges on this pin that it
+      is ready for configuration (within 1.2 s), which the driver listens for as
+      interrupts.
+      During configuration, it is used as clock for data reading and writing (on
+      data-gpio). The driver drives this pin with the frequency of 1 kHz (bit
+      banging).
+      After all this, the device is now in operational mode and signals on this
+      pin for any detections. The driver listens for this as interrupts.
+
+  data-gpio:

There is no such pin.

Quoted 10 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +9 -0

+    maxItems: 1
+    description:
+      GPIO for data reading and writing.
+      During configuration, configuration data will be written and read back
+      with bit banging (together with clk-vout-gpio as clock).
+      After this, during operational mode, the device will output serial data on
+      this GPIO. However, the driver currently doesn't read this.
+
+required:

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 59

Quoted 10 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +10 -0

+  - compatible
+  - vdd-gpio
+  - clk-vout-gpio
+  - data-gpio
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>

So you included that header

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 65

Quoted 6 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +6 -0

+
+    proximity {
+        compatible = "nicera,d3323aa";
+        vdd-gpio = <&gpio 73 0>;
+        clk-vout-gpio = <&gpio 78 0>;
+        data-gpio = <&gpio 76 0>;

But where are you using it?

Quoted 11 lines · On Fri, May 09, 2025 at 17:06 +0200 Krzysztof Kozlowski <[email protected]> wrote:

Quoted 7 lines · Waqar Hameed

Nicera D3-323-AA is a PIR sensor for human detection. It has support for raw data measurements and detection notification. The communication protocol is custom made and therefore needs to be GPIO bit banged.

Add devicetree bindings requiring the compatible string and the various GPIOs needed for operation. Some of the GPIOs have multiple use-cases depending on device state. Describe these thoroughly.

Drop redundant parts of description. Describe the hardware.

I'll reformulate and incorporate some information of the pins and how it is used in the hardware.

Quoted 1 line

Entire last paragraph is pretty pointless.

I'll remove it then. (Some sub-system maintainers want a description of what the patch does, in imperative form. But I can see why it might not add any value when it comes to dt-bindings.)

Quoted 13 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 23

Quoted 10 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +9 -0

+
+properties:
+  compatible:
+    const: nicera,d3323aa
+
+  vdd-gpio:
+    maxItems: 1
+    description:
+      GPIO for supply voltage (1.8 to 5.5 V).

This is not how pins are represented in the kernel. Either you have here regulator (supply) or reset gpios.

I'll change it to vdd-supply.

Quoted 1 line

Plus 'gpio' suffix is not valid, btw.

I actually greped before writing this to see if there were other dt-bindings with this suffix. Because the GPIO framework supports both gpio and gpios as suffixes (see gpio_suffixes[] in gpiolib.c). However, since -gpios are clearly in majority, we should go for that.

[...]

Quoted 6 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 27

Quoted 4 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +4 -0

+      This GPIO will be driven low by the driver in order to cut the supply and
+      reset the device (driving it then back to high to power it on).
+
+  clk-vout-gpio:

No, for the similar reasons. Which pin is this?

This pin is a little weird actually. As described below, right after power on, it is used as an interrupt to signal "ready for configuration". Then, it used as a bit banged clock signal for configuration. Finally, it is back as interrupt pin for threshold PIR sensor detections.

So, I'm not really sure what to call this (just opted for what it's called in the data sheet: "Vout (CLK)"). Just clk-gpios wouldn't be correct either, right? Should we prefix it with the vendor nicera,? Or any other suggestions?

Quoted 16 lines

Quoted 15 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +13 -0

+    maxItems: 1
+    description:
+      GPIO for clock and detection.
+      After reset, the device signals with two falling edges on this pin that it
+      is ready for configuration (within 1.2 s), which the driver listens for as
+      interrupts.
+      During configuration, it is used as clock for data reading and writing (on
+      data-gpio). The driver drives this pin with the frequency of 1 kHz (bit
+      banging).
+      After all this, the device is now in operational mode and signals on this
+      pin for any detections. The driver listens for this as interrupts.
+
+  data-gpio:

Quoted 1 line

There is no such pin.

You mean to change it to data-gpios? (There are some using that, e.g. sensirion,sht15.yaml).

Quoted 9 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 47

Quoted 9 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +7 -0

+    maxItems: 1
+    description:
+      GPIO for data reading and writing.
+      During configuration, configuration data will be written and read back
+      with bit banging (together with clk-vout-gpio as clock).
+      After this, during operational mode, the device will output serial data on
+      this GPIO. However, the driver currently doesn't read this.

Quoted 41 lines · Waqar Hameed

Quoted 11 lines · On Fri, May 09, 2025 at 17:06 +0200 Krzysztof Kozlowski <[email protected]> wrote:

Quoted 7 lines · Waqar Hameed

Nicera D3-323-AA is a PIR sensor for human detection. It has support for raw data measurements and detection notification. The communication protocol is custom made and therefore needs to be GPIO bit banged.

Add devicetree bindings requiring the compatible string and the various GPIOs needed for operation. Some of the GPIOs have multiple use-cases depending on device state. Describe these thoroughly.

Drop redundant parts of description. Describe the hardware.

I'll reformulate and incorporate some information of the pins and how it is used in the hardware.

Quoted 1 line

Entire last paragraph is pretty pointless.

I'll remove it then. (Some sub-system maintainers want a description of what the patch does, in imperative form. But I can see why it might not add any value when it comes to dt-bindings.)

Quoted 13 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 23

Quoted 10 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +9 -0

+
+properties:
+  compatible:
+    const: nicera,d3323aa
+
+  vdd-gpio:
+    maxItems: 1
+    description:
+      GPIO for supply voltage (1.8 to 5.5 V).

This is not how pins are represented in the kernel. Either you have here regulator (supply) or reset gpios.

I'll change it to vdd-supply.

Quoted 1 line

Plus 'gpio' suffix is not valid, btw.

Quoted 4 lines

I actually greped before writing this to see if there were other dt-bindings with this suffix. Because the GPIO framework supports both gpio and gpios as suffixes (see gpio_suffixes[] in gpiolib.c). However, since -gpios are clearly in majority, we should go for that.

One is deprecated. It is always, always gpios.

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +13 -0

+    maxItems: 1
+    description:
+      GPIO for clock and detection.
+      After reset, the device signals with two falling edges on this pin that it
+      is ready for configuration (within 1.2 s), which the driver listens for as
+      interrupts.
+      During configuration, it is used as clock for data reading and writing (on
+      data-gpio). The driver drives this pin with the frequency of 1 kHz (bit
+      banging).
+      After all this, the device is now in operational mode and signals on this
+      pin for any detections. The driver listens for this as interrupts.
+
+  data-gpio:

Quoted 4 lines

Quoted 1 line

There is no such pin.

You mean to change it to data-gpios? (There are some using that, e.g. sensirion,sht15.yaml).

No, I meant I opened datasheet and found no such pin. Unless you meant DO, but then mention in description the actual name of the pin, if you are using some more descriptive property name for it.

Quoted 7 lines

Quoted 7 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml line 47

Quoted 7 lines

Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +7 -0

+    maxItems: 1
+    description:
+      GPIO for data reading and writing.
+      During configuration, configuration data will be written and read back
+      with bit banging (together with clk-vout-gpio as clock).
+      After this, during operational mode, the device will output serial data on
+      this GPIO. However, the driver currently doesn't read this.

BTW, drop all references to driver here and in other places. If driver change, you will change binding? If my driver behaves differently, why binding would be claiming something else?

Nicera D3-323-AA is a PIR sensor for human detection. It has support for raw data measurements and detection notification. The communication protocol is custom made and therefore needs to be GPIO bit banged.

The device has two main settings that can be configured: a threshold value for detection and a band-pass filter. The configurable parameters for the band-pass filter are the high-pass and low-pass cutoff frequencies and its peak gain. Map these settings to the corresponding parameters in the iio framework.

The low-pass and high-pass cutoff frequencies are pairwise for the different available filter types. Because of this, only allow to set the low-pass cutoff frequency from sysfs and use that to configure the corresponding high-pass cutoff frequency. This is sufficient to unambiguously choose a filter type.

Raw data measurements can be obtained from the device. However, since we rely on bit banging, it will be rather cumbersome with buffer support. The main reason being that the data protocol has strict timing requirements (it's serial like UART), and it's mainly used during debugging since in real-world applications only the event notification is of importance. Therefore, only add support for events (for now).

Signed-off-by Waqar Hameed <[email protected]>

---

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

+		if (val == integer && val2 == fract)
+			break;
+	}
+
+	if (idx == ARRAY_SIZE(d3323aa_lp_filter_freq))
+		return -ERANGE;
+
+	bitmap_write(regmap, d3323aa_lp_filter_regval[idx],
+		     D3323AA_REG_BIT_FILSEL0, D3323AA_FILTER_TYPE_NR_BITS);
+
+	return 0;
+}

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

+static int d3323aa_probe(struct platform_device *pdev)
+{
+	DECLARE_BITMAP(default_regmap, D3323AA_REG_NR_BITS);
+	struct d3323aa_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
+	if (!indio_dev)
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "Could not allocate iio device\n");
+

drivers/iio/proximity/d3323aa.c new file +40 -0

+
+	/* Request GPIOs. */
+	data->gpiod_vdd = devm_gpiod_get(data->dev, "vdd", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_vdd))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_vdd),
+				     "Could not get GPIO vdd (%ld)\n",
+				     PTR_ERR(data->gpiod_vdd));
+
+	data->gpiod_clk_vout =
+		devm_gpiod_get(data->dev, "clk-vout", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_clk_vout))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_clk_vout),
+				     "Could not get GPIO clk-vout (%ld)\n",
+				     PTR_ERR(data->gpiod_clk_vout));
+
+	data->gpiod_data = devm_gpiod_get(data->dev, "data", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_data))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_data),
+				     "Could not get GPIO data (%ld)\n",
+				     PTR_ERR(data->gpiod_data));
+
+	ret = gpiod_to_irq(data->gpiod_clk_vout);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret, "Could not get IRQ (%d)\n",
+				     ret);
+
+	data->irq = ret;
+
+	/* Do one setup with the default values. */
+	bitmap_zero(default_regmap, D3323AA_REG_NR_BITS);
+	d3323aa_set_threshold(default_regmap, D3323AA_THRESH_DEFAULT_VAL);
+	d3323aa_set_filter_gain(default_regmap,
+				D3323AA_FILTER_GAIN_DEFAULT_VAL);
+	ret = d3323aa_setup(indio_dev, default_regmap);
+	if (ret)
+		return ret;
+
+	indio_dev->info = &d3323aa_info;
+	indio_dev->name = D3323AA_DRV_NAME;
+	indio_dev->channels = d3323aa_channels;

Hi,

...

drivers/iio/proximity/d3323aa.c line 519

Quoted 13 lines

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

+static int d3323aa_set_lp_filter_freq(unsigned long *regmap, const int val,
+				      int val2)
+{
+	size_t idx;
+
+	/* Truncate fractional part to one digit. */
+	val2 /= 100000;
+
+	for (idx = 0; idx < ARRAY_SIZE(d3323aa_lp_filter_freq); ++idx) {
+		int integer = d3323aa_lp_filter_freq[idx][0] /
+			      d3323aa_lp_filter_freq[idx][1];
+		int fract = d3323aa_lp_filter_freq[idx][0] %
+			    d3323aa_lp_filter_freq[idx][1];

Missing newline.

Quoted 13 lines

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

+		if (val == integer && val2 == fract)
+			break;
+	}
+
+	if (idx == ARRAY_SIZE(d3323aa_lp_filter_freq))
+		return -ERANGE;
+
+	bitmap_write(regmap, d3323aa_lp_filter_regval[idx],
+		     D3323AA_REG_BIT_FILSEL0, D3323AA_FILTER_TYPE_NR_BITS);
+
+	return 0;
+}

...

Quoted 12 lines

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

+static int d3323aa_probe(struct platform_device *pdev)
+{
+	DECLARE_BITMAP(default_regmap, D3323AA_REG_NR_BITS);
+	struct d3323aa_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
+	if (!indio_dev)
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "Could not allocate iio device\n");
+

drivers/iio/proximity/d3323aa.c line 787

Quoted 10 lines

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

+	data = iio_priv(indio_dev);
+	data->dev = &pdev->dev;
+	platform_set_drvdata(pdev, indio_dev);
+
+	init_completion(&data->reset_completion);
+
+	ret = devm_mutex_init(data->dev, &data->regmap_lock);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not initialize mutex (%d)\n", ret);

No need to repeat the error code in the message, when using dev_err_probe().

Same for all calls below.

Quoted 41 lines

drivers/iio/proximity/d3323aa.c new file +40 -0

+
+	/* Request GPIOs. */
+	data->gpiod_vdd = devm_gpiod_get(data->dev, "vdd", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_vdd))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_vdd),
+				     "Could not get GPIO vdd (%ld)\n",
+				     PTR_ERR(data->gpiod_vdd));
+
+	data->gpiod_clk_vout =
+		devm_gpiod_get(data->dev, "clk-vout", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_clk_vout))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_clk_vout),
+				     "Could not get GPIO clk-vout (%ld)\n",
+				     PTR_ERR(data->gpiod_clk_vout));
+
+	data->gpiod_data = devm_gpiod_get(data->dev, "data", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_data))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_data),
+				     "Could not get GPIO data (%ld)\n",
+				     PTR_ERR(data->gpiod_data));
+
+	ret = gpiod_to_irq(data->gpiod_clk_vout);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret, "Could not get IRQ (%d)\n",
+				     ret);
+
+	data->irq = ret;
+
+	/* Do one setup with the default values. */
+	bitmap_zero(default_regmap, D3323AA_REG_NR_BITS);
+	d3323aa_set_threshold(default_regmap, D3323AA_THRESH_DEFAULT_VAL);
+	d3323aa_set_filter_gain(default_regmap,
+				D3323AA_FILTER_GAIN_DEFAULT_VAL);
+	ret = d3323aa_setup(indio_dev, default_regmap);
+	if (ret)
+		return ret;
+
+	indio_dev->info = &d3323aa_info;
+	indio_dev->name = D3323AA_DRV_NAME;
+	indio_dev->channels = d3323aa_channels;

drivers/iio/proximity/d3323aa.c line 837

Quoted 10 lines

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

+	indio_dev->num_channels = ARRAY_SIZE(d3323aa_channels);
+
+	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;
+}

...

CJ

Nicera D3-323-AA is a PIR sensor for human detection. It has support for raw data measurements and detection notification. The communication protocol is custom made and therefore needs to be GPIO bit banged.

The device has two main settings that can be configured: a threshold value for detection and a band-pass filter. The configurable parameters for the band-pass filter are the high-pass and low-pass cutoff frequencies and its peak gain. Map these settings to the corresponding parameters in the iio framework.

The low-pass and high-pass cutoff frequencies are pairwise for the different available filter types. Because of this, only allow to set the low-pass cutoff frequency from sysfs and use that to configure the corresponding high-pass cutoff frequency. This is sufficient to unambiguously choose a filter type.

Raw data measurements can be obtained from the device. However, since we rely on bit banging, it will be rather cumbersome with buffer support. The main reason being that the data protocol has strict timing requirements (it's serial like UART), and it's mainly used during debugging since in real-world applications only the event notification is of importance. Therefore, only add support for events (for now).

Signed-off-by Waqar Hameed <[email protected]>

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

new file mode 100644
index 000000000000..fa08b52636ba
@@ -0,0 +1,868 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for Nicera D3-323-AA PIR sensor.
+ *
+ * Copyright (C) 2025 Axis Communications AB
+ */
+
+#include <linux/atomic.h>
+#include <linux/bitmap.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>

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

+
+/*
+ * Register bitmap.
+ * For some reason the first bit is denoted as F37 in the datasheet, the second
+ * as F38 and so on. Note the gap between F60 and F64.

drivers/iio/proximity/d3323aa.c new file +70 -0

+#define D3323AA_REG_BIT_SLAVEA10	9	/* F46. */
+#define D3323AA_REG_BIT_DETLVLABS0	10	/* F47. */
+#define D3323AA_REG_BIT_DETLVLABS1	11	/* F48. */
+#define D3323AA_REG_BIT_DETLVLABS2	12	/* F49. */
+#define D3323AA_REG_BIT_DETLVLABS3	13	/* F50. */
+#define D3323AA_REG_BIT_DETLVLABS4	14	/* F51. */
+#define D3323AA_REG_BIT_DETLVLABS5	15	/* F52. */
+#define D3323AA_REG_BIT_DETLVLABS6	16	/* F53. */
+#define D3323AA_REG_BIT_DETLVLABS7	17	/* F54. */
+#define D3323AA_REG_BIT_DSLP		18	/* F55. */
+#define D3323AA_REG_BIT_FSTEP0		19	/* F56. */
+#define D3323AA_REG_BIT_FSTEP1		20	/* F57. */
+#define D3323AA_REG_BIT_FILSEL0		21	/* F58. */
+#define D3323AA_REG_BIT_FILSEL1		22	/* F59. */
+#define D3323AA_REG_BIT_FILSEL2		23	/* F60. */
+#define D3323AA_REG_BIT_FDSET		24	/* F64. */
+#define D3323AA_REG_BIT_F65		25
+#define D3323AA_REG_BIT_F87		(D3323AA_REG_BIT_F65 + (87 - 65))
+
+#define D3323AA_REG_NR_BITS (D3323AA_REG_BIT_F87 - D3323AA_REG_BIT_SLAVEA1 + 1)
+#define D3323AA_THRESH_REG_NR_BITS                                             \
+	(D3323AA_REG_BIT_DETLVLABS7 - D3323AA_REG_BIT_DETLVLABS0 + 1)
+#define D3323AA_FILTER_TYPE_NR_BITS                                            \
+	(D3323AA_REG_BIT_FILSEL2 - D3323AA_REG_BIT_FILSEL0 + 1)
+#define D3323AA_FILTER_GAIN_REG_NR_BITS                                        \
+	(D3323AA_REG_BIT_FSTEP1 - D3323AA_REG_BIT_FSTEP0 + 1)
 
+/*
+ * High-pass filter cutoff frequency for the band-pass filter. There is a
+ * corresponding low-pass cutoff frequency for each of the filter types
+ * (denoted A, B, C and D in the datasheet). The index in this array matches
+ * that corresponding value in d3323aa_lp_filter_freq.
+ * Note that this represents a fractional value (e.g. the first value
+ * corresponds to 4 / 10 = 0.4 Hz).
+ */
+static const int d3323aa_hp_filter_freq[][2] = {
+	{ 4, 10 },
+	{ 3, 10 },
+	{ 3, 10 },
+	{ 1, 100 },
+};
+
+/*
+ * Low-pass filter cutoff frequency for the band-pass filter. There is a
+ * corresponding high-pass cutoff frequency for each of the filter types
+ * (denoted A, B, C and D in the datasheet). The index in this array matches
+ * that corresponding value in d3323aa_hp_filter_freq.
+ * Note that this represents a fractional value (e.g. the first value
+ * corresponds to 27 / 10 = 2.7 Hz).
+ */
+static const int d3323aa_lp_filter_freq[][2] = {
+	{ 27, 10 },
+	{ 15, 10 },
+	{ 5, 1 },
+	{ 100, 1 },
+};
+
+/*
+ * Register bitmap values for filter types (denoted A, B, C and D in the
+ * datasheet). The index in this array matches the corresponding value in
+ * d3323aa_lp_filter_freq (which in turn matches d3323aa_hp_filter_freq). For
+ * example, the first value 7 corresponds to 2.7 Hz low-pass and 0.4 Hz
+ * high-pass cutoff frequency.
+ */
+static const int d3323aa_lp_filter_regval[] = {
+	7,
+	0,
+	1,
+	2,
+};
+

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

+
+/*
+ * Register bitmap values for the filter gain. The index in this array is the
+ * corresponding index in d3323aa_filter_gain for the gain value.
+ */
+static const u8 d3323aa_filter_gain_regval[] = {
+	1,
+	3,
+	0,
+};
+
+struct d3323aa_data {
+	struct completion reset_completion;
+	/*
+	 *  Since the setup process always requires a complete write of the
+	 *  _whole_ register bitmap, we need to synchronize it with a lock.

drivers/iio/proximity/d3323aa.c new file +45 -0

+	struct gpio_desc *gpiod_clk_vout;
+	/* Input (setting) or output data. */
+	struct gpio_desc *gpiod_data;
+
+	DECLARE_BITMAP(regmap, D3323AA_REG_NR_BITS);
+
+	/*
+	 * Indicator for operational mode (configuring or detecting), i.e.
+	 * d3323aa_irq_detection() registered or not.
+	 */
+	bool detecting;
+};
+
+static int d3323aa_read_settings(struct iio_dev *indio_dev,
+				 unsigned long *regmap)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	size_t i;
+	int ret;
+
+	/* Bit bang the clock and data pins. */
+	ret = gpiod_direction_output(data->gpiod_clk_vout, 0);
+	if (ret)
+		return ret;
+
+	ret = gpiod_direction_input(data->gpiod_data);
+	if (ret)
+		return ret;
+
+	dev_dbg(data->dev, "Reading settings...\n");
+
+	for (i = 0; i < D3323AA_REG_NR_BITS; ++i) {
+		/* Clock frequency needs to be 1 kHz. */
+		gpiod_set_value(data->gpiod_clk_vout, 1);
+		udelay(500);
+
+		/* The data seems to change when clock signal is high. */
+		if (gpiod_get_value(data->gpiod_data))
+			set_bit(i, regmap);
+
+		gpiod_set_value(data->gpiod_clk_vout, 0);
+		udelay(500);
+	}
+
+	/* The first bit (F37) is just dummy data. Discard it. */

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

+{
+	DECLARE_BITMAP(end_pattern, D3323AA_SETTING_END_PATTERN_NR_BITS);
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	size_t i;
+	int ret;
+
+	/* Bit bang the clock and data pins. */
+	ret = gpiod_direction_output(data->gpiod_clk_vout, 0);
+	if (ret)
+		return ret;
+
+	ret = gpiod_direction_output(data->gpiod_data, 0);
+	if (ret)
+		return ret;
+
+	dev_dbg(data->dev, "Writing settings...\n");
+
+	/* First bit (F37) is not used when writing the register map. */
+	for (i = 1; i < D3323AA_REG_NR_BITS; ++i) {
+		gpiod_set_value(data->gpiod_data, test_bit(i, regmap));
+

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

+	for (i = 0; i < D3323AA_SETTING_END_PATTERN_NR_BITS; ++i) {
+		gpiod_set_value(data->gpiod_data, test_bit(i, end_pattern));
+
+		gpiod_set_value(data->gpiod_clk_vout, 1);
+		udelay(500);
+		gpiod_set_value(data->gpiod_clk_vout, 0);
+		udelay(500);
+	}
+
+	/* Datasheet says to wait 30 ms after writing the settings. */
+	msleep(30);
+
+	return 0;

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

+	if (val < 0) {
+		dev_err_ratelimited(data->dev,
+				    "Could not read from GPIO clk-vout (%d)\n",
+				    val);
+		return IRQ_HANDLED;
+	}
+
+	dir = val ? IIO_EV_DIR_RISING : IIO_EV_DIR_FALLING;
+	iio_push_event(indio_dev,
+		       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+					    IIO_EV_TYPE_THRESH, dir),
+		       iio_get_time_ns(indio_dev));

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

+	if (count == D3323AA_IRQ_RESET_COUNT)
+		complete(&data->reset_completion);
+
+	return IRQ_HANDLED;
+}
+
+static int d3323aa_reset(struct iio_dev *indio_dev)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	long time;
+	int ret;
+
+	/*
+	 * Datasheet says VDD needs to be low at least for 30 ms. Let's add a
+	 * couple more to allow VDD to completely discharge as well.
+	 */
+	gpiod_set_value(data->gpiod_vdd, 0);
+	msleep(30 + 5);
+
+	/*
+	 * After setting VDD to high, the device signals with
+	 * D3323AA_IRQ_RESET_COUNT falling edges on CLK/Vout that it is now
+	 * ready for configuration. Datasheet says that this should happen
+	 * within D3323AA_RESET_TIMEOUT ms. Count these two edges within that
+	 * timeout.
+	 */
+	atomic_set(&data->irq_reset_count, 0);
+	reinit_completion(&data->reset_completion);
+
+	ret = gpiod_direction_input(data->gpiod_clk_vout);
+	if (ret)
+		return ret;
+
+	dev_dbg(data->dev, "Resetting...\n");
+
+	gpiod_set_value(data->gpiod_vdd, 1);
+
+	/*
+	 * Datasheet doesn't mention this but measurements have shown that
+	 * CLK/Vout signal slowly ramps up during the first 1.5 ms after reset.
+	 * This means that the digital signal will have bogus values during this
+	 * period. Let's wait for this ramp-up before counting the "real"
+	 * falling edges.
+	 */
+	usleep_range(2000, 5000);
+
+	if (data->detecting) {
+		/*

drivers/iio/proximity/d3323aa.c new file +74 -0

+	if (ret)
+		return ret;
+
+	time = wait_for_completion_killable_timeout(
+		&data->reset_completion,
+		msecs_to_jiffies(D3323AA_RESET_TIMEOUT));
+	free_irq(data->irq, indio_dev);
+	if (time == 0) {
+		return -ETIMEDOUT;
+	} else if (time < 0) {
+		/* Got interrupted. */
+		return time;
+	}
+
+	dev_dbg(data->dev, "Reset completed\n");
+
+	return 0;
+}
+
+static int d3323aa_setup(struct iio_dev *indio_dev, const unsigned long *regmap)
+{
+	DECLARE_BITMAP(read_regmap, D3323AA_REG_NR_BITS);
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	unsigned long start_time;
+	int ret;
+
+	ret = d3323aa_reset(indio_dev);
+	if (ret) {
+		if (ret != -ERESTARTSYS)
+			dev_err(data->dev, "Could not reset device (%d)\n",
+				ret);
+
+		return ret;
+	}
+
+	/*
+	 * Datasheet says to wait 10 us before setting the configuration.
+	 * Moreover, the total configuration should be done within
+	 * D3323AA_CONFIG_TIMEOUT ms. Clock it.
+	 */
+	usleep_range(10, 20);
+	start_time = jiffies;
+
+	ret = d3323aa_write_settings(indio_dev, regmap);
+	if (ret) {
+		dev_err(data->dev, "Could not write settings (%d)\n", ret);
+		return ret;
+	}
+
+	ret = d3323aa_read_settings(indio_dev, read_regmap);
+	if (ret) {
+		dev_err(data->dev, "Could not read settings (%d)\n", ret);
+		return ret;
+	}
+
+	if (time_is_before_jiffies(start_time +
+				   msecs_to_jiffies(D3323AA_CONFIG_TIMEOUT))) {
+		dev_err(data->dev, "Could not set up configuration in time\n");
+		return -EAGAIN;
+	}
+
+	/* Check if settings were set successfully. */
+	if (!bitmap_equal(regmap, read_regmap, D3323AA_REG_NR_BITS)) {
+		dev_err(data->dev, "Settings data mismatch\n");
+		return -EIO;
+	}
+
+	/* Now in operational mode. */
+	ret = gpiod_direction_input(data->gpiod_clk_vout);
+	if (ret) {
+		dev_err(data->dev,
+			"Could not set GPIO clk-vout as input (%d)\n", ret);
+		return ret;
+	}

drivers/iio/proximity/d3323aa.c new file +25 -0

+			  dev_name(data->dev), indio_dev);
+	if (ret) {
+		dev_err(data->dev, "Could not request IRQ for detection (%d)\n",
+			ret);
+		return ret;
+	}
+
+	bitmap_copy(data->regmap, regmap, D3323AA_REG_NR_BITS);
+	data->detecting = true;
+
+	dev_dbg(data->dev, "Setup done\n");
+
+	return 0;
+}
 
+
+static int d3323aa_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	DECLARE_BITMAP(regmap, D3323AA_REG_NR_BITS);
+	int ret;
+
+	guard(mutex)(&data->regmap_lock);
+

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

+		 */
+		return -EINVAL;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = d3323aa_set_lp_filter_freq(regmap, val, val2);
+		if (ret)
+			return ret;
+		break;

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

+}
 
+
+static int d3323aa_probe(struct platform_device *pdev)
+{
+	DECLARE_BITMAP(default_regmap, D3323AA_REG_NR_BITS);
+	struct d3323aa_data *data;
+	struct iio_dev *indio_dev;

drivers/iio/proximity/d3323aa.c new file +45 -0

+	if (IS_ERR(data->gpiod_vdd))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_vdd),
+				     "Could not get GPIO vdd (%ld)\n",
+				     PTR_ERR(data->gpiod_vdd));
+
+	data->gpiod_clk_vout =
+		devm_gpiod_get(data->dev, "clk-vout", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_clk_vout))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_clk_vout),
+				     "Could not get GPIO clk-vout (%ld)\n",
+				     PTR_ERR(data->gpiod_clk_vout));
+
+	data->gpiod_data = devm_gpiod_get(data->dev, "data", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_data))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_data),
+				     "Could not get GPIO data (%ld)\n",
+				     PTR_ERR(data->gpiod_data));
+
+	ret = gpiod_to_irq(data->gpiod_clk_vout);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret, "Could not get IRQ (%d)\n",
+				     ret);
+
+	data->irq = ret;
+
+	/* Do one setup with the default values. */
+	bitmap_zero(default_regmap, D3323AA_REG_NR_BITS);
+	d3323aa_set_threshold(default_regmap, D3323AA_THRESH_DEFAULT_VAL);
+	d3323aa_set_filter_gain(default_regmap,
+				D3323AA_FILTER_GAIN_DEFAULT_VAL);
+	ret = d3323aa_setup(indio_dev, default_regmap);
+	if (ret)
+		return ret;
+
+	indio_dev->info = &d3323aa_info;
+	indio_dev->name = D3323AA_DRV_NAME
+	indio_dev->channels = d3323aa_channels;
+	indio_dev->num_channels = ARRAY_SIZE(d3323aa_channels);
+
+	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);
+

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

+};
+MODULE_DEVICE_TABLE(of, d3323aa_of_match);
+
+static struct platform_driver d3323aa_driver = {
+	.probe = d3323aa_probe,
+	.remove = d3323aa_remove,
+	.driver = {
+		.name = D3323AA_DRV_NAME,
+		.of_match_table = d3323aa_of_match,
+	},
+};
+module_platform_driver(d3323aa_driver);
+
+MODULE_AUTHOR("Waqar Hameed <[email protected]>");
+MODULE_DESCRIPTION("Nicera D3-323-AA PIR sensor driver");
+MODULE_LICENSE("GPL");

Hi Waqar,

Biggest suggestion in here is hide those bitmaps. They are of interest only at point of reading and writing. I wouldn't use them for device state elsewhere as they make for much less readable code than using simple fields in iio_priv() for the state elements and building up the bitmaps only where needed.

Quoted 1 line

+#define D3323AA_DRV_NAME "d3323aa"

Put that inline where used. A define like this both implies that various values must be the same when they need not be and means that we have to go find the define to fine out what they are set to. Just setting the strings directly tends to end up more readable.

Quoted 11 lines

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

+
+/*
+ * Register bitmap.
+ * For some reason the first bit is denoted as F37 in the datasheet, the second
+ * as F38 and so on. Note the gap between F60 and F64.
+ */
+#define D3323AA_REG_BIT_SLAVEA1		0	/* F37. */
+#define D3323AA_REG_BIT_SLAVEA2		1	/* F38. */
+#define D3323AA_REG_BIT_SLAVEA3		2	/* F39. */
+#define D3323AA_REG_BIT_SLAVEA4		3	/* F40. */
+#define D3323AA_REG_BIT_SLAVEA5		4	/* F41. */

drivers/iio/proximity/d3323aa.c line 41

Quoted 4 lines

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

+#define D3323AA_REG_BIT_SLAVEA6		5	/* F42. */
+#define D3323AA_REG_BIT_SLAVEA7		6	/* F43. */
+#define D3323AA_REG_BIT_SLAVEA8		7	/* F44. */
+#define D3323AA_REG_BIT_SLAVEA9		8	/* F45. */

Perhaps these can be represented as masks using GENMASK() rather than bits. A lot of this will be hidden away if you follow suggesting to only expose that you are using a bitmap to bitbang in the read/write functions.

drivers/iio/proximity/d3323aa.c line 148

Quoted 10 lines

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

+/*
+ * This is denoted as "step" in datasheet and corresponds to the gain at peak
+ * for the band-pass filter. The index in this array is the corresponding index
+ * in d3323aa_filter_gain_regval for the register bitmap value.
+ */
+static const int d3323aa_filter_gain[] = {
+	1,
+	2,
+	3,
+};

	1, 2, 3,

is fine for such a short list.

Quoted 16 lines

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

+
+/*
+ * Register bitmap values for the filter gain. The index in this array is the
+ * corresponding index in d3323aa_filter_gain for the gain value.
+ */
+static const u8 d3323aa_filter_gain_regval[] = {
+	1,
+	3,
+	0,
+};
+
+struct d3323aa_data {
+	struct completion reset_completion;
+	/*
+	 *  Since the setup process always requires a complete write of the
+	 *  _whole_ register bitmap, we need to synchronize it with a lock.

drivers/iio/proximity/d3323aa.c line 174

Quoted 10 lines

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

+	 */
+	struct mutex regmap_lock;
+	atomic_t irq_reset_count;
+	unsigned int irq;
+
+	struct device *dev;
+
+	/* Supply voltage. */
+	struct gpio_desc *gpiod_vdd;
+	/* Input clock or output detection signal (Vout). */

I'd rename. Vout kind of suggests a variable voltage. This seems to just be a level signal.

drivers/iio/proximity/d3323aa.c line 175

Quoted 1 line

drivers/iio/proximity/d3323aa.c new file +1 -0

+	struct gpio_desc *gpiod_clk_vout;

Quoted 44 lines

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

+	/* Input (setting) or output data. */
+	struct gpio_desc *gpiod_data;
+
+	DECLARE_BITMAP(regmap, D3323AA_REG_NR_BITS);
+
+	/*
+	 * Indicator for operational mode (configuring or detecting), i.e.
+	 * d3323aa_irq_detection() registered or not.
+	 */
+	bool detecting;
+};
+
+static int d3323aa_read_settings(struct iio_dev *indio_dev,
+				 unsigned long *regmap)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	size_t i;
+	int ret;
+
+	/* Bit bang the clock and data pins. */
+	ret = gpiod_direction_output(data->gpiod_clk_vout, 0);
+	if (ret)
+		return ret;
+
+	ret = gpiod_direction_input(data->gpiod_data);
+	if (ret)
+		return ret;
+
+	dev_dbg(data->dev, "Reading settings...\n");
+
+	for (i = 0; i < D3323AA_REG_NR_BITS; ++i) {
+		/* Clock frequency needs to be 1 kHz. */
+		gpiod_set_value(data->gpiod_clk_vout, 1);
+		udelay(500);
+
+		/* The data seems to change when clock signal is high. */
+		if (gpiod_get_value(data->gpiod_data))
+			set_bit(i, regmap);
+
+		gpiod_set_value(data->gpiod_clk_vout, 0);
+		udelay(500);
+	}
+
+	/* The first bit (F37) is just dummy data. Discard it. */

drivers/iio/proximity/d3323aa.c line 229

Quoted 10 lines

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

+	clear_bit(0, regmap);
+
+	/* Datasheet says to wait 30 ms after reading the settings. */
+	msleep(30);
+
+	return 0;
+}
+
+static int d3323aa_write_settings(struct iio_dev *indio_dev,
+				  const unsigned long *regmap)

Rename regmap. regmap means some specific stuff in the kernel register_bm or something like that avoids that potential confusion.

However, it would be much easier to read this driver if only this function and the read one knew about the bitmap stuff. Inside the reset of the driver just store and pass around a structure with the various fields. Then in here use that to build up the bitmap locally and write to the device. The opposite in read which decodes the bitmap into those fields.

That will make for a more standard and easier to review driver.

Quoted 26 lines

drivers/iio/proximity/d3323aa.c new file +26 -0

+{
+	DECLARE_BITMAP(end_pattern, D3323AA_SETTING_END_PATTERN_NR_BITS);
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	size_t i;
+	int ret;
+
+	/* Bit bang the clock and data pins. */
+	ret = gpiod_direction_output(data->gpiod_clk_vout, 0);
+	if (ret)
+		return ret;
+
+	ret = gpiod_direction_output(data->gpiod_data, 0);
+	if (ret)
+		return ret;
+
+	dev_dbg(data->dev, "Writing settings...\n");
+
+	/* First bit (F37) is not used when writing the register map. */
+	for (i = 1; i < D3323AA_REG_NR_BITS; ++i) {
+		gpiod_set_value(data->gpiod_data, test_bit(i, regmap));
+
+		/* Clock frequency needs to be 1 kHz. */
+		gpiod_set_value(data->gpiod_clk_vout, 1);
+		udelay(500);
+		gpiod_set_value(data->gpiod_clk_vout, 0);
+		udelay(500);

drivers/iio/proximity/d3323aa.c line 260

Quoted 5 lines

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

+	}
+
+	/* Compulsory end pattern. */
+	bitmap_write(end_pattern, D3323AA_SETTING_END_PATTERN, 0,
+		     D3323AA_SETTING_END_PATTERN_NR_BITS);

Why not use a larger bitmap with padding for this end pattern and just write this into that? At which point this can all be one loop.

Quoted 13 lines

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

+	for (i = 0; i < D3323AA_SETTING_END_PATTERN_NR_BITS; ++i) {
+		gpiod_set_value(data->gpiod_data, test_bit(i, end_pattern));
+
+		gpiod_set_value(data->gpiod_clk_vout, 1);
+		udelay(500);
+		gpiod_set_value(data->gpiod_clk_vout, 0);
+		udelay(500);
+	}
+
+	/* Datasheet says to wait 30 ms after writing the settings. */
+	msleep(30);
+
+	return 0;

drivers/iio/proximity/d3323aa.c line 283

Quoted 10 lines

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

+}
+
+static irqreturn_t d3323aa_irq_detection(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	enum iio_event_direction dir;
+	int val;
+
+	val = gpiod_get_value(data->gpiod_clk_vout);

Ideally I'd like a setup where we can wire the interrupt side of this to one pin and the gpio needed for writing to another. In practice they may well be the same pin but that does introduced a bunch of races and dependency on what the interrupt controller does when an irq is disabled.

Using one pin as an irq and as a data line is prone to some nasty corner cases though it works on some SoCs.

Quoted 19 lines

drivers/iio/proximity/d3323aa.c new file +17 -0

+	if (val < 0) {
+		dev_err_ratelimited(data->dev,
+				    "Could not read from GPIO clk-vout (%d)\n",
+				    val);
+		return IRQ_HANDLED;
+	}
+
+	dir = val ? IIO_EV_DIR_RISING : IIO_EV_DIR_FALLING;
+	iio_push_event(indio_dev,
+		       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+					    IIO_EV_TYPE_THRESH, dir),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t d3323aa_irq_reset(int irq, void *dev_id)

drivers/iio/proximity/d3323aa.c line 305

Quoted 5 lines

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

+{
+	struct iio_dev *indio_dev = dev_id;
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	int count = atomic_inc_return(&data->irq_reset_count);
+

As below. It should be easy enough to have a unified handler and avoid the need to unregister / reregister the irq.

Quoted 54 lines

drivers/iio/proximity/d3323aa.c new file +54 -0

+	if (count == D3323AA_IRQ_RESET_COUNT)
+		complete(&data->reset_completion);
+
+	return IRQ_HANDLED;
+}
+
+static int d3323aa_reset(struct iio_dev *indio_dev)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	long time;
+	int ret;
+
+	/*
+	 * Datasheet says VDD needs to be low at least for 30 ms. Let's add a
+	 * couple more to allow VDD to completely discharge as well.
+	 */
+	gpiod_set_value(data->gpiod_vdd, 0);
+	msleep(30 + 5);
+
+	/*
+	 * After setting VDD to high, the device signals with
+	 * D3323AA_IRQ_RESET_COUNT falling edges on CLK/Vout that it is now
+	 * ready for configuration. Datasheet says that this should happen
+	 * within D3323AA_RESET_TIMEOUT ms. Count these two edges within that
+	 * timeout.
+	 */
+	atomic_set(&data->irq_reset_count, 0);
+	reinit_completion(&data->reset_completion);
+
+	ret = gpiod_direction_input(data->gpiod_clk_vout);
+	if (ret)
+		return ret;
+
+	dev_dbg(data->dev, "Resetting...\n");
+
+	gpiod_set_value(data->gpiod_vdd, 1);
+
+	/*
+	 * Datasheet doesn't mention this but measurements have shown that
+	 * CLK/Vout signal slowly ramps up during the first 1.5 ms after reset.
+	 * This means that the digital signal will have bogus values during this
+	 * period. Let's wait for this ramp-up before counting the "real"
+	 * falling edges.
+	 */
+	usleep_range(2000, 5000);
+
+	if (data->detecting) {
+		/*
+		 * Device had previously been set up and was in operational
+		 * mode. Thus, free that detection IRQ handler before requesting
+		 * the reset IRQ handler.
+		 */
+		free_irq(data->irq, indio_dev);
+		data->detecting = false;

drivers/iio/proximity/d3323aa.c line 363

Quoted 4 lines

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

+	}
+
+	ret = request_irq(data->irq, d3323aa_irq_reset, IRQF_TRIGGER_FALLING,
+			  dev_name(data->dev), indio_dev);

Can you keep to a single handler with a check on device state? This dance with releasing and requesting irqs is rather complex when all you need to know is what the irq means and that is state the driver knows.

Quoted 78 lines

drivers/iio/proximity/d3323aa.c new file +78 -0

+	if (ret)
+		return ret;
+
+	time = wait_for_completion_killable_timeout(
+		&data->reset_completion,
+		msecs_to_jiffies(D3323AA_RESET_TIMEOUT));
+	free_irq(data->irq, indio_dev);
+	if (time == 0) {
+		return -ETIMEDOUT;
+	} else if (time < 0) {
+		/* Got interrupted. */
+		return time;
+	}
+
+	dev_dbg(data->dev, "Reset completed\n");
+
+	return 0;
+}
+
+static int d3323aa_setup(struct iio_dev *indio_dev, const unsigned long *regmap)
+{
+	DECLARE_BITMAP(read_regmap, D3323AA_REG_NR_BITS);
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	unsigned long start_time;
+	int ret;
+
+	ret = d3323aa_reset(indio_dev);
+	if (ret) {
+		if (ret != -ERESTARTSYS)
+			dev_err(data->dev, "Could not reset device (%d)\n",
+				ret);
+
+		return ret;
+	}
+
+	/*
+	 * Datasheet says to wait 10 us before setting the configuration.
+	 * Moreover, the total configuration should be done within
+	 * D3323AA_CONFIG_TIMEOUT ms. Clock it.
+	 */
+	usleep_range(10, 20);
+	start_time = jiffies;
+
+	ret = d3323aa_write_settings(indio_dev, regmap);
+	if (ret) {
+		dev_err(data->dev, "Could not write settings (%d)\n", ret);
+		return ret;
+	}
+
+	ret = d3323aa_read_settings(indio_dev, read_regmap);
+	if (ret) {
+		dev_err(data->dev, "Could not read settings (%d)\n", ret);
+		return ret;
+	}
+
+	if (time_is_before_jiffies(start_time +
+				   msecs_to_jiffies(D3323AA_CONFIG_TIMEOUT))) {
+		dev_err(data->dev, "Could not set up configuration in time\n");
+		return -EAGAIN;
+	}
+
+	/* Check if settings were set successfully. */
+	if (!bitmap_equal(regmap, read_regmap, D3323AA_REG_NR_BITS)) {
+		dev_err(data->dev, "Settings data mismatch\n");
+		return -EIO;
+	}
+
+	/* Now in operational mode. */
+	ret = gpiod_direction_input(data->gpiod_clk_vout);
+	if (ret) {
+		dev_err(data->dev,
+			"Could not set GPIO clk-vout as input (%d)\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_direction_input(data->gpiod_data);
+	if (ret) {
+		dev_err(data->dev, "Could not set GPIO data as input (%d)\n",

drivers/iio/proximity/d3323aa.c line 447

Quoted 6 lines

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

+			ret);
+		return ret;
+	}
+
+	ret = request_irq(data->irq, d3323aa_irq_detection,
+			  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,

Why both ways? Add a comment.

Quoted 27 lines

drivers/iio/proximity/d3323aa.c new file +25 -0

+			  dev_name(data->dev), indio_dev);
+	if (ret) {
+		dev_err(data->dev, "Could not request IRQ for detection (%d)\n",
+			ret);
+		return ret;
+	}
+
+	bitmap_copy(data->regmap, regmap, D3323AA_REG_NR_BITS);
+	data->detecting = true;
+
+	dev_dbg(data->dev, "Setup done\n");
+
+	return 0;
+}
 
+
+static int d3323aa_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	DECLARE_BITMAP(regmap, D3323AA_REG_NR_BITS);
+	int ret;
+
+	guard(mutex)(&data->regmap_lock);
+

drivers/iio/proximity/d3323aa.c

Quoted 10 lines

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

+	bitmap_copy(regmap, data->regmap, D3323AA_REG_NR_BITS);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		/*
+		 * We only allow to set the low-pass cutoff frequency, since
+		 * that is the only way to unambigously choose the type of
+		 * band-pass filter. For example, both filter type B and C have
+		 * 0.3 Hz as high-pass cutoff frequency (see
+		 * d3323aa_hp_filter_freq).

This is somewhat unintiutive userspace inteface. I'd try and do the best possible in response to a userspace request. So if the lp frequency is already set to one of the values that matches requested hp then use that. If it's not then pick the one that has best match lp (so nearest) on assumption userspace is probably about to update the lp side anyway.

Slight fiddlier code than you have here, but a better attempt to figure out what the users is after than rejecting writes.

Quoted 12 lines

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

+		 */
+		return -EINVAL;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = d3323aa_set_lp_filter_freq(regmap, val, val2);
+		if (ret)
+			return ret;
+		break;
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		ret = d3323aa_set_filter_gain(regmap, val);
+		if (ret)
+			return ret;
+		break;

drivers/iio/proximity/d3323aa.c line 680

Quoted 5 lines

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

+	default:
+		return -EINVAL;
+	}
+
+	return d3323aa_setup(indio_dev, regmap);

I'd bury this call in set_lp_filter() and set_fillter_gain() so they actually do the setting.

Quoted 13 lines

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

+}
 
+
+static int d3323aa_probe(struct platform_device *pdev)
+{
+	DECLARE_BITMAP(default_regmap, D3323AA_REG_NR_BITS);
+	struct d3323aa_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
+	if (!indio_dev)

drivers/iio/proximity/d3323aa.c line 780

Quoted 6 lines

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

+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "Could not allocate iio device\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = &pdev->dev;
+	platform_set_drvdata(pdev, indio_dev);

Should be unnecessary with the suggested change below.

drivers/iio/proximity/d3323aa.c line 787

Quoted 8 lines

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

+
+	init_completion(&data->reset_completion);
+
+	ret = devm_mutex_init(data->dev, &data->regmap_lock);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not initialize mutex (%d)\n", ret);

As already pointed out in other review - look at what dev_err_probe() does.

drivers/iio/proximity/d3323aa.c line 789

Quoted 3 lines

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

+
+	/* Request GPIOs. */

This sort of code structure comment adds nothing useful. We can see you are requesting gpios from the code. As such, all it can do is become wrong in future if code is reorganised. Hence drop the comment.

drivers/iio/proximity/d3323aa.c line 790

Quoted 1 line

drivers/iio/proximity/d3323aa.c new file +1 -0

+	data->gpiod_vdd = devm_gpiod_get(data->dev, "vdd", GPIOD_OUT_LOW);

This seems to be a supply, not a gpio. Use the regulator framework. Make sure to request exclusive use though so you can turn it on and off. _get_exclusive() etc should ensure no one else is using it.

Quoted 5 lines

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

+	if (IS_ERR(data->gpiod_vdd))
+		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_vdd),
+				     "Could not get GPIO vdd (%ld)\n",
+				     PTR_ERR(data->gpiod_vdd));
+

drivers/iio/proximity/d3323aa.c line 845

Quoted 7 lines

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

+static void d3323aa_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct d3323aa_data *data = iio_priv(indio_dev);
+
+	if (data->detecting)
+		free_irq(data->irq, indio_dev);

So this is an ordering mess. You should never mix devm handling and non devm handling - once something happens in probe that is non devm you stop using devm_* from that point on.

The irq handling in here is complex, but not on fast paths. As such I'd do devm_irq_request() and if you need to free it do so with devm_free_irq() in the places where you immediately re request it. However see above. That should allow you to to drop this remove callabck.

drivers/iio/proximity/d3323aa.c line 852

Quoted 7 lines

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

+}
+
+static const struct of_device_id d3323aa_of_match[] = {
+	{
+		.compatible = "nicera,d3323aa",
+	},
+	{}

	{ }

Is now standard format for these in IIO.

drivers/iio/proximity/d3323aa.c line 854

Quoted 2 lines

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

+};
+MODULE_DEVICE_TABLE(of, d3323aa_of_match);

Quoted 7 lines

Quoted 2 lines

+#define D3323AA_DRV_NAME "d3323aa"

Put that inline where used. A define like this both implies that various values must be the same when they need not be and means that we have to go find the define to fine out what they are set to. Just setting the strings directly tends to end up more readable.

Sure, we can do that. (There are a bunch of IIO-drivers doing this, so I just thought that was the "convention".)

Quoted 7 lines

drivers/iio/proximity/d3323aa.c line 174

Quoted 2 lines

drivers/iio/proximity/d3323aa.c new file +1 -0

+	/* Input clock or output detection signal (Vout). */  

I'd rename. Vout kind of suggests a variable voltage. This seems to just be a level signal.

drivers/iio/proximity/d3323aa.c line 175

Quoted 1 line

drivers/iio/proximity/d3323aa.c new file +1 -0

+	struct gpio_desc *gpiod_clk_vout;  

Yeah, it's a weird pin with multiple use-cases... I just named it according to what the datasheet calls it. What about gpiod_clk_detection?

Quoted 6 lines

drivers/iio/proximity/d3323aa.c line 229

Quoted 2 lines

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

+static int d3323aa_write_settings(struct iio_dev *indio_dev,
+				  const unsigned long *regmap)  

Rename regmap. regmap means some specific stuff in the kernel register_bm or something like that avoids that potential confusion.

Quoted 7 lines

However, it would be much easier to read this driver if only this function and the read one knew about the bitmap stuff. Inside the reset of the driver just store and pass around a structure with the various fields. Then in here use that to build up the bitmap locally and write to the device. The opposite in read which decodes the bitmap into those fields.

That will make for a more standard and easier to review driver.

Hm, maybe it would be easier to understand it if we remove the bitmap from struct d3323aa_data and instead only store the relevant state variables. We still would need to have the regmap_lock but to instead protect all of the variables.

Let's go for that then! I actually don't have any strong arguments (or opinions) against it.

Quoted 15 lines

drivers/iio/proximity/d3323aa.c line 283

Quoted 8 lines

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

+static irqreturn_t d3323aa_irq_detection(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	enum iio_event_direction dir;
+	int val;
+
+	val = gpiod_get_value(data->gpiod_clk_vout);  

Ideally I'd like a setup where we can wire the interrupt side of this to one pin and the gpio needed for writing to another. In practice they may well be the same pin but that does introduced a bunch of races and dependency on what the interrupt controller does when an irq is disabled.

Quoted 2 lines

Using one pin as an irq and as a data line is prone to some nasty corner cases though it works on some SoCs.

Hm, that's an interesting idea. However, as you say there might then be other issues to take care of.

On the other hand, the scope of the clock bit bang is quite small (only in *read/write_settings()). So I guess we just leave it then?

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

+#define D3323AA_DRV_NAME "d3323aa"

Put that inline where used. A define like this both implies that various values must be the same when they need not be and means that we have to go find the define to fine out what they are set to. Just setting the strings directly tends to end up more readable.

Quoted 2 lines

Sure, we can do that. (There are a bunch of IIO-drivers doing this, so I just thought that was the "convention".)

I'm sometimes in less fussy mood. One day I might just clean those up so there is nothing to copy into new drivers!

Quoted 6 lines

Quoted 5 lines

drivers/iio/proximity/d3323aa.c line 174

Quoted 2 lines

drivers/iio/proximity/d3323aa.c new file +1 -0

+	/* Input clock or output detection signal (Vout). */  

I'd rename. Vout kind of suggests a variable voltage. This seems to just be a level signal.

Quoted 5 lines

Quoted 1 line

drivers/iio/proximity/d3323aa.c line 175

Quoted 1 line

drivers/iio/proximity/d3323aa.c new file +1 -0

+	struct gpio_desc *gpiod_clk_vout;  

Yeah, it's a weird pin with multiple use-cases... I just named it according to what the datasheet calls it. What about gpiod_clk_detection?

That sounds like it's detecting a clock. Hmm.
gpiod_clkin_detectout maybe?

Quoted 7 lines · On Sun, May 18, 2025 at 18:38 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

+#define D3323AA_DRV_NAME "d3323aa"

Put that inline where used. A define like this both implies that various values must be the same when they need not be and means that we have to go find the define to fine out what they are set to. Just setting the strings directly tends to end up more readable.

Quoted 5 lines

Quoted 2 lines

Sure, we can do that. (There are a bunch of IIO-drivers doing this, so I just thought that was the "convention".)

I'm sometimes in less fussy mood. One day I might just clean those up so there is nothing to copy into new drivers!

A quick search tells that there are (at least) 105 of those:

rgrep -A 30 "\.driver" drivers/iio/ | grep "\.name" | grep -v '"'

I was just about to write a small Python script to fix those, but just wanted to confirm with you before spending more time on this. So if you don't want to do this yourself, I can help your here :)

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

drivers/iio/proximity/d3323aa.c line 174

Quoted 3 lines

drivers/iio/proximity/d3323aa.c new file +1 -0

+	/* Input clock or output detection signal (Vout). */    

I'd rename. Vout kind of suggests a variable voltage. This seems to just be a level signal.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

drivers/iio/proximity/d3323aa.c line 175

Quoted 1 line

drivers/iio/proximity/d3323aa.c new file +1 -0

+	struct gpio_desc *gpiod_clk_vout;    

Yeah, it's a weird pin with multiple use-cases... I just named it according to what the datasheet calls it. What about gpiod_clk_detection?

That sounds like it's detecting a clock. Hmm.
gpiod_clkin_detectout maybe?

No objections.

[...]

Quoted 19 lines · On Tue, 20 May 2025 13:27:54 +0200 Waqar Hameed <[email protected]> wrote:

Quoted 7 lines · On Sun, May 18, 2025 at 18:38 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

+#define D3323AA_DRV_NAME "d3323aa"

Put that inline where used. A define like this both implies that various values must be the same when they need not be and means that we have to go find the define to fine out what they are set to. Just setting the strings directly tends to end up more readable.

Quoted 5 lines

Quoted 2 lines

Sure, we can do that. (There are a bunch of IIO-drivers doing this, so I just thought that was the "convention".)

I'm sometimes in less fussy mood. One day I might just clean those up so there is nothing to copy into new drivers!

A quick search tells that there are (at least) 105 of those:

rgrep -A 30 "\.driver" drivers/iio/ | grep "\.name" | grep -v '"'

Quoted 3 lines

I was just about to write a small Python script to fix those, but just wanted to confirm with you before spending more time on this. So if you don't want to do this yourself, I can help your here :)

It's probably not worth the churn on the ones that have the string repeated multiple times. However, perhaps any that are only using it for .name would be good to tidy up? Those are less a case of it being 'taste' vs it being silly to have a define!

Quoted 22 lines · On Sun, May 25, 2025 at 10:30 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 19 lines · On Tue, 20 May 2025 13:27:54 +0200 Waqar Hameed <[email protected]> wrote:

Quoted 7 lines · On Sun, May 18, 2025 at 18:38 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

+#define D3323AA_DRV_NAME "d3323aa"

Put that inline where used. A define like this both implies that various values must be the same when they need not be and means that we have to go find the define to fine out what they are set to. Just setting the strings directly tends to end up more readable.

Quoted 5 lines

Quoted 2 lines

Sure, we can do that. (There are a bunch of IIO-drivers doing this, so I just thought that was the "convention".)

I'm sometimes in less fussy mood. One day I might just clean those up so there is nothing to copy into new drivers!

A quick search tells that there are (at least) 105 of those:

rgrep -A 30 "\.driver" drivers/iio/ | grep "\.name" | grep -v '"'

Quoted 8 lines

Quoted 3 lines

I was just about to write a small Python script to fix those, but just wanted to confirm with you before spending more time on this. So if you don't want to do this yourself, I can help your here :)

It's probably not worth the churn on the ones that have the string repeated multiple times. However, perhaps any that are only using it for .name would be good to tidy up? Those are less a case of it being 'taste' vs it being silly to have a define!

I think if you use it in multiple places, it should definitively be a macro definition. I just sent some patches for those that only used it once (I didn't include those with KBUILD_MODNAME. We can discuss if we should also address those in that thread).

However, there are a bunch of drivers that _only_ use a macro definition in .name and indio_dev->name, including this one. That _is_ more than one place, so we should actually leave it? Or do you still think we should have the same string literal in both places, as you originally commented above?

Quoted 4 lines

I think if you use it in multiple places, it should definitively be a macro definition. I just sent some patches for those that only used it once (I didn't include those with KBUILD_MODNAME. We can discuss if we should also address those in that thread).

I would disagree slightly. If it is used in multiple places because there is some inherent reason they should have same string then I absolutely agree. If it's just because it's a convenient string that is used twice in places that could have had different string then not so much.

Quoted 5 lines

However, there are a bunch of drivers that _only_ use a macro definition in .name and indio_dev->name, including this one. That _is_ more than one place, so we should actually leave it? Or do you still think we should have the same string literal in both places, as you originally commented above?

I'd prefer that for new code, but it is a less clear cut case than the ones you have tidied up, so not worth the churn of tidying up unless people are otherwise working on the relevant drivers.

On Sun, May 11, 2025 at 13:14 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 11 lines

Quoted 11 lines

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

+
+/*
+ * Register bitmap.
+ * For some reason the first bit is denoted as F37 in the datasheet, the second
+ * as F38 and so on. Note the gap between F60 and F64.
+ */
+#define D3323AA_REG_BIT_SLAVEA1		0	/* F37. */
+#define D3323AA_REG_BIT_SLAVEA2		1	/* F38. */
+#define D3323AA_REG_BIT_SLAVEA3		2	/* F39. */
+#define D3323AA_REG_BIT_SLAVEA4		3	/* F40. */
+#define D3323AA_REG_BIT_SLAVEA5		4	/* F41. */

Quoted 8 lines

drivers/iio/proximity/d3323aa.c line 41

Quoted 4 lines

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

+#define D3323AA_REG_BIT_SLAVEA6		5	/* F42. */
+#define D3323AA_REG_BIT_SLAVEA7		6	/* F43. */
+#define D3323AA_REG_BIT_SLAVEA8		7	/* F44. */
+#define D3323AA_REG_BIT_SLAVEA9		8	/* F45. */

Perhaps these can be represented as masks using GENMASK() rather than bits. A lot of this will be hidden away if you follow suggesting to only expose that you are using a bitmap to bitbang in the read/write functions.

Yes, that would be the natural thing to do when moving the bitmap stuff to the read/write functions (as answered below).

Nicera D3-323-AA is a PIR sensor for human detection. It has support for raw data measurements and detection notification. The communication protocol is custom made and therefore needs to be GPIO bit banged.

Previously, there has been an attempt to add a driver for this device [1]. However, that driver was written for the wrong sub-system. hwmon

is clearly not a suitable framework for a proximity device.

In this series, we add a driver for support for event notification for detections through IIO (the more appropriate sub-system!). The various settings have been mapped to existing sysfs ABIs in the IIO framework.

The public datasheet [2] is quite sparse. A more detailed version can be obtained through the company.

[1] https://lore.kernel.org/lkml/[email protected]/

Quoted 6 lines · Waqar Hameed

Nicera D3-323-AA is a PIR sensor for human detection. It has support for raw data measurements and detection notification. The communication protocol is custom made and therefore needs to be GPIO bit banged.

Previously, there has been an attempt to add a driver for this device [1]. However, that driver was written for the wrong sub-system. hwmon

So that's a v2. Mark your patches correctly.

Quoted 10 lines

is clearly not a suitable framework for a proximity device.

In this series, we add a driver for support for event notification for detections through IIO (the more appropriate sub-system!). The various settings have been mapped to existing sysfs ABIs in the IIO framework.

The public datasheet [2] is quite sparse. A more detailed version can be obtained through the company.

Quoted 1 line

[1] https://lore.kernel.org/lkml/[email protected]/

Read the comments given in that review:
https://lore.kernel.org/lkml/wy7nyg3cztixe5y5rg4kbsbbly32h547hwumwwvrfme4fdgsj5@znfpypleebrb/

You repeated same mistakes, which means I did same review second time which is waste of my time.