Message-ID: <i6heylzrq3kez23oq3brbbhpw4ktt6umzh2vcsawch52upv4oa@muxsuackv6er>
Date: Wed, 22 May 2024 20:41:18 -0300
From: Gustavo Silva <gustavograzs@...il.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: jic23@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, lars@...afoo.de, gerald.loacker@...fvision.net, 
	devicetree@...r.kernel.org, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor

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@...lic.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);
> > +}
> 
> 

Powered by blists - more mailing lists