Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Mon, Dec 03, 2018 at 11:30:45AM +0100, Andreas Brauchli wrote: > From: Andreas Brauchli > > On Sat, 1 Dec 2018 15:58:57 +, Jonathan Cameron wrote: > > On Sun, 25 Nov 2018 20:05:09 +0100 > > Tomasz Duszynski wrote: > > > > > On Sun, Nov 25, 2018 at 08:56:59AM +, Jonathan Cameron wrote: > > > > On Sat, 24 Nov 2018 23:14:14 +0100 > > > > Tomasz Duszynski wrote: > > > > > > > > > Add support for Sensirion SPS30 particulate matter sensor. > > Thanks a lot for your work! A few comments inline, I'm also happy to assist > with > further clarifications. > > Since I work for Sensirion, feel free to add me on the Acked-by list. (The > company server molests emails..) > > Acked-by: Andreas Brauchli > > > > > > > > > > > Signed-off-by: Tomasz Duszynski > > > > A few things inline. > > > > > > > > I'm particularly curious as to why we are ignoring two of the particle > > > > sizes that the sensor seems to capture? > > > > > > > > > > I was thinking about adding first the most common ones i.e PM2.5 and > > > PM10 and the rest of them later on if there's a particular need. > > > > > > On the other hand I can add PM1.0 and PM4.0 now so that we have > > > everything in place once new dust sensors appear on the market. > > > > I would. I do hope there don't turn out to be 100s of different > > views of what these values should be though. The various standards > > should prevent that even if people tuck in one or two extra > > just because they could. > > PM1.0 is definitely used in other sensors (e.g. Plantower), but on a quick > search I couldn't find another PM4.0 one. > > Our rationale for the value is that, according to medical studies, anything > below PM4.0 can affect the lungs. > Fair enough. > > > > > > > > It's seems reasonable to assume they will measure the very same > > > subset of particulates. > > > > > > > Also, a 'potential' race in remove that I'd like us to make > > > > 'obviously' correct rather than requiring hard thought on whether > > > > it would be a problem. > > > > > > > > Thanks, > > > > > > > > Jonathan > > > > > > > > > --- > > > > > drivers/iio/chemical/Kconfig | 11 ++ > > > > > drivers/iio/chemical/Makefile | 1 + > > > > > drivers/iio/chemical/sps30.c | 359 > > > > > ++ > > > > > 3 files changed, 371 insertions(+) > > > > > create mode 100644 drivers/iio/chemical/sps30.c > > > > > > > > > > diff --git a/drivers/iio/chemical/Kconfig > > > > > b/drivers/iio/chemical/Kconfig > > > > > index b8e005be4f87..40057ecf8130 100644 > > > > > --- a/drivers/iio/chemical/Kconfig > > > > > +++ b/drivers/iio/chemical/Kconfig > > > > > @@ -61,6 +61,17 @@ config IAQCORE > > > > > iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds) > > > > > sensors > > > > > > > > > > +config SPS30 > > > > > + tristate "SPS30 Particulate Matter sensor" > > > > > + depends on I2C > > > > > + select CRC8 > > > > > + help > > > > > + Say Y here to build support for the Sensirion SPS30 > > > > > Particulate > > > > > + Matter sensor. > > > > > + > > > > > + To compile this driver as a module, choose M here: the module > > > > > will > > > > > + be called sps30. > > > > > + > > > > > config VZ89X > > > > > tristate "SGX Sensortech MiCS VZ89X VOC sensor" > > > > > depends on I2C > > > > > diff --git a/drivers/iio/chemical/Makefile > > > > > b/drivers/iio/chemical/Makefile > > > > > index 2f4c4ba4d781..9f42f4252151 100644 > > > > > --- a/drivers/iio/chemical/Makefile > > > > > +++ b/drivers/iio/chemical/Makefile > > > > > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o > > > > > obj-$(CONFIG_BME680_SPI) += bme680_spi.o > > > > > obj-$(CONFIG_CCS811) += ccs811.o > > > > > obj-$(CONFIG_IAQCORE)+= ams-iaq-core.o > > > > > +obj-$(CONFIG_SPS30) += sps30.o > > > > > obj-$(CONFIG_VZ89X) += vz89x.o > > > > > diff --git a/drivers/iio/chemical/sps30.c > > > > > b/drivers/iio/chemical/sps30.c > > > > > new file mode 100644 > > > > > index ..bf802621ae7f > > > > > --- /dev/null > > > > > +++ b/drivers/iio/chemical/sps30.c > > > > > @@ -0,0 +1,359 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > +/* > > > > > + * Sensirion SPS30 Particulate Matter sensor driver > > > > > + * > > > > > + * Copyright (c) Tomasz Duszynski > > > > > + * > > > > > + * I2C slave address: 0x69 > > > > > + * > > > > > + * TODO: > > > > > + * - support for turning on fan cleaning > > > > > + * - support for reading/setting auto cleaning interval > > > > > + */ > > > > > + > > > > > +#define pr_fmt(fmt) "sps30: " fmt > > > > > + > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > + > > > > > +#define SPS30_CRC8_POLYNOMIAL 0x31 > > > > > + > > > > > +/* SPS30 commands */ > > > > > +#define SPS30_START_MEAS 0x0010 > >
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Mon, Dec 03, 2018 at 11:30:45AM +0100, Andreas Brauchli wrote: > From: Andreas Brauchli > > On Sat, 1 Dec 2018 15:58:57 +, Jonathan Cameron wrote: > > On Sun, 25 Nov 2018 20:05:09 +0100 > > Tomasz Duszynski wrote: > > > > > On Sun, Nov 25, 2018 at 08:56:59AM +, Jonathan Cameron wrote: > > > > On Sat, 24 Nov 2018 23:14:14 +0100 > > > > Tomasz Duszynski wrote: > > > > > > > > > Add support for Sensirion SPS30 particulate matter sensor. > > Thanks a lot for your work! A few comments inline, I'm also happy to assist > with > further clarifications. > > Since I work for Sensirion, feel free to add me on the Acked-by list. (The > company server molests emails..) > > Acked-by: Andreas Brauchli > > > > > > > > > > > Signed-off-by: Tomasz Duszynski > > > > A few things inline. > > > > > > > > I'm particularly curious as to why we are ignoring two of the particle > > > > sizes that the sensor seems to capture? > > > > > > > > > > I was thinking about adding first the most common ones i.e PM2.5 and > > > PM10 and the rest of them later on if there's a particular need. > > > > > > On the other hand I can add PM1.0 and PM4.0 now so that we have > > > everything in place once new dust sensors appear on the market. > > > > I would. I do hope there don't turn out to be 100s of different > > views of what these values should be though. The various standards > > should prevent that even if people tuck in one or two extra > > just because they could. > > PM1.0 is definitely used in other sensors (e.g. Plantower), but on a quick > search I couldn't find another PM4.0 one. > > Our rationale for the value is that, according to medical studies, anything > below PM4.0 can affect the lungs. > Fair enough. > > > > > > > > It's seems reasonable to assume they will measure the very same > > > subset of particulates. > > > > > > > Also, a 'potential' race in remove that I'd like us to make > > > > 'obviously' correct rather than requiring hard thought on whether > > > > it would be a problem. > > > > > > > > Thanks, > > > > > > > > Jonathan > > > > > > > > > --- > > > > > drivers/iio/chemical/Kconfig | 11 ++ > > > > > drivers/iio/chemical/Makefile | 1 + > > > > > drivers/iio/chemical/sps30.c | 359 > > > > > ++ > > > > > 3 files changed, 371 insertions(+) > > > > > create mode 100644 drivers/iio/chemical/sps30.c > > > > > > > > > > diff --git a/drivers/iio/chemical/Kconfig > > > > > b/drivers/iio/chemical/Kconfig > > > > > index b8e005be4f87..40057ecf8130 100644 > > > > > --- a/drivers/iio/chemical/Kconfig > > > > > +++ b/drivers/iio/chemical/Kconfig > > > > > @@ -61,6 +61,17 @@ config IAQCORE > > > > > iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds) > > > > > sensors > > > > > > > > > > +config SPS30 > > > > > + tristate "SPS30 Particulate Matter sensor" > > > > > + depends on I2C > > > > > + select CRC8 > > > > > + help > > > > > + Say Y here to build support for the Sensirion SPS30 > > > > > Particulate > > > > > + Matter sensor. > > > > > + > > > > > + To compile this driver as a module, choose M here: the module > > > > > will > > > > > + be called sps30. > > > > > + > > > > > config VZ89X > > > > > tristate "SGX Sensortech MiCS VZ89X VOC sensor" > > > > > depends on I2C > > > > > diff --git a/drivers/iio/chemical/Makefile > > > > > b/drivers/iio/chemical/Makefile > > > > > index 2f4c4ba4d781..9f42f4252151 100644 > > > > > --- a/drivers/iio/chemical/Makefile > > > > > +++ b/drivers/iio/chemical/Makefile > > > > > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o > > > > > obj-$(CONFIG_BME680_SPI) += bme680_spi.o > > > > > obj-$(CONFIG_CCS811) += ccs811.o > > > > > obj-$(CONFIG_IAQCORE)+= ams-iaq-core.o > > > > > +obj-$(CONFIG_SPS30) += sps30.o > > > > > obj-$(CONFIG_VZ89X) += vz89x.o > > > > > diff --git a/drivers/iio/chemical/sps30.c > > > > > b/drivers/iio/chemical/sps30.c > > > > > new file mode 100644 > > > > > index ..bf802621ae7f > > > > > --- /dev/null > > > > > +++ b/drivers/iio/chemical/sps30.c > > > > > @@ -0,0 +1,359 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > +/* > > > > > + * Sensirion SPS30 Particulate Matter sensor driver > > > > > + * > > > > > + * Copyright (c) Tomasz Duszynski > > > > > + * > > > > > + * I2C slave address: 0x69 > > > > > + * > > > > > + * TODO: > > > > > + * - support for turning on fan cleaning > > > > > + * - support for reading/setting auto cleaning interval > > > > > + */ > > > > > + > > > > > +#define pr_fmt(fmt) "sps30: " fmt > > > > > + > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > + > > > > > +#define SPS30_CRC8_POLYNOMIAL 0x31 > > > > > + > > > > > +/* SPS30 commands */ > > > > > +#define SPS30_START_MEAS 0x0010 > >
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
From: Andreas Brauchli
On Sat, 1 Dec 2018 15:58:57 +, Jonathan Cameron wrote:
> On Sun, 25 Nov 2018 20:05:09 +0100
> Tomasz Duszynski wrote:
>
> > On Sun, Nov 25, 2018 at 08:56:59AM +, Jonathan Cameron wrote:
> > > On Sat, 24 Nov 2018 23:14:14 +0100
> > > Tomasz Duszynski wrote:
> > >
> > > > Add support for Sensirion SPS30 particulate matter sensor.
Thanks a lot for your work! A few comments inline, I'm also happy to assist with
further clarifications.
Since I work for Sensirion, feel free to add me on the Acked-by list. (The
company server molests emails..)
Acked-by: Andreas Brauchli
> > > >
> > > > Signed-off-by: Tomasz Duszynski
> > > A few things inline.
> > >
> > > I'm particularly curious as to why we are ignoring two of the particle
> > > sizes that the sensor seems to capture?
> > >
> >
> > I was thinking about adding first the most common ones i.e PM2.5 and
> > PM10 and the rest of them later on if there's a particular need.
> >
> > On the other hand I can add PM1.0 and PM4.0 now so that we have
> > everything in place once new dust sensors appear on the market.
>
> I would. I do hope there don't turn out to be 100s of different
> views of what these values should be though. The various standards
> should prevent that even if people tuck in one or two extra
> just because they could.
PM1.0 is definitely used in other sensors (e.g. Plantower), but on a quick
search I couldn't find another PM4.0 one.
Our rationale for the value is that, according to medical studies, anything
below PM4.0 can affect the lungs.
>
> >
> > It's seems reasonable to assume they will measure the very same
> > subset of particulates.
> >
> > > Also, a 'potential' race in remove that I'd like us to make
> > > 'obviously' correct rather than requiring hard thought on whether
> > > it would be a problem.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > > > ---
> > > > drivers/iio/chemical/Kconfig | 11 ++
> > > > drivers/iio/chemical/Makefile | 1 +
> > > > drivers/iio/chemical/sps30.c | 359 ++
> > > > 3 files changed, 371 insertions(+)
> > > > create mode 100644 drivers/iio/chemical/sps30.c
> > > >
> > > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > > index b8e005be4f87..40057ecf8130 100644
> > > > --- a/drivers/iio/chemical/Kconfig
> > > > +++ b/drivers/iio/chemical/Kconfig
> > > > @@ -61,6 +61,17 @@ config IAQCORE
> > > > iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> > > > sensors
> > > >
> > > > +config SPS30
> > > > + tristate "SPS30 Particulate Matter sensor"
> > > > + depends on I2C
> > > > + select CRC8
> > > > + help
> > > > + Say Y here to build support for the Sensirion SPS30
> > > > Particulate
> > > > + Matter sensor.
> > > > +
> > > > + To compile this driver as a module, choose M here: the module
> > > > will
> > > > + be called sps30.
> > > > +
> > > > config VZ89X
> > > > tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> > > > depends on I2C
> > > > diff --git a/drivers/iio/chemical/Makefile
> > > > b/drivers/iio/chemical/Makefile
> > > > index 2f4c4ba4d781..9f42f4252151 100644
> > > > --- a/drivers/iio/chemical/Makefile
> > > > +++ b/drivers/iio/chemical/Makefile
> > > > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> > > > obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > > > obj-$(CONFIG_CCS811) += ccs811.o
> > > > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > > > +obj-$(CONFIG_SPS30) += sps30.o
> > > > obj-$(CONFIG_VZ89X)+= vz89x.o
> > > > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > > > new file mode 100644
> > > > index ..bf802621ae7f
> > > > --- /dev/null
> > > > +++ b/drivers/iio/chemical/sps30.c
> > > > @@ -0,0 +1,359 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Sensirion SPS30 Particulate Matter sensor driver
> > > > + *
> > > > + * Copyright (c) Tomasz Duszynski
> > > > + *
> > > > + * I2C slave address: 0x69
> > > > + *
> > > > + * TODO:
> > > > + * - support for turning on fan cleaning
> > > > + * - support for reading/setting auto cleaning interval
> > > > + */
> > > > +
> > > > +#define pr_fmt(fmt) "sps30: " fmt
> > > > +
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +
> > > > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > > > +
> > > > +/* SPS30 commands */
> > > > +#define SPS30_START_MEAS 0x0010
> > > > +#define SPS30_STOP_MEAS 0x0104
> > > > +#define SPS30_RESET 0xd304
lower case d vs upper case in SPS30_READ_SERIAL
> > > > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > > > +#define SPS30_READ_DATA 0x0300
> > > > +#define SPS30_READ_SERIAL 0xD033
> > > > +
> > > > +#define SPS30_CHAN(_index, _mod) { \
> > > > + .type =
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
From: Andreas Brauchli
On Sat, 1 Dec 2018 15:58:57 +, Jonathan Cameron wrote:
> On Sun, 25 Nov 2018 20:05:09 +0100
> Tomasz Duszynski wrote:
>
> > On Sun, Nov 25, 2018 at 08:56:59AM +, Jonathan Cameron wrote:
> > > On Sat, 24 Nov 2018 23:14:14 +0100
> > > Tomasz Duszynski wrote:
> > >
> > > > Add support for Sensirion SPS30 particulate matter sensor.
Thanks a lot for your work! A few comments inline, I'm also happy to assist with
further clarifications.
Since I work for Sensirion, feel free to add me on the Acked-by list. (The
company server molests emails..)
Acked-by: Andreas Brauchli
> > > >
> > > > Signed-off-by: Tomasz Duszynski
> > > A few things inline.
> > >
> > > I'm particularly curious as to why we are ignoring two of the particle
> > > sizes that the sensor seems to capture?
> > >
> >
> > I was thinking about adding first the most common ones i.e PM2.5 and
> > PM10 and the rest of them later on if there's a particular need.
> >
> > On the other hand I can add PM1.0 and PM4.0 now so that we have
> > everything in place once new dust sensors appear on the market.
>
> I would. I do hope there don't turn out to be 100s of different
> views of what these values should be though. The various standards
> should prevent that even if people tuck in one or two extra
> just because they could.
PM1.0 is definitely used in other sensors (e.g. Plantower), but on a quick
search I couldn't find another PM4.0 one.
Our rationale for the value is that, according to medical studies, anything
below PM4.0 can affect the lungs.
>
> >
> > It's seems reasonable to assume they will measure the very same
> > subset of particulates.
> >
> > > Also, a 'potential' race in remove that I'd like us to make
> > > 'obviously' correct rather than requiring hard thought on whether
> > > it would be a problem.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > > > ---
> > > > drivers/iio/chemical/Kconfig | 11 ++
> > > > drivers/iio/chemical/Makefile | 1 +
> > > > drivers/iio/chemical/sps30.c | 359 ++
> > > > 3 files changed, 371 insertions(+)
> > > > create mode 100644 drivers/iio/chemical/sps30.c
> > > >
> > > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > > index b8e005be4f87..40057ecf8130 100644
> > > > --- a/drivers/iio/chemical/Kconfig
> > > > +++ b/drivers/iio/chemical/Kconfig
> > > > @@ -61,6 +61,17 @@ config IAQCORE
> > > > iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> > > > sensors
> > > >
> > > > +config SPS30
> > > > + tristate "SPS30 Particulate Matter sensor"
> > > > + depends on I2C
> > > > + select CRC8
> > > > + help
> > > > + Say Y here to build support for the Sensirion SPS30
> > > > Particulate
> > > > + Matter sensor.
> > > > +
> > > > + To compile this driver as a module, choose M here: the module
> > > > will
> > > > + be called sps30.
> > > > +
> > > > config VZ89X
> > > > tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> > > > depends on I2C
> > > > diff --git a/drivers/iio/chemical/Makefile
> > > > b/drivers/iio/chemical/Makefile
> > > > index 2f4c4ba4d781..9f42f4252151 100644
> > > > --- a/drivers/iio/chemical/Makefile
> > > > +++ b/drivers/iio/chemical/Makefile
> > > > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> > > > obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > > > obj-$(CONFIG_CCS811) += ccs811.o
> > > > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > > > +obj-$(CONFIG_SPS30) += sps30.o
> > > > obj-$(CONFIG_VZ89X)+= vz89x.o
> > > > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > > > new file mode 100644
> > > > index ..bf802621ae7f
> > > > --- /dev/null
> > > > +++ b/drivers/iio/chemical/sps30.c
> > > > @@ -0,0 +1,359 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Sensirion SPS30 Particulate Matter sensor driver
> > > > + *
> > > > + * Copyright (c) Tomasz Duszynski
> > > > + *
> > > > + * I2C slave address: 0x69
> > > > + *
> > > > + * TODO:
> > > > + * - support for turning on fan cleaning
> > > > + * - support for reading/setting auto cleaning interval
> > > > + */
> > > > +
> > > > +#define pr_fmt(fmt) "sps30: " fmt
> > > > +
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +
> > > > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > > > +
> > > > +/* SPS30 commands */
> > > > +#define SPS30_START_MEAS 0x0010
> > > > +#define SPS30_STOP_MEAS 0x0104
> > > > +#define SPS30_RESET 0xd304
lower case d vs upper case in SPS30_READ_SERIAL
> > > > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > > > +#define SPS30_READ_DATA 0x0300
> > > > +#define SPS30_READ_SERIAL 0xD033
> > > > +
> > > > +#define SPS30_CHAN(_index, _mod) { \
> > > > + .type =
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Mon, 26 Nov 2018 21:48:07 +0100
Tomasz Duszynski wrote:
> On Sun, Nov 25, 2018 at 04:14:34PM +0530, Himanshu Jha wrote:
> > On Sat, Nov 24, 2018 at 11:14:14PM +0100, Tomasz Duszynski wrote:
> > > Add support for Sensirion SPS30 particulate matter sensor.
> > >
> > > Signed-off-by: Tomasz Duszynski
> > > ---
> > > drivers/iio/chemical/Kconfig | 11 ++
> > > drivers/iio/chemical/Makefile | 1 +
> > > drivers/iio/chemical/sps30.c | 359 ++
> > > 3 files changed, 371 insertions(+)
> > > create mode 100644 drivers/iio/chemical/sps30.c
> >
> > []
> >
> > > +#define pr_fmt(fmt) "sps30: " fmt
> >
> > I don't see its usage ?
> >
>
> Hint: checkout how dev_err() is defined.
>
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > > +
> > > +/* SPS30 commands */
> > > +#define SPS30_START_MEAS 0x0010
> > > +#define SPS30_STOP_MEAS 0x0104
> > > +#define SPS30_RESET 0xd304
> > > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > > +#define SPS30_READ_DATA 0x0300
> > > +#define SPS30_READ_SERIAL 0xD033
> >
> > Could you please put a tab and align these macros.
> >
> > #define SPS30_START_MEAS 0x0010
> > #define SPS30_STOP_MEAS 0x0104
> >
>
> In my opinion this sort of alignment does not pay off in the long run.
> Adding a new definition, a slightly longer one perhaps, can easily break
> formatting.
>
> So I would stay with current one.
Personally I agree with you on this one. I don't care enough to
say either way though normally!
>
> >
> > > +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> > > + int buf_size, u8 *data, int data_size)
> > > +{
> > > + /* every two received data bytes are checksummed */
> > > + u8 tmp[data_size + data_size / 2];
> >
> > No VLAs!
> >
> > https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com/
> >
>
> Looks like -Wvla is some fairly recent addition to KBUILD_CFLAGS.
Yeah, that was only after a 'lot' of effort over years to get the number
present down to a small number.
I wrote plenty of them in the early days of IIO so got a lot of
those patches as part of the move to introducing the warning :)
Jonathan
...
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Mon, 26 Nov 2018 21:48:07 +0100
Tomasz Duszynski wrote:
> On Sun, Nov 25, 2018 at 04:14:34PM +0530, Himanshu Jha wrote:
> > On Sat, Nov 24, 2018 at 11:14:14PM +0100, Tomasz Duszynski wrote:
> > > Add support for Sensirion SPS30 particulate matter sensor.
> > >
> > > Signed-off-by: Tomasz Duszynski
> > > ---
> > > drivers/iio/chemical/Kconfig | 11 ++
> > > drivers/iio/chemical/Makefile | 1 +
> > > drivers/iio/chemical/sps30.c | 359 ++
> > > 3 files changed, 371 insertions(+)
> > > create mode 100644 drivers/iio/chemical/sps30.c
> >
> > []
> >
> > > +#define pr_fmt(fmt) "sps30: " fmt
> >
> > I don't see its usage ?
> >
>
> Hint: checkout how dev_err() is defined.
>
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > > +
> > > +/* SPS30 commands */
> > > +#define SPS30_START_MEAS 0x0010
> > > +#define SPS30_STOP_MEAS 0x0104
> > > +#define SPS30_RESET 0xd304
> > > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > > +#define SPS30_READ_DATA 0x0300
> > > +#define SPS30_READ_SERIAL 0xD033
> >
> > Could you please put a tab and align these macros.
> >
> > #define SPS30_START_MEAS 0x0010
> > #define SPS30_STOP_MEAS 0x0104
> >
>
> In my opinion this sort of alignment does not pay off in the long run.
> Adding a new definition, a slightly longer one perhaps, can easily break
> formatting.
>
> So I would stay with current one.
Personally I agree with you on this one. I don't care enough to
say either way though normally!
>
> >
> > > +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> > > + int buf_size, u8 *data, int data_size)
> > > +{
> > > + /* every two received data bytes are checksummed */
> > > + u8 tmp[data_size + data_size / 2];
> >
> > No VLAs!
> >
> > https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com/
> >
>
> Looks like -Wvla is some fairly recent addition to KBUILD_CFLAGS.
Yeah, that was only after a 'lot' of effort over years to get the number
present down to a small number.
I wrote plenty of them in the early days of IIO so got a lot of
those patches as part of the move to introducing the warning :)
Jonathan
...
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sun, 25 Nov 2018 20:05:09 +0100
Tomasz Duszynski wrote:
> On Sun, Nov 25, 2018 at 08:56:59AM +, Jonathan Cameron wrote:
> > On Sat, 24 Nov 2018 23:14:14 +0100
> > Tomasz Duszynski wrote:
> >
> > > Add support for Sensirion SPS30 particulate matter sensor.
> > >
> > > Signed-off-by: Tomasz Duszynski
> > A few things inline.
> >
> > I'm particularly curious as to why we are ignoring two of the particle
> > sizes that the sensor seems to capture?
> >
>
> I was thinking about adding first the most common ones i.e PM2.5 and
> PM10 and the rest of them later on if there's a particular need.
>
> On the other hand I can add PM1.0 and PM4.0 now so that we have
> everything in place once new dust sensors appear on the market.
I would. I do hope there don't turn out to be 100s of different
views of what these values should be though. The various standards
should prevent that even if people tuck in one or two extra
just because they could.
>
> It's seems reasonable to assume they will measure the very same
> subset of particulates.
>
> > Also, a 'potential' race in remove that I'd like us to make
> > 'obviously' correct rather than requiring hard thought on whether
> > it would be a problem.
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/chemical/Kconfig | 11 ++
> > > drivers/iio/chemical/Makefile | 1 +
> > > drivers/iio/chemical/sps30.c | 359 ++
> > > 3 files changed, 371 insertions(+)
> > > create mode 100644 drivers/iio/chemical/sps30.c
> > >
> > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > index b8e005be4f87..40057ecf8130 100644
> > > --- a/drivers/iio/chemical/Kconfig
> > > +++ b/drivers/iio/chemical/Kconfig
> > > @@ -61,6 +61,17 @@ config IAQCORE
> > > iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> > > sensors
> > >
> > > +config SPS30
> > > + tristate "SPS30 Particulate Matter sensor"
> > > + depends on I2C
> > > + select CRC8
> > > + help
> > > + Say Y here to build support for the Sensirion SPS30 Particulate
> > > + Matter sensor.
> > > +
> > > + To compile this driver as a module, choose M here: the module will
> > > + be called sps30.
> > > +
> > > config VZ89X
> > > tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> > > depends on I2C
> > > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > > index 2f4c4ba4d781..9f42f4252151 100644
> > > --- a/drivers/iio/chemical/Makefile
> > > +++ b/drivers/iio/chemical/Makefile
> > > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> > > obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > > obj-$(CONFIG_CCS811) += ccs811.o
> > > obj-$(CONFIG_IAQCORE)+= ams-iaq-core.o
> > > +obj-$(CONFIG_SPS30) += sps30.o
> > > obj-$(CONFIG_VZ89X) += vz89x.o
> > > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > > new file mode 100644
> > > index ..bf802621ae7f
> > > --- /dev/null
> > > +++ b/drivers/iio/chemical/sps30.c
> > > @@ -0,0 +1,359 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Sensirion SPS30 Particulate Matter sensor driver
> > > + *
> > > + * Copyright (c) Tomasz Duszynski
> > > + *
> > > + * I2C slave address: 0x69
> > > + *
> > > + * TODO:
> > > + * - support for turning on fan cleaning
> > > + * - support for reading/setting auto cleaning interval
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "sps30: " fmt
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > > +
> > > +/* SPS30 commands */
> > > +#define SPS30_START_MEAS 0x0010
> > > +#define SPS30_STOP_MEAS 0x0104
> > > +#define SPS30_RESET 0xd304
> > > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > > +#define SPS30_READ_DATA 0x0300
> > > +#define SPS30_READ_SERIAL 0xD033
> > > +
> > > +#define SPS30_CHAN(_index, _mod) { \
> > > + .type = IIO_MASSCONCENTRATION, \
> > > + .modified = 1, \
> > > + .channel2 = IIO_MOD_ ## _mod, \
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > > + .scan_index = _index, \
> > > + .scan_type = { \
> > > + .sign = 'u', \
> > > + .realbits = 12, \
> > > + .storagebits = 32, \
> >
> > That is unusual. Why do we need 32 bits to store a 12 bit value?
> > 16 should be plenty. It'll make a difference to the buffer
> > sizes if people just want to stream data and don't care about
> > timestamps as it'll halve the memory usage.
>
> Right, I could have picked up a petter values. But I've got at least one
> valid reason for doing that. Sensor datasheet specifies 0-1000
> measurement range but simple tests showed that SPS30 can measure way beyond
> that. Interestingly, measurements are consistent with third party sensors.
I'm guessing there are certification bodies for this sort of thing. They
may only have
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sun, 25 Nov 2018 20:05:09 +0100
Tomasz Duszynski wrote:
> On Sun, Nov 25, 2018 at 08:56:59AM +, Jonathan Cameron wrote:
> > On Sat, 24 Nov 2018 23:14:14 +0100
> > Tomasz Duszynski wrote:
> >
> > > Add support for Sensirion SPS30 particulate matter sensor.
> > >
> > > Signed-off-by: Tomasz Duszynski
> > A few things inline.
> >
> > I'm particularly curious as to why we are ignoring two of the particle
> > sizes that the sensor seems to capture?
> >
>
> I was thinking about adding first the most common ones i.e PM2.5 and
> PM10 and the rest of them later on if there's a particular need.
>
> On the other hand I can add PM1.0 and PM4.0 now so that we have
> everything in place once new dust sensors appear on the market.
I would. I do hope there don't turn out to be 100s of different
views of what these values should be though. The various standards
should prevent that even if people tuck in one or two extra
just because they could.
>
> It's seems reasonable to assume they will measure the very same
> subset of particulates.
>
> > Also, a 'potential' race in remove that I'd like us to make
> > 'obviously' correct rather than requiring hard thought on whether
> > it would be a problem.
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/chemical/Kconfig | 11 ++
> > > drivers/iio/chemical/Makefile | 1 +
> > > drivers/iio/chemical/sps30.c | 359 ++
> > > 3 files changed, 371 insertions(+)
> > > create mode 100644 drivers/iio/chemical/sps30.c
> > >
> > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > index b8e005be4f87..40057ecf8130 100644
> > > --- a/drivers/iio/chemical/Kconfig
> > > +++ b/drivers/iio/chemical/Kconfig
> > > @@ -61,6 +61,17 @@ config IAQCORE
> > > iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> > > sensors
> > >
> > > +config SPS30
> > > + tristate "SPS30 Particulate Matter sensor"
> > > + depends on I2C
> > > + select CRC8
> > > + help
> > > + Say Y here to build support for the Sensirion SPS30 Particulate
> > > + Matter sensor.
> > > +
> > > + To compile this driver as a module, choose M here: the module will
> > > + be called sps30.
> > > +
> > > config VZ89X
> > > tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> > > depends on I2C
> > > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > > index 2f4c4ba4d781..9f42f4252151 100644
> > > --- a/drivers/iio/chemical/Makefile
> > > +++ b/drivers/iio/chemical/Makefile
> > > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> > > obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > > obj-$(CONFIG_CCS811) += ccs811.o
> > > obj-$(CONFIG_IAQCORE)+= ams-iaq-core.o
> > > +obj-$(CONFIG_SPS30) += sps30.o
> > > obj-$(CONFIG_VZ89X) += vz89x.o
> > > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > > new file mode 100644
> > > index ..bf802621ae7f
> > > --- /dev/null
> > > +++ b/drivers/iio/chemical/sps30.c
> > > @@ -0,0 +1,359 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Sensirion SPS30 Particulate Matter sensor driver
> > > + *
> > > + * Copyright (c) Tomasz Duszynski
> > > + *
> > > + * I2C slave address: 0x69
> > > + *
> > > + * TODO:
> > > + * - support for turning on fan cleaning
> > > + * - support for reading/setting auto cleaning interval
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "sps30: " fmt
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > > +
> > > +/* SPS30 commands */
> > > +#define SPS30_START_MEAS 0x0010
> > > +#define SPS30_STOP_MEAS 0x0104
> > > +#define SPS30_RESET 0xd304
> > > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > > +#define SPS30_READ_DATA 0x0300
> > > +#define SPS30_READ_SERIAL 0xD033
> > > +
> > > +#define SPS30_CHAN(_index, _mod) { \
> > > + .type = IIO_MASSCONCENTRATION, \
> > > + .modified = 1, \
> > > + .channel2 = IIO_MOD_ ## _mod, \
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > > + .scan_index = _index, \
> > > + .scan_type = { \
> > > + .sign = 'u', \
> > > + .realbits = 12, \
> > > + .storagebits = 32, \
> >
> > That is unusual. Why do we need 32 bits to store a 12 bit value?
> > 16 should be plenty. It'll make a difference to the buffer
> > sizes if people just want to stream data and don't care about
> > timestamps as it'll halve the memory usage.
>
> Right, I could have picked up a petter values. But I've got at least one
> valid reason for doing that. Sensor datasheet specifies 0-1000
> measurement range but simple tests showed that SPS30 can measure way beyond
> that. Interestingly, measurements are consistent with third party sensors.
I'm guessing there are certification bodies for this sort of thing. They
may only have
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sun, Nov 25, 2018 at 04:14:34PM +0530, Himanshu Jha wrote:
> On Sat, Nov 24, 2018 at 11:14:14PM +0100, Tomasz Duszynski wrote:
> > Add support for Sensirion SPS30 particulate matter sensor.
> >
> > Signed-off-by: Tomasz Duszynski
> > ---
> > drivers/iio/chemical/Kconfig | 11 ++
> > drivers/iio/chemical/Makefile | 1 +
> > drivers/iio/chemical/sps30.c | 359 ++
> > 3 files changed, 371 insertions(+)
> > create mode 100644 drivers/iio/chemical/sps30.c
>
> []
>
> > +#define pr_fmt(fmt) "sps30: " fmt
>
> I don't see its usage ?
>
Hint: checkout how dev_err() is defined.
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > +
> > +/* SPS30 commands */
> > +#define SPS30_START_MEAS 0x0010
> > +#define SPS30_STOP_MEAS 0x0104
> > +#define SPS30_RESET 0xd304
> > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > +#define SPS30_READ_DATA 0x0300
> > +#define SPS30_READ_SERIAL 0xD033
>
> Could you please put a tab and align these macros.
>
> #define SPS30_START_MEAS0x0010
> #define SPS30_STOP_MEAS 0x0104
>
In my opinion this sort of alignment does not pay off in the long run.
Adding a new definition, a slightly longer one perhaps, can easily break
formatting.
So I would stay with current one.
>
> > +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> > +int buf_size, u8 *data, int data_size)
> > +{
> > + /* every two received data bytes are checksummed */
> > + u8 tmp[data_size + data_size / 2];
>
> No VLAs!
>
> https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com/
>
Looks like -Wvla is some fairly recent addition to KBUILD_CFLAGS.
> > + int ret, i;
> > +
> > + /*
> > +* Sensor does not support repeated start so instead of
> > +* sending two i2c messages in a row we just send one by one.
> > +*/
> > + ret = i2c_master_send(state->client, buf, buf_size);
> > + if (ret != buf_size)
> > + return ret < 0 ? ret : -EIO;
> > +
> > + if (!data)
> > + return 0;
> > +
> > + ret = i2c_master_recv(state->client, tmp, sizeof(tmp));
> > + if (ret != sizeof(tmp))
> > + return ret < 0 ? ret : -EIO;
> > +
> > + for (i = 0; i < sizeof(tmp); i += 3) {
> > + u8 crc = crc8(sps30_crc8_table, [i], 2, CRC8_INIT_VALUE);
> > +
> > + if (crc != tmp[i + 2]) {
> > + dev_err(>client->dev,
> > + "data integrity check failed\n");
> > + return -EIO;
> > + }
> > +
> > + *data++ = tmp[i];
> > + *data++ = tmp[i + 1];
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int sps30_do_cmd(struct sps30_state *state, u16 cmd, u8 *data, int
> > size)
> > +{
> > + /* depending on the command up to 3 bytes may be needed for argument */
> > + u8 buf[sizeof(cmd) + 3] = { cmd >> 8, cmd };
>
> This is fine, since sizeof returns constant integer expression.
>
> > + switch (cmd) {
> > + case SPS30_START_MEAS:
> > + buf[2] = 0x03;
> > + buf[3] = 0x00;
> > + buf[4] = 0xac; /* precomputed crc */
> > + return sps30_write_then_read(state, buf, 5, NULL, 0);
> > + case SPS30_STOP_MEAS:
> > + case SPS30_RESET:
> > + return sps30_write_then_read(state, buf, 2, NULL, 0);
> > + case SPS30_READ_DATA_READY_FLAG:
> > + case SPS30_READ_DATA:
> > + case SPS30_READ_SERIAL:
> > + return sps30_write_then_read(state, buf, 2, data, size);
> > + default:
> > + return -EINVAL;
> > + };
> > +}
>
>
> > +static int sps30_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct sps30_state *state = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + switch (chan->type) {
> > + case IIO_MASSCONCENTRATION:
> > + mutex_lock(>lock);
> > + switch (chan->channel2) {
> > + case IIO_MOD_PM2p5:
>
> I think lock should be placed here
>
> > + ret = sps30_do_meas(state, val, val2);
>
> ... and unlock here.
>
> > + break;
> > + case IIO_MOD_PM10:
> > + ret = sps30_do_meas(state, val2, val);
> > + break;
> > + default:
> > + break;
> > + }
> > + mutex_unlock(>lock);
> > + if (ret)
> > + return ret;
> > +
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> > +
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sun, Nov 25, 2018 at 04:14:34PM +0530, Himanshu Jha wrote:
> On Sat, Nov 24, 2018 at 11:14:14PM +0100, Tomasz Duszynski wrote:
> > Add support for Sensirion SPS30 particulate matter sensor.
> >
> > Signed-off-by: Tomasz Duszynski
> > ---
> > drivers/iio/chemical/Kconfig | 11 ++
> > drivers/iio/chemical/Makefile | 1 +
> > drivers/iio/chemical/sps30.c | 359 ++
> > 3 files changed, 371 insertions(+)
> > create mode 100644 drivers/iio/chemical/sps30.c
>
> []
>
> > +#define pr_fmt(fmt) "sps30: " fmt
>
> I don't see its usage ?
>
Hint: checkout how dev_err() is defined.
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > +
> > +/* SPS30 commands */
> > +#define SPS30_START_MEAS 0x0010
> > +#define SPS30_STOP_MEAS 0x0104
> > +#define SPS30_RESET 0xd304
> > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > +#define SPS30_READ_DATA 0x0300
> > +#define SPS30_READ_SERIAL 0xD033
>
> Could you please put a tab and align these macros.
>
> #define SPS30_START_MEAS0x0010
> #define SPS30_STOP_MEAS 0x0104
>
In my opinion this sort of alignment does not pay off in the long run.
Adding a new definition, a slightly longer one perhaps, can easily break
formatting.
So I would stay with current one.
>
> > +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> > +int buf_size, u8 *data, int data_size)
> > +{
> > + /* every two received data bytes are checksummed */
> > + u8 tmp[data_size + data_size / 2];
>
> No VLAs!
>
> https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com/
>
Looks like -Wvla is some fairly recent addition to KBUILD_CFLAGS.
> > + int ret, i;
> > +
> > + /*
> > +* Sensor does not support repeated start so instead of
> > +* sending two i2c messages in a row we just send one by one.
> > +*/
> > + ret = i2c_master_send(state->client, buf, buf_size);
> > + if (ret != buf_size)
> > + return ret < 0 ? ret : -EIO;
> > +
> > + if (!data)
> > + return 0;
> > +
> > + ret = i2c_master_recv(state->client, tmp, sizeof(tmp));
> > + if (ret != sizeof(tmp))
> > + return ret < 0 ? ret : -EIO;
> > +
> > + for (i = 0; i < sizeof(tmp); i += 3) {
> > + u8 crc = crc8(sps30_crc8_table, [i], 2, CRC8_INIT_VALUE);
> > +
> > + if (crc != tmp[i + 2]) {
> > + dev_err(>client->dev,
> > + "data integrity check failed\n");
> > + return -EIO;
> > + }
> > +
> > + *data++ = tmp[i];
> > + *data++ = tmp[i + 1];
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int sps30_do_cmd(struct sps30_state *state, u16 cmd, u8 *data, int
> > size)
> > +{
> > + /* depending on the command up to 3 bytes may be needed for argument */
> > + u8 buf[sizeof(cmd) + 3] = { cmd >> 8, cmd };
>
> This is fine, since sizeof returns constant integer expression.
>
> > + switch (cmd) {
> > + case SPS30_START_MEAS:
> > + buf[2] = 0x03;
> > + buf[3] = 0x00;
> > + buf[4] = 0xac; /* precomputed crc */
> > + return sps30_write_then_read(state, buf, 5, NULL, 0);
> > + case SPS30_STOP_MEAS:
> > + case SPS30_RESET:
> > + return sps30_write_then_read(state, buf, 2, NULL, 0);
> > + case SPS30_READ_DATA_READY_FLAG:
> > + case SPS30_READ_DATA:
> > + case SPS30_READ_SERIAL:
> > + return sps30_write_then_read(state, buf, 2, data, size);
> > + default:
> > + return -EINVAL;
> > + };
> > +}
>
>
> > +static int sps30_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct sps30_state *state = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + switch (chan->type) {
> > + case IIO_MASSCONCENTRATION:
> > + mutex_lock(>lock);
> > + switch (chan->channel2) {
> > + case IIO_MOD_PM2p5:
>
> I think lock should be placed here
>
> > + ret = sps30_do_meas(state, val, val2);
>
> ... and unlock here.
>
> > + break;
> > + case IIO_MOD_PM10:
> > + ret = sps30_do_meas(state, val2, val);
> > + break;
> > + default:
> > + break;
> > + }
> > + mutex_unlock(>lock);
> > + if (ret)
> > + return ret;
> > +
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> > +
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sun, Nov 25, 2018 at 08:56:59AM +, Jonathan Cameron wrote:
> On Sat, 24 Nov 2018 23:14:14 +0100
> Tomasz Duszynski wrote:
>
> > Add support for Sensirion SPS30 particulate matter sensor.
> >
> > Signed-off-by: Tomasz Duszynski
> A few things inline.
>
> I'm particularly curious as to why we are ignoring two of the particle
> sizes that the sensor seems to capture?
>
I was thinking about adding first the most common ones i.e PM2.5 and
PM10 and the rest of them later on if there's a particular need.
On the other hand I can add PM1.0 and PM4.0 now so that we have
everything in place once new dust sensors appear on the market.
It's seems reasonable to assume they will measure the very same
subset of particulates.
> Also, a 'potential' race in remove that I'd like us to make
> 'obviously' correct rather than requiring hard thought on whether
> it would be a problem.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/chemical/Kconfig | 11 ++
> > drivers/iio/chemical/Makefile | 1 +
> > drivers/iio/chemical/sps30.c | 359 ++
> > 3 files changed, 371 insertions(+)
> > create mode 100644 drivers/iio/chemical/sps30.c
> >
> > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > index b8e005be4f87..40057ecf8130 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -61,6 +61,17 @@ config IAQCORE
> > iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> > sensors
> >
> > +config SPS30
> > + tristate "SPS30 Particulate Matter sensor"
> > + depends on I2C
> > + select CRC8
> > + help
> > + Say Y here to build support for the Sensirion SPS30 Particulate
> > + Matter sensor.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called sps30.
> > +
> > config VZ89X
> > tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> > depends on I2C
> > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > index 2f4c4ba4d781..9f42f4252151 100644
> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile
> > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> > obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > obj-$(CONFIG_CCS811) += ccs811.o
> > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > +obj-$(CONFIG_SPS30) += sps30.o
> > obj-$(CONFIG_VZ89X)+= vz89x.o
> > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > new file mode 100644
> > index ..bf802621ae7f
> > --- /dev/null
> > +++ b/drivers/iio/chemical/sps30.c
> > @@ -0,0 +1,359 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Sensirion SPS30 Particulate Matter sensor driver
> > + *
> > + * Copyright (c) Tomasz Duszynski
> > + *
> > + * I2C slave address: 0x69
> > + *
> > + * TODO:
> > + * - support for turning on fan cleaning
> > + * - support for reading/setting auto cleaning interval
> > + */
> > +
> > +#define pr_fmt(fmt) "sps30: " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > +
> > +/* SPS30 commands */
> > +#define SPS30_START_MEAS 0x0010
> > +#define SPS30_STOP_MEAS 0x0104
> > +#define SPS30_RESET 0xd304
> > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > +#define SPS30_READ_DATA 0x0300
> > +#define SPS30_READ_SERIAL 0xD033
> > +
> > +#define SPS30_CHAN(_index, _mod) { \
> > + .type = IIO_MASSCONCENTRATION, \
> > + .modified = 1, \
> > + .channel2 = IIO_MOD_ ## _mod, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .scan_index = _index, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 12, \
> > + .storagebits = 32, \
>
> That is unusual. Why do we need 32 bits to store a 12 bit value?
> 16 should be plenty. It'll make a difference to the buffer
> sizes if people just want to stream data and don't care about
> timestamps as it'll halve the memory usage.
Right, I could have picked up a petter values. But I've got at least one
valid reason for doing that. Sensor datasheet specifies 0-1000
measurement range but simple tests showed that SPS30 can measure way beyond
that. Interestingly, measurements are consistent with third party sensors.
So having 12/32 bit setting, especially 32 storagebits protects against
future changes in datasheet (which is btw preliminary) i.e updating
measurement range.
But, I guess that 16 for real and storage bits should be more
that enough.
>
> Also, convention has always been to got for next 8,16,32,64 above
> the realbits if there isn't a reason not to (shifting etc)
>
> How did we end up with 12 bits off the floating point conversion
> anyway? Needs some docs.
Matter of simple tests. I was able to trigger measurements of over
3000 ug/m3.
>
>
> > + .endianness = IIO_CPU, \
> > +
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sun, Nov 25, 2018 at 08:56:59AM +, Jonathan Cameron wrote:
> On Sat, 24 Nov 2018 23:14:14 +0100
> Tomasz Duszynski wrote:
>
> > Add support for Sensirion SPS30 particulate matter sensor.
> >
> > Signed-off-by: Tomasz Duszynski
> A few things inline.
>
> I'm particularly curious as to why we are ignoring two of the particle
> sizes that the sensor seems to capture?
>
I was thinking about adding first the most common ones i.e PM2.5 and
PM10 and the rest of them later on if there's a particular need.
On the other hand I can add PM1.0 and PM4.0 now so that we have
everything in place once new dust sensors appear on the market.
It's seems reasonable to assume they will measure the very same
subset of particulates.
> Also, a 'potential' race in remove that I'd like us to make
> 'obviously' correct rather than requiring hard thought on whether
> it would be a problem.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/chemical/Kconfig | 11 ++
> > drivers/iio/chemical/Makefile | 1 +
> > drivers/iio/chemical/sps30.c | 359 ++
> > 3 files changed, 371 insertions(+)
> > create mode 100644 drivers/iio/chemical/sps30.c
> >
> > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > index b8e005be4f87..40057ecf8130 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -61,6 +61,17 @@ config IAQCORE
> > iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> > sensors
> >
> > +config SPS30
> > + tristate "SPS30 Particulate Matter sensor"
> > + depends on I2C
> > + select CRC8
> > + help
> > + Say Y here to build support for the Sensirion SPS30 Particulate
> > + Matter sensor.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called sps30.
> > +
> > config VZ89X
> > tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> > depends on I2C
> > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > index 2f4c4ba4d781..9f42f4252151 100644
> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile
> > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> > obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > obj-$(CONFIG_CCS811) += ccs811.o
> > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > +obj-$(CONFIG_SPS30) += sps30.o
> > obj-$(CONFIG_VZ89X)+= vz89x.o
> > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > new file mode 100644
> > index ..bf802621ae7f
> > --- /dev/null
> > +++ b/drivers/iio/chemical/sps30.c
> > @@ -0,0 +1,359 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Sensirion SPS30 Particulate Matter sensor driver
> > + *
> > + * Copyright (c) Tomasz Duszynski
> > + *
> > + * I2C slave address: 0x69
> > + *
> > + * TODO:
> > + * - support for turning on fan cleaning
> > + * - support for reading/setting auto cleaning interval
> > + */
> > +
> > +#define pr_fmt(fmt) "sps30: " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > +
> > +/* SPS30 commands */
> > +#define SPS30_START_MEAS 0x0010
> > +#define SPS30_STOP_MEAS 0x0104
> > +#define SPS30_RESET 0xd304
> > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > +#define SPS30_READ_DATA 0x0300
> > +#define SPS30_READ_SERIAL 0xD033
> > +
> > +#define SPS30_CHAN(_index, _mod) { \
> > + .type = IIO_MASSCONCENTRATION, \
> > + .modified = 1, \
> > + .channel2 = IIO_MOD_ ## _mod, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .scan_index = _index, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 12, \
> > + .storagebits = 32, \
>
> That is unusual. Why do we need 32 bits to store a 12 bit value?
> 16 should be plenty. It'll make a difference to the buffer
> sizes if people just want to stream data and don't care about
> timestamps as it'll halve the memory usage.
Right, I could have picked up a petter values. But I've got at least one
valid reason for doing that. Sensor datasheet specifies 0-1000
measurement range but simple tests showed that SPS30 can measure way beyond
that. Interestingly, measurements are consistent with third party sensors.
So having 12/32 bit setting, especially 32 storagebits protects against
future changes in datasheet (which is btw preliminary) i.e updating
measurement range.
But, I guess that 16 for real and storage bits should be more
that enough.
>
> Also, convention has always been to got for next 8,16,32,64 above
> the realbits if there isn't a reason not to (shifting etc)
>
> How did we end up with 12 bits off the floating point conversion
> anyway? Needs some docs.
Matter of simple tests. I was able to trigger measurements of over
3000 ug/m3.
>
>
> > + .endianness = IIO_CPU, \
> > +
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sat, Nov 24, 2018 at 11:14:14PM +0100, Tomasz Duszynski wrote:
> Add support for Sensirion SPS30 particulate matter sensor.
>
> Signed-off-by: Tomasz Duszynski
> ---
> drivers/iio/chemical/Kconfig | 11 ++
> drivers/iio/chemical/Makefile | 1 +
> drivers/iio/chemical/sps30.c | 359 ++
> 3 files changed, 371 insertions(+)
> create mode 100644 drivers/iio/chemical/sps30.c
[]
> +#define pr_fmt(fmt) "sps30: " fmt
I don't see its usage ?
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SPS30_CRC8_POLYNOMIAL 0x31
> +
> +/* SPS30 commands */
> +#define SPS30_START_MEAS 0x0010
> +#define SPS30_STOP_MEAS 0x0104
> +#define SPS30_RESET 0xd304
> +#define SPS30_READ_DATA_READY_FLAG 0x0202
> +#define SPS30_READ_DATA 0x0300
> +#define SPS30_READ_SERIAL 0xD033
Could you please put a tab and align these macros.
#define SPS30_START_MEAS 0x0010
#define SPS30_STOP_MEAS 0x0104
> +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> + int buf_size, u8 *data, int data_size)
> +{
> + /* every two received data bytes are checksummed */
> + u8 tmp[data_size + data_size / 2];
No VLAs!
https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com/
> + int ret, i;
> +
> + /*
> + * Sensor does not support repeated start so instead of
> + * sending two i2c messages in a row we just send one by one.
> + */
> + ret = i2c_master_send(state->client, buf, buf_size);
> + if (ret != buf_size)
> + return ret < 0 ? ret : -EIO;
> +
> + if (!data)
> + return 0;
> +
> + ret = i2c_master_recv(state->client, tmp, sizeof(tmp));
> + if (ret != sizeof(tmp))
> + return ret < 0 ? ret : -EIO;
> +
> + for (i = 0; i < sizeof(tmp); i += 3) {
> + u8 crc = crc8(sps30_crc8_table, [i], 2, CRC8_INIT_VALUE);
> +
> + if (crc != tmp[i + 2]) {
> + dev_err(>client->dev,
> + "data integrity check failed\n");
> + return -EIO;
> + }
> +
> + *data++ = tmp[i];
> + *data++ = tmp[i + 1];
> + }
> +
> + return 0;
> +}
> +
> +static int sps30_do_cmd(struct sps30_state *state, u16 cmd, u8 *data, int
> size)
> +{
> + /* depending on the command up to 3 bytes may be needed for argument */
> + u8 buf[sizeof(cmd) + 3] = { cmd >> 8, cmd };
This is fine, since sizeof returns constant integer expression.
> + switch (cmd) {
> + case SPS30_START_MEAS:
> + buf[2] = 0x03;
> + buf[3] = 0x00;
> + buf[4] = 0xac; /* precomputed crc */
> + return sps30_write_then_read(state, buf, 5, NULL, 0);
> + case SPS30_STOP_MEAS:
> + case SPS30_RESET:
> + return sps30_write_then_read(state, buf, 2, NULL, 0);
> + case SPS30_READ_DATA_READY_FLAG:
> + case SPS30_READ_DATA:
> + case SPS30_READ_SERIAL:
> + return sps30_write_then_read(state, buf, 2, data, size);
> + default:
> + return -EINVAL;
> + };
> +}
> +static int sps30_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct sps30_state *state = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + switch (chan->type) {
> + case IIO_MASSCONCENTRATION:
> + mutex_lock(>lock);
> + switch (chan->channel2) {
> + case IIO_MOD_PM2p5:
I think lock should be placed here
> + ret = sps30_do_meas(state, val, val2);
... and unlock here.
> + break;
> + case IIO_MOD_PM10:
> + ret = sps30_do_meas(state, val2, val);
> + break;
> + default:
> + break;
> + }
> + mutex_unlock(>lock);
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +}
[]
> +static int sps30_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct sps30_state *state;
> + u8 buf[32] = { };
This is not valid in ISO-C but only in C++.
Instead,
u8 buf[32] = {0};
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -EOPNOTSUPP;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*state));
> +
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sat, Nov 24, 2018 at 11:14:14PM +0100, Tomasz Duszynski wrote:
> Add support for Sensirion SPS30 particulate matter sensor.
>
> Signed-off-by: Tomasz Duszynski
> ---
> drivers/iio/chemical/Kconfig | 11 ++
> drivers/iio/chemical/Makefile | 1 +
> drivers/iio/chemical/sps30.c | 359 ++
> 3 files changed, 371 insertions(+)
> create mode 100644 drivers/iio/chemical/sps30.c
[]
> +#define pr_fmt(fmt) "sps30: " fmt
I don't see its usage ?
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SPS30_CRC8_POLYNOMIAL 0x31
> +
> +/* SPS30 commands */
> +#define SPS30_START_MEAS 0x0010
> +#define SPS30_STOP_MEAS 0x0104
> +#define SPS30_RESET 0xd304
> +#define SPS30_READ_DATA_READY_FLAG 0x0202
> +#define SPS30_READ_DATA 0x0300
> +#define SPS30_READ_SERIAL 0xD033
Could you please put a tab and align these macros.
#define SPS30_START_MEAS 0x0010
#define SPS30_STOP_MEAS 0x0104
> +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> + int buf_size, u8 *data, int data_size)
> +{
> + /* every two received data bytes are checksummed */
> + u8 tmp[data_size + data_size / 2];
No VLAs!
https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com/
> + int ret, i;
> +
> + /*
> + * Sensor does not support repeated start so instead of
> + * sending two i2c messages in a row we just send one by one.
> + */
> + ret = i2c_master_send(state->client, buf, buf_size);
> + if (ret != buf_size)
> + return ret < 0 ? ret : -EIO;
> +
> + if (!data)
> + return 0;
> +
> + ret = i2c_master_recv(state->client, tmp, sizeof(tmp));
> + if (ret != sizeof(tmp))
> + return ret < 0 ? ret : -EIO;
> +
> + for (i = 0; i < sizeof(tmp); i += 3) {
> + u8 crc = crc8(sps30_crc8_table, [i], 2, CRC8_INIT_VALUE);
> +
> + if (crc != tmp[i + 2]) {
> + dev_err(>client->dev,
> + "data integrity check failed\n");
> + return -EIO;
> + }
> +
> + *data++ = tmp[i];
> + *data++ = tmp[i + 1];
> + }
> +
> + return 0;
> +}
> +
> +static int sps30_do_cmd(struct sps30_state *state, u16 cmd, u8 *data, int
> size)
> +{
> + /* depending on the command up to 3 bytes may be needed for argument */
> + u8 buf[sizeof(cmd) + 3] = { cmd >> 8, cmd };
This is fine, since sizeof returns constant integer expression.
> + switch (cmd) {
> + case SPS30_START_MEAS:
> + buf[2] = 0x03;
> + buf[3] = 0x00;
> + buf[4] = 0xac; /* precomputed crc */
> + return sps30_write_then_read(state, buf, 5, NULL, 0);
> + case SPS30_STOP_MEAS:
> + case SPS30_RESET:
> + return sps30_write_then_read(state, buf, 2, NULL, 0);
> + case SPS30_READ_DATA_READY_FLAG:
> + case SPS30_READ_DATA:
> + case SPS30_READ_SERIAL:
> + return sps30_write_then_read(state, buf, 2, data, size);
> + default:
> + return -EINVAL;
> + };
> +}
> +static int sps30_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct sps30_state *state = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + switch (chan->type) {
> + case IIO_MASSCONCENTRATION:
> + mutex_lock(>lock);
> + switch (chan->channel2) {
> + case IIO_MOD_PM2p5:
I think lock should be placed here
> + ret = sps30_do_meas(state, val, val2);
... and unlock here.
> + break;
> + case IIO_MOD_PM10:
> + ret = sps30_do_meas(state, val2, val);
> + break;
> + default:
> + break;
> + }
> + mutex_unlock(>lock);
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +}
[]
> +static int sps30_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct sps30_state *state;
> + u8 buf[32] = { };
This is not valid in ISO-C but only in C++.
Instead,
u8 buf[32] = {0};
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -EOPNOTSUPP;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*state));
> +
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sat, 24 Nov 2018 23:14:14 +0100
Tomasz Duszynski wrote:
> Add support for Sensirion SPS30 particulate matter sensor.
>
> Signed-off-by: Tomasz Duszynski
A few things inline.
I'm particularly curious as to why we are ignoring two of the particle
sizes that the sensor seems to capture?
Also, a 'potential' race in remove that I'd like us to make
'obviously' correct rather than requiring hard thought on whether
it would be a problem.
Thanks,
Jonathan
> ---
> drivers/iio/chemical/Kconfig | 11 ++
> drivers/iio/chemical/Makefile | 1 +
> drivers/iio/chemical/sps30.c | 359 ++
> 3 files changed, 371 insertions(+)
> create mode 100644 drivers/iio/chemical/sps30.c
>
> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> index b8e005be4f87..40057ecf8130 100644
> --- a/drivers/iio/chemical/Kconfig
> +++ b/drivers/iio/chemical/Kconfig
> @@ -61,6 +61,17 @@ config IAQCORE
> iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> sensors
>
> +config SPS30
> + tristate "SPS30 Particulate Matter sensor"
> + depends on I2C
> + select CRC8
> + help
> + Say Y here to build support for the Sensirion SPS30 Particulate
> + Matter sensor.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called sps30.
> +
> config VZ89X
> tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> depends on I2C
> diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> index 2f4c4ba4d781..9f42f4252151 100644
> --- a/drivers/iio/chemical/Makefile
> +++ b/drivers/iio/chemical/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> obj-$(CONFIG_CCS811) += ccs811.o
> obj-$(CONFIG_IAQCORE)+= ams-iaq-core.o
> +obj-$(CONFIG_SPS30) += sps30.o
> obj-$(CONFIG_VZ89X) += vz89x.o
> diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> new file mode 100644
> index ..bf802621ae7f
> --- /dev/null
> +++ b/drivers/iio/chemical/sps30.c
> @@ -0,0 +1,359 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sensirion SPS30 Particulate Matter sensor driver
> + *
> + * Copyright (c) Tomasz Duszynski
> + *
> + * I2C slave address: 0x69
> + *
> + * TODO:
> + * - support for turning on fan cleaning
> + * - support for reading/setting auto cleaning interval
> + */
> +
> +#define pr_fmt(fmt) "sps30: " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SPS30_CRC8_POLYNOMIAL 0x31
> +
> +/* SPS30 commands */
> +#define SPS30_START_MEAS 0x0010
> +#define SPS30_STOP_MEAS 0x0104
> +#define SPS30_RESET 0xd304
> +#define SPS30_READ_DATA_READY_FLAG 0x0202
> +#define SPS30_READ_DATA 0x0300
> +#define SPS30_READ_SERIAL 0xD033
> +
> +#define SPS30_CHAN(_index, _mod) { \
> + .type = IIO_MASSCONCENTRATION, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_ ## _mod, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .scan_index = _index, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 12, \
> + .storagebits = 32, \
That is unusual. Why do we need 32 bits to store a 12 bit value?
16 should be plenty. It'll make a difference to the buffer
sizes if people just want to stream data and don't care about
timestamps as it'll halve the memory usage.
Also, convention has always been to got for next 8,16,32,64 above
the realbits if there isn't a reason not to (shifting etc)
How did we end up with 12 bits off the floating point conversion
anyway? Needs some docs.
> + .endianness = IIO_CPU, \
> + }, \
> +}
> +
> +enum {
> + PM1p0, /* just a placeholder */
> + PM2p5,
> + PM4p0, /* just a placeholder */
> + PM10,
> +};
> +
> +struct sps30_state {
> + struct i2c_client *client;
> + /* guards against concurrent access to sensor registers */
> + struct mutex lock;
> +};
> +
> +DECLARE_CRC8_TABLE(sps30_crc8_table);
> +
I think you are fine locking wise, but it would be good to add a note
on what locks are expected to be held on entering this function.
Without locks it's obviously very racey!
> +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> + int buf_size, u8 *data, int data_size)
> +{
> + /* every two received data bytes are checksummed */
> + u8 tmp[data_size + data_size / 2];
> + int ret, i;
> +
> + /*
> + * Sensor does not support repeated start so instead of
> + * sending two i2c messages in a row we just send one by one.
> + */
> + ret = i2c_master_send(state->client, buf, buf_size);
> + if (ret != buf_size)
> + return ret < 0 ? ret : -EIO;
> +
> + if (!data)
> + return 0;
> +
> + ret = i2c_master_recv(state->client, tmp, sizeof(tmp));
> +
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
On Sat, 24 Nov 2018 23:14:14 +0100
Tomasz Duszynski wrote:
> Add support for Sensirion SPS30 particulate matter sensor.
>
> Signed-off-by: Tomasz Duszynski
A few things inline.
I'm particularly curious as to why we are ignoring two of the particle
sizes that the sensor seems to capture?
Also, a 'potential' race in remove that I'd like us to make
'obviously' correct rather than requiring hard thought on whether
it would be a problem.
Thanks,
Jonathan
> ---
> drivers/iio/chemical/Kconfig | 11 ++
> drivers/iio/chemical/Makefile | 1 +
> drivers/iio/chemical/sps30.c | 359 ++
> 3 files changed, 371 insertions(+)
> create mode 100644 drivers/iio/chemical/sps30.c
>
> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> index b8e005be4f87..40057ecf8130 100644
> --- a/drivers/iio/chemical/Kconfig
> +++ b/drivers/iio/chemical/Kconfig
> @@ -61,6 +61,17 @@ config IAQCORE
> iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> sensors
>
> +config SPS30
> + tristate "SPS30 Particulate Matter sensor"
> + depends on I2C
> + select CRC8
> + help
> + Say Y here to build support for the Sensirion SPS30 Particulate
> + Matter sensor.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called sps30.
> +
> config VZ89X
> tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> depends on I2C
> diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> index 2f4c4ba4d781..9f42f4252151 100644
> --- a/drivers/iio/chemical/Makefile
> +++ b/drivers/iio/chemical/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> obj-$(CONFIG_CCS811) += ccs811.o
> obj-$(CONFIG_IAQCORE)+= ams-iaq-core.o
> +obj-$(CONFIG_SPS30) += sps30.o
> obj-$(CONFIG_VZ89X) += vz89x.o
> diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> new file mode 100644
> index ..bf802621ae7f
> --- /dev/null
> +++ b/drivers/iio/chemical/sps30.c
> @@ -0,0 +1,359 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sensirion SPS30 Particulate Matter sensor driver
> + *
> + * Copyright (c) Tomasz Duszynski
> + *
> + * I2C slave address: 0x69
> + *
> + * TODO:
> + * - support for turning on fan cleaning
> + * - support for reading/setting auto cleaning interval
> + */
> +
> +#define pr_fmt(fmt) "sps30: " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SPS30_CRC8_POLYNOMIAL 0x31
> +
> +/* SPS30 commands */
> +#define SPS30_START_MEAS 0x0010
> +#define SPS30_STOP_MEAS 0x0104
> +#define SPS30_RESET 0xd304
> +#define SPS30_READ_DATA_READY_FLAG 0x0202
> +#define SPS30_READ_DATA 0x0300
> +#define SPS30_READ_SERIAL 0xD033
> +
> +#define SPS30_CHAN(_index, _mod) { \
> + .type = IIO_MASSCONCENTRATION, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_ ## _mod, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .scan_index = _index, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 12, \
> + .storagebits = 32, \
That is unusual. Why do we need 32 bits to store a 12 bit value?
16 should be plenty. It'll make a difference to the buffer
sizes if people just want to stream data and don't care about
timestamps as it'll halve the memory usage.
Also, convention has always been to got for next 8,16,32,64 above
the realbits if there isn't a reason not to (shifting etc)
How did we end up with 12 bits off the floating point conversion
anyway? Needs some docs.
> + .endianness = IIO_CPU, \
> + }, \
> +}
> +
> +enum {
> + PM1p0, /* just a placeholder */
> + PM2p5,
> + PM4p0, /* just a placeholder */
> + PM10,
> +};
> +
> +struct sps30_state {
> + struct i2c_client *client;
> + /* guards against concurrent access to sensor registers */
> + struct mutex lock;
> +};
> +
> +DECLARE_CRC8_TABLE(sps30_crc8_table);
> +
I think you are fine locking wise, but it would be good to add a note
on what locks are expected to be held on entering this function.
Without locks it's obviously very racey!
> +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> + int buf_size, u8 *data, int data_size)
> +{
> + /* every two received data bytes are checksummed */
> + u8 tmp[data_size + data_size / 2];
> + int ret, i;
> +
> + /*
> + * Sensor does not support repeated start so instead of
> + * sending two i2c messages in a row we just send one by one.
> + */
> + ret = i2c_master_send(state->client, buf, buf_size);
> + if (ret != buf_size)
> + return ret < 0 ? ret : -EIO;
> +
> + if (!data)
> + return 0;
> +
> + ret = i2c_master_recv(state->client, tmp, sizeof(tmp));
> +
