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