devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] dt-bindings: vendor-prefixes: Add Nicera
  2025-05-09 15:03 [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor Waqar Hameed
@ 2025-05-09 15:03 ` Waqar Hameed
  2025-05-09 15:03 ` [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor Waqar Hameed
  2025-05-09 15:09 ` [PATCH 0/3] Add driver for " Krzysztof Kozlowski
  2 siblings, 0 replies; 11+ messages in thread
From: Waqar Hameed @ 2025-05-09 15:03 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: kernel, Rob Herring, devicetree, linux-kernel

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

Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 86f6a19b28ae..e01bb70405a2 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1061,6 +1061,8 @@ patternProperties:
     description: Next Thing Co.
   "^ni,.*":
     description: National Instruments
+  "^nicera,.*":
+    description: Nippon Ceramic Co., Ltd.
   "^nintendo,.*":
     description: Nintendo
   "^nlt,.*":
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor
@ 2025-05-09 15:03 Waqar Hameed
  2025-05-09 15:03 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add Nicera Waqar Hameed
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Waqar Hameed @ 2025-05-09 15:03 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski
  Cc: kernel, linux-kernel, linux-iio, devicetree

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

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


base-commit: d76bb1ebb5587f66b0f8b8099bfbb44722bc08b3
-- 
2.39.5


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor
  2025-05-09 15:03 [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor Waqar Hameed
  2025-05-09 15:03 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add Nicera Waqar Hameed
@ 2025-05-09 15:03 ` Waqar Hameed
  2025-05-09 15:06   ` Krzysztof Kozlowski
  2025-05-09 15:09 ` [PATCH 0/3] Add driver for " Krzysztof Kozlowski
  2 siblings, 1 reply; 11+ messages in thread
From: Waqar Hameed @ 2025-05-09 15:03 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: kernel, linux-iio, devicetree, linux-kernel

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.

Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
 .../iio/proximity/nicera,d3323aa.yaml         | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml

diff --git a/Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml b/Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml
new file mode 100644
index 000000000000..1ff24dad0086
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/nicera,d3323aa.yaml
@@ -0,0 +1,67 @@
+# 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 <waqar.hameed@axis.com>
+
+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-gpio:
+    maxItems: 1
+    description:
+      GPIO for supply voltage (1.8 to 5.5 V).
+      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:
+    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:
+    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:
+  - compatible
+  - vdd-gpio
+  - clk-vout-gpio
+  - data-gpio
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    proximity {
+        compatible = "nicera,d3323aa";
+        vdd-gpio = <&gpio 73 0>;
+        clk-vout-gpio = <&gpio 78 0>;
+        data-gpio = <&gpio 76 0>;
+    };
+...
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor
  2025-05-09 15:03 ` [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor Waqar Hameed
@ 2025-05-09 15:06   ` Krzysztof Kozlowski
  2025-05-16 17:15     ` Waqar Hameed
  0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-09 15:06 UTC (permalink / raw)
  To: Waqar Hameed, Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: kernel, linux-iio, devicetree, linux-kernel

On 09/05/2025 17:03, Waqar Hameed 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.
> 
> 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.

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

Datasheet says this is a supply.

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

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

There is no such pin.

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

So you included that header

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

> +    };
> +...


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor
  2025-05-09 15:03 [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor Waqar Hameed
  2025-05-09 15:03 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add Nicera Waqar Hameed
  2025-05-09 15:03 ` [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor Waqar Hameed
@ 2025-05-09 15:09 ` Krzysztof Kozlowski
  2025-05-16 17:14   ` Waqar Hameed
  2 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-09 15:09 UTC (permalink / raw)
  To: Waqar Hameed, Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski
  Cc: kernel, linux-kernel, linux-iio, devicetree

On 09/05/2025 17:03, Waqar Hameed 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`

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

> 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/20241212042412.702044-2-Hermes.Zhang@axis.com/
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.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor
  2025-05-09 15:09 ` [PATCH 0/3] Add driver for " Krzysztof Kozlowski
@ 2025-05-16 17:14   ` Waqar Hameed
  0 siblings, 0 replies; 11+ messages in thread
From: Waqar Hameed @ 2025-05-16 17:14 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, kernel, linux-kernel, linux-iio, devicetree

On Fri, May 09, 2025 at 17:09 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 09/05/2025 17:03, Waqar Hameed 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`
>
> So that's a v2. Mark your patches correctly.

I figured that since it was a complete rewrite (and from another
author), I'd start a new series. But I also understand your point.

To not confuse others, I'll mark the next one as V2 instead (if that's
fine with you).

>
>> 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/20241212042412.702044-2-Hermes.Zhang@axis.com/
> 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.

I'm really sorry! I actually completely missed your response there. 

Thank you again for reviewing! I know it's a lot of work sometimes...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor
  2025-05-09 15:06   ` Krzysztof Kozlowski
@ 2025-05-16 17:15     ` Waqar Hameed
  2025-05-20  6:36       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Waqar Hameed @ 2025-05-16 17:15 UTC (permalink / raw)
  To: Krzysztof Koowski
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, kernel, linux-iio, devicetree,
	linux-kernel

On Fri, May 09, 2025 at 17:06 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 09/05/2025 17:03, Waqar Hameed 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.
>> 
>> 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.

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

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

> Plus 'gpio' suffix is not valid, btw.

I actually `grep`ed 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.

[...]

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

>
>> +    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:
>
> There is no such pin.

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

>
>> +    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:
>> +  - compatible
>> +  - vdd-gpio
>> +  - clk-vout-gpio
>> +  - data-gpio
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>
> So you included that header
>
>> +
>> +    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?

True, I'll add `GPIO_ACTIVE_*` to the properties.

[...]


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor
  2025-05-16 17:15     ` Waqar Hameed
@ 2025-05-20  6:36       ` Krzysztof Kozlowski
  2025-05-20 12:00         ` Waqar Hameed
  0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-20  6:36 UTC (permalink / raw)
  To: Waqar Hameed
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, kernel, linux-iio, devicetree,
	linux-kernel

On 16/05/2025 19:15, Waqar Hameed wrote:
> On Fri, May 09, 2025 at 17:06 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
>> On 09/05/2025 17:03, Waqar Hameed 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.
>>>
>>> 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.
> 
>> 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.)
> 
>>
>>> +
>>> +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`.
> 
>> Plus 'gpio' suffix is not valid, btw.
> 
> I actually `grep`ed 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.

> 
> [...]
> 
>>> +      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?

Call it by the name of pin, so vout-clk-gpios. I guess from SoC/CPU side
this cannot be anything else than GPIO.

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

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


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor
  2025-05-20  6:36       ` Krzysztof Kozlowski
@ 2025-05-20 12:00         ` Waqar Hameed
  0 siblings, 0 replies; 11+ messages in thread
From: Waqar Hameed @ 2025-05-20 12:00 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, kernel, linux-iio, devicetree,
	linux-kernel

On Tue, May 20, 2025 at 08:36 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 16/05/2025 19:15, Waqar Hameed wrote:
>> On Fri, May 09, 2025 at 17:06 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> 
>>> On 09/05/2025 17:03, Waqar Hameed 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.
>>>>
>>>> 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.
>> 
>>> 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.)
>> 
>>>
>>>> +
>>>> +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`.
>> 
>>> Plus 'gpio' suffix is not valid, btw.
>> 
>> I actually `grep`ed 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.

Ah, ok! Good to know.

[...]

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

Got it! Let's do that.

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

Understood, will do that!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor
@ 2025-06-14 21:56 Waqar Hameed
  2025-06-14 22:09 ` Waqar Hameed
  0 siblings, 1 reply; 11+ messages in thread
From: Waqar Hameed @ 2025-06-14 21:56 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski
  Cc: kernel, linux-kernel, linux-iio, devicetree

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/20241212042412.702044-2-Hermes.Zhang@axis.com/
[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/cover.1746802541.git.waqar.hameed@axis.com/

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor
  2025-06-14 21:56 Waqar Hameed
@ 2025-06-14 22:09 ` Waqar Hameed
  0 siblings, 0 replies; 11+ messages in thread
From: Waqar Hameed @ 2025-06-14 22:09 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, kernel,
	linux-kernel, linux-iio, devicetree

On Sat, Jun 14, 2025 at 23:56 +0200 Waqar Hameed <waqar.hameed@axis.com> wrote:

[...]

> 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/cover.1746802541.git.waqar.hameed@axis.com/

Sigh, forgot to mark the subject as v2... Resending it...

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-06-14 22:10 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-09 15:03 [PATCH 0/3] Add driver for Nicera D3-323-AA PIR sensor Waqar Hameed
2025-05-09 15:03 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add Nicera Waqar Hameed
2025-05-09 15:03 ` [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor Waqar Hameed
2025-05-09 15:06   ` Krzysztof Kozlowski
2025-05-16 17:15     ` Waqar Hameed
2025-05-20  6:36       ` Krzysztof Kozlowski
2025-05-20 12:00         ` Waqar Hameed
2025-05-09 15:09 ` [PATCH 0/3] Add driver for " Krzysztof Kozlowski
2025-05-16 17:14   ` Waqar Hameed
  -- strict thread matches above, loose matches on Subject: below --
2025-06-14 21:56 Waqar Hameed
2025-06-14 22:09 ` Waqar Hameed

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).