[PATCH 0/3] iio: Add Everlight PM16D17 proximity sensor

[PATCH 0/3] iio: Add Everlight PM16D17 proximity sensor

[Jan Kiszka]

The Everlight PM16D17 is attached via I2C and can measure the proximity
of objects optically. Model it as IIO driver and include it into the
device tree of the IOT2050-SM where it is available on the main board.

Note that at the time of writing, the datasheet link is still work in
progress at Everlight (product listed, link still broken). We are in
contact with them.

Jan

Baocheng Su (1):
  dt-bindings: vendor-prefixes: Add EVERLIGHT

Chao Zeng (2):
  dt-bindings: iio: Add everlight pm16d17 binding
  iio: proximity: Add support for everlight pmd16d17 sensor

 .../iio/proximity/everlight,pm16d17.yaml      |  95 +++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/iio/proximity/Kconfig                 |  11 +
 drivers/iio/proximity/Makefile                |   1 +
 drivers/iio/proximity/pm16d17.c               | 324 ++++++++++++++++++
 5 files changed, 433 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
 create mode 100644 drivers/iio/proximity/pm16d17.c

-- 
2.43.0

[PATCH 1/3] dt-bindings: vendor-prefixes: Add EVERLIGHT

[Jan Kiszka]

From: Baocheng Su <baocheng.su@siemens.com>

Add vendor prefix for EVERLIGHT Electronics Co., Ltd.

Signed-off-by: Baocheng Su <baocheng.su@siemens.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 a70ce43b3dc0..1d2bf326fe91 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -486,6 +486,8 @@ patternProperties:
     description: Eukréa Electromatique
   "^everest,.*":
     description: Everest Semiconductor Co. Ltd.
+  "^everlight,.*":
+    description: EVERLIGHT Electronics Co., Ltd.
   "^everspin,.*":
     description: Everspin Technologies, Inc.
   "^evervision,.*":
-- 
2.43.0

[PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Jan Kiszka]

From: Chao Zeng <chao.zeng@siemens.com>

Add the binding document for the everlight pm16d17 sensor.

Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
---
 .../iio/proximity/everlight,pm16d17.yaml      | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml

diff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
new file mode 100644
index 000000000000..fadc3075181a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Everlight PM-16D17 Ambient Light & Proximity Sensor
+
+maintainers:
+  - Chao Zeng <chao.zeng@siemens.com>
+
+description: |
+  This sensor uses standard I2C interface. Interrupt function is not covered.
+  Datasheet: https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/
+
+properties:
+  compatible:
+    enum:
+      - everlight,pm16d17
+
+  reg:
+    maxItems: 1
+
+  ps-gain:
+    description: Receiver gain of proximity sensor
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 8]
+    default: 1
+
+  ps-itime:
+    description: Conversion time for proximity sensor [ms]
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+      - "0.4"
+      - "0.8"
+      - "1.6"
+      - "3.2"
+      - "6.3"
+      - "12.6"
+      - "25.2"
+    default: "0.4"
+
+  ps-wtime:
+    description: Waiting time for proximity sensor [ms]
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+      - "12.5"
+      - "25"
+      - "50"
+      - "100"
+      - "200"
+      - "400"
+      - "800"
+      - "1600"
+    default: "12.5"
+
+  ps-ir-led-pulse-count:
+    description: IR LED drive pulse count
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 256
+    default: 1
+
+  ps-offset-cancel:
+    description: |
+      When PS offset cancel function is enabled, the result of subtracting any
+      value specified by the PS offset cancel register from the internal PS
+      output data is written to the PS output data register.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+    maximum: 65535
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        lightsensor: pm16d17@44 {
+            compatible = "everlight,pm16d17";
+            reg = <0x44>;
+
+            ps-gain = <1>;
+            ps-itime = "0.4";
+            ps-wtime = "12.5";
+            ps-ir-led-pulse-count = <1>;
+            ps-offset-cancel = <280>;
+        };
+    };
-- 
2.43.0

[PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor

[Jan Kiszka]

From: Chao Zeng <chao.zeng@siemens.com>

Add initial support for everlight pm16d17 proximity sensor.

Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/iio/proximity/Kconfig   |  11 ++
 drivers/iio/proximity/Makefile  |   1 +
 drivers/iio/proximity/pm16d17.c | 324 ++++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/iio/proximity/pm16d17.c

diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 2ca3b0bc5eba..4c26bc3a0390 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -96,6 +96,17 @@ config PING
 	  To compile this driver as a module, choose M here: the
 	  module will be called ping.
 
+config PM16D17
+	tristate "PM16D17 proximity sensor"
+	select REGMAP_I2C
+	depends on I2C
+	help
+	 Say Y here to build a driver for the Everlight Devices
+	 PM16D17 proximity sensor.
+
+	 To compile this driver as a module, choose M here: the
+	 module will be called pm16d17.
+
 config RFD77402
 	tristate "RFD77402 ToF sensor"
 	depends on I2C
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index f36598380446..e41bba9c7cd3 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_ISL29501)		+= isl29501.o
 obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
 obj-$(CONFIG_MB1232)		+= mb1232.o
 obj-$(CONFIG_PING)		+= ping.o
+obj-$(CONFIG_PM16D17)		+= pm16d17.o
 obj-$(CONFIG_RFD77402)		+= rfd77402.o
 obj-$(CONFIG_SRF04)		+= srf04.o
 obj-$(CONFIG_SRF08)		+= srf08.o
diff --git a/drivers/iio/proximity/pm16d17.c b/drivers/iio/proximity/pm16d17.c
new file mode 100644
index 000000000000..94f21fc5e2fb
--- /dev/null
+++ b/drivers/iio/proximity/pm16d17.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Siemens AG, 2023-2024
+ *
+ * Driver for Everlight PM-16d17 proximity sensor
+ *
+ * Author: Chao Zeng <chao.zeng@siemens.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+#define PM16D17_DRV_NAME		"pm16d17"
+#define PM16D17_REGMAP_NAME		"pm16d17_regmap"
+
+/* Registers Address */
+#define PM16D17_OP_MODE			0x00
+#define PM16D17_INTERRUPT_FLAG		0x01
+#define PM16D17_PS_SETTING		0x0A
+#define PM16D17_VCSEL_DRIVE_CURRENT	0x0B
+#define PM16D17_VCSEL_DRIVE_PULSE	0x0C
+#define PM16D17_PS_INTUPT_LTHD_L	0x0D
+#define PM16D17_PS_INTUPT_LTHD_H	0x0E
+#define PM16D17_PS_INTUPT_HTHD_L	0x0F
+#define PM16D17_PS_INTUPT_HTHD_H	0x10
+#define PM16D17_PS_DATA_L		0x11
+#define PM16D17_PS_DATA_H		0x12
+#define PM16D17_PS_SETTING2		0x13
+#define PM16D17_PS_OFFSET_CANCEL_L	0x14
+#define PM16D17_PS_OFFSET_CANCEL_H	0x15
+#define PM16D17_DEV_ID			0x18
+
+#define DEVICE_ID			0x11
+
+#define ENABLE_PS_FUNCTION		BIT(3)
+#define PS_GAIN_MASK			GENMASK(7, 6)
+#define PS_ITIME_MASK			GENMASK(5, 3)
+#define PS_WTIME_MASK			GENMASK(2, 0)
+#define OFFSET_CANCEL_ENABLE		BIT(7)
+#define PS_OFFSET_CANCEL_LSB_MASK	GENMASK(7, 0)
+#define PS_OFFSET_CANCEL_MSB_MASK	GENMASK(15, 8)
+
+enum {
+	PITIME_0_POINT_4_MS = (0 << 3),
+	PITIME_0_POINT_8_MS = (1 << 3),
+	PITIME_1_POINT_6_MS = (2 << 3),
+	PITIME_3_POINT_2_MS = (3 << 3),
+	PITIME_6_POINT_3_MS = (4 << 3),
+	PITIME_12_POINT_6_MS = (5 << 3),
+	PITIME_25_POINT_2_MS = (6 << 3),
+};
+
+enum {
+	PWTIME_12_POINT_5_MS = 0,
+	PWTIME_25_MS,
+	PWTIME_50_MS,
+	PWTIME_100_MS,
+	PWTIME_200_MS,
+	PWTIME_400_MS,
+	PWTIME_800_MS,
+	PWTIME_1600_MS,
+};
+
+struct pm16d17_data {
+	struct i2c_client *client;
+	struct regmap *regmap;
+};
+
+static const struct regmap_config pm16d17_regmap_config = {
+	.name = PM16D17_REGMAP_NAME,
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_NONE,
+};
+
+static const struct iio_chan_spec pm16d17_channels[] = {
+	{
+		.type = IIO_PROXIMITY,
+		.indexed = 1,
+		.channel = 0,
+		.scan_index = -1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}
+};
+
+static inline int pm16d17_write_reg(struct pm16d17_data *data,
+				    unsigned int reg,
+				    unsigned int value)
+{
+	return regmap_write(data->regmap, reg, value);
+}
+
+static inline unsigned int pm16d17_read_reg(struct pm16d17_data *data,
+					    unsigned int reg,
+					    unsigned int *reg_val)
+{
+	return regmap_read(data->regmap, reg, reg_val);
+}
+
+static int pm16d17_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct pm16d17_data *data = iio_priv(indio_dev);
+	unsigned int ps_data_l;
+	unsigned int ps_data_h;
+	uint16_t ps_data;
+	int ret = -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = pm16d17_read_reg(data, PM16D17_PS_DATA_L, &ps_data_l);
+			if (ret < 0)
+				return ret;
+
+			ret = pm16d17_read_reg(data, PM16D17_PS_DATA_H, &ps_data_h);
+			if (ret < 0)
+				return ret;
+
+			ps_data = (ps_data_h << 8) | ps_data_l;
+
+			dev_dbg(&data->client->dev, "PS data: %x\n", ps_data);
+
+			*val = ps_data;
+			ret = IIO_VAL_INT;
+			break;
+		default:
+			break;
+		}
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static const struct iio_info pm16d17_info = {
+	.read_raw = pm16d17_read_raw,
+};
+
+static int pm16d17_chip_init(struct pm16d17_data *data)
+{
+	struct i2c_client *client = data->client;
+	struct device_node *np = client->dev.of_node;
+	const char *conv_time = NULL;
+	const char *wait_time = NULL;
+	uint8_t op_mode_setting_val;
+	uint32_t ps_offset_cancel;
+	uint8_t offset_lsb;
+	uint8_t offset_msb;
+	uint32_t pulse_count;
+	uint32_t pgain;
+	unsigned int val;
+	int ret;
+
+	ret = pm16d17_read_reg(data, PM16D17_DEV_ID, &val);
+
+	if (ret < 0 || (val != DEVICE_ID)) {
+		dev_err(&client->dev, "Invalid chip id 0x%04x\n", val);
+		return -ENODEV;
+	}
+
+	dev_dbg(&client->dev, "Detected PM16D17 with chip id: 0x%04x\n", val);
+
+	ret = pm16d17_write_reg(data, PM16D17_OP_MODE, ENABLE_PS_FUNCTION);
+	if (ret < 0)
+		return ret;
+
+	of_property_read_u32(np, "ps-gain", &pgain);
+	switch (pgain) {
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+		op_mode_setting_val |= (ilog2(pgain) << 6) & PS_GAIN_MASK;
+		break;
+	default:
+		break;
+	}
+
+	of_property_read_string(np, "ps-itime", &conv_time);
+	if (strcmp(conv_time, "0.4") == 0)
+		op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
+	else if (strcmp(conv_time, "0.8") == 0)
+		op_mode_setting_val |= PITIME_0_POINT_8_MS & PS_ITIME_MASK;
+	else if (strcmp(conv_time, "1.6") == 0)
+		op_mode_setting_val |= PITIME_1_POINT_6_MS & PS_ITIME_MASK;
+	else if (strcmp(conv_time, "3.2") == 0)
+		op_mode_setting_val |= PITIME_3_POINT_2_MS & PS_ITIME_MASK;
+	else if (strcmp(conv_time, "6.3") == 0)
+		op_mode_setting_val |= PITIME_6_POINT_3_MS & PS_ITIME_MASK;
+	else if (strcmp(conv_time, "12.6") == 0)
+		op_mode_setting_val |= PITIME_12_POINT_6_MS & PS_ITIME_MASK;
+	else if (strcmp(conv_time, "25.2") == 0)
+		op_mode_setting_val |= PITIME_25_POINT_2_MS & PS_ITIME_MASK;
+	else {
+		dev_info(&client->dev, "Using default ps itime value\n");
+		op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
+	}
+
+	of_property_read_string(np, "ps-wtime", &wait_time);
+	if (strcmp(wait_time, "12.5") == 0)
+		op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
+	else if (strcmp(wait_time, "25") == 0)
+		op_mode_setting_val |= PWTIME_25_MS & PS_WTIME_MASK;
+	else if (strcmp(wait_time, "50") == 0)
+		op_mode_setting_val |= PWTIME_50_MS & PS_WTIME_MASK;
+	else if (strcmp(wait_time, "100") == 0)
+		op_mode_setting_val |= PWTIME_100_MS & PS_WTIME_MASK;
+	else if (strcmp(wait_time, "200") == 0)
+		op_mode_setting_val |= PWTIME_200_MS & PS_WTIME_MASK;
+	else if (strcmp(wait_time, "400") == 0)
+		op_mode_setting_val |= PWTIME_400_MS & PS_WTIME_MASK;
+	else if (strcmp(wait_time, "800") == 0)
+		op_mode_setting_val |= PWTIME_800_MS & PS_WTIME_MASK;
+	else if (strcmp(wait_time, "1600") == 0)
+		op_mode_setting_val |= PWTIME_1600_MS & PS_WTIME_MASK;
+	else {
+		dev_info(&client->dev, "Using default ps wtime value\n");
+		op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
+	}
+
+	ret = pm16d17_write_reg(data, PM16D17_PS_SETTING, op_mode_setting_val);
+	if (ret < 0)
+		return ret;
+
+	of_property_read_u32(np, "ps-ir-led-pulse-count", &pulse_count);
+	if (pulse_count > 256)
+		pulse_count = 256;
+	ret = pm16d17_write_reg(data, PM16D17_VCSEL_DRIVE_PULSE, pulse_count - 1);
+	if (ret < 0)
+		return ret;
+
+	of_property_read_u32(np, "ps-offset-cancel", &ps_offset_cancel);
+	if (ps_offset_cancel != 0) {
+		ret = pm16d17_write_reg(data, PM16D17_PS_SETTING2, OFFSET_CANCEL_ENABLE);
+		if (ret < 0)
+			return ret;
+
+		offset_lsb = ps_offset_cancel & PS_OFFSET_CANCEL_LSB_MASK;
+		offset_msb = (ps_offset_cancel & PS_OFFSET_CANCEL_MSB_MASK) >> 8;
+
+		ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_L, offset_lsb);
+		if (ret < 0)
+			return ret;
+
+		ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_H, offset_msb);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int pm16d17_probe(struct i2c_client *client)
+{
+	struct pm16d17_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &pm16d17_info;
+	indio_dev->name = PM16D17_DRV_NAME;
+	indio_dev->channels = pm16d17_channels;
+	indio_dev->num_channels = ARRAY_SIZE(pm16d17_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+
+	data->regmap = devm_regmap_init_i2c(client, &pm16d17_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(&client->dev, "regmap initialization failed.\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	ret = pm16d17_chip_init(data);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id pm16d17_id[] = {
+	{"pm16d17", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, pm16d17_id);
+
+static const struct of_device_id pm16d17_of_match[] = {
+	{ .compatible = "everlight,pm16d17" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pm16d17_of_match);
+
+static struct i2c_driver pm16d17_driver = {
+	.driver = {
+		.name = PM16D17_DRV_NAME,
+		.of_match_table = pm16d17_of_match,
+	},
+	.probe = pm16d17_probe,
+	.id_table = pm16d17_id,
+};
+module_i2c_driver(pm16d17_driver);
+
+MODULE_AUTHOR("Chao Zeng <chao.zeng@siemens.com>");
+MODULE_DESCRIPTION("PM16D17 proximity sensor");
+MODULE_LICENSE("GPL");
-- 
2.43.0

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On Tue, Aug 13, 2024 at 07:40:40AM +0200, Jan Kiszka wrote:
> From: Baocheng Su <baocheng.su@siemens.com>
> 
> Add vendor prefix for EVERLIGHT Electronics Co., Ltd.
> 
> Signed-off-by: Baocheng Su <baocheng.su@siemens.com>

This is missing your signoff Jan. With it,
Acked-by: Conor Dooley <conor.dooley@microchip.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 a70ce43b3dc0..1d2bf326fe91 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -486,6 +486,8 @@ patternProperties:
>      description: Eukréa Electromatique
>    "^everest,.*":
>      description: Everest Semiconductor Co. Ltd.
> +  "^everlight,.*":
> +    description: EVERLIGHT Electronics Co., Ltd.
>    "^everspin,.*":
>      description: Everspin Technologies, Inc.
>    "^evervision,.*":
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Krzysztof Kozlowski]

On 13/08/2024 17:41, Conor Dooley wrote:
> On Tue, Aug 13, 2024 at 07:40:40AM +0200, Jan Kiszka wrote:
>> From: Baocheng Su <baocheng.su@siemens.com>
>>
>> Add vendor prefix for EVERLIGHT Electronics Co., Ltd.
>>
>> Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
> 
> This is missing your signoff Jan. With it,
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

The next one as well...

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4815 bytes --]

On Tue, Aug 13, 2024 at 07:40:41AM +0200, Jan Kiszka wrote:
> From: Chao Zeng <chao.zeng@siemens.com>
> 
> Add the binding document for the everlight pm16d17 sensor.
> 
> Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> Signed-off-by: Baocheng Su <baocheng.su@siemens.com>

Ditto here Jan.

> ---
>  .../iio/proximity/everlight,pm16d17.yaml      | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
> new file mode 100644
> index 000000000000..fadc3075181a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Everlight PM-16D17 Ambient Light & Proximity Sensor
> +
> +maintainers:
> +  - Chao Zeng <chao.zeng@siemens.com>
> +
> +description: |
> +  This sensor uses standard I2C interface. Interrupt function is not covered.

Bindings should be complete, even if software doesn't use the
interrupts. Can you document them please.

> +  Datasheet: https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/

Do you have a link to a datasheet? The link to the pm16d17 here 404s for
me.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - everlight,pm16d17
> +
> +  reg:
> +    maxItems: 1
> +
> +  ps-gain:
> +    description: Receiver gain of proximity sensor
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 8]
> +    default: 1
> +
> +  ps-itime:

How did you get itime from conversion time? To the layman (like me!)
conversion-time would make more sense...

Also, "ps"? The whole thing is a proxy sensor, so why have that prefix
on properties. What is missing however is a vendor prefix.

> +    description: Conversion time for proximity sensor [ms]
> +    $ref: /schemas/types.yaml#/definitions/string

Instead of a string, please use the -us suffix, and put this in
microseconds instead.

In total, that would be s/ps-itime/everlight,conversion-time-us/.

I would, however, like to know why this is a property of the hardware.
What factors do you have to consider when determining what value to put
in here?

> +    enum:
> +      - "0.4"
> +      - "0.8"
> +      - "1.6"
> +      - "3.2"
> +      - "6.3"
> +      - "12.6"
> +      - "25.2"
> +    default: "0.4"
> +
> +  ps-wtime:
> +    description: Waiting time for proximity sensor [ms]
> +    $ref: /schemas/types.yaml#/definitions/string

All of the same comments apply here. E.g. why "wtime" isntead of
"waiting-time" and so on.
I would really like to know why these things are properties of the
hardware, rather than something that software should control.

> +    enum:
> +      - "12.5"
> +      - "25"
> +      - "50"
> +      - "100"
> +      - "200"
> +      - "400"
> +      - "800"
> +      - "1600"
> +    default: "12.5"
> +
> +  ps-ir-led-pulse-count:
> +    description: IR LED drive pulse count
> +    $ref: /schemas/types.yaml#/definitions/uint32

All custom properties require a vendor prefix, not "ps". Again, what
makes this a property of the hardware, rather than something that
software should control?

> +    minimum: 1
> +    maximum: 256
> +    default: 1
> +
> +  ps-offset-cancel:
> +    description: |
> +      When PS offset cancel function is enabled, the result of subtracting any
> +      value specified by the PS offset cancel register from the internal PS
> +      output data is written to the PS output data register.

Again, what makes this a property of the hardware? What hardware related
factors determine that value that you put in here?

Thanks,
Conor.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0
> +    maximum: 65535
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        lightsensor: pm16d17@44 {
> +            compatible = "everlight,pm16d17";
> +            reg = <0x44>;
> +
> +            ps-gain = <1>;
> +            ps-itime = "0.4";
> +            ps-wtime = "12.5";
> +            ps-ir-led-pulse-count = <1>;
> +            ps-offset-cancel = <280>;
> +        };
> +    };
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

On Tue, Aug 13, 2024 at 05:46:28PM +0200, Krzysztof Kozlowski wrote:
> On 13/08/2024 17:41, Conor Dooley wrote:
> > On Tue, Aug 13, 2024 at 07:40:40AM +0200, Jan Kiszka wrote:
> >> From: Baocheng Su <baocheng.su@siemens.com>
> >>
> >> Add vendor prefix for EVERLIGHT Electronics Co., Ltd.
> >>
> >> Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
> > 
> > This is missing your signoff Jan. With it,
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 
> The next one as well...

I had a longer reply for the other one ;)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Jonathan Cameron]

On Tue, 13 Aug 2024 07:40:41 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> From: Chao Zeng <chao.zeng@siemens.com>
> 
> Add the binding document for the everlight pm16d17 sensor.
> 
> Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
hi Jan,

From a first read at least, almost everything in here
is stuff we should be controlling from the driver, not
providing as fixed values from firmware.

Specific comments inline.

Jonathan

> ---
>  .../iio/proximity/everlight,pm16d17.yaml      | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
> new file mode 100644
> index 000000000000..fadc3075181a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Everlight PM-16D17 Ambient Light & Proximity Sensor
> +
> +maintainers:
> +  - Chao Zeng <chao.zeng@siemens.com>
> +
> +description: |
> +  This sensor uses standard I2C interface. Interrupt function is not covered.
> +  Datasheet: https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/
> +
> +properties:
> +  compatible:
> +    enum:
> +      - everlight,pm16d17
> +
> +  reg:
> +    maxItems: 1
> +
> +  ps-gain:
> +    description: Receiver gain of proximity sensor
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 8]
> +    default: 1

This should I think be a userspace control.
Given it's not related to proximity as such, probably 
in_proximity0_hardwaregain

> +
> +  ps-itime:
> +    description: Conversion time for proximity sensor [ms]
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum:
> +      - "0.4"
> +      - "0.8"
> +      - "1.6"
> +      - "3.2"
> +      - "6.3"
> +      - "12.6"
> +      - "25.2"
> +    default: "0.4"
Definitely a userspace control.  Is this actually integration time
which we'd expect to affect the hardwaregain or is it just
1/ frequency of sampling (with fixed integration time).
Looking at datasheet it's coupled to resolution which may
make this oversampling related. Hard to tell.

> +
> +  ps-wtime:
> +    description: Waiting time for proximity sensor [ms]
I guess the above was the integration time and this sets
the sampling_frequency.  In that case definitely a userspace
thing, doesn't belong in DT.

> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum:
> +      - "12.5"
> +      - "25"
> +      - "50"
> +      - "100"
> +      - "200"
> +      - "400"
> +      - "800"
> +      - "1600"
> +    default: "12.5"
> +
> +  ps-ir-led-pulse-count:
> +    description: IR LED drive pulse count

This needs more information.  Why would this be changed?
Seems from datasheet that this is effectively a different
form of gain. Why would we choose one rather than the other?
Or are they both ways of increasing the overall sensitivity?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 256
> +    default: 1
> +
> +  ps-offset-cancel:
> +    description: |
> +      When PS offset cancel function is enabled, the result of subtracting any
> +      value specified by the PS offset cancel register from the internal PS
> +      output data is written to the PS output data register.
That sounds like a calibbias userspace control, but more info needed.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0
> +    maximum: 65535
> +
As Conor mentioned, need to describe the hardware as fully as possible so interrupts
and power supplies (even if they are always on for your particular board)

> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        lightsensor: pm16d17@44 {
> +            compatible = "everlight,pm16d17";
> +            reg = <0x44>;
> +
> +            ps-gain = <1>;
> +            ps-itime = "0.4";
> +            ps-wtime = "12.5";
> +            ps-ir-led-pulse-count = <1>;
> +            ps-offset-cancel = <280>;
> +        };
> +    };

Re: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor

[kernel test robot]

Hi Jan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.11-rc3 next-20240814]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jan-Kiszka/dt-bindings-vendor-prefixes-Add-EVERLIGHT/20240814-234801
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/abb0c1c0724be733138276f638e43e98784bd191.1723527641.git.jan.kiszka%40siemens.com
patch subject: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240815/202408151352.u8v6HOIQ-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240815/202408151352.u8v6HOIQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408151352.u8v6HOIQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/proximity/pm16d17.c:142:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
     142 |         default:
         |         ^
   drivers/iio/proximity/pm16d17.c:142:2: note: insert 'break;' to avoid fall-through
     142 |         default:
         |         ^
         |         break; 
>> drivers/iio/proximity/pm16d17.c:187:3: warning: variable 'op_mode_setting_val' is uninitialized when used here [-Wuninitialized]
     187 |                 op_mode_setting_val |= (ilog2(pgain) << 6) & PS_GAIN_MASK;
         |                 ^~~~~~~~~~~~~~~~~~~
   drivers/iio/proximity/pm16d17.c:159:29: note: initialize the variable 'op_mode_setting_val' to silence this warning
     159 |         uint8_t op_mode_setting_val;
         |                                    ^
         |                                     = '\0'
   2 warnings generated.


vim +142 drivers/iio/proximity/pm16d17.c

   109	
   110	static int pm16d17_read_raw(struct iio_dev *indio_dev,
   111				    struct iio_chan_spec const *chan,
   112				    int *val, int *val2, long mask)
   113	{
   114		struct pm16d17_data *data = iio_priv(indio_dev);
   115		unsigned int ps_data_l;
   116		unsigned int ps_data_h;
   117		uint16_t ps_data;
   118		int ret = -EINVAL;
   119	
   120		switch (mask) {
   121		case IIO_CHAN_INFO_RAW:
   122			switch (chan->type) {
   123			case IIO_PROXIMITY:
   124				ret = pm16d17_read_reg(data, PM16D17_PS_DATA_L, &ps_data_l);
   125				if (ret < 0)
   126					return ret;
   127	
   128				ret = pm16d17_read_reg(data, PM16D17_PS_DATA_H, &ps_data_h);
   129				if (ret < 0)
   130					return ret;
   131	
   132				ps_data = (ps_data_h << 8) | ps_data_l;
   133	
   134				dev_dbg(&data->client->dev, "PS data: %x\n", ps_data);
   135	
   136				*val = ps_data;
   137				ret = IIO_VAL_INT;
   138				break;
   139			default:
   140				break;
   141			}
 > 142		default:
   143			break;
   144		}
   145	
   146		return ret;
   147	}
   148	
   149	static const struct iio_info pm16d17_info = {
   150		.read_raw = pm16d17_read_raw,
   151	};
   152	
   153	static int pm16d17_chip_init(struct pm16d17_data *data)
   154	{
   155		struct i2c_client *client = data->client;
   156		struct device_node *np = client->dev.of_node;
   157		const char *conv_time = NULL;
   158		const char *wait_time = NULL;
   159		uint8_t op_mode_setting_val;
   160		uint32_t ps_offset_cancel;
   161		uint8_t offset_lsb;
   162		uint8_t offset_msb;
   163		uint32_t pulse_count;
   164		uint32_t pgain;
   165		unsigned int val;
   166		int ret;
   167	
   168		ret = pm16d17_read_reg(data, PM16D17_DEV_ID, &val);
   169	
   170		if (ret < 0 || (val != DEVICE_ID)) {
   171			dev_err(&client->dev, "Invalid chip id 0x%04x\n", val);
   172			return -ENODEV;
   173		}
   174	
   175		dev_dbg(&client->dev, "Detected PM16D17 with chip id: 0x%04x\n", val);
   176	
   177		ret = pm16d17_write_reg(data, PM16D17_OP_MODE, ENABLE_PS_FUNCTION);
   178		if (ret < 0)
   179			return ret;
   180	
   181		of_property_read_u32(np, "ps-gain", &pgain);
   182		switch (pgain) {
   183		case 1:
   184		case 2:
   185		case 4:
   186		case 8:
 > 187			op_mode_setting_val |= (ilog2(pgain) << 6) & PS_GAIN_MASK;
   188			break;
   189		default:
   190			break;
   191		}
   192	
   193		of_property_read_string(np, "ps-itime", &conv_time);
   194		if (strcmp(conv_time, "0.4") == 0)
   195			op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
   196		else if (strcmp(conv_time, "0.8") == 0)
   197			op_mode_setting_val |= PITIME_0_POINT_8_MS & PS_ITIME_MASK;
   198		else if (strcmp(conv_time, "1.6") == 0)
   199			op_mode_setting_val |= PITIME_1_POINT_6_MS & PS_ITIME_MASK;
   200		else if (strcmp(conv_time, "3.2") == 0)
   201			op_mode_setting_val |= PITIME_3_POINT_2_MS & PS_ITIME_MASK;
   202		else if (strcmp(conv_time, "6.3") == 0)
   203			op_mode_setting_val |= PITIME_6_POINT_3_MS & PS_ITIME_MASK;
   204		else if (strcmp(conv_time, "12.6") == 0)
   205			op_mode_setting_val |= PITIME_12_POINT_6_MS & PS_ITIME_MASK;
   206		else if (strcmp(conv_time, "25.2") == 0)
   207			op_mode_setting_val |= PITIME_25_POINT_2_MS & PS_ITIME_MASK;
   208		else {
   209			dev_info(&client->dev, "Using default ps itime value\n");
   210			op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
   211		}
   212	
   213		of_property_read_string(np, "ps-wtime", &wait_time);
   214		if (strcmp(wait_time, "12.5") == 0)
   215			op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
   216		else if (strcmp(wait_time, "25") == 0)
   217			op_mode_setting_val |= PWTIME_25_MS & PS_WTIME_MASK;
   218		else if (strcmp(wait_time, "50") == 0)
   219			op_mode_setting_val |= PWTIME_50_MS & PS_WTIME_MASK;
   220		else if (strcmp(wait_time, "100") == 0)
   221			op_mode_setting_val |= PWTIME_100_MS & PS_WTIME_MASK;
   222		else if (strcmp(wait_time, "200") == 0)
   223			op_mode_setting_val |= PWTIME_200_MS & PS_WTIME_MASK;
   224		else if (strcmp(wait_time, "400") == 0)
   225			op_mode_setting_val |= PWTIME_400_MS & PS_WTIME_MASK;
   226		else if (strcmp(wait_time, "800") == 0)
   227			op_mode_setting_val |= PWTIME_800_MS & PS_WTIME_MASK;
   228		else if (strcmp(wait_time, "1600") == 0)
   229			op_mode_setting_val |= PWTIME_1600_MS & PS_WTIME_MASK;
   230		else {
   231			dev_info(&client->dev, "Using default ps wtime value\n");
   232			op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
   233		}
   234	
   235		ret = pm16d17_write_reg(data, PM16D17_PS_SETTING, op_mode_setting_val);
   236		if (ret < 0)
   237			return ret;
   238	
   239		of_property_read_u32(np, "ps-ir-led-pulse-count", &pulse_count);
   240		if (pulse_count > 256)
   241			pulse_count = 256;
   242		ret = pm16d17_write_reg(data, PM16D17_VCSEL_DRIVE_PULSE, pulse_count - 1);
   243		if (ret < 0)
   244			return ret;
   245	
   246		of_property_read_u32(np, "ps-offset-cancel", &ps_offset_cancel);
   247		if (ps_offset_cancel != 0) {
   248			ret = pm16d17_write_reg(data, PM16D17_PS_SETTING2, OFFSET_CANCEL_ENABLE);
   249			if (ret < 0)
   250				return ret;
   251	
   252			offset_lsb = ps_offset_cancel & PS_OFFSET_CANCEL_LSB_MASK;
   253			offset_msb = (ps_offset_cancel & PS_OFFSET_CANCEL_MSB_MASK) >> 8;
   254	
   255			ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_L, offset_lsb);
   256			if (ret < 0)
   257				return ret;
   258	
   259			ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_H, offset_msb);
   260			if (ret < 0)
   261				return ret;
   262		}
   263	
   264		return 0;
   265	}
   266	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Jan Kiszka]

On 14.08.24 21:10, Jonathan Cameron wrote:
> On Tue, 13 Aug 2024 07:40:41 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> From: Chao Zeng <chao.zeng@siemens.com>
>>
>> Add the binding document for the everlight pm16d17 sensor.
>>
>> Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
>> Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
>> Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
> hi Jan,
> 
> From a first read at least, almost everything in here
> is stuff we should be controlling from the driver, not
> providing as fixed values from firmware.
> 
> Specific comments inline.
> 

Thanks for the feedback. I'm looping in my colleague Hua Quian who will
try to answer your valid questions ASAP.

Good news: The datasheet link is now working, see
https://www.everlight.com.cn/wp-content/plugins/ItemRelationship/product_files/pdf/PM-16D17-2010-DF6-TR8_datasheet_V1.pdf

We just hope that we don't need to expand the IIO subsystem for this
device. ;)

Jan

> Jonathan
> 
>> ---
>>  .../iio/proximity/everlight,pm16d17.yaml      | 95 +++++++++++++++++++
>>  1 file changed, 95 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
>> new file mode 100644
>> index 000000000000..fadc3075181a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
>> @@ -0,0 +1,95 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Everlight PM-16D17 Ambient Light & Proximity Sensor
>> +
>> +maintainers:
>> +  - Chao Zeng <chao.zeng@siemens.com>
>> +
>> +description: |
>> +  This sensor uses standard I2C interface. Interrupt function is not covered.
>> +  Datasheet: https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - everlight,pm16d17
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  ps-gain:
>> +    description: Receiver gain of proximity sensor
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 4, 8]
>> +    default: 1
> 
> This should I think be a userspace control.
> Given it's not related to proximity as such, probably 
> in_proximity0_hardwaregain
> 
>> +
>> +  ps-itime:
>> +    description: Conversion time for proximity sensor [ms]
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum:
>> +      - "0.4"
>> +      - "0.8"
>> +      - "1.6"
>> +      - "3.2"
>> +      - "6.3"
>> +      - "12.6"
>> +      - "25.2"
>> +    default: "0.4"
> Definitely a userspace control.  Is this actually integration time
> which we'd expect to affect the hardwaregain or is it just
> 1/ frequency of sampling (with fixed integration time).
> Looking at datasheet it's coupled to resolution which may
> make this oversampling related. Hard to tell.
> 
>> +
>> +  ps-wtime:
>> +    description: Waiting time for proximity sensor [ms]
> I guess the above was the integration time and this sets
> the sampling_frequency.  In that case definitely a userspace
> thing, doesn't belong in DT.
> 
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum:
>> +      - "12.5"
>> +      - "25"
>> +      - "50"
>> +      - "100"
>> +      - "200"
>> +      - "400"
>> +      - "800"
>> +      - "1600"
>> +    default: "12.5"
>> +
>> +  ps-ir-led-pulse-count:
>> +    description: IR LED drive pulse count
> 
> This needs more information.  Why would this be changed?
> Seems from datasheet that this is effectively a different
> form of gain. Why would we choose one rather than the other?
> Or are they both ways of increasing the overall sensitivity?
> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 256
>> +    default: 1
>> +
>> +  ps-offset-cancel:
>> +    description: |
>> +      When PS offset cancel function is enabled, the result of subtracting any
>> +      value specified by the PS offset cancel register from the internal PS
>> +      output data is written to the PS output data register.
> That sounds like a calibbias userspace control, but more info needed.
> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    default: 0
>> +    maximum: 65535
>> +
> As Conor mentioned, need to describe the hardware as fully as possible so interrupts
> and power supplies (even if they are always on for your particular board)
> 
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        lightsensor: pm16d17@44 {
>> +            compatible = "everlight,pm16d17";
>> +            reg = <0x44>;
>> +
>> +            ps-gain = <1>;
>> +            ps-itime = "0.4";
>> +            ps-wtime = "12.5";
>> +            ps-ir-led-pulse-count = <1>;
>> +            ps-offset-cancel = <280>;
>> +        };
>> +    };
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor

[kernel test robot]

Hi Jan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.11-rc3 next-20240815]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jan-Kiszka/dt-bindings-vendor-prefixes-Add-EVERLIGHT/20240814-234801
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/abb0c1c0724be733138276f638e43e98784bd191.1723527641.git.jan.kiszka%40siemens.com
patch subject: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor
config: x86_64-randconfig-101-20240816 (https://download.01.org/0day-ci/archive/20240816/202408160542.UEjIkjkC-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240816/202408160542.UEjIkjkC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408160542.UEjIkjkC-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/iio/proximity/pm16d17.c: In function 'pm16d17_chip_init':
>> drivers/iio/proximity/pm16d17.c:194:13: warning: argument 1 null where non-null expected [-Wnonnull]
     194 |         if (strcmp(conv_time, "0.4") == 0)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/bitmap.h:13,
                    from include/linux/cpumask.h:12,
                    from arch/x86/include/asm/tlbbatch.h:5,
                    from include/linux/mm_types_task.h:16,
                    from include/linux/sched.h:38,
                    from include/linux/delay.h:23,
                    from drivers/iio/proximity/pm16d17.c:11:
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:196:18: warning: argument 1 null where non-null expected [-Wnonnull]
     196 |         else if (strcmp(conv_time, "0.8") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:198:18: warning: argument 1 null where non-null expected [-Wnonnull]
     198 |         else if (strcmp(conv_time, "1.6") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:200:18: warning: argument 1 null where non-null expected [-Wnonnull]
     200 |         else if (strcmp(conv_time, "3.2") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:202:18: warning: argument 1 null where non-null expected [-Wnonnull]
     202 |         else if (strcmp(conv_time, "6.3") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:204:18: warning: argument 1 null where non-null expected [-Wnonnull]
     204 |         else if (strcmp(conv_time, "12.6") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:206:18: warning: argument 1 null where non-null expected [-Wnonnull]
     206 |         else if (strcmp(conv_time, "25.2") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:214:13: warning: argument 1 null where non-null expected [-Wnonnull]
     214 |         if (strcmp(wait_time, "12.5") == 0)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:216:18: warning: argument 1 null where non-null expected [-Wnonnull]
     216 |         else if (strcmp(wait_time, "25") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:218:18: warning: argument 1 null where non-null expected [-Wnonnull]
     218 |         else if (strcmp(wait_time, "50") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:220:18: warning: argument 1 null where non-null expected [-Wnonnull]
     220 |         else if (strcmp(wait_time, "100") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:222:18: warning: argument 1 null where non-null expected [-Wnonnull]
     222 |         else if (strcmp(wait_time, "200") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:224:18: warning: argument 1 null where non-null expected [-Wnonnull]
     224 |         else if (strcmp(wait_time, "400") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:226:18: warning: argument 1 null where non-null expected [-Wnonnull]
     226 |         else if (strcmp(wait_time, "800") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~
   drivers/iio/proximity/pm16d17.c:228:18: warning: argument 1 null where non-null expected [-Wnonnull]
     228 |         else if (strcmp(wait_time, "1600") == 0)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
     156 | extern int strcmp(const char *,const char *);
         |            ^~~~~~


vim +194 drivers/iio/proximity/pm16d17.c

   152	
   153	static int pm16d17_chip_init(struct pm16d17_data *data)
   154	{
   155		struct i2c_client *client = data->client;
   156		struct device_node *np = client->dev.of_node;
   157		const char *conv_time = NULL;
   158		const char *wait_time = NULL;
   159		uint8_t op_mode_setting_val;
   160		uint32_t ps_offset_cancel;
   161		uint8_t offset_lsb;
   162		uint8_t offset_msb;
   163		uint32_t pulse_count;
   164		uint32_t pgain;
   165		unsigned int val;
   166		int ret;
   167	
   168		ret = pm16d17_read_reg(data, PM16D17_DEV_ID, &val);
   169	
   170		if (ret < 0 || (val != DEVICE_ID)) {
   171			dev_err(&client->dev, "Invalid chip id 0x%04x\n", val);
   172			return -ENODEV;
   173		}
   174	
   175		dev_dbg(&client->dev, "Detected PM16D17 with chip id: 0x%04x\n", val);
   176	
   177		ret = pm16d17_write_reg(data, PM16D17_OP_MODE, ENABLE_PS_FUNCTION);
   178		if (ret < 0)
   179			return ret;
   180	
   181		of_property_read_u32(np, "ps-gain", &pgain);
   182		switch (pgain) {
   183		case 1:
   184		case 2:
   185		case 4:
   186		case 8:
   187			op_mode_setting_val |= (ilog2(pgain) << 6) & PS_GAIN_MASK;
   188			break;
   189		default:
   190			break;
   191		}
   192	
   193		of_property_read_string(np, "ps-itime", &conv_time);
 > 194		if (strcmp(conv_time, "0.4") == 0)
   195			op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
   196		else if (strcmp(conv_time, "0.8") == 0)
   197			op_mode_setting_val |= PITIME_0_POINT_8_MS & PS_ITIME_MASK;
   198		else if (strcmp(conv_time, "1.6") == 0)
   199			op_mode_setting_val |= PITIME_1_POINT_6_MS & PS_ITIME_MASK;
   200		else if (strcmp(conv_time, "3.2") == 0)
   201			op_mode_setting_val |= PITIME_3_POINT_2_MS & PS_ITIME_MASK;
   202		else if (strcmp(conv_time, "6.3") == 0)
   203			op_mode_setting_val |= PITIME_6_POINT_3_MS & PS_ITIME_MASK;
   204		else if (strcmp(conv_time, "12.6") == 0)
   205			op_mode_setting_val |= PITIME_12_POINT_6_MS & PS_ITIME_MASK;
   206		else if (strcmp(conv_time, "25.2") == 0)
   207			op_mode_setting_val |= PITIME_25_POINT_2_MS & PS_ITIME_MASK;
   208		else {
   209			dev_info(&client->dev, "Using default ps itime value\n");
   210			op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
   211		}
   212	
   213		of_property_read_string(np, "ps-wtime", &wait_time);
   214		if (strcmp(wait_time, "12.5") == 0)
   215			op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
   216		else if (strcmp(wait_time, "25") == 0)
   217			op_mode_setting_val |= PWTIME_25_MS & PS_WTIME_MASK;
   218		else if (strcmp(wait_time, "50") == 0)
   219			op_mode_setting_val |= PWTIME_50_MS & PS_WTIME_MASK;
   220		else if (strcmp(wait_time, "100") == 0)
   221			op_mode_setting_val |= PWTIME_100_MS & PS_WTIME_MASK;
   222		else if (strcmp(wait_time, "200") == 0)
   223			op_mode_setting_val |= PWTIME_200_MS & PS_WTIME_MASK;
   224		else if (strcmp(wait_time, "400") == 0)
   225			op_mode_setting_val |= PWTIME_400_MS & PS_WTIME_MASK;
   226		else if (strcmp(wait_time, "800") == 0)
   227			op_mode_setting_val |= PWTIME_800_MS & PS_WTIME_MASK;
   228		else if (strcmp(wait_time, "1600") == 0)
   229			op_mode_setting_val |= PWTIME_1600_MS & PS_WTIME_MASK;
   230		else {
   231			dev_info(&client->dev, "Using default ps wtime value\n");
   232			op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
   233		}
   234	
   235		ret = pm16d17_write_reg(data, PM16D17_PS_SETTING, op_mode_setting_val);
   236		if (ret < 0)
   237			return ret;
   238	
   239		of_property_read_u32(np, "ps-ir-led-pulse-count", &pulse_count);
   240		if (pulse_count > 256)
   241			pulse_count = 256;
   242		ret = pm16d17_write_reg(data, PM16D17_VCSEL_DRIVE_PULSE, pulse_count - 1);
   243		if (ret < 0)
   244			return ret;
   245	
   246		of_property_read_u32(np, "ps-offset-cancel", &ps_offset_cancel);
   247		if (ps_offset_cancel != 0) {
   248			ret = pm16d17_write_reg(data, PM16D17_PS_SETTING2, OFFSET_CANCEL_ENABLE);
   249			if (ret < 0)
   250				return ret;
   251	
   252			offset_lsb = ps_offset_cancel & PS_OFFSET_CANCEL_LSB_MASK;
   253			offset_msb = (ps_offset_cancel & PS_OFFSET_CANCEL_MSB_MASK) >> 8;
   254	
   255			ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_L, offset_lsb);
   256			if (ret < 0)
   257				return ret;
   258	
   259			ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_H, offset_msb);
   260			if (ret < 0)
   261				return ret;
   262		}
   263	
   264		return 0;
   265	}
   266	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Li, Hua Qian]

On Tue, 2024-08-13 at 16:52 +0100, Conor Dooley wrote:
> On Tue, Aug 13, 2024 at 07:40:41AM +0200, Jan Kiszka wrote:
> > From: Chao Zeng <chao.zeng@siemens.com>
> > 
> > Add the binding document for the everlight pm16d17 sensor.
> > 
> > Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> > Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> > Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
> 
> Ditto here Jan.
> 
> > ---
> >  .../iio/proximity/everlight,pm16d17.yaml      | 95
> > +++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.y
> > aml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > .yaml
> > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > .yaml
> > new file mode 100644
> > index 000000000000..fadc3075181a
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > .yaml
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Everlight PM-16D17 Ambient Light & Proximity Sensor
> > +
> > +maintainers:
> > +  - Chao Zeng <chao.zeng@siemens.com>
> > +
> > +description: |
> > +  This sensor uses standard I2C interface. Interrupt function is
> > not covered.
> 
> Bindings should be complete, even if software doesn't use the
> interrupts. Can you document them please.
> 
> > +  Datasheet:
> > https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/
> 
> Do you have a link to a datasheet? The link to the pm16d17 here 404s
> for
> me.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - everlight,pm16d17
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  ps-gain:
> > +    description: Receiver gain of proximity sensor
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2, 4, 8]
> > +    default: 1
> > +
> > +  ps-itime:
> 
> How did you get itime from conversion time? To the layman (like me!)
> conversion-time would make more sense...
> 
> Also, "ps"? The whole thing is a proxy sensor, so why have that
> prefix
> on properties. What is missing however is a vendor prefix.
> 
> > +    description: Conversion time for proximity sensor [ms]
> > +    $ref: /schemas/types.yaml#/definitions/string
> 
> Instead of a string, please use the -us suffix, and put this in
> microseconds instead.
> 
> In total, that would be s/ps-itime/everlight,conversion-time-us/.
> 
> I would, however, like to know why this is a property of the
> hardware.
> What factors do you have to consider when determining what value to
> put
> in here?
> 
> > +    enum:
> > +      - "0.4"
> > +      - "0.8"
> > +      - "1.6"
> > +      - "3.2"
> > +      - "6.3"
> > +      - "12.6"
> > +      - "25.2"
> > +    default: "0.4"
> > +
> > +  ps-wtime:
> > +    description: Waiting time for proximity sensor [ms]
> > +    $ref: /schemas/types.yaml#/definitions/string
> 
> All of the same comments apply here. E.g. why "wtime" isntead of
> "waiting-time" and so on.
> I would really like to know why these things are properties of the
> hardware, rather than something that software should control.
> 
> > +    enum:
> > +      - "12.5"
> > +      - "25"
> > +      - "50"
> > +      - "100"
> > +      - "200"
> > +      - "400"
> > +      - "800"
> > +      - "1600"
> > +    default: "12.5"
> > +
> > +  ps-ir-led-pulse-count:
> > +    description: IR LED drive pulse count
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> All custom properties require a vendor prefix, not "ps". Again, what
> makes this a property of the hardware, rather than something that
> software should control?
> 
> > +    minimum: 1
> > +    maximum: 256
> > +    default: 1
> > +
> > +  ps-offset-cancel:
> > +    description: |
> > +      When PS offset cancel function is enabled, the result of
> > subtracting any
> > +      value specified by the PS offset cancel register from the
> > internal PS
> > +      output data is written to the PS output data register.
> 
> Again, what makes this a property of the hardware? What hardware
> related
> factors determine that value that you put in here?
> 
> Thanks,
> Conor.

Certain parameters such as conversion time, wait time, or sampling rate
are directly tied to the physical characteristics and capabilities of
the sensor. These parameters are typically determined by the sensor
specifications, and the datasheet usually provides recommended values
for these parameters. Below is an excerpt from the datasheet:

/*
+-----------------------+-------+------+------+------+-----+----------+
| Parameter             | Symbol| Min  | Typ  | Max  | Unit| Condition|
+-----------------------+-------+------+------+------+-----+----------+
| PS A/D conversion time| TPS   | 21.4 | 25.2 | 28.9 | ms  | PS
A/DC=16bit  |
| PS wait time setting  | TPSWAIT| 10.6| 12.5 | 14.3 | ms  | 12.5ms
setting |
+-----------------------+-------+------+------+------+-----+----------+
*/


However, there are some similar cases in the kernel, as follows:

Documentation/devicetree/bindings/iio/proximity/devantech-srf04.yaml
    - startup-time-ms
Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
Documentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
    - semtech,avg-pos-strength
    - semtech,ph01-resolution
    - semtech,input-analog-gain
    - ...
Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
    - vishay,led-current-microamp

This is why we are leveraging the hardware properties.

Thanks,
Hua Qian

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 0
> > +    maximum: 65535
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        lightsensor: pm16d17@44 {
> > +            compatible = "everlight,pm16d17";
> > +            reg = <0x44>;
> > +
> > +            ps-gain = <1>;
> > +            ps-itime = "0.4";
> > +            ps-wtime = "12.5";
> > +            ps-ir-led-pulse-count = <1>;
> > +            ps-offset-cancel = <280>;
> > +        };
> > +    };
> > -- 
> > 2.43.0
> > 

-- 
Hua Qian Li
Siemens AG
www.siemens.com

Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6892 bytes --]

On Fri, Aug 16, 2024 at 01:48:36AM +0000, Li, Hua Qian wrote:
> On Tue, 2024-08-13 at 16:52 +0100, Conor Dooley wrote:
> > On Tue, Aug 13, 2024 at 07:40:41AM +0200, Jan Kiszka wrote:
> > > From: Chao Zeng <chao.zeng@siemens.com>
> > > 
> > > Add the binding document for the everlight pm16d17 sensor.
> > > 
> > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> > > Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> > > Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
> > 
> > Ditto here Jan.
> > 
> > > ---
> > >  .../iio/proximity/everlight,pm16d17.yaml      | 95
> > > +++++++++++++++++++
> > >  1 file changed, 95 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.y
> > > aml
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > > .yaml
> > > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > > .yaml
> > > new file mode 100644
> > > index 000000000000..fadc3075181a
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > > .yaml
> > > @@ -0,0 +1,95 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id:
> > > http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Everlight PM-16D17 Ambient Light & Proximity Sensor
> > > +
> > > +maintainers:
> > > +  - Chao Zeng <chao.zeng@siemens.com>
> > > +
> > > +description: |
> > > +  This sensor uses standard I2C interface. Interrupt function is
> > > not covered.
> > 
> > Bindings should be complete, even if software doesn't use the
> > interrupts. Can you document them please.
> > 
> > > +  Datasheet:
> > > https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/
> > 
> > Do you have a link to a datasheet? The link to the pm16d17 here 404s
> > for
> > me.
> > 
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - everlight,pm16d17
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  ps-gain:
> > > +    description: Receiver gain of proximity sensor
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [1, 2, 4, 8]
> > > +    default: 1
> > > +
> > > +  ps-itime:
> > 
> > How did you get itime from conversion time? To the layman (like me!)
> > conversion-time would make more sense...
> > 
> > Also, "ps"? The whole thing is a proxy sensor, so why have that
> > prefix
> > on properties. What is missing however is a vendor prefix.
> > 
> > > +    description: Conversion time for proximity sensor [ms]
> > > +    $ref: /schemas/types.yaml#/definitions/string
> > 
> > Instead of a string, please use the -us suffix, and put this in
> > microseconds instead.
> > 
> > In total, that would be s/ps-itime/everlight,conversion-time-us/.
> > 
> > I would, however, like to know why this is a property of the
> > hardware.
> > What factors do you have to consider when determining what value to
> > put
> > in here?
> > 
> > > +    enum:
> > > +      - "0.4"
> > > +      - "0.8"
> > > +      - "1.6"
> > > +      - "3.2"
> > > +      - "6.3"
> > > +      - "12.6"
> > > +      - "25.2"
> > > +    default: "0.4"
> > > +
> > > +  ps-wtime:
> > > +    description: Waiting time for proximity sensor [ms]
> > > +    $ref: /schemas/types.yaml#/definitions/string
> > 
> > All of the same comments apply here. E.g. why "wtime" isntead of
> > "waiting-time" and so on.
> > I would really like to know why these things are properties of the
> > hardware, rather than something that software should control.
> > 
> > > +    enum:
> > > +      - "12.5"
> > > +      - "25"
> > > +      - "50"
> > > +      - "100"
> > > +      - "200"
> > > +      - "400"
> > > +      - "800"
> > > +      - "1600"
> > > +    default: "12.5"
> > > +
> > > +  ps-ir-led-pulse-count:
> > > +    description: IR LED drive pulse count
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > 
> > All custom properties require a vendor prefix, not "ps". Again, what
> > makes this a property of the hardware, rather than something that
> > software should control?
> > 
> > > +    minimum: 1
> > > +    maximum: 256
> > > +    default: 1
> > > +
> > > +  ps-offset-cancel:
> > > +    description: |
> > > +      When PS offset cancel function is enabled, the result of
> > > subtracting any
> > > +      value specified by the PS offset cancel register from the
> > > internal PS
> > > +      output data is written to the PS output data register.
> > 
> > Again, what makes this a property of the hardware? What hardware
> > related
> > factors determine that value that you put in here?
> > 
> > Thanks,
> > Conor.
> 
> Certain parameters such as conversion time, wait time, or sampling rate
> are directly tied to the physical characteristics and capabilities of
> the sensor. These parameters are typically determined by the sensor
> specifications, and the datasheet usually provides recommended values
> for these parameters. Below is an excerpt from the datasheet:
> 
> /*
> +-----------------------+-------+------+------+------+-----+----------+
> | Parameter             | Symbol| Min  | Typ  | Max  | Unit| Condition|
> +-----------------------+-------+------+------+------+-----+----------+
> | PS A/D conversion time| TPS   | 21.4 | 25.2 | 28.9 | ms  | PS
> A/DC=16bit  |
> | PS wait time setting  | TPSWAIT| 10.6| 12.5 | 14.3 | ms  | 12.5ms
> setting |
> +-----------------------+-------+------+------+------+-----+----------+
> */
> 
> 
> However, there are some similar cases in the kernel, as follows:
> 
> Documentation/devicetree/bindings/iio/proximity/devantech-srf04.yaml
>     - startup-time-ms
> Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> Documentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
>     - semtech,avg-pos-strength
>     - semtech,ph01-resolution
>     - semtech,input-analog-gain
>     - ...
> Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
>     - vishay,led-current-microamp
> 
> This is why we are leveraging the hardware properties.

"Other people did it" is not sufficient justification, you need to
independently justify the properties you add. It appears however, that
Jonathan (who understands these devices better than I, and had a
functioning datasheet link), is also of the opinion that these would be
better suited as userspace controls or require improved explanations.
I suggest you read and reply to his mail in an itemised manner.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Jonathan Cameron]

On Fri, 16 Aug 2024 01:48:36 +0000
"Li, Hua Qian" <HuaQian.Li@siemens.com> wrote:

> On Tue, 2024-08-13 at 16:52 +0100, Conor Dooley wrote:
> > On Tue, Aug 13, 2024 at 07:40:41AM +0200, Jan Kiszka wrote:  
> > > From: Chao Zeng <chao.zeng@siemens.com>
> > > 
> > > Add the binding document for the everlight pm16d17 sensor.
> > > 
> > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> > > Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> > > Signed-off-by: Baocheng Su <baocheng.su@siemens.com>  
> > 
> > Ditto here Jan.
> >   
> > > ---
> > >  .../iio/proximity/everlight,pm16d17.yaml      | 95
> > > +++++++++++++++++++
> > >  1 file changed, 95 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.y
> > > aml
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > > .yaml
> > > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > > .yaml
> > > new file mode 100644
> > > index 000000000000..fadc3075181a
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17
> > > .yaml
> > > @@ -0,0 +1,95 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id:
> > > http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Everlight PM-16D17 Ambient Light & Proximity Sensor
> > > +
> > > +maintainers:
> > > +  - Chao Zeng <chao.zeng@siemens.com>
> > > +
> > > +description: |
> > > +  This sensor uses standard I2C interface. Interrupt function is
> > > not covered.  
> > 
> > Bindings should be complete, even if software doesn't use the
> > interrupts. Can you document them please.
> >   
> > > +  Datasheet:
> > > https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/  
> > 
> > Do you have a link to a datasheet? The link to the pm16d17 here 404s
> > for
> > me.
> >   
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - everlight,pm16d17
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  ps-gain:
> > > +    description: Receiver gain of proximity sensor
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [1, 2, 4, 8]
> > > +    default: 1
> > > +
> > > +  ps-itime:  
> > 
> > How did you get itime from conversion time? To the layman (like me!)
> > conversion-time would make more sense...
> > 
> > Also, "ps"? The whole thing is a proxy sensor, so why have that
> > prefix
> > on properties. What is missing however is a vendor prefix.
> >   
> > > +    description: Conversion time for proximity sensor [ms]
> > > +    $ref: /schemas/types.yaml#/definitions/string  
> > 
> > Instead of a string, please use the -us suffix, and put this in
> > microseconds instead.
> > 
> > In total, that would be s/ps-itime/everlight,conversion-time-us/.
> > 
> > I would, however, like to know why this is a property of the
> > hardware.
> > What factors do you have to consider when determining what value to
> > put
> > in here?
> >   
> > > +    enum:
> > > +      - "0.4"
> > > +      - "0.8"
> > > +      - "1.6"
> > > +      - "3.2"
> > > +      - "6.3"
> > > +      - "12.6"
> > > +      - "25.2"
> > > +    default: "0.4"
> > > +
> > > +  ps-wtime:
> > > +    description: Waiting time for proximity sensor [ms]
> > > +    $ref: /schemas/types.yaml#/definitions/string  
> > 
> > All of the same comments apply here. E.g. why "wtime" isntead of
> > "waiting-time" and so on.
> > I would really like to know why these things are properties of the
> > hardware, rather than something that software should control.
> >   
> > > +    enum:
> > > +      - "12.5"
> > > +      - "25"
> > > +      - "50"
> > > +      - "100"
> > > +      - "200"
> > > +      - "400"
> > > +      - "800"
> > > +      - "1600"
> > > +    default: "12.5"
> > > +
> > > +  ps-ir-led-pulse-count:
> > > +    description: IR LED drive pulse count
> > > +    $ref: /schemas/types.yaml#/definitions/uint32  
> > 
> > All custom properties require a vendor prefix, not "ps". Again, what
> > makes this a property of the hardware, rather than something that
> > software should control?
> >   
> > > +    minimum: 1
> > > +    maximum: 256
> > > +    default: 1
> > > +
> > > +  ps-offset-cancel:
> > > +    description: |
> > > +      When PS offset cancel function is enabled, the result of
> > > subtracting any
> > > +      value specified by the PS offset cancel register from the
> > > internal PS
> > > +      output data is written to the PS output data register.  
> > 
> > Again, what makes this a property of the hardware? What hardware
> > related
> > factors determine that value that you put in here?
> > 
> > Thanks,
> > Conor.  
> 
> Certain parameters such as conversion time, wait time, or sampling rate
> are directly tied to the physical characteristics and capabilities of
> the sensor.

Ah. I think I'd missed this uses an external LED (rather than it being
in package).  In that case, the characteristics that 'work' for
proximity sensing are somewhat dependent on the system design
(simplifying heavily, led output for a given current, optical filter
 on that LED etc).

For the sensor specific side, it should be just from the compatible, but
when another part is involved, DT binding based tuning may make sense
as long as it is 'per consumer device / board' not per specific instance.




> These parameters are typically determined by the sensor
> specifications, and the datasheet usually provides recommended values
> for these parameters. Below is an excerpt from the datasheet:
> 
> /*
> +-----------------------+-------+------+------+------+-----+----------+
> | Parameter             | Symbol| Min  | Typ  | Max  | Unit| Condition|
> +-----------------------+-------+------+------+------+-----+----------+
> | PS A/D conversion time| TPS   | 21.4 | 25.2 | 28.9 | ms  | PS
> A/DC=16bit  |
> | PS wait time setting  | TPSWAIT| 10.6| 12.5 | 14.3 | ms  | 12.5ms
> setting |
> +-----------------------+-------+------+------+------+-----+----------+
> */

Doesn't apply to everything you have here though. wtime / wait time
is about how often you get a reading not the physical device.  How is
that affected by the physical device?

I 'think' the table above is giving precision of the value for a particular
control setting. If you set wtime to 12.5msec (value 0 in register)
then it will actually be between 10.6 and 14.3 msec, not that you should
set it to 12.5msec.

There are 3 controls related to gain that you could argue for defaults
for in DT (maybe) but given proximity sensing is also about the
target, not just the measurement device, there won't be a right answer
unless your proximity sensor is being used for a fixed purpose (e.g.
WIFI signal strength limiting or a button type control).
> 
> 
> However, there are some similar cases in the kernel, as follows:
> 
> Documentation/devicetree/bindings/iio/proximity/devantech-srf04.yaml
>     - startup-time-ms
That's after a resume and I think depends one exactly what the circuitry
is (in this case the device is more of a reference design than a single
device).

> Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> Documentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
>     - semtech,avg-pos-strength
>     - semtech,ph01-resolution
>     - semtech,input-analog-gain
These are SAR sensors I think, so the sensor element is external to
the device.  In theory we could have described the sensing element
and used that to work out the right values of these, but in practice
it was easier to just provide the parameters from some 'per design'
tuning.

>     - ...
> Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
>     - vishay,led-current-microamp

I think this is about whether you can burn the external LED out or not.
On the datasheet I'm looking at for this device, only value 000 is
specified in this 3bit field so I guess it's not controllable?

Pulse counts are less likely to be relevant to the LED burning out, but
maybe(?)

Anyhow, it's not entirely obvious to me that it makes sense to control
so much in DT for this device.  Better to put it in userspace control
and rely on udev etc setting things right for a given device + application.

Jonathan




> 
> This is why we are leveraging the hardware properties.
> 
> Thanks,
> Hua Qian
> 
> >   
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    default: 0
> > > +    maximum: 65535
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        lightsensor: pm16d17@44 {
> > > +            compatible = "everlight,pm16d17";
> > > +            reg = <0x44>;
> > > +
> > > +            ps-gain = <1>;
> > > +            ps-itime = "0.4";
> > > +            ps-wtime = "12.5";
> > > +            ps-ir-led-pulse-count = <1>;
> > > +            ps-offset-cancel = <280>;
> > > +        };
> > > +    };
> > > -- 
> > > 2.43.0
> > >   
> 

Re: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor

[Jonathan Cameron]

On Tue, 13 Aug 2024 07:40:42 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> From: Chao Zeng <chao.zeng@siemens.com>
> 
> Add initial support for everlight pm16d17 proximity sensor.
> 
> Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Hi Jan,

Various comments inline. 

Thanks,

Jonathan

> diff --git a/drivers/iio/proximity/pm16d17.c b/drivers/iio/proximity/pm16d17.c
> new file mode 100644
> index 000000000000..94f21fc5e2fb
> --- /dev/null
> +++ b/drivers/iio/proximity/pm16d17.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Siemens AG, 2023-2024
> + *
> + * Driver for Everlight PM-16d17 proximity sensor
> + *
> + * Author: Chao Zeng <chao.zeng@siemens.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
Unlikely you need this one...

> +#include <linux/interrupt.h>
Not in use yet?
> +#include <linux/irq.h>
> +#include <linux/module.h>

mod_devicetable.h
property.h

Also check these again as you should as currently written have had
of.h and don't, so may be other things missing.
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
Only needed if you are doing custom attrs. So drop.
> +#include <linux/iio/events.h>

Only needed for events. So drop for now.

> +
> +#define PM16D17_DRV_NAME		"pm16d17"
> +#define PM16D17_REGMAP_NAME		"pm16d17_regmap"
Don't have defines for these two. See later for more comments on that.
> +
> +/* Registers Address */
To keep clear distinction between fields and registers I'd add
_REG to end of these.

> +#define PM16D17_OP_MODE			0x00
> +#define PM16D17_INTERRUPT_FLAG		0x01
> +#define PM16D17_PS_SETTING		0x0A
> +#define PM16D17_VCSEL_DRIVE_CURRENT	0x0B
> +#define PM16D17_VCSEL_DRIVE_PULSE	0x0C
> +#define PM16D17_PS_INTUPT_LTHD_L	0x0D
> +#define PM16D17_PS_INTUPT_LTHD_H	0x0E
> +#define PM16D17_PS_INTUPT_HTHD_L	0x0F
> +#define PM16D17_PS_INTUPT_HTHD_H	0x10
> +#define PM16D17_PS_DATA_L		0x11
> +#define PM16D17_PS_DATA_H		0x12
> +#define PM16D17_PS_SETTING2		0x13
> +#define PM16D17_PS_OFFSET_CANCEL_L	0x14
> +#define PM16D17_PS_OFFSET_CANCEL_H	0x15
> +#define PM16D17_DEV_ID			0x18
> +
> +#define DEVICE_ID			0x11
Needs to be prefixed as DEVICE_ID is a very generic define!
PM16D17_DEV_ID_PM16D17 
or something like that to indicate which register and value means
what.

> +
> +#define ENABLE_PS_FUNCTION		BIT(3)
> +#define PS_GAIN_MASK			GENMASK(7, 6)
> +#define PS_ITIME_MASK			GENMASK(5, 3)
> +#define PS_WTIME_MASK			GENMASK(2, 0)
> +#define OFFSET_CANCEL_ENABLE		BIT(7)
> +#define PS_OFFSET_CANCEL_LSB_MASK	GENMASK(7, 0)
> +#define PS_OFFSET_CANCEL_MSB_MASK	GENMASK(15, 8)
> +
> +enum {
> +	PITIME_0_POINT_4_MS = (0 << 3),
> +	PITIME_0_POINT_8_MS = (1 << 3),
> +	PITIME_1_POINT_6_MS = (2 << 3),
> +	PITIME_3_POINT_2_MS = (3 << 3),
> +	PITIME_6_POINT_3_MS = (4 << 3),
> +	PITIME_12_POINT_6_MS = (5 << 3),
> +	PITIME_25_POINT_2_MS = (6 << 3),
Don't shift these values.

Use a suitable mask and FIELD_PREP() to shift them at the
point where you are writing them.

> +};
> +
> +enum {
> +	PWTIME_12_POINT_5_MS = 0,
> +	PWTIME_25_MS,
> +	PWTIME_50_MS,
> +	PWTIME_100_MS,
> +	PWTIME_200_MS,
> +	PWTIME_400_MS,
> +	PWTIME_800_MS,
> +	PWTIME_1600_MS,
> +};
> +
> +struct pm16d17_data {
> +	struct i2c_client *client;
As below. More useful to store &client->dev

> +	struct regmap *regmap;
> +};
> +
> +static const struct regmap_config pm16d17_regmap_config = {
> +	.name = PM16D17_REGMAP_NAME,
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static const struct iio_chan_spec pm16d17_channels[] = {
> +	{
> +		.type = IIO_PROXIMITY,
> +		.indexed = 1,
> +		.channel = 0,
> +		.scan_index = -1,
Don't need to set this unless you are registering the buffered
interfaces. So don't set it for now.

> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}
> +};
> +
> +static inline int pm16d17_write_reg(struct pm16d17_data *data,
> +				    unsigned int reg,
> +				    unsigned int value)
> +{
> +	return regmap_write(data->regmap, reg, value);

Get rid of these wrappers and call regmap_write / read directly inline.
These just make the code a little harder to read for no benefit.

> +}
> +
> +static inline unsigned int pm16d17_read_reg(struct pm16d17_data *data,
> +					    unsigned int reg,
> +					    unsigned int *reg_val)
> +{
> +	return regmap_read(data->regmap, reg, reg_val);
> +}
> +
> +static int pm16d17_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct pm16d17_data *data = iio_priv(indio_dev);
> +	unsigned int ps_data_l;
> +	unsigned int ps_data_h;
> +	uint16_t ps_data;
> +	int ret = -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = pm16d17_read_reg(data, PM16D17_PS_DATA_L, &ps_data_l);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = pm16d17_read_reg(data, PM16D17_PS_DATA_H, &ps_data_h);
> +			if (ret < 0)
> +				return ret;
> +
> +			ps_data = (ps_data_h << 8) | ps_data_l;

read both values into a u8 [2] and use get_unaligned_be16() or similar.

Actually you should be able to do a bulk read (which is obvious once you
stop wrapping regmap calls with function s that suggest something more is going on).
> +
> +			dev_dbg(&data->client->dev, "PS data: %x\n", ps_data);
> +
> +			*val = ps_data;
> +			ret = IIO_VAL_INT;
> +			break;
			return IIO_VAL_INT;
> +		default:
> +			break;
return an error
> +		}
> +	default:
> +		break;
return an error.
> +	}
> +
Should not be able to get here so drop this (with above changes to return early)
> +	return ret;
> +}
> +
> +static const struct iio_info pm16d17_info = {
> +	.read_raw = pm16d17_read_raw,
> +};
> +
> +static int pm16d17_chip_init(struct pm16d17_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	struct device_node *np = client->dev.of_node;
> +	const char *conv_time = NULL;
> +	const char *wait_time = NULL;
> +	uint8_t op_mode_setting_val;
> +	uint32_t ps_offset_cancel;
> +	uint8_t offset_lsb;
> +	uint8_t offset_msb;
> +	uint32_t pulse_count;
> +	uint32_t pgain;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = pm16d17_read_reg(data, PM16D17_DEV_ID, &val);
> +
No blank line between a call and the check of it's error. Good keep them
logically associated.

> +	if (ret < 0 || (val != DEVICE_ID)) {
> +		dev_err(&client->dev, "Invalid chip id 0x%04x\n", val);
if (ret < 0)
	return dev_err_probe()
if (val != DEVICE_ID)
	dev_info(&client->dev, "Unexpected chip id ..." 
and carry on.

The reason for this is to enable use in future of fallback DT compatibles
so if a new device is released that is backwards compatible it can be supported
by existing distro kernels etc without change.

We used to get this wrong in IIO and haven't yet fixed all drivers so you
will see lots of examples similar to what you have here.


> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(&client->dev, "Detected PM16D17 with chip id: 0x%04x\n", val);
> +
> +	ret = pm16d17_write_reg(data, PM16D17_OP_MODE, ENABLE_PS_FUNCTION);
> +	if (ret < 0)
> +		return ret;
> +
> +	of_property_read_u32(np, "ps-gain", &pgain);
> +	switch (pgain) {
> +	case 1:
> +	case 2:
> +	case 4:
> +	case 8:
> +		op_mode_setting_val |= (ilog2(pgain) << 6) & PS_GAIN_MASK;
> +		break;
> +	default:
> +		break;

Not an error?

> +	}
> +
> +	of_property_read_string(np, "ps-itime", &conv_time);
Is strcmp safe for NULL? Seems unlikely.

Make the time in msecs / usecs of appropriate so you can handle it as an integer.

device_property_read_string()
Use the generic firmware stuff in linux/property.h instead of of calls thoughout.
That enables use of things like ACPI bindings via PRP0001 _HID

> +	if (strcmp(conv_time, "0.4") == 0)
> +		op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
> +	else if (strcmp(conv_time, "0.8") == 0)
> +		op_mode_setting_val |= PITIME_0_POINT_8_MS & PS_ITIME_MASK;
> +	else if (strcmp(conv_time, "1.6") == 0)
> +		op_mode_setting_val |= PITIME_1_POINT_6_MS & PS_ITIME_MASK;
> +	else if (strcmp(conv_time, "3.2") == 0)
> +		op_mode_setting_val |= PITIME_3_POINT_2_MS & PS_ITIME_MASK;
> +	else if (strcmp(conv_time, "6.3") == 0)
> +		op_mode_setting_val |= PITIME_6_POINT_3_MS & PS_ITIME_MASK;
> +	else if (strcmp(conv_time, "12.6") == 0)
> +		op_mode_setting_val |= PITIME_12_POINT_6_MS & PS_ITIME_MASK;
> +	else if (strcmp(conv_time, "25.2") == 0)
> +		op_mode_setting_val |= PITIME_25_POINT_2_MS & PS_ITIME_MASK;
> +	else {
> +		dev_info(&client->dev, "Using default ps itime value\n");

If a property was there and invalid value error out, otherwise just
use default here without printing anything.

> +		op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
> +	}
> +
> +	of_property_read_string(np, "ps-wtime", &wait_time);
Again, pick units so it's an integer. Other comments as above.

> +	if (strcmp(wait_time, "12.5") == 0)
> +		op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
> +	else if (strcmp(wait_time, "25") == 0)
> +		op_mode_setting_val |= PWTIME_25_MS & PS_WTIME_MASK;
> +	else if (strcmp(wait_time, "50") == 0)
> +		op_mode_setting_val |= PWTIME_50_MS & PS_WTIME_MASK;
> +	else if (strcmp(wait_time, "100") == 0)
> +		op_mode_setting_val |= PWTIME_100_MS & PS_WTIME_MASK;
> +	else if (strcmp(wait_time, "200") == 0)
> +		op_mode_setting_val |= PWTIME_200_MS & PS_WTIME_MASK;
> +	else if (strcmp(wait_time, "400") == 0)
> +		op_mode_setting_val |= PWTIME_400_MS & PS_WTIME_MASK;
> +	else if (strcmp(wait_time, "800") == 0)
> +		op_mode_setting_val |= PWTIME_800_MS & PS_WTIME_MASK;
> +	else if (strcmp(wait_time, "1600") == 0)
> +		op_mode_setting_val |= PWTIME_1600_MS & PS_WTIME_MASK;
> +	else {
> +		dev_info(&client->dev, "Using default ps wtime value\n");
> +		op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
> +	}
> +
> +	ret = pm16d17_write_reg(data, PM16D17_PS_SETTING, op_mode_setting_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	of_property_read_u32(np, "ps-ir-led-pulse-count", &pulse_count);
> +	if (pulse_count > 256)
> +		pulse_count = 256;
	pulse_count = min(256, pulse_count);

> +	ret = pm16d17_write_reg(data, PM16D17_VCSEL_DRIVE_PULSE, pulse_count - 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	of_property_read_u32(np, "ps-offset-cancel", &ps_offset_cancel);
> +	if (ps_offset_cancel != 0) {
> +		ret = pm16d17_write_reg(data, PM16D17_PS_SETTING2, OFFSET_CANCEL_ENABLE);
> +		if (ret < 0)
> +			return ret;
> +
> +		offset_lsb = ps_offset_cancel & PS_OFFSET_CANCEL_LSB_MASK;
> +		offset_msb = (ps_offset_cancel & PS_OFFSET_CANCEL_MSB_MASK) >> 8;
> +
> +		ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_L, offset_lsb);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_H, offset_msb);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pm16d17_probe(struct i2c_client *client)
> +{
> +	struct pm16d17_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	indio_dev->dev.parent = &client->dev;

No need to set that - devm_iio_device_alloc() does it for you.

> +	indio_dev->info = &pm16d17_info;
> +	indio_dev->name = PM16D17_DRV_NAME;

There is no particularly reason why the name here and the driver name should
match.  As such, I'd much rather just see the explicit string listed here
directly and that define being dropped.

> +	indio_dev->channels = pm16d17_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(pm16d17_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;

You only ever use this to get to client->dev.  Just have a struct device
in there instead.  Or you can retrieve it via regmap_get_device()
if you prefer not to store it explicitly.
> +
> +	data->regmap = devm_regmap_init_i2c(client, &pm16d17_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "regmap initialization failed.\n");
> +		return PTR_ERR(data->regmap);
For error prints in probe or functions just called from probe use
		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
				     ....);


> +	}
> +
> +	ret = pm16d17_chip_init(data);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id pm16d17_id[] = {
> +	{"pm16d17", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, pm16d17_id);
> +
> +static const struct of_device_id pm16d17_of_match[] = {
> +	{ .compatible = "everlight,pm16d17" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, pm16d17_of_match);
> +
> +static struct i2c_driver pm16d17_driver = {
> +	.driver = {
> +		.name = PM16D17_DRV_NAME,
Put the string in directly here

> +		.of_match_table = pm16d17_of_match,
> +	},
> +	.probe = pm16d17_probe,
> +	.id_table = pm16d17_id,
> +};
> +module_i2c_driver(pm16d17_driver);
> +
> +MODULE_AUTHOR("Chao Zeng <chao.zeng@siemens.com>");
> +MODULE_DESCRIPTION("PM16D17 proximity sensor");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Li, Hua Qian]

On Sat, 2024-08-17 at 14:42 +0100, Jonathan Cameron wrote:
> On Fri, 16 Aug 2024 01:48:36 +0000
> "Li, Hua Qian" <HuaQian.Li@siemens.com> wrote:
>
> > On Tue, 2024-08-13 at 16:52 +0100, Conor Dooley wrote:
> > > On Tue, Aug 13, 2024 at 07:40:41AM +0200, Jan Kiszka wrote:
> > > > From: Chao Zeng <chao.zeng@siemens.com>
> > > >
> > > > Add the binding document for the everlight pm16d17 sensor.
> > > >
> > > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> > > > Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> > > > Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
> > >
> > > Ditto here Jan.
> > >
> > > > ---
> > > >  .../iio/proximity/everlight,pm16d17.yaml      | 95
> > > > +++++++++++++++++++
> > > >  1 file changed, 95 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/iio/proximity/everlight,pm16d
> > > > 17.y
> > > > aml
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/iio/proximity/everlight,pm1
> > > > 6d17
> > > > .yaml
> > > > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm1
> > > > 6d17
> > > > .yaml
> > > > new file mode 100644
> > > > index 000000000000..fadc3075181a
> > > > --- /dev/null
> > > > +++
> > > > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm1
> > > > 6d17
> > > > .yaml
> > > > @@ -0,0 +1,95 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id:
> > > > http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
> > > > +$schema:
> > > > http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Everlight PM-16D17 Ambient Light & Proximity Sensor
> > > > +
> > > > +maintainers:
> > > > +  - Chao Zeng <chao.zeng@siemens.com>
> > > > +
> > > > +description: |
> > > > +  This sensor uses standard I2C interface. Interrupt function
> > > > is
> > > > not covered.
> > >
> > > Bindings should be complete, even if software doesn't use the
> > > interrupts. Can you document them please.
> > >
> > > > +  Datasheet:
> > > > https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/
> > > >
> > >
> > > Do you have a link to a datasheet? The link to the pm16d17 here
> > > 404s
> > > for
> > > me.
> > >
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - everlight,pm16d17
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  ps-gain:
> > > > +    description: Receiver gain of proximity sensor
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [1, 2, 4, 8]
> > > > +    default: 1
> > > > +
How about using `in_proximity0_hw_gain` as a userspace interface here?
> > > > +  ps-itime:
> > >
> > > How did you get itime from conversion time? To the layman (like
> > > me!)
> > > conversion-time would make more sense...
> > >
> > > Also, "ps"? The whole thing is a proxy sensor, so why have that
> > > prefix
> > > on properties. What is missing however is a vendor prefix.
How about using `in_proximity0_conversion_time` as a userspace
interface here?
> > >
> > > > +    description: Conversion time for proximity sensor [ms]
> > > > +    $ref: /schemas/types.yaml#/definitions/string
> > >
> > > Instead of a string, please use the -us suffix, and put this in
> > > microseconds instead.
> > >
> > > In total, that would be s/ps-itime/everlight,conversion-time-us/.
> > >
> > > I would, however, like to know why this is a property of the
> > > hardware.
> > > What factors do you have to consider when determining what value
> > > to
> > > put
> > > in here?
> > >
> > > > +    enum:
> > > > +      - "0.4"
> > > > +      - "0.8"
> > > > +      - "1.6"
> > > > +      - "3.2"
> > > > +      - "6.3"
> > > > +      - "12.6"
> > > > +      - "25.2"
> > > > +    default: "0.4"
> > > > +
> > > > +  ps-wtime:
> > > > +    description: Waiting time for proximity sensor [ms]
> > > > +    $ref: /schemas/types.yaml#/definitions/string
> > >
> > > All of the same comments apply here. E.g. why "wtime" isntead of
> > > "waiting-time" and so on.
> > > I would really like to know why these things are properties of
> > > the
> > > hardware, rather than something that software should control.
> > >
How about using `in_proximity0_wait_time` as a userspace interface
here?
> > > > +    enum:
> > > > +      - "12.5"
> > > > +      - "25"
> > > > +      - "50"
> > > > +      - "100"
> > > > +      - "200"
> > > > +      - "400"
> > > > +      - "800"
> > > > +      - "1600"
> > > > +    default: "12.5"
> > > > +
> > > > +  ps-ir-led-pulse-count:
> > > > +    description: IR LED drive pulse count
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > >
> > > All custom properties require a vendor prefix, not "ps". Again,
> > > what
> > > makes this a property of the hardware, rather than something that
> > > software should control?
> > >
How about using `in_proximity0_pulse_count` as a userspace interface
here?
> > > > +    minimum: 1
> > > > +    maximum: 256
> > > > +    default: 1
> > > > +
> > > > +  ps-offset-cancel:
> > > > +    description: |
> > > > +      When PS offset cancel function is enabled, the result of
> > > > subtracting any
> > > > +      value specified by the PS offset cancel register from
> > > > the
> > > > internal PS
> > > > +      output data is written to the PS output data register.
> > >
How about using `in_proximity0_offset_cancel` as a userspace interface
here?
> > > Again, what makes this a property of the hardware? What hardware
> > > related
> > > factors determine that value that you put in here?
> > >
> > > Thanks,
> > > Conor.
> >
> > Certain parameters such as conversion time, wait time, or sampling
> > rate
> > are directly tied to the physical characteristics and capabilities
> > of
> > the sensor.
>
> Ah. I think I'd missed this uses an external LED (rather than it
> being
> in package).  In that case, the characteristics that 'work' for
> proximity sensing are somewhat dependent on the system design
> (simplifying heavily, led output for a given current, optical filter
>  on that LED etc).
>
> For the sensor specific side, it should be just from the compatible,
> but
> when another part is involved, DT binding based tuning may make sense
> as long as it is 'per consumer device / board' not per specific
> instance.
>
>
>
>
> > These parameters are typically determined by the sensor
> > specifications, and the datasheet usually provides recommended
> > values
> > for these parameters. Below is an excerpt from the datasheet:
> >
> > /*
> > +-----------------------+-------+------+------+------+-----+-------
> > ---+
> > > Parameter             | Symbol| Min  | Typ  | Max  | Unit|
> > > Condition|
> > +-----------------------+-------+------+------+------+-----+-------
> > ---+
> > > PS A/D conversion time| TPS   | 21.4 | 25.2 | 28.9 | ms  | PS
> > A/DC=16bit  |
> > > PS wait time setting  | TPSWAIT| 10.6| 12.5 | 14.3 | ms  | 12.5ms
> > setting |
> > +-----------------------+-------+------+------+------+-----+-------
> > ---+
> > */
>
> Doesn't apply to everything you have here though. wtime / wait time
> is about how often you get a reading not the physical device.  How is
> that affected by the physical device?
>
> I 'think' the table above is giving precision of the value for a
> particular
> control setting. If you set wtime to 12.5msec (value 0 in register)
> then it will actually be between 10.6 and 14.3 msec, not that you
> should
> set it to 12.5msec.
>
> There are 3 controls related to gain that you could argue for
> defaults
> for in DT (maybe) but given proximity sensing is also about the
> target, not just the measurement device, there won't be a right
> answer
> unless your proximity sensor is being used for a fixed purpose (e.g.
> WIFI signal strength limiting or a button type control).
> >
> >
> > However, there are some similar cases in the kernel, as follows:
> >
> > Documentation/devicetree/bindings/iio/proximity/devantech-
> > srf04.yaml
> >     - startup-time-ms
> That's after a resume and I think depends one exactly what the
> circuitry
> is (in this case the device is more of a reference design than a
> single
> device).
>
> > Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > Documentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
> >     - semtech,avg-pos-strength
> >     - semtech,ph01-resolution
> >     - semtech,input-analog-gain
> These are SAR sensors I think, so the sensor element is external to
> the device.  In theory we could have described the sensing element
> and used that to work out the right values of these, but in practice
> it was easier to just provide the parameters from some 'per design'
> tuning.
>
> >     - ...
> > Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yam
> > l
> >     - vishay,led-current-microamp
>
> I think this is about whether you can burn the external LED out or
> not.
> On the datasheet I'm looking at for this device, only value 000 is
> specified in this 3bit field so I guess it's not controllable?
>
> Pulse counts are less likely to be relevant to the LED burning out,
> but
> maybe(?)
>
> Anyhow, it's not entirely obvious to me that it makes sense to
> control
> so much in DT for this device.  Better to put it in userspace control
> and rely on udev etc setting things right for a given device +
> application.
>
> Jonathan
>
I agree with your comments, we'll modify the DT to allow userspace
control as introduced above. Consequently, all the dt properites will
be removed.


Thanks,
Hua Qian
>
>
>
> >
> > This is why we are leveraging the hardware properties.
> >
> > Thanks,
> > Hua Qian
> >
> > >
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    default: 0
> > > > +    maximum: 65535
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +
> > > > +unevaluatedProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    i2c {
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +
> > > > +        lightsensor: pm16d17@44 {
> > > > +            compatible = "everlight,pm16d17";
> > > > +            reg = <0x44>;
> > > > +
> > > > +            ps-gain = <1>;
> > > > +            ps-itime = "0.4";
> > > > +            ps-wtime = "12.5";
> > > > +            ps-ir-led-pulse-count = <1>;
> > > > +            ps-offset-cancel = <280>;
> > > > +        };
> > > > +    };
> > > > --
> > > > 2.43.0
> > > >
> >
>

--
Hua Qian Li
Siemens AG
http://www.siemens.com/

Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Jonathan Cameron]

On Mon, 26 Aug 2024 07:12:53 +0000
"Li, Hua Qian" <HuaQian.Li@siemens.com> wrote:

> On Sat, 2024-08-17 at 14:42 +0100, Jonathan Cameron wrote:
> > On Fri, 16 Aug 2024 01:48:36 +0000
> > "Li, Hua Qian" <HuaQian.Li@siemens.com> wrote:
> >  
> > > On Tue, 2024-08-13 at 16:52 +0100, Conor Dooley wrote:  
> > > > On Tue, Aug 13, 2024 at 07:40:41AM +0200, Jan Kiszka wrote:  
> > > > > From: Chao Zeng <chao.zeng@siemens.com>
> > > > >
> > > > > Add the binding document for the everlight pm16d17 sensor.
> > > > >
> > > > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> > > > > Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> > > > > Signed-off-by: Baocheng Su <baocheng.su@siemens.com>  
> > > >
> > > > Ditto here Jan.
> > > >  
> > > > > ---
> > > > >  .../iio/proximity/everlight,pm16d17.yaml      | 95
> > > > > +++++++++++++++++++
> > > > >  1 file changed, 95 insertions(+)
> > > > >  create mode 100644
> > > > > Documentation/devicetree/bindings/iio/proximity/everlight,pm16d
> > > > > 17.y
> > > > > aml
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/iio/proximity/everlight,pm1
> > > > > 6d17
> > > > > .yaml
> > > > > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm1
> > > > > 6d17
> > > > > .yaml
> > > > > new file mode 100644
> > > > > index 000000000000..fadc3075181a
> > > > > --- /dev/null
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/iio/proximity/everlight,pm1
> > > > > 6d17
> > > > > .yaml
> > > > > @@ -0,0 +1,95 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id:
> > > > > http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
> > > > > +$schema:
> > > > > http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Everlight PM-16D17 Ambient Light & Proximity Sensor
> > > > > +
> > > > > +maintainers:
> > > > > +  - Chao Zeng <chao.zeng@siemens.com>
> > > > > +
> > > > > +description: |
> > > > > +  This sensor uses standard I2C interface. Interrupt function
> > > > > is
> > > > > not covered.  
> > > >
> > > > Bindings should be complete, even if software doesn't use the
> > > > interrupts. Can you document them please.
> > > >  
> > > > > +  Datasheet:
> > > > > https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/
> > > > >  
> > > >
> > > > Do you have a link to a datasheet? The link to the pm16d17 here
> > > > 404s
> > > > for
> > > > me.
> > > >  
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - everlight,pm16d17
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  ps-gain:
> > > > > +    description: Receiver gain of proximity sensor
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    enum: [1, 2, 4, 8]
> > > > > +    default: 1
> > > > > +  
> How about using `in_proximity0_hw_gain` as a userspace interface here?

hwgain is used for cases where the gain does not affect the singal
reported (so things like brightness on a time of flight sensor).
If this directly affects the output values for proximity then
it should be scale. If there are multiple things affecting the scale
it is up to the driver to figure out the 'right' combination to satisfy the
user request.

> > > > > +  ps-itime:  
> > > >
> > > > How did you get itime from conversion time? To the layman (like
> > > > me!)
> > > > conversion-time would make more sense...
> > > >
> > > > Also, "ps"? The whole thing is a proxy sensor, so why have that
> > > > prefix
> > > > on properties. What is missing however is a vendor prefix.  
> How about using `in_proximity0_conversion_time` as a userspace
> interface here?

See if you can use standard ABI.  New ABI is effectively ABI that
is unused by all existing software and unlikely to be adopted by
anything generic in future.

We have various controls this might map to.  integration_time
for the amount of exposure time for a given sensor, sampling_frequency
covers the rate at which the overall process occurs (1/sampling period).
That covers the majority of cases.  If it's something else that
can't be mapped to these interfaces I want to fully understand
why userspace would choose to change it.

> > > >  
> > > > > +    description: Conversion time for proximity sensor [ms]
> > > > > +    $ref: /schemas/types.yaml#/definitions/string  
> > > >
> > > > Instead of a string, please use the -us suffix, and put this in
> > > > microseconds instead.
> > > >
> > > > In total, that would be s/ps-itime/everlight,conversion-time-us/.
> > > >
> > > > I would, however, like to know why this is a property of the
> > > > hardware.
> > > > What factors do you have to consider when determining what value
> > > > to
> > > > put
> > > > in here?
> > > >  
> > > > > +    enum:
> > > > > +      - "0.4"
> > > > > +      - "0.8"
> > > > > +      - "1.6"
> > > > > +      - "3.2"
> > > > > +      - "6.3"
> > > > > +      - "12.6"
> > > > > +      - "25.2"
> > > > > +    default: "0.4"
> > > > > +
> > > > > +  ps-wtime:
> > > > > +    description: Waiting time for proximity sensor [ms]
> > > > > +    $ref: /schemas/types.yaml#/definitions/string  
> > > >
> > > > All of the same comments apply here. E.g. why "wtime" isntead of
> > > > "waiting-time" and so on.
> > > > I would really like to know why these things are properties of
> > > > the
> > > > hardware, rather than something that software should control.
> > > >  
> How about using `in_proximity0_wait_time` as a userspace interface
> here?
Again, if you add new ABI, it will be unused.
So basically do not do so.  Work out how to map to existing ABI.

Superficially seems like sampling_frequency = 1/(wait_time + conversion_time)
And 'maybe' conversion time is close at least to 'integration_time'.

It is sometimes a pain to map to standard ABI for hardware that chooses
different control schemes, but it is necessary to do so if you want to be
able to use normal software / scripts etc.

If there is something we can't represent and a strong usecase for
why userspace will want to directly tune it (rather than it being
one element of the hardware controls that back a single userspace
control) then it is fine to propose new userspace ABI along with
documentation.  We'll consider it, but it will be much more challenging
to get accepted than using standard ABI.

> > > > > +    enum:
> > > > > +      - "12.5"
> > > > > +      - "25"
> > > > > +      - "50"
> > > > > +      - "100"
> > > > > +      - "200"
> > > > > +      - "400"
> > > > > +      - "800"
> > > > > +      - "1600"
> > > > > +    default: "12.5"
> > > > > +
> > > > > +  ps-ir-led-pulse-count:
> > > > > +    description: IR LED drive pulse count
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32  
> > > >
> > > > All custom properties require a vendor prefix, not "ps". Again,
> > > > what
> > > > makes this a property of the hardware, rather than something that
> > > > software should control?
> > > >  
> How about using `in_proximity0_pulse_count` as a userspace interface
> here?
This one is unsual, so I want to understand what it actually means
for a user.  Why would they control pulse count? What does it do?
- Increase gain / sensitivity?
- Reduce noise?

> > > > > +    minimum: 1
> > > > > +    maximum: 256
> > > > > +    default: 1
> > > > > +
> > > > > +  ps-offset-cancel:
> > > > > +    description: |
> > > > > +      When PS offset cancel function is enabled, the result of
> > > > > subtracting any
> > > > > +      value specified by the PS offset cancel register from
> > > > > the
> > > > > internal PS
> > > > > +      output data is written to the PS output data register.  
> > > >  
> How about using `in_proximity0_offset_cancel` as a userspace interface
> here?

This seems likely to be calibbias or offset.  The cancel or not
is just whether the offset value is 0 or not.

> > > > Again, what makes this a property of the hardware? What hardware
> > > > related
> > > > factors determine that value that you put in here?
> > > >
> > > > Thanks,
> > > > Conor.  
> > >
> > > Certain parameters such as conversion time, wait time, or sampling
> > > rate
> > > are directly tied to the physical characteristics and capabilities
> > > of
> > > the sensor.  
> >
> > Ah. I think I'd missed this uses an external LED (rather than it
> > being
> > in package).  In that case, the characteristics that 'work' for
> > proximity sensing are somewhat dependent on the system design
> > (simplifying heavily, led output for a given current, optical filter
> >  on that LED etc).
> >
> > For the sensor specific side, it should be just from the compatible,
> > but
> > when another part is involved, DT binding based tuning may make sense
> > as long as it is 'per consumer device / board' not per specific
> > instance.
> >
> >
> >
> >  
> > > These parameters are typically determined by the sensor
> > > specifications, and the datasheet usually provides recommended
> > > values
> > > for these parameters. Below is an excerpt from the datasheet:
> > >
> > > /*
> > > +-----------------------+-------+------+------+------+-----+-------
> > > ---+  
> > > > Parameter             | Symbol| Min  | Typ  | Max  | Unit|
> > > > Condition|  
> > > +-----------------------+-------+------+------+------+-----+-------
> > > ---+  
> > > > PS A/D conversion time| TPS   | 21.4 | 25.2 | 28.9 | ms  | PS  
> > > A/DC=16bit  |  
> > > > PS wait time setting  | TPSWAIT| 10.6| 12.5 | 14.3 | ms  | 12.5ms  
> > > setting |
> > > +-----------------------+-------+------+------+------+-----+-------
> > > ---+
> > > */  
> >
> > Doesn't apply to everything you have here though. wtime / wait time
> > is about how often you get a reading not the physical device.  How is
> > that affected by the physical device?
> >
> > I 'think' the table above is giving precision of the value for a
> > particular
> > control setting. If you set wtime to 12.5msec (value 0 in register)
> > then it will actually be between 10.6 and 14.3 msec, not that you
> > should
> > set it to 12.5msec.
> >
> > There are 3 controls related to gain that you could argue for
> > defaults
> > for in DT (maybe) but given proximity sensing is also about the
> > target, not just the measurement device, there won't be a right
> > answer
> > unless your proximity sensor is being used for a fixed purpose (e.g.
> > WIFI signal strength limiting or a button type control).  
> > >
> > >
> > > However, there are some similar cases in the kernel, as follows:
> > >
> > > Documentation/devicetree/bindings/iio/proximity/devantech-
> > > srf04.yaml
> > >     - startup-time-ms  
> > That's after a resume and I think depends one exactly what the
> > circuitry
> > is (in this case the device is more of a reference design than a
> > single
> > device).
> >  
> > > Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > > Documentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
> > >     - semtech,avg-pos-strength
> > >     - semtech,ph01-resolution
> > >     - semtech,input-analog-gain  
> > These are SAR sensors I think, so the sensor element is external to
> > the device.  In theory we could have described the sensing element
> > and used that to work out the right values of these, but in practice
> > it was easier to just provide the parameters from some 'per design'
> > tuning.
> >  
> > >     - ...
> > > Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yam
> > > l
> > >     - vishay,led-current-microamp  
> >
> > I think this is about whether you can burn the external LED out or
> > not.
> > On the datasheet I'm looking at for this device, only value 000 is
> > specified in this 3bit field so I guess it's not controllable?
> >
> > Pulse counts are less likely to be relevant to the LED burning out,
> > but
> > maybe(?)
> >
> > Anyhow, it's not entirely obvious to me that it makes sense to
> > control
> > so much in DT for this device.  Better to put it in userspace control
> > and rely on udev etc setting things right for a given device +
> > application.
> >
> > Jonathan
> >  
> I agree with your comments, we'll modify the DT to allow userspace
> control as introduced above. Consequently, all the dt properites will
> be removed.

That's good, but do focus on how to fit existing ABI as that's
what existing userspace is aware of.

Jonathan

> 
> 
> Thanks,
> Hua Qian
> >
> >
> >  
> > >
> > > This is why we are leveraging the hardware properties.
> > >
> > > Thanks,
> > > Hua Qian
> > >  
> > > >  
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    default: 0
> > > > > +    maximum: 65535
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +
> > > > > +unevaluatedProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    i2c {
> > > > > +        #address-cells = <1>;
> > > > > +        #size-cells = <0>;
> > > > > +
> > > > > +        lightsensor: pm16d17@44 {
> > > > > +            compatible = "everlight,pm16d17";
> > > > > +            reg = <0x44>;
> > > > > +
> > > > > +            ps-gain = <1>;
> > > > > +            ps-itime = "0.4";
> > > > > +            ps-wtime = "12.5";
> > > > > +            ps-ir-led-pulse-count = <1>;
> > > > > +            ps-offset-cancel = <280>;
> > > > > +        };
> > > > > +    };
> > > > > --
> > > > > 2.43.0
> > > > >  
> > >  
> >  
> 
> --
> Hua Qian Li
> Siemens AG
> http://www.siemens.com/