On Wed, 5 Jul 2017 14:04:12 +0200
Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> wrote:
> > Add support for CCS811 VOC sensor. This patch adds support
> > for reading current and voltage across the sensor and TVOC
> > and equivalent CO2 processed values.
>
> nice, comments below
> link to datasheet would be nice
A few more trivial bits from me, though Peter's review was a lot more
thorough!
Pretty good start though.
Jonathan
>
> should list limitations and TODOs, e.g. interrupt support, NTC
>
> > Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@xxxxxxxxx>
> > ---
> > This patch introduces a probe function and adds support for readings
> > from CCS811 sensor. In probe function, ccs811_setup() function is called
> > to perform initial setup of the sensor:
> > * ccs811_switch_to_application_mode() assures transition from boot mode
> > to application mode, by first checking for a valid application and then
> > selecting the APP_START register in order to start running the loaded app.
> >
> > * ccs811_set_drive_mode configures the sensor in the desired operating mode.
> >
> > ccs811_read_raw performs readings from the 4 defined channels:
> > - current and voltage are read from the 2-byte RAW_DATA register.
> > 6 bits represent the current and the lower 10 bits represent
> > the voltage across the sensor.
> >
> > - eCO2 and TVOC are read from the 8-byte ALG_RESULT_DATA register.
> > Only 5 bytes are read here: first 2 bytes represent equivalent CO2,
> > which ranges from 400 ppm to 8192 ppm; next 2 bytes represent TVOC,
> > varying from 0 ppb to 1187 ppb. Last byte is the status register,
>
> see sysfs-bus-iio; IIO_CONCENTRATION should be a percentage value
Or use _raw and _scale to end up with that result.
>
>
> > which is read in order to check for errors.
> >
> > drivers/iio/chemical/Kconfig | 7 ++
> > drivers/iio/chemical/Makefile | 1 +
> > drivers/iio/chemical/ccs811.c | 274 ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 282 insertions(+)
> > create mode 100644 drivers/iio/chemical/ccs811.c
> >
> > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > index cea7f98..4d799b5 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -21,6 +21,13 @@ config ATLAS_PH_SENSOR
> > To compile this driver as module, choose M here: the
> > module will be called atlas-ph-sensor.
> >
> > +config CCS811
> > + tristate "AMS CCS811 VOC sensor"
> > + depends on I2C
> > + help
> > + Say Y here to build I2C interface support for the AMS
> > + CCS811 VOC (Volatile Organic Compounds) sensor
> > +
> > config IAQCORE
> > tristate "AMS iAQ-Core VOC sensors"
> > depends on I2C
> > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > index b02202b..a629b29 100644
> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile
> > @@ -4,5 +4,6 @@
> >
> > # When adding new entries keep the list in alphabetical order
> > obj-$(CONFIG_ATLAS_PH_SENSOR) += atlas-ph-sensor.o
> > +obj-$(CONFIG_CCS811) += ccs811.o
> > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > obj-$(CONFIG_VZ89X) += vz89x.o
> > diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
> > new file mode 100644
> > index 0000000..2d736d4
> > --- /dev/null
> > +++ b/drivers/iio/chemical/ccs811.c
> > @@ -0,0 +1,274 @@
> > +/*
> > + * ccs811.c - Support for AMS CCS811 VOC Sensor
> > + *
> > + * Copyright (C) 2017 Narcisa Vasile <narcisaanamaria12@xxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * IIO driver for AMS CCS811 (I2C address 0x5A/0x5B set by ADDR Low/High)
> > + *
No need for this empty comment line.
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +
> > +#define CCS811_STATUS 0x00
> > +#define CCS811_MEAS_MODE 0x01
> > +#define CCS811_ALG_RESULT_DATA 0x02
> > +#define CCS811_RAW_DATA 0x03
> > +#define CCS811_HW_ID 0x20
> > +#define CCS811_ERR 0xE0
> > +#define CCS811_APP_START 0xF4
>
> APP_START is undocumented?
>
> > +
> > +#define CCS811_VOLTAGE_MASK 0x3FF
> > +
> > +#define STATUS_DATA_READY BIT(3)
> > +#define STATUS_APP_VALID BIT(4)
> > +#define STATUS_FW_MODE BIT(7)
>
> prefix please
>
> > +
> > +enum operation_mode {
>
> prefix please
Or leave it anonymous as 'currently' you don't use it as a type anyway.
>
> > + CCS811_MODE_IDLE = 0,
> > + CCS811_MODE_IAQ_SEC_1 = 1,
> > + CCS811_MODE_IAQ_SEC_10 = 2,
> > + CCS811_MODE_IAQ_SEC_60 = 3,
> > + CCS811_MODE_RAW_DATA = 4
> > +};
> > +
> > +struct ccs811_data {
> > + struct i2c_client *client;
> > + u8 buffer[8];
> > +};
> > +
> > +static const struct iio_chan_spec ccs811_channels[] = {
> > + {
> > + .type = IIO_CURRENT,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > + },
> > + {
Slight preference for }, { as more compact and doesn't loose
readability.
> > + .type = IIO_VOLTAGE,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > + },
> > + {
> > + .type = IIO_CONCENTRATION,
> > + .channel2 = IIO_MOD_CO2,
> > + .modified = 1,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + },
> > + {
> > + .type = IIO_CONCENTRATION,
> > + .channel2 = IIO_MOD_VOC,
> > + .modified = 1,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + },
> > +};
> > +
> > +static int ccs811_data_ready(struct i2c_client *client)
> > +{
> > + int status;
>
> maybe use 'ret' as everywhere else
>
> > +
> > + status = i2c_smbus_read_byte_data(client, CCS811_STATUS);
> > + if (status < 0)
> > + return status;
> > +
> > + if (!(status & STATUS_DATA_READY))
> > + return -EAGAIN;
>
> most other drivers wait for data to become ready instead of erroring out
Perhaps try a few times with suitable sleeps. Ultimately you want to error
out if the device isn't working for some reason and this bit never gets set.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int ccs811_set_drive_mode(struct i2c_client *client, int mode)
>
> use
> enum operation_mode
> instead of int?
Agreed - though with a prefix as Peter suggested above.
>
> > +{
> > + int ret, status, meas_mode = 0;
>
> no need to initialize meas_mode
>
> > +
> > + if (mode < 0 || mode > 4)
If using the enum type you might want to use the trick of adding
a 'MAX' element to the end then checking less than that.
> > + return -EINVAL;
> > +
> > + meas_mode = mode << 4;
>
> temporary variable needed?
>
> > +
> > + ret = i2c_smbus_write_byte_data(client, CCS811_MEAS_MODE, meas_mode);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Check status for errors */
> > + status = i2c_smbus_read_byte_data(client, CCS811_STATUS);
> > + if (status < 0)
> > + return status;
> > +
> > + /* First bit of status is set to 1, if an error occurred */
>
> comments here are rather pointless
>
> > + if (status & 1)
>
> maybe a #define? there are already _STATUS BIT()s
>
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +static int ccs811_switch_to_application_mode(struct i2c_client *client)
> > +{
> > + int status, ret;
>
> both needed?
>
> > +
> > + status = i2c_smbus_read_byte_data(client, CCS811_STATUS);
> > + if (status < 0)
> > + return status;
> > +
> > + if (!(status & STATUS_APP_VALID))
> > + return -EIO;
> > +
> > + ret = i2c_smbus_read_byte_data(client, CCS811_APP_START);
> > + if (ret < 0)
> > + return ret;
> > +
> > + status = i2c_smbus_read_byte_data(client, CCS811_STATUS);
> > + if (status < 0)
> > + return status;
> > +
> > + if (!(status & STATUS_FW_MODE))
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +static int ccs811_setup(struct i2c_client *client)
> > +{
> > + int ret;
> > +
> > + ret = ccs811_switch_to_application_mode(client);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = ccs811_set_drive_mode(client, CCS811_MODE_IAQ_SEC_1);
>
> to would be nice to expose the 1 second interval to userspace... (or at
> least note this as a TODO)
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int ccs811_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct ccs811_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ret = ccs811_data_ready(data->client);
> > + if (ret < 0)
> > + return ret;
>
> I think there should be a mutex so that
> data_ready() and read()
> are executed in a critical section
>
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = i2c_smbus_read_word_swapped(data->client,
> > + CCS811_RAW_DATA);
> > + if (ret < 0)
> > + return ret;
> > +
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + *val = ret & CCS811_VOLTAGE_MASK;
>
> this should be in millivolts (after _SCALE, _OFFSET)?
> datasheet says that 1023 = 1.65V
>
> > + return IIO_VAL_INT;
> > + case IIO_CURRENT:
>
> this should be in milliamps?
> datasheet says this is 0 to 63 microamps...
>
> > + *val = ret >> 10;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
>
> break not needed
>
> > + case IIO_CHAN_INFO_PROCESSED:
> > + if (chan->type != IIO_CONCENTRATION)
> > + return -EINVAL;
> > +
> > + ret = i2c_smbus_read_i2c_block_data(data->client,
> > + CCS811_ALG_RESULT_DATA,
> > + 5, data->buffer);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* If the number of bytes read is different than what was
>
> proper multi-line comment format please
> /*
> *
> */
>
> > + * requested or status error bit was set
> > + */
> > + if (ret != 5 || (data->buffer[4] & 1))
> > + return -EIO;
> > +
> > + switch (chan->channel2) {
> > + case IIO_MOD_CO2:
>
> data->buffer could be a (packed) struct?
> __be16 co2;
> __be16 voc;
> u8 status;
> ?
>
>
> > + *val = data->buffer[0] * 256 + data->buffer[1];
> > + return IIO_VAL_INT;
> > + case IIO_MOD_VOC:
> > + *val = data->buffer[2] * 256 + data->buffer[3];
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return -EINVAL;
>
> return not needed
>
> > +}
> > +
> > +static const struct iio_info ccs811_info = {
> > + .read_raw = ccs811_read_raw,
> > +};
> > +
> > +static int ccs811_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct ccs811_data *data;
> > + int hwid, ret;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA
> > + | I2C_FUNC_SMBUS_WORD_DATA
> > + | I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> > + return -EOPNOTSUPP;
> > +
> > + /* Check hardware id (should be 0x81 for this family of devices) */
> > + hwid = i2c_smbus_read_byte_data(client, CCS811_HW_ID);
> > + if (hwid < 0)
> > + return hwid;
> > +
> > + if (hwid != 0x81) {
> > + dev_err(&client->dev, "no CCS811 sensor\n");
> > + return -ENODEV;
> > + }
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + ret = ccs811_setup(client);
> > + if (ret < 0)
> > + return ret;
> > +
> > + data = iio_priv(indio_dev);
> > + i2c_set_clientdata(client, indio_dev);
> > + data->client = client;
> > +
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->name = id->name;
> > + indio_dev->info = &ccs811_info;
> > +
> > + indio_dev->channels = ccs811_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ccs811_channels);
> > +
> > + return devm_iio_device_register(&client->dev, indio_dev);
>
> set chip to idle if _register fails?
>
> maye set chip to idle in _remove()?
If you do, please note that you will want to use
iio_device_register / unregister to avoid potential races with
userspace going away.
>
> > +}
> > +
> > +static const struct i2c_device_id ccs811_id[] = {
> > + {"ccs811", 0},
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ccs811_id);
> > +
> > +static struct i2c_driver ccs811_driver = {
> > + .driver = {
> > + .name = "ccs811",
> > + },
> > + .probe = ccs811_probe,
> > + .id_table = ccs811_id,
> > +};
> > +module_i2c_driver(ccs811_driver);
> > +
> > +MODULE_AUTHOR("Narcisa Vasile <narcisaanamaria12@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("CCS811 VOC SENSOR");
>
> sensor
Given it will still be fairly short, I'd spell out 'volatile organic compounds'
(or smelly breath ;)
>
> > +MODULE_LICENSE("GPL v2");
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
