Skip to contentSkip to conversation

Add driver for Nicera D3-323-AA PIR sensor

PATCH Superseded Waqar Hameed 3 participants 13 messages Jun 14, 2025 – Jun 30, 2025

Subsystems (2)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

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

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]/
[2] https://www.endrich.com/Datenbl%C3%A4tter/Sensoren/D3-323-AA_e.pdf

Changes in v2:

[dt-bindings]
* Convert vdd-gpio to a vdd-supply.
* Rename clk-vout-gpio to vout-clk-gpios.
* Add description for data-gpios explaining the rename to a more descriptive name.
* Drop all references to driver.
* Remove unused gpio include in examples.
* Re-phrase commit message to only describe the hardware.

[iio]
* Add newline after variable definitions inside the for-loop in d3323aa_set_lp_filter_freq().
* Remove error code in string in dev_err_probe().
* Remove driver name macro and use it inline instead.
* Format filter gain arrays into one line.
* Drop structure comment in probe().
* Format sentinel value in of_device_id with a space.
* Rename gpiod_clk_vout to gpiod_clkin_detectout.
* Request vout-clk GPIO to match rename in dt-bindings.
* Use the regulator framework for supply voltage.
* Use only one IRQ handler for both reset and detection.
* Reword comment about Vout/CLK ramp-up behavior (it's because of VDD charging up).
* Add comment for why we have both IRQF_TRIGGER_RISING and IRQF_TRIGGER_FALLING.
* Rename regmap to regbitmap to not confuse with the regmap-framework.
* Move d3323aa_setup() into the set-functions.
* Use state variables in d3323aa_data instead of bitmap and move bitmap handling to read/write settings functions.
* Pad bitmap with compulsory end pattern in d3323aa_write_settings().
* Add d3323aa_set_hp_filter_freq() and allow userspace to set it.

Link to v1: https://lore.kernel.org/lkml/[email protected]/

Waqar Hameed (3):
dt-bindings: vendor-prefixes: Add Nicera
dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor
iio: Add driver for Nicera D3-323-AA PIR sensor

 .../iio/proximity/nicera,d3323aa.yaml         |  60 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/iio/proximity/Kconfig                 |   9 +
 drivers/iio/proximity/Makefile                |   1 +
 drivers/iio/proximity/d3323aa.c               | 808 ++++++++++++++++++
 5 files changed, 880 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml
 create mode 100644 drivers/iio/proximity/d3323aa.c

base-commit: 5abc7438f1e9d62e91ad775cc83c9594c48d2282

Signature · 1 line
2.39.5
#1 lore raw

PATCH v2 1/3 dt-bindings: vendor-prefixes: Add Nicera

Nicera (Nippon Ceramic Co.) is a manufacturer of a wide range of sensors. For example infrared, ultrasonic, gas sensors and much more.

---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/vendor-prefixes.yaml +2 -0
index c01adbaacbbb..b41cf7ba1b04 100644
@@ -1067,6 +1067,8 @@ patternProperties:
     description: Next Thing Co.
   "^ni,.*":
     description: National Instruments
+  "^nicera,.*":
+    description: Nippon Ceramic Co., Ltd.
   "^nintendo,.*":
     description: Nintendo
   "^nlt,.*":
Signature · 1 line
2.39.5
Acked-by on v2 1/3
#2 lore raw

Re: [PATCH v2 1/3] dt-bindings: vendor-prefixes: Add Nicera

Quoted 7 lines · Waqar Hameed

Nicera (Nippon Ceramic Co.) is a manufacturer of a wide range of sensors. For example infrared, ultrasonic, gas sensors and much more.

---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

Best regards,
Krzysztof

#3 lore raw

PATCH v2 2/3 dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor

In reply to Waqar Hameed

Nicera D3-323-AA is a PIR sensor for human detection. It has two GPIOs for detection and data communication.

---
 .../iio/proximity/nicera,d3323aa.yaml         | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml
Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +60 -0
new file mode 100644
index 000000000000..01c084de96c3
@@ -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/nicera,d3323aa.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nicera D3-323-AA PIR sensor
+
+maintainers:
+  - Waqar Hameed <[email protected]>
+
+description: |
+  PIR sensor for human detection.
+  Datasheet: https://www.endrich.com/Datenbl%C3%A4tter/Sensoren/D3-323-AA_e.pdf
+
+properties:
+  compatible:
+    const: nicera,d3323aa
+
+  vdd-supply:
+    description:
+      Supply voltage (1.8 to 5.5 V).
+
+  vout-clk-gpios:
+    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).
+      During configuration, it is used as clock for data reading and writing (on
+      data-gpios).
+      After all this, when device is in operational mode, it signals on this pin
+      for any detections.
+
+  data-gpios:
+    maxItems: 1
+    description:
+      GPIO for data reading and writing. This is denoted "DO (SI)" in datasheet.
+      During configuration, this pin is used for writing and reading
+      configuration data (together with vout-clk-gpios as clock).
+      After this, during operational mode, the device will output serial data on
+      this GPIO.
+
+required:
+  - compatible
+  - vdd-supply
+  - vout-clk-gpios
+  - data-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    proximity {
+        compatible = "nicera,d3323aa";
+        vdd-supply = <&regulator_3v3>;
+        vout-clk-gpios = <&gpio 78 0>;
+        data-gpios = <&gpio 76 0>;
+    };
+...
Signature · 1 line
2.39.5
#4 lore raw

Re: [PATCH v2 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor

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

Quoted 7 lines · Waqar Hameed
Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +7 -0
+examples:
+  - |
+    proximity {
+        compatible = "nicera,d3323aa";
+        vdd-supply = <&regulator_3v3>;
+        vout-clk-gpios = <&gpio 78 0>;
+        data-gpios = <&gpio 76 0>;

Same comment as before.

Quoted 5 lines

+ };
+...

Signature · 1 line
2.39.5
#5 lore raw

Re: [PATCH v2 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor

Quoted 10 lines · On Tue, Jun 17, 2025 at 11:06 +0200 Krzysztof Kozlowski <[email protected]> wrote:

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

Quoted 7 lines · Waqar Hameed
Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +7 -0
+examples:
+  - |
+    proximity {
+        compatible = "nicera,d3323aa";
+        vdd-supply = <&regulator_3v3>;
+        vout-clk-gpios = <&gpio 78 0>;
+        data-gpios = <&gpio 76 0>;

Same comment as before.

I removed the include header thinking that would be fine, since there were already a bunch of other examples just using 0 as the GPIO flag (without the include). I'll re-add the include header in next version then and use GPIO_ACTIVE_HIGH on both -gpios.

#6 lore raw

PATCH v2 3/3 iio: Add driver for Nicera D3-323-AA PIR sensor

In reply to 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.

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.

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

---
 drivers/iio/proximity/Kconfig   |   9 +
 drivers/iio/proximity/Makefile  |   1 +
 drivers/iio/proximity/d3323aa.c | 808 ++++++++++++++++++++++++++++++++
 3 files changed, 818 insertions(+)
 create mode 100644 drivers/iio/proximity/d3323aa.c
drivers/iio/proximity/Kconfig +9 -0
index a562a78b7d0d..6070974c2c85 100644
@@ -32,6 +32,15 @@ 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 D3323AA
+	tristate "Nicera (Nippon Ceramic Co.) D3-323-AA PIR sensor"
+	depends on GPIOLIB
+	help
+	  Say Y here to build a driver for the Nicera D3-323-AA PIR sensor.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called d3323aa.
+
 config HX9023S
 	tristate "TYHX HX9023S SAR sensor"
 	select IIO_BUFFER
drivers/iio/proximity/Makefile +1 -0
index c5e76995764a..152034d38c49 100644
@@ -6,6 +6,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AS3935)		+= as3935.o
 obj-$(CONFIG_CROS_EC_MKBP_PROXIMITY) += cros_ec_mkbp_proximity.o
+obj-$(CONFIG_D3323AA)		+= d3323aa.o
 obj-$(CONFIG_HX9023S)		+= hx9023s.o
 obj-$(CONFIG_IRSD200)		+= irsd200.o
 obj-$(CONFIG_ISL29501)		+= isl29501.o
drivers/iio/proximity/d3323aa.c new file +808 -0

808 lines not on this page · raw message

Signature · 1 line
2.39.5
maintainer
#7 lore raw

Re: [PATCH v2 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor

Quoted 10 lines · On Sun, 15 Jun 2025 00:14:05 +0200 Waqar Hameed <[email protected]> wrote:

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.

Quoted 8 lines

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

A few minor comments inline given you are doing another version for the DT change.

Quoted 17 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
new file mode 100644
index 000000000000..71bccca75abd
@@ -0,0 +1,808 @@
 
+
+static irqreturn_t d3323aa_irq_handler(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_clkin_detectout);
+	if (val < 0) {

drivers/iio/proximity/d3323aa.c line 299

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+		dev_err_ratelimited(data->dev,
+				    "Could not read from GPIO vout-clk (%d)\n",
+				    val);
+		return IRQ_HANDLED;
+	}
+
+	if (!data->detecting) {
+		/* Reset interrupt counting falling edges. */
+		if (!val && atomic_inc_return(&data->irq_reset_count) ==
+				    D3323AA_IRQ_RESET_COUNT)

Odd line break

		if (!val &&
		    atomic_inc_return(&data->irq_reset_count) == D3323AA_IRQ_RESET_COUNT)

Is a bit better though rather long line. I'm not completely clear on why it needs to be an atomic counter though as the comment in reset() to to imply this can't race with the zeroing and no one else touches it.

Quoted 12 lines
drivers/iio/proximity/d3323aa.c new file +12 -0
+			complete(&data->reset_completion);
+
+		return IRQ_HANDLED;
+	}
+
+	/* Detection interrupt. */
+	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 line 321

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+	return IRQ_HANDLED;
+}
+
+static int d3323aa_reset(struct iio_dev *indio_dev)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	long time;
+	int ret;
+
+	if (regulator_is_enabled(data->regulator_vdd)) {

Add a comment to say this check is only for the use in probe() where the power may not be on yet. In other paths I think it will always be on.

drivers/iio/proximity/d3323aa.c line 331

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+		ret = regulator_disable(data->regulator_vdd);
+		if (ret)
+			return 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.
+	 */
+	msleep(30 + 5);

Use fsleep here as well with the parameter value in usecs. It will use msleep under the hood but we don't need to enforce that in the driver. On some particular platform some other sleep call may be more suited.

drivers/iio/proximity/d3323aa.c line 334

Quoted 3 lines
drivers/iio/proximity/d3323aa.c new file +3 -0
+
+	/*
+	 * After setting VDD to high, the device signals with

This is a little odd. vdd, as the regulator is currently low as you powered it down. This is referring I think to what happens later. Edit the comment to make that clearer. Someting like * When VDD is later enabled...

Quoted 56 lines
drivers/iio/proximity/d3323aa.c new file +56 -0
+	 * D3323AA_IRQ_RESET_COUNT falling edges on Vout/CLK 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);
+	data->detecting = false;
+
+	ret = gpiod_direction_input(data->gpiod_clkin_detectout);
+	if (ret)
+		return ret;
+
+	dev_dbg(data->dev, "Resetting...\n");
+
+	ret = regulator_enable(data->regulator_vdd);
+	if (ret)
+		return ret;
+
+	/*
+	 * Wait for VDD to completely charge up. Measurements have shown that
+	 * Vout/CLK signal slowly ramps up during this period. Thus, the digital
+	 * signal will have bogus values. It is therefore necessary to wait
+	 * before we can count the "real" falling edges.
+	 */
+	usleep_range(2000, 5000);
+
+	time = wait_for_completion_killable_timeout(
+		&data->reset_completion,
+		msecs_to_jiffies(D3323AA_RESET_TIMEOUT));
+	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, size_t lp_filter_freq_idx,
+			 size_t filter_gain_idx, u8 detect_thresh)
+{
+	DECLARE_BITMAP(write_regbitmap, D3323AA_REG_NR_BITS);
+	DECLARE_BITMAP(read_regbitmap, 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);

drivers/iio/proximity/d3323aa.c line 400

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+
+		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);

fsleep() preferred as that generalizes the tolerances fed to usleep_range() so we don't have to think if they are appropriate in each call.

Quoted 63 lines
drivers/iio/proximity/d3323aa.c new file +63 -0
+	start_time = jiffies;
+
+	ret = d3323aa_write_settings(indio_dev, write_regbitmap);
+	if (ret) {
+		dev_err(data->dev, "Could not write settings (%d)\n", ret);
+		return ret;
+	}
+
+	ret = d3323aa_read_settings(indio_dev, read_regbitmap);
+	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(write_regbitmap, read_regbitmap,
+			  D3323AA_REG_NR_BITS)) {
+		dev_err(data->dev, "Settings data mismatch\n");
+		return -EIO;
+	}
+
+	/* Now in operational mode. */
+	ret = gpiod_direction_input(data->gpiod_clkin_detectout);
+	if (ret) {
+		dev_err(data->dev,
+			"Could not set GPIO vout-clk 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",
+			ret);
+		return ret;
+	}
+
+	data->lp_filter_freq_idx = lp_filter_freq_idx;
+	data->filter_gain_idx = filter_gain_idx;
+	data->detect_thresh = detect_thresh;
+	data->detecting = true;
+
+	dev_dbg(data->dev, "Setup done\n");
+
+	return 0;
+}
+
+static int d3323aa_set_lp_filter_freq(struct iio_dev *indio_dev, const int val,
+				      int val2)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	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] /

drivers/iio/proximity/d3323aa.c line 473

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+			      d3323aa_lp_filter_freq[idx][1];
+		int fract = d3323aa_lp_filter_freq[idx][0] %
+			    d3323aa_lp_filter_freq[idx][1];
+
+		if (val == integer && val2 == fract)
+			break;
+	}
+
+	if (idx == ARRAY_SIZE(d3323aa_lp_filter_freq))
+		return -ERANGE;

It's a patch not a range check, so -EINVAL may make more sense as a return value.

Quoted 4 lines
drivers/iio/proximity/d3323aa.c new file +4 -0
+
+	return d3323aa_setup(indio_dev, idx, data->filter_gain_idx,
+			     data->detect_thresh);
+}

drivers/iio/proximity/d3323aa.c line 705

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +9 -0
 
+
+static void d3323aa_disable_regulator(void *indata)
+{
+	struct d3323aa_data *data = indata;
+	int ret;
+
+	if (!regulator_is_enabled(data->regulator_vdd))
+		return;
+

This is unusual and I think an artefact of where you are registering the devm callback and the use of reset later. Needs a comments at the very least to explain why the check is needed.

Quoted 4 lines
drivers/iio/proximity/d3323aa.c new file +4 -0
+	ret = regulator_disable(data->regulator_vdd);
+	if (ret)
+		dev_err(data->dev, "Could not disable regulator (%d)\n", ret);
+}

drivers/iio/proximity/d3323aa.c line 719

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+
+static int d3323aa_probe(struct platform_device *pdev)
+{
+	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,

Given there is a lot of use of either pdev->dev or data->dev which is the same thing, I'd suggest a local declaration at the top and use that throughout.

	struct device *dev = &pdev->dev;
Quoted 8 lines
drivers/iio/proximity/d3323aa.c new file +8 -0
+				     "Could not allocate iio device\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = &pdev->dev;
+
+	init_completion(&data->reset_completion);
+
+	ret = devm_mutex_init(data->dev, &data->statevar_lock);

drivers/iio/proximity/d3323aa.c line 737

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not initialize mutex\n");
+
+	data->regulator_vdd = devm_regulator_get_exclusive(data->dev, "vdd");
+	if (IS_ERR(data->regulator_vdd))
+		return dev_err_probe(data->dev, PTR_ERR(data->regulator_vdd),
+				     "Could not get regulator\n");
+
+	ret = devm_add_action_or_reset(data->dev, d3323aa_disable_regulator,

This is in a slightly odd place as you haven't turned it on yet. At very least add a comment.

drivers/iio/proximity/d3323aa.c line 742

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+				       data);
+	if (ret)
+		return dev_err_probe(
+			data->dev, ret,
+			"Could not add disable regulator action\n");

Odd formatting.

		return dev_err_probe(dev, ret,
				     "Could not add disable regulator action\n");

It's fine to go a little over 80 chars if it helps readability and here I think it does. However it is vanishingly unlikely this would fail (as it basically means memory allocation is failing in which case not much is going to work) so common practice is not to bother with prints for failed devm_add_action_or_reset(). Those prints do make sense for devm calls that are doing something more complex though so keep the rest.

	if (ret)
		return ret;

is fine here.

Quoted 45 lines
drivers/iio/proximity/d3323aa.c new file +45 -0
+
+	data->gpiod_clkin_detectout =
+		devm_gpiod_get(data->dev, "vout-clk", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_clkin_detectout))
+		return dev_err_probe(data->dev,
+				     PTR_ERR(data->gpiod_clkin_detectout),
+				     "Could not get GPIO vout-clk\n");
+
+	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\n");
+
+	ret = gpiod_to_irq(data->gpiod_clkin_detectout);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret, "Could not get IRQ\n");
+
+	/*
+	 * Device signals with a rising or falling detection signal when the
+	 * proximity data is above or below the threshold, respectively.
+	 */
+	ret = devm_request_irq(data->dev, ret, d3323aa_irq_handler,
+			       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			       dev_name(data->dev), indio_dev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+	ret = d3323aa_setup(indio_dev, D3323AA_LP_FILTER_FREQ_DEFAULT_IDX,
+			    D3323AA_FILTER_GAIN_DEFAULT_IDX,
+			    D3323AA_THRESH_DEFAULT_VAL);
+	if (ret)
+		return ret;
+
+	indio_dev->info = &d3323aa_info;
+	indio_dev->name = "d3323aa";
+	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\n");
+
+	return 0;
+}
#8 lore raw

Re: [PATCH v2 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor

Quoted 2 lines · On Sun, Jun 22, 2025 at 12:07 +0100 Jonathan Cameron <[email protected]> wrote:

On Sun, 15 Jun 2025 00:14:05 +0200
Waqar Hameed <[email protected]> wrote:

[...]

Quoted 20 lines
Quoted 9 lines
drivers/iio/proximity/d3323aa.c new file +9 -0
+static irqreturn_t d3323aa_irq_handler(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_clkin_detectout);
+	if (val < 0) {

drivers/iio/proximity/d3323aa.c line 299

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+		dev_err_ratelimited(data->dev,
+				    "Could not read from GPIO vout-clk (%d)\n",
+				    val);
+		return IRQ_HANDLED;
+	}
+
+	if (!data->detecting) {
+		/* Reset interrupt counting falling edges. */
+		if (!val && atomic_inc_return(&data->irq_reset_count) ==
+				    D3323AA_IRQ_RESET_COUNT)

Odd line break

Quoted 6 lines
		if (!val &&
		    atomic_inc_return(&data->irq_reset_count) == D3323AA_IRQ_RESET_COUNT)

Is a bit better though rather long line. I'm not completely clear on why it needs to be an atomic counter though as the comment in reset() to to imply this can't race with the zeroing and no one else touches it.

Hm, now when we have unified the IRQ handlers, there is actually no need for it to be atomic anymore. Since it is now "guarded" with an if-statement on data->detecting and can't race. Good catch! Let's convert irq_reset_count to an u8. This will also fix the formatting when removing atomic_inc_return().

Quoted 11 lines

drivers/iio/proximity/d3323aa.c line 321

Quoted 7 lines
drivers/iio/proximity/d3323aa.c new file +7 -0
+static int d3323aa_reset(struct iio_dev *indio_dev)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	long time;
+	int ret;
+
+	if (regulator_is_enabled(data->regulator_vdd)) {

Add a comment to say this check is only for the use in probe() where the power may not be on yet. In other paths I think it will always be on.

Yeah, that might not be immediately obvious. Will add one.

Quoted 6 lines
Quoted 6 lines
drivers/iio/proximity/d3323aa.c new file +6 -0
+		ret = regulator_disable(data->regulator_vdd);
+		if (ret)
+			return ret;
+	}
+
+	/*
Quoted 8 lines

drivers/iio/proximity/d3323aa.c line 331

Quoted 4 lines
drivers/iio/proximity/d3323aa.c new file +4 -0
+	 * 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.
+	 */
+	msleep(30 + 5);

Use fsleep here as well with the parameter value in usecs. It will use msleep under the hood but we don't need to enforce that in the driver. On some particular platform some other sleep call may be more suited.

I agree, that's a better approach.

Quoted 7 lines

drivers/iio/proximity/d3323aa.c line 334

Quoted 3 lines
drivers/iio/proximity/d3323aa.c new file +3 -0
+
+	/*
+	 * After setting VDD to high, the device signals with

This is a little odd. vdd, as the regulator is currently low as you powered it down. This is referring I think to what happens later. Edit the comment to make that clearer. Someting like * When VDD is later enabled...

I can see how one can get a little confused by that. I'll rephrase it!

Quoted 19 lines
Quoted 19 lines
drivers/iio/proximity/d3323aa.c new file +19 -0
+static int d3323aa_setup(struct iio_dev *indio_dev, size_t lp_filter_freq_idx,
+			 size_t filter_gain_idx, u8 detect_thresh)
+{
+	DECLARE_BITMAP(write_regbitmap, D3323AA_REG_NR_BITS);
+	DECLARE_BITMAP(read_regbitmap, 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;
+	}
+
+	/*
Quoted 8 lines

drivers/iio/proximity/d3323aa.c line 400

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+	 * 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);

fsleep() preferred as that generalizes the tolerances fed to usleep_range() so we don't have to think if they are appropriate in each call.

As above, I agree.

Quoted 16 lines
Quoted 16 lines
drivers/iio/proximity/d3323aa.c new file +16 -0
+static int d3323aa_set_lp_filter_freq(struct iio_dev *indio_dev, const int val,
+				      int val2)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	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];
+
+		if (val == integer && val2 == fract)
Quoted 8 lines

drivers/iio/proximity/d3323aa.c line 473

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+			break;
+	}
+
+	if (idx == ARRAY_SIZE(d3323aa_lp_filter_freq))
+		return -ERANGE;

It's a patch not a range check, so -EINVAL may make more sense as a return value.

Hm, ERANGEs message does say "*Numerical result* out of range...", which I can see is not really applicable here (strictly speaking, we are really not "calculating" anything...). However, isn't EDOM actually the better alternative here? Consider the following

echo a > in_proximity_hardwaregain
sh: write error: Invalid argument

echo 1234 > in_proximity_hardwaregain
sh: write error: Numerical argument out of domain

Quoted 4 lines
Quoted 4 lines
drivers/iio/proximity/d3323aa.c new file +4 -0
+static void d3323aa_disable_regulator(void *indata)
+{
+	struct d3323aa_data *data = indata;
+	int ret;
Quoted 8 lines

drivers/iio/proximity/d3323aa.c line 705

Quoted 4 lines
drivers/iio/proximity/d3323aa.c new file +4 -0
+
+	if (!regulator_is_enabled(data->regulator_vdd))
+		return;
+

This is unusual and I think an artefact of where you are registering the devm callback and the use of reset later. Needs a comments at the very least to explain why the check is needed.

Correct! I'll add a comment clarifying this.

Quoted 20 lines
Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +4 -0
+	ret = regulator_disable(data->regulator_vdd);
+	if (ret)
+		dev_err(data->dev, "Could not disable regulator (%d)\n", ret);
+}

drivers/iio/proximity/d3323aa.c line 719

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+
+static int d3323aa_probe(struct platform_device *pdev)
+{
+	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,

Given there is a lot of use of either pdev->dev or data->dev which is the same thing, I'd suggest a local declaration at the top and use that throughout.

Quoted 1 line
	struct device *dev = &pdev->dev;

Alright, we can do that.

Quoted 13 lines
Quoted 13 lines
drivers/iio/proximity/d3323aa.c new file +13 -0
+				     "Could not allocate iio device\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = &pdev->dev;
+
+	init_completion(&data->reset_completion);
+
+	ret = devm_mutex_init(data->dev, &data->statevar_lock);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not initialize mutex\n");
+
+	data->regulator_vdd = devm_regulator_get_exclusive(data->dev, "vdd");
Quoted 8 lines

drivers/iio/proximity/d3323aa.c line 737

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+	if (IS_ERR(data->regulator_vdd))
+		return dev_err_probe(data->dev, PTR_ERR(data->regulator_vdd),
+				     "Could not get regulator\n");
+
+	ret = devm_add_action_or_reset(data->dev, d3323aa_disable_regulator,

This is in a slightly odd place as you haven't turned it on yet. At very least add a comment.

Will do!

Quoted 17 lines

drivers/iio/proximity/d3323aa.c line 742

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+				       data);
+	if (ret)
+		return dev_err_probe(
+			data->dev, ret,
+			"Could not add disable regulator action\n");

Odd formatting.

		return dev_err_probe(dev, ret,
				     "Could not add disable regulator action\n");

It's fine to go a little over 80 chars if it helps readability and here I think it does. However it is vanishingly unlikely this would fail (as it basically means memory allocation is failing in which case not much is going to work) so common practice is not to bother with prints for failed devm_add_action_or_reset(). Those prints do make sense for devm calls that are doing something more complex though so keep the rest.

Quoted 4 lines
	if (ret)
		return ret;

is fine here.

clang-format trying its best here. Let's just remove the print then.

There are a bunch drivers in iio that are printing in this devm_add_action_or_reset()-error-path (though it looks like the majority are not doing that). Probably not really worth changing those; in case someone would really "miss" the (very unlikely) prints.

maintainer
#9 lore raw

Re: [PATCH v2 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor

Quoted 30 lines
Quoted 16 lines
Quoted 16 lines
drivers/iio/proximity/d3323aa.c new file +16 -0
+static int d3323aa_set_lp_filter_freq(struct iio_dev *indio_dev, const int val,
+				      int val2)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	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];
+
+		if (val == integer && val2 == fract)
Quoted 8 lines

drivers/iio/proximity/d3323aa.c line 473

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+			break;
+	}
+
+	if (idx == ARRAY_SIZE(d3323aa_lp_filter_freq))
+		return -ERANGE;  

It's a patch not a range check, so -EINVAL may make more sense as a return value.

Hm, ERANGEs message does say "*Numerical result* out of range...", which I can see is not really applicable here (strictly speaking, we are really not "calculating" anything...). However, isn't EDOM actually the better alternative here? Consider the following

Quoted 5 lines

echo a > in_proximity_hardwaregain
sh: write error: Invalid argument

echo 1234 > in_proximity_hardwaregain
sh: write error: Numerical argument out of domain

I'd still stick to -EINVAL as correct if not that informative simply because EDOM is very rarely used (wasn't one I even knew existed until today ;)

Quoted 20 lines
Quoted 20 lines

drivers/iio/proximity/d3323aa.c line 742

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+				       data);
+	if (ret)
+		return dev_err_probe(
+			data->dev, ret,
+			"Could not add disable regulator action\n");  

Odd formatting.

		return dev_err_probe(dev, ret,
				     "Could not add disable regulator action\n");

It's fine to go a little over 80 chars if it helps readability and here I think it does. However it is vanishingly unlikely this would fail (as it basically means memory allocation is failing in which case not much is going to work) so common practice is not to bother with prints for failed devm_add_action_or_reset(). Those prints do make sense for devm calls that are doing something more complex though so keep the rest.

	if (ret)
		return ret;
Quoted 8 lines
Quoted 1 line

is fine here.

clang-format trying its best here. Let's just remove the print then.

There are a bunch drivers in iio that are printing in this devm_add_action_or_reset()-error-path (though it looks like the majority are not doing that). Probably not really worth changing those; in case someone would really "miss" the (very unlikely) prints.

If they are doing dev_err_probe() it won't print anyway as that only returns -ENOMEM which dev_err_probe() doesn't print on simply because you get lots of info if a memory allocation fails anyway.

https://elixir.bootlin.com/linux/v6.15.3/source/drivers/base/core.c#L5017

So on that basis it would be a sensible I think to do a cleanup patch set to drop that particular devm_add_action_or_reset() / dev_err_probe() combination. If it were just a case of unlikely (rather than impossible) I'd agree that it wasn't worth the churn!

Jonathan

#10 lore raw

Re: [PATCH v2 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor

Quoted 33 lines · On Sat, Jun 28, 2025 at 18:11 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 33 lines
Quoted 16 lines
Quoted 16 lines
drivers/iio/proximity/d3323aa.c new file +16 -0
+static int d3323aa_set_lp_filter_freq(struct iio_dev *indio_dev, const int val,
+				      int val2)
+{
+	struct d3323aa_data *data = iio_priv(indio_dev);
+	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];
+
+		if (val == integer && val2 == fract)
Quoted 8 lines

drivers/iio/proximity/d3323aa.c line 473

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+			break;
+	}
+
+	if (idx == ARRAY_SIZE(d3323aa_lp_filter_freq))
+		return -ERANGE;  

It's a patch not a range check, so -EINVAL may make more sense as a return value.

Hm, ERANGEs message does say "*Numerical result* out of range...", which I can see is not really applicable here (strictly speaking, we are really not "calculating" anything...). However, isn't EDOM actually the better alternative here? Consider the following

echo a > in_proximity_hardwaregain
sh: write error: Invalid argument

Quoted 6 lines
Quoted 2 lines

echo 1234 > in_proximity_hardwaregain
sh: write error: Numerical argument out of domain

I'd still stick to -EINVAL as correct if not that informative simply because EDOM is very rarely used (wasn't one I even knew existed until today ;)

Alright, I don't really have any strong opinions on this. Let's use -EINVAL.

Quoted 33 lines
Quoted 20 lines
Quoted 20 lines

drivers/iio/proximity/d3323aa.c line 742

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+				       data);
+	if (ret)
+		return dev_err_probe(
+			data->dev, ret,
+			"Could not add disable regulator action\n");  

Odd formatting.

		return dev_err_probe(dev, ret,
				     "Could not add disable regulator action\n");

It's fine to go a little over 80 chars if it helps readability and here I think it does. However it is vanishingly unlikely this would fail (as it basically means memory allocation is failing in which case not much is going to work) so common practice is not to bother with prints for failed devm_add_action_or_reset(). Those prints do make sense for devm calls that are doing something more complex though so keep the rest.

	if (ret)
		return ret;
Quoted 8 lines
Quoted 1 line

is fine here.

clang-format trying its best here. Let's just remove the print then.

There are a bunch drivers in iio that are printing in this devm_add_action_or_reset()-error-path (though it looks like the majority are not doing that). Probably not really worth changing those; in case someone would really "miss" the (very unlikely) prints.

If they are doing dev_err_probe() it won't print anyway as that only returns -ENOMEM which dev_err_probe() doesn't print on simply because you get lots of info if a memory allocation fails anyway.

Quoted 6 lines

https://elixir.bootlin.com/linux/v6.15.3/source/drivers/base/core.c#L5017

So on that basis it would be a sensible I think to do a cleanup patch set to drop that particular devm_add_action_or_reset() / dev_err_probe() combination. If it were just a case of unlikely (rather than impossible) I'd agree that it wasn't worth the churn!

Right, I'll send a clean-up patch for those then.

maintainer
#11 lore raw
In reply to Waqar Hameed
Quoted 52 lines · On Sun, 15 Jun 2025 00:13:52 +0200 Waqar Hameed <[email protected]> wrote:

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]/
[2] https://www.endrich.com/Datenbl%C3%A4tter/Sensoren/D3-323-AA_e.pdf

Changes in v2:

[dt-bindings]
* Convert vdd-gpio to a vdd-supply.
* Rename clk-vout-gpio to vout-clk-gpios.
* Add description for data-gpios explaining the rename to a more descriptive name.
* Drop all references to driver.
* Remove unused gpio include in examples.
* Re-phrase commit message to only describe the hardware.

[iio]
* Add newline after variable definitions inside the for-loop in d3323aa_set_lp_filter_freq().
* Remove error code in string in dev_err_probe().
* Remove driver name macro and use it inline instead.
* Format filter gain arrays into one line.
* Drop structure comment in probe().
* Format sentinel value in of_device_id with a space.
* Rename gpiod_clk_vout to gpiod_clkin_detectout.
* Request vout-clk GPIO to match rename in dt-bindings.
* Use the regulator framework for supply voltage.
* Use only one IRQ handler for both reset and detection.
* Reword comment about Vout/CLK ramp-up behavior (it's because of VDD charging up).
* Add comment for why we have both IRQF_TRIGGER_RISING and IRQF_TRIGGER_FALLING.
* Rename regmap to regbitmap to not confuse with the regmap-framework.
* Move d3323aa_setup() into the set-functions.
* Use state variables in d3323aa_data instead of bitmap and move bitmap handling to read/write settings functions.
* Pad bitmap with compulsory end pattern in d3323aa_write_settings().
* Add d3323aa_set_hp_filter_freq() and allow userspace to set it.

Quoted 4 lines

Link to v1: https://lore.kernel.org/lkml/[email protected]/

Waqar Hameed (3):
dt-bindings: vendor-prefixes: Add Nicera

I guess you didn't +CC linux-iio on this as I'm not seeing it locally.

Given the whole series including that patch will ultimately go through the IIO tree, please make sure to include that on the next version.

Jonathan

Quoted 14 lines

dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor
iio: Add driver for Nicera D3-323-AA PIR sensor

 .../iio/proximity/nicera,d3323aa.yaml         |  60 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/iio/proximity/Kconfig                 |   9 +
 drivers/iio/proximity/Makefile                |   1 +
 drivers/iio/proximity/d3323aa.c               | 808 ++++++++++++++++++
 5 files changed, 880 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml
 create mode 100644 drivers/iio/proximity/d3323aa.c

base-commit: 5abc7438f1e9d62e91ad775cc83c9594c48d2282

#12 lore raw
Quoted 2 lines · On Sun, Jun 22, 2025 at 11:33 +0100 Jonathan Cameron <[email protected]> wrote:

On Sun, 15 Jun 2025 00:13:52 +0200
Waqar Hameed <[email protected]> wrote:

[...]

Quoted 3 lines
Quoted 2 lines

Waqar Hameed (3):
dt-bindings: vendor-prefixes: Add Nicera

I guess you didn't +CC linux-iio on this as I'm not seeing it locally.

That's correct, solely relied on get_maintainers.pl here.

Quoted 3 lines

Given the whole series including that patch will ultimately go through the IIO tree, please make sure to include that on the next version.

Makes perfect sense, will do!