[PATCH 1/3] iio: add IIO_MOD_NOX modifier

[PATCH 1/3] iio: add IIO_MOD_NOX modifier

[Hermes Zhang]

Add modifier IIO_MOD_NOX for NOx concentration reporting. NOx (a generic
term for the mono-nitrogen oxides) are used in environment sensor as a
parameter to show the concentration in index format.

An example case: https://www.sensirion.com/media/documents/9F289B95/
6294DFFC/Info_Note_NOx_Index.pdf

Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 2 ++
 drivers/iio/industrialio-core.c         | 1 +
 include/uapi/linux/iio/types.h          | 1 +
 tools/iio/iio_event_monitor.c           | 2 ++
 4 files changed, 6 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index f83bd6829285..c7d54dc1f226 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1898,6 +1898,8 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_o2_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_concentrationX_o2_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_voc_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_concentrationX_voc_raw
+What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_nox_raw
+What:		/sys/bus/iio/devices/iio:deviceX/in_concentrationX_nox_raw
 KernelVersion:	4.3
 Contact:	linux-iio@vger.kernel.org
 Description:
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index a2117ad1337d..6a85688c9148 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -151,6 +151,7 @@ static const char * const iio_modifier_names[] = {
 	[IIO_MOD_PITCH] = "pitch",
 	[IIO_MOD_YAW] = "yaw",
 	[IIO_MOD_ROLL] = "roll",
+	[IIO_MOD_NOX] = "nox",
 };
 
 /* relies on pairs of these shared then separate */
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 12886d4465e4..f7dfc4c71495 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -108,6 +108,7 @@ enum iio_modifier {
 	IIO_MOD_ROLL,
 	IIO_MOD_LIGHT_UVA,
 	IIO_MOD_LIGHT_UVB,
+	IIO_MOD_NOX,
 };
 
 enum iio_event_type {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index cccf62ea2b8f..51c6f753e7d4 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -138,6 +138,7 @@ static const char * const iio_modifier_names[] = {
 	[IIO_MOD_PITCH] = "pitch",
 	[IIO_MOD_YAW] = "yaw",
 	[IIO_MOD_ROLL] = "roll",
+	[IIO_MOD_NOX] = "nox",
 };
 
 static bool event_is_known(struct iio_event_data *event)
@@ -236,6 +237,7 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_MOD_PM4:
 	case IIO_MOD_PM10:
 	case IIO_MOD_O2:
+	case IIO_MOD_NOX:
 		break;
 	default:
 		return false;
-- 
2.45.2

[PATCH 2/3] dt-bindings: iio: chemical: sensirion,senxx: Add yaml description

[Hermes Zhang]

Add documentation for the SEN5x/SEN6x environmental sensor from Sensirion.

Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
---
 .../iio/chemical/sensirion,senxx.yaml         | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml

diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
new file mode 100644
index 000000000000..4d998eabe441
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/sensirion,senxx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sensirion SEN5x/SEN6x environmental sensor
+
+maintainers:
+  - Hermes Zhang <chenhuiz@axis.com>
+
+description: |
+  The SEN5x/SEN6x is a sensor module family combining the measurement of air
+  quality parameters: particulate matter, VOC, NOx, humidity, and temperature.
+
+  Datasheet:
+  https://sensirion.com/media/documents/6791EFA0/62A1F68F/Sensirion_Datasheet_Environmental_Node_SEN5x.pdf
+  https://sensirion.com/media/documents/FAFC548D/6731FFFA/Sensirion_Datasheet_SEN6x.pdf
+
+properties:
+  compatible:
+    enum:
+      - sensirion,sen50
+      - sensirion,sen54
+      - sensirion,sen55
+      - sensirion,sen60
+      - sensirion,sen65
+      - sensirion,sen66
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      sen55@69 {
+        compatible = "sensirion,sen55";
+        reg = <0x69>;
+      };
+    };
-- 
2.45.2

[PATCH 3/3] iio: chemical: add support for Sensirion SEN5x/SEN6x

[Hermes Zhang]

Add support for the Senseair SEN5x/SEN6x environment sensor through the
IIO subsystem, include SEN50, SEN54, SEN55, SEN60, SEN65 and SEN66.
Different sensor channels will be supported in different models:

SEN50: PM1, PM2.5, PM4, PM10
SEN54: PM1, PM2.5, PM4, PM10, Humidity, Temperature, VOC
SEN55: PM1, PM2.5, PM4, PM10, Humidity, Temperature, VOC, NOx

SEN60: PM1, PM2.5, PM4, PM10
SEN65: PM1, PM2.5, PM4, PM10, Humidity, Temperature, VOC, NOx
SEN66: PM1, PM2.5, PM4, PM10, Humidity, Temperature, VOC, NOx, CO2

The driver support to read sensor data from raw data or iio buffer with
software trigger configured.

Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
---
 drivers/iio/chemical/Kconfig      |  27 ++
 drivers/iio/chemical/Makefile     |   3 +
 drivers/iio/chemical/sen5x.c      |  76 +++++
 drivers/iio/chemical/sen6x.c      |  76 +++++
 drivers/iio/chemical/sen_common.c | 464 ++++++++++++++++++++++++++++++
 drivers/iio/chemical/sen_common.h |  29 ++
 6 files changed, 675 insertions(+)
 create mode 100644 drivers/iio/chemical/sen5x.c
 create mode 100644 drivers/iio/chemical/sen6x.c
 create mode 100644 drivers/iio/chemical/sen_common.c
 create mode 100644 drivers/iio/chemical/sen_common.h

diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 330fe0af946f..8a21ac74d12b 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -166,6 +166,33 @@ config SCD4X
 	  To compile this driver as a module, choose M here: the module will
 	  be called scd4x.
 
+config SENSIRION_SEN_COMMON
+	tristate
+	depends on I2C
+	select CRC8
+	help
+	  Common Sensirion environmental sensor code
+
+config SENSIRION_SEN5X
+	tristate "Sensirion SEN5x environmental sensor"
+	select SENSIRION_SEN_COMMON
+	help
+	  Say Y here to build support for the Sensirion SEN5x environmental
+	  sensor driver.
+
+	  To compile this driver as module, choose M here: the module will
+	  be called sen5x.
+
+config SENSIRION_SEN6X
+	tristate "Sensirion SEN6x environmental sensor"
+	select SENSIRION_SEN_COMMON
+	help
+	  Say Y here to build support for the Sensirion SEN6x environmental
+	  sensor driver.
+
+	  To compile this driver as module, choose M here: the module will
+	  be called sen6x.
+
 config SENSIRION_SGP30
 	tristate "Sensirion SGPxx gas sensors"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index 4866db06bdc9..988c929383d4 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -21,6 +21,9 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
 obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
 obj-$(CONFIG_SCD4X) += scd4x.o
 obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
+obj-$(CONFIG_SENSIRION_SEN_COMMON)	+= sen_common.o
+obj-$(CONFIG_SENSIRION_SEN5X)	+= sen5x.o
+obj-$(CONFIG_SENSIRION_SEN6X)	+= sen6x.o
 obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
 obj-$(CONFIG_SENSIRION_SGP40)	+= sgp40.o
 obj-$(CONFIG_SPS30) += sps30.o
diff --git a/drivers/iio/chemical/sen5x.c b/drivers/iio/chemical/sen5x.c
new file mode 100644
index 000000000000..ddd1cbf18d0c
--- /dev/null
+++ b/drivers/iio/chemical/sen5x.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "sen_common.h"
+
+#define SEN50_NUM_CHANNELS 4 /* PM1,PM2.5,PM4,PM10 */
+#define SEN54_NUM_CHANNELS 7 /* PM1,PM2.5,PM4,PM10,RHT,TEMP,VOC */
+#define SEN55_NUM_CHANNELS 8 /* PM1,PM2.5,PM4,PM10,RHT,TEMP,VOC,NOx */
+
+#define SEN5X_RESET_DELAY 100
+
+#define SEN5X_READ_MEASURED 0x03C4
+
+enum {
+	SEN50,
+	SEN54,
+	SEN55,
+};
+
+static const struct sen_chip_info sen5x_chips[] = {
+	[SEN50] = {
+		.reset_delay = SEN5X_RESET_DELAY,
+		.num_iio_channels = SEN50_NUM_CHANNELS,
+		.read_measured_cmd = SEN5X_READ_MEASURED,
+		.read_measured_rx_size = 24,
+		.temperature_compensation_required = false,
+	},
+	[SEN54] = {
+		.reset_delay = SEN5X_RESET_DELAY,
+		.num_iio_channels = SEN54_NUM_CHANNELS,
+		.read_measured_cmd = SEN5X_READ_MEASURED,
+		.read_measured_rx_size = 24,
+		.temperature_compensation_required = true,
+	},
+	[SEN55] = {
+		.reset_delay = SEN5X_RESET_DELAY,
+		.num_iio_channels = SEN55_NUM_CHANNELS,
+		.read_measured_cmd = SEN5X_READ_MEASURED,
+		.read_measured_rx_size = 24,
+		.temperature_compensation_required = true,
+	},
+};
+
+static int sen5x_probe(struct i2c_client *client)
+{
+	return sen_common_probe(client);
+}
+
+static const struct i2c_device_id sen5x_id[] = {
+						 { "sen50", },
+						 { "sen54", },
+						 { "sen55", },
+						 {} };
+
+MODULE_DEVICE_TABLE(i2c, sen5x_id);
+
+static const struct of_device_id sen5x_dt_ids[] = {
+	{ .compatible = "sensirion,sen50", .data = &sen5x_chips[SEN50] },
+	{ .compatible = "sensirion,sen54", .data = &sen5x_chips[SEN54] },
+	{ .compatible = "sensirion,sen55", .data = &sen5x_chips[SEN55] },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, sen5x_dt_ids);
+
+static struct i2c_driver sen5x_driver = {
+	.driver = {
+		.name = "sen5x",
+		.of_match_table = sen5x_dt_ids,
+	},
+	.probe = sen5x_probe,
+	.id_table = sen5x_id,
+};
+module_i2c_driver(sen5x_driver);
+
+MODULE_DESCRIPTION("Sensirion SEN5x environmental sensor");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/chemical/sen6x.c b/drivers/iio/chemical/sen6x.c
new file mode 100644
index 000000000000..ba99242f30ef
--- /dev/null
+++ b/drivers/iio/chemical/sen6x.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "sen_common.h"
+
+#define SEN60_NUM_CHANNELS 4 /* PM1,PM2.5,PM4,PM10 */
+#define SEN65_NUM_CHANNELS 8 /* PM1,PM2.5,PM4,PM10,RHT,TEMP,VOC,NOx */
+#define SEN66_NUM_CHANNELS 9 /* PM1,PM2.5,PM4,PM10,RHT,TEMP,VOC,NOx,CO2 */
+
+#define SEN6X_RESET_DELAY 1200
+
+#define SEN6X_READ_MEASURED 0x0300
+
+enum {
+	SEN60,
+	SEN65,
+	SEN66,
+};
+
+static const struct sen_chip_info sen6x_chips[] = {
+	[SEN60] = {
+		.reset_delay = SEN6X_RESET_DELAY,
+		.num_iio_channels = SEN60_NUM_CHANNELS,
+		.read_measured_cmd = SEN6X_READ_MEASURED,
+		.read_measured_rx_size = 27,
+		.temperature_compensation_required = false,
+	},
+	[SEN65] = {
+		.reset_delay = SEN6X_RESET_DELAY,
+		.num_iio_channels = SEN65_NUM_CHANNELS,
+		.read_measured_cmd = SEN6X_READ_MEASURED,
+		.read_measured_rx_size = 27,
+		.temperature_compensation_required = false,
+	},
+	[SEN66] = {
+		.reset_delay = SEN6X_RESET_DELAY,
+		.num_iio_channels = SEN66_NUM_CHANNELS,
+		.read_measured_cmd = SEN6X_READ_MEASURED,
+		.read_measured_rx_size = 27,
+		.temperature_compensation_required = false,
+	},
+};
+
+static int sen6x_probe(struct i2c_client *client)
+{
+	return sen_common_probe(client);
+}
+
+static const struct i2c_device_id sen6x_id[] = {
+						 { "sen60", },
+						 { "sen65", },
+						 { "sen66", },
+						 {} };
+
+MODULE_DEVICE_TABLE(i2c, sen6x_id);
+
+static const struct of_device_id sen6x_dt_ids[] = {
+	{ .compatible = "sensirion,sen60", .data = &sen6x_chips[SEN60] },
+	{ .compatible = "sensirion,sen65", .data = &sen6x_chips[SEN65] },
+	{ .compatible = "sensirion,sen66", .data = &sen6x_chips[SEN66] },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, sen6x_dt_ids);
+
+static struct i2c_driver sen6x_driver = {
+	.driver = {
+		.name = "sen6x",
+		.of_match_table = sen6x_dt_ids,
+	},
+	.probe = sen6x_probe,
+	.id_table = sen6x_id,
+};
+module_i2c_driver(sen6x_driver);
+
+MODULE_DESCRIPTION("Sensirion SEN6x environmental sensor");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/chemical/sen_common.c b/drivers/iio/chemical/sen_common.c
new file mode 100644
index 000000000000..5fad439c87d9
--- /dev/null
+++ b/drivers/iio/chemical/sen_common.c
@@ -0,0 +1,464 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/crc8.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include "sen_common.h"
+
+DECLARE_CRC8_TABLE(sen_common_crc8_table);
+
+enum {
+	PM1,
+	PM2P5,
+	PM4,
+	PM10,
+	RHT,
+	TEMP,
+	VOC,
+	NOX,
+	CO2,
+};
+
+#define SENXX_CRC8_POLYNOMIAL 0x31
+#define SENXX_CRC8_INIT 0xff
+
+/* Command */
+#define SENXX_START_MEASUREMENT 0x0021
+#define SENXX_STOP_MEASUREMENT 0x0104
+#define SENXX_READ_DATA_READY 0x0202
+#define SENXX_PRODUCT_NAME 0xD014
+#define SENXX_DEVICE_STATUS 0xD206
+#define SENXX_RESET 0xD304
+#define SEN5X_TEMPERATURE 0x60B2
+
+#define SENXX_CMD_HEAD_LEN 2
+
+#define SENXX_PRODUCT_NAME_RSP_LEN 48
+#define SENXX_DEVICE_STATUS_RSP_LEN 6
+
+#define SEN5X_TEMPERATURE_REQ_LEN 9
+
+/* For sen5x(03C4) is 24 and sen6x(0300) is 27 */
+#define SENXX_MAX_MEASURED_RSP_LEN 27
+
+#define PACK_CMD_HEAD(value, buffer)               \
+	do {                                       \
+		(buffer)[0] = ((value) >> 8) & 0xFF; \
+		(buffer)[1] = (value) & 0xFF;        \
+	} while (0)
+
+/* Convert a 16-bit value to big-endian, store in a 3-bytes buffer with calculated CRC */
+#define PACK_TO_BUFFER_WITH_CRC(value, buffer)                       \
+	do {                                                         \
+		u16 __be_value = cpu_to_be16(value);                 \
+		(buffer)[0] = (__be_value >> 8) & 0xFF;              \
+		(buffer)[1] = __be_value & 0xFF;                     \
+		(buffer)[2] = crc8(sen_common_crc8_table, buffer, 2, \
+				   CRC8_INIT_VALUE);                 \
+	} while (0)
+
+static int sen_common_i2c_recv_reply(struct i2c_client *client,
+				     unsigned char *buf, unsigned int size)
+{
+	struct device *dev = &client->dev;
+	unsigned char *tmp = buf;
+	int ret;
+	int i;
+
+	ret = i2c_master_recv(client, buf, size);
+	if (ret < 0)
+		return ret;
+	if (ret != size) {
+		dev_warn(dev, "i2c_master_recv ret: %d size: %u\n", ret, size);
+		return -EIO;
+	}
+
+	/* All the Read and Write Commands transmit the data in 2-byte packets,
+	 * followed by an 8-bit checksum
+	 */
+	for (i = 0; i < size / 3; i++) {
+		unsigned char crc;
+
+		crc = crc8(sen_common_crc8_table, buf + i * 3, 2,
+			   CRC8_INIT_VALUE);
+		if (crc != buf[i * 3 + 2]) {
+			dev_err(dev, "data integrity check failed\n");
+			return -EIO;
+		}
+
+		*tmp++ = buf[i * 3];
+		*tmp++ = buf[i * 3 + 1];
+	}
+
+	return tmp - buf;
+}
+
+static int sen_common_simple_command(struct i2c_client *client,
+				     unsigned int cmd, unsigned int time)
+{
+	unsigned char txbuf[SENXX_CMD_HEAD_LEN];
+	int ret;
+
+	PACK_CMD_HEAD(cmd, &txbuf[0]);
+
+	ret = i2c_master_send(client, txbuf, 2);
+	if (ret < 0)
+		return ret;
+
+	msleep(time);
+
+	return 0;
+}
+
+static int sen_common_product_name(struct i2c_client *client, char *name,
+				   size_t len)
+{
+	unsigned char txbuf[SENXX_CMD_HEAD_LEN];
+	unsigned char rxbuf[SENXX_PRODUCT_NAME_RSP_LEN];
+	int ret;
+
+	PACK_CMD_HEAD(SENXX_PRODUCT_NAME, &txbuf[0]);
+
+	ret = i2c_master_send(client, txbuf, 2);
+	if (ret < 0)
+		return ret;
+
+	msleep(20);
+
+	ret = sen_common_i2c_recv_reply(client, rxbuf, sizeof(rxbuf));
+	if (ret < 0)
+		return ret;
+
+	rxbuf[ret] = '\0';
+
+	strscpy(name, rxbuf, len);
+
+	return 0;
+}
+
+static int sen_common_status(struct i2c_client *client, int *status)
+{
+	unsigned char txbuf[SENXX_CMD_HEAD_LEN];
+	unsigned char rxbuf[SENXX_DEVICE_STATUS_RSP_LEN];
+	int ret;
+
+	PACK_CMD_HEAD(SENXX_DEVICE_STATUS, &txbuf[0]);
+
+	ret = i2c_master_send(client, txbuf, 2);
+	if (ret < 0)
+		return ret;
+
+	msleep(20);
+
+	ret = sen_common_i2c_recv_reply(client, rxbuf, sizeof(rxbuf));
+	if (ret < 0)
+		return ret;
+
+	*status = (rxbuf[0] << 24) + (rxbuf[1] << 16) + (rxbuf[2] << 8) +
+		  rxbuf[3];
+
+	return 0;
+}
+
+static int sen5x_set_temperature_compensation(struct i2c_client *client,
+					      s16 offset, s16 slope, u16 time)
+{
+	unsigned char txbuf[SENXX_CMD_HEAD_LEN + SEN5X_TEMPERATURE_REQ_LEN];
+	int ret;
+
+	PACK_CMD_HEAD(SEN5X_TEMPERATURE, &txbuf[0]);
+
+	PACK_TO_BUFFER_WITH_CRC(offset, &txbuf[2]);
+	PACK_TO_BUFFER_WITH_CRC(slope, &txbuf[5]);
+	PACK_TO_BUFFER_WITH_CRC(time, &txbuf[8]);
+
+	ret = i2c_master_send(client, txbuf, sizeof(txbuf));
+	if (ret < 0)
+		return ret;
+
+	msleep(20);
+
+	return 0;
+}
+
+static int sen_common_fetch_measured(struct sen_common_state *state)
+{
+	struct i2c_client *client = state->client;
+	unsigned char txbuf[SENXX_CMD_HEAD_LEN];
+	unsigned char rxbuf[SENXX_MAX_MEASURED_RSP_LEN];
+	int rx_size = state->chip->read_measured_rx_size;
+	int ret;
+
+	if (rx_size > SENXX_MAX_MEASURED_RSP_LEN)
+		return -EINVAL;
+
+	PACK_CMD_HEAD(state->chip->read_measured_cmd, &txbuf[0]);
+
+	/* sensor can only be polled once a second max per datasheet */
+	if (!time_after(jiffies, state->last_update + HZ))
+		return 0;
+
+	ret = i2c_master_send(client, txbuf, sizeof(txbuf));
+	if (ret < 0)
+		return ret;
+
+	msleep(20);
+
+	ret = sen_common_i2c_recv_reply(client, rxbuf, rx_size);
+	if (ret < 0)
+		return ret;
+
+	memcpy(&state->data, rxbuf, ret);
+
+	state->last_update = jiffies;
+
+	return 0;
+}
+
+static ssize_t status_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct sen_common_state *state = iio_priv(indio_dev);
+	int status;
+	int ret;
+
+	ret = sen_common_status(state->client, &status);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%x\n", status);
+}
+
+static IIO_DEVICE_ATTR_RO(status, 0);
+
+static struct attribute *sen_common_attrs[] = {
+	&iio_dev_attr_status.dev_attr.attr, NULL
+};
+
+static const struct attribute_group sen_common_attr_group = {
+	.attrs = sen_common_attrs,
+};
+
+#define SEN_CHAN_SCAN_TYPE(_sign)     \
+	.scan_type = {                \
+		.sign = _sign,        \
+		.realbits = 16,       \
+		.storagebits = 16,    \
+		.endianness = IIO_BE, \
+	}
+
+#define SEN_MASSCONC_CHAN(_index, _mod, _sign)                           \
+	{                                                                \
+		.type = IIO_MASSCONCENTRATION, .modified = 1,            \
+		.channel2 = IIO_MOD_##_mod,                              \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),            \
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+		.address = _mod, .scan_index = _index,                   \
+		SEN_CHAN_SCAN_TYPE(_sign),                               \
+	}
+
+#define SEN_CONC_CHAN(_index, _mod, _sign)                               \
+	{                                                                \
+		.type = IIO_CONCENTRATION, .modified = 1,                \
+		.channel2 = IIO_MOD_##_mod,                              \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |           \
+				      BIT(IIO_CHAN_INFO_SCALE),          \
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+		.address = _mod, .scan_index = _index,                   \
+		SEN_CHAN_SCAN_TYPE(_sign),                               \
+	}
+
+static const struct iio_chan_spec sen_common_channels[] = {
+	SEN_MASSCONC_CHAN(0, PM1, 'u'),
+	SEN_MASSCONC_CHAN(1, PM2P5, 'u'),
+	SEN_MASSCONC_CHAN(2, PM4, 'u'),
+	SEN_MASSCONC_CHAN(3, PM10, 'u'),
+	{
+		.type = IIO_HUMIDITYRELATIVE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.address = RHT,
+		.scan_index = 4,
+		SEN_CHAN_SCAN_TYPE('s'),
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.address = TEMP,
+		.scan_index = 5,
+		SEN_CHAN_SCAN_TYPE('s'),
+	},
+	SEN_CONC_CHAN(6, VOC, 's'),
+	SEN_CONC_CHAN(7, NOX, 's'),
+	SEN_CONC_CHAN(8, CO2, 'u'),
+};
+
+static int sen_common_read_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan, int *val,
+			       int *val2, long mask)
+{
+	struct sen_common_state *state = iio_priv(indio_dev);
+	int ret = IIO_VAL_INT;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&state->lock);
+		ret = sen_common_fetch_measured(state);
+		if (ret < 0) {
+			mutex_unlock(&state->lock);
+			return ret;
+		}
+
+		*val = be16_to_cpu(state->data[chan->address]);
+
+		mutex_unlock(&state->lock);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_MASSCONCENTRATION:
+			*val = 0;
+			*val2 = 100000;
+
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_HUMIDITYRELATIVE:
+			*val = 0;
+			*val2 = 10000;
+
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_TEMP:
+			*val = 0;
+			*val2 = 5000;
+
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_CONCENTRATION:
+			switch (chan->channel2) {
+			case IIO_MOD_NOX:
+			case IIO_MOD_VOC:
+				*val = 0;
+				*val2 = 100000;
+
+				return IIO_VAL_INT_PLUS_MICRO;
+			case IIO_MOD_CO2:
+				*val = 1;
+				*val2 = 0;
+
+				return IIO_VAL_INT_PLUS_MICRO;
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = 1;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info sen_common_info = {
+	.attrs = &sen_common_attr_group,
+	.read_raw = sen_common_read_raw,
+};
+
+static irqreturn_t sen_common_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct sen_common_state *state = iio_priv(indio_dev);
+
+	mutex_lock(&state->lock);
+
+	if (sen_common_fetch_measured(state) < 0)
+		goto out;
+
+	iio_push_to_buffers(indio_dev, &state->data);
+
+out:
+	mutex_unlock(&state->lock);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+int sen_common_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	const struct sen_chip_info *chip;
+	const char *name = client->name;
+	struct sen_common_state *state;
+	struct iio_dev *indio_dev;
+	char senxx[8];
+	int ret = 0;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct sen_common_state));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = i2c_get_match_data(client);
+
+	state = iio_priv(indio_dev);
+	state->client = client;
+	state->dev = dev;
+	mutex_init(&state->lock);
+	state->chip = chip;
+
+	crc8_populate_msb(sen_common_crc8_table, SENXX_CRC8_POLYNOMIAL);
+
+	ret = sen_common_simple_command(client, SENXX_RESET, chip->reset_delay);
+	if (ret < 0)
+		return ret;
+
+	ret = sen_common_product_name(client, senxx, sizeof(senxx));
+	if (ret < 0)
+		return ret;
+
+	if (strncasecmp(senxx, name, sizeof(senxx)) != 0)
+		dev_warn(dev, "chip mismatch: %s != %s\n", senxx, name);
+
+	/* Set default temperature compensation parameters */
+	if (chip->temperature_compensation_required) {
+		ret = sen5x_set_temperature_compensation(client, 0, 0, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = sen_common_simple_command(client, SENXX_START_MEASUREMENT, 50);
+	if (ret < 0)
+		return ret;
+
+	indio_dev->name = name;
+	indio_dev->info = &sen_common_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = sen_common_channels;
+	indio_dev->num_channels = chip->num_iio_channels;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      sen_common_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		dev_err(dev, "failed to register iio device\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sen_common_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Sensirion SEN5x/SEN6x common functionality");
diff --git a/drivers/iio/chemical/sen_common.h b/drivers/iio/chemical/sen_common.h
new file mode 100644
index 000000000000..aa40053c53ae
--- /dev/null
+++ b/drivers/iio/chemical/sen_common.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef IIO_SEN_COMMON_H
+#define IIO_SEN_COMMON_H
+
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+
+struct sen_chip_info {
+	int reset_delay;
+	int num_iio_channels;
+	unsigned int read_measured_cmd;
+	unsigned int read_measured_rx_size;
+	bool temperature_compensation_required;
+};
+
+struct sen_common_state {
+	struct device *dev;
+	struct i2c_client *client;
+	struct mutex lock;
+	/* PM1, PM2.5, PM4, PM10, RHT, TEMP, VOC, NOX, CO2 */
+	__be16 data[9];
+	const struct sen_chip_info *chip;
+	unsigned long last_update;
+};
+
+int sen_common_probe(struct i2c_client *client);
+
+#endif
-- 
2.45.2

Re: [PATCH 2/3] dt-bindings: iio: chemical: sensirion,senxx: Add yaml description

[Conor Dooley]

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

On Thu, Feb 06, 2025 at 02:15:16PM +0800, Hermes Zhang wrote:
> Add documentation for the SEN5x/SEN6x environmental sensor from Sensirion.
> 
> Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
> ---
>  .../iio/chemical/sensirion,senxx.yaml         | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
> new file mode 100644
> index 000000000000..4d998eabe441
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml

filename matching a compatible please.

> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/chemical/sensirion,senxx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sensirion SEN5x/SEN6x environmental sensor
> +
> +maintainers:
> +  - Hermes Zhang <chenhuiz@axis.com>
> +
> +description: |
> +  The SEN5x/SEN6x is a sensor module family combining the measurement of air
> +  quality parameters: particulate matter, VOC, NOx, humidity, and temperature.
> +
> +  Datasheet:
> +  https://sensirion.com/media/documents/6791EFA0/62A1F68F/Sensirion_Datasheet_Environmental_Node_SEN5x.pdf
> +  https://sensirion.com/media/documents/FAFC548D/6731FFFA/Sensirion_Datasheet_SEN6x.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sensirion,sen50
> +      - sensirion,sen54
> +      - sensirion,sen55
> +      - sensirion,sen60
> +      - sensirion,sen65
> +      - sensirion,sen66

I'd like a note in the commit message as to how all of these devices are
different please.

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg

No supplies needed for this device? Seems like you would need at least
one, no?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      sen55@69 {
> +        compatible = "sensirion,sen55";
> +        reg = <0x69>;
> +      };
> +    };
> -- 
> 2.45.2
> 

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

Re: [PATCH 2/3] dt-bindings: iio: chemical: sensirion,senxx: Add yaml description

[Hermes Zhang]

Hi,

On 2025/2/7 2:20, Conor Dooley wrote:
> diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
> new file mode 100644
> index 000000000000..4d998eabe441
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
> filename matching a compatible please.
So sensirion,sen66.yaml?
> +  https://sensirion.com/media/documents/6791EFA0/62A1F68F/Sensirion_Datasheet_Environmental_Node_SEN5x.pdf
> +  https://sensirion.com/media/documents/FAFC548D/6731FFFA/Sensirion_Datasheet_SEN6x.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sensirion,sen50
> +      - sensirion,sen54
> +      - sensirion,sen55
> +      - sensirion,sen60
> +      - sensirion,sen65
> +      - sensirion,sen66
> I'd like a note in the commit message as to how all of these devices are
> different please.
Sure, will fix in v2.
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
> No supplies needed for this device? Seems like you would need at least
> one, no?

You mean the vdd-supply? The chip require a e.g. 3.3v VDD, but in our 
HW, we have no gpio/regulator to control it, connect directly by the HW, 
should I still need to have one vdd-supply here?


Best Regards,

Hermes

Re: [PATCH 1/3] iio: add IIO_MOD_NOX modifier

[Jonathan Cameron]

On Thu, 6 Feb 2025 14:15:15 +0800
Hermes Zhang <Hermes.Zhang@axis.com> wrote:

> Add modifier IIO_MOD_NOX for NOx concentration reporting. NOx (a generic
> term for the mono-nitrogen oxides) are used in environment sensor as a
> parameter to show the concentration in index format.
> 
> An example case: https://www.sensirion.com/media/documents/9F289B95/
> 6294DFFC/Info_Note_NOx_Index.pdf
Use a Link tag directly at the top of the tag block (no blank line between
it and your SoB)

> 
> Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 2 ++
>  drivers/iio/industrialio-core.c         | 1 +
>  include/uapi/linux/iio/types.h          | 1 +
>  tools/iio/iio_event_monitor.c           | 2 ++
>  4 files changed, 6 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index f83bd6829285..c7d54dc1f226 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1898,6 +1898,8 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_o2_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_concentrationX_o2_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_voc_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_concentrationX_voc_raw
> +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_nox_raw
> +What:		/sys/bus/iio/devices/iio:deviceX/in_concentrationX_nox_raw

This section defines the units as percent.  Given the link above, this nox value
seems very much to not be in percent and arguably isn't a concentration measure
or if I read that right even a scaled value of that.

I'm not sure how we handle this in a consistent fashion :(
At very least needs a new documentation block with the details of these units.
This a raw attribute so it doesn't have to be possible to scale it to a 'standard'
unit (light color sensors / intensity channels for a similar example).

Is it possible to compare readings off sensors from different companies?




>  KernelVersion:	4.3
>  Contact:	linux-iio@vger.kernel.org
>  Description:
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index a2117ad1337d..6a85688c9148 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -151,6 +151,7 @@ static const char * const iio_modifier_names[] = {
>  	[IIO_MOD_PITCH] = "pitch",
>  	[IIO_MOD_YAW] = "yaw",
>  	[IIO_MOD_ROLL] = "roll",
> +	[IIO_MOD_NOX] = "nox",
>  };
>  
>  /* relies on pairs of these shared then separate */
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index 12886d4465e4..f7dfc4c71495 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -108,6 +108,7 @@ enum iio_modifier {
>  	IIO_MOD_ROLL,
>  	IIO_MOD_LIGHT_UVA,
>  	IIO_MOD_LIGHT_UVB,
> +	IIO_MOD_NOX,
>  };
>  
>  enum iio_event_type {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index cccf62ea2b8f..51c6f753e7d4 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -138,6 +138,7 @@ static const char * const iio_modifier_names[] = {
>  	[IIO_MOD_PITCH] = "pitch",
>  	[IIO_MOD_YAW] = "yaw",
>  	[IIO_MOD_ROLL] = "roll",
> +	[IIO_MOD_NOX] = "nox",
>  };
>  
>  static bool event_is_known(struct iio_event_data *event)
> @@ -236,6 +237,7 @@ static bool event_is_known(struct iio_event_data *event)
>  	case IIO_MOD_PM4:
>  	case IIO_MOD_PM10:
>  	case IIO_MOD_O2:
> +	case IIO_MOD_NOX:
>  		break;
>  	default:
>  		return false;

Re: [PATCH 3/3] iio: chemical: add support for Sensirion SEN5x/SEN6x

[Jonathan Cameron]

On Thu, 6 Feb 2025 14:15:17 +0800
Hermes Zhang <Hermes.Zhang@axis.com> wrote:

> Add support for the Senseair SEN5x/SEN6x environment sensor through the
> IIO subsystem, include SEN50, SEN54, SEN55, SEN60, SEN65 and SEN66.
> Different sensor channels will be supported in different models:
> 
> SEN50: PM1, PM2.5, PM4, PM10
> SEN54: PM1, PM2.5, PM4, PM10, Humidity, Temperature, VOC
> SEN55: PM1, PM2.5, PM4, PM10, Humidity, Temperature, VOC, NOx
> 
> SEN60: PM1, PM2.5, PM4, PM10
> SEN65: PM1, PM2.5, PM4, PM10, Humidity, Temperature, VOC, NOx
> SEN66: PM1, PM2.5, PM4, PM10, Humidity, Temperature, VOC, NOx, CO2
> 
> The driver support to read sensor data from raw data or iio buffer with
> software trigger configured.
> 
> Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
Hi Hermes,

Various comments inline.

Thanks,

Jonathan
> ---
>  drivers/iio/chemical/Kconfig      |  27 ++
>  drivers/iio/chemical/Makefile     |   3 +
>  drivers/iio/chemical/sen5x.c      |  76 +++++
>  drivers/iio/chemical/sen6x.c      |  76 +++++

The two specific drivers are very small. Probably makes more sense to squash
this into one file that supports all the parts.

>  drivers/iio/chemical/sen_common.c | 464 ++++++++++++++++++++++++++++++
>  drivers/iio/chemical/sen_common.h |  29 ++
>  6 files changed, 675 insertions(+)
>  create mode 100644 drivers/iio/chemical/sen5x.c
>  create mode 100644 drivers/iio/chemical/sen6x.c
>  create mode 100644 drivers/iio/chemical/sen_common.c
>  create mode 100644 drivers/iio/chemical/sen_common.h
> 
> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> index 330fe0af946f..8a21ac74d12b 100644
> --- a/drivers/iio/chemical/Kconfig
> +++ b/drivers/iio/chemical/Kconfig
> @@ -166,6 +166,33 @@ config SCD4X
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called scd4x.
>  
> +config SENSIRION_SEN_COMMON
> +	tristate
> +	depends on I2C
> +	select CRC8
> +	help
> +	  Common Sensirion environmental sensor code
> +
> +config SENSIRION_SEN5X
Please avoid wildcards.  We have had too many problems with them in the
past so now we name everything after one particular supported part.

> +	tristate "Sensirion SEN5x environmental sensor"
> +	select SENSIRION_SEN_COMMON
> +	help
> +	  Say Y here to build support for the Sensirion SEN5x environmental
> +	  sensor driver.
It is useful to provide a list of support sensors in here.
> +
> +	  To compile this driver as module, choose M here: the module will
> +	  be called sen5x.
> +
> +config SENSIRION_SEN6X
> +	tristate "Sensirion SEN6x environmental sensor"
> +	select SENSIRION_SEN_COMMON
> +	help
> +	  Say Y here to build support for the Sensirion SEN6x environmental
> +	  sensor driver.
> +
> +	  To compile this driver as module, choose M here: the module will
> +	  be called sen6x.
> +
>  config SENSIRION_SGP30
>  	tristate "Sensirion SGPxx gas sensors"
>  	depends on I2C
> diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> index 4866db06bdc9..988c929383d4 100644
> --- a/drivers/iio/chemical/Makefile
> +++ b/drivers/iio/chemical/Makefile
> @@ -21,6 +21,9 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
>  obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
>  obj-$(CONFIG_SCD4X) += scd4x.o
>  obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
> +obj-$(CONFIG_SENSIRION_SEN_COMMON)	+= sen_common.o
> +obj-$(CONFIG_SENSIRION_SEN5X)	+= sen5x.o
> +obj-$(CONFIG_SENSIRION_SEN6X)	+= sen6x.o
>  obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
>  obj-$(CONFIG_SENSIRION_SGP40)	+= sgp40.o
>  obj-$(CONFIG_SPS30) += sps30.o
> diff --git a/drivers/iio/chemical/sen5x.c b/drivers/iio/chemical/sen5x.c
> new file mode 100644
> index 000000000000..ddd1cbf18d0c
> --- /dev/null
> +++ b/drivers/iio/chemical/sen5x.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include "sen_common.h"
> +
> +#define SEN50_NUM_CHANNELS 4 /* PM1,PM2.5,PM4,PM10 */
> +#define SEN54_NUM_CHANNELS 7 /* PM1,PM2.5,PM4,PM10,RHT,TEMP,VOC */
> +#define SEN55_NUM_CHANNELS 8 /* PM1,PM2.5,PM4,PM10,RHT,TEMP,VOC,NOx */

These defines aren't particularly useful.  Just put the numbers directly
in below and move the comments down there.

> +
> +#define SEN5X_RESET_DELAY 100
> +
> +#define SEN5X_READ_MEASURED 0x03C4
> +
> +enum {
> +	SEN50,
> +	SEN54,
> +	SEN55,
In more recent drivers we have moved away from arrays
of chip info structures indexed by enum values.  It
ends up being easier to read if we just have separate
static const struct sen_chip_info sen50_chip;
static const struct sen_chip_info sen54_chip;

etc and then us pointers to those in the id tables.

> +};
> +
> +static const struct sen_chip_info sen5x_chips[] = {
> +	[SEN50] = {
> +		.reset_delay = SEN5X_RESET_DELAY,
> +		.num_iio_channels = SEN50_NUM_CHANNELS,
> +		.read_measured_cmd = SEN5X_READ_MEASURED,
> +		.read_measured_rx_size = 24,
> +		.temperature_compensation_required = false,
> +	},
> +	[SEN54] = {
> +		.reset_delay = SEN5X_RESET_DELAY,
> +		.num_iio_channels = SEN54_NUM_CHANNELS,
> +		.read_measured_cmd = SEN5X_READ_MEASURED,
> +		.read_measured_rx_size = 24,
> +		.temperature_compensation_required = true,
> +	},
> +	[SEN55] = {
> +		.reset_delay = SEN5X_RESET_DELAY,
> +		.num_iio_channels = SEN55_NUM_CHANNELS,
> +		.read_measured_cmd = SEN5X_READ_MEASURED,
> +		.read_measured_rx_size = 24,
> +		.temperature_compensation_required = true,
> +	},
> +};
> +
> +static int sen5x_probe(struct i2c_client *client)
> +{
> +	return sen_common_probe(client);
> +}
> +
> +static const struct i2c_device_id sen5x_id[] = {
> +						 { "sen50", },
> +						 { "sen54", },
> +						 { "sen55", },
These should have the same das as in the of_device_id table
and look ups should use the i2c core provided functions that fallback
to this array if a match isn't found in the of_device_id one.
That avoids tightly coupling the two as sometimes they may be out
of sync for various reasons..

> +						 {} };
Formatting wise:
static const struct i2c_device_id sen5x_id[] = {
	{ "sens50", ...

Just one tab ore than the line above.

> +
> +MODULE_DEVICE_TABLE(i2c, sen5x_id);
> +
> +static const struct of_device_id sen5x_dt_ids[] = {
> +	{ .compatible = "sensirion,sen50", .data = &sen5x_chips[SEN50] },
> +	{ .compatible = "sensirion,sen54", .data = &sen5x_chips[SEN54] },
> +	{ .compatible = "sensirion,sen55", .data = &sen5x_chips[SEN55] },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, sen5x_dt_ids);
> +
> +static struct i2c_driver sen5x_driver = {
> +	.driver = {
> +		.name = "sen5x",
> +		.of_match_table = sen5x_dt_ids,
> +	},
> +	.probe = sen5x_probe,
> +	.id_table = sen5x_id,
> +};
> +module_i2c_driver(sen5x_driver);
> +
> +MODULE_DESCRIPTION("Sensirion SEN5x environmental sensor");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/chemical/sen6x.c b/drivers/iio/chemical/sen6x.c
> new file mode 100644
> index 000000000000..ba99242f30ef
> --- /dev/null
> +++ b/drivers/iio/chemical/sen6x.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include "sen_common.h"
> +
> +#define SEN60_NUM_CHANNELS 4 /* PM1,PM2.5,PM4,PM10 */
> +#define SEN65_NUM_CHANNELS 8 /* PM1,PM2.5,PM4,PM10,RHT,TEMP,VOC,NOx */
> +#define SEN66_NUM_CHANNELS 9 /* PM1,PM2.5,PM4,PM10,RHT,TEMP,VOC,NOx,CO2 */
> +
> +#define SEN6X_RESET_DELAY 1200
> +
> +#define SEN6X_READ_MEASURED 0x0300
> +
> +enum {
> +	SEN60,
> +	SEN65,
> +	SEN66,
> +};
> +
> +static const struct sen_chip_info sen6x_chips[] = {
> +	[SEN60] = {
> +		.reset_delay = SEN6X_RESET_DELAY,
> +		.num_iio_channels = SEN60_NUM_CHANNELS,
> +		.read_measured_cmd = SEN6X_READ_MEASURED,
> +		.read_measured_rx_size = 27,
> +		.temperature_compensation_required = false,
> +	},
> +	[SEN65] = {
> +		.reset_delay = SEN6X_RESET_DELAY,
> +		.num_iio_channels = SEN65_NUM_CHANNELS,
> +		.read_measured_cmd = SEN6X_READ_MEASURED,
> +		.read_measured_rx_size = 27,
> +		.temperature_compensation_required = false,
> +	},
> +	[SEN66] = {
> +		.reset_delay = SEN6X_RESET_DELAY,
> +		.num_iio_channels = SEN66_NUM_CHANNELS,
> +		.read_measured_cmd = SEN6X_READ_MEASURED,
> +		.read_measured_rx_size = 27,
> +		.temperature_compensation_required = false,
> +	},
> +};
> +
> +static int sen6x_probe(struct i2c_client *client)
> +{
> +	return sen_common_probe(client);
> +}
> +
> +static const struct i2c_device_id sen6x_id[] = {
> +						 { "sen60", },
> +						 { "sen65", },
> +						 { "sen66", },
> +						 {} };
> +
> +MODULE_DEVICE_TABLE(i2c, sen6x_id);
> +
> +static const struct of_device_id sen6x_dt_ids[] = {
> +	{ .compatible = "sensirion,sen60", .data = &sen6x_chips[SEN60] },
> +	{ .compatible = "sensirion,sen65", .data = &sen6x_chips[SEN65] },
> +	{ .compatible = "sensirion,sen66", .data = &sen6x_chips[SEN66] },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, sen6x_dt_ids);
> +
> +static struct i2c_driver sen6x_driver = {
> +	.driver = {
> +		.name = "sen6x",
> +		.of_match_table = sen6x_dt_ids,
> +	},
> +	.probe = sen6x_probe,
> +	.id_table = sen6x_id,
> +};
> +module_i2c_driver(sen6x_driver);

As above, there seems to be no advantage in having two drivers. Just merge
them into one that supports all the parts directly. 

> +
> +MODULE_DESCRIPTION("Sensirion SEN6x environmental sensor");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/chemical/sen_common.c b/drivers/iio/chemical/sen_common.c
> new file mode 100644
> index 000000000000..5fad439c87d9
> --- /dev/null
> +++ b/drivers/iio/chemical/sen_common.c
> @@ -0,0 +1,464 @@

> +#define PACK_CMD_HEAD(value, buffer)               \
> +	do {                                       \
> +		(buffer)[0] = ((value) >> 8) & 0xFF; \
> +		(buffer)[1] = (value) & 0xFF;        \
> +	} while (0)
This is a put_unaligned_be16() so just use that.


> +
> +/* Convert a 16-bit value to big-endian, store in a 3-bytes buffer with calculated CRC */
> +#define PACK_TO_BUFFER_WITH_CRC(value, buffer)                       \
> +	do {                                                         \
Better as a function I think. Let the compiler figure out if it wants to inline it
or not.

> +		u16 __be_value = cpu_to_be16(value);                 \
> +		(buffer)[0] = (__be_value >> 8) & 0xFF;              \
> +		(buffer)[1] = __be_value & 0xFF;                     \

unaligned put.

> +		(buffer)[2] = crc8(sen_common_crc8_table, buffer, 2, \
> +				   CRC8_INIT_VALUE);                 \
> +	} while (0)

>
> +
> +static int sen_common_status(struct i2c_client *client, int *status)
> +{
> +	unsigned char txbuf[SENXX_CMD_HEAD_LEN];
> +	unsigned char rxbuf[SENXX_DEVICE_STATUS_RSP_LEN];
> +	int ret;
> +
> +	PACK_CMD_HEAD(SENXX_DEVICE_STATUS, &txbuf[0]);
> +
> +	ret = i2c_master_send(client, txbuf, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ret = sen_common_i2c_recv_reply(client, rxbuf, sizeof(rxbuf));
> +	if (ret < 0)
> +		return ret;
> +
> +	*status = (rxbuf[0] << 24) + (rxbuf[1] << 16) + (rxbuf[2] << 8) +
> +		  rxbuf[3];
get_unaligned_be32()?

> +
> +	return 0;
> +}
> +
> +static int sen5x_set_temperature_compensation(struct i2c_client *client,
> +					      s16 offset, s16 slope, u16 time)
> +{
> +	unsigned char txbuf[SENXX_CMD_HEAD_LEN + SEN5X_TEMPERATURE_REQ_LEN];
> +	int ret;
> +
> +	PACK_CMD_HEAD(SEN5X_TEMPERATURE, &txbuf[0]);
> +
> +	PACK_TO_BUFFER_WITH_CRC(offset, &txbuf[2]);
> +	PACK_TO_BUFFER_WITH_CRC(slope, &txbuf[5]);
> +	PACK_TO_BUFFER_WITH_CRC(time, &txbuf[8]);
> +
> +	ret = i2c_master_send(client, txbuf, sizeof(txbuf));
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(20);
> +
> +	return 0;
> +}
> +
> +static int sen_common_fetch_measured(struct sen_common_state *state)
> +{
> +	struct i2c_client *client = state->client;
> +	unsigned char txbuf[SENXX_CMD_HEAD_LEN];
> +	unsigned char rxbuf[SENXX_MAX_MEASURED_RSP_LEN];

u8 for these.

> +	int rx_size = state->chip->read_measured_rx_size;
> +	int ret;
> +
> +	if (rx_size > SENXX_MAX_MEASURED_RSP_LEN)
> +		return -EINVAL;
> +
> +	PACK_CMD_HEAD(state->chip->read_measured_cmd, &txbuf[0]);

As above, this is a put_unaligned_be16() I think, so use that instead
of custom code.

> +
> +	/* sensor can only be polled once a second max per datasheet */
> +	if (!time_after(jiffies, state->last_update + HZ))
> +		return 0;
> +
> +	ret = i2c_master_send(client, txbuf, sizeof(txbuf));
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ret = sen_common_i2c_recv_reply(client, rxbuf, rx_size);
> +	if (ret < 0)
> +		return ret;
> +
> +	memcpy(&state->data, rxbuf, ret);

Why not just do the recv directly into state->data?

> +
> +	state->last_update = jiffies;
> +
> +	return 0;
> +}
> +
> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct sen_common_state *state = iio_priv(indio_dev);
> +	int status;
> +	int ret;
> +
> +	ret = sen_common_status(state->client, &status);

This is custom ABI. So it would need documentation and will need
to overcome quite a high barrier.

Superficially this looks like debug perhaps that should be
in debugfs?

> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "%x\n", status);
> +}
> +
> +static IIO_DEVICE_ATTR_RO(status, 0);
> +
> +static struct attribute *sen_common_attrs[] = {
> +	&iio_dev_attr_status.dev_attr.attr, NULL
> +};
> +
> +static const struct attribute_group sen_common_attr_group = {
> +	.attrs = sen_common_attrs,
> +};
>
> +static int sen_common_read_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan, int *val,
> +			       int *val2, long mask)
> +{
> +	struct sen_common_state *state = iio_priv(indio_dev);
> +	int ret = IIO_VAL_INT;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&state->lock);

Add scope with {} and use guard(mutex) + include cleanup.h
that means you don't need to manually release the mutex.


> +		ret = sen_common_fetch_measured(state);
> +		if (ret < 0) {
> +			mutex_unlock(&state->lock);
> +			return ret;
> +		}
> +
> +		*val = be16_to_cpu(state->data[chan->address]);
> +
> +		mutex_unlock(&state->lock);
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_MASSCONCENTRATION:
> +			*val = 0;
> +			*val2 = 100000;
> +
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_HUMIDITYRELATIVE:
> +			*val = 0;
> +			*val2 = 10000;
> +
> +			return IIO_VAL_INT_PLUS_MICRO;
As these first two are the same combine them

		case IIO_MASSCONCENTRATION:
		case IIO_HUMIIDTYRELATIVE:
			*val = ..

> +		case IIO_TEMP:
> +			*val = 0;
> +			*val2 = 5000;
> +
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_CONCENTRATION:
> +			switch (chan->channel2) {
> +			case IIO_MOD_NOX:
> +			case IIO_MOD_VOC:
> +				*val = 0;
> +				*val2 = 100000;
> +
> +				return IIO_VAL_INT_PLUS_MICRO;
> +			case IIO_MOD_CO2:
> +				*val = 1;
> +				*val2 = 0;

If the scale is 1, convention is don't provide the attribute / info_mask
element for that channel as it is the default value anyway.

> +
> +				return IIO_VAL_INT_PLUS_MICRO;
> +			default:
> +				return -EINVAL;
> +			}
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:

> +		*val = 1;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}

> +
> +int sen_common_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	const struct sen_chip_info *chip;
> +	const char *name = client->name;
> +	struct sen_common_state *state;
> +	struct iio_dev *indio_dev;
> +	char senxx[8];
> +	int ret = 0;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct sen_common_state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = i2c_get_match_data(client);
> +
> +	state = iio_priv(indio_dev);
> +	state->client = client;
> +	state->dev = dev;
> +	mutex_init(&state->lock);
> +	state->chip = chip;
> +
> +	crc8_populate_msb(sen_common_crc8_table, SENXX_CRC8_POLYNOMIAL);
> +
> +	ret = sen_common_simple_command(client, SENXX_RESET, chip->reset_delay);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sen_common_product_name(client, senxx, sizeof(senxx));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (strncasecmp(senxx, name, sizeof(senxx)) != 0)
> +		dev_warn(dev, "chip mismatch: %s != %s\n", senxx, name);
> +
> +	/* Set default temperature compensation parameters */
> +	if (chip->temperature_compensation_required) {
> +		ret = sen5x_set_temperature_compensation(client, 0, 0, 0);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = sen_common_simple_command(client, SENXX_START_MEASUREMENT, 50);
> +	if (ret < 0)
> +		return ret;
> +
> +	indio_dev->name = name;
> +	indio_dev->info = &sen_common_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = sen_common_channels;
> +	indio_dev->num_channels = chip->num_iio_channels;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      sen_common_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		dev_err(dev, "failed to register iio device\n");
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sen_common_probe);

After changes requested above you will only have one file anyway, but
for future reference please namespace driver specific exports.
EXPORT_SYMBOL_NS_GPL()

> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Sensirion SEN5x/SEN6x common functionality");

Re: [PATCH 3/3] iio: chemical: add support for Sensirion SEN5x/SEN6x

[Hermes Zhang]

Hi Jonathan,

Thanks a lot for your review. I will fix most of them in v2, just one 
question below.

Best Regards,
Hermes


On 2025/2/9 0:15, Jonathan Cameron wrote:
> On Thu, 6 Feb 2025 14:15:17 +0800
> Hermes Zhang <Hermes.Zhang@axis.com> wrote:
>
>
>> +
>> +	state->last_update = jiffies;
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
>> +			   char *buf)
>> +{
>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +	struct sen_common_state *state = iio_priv(indio_dev);
>> +	int status;
>> +	int ret;
>> +
>> +	ret = sen_common_status(state->client, &status);
> This is custom ABI. So it would need documentation and will need
> to overcome quite a high barrier.
>
> Superficially this looks like debug perhaps that should be
> in debugfs?

The status is one of the support commands from the chip, we (from 
userspace) could read it and notify customer if the sensor is wrong or 
not. So it is ued in normal usage, regarding the ABI, I see your point, 
so instead of my way, do you have any suggestion for how to handle it in 
iio? Thanks.

Best Regards,

Hermes

Re: [PATCH 3/3] iio: chemical: add support for Sensirion SEN5x/SEN6x

[Jonathan Cameron]

On Mon, 10 Feb 2025 17:16:57 +0800
Hermes Zhang <chenhuiz@axis.com> wrote:

> Hi Jonathan,
> 
> Thanks a lot for your review. I will fix most of them in v2, just one 
> question below.
> 
> Best Regards,
> Hermes
> 
> 
> On 2025/2/9 0:15, Jonathan Cameron wrote:
> > On Thu, 6 Feb 2025 14:15:17 +0800
> > Hermes Zhang <Hermes.Zhang@axis.com> wrote:
> >
> >  
> >> +
> >> +	state->last_update = jiffies;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> >> +			   char *buf)
> >> +{
> >> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >> +	struct sen_common_state *state = iio_priv(indio_dev);
> >> +	int status;
> >> +	int ret;
> >> +
> >> +	ret = sen_common_status(state->client, &status);  
> > This is custom ABI. So it would need documentation and will need
> > to overcome quite a high barrier.
> >
> > Superficially this looks like debug perhaps that should be
> > in debugfs?  
> 
> The status is one of the support commands from the chip, we (from 
> userspace) could read it and notify customer if the sensor is wrong or 
> not. So it is ued in normal usage, regarding the ABI, I see your point, 
> so instead of my way, do you have any suggestion for how to handle it in 
> iio? Thanks.

What is actually reporting?  I hope something more specific than
'wrong'.  Also when do you read it?  Standard software is never going to
so you may be better of doing some scheduled reading or reading it
on the back of a channel read.  Unfortunately the nature of failure modes
is that they are not easy to describe in a generic ABI - sometimes
our best bet is to just log an error (rate limited).

Some of these look like they mean the data read is garbage when they
are happening?

Jonathan


> 
> Best Regards,
> 
> Hermes
> 
> 

Re: [PATCH 3/3] iio: chemical: add support for Sensirion SEN5x/SEN6x

[Hermes Zhang]

On 2025/2/11 3:09, Jonathan Cameron wrote:
> On Mon, 10 Feb 2025 17:16:57 +0800
> Hermes Zhang <chenhuiz@axis.com> wrote:
>
>> Hi Jonathan,
>>
>> Thanks a lot for your review. I will fix most of them in v2, just one
>> question below.
>>
>> Best Regards,
>> Hermes
>>
>>
>> On 2025/2/9 0:15, Jonathan Cameron wrote:
>>> On Thu, 6 Feb 2025 14:15:17 +0800
>>> Hermes Zhang <Hermes.Zhang@axis.com> wrote:
>>>
>>>   
>>>> +
>>>> +	state->last_update = jiffies;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
>>>> +			   char *buf)
>>>> +{
>>>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>> +	struct sen_common_state *state = iio_priv(indio_dev);
>>>> +	int status;
>>>> +	int ret;
>>>> +
>>>> +	ret = sen_common_status(state->client, &status);
>>> This is custom ABI. So it would need documentation and will need
>>> to overcome quite a high barrier.
>>>
>>> Superficially this looks like debug perhaps that should be
>>> in debugfs?
>> The status is one of the support commands from the chip, we (from
>> userspace) could read it and notify customer if the sensor is wrong or
>> not. So it is ued in normal usage, regarding the ABI, I see your point,
>> so instead of my way, do you have any suggestion for how to handle it in
>> iio? Thanks.
> What is actually reporting?  I hope something more specific than
> 'wrong'.  Also when do you read it?  Standard software is never going to
> so you may be better of doing some scheduled reading or reading it
> on the back of a channel read.  Unfortunately the nature of failure modes
> is that they are not easy to describe in a generic ABI - sometimes
> our best bet is to just log an error (rate limited).
>
> Some of these look like they mean the data read is garbage when they
> are happening?
>
> Jonathan
>
It's e.g. a 32bit data which some of the bits indicate if the 
PM/CO2/GAS/RHT/FAN is error. Yes, the software will scheduled to read it 
and deal the error based on some policy configurable (e.g reboot the 
device). I hope if the driver can provide such inteface to read it, so 
userspace can decide how to handle it. Is there some similar case 
already in driver now? Thanks.


Best Regards,

Hermes

Re: [PATCH 2/3] dt-bindings: iio: chemical: sensirion,senxx: Add yaml description

[Conor Dooley]

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

On Sat, Feb 08, 2025 at 03:07:08PM +0800, Hermes Zhang wrote:
> Hi,
> 
> On 2025/2/7 2:20, Conor Dooley wrote:
> > diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
> > new file mode 100644
> > index 000000000000..4d998eabe441
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
> > filename matching a compatible please.
> So sensirion,sen66.yaml?
> > +  https://sensirion.com/media/documents/6791EFA0/62A1F68F/Sensirion_Datasheet_Environmental_Node_SEN5x.pdf
> > +  https://sensirion.com/media/documents/FAFC548D/6731FFFA/Sensirion_Datasheet_SEN6x.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sensirion,sen50
> > +      - sensirion,sen54
> > +      - sensirion,sen55
> > +      - sensirion,sen60
> > +      - sensirion,sen65
> > +      - sensirion,sen66
> > I'd like a note in the commit message as to how all of these devices are
> > different please.
> Sure, will fix in v2.
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > No supplies needed for this device? Seems like you would need at least
> > one, no?
> 
> You mean the vdd-supply? The chip require a e.g. 3.3v VDD, but in our HW, we
> have no gpio/regulator to control it, connect directly by the HW, should I
> still need to have one vdd-supply here?

Might not be controllable in your case, but if the device needs power
from somewhere it should have one in the binding.

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

Re: [PATCH 3/3] iio: chemical: add support for Sensirion SEN5x/SEN6x

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:29:54 +0800
Hermes Zhang <chenhuiz@axis.com> wrote:

> On 2025/2/11 3:09, Jonathan Cameron wrote:
> > On Mon, 10 Feb 2025 17:16:57 +0800
> > Hermes Zhang <chenhuiz@axis.com> wrote:
> >  
> >> Hi Jonathan,
> >>
> >> Thanks a lot for your review. I will fix most of them in v2, just one
> >> question below.
> >>
> >> Best Regards,
> >> Hermes
> >>
> >>
> >> On 2025/2/9 0:15, Jonathan Cameron wrote:  
> >>> On Thu, 6 Feb 2025 14:15:17 +0800
> >>> Hermes Zhang <Hermes.Zhang@axis.com> wrote:
> >>>
> >>>     
> >>>> +
> >>>> +	state->last_update = jiffies;
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> >>>> +			   char *buf)
> >>>> +{
> >>>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >>>> +	struct sen_common_state *state = iio_priv(indio_dev);
> >>>> +	int status;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = sen_common_status(state->client, &status);  
> >>> This is custom ABI. So it would need documentation and will need
> >>> to overcome quite a high barrier.
> >>>
> >>> Superficially this looks like debug perhaps that should be
> >>> in debugfs?  
> >> The status is one of the support commands from the chip, we (from
> >> userspace) could read it and notify customer if the sensor is wrong or
> >> not. So it is ued in normal usage, regarding the ABI, I see your point,
> >> so instead of my way, do you have any suggestion for how to handle it in
> >> iio? Thanks.  
> > What is actually reporting?  I hope something more specific than
> > 'wrong'.  Also when do you read it?  Standard software is never going to
> > so you may be better of doing some scheduled reading or reading it
> > on the back of a channel read.  Unfortunately the nature of failure modes
> > is that they are not easy to describe in a generic ABI - sometimes
> > our best bet is to just log an error (rate limited).
> >
> > Some of these look like they mean the data read is garbage when they
> > are happening?
> >
> > Jonathan
> >  
> It's e.g. a 32bit data which some of the bits indicate if the 
> PM/CO2/GAS/RHT/FAN is error. Yes, the software will scheduled to read it 
> and deal the error based on some policy configurable (e.g reboot the 
> device). I hope if the driver can provide such inteface to read it, so 
> userspace can decide how to handle it. Is there some similar case 
> already in driver now? Thanks.

There are some but they aren't typically left to userspace because
that would require custom userspace for every driver (which is typically
not possible).  Hence checks like this are usually done by the driver
directly (often the device has an optimized read of data + status) and
we report a simple error to userspace + restart device etc.

Typical cases of this are things like fifo overflow which sometimes needs
a device reset to recover.

So can you do this check in the driver on each read? It's a really slow
sensor so the extra bus access time isn't going to make much difference
and then you can take correct action.

Jonathan


> 
> 
> Best Regards,
> 
> Hermes
> 

Re: [PATCH 2/3] dt-bindings: iio: chemical: sensirion,senxx: Add yaml description

[Hermes Zhang]

On 2025/2/12 2:44, Conor Dooley wrote:
> On Sat, Feb 08, 2025 at 03:07:08PM +0800, Hermes Zhang wrote:
>> Hi,
>>
>> On 2025/2/7 2:20, Conor Dooley wrote:
>>> diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
>>> new file mode 100644
>>> index 000000000000..4d998eabe441
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,senxx.yaml
>>> filename matching a compatible please.
>> So sensirion,sen66.yaml?
>>> +  https://sensirion.com/media/documents/6791EFA0/62A1F68F/Sensirion_Datasheet_Environmental_Node_SEN5x.pdf
>>> +  https://sensirion.com/media/documents/FAFC548D/6731FFFA/Sensirion_Datasheet_SEN6x.pdf
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - sensirion,sen50
>>> +      - sensirion,sen54
>>> +      - sensirion,sen55
>>> +      - sensirion,sen60
>>> +      - sensirion,sen65
>>> +      - sensirion,sen66
>>> I'd like a note in the commit message as to how all of these devices are
>>> different please.
>> Sure, will fix in v2.
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>> No supplies needed for this device? Seems like you would need at least
>>> one, no?
>> You mean the vdd-supply? The chip require a e.g. 3.3v VDD, but in our HW, we
>> have no gpio/regulator to control it, connect directly by the HW, should I
>> still need to have one vdd-supply here?
> Might not be controllable in your case, but if the device needs power
> from somewhere it should have one in the binding.

OK, I see. I will add it in v2. Thanks.


Best Regards,

Hermes