| Message ID | 20240512210444.30824-4-gustavograzs@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add driver for ENS160 sensor | expand |
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@public.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. ... > +#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 > +#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? > + > + 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() > + 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() > + 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() CJ > + return PTR_ERR(regmap); > + } > + > + return ens160_core_probe(&spi->dev, regmap, id->name); > +}
On Mon, 13 May 2024 21:12:55 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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@public.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. Now we have evidence you'll review if CC'd, wait for the deluge :)
On Sun, 12 May 2024 18:04:39 -0300 Gustavo Silva <gustavograzs@gmail.com> wrote: > 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@gmail.com> Hi Gustavo, Nice driver in general. 2 things that need fixing though. - DMA safe buffers for the bulk accesses etc. This is esoteric and won't actually cause you any problems today (and depending on your host may never do so) but I'm trying to keep the IIO drivers inline with the rule that if they do SPI even via regmap and bulk accesses they need to use DMA safe buffers - devm vs non-devm cleanup. If you mix these it has to be a clean transition. So devm for first N and non devm for everything from N+1. They cannot be mixed safely. Easiest option is devm_ for everything. Jonathan > diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c > new file mode 100644 > index 000000000..25593420d > --- /dev/null > +++ b/drivers/iio/chemical/ens160_core.c > @@ -0,0 +1,227 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * ScioSense ENS160 multi-gas sensor driver > + * > + * Copyright (c) 2024 Gustavo Silva <gustavograzs@gmail.com> > + * > + * Data sheet: > + * https://www.sciosense.com/wp-content/uploads/2023/12/ENS160-Datasheet.pdf > + * Trivial, but this blank line adds nothing so drop it. > + */ > + > +static int ens160_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ens160_data *data = iio_priv(indio_dev); > + __le16 buf; > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = regmap_bulk_read(data->regmap, chan->address, > + &buf, sizeof(buf)); As below, should use a DMA safe buffer. > + if (ret) > + return ret; > + *val = le16_to_cpu(buf); > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + switch (chan->channel2) { > + case IIO_MOD_CO2: > + /* The sensor reads CO2 data as ppm */ > + *val = 0; > + *val2 = 100; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_MOD_VOC: > + /* The sensor reads VOC data as ppb */ > + *val = 0; > + *val2 = 100; > + return IIO_VAL_INT_PLUS_NANO; > + } default: return -EINVAL; and drop the one below. > + } > + > + return -EINVAL; > +} > + > +static int ens160_set_mode(struct ens160_data *data, u8 mode) > +{ > + int ret; > + > + ret = regmap_write(data->regmap, ENS160_REG_OPMODE, mode); > + if (ret) > + return ret; > + > + msleep(ENS160_BOOTING_TIME_MS); > + > + return 0; > +} > + > +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; No docs that I can see on what this means for access to registers etc. Good to add a comment if you have info on this. > + > + ret = regmap_bulk_read(data->regmap, ENS160_REG_PART_ID, &part_id, > + sizeof(part_id)); Ah. So this is a fun corner case. Currently regmap makes not guarantees to always bounce buffer things (though last time I checked it actually did do so - there are optimisations that may make sense where it will again not do so). So given we have an SPI bus involved, we should ensure that only DMA safe buffers are used. These need to ensure that no other data that might be changed concurrently with DMA is in the same IIO_DMA_MINALIGN of aligned data (traditionally a cacheline but it gets more complex in some systems and is less in others). Upshot is that if you are are doing bulk accesses you need to use a buffer that is either on the heap (kzalloc etc) or carefully placed at the end of the iio_priv() structure marked __align(IIO_DMA_MINALIGN). Lots of examples of that in tree. If you are curious, wolfram did a good talk on the i2c equivalent of this a few years back. https://www.youtube.com/watch?v=JDwaMClvV-s I think. > + 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); Why here? Not obviously associated with a boot command? A comment might make this easier to follow. I 'think' it is because this next read is the first time it matters. If so that isn't obvious. Also, there is an existing sleep in the mode set, so I'm not sure why we need another one. > + > + ret = regmap_bulk_read(data->regmap, ENS160_REG_GPR_READ4, > + fw_version, sizeof(fw_version)); The first datasheet that google provided me has this GPR_READ0/GPR_READ1 and only 2 bytes. I hope they have maintained backwards compatibility with that earlier doc! When you do a separate DT binding in v2, make sure to include a link to the datasheet you are using. Also use a Datasheet: tag in this patch to make it easy to find that. I dug a little deeper and found the one on sciosense own website - ah, you do have it in the comments. Add to the commit message and DT binding as well. > + if (ret) > + return ret; > + > + msleep(ENS160_BOOTING_TIME_MS); Why again? > + > + dev_info(dev, "firmware version: %u.%u.%u\n", fw_version[2], > + fw_version[1], fw_version[0]); Can definitely do this before the sleep above. > + > + 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; > + > + 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); After you've moved to devm_add_action_or_reset() for the unwind of ens160_chip_init() you probably don't need to set the drvdata. > + 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"); > + return ret; return dev_err_probe(); > + } > + > + return devm_iio_device_register(dev, indio_dev); > +} > +EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160); > + > +void ens160_core_remove(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct ens160_data *data = iio_priv(indio_dev); > + > + ens160_set_mode(data, ENS160_REG_MODE_IDLE); This looks to be mixing devm and manual cleanup. My guess is this is the unwind for code in ens160_chip_init() If so that unwind should definitely happen after we unregister the userspace intefaces in the unwind of devm_iio_device_register(). Currently it happens before this. This is an even stronger reason to use devm_add_action_or_reset() for this than tidying up as mentioned below (note I tend to review backwards through patches so my comments may make more sense read that way around). > +} > +EXPORT_SYMBOL_NS(ens160_core_remove, IIO_ENS160); > + > +MODULE_AUTHOR("Gustavo Silva <gustavograzs@gmail.com>"); > +MODULE_DESCRIPTION("ScioSense ENS160 driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/iio/chemical/ens160_i2c.c b/drivers/iio/chemical/ens160_i2c.c > new file mode 100644 > index 000000000..ee2b44184 > --- /dev/null > +++ b/drivers/iio/chemical/ens160_i2c.c > @@ -0,0 +1,68 @@ ... > +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)); > + return PTR_ERR(regmap); > + } > + > + return ens160_core_probe(&client->dev, regmap, client->name); As below, hardcode the name for now. If it matters in future, get it from a chip specific structure that we can look up from whichever firmware table we have matched against. > +} > + > +static void ens160_i2c_remove(struct i2c_client *client) > +{ > + ens160_core_remove(&client->dev); As below, switch to devm_add_action_or_reset() called from devm_ens160_core_probe() to avoid need to have manual remove in here. > +} > + > +static const struct i2c_device_id ens160_i2c_id[] = { > + { "ens160", 0 }, As below - drop the 0. > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, ens160_i2c_id); ... > diff --git a/drivers/iio/chemical/ens160_spi.c b/drivers/iio/chemical/ens160_spi.c > new file mode 100644 > index 000000000..effc4acee > --- /dev/null > +++ b/drivers/iio/chemical/ens160_spi.c ... > +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); > + return PTR_ERR(regmap); > + } > + > + return ens160_core_probe(&spi->dev, regmap, id->name); Hardcode the name here for now. When you support multiple drivers you will want to get it from a chip type specific structure, not id->name because that path gives rather unexpected answers if we are using devicetree fallback compatibles or the lists end up not matching perfectly for some other reason. That fragility means we mostly just use another source for the name and where possible hard code it. > +} > + > +static void ens160_spi_remove(struct spi_device *spi) > +{ > + ens160_core_remove(&spi->dev); Might as well register an automated cleanup, particularly as you can do it in ens160_core_probe() and have it apply to any other buses for free. If you do that, rename that function devm_ens160_core_probe() to make it obvious that is what is going on. Use devm_add_action_or_reset() to register custom cleanup. Then you can drop this remove function entirely. > +} > + > +static const struct of_device_id ens160_spi_of_match[] = { > + { .compatible = "sciosense,ens160" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ens160_spi_of_match); > + > +static const struct spi_device_id ens160_spi_id[] = { > + { "ens160", 0 }, Don't set the 0 as that suggests it matters whereas it doesn't yet. Maybe it will matter when more parts are added to this driver in future - if so introduce it then. As a general best practice, this is almost always a pointer these days anyway rather than an integer. > + { } > +}; > +MODULE_DEVICE_TABLE(spi, ens160_spi_id);
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@public.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); > > +} > >
diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig index 02649ab81..e407afab8 100644 --- a/drivers/iio/chemical/Kconfig +++ b/drivers/iio/chemical/Kconfig @@ -76,6 +76,28 @@ config CCS811 Say Y here to build I2C interface support for the AMS CCS811 VOC (Volatile Organic Compounds) sensor +config ENS160 + tristate "ScioSense ENS160 sensor driver" + depends on (I2C || SPI) + select REGMAP + select ENS160_I2C if I2C + select ENS160_SPI if SPI + help + Say yes here to build support for ScioSense ENS160 multi-gas sensor. + + This driver can also be built as a module. If so, the module for I2C + would be called ens160_i2c and ens160_spi for SPI support. + +config ENS160_I2C + tristate + depends on I2C && ENS160 + select REGMAP_I2C + +config ENS160_SPI + tristate + depends on SPI && ENS160 + select REGMAP_SPI + config IAQCORE tristate "AMS iAQ-Core VOC sensors" depends on I2C diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile index 2f3dee8bb..4866db06b 100644 --- a/drivers/iio/chemical/Makefile +++ b/drivers/iio/chemical/Makefile @@ -11,6 +11,9 @@ obj-$(CONFIG_BME680) += bme680_core.o obj-$(CONFIG_BME680_I2C) += bme680_i2c.o obj-$(CONFIG_BME680_SPI) += bme680_spi.o obj-$(CONFIG_CCS811) += ccs811.o +obj-$(CONFIG_ENS160) += ens160_core.o +obj-$(CONFIG_ENS160_I2C) += ens160_i2c.o +obj-$(CONFIG_ENS160_SPI) += ens160_spi.o obj-$(CONFIG_IAQCORE) += ams-iaq-core.o obj-$(CONFIG_PMS7003) += pms7003.o obj-$(CONFIG_SCD30_CORE) += scd30_core.o diff --git a/drivers/iio/chemical/ens160.h b/drivers/iio/chemical/ens160.h new file mode 100644 index 000000000..3fd079bc4 --- /dev/null +++ b/drivers/iio/chemical/ens160.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef ENS160_H_ +#define ENS160_H_ + +int ens160_core_probe(struct device *dev, struct regmap *regmap, + const char *name); +void ens160_core_remove(struct device *dev); + +#endif diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c new file mode 100644 index 000000000..25593420d --- /dev/null +++ b/drivers/iio/chemical/ens160_core.c @@ -0,0 +1,227 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ScioSense ENS160 multi-gas sensor driver + * + * Copyright (c) 2024 Gustavo Silva <gustavograzs@gmail.com> + * + * Data sheet: + * https://www.sciosense.com/wp-content/uploads/2023/12/ENS160-Datasheet.pdf + * + */ + +#include <linux/bitfield.h> +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/regmap.h> + +#include "ens160.h" + +#define ENS160_PART_ID 0x160 + +#define ENS160_BOOTING_TIME_MS 10U + +#define ENS160_REG_PART_ID 0x00 + +#define ENS160_REG_OPMODE 0x10 + +#define ENS160_REG_MODE_DEEP_SLEEP 0x00 +#define ENS160_REG_MODE_IDLE 0x01 +#define ENS160_REG_MODE_STANDARD 0x02 +#define ENS160_REG_MODE_RESET 0xF0 + +#define ENS160_REG_COMMAND 0x12 +#define ENS160_REG_COMMAND_GET_APPVER 0x0E +#define ENS160_REG_COMMAND_CLRGPR 0xCC + +#define ENS160_REG_TEMP_IN 0x13 +#define ENS160_REG_RH_IN 0x15 +#define ENS160_REG_DEVICE_STATUS 0x20 +#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 + +#define ENS160_STATUS_VALIDITY_FLAG GENMASK(3, 2) + +#define ENS160_STATUS_NORMAL 0x00 + +struct ens160_data { + struct regmap *regmap; +}; + +static const struct iio_chan_spec ens160_channels[] = { + { + .type = IIO_CONCENTRATION, + .channel2 = IIO_MOD_VOC, + .modified = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .address = ENS160_REG_DATA_TVOC, + }, + { + .type = IIO_CONCENTRATION, + .channel2 = IIO_MOD_CO2, + .modified = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .address = ENS160_REG_DATA_ECO2, + }, +}; + +static int ens160_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct ens160_data *data = iio_priv(indio_dev); + __le16 buf; + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = regmap_bulk_read(data->regmap, chan->address, + &buf, sizeof(buf)); + if (ret) + return ret; + *val = le16_to_cpu(buf); + return IIO_VAL_INT; + + case IIO_CHAN_INFO_SCALE: + switch (chan->channel2) { + case IIO_MOD_CO2: + /* The sensor reads CO2 data as ppm */ + *val = 0; + *val2 = 100; + return IIO_VAL_INT_PLUS_MICRO; + case IIO_MOD_VOC: + /* The sensor reads VOC data as ppb */ + *val = 0; + *val2 = 100; + return IIO_VAL_INT_PLUS_NANO; + } + } + + return -EINVAL; +} + +static int ens160_set_mode(struct ens160_data *data, u8 mode) +{ + int ret; + + ret = regmap_write(data->regmap, ENS160_REG_OPMODE, mode); + if (ret) + return ret; + + msleep(ENS160_BOOTING_TIME_MS); + + return 0; +} + +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; + + 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"); + return ret; + } + + return devm_iio_device_register(dev, indio_dev); +} +EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160); + +void ens160_core_remove(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct ens160_data *data = iio_priv(indio_dev); + + ens160_set_mode(data, ENS160_REG_MODE_IDLE); +} +EXPORT_SYMBOL_NS(ens160_core_remove, IIO_ENS160); + +MODULE_AUTHOR("Gustavo Silva <gustavograzs@gmail.com>"); +MODULE_DESCRIPTION("ScioSense ENS160 driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/iio/chemical/ens160_i2c.c b/drivers/iio/chemical/ens160_i2c.c new file mode 100644 index 000000000..ee2b44184 --- /dev/null +++ b/drivers/iio/chemical/ens160_i2c.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ScioSense ENS160 multi-gas sensor I2C driver + * + * Copyright (c) 2024 Gustavo Silva <gustavograzs@gmail.com> + * + * 7-Bit I2C slave address is: + * - 0x52 if ADDR pin LOW + * - 0x53 if ADDR pin HIGH + */ + +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/regmap.h> + +#include "ens160.h" + +static const struct regmap_config ens160_regmap_i2c_conf = { + .reg_bits = 8, + .val_bits = 8, +}; + +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)); + return PTR_ERR(regmap); + } + + return ens160_core_probe(&client->dev, regmap, client->name); +} + +static void ens160_i2c_remove(struct i2c_client *client) +{ + ens160_core_remove(&client->dev); +} + +static const struct i2c_device_id ens160_i2c_id[] = { + { "ens160", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, ens160_i2c_id); + +static const struct of_device_id ens160_of_i2c_match[] = { + { .compatible = "sciosense,ens160" }, + { } +}; +MODULE_DEVICE_TABLE(of, ens160_of_i2c_match); + +static struct i2c_driver ens160_i2c_driver = { + .driver = { + .name = "ens160_i2c", + .of_match_table = ens160_of_i2c_match, + }, + .probe = ens160_i2c_probe, + .remove = ens160_i2c_remove, + .id_table = ens160_i2c_id, +}; +module_i2c_driver(ens160_i2c_driver); + +MODULE_AUTHOR("Gustavo Silva <gustavograzs@gmail.com>"); +MODULE_DESCRIPTION("ScioSense ENS160 I2C driver"); +MODULE_LICENSE("GPL v2"); +MODULE_IMPORT_NS(IIO_ENS160); diff --git a/drivers/iio/chemical/ens160_spi.c b/drivers/iio/chemical/ens160_spi.c new file mode 100644 index 000000000..effc4acee --- /dev/null +++ b/drivers/iio/chemical/ens160_spi.c @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ScioSense ENS160 multi-gas sensor SPI driver + * + * Copyright (c) 2024 Gustavo Silva <gustavograzs@gmail.com> + */ + +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/spi/spi.h> + +#include "ens160.h" + +#define ENS160_SPI_READ BIT(0) + +static const struct regmap_config ens160_regmap_spi_conf = { + .reg_bits = 8, + .val_bits = 8, + .reg_shift = -1, + .read_flag_mask = ENS160_SPI_READ, +}; + +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); + return PTR_ERR(regmap); + } + + return ens160_core_probe(&spi->dev, regmap, id->name); +} + +static void ens160_spi_remove(struct spi_device *spi) +{ + ens160_core_remove(&spi->dev); +} + +static const struct of_device_id ens160_spi_of_match[] = { + { .compatible = "sciosense,ens160" }, + { } +}; +MODULE_DEVICE_TABLE(of, ens160_spi_of_match); + +static const struct spi_device_id ens160_spi_id[] = { + { "ens160", 0 }, + { } +}; +MODULE_DEVICE_TABLE(spi, ens160_spi_id); + +static struct spi_driver ens160_spi_driver = { + .driver = { + .name = "ens160_spi", + .of_match_table = ens160_spi_of_match, + }, + .probe = ens160_spi_probe, + .remove = ens160_spi_remove, + .id_table = ens160_spi_id, +}; +module_spi_driver(ens160_spi_driver); + +MODULE_AUTHOR("Gustavo Silva <gustavograzs@gmail.com>"); +MODULE_DESCRIPTION("ScioSense ENS160 SPI driver"); +MODULE_LICENSE("GPL v2"); +MODULE_IMPORT_NS(IIO_ENS160);
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@gmail.com> --- drivers/iio/chemical/Kconfig | 22 +++ drivers/iio/chemical/Makefile | 3 + drivers/iio/chemical/ens160.h | 9 ++ drivers/iio/chemical/ens160_core.c | 227 +++++++++++++++++++++++++++++ drivers/iio/chemical/ens160_i2c.c | 68 +++++++++ drivers/iio/chemical/ens160_spi.c | 69 +++++++++ 6 files changed, 398 insertions(+) create mode 100644 drivers/iio/chemical/ens160.h create mode 100644 drivers/iio/chemical/ens160_core.c create mode 100644 drivers/iio/chemical/ens160_i2c.c create mode 100644 drivers/iio/chemical/ens160_spi.c