[PATCH 0/6] Add driver for ENS160 sensor

[PATCH 0/6] Add driver for ENS160 sensor

[Gustavo Silva]

This series of patches adds a driver for ScioSense ENS160 multi-gas
sensor, designed for indoor air quality monitoring.

Gustavo Silva (6):
  dt-bindings: vendor-prefixes: add ScioSense
  dt-bindings: Add ENS160 as trivial device
  iio: chemical: add driver for ENS160 sensor
  iio: chemical: ens160: add triggered buffer support
  iio: chemical: ens160: add power management support
  MAINTAINERS: Add ScioSense ENS160

 .../devicetree/bindings/trivial-devices.yaml  |   2 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   8 +
 drivers/iio/chemical/Kconfig                  |  22 +
 drivers/iio/chemical/Makefile                 |   3 +
 drivers/iio/chemical/ens160.h                 |  11 +
 drivers/iio/chemical/ens160_core.c            | 401 ++++++++++++++++++
 drivers/iio/chemical/ens160_i2c.c             |  69 +++
 drivers/iio/chemical/ens160_spi.c             |  70 +++
 9 files changed, 588 insertions(+)
 create mode 100644 drivers/iio/chemical/ens160.h
 create mode 100644 drivers/iio/chemical/ens160_core.c
 create mode 100644 drivers/iio/chemical/ens160_i2c.c
 create mode 100644 drivers/iio/chemical/ens160_spi.c


base-commit: 084eeee1d8da6b4712719264b01cb27b41307f54
-- 
2.45.0

[PATCH 1/6] dt-bindings: vendor-prefixes: add ScioSense

[Gustavo Silva]

Add vendor prefix for ScioSense B.V.
https://www.sciosense.com/

Signed-off-by: Gustavo Silva <gustavograzs@gmail.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 b97d298b3..298f13a0d 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1246,6 +1246,8 @@ patternProperties:
     description: Smart Battery System
   "^schindler,.*":
     description: Schindler
+  "^sciosense,.*":
+    description: ScioSense B.V.
   "^seagate,.*":
     description: Seagate Technology PLC
   "^seeed,.*":
-- 
2.45.0

[PATCH 2/6] dt-bindings: Add ENS160 as trivial device

[Gustavo Silva]

ScioSense ENS160 is a multi-gas sensor.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index e07be7bf8..cdd7f0b46 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -318,6 +318,8 @@ properties:
           - samsung,24ad0xd1
             # Samsung Exynos SoC SATA PHY I2C device
           - samsung,exynos-sataphy-i2c
+            # ScioSense ENS160 multi-gas sensor
+          - sciosense,ens160
             # Semtech sx1301 baseband processor
           - semtech,sx1301
             # Sensirion multi-pixel gas sensor with I2C interface
-- 
2.45.0

[PATCH 3/6] iio: chemical: add driver for ENS160 sensor

[Gustavo Silva]

ScioSense ENS160 is a digital metal oxide multi-gas sensor, designed
for indoor air quality monitoring. The driver supports readings of
CO2 and VOC, and can be accessed via both SPI and I2C.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 drivers/iio/chemical/Kconfig       |  22 +++
 drivers/iio/chemical/Makefile      |   3 +
 drivers/iio/chemical/ens160.h      |   9 ++
 drivers/iio/chemical/ens160_core.c | 227 +++++++++++++++++++++++++++++
 drivers/iio/chemical/ens160_i2c.c  |  68 +++++++++
 drivers/iio/chemical/ens160_spi.c  |  69 +++++++++
 6 files changed, 398 insertions(+)
 create mode 100644 drivers/iio/chemical/ens160.h
 create mode 100644 drivers/iio/chemical/ens160_core.c
 create mode 100644 drivers/iio/chemical/ens160_i2c.c
 create mode 100644 drivers/iio/chemical/ens160_spi.c

diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 02649ab81..e407afab8 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -76,6 +76,28 @@ config CCS811
 	  Say Y here to build I2C interface support for the AMS
 	  CCS811 VOC (Volatile Organic Compounds) sensor
 
+config ENS160
+	tristate "ScioSense ENS160 sensor driver"
+	depends on (I2C || SPI)
+	select REGMAP
+	select ENS160_I2C if I2C
+	select ENS160_SPI if SPI
+	help
+	  Say yes here to build support for ScioSense ENS160 multi-gas sensor.
+
+	  This driver can also be built as a module. If so, the module for I2C
+	  would be called ens160_i2c and ens160_spi for SPI support.
+
+config ENS160_I2C
+	tristate
+	depends on I2C && ENS160
+	select REGMAP_I2C
+
+config ENS160_SPI
+	tristate
+	depends on SPI && ENS160
+	select REGMAP_SPI
+
 config IAQCORE
 	tristate "AMS iAQ-Core VOC sensors"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index 2f3dee8bb..4866db06b 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -11,6 +11,9 @@ obj-$(CONFIG_BME680) += bme680_core.o
 obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
 obj-$(CONFIG_BME680_SPI) += bme680_spi.o
 obj-$(CONFIG_CCS811)		+= ccs811.o
+obj-$(CONFIG_ENS160) += ens160_core.o
+obj-$(CONFIG_ENS160_I2C) += ens160_i2c.o
+obj-$(CONFIG_ENS160_SPI) += ens160_spi.o
 obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
 obj-$(CONFIG_PMS7003) += pms7003.o
 obj-$(CONFIG_SCD30_CORE) += scd30_core.o
diff --git a/drivers/iio/chemical/ens160.h b/drivers/iio/chemical/ens160.h
new file mode 100644
index 000000000..3fd079bc4
--- /dev/null
+++ b/drivers/iio/chemical/ens160.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef ENS160_H_
+#define ENS160_H_
+
+int ens160_core_probe(struct device *dev, struct regmap *regmap,
+		      const char *name);
+void ens160_core_remove(struct device *dev);
+
+#endif
diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c
new file mode 100644
index 000000000..25593420d
--- /dev/null
+++ b/drivers/iio/chemical/ens160_core.c
@@ -0,0 +1,227 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ScioSense ENS160 multi-gas sensor driver
+ *
+ * Copyright (c) 2024 Gustavo Silva <gustavograzs@gmail.com>
+ *
+ * Data sheet:
+ *  https://www.sciosense.com/wp-content/uploads/2023/12/ENS160-Datasheet.pdf
+ *
+ */
+
+#include <linux/bitfield.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "ens160.h"
+
+#define ENS160_PART_ID 0x160
+
+#define ENS160_BOOTING_TIME_MS 10U
+
+#define ENS160_REG_PART_ID	0x00
+
+#define ENS160_REG_OPMODE	0x10
+
+#define ENS160_REG_MODE_DEEP_SLEEP	0x00
+#define ENS160_REG_MODE_IDLE		0x01
+#define ENS160_REG_MODE_STANDARD	0x02
+#define ENS160_REG_MODE_RESET		0xF0
+
+#define ENS160_REG_COMMAND		0x12
+#define ENS160_REG_COMMAND_GET_APPVER	0x0E
+#define ENS160_REG_COMMAND_CLRGPR	0xCC
+
+#define ENS160_REG_TEMP_IN		0x13
+#define ENS160_REG_RH_IN		0x15
+#define ENS160_REG_DEVICE_STATUS	0x20
+#define ENS160_REG_DATA_TVOC		0x22
+#define ENS160_REG_DATA_ECO2		0x24
+#define ENS160_REG_DATA_T		0x30
+#define ENS160_REG_DATA_RH		0x32
+#define ENS160_REG_GPR_READ4		0x4C
+
+#define ENS160_STATUS_VALIDITY_FLAG	GENMASK(3, 2)
+
+#define ENS160_STATUS_NORMAL	0x00
+
+struct ens160_data {
+	struct regmap *regmap;
+};
+
+static const struct iio_chan_spec ens160_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_VOC,
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.address = ENS160_REG_DATA_TVOC,
+	},
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_CO2,
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.address = ENS160_REG_DATA_ECO2,
+	},
+};
+
+static int ens160_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct ens160_data *data = iio_priv(indio_dev);
+	__le16 buf;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_bulk_read(data->regmap, chan->address,
+					&buf, sizeof(buf));
+		if (ret)
+			return ret;
+		*val = le16_to_cpu(buf);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->channel2) {
+		case IIO_MOD_CO2:
+			/* The sensor reads CO2 data as ppm */
+			*val = 0;
+			*val2 = 100;
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_MOD_VOC:
+			/* The sensor reads VOC data as ppb */
+			*val = 0;
+			*val2 = 100;
+			return IIO_VAL_INT_PLUS_NANO;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int ens160_set_mode(struct ens160_data *data, u8 mode)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, ENS160_REG_OPMODE, mode);
+	if (ret)
+		return ret;
+
+	msleep(ENS160_BOOTING_TIME_MS);
+
+	return 0;
+}
+
+static int ens160_chip_init(struct ens160_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	u8 fw_version[3];
+	__le16 part_id;
+	unsigned int status;
+	int ret;
+
+	ret = ens160_set_mode(data, ENS160_REG_MODE_RESET);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap, ENS160_REG_PART_ID, &part_id,
+			       sizeof(part_id));
+	if (ret)
+		return ret;
+
+	if (le16_to_cpu(part_id) != ENS160_PART_ID)
+		return -ENODEV;
+
+	ret = ens160_set_mode(data, ENS160_REG_MODE_IDLE);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
+			   ENS160_REG_COMMAND_CLRGPR);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
+			   ENS160_REG_COMMAND_GET_APPVER);
+	if (ret)
+		return ret;
+
+	msleep(ENS160_BOOTING_TIME_MS);
+
+	ret = regmap_bulk_read(data->regmap, ENS160_REG_GPR_READ4,
+			       fw_version, sizeof(fw_version));
+	if (ret)
+		return ret;
+
+	msleep(ENS160_BOOTING_TIME_MS);
+
+	dev_info(dev, "firmware version: %u.%u.%u\n", fw_version[2],
+		 fw_version[1], fw_version[0]);
+
+	ret = ens160_set_mode(data, ENS160_REG_MODE_STANDARD);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, ENS160_REG_DEVICE_STATUS, &status);
+	if (ret)
+		return ret;
+
+	if (FIELD_GET(ENS160_STATUS_VALIDITY_FLAG, status)
+	    != ENS160_STATUS_NORMAL)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct iio_info ens160_info = {
+	.read_raw = ens160_read_raw,
+};
+
+int ens160_core_probe(struct device *dev, struct regmap *regmap,
+		      const char *name)
+{
+	struct ens160_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	dev_set_drvdata(dev, indio_dev);
+	data->regmap = regmap;
+
+	indio_dev->name = name;
+	indio_dev->info = &ens160_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ens160_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
+
+	ret = ens160_chip_init(data);
+	if (ret) {
+		dev_err_probe(dev, ret, "chip initialization failed\n");
+		return ret;
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160);
+
+void ens160_core_remove(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ens160_data *data = iio_priv(indio_dev);
+
+	ens160_set_mode(data, ENS160_REG_MODE_IDLE);
+}
+EXPORT_SYMBOL_NS(ens160_core_remove, IIO_ENS160);
+
+MODULE_AUTHOR("Gustavo Silva <gustavograzs@gmail.com>");
+MODULE_DESCRIPTION("ScioSense ENS160 driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/chemical/ens160_i2c.c b/drivers/iio/chemical/ens160_i2c.c
new file mode 100644
index 000000000..ee2b44184
--- /dev/null
+++ b/drivers/iio/chemical/ens160_i2c.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ScioSense ENS160 multi-gas sensor I2C driver
+ *
+ * Copyright (c) 2024 Gustavo Silva <gustavograzs@gmail.com>
+ *
+ * 7-Bit I2C slave address is:
+ *	- 0x52 if ADDR pin LOW
+ *	- 0x53 if ADDR pin HIGH
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "ens160.h"
+
+static const struct regmap_config ens160_regmap_i2c_conf = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int ens160_i2c_probe(struct i2c_client *client)
+{
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i2c(client, &ens160_regmap_i2c_conf);
+	if (IS_ERR(regmap)) {
+		dev_err(&client->dev, "Failed to register i2c regmap %ld\n",
+			PTR_ERR(regmap));
+		return PTR_ERR(regmap);
+	}
+
+	return ens160_core_probe(&client->dev, regmap, client->name);
+}
+
+static void ens160_i2c_remove(struct i2c_client *client)
+{
+	ens160_core_remove(&client->dev);
+}
+
+static const struct i2c_device_id ens160_i2c_id[] = {
+	{ "ens160", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ens160_i2c_id);
+
+static const struct of_device_id ens160_of_i2c_match[] = {
+	{ .compatible = "sciosense,ens160" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ens160_of_i2c_match);
+
+static struct i2c_driver ens160_i2c_driver = {
+	.driver = {
+		.name		= "ens160_i2c",
+		.of_match_table	= ens160_of_i2c_match,
+	},
+	.probe = ens160_i2c_probe,
+	.remove = ens160_i2c_remove,
+	.id_table = ens160_i2c_id,
+};
+module_i2c_driver(ens160_i2c_driver);
+
+MODULE_AUTHOR("Gustavo Silva <gustavograzs@gmail.com>");
+MODULE_DESCRIPTION("ScioSense ENS160 I2C driver");
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(IIO_ENS160);
diff --git a/drivers/iio/chemical/ens160_spi.c b/drivers/iio/chemical/ens160_spi.c
new file mode 100644
index 000000000..effc4acee
--- /dev/null
+++ b/drivers/iio/chemical/ens160_spi.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ScioSense ENS160 multi-gas sensor SPI driver
+ *
+ * Copyright (c) 2024 Gustavo Silva <gustavograzs@gmail.com>
+ */
+
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "ens160.h"
+
+#define ENS160_SPI_READ BIT(0)
+
+static const struct regmap_config ens160_regmap_spi_conf = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_shift = -1,
+	.read_flag_mask = ENS160_SPI_READ,
+};
+
+static int ens160_spi_probe(struct spi_device *spi)
+{
+	struct regmap *regmap;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+
+	regmap = devm_regmap_init_spi(spi, &ens160_regmap_spi_conf);
+	if (IS_ERR(regmap)) {
+		dev_err(&spi->dev, "Failed to register spi regmap: %pe\n",
+			regmap);
+		return PTR_ERR(regmap);
+	}
+
+	return ens160_core_probe(&spi->dev, regmap, id->name);
+}
+
+static void ens160_spi_remove(struct spi_device *spi)
+{
+	ens160_core_remove(&spi->dev);
+}
+
+static const struct of_device_id ens160_spi_of_match[] = {
+	{ .compatible = "sciosense,ens160" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ens160_spi_of_match);
+
+static const struct spi_device_id ens160_spi_id[] = {
+	{ "ens160", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ens160_spi_id);
+
+static struct spi_driver ens160_spi_driver = {
+	.driver = {
+		.name	= "ens160_spi",
+		.of_match_table = ens160_spi_of_match,
+	},
+	.probe		= ens160_spi_probe,
+	.remove		= ens160_spi_remove,
+	.id_table	= ens160_spi_id,
+};
+module_spi_driver(ens160_spi_driver);
+
+MODULE_AUTHOR("Gustavo Silva <gustavograzs@gmail.com>");
+MODULE_DESCRIPTION("ScioSense ENS160 SPI driver");
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(IIO_ENS160);
-- 
2.45.0

[PATCH 4/6] iio: chemical: ens160: add triggered buffer support

[Gustavo Silva]

ENS160 supports a data ready interrupt. Use it in combination with
triggered buffer for continuous data readings.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 drivers/iio/chemical/ens160.h      |   2 +-
 drivers/iio/chemical/ens160_core.c | 155 ++++++++++++++++++++++++++++-
 drivers/iio/chemical/ens160_i2c.c  |   2 +-
 drivers/iio/chemical/ens160_spi.c  |   2 +-
 4 files changed, 156 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/chemical/ens160.h b/drivers/iio/chemical/ens160.h
index 3fd079bc4..a8a2f1263 100644
--- a/drivers/iio/chemical/ens160.h
+++ b/drivers/iio/chemical/ens160.h
@@ -2,7 +2,7 @@
 #ifndef ENS160_H_
 #define ENS160_H_
 
-int ens160_core_probe(struct device *dev, struct regmap *regmap,
+int ens160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		      const char *name);
 void ens160_core_remove(struct device *dev);
 
diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c
index 25593420d..4b960ef00 100644
--- a/drivers/iio/chemical/ens160_core.c
+++ b/drivers/iio/chemical/ens160_core.c
@@ -11,6 +11,9 @@
 
 #include <linux/bitfield.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 
@@ -24,6 +27,11 @@
 
 #define ENS160_REG_OPMODE	0x10
 
+#define ENS160_REG_CONFIG		0x11
+#define ENS160_REG_CONFIG_INTEN		BIT(0)
+#define ENS160_REG_CONFIG_INTDAT	BIT(1)
+#define ENS160_REG_CONFIG_INT_CFG	BIT(5)
+
 #define ENS160_REG_MODE_DEEP_SLEEP	0x00
 #define ENS160_REG_MODE_IDLE		0x01
 #define ENS160_REG_MODE_STANDARD	0x02
@@ -48,6 +56,12 @@
 
 struct ens160_data {
 	struct regmap *regmap;
+	struct mutex mutex;
+	struct {
+		u16 chans[2];
+		s64 timestamp __aligned(8);
+	} scan;
+	int irq;
 };
 
 static const struct iio_chan_spec ens160_channels[] = {
@@ -58,6 +72,13 @@ static const struct iio_chan_spec ens160_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE),
 		.address = ENS160_REG_DATA_TVOC,
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
 	},
 	{
 		.type = IIO_CONCENTRATION,
@@ -66,7 +87,15 @@ static const struct iio_chan_spec ens160_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE),
 		.address = ENS160_REG_DATA_ECO2,
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
 };
 
 static int ens160_read_raw(struct iio_dev *indio_dev,
@@ -79,10 +108,19 @@ static int ens160_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+		mutex_lock(&data->mutex);
 		ret = regmap_bulk_read(data->regmap, chan->address,
 					&buf, sizeof(buf));
-		if (ret)
+		if (ret) {
+			mutex_unlock(&data->mutex);
+			iio_device_release_direct_mode(indio_dev);
 			return ret;
+		}
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(indio_dev);
 		*val = le16_to_cpu(buf);
 		return IIO_VAL_INT;
 
@@ -182,7 +220,104 @@ static const struct iio_info ens160_info = {
 	.read_raw = ens160_read_raw,
 };
 
-int ens160_core_probe(struct device *dev, struct regmap *regmap,
+static irqreturn_t ens160_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+
+	if (iio_buffer_enabled(indio_dev))
+		iio_trigger_poll(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t ens160_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ens160_data *data = iio_priv(indio_dev);
+	__le16 val;
+	int ret, i, j = 0;
+
+	mutex_lock(&data->mutex);
+
+	for_each_set_bit(i, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		ret = regmap_bulk_read(data->regmap,
+				       ENS160_REG_DATA_TVOC + 2 * i, &val, 2U);
+		if (ret)
+			goto err;
+
+		data->scan.chans[j++] = val;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+					   pf->timestamp);
+err:
+	mutex_unlock(&data->mutex);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int ens160_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct ens160_data *data = iio_priv(indio_dev);
+	unsigned int int_bits = ENS160_REG_CONFIG_INTEN |
+				ENS160_REG_CONFIG_INTDAT |
+				ENS160_REG_CONFIG_INT_CFG;
+	int ret;
+
+	if (state)
+		ret = regmap_set_bits(data->regmap, ENS160_REG_CONFIG,
+				      int_bits);
+	else
+		ret = regmap_clear_bits(data->regmap, ENS160_REG_CONFIG,
+					int_bits);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops ens160_trigger_ops = {
+	.set_trigger_state = ens160_set_trigger_state,
+	.validate_device = iio_trigger_validate_own_device,
+};
+
+static int ens160_setup_trigger(struct iio_dev *indio_dev)
+{
+	struct ens160_data *data = iio_priv(indio_dev);
+	struct device *dev = indio_dev->dev.parent;
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
+				      iio_device_id(indio_dev));
+	if (!trig)
+		return dev_err_probe(dev, -ENOMEM,
+				     "failed to allocate trigger\n");
+
+	trig->ops = &ens160_trigger_ops;
+	iio_trigger_set_drvdata(trig, indio_dev);
+
+	ret = devm_iio_trigger_register(dev, trig);
+	if (ret)
+		return ret;
+
+	indio_dev->trig = iio_trigger_get(trig);
+
+	ret = devm_request_threaded_irq(dev, data->irq,
+					ens160_irq_handler,
+					NULL,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					indio_dev->name,
+					indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to request irq\n");
+
+	return 0;
+}
+
+int ens160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		      const char *name)
 {
 	struct ens160_data *data;
@@ -196,6 +331,7 @@ int ens160_core_probe(struct device *dev, struct regmap *regmap,
 	data = iio_priv(indio_dev);
 	dev_set_drvdata(dev, indio_dev);
 	data->regmap = regmap;
+	data->irq = irq;
 
 	indio_dev->name = name;
 	indio_dev->info = &ens160_info;
@@ -203,12 +339,27 @@ int ens160_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->channels = ens160_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
 
+	if (data->irq > 0) {
+		ret = ens160_setup_trigger(indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to setup trigger\n");
+	}
+
 	ret = ens160_chip_init(data);
 	if (ret) {
 		dev_err_probe(dev, ret, "chip initialization failed\n");
 		return ret;
 	}
 
+	mutex_init(&data->mutex);
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ens160_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160);
diff --git a/drivers/iio/chemical/ens160_i2c.c b/drivers/iio/chemical/ens160_i2c.c
index ee2b44184..28d4988c0 100644
--- a/drivers/iio/chemical/ens160_i2c.c
+++ b/drivers/iio/chemical/ens160_i2c.c
@@ -31,7 +31,7 @@ static int ens160_i2c_probe(struct i2c_client *client)
 		return PTR_ERR(regmap);
 	}
 
-	return ens160_core_probe(&client->dev, regmap, client->name);
+	return ens160_core_probe(&client->dev, regmap, client->irq, client->name);
 }
 
 static void ens160_i2c_remove(struct i2c_client *client)
diff --git a/drivers/iio/chemical/ens160_spi.c b/drivers/iio/chemical/ens160_spi.c
index effc4acee..568b9761d 100644
--- a/drivers/iio/chemical/ens160_spi.c
+++ b/drivers/iio/chemical/ens160_spi.c
@@ -32,7 +32,7 @@ static int ens160_spi_probe(struct spi_device *spi)
 		return PTR_ERR(regmap);
 	}
 
-	return ens160_core_probe(&spi->dev, regmap, id->name);
+	return ens160_core_probe(&spi->dev, regmap, spi->irq, id->name);
 }
 
 static void ens160_spi_remove(struct spi_device *spi)
-- 
2.45.0

[PATCH 5/6] iio: chemical: ens160: add power management support

[Gustavo Silva]

ENS160 supports a deep sleep mode for minimal power consumption.
Use it to add PM sleep capability to the driver.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 drivers/iio/chemical/ens160.h      |  2 ++
 drivers/iio/chemical/ens160_core.c | 23 +++++++++++++++++++++++
 drivers/iio/chemical/ens160_i2c.c  |  1 +
 drivers/iio/chemical/ens160_spi.c  |  1 +
 4 files changed, 27 insertions(+)

diff --git a/drivers/iio/chemical/ens160.h b/drivers/iio/chemical/ens160.h
index a8a2f1263..9ed532615 100644
--- a/drivers/iio/chemical/ens160.h
+++ b/drivers/iio/chemical/ens160.h
@@ -6,4 +6,6 @@ int ens160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		      const char *name);
 void ens160_core_remove(struct device *dev);
 
+extern const struct dev_pm_ops ens160_pm_ops;
+
 #endif
diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c
index 4b960ef00..a444034a4 100644
--- a/drivers/iio/chemical/ens160_core.c
+++ b/drivers/iio/chemical/ens160_core.c
@@ -220,6 +220,29 @@ static const struct iio_info ens160_info = {
 	.read_raw = ens160_read_raw,
 };
 
+static int ens160_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ens160_data *data = iio_priv(indio_dev);
+
+	return ens160_set_mode(data, ENS160_REG_MODE_DEEP_SLEEP);
+}
+
+static int ens160_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ens160_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = ens160_set_mode(data, ENS160_REG_MODE_IDLE);
+	if (ret)
+		return ret;
+
+	return ens160_set_mode(data, ENS160_REG_MODE_STANDARD);
+}
+EXPORT_NS_SIMPLE_DEV_PM_OPS(ens160_pm_ops, ens160_suspend, ens160_resume,
+			    IIO_ENS160);
+
 static irqreturn_t ens160_irq_handler(int irq, void *private)
 {
 	struct iio_dev *indio_dev = private;
diff --git a/drivers/iio/chemical/ens160_i2c.c b/drivers/iio/chemical/ens160_i2c.c
index 28d4988c0..b8785d199 100644
--- a/drivers/iio/chemical/ens160_i2c.c
+++ b/drivers/iio/chemical/ens160_i2c.c
@@ -55,6 +55,7 @@ static struct i2c_driver ens160_i2c_driver = {
 	.driver = {
 		.name		= "ens160_i2c",
 		.of_match_table	= ens160_of_i2c_match,
+		.pm		= pm_sleep_ptr(&ens160_pm_ops),
 	},
 	.probe = ens160_i2c_probe,
 	.remove = ens160_i2c_remove,
diff --git a/drivers/iio/chemical/ens160_spi.c b/drivers/iio/chemical/ens160_spi.c
index 568b9761d..2cf494032 100644
--- a/drivers/iio/chemical/ens160_spi.c
+++ b/drivers/iio/chemical/ens160_spi.c
@@ -56,6 +56,7 @@ static struct spi_driver ens160_spi_driver = {
 	.driver = {
 		.name	= "ens160_spi",
 		.of_match_table = ens160_spi_of_match,
+		.pm = pm_sleep_ptr(&ens160_pm_ops),
 	},
 	.probe		= ens160_spi_probe,
 	.remove		= ens160_spi_remove,
-- 
2.45.0

[PATCH 6/6] MAINTAINERS: Add ScioSense ENS160

[Gustavo Silva]

Add myself as maintainer for ScioSense ENS160 multi-gas sensor driver.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 304429f9b..92a130c8c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19660,6 +19660,14 @@ F:	include/linux/wait.h
 F:	include/uapi/linux/sched.h
 F:	kernel/sched/
 
+SCIOSENSE ENS160 MULTI-GAS SENSOR DRIVER
+M:	Gustavo Silva <gustavograzs@gmail.com>
+S:	Maintained
+F:	drivers/iio/chemical/ens160_core.c
+F:	drivers/iio/chemical/ens160_i2c.c
+F:	drivers/iio/chemical/ens160_spi.c
+F:	drivers/iio/chemical/ens160.h
+
 SCSI LIBSAS SUBSYSTEM
 R:	John Garry <john.g.garry@oracle.com>
 R:	Jason Yan <yanaijie@huawei.com>
-- 
2.45.0

Re: [PATCH 1/6] dt-bindings: vendor-prefixes: add ScioSense

[Conor Dooley]

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

On Sun, May 12, 2024 at 06:04:37PM -0300, Gustavo Silva wrote:
> Add vendor prefix for ScioSense B.V.
> https://www.sciosense.com/
> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH 2/6] dt-bindings: Add ENS160 as trivial device

[Conor Dooley]

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

On Sun, May 12, 2024 at 06:04:38PM -0300, Gustavo Silva wrote:
> ScioSense ENS160 is a multi-gas sensor.
> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>

Looks like this device has two supplies, Vdd and Vddio.
Jonathan generally likes supplies to be documented, so that would
disqualify this as a trivial device.

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index e07be7bf8..cdd7f0b46 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -318,6 +318,8 @@ properties:
>            - samsung,24ad0xd1
>              # Samsung Exynos SoC SATA PHY I2C device
>            - samsung,exynos-sataphy-i2c
> +            # ScioSense ENS160 multi-gas sensor
> +          - sciosense,ens160
>              # Semtech sx1301 baseband processor
>            - semtech,sx1301
>              # Sensirion multi-pixel gas sensor with I2C interface
> -- 
> 2.45.0
> 

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

Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor

[Christophe JAILLET]

Le 12/05/2024 à 23:04, Gustavo Silva a écrit :
> ScioSense ENS160 is a digital metal oxide multi-gas sensor, designed
> for indoor air quality monitoring. The driver supports readings of
> CO2 and VOC, and can be accessed via both SPI and I2C.
> 
> Signed-off-by: Gustavo Silva <gustavograzs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---

Hi,
a few comments below, for what it worth.

BTW, why I'm in copy of the mail?
I'm not a maintainer, and not active on drivers/iio/chemical/
Slightly proud, but curious as well.

...

> +#define ENS160_REG_TEMP_IN		0x13
> +#define ENS160_REG_RH_IN		0x15
> +#define ENS160_REG_DEVICE_STATUS	0x20

If defining everything, maybe:
#define ENS160_REG_DATA_AQI	0x21

> +#define ENS160_REG_DATA_TVOC		0x22
> +#define ENS160_REG_DATA_ECO2		0x24
> +#define ENS160_REG_DATA_T		0x30
> +#define ENS160_REG_DATA_RH		0x32
> +#define ENS160_REG_GPR_READ4		0x4C

...

> +static int ens160_chip_init(struct ens160_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	u8 fw_version[3];
> +	__le16 part_id;
> +	unsigned int status;
> +	int ret;
> +
> +	ret = ens160_set_mode(data, ENS160_REG_MODE_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_read(data->regmap, ENS160_REG_PART_ID, &part_id,
> +			       sizeof(part_id));
> +	if (ret)
> +		return ret;
> +
> +	if (le16_to_cpu(part_id) != ENS160_PART_ID)
> +		return -ENODEV;
> +
> +	ret = ens160_set_mode(data, ENS160_REG_MODE_IDLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> +			   ENS160_REG_COMMAND_CLRGPR);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> +			   ENS160_REG_COMMAND_GET_APPVER);
> +	if (ret)
> +		return ret;
> +
> +	msleep(ENS160_BOOTING_TIME_MS);
> +
> +	ret = regmap_bulk_read(data->regmap, ENS160_REG_GPR_READ4,
> +			       fw_version, sizeof(fw_version));
> +	if (ret)
> +		return ret;
> +
> +	msleep(ENS160_BOOTING_TIME_MS);
> +
> +	dev_info(dev, "firmware version: %u.%u.%u\n", fw_version[2],
> +		 fw_version[1], fw_version[0]);
> +
> +	ret = ens160_set_mode(data, ENS160_REG_MODE_STANDARD);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, ENS160_REG_DEVICE_STATUS, &status);
> +	if (ret)
> +		return ret;
> +
> +	if (FIELD_GET(ENS160_STATUS_VALIDITY_FLAG, status)
> +	    != ENS160_STATUS_NORMAL)
> +		return -EINVAL;

Just wondering how it works with the Warm-up and initial Start-up times.
If the probe is executed and the corresponding duration has not elpased, 
then the probe fails.

Is it what is expected?

> +
> +	return 0;
> +}
> +
> +static const struct iio_info ens160_info = {
> +	.read_raw = ens160_read_raw,
> +};
> +
> +int ens160_core_probe(struct device *dev, struct regmap *regmap,
> +		      const char *name)
> +{
> +	struct ens160_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	dev_set_drvdata(dev, indio_dev);
> +	data->regmap = regmap;
> +
> +	indio_dev->name = name;
> +	indio_dev->info = &ens160_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ens160_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
> +
> +	ret = ens160_chip_init(data);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "chip initialization failed\n");

Nitpick: return dev_err_probe()

> +		return ret;
> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

...

> +static int ens160_i2c_probe(struct i2c_client *client)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i2c(client, &ens160_regmap_i2c_conf);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "Failed to register i2c regmap %ld\n",
> +			PTR_ERR(regmap));

Nitpick: dev_err_probe()

> +		return PTR_ERR(regmap);
> +	}

...

> +static int ens160_spi_probe(struct spi_device *spi)
> +{
> +	struct regmap *regmap;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	regmap = devm_regmap_init_spi(spi, &ens160_regmap_spi_conf);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&spi->dev, "Failed to register spi regmap: %pe\n",
> +			regmap);

Nitpick: dev_err_probe()

CJ

> +		return PTR_ERR(regmap);
> +	}
> +
> +	return ens160_core_probe(&spi->dev, regmap, id->name);
> +}

Re: [PATCH 4/6] iio: chemical: ens160: add triggered buffer support

[Christophe JAILLET]

Le 12/05/2024 à 23:04, Gustavo Silva a écrit :
> ENS160 supports a data ready interrupt. Use it in combination with
> triggered buffer for continuous data readings.
> 
> Signed-off-by: Gustavo Silva <gustavograzs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---

...

> +static irqreturn_t ens160_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ens160_data *data = iio_priv(indio_dev);
> +	__le16 val;
> +	int ret, i, j = 0;
> +
> +	mutex_lock(&data->mutex);
> +
> +	for_each_set_bit(i, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		ret = regmap_bulk_read(data->regmap,
> +				       ENS160_REG_DATA_TVOC + 2 * i, &val, 2U);
> +		if (ret)
> +			goto err;
> +
> +		data->scan.chans[j++] = val;

Is it safe? How can we know if it has been only *partly* updated? Does 
it matter to know?

CJ

> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +					   pf->timestamp);
> +err:
> +	mutex_unlock(&data->mutex);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

...

Re: [PATCH 4/6] iio: chemical: ens160: add triggered buffer support

[kernel test robot]

Hi Gustavo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 084eeee1d8da6b4712719264b01cb27b41307f54]

url:    https://github.com/intel-lab-lkp/linux/commits/Gustavo-Silva/dt-bindings-vendor-prefixes-add-ScioSense/20240513-050745
base:   084eeee1d8da6b4712719264b01cb27b41307f54
patch link:    https://lore.kernel.org/r/20240512210444.30824-5-gustavograzs%40gmail.com
patch subject: [PATCH 4/6] iio: chemical: ens160: add triggered buffer support
config: arc-randconfig-r123-20240514 (https://download.01.org/0day-ci/archive/20240514/202405140721.LuiSHRvx-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240514/202405140721.LuiSHRvx-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/202405140721.LuiSHRvx-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/chemical/ens160_core.c:250:39: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned short @@     got restricted __le16 [addressable] [usertype] val @@
   drivers/iio/chemical/ens160_core.c:250:39: sparse:     expected unsigned short
   drivers/iio/chemical/ens160_core.c:250:39: sparse:     got restricted __le16 [addressable] [usertype] val
   drivers/iio/chemical/ens160_core.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/xarray.h, ...):
   include/linux/page-flags.h:242:46: sparse: sparse: self-comparison always evaluates to false
   include/linux/page-flags.h:242:46: sparse: sparse: self-comparison always evaluates to false

vim +250 drivers/iio/chemical/ens160_core.c

   232	
   233	static irqreturn_t ens160_trigger_handler(int irq, void *p)
   234	{
   235		struct iio_poll_func *pf = p;
   236		struct iio_dev *indio_dev = pf->indio_dev;
   237		struct ens160_data *data = iio_priv(indio_dev);
   238		__le16 val;
   239		int ret, i, j = 0;
   240	
   241		mutex_lock(&data->mutex);
   242	
   243		for_each_set_bit(i, indio_dev->active_scan_mask,
   244				 indio_dev->masklength) {
   245			ret = regmap_bulk_read(data->regmap,
   246					       ENS160_REG_DATA_TVOC + 2 * i, &val, 2U);
   247			if (ret)
   248				goto err;
   249	
 > 250			data->scan.chans[j++] = val;
   251		}
   252	
   253		iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
   254						   pf->timestamp);
   255	err:
   256		mutex_unlock(&data->mutex);
   257		iio_trigger_notify_done(indio_dev->trig);
   258	
   259		return IRQ_HANDLED;
   260	}
   261	

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

Re: [PATCH 2/6] dt-bindings: Add ENS160 as trivial device

[Jonathan Cameron]

On Mon, 13 May 2024 17:09:46 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Sun, May 12, 2024 at 06:04:38PM -0300, Gustavo Silva wrote:
> > ScioSense ENS160 is a multi-gas sensor.
> > 
> > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>  
> 
> Looks like this device has two supplies, Vdd and Vddio.
> Jonathan generally likes supplies to be documented, so that would
> disqualify this as a trivial device.

Agreed. History here is that we have put lots of IIO supported
devices in trivial-devices in the past and then it's turned out
someone has them on a board where they are controllable supplies.
So we end up having introduced insufficient binding docs + need
to move it later.

Much better to just have them documented from the start + just
turn them on at driver probe and off at remove (relying on regulator
subsystem support for fake supplies / fixed regulators where these
operations do nothing).  If someone wants to later do better power
control then that can be added without adding to the binding.

Jonathan

> 
> Cheers,
> Conor.
> 
> > ---
> >  Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> > index e07be7bf8..cdd7f0b46 100644
> > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > @@ -318,6 +318,8 @@ properties:
> >            - samsung,24ad0xd1
> >              # Samsung Exynos SoC SATA PHY I2C device
> >            - samsung,exynos-sataphy-i2c
> > +            # ScioSense ENS160 multi-gas sensor
> > +          - sciosense,ens160
> >              # Semtech sx1301 baseband processor
> >            - semtech,sx1301
> >              # Sensirion multi-pixel gas sensor with I2C interface
> > -- 
> > 2.45.0
> >   

Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor

[Jonathan Cameron]

On Mon, 13 May 2024 21:12:55 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> Le 12/05/2024 à 23:04, Gustavo Silva a écrit :
> > ScioSense ENS160 is a digital metal oxide multi-gas sensor, designed
> > for indoor air quality monitoring. The driver supports readings of
> > CO2 and VOC, and can be accessed via both SPI and I2C.
> > 
> > Signed-off-by: Gustavo Silva <gustavograzs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---  
> 
> Hi,
> a few comments below, for what it worth.
> 
> BTW, why I'm in copy of the mail?
> I'm not a maintainer, and not active on drivers/iio/chemical/
> Slightly proud, but curious as well.

Now we have evidence you'll review if CC'd, wait for the deluge :)

Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor

[Jonathan Cameron]

On Sun, 12 May 2024 18:04:39 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> ScioSense ENS160 is a digital metal oxide multi-gas sensor, designed
> for indoor air quality monitoring. The driver supports readings of
> CO2 and VOC, and can be accessed via both SPI and I2C.
> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>

Hi Gustavo,

Nice driver in general. 2 things that need fixing though.

- DMA safe buffers for the bulk accesses etc. This is esoteric and won't actually
  cause you any problems today (and depending on your host may never do so)
  but I'm trying to keep the IIO drivers inline with the rule that if they do SPI
  even via regmap and bulk accesses they need to use DMA safe buffers
- devm vs non-devm cleanup. If you mix these it has to be a clean transition. 
  So devm for first N and non devm for everything from N+1. They cannot be mixed
  safely. Easiest option is devm_ for everything.

Jonathan

> diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c
> new file mode 100644
> index 000000000..25593420d
> --- /dev/null
> +++ b/drivers/iio/chemical/ens160_core.c
> @@ -0,0 +1,227 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ScioSense ENS160 multi-gas sensor driver
> + *
> + * Copyright (c) 2024 Gustavo Silva <gustavograzs@gmail.com>
> + *
> + * Data sheet:
> + *  https://www.sciosense.com/wp-content/uploads/2023/12/ENS160-Datasheet.pdf
> + *
Trivial, but this blank line adds nothing so drop it.
> + */

> +
> +static int ens160_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ens160_data *data = iio_priv(indio_dev);
> +	__le16 buf;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(data->regmap, chan->address,
> +					&buf, sizeof(buf));

As below, should use a DMA safe buffer.

> +		if (ret)
> +			return ret;
> +		*val = le16_to_cpu(buf);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->channel2) {
> +		case IIO_MOD_CO2:
> +			/* The sensor reads CO2 data as ppm */
> +			*val = 0;
> +			*val2 = 100;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_MOD_VOC:
> +			/* The sensor reads VOC data as ppb */
> +			*val = 0;
> +			*val2 = 100;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		}

		default:
			return -EINVAL;
and drop the one below.

> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ens160_set_mode(struct ens160_data *data, u8 mode)
> +{
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, ENS160_REG_OPMODE, mode);
> +	if (ret)
> +		return ret;
> +
> +	msleep(ENS160_BOOTING_TIME_MS);
> +
> +	return 0;
> +}
> +
> +static int ens160_chip_init(struct ens160_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	u8 fw_version[3];
> +	__le16 part_id;
> +	unsigned int status;
> +	int ret;
> +
> +	ret = ens160_set_mode(data, ENS160_REG_MODE_RESET);
> +	if (ret)
> +		return ret;

No docs that I can see on what this means for access to registers etc.
Good to add a comment if you have info on this.

> +
> +	ret = regmap_bulk_read(data->regmap, ENS160_REG_PART_ID, &part_id,
> +			       sizeof(part_id));

Ah. So this is a fun corner case.  Currently regmap makes not guarantees
to always bounce buffer things (though last time I checked it actually did
do so - there are optimisations that may make sense where it will again
not do so).  So given we have an SPI bus involved, we should ensure that
only DMA safe buffers are used. These need to ensure that no other data
that might be changed concurrently with DMA is in the same IIO_DMA_MINALIGN
of aligned data (traditionally a cacheline but it gets more complex in some
systems and is less in others).  Upshot is that if you are are doing
bulk accesses you need to use a buffer that is either on the heap (kzalloc etc)
or carefully placed at the end of the iio_priv() structure marked
__align(IIO_DMA_MINALIGN). Lots of examples of that in tree.
If you are curious, wolfram did a good talk on the i2c equivalent of this
a few years back. 
https://www.youtube.com/watch?v=JDwaMClvV-s I think.

> +	if (ret)
> +		return ret;
> +
> +	if (le16_to_cpu(part_id) != ENS160_PART_ID)
> +		return -ENODEV;
> +
> +	ret = ens160_set_mode(data, ENS160_REG_MODE_IDLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> +			   ENS160_REG_COMMAND_CLRGPR);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> +			   ENS160_REG_COMMAND_GET_APPVER);
> +	if (ret)
> +		return ret;
> +
> +	msleep(ENS160_BOOTING_TIME_MS);

Why here?  Not obviously associated with a boot command?
A comment might make this easier to follow.  I 'think' it is
because this next read is the first time it matters. If so that
isn't obvious.  Also, there is an existing sleep in the mode set,
so I'm not sure why we need another one.

> +
> +	ret = regmap_bulk_read(data->regmap, ENS160_REG_GPR_READ4,
> +			       fw_version, sizeof(fw_version));

The first datasheet that google provided me has this 
GPR_READ0/GPR_READ1 and only 2 bytes. I hope they have maintained backwards
compatibility with that earlier doc!

When you do a separate DT binding in v2, make sure to include a link
to the datasheet you are using.  Also use a Datasheet: tag
in this patch to make it easy to find that.
I dug a little deeper and found the one on sciosense own website
- ah, you do have it in the comments.  Add to the commit message
and DT binding as well.


> +	if (ret)
> +		return ret;
> +
> +	msleep(ENS160_BOOTING_TIME_MS);
Why again?
> +
> +	dev_info(dev, "firmware version: %u.%u.%u\n", fw_version[2],
> +		 fw_version[1], fw_version[0]);

Can definitely do this before the sleep above.

> +
> +	ret = ens160_set_mode(data, ENS160_REG_MODE_STANDARD);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, ENS160_REG_DEVICE_STATUS, &status);
> +	if (ret)
> +		return ret;
> +
> +	if (FIELD_GET(ENS160_STATUS_VALIDITY_FLAG, status)
> +	    != ENS160_STATUS_NORMAL)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static const struct iio_info ens160_info = {
> +	.read_raw = ens160_read_raw,
> +};
> +
> +int ens160_core_probe(struct device *dev, struct regmap *regmap,
> +		      const char *name)
> +{
> +	struct ens160_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	dev_set_drvdata(dev, indio_dev);

After you've moved to devm_add_action_or_reset() for the unwind of
ens160_chip_init() you probably don't need to set the drvdata.

> +	data->regmap = regmap;
> +
> +	indio_dev->name = name;
> +	indio_dev->info = &ens160_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ens160_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
> +
> +	ret = ens160_chip_init(data);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "chip initialization failed\n");
> +		return ret;
		return dev_err_probe();

> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160);
> +
> +void ens160_core_remove(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ens160_data *data = iio_priv(indio_dev);
> +
> +	ens160_set_mode(data, ENS160_REG_MODE_IDLE);

This looks to be mixing devm and manual cleanup.
My guess is this is the unwind for code in ens160_chip_init()
If so that unwind should definitely happen after we unregister
the userspace intefaces in the unwind of devm_iio_device_register().
Currently it happens before this.

This is an even stronger reason to use devm_add_action_or_reset()
for this than tidying up as mentioned below (note I tend to
review backwards through patches so my comments may make more
sense read that way around).

> +}
> +EXPORT_SYMBOL_NS(ens160_core_remove, IIO_ENS160);
> +
> +MODULE_AUTHOR("Gustavo Silva <gustavograzs@gmail.com>");
> +MODULE_DESCRIPTION("ScioSense ENS160 driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/chemical/ens160_i2c.c b/drivers/iio/chemical/ens160_i2c.c
> new file mode 100644
> index 000000000..ee2b44184
> --- /dev/null
> +++ b/drivers/iio/chemical/ens160_i2c.c
> @@ -0,0 +1,68 @@
...

> +static int ens160_i2c_probe(struct i2c_client *client)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i2c(client, &ens160_regmap_i2c_conf);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "Failed to register i2c regmap %ld\n",
> +			PTR_ERR(regmap));
> +		return PTR_ERR(regmap);
> +	}
> +
> +	return ens160_core_probe(&client->dev, regmap, client->name);

As below, hardcode the name for now.  If it matters in future, get it
from a chip specific structure that we can look up from whichever
firmware table we have matched against.

> +}
> +
> +static void ens160_i2c_remove(struct i2c_client *client)
> +{
> +	ens160_core_remove(&client->dev);
As below, switch to devm_add_action_or_reset() called from
devm_ens160_core_probe() to avoid need to have manual remove
in here.

> +}
> +
> +static const struct i2c_device_id ens160_i2c_id[] = {
> +	{ "ens160", 0 },

As below - drop the 0.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ens160_i2c_id);
...

> diff --git a/drivers/iio/chemical/ens160_spi.c b/drivers/iio/chemical/ens160_spi.c
> new file mode 100644
> index 000000000..effc4acee
> --- /dev/null
> +++ b/drivers/iio/chemical/ens160_spi.c
...

> +static int ens160_spi_probe(struct spi_device *spi)
> +{
> +	struct regmap *regmap;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	regmap = devm_regmap_init_spi(spi, &ens160_regmap_spi_conf);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&spi->dev, "Failed to register spi regmap: %pe\n",
> +			regmap);
> +		return PTR_ERR(regmap);
> +	}
> +
> +	return ens160_core_probe(&spi->dev, regmap, id->name);

Hardcode the name here for now.  When you support multiple drivers you will want to
get it from a chip type specific structure, not id->name because that path gives
rather unexpected answers if we are using devicetree fallback compatibles
or the lists end up not matching perfectly for some other reason.

That fragility means we mostly just use another source for the name
and where possible hard code it.

> +}
> +
> +static void ens160_spi_remove(struct spi_device *spi)
> +{
> +	ens160_core_remove(&spi->dev);
Might as well register an automated cleanup, particularly as you can
do it in ens160_core_probe() and have it apply to any other buses for
free.  If you do that, rename that function devm_ens160_core_probe()
to make it obvious that is what is going on.

Use devm_add_action_or_reset() to register custom cleanup.

Then you can drop this remove function entirely.

> +}
> +
> +static const struct of_device_id ens160_spi_of_match[] = {
> +	{ .compatible = "sciosense,ens160" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ens160_spi_of_match);
> +
> +static const struct spi_device_id ens160_spi_id[] = {
> +	{ "ens160", 0 },

Don't set the 0 as that suggests it matters whereas it doesn't
yet. Maybe it will matter when more parts are added to this driver in future
 - if so introduce it then.  As a general best practice, this is almost
always a pointer these days anyway rather than an integer.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ens160_spi_id);

Re: [PATCH 4/6] iio: chemical: ens160: add triggered buffer support

[Jonathan Cameron]

On Mon, 13 May 2024 21:13:07 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> Le 12/05/2024 à 23:04, Gustavo Silva a écrit :
> > ENS160 supports a data ready interrupt. Use it in combination with
> > triggered buffer for continuous data readings.
> > 
> > Signed-off-by: Gustavo Silva <gustavograzs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---  
> 
> ...
> 
> > +static irqreturn_t ens160_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct ens160_data *data = iio_priv(indio_dev);
> > +	__le16 val;
> > +	int ret, i, j = 0;
> > +
> > +	mutex_lock(&data->mutex);
> > +
> > +	for_each_set_bit(i, indio_dev->active_scan_mask,
> > +			 indio_dev->masklength) {
> > +		ret = regmap_bulk_read(data->regmap,
> > +				       ENS160_REG_DATA_TVOC + 2 * i, &val, 2U);
> > +		if (ret)
> > +			goto err;
> > +
> > +		data->scan.chans[j++] = val;  
> 
> Is it safe? How can we know if it has been only *partly* updated? Does 
> it matter to know?

You've lost me. What do you mean by partly updated? 
This won't push anything to the kfifo etc unless all succeeded.
Or is there a race with something else in here?

> 
> CJ
> 
> > +	}
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> > +					   pf->timestamp);
> > +err:
> > +	mutex_unlock(&data->mutex);
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}  
> 
> ...

Re: [PATCH 4/6] iio: chemical: ens160: add triggered buffer support

[Jonathan Cameron]

On Sun, 12 May 2024 18:04:40 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> ENS160 supports a data ready interrupt. Use it in combination with
> triggered buffer for continuous data readings.
> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
Hi Gustavo,

Various comments inline. Mostly simplifications you can probably make.

Jonathan

> ---
>  drivers/iio/chemical/ens160.h      |   2 +-
>  drivers/iio/chemical/ens160_core.c | 155 ++++++++++++++++++++++++++++-
>  drivers/iio/chemical/ens160_i2c.c  |   2 +-
>  drivers/iio/chemical/ens160_spi.c  |   2 +-
>  4 files changed, 156 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/chemical/ens160.h b/drivers/iio/chemical/ens160.h
> index 3fd079bc4..a8a2f1263 100644
> --- a/drivers/iio/chemical/ens160.h
> +++ b/drivers/iio/chemical/ens160.h
> @@ -2,7 +2,7 @@
>  #ifndef ENS160_H_
>  #define ENS160_H_
>  
> -int ens160_core_probe(struct device *dev, struct regmap *regmap,
> +int ens160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  		      const char *name);
>  void ens160_core_remove(struct device *dev);
>  
> diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c
> index 25593420d..4b960ef00 100644
> --- a/drivers/iio/chemical/ens160_core.c
> +++ b/drivers/iio/chemical/ens160_core.c
> @@ -11,6 +11,9 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  
> @@ -24,6 +27,11 @@
>  
>  #define ENS160_REG_OPMODE	0x10
>  
> +#define ENS160_REG_CONFIG		0x11
> +#define ENS160_REG_CONFIG_INTEN		BIT(0)
> +#define ENS160_REG_CONFIG_INTDAT	BIT(1)
> +#define ENS160_REG_CONFIG_INT_CFG	BIT(5)
> +
>  #define ENS160_REG_MODE_DEEP_SLEEP	0x00
>  #define ENS160_REG_MODE_IDLE		0x01
>  #define ENS160_REG_MODE_STANDARD	0x02
> @@ -48,6 +56,12 @@
>  
>  struct ens160_data {
>  	struct regmap *regmap;
> +	struct mutex mutex;
> +	struct {
> +		u16 chans[2];

As per the bot reply. This should be __le16.
> +		s64 timestamp __aligned(8);
> +	} scan;

You can do spi read directly into here but if you do
move it to the end of the structure and align it to IIO_DMA_MINALIGN.

> +	int irq;
As below - not sure there is any advantage in keeping a copy
of this after probe. I'd just pass it into the functions that need it.
>  };

>  
>  static int ens160_read_raw(struct iio_dev *indio_dev,
> @@ -79,10 +108,19 @@ static int ens160_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);

Use iio_device_claim_direct_scoped() and guard() for the mutex
as will automate the unwinding of the two types of lock and avoid
you having to do it by hand.


> +		if (ret)
> +			return ret;
> +		mutex_lock(&data->mutex);
>  		ret = regmap_bulk_read(data->regmap, chan->address,
>  					&buf, sizeof(buf));
> -		if (ret)
> +		if (ret) {
> +			mutex_unlock(&data->mutex);
> +			iio_device_release_direct_mode(indio_dev);
>  			return ret;
> +		}
> +		mutex_unlock(&data->mutex);
> +		iio_device_release_direct_mode(indio_dev);
>  		*val = le16_to_cpu(buf);
>  		return IIO_VAL_INT;
>  
> @@ -182,7 +220,104 @@ static const struct iio_info ens160_info = {
>  	.read_raw = ens160_read_raw,
>  };
>  
> -int ens160_core_probe(struct device *dev, struct regmap *regmap,
> +static irqreturn_t ens160_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +
> +	if (iio_buffer_enabled(indio_dev))

How else did you get here?  Either you should use a threaded interrupt
to check the status registers on the device, or you should assume
there is no other way of getting here (and hence no sharing of interrupt
etc) in which case this check is unnecessary and you can use
iio_trigger_generic_data_rdy_poll().



> +		iio_trigger_poll(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t ens160_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ens160_data *data = iio_priv(indio_dev);
> +	__le16 val;
> +	int ret, i, j = 0;
> +
> +	mutex_lock(&data->mutex);
> +
> +	for_each_set_bit(i, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		ret = regmap_bulk_read(data->regmap,
> +				       ENS160_REG_DATA_TVOC + 2 * i, &val, 2U);

sizeof(val) instead of hardcoded 2. Though better still to just bulk
read the lot ever time and loose this loop in favour of the demux in the IIO
core handling the rare occasion of people wanting one channel.

> +		if (ret)
> +			goto err;
> +
> +		data->scan.chans[j++] = val;

Read directly into the data->scan.chans[]

Also, I'd assume that 90% of the time, people want all the channels.  A such
can you just bulk read them all?  Then you can use available_scan_masks
to let the IIO core handle the 10% of the time when only one channel is requested.


> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +					   pf->timestamp);
> +err:
> +	mutex_unlock(&data->mutex);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ens160_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct ens160_data *data = iio_priv(indio_dev);
> +	unsigned int int_bits = ENS160_REG_CONFIG_INTEN |
> +				ENS160_REG_CONFIG_INTDAT |
> +				ENS160_REG_CONFIG_INT_CFG;
> +	int ret;
> +
> +	if (state)
> +		ret = regmap_set_bits(data->regmap, ENS160_REG_CONFIG,
> +				      int_bits);
		return ...
> +	else
> +		ret = regmap_clear_bits(data->regmap, ENS160_REG_CONFIG,
> +					int_bits);
		return ...

> +
> +	return ret;
> +}
> +
> +static const struct iio_trigger_ops ens160_trigger_ops = {
> +	.set_trigger_state = ens160_set_trigger_state,
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int ens160_setup_trigger(struct iio_dev *indio_dev)
> +{
> +	struct ens160_data *data = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_trigger *trig;
> +	int ret;
> +
> +	trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> +				      iio_device_id(indio_dev));
> +	if (!trig)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "failed to allocate trigger\n");
> +
> +	trig->ops = &ens160_trigger_ops;
> +	iio_trigger_set_drvdata(trig, indio_dev);
> +
> +	ret = devm_iio_trigger_register(dev, trig);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(trig);
> +
> +	ret = devm_request_threaded_irq(dev, data->irq,
> +					ens160_irq_handler,
> +					NULL,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
Generally, for new drivers we leave the direction control up to firmware.
A nasty, but common trick is to use an inverter to do level conversion.
That results in the polarity being switched but is not explicitly described
in the firmware. So we rely in those cases on the firmware settings for
the interrupt not being modified by the driver.

IRQF_ONESHOT, only here.

> +					indio_dev->name,
> +					indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request irq\n");
> +
> +	return 0;
> +}
> +
> +int ens160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  		      const char *name)
>  {
>  	struct ens160_data *data;
> @@ -196,6 +331,7 @@ int ens160_core_probe(struct device *dev, struct regmap *regmap,
>  	data = iio_priv(indio_dev);
>  	dev_set_drvdata(dev, indio_dev);
>  	data->regmap = regmap;
> +	data->irq = irq;

As below. This stashing of a copy of irq is an unnecessary complication.

>  
>  	indio_dev->name = name;
>  	indio_dev->info = &ens160_info;
> @@ -203,12 +339,27 @@ int ens160_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->channels = ens160_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
>  
> +	if (data->irq > 0) {

Pass the irq into the setup_trigger call. You don't need it other than for
registration so no point in keeping it in the data structure.

> +		ret = ens160_setup_trigger(indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to setup trigger\n");
> +	}
> +
>  	ret = ens160_chip_init(data);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "chip initialization failed\n");
>  		return ret;
>  	}
>  
> +	mutex_init(&data->mutex);
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ens160_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160);

Re: [PATCH 5/6] iio: chemical: ens160: add power management support

[Jonathan Cameron]

On Sun, 12 May 2024 18:04:41 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> ENS160 supports a deep sleep mode for minimal power consumption.
> Use it to add PM sleep capability to the driver.
> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
Looks good.

J

Re: [PATCH 4/6] iio: chemical: ens160: add triggered buffer support

[Christophe JAILLET]

Le 19/05/2024 à 16:03, Jonathan Cameron a écrit :
> On Mon, 13 May 2024 21:13:07 +0200
> Christophe JAILLET <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org> wrote:
> 
>> Le 12/05/2024 à 23:04, Gustavo Silva a écrit :
>>> ENS160 supports a data ready interrupt. Use it in combination with
>>> triggered buffer for continuous data readings.
>>>
>>> Signed-off-by: Gustavo Silva <gustavograzs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>
>> ...
>>
>>> +static irqreturn_t ens160_trigger_handler(int irq, void *p)
>>> +{
>>> +	struct iio_poll_func *pf = p;
>>> +	struct iio_dev *indio_dev = pf->indio_dev;
>>> +	struct ens160_data *data = iio_priv(indio_dev);
>>> +	__le16 val;
>>> +	int ret, i, j = 0;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +
>>> +	for_each_set_bit(i, indio_dev->active_scan_mask,
>>> +			 indio_dev->masklength) {
>>> +		ret = regmap_bulk_read(data->regmap,
>>> +				       ENS160_REG_DATA_TVOC + 2 * i, &val, 2U);
>>> +		if (ret)
>>> +			goto err;
>>> +
>>> +		data->scan.chans[j++] = val;
>>
>> Is it safe? How can we know if it has been only *partly* updated? Does
>> it matter to know?
> 
> You've lost me. What do you mean by partly updated?
> This won't push anything to the kfifo etc unless all succeeded.
> Or is there a race with something else in here?

Forget it, I misread the place of iio_push_to_buffers_with_timestamp().

I thought we were going through it when 'goto err'.

CJ

> 
>>
>> CJ
>>
>>> +	}
>>> +
>>> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
>>> +					   pf->timestamp);
>>> +err:
>>> +	mutex_unlock(&data->mutex);
>>> +	iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>
>> ...
> 

Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor

[Gustavo Silva]

On Mon, May 13, 2024 at 09:12:55PM GMT, Christophe JAILLET wrote:
> Le 12/05/2024 à 23:04, Gustavo Silva a écrit :
> > ScioSense ENS160 is a digital metal oxide multi-gas sensor, designed
> > for indoor air quality monitoring. The driver supports readings of
> > CO2 and VOC, and can be accessed via both SPI and I2C.
> > 
> > Signed-off-by: Gustavo Silva <gustavograzs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> 
> Hi,
> a few comments below, for what it worth.
> 
> BTW, why I'm in copy of the mail?
> I'm not a maintainer, and not active on drivers/iio/chemical/
> Slightly proud, but curious as well.
> 
Hi Christophe,

Your name was listed by the `get_maintainer.pl` script, so I may have
added you to CC accidentally, my bad. I appreciate your review
nonetheless.

> ...
> 
> > +#define ENS160_REG_TEMP_IN		0x13
> > +#define ENS160_REG_RH_IN		0x15
> > +#define ENS160_REG_DEVICE_STATUS	0x20
> 
> If defining everything, maybe:
> #define ENS160_REG_DATA_AQI	0x21
> 
Ack.

> > +#define ENS160_REG_DATA_TVOC		0x22
> > +#define ENS160_REG_DATA_ECO2		0x24
> > +#define ENS160_REG_DATA_T		0x30
> > +#define ENS160_REG_DATA_RH		0x32
> > +#define ENS160_REG_GPR_READ4		0x4C
> 
> ...
> 
> > +static int ens160_chip_init(struct ens160_data *data)
> > +{
> > +	struct device *dev = regmap_get_device(data->regmap);
> > +	u8 fw_version[3];
> > +	__le16 part_id;
> > +	unsigned int status;
> > +	int ret;
> > +
> > +	ret = ens160_set_mode(data, ENS160_REG_MODE_RESET);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_bulk_read(data->regmap, ENS160_REG_PART_ID, &part_id,
> > +			       sizeof(part_id));
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (le16_to_cpu(part_id) != ENS160_PART_ID)
> > +		return -ENODEV;
> > +
> > +	ret = ens160_set_mode(data, ENS160_REG_MODE_IDLE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> > +			   ENS160_REG_COMMAND_CLRGPR);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> > +			   ENS160_REG_COMMAND_GET_APPVER);
> > +	if (ret)
> > +		return ret;
> > +
> > +	msleep(ENS160_BOOTING_TIME_MS);
> > +
> > +	ret = regmap_bulk_read(data->regmap, ENS160_REG_GPR_READ4,
> > +			       fw_version, sizeof(fw_version));
> > +	if (ret)
> > +		return ret;
> > +
> > +	msleep(ENS160_BOOTING_TIME_MS);
> > +
> > +	dev_info(dev, "firmware version: %u.%u.%u\n", fw_version[2],
> > +		 fw_version[1], fw_version[0]);
> > +
> > +	ret = ens160_set_mode(data, ENS160_REG_MODE_STANDARD);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(data->regmap, ENS160_REG_DEVICE_STATUS, &status);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (FIELD_GET(ENS160_STATUS_VALIDITY_FLAG, status)
> > +	    != ENS160_STATUS_NORMAL)
> > +		return -EINVAL;
> 
> Just wondering how it works with the Warm-up and initial Start-up times.
> If the probe is executed and the corresponding duration has not elpased,
> then the probe fails.
> 
> Is it what is expected?
>
According to the datasheet, the warm-up time corresponds to the first 3
minutes after power-on. However, the chip I'm working with always seems
to go straight to standard operating mode (validity flag = 0x00)
immediately after power-on.

Also, checking other drivers for the same sensor, including ScioSense's
official arduino driver, none of them seem to consider this initial
warm-up time.

Maybe it is more reasonable not to fail the probe based on this
condition but instead only log the status. If the status reads 1
(warm-up) or 2 (initial start-up) the readings may be unreliable for
some time, but the user will be warned. What do you think?

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct iio_info ens160_info = {
> > +	.read_raw = ens160_read_raw,
> > +};
> > +
> > +int ens160_core_probe(struct device *dev, struct regmap *regmap,
> > +		      const char *name)
> > +{
> > +	struct ens160_data *data;
> > +	struct iio_dev *indio_dev;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	dev_set_drvdata(dev, indio_dev);
> > +	data->regmap = regmap;
> > +
> > +	indio_dev->name = name;
> > +	indio_dev->info = &ens160_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = ens160_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
> > +
> > +	ret = ens160_chip_init(data);
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "chip initialization failed\n");
> 
> Nitpick: return dev_err_probe()
Ack.

> 
> > +		return ret;
> > +	}
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> 
> ...
> 
> > +static int ens160_i2c_probe(struct i2c_client *client)
> > +{
> > +	struct regmap *regmap;
> > +
> > +	regmap = devm_regmap_init_i2c(client, &ens160_regmap_i2c_conf);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(&client->dev, "Failed to register i2c regmap %ld\n",
> > +			PTR_ERR(regmap));
> 
> Nitpick: dev_err_probe()
Ack.

> 
> > +		return PTR_ERR(regmap);
> > +	}
> 
> ...
> 
> > +static int ens160_spi_probe(struct spi_device *spi)
> > +{
> > +	struct regmap *regmap;
> > +	const struct spi_device_id *id = spi_get_device_id(spi);
> > +
> > +	regmap = devm_regmap_init_spi(spi, &ens160_regmap_spi_conf);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(&spi->dev, "Failed to register spi regmap: %pe\n",
> > +			regmap);
> 
> Nitpick: dev_err_probe()
Ack.

> 
> CJ
> 
> > +		return PTR_ERR(regmap);
> > +	}
> > +
> > +	return ens160_core_probe(&spi->dev, regmap, id->name);
> > +}
> 
> 

Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor

[Gustavo Silva]

Hi Jonathan,

Thank you for your review. I've got a few questions inline.

On Sun, May 19, 2024 at 03:01:51PM GMT, Jonathan Cameron wrote:
> On Sun, 12 May 2024 18:04:39 -0300
> Gustavo Silva <gustavograzs@gmail.com> wrote:
> 
> > ScioSense ENS160 is a digital metal oxide multi-gas sensor, designed
> > for indoor air quality monitoring. The driver supports readings of
> > CO2 and VOC, and can be accessed via both SPI and I2C.
> > 
> > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
> 
> > +
> > +static int ens160_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct ens160_data *data = iio_priv(indio_dev);
> > +	__le16 buf;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = regmap_bulk_read(data->regmap, chan->address,
> > +					&buf, sizeof(buf));
> 
> As below, should use a DMA safe buffer.
> 
> > +static int ens160_chip_init(struct ens160_data *data)
> > +{
> > +	struct device *dev = regmap_get_device(data->regmap);
> > +	u8 fw_version[3];
> > +	__le16 part_id;
> > +	unsigned int status;
> > +	int ret;
> > +
> > +	ret = ens160_set_mode(data, ENS160_REG_MODE_RESET);
> > +	if (ret)
> > +		return ret;
> 
> No docs that I can see on what this means for access to registers etc.
> Good to add a comment if you have info on this.
> 
Performing a reset at this point isn't strictly necessary. When we reach
this point the chip should be in idle state because:

a) it was just powered on

b) the driver had been previously removed

This is documented in the state diagram on page 24 of the datasheet.

I'll remove this reset.

> > +
> > +	ret = regmap_bulk_read(data->regmap, ENS160_REG_PART_ID, &part_id,
> > +			       sizeof(part_id));
> 
> Ah. So this is a fun corner case.  Currently regmap makes not guarantees
> to always bounce buffer things (though last time I checked it actually did
> do so - there are optimisations that may make sense where it will again
> not do so).  So given we have an SPI bus involved, we should ensure that
> only DMA safe buffers are used. These need to ensure that no other data
> that might be changed concurrently with DMA is in the same IIO_DMA_MINALIGN
> of aligned data (traditionally a cacheline but it gets more complex in some
> systems and is less in others).  Upshot is that if you are are doing
> bulk accesses you need to use a buffer that is either on the heap (kzalloc etc)
> or carefully placed at the end of the iio_priv() structure marked
> __align(IIO_DMA_MINALIGN). Lots of examples of that in tree.
> If you are curious, wolfram did a good talk on the i2c equivalent of this
> a few years back. 
> https://www.youtube.com/watch?v=JDwaMClvV-s I think.
>
Interesting. Thank you for the detailed info.

> > +	if (ret)
> > +		return ret;
> > +
> > +	if (le16_to_cpu(part_id) != ENS160_PART_ID)
> > +		return -ENODEV;
> > +
> > +	ret = ens160_set_mode(data, ENS160_REG_MODE_IDLE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> > +			   ENS160_REG_COMMAND_CLRGPR);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> > +			   ENS160_REG_COMMAND_GET_APPVER);
> > +	if (ret)
> > +		return ret;
> > +
> > +	msleep(ENS160_BOOTING_TIME_MS);
> 
> Why here?  Not obviously associated with a boot command?
> A comment might make this easier to follow.  I 'think' it is
> because this next read is the first time it matters. If so that
> isn't obvious.  Also, there is an existing sleep in the mode set,
> so I'm not sure why we need another one.
>
The usage of booting time is not documented in the datasheet. From
ScioSense's arduino driver the booting time is necessary after setting
the operation mode. I performed some tests that confirm this.

In this case in particular it is not necessary. Maybe I forgot to remove
it after some testing.
> > +
> > +	ret = regmap_bulk_read(data->regmap, ENS160_REG_GPR_READ4,
> > +			       fw_version, sizeof(fw_version));
> 
Does this bulk read also need to be made DMA safe? I'm guessing in this
case it would be best to devm_kzalloc() a buffer of three bytes?

> The first datasheet that google provided me has this 
> GPR_READ0/GPR_READ1 and only 2 bytes. I hope they have maintained backwards
> compatibility with that earlier doc!
> 
> When you do a separate DT binding in v2, make sure to include a link
> to the datasheet you are using.  Also use a Datasheet: tag
> in this patch to make it easy to find that.
> I dug a little deeper and found the one on sciosense own website
> - ah, you do have it in the comments.  Add to the commit message
> and DT binding as well.
> 
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	msleep(ENS160_BOOTING_TIME_MS);
> Why again?
Again, not needed. I'll remove it.

> > +	data = iio_priv(indio_dev);
> > +	dev_set_drvdata(dev, indio_dev);
> 
> After you've moved to devm_add_action_or_reset() for the unwind of
> ens160_chip_init() you probably don't need to set the drvdata.
> 
I don't get it. Could you please elaborate on why it isn't needed to
set drvdata after the change?

> > +	data->regmap = regmap;
> > +
> > +	indio_dev->name = name;
> > +	indio_dev->info = &ens160_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = ens160_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
> > +
> > +	ret = ens160_chip_init(data);
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "chip initialization failed\n");
> > +		return ret;
> 		return dev_err_probe();
> 
> > +	}
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> > +EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160);
> > +
> > +void ens160_core_remove(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct ens160_data *data = iio_priv(indio_dev);
> > +
> > +	ens160_set_mode(data, ENS160_REG_MODE_IDLE);
> 
> This looks to be mixing devm and manual cleanup.
> My guess is this is the unwind for code in ens160_chip_init()
> If so that unwind should definitely happen after we unregister
> the userspace intefaces in the unwind of devm_iio_device_register().
> Currently it happens before this.
> 
> This is an even stronger reason to use devm_add_action_or_reset()
> for this than tidying up as mentioned below (note I tend to
> review backwards through patches so my comments may make more
> sense read that way around).
>
The intention was to transition the chip into idle mode upon driver
removal, ensuring the sensor ceased data readings.
I'll use devm_add_action_or_reset() as suggested.

Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor

[Jonathan Cameron]

On Sat, 25 May 2024 21:29:42 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> Hi Jonathan,
> 
> Thank you for your review. I've got a few questions inline.
> 
> On Sun, May 19, 2024 at 03:01:51PM GMT, Jonathan Cameron wrote:
> > On Sun, 12 May 2024 18:04:39 -0300
> > Gustavo Silva <gustavograzs@gmail.com> wrote:
> >   
> > > ScioSense ENS160 is a digital metal oxide multi-gas sensor, designed
> > > for indoor air quality monitoring. The driver supports readings of
> > > CO2 and VOC, and can be accessed via both SPI and I2C.
> > > 
> > > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>  
> >   
> > > +
> > > +static int ens160_read_raw(struct iio_dev *indio_dev,
> > > +			   struct iio_chan_spec const *chan,
> > > +			   int *val, int *val2, long mask)
> > > +{
> > > +	struct ens160_data *data = iio_priv(indio_dev);
> > > +	__le16 buf;
> > > +	int ret;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		ret = regmap_bulk_read(data->regmap, chan->address,
> > > +					&buf, sizeof(buf));  
> > 
> > As below, should use a DMA safe buffer.
> >   
> > > +static int ens160_chip_init(struct ens160_data *data)
> > > +{
> > > +	struct device *dev = regmap_get_device(data->regmap);
> > > +	u8 fw_version[3];
> > > +	__le16 part_id;
> > > +	unsigned int status;
> > > +	int ret;
> > > +
> > > +	ret = ens160_set_mode(data, ENS160_REG_MODE_RESET);
> > > +	if (ret)
> > > +		return ret;  
> > 
> > No docs that I can see on what this means for access to registers etc.
> > Good to add a comment if you have info on this.
> >   
> Performing a reset at this point isn't strictly necessary. When we reach
> this point the chip should be in idle state because:
> 
> a) it was just powered on

Maybe but we have no way of telling that
> 
> b) the driver had been previously removed

Maybe or maybe not - the device may have just done a soft reboot and switched
operating system. We have no idea what the hardware state is.
As such the reset is a good idea.
> 
> This is documented in the state diagram on page 24 of the datasheet.
> 
> I'll remove this reset.

Better to keep the reset and provide info on what it means wrt to accessing
registers etc if possible. If there is no information then obviously not
much you can add in the way of documentation!

> 
> > > +
> > > +	ret = regmap_bulk_read(data->regmap, ENS160_REG_PART_ID, &part_id,
> > > +			       sizeof(part_id));  
> > 
> > Ah. So this is a fun corner case.  Currently regmap makes not guarantees
> > to always bounce buffer things (though last time I checked it actually did
> > do so - there are optimisations that may make sense where it will again
> > not do so).  So given we have an SPI bus involved, we should ensure that
> > only DMA safe buffers are used. These need to ensure that no other data
> > that might be changed concurrently with DMA is in the same IIO_DMA_MINALIGN
> > of aligned data (traditionally a cacheline but it gets more complex in some
> > systems and is less in others).  Upshot is that if you are are doing
> > bulk accesses you need to use a buffer that is either on the heap (kzalloc etc)
> > or carefully placed at the end of the iio_priv() structure marked
> > __align(IIO_DMA_MINALIGN). Lots of examples of that in tree.
> > If you are curious, wolfram did a good talk on the i2c equivalent of this
> > a few years back. 
> > https://www.youtube.com/watch?v=JDwaMClvV-s I think.
> >  
> Interesting. Thank you for the detailed info.
> 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (le16_to_cpu(part_id) != ENS160_PART_ID)
> > > +		return -ENODEV;
> > > +
> > > +	ret = ens160_set_mode(data, ENS160_REG_MODE_IDLE);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> > > +			   ENS160_REG_COMMAND_CLRGPR);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> > > +			   ENS160_REG_COMMAND_GET_APPVER);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	msleep(ENS160_BOOTING_TIME_MS);  
> > 
> > Why here?  Not obviously associated with a boot command?
> > A comment might make this easier to follow.  I 'think' it is
> > because this next read is the first time it matters. If so that
> > isn't obvious.  Also, there is an existing sleep in the mode set,
> > so I'm not sure why we need another one.
> >  
> The usage of booting time is not documented in the datasheet. From
> ScioSense's arduino driver the booting time is necessary after setting
> the operation mode. I performed some tests that confirm this.
> 
> In this case in particular it is not necessary. Maybe I forgot to remove
> it after some testing.
> > > +
> > > +	ret = regmap_bulk_read(data->regmap, ENS160_REG_GPR_READ4,
> > > +			       fw_version, sizeof(fw_version));  
> >   
> Does this bulk read also need to be made DMA safe? I'm guessing in this
> case it would be best to devm_kzalloc() a buffer of three bytes?
All bulk accesses need dma safe buffers.
I would take the approach many IIO drivers do of not doing another allocation
(which has overheads etc) but instead just add a suitable __aligned(IIO_DMA_MINALIGN)
buffer to the iio_priv structure.  Note you normally only need to do mark
the first one like that as we don't care if various different DMA buffers
are in the same cacheline as the SPI controller should not cause DMA safety
issues with itself.

> 
> > The first datasheet that google provided me has this 
> > GPR_READ0/GPR_READ1 and only 2 bytes. I hope they have maintained backwards
> > compatibility with that earlier doc!
> > 
> > When you do a separate DT binding in v2, make sure to include a link
> > to the datasheet you are using.  Also use a Datasheet: tag
> > in this patch to make it easy to find that.
> > I dug a little deeper and found the one on sciosense own website
> > - ah, you do have it in the comments.  Add to the commit message
> > and DT binding as well.
> > 
> >   
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	msleep(ENS160_BOOTING_TIME_MS);  
> > Why again?  
> Again, not needed. I'll remove it.
> 
> > > +	data = iio_priv(indio_dev);
> > > +	dev_set_drvdata(dev, indio_dev);  
> > 
> > After you've moved to devm_add_action_or_reset() for the unwind of
> > ens160_chip_init() you probably don't need to set the drvdata.
> >   
> I don't get it. Could you please elaborate on why it isn't needed to
> set drvdata after the change?

No other users.  Only the remove() callback calls the matching get_drvdata()
and that function won't exist once you've added device managed callbacks
to handle everything it does.

Jonathan