[PATCH v3 0/2] iio: humidity: Add support for en21x sensor family

[PATCH v3 0/2] iio: humidity: Add support for en21x sensor family

[Joshua Felmeden]

This patch series adds support for the
ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215 temperature and humidity
sensors.

Patch 1 adds the required device tree bindings.

Patch 2 adds the driver, providing the probe and read functions.

Signed-off-by: Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>

changelog v1 -> v2:
sciosense,ens21x.yaml: Add supply to documentation
sciosense,ens21x.yaml: Add fallback to compatible strings
ens21x.c: Move i2c_device_id next to of_device_id
ens21x.c: Use i2c_of_match_device() instead of of_match_device()

Many thanks for taking the time to review my patch.

Thanks,
Josh

---
changelog v2 -> v3:
sciosense,ens21x.yaml: Update yaml to match dt_binding_check

- Link to V1: https://lore.kernel.org/all/20240709-ens21x-v1-2-678521433cdd@thegoodpenguin.co.uk/
- Link to v2: https://lore.kernel.org/r/20240710-ens21x-v2-0-a37c22018b0c@thegoodpenguin.co.uk

---
Joshua Felmeden (2):
      dt-bindings: iio: humidity: add ENS21x sensor family
      iio: humidity: Add support for ENS21x

 .../bindings/iio/humidity/sciosense,ens21x.yaml    |  55 ++++
 drivers/iio/humidity/Kconfig                       |  11 +
 drivers/iio/humidity/Makefile                      |   1 +
 drivers/iio/humidity/ens21x.c                      | 346 +++++++++++++++++++++
 4 files changed, 413 insertions(+)
---
base-commit: 1ebab783647a9e3bf357002d5c4ff060c8474a0a
change-id: 20240709-ens21x-8f2530968f2e

Best regards,
-- 
Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>

[PATCH v3 1/2] dt-bindings: iio: humidity: add ENS21x sensor family

[Joshua Felmeden]

Add device tree documentation for ENS21x family of temperature and
humidity sensors

Signed-off-by: Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>
---
 .../bindings/iio/humidity/sciosense,ens21x.yaml    | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/humidity/sciosense,ens21x.yaml b/Documentation/devicetree/bindings/iio/humidity/sciosense,ens21x.yaml
new file mode 100644
index 000000000000..425d3b57f701
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/humidity/sciosense,ens21x.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/humidity/sciosense,ens21x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ScioSense ENS21x temperature and humidity sensor
+
+maintainers:
+  - Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>
+
+description: |
+  Temperature and Humidity sensor.
+
+  Datasheet:
+    https://www.sciosense.com/wp-content/uploads/2024/04/ENS21x-Datasheet.pdf
+    https://www.sciosense.com/wp-content/uploads/2023/12/ENS210-Datasheet.pdf
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - sciosense,ens210a
+              - sciosense,ens211
+              - sciosense,ens212
+              - sciosense,ens213a
+              - sciosense,ens215
+          - const: sciosense,ens210
+      - const: sciosense,ens210
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+       #address-cells = <1>;
+       #size-cells = <0>;
+
+       temperature-sensor@43 {
+           compatible = "sciosense,ens210";
+           reg = <0x43>;
+       };
+    };
+...
+

-- 
2.39.2

[PATCH v3 2/2] iio: humidity: Add support for ENS21x

[Joshua Felmeden]

Add support for ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215.

The ENS21x is a family of temperature and relative humidity sensors with
accuracies tailored to the needs of specific applications.

Signed-off-by: Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>
---
 drivers/iio/humidity/Kconfig  |  11 ++
 drivers/iio/humidity/Makefile |   1 +
 drivers/iio/humidity/ens21x.c | 346 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 358 insertions(+)

diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
index b15b7a3b66d5..ff62abf730d1 100644
--- a/drivers/iio/humidity/Kconfig
+++ b/drivers/iio/humidity/Kconfig
@@ -25,6 +25,17 @@ config DHT11
 	  Other sensors should work as well as long as they speak the
 	  same protocol.
 
+config ENS21X
+	tristate "ENS21X temperature and humidity sensor"
+	depends on I2C
+	help
+	  Say yes here to get support for the ScioSense ENS21X family of
+	  humidity and temperature sensors.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called ens21x.
+
+
 config HDC100X
 	tristate "TI HDC100x relative humidity and temperature sensor"
 	depends on I2C
diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
index 5fbeef299f61..26590d06d11f 100644
--- a/drivers/iio/humidity/Makefile
+++ b/drivers/iio/humidity/Makefile
@@ -5,6 +5,7 @@
 
 obj-$(CONFIG_AM2315) += am2315.o
 obj-$(CONFIG_DHT11) += dht11.o
+obj-$(CONFIG_ENS21X) += ens21x.o
 obj-$(CONFIG_HDC100X) += hdc100x.o
 obj-$(CONFIG_HDC2010) += hdc2010.o
 obj-$(CONFIG_HDC3020) += hdc3020.o
diff --git a/drivers/iio/humidity/ens21x.c b/drivers/iio/humidity/ens21x.c
new file mode 100644
index 000000000000..7b2e279d1559
--- /dev/null
+++ b/drivers/iio/humidity/ens21x.c
@@ -0,0 +1,346 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ens21x.c - Support for ScioSense ens21x
+ *           temperature & humidity sensor
+ *
+ * (7-bit I2C slave address 0x43 ENS210)
+ * (7-bit I2C slave address 0x43 ENS210A)
+ * (7-bit I2C slave address 0x44 ENS211)
+ * (7-bit I2C slave address 0x45 ENS212)
+ * (7-bit I2C slave address 0x46 ENS213A)
+ * (7-bit I2C slave address 0x47 ENS215)
+ *
+ * Datasheet:
+ *  https://www.sciosense.com/wp-content/uploads/2024/04/ENS21x-Datasheet.pdf
+ *  https://www.sciosense.com/wp-content/uploads/2023/12/ENS210-Datasheet.pdf
+ */
+
+#include <linux/types.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/crc7.h>
+
+/* register definitions */
+#define ENS21X_REG_PART_ID		0x00
+#define ENS21X_REG_DIE_REV		0x02
+#define ENS21X_REG_UID			0x04
+#define ENS21X_REG_SYS_CTRL		0x10
+#define ENS21X_REG_SYS_STAT		0x11
+#define ENS21X_REG_SENS_RUN		0x21
+#define ENS21X_REG_SENS_START		0x22
+#define ENS21X_REG_SENS_STOP		0x23
+#define ENS21X_REG_SENS_STAT		0x24
+#define ENS21X_REG_T_VAL		0x30
+#define ENS21X_REG_H_VAL		0x33
+
+/* value definitions */
+#define ENS21X_SENS_START_T_START		BIT(0)
+#define ENS21X_SENS_START_H_START		BIT(1)
+
+#define ENS21X_SENS_STAT_T_ACTIVE		BIT(0)
+#define ENS21X_SENS_STAT_H_ACTIVE		BIT(1)
+
+#define ENS21X_SYS_CTRL_LOW_POWER_ENABLE	BIT(0)
+#define ENS21X_SYS_CTRL_SYS_RESET		BIT(7)
+
+#define ENS21X_SYS_STAT_SYS_ACTIVE		BIT(0)
+
+/* magic constants */
+#define ENS21X_CONST_TEMP_SCALE_INT 15 /* integer part of temperature scale (1/64) */
+#define ENS21X_CONST_TEMP_SCALE_DEC 625000 /* decimal part of temperature scale */
+#define ENS21X_CONST_HUM_SCALE_INT 1 /* integer part of humidity scale (1/512) */
+#define ENS21X_CONST_HUM_SCALE_DEC 953125 /* decimal part of humidity scale */
+#define ENS21X_CONST_TEMP_OFFSET_INT -17481 /* temperature offset (64 * -273.15) */
+#define ENS21X_CONST_TEMP_OFFSET_DEC 600000 /* decimal part of offset */
+#define ENS210_CONST_CONVERSION_TIME 130
+#define ENS212_CONST_CONVERSION_TIME 32
+#define ENS215_CONST_CONVERSION_TIME 132
+
+static const struct of_device_id ens21x_of_match[];
+
+struct ens21x_dev {
+	struct i2c_client *client;
+	struct mutex lock;
+	int part_id;
+};
+
+enum ens21x_partnumber {
+	ENS210	= 0x0210,
+	ENS210A	= 0xa210,
+	ENS211	= 0x0211,
+	ENS212	= 0x0212,
+	ENS213A	= 0xa213,
+	ENS215	= 0x0215,
+};
+
+/* calculate 17-bit crc7 */
+static u8 ens21x_crc7(u32 val)
+{
+	u32 val_be = (htonl(val & 0x1ffff) >> 0x8);
+
+	return crc7_be(0xde, (u8 *)&val_be, 3) >> 1;
+}
+
+static int ens21x_get_measurement(struct iio_dev *indio_dev, bool temp, int *val)
+{
+	u32 regval, regval_le;
+	int ret, tries;
+	struct ens21x_dev *dev_data = iio_priv(indio_dev);
+
+	/* assert read */
+	i2c_smbus_write_byte_data(dev_data->client, ENS21X_REG_SENS_START,
+				  temp ? ENS21X_SENS_START_T_START :
+					 ENS21X_SENS_START_H_START);
+
+	/* wait for conversion to be ready */
+	switch (dev_data->part_id) {
+	case ENS210:
+	case ENS210A:
+		msleep(ENS210_CONST_CONVERSION_TIME);
+		break;
+	case ENS211:
+	case ENS212:
+		msleep(ENS212_CONST_CONVERSION_TIME);
+		break;
+	case ENS213A:
+	case ENS215:
+		msleep(ENS215_CONST_CONVERSION_TIME);
+		break;
+	default:
+		dev_err(&dev_data->client->dev, "unrecognised device");
+		return -ENODEV;
+	}
+
+	tries = 10;
+	while (tries-- > 0) {
+		usleep_range(4000, 5000);
+		ret = i2c_smbus_read_byte_data(dev_data->client,
+					       ENS21X_REG_SENS_STAT);
+		if (ret < 0)
+			continue;
+		if (!(ret & (temp ? ENS21X_SENS_STAT_T_ACTIVE :
+				    ENS21X_SENS_STAT_H_ACTIVE)))
+			break;
+	}
+	if (tries < 0) {
+		dev_err(&indio_dev->dev, "timeout waiting for sensor reading\n");
+		return -EIO;
+	}
+
+	/* perform read */
+	ret = i2c_smbus_read_i2c_block_data(
+		dev_data->client, temp ? ENS21X_REG_T_VAL : ENS21X_REG_H_VAL, 3,
+		(u8 *)&regval_le);
+	if (ret < 0) {
+		dev_err(&dev_data->client->dev, "failed to read register");
+		return -EIO;
+	} else if (ret == 3) {
+		regval = le32_to_cpu(regval_le);
+		if (ens21x_crc7(regval) == ((regval >> 17) & 0x7f)) {
+			*val = regval & 0xffff;
+			return IIO_VAL_INT;
+		}
+		/* crc fail */
+		dev_err(&indio_dev->dev, "ens invalid crc\n");
+		return -EIO;
+	}
+
+	dev_err(&indio_dev->dev, "expected 3 bytes, received %d\n", ret);
+	return -EIO;
+}
+
+static int ens21x_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *channel, int *val,
+			   int *val2, long mask)
+{
+	struct ens21x_dev *dev_data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&dev_data->lock);
+		ret = ens21x_get_measurement(
+			indio_dev, channel->type == IIO_TEMP, val);
+		mutex_unlock(&dev_data->lock);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		if (channel->type == IIO_TEMP) {
+			*val = ENS21X_CONST_TEMP_SCALE_INT;
+			*val2 = ENS21X_CONST_TEMP_SCALE_DEC;
+		} else {
+			*val = ENS21X_CONST_HUM_SCALE_INT;
+			*val2 = ENS21X_CONST_HUM_SCALE_DEC;
+		}
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	case IIO_CHAN_INFO_OFFSET:
+		if (channel->type == IIO_TEMP) {
+			*val = ENS21X_CONST_TEMP_OFFSET_INT;
+			*val2 = ENS21X_CONST_TEMP_OFFSET_DEC;
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			break;
+		}
+		*val = 0;
+		ret =  IIO_VAL_INT;
+		break;
+	default:
+		break;
+	}
+	return ret;
+}
+
+static const struct iio_chan_spec ens21x_channels[] = {
+	/* Temperature channel */
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+	},
+	/* Humidity channel */
+	{
+		.type = IIO_HUMIDITYRELATIVE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+	}
+};
+
+static const struct iio_info ens21x_info = {
+	.read_raw = ens21x_read_raw,
+};
+
+static int ens21x_probe(struct i2c_client *client)
+{
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
+	const struct of_device_id *match;
+	struct ens21x_dev *dev_data;
+	struct iio_dev *indio_dev;
+	uint16_t part_id_le, part_id;
+	int ret, tries;
+
+	if (!i2c_check_functionality(client->adapter,
+			I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+			I2C_FUNC_SMBUS_WRITE_BYTE |
+			I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+		dev_err(&client->dev,
+			"adapter does not support some i2c transactions\n");
+		return -EOPNOTSUPP;
+	}
+
+	match = i2c_of_match_device(ens21x_of_match, client);
+	if (!match)
+		return -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*dev_data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	dev_data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	dev_data->client = client;
+	mutex_init(&dev_data->lock);
+
+	/* reset device */
+	ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL,
+					ENS21X_SYS_CTRL_SYS_RESET);
+	if (ret)
+		return ret;
+
+	/* wait for device to become active */
+	usleep_range(4000, 5000);
+
+	/* disable low power mode */
+	ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, 0x00);
+	if (ret)
+		return ret;
+
+	/* wait for device to become active */
+	tries = 10;
+	while (tries-- > 0) {
+		msleep(20);
+		ret = i2c_smbus_read_byte_data(client, ENS21X_REG_SYS_STAT);
+		if (ret < 0)
+			return ret;
+		if (ret & ENS21X_SYS_STAT_SYS_ACTIVE)
+			break;
+	}
+	if (tries < 0) {
+		dev_err(&client->dev,
+			"timeout waiting for ens21x to become active\n");
+		return -EIO;
+	}
+
+	/* get part_id */
+	part_id_le = i2c_smbus_read_word_data(client, ENS21X_REG_PART_ID);
+	if (part_id_le < 0)
+		return part_id_le;
+	part_id = le16_to_cpu(part_id_le);
+
+	if (part_id != id->driver_data) {
+		dev_err(&client->dev,
+			"Part ID does not match (0x%04x != 0x%04lx)\n", part_id,
+			id->driver_data);
+		return -ENODEV;
+	}
+
+	/* reenable low power */
+	ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL,
+					ENS21X_SYS_CTRL_LOW_POWER_ENABLE);
+	if (ret)
+		return ret;
+
+	dev_data->part_id = part_id;
+
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ens21x_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ens21x_channels);
+	indio_dev->info = &ens21x_info;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+
+static const struct of_device_id ens21x_of_match[] = {
+	{ .compatible = "sciosense,ens210", .data = (void *)ENS210},
+	{ .compatible = "sciosense,ens210a", .data = (void *)ENS210A },
+	{ .compatible = "sciosense,ens211", .data = (void *)ENS211},
+	{ .compatible = "sciosense,ens212", .data = (void *)ENS212},
+	{ .compatible = "sciosense,ens213a", .data = (void *)ENS213A },
+	{ .compatible = "sciosense,ens215", .data = (void *)ENS215},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ens21x_of_match);
+
+static const struct i2c_device_id ens21x_id[] = {
+	{"ens210", ENS210},
+	{"ens210a", ENS210A},
+	{"ens211", ENS211},
+	{"ens212", ENS212},
+	{"ens213a", ENS213A},
+	{"ens215", ENS215},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ens21x_id);
+
+static struct i2c_driver ens21x_driver = {
+	.probe = ens21x_probe,
+	.id_table = ens21x_id,
+	.driver = {
+		.name = "ens21x",
+		.of_match_table = ens21x_of_match,
+	},
+};
+
+module_i2c_driver(ens21x_driver);
+
+MODULE_DESCRIPTION("ScioSense ENS21x temperature and humidity sensor driver");
+MODULE_AUTHOR("Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>");
+MODULE_LICENSE("GPL");
+

-- 
2.39.2

Re: [PATCH v3 1/2] dt-bindings: iio: humidity: add ENS21x sensor family

[Rob Herring (Arm)]

On Wed, 10 Jul 2024 14:24:04 +0100, Joshua Felmeden wrote:
> Add device tree documentation for ENS21x family of temperature and
> humidity sensors
> 
> Signed-off-by: Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>
> ---
>  .../bindings/iio/humidity/sciosense,ens21x.yaml    | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v3 2/2] iio: humidity: Add support for ENS21x

[kernel test robot]

Hi Joshua,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 1ebab783647a9e3bf357002d5c4ff060c8474a0a]

url:    https://github.com/intel-lab-lkp/linux/commits/Joshua-Felmeden/dt-bindings-iio-humidity-add-ENS21x-sensor-family/20240711-022826
base:   1ebab783647a9e3bf357002d5c4ff060c8474a0a
patch link:    https://lore.kernel.org/r/20240710-ens21x-v3-2-4e3fbcf2a7fb%40thegoodpenguin.co.uk
patch subject: [PATCH v3 2/2] iio: humidity: Add support for ENS21x
config: loongarch-randconfig-r122-20240712 (https://download.01.org/0day-ci/archive/20240712/202407121921.LHla7p7i-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20240712/202407121921.LHla7p7i-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/202407121921.LHla7p7i-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/humidity/ens21x.c:84:23: sparse: sparse: restricted __be32 degrades to integer
>> drivers/iio/humidity/ens21x.c:143:26: sparse: sparse: cast to restricted __le32
>> drivers/iio/humidity/ens21x.c:283:19: sparse: sparse: cast to restricted __le16

vim +84 drivers/iio/humidity/ens21x.c

    80	
    81	/* calculate 17-bit crc7 */
    82	static u8 ens21x_crc7(u32 val)
    83	{
  > 84		u32 val_be = (htonl(val & 0x1ffff) >> 0x8);
    85	
    86		return crc7_be(0xde, (u8 *)&val_be, 3) >> 1;
    87	}
    88	
    89	static int ens21x_get_measurement(struct iio_dev *indio_dev, bool temp, int *val)
    90	{
    91		u32 regval, regval_le;
    92		int ret, tries;
    93		struct ens21x_dev *dev_data = iio_priv(indio_dev);
    94	
    95		/* assert read */
    96		i2c_smbus_write_byte_data(dev_data->client, ENS21X_REG_SENS_START,
    97					  temp ? ENS21X_SENS_START_T_START :
    98						 ENS21X_SENS_START_H_START);
    99	
   100		/* wait for conversion to be ready */
   101		switch (dev_data->part_id) {
   102		case ENS210:
   103		case ENS210A:
   104			msleep(ENS210_CONST_CONVERSION_TIME);
   105			break;
   106		case ENS211:
   107		case ENS212:
   108			msleep(ENS212_CONST_CONVERSION_TIME);
   109			break;
   110		case ENS213A:
   111		case ENS215:
   112			msleep(ENS215_CONST_CONVERSION_TIME);
   113			break;
   114		default:
   115			dev_err(&dev_data->client->dev, "unrecognised device");
   116			return -ENODEV;
   117		}
   118	
   119		tries = 10;
   120		while (tries-- > 0) {
   121			usleep_range(4000, 5000);
   122			ret = i2c_smbus_read_byte_data(dev_data->client,
   123						       ENS21X_REG_SENS_STAT);
   124			if (ret < 0)
   125				continue;
   126			if (!(ret & (temp ? ENS21X_SENS_STAT_T_ACTIVE :
   127					    ENS21X_SENS_STAT_H_ACTIVE)))
   128				break;
   129		}
   130		if (tries < 0) {
   131			dev_err(&indio_dev->dev, "timeout waiting for sensor reading\n");
   132			return -EIO;
   133		}
   134	
   135		/* perform read */
   136		ret = i2c_smbus_read_i2c_block_data(
   137			dev_data->client, temp ? ENS21X_REG_T_VAL : ENS21X_REG_H_VAL, 3,
   138			(u8 *)&regval_le);
   139		if (ret < 0) {
   140			dev_err(&dev_data->client->dev, "failed to read register");
   141			return -EIO;
   142		} else if (ret == 3) {
 > 143			regval = le32_to_cpu(regval_le);
   144			if (ens21x_crc7(regval) == ((regval >> 17) & 0x7f)) {
   145				*val = regval & 0xffff;
   146				return IIO_VAL_INT;
   147			}
   148			/* crc fail */
   149			dev_err(&indio_dev->dev, "ens invalid crc\n");
   150			return -EIO;
   151		}
   152	
   153		dev_err(&indio_dev->dev, "expected 3 bytes, received %d\n", ret);
   154		return -EIO;
   155	}
   156	
   157	static int ens21x_read_raw(struct iio_dev *indio_dev,
   158				   struct iio_chan_spec const *channel, int *val,
   159				   int *val2, long mask)
   160	{
   161		struct ens21x_dev *dev_data = iio_priv(indio_dev);
   162		int ret = -EINVAL;
   163	
   164		switch (mask) {
   165		case IIO_CHAN_INFO_RAW:
   166			mutex_lock(&dev_data->lock);
   167			ret = ens21x_get_measurement(
   168				indio_dev, channel->type == IIO_TEMP, val);
   169			mutex_unlock(&dev_data->lock);
   170			break;
   171		case IIO_CHAN_INFO_SCALE:
   172			if (channel->type == IIO_TEMP) {
   173				*val = ENS21X_CONST_TEMP_SCALE_INT;
   174				*val2 = ENS21X_CONST_TEMP_SCALE_DEC;
   175			} else {
   176				*val = ENS21X_CONST_HUM_SCALE_INT;
   177				*val2 = ENS21X_CONST_HUM_SCALE_DEC;
   178			}
   179			ret = IIO_VAL_INT_PLUS_MICRO;
   180			break;
   181		case IIO_CHAN_INFO_OFFSET:
   182			if (channel->type == IIO_TEMP) {
   183				*val = ENS21X_CONST_TEMP_OFFSET_INT;
   184				*val2 = ENS21X_CONST_TEMP_OFFSET_DEC;
   185				ret = IIO_VAL_INT_PLUS_MICRO;
   186				break;
   187			}
   188			*val = 0;
   189			ret =  IIO_VAL_INT;
   190			break;
   191		default:
   192			break;
   193		}
   194		return ret;
   195	}
   196	
   197	static const struct iio_chan_spec ens21x_channels[] = {
   198		/* Temperature channel */
   199		{
   200			.type = IIO_TEMP,
   201			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
   202					      BIT(IIO_CHAN_INFO_SCALE) |
   203					      BIT(IIO_CHAN_INFO_OFFSET),
   204		},
   205		/* Humidity channel */
   206		{
   207			.type = IIO_HUMIDITYRELATIVE,
   208			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
   209					      BIT(IIO_CHAN_INFO_SCALE) |
   210					      BIT(IIO_CHAN_INFO_OFFSET),
   211		}
   212	};
   213	
   214	static const struct iio_info ens21x_info = {
   215		.read_raw = ens21x_read_raw,
   216	};
   217	
   218	static int ens21x_probe(struct i2c_client *client)
   219	{
   220		const struct i2c_device_id *id = i2c_client_get_device_id(client);
   221		const struct of_device_id *match;
   222		struct ens21x_dev *dev_data;
   223		struct iio_dev *indio_dev;
   224		uint16_t part_id_le, part_id;
   225		int ret, tries;
   226	
   227		if (!i2c_check_functionality(client->adapter,
   228				I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
   229				I2C_FUNC_SMBUS_WRITE_BYTE |
   230				I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
   231			dev_err(&client->dev,
   232				"adapter does not support some i2c transactions\n");
   233			return -EOPNOTSUPP;
   234		}
   235	
   236		match = i2c_of_match_device(ens21x_of_match, client);
   237		if (!match)
   238			return -ENODEV;
   239	
   240		indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*dev_data));
   241		if (!indio_dev)
   242			return -ENOMEM;
   243	
   244		dev_data = iio_priv(indio_dev);
   245		i2c_set_clientdata(client, indio_dev);
   246		dev_data->client = client;
   247		mutex_init(&dev_data->lock);
   248	
   249		/* reset device */
   250		ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL,
   251						ENS21X_SYS_CTRL_SYS_RESET);
   252		if (ret)
   253			return ret;
   254	
   255		/* wait for device to become active */
   256		usleep_range(4000, 5000);
   257	
   258		/* disable low power mode */
   259		ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, 0x00);
   260		if (ret)
   261			return ret;
   262	
   263		/* wait for device to become active */
   264		tries = 10;
   265		while (tries-- > 0) {
   266			msleep(20);
   267			ret = i2c_smbus_read_byte_data(client, ENS21X_REG_SYS_STAT);
   268			if (ret < 0)
   269				return ret;
   270			if (ret & ENS21X_SYS_STAT_SYS_ACTIVE)
   271				break;
   272		}
   273		if (tries < 0) {
   274			dev_err(&client->dev,
   275				"timeout waiting for ens21x to become active\n");
   276			return -EIO;
   277		}
   278	
   279		/* get part_id */
   280		part_id_le = i2c_smbus_read_word_data(client, ENS21X_REG_PART_ID);
   281		if (part_id_le < 0)
   282			return part_id_le;
 > 283		part_id = le16_to_cpu(part_id_le);
   284	
   285		if (part_id != id->driver_data) {
   286			dev_err(&client->dev,
   287				"Part ID does not match (0x%04x != 0x%04lx)\n", part_id,
   288				id->driver_data);
   289			return -ENODEV;
   290		}
   291	
   292		/* reenable low power */
   293		ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL,
   294						ENS21X_SYS_CTRL_LOW_POWER_ENABLE);
   295		if (ret)
   296			return ret;
   297	
   298		dev_data->part_id = part_id;
   299	
   300		indio_dev->name = id->name;
   301		indio_dev->modes = INDIO_DIRECT_MODE;
   302		indio_dev->channels = ens21x_channels;
   303		indio_dev->num_channels = ARRAY_SIZE(ens21x_channels);
   304		indio_dev->info = &ens21x_info;
   305	
   306		return devm_iio_device_register(&client->dev, indio_dev);
   307	}
   308	

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

Re: [PATCH v3 2/2] iio: humidity: Add support for ENS21x

[kernel test robot]

Hi Joshua,

kernel test robot noticed the following build errors:

[auto build test ERROR on 1ebab783647a9e3bf357002d5c4ff060c8474a0a]

url:    https://github.com/intel-lab-lkp/linux/commits/Joshua-Felmeden/dt-bindings-iio-humidity-add-ENS21x-sensor-family/20240711-022826
base:   1ebab783647a9e3bf357002d5c4ff060c8474a0a
patch link:    https://lore.kernel.org/r/20240710-ens21x-v3-2-4e3fbcf2a7fb%40thegoodpenguin.co.uk
patch subject: [PATCH v3 2/2] iio: humidity: Add support for ENS21x
config: sparc64-randconfig-r053-20240712 (https://download.01.org/0day-ci/archive/20240713/202407131554.tOb1HnRA-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240713/202407131554.tOb1HnRA-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/202407131554.tOb1HnRA-lkp@intel.com/

All errors (new ones prefixed by >>):

   sparc64-linux-ld: drivers/iio/humidity/ens21x.o: in function `ens21x_get_measurement':
>> ens21x.c:(.text+0x388): undefined reference to `crc7_be'

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

Re: [PATCH v3 2/2] iio: humidity: Add support for ENS21x

[Christophe JAILLET]

Le 10/07/2024 à 15:24, Joshua Felmeden a écrit :
> Add support for ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215.
> 
> The ENS21x is a family of temperature and relative humidity sensors with
> accuracies tailored to the needs of specific applications.
> 
> Signed-off-by: Joshua Felmeden <jfelmeden-tUaQ5FxYRYX4aQPF92CzsNBc4/FLrbF6@public.gmane.org>
> ---
>   drivers/iio/humidity/Kconfig  |  11 ++
>   drivers/iio/humidity/Makefile |   1 +
>   drivers/iio/humidity/ens21x.c | 346 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 358 insertions(+)

Hi,

as kernel test robot complained, there will be a v4.

So here are a few nitpicks/questions, in case it helps.

...

> +#include <linux/types.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/crc7.h>

Nitpick: usually, it is prefered to keep #include alphabetically ordered.

...

> +
> +/* magic constants */
> +#define ENS21X_CONST_TEMP_SCALE_INT 15 /* integer part of temperature scale (1/64) */
> +#define ENS21X_CONST_TEMP_SCALE_DEC 625000 /* decimal part of temperature scale */
> +#define ENS21X_CONST_HUM_SCALE_INT 1 /* integer part of humidity scale (1/512) */
> +#define ENS21X_CONST_HUM_SCALE_DEC 953125 /* decimal part of humidity scale */
> +#define ENS21X_CONST_TEMP_OFFSET_INT -17481 /* temperature offset (64 * -273.15) */
> +#define ENS21X_CONST_TEMP_OFFSET_DEC 600000 /* decimal part of offset */
> +#define ENS210_CONST_CONVERSION_TIME 130
> +#define ENS212_CONST_CONVERSION_TIME 32
> +#define ENS215_CONST_CONVERSION_TIME 132

Datasheet says 130 for ENS213A and ENS215.
Is it a typo?
If 132 is intentional, maybe a samll comment explaining why would be 
welcomed?

...

> +static int ens21x_get_measurement(struct iio_dev *indio_dev, bool temp, int *val)
> +{
> +	u32 regval, regval_le;
> +	int ret, tries;
> +	struct ens21x_dev *dev_data = iio_priv(indio_dev);
> +
> +	/* assert read */
> +	i2c_smbus_write_byte_data(dev_data->client, ENS21X_REG_SENS_START,
> +				  temp ? ENS21X_SENS_START_T_START :
> +					 ENS21X_SENS_START_H_START);
> +
> +	/* wait for conversion to be ready */
> +	switch (dev_data->part_id) {
> +	case ENS210:
> +	case ENS210A:
> +		msleep(ENS210_CONST_CONVERSION_TIME);
> +		break;
> +	case ENS211:
> +	case ENS212:
> +		msleep(ENS212_CONST_CONVERSION_TIME);
> +		break;
> +	case ENS213A:
> +	case ENS215:
> +		msleep(ENS215_CONST_CONVERSION_TIME);
> +		break;
> +	default:
> +		dev_err(&dev_data->client->dev, "unrecognised device");
> +		return -ENODEV;
> +	}
> +
> +	tries = 10;
> +	while (tries-- > 0) {
> +		usleep_range(4000, 5000);

We just msleep()'ed the max expected time for the conversion. So, maybe 
the code could be re-arranged so that this delay is done only if we retry?

> +		ret = i2c_smbus_read_byte_data(dev_data->client,
> +					       ENS21X_REG_SENS_STAT);
> +		if (ret < 0)
> +			continue;
> +		if (!(ret & (temp ? ENS21X_SENS_STAT_T_ACTIVE :
> +				    ENS21X_SENS_STAT_H_ACTIVE)))
> +			break;
> +	}
> +	if (tries < 0) {
> +		dev_err(&indio_dev->dev, "timeout waiting for sensor reading\n");
> +		return -EIO;
> +	}

...

> +	indio_dev->name = id->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ens21x_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ens21x_channels);
> +	indio_dev->info = &ens21x_info;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +

Nitpick: unneeded 2nd new line.

> +static const struct of_device_id ens21x_of_match[] = {
> +	{ .compatible = "sciosense,ens210", .data = (void *)ENS210},
> +	{ .compatible = "sciosense,ens210a", .data = (void *)ENS210A },
> +	{ .compatible = "sciosense,ens211", .data = (void *)ENS211},

...

CJ

Re: [PATCH v3 1/2] dt-bindings: iio: humidity: add ENS21x sensor family

[Jonathan Cameron]

On Wed, 10 Jul 2024 14:24:04 +0100
Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk> wrote:

> Add device tree documentation for ENS21x family of temperature and
> humidity sensors
> 
> Signed-off-by: Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>
> ---
>  .../bindings/iio/humidity/sciosense,ens21x.yaml    | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/humidity/sciosense,ens21x.yaml b/Documentation/devicetree/bindings/iio/humidity/sciosense,ens21x.yaml
> new file mode 100644
> index 000000000000..425d3b57f701
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/humidity/sciosense,ens21x.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/humidity/sciosense,ens21x.yaml#

Normally we don't allow wild cares in binding names, but in this case the
datasheet uses this wild cards, so I guess we have strong guarantees
the manufacturer won't slip something else in the gaps.

Even with that in mind I'd rather this was sciosense,ens210.yaml

As much as anything as to not provide more precedence for wild cards in binding
names that might lead people astray when they don't have such strong guarantees.

Otherwise looks fine to me.

Jonathan


> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ScioSense ENS21x temperature and humidity sensor
> +
> +maintainers:
> +  - Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>
> +
> +description: |
> +  Temperature and Humidity sensor.
> +
> +  Datasheet:
> +    https://www.sciosense.com/wp-content/uploads/2024/04/ENS21x-Datasheet.pdf
> +    https://www.sciosense.com/wp-content/uploads/2023/12/ENS210-Datasheet.pdf
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - sciosense,ens210a
> +              - sciosense,ens211
> +              - sciosense,ens212
> +              - sciosense,ens213a
> +              - sciosense,ens215
> +          - const: sciosense,ens210
> +      - const: sciosense,ens210
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +
> +       temperature-sensor@43 {
> +           compatible = "sciosense,ens210";
> +           reg = <0x43>;
> +       };
> +    };
> +...
> +
> 

Re: [PATCH v3 2/2] iio: humidity: Add support for ENS21x

[Jonathan Cameron]

On Wed, 10 Jul 2024 14:24:05 +0100
Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk> wrote:

> Add support for ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215.
> 
> The ENS21x is a family of temperature and relative humidity sensors with
> accuracies tailored to the needs of specific applications.
> 
> Signed-off-by: Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>
Hi Joshua,

Various comments inline

Jonathan

> ---
>  drivers/iio/humidity/Kconfig  |  11 ++
>  drivers/iio/humidity/Makefile |   1 +
>  drivers/iio/humidity/ens21x.c | 346 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 358 insertions(+)
> 
> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
> index b15b7a3b66d5..ff62abf730d1 100644
> --- a/drivers/iio/humidity/Kconfig
> +++ b/drivers/iio/humidity/Kconfig
> @@ -25,6 +25,17 @@ config DHT11
>  	  Other sensors should work as well as long as they speak the
>  	  same protocol.
>  
> +config ENS21X
> +	tristate "ENS21X temperature and humidity sensor"
> +	depends on I2C
> +	help
> +	  Say yes here to get support for the ScioSense ENS21X family of
> +	  humidity and temperature sensors.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called ens21x.
> +

Keep to local style, so one blank line only.

> +
>  config HDC100X
>  	tristate "TI HDC100x relative humidity and temperature sensor"
>  	depends on I2C

> diff --git a/drivers/iio/humidity/ens21x.c b/drivers/iio/humidity/ens21x.c
> new file mode 100644
> index 000000000000..7b2e279d1559
> --- /dev/null
> +++ b/drivers/iio/humidity/ens21x.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ens21x.c - Support for ScioSense ens21x
> + *           temperature & humidity sensor

Very short line wrap so odd looking formatting.

> + *
> + * (7-bit I2C slave address 0x43 ENS210)
> + * (7-bit I2C slave address 0x43 ENS210A)
> + * (7-bit I2C slave address 0x44 ENS211)
> + * (7-bit I2C slave address 0x45 ENS212)
> + * (7-bit I2C slave address 0x46 ENS213A)
> + * (7-bit I2C slave address 0x47 ENS215)
> + *
> + * Datasheet:
> + *  https://www.sciosense.com/wp-content/uploads/2024/04/ENS21x-Datasheet.pdf
> + *  https://www.sciosense.com/wp-content/uploads/2023/12/ENS210-Datasheet.pdf
> + */
> +
> +#include <linux/types.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>

Fix the various of stuff below and there won't be anything from either of these
headers but you should include
mod_devicetable.h for the various id tables.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Not seeing this used. It's for custom attributes only.

> +#include <linux/crc7.h>
> +
> +/* register definitions */
> +#define ENS21X_REG_PART_ID		0x00
> +#define ENS21X_REG_DIE_REV		0x02
> +#define ENS21X_REG_UID			0x04
> +#define ENS21X_REG_SYS_CTRL		0x10
> +#define ENS21X_REG_SYS_STAT		0x11
> +#define ENS21X_REG_SENS_RUN		0x21
> +#define ENS21X_REG_SENS_START		0x22
> +#define ENS21X_REG_SENS_STOP		0x23
> +#define ENS21X_REG_SENS_STAT		0x24
> +#define ENS21X_REG_T_VAL		0x30
> +#define ENS21X_REG_H_VAL		0x33
> +
> +/* value definitions */
> +#define ENS21X_SENS_START_T_START		BIT(0)
> +#define ENS21X_SENS_START_H_START		BIT(1)
> +
> +#define ENS21X_SENS_STAT_T_ACTIVE		BIT(0)
> +#define ENS21X_SENS_STAT_H_ACTIVE		BIT(1)
> +
> +#define ENS21X_SYS_CTRL_LOW_POWER_ENABLE	BIT(0)
> +#define ENS21X_SYS_CTRL_SYS_RESET		BIT(7)
> +
> +#define ENS21X_SYS_STAT_SYS_ACTIVE		BIT(0)
> +
> +/* magic constants */
> +#define ENS21X_CONST_TEMP_SCALE_INT 15 /* integer part of temperature scale (1/64) */
> +#define ENS21X_CONST_TEMP_SCALE_DEC 625000 /* decimal part of temperature scale */

These defines aren't magic in any sense! They are the actual values.
As such, just use them inline in the code where they will also
be self documenting (so probably no need for comments unless they are
relating them to maths used to find the values).


> +#define ENS21X_CONST_HUM_SCALE_INT 1 /* integer part of humidity scale (1/512) */
> +#define ENS21X_CONST_HUM_SCALE_DEC 953125 /* decimal part of humidity scale */
> +#define ENS21X_CONST_TEMP_OFFSET_INT -17481 /* temperature offset (64 * -273.15) */
> +#define ENS21X_CONST_TEMP_OFFSET_DEC 600000 /* decimal part of offset */
> +#define ENS210_CONST_CONVERSION_TIME 130

Conversion times are more reasonable things to have defines for if they
turn up in multiple places.  Good if the define tells us the units
though and no need for it to involve CONST.  Anything in capitals is assumed
to be a define.
ENS210_CONVERSION_TIME_MSECS etc


> +#define ENS212_CONST_CONVERSION_TIME 32
> +#define ENS215_CONST_CONVERSION_TIME 132
> +
> +static const struct of_device_id ens21x_of_match[];

This won't be needed after changes suggested below.

> +
> +struct ens21x_dev {
> +	struct i2c_client *client;
> +	struct mutex lock;
All locks need documentation of what data they are protecting.

> +	int part_id;

I'd expect to see this embedded in the chip_info structure suggested below.

> +};
> +
> +enum ens21x_partnumber {
> +	ENS210	= 0x0210,
> +	ENS210A	= 0xa210,
> +	ENS211	= 0x0211,
> +	ENS212	= 0x0212,
> +	ENS213A	= 0xa213,
> +	ENS215	= 0x0215,
> +};
> +
> +/* calculate 17-bit crc7 */
> +static u8 ens21x_crc7(u32 val)
> +{
> +	u32 val_be = (htonl(val & 0x1ffff) >> 0x8);
Long time since I've seen htonl in kernel code.

	__be32 val_be = cpu_to_be32(val);

I'm suspicious though. You take a le24 below, convert
that to CPU then to be24. That's just always byte
swapping.  If you need to flip the byte order, just do
that.


> +
> +	return crc7_be(0xde, (u8 *)&val_be, 3) >> 1;
> +}
> +
> +static int ens21x_get_measurement(struct iio_dev *indio_dev, bool temp, int *val)
> +{
> +	u32 regval, regval_le;
> +	int ret, tries;
> +	struct ens21x_dev *dev_data = iio_priv(indio_dev);

dev_data tends to suggest output dev_get_drvdata() or similar.
More common in IIO to name it after the part so ens210_data
or similar.


> +
> +	/* assert read */
> +	i2c_smbus_write_byte_data(dev_data->client, ENS21X_REG_SENS_START,
> +				  temp ? ENS21X_SENS_START_T_START :
> +					 ENS21X_SENS_START_H_START);

Check return values from all i2c calls.  If it's expected to return an
error (that happens for some corner cases) then document why we ignore it.


> +
> +	/* wait for conversion to be ready */
> +	switch (dev_data->part_id) {

This per chip type data belongs in a chip_info structure (see below)
so that this just ebcomes

	msleep(dev_data->chip_info.conv_time_msec);

or something like that. It is almost always preferable to make this
sort of stuff picking between data, rather than inline code as
it puts all the per device stuff in one place rather than scattered
throughout.


> +	case ENS210:
> +	case ENS210A:
> +		msleep(ENS210_CONST_CONVERSION_TIME);
> +		break;
> +	case ENS211:
> +	case ENS212:
> +		msleep(ENS212_CONST_CONVERSION_TIME);
> +		break;
> +	case ENS213A:
> +	case ENS215:
> +		msleep(ENS215_CONST_CONVERSION_TIME);
> +		break;
> +	default:
> +		dev_err(&dev_data->client->dev, "unrecognised device");
> +		return -ENODEV;
> +	}
> +
> +	tries = 10;
> +	while (tries-- > 0) {

A retry loop like this needs a comment on why it's needed and
why this particular number of retries.

> +		usleep_range(4000, 5000);
> +		ret = i2c_smbus_read_byte_data(dev_data->client,
> +					       ENS21X_REG_SENS_STAT);
> +		if (ret < 0)
> +			continue;
Error expected here?  That's nasty if true that the device stops
talking i2c right when capturing data.  If not, return an error,
not carry on, as it's a sign the comms is broken.

> +		if (!(ret & (temp ? ENS21X_SENS_STAT_T_ACTIVE :
> +				    ENS21X_SENS_STAT_H_ACTIVE)))
> +			break;
> +	}
> +	if (tries < 0) {
> +		dev_err(&indio_dev->dev, "timeout waiting for sensor reading\n");
> +		return -EIO;
> +	}
> +
> +	/* perform read */
> +	ret = i2c_smbus_read_i2c_block_data(
> +		dev_data->client, temp ? ENS21X_REG_T_VAL : ENS21X_REG_H_VAL, 3,
> +		(u8 *)&regval_le);

If it is 3 bytes, read into a u8[3] and use get_unaligned_le24()

> +	if (ret < 0) {
> +		dev_err(&dev_data->client->dev, "failed to read register");
> +		return -EIO;
> +	} else if (ret == 3) {
> +		regval = le32_to_cpu(regval_le);
> +		if (ens21x_crc7(regval) == ((regval >> 17) & 0x7f)) {

pull that bytes from the u8[3] mentioned above

> +			*val = regval & 0xffff;
> +			return IIO_VAL_INT;
Always prefer the good path to be the inline one for ease of code reading..
So flip the logic.
Generally we keep the IIO return values to the boundary code with
the IIO core, so I'd return 0 here and check for errors at the claller
before returning IIO_VAL_INT.  That way we don't need to check all callers
of this function handling the unusual return code right.

> +		}get_measurement
> +		/* crc fail */
> +		dev_err(&indio_dev->dev, "ens invalid crc\n");
> +		return -EIO;
> +	}
> +
> +	dev_err(&indio_dev->dev, "expected 3 bytes, received %d\n", ret);
Check that before the crc.  As then you can reduce indent of the more
complex code.

> +	return -EIO;
> +}
> +
> +static int ens21x_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *channel, int *val,
> +			   int *val2, long mask)
> +{
> +	struct ens21x_dev *dev_data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&dev_data->lock);

		scoped_guard(mutex)(&dev_data->lock) {
			ret = ens21x_get_measurement();
			if (ret)
				return ret;

			return IIO_VAL_INT;

		}	

> +		ret = ens21x_get_measurement(
> +			indio_dev, channel->type == IIO_TEMP, val);
> +		mutex_unlock(&dev_data->lock);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (channel->type == IIO_TEMP) {
> +			*val = ENS21X_CONST_TEMP_SCALE_INT;
> +			*val2 = ENS21X_CONST_TEMP_SCALE_DEC;
> +		} else {
> +			*val = ENS21X_CONST_HUM_SCALE_INT;
> +			*val2 = ENS21X_CONST_HUM_SCALE_DEC;
> +		}
> +		ret = IIO_VAL_INT_PLUS_MICRO;
		return IIO_VAL_INT_PLUS_MICRO;

> +		break;
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (channel->type == IIO_TEMP) {
> +			*val = ENS21X_CONST_TEMP_OFFSET_INT;
> +			*val2 = ENS21X_CONST_TEMP_OFFSET_DEC;
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +			break;
> +		}
> +		*val = 0;
> +		ret =  IIO_VAL_INT;
Odd spacing + 
		return IIO_VAL_INT;
> +		break;
> +	default:
> +		break;
		return -EINVAL;
> +	}
> +	return ret;
get rid of this as after changes above this is unreachable.

For code readability reasons, early returns are good.
Someone interested in a given path has to look at the minimal amount
of code rather than having to chase through to a much later return
with nothing actually done on the way there.

> +}
> +
> +static const struct iio_chan_spec ens21x_channels[] = {
> +	/* Temperature channel */

Obvious from .type, so I'd drop the comments.

> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +	},
> +	/* Humidity channel */
> +	{
> +		.type = IIO_HUMIDITYRELATIVE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +	}
> +};
> +
> +static const struct iio_info ens21x_info = {
> +	.read_raw = ens21x_read_raw,
> +};
> +
> +static int ens21x_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	const struct of_device_id *match;
> +	struct ens21x_dev *dev_data;
> +	struct iio_dev *indio_dev;
> +	uint16_t part_id_le, part_id;
> +	int ret, tries;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +			I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> +			I2C_FUNC_SMBUS_WRITE_BYTE |
> +			I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		dev_err(&client->dev,
> +			"adapter does not support some i2c transactions\n");
> +		return -EOPNOTSUPP;
in probe, nicer to use dev_err_probe() throughout. Doesn't matter much in this
case, but good for consistency with where it might.

		return dev_err_probe(&client->dev, -EOPNOTSUPP,
				     "adapter does not...
> +	}
> +
> +	match = i2c_of_match_device(ens21x_of_match, client);
> +	if (!match)
> +		return -ENODEV;
Why?

I suspect what you actually want is i2c_get_match_data(client);
That gets the data for all supported firmware types including DT.



> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*dev_data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	dev_data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	dev_data->client = client;
> +	mutex_init(&dev_data->lock);
> +
turn the power on?
	ret = devm_regulator_get_enabled() or similar

> +	/* reset device */
> +	ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL,
> +					ENS21X_SYS_CTRL_SYS_RESET);
> +	if (ret)
> +		return ret;
> +
> +	/* wait for device to become active */
> +	usleep_range(4000, 5000);
> +
> +	/* disable low power mode */
> +	ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, 0x00);
> +	if (ret)
> +		return ret;
> +
> +	/* wait for device to become active */
Why this long?  Reference some power up delay in the datahseet?
> +	tries = 10;
> +	while (tries-- > 0) {
> +		msleep(20);
> +		ret = i2c_smbus_read_byte_data(client, ENS21X_REG_SYS_STAT);
> +		if (ret < 0)
> +			return ret;
> +		if (ret & ENS21X_SYS_STAT_SYS_ACTIVE)
> +			break;
> +	}
> +	if (tries < 0) {
> +		dev_err(&client->dev,
> +			"timeout waiting for ens21x to become active\n");
> +		return -EIO;
> +	}
> +
> +	/* get part_id */
> +	part_id_le = i2c_smbus_read_word_data(client, ENS21X_REG_PART_ID);


> +	if (part_id_le < 0)
> +		return part_id_le;
> +	part_id = le16_to_cpu(part_id_le);

Smbus has a byte order and the values returned from i2c_smbus_read_word_data()
are already cpu endian.  So this shouldn't be needed.
(that's the reason we have i2c_smbus_read_word_swapped() for when it is in
reverse order of the smbus spec - happens annoyingly commonly).

> +
> +	if (part_id != id->driver_data) {
> +		dev_err(&client->dev,
> +			"Part ID does not match (0x%04x != 0x%04lx)\n", part_id,

dev_info() and no error return only.
This hard check is something we used to do, but DT maintainers pointed out a while
back that it breaks the use of fallback compatibles to allow old drivers to work
out of the box with new hardware.   It is useful to print a message though saying
we've seen something unexpected.

> +			id->driver_data);
> +		return -ENODEV;
> +	}
> +
> +	/* reenable low power */
> +	ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL,
> +					ENS21X_SYS_CTRL_LOW_POWER_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	dev_data->part_id = part_id;
> +
> +	indio_dev->name = id->name;

This tends to be fragile as we add other firmware types etc.  So
I'd prefer to see the name inside the chip_info structure suggested below.

> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ens21x_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ens21x_channels);
> +	indio_dev->info = &ens21x_info;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +
> +static const struct of_device_id ens21x_of_match[] = {
> +	{ .compatible = "sciosense,ens210", .data = (void *)ENS210},

Putting enums into the data fields has a habit of causing bugs.
You've avoided the most common one which is a 0 value, but still
I'd prefer these to be a pointer to a ens210_chip_info structure.

> +	{ .compatible = "sciosense,ens210a", .data = (void *)ENS210A },
> +	{ .compatible = "sciosense,ens211", .data = (void *)ENS211},
> +	{ .compatible = "sciosense,ens212", .data = (void *)ENS212},
> +	{ .compatible = "sciosense,ens213a", .data = (void *)ENS213A },
> +	{ .compatible = "sciosense,ens215", .data = (void *)ENS215},
> +	{},
No comma after a 'terminator' like this as we don't want it to be easy
for people to put something here.

> +};
> +MODULE_DEVICE_TABLE(of, ens21x_of_match);
> +
> +static const struct i2c_device_id ens21x_id[] = {
> +	{"ens210", ENS210},
> +	{"ens210a", ENS210A},
> +	{"ens211", ENS211},
> +	{"ens212", ENS212},
> +	{"ens213a", ENS213A},
> +	{"ens215", ENS215},
> +	{}
Consistent spacing needed so after { and before } in all cases like htis.

> +};
> +MODULE_DEVICE_TABLE(i2c, ens21x_id);
> +
> +static struct i2c_driver ens21x_driver = {
> +	.probe = ens21x_probe,
> +	.id_table = ens21x_id,
> +	.driver = {
> +		.name = "ens21x",
> +		.of_match_table = ens21x_of_match,
> +	},
> +};
> +
> +module_i2c_driver(ens21x_driver);
> +
> +MODULE_DESCRIPTION("ScioSense ENS21x temperature and humidity sensor driver");
> +MODULE_AUTHOR("Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>");
> +MODULE_LICENSE("GPL");
> +

Unless I'm reading the diff wrong looks like a spare blank line at the end.
Trivial but delete that.

>