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