[PATCH 1/2] dt-bindings: hwmon: Add D3-323-AA

[PATCH 1/2] dt-bindings: hwmon: Add D3-323-AA

[Hermes Zhang]

Add Devicetree binding documentation for Nicera D3-323-AA Pyroelectric
IR sensor.

Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
---
 .../bindings/hwmon/nicera,d3-323-aa.yaml      | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml b/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml
new file mode 100644
index 000000000000..31690e630b5f
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/nicera,d3-323-aa.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nicera D3-323-AA Pyroelectric IR sensor
+
+maintainers:
+  - Hermes Zhang <Hermes.Zhang@axis.com>
+
+description: |
+  Nicera D3-323-AA Pyroelectric IR sensor
+
+  datasheet:
+    https://www.nicera.co.jp/wordpress/wp-content/uploads/2022/01/D3-323-AA_e.pdf
+
+properties:
+  compatible:
+    const: nicera,d3-323-aa
+
+  reset-gpios:
+    description: The GPIO pin connected to the reset pin on the sensor
+    maxItems: 1
+
+  clk-gpios:
+    description: The GPIO pin connected to the clk pin on the sensor
+    maxItems: 1
+
+  si-gpios:
+    description: The GPIO pin connected to the si pin on the sensor
+    maxItems: 1
+
+required:
+  - compatible
+  - reset-gpios
+  - clk-gpios
+  - si-gpios
+
+
+additionalProperties: false
+
+examples:
+  - |
+    sensor {
+        compatible = "nicera,d3-323-aa";
+        reset-gpios = <&gpio4 12 0>;
+        clk-gpios = <&gpio4 13 0>;
+        si-gpios = <&gpio4 14 0>;
+    };
-- 
2.45.2

[PATCH 2/2] hwmon: Add support for D3-323-AA Pyroelectric IR sensor

[Hermes Zhang]

Add support for Nicera D3-323-AA Pyroelectric IR sensor. The sensor
support to config the threshold/filter_type/filter_step and return the
detect result in sysfs attribute.

Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
---
 drivers/hwmon/Kconfig     |  11 +
 drivers/hwmon/Makefile    |   1 +
 drivers/hwmon/d3-323-aa.c | 493 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 505 insertions(+)
 create mode 100644 drivers/hwmon/d3-323-aa.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index dd376602f3f1..25dbfc85d7ab 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -507,6 +507,17 @@ config SENSORS_CROS_EC
 	  This driver can also be built as a module. If so, the module
 	  will be called cros_ec_hwmon.
 
+config SENSORS_D3323AA
+	tristate "Nicera Pyroelectric IR sensors"
+	depends on GPIOLIB && OF
+	select BITREVERSE
+	help
+	  This driver provide support for Nicera D3-323-AA Pyroelectric IR
+	  sensor.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called d3-323-aa.
+
 config SENSORS_DRIVETEMP
 	tristate "Hard disk drives with temperature sensors"
 	depends on SCSI && ATA
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b827b92f2a78..25b2f55c18ce 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
 obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o
 obj-$(CONFIG_SENSORS_CROS_EC)	+= cros_ec_hwmon.o
+obj-$(CONFIG_SENSORS_D3323AA)	+= d3-323-aa.o
 obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
 obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
 obj-$(CONFIG_SENSORS_DELL_SMM)	+= dell-smm-hwmon.o
diff --git a/drivers/hwmon/d3-323-aa.c b/drivers/hwmon/d3-323-aa.c
new file mode 100644
index 000000000000..d4089e89e678
--- /dev/null
+++ b/drivers/hwmon/d3-323-aa.c
@@ -0,0 +1,493 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * d3-322-aa.c - support for the D3-323-AA Pyroelectric Passive Infrared Sensor
+ *
+ */
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/atomic.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+
+#define DELAY_MS 10
+
+/*
+ *        0  ...  8     9  ...  16    17    18  19    20  21  22    23    24 ...  46   47 48 49 50 51
+ * -------------------------------------------------------------------------------------------------------
+ * F37   F38 ... F46   F47 ... F54   F55   F56 F57   F58 F59 F60   F64   F65 ... F87   Input End Pattern
+ *     |      0      |  threshold  |  0  |  FSTEP  |    FILSEL   |  0  |      0      |  0  1  1  0  1    |
+ *
+ * NOTE: F37 is not used
+ */
+#define THRESHOLD_OFFSET 9
+#define THRESHOLD_LEN 8
+#define FSTEP_OFFSET 18
+#define FSTEP_LEN 2
+#define FILSEL_OFFSET 20
+#define FILSEL_LEN 3
+#define INPUT_END_PATTERN_OFFSET 47
+#define INPUT_END_PATTERN_LEN 5
+
+#define REG_SETTING_SIZE 52
+
+#define DEFAULT_THRESHOLD 0x1C
+/* Input End Pattern: 01101 -> 10110 */
+#define INPUT_END_PATTERN 0x16
+
+#define SET_REGISTER_SEQ_CNT 104 /* (47 + 5) * 2 */
+#define READ_REGISTER_SEQ_CNT 116 /* (47 + 1 + 10) * 2 */
+
+static atomic_t clk_irq_count = ATOMIC_INIT(0);
+
+enum filter_step {
+	STEP_THREE = 0,
+	STEP_ONE = 1,
+	STEP_TWO = 3,
+};
+
+enum filter_type {
+	TYPE_B = 0,
+	TYPE_C = 1,
+	TYPE_D = 2,
+	TYPE_DIRECT = 3,
+	TYPE_A = 7,
+};
+
+enum d3323aa_state {
+	IDLE,
+	POWER_ON,
+	SETUP_WRITE,
+	SETUP_READ,
+	WAIT_FOR_STABLE,
+	RUNNING,
+};
+
+struct d3323aa_data {
+	struct device *dev;
+	struct gpio_desc *clk;
+	struct gpio_desc *si;
+	struct gpio_desc *reset;
+	u8 threshold; /* 0 ~ 255 */
+	enum filter_step step;
+	enum filter_type type;
+	struct delayed_work setup_work;
+	struct work_struct state_worker;
+	struct hrtimer timer;
+	/* Save the clk seq number */
+	int seq;
+	bool error;
+	enum d3323aa_state state;
+	bool detector;
+	/* index of the bitmap */
+	int idx;
+	DECLARE_BITMAP(register_bitmap, REG_SETTING_SIZE);
+};
+
+static ssize_t pir_filter_type_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct d3323aa_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int err;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+
+	switch (val) {
+	case TYPE_A:
+	case TYPE_B:
+	case TYPE_C:
+	case TYPE_D:
+	case TYPE_DIRECT:
+		data->type = val;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	cancel_delayed_work_sync(&data->setup_work);
+	schedule_delayed_work(&data->setup_work, msecs_to_jiffies(DELAY_MS));
+
+	return count;
+}
+
+static ssize_t pir_filter_step_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct d3323aa_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int err;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+
+	switch (val) {
+	case STEP_ONE:
+	case STEP_TWO:
+	case STEP_THREE:
+		data->type = val;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	cancel_delayed_work_sync(&data->setup_work);
+	schedule_delayed_work(&data->setup_work, msecs_to_jiffies(DELAY_MS));
+
+	return count;
+}
+
+static ssize_t pir_threshold_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct d3323aa_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int err;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+
+	if (val > 255)
+		return -EINVAL;
+
+	data->threshold = val;
+
+	cancel_delayed_work_sync(&data->setup_work);
+	schedule_delayed_work(&data->setup_work, msecs_to_jiffies(DELAY_MS));
+
+	return count;
+}
+
+static ssize_t pir_detector_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct d3323aa_data *data = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", data->detector);
+}
+
+static DEVICE_ATTR_WO(pir_threshold);
+static DEVICE_ATTR_WO(pir_filter_step);
+static DEVICE_ATTR_WO(pir_filter_type);
+static DEVICE_ATTR_RO(pir_detector);
+
+static struct attribute *d3323aa_attrs[] = {
+	&dev_attr_pir_threshold.attr,
+	&dev_attr_pir_filter_step.attr,
+	&dev_attr_pir_filter_type.attr,
+	&dev_attr_pir_detector.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(d3323aa);
+
+static void set_field(unsigned long *bmp, int start, int value, int size)
+{
+	int i;
+	int mask = 1;
+
+	for (i = 0; i < size; i++) {
+		assign_bit(start + i, bmp, value & mask);
+		mask <<= 1;
+	}
+}
+
+static void build_register_data(struct d3323aa_data *data)
+{
+	unsigned long *bmap = data->register_bitmap;
+
+	bitmap_zero(bmap, REG_SETTING_SIZE);
+
+	set_field(bmap, THRESHOLD_OFFSET, data->threshold, THRESHOLD_LEN);
+
+	set_field(bmap, FSTEP_OFFSET, data->step, FSTEP_LEN);
+
+	set_field(bmap, FILSEL_OFFSET, data->type, FILSEL_LEN);
+
+	set_field(bmap, INPUT_END_PATTERN_OFFSET, INPUT_END_PATTERN,
+		  INPUT_END_PATTERN_LEN);
+}
+
+static irqreturn_t irq_handler(int irq, void *dev)
+{
+	struct d3323aa_data *data = dev;
+	int count;
+
+	if (data->state == POWER_ON) {
+		int v = gpiod_get_value(data->clk);
+
+		if (v == 1)
+			return IRQ_HANDLED;
+
+		count = atomic_inc_return(&clk_irq_count);
+
+		/* This register setting and verification must be done during the
+		 * configurable period and the starting point of the period is second
+		 * falling edge of VOUT/CLK and DO/SI after turning on
+		 */
+		if (count == 2)
+			schedule_work(&data->state_worker);
+	} else {
+		int v = gpiod_get_value(data->clk);
+
+		data->detector = v ? true : false;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void d3323aa_reset(struct d3323aa_data *data)
+{
+	gpiod_set_value(data->reset, 1);
+
+	/* The supply voltage should be less than 0.5V for 30msec,
+	 * add 10ms more for VDD discharge
+	 */
+	msleep(40);
+}
+
+static void d3323aa_poweron(struct d3323aa_data *data)
+{
+	int ret;
+
+	atomic_set(&clk_irq_count, 0);
+
+	gpiod_direction_input(data->clk);
+	gpiod_direction_input(data->si);
+
+	if (data->state == RUNNING || data->state == POWER_ON)
+		free_irq(gpiod_to_irq(data->clk), data);
+
+	ret = devm_request_irq(data->dev, gpiod_to_irq(data->clk), irq_handler,
+			       IRQF_TRIGGER_FALLING, "d3323aa_poweron_irq",
+			       data);
+	if (ret) {
+		pr_err("Failed to request IRQ\n");
+		return;
+	}
+
+	data->state = POWER_ON;
+	data->detector = false;
+
+	gpiod_set_value(data->reset, 0);
+}
+
+static void state_worker_func(struct work_struct *work)
+{
+	struct d3323aa_data *data =
+		container_of(work, struct d3323aa_data, state_worker);
+
+	switch (data->state) {
+	case POWER_ON:
+		free_irq(gpiod_to_irq(data->clk), data);
+
+		gpiod_direction_output(data->clk, 0);
+		gpiod_direction_output(data->si, 0);
+
+		data->state = SETUP_WRITE;
+
+		data->seq = 0;
+		/* clk for register setting is 1kHz */
+		hrtimer_start(&data->timer, ktime_set(0, 500 * 1000),
+			      HRTIMER_MODE_REL_HARD);
+		break;
+	case SETUP_WRITE:
+		/* si pin will receive the register setting */
+		gpiod_direction_input(data->si);
+		data->seq = 0;
+		data->idx = -1; /* idx will be reset when dummy bit received */
+		data->error = false;
+		data->state = SETUP_READ;
+
+		/* 9.5ms * 2 */
+		hrtimer_start(&data->timer, ktime_set(0, ms_to_ktime(20)),
+			      HRTIMER_MODE_REL_HARD);
+		break;
+	case SETUP_READ:
+		/* clk pin will receive the pir detect signal */
+		gpiod_direction_input(data->clk);
+		data->state = WAIT_FOR_STABLE;
+
+		/* The stability time(Max. 30 sec) is required for stability of signal
+		 * output after turning on of VDD and after register setting.
+		 */
+		hrtimer_start(&data->timer, ktime_set(30, 0),
+			      HRTIMER_MODE_REL_HARD);
+		break;
+	case WAIT_FOR_STABLE:
+		int ret;
+
+		data->state = RUNNING;
+		ret = devm_request_irq(
+			data->dev, gpiod_to_irq(data->clk), irq_handler,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			"d3323aa_detect_irq", data);
+
+		break;
+	default:
+		break;
+	}
+}
+
+static void setup_func(struct work_struct *work)
+{
+	struct d3323aa_data *data =
+		container_of(work, struct d3323aa_data, setup_work.work);
+
+	build_register_data(data);
+
+	d3323aa_reset(data);
+
+	d3323aa_poweron(data);
+}
+
+static enum hrtimer_restart hrtimer_handler(struct hrtimer *hrtimer)
+{
+	struct d3323aa_data *data =
+		container_of(hrtimer, struct d3323aa_data, timer);
+
+	switch (data->state) {
+	case SETUP_WRITE:
+
+		if (data->seq % 2 == 0)
+			gpiod_set_value(data->clk, 0);
+		else
+			gpiod_set_value(data->clk, 1);
+
+		gpiod_set_value(data->si,
+				test_bit(data->seq / 2, data->register_bitmap));
+
+		if (data->seq++ == SET_REGISTER_SEQ_CNT) {
+			schedule_work(&data->state_worker);
+			return HRTIMER_NORESTART;
+		}
+		break;
+	case SETUP_READ:
+		if (data->seq % 2 == 0) {
+			gpiod_set_value(data->clk, 0);
+		} else {
+			gpiod_set_value(data->clk, 1);
+
+			if (data->idx < 0) {
+				/* Reset the idx when dummy bit received */
+				if (gpiod_get_value(data->si) == 1)
+					data->idx = 0;
+			} else if (data->idx < REG_SETTING_SIZE) {
+				if (gpiod_get_value(data->si) !=
+				    test_bit(data->idx++,
+					     data->register_bitmap))
+					data->error = true;
+			}
+		}
+
+		if (data->seq++ == READ_REGISTER_SEQ_CNT) {
+			schedule_work(&data->state_worker);
+			return HRTIMER_NORESTART;
+		}
+		break;
+	case WAIT_FOR_STABLE:
+		schedule_work(&data->state_worker);
+		return HRTIMER_NORESTART;
+	default:
+		break;
+	}
+
+	hrtimer_forward_now(hrtimer, ktime_set(0, 500 * 1000));
+
+	return HRTIMER_RESTART;
+}
+
+static int d3323aa_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct d3323aa_data *data;
+	struct device *hwmon_dev;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->dev = dev;
+	platform_set_drvdata(pdev, data);
+
+	data->state = IDLE;
+	INIT_WORK(&data->state_worker, state_worker_func);
+	INIT_DELAYED_WORK(&data->setup_work, setup_func);
+	hrtimer_init(&data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD);
+	data->timer.function = hrtimer_handler;
+
+	/* Set default register settings */
+	data->threshold = DEFAULT_THRESHOLD;
+	data->step = STEP_TWO;
+	data->type = TYPE_B;
+
+	/* Try requesting the GPIOs */
+	data->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->reset)) {
+		ret = PTR_ERR(data->reset);
+		dev_err(dev, "Reset line GPIO request failed\n");
+		goto err_release_reg;
+	}
+
+	data->clk = devm_gpiod_get(dev, "clk", GPIOD_OUT_LOW);
+	if (IS_ERR(data->clk)) {
+		ret = PTR_ERR(data->clk);
+		dev_err(dev, "CLK line GPIO request failed\n");
+		goto err_release_reg;
+	}
+
+	data->si = devm_gpiod_get(dev, "si", GPIOD_OUT_LOW);
+	if (IS_ERR(data->si)) {
+		ret = PTR_ERR(data->si);
+		dev_err(dev, "SI line GPIO request failed\n");
+		goto err_release_reg;
+	}
+
+	build_register_data(data);
+
+	d3323aa_reset(data);
+
+	d3323aa_poweron(data);
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(
+		dev, pdev->name, data, d3323aa_groups);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+
+err_release_reg:
+	return ret;
+}
+
+static const struct of_device_id d3323aa_dt_match[] = {
+	{ .compatible = "nicera,d3-323-aa" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, d3323aa_dt_match);
+
+static struct platform_driver d3323aa_driver = {
+	.driver = {
+		.name = "d3-323-aa",
+		.of_match_table = of_match_ptr(d3323aa_dt_match),
+	},
+	.probe = d3323aa_probe,
+};
+module_platform_driver(d3323aa_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Nicera D3-323-AA Pyroelectric Infrared sensor driver");
-- 
2.45.2

Re: [PATCH 2/2] hwmon: Add support for D3-323-AA Pyroelectric IR sensor

[Guenter Roeck]

Hi,

On 12/11/24 20:24, Hermes Zhang wrote:
> Add support for Nicera D3-323-AA Pyroelectric IR sensor. The sensor
> support to config the threshold/filter_type/filter_step and return the
> detect result in sysfs attribute.
> 
> Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
> ---
>
...

> +
> +static DEVICE_ATTR_WO(pir_threshold);
> +static DEVICE_ATTR_WO(pir_filter_step);
> +static DEVICE_ATTR_WO(pir_filter_type);
> +static DEVICE_ATTR_RO(pir_detector);
> +
> +static struct attribute *d3323aa_attrs[] = {
> +	&dev_attr_pir_threshold.attr,
> +	&dev_attr_pir_filter_step.attr,
> +	&dev_attr_pir_filter_type.attr,
> +	&dev_attr_pir_detector.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(d3323aa);
> +

I don't know what this is, but it is most definitely not a hardware
monitoring device. I don't see a definition of those attributes,
so I have no idea what they represent.

Maybe this is an iio device, but given the unusual attributes
I am not even sure about that. Jonathan, any thoughts ?

Guenter

Re: [PATCH 2/2] hwmon: Add support for D3-323-AA Pyroelectric IR sensor

[Jonathan Cameron]

On Wed, 11 Dec 2024 22:17:49 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> Hi,
> 
> On 12/11/24 20:24, Hermes Zhang wrote:
> > Add support for Nicera D3-323-AA Pyroelectric IR sensor. The sensor
> > support to config the threshold/filter_type/filter_step and return the
> > detect result in sysfs attribute.
> > 
> > Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
> > ---
> >  
> ...
> 
> > +
> > +static DEVICE_ATTR_WO(pir_threshold);
> > +static DEVICE_ATTR_WO(pir_filter_step);
> > +static DEVICE_ATTR_WO(pir_filter_type);
> > +static DEVICE_ATTR_RO(pir_detector);
> > +
> > +static struct attribute *d3323aa_attrs[] = {
> > +	&dev_attr_pir_threshold.attr,
> > +	&dev_attr_pir_filter_step.attr,
> > +	&dev_attr_pir_filter_type.attr,
> > +	&dev_attr_pir_detector.attr,
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(d3323aa);
> > +  
> 
> I don't know what this is, but it is most definitely not a hardware
> monitoring device. I don't see a definition of those attributes,
> so I have no idea what they represent.
> 
> Maybe this is an iio device, but given the unusual attributes
> I am not even sure about that. Jonathan, any thoughts ?

New type of sensor, but sure could be in IIO. 

Seems mostly a human presence sensor. Not that different from some
types of proximity sensor and indeed that might be the path to take
here.

Taking a quick look at the driver suggests there is lots more information
needed to understand the ABI.  At very least needs ABI docs so we can
discuss how that is generalized. So if submitting an IIO driver
I want to see
Documenation/ABI/testing/sysfs-bus-iio-xxxx
with significant detail. The datasheet provides no where near enough
info.

Jonathan


> 
> Guenter
> 
> 

Re: [PATCH 2/2] hwmon: Add support for D3-323-AA Pyroelectric IR sensor

[Hermes Zhang]

Hi

On 2024/12/13 0:59, Jonathan Cameron wrote:
> On Wed, 11 Dec 2024 22:17:49 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> Hi,
>>
>> On 12/11/24 20:24, Hermes Zhang wrote:
>>> Add support for Nicera D3-323-AA Pyroelectric IR sensor. The sensor
>>> support to config the threshold/filter_type/filter_step and return the
>>> detect result in sysfs attribute.
>>>
>>> Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
>>> ---
>>>   
>> ...
>>
>>> +
>>> +static DEVICE_ATTR_WO(pir_threshold);
>>> +static DEVICE_ATTR_WO(pir_filter_step);
>>> +static DEVICE_ATTR_WO(pir_filter_type);
>>> +static DEVICE_ATTR_RO(pir_detector);
>>> +
>>> +static struct attribute *d3323aa_attrs[] = {
>>> +	&dev_attr_pir_threshold.attr,
>>> +	&dev_attr_pir_filter_step.attr,
>>> +	&dev_attr_pir_filter_type.attr,
>>> +	&dev_attr_pir_detector.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +ATTRIBUTE_GROUPS(d3323aa);
>>> +
>> I don't know what this is, but it is most definitely not a hardware
>> monitoring device. I don't see a definition of those attributes,
>> so I have no idea what they represent.
>>
>> Maybe this is an iio device, but given the unusual attributes
>> I am not even sure about that. Jonathan, any thoughts ?
> New type of sensor, but sure could be in IIO.
>
> Seems mostly a human presence sensor. Not that different from some
> types of proximity sensor and indeed that might be the path to take
> here.
>
> Taking a quick look at the driver suggests there is lots more information
> needed to understand the ABI.  At very least needs ABI docs so we can
> discuss how that is generalized. So if submitting an IIO driver
> I want to see
> Documenation/ABI/testing/sysfs-bus-iio-xxxx
> with significant detail. The datasheet provides no where near enough
> info.
>
> Jonathan

Thanks for your suggestions. For the new sensor, it seems require three 
attributes, e.g. /sys/bus/iio/devices/iio:deviceX/in_threshold 
(in_filter_step, in_filter_type), then one data to indicate if it is 
been triggerred (bool), but I'm not sure what is sutible IIO type could 
it used? Do you have any suggestion?

Best Regards,
Hermes

Re: [PATCH 1/2] dt-bindings: hwmon: Add D3-323-AA

[Krzysztof Kozlowski]

On Thu, Dec 12, 2024 at 12:24:09PM +0800, Hermes Zhang wrote:
> Add Devicetree binding documentation for Nicera D3-323-AA Pyroelectric
> IR sensor.
> 
> Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
> ---
>  .../bindings/hwmon/nicera,d3-323-aa.yaml      | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml b/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml
> new file mode 100644
> index 000000000000..31690e630b5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/nicera,d3-323-aa.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nicera D3-323-AA Pyroelectric IR sensor
> +
> +maintainers:
> +  - Hermes Zhang <Hermes.Zhang@axis.com>
> +
> +description: |
> +  Nicera D3-323-AA Pyroelectric IR sensor
> +
> +  datasheet:
> +    https://www.nicera.co.jp/wordpress/wp-content/uploads/2022/01/D3-323-AA_e.pdf
> +
> +properties:
> +  compatible:
> +    const: nicera,d3-323-aa

Undocumented vendor prefix.

It does not look like you tested the bindings, at least after quick
look. Please run 'make dt_binding_check' (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.

It does not look like you tested the DTS against bindings. Please run
'make dtbs_check W=1' (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.

> +
> +  reset-gpios:
> +    description: The GPIO pin connected to the reset pin on the sensor
> +    maxItems: 1
> +
> +  clk-gpios:
> +    description: The GPIO pin connected to the clk pin on the sensor

What is a clk pin? Usually clock pins are clocks, not GPIOs. This needs
explanation


> +    maxItems: 1
> +
> +  si-gpios:
> +    description: The GPIO pin connected to the si pin on the sensor

In all description please drop redundant pieces. It cannot be anything
else than GPIO pin and cannot be connected to something else than the
sensor.

So basically the only useful information you wrote above - after
dropping obvious and redundant pieces - is "si".

Write something useful, what is "si"? This applies also to "clk".
reset-gpios can be without description, because it is obvious - cannot
be anything else than reset pin. OTOH, you could say if pin is active
low or high.

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reset-gpios
> +  - clk-gpios
> +  - si-gpios
> +

Just one blank line.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    sensor {
> +        compatible = "nicera,d3-323-aa";
> +        reset-gpios = <&gpio4 12 0>;

Include the header and use standard defines for GPIO flags.

Best regards,
Krzysztof

Re: [PATCH 2/2] hwmon: Add support for D3-323-AA Pyroelectric IR sensor

[Jonathan Cameron]

On Fri, 13 Dec 2024 13:39:01 +0800
Hermes Zhang <chenhuiz@axis.com> wrote:

> Hi
> 
> On 2024/12/13 0:59, Jonathan Cameron wrote:
> > On Wed, 11 Dec 2024 22:17:49 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >  
> >> Hi,
> >>
> >> On 12/11/24 20:24, Hermes Zhang wrote:  
> >>> Add support for Nicera D3-323-AA Pyroelectric IR sensor. The sensor
> >>> support to config the threshold/filter_type/filter_step and return the
> >>> detect result in sysfs attribute.
> >>>
> >>> Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
> >>> ---
> >>>     
> >> ...
> >>  
> >>> +
> >>> +static DEVICE_ATTR_WO(pir_threshold);
> >>> +static DEVICE_ATTR_WO(pir_filter_step);
> >>> +static DEVICE_ATTR_WO(pir_filter_type);
> >>> +static DEVICE_ATTR_RO(pir_detector);
> >>> +
> >>> +static struct attribute *d3323aa_attrs[] = {
> >>> +	&dev_attr_pir_threshold.attr,
> >>> +	&dev_attr_pir_filter_step.attr,
> >>> +	&dev_attr_pir_filter_type.attr,
> >>> +	&dev_attr_pir_detector.attr,
> >>> +	NULL,
> >>> +};
> >>> +
> >>> +ATTRIBUTE_GROUPS(d3323aa);
> >>> +  
> >> I don't know what this is, but it is most definitely not a hardware
> >> monitoring device. I don't see a definition of those attributes,
> >> so I have no idea what they represent.
> >>
> >> Maybe this is an iio device, but given the unusual attributes
> >> I am not even sure about that. Jonathan, any thoughts ?  
> > New type of sensor, but sure could be in IIO.
> >
> > Seems mostly a human presence sensor. Not that different from some
> > types of proximity sensor and indeed that might be the path to take
> > here.
> >
> > Taking a quick look at the driver suggests there is lots more information
> > needed to understand the ABI.  At very least needs ABI docs so we can
> > discuss how that is generalized. So if submitting an IIO driver
> > I want to see
> > Documenation/ABI/testing/sysfs-bus-iio-xxxx
> > with significant detail. The datasheet provides no where near enough
> > info.
> >
> > Jonathan  
> 
> Thanks for your suggestions. For the new sensor, it seems require three 
> attributes, e.g. /sys/bus/iio/devices/iio:deviceX/in_threshold 
> (in_filter_step, in_filter_type), then one data to indicate if it is 
> been triggerred (bool), but I'm not sure what is sutible IIO type could 
> it used? Do you have any suggestion?

Look at the existing ABI in Documentation/ABI/testing/sysfs-bus-iio
and aim to fit within that scheme.

I'm hoping you have access to a datasheet that tells you something about the
filters that lets you map them to something standard.  Normally we aim
for something like 3DB frequency.  Filter types are harder but there tend to
only be so many types people actually build.

Channel type wise, I'm thinking this is kind of a form of proximity sensor
so IIO_PROXIMITY is probably appropriate.  That has always been a bit vague
as many proximity sensors are kind of 'there is something nearish' rather
than providing actual units etc.

It's a little different as I believe these only detect movement rather
than entirely static people, but in the case of the ones for presence detection
they work on tiny movements so more or less the same as detecting proximity.

Jonathan



> 
> Best Regards,
> Hermes
> 
>