Skip to contentSkip to conversation

Add driver for Nicera D3-323-AA PIR sensor

PATCH In review Waqar Hameed 3 participants 9 messages Jul 4, 2025 – Jul 13, 2025

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v3 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 v3:

[dt-bindings]
* Re-add gpio include header and use a constant from that header.

[iio]
* Convert irq_reset_count from atomic_t into an u8.
* Add a comment in d3323aa_reset() explaining regulator_is_enabled()-check is for probe().
* Use flseep() in places where the delay is flexible.
* Rephrase comment about enabling VDD and counting falling edges.
* Declare and use a variable for struct device* in probe().
* Remove impossible error print for devm_add_action_or_reset().
* Add a comment for devm_add_action_or_reset() call in d3323aa_probe() explaining when the regulator is enabled.
* Add a comment clarifying the regulator_is_enabled() check in d3323aa_disable_regulator().
* Change -ERANGE return values to -EINVAL for the set-functions in d3323aa_write_raw().

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

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         |  62 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/iio/proximity/Kconfig                 |   9 +
 drivers/iio/proximity/Makefile                |   1 +
 drivers/iio/proximity/d3323aa.c               | 814 ++++++++++++++++++
 5 files changed, 888 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml
 create mode 100644 drivers/iio/proximity/d3323aa.c

base-commit: 66701750d5565c574af42bef0b789ce0203e3071

Signature · 1 line
2.39.5
#1 lore raw

PATCH v3 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 5d2a7a8d3ac6..9df4cb54eea9 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
#2 lore raw

PATCH v3 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         | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml
Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +62 -0
new file mode 100644
index 000000000000..65d9b44fcd5e
@@ -0,0 +1,62 @@
+# 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:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    proximity {
+        compatible = "nicera,d3323aa";
+        vdd-supply = <&regulator_3v3>;
+        vout-clk-gpios = <&gpio 78 GPIO_ACTIVE_HIGH>;
+        data-gpios = <&gpio 76 GPIO_ACTIVE_HIGH>;
+    };
+...
Signature · 1 line
2.39.5
maintainer
#3 lore raw

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

Quoted 38 lines · On Fri, 4 Jul 2025 18:14:38 +0200 Waqar Hameed <[email protected]> wrote:

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         | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml
Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +23 -0
new file mode 100644
index 000000000000..65d9b44fcd5e
@@ -0,0 +1,62 @@
+# 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).
+

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

Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +10 -0
+  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.

Don't start a new line for a new sentence.

Quoted 29 lines
Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml new file +29 -0
+
+  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:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    proximity {
+        compatible = "nicera,d3323aa";
+        vdd-supply = <&regulator_3v3>;
+        vout-clk-gpios = <&gpio 78 GPIO_ACTIVE_HIGH>;
+        data-gpios = <&gpio 76 GPIO_ACTIVE_HIGH>;
+    };
+...
Reviewed-by on v3 2/3
#4 lore raw

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

In reply to Waqar Hameed
Quoted 8 lines · 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         | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml

Best regards,
Krzysztof

#5 lore raw

PATCH v3 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 | 814 ++++++++++++++++++++++++++++++++
 3 files changed, 824 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 +814 -0

814 lines not on this page · raw message

Signature · 1 line
2.39.5
maintainer
#6 lore raw

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

Quoted 10 lines · On Fri, 4 Jul 2025 18:14:38 +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).

One suggestion inline on providing more information on the 'why' behind the regulator handling.

I want to leave this on list anyway to give more time for other reviews, but if nothing else comes up and you are happy with my description I can tweak this whilst applying.

Jonathan

Quoted 86 lines
---
 drivers/iio/proximity/Kconfig   |   9 +
 drivers/iio/proximity/Makefile  |   1 +
 drivers/iio/proximity/d3323aa.c | 814 ++++++++++++++++++++++++++++++++
 3 files changed, 824 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 +39 -0
new file mode 100644
index 000000000000..b1bc3204c0c0
@@ -0,0 +1,814 @@
 
 
+static void d3323aa_disable_regulator(void *indata)
+{
+	struct d3323aa_data *data = indata;
+	int ret;
+
+	/*
+	 * During probe() the regulator may be disabled. It is enabled during
+	 * device setup (in d3323aa_reset(), where it is also briefly disabled).
+	 * The check is therefore needed in order to have balanced
+	 * regulator_enable/disable() calls.
+	 */
+	if (!regulator_is_enabled(data->regulator_vdd))
+		return;
+
+	ret = regulator_disable(data->regulator_vdd);
+	if (ret)
+		dev_err(data->dev, "Could not disable regulator (%d)\n", ret);
+}
+
+static int d3323aa_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct d3323aa_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Could not allocate iio device\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+
+	init_completion(&data->reset_completion);
+
+	ret = devm_mutex_init(dev, &data->statevar_lock);
+	if (ret)
+		return dev_err_probe(dev, ret, "Could not initialize mutex\n");

drivers/iio/proximity/d3323aa.c line 745

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+
+	data->regulator_vdd = devm_regulator_get_exclusive(dev, "vdd");
+	if (IS_ERR(data->regulator_vdd))
+		return dev_err_probe(dev, PTR_ERR(data->regulator_vdd),
+				     "Could not get regulator\n");
+
+	/*
+	 * The regulator will be enabled during the device setup below (in
+	 * d3323aa_reset()). Note that d3323aa_disable_regulator() also checks
+	 * for the regulator state.

This comment doesn't explain why you do this here as opposed to after reset. Key is that there are complex paths in which the regulator is disabled that are unrelated to probe()/remove() Talk about those rather than why this 'works'. It's the why that matters in a comment more than the how.

If nothing else comes up in review, I can chagne this to something like

* The regulator will be enabled for the first time during the
* device setup below (in d3323aa_reset()). However parameter changes
* from userspace can require a temporary disable of the regulator.
* To avoid complex handling of state, use a callback that will disable
* the regulator if it happens to be enabled at time of devm unwind. */

Quoted 5 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+	ret = d3323aa_setup(indio_dev, D3323AA_LP_FILTER_FREQ_DEFAULT_IDX,
+			    D3323AA_FILTER_GAIN_DEFAULT_IDX,
+			    D3323AA_THRESH_DEFAULT_VAL);
+	if (ret)
+		return ret;
#7 lore raw

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

On Sun, Jul 06, 2025 at 12:11 +0100 Jonathan Cameron <[email protected]> wrote:

[...]

Quoted 6 lines

One suggestion inline on providing more information on the 'why' behind the regulator handling.

I want to leave this on list anyway to give more time for other reviews, but if nothing else comes up and you are happy with my description I can tweak this whilst applying.

Sure, we can let it breathe for a bit. I'm fine with you editing it while applying it (maybe also the minor format comment in the dt-bindings patch then?). Either way, if there is anything else you want me to do do, just tell! Thanks again Jonathan!

Quoted 105 lines

Jonathan

Quoted 86 lines
---
 drivers/iio/proximity/Kconfig   |   9 +
 drivers/iio/proximity/Makefile  |   1 +
 drivers/iio/proximity/d3323aa.c | 814 ++++++++++++++++++++++++++++++++
 3 files changed, 824 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 +39 -0
new file mode 100644
index 000000000000..b1bc3204c0c0
@@ -0,0 +1,814 @@
 
 
+static void d3323aa_disable_regulator(void *indata)
+{
+	struct d3323aa_data *data = indata;
+	int ret;
+
+	/*
+	 * During probe() the regulator may be disabled. It is enabled during
+	 * device setup (in d3323aa_reset(), where it is also briefly disabled).
+	 * The check is therefore needed in order to have balanced
+	 * regulator_enable/disable() calls.
+	 */
+	if (!regulator_is_enabled(data->regulator_vdd))
+		return;
+
+	ret = regulator_disable(data->regulator_vdd);
+	if (ret)
+		dev_err(data->dev, "Could not disable regulator (%d)\n", ret);
+}
+
+static int d3323aa_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct d3323aa_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Could not allocate iio device\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+
+	init_completion(&data->reset_completion);
+
+	ret = devm_mutex_init(dev, &data->statevar_lock);
+	if (ret)
+		return dev_err_probe(dev, ret, "Could not initialize mutex\n");

drivers/iio/proximity/d3323aa.c line 745

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+
+	data->regulator_vdd = devm_regulator_get_exclusive(dev, "vdd");
+	if (IS_ERR(data->regulator_vdd))
+		return dev_err_probe(dev, PTR_ERR(data->regulator_vdd),
+				     "Could not get regulator\n");
+
+	/*
+	 * The regulator will be enabled during the device setup below (in
+	 * d3323aa_reset()). Note that d3323aa_disable_regulator() also checks
+	 * for the regulator state.

This comment doesn't explain why you do this here as opposed to after reset. Key is that there are complex paths in which the regulator is disabled that are unrelated to probe()/remove() Talk about those rather than why this 'works'. It's the why that matters in a comment more than the how.

Quoted 8 lines

If nothing else comes up in review, I can chagne this to something like

* The regulator will be enabled for the first time during the
* device setup below (in d3323aa_reset()). However parameter changes
* from userspace can require a temporary disable of the regulator.
* To avoid complex handling of state, use a callback that will disable
* the regulator if it happens to be enabled at time of devm unwind. */

Ah, I see that I misunderstood you the first time! The comment looks fine to me.

Quoted 6 lines
Quoted 6 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+	ret = d3323aa_setup(indio_dev, D3323AA_LP_FILTER_FREQ_DEFAULT_IDX,
+			    D3323AA_FILTER_GAIN_DEFAULT_IDX,
+			    D3323AA_THRESH_DEFAULT_VAL);
+	if (ret)
+		return ret;
maintainer
#8 lore raw

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

Quoted 7 lines · On Mon, 7 Jul 2025 10:46:09 +0200 Waqar Hameed <[email protected]> wrote:

On Sun, Jul 06, 2025 at 12:11 +0100 Jonathan Cameron <[email protected]> wrote:

[...]

Quoted 3 lines

One suggestion inline on providing more information on the 'why' behind the regulator handling.

Quoted 9 lines
Quoted 3 lines

I want to leave this on list anyway to give more time for other reviews, but if nothing else comes up and you are happy with my description I can tweak this whilst applying.

Sure, we can let it breathe for a bit. I'm fine with you editing it while applying it (maybe also the minor format comment in the dt-bindings patch then?). Either way, if there is anything else you want me to do do, just tell! Thanks again Jonathan!

I decided to be lazy and only tidied up the comment. The slightly odd formatting in the dt binding can stay.

Applied to the togreg branch of iio.git and pushed out as testing for 0-day to look at them.

J

Quoted 123 lines
Quoted 105 lines

Jonathan

Quoted 86 lines
---
 drivers/iio/proximity/Kconfig   |   9 +
 drivers/iio/proximity/Makefile  |   1 +
 drivers/iio/proximity/d3323aa.c | 814 ++++++++++++++++++++++++++++++++
 3 files changed, 824 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 +39 -0
new file mode 100644
index 000000000000..b1bc3204c0c0
@@ -0,0 +1,814 @@  
 
 
+static void d3323aa_disable_regulator(void *indata)
+{
+	struct d3323aa_data *data = indata;
+	int ret;
+
+	/*
+	 * During probe() the regulator may be disabled. It is enabled during
+	 * device setup (in d3323aa_reset(), where it is also briefly disabled).
+	 * The check is therefore needed in order to have balanced
+	 * regulator_enable/disable() calls.
+	 */
+	if (!regulator_is_enabled(data->regulator_vdd))
+		return;
+
+	ret = regulator_disable(data->regulator_vdd);
+	if (ret)
+		dev_err(data->dev, "Could not disable regulator (%d)\n", ret);
+}
+
+static int d3323aa_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct d3323aa_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Could not allocate iio device\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+
+	init_completion(&data->reset_completion);
+
+	ret = devm_mutex_init(dev, &data->statevar_lock);
+	if (ret)
+		return dev_err_probe(dev, ret, "Could not initialize mutex\n");

drivers/iio/proximity/d3323aa.c line 745

Quoted 10 lines
drivers/iio/proximity/d3323aa.c new file +10 -0
+
+	data->regulator_vdd = devm_regulator_get_exclusive(dev, "vdd");
+	if (IS_ERR(data->regulator_vdd))
+		return dev_err_probe(dev, PTR_ERR(data->regulator_vdd),
+				     "Could not get regulator\n");
+
+	/*
+	 * The regulator will be enabled during the device setup below (in
+	 * d3323aa_reset()). Note that d3323aa_disable_regulator() also checks
+	 * for the regulator state.  

This comment doesn't explain why you do this here as opposed to after reset. Key is that there are complex paths in which the regulator is disabled that are unrelated to probe()/remove() Talk about those rather than why this 'works'. It's the why that matters in a comment more than the how.

Quoted 8 lines

If nothing else comes up in review, I can chagne this to something like

* The regulator will be enabled for the first time during the
* device setup below (in d3323aa_reset()). However parameter changes
* from userspace can require a temporary disable of the regulator.
* To avoid complex handling of state, use a callback that will disable
* the regulator if it happens to be enabled at time of devm unwind. */

Ah, I see that I misunderstood you the first time! The comment looks fine to me.

Quoted 6 lines
Quoted 6 lines
drivers/iio/proximity/d3323aa.c new file +5 -0
+	ret = d3323aa_setup(indio_dev, D3323AA_LP_FILTER_FREQ_DEFAULT_IDX,
+			    D3323AA_FILTER_GAIN_DEFAULT_IDX,
+			    D3323AA_THRESH_DEFAULT_VAL);
+	if (ret)
+		return ret;