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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
---
  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





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux