[PATCH 0/3] add support for Sensirion SPS30 PM sensor

[PATCH 0/3] add support for Sensirion SPS30 PM sensor

[Tomasz Duszynski]

This patch series adds support for Sensirion SPS30 particulate matter
sensor. Along with a driver itself, new channel type and
two modifiers for distinguishing between coarse and fine particles
measurements are introduced.

Sensor datasheet can be downloaded from

https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/0_Datasheets/Particulate_Matter/Sensirion_PM_Sensors_SPS30_Datasheet.pdf

Tomasz Duszynski (3):
  iio: add IIO_MASSCONCENTRATION channel type
  iio: chemical: add support for Sensirion SPS30 sensor
  iio: chemical: sps30: add device tree support

 Documentation/ABI/testing/sysfs-bus-iio       |  11 +-
 .../bindings/iio/chemical/sensirion,sps30.txt |  12 +
 drivers/iio/chemical/Kconfig                  |  11 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/sps30.c                  | 359 ++++++++++++++++++
 drivers/iio/industrialio-core.c               |   3 +
 include/uapi/linux/iio/types.h                |   3 +
 tools/iio/iio_event_monitor.c                 |   6 +
 8 files changed, 405 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
 create mode 100644 drivers/iio/chemical/sps30.c

--
2.19.2

[PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Tomasz Duszynski]

Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
is de facto standard. Existing air quality sensors usually follow
this convention and are capable of returning measurements using
this unit.

IIO currently does not offer suitable channel type for this
type of measurements hence this patch adds this.

In addition, two modifiers are introduced used for distinguishing
between coarse (PM10) and fine particles (PM2p5) measurements, i.e
IIO_MOD_PM10 and IIO_MOD_PM2p5.

Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
 drivers/iio/industrialio-core.c         |  3 +++
 include/uapi/linux/iio/types.h          |  3 +++
 tools/iio/iio_event_monitor.c           |  6 ++++++
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 8127a08e366d..ce0ed140660d 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1684,4 +1684,13 @@ KernelVersion:	4.18
 Contact:	linux-iio@vger.kernel.org
 Description:
 		Raw (unscaled) phase difference reading from channel Y
-		that can be processed to radians.
\ No newline at end of file
+		that can be processed to radians.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
+What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
+What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
+What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
+KernelVersion:	4.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Mass concentration reading of particulate matter in ug / m3.
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index a062cfddc5af..2a9ef600c1fb 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_GRAVITY]  = "gravity",
 	[IIO_POSITIONRELATIVE]  = "positionrelative",
 	[IIO_PHASE] = "phase",
+	[IIO_MASSCONCENTRATION] = "massconcentration",
 };
 
 static const char * const iio_modifier_names[] = {
@@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
 	[IIO_MOD_Q] = "q",
 	[IIO_MOD_CO2] = "co2",
 	[IIO_MOD_VOC] = "voc",
+	[IIO_MOD_PM2p5] = "pm2p5",
+	[IIO_MOD_PM10] = "pm10",
 };
 
 /* relies on pairs of these shared then separate */
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 92baabc103ac..465044b42af5 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -46,6 +46,7 @@ enum iio_chan_type {
 	IIO_GRAVITY,
 	IIO_POSITIONRELATIVE,
 	IIO_PHASE,
+	IIO_MASSCONCENTRATION,
 };
 
 enum iio_modifier {
@@ -87,6 +88,8 @@ enum iio_modifier {
 	IIO_MOD_VOC,
 	IIO_MOD_LIGHT_UV,
 	IIO_MOD_LIGHT_DUV,
+	IIO_MOD_PM2p5,
+	IIO_MOD_PM10,
 };
 
 enum iio_event_type {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index ac2de6b7e89f..f0fcfeddba2b 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_GRAVITY] = "gravity",
 	[IIO_POSITIONRELATIVE] = "positionrelative",
 	[IIO_PHASE] = "phase",
+	[IIO_MASSCONCENTRATION] = "massconcentration",
 };
 
 static const char * const iio_ev_type_text[] = {
@@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
 	[IIO_MOD_Q] = "q",
 	[IIO_MOD_CO2] = "co2",
 	[IIO_MOD_VOC] = "voc",
+	[IIO_MOD_PM2p5] = "pm2p5",
+	[IIO_MOD_PM10] = "pm10",
 };
 
 static bool event_is_known(struct iio_event_data *event)
@@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_GRAVITY:
 	case IIO_POSITIONRELATIVE:
 	case IIO_PHASE:
+	case IIO_MASSCONCENTRATION:
 		break;
 	default:
 		return false;
@@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_MOD_Q:
 	case IIO_MOD_CO2:
 	case IIO_MOD_VOC:
+	case IIO_MOD_PM2p5:
+	case IIO_MOD_PM10:
 		break;
 	default:
 		return false;
-- 
2.19.2

[PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Tomasz Duszynski]

Add support for Sensirion SPS30 particulate matter sensor.

Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
---
 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 000000000000..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 <tduszyns@gmail.com>
+ *
+ * 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 <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/module.h>
+
+#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, \
+		.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);
+
+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));
+	if (ret != sizeof(tmp))
+		return ret < 0 ? ret : -EIO;
+
+	for (i = 0; i < sizeof(tmp); i += 3) {
+		u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE);
+
+		if (crc != tmp[i + 2]) {
+			dev_err(&state->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 };
+
+	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_ieee754_to_int(const u8 *data)
+{
+	u32 val = ((u32)data[0] << 24) | ((u32)data[1] << 16) |
+		  ((u32)data[2] << 8) | (u32)data[3],
+	    mantissa = (val << 9) >> 9;
+	int exp = (val >> 23) - 127;
+
+	if (!exp && !mantissa)
+		return 0;
+
+	if (exp < 0)
+		return 0;
+
+	return (1 << exp) + (mantissa >> (23 - exp));
+}
+
+static int sps30_do_meas(struct sps30_state *state, int *pm2p5, int *pm10)
+{
+	/*
+	 * Internally sensor stores measurements in a following manner:
+	 *
+	 * PM1p0: upper two bytes, crc8, lower two bytes, crc8
+	 * PM2p5: upper two bytes, crc8, lower two bytes, crc8
+	 * PM4p0: upper two bytes, crc8, lower two bytes, crc8
+	 * PM10:  upper two bytes, crc8, lower two bytes, crc8
+	 *
+	 * What follows next are number concentration measurements and
+	 * typical particle size measurement.
+	 *
+	 * Once data is read from sensor crc bytes are stripped off
+	 * hence we need 16 bytes of buffer space.
+	 */
+	int ret, tries = 5;
+	u8 buf[16];
+
+	while (tries--) {
+		ret = sps30_do_cmd(state, SPS30_READ_DATA_READY_FLAG, buf, 2);
+		if (ret)
+			return -EIO;
+
+		/* new measurements ready to be read */
+		if (buf[1] == 1)
+			break;
+
+		usleep_range(300000, 400000);
+	}
+
+	if (!tries)
+		return -ETIMEDOUT;
+
+	ret = sps30_do_cmd(state, SPS30_READ_DATA, buf, sizeof(buf));
+	if (ret)
+		return ret;
+
+	/*
+	 * All measurements come in IEEE754 single precision floating point
+	 * format but sensor itself is not precise enough (-+ 10% error)
+	 * to take full advantage of it. Hence converting result to int
+	 * to keep things simple.
+	 */
+	*pm2p5 = sps30_ieee754_to_int(&buf[PM2p5 * 4]);
+	*pm10 = sps30_ieee754_to_int(&buf[PM10 * 4]);
+
+	return 0;
+}
+
+static irqreturn_t sps30_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct sps30_state *state = iio_priv(indio_dev);
+	u32 buf[4]; /* PM2p5, PM10, timestamp */
+	int ret;
+
+	mutex_lock(&state->lock);
+	ret = sps30_do_meas(state, &buf[0], &buf[1]);
+	mutex_unlock(&state->lock);
+	if (ret < 0)
+		goto err;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+					   iio_get_time_ns(indio_dev));
+err:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+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(&state->lock);
+			switch (chan->channel2) {
+			case IIO_MOD_PM2p5:
+				ret = sps30_do_meas(state, val, val2);
+				break;
+			case IIO_MOD_PM10:
+				ret = sps30_do_meas(state, val2, val);
+				break;
+			default:
+				break;
+			}
+			mutex_unlock(&state->lock);
+			if (ret)
+				return ret;
+
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info sps30_info = {
+	.read_raw = sps30_read_raw,
+};
+
+static const struct iio_chan_spec sps30_channels[] = {
+	SPS30_CHAN(0, PM2p5),
+	SPS30_CHAN(1, PM10),
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+static const unsigned long sps30_scan_masks[] = { 0x03, 0x00 };
+
+static int sps30_probe(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev;
+	struct sps30_state *state;
+	u8 buf[32] = { };
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -EOPNOTSUPP;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*state));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	state = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	state->client = client;
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &sps30_info;
+	indio_dev->name = client->name;
+	indio_dev->channels = sps30_channels;
+	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->available_scan_masks = sps30_scan_masks;
+
+	mutex_init(&state->lock);
+	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
+
+	/*
+	 * Power-on-reset causes sensor to produce some glitch on i2c bus
+	 * and some controllers end up in error state. Recover simply
+	 * by placing something on the bus.
+	 */
+	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
+	if (ret) {
+		dev_err(&client->dev, "failed to reset device\n");
+		return ret;
+	}
+	usleep_range(2500000, 3500000);
+	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
+
+	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
+	if (ret) {
+		dev_err(&client->dev, "failed to read serial number\n");
+		return ret;
+	}
+	dev_info(&client->dev, "serial number: %s\n", buf);
+
+	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
+	if (ret) {
+		dev_err(&client->dev, "failed to start measurement\n");
+		return ret;
+	}
+
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+					      sps30_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static int sps30_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct sps30_state *state = iio_priv(indio_dev);
+
+	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
+
+	return 0;
+}
+
+static const struct i2c_device_id sps30_id[] = {
+	{ "sps30" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, sps30_id);
+
+static const struct of_device_id sps30_of_match[] = {
+	{ .compatible = "sensirion,sps30" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sps30_of_match);
+
+static struct i2c_driver sps30_driver = {
+	.driver = {
+		.name = "sps30",
+		.of_match_table = sps30_of_match,
+	},
+	.id_table = sps30_id,
+	.probe_new = sps30_probe,
+	.remove = sps30_remove,
+};
+module_i2c_driver(sps30_driver);
+
+MODULE_AUTHOR("Tomasz Duszynski <tduszyns@gmail.com>");
+MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
2.19.2

[PATCH 3/3] iio: chemical: sps30: add device tree support

[Tomasz Duszynski]

Add device tree support for Sensirion SPS30 particulate
matter sensor.

Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
---
 .../bindings/iio/chemical/sensirion,sps30.txt        | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt

diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt b/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
new file mode 100644
index 000000000000..6eee2709b5b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
@@ -0,0 +1,12 @@
+* Sensirion SPS30 particulate matter sensor
+
+Required properties:
+- compatible: must be "sensirion,sps30"
+- reg: the I2C address of the sensor
+
+Example:
+
+sps30@69 {
+	compatible = "sensirion,sps30";
+	reg = <0x69>;
+};
-- 
2.19.2

Re: [PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Jonathan Cameron]

On Sat, 24 Nov 2018 23:14:13 +0100
Tomasz Duszynski <tduszyns@gmail.com> wrote:

> Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> is de facto standard. Existing air quality sensors usually follow
> this convention and are capable of returning measurements using
> this unit.
> 
> IIO currently does not offer suitable channel type for this
> type of measurements hence this patch adds this.
> 
> In addition, two modifiers are introduced used for distinguishing
> between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> IIO_MOD_PM10 and IIO_MOD_PM2p5.
I initially wondered why these particular numbers and why don't
we provide an attribute specify this separately?

However google suggests these two are pretty much the only
ones anyone worries about in pollution sensors.  I ended
up fairly quickly at the EPA website
https://www.epa.gov/pm-pollution/table-historical-particulate-matter-pm-national-ambient-air-quality-standards-naaqs
which tells me these two have be used since about 1987.

So I think the modifier route is the right approach here
(I think we've gotten this wrong in the past such as light
sensor colours where the list keeps on growing).

I would like a reference or two in the docs though to point
people to these definitions.

Thanks,

Jonathan

> 
> Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
>  drivers/iio/industrialio-core.c         |  3 +++
>  include/uapi/linux/iio/types.h          |  3 +++
>  tools/iio/iio_event_monitor.c           |  6 ++++++
>  4 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 8127a08e366d..ce0ed140660d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1684,4 +1684,13 @@ KernelVersion:	4.18
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Raw (unscaled) phase difference reading from channel Y
> -		that can be processed to radians.
> \ No newline at end of file
> +		that can be processed to radians.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> +What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> +What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> +What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> +KernelVersion:	4.21
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Mass concentration reading of particulate matter in ug / m3.
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index a062cfddc5af..2a9ef600c1fb 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
>  	[IIO_GRAVITY]  = "gravity",
>  	[IIO_POSITIONRELATIVE]  = "positionrelative",
>  	[IIO_PHASE] = "phase",
> +	[IIO_MASSCONCENTRATION] = "massconcentration",
>  };
>  
>  static const char * const iio_modifier_names[] = {
> @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
>  	[IIO_MOD_Q] = "q",
>  	[IIO_MOD_CO2] = "co2",
>  	[IIO_MOD_VOC] = "voc",
> +	[IIO_MOD_PM2p5] = "pm2p5",
> +	[IIO_MOD_PM10] = "pm10",
>  };
>  
>  /* relies on pairs of these shared then separate */
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index 92baabc103ac..465044b42af5 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -46,6 +46,7 @@ enum iio_chan_type {
>  	IIO_GRAVITY,
>  	IIO_POSITIONRELATIVE,
>  	IIO_PHASE,
> +	IIO_MASSCONCENTRATION,
>  };
>  
>  enum iio_modifier {
> @@ -87,6 +88,8 @@ enum iio_modifier {
>  	IIO_MOD_VOC,
>  	IIO_MOD_LIGHT_UV,
>  	IIO_MOD_LIGHT_DUV,
> +	IIO_MOD_PM2p5,
> +	IIO_MOD_PM10,
>  };
>  
>  enum iio_event_type {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index ac2de6b7e89f..f0fcfeddba2b 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
>  	[IIO_GRAVITY] = "gravity",
>  	[IIO_POSITIONRELATIVE] = "positionrelative",
>  	[IIO_PHASE] = "phase",
> +	[IIO_MASSCONCENTRATION] = "massconcentration",
>  };
>  
>  static const char * const iio_ev_type_text[] = {
> @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
>  	[IIO_MOD_Q] = "q",
>  	[IIO_MOD_CO2] = "co2",
>  	[IIO_MOD_VOC] = "voc",
> +	[IIO_MOD_PM2p5] = "pm2p5",
> +	[IIO_MOD_PM10] = "pm10",
>  };
>  
>  static bool event_is_known(struct iio_event_data *event)
> @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
>  	case IIO_GRAVITY:
>  	case IIO_POSITIONRELATIVE:
>  	case IIO_PHASE:
> +	case IIO_MASSCONCENTRATION:
>  		break;
>  	default:
>  		return false;
> @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
>  	case IIO_MOD_Q:
>  	case IIO_MOD_CO2:
>  	case IIO_MOD_VOC:
> +	case IIO_MOD_PM2p5:
> +	case IIO_MOD_PM10:
>  		break;
>  	default:
>  		return false;

Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Jonathan Cameron]

On Sat, 24 Nov 2018 23:14:14 +0100
Tomasz Duszynski <tduszyns@gmail.com> wrote:

> Add support for Sensirion SPS30 particulate matter sensor.
> 
> Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
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 000000000000..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 <tduszyns@gmail.com>
> + *
> + * 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 <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/module.h>
> +
> +#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));
> +	if (ret != sizeof(tmp))
> +		return ret < 0 ? ret : -EIO;
> +
> +	for (i = 0; i < sizeof(tmp); i += 3) {
> +		u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE);
> +
> +		if (crc != tmp[i + 2]) {
> +			dev_err(&state->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 };
> +
> +	switch (cmd) {
> +	case SPS30_START_MEAS:
> +		buf[2] = 0x03;
> +		buf[3] = 0x00;
> +		buf[4] = 0xac; /* precomputed crc */

Could you put that in code and let the compiler do the pre compute?
Might be a little more elegant.

> +		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_ieee754_to_int(const u8 *data)
> +{
> +	u32 val = ((u32)data[0] << 24) | ((u32)data[1] << 16) |
> +		  ((u32)data[2] << 8) | (u32)data[3],
Have a separate
u32 mantissa = (val << 9) >> 9 line.
What is this next line actually doing?  Kind of looks like
a mask?  If so just mask it with & as more readable.

> +	    mantissa = (val << 9) >> 9;
> +	int exp = (val >> 23) - 127;
> +
> +	if (!exp && !mantissa)
> +		return 0;
> +
> +	if (exp < 0)
> +		return 0;
> +
> +	return (1 << exp) + (mantissa >> (23 - exp));
> +}
> +
> +static int sps30_do_meas(struct sps30_state *state, int *pm2p5, int *pm10)
> +{
> +	/*
> +	 * Internally sensor stores measurements in a following manner:
> +	 *
> +	 * PM1p0: upper two bytes, crc8, lower two bytes, crc8

So there are other particle sizes that this sensor reads?  Why
are we mapping them down to just the two channel types?

> +	 * PM2p5: upper two bytes, crc8, lower two bytes, crc8
> +	 * PM4p0: upper two bytes, crc8, lower two bytes, crc8
> +	 * PM10:  upper two bytes, crc8, lower two bytes, crc8
> +	 *
> +	 * What follows next are number concentration measurements and
> +	 * typical particle size measurement.
> +	 *
> +	 * Once data is read from sensor crc bytes are stripped off
> +	 * hence we need 16 bytes of buffer space.
> +	 */
> +	int ret, tries = 5;
> +	u8 buf[16];
> +
> +	while (tries--) {
> +		ret = sps30_do_cmd(state, SPS30_READ_DATA_READY_FLAG, buf, 2);
> +		if (ret)
> +			return -EIO;
> +
> +		/* new measurements ready to be read */
> +		if (buf[1] == 1)
> +			break;
> +
> +		usleep_range(300000, 400000);
> +	}
> +
> +	if (!tries)
> +		return -ETIMEDOUT;
> +
> +	ret = sps30_do_cmd(state, SPS30_READ_DATA, buf, sizeof(buf));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * All measurements come in IEEE754 single precision floating point
> +	 * format but sensor itself is not precise enough (-+ 10% error)
> +	 * to take full advantage of it. Hence converting result to int
> +	 * to keep things simple.

Interesting.  We have talked about allowing floating point formats for
sensors the provide them.  Would that be beneficial here?

> +	 */
> +	*pm2p5 = sps30_ieee754_to_int(&buf[PM2p5 * 4]);
> +	*pm10 = sps30_ieee754_to_int(&buf[PM10 * 4]);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t sps30_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct sps30_state *state = iio_priv(indio_dev);
> +	u32 buf[4]; /* PM2p5, PM10, timestamp */
> +	int ret;
> +
> +	mutex_lock(&state->lock);
> +	ret = sps30_do_meas(state, &buf[0], &buf[1]);
> +	mutex_unlock(&state->lock);
> +	if (ret < 0)
> +		goto err;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> +					   iio_get_time_ns(indio_dev));
> +err:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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(&state->lock);
> +			switch (chan->channel2) {
> +			case IIO_MOD_PM2p5:
> +				ret = sps30_do_meas(state, val, val2);
> +				break;
> +			case IIO_MOD_PM10:
> +				ret = sps30_do_meas(state, val2, val);
> +				break;
> +			default:
> +				break;
> +			}
> +			mutex_unlock(&state->lock);
> +			if (ret)
> +				return ret;
> +
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
You can't get here.  Is this a warning suppression?
If so just add a comment saying so to prevent it being removed
by someone else later.

> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info sps30_info = {
> +	.read_raw = sps30_read_raw,
> +};
> +
From a readability point of view, it would be helpful to pull
the macro SPS30_CHAN down here in the code so we don't
need to go jumping around to check what it is doing.

> +static const struct iio_chan_spec sps30_channels[] = {
> +	SPS30_CHAN(0, PM2p5),
> +	SPS30_CHAN(1, PM10),
> +	IIO_CHAN_SOFT_TIMESTAMP(2),
> +};
> +
> +static const unsigned long sps30_scan_masks[] = { 0x03, 0x00 };
> +
> +static int sps30_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct sps30_state *state;
> +	u8 buf[32] = { };
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	state->client = client;
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &sps30_info;
> +	indio_dev->name = client->name;
> +	indio_dev->channels = sps30_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->available_scan_masks = sps30_scan_masks;
> +
> +	mutex_init(&state->lock);
> +	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> +
> +	/*
> +	 * Power-on-reset causes sensor to produce some glitch on i2c bus
> +	 * and some controllers end up in error state. Recover simply
> +	 * by placing something on the bus.
> +	 */
> +	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to reset device\n");
> +		return ret;
> +	}
> +	usleep_range(2500000, 3500000);
> +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);

Talk me through this logic.  We stop it then pretty much immediately
start it again?  Needs a comment.

> +
> +	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read serial number\n");
> +		return ret;
> +	}
> +	dev_info(&client->dev, "serial number: %s\n", buf);
> +
> +	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> +	if (ret) {

This is not unwound on error.   See comment on remove race below.

> +		dev_err(&client->dev, "failed to start measurement\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> +					      sps30_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static int sps30_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct sps30_state *state = iio_priv(indio_dev);
> +
> +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
This looks like a race to me.

You turn off the measurements before removing the interface to them.
It's certainly not in the 'obviously' right category.

As such I would either use a devm action to do this stop,
or not use the managed interfaces for everything in probe
after the equivalent start.
The devm_add_action_or_reset would be the cleanest option I think..
Will also fix the cleanup on error in probe.

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id sps30_id[] = {
> +	{ "sps30" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, sps30_id);
> +
> +static const struct of_device_id sps30_of_match[] = {
> +	{ .compatible = "sensirion,sps30" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sps30_of_match);
> +
> +static struct i2c_driver sps30_driver = {
> +	.driver = {
> +		.name = "sps30",
> +		.of_match_table = sps30_of_match,
> +	},
> +	.id_table = sps30_id,
> +	.probe_new = sps30_probe,
> +	.remove = sps30_remove,
> +};
> +module_i2c_driver(sps30_driver);
> +
> +MODULE_AUTHOR("Tomasz Duszynski <tduszyns@gmail.com>");
> +MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 3/3] iio: chemical: sps30: add device tree support

[Jonathan Cameron]

On Sat, 24 Nov 2018 23:14:15 +0100
Tomasz Duszynski <tduszyns@gmail.com> wrote:

> Add device tree support for Sensirion SPS30 particulate
> matter sensor.
> 
> Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
one comment inine, around the fact we are trying to move
to generic names in DT where ever possible.  Now we don't
have a suitable one (IIRC) yet so time to make one up ;)

+CC Rob for his input on that.
> ---
>  .../bindings/iio/chemical/sensirion,sps30.txt        | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt b/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> new file mode 100644
> index 000000000000..6eee2709b5b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> @@ -0,0 +1,12 @@
> +* Sensirion SPS30 particulate matter sensor
> +
> +Required properties:
> +- compatible: must be "sensirion,sps30"
> +- reg: the I2C address of the sensor
> +
> +Example:
> +
> +sps30@69 {
We should define a generic type.  Rob, what would work for this
one?

particlesensor@69?

> +	compatible = "sensirion,sps30";
> +	reg = <0x69>;
> +};

Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Himanshu Jha]

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 <tduszyns@gmail.com>
> ---
>  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 <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/module.h>
> +
> +#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=qPXydAacU1RqZWA@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, &tmp[i], 2, CRC8_INIT_VALUE);
> +
> +		if (crc != tmp[i + 2]) {
> +			dev_err(&state->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(&state->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(&state->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(&client->dev, sizeof(*state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	state->client = client;
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &sps30_info;
> +	indio_dev->name = client->name;
> +	indio_dev->channels = sps30_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->available_scan_masks = sps30_scan_masks;
> +
> +	mutex_init(&state->lock);
> +	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> +
> +	/*
> +	 * Power-on-reset causes sensor to produce some glitch on i2c bus
> +	 * and some controllers end up in error state. Recover simply
> +	 * by placing something on the bus.
> +	 */
> +	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to reset device\n");
> +		return ret;
> +	}
> +	usleep_range(2500000, 3500000);

Isn't that range too high ?
msleep_interruptible ?

> +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> +
> +	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read serial number\n");
> +		return ret;
> +	}
> +	dev_info(&client->dev, "serial number: %s\n", buf);
> +
> +	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to start measurement\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> +					      sps30_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static int sps30_remove(struct i2c_client *client)

Perfect candidate for `devm_add_action_or_reset()` ?

> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct sps30_state *state = iio_priv(indio_dev);
> +
> +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> +
> +	return 0;
> +}


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Matt Ranostay]

On Sat, Nov 24, 2018 at 2:14 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
>
> Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> is de facto standard. Existing air quality sensors usually follow
> this convention and are capable of returning measurements using
> this unit.
>
> IIO currently does not offer suitable channel type for this
> type of measurements hence this patch adds this.
>
> In addition, two modifiers are introduced used for distinguishing
> between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> IIO_MOD_PM10 and IIO_MOD_PM2p5.
>
> Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
>  drivers/iio/industrialio-core.c         |  3 +++
>  include/uapi/linux/iio/types.h          |  3 +++
>  tools/iio/iio_event_monitor.c           |  6 ++++++
>  4 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 8127a08e366d..ce0ed140660d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1684,4 +1684,13 @@ KernelVersion:   4.18
>  Contact:       linux-iio@vger.kernel.org
>  Description:
>                 Raw (unscaled) phase difference reading from channel Y
> -               that can be processed to radians.
> \ No newline at end of file
> +               that can be processed to radians.
> +
> +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> +KernelVersion: 4.21
> +Contact:       linux-iio@vger.kernel.org
> +Description:
> +               Mass concentration reading of particulate matter in ug / m3.
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index a062cfddc5af..2a9ef600c1fb 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
>         [IIO_GRAVITY]  = "gravity",
>         [IIO_POSITIONRELATIVE]  = "positionrelative",
>         [IIO_PHASE] = "phase",
> +       [IIO_MASSCONCENTRATION] = "massconcentration",
>  };
>
>  static const char * const iio_modifier_names[] = {
> @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
>         [IIO_MOD_Q] = "q",
>         [IIO_MOD_CO2] = "co2",
>         [IIO_MOD_VOC] = "voc",
> +       [IIO_MOD_PM2p5] = "pm2p5",
> +       [IIO_MOD_PM10] = "pm10",
>  };
>
>  /* relies on pairs of these shared then separate */
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index 92baabc103ac..465044b42af5 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -46,6 +46,7 @@ enum iio_chan_type {
>         IIO_GRAVITY,
>         IIO_POSITIONRELATIVE,
>         IIO_PHASE,
> +       IIO_MASSCONCENTRATION,

So I'm guessing IIO_CONCENTRATION can't be scaled to the micro-grams
per cubic meter?

>  };
>
>  enum iio_modifier {
> @@ -87,6 +88,8 @@ enum iio_modifier {
>         IIO_MOD_VOC,
>         IIO_MOD_LIGHT_UV,
>         IIO_MOD_LIGHT_DUV,
> +       IIO_MOD_PM2p5,

I know this is unit of measure but the lowercase p in IIO_MOD_PM2p5 is
a bit non-standard for iio defines/enum.

- Matt

> +       IIO_MOD_PM10,
>  };
>
>  enum iio_event_type {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index ac2de6b7e89f..f0fcfeddba2b 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
>         [IIO_GRAVITY] = "gravity",
>         [IIO_POSITIONRELATIVE] = "positionrelative",
>         [IIO_PHASE] = "phase",
> +       [IIO_MASSCONCENTRATION] = "massconcentration",
>  };
>
>  static const char * const iio_ev_type_text[] = {
> @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
>         [IIO_MOD_Q] = "q",
>         [IIO_MOD_CO2] = "co2",
>         [IIO_MOD_VOC] = "voc",
> +       [IIO_MOD_PM2p5] = "pm2p5",
> +       [IIO_MOD_PM10] = "pm10",
>  };
>
>  static bool event_is_known(struct iio_event_data *event)
> @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
>         case IIO_GRAVITY:
>         case IIO_POSITIONRELATIVE:
>         case IIO_PHASE:
> +       case IIO_MASSCONCENTRATION:
>                 break;
>         default:
>                 return false;
> @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
>         case IIO_MOD_Q:
>         case IIO_MOD_CO2:
>         case IIO_MOD_VOC:
> +       case IIO_MOD_PM2p5:
> +       case IIO_MOD_PM10:
>                 break;
>         default:
>                 return false;
> --
> 2.19.2
>

Re: [PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Jonathan Cameron]

On Sun, 25 Nov 2018 05:51:32 -0800
Matt Ranostay <matt.ranostay@konsulko.com> wrote:

> On Sat, Nov 24, 2018 at 2:14 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
> >
> > Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> > is de facto standard. Existing air quality sensors usually follow
> > this convention and are capable of returning measurements using
> > this unit.
> >
> > IIO currently does not offer suitable channel type for this
> > type of measurements hence this patch adds this.
> >
> > In addition, two modifiers are introduced used for distinguishing
> > between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> > IIO_MOD_PM10 and IIO_MOD_PM2p5.
> >
> > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
> >  drivers/iio/industrialio-core.c         |  3 +++
> >  include/uapi/linux/iio/types.h          |  3 +++
> >  tools/iio/iio_event_monitor.c           |  6 ++++++
> >  4 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index 8127a08e366d..ce0ed140660d 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -1684,4 +1684,13 @@ KernelVersion:   4.18
> >  Contact:       linux-iio@vger.kernel.org
> >  Description:
> >                 Raw (unscaled) phase difference reading from channel Y
> > -               that can be processed to radians.
> > \ No newline at end of file
> > +               that can be processed to radians.
> > +
> > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> > +KernelVersion: 4.21
> > +Contact:       linux-iio@vger.kernel.org
> > +Description:
> > +               Mass concentration reading of particulate matter in ug / m3.
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index a062cfddc5af..2a9ef600c1fb 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
> >         [IIO_GRAVITY]  = "gravity",
> >         [IIO_POSITIONRELATIVE]  = "positionrelative",
> >         [IIO_PHASE] = "phase",
> > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> >  };
> >
> >  static const char * const iio_modifier_names[] = {
> > @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
> >         [IIO_MOD_Q] = "q",
> >         [IIO_MOD_CO2] = "co2",
> >         [IIO_MOD_VOC] = "voc",
> > +       [IIO_MOD_PM2p5] = "pm2p5",
> > +       [IIO_MOD_PM10] = "pm10",
> >  };
> >
> >  /* relies on pairs of these shared then separate */
> > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > index 92baabc103ac..465044b42af5 100644
> > --- a/include/uapi/linux/iio/types.h
> > +++ b/include/uapi/linux/iio/types.h
> > @@ -46,6 +46,7 @@ enum iio_chan_type {
> >         IIO_GRAVITY,
> >         IIO_POSITIONRELATIVE,
> >         IIO_PHASE,
> > +       IIO_MASSCONCENTRATION,  
> 
> So I'm guessing IIO_CONCENTRATION can't be scaled to the micro-grams
> per cubic meter?

Currently concentration is defined as a percentage reading of a substance
(which does make me wonder if it is meant to be percentage of the volume or
percentage of the mass?)  Either way, if can't convert to a density measurement
as it's a unit free ratio (I think).

> 
> >  };
> >
> >  enum iio_modifier {
> > @@ -87,6 +88,8 @@ enum iio_modifier {
> >         IIO_MOD_VOC,
> >         IIO_MOD_LIGHT_UV,
> >         IIO_MOD_LIGHT_DUV,
> > +       IIO_MOD_PM2p5,  
> 
> I know this is unit of measure but the lowercase p in IIO_MOD_PM2p5 is
> a bit non-standard for iio defines/enum.
It is a bit odd and will get us scripted reports so maybe best to
just go upper case and not worry about it.

Jonathan
> 
> - Matt
> 
> > +       IIO_MOD_PM10,
> >  };
> >
> >  enum iio_event_type {
> > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > index ac2de6b7e89f..f0fcfeddba2b 100644
> > --- a/tools/iio/iio_event_monitor.c
> > +++ b/tools/iio/iio_event_monitor.c
> > @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
> >         [IIO_GRAVITY] = "gravity",
> >         [IIO_POSITIONRELATIVE] = "positionrelative",
> >         [IIO_PHASE] = "phase",
> > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> >  };
> >
> >  static const char * const iio_ev_type_text[] = {
> > @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
> >         [IIO_MOD_Q] = "q",
> >         [IIO_MOD_CO2] = "co2",
> >         [IIO_MOD_VOC] = "voc",
> > +       [IIO_MOD_PM2p5] = "pm2p5",
> > +       [IIO_MOD_PM10] = "pm10",
> >  };
> >
> >  static bool event_is_known(struct iio_event_data *event)
> > @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
> >         case IIO_GRAVITY:
> >         case IIO_POSITIONRELATIVE:
> >         case IIO_PHASE:
> > +       case IIO_MASSCONCENTRATION:
> >                 break;
> >         default:
> >                 return false;
> > @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
> >         case IIO_MOD_Q:
> >         case IIO_MOD_CO2:
> >         case IIO_MOD_VOC:
> > +       case IIO_MOD_PM2p5:
> > +       case IIO_MOD_PM10:
> >                 break;
> >         default:
> >                 return false;
> > --
> > 2.19.2
> >  

Re: [PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Matt Ranostay]

On Sun, Nov 25, 2018 at 6:03 AM Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
>
> On Sun, 25 Nov 2018 05:51:32 -0800
> Matt Ranostay <matt.ranostay@konsulko.com> wrote:
>
> > On Sat, Nov 24, 2018 at 2:14 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
> > >
> > > Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> > > is de facto standard. Existing air quality sensors usually follow
> > > this convention and are capable of returning measurements using
> > > this unit.
> > >
> > > IIO currently does not offer suitable channel type for this
> > > type of measurements hence this patch adds this.
> > >
> > > In addition, two modifiers are introduced used for distinguishing
> > > between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> > > IIO_MOD_PM10 and IIO_MOD_PM2p5.
> > >
> > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
> > >  drivers/iio/industrialio-core.c         |  3 +++
> > >  include/uapi/linux/iio/types.h          |  3 +++
> > >  tools/iio/iio_event_monitor.c           |  6 ++++++
> > >  4 files changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index 8127a08e366d..ce0ed140660d 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -1684,4 +1684,13 @@ KernelVersion:   4.18
> > >  Contact:       linux-iio@vger.kernel.org
> > >  Description:
> > >                 Raw (unscaled) phase difference reading from channel Y
> > > -               that can be processed to radians.
> > > \ No newline at end of file
> > > +               that can be processed to radians.
> > > +
> > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> > > +KernelVersion: 4.21
> > > +Contact:       linux-iio@vger.kernel.org
> > > +Description:
> > > +               Mass concentration reading of particulate matter in ug / m3.
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > index a062cfddc5af..2a9ef600c1fb 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > >         [IIO_GRAVITY]  = "gravity",
> > >         [IIO_POSITIONRELATIVE]  = "positionrelative",
> > >         [IIO_PHASE] = "phase",
> > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > >  };
> > >
> > >  static const char * const iio_modifier_names[] = {
> > > @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
> > >         [IIO_MOD_Q] = "q",
> > >         [IIO_MOD_CO2] = "co2",
> > >         [IIO_MOD_VOC] = "voc",
> > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > +       [IIO_MOD_PM10] = "pm10",
> > >  };
> > >
> > >  /* relies on pairs of these shared then separate */
> > > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > > index 92baabc103ac..465044b42af5 100644
> > > --- a/include/uapi/linux/iio/types.h
> > > +++ b/include/uapi/linux/iio/types.h
> > > @@ -46,6 +46,7 @@ enum iio_chan_type {
> > >         IIO_GRAVITY,
> > >         IIO_POSITIONRELATIVE,
> > >         IIO_PHASE,
> > > +       IIO_MASSCONCENTRATION,
> >
> > So I'm guessing IIO_CONCENTRATION can't be scaled to the micro-grams
> > per cubic meter?
>
> Currently concentration is defined as a percentage reading of a substance
> (which does make me wonder if it is meant to be percentage of the volume or
> percentage of the mass?)  Either way, if can't convert to a density measurement
> as it's a unit free ratio (I think).

Seems like it can be both..  guessing all the atmosphere ( CO2, VOC,
etc) ones are mass/density because on how they work.
But the electro-conductivity sensor that is using IIO_CONCENTRATION
channels is likely from percentage of volume.

- Matt

>
> >
> > >  };
> > >
> > >  enum iio_modifier {
> > > @@ -87,6 +88,8 @@ enum iio_modifier {
> > >         IIO_MOD_VOC,
> > >         IIO_MOD_LIGHT_UV,
> > >         IIO_MOD_LIGHT_DUV,
> > > +       IIO_MOD_PM2p5,
> >
> > I know this is unit of measure but the lowercase p in IIO_MOD_PM2p5 is
> > a bit non-standard for iio defines/enum.
> It is a bit odd and will get us scripted reports so maybe best to
> just go upper case and not worry about it.
>
> Jonathan
> >
> > - Matt
> >
> > > +       IIO_MOD_PM10,
> > >  };
> > >
> > >  enum iio_event_type {
> > > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > > index ac2de6b7e89f..f0fcfeddba2b 100644
> > > --- a/tools/iio/iio_event_monitor.c
> > > +++ b/tools/iio/iio_event_monitor.c
> > > @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > >         [IIO_GRAVITY] = "gravity",
> > >         [IIO_POSITIONRELATIVE] = "positionrelative",
> > >         [IIO_PHASE] = "phase",
> > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > >  };
> > >
> > >  static const char * const iio_ev_type_text[] = {
> > > @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
> > >         [IIO_MOD_Q] = "q",
> > >         [IIO_MOD_CO2] = "co2",
> > >         [IIO_MOD_VOC] = "voc",
> > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > +       [IIO_MOD_PM10] = "pm10",
> > >  };
> > >
> > >  static bool event_is_known(struct iio_event_data *event)
> > > @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
> > >         case IIO_GRAVITY:
> > >         case IIO_POSITIONRELATIVE:
> > >         case IIO_PHASE:
> > > +       case IIO_MASSCONCENTRATION:
> > >                 break;
> > >         default:
> > >                 return false;
> > > @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
> > >         case IIO_MOD_Q:
> > >         case IIO_MOD_CO2:
> > >         case IIO_MOD_VOC:
> > > +       case IIO_MOD_PM2p5:
> > > +       case IIO_MOD_PM10:
> > >                 break;
> > >         default:
> > >                 return false;
> > > --
> > > 2.19.2
> > >
>

Re: [PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Tomasz Duszynski]

On Sun, Nov 25, 2018 at 08:35:07AM +0000, Jonathan Cameron wrote:
> On Sat, 24 Nov 2018 23:14:13 +0100
> Tomasz Duszynski <tduszyns@gmail.com> wrote:
>
> > Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> > is de facto standard. Existing air quality sensors usually follow
> > this convention and are capable of returning measurements using
> > this unit.
> >
> > IIO currently does not offer suitable channel type for this
> > type of measurements hence this patch adds this.
> >
> > In addition, two modifiers are introduced used for distinguishing
> > between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> > IIO_MOD_PM10 and IIO_MOD_PM2p5.
> I initially wondered why these particular numbers and why don't
> we provide an attribute specify this separately?
>
> However google suggests these two are pretty much the only
> ones anyone worries about in pollution sensors.  I ended
> up fairly quickly at the EPA website
> https://www.epa.gov/pm-pollution/table-historical-particulate-matter-pm-national-ambient-air-quality-standards-naaqs
> which tells me these two have be used since about 1987.
>

Usually sensors can measure more than just PM10 and PM2.5. Fairy
common is measurement of PM1.0. SPS30 additionally measures PM4.0
but frankly I don't now know exactly what's the real usecase for that.

> So I think the modifier route is the right approach here
> (I think we've gotten this wrong in the past such as light
> sensor colours where the list keeps on growing).
>
> I would like a reference or two in the docs though to point
> people to these definitions.
>

Okay, will add something reasonable so everyone interested
could get the basic idea quickly.

> Thanks,
>
> Jonathan
>
> >
> > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
> >  drivers/iio/industrialio-core.c         |  3 +++
> >  include/uapi/linux/iio/types.h          |  3 +++
> >  tools/iio/iio_event_monitor.c           |  6 ++++++
> >  4 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index 8127a08e366d..ce0ed140660d 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -1684,4 +1684,13 @@ KernelVersion:	4.18
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		Raw (unscaled) phase difference reading from channel Y
> > -		that can be processed to radians.
> > \ No newline at end of file
> > +		that can be processed to radians.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> > +KernelVersion:	4.21
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Mass concentration reading of particulate matter in ug / m3.
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index a062cfddc5af..2a9ef600c1fb 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
> >  	[IIO_GRAVITY]  = "gravity",
> >  	[IIO_POSITIONRELATIVE]  = "positionrelative",
> >  	[IIO_PHASE] = "phase",
> > +	[IIO_MASSCONCENTRATION] = "massconcentration",
> >  };
> >
> >  static const char * const iio_modifier_names[] = {
> > @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
> >  	[IIO_MOD_Q] = "q",
> >  	[IIO_MOD_CO2] = "co2",
> >  	[IIO_MOD_VOC] = "voc",
> > +	[IIO_MOD_PM2p5] = "pm2p5",
> > +	[IIO_MOD_PM10] = "pm10",
> >  };
> >
> >  /* relies on pairs of these shared then separate */
> > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > index 92baabc103ac..465044b42af5 100644
> > --- a/include/uapi/linux/iio/types.h
> > +++ b/include/uapi/linux/iio/types.h
> > @@ -46,6 +46,7 @@ enum iio_chan_type {
> >  	IIO_GRAVITY,
> >  	IIO_POSITIONRELATIVE,
> >  	IIO_PHASE,
> > +	IIO_MASSCONCENTRATION,
> >  };
> >
> >  enum iio_modifier {
> > @@ -87,6 +88,8 @@ enum iio_modifier {
> >  	IIO_MOD_VOC,
> >  	IIO_MOD_LIGHT_UV,
> >  	IIO_MOD_LIGHT_DUV,
> > +	IIO_MOD_PM2p5,
> > +	IIO_MOD_PM10,
> >  };
> >
> >  enum iio_event_type {
> > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > index ac2de6b7e89f..f0fcfeddba2b 100644
> > --- a/tools/iio/iio_event_monitor.c
> > +++ b/tools/iio/iio_event_monitor.c
> > @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
> >  	[IIO_GRAVITY] = "gravity",
> >  	[IIO_POSITIONRELATIVE] = "positionrelative",
> >  	[IIO_PHASE] = "phase",
> > +	[IIO_MASSCONCENTRATION] = "massconcentration",
> >  };
> >
> >  static const char * const iio_ev_type_text[] = {
> > @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
> >  	[IIO_MOD_Q] = "q",
> >  	[IIO_MOD_CO2] = "co2",
> >  	[IIO_MOD_VOC] = "voc",
> > +	[IIO_MOD_PM2p5] = "pm2p5",
> > +	[IIO_MOD_PM10] = "pm10",
> >  };
> >
> >  static bool event_is_known(struct iio_event_data *event)
> > @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
> >  	case IIO_GRAVITY:
> >  	case IIO_POSITIONRELATIVE:
> >  	case IIO_PHASE:
> > +	case IIO_MASSCONCENTRATION:
> >  		break;
> >  	default:
> >  		return false;
> > @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
> >  	case IIO_MOD_Q:
> >  	case IIO_MOD_CO2:
> >  	case IIO_MOD_VOC:
> > +	case IIO_MOD_PM2p5:
> > +	case IIO_MOD_PM10:
> >  		break;
> >  	default:
> >  		return false;
>

Re: [PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Tomasz Duszynski]

On Sun, Nov 25, 2018 at 02:03:16PM +0000, Jonathan Cameron wrote:
> On Sun, 25 Nov 2018 05:51:32 -0800
> Matt Ranostay <matt.ranostay@konsulko.com> wrote:
>
> > On Sat, Nov 24, 2018 at 2:14 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
> > >
> > > Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> > > is de facto standard. Existing air quality sensors usually follow
> > > this convention and are capable of returning measurements using
> > > this unit.
> > >
> > > IIO currently does not offer suitable channel type for this
> > > type of measurements hence this patch adds this.
> > >
> > > In addition, two modifiers are introduced used for distinguishing
> > > between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> > > IIO_MOD_PM10 and IIO_MOD_PM2p5.
> > >
> > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
> > >  drivers/iio/industrialio-core.c         |  3 +++
> > >  include/uapi/linux/iio/types.h          |  3 +++
> > >  tools/iio/iio_event_monitor.c           |  6 ++++++
> > >  4 files changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index 8127a08e366d..ce0ed140660d 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -1684,4 +1684,13 @@ KernelVersion:   4.18
> > >  Contact:       linux-iio@vger.kernel.org
> > >  Description:
> > >                 Raw (unscaled) phase difference reading from channel Y
> > > -               that can be processed to radians.
> > > \ No newline at end of file
> > > +               that can be processed to radians.
> > > +
> > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> > > +KernelVersion: 4.21
> > > +Contact:       linux-iio@vger.kernel.org
> > > +Description:
> > > +               Mass concentration reading of particulate matter in ug / m3.
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > index a062cfddc5af..2a9ef600c1fb 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > >         [IIO_GRAVITY]  = "gravity",
> > >         [IIO_POSITIONRELATIVE]  = "positionrelative",
> > >         [IIO_PHASE] = "phase",
> > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > >  };
> > >
> > >  static const char * const iio_modifier_names[] = {
> > > @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
> > >         [IIO_MOD_Q] = "q",
> > >         [IIO_MOD_CO2] = "co2",
> > >         [IIO_MOD_VOC] = "voc",
> > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > +       [IIO_MOD_PM10] = "pm10",
> > >  };
> > >
> > >  /* relies on pairs of these shared then separate */
> > > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > > index 92baabc103ac..465044b42af5 100644
> > > --- a/include/uapi/linux/iio/types.h
> > > +++ b/include/uapi/linux/iio/types.h
> > > @@ -46,6 +46,7 @@ enum iio_chan_type {
> > >         IIO_GRAVITY,
> > >         IIO_POSITIONRELATIVE,
> > >         IIO_PHASE,
> > > +       IIO_MASSCONCENTRATION,
> >
> > So I'm guessing IIO_CONCENTRATION can't be scaled to the micro-grams
> > per cubic meter?
>
> Currently concentration is defined as a percentage reading of a substance
> (which does make me wonder if it is meant to be percentage of the volume or
> percentage of the mass?)  Either way, if can't convert to a density measurement
> as it's a unit free ratio (I think).
>

And this is the main reason behind introducing a new channel type.

> >
> > >  };
> > >
> > >  enum iio_modifier {
> > > @@ -87,6 +88,8 @@ enum iio_modifier {
> > >         IIO_MOD_VOC,
> > >         IIO_MOD_LIGHT_UV,
> > >         IIO_MOD_LIGHT_DUV,
> > > +       IIO_MOD_PM2p5,
> >
> > I know this is unit of measure but the lowercase p in IIO_MOD_PM2p5 is
> > a bit non-standard for iio defines/enum.
> It is a bit odd and will get us scripted reports so maybe best to
> just go upper case and not worry about it.
>

Initially I came up with IIO_MOD_PM2_5 but eventually replaced
underscore with 'p' meaning 'decimal point'. Anyway, I am okay with the
suggested change.

> Jonathan
> >
> > - Matt
> >
> > > +       IIO_MOD_PM10,
> > >  };
> > >
> > >  enum iio_event_type {
> > > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > > index ac2de6b7e89f..f0fcfeddba2b 100644
> > > --- a/tools/iio/iio_event_monitor.c
> > > +++ b/tools/iio/iio_event_monitor.c
> > > @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > >         [IIO_GRAVITY] = "gravity",
> > >         [IIO_POSITIONRELATIVE] = "positionrelative",
> > >         [IIO_PHASE] = "phase",
> > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > >  };
> > >
> > >  static const char * const iio_ev_type_text[] = {
> > > @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
> > >         [IIO_MOD_Q] = "q",
> > >         [IIO_MOD_CO2] = "co2",
> > >         [IIO_MOD_VOC] = "voc",
> > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > +       [IIO_MOD_PM10] = "pm10",
> > >  };
> > >
> > >  static bool event_is_known(struct iio_event_data *event)
> > > @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
> > >         case IIO_GRAVITY:
> > >         case IIO_POSITIONRELATIVE:
> > >         case IIO_PHASE:
> > > +       case IIO_MASSCONCENTRATION:
> > >                 break;
> > >         default:
> > >                 return false;
> > > @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
> > >         case IIO_MOD_Q:
> > >         case IIO_MOD_CO2:
> > >         case IIO_MOD_VOC:
> > > +       case IIO_MOD_PM2p5:
> > > +       case IIO_MOD_PM10:
> > >                 break;
> > >         default:
> > >                 return false;
> > > --
> > > 2.19.2
> > >
>

Re: [PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Tomasz Duszynski]

On Sun, Nov 25, 2018 at 06:14:44AM -0800, Matt Ranostay wrote:
> On Sun, Nov 25, 2018 at 6:03 AM Jonathan Cameron
> <jic23@jic23.retrosnub.co.uk> wrote:
> >
> > On Sun, 25 Nov 2018 05:51:32 -0800
> > Matt Ranostay <matt.ranostay@konsulko.com> wrote:
> >
> > > On Sat, Nov 24, 2018 at 2:14 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
> > > >
> > > > Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> > > > is de facto standard. Existing air quality sensors usually follow
> > > > this convention and are capable of returning measurements using
> > > > this unit.
> > > >
> > > > IIO currently does not offer suitable channel type for this
> > > > type of measurements hence this patch adds this.
> > > >
> > > > In addition, two modifiers are introduced used for distinguishing
> > > > between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> > > > IIO_MOD_PM10 and IIO_MOD_PM2p5.
> > > >
> > > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
> > > >  drivers/iio/industrialio-core.c         |  3 +++
> > > >  include/uapi/linux/iio/types.h          |  3 +++
> > > >  tools/iio/iio_event_monitor.c           |  6 ++++++
> > > >  4 files changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > > index 8127a08e366d..ce0ed140660d 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > > @@ -1684,4 +1684,13 @@ KernelVersion:   4.18
> > > >  Contact:       linux-iio@vger.kernel.org
> > > >  Description:
> > > >                 Raw (unscaled) phase difference reading from channel Y
> > > > -               that can be processed to radians.
> > > > \ No newline at end of file
> > > > +               that can be processed to radians.
> > > > +
> > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> > > > +KernelVersion: 4.21
> > > > +Contact:       linux-iio@vger.kernel.org
> > > > +Description:
> > > > +               Mass concentration reading of particulate matter in ug / m3.
> > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > > index a062cfddc5af..2a9ef600c1fb 100644
> > > > --- a/drivers/iio/industrialio-core.c
> > > > +++ b/drivers/iio/industrialio-core.c
> > > > @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > > >         [IIO_GRAVITY]  = "gravity",
> > > >         [IIO_POSITIONRELATIVE]  = "positionrelative",
> > > >         [IIO_PHASE] = "phase",
> > > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > > >  };
> > > >
> > > >  static const char * const iio_modifier_names[] = {
> > > > @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
> > > >         [IIO_MOD_Q] = "q",
> > > >         [IIO_MOD_CO2] = "co2",
> > > >         [IIO_MOD_VOC] = "voc",
> > > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > > +       [IIO_MOD_PM10] = "pm10",
> > > >  };
> > > >
> > > >  /* relies on pairs of these shared then separate */
> > > > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > > > index 92baabc103ac..465044b42af5 100644
> > > > --- a/include/uapi/linux/iio/types.h
> > > > +++ b/include/uapi/linux/iio/types.h
> > > > @@ -46,6 +46,7 @@ enum iio_chan_type {
> > > >         IIO_GRAVITY,
> > > >         IIO_POSITIONRELATIVE,
> > > >         IIO_PHASE,
> > > > +       IIO_MASSCONCENTRATION,
> > >
> > > So I'm guessing IIO_CONCENTRATION can't be scaled to the micro-grams
> > > per cubic meter?
> >
> > Currently concentration is defined as a percentage reading of a substance
> > (which does make me wonder if it is meant to be percentage of the volume or
> > percentage of the mass?)  Either way, if can't convert to a density measurement
> > as it's a unit free ratio (I think).
>
> Seems like it can be both..  guessing all the atmosphere ( CO2, VOC,
> etc) ones are mass/density because on how they work.

Hmm, but still percentage was picked up for IIO_CONCENTRATION which does
really match PM expectations. Perhaps if units were sticked to modifiers
it whould be easier to reuse that.

> But the electro-conductivity sensor that is using IIO_CONCENTRATION
> channels is likely from percentage of volume.
>
> - Matt
>
> >
> > >
> > > >  };
> > > >
> > > >  enum iio_modifier {
> > > > @@ -87,6 +88,8 @@ enum iio_modifier {
> > > >         IIO_MOD_VOC,
> > > >         IIO_MOD_LIGHT_UV,
> > > >         IIO_MOD_LIGHT_DUV,
> > > > +       IIO_MOD_PM2p5,
> > >
> > > I know this is unit of measure but the lowercase p in IIO_MOD_PM2p5 is
> > > a bit non-standard for iio defines/enum.
> > It is a bit odd and will get us scripted reports so maybe best to
> > just go upper case and not worry about it.
> >
> > Jonathan
> > >
> > > - Matt
> > >
> > > > +       IIO_MOD_PM10,
> > > >  };
> > > >
> > > >  enum iio_event_type {
> > > > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > > > index ac2de6b7e89f..f0fcfeddba2b 100644
> > > > --- a/tools/iio/iio_event_monitor.c
> > > > +++ b/tools/iio/iio_event_monitor.c
> > > > @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > > >         [IIO_GRAVITY] = "gravity",
> > > >         [IIO_POSITIONRELATIVE] = "positionrelative",
> > > >         [IIO_PHASE] = "phase",
> > > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > > >  };
> > > >
> > > >  static const char * const iio_ev_type_text[] = {
> > > > @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
> > > >         [IIO_MOD_Q] = "q",
> > > >         [IIO_MOD_CO2] = "co2",
> > > >         [IIO_MOD_VOC] = "voc",
> > > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > > +       [IIO_MOD_PM10] = "pm10",
> > > >  };
> > > >
> > > >  static bool event_is_known(struct iio_event_data *event)
> > > > @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
> > > >         case IIO_GRAVITY:
> > > >         case IIO_POSITIONRELATIVE:
> > > >         case IIO_PHASE:
> > > > +       case IIO_MASSCONCENTRATION:
> > > >                 break;
> > > >         default:
> > > >                 return false;
> > > > @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
> > > >         case IIO_MOD_Q:
> > > >         case IIO_MOD_CO2:
> > > >         case IIO_MOD_VOC:
> > > > +       case IIO_MOD_PM2p5:
> > > > +       case IIO_MOD_PM10:
> > > >                 break;
> > > >         default:
> > > >                 return false;
> > > > --
> > > > 2.19.2
> > > >
> >

Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Tomasz Duszynski]

On Sun, Nov 25, 2018 at 08:56:59AM +0000, Jonathan Cameron wrote:
> On Sat, 24 Nov 2018 23:14:14 +0100
> Tomasz Duszynski <tduszyns@gmail.com> wrote:
>
> > Add support for Sensirion SPS30 particulate matter sensor.
> >
> > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> 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 000000000000..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 <tduszyns@gmail.com>
> > + *
> > + * 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 <linux/crc8.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/module.h>
> > +
> > +#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, \
> > +	}, \
> > +}
> > +
> > +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!
>

Understood.

> > +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));
> > +	if (ret != sizeof(tmp))
> > +		return ret < 0 ? ret : -EIO;
> > +
> > +	for (i = 0; i < sizeof(tmp); i += 3) {
> > +		u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE);
> > +
> > +		if (crc != tmp[i + 2]) {
> > +			dev_err(&state->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 };
> > +
> > +	switch (cmd) {
> > +	case SPS30_START_MEAS:
> > +		buf[2] = 0x03;
> > +		buf[3] = 0x00;
> > +		buf[4] = 0xac; /* precomputed crc */
>
> Could you put that in code and let the compiler do the pre compute?
> Might be a little more elegant.
>

Okay.

> > +		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_ieee754_to_int(const u8 *data)
> > +{
> > +	u32 val = ((u32)data[0] << 24) | ((u32)data[1] << 16) |
> > +		  ((u32)data[2] << 8) | (u32)data[3],
> Have a separate
> u32 mantissa = (val << 9) >> 9 line.
> What is this next line actually doing?  Kind of looks like
> a mask?  If so just mask it with & as more readable.
>

Do you mean mantissa? We want to get rid of sign bit and 8 exponent bits.

> > +	    mantissa = (val << 9) >> 9;
> > +	int exp = (val >> 23) - 127;
> > +
> > +	if (!exp && !mantissa)
> > +		return 0;
> > +
> > +	if (exp < 0)
> > +		return 0;
> > +
> > +	return (1 << exp) + (mantissa >> (23 - exp));
> > +}
> > +
> > +static int sps30_do_meas(struct sps30_state *state, int *pm2p5, int *pm10)
> > +{
> > +	/*
> > +	 * Internally sensor stores measurements in a following manner:
> > +	 *
> > +	 * PM1p0: upper two bytes, crc8, lower two bytes, crc8
>
> So there are other particle sizes that this sensor reads?  Why
> are we mapping them down to just the two channel types?
>

As said before, introducing PM1.0 and PM4.0 readings seems
a reasonable option.

> > +	 * PM2p5: upper two bytes, crc8, lower two bytes, crc8
> > +	 * PM4p0: upper two bytes, crc8, lower two bytes, crc8
> > +	 * PM10:  upper two bytes, crc8, lower two bytes, crc8
> > +	 *
> > +	 * What follows next are number concentration measurements and
> > +	 * typical particle size measurement.
> > +	 *
> > +	 * Once data is read from sensor crc bytes are stripped off
> > +	 * hence we need 16 bytes of buffer space.
> > +	 */
> > +	int ret, tries = 5;
> > +	u8 buf[16];
> > +
> > +	while (tries--) {
> > +		ret = sps30_do_cmd(state, SPS30_READ_DATA_READY_FLAG, buf, 2);
> > +		if (ret)
> > +			return -EIO;
> > +
> > +		/* new measurements ready to be read */
> > +		if (buf[1] == 1)
> > +			break;
> > +
> > +		usleep_range(300000, 400000);
> > +	}
> > +
> > +	if (!tries)
> > +		return -ETIMEDOUT;
> > +
> > +	ret = sps30_do_cmd(state, SPS30_READ_DATA, buf, sizeof(buf));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * All measurements come in IEEE754 single precision floating point
> > +	 * format but sensor itself is not precise enough (-+ 10% error)
> > +	 * to take full advantage of it. Hence converting result to int
> > +	 * to keep things simple.
>
> Interesting.  We have talked about allowing floating point formats for
> sensors the provide them.  Would that be beneficial here?
>

I recall this was discussed once but unfortunately do not
remember final conclusion. Anyway, in case of this device datasheet
states that readings have maximum error of -+10% in given range which
makes me think that using floats here is an overkill.

Example, reading of 1000.123412ug/m3 has error of -+100ug/m3.
Does it really matter if care about fractional part?

Just out of curiosity, if IIO gets support for floats would it
be problematic to to add extra channel returning measurements in floats?
Or it rather will not fly..

> > +	 */
> > +	*pm2p5 = sps30_ieee754_to_int(&buf[PM2p5 * 4]);
> > +	*pm10 = sps30_ieee754_to_int(&buf[PM10 * 4]);
> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t sps30_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct sps30_state *state = iio_priv(indio_dev);
> > +	u32 buf[4]; /* PM2p5, PM10, timestamp */
> > +	int ret;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = sps30_do_meas(state, &buf[0], &buf[1]);
> > +	mutex_unlock(&state->lock);
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> > +					   iio_get_time_ns(indio_dev));
> > +err:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +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(&state->lock);
> > +			switch (chan->channel2) {
> > +			case IIO_MOD_PM2p5:
> > +				ret = sps30_do_meas(state, val, val2);
> > +				break;
> > +			case IIO_MOD_PM10:
> > +				ret = sps30_do_meas(state, val2, val);
> > +				break;
> > +			default:
> > +				break;
> > +			}
> > +			mutex_unlock(&state->lock);
> > +			if (ret)
> > +				return ret;
> > +
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	default:
> You can't get here.  Is this a warning suppression?
> If so just add a comment saying so to prevent it being removed
> by someone else later.
>

Okay.

> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info sps30_info = {
> > +	.read_raw = sps30_read_raw,
> > +};
> > +
> From a readability point of view, it would be helpful to pull
> the macro SPS30_CHAN down here in the code so we don't
> need to go jumping around to check what it is doing.
>

Okay.

> > +static const struct iio_chan_spec sps30_channels[] = {
> > +	SPS30_CHAN(0, PM2p5),
> > +	SPS30_CHAN(1, PM10),
> > +	IIO_CHAN_SOFT_TIMESTAMP(2),
> > +};
> > +
> > +static const unsigned long sps30_scan_masks[] = { 0x03, 0x00 };
> > +
> > +static int sps30_probe(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct sps30_state *state;
> > +	u8 buf[32] = { };
> > +	int ret;
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > +		return -EOPNOTSUPP;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*state));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	state = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	state->client = client;
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->info = &sps30_info;
> > +	indio_dev->name = client->name;
> > +	indio_dev->channels = sps30_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->available_scan_masks = sps30_scan_masks;
> > +
> > +	mutex_init(&state->lock);
> > +	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> > +
> > +	/*
> > +	 * Power-on-reset causes sensor to produce some glitch on i2c bus
> > +	 * and some controllers end up in error state. Recover simply
> > +	 * by placing something on the bus.
> > +	 */
> > +	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to reset device\n");
> > +		return ret;
> > +	}
> > +	usleep_range(2500000, 3500000);
> > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
>
> Talk me through this logic.  We stop it then pretty much immediately
> start it again?  Needs a comment.
>

The idea is to send some message after resetting sensor so that
i2c controller recovers from error state. I decided to send STOP_MEAS
as it's a NOP in this case. I'll try to do more testing with other
SPS30s and perhaps different boards.

> > +
> > +	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to read serial number\n");
> > +		return ret;
> > +	}
> > +	dev_info(&client->dev, "serial number: %s\n", buf);
> > +
> > +	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> > +	if (ret) {
>
> This is not unwound on error.   See comment on remove race below.
>
> > +		dev_err(&client->dev, "failed to start measurement\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> > +					      sps30_trigger_handler, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static int sps30_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct sps30_state *state = iio_priv(indio_dev);
> > +
> > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> This looks like a race to me.
>

Right, this needs fixing.

> You turn off the measurements before removing the interface to them.
> It's certainly not in the 'obviously' right category.
>
> As such I would either use a devm action to do this stop,
> or not use the managed interfaces for everything in probe
> after the equivalent start.
> The devm_add_action_or_reset would be the cleanest option I think..
> Will also fix the cleanup on error in probe.
>
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id sps30_id[] = {
> > +	{ "sps30" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sps30_id);
> > +
> > +static const struct of_device_id sps30_of_match[] = {
> > +	{ .compatible = "sensirion,sps30" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sps30_of_match);
> > +
> > +static struct i2c_driver sps30_driver = {
> > +	.driver = {
> > +		.name = "sps30",
> > +		.of_match_table = sps30_of_match,
> > +	},
> > +	.id_table = sps30_id,
> > +	.probe_new = sps30_probe,
> > +	.remove = sps30_remove,
> > +};
> > +module_i2c_driver(sps30_driver);
> > +
> > +MODULE_AUTHOR("Tomasz Duszynski <tduszyns@gmail.com>");
> > +MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver");
> > +MODULE_LICENSE("GPL v2");
>

Re: [PATCH 3/3] iio: chemical: sps30: add device tree support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4022 bytes --]

Hi Tomasz,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on v4.20-rc4 next-20181123]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tomasz-Duszynski/add-support-for-Sensirion-SPS30-PM-sensor/20181125-172634
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

>> drivers/iio/chemical/sps30.c:69:26: warning: Variable length array is used.
   drivers/iio/chemical/sps30.c: In function 'sps30_read_raw':
   drivers/iio/chemical/sps30.c:237:7: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
       if (ret)
          ^

vim +69 drivers/iio/chemical/sps30.c

0e4f3167f Tomasz Duszynski 2018-11-24   64  
0e4f3167f Tomasz Duszynski 2018-11-24   65  static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
0e4f3167f Tomasz Duszynski 2018-11-24   66  				 int buf_size, u8 *data, int data_size)
0e4f3167f Tomasz Duszynski 2018-11-24   67  {
0e4f3167f Tomasz Duszynski 2018-11-24   68  	/* every two received data bytes are checksummed */
0e4f3167f Tomasz Duszynski 2018-11-24  @69  	u8 tmp[data_size + data_size / 2];
0e4f3167f Tomasz Duszynski 2018-11-24   70  	int ret, i;
0e4f3167f Tomasz Duszynski 2018-11-24   71  
0e4f3167f Tomasz Duszynski 2018-11-24   72  	/*
0e4f3167f Tomasz Duszynski 2018-11-24   73  	 * Sensor does not support repeated start so instead of
0e4f3167f Tomasz Duszynski 2018-11-24   74  	 * sending two i2c messages in a row we just send one by one.
0e4f3167f Tomasz Duszynski 2018-11-24   75  	 */
0e4f3167f Tomasz Duszynski 2018-11-24   76  	ret = i2c_master_send(state->client, buf, buf_size);
0e4f3167f Tomasz Duszynski 2018-11-24   77  	if (ret != buf_size)
0e4f3167f Tomasz Duszynski 2018-11-24   78  		return ret < 0 ? ret : -EIO;
0e4f3167f Tomasz Duszynski 2018-11-24   79  
0e4f3167f Tomasz Duszynski 2018-11-24   80  	if (!data)
0e4f3167f Tomasz Duszynski 2018-11-24   81  		return 0;
0e4f3167f Tomasz Duszynski 2018-11-24   82  
0e4f3167f Tomasz Duszynski 2018-11-24   83  	ret = i2c_master_recv(state->client, tmp, sizeof(tmp));
0e4f3167f Tomasz Duszynski 2018-11-24   84  	if (ret != sizeof(tmp))
0e4f3167f Tomasz Duszynski 2018-11-24   85  		return ret < 0 ? ret : -EIO;
0e4f3167f Tomasz Duszynski 2018-11-24   86  
0e4f3167f Tomasz Duszynski 2018-11-24   87  	for (i = 0; i < sizeof(tmp); i += 3) {
0e4f3167f Tomasz Duszynski 2018-11-24   88  		u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE);
0e4f3167f Tomasz Duszynski 2018-11-24   89  
0e4f3167f Tomasz Duszynski 2018-11-24   90  		if (crc != tmp[i + 2]) {
0e4f3167f Tomasz Duszynski 2018-11-24   91  			dev_err(&state->client->dev,
0e4f3167f Tomasz Duszynski 2018-11-24   92  				"data integrity check failed\n");
0e4f3167f Tomasz Duszynski 2018-11-24   93  			return -EIO;
0e4f3167f Tomasz Duszynski 2018-11-24   94  		}
0e4f3167f Tomasz Duszynski 2018-11-24   95  
0e4f3167f Tomasz Duszynski 2018-11-24   96  		*data++ = tmp[i];
0e4f3167f Tomasz Duszynski 2018-11-24   97  		*data++ = tmp[i + 1];
0e4f3167f Tomasz Duszynski 2018-11-24   98  	}
0e4f3167f Tomasz Duszynski 2018-11-24   99  
0e4f3167f Tomasz Duszynski 2018-11-24  100  	return 0;
0e4f3167f Tomasz Duszynski 2018-11-24  101  }
0e4f3167f Tomasz Duszynski 2018-11-24  102  

:::::: The code at line 69 was first introduced by commit
:::::: 0e4f3167f739fa067d7e1ba672f0b46569d04d84 iio: chemical: add support for Sensirion SPS30 sensor

:::::: TO: Tomasz Duszynski <tduszyns@gmail.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65851 bytes --]

Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Tomasz Duszynski]

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 <tduszyns@gmail.com>
> > ---
> >  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 <linux/crc8.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/module.h>
> > +
> > +#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.

>
> > +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=qPXydAacU1RqZWA@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, &tmp[i], 2, CRC8_INIT_VALUE);
> > +
> > +		if (crc != tmp[i + 2]) {
> > +			dev_err(&state->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(&state->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(&state->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(&client->dev, sizeof(*state));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	state = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	state->client = client;
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->info = &sps30_info;
> > +	indio_dev->name = client->name;
> > +	indio_dev->channels = sps30_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->available_scan_masks = sps30_scan_masks;
> > +
> > +	mutex_init(&state->lock);
> > +	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> > +
> > +	/*
> > +	 * Power-on-reset causes sensor to produce some glitch on i2c bus
> > +	 * and some controllers end up in error state. Recover simply
> > +	 * by placing something on the bus.
> > +	 */
> > +	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to reset device\n");
> > +		return ret;
> > +	}
> > +	usleep_range(2500000, 3500000);
>
> Isn't that range too high ?
> msleep_interruptible ?
>

Might be.

> > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> > +
> > +	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to read serial number\n");
> > +		return ret;
> > +	}
> > +	dev_info(&client->dev, "serial number: %s\n", buf);
> > +
> > +	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to start measurement\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> > +					      sps30_trigger_handler, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static int sps30_remove(struct i2c_client *client)
>
> Perfect candidate for `devm_add_action_or_reset()` ?
>
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct sps30_state *state = iio_priv(indio_dev);
> > +
> > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> > +
> > +	return 0;
> > +}
>
>
> --
> Himanshu Jha
> Undergraduate Student
> Department of Electronics & Communication
> Guru Tegh Bahadur Institute of Technology

Re: [PATCH 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Jonathan Cameron]

On Sun, 25 Nov 2018 16:44:23 +0100
Tomasz Duszynski <tduszyns@gmail.com> wrote:

> On Sun, Nov 25, 2018 at 06:14:44AM -0800, Matt Ranostay wrote:
> > On Sun, Nov 25, 2018 at 6:03 AM Jonathan Cameron
> > <jic23@jic23.retrosnub.co.uk> wrote:  
> > >
> > > On Sun, 25 Nov 2018 05:51:32 -0800
> > > Matt Ranostay <matt.ranostay@konsulko.com> wrote:
> > >  
> > > > On Sat, Nov 24, 2018 at 2:14 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:  
> > > > >
> > > > > Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> > > > > is de facto standard. Existing air quality sensors usually follow
> > > > > this convention and are capable of returning measurements using
> > > > > this unit.
> > > > >
> > > > > IIO currently does not offer suitable channel type for this
> > > > > type of measurements hence this patch adds this.
> > > > >
> > > > > In addition, two modifiers are introduced used for distinguishing
> > > > > between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> > > > > IIO_MOD_PM10 and IIO_MOD_PM2p5.
> > > > >
> > > > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
> > > > >  drivers/iio/industrialio-core.c         |  3 +++
> > > > >  include/uapi/linux/iio/types.h          |  3 +++
> > > > >  tools/iio/iio_event_monitor.c           |  6 ++++++
> > > > >  4 files changed, 22 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > > > index 8127a08e366d..ce0ed140660d 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > > > @@ -1684,4 +1684,13 @@ KernelVersion:   4.18
> > > > >  Contact:       linux-iio@vger.kernel.org
> > > > >  Description:
> > > > >                 Raw (unscaled) phase difference reading from channel Y
> > > > > -               that can be processed to radians.
> > > > > \ No newline at end of file
> > > > > +               that can be processed to radians.
> > > > > +
> > > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> > > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> > > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> > > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> > > > > +KernelVersion: 4.21
> > > > > +Contact:       linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +               Mass concentration reading of particulate matter in ug / m3.
> > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > > > index a062cfddc5af..2a9ef600c1fb 100644
> > > > > --- a/drivers/iio/industrialio-core.c
> > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > > > >         [IIO_GRAVITY]  = "gravity",
> > > > >         [IIO_POSITIONRELATIVE]  = "positionrelative",
> > > > >         [IIO_PHASE] = "phase",
> > > > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > > > >  };
> > > > >
> > > > >  static const char * const iio_modifier_names[] = {
> > > > > @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
> > > > >         [IIO_MOD_Q] = "q",
> > > > >         [IIO_MOD_CO2] = "co2",
> > > > >         [IIO_MOD_VOC] = "voc",
> > > > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > > > +       [IIO_MOD_PM10] = "pm10",
> > > > >  };
> > > > >
> > > > >  /* relies on pairs of these shared then separate */
> > > > > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > > > > index 92baabc103ac..465044b42af5 100644
> > > > > --- a/include/uapi/linux/iio/types.h
> > > > > +++ b/include/uapi/linux/iio/types.h
> > > > > @@ -46,6 +46,7 @@ enum iio_chan_type {
> > > > >         IIO_GRAVITY,
> > > > >         IIO_POSITIONRELATIVE,
> > > > >         IIO_PHASE,
> > > > > +       IIO_MASSCONCENTRATION,  
> > > >
> > > > So I'm guessing IIO_CONCENTRATION can't be scaled to the micro-grams
> > > > per cubic meter?  
> > >
> > > Currently concentration is defined as a percentage reading of a substance
> > > (which does make me wonder if it is meant to be percentage of the volume or
> > > percentage of the mass?)  Either way, if can't convert to a density measurement
> > > as it's a unit free ratio (I think).  
> >
> > Seems like it can be both..  guessing all the atmosphere ( CO2, VOC,
> > etc) ones are mass/density because on how they work.  
> 
> Hmm, but still percentage was picked up for IIO_CONCENTRATION which does
> really match PM expectations. Perhaps if units were sticked to modifiers
> it whould be easier to reuse that.

It gets very messy if we start doing that modifiers, I'd rather stick to
different channel types.  We already do this with other channel types
position / positionrelative for example.

It isn't as though it's actually possible to convert between the two without
knowing what the particles actually are...

Jonathan
> 
> > But the electro-conductivity sensor that is using IIO_CONCENTRATION
> > channels is likely from percentage of volume.
> >
> > - Matt
> >  
> > >  
> > > >  
> > > > >  };
> > > > >
> > > > >  enum iio_modifier {
> > > > > @@ -87,6 +88,8 @@ enum iio_modifier {
> > > > >         IIO_MOD_VOC,
> > > > >         IIO_MOD_LIGHT_UV,
> > > > >         IIO_MOD_LIGHT_DUV,
> > > > > +       IIO_MOD_PM2p5,  
> > > >
> > > > I know this is unit of measure but the lowercase p in IIO_MOD_PM2p5 is
> > > > a bit non-standard for iio defines/enum.  
> > > It is a bit odd and will get us scripted reports so maybe best to
> > > just go upper case and not worry about it.
> > >
> > > Jonathan  
> > > >
> > > > - Matt
> > > >  
> > > > > +       IIO_MOD_PM10,
> > > > >  };
> > > > >
> > > > >  enum iio_event_type {
> > > > > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > > > > index ac2de6b7e89f..f0fcfeddba2b 100644
> > > > > --- a/tools/iio/iio_event_monitor.c
> > > > > +++ b/tools/iio/iio_event_monitor.c
> > > > > @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > > > >         [IIO_GRAVITY] = "gravity",
> > > > >         [IIO_POSITIONRELATIVE] = "positionrelative",
> > > > >         [IIO_PHASE] = "phase",
> > > > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > > > >  };
> > > > >
> > > > >  static const char * const iio_ev_type_text[] = {
> > > > > @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
> > > > >         [IIO_MOD_Q] = "q",
> > > > >         [IIO_MOD_CO2] = "co2",
> > > > >         [IIO_MOD_VOC] = "voc",
> > > > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > > > +       [IIO_MOD_PM10] = "pm10",
> > > > >  };
> > > > >
> > > > >  static bool event_is_known(struct iio_event_data *event)
> > > > > @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
> > > > >         case IIO_GRAVITY:
> > > > >         case IIO_POSITIONRELATIVE:
> > > > >         case IIO_PHASE:
> > > > > +       case IIO_MASSCONCENTRATION:
> > > > >                 break;
> > > > >         default:
> > > > >                 return false;
> > > > > @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
> > > > >         case IIO_MOD_Q:
> > > > >         case IIO_MOD_CO2:
> > > > >         case IIO_MOD_VOC:
> > > > > +       case IIO_MOD_PM2p5:
> > > > > +       case IIO_MOD_PM10:
> > > > >                 break;
> > > > >         default:
> > > > >                 return false;
> > > > > --
> > > > > 2.19.2
> > > > >  
> > >  

Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Jonathan Cameron]

On Sun, 25 Nov 2018 20:05:09 +0100
Tomasz Duszynski <tduszyns@gmail.com> wrote:

> On Sun, Nov 25, 2018 at 08:56:59AM +0000, Jonathan Cameron wrote:
> > On Sat, 24 Nov 2018 23:14:14 +0100
> > Tomasz Duszynski <tduszyns@gmail.com> wrote:
> >  
> > > Add support for Sensirion SPS30 particulate matter sensor.
> > >
> > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>  
> > 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 000000000000..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 <tduszyns@gmail.com>
> > > + *
> > > + * 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 <linux/crc8.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +#include <linux/module.h>
> > > +
> > > +#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 paid for the low end to be certified (or there may be no
official test for higher levels?)

> 
> So having 12/32 bit setting, especially 32 storagebits protects against
> future changes in datasheet (which is btw preliminary) i.e updating
> measurement range.

Not really because userspace is going to assume the value is 12 bits and
will probably mask it as such, thus any change requires userspace to cope
with it. Once you are doing that, might as well just change the padding
as well.

> 
> But, I guess that 16 for real and storage bits should be more
> that enough.

Feels like it to me as well.

> 
> >
> > 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.

Hmm. So potentially it will give readings with more?  If so we should
work out a hard justification for that limit.  Userspace will mask
against it (as the rest of the bits may be garbage - we don't force
them to 0 in other drivers and they often contain meta data if
not actual garbage).

> 
> >
> >  
> > > +		.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!
> >  
> 
> Understood.
> 
> > > +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));
> > > +	if (ret != sizeof(tmp))
> > > +		return ret < 0 ? ret : -EIO;
> > > +
> > > +	for (i = 0; i < sizeof(tmp); i += 3) {
> > > +		u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE);
> > > +
> > > +		if (crc != tmp[i + 2]) {
> > > +			dev_err(&state->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 };
> > > +
> > > +	switch (cmd) {
> > > +	case SPS30_START_MEAS:
> > > +		buf[2] = 0x03;
> > > +		buf[3] = 0x00;
> > > +		buf[4] = 0xac; /* precomputed crc */  
> >
> > Could you put that in code and let the compiler do the pre compute?
> > Might be a little more elegant.
> >  
> 
> Okay.
> 
> > > +		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_ieee754_to_int(const u8 *data)
> > > +{
> > > +	u32 val = ((u32)data[0] << 24) | ((u32)data[1] << 16) |
> > > +		  ((u32)data[2] << 8) | (u32)data[3],  
> > Have a separate
> > u32 mantissa = (val << 9) >> 9 line.
> > What is this next line actually doing?  Kind of looks like
> > a mask?  If so just mask it with & as more readable.
> >  
> 
> Do you mean mantissa? We want to get rid of sign bit and 8 exponent bits.
Got it, but the above is the same as.

val & 0x7ffffff; I think...  Shifting like this immediately made me
think you were manually sign extending (but it's unsigned so obviously not).

It's a mask to extract just the mantissa, so code it as one.


> 
> > > +	    mantissa = (val << 9) >> 9;
> > > +	int exp = (val >> 23) - 127;
> > > +
> > > +	if (!exp && !mantissa)
> > > +		return 0;
> > > +
> > > +	if (exp < 0)
> > > +		return 0;
> > > +
> > > +	return (1 << exp) + (mantissa >> (23 - exp));
> > > +}
> > > +
> > > +static int sps30_do_meas(struct sps30_state *state, int *pm2p5, int *pm10)
> > > +{
> > > +	/*
> > > +	 * Internally sensor stores measurements in a following manner:
> > > +	 *
> > > +	 * PM1p0: upper two bytes, crc8, lower two bytes, crc8  
> >
> > So there are other particle sizes that this sensor reads?  Why
> > are we mapping them down to just the two channel types?
> >  
> 
> As said before, introducing PM1.0 and PM4.0 readings seems
> a reasonable option.

Go for it.  Userspace who doesn't care won't read them.

> 
> > > +	 * PM2p5: upper two bytes, crc8, lower two bytes, crc8
> > > +	 * PM4p0: upper two bytes, crc8, lower two bytes, crc8
> > > +	 * PM10:  upper two bytes, crc8, lower two bytes, crc8
> > > +	 *
> > > +	 * What follows next are number concentration measurements and
> > > +	 * typical particle size measurement.
> > > +	 *
> > > +	 * Once data is read from sensor crc bytes are stripped off
> > > +	 * hence we need 16 bytes of buffer space.
> > > +	 */
> > > +	int ret, tries = 5;
> > > +	u8 buf[16];
> > > +
> > > +	while (tries--) {
> > > +		ret = sps30_do_cmd(state, SPS30_READ_DATA_READY_FLAG, buf, 2);
> > > +		if (ret)
> > > +			return -EIO;
> > > +
> > > +		/* new measurements ready to be read */
> > > +		if (buf[1] == 1)
> > > +			break;
> > > +
> > > +		usleep_range(300000, 400000);
> > > +	}
> > > +
> > > +	if (!tries)
> > > +		return -ETIMEDOUT;
> > > +
> > > +	ret = sps30_do_cmd(state, SPS30_READ_DATA, buf, sizeof(buf));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * All measurements come in IEEE754 single precision floating point
> > > +	 * format but sensor itself is not precise enough (-+ 10% error)
> > > +	 * to take full advantage of it. Hence converting result to int
> > > +	 * to keep things simple.  
> >
> > Interesting.  We have talked about allowing floating point formats for
> > sensors the provide them.  Would that be beneficial here?
> >  
> 
> I recall this was discussed once but unfortunately do not
> remember final conclusion. Anyway, in case of this device datasheet
> states that readings have maximum error of -+10% in given range which
> makes me think that using floats here is an overkill.
> 
> Example, reading of 1000.123412ug/m3 has error of -+100ug/m3.
> Does it really matter if care about fractional part?

*laughs*.  It does make you wonder why they ended up as floating point
unless it is sensitive at very low levels? Note I know nothing about
particle sensing at all! 

> 
> Just out of curiosity, if IIO gets support for floats would it
> be problematic to to add extra channel returning measurements in floats?
> Or it rather will not fly..

It's hard for userspace to interpret the case of two channels measuring the
same thing.  So it could be done (and we have once or twice done this)
but it's messy.

> 
> > > +	 */
> > > +	*pm2p5 = sps30_ieee754_to_int(&buf[PM2p5 * 4]);
> > > +	*pm10 = sps30_ieee754_to_int(&buf[PM10 * 4]);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static irqreturn_t sps30_trigger_handler(int irq, void *p)
> > > +{
> > > +	struct iio_poll_func *pf = p;
> > > +	struct iio_dev *indio_dev = pf->indio_dev;
> > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > +	u32 buf[4]; /* PM2p5, PM10, timestamp */
> > > +	int ret;
> > > +
> > > +	mutex_lock(&state->lock);
> > > +	ret = sps30_do_meas(state, &buf[0], &buf[1]);
> > > +	mutex_unlock(&state->lock);
> > > +	if (ret < 0)
> > > +		goto err;
> > > +
> > > +	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> > > +					   iio_get_time_ns(indio_dev));
> > > +err:
> > > +	iio_trigger_notify_done(indio_dev->trig);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +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(&state->lock);
> > > +			switch (chan->channel2) {
> > > +			case IIO_MOD_PM2p5:
> > > +				ret = sps30_do_meas(state, val, val2);
> > > +				break;
> > > +			case IIO_MOD_PM10:
> > > +				ret = sps30_do_meas(state, val2, val);
> > > +				break;
> > > +			default:
> > > +				break;
> > > +			}
> > > +			mutex_unlock(&state->lock);
> > > +			if (ret)
> > > +				return ret;
> > > +
> > > +			return IIO_VAL_INT;
> > > +		default:
> > > +			return -EINVAL;
> > > +		}
> > > +		break;
> > > +	default:  
> > You can't get here.  Is this a warning suppression?
> > If so just add a comment saying so to prevent it being removed
> > by someone else later.
> >  
> 
> Okay.
> 
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static const struct iio_info sps30_info = {
> > > +	.read_raw = sps30_read_raw,
> > > +};
> > > +  
> > From a readability point of view, it would be helpful to pull
> > the macro SPS30_CHAN down here in the code so we don't
> > need to go jumping around to check what it is doing.
> >  
> 
> Okay.
> 
> > > +static const struct iio_chan_spec sps30_channels[] = {
> > > +	SPS30_CHAN(0, PM2p5),
> > > +	SPS30_CHAN(1, PM10),
> > > +	IIO_CHAN_SOFT_TIMESTAMP(2),
> > > +};
> > > +
> > > +static const unsigned long sps30_scan_masks[] = { 0x03, 0x00 };
> > > +
> > > +static int sps30_probe(struct i2c_client *client)
> > > +{
> > > +	struct iio_dev *indio_dev;
> > > +	struct sps30_state *state;
> > > +	u8 buf[32] = { };
> > > +	int ret;
> > > +
> > > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*state));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	state = iio_priv(indio_dev);
> > > +	i2c_set_clientdata(client, indio_dev);
> > > +	state->client = client;
> > > +	indio_dev->dev.parent = &client->dev;
> > > +	indio_dev->info = &sps30_info;
> > > +	indio_dev->name = client->name;
> > > +	indio_dev->channels = sps30_channels;
> > > +	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > +	indio_dev->available_scan_masks = sps30_scan_masks;
> > > +
> > > +	mutex_init(&state->lock);
> > > +	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> > > +
> > > +	/*
> > > +	 * Power-on-reset causes sensor to produce some glitch on i2c bus
> > > +	 * and some controllers end up in error state. Recover simply
> > > +	 * by placing something on the bus.
> > > +	 */
> > > +	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> > > +	if (ret) {
> > > +		dev_err(&client->dev, "failed to reset device\n");
> > > +		return ret;
> > > +	}
> > > +	usleep_range(2500000, 3500000);
> > > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);  
> >
> > Talk me through this logic.  We stop it then pretty much immediately
> > start it again?  Needs a comment.
> >  
> 
> The idea is to send some message after resetting sensor so that
> i2c controller recovers from error state. I decided to send STOP_MEAS
> as it's a NOP in this case. I'll try to do more testing with other
> SPS30s and perhaps different boards.

Ah.  Makes sense.  Just add a comment to the code saying this and it's
fine.

> 
> > > +
> > > +	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> > > +	if (ret) {
> > > +		dev_err(&client->dev, "failed to read serial number\n");
> > > +		return ret;
> > > +	}
> > > +	dev_info(&client->dev, "serial number: %s\n", buf);
> > > +
> > > +	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> > > +	if (ret) {  
> >
> > This is not unwound on error.   See comment on remove race below.
> >  
> > > +		dev_err(&client->dev, "failed to start measurement\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> > > +					      sps30_trigger_handler, NULL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return devm_iio_device_register(&client->dev, indio_dev);
> > > +}
> > > +
> > > +static int sps30_remove(struct i2c_client *client)
> > > +{
> > > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > +
> > > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);  
> > This looks like a race to me.
> >  
> 
> Right, this needs fixing.
> 
> > You turn off the measurements before removing the interface to them.
> > It's certainly not in the 'obviously' right category.
> >
> > As such I would either use a devm action to do this stop,
> > or not use the managed interfaces for everything in probe
> > after the equivalent start.
> > The devm_add_action_or_reset would be the cleanest option I think..
> > Will also fix the cleanup on error in probe.
> >  
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id sps30_id[] = {
> > > +	{ "sps30" },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, sps30_id);
> > > +
> > > +static const struct of_device_id sps30_of_match[] = {
> > > +	{ .compatible = "sensirion,sps30" },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, sps30_of_match);
> > > +
> > > +static struct i2c_driver sps30_driver = {
> > > +	.driver = {
> > > +		.name = "sps30",
> > > +		.of_match_table = sps30_of_match,
> > > +	},
> > > +	.id_table = sps30_id,
> > > +	.probe_new = sps30_probe,
> > > +	.remove = sps30_remove,
> > > +};
> > > +module_i2c_driver(sps30_driver);
> > > +
> > > +MODULE_AUTHOR("Tomasz Duszynski <tduszyns@gmail.com>");
> > > +MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver");
> > > +MODULE_LICENSE("GPL v2");  
> >  

Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Jonathan Cameron]

On Mon, 26 Nov 2018 21:48:07 +0100
Tomasz Duszynski <tduszyns@gmail.com> 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 <tduszyns@gmail.com>
> > > ---
> > >  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 <linux/crc8.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +#include <linux/module.h>
> > > +
> > > +#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=qPXydAacU1RqZWA@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 1/3] iio: add IIO_MASSCONCENTRATION channel type

[Matt Ranostay]

On Sat, Dec 1, 2018 at 5:48 PM Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
>
> On Sun, 25 Nov 2018 16:44:23 +0100
> Tomasz Duszynski <tduszyns@gmail.com> wrote:
>
> > On Sun, Nov 25, 2018 at 06:14:44AM -0800, Matt Ranostay wrote:
> > > On Sun, Nov 25, 2018 at 6:03 AM Jonathan Cameron
> > > <jic23@jic23.retrosnub.co.uk> wrote:
> > > >
> > > > On Sun, 25 Nov 2018 05:51:32 -0800
> > > > Matt Ranostay <matt.ranostay@konsulko.com> wrote:
> > > >
> > > > > On Sat, Nov 24, 2018 at 2:14 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
> > > > > >
> > > > > > Measuring particulate matter in ug / m3 (micro-grams per cubic meter)
> > > > > > is de facto standard. Existing air quality sensors usually follow
> > > > > > this convention and are capable of returning measurements using
> > > > > > this unit.
> > > > > >
> > > > > > IIO currently does not offer suitable channel type for this
> > > > > > type of measurements hence this patch adds this.
> > > > > >
> > > > > > In addition, two modifiers are introduced used for distinguishing
> > > > > > between coarse (PM10) and fine particles (PM2p5) measurements, i.e
> > > > > > IIO_MOD_PM10 and IIO_MOD_PM2p5.
> > > > > >
> > > > > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> > > > > > ---
> > > > > >  Documentation/ABI/testing/sysfs-bus-iio | 11 ++++++++++-
> > > > > >  drivers/iio/industrialio-core.c         |  3 +++
> > > > > >  include/uapi/linux/iio/types.h          |  3 +++
> > > > > >  tools/iio/iio_event_monitor.c           |  6 ++++++
> > > > > >  4 files changed, 22 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > > > > index 8127a08e366d..ce0ed140660d 100644
> > > > > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > > > > @@ -1684,4 +1684,13 @@ KernelVersion:   4.18
> > > > > >  Contact:       linux-iio@vger.kernel.org
> > > > > >  Description:
> > > > > >                 Raw (unscaled) phase difference reading from channel Y
> > > > > > -               that can be processed to radians.
> > > > > > \ No newline at end of file
> > > > > > +               that can be processed to radians.
> > > > > > +
> > > > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm2p5_input
> > > > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm2p5_input
> > > > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentration_pm10_input
> > > > > > +What:          /sys/bus/iio/devices/iio:deviceX/in_massconcentrationY_pm10_input
> > > > > > +KernelVersion: 4.21
> > > > > > +Contact:       linux-iio@vger.kernel.org
> > > > > > +Description:
> > > > > > +               Mass concentration reading of particulate matter in ug / m3.
> > > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > > > > index a062cfddc5af..2a9ef600c1fb 100644
> > > > > > --- a/drivers/iio/industrialio-core.c
> > > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > > @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > > > > >         [IIO_GRAVITY]  = "gravity",
> > > > > >         [IIO_POSITIONRELATIVE]  = "positionrelative",
> > > > > >         [IIO_PHASE] = "phase",
> > > > > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > > > > >  };
> > > > > >
> > > > > >  static const char * const iio_modifier_names[] = {
> > > > > > @@ -127,6 +128,8 @@ static const char * const iio_modifier_names[] = {
> > > > > >         [IIO_MOD_Q] = "q",
> > > > > >         [IIO_MOD_CO2] = "co2",
> > > > > >         [IIO_MOD_VOC] = "voc",
> > > > > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > > > > +       [IIO_MOD_PM10] = "pm10",
> > > > > >  };
> > > > > >
> > > > > >  /* relies on pairs of these shared then separate */
> > > > > > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > > > > > index 92baabc103ac..465044b42af5 100644
> > > > > > --- a/include/uapi/linux/iio/types.h
> > > > > > +++ b/include/uapi/linux/iio/types.h
> > > > > > @@ -46,6 +46,7 @@ enum iio_chan_type {
> > > > > >         IIO_GRAVITY,
> > > > > >         IIO_POSITIONRELATIVE,
> > > > > >         IIO_PHASE,
> > > > > > +       IIO_MASSCONCENTRATION,
> > > > >
> > > > > So I'm guessing IIO_CONCENTRATION can't be scaled to the micro-grams
> > > > > per cubic meter?
> > > >
> > > > Currently concentration is defined as a percentage reading of a substance
> > > > (which does make me wonder if it is meant to be percentage of the volume or
> > > > percentage of the mass?)  Either way, if can't convert to a density measurement
> > > > as it's a unit free ratio (I think).
> > >
> > > Seems like it can be both..  guessing all the atmosphere ( CO2, VOC,
> > > etc) ones are mass/density because on how they work.
> >
> > Hmm, but still percentage was picked up for IIO_CONCENTRATION which does
> > really match PM expectations. Perhaps if units were sticked to modifiers
> > it whould be easier to reuse that.
>
> It gets very messy if we start doing that modifiers, I'd rather stick to
> different channel types.  We already do this with other channel types
> position / positionrelative for example.
>
> It isn't as though it's actually possible to convert between the two without
> knowing what the particles actually are...

Wondering if some of the current chemical sensors in the tree should
be switched to IIO_MASSCONCENTRATION, and only one I can think is
truly by volume is the electro-conductivity sensor (part of the
atlas-ph-sensor.c)

- Matt

>
> Jonathan
> >
> > > But the electro-conductivity sensor that is using IIO_CONCENTRATION
> > > channels is likely from percentage of volume.
> > >
> > > - Matt
> > >
> > > >
> > > > >
> > > > > >  };
> > > > > >
> > > > > >  enum iio_modifier {
> > > > > > @@ -87,6 +88,8 @@ enum iio_modifier {
> > > > > >         IIO_MOD_VOC,
> > > > > >         IIO_MOD_LIGHT_UV,
> > > > > >         IIO_MOD_LIGHT_DUV,
> > > > > > +       IIO_MOD_PM2p5,
> > > > >
> > > > > I know this is unit of measure but the lowercase p in IIO_MOD_PM2p5 is
> > > > > a bit non-standard for iio defines/enum.
> > > > It is a bit odd and will get us scripted reports so maybe best to
> > > > just go upper case and not worry about it.
> > > >
> > > > Jonathan
> > > > >
> > > > > - Matt
> > > > >
> > > > > > +       IIO_MOD_PM10,
> > > > > >  };
> > > > > >
> > > > > >  enum iio_event_type {
> > > > > > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > > > > > index ac2de6b7e89f..f0fcfeddba2b 100644
> > > > > > --- a/tools/iio/iio_event_monitor.c
> > > > > > +++ b/tools/iio/iio_event_monitor.c
> > > > > > @@ -60,6 +60,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > > > > >         [IIO_GRAVITY] = "gravity",
> > > > > >         [IIO_POSITIONRELATIVE] = "positionrelative",
> > > > > >         [IIO_PHASE] = "phase",
> > > > > > +       [IIO_MASSCONCENTRATION] = "massconcentration",
> > > > > >  };
> > > > > >
> > > > > >  static const char * const iio_ev_type_text[] = {
> > > > > > @@ -115,6 +116,8 @@ static const char * const iio_modifier_names[] = {
> > > > > >         [IIO_MOD_Q] = "q",
> > > > > >         [IIO_MOD_CO2] = "co2",
> > > > > >         [IIO_MOD_VOC] = "voc",
> > > > > > +       [IIO_MOD_PM2p5] = "pm2p5",
> > > > > > +       [IIO_MOD_PM10] = "pm10",
> > > > > >  };
> > > > > >
> > > > > >  static bool event_is_known(struct iio_event_data *event)
> > > > > > @@ -156,6 +159,7 @@ static bool event_is_known(struct iio_event_data *event)
> > > > > >         case IIO_GRAVITY:
> > > > > >         case IIO_POSITIONRELATIVE:
> > > > > >         case IIO_PHASE:
> > > > > > +       case IIO_MASSCONCENTRATION:
> > > > > >                 break;
> > > > > >         default:
> > > > > >                 return false;
> > > > > > @@ -200,6 +204,8 @@ static bool event_is_known(struct iio_event_data *event)
> > > > > >         case IIO_MOD_Q:
> > > > > >         case IIO_MOD_CO2:
> > > > > >         case IIO_MOD_VOC:
> > > > > > +       case IIO_MOD_PM2p5:
> > > > > > +       case IIO_MOD_PM10:
> > > > > >                 break;
> > > > > >         default:
> > > > > >                 return false;
> > > > > > --
> > > > > > 2.19.2
> > > > > >
> > > >
>

Re: [PATCH 3/3] iio: chemical: sps30: add device tree support

[Tomasz Duszynski]

On Sun, Nov 25, 2018 at 08:59:39AM +0000, Jonathan Cameron wrote:
> On Sat, 24 Nov 2018 23:14:15 +0100
> Tomasz Duszynski <tduszyns@gmail.com> wrote:
>
> > Add device tree support for Sensirion SPS30 particulate
> > matter sensor.
> >
> > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> one comment inine, around the fact we are trying to move
> to generic names in DT where ever possible.  Now we don't
> have a suitable one (IIRC) yet so time to make one up ;)
>
> +CC Rob for his input on that.
> > ---
> >  .../bindings/iio/chemical/sensirion,sps30.txt        | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> >
> > diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt b/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> > new file mode 100644
> > index 000000000000..6eee2709b5b6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> > @@ -0,0 +1,12 @@
> > +* Sensirion SPS30 particulate matter sensor
> > +
> > +Required properties:
> > +- compatible: must be "sensirion,sps30"
> > +- reg: the I2C address of the sensor
> > +
> > +Example:
> > +
> > +sps30@69 {
> We should define a generic type.  Rob, what would work for this
> one?
>
> particlesensor@69?
>

Wouldn't air-pollution-sensor be somewhat more generic? At least
wikipedia has some article about that. Various other names like
particle-sensor, pm-sensor, particulate-matter-sensor,
air-quality-sensor, tend to return more or less similar number
of search hits. Which means there's no universal naming convention.

> > +	compatible = "sensirion,sps30";
> > +	reg = <0x69>;
> > +};
>

Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Andreas Brauchli]

From: Andreas Brauchli <a.brauchli@elementarea.net>

On Sat, 1 Dec 2018 15:58:57 +0000, Jonathan Cameron wrote:
> On Sun, 25 Nov 2018 20:05:09 +0100
> Tomasz Duszynski <tduszyns@gmail.com> wrote:
> 
> > On Sun, Nov 25, 2018 at 08:56:59AM +0000, Jonathan Cameron wrote:
> > > On Sat, 24 Nov 2018 23:14:14 +0100
> > > Tomasz Duszynski <tduszyns@gmail.com> 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 <andreas.brauchli@sensirion.com>

> > > >
> > > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>  
> > > 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 000000000000..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 <tduszyns@gmail.com>
> > > > + *
> > > > + * 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 <linux/crc8.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/iio/buffer.h>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
> > > > +#include <linux/iio/trigger_consumer.h>
> > > > +#include <linux/iio/triggered_buffer.h>
> > > > +#include <linux/module.h>
> > > > +
> > > > +#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 = 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 paid for the low end to be certified (or there may be no
> official test for higher levels?)
> 
> > 
> > So having 12/32 bit setting, especially 32 storagebits protects against
> > future changes in datasheet (which is btw preliminary) i.e updating
> > measurement range.

The final datasheet is expected in Q1 2019. You could add the following link
which always points to the latest version:

https://www.sensirion.com/file/datasheet_sps30

> 
> Not really because userspace is going to assume the value is 12 bits and
> will probably mask it as such, thus any change requires userspace to cope
> with it. Once you are doing that, might as well just change the padding
> as well.
> 
> > 
> > But, I guess that 16 for real and storage bits should be more
> > that enough.
> 
> Feels like it to me as well.
> 
> > 
> > >
> > > 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.
> 
> Hmm. So potentially it will give readings with more?  If so we should
> work out a hard justification for that limit.  Userspace will mask
> against it (as the rest of the bits may be garbage - we don't force
> them to 0 in other drivers and they often contain meta data if
> not actual garbage).

The SPS30 can output high values (e.g. 60,000), but the measurement principle
only applies up to somewhere between 3000-5000 ug/m3. Anything above 3000 is
highly imprecise.

> 
> > 
> > >
> > >  
> > > > +		.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!
> > >  
> > 
> > Understood.
> > 
> > > > +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));
> > > > +	if (ret != sizeof(tmp))
> > > > +		return ret < 0 ? ret : -EIO;
> > > > +
> > > > +	for (i = 0; i < sizeof(tmp); i += 3) {
> > > > +		u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE);
> > > > +
> > > > +		if (crc != tmp[i + 2]) {
> > > > +			dev_err(&state->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 };
> > > > +
> > > > +	switch (cmd) {
> > > > +	case SPS30_START_MEAS:
> > > > +		buf[2] = 0x03;
> > > > +		buf[3] = 0x00;
> > > > +		buf[4] = 0xac; /* precomputed crc */  
> > >
> > > Could you put that in code and let the compiler do the pre compute?
> > > Might be a little more elegant.
> > >  
> > 
> > Okay.
> > 
> > > > +		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_ieee754_to_int(const u8 *data)
> > > > +{
> > > > +	u32 val = ((u32)data[0] << 24) | ((u32)data[1] << 16) |
> > > > +		  ((u32)data[2] << 8) | (u32)data[3],  
> > > Have a separate
> > > u32 mantissa = (val << 9) >> 9 line.
> > > What is this next line actually doing?  Kind of looks like
> > > a mask?  If so just mask it with & as more readable.
> > >  
> > 
> > Do you mean mantissa? We want to get rid of sign bit and 8 exponent bits.
> Got it, but the above is the same as.
> 
> val & 0x7ffffff; I think...  Shifting like this immediately made me
> think you were manually sign extending (but it's unsigned so obviously not).
> 
> It's a mask to extract just the mantissa, so code it as one.
> 
> 
> > 
> > > > +	    mantissa = (val << 9) >> 9;
> > > > +	int exp = (val >> 23) - 127;
> > > > +
> > > > +	if (!exp && !mantissa)
> > > > +		return 0;
> > > > +
> > > > +	if (exp < 0)
> > > > +		return 0;
> > > > +
> > > > +	return (1 << exp) + (mantissa >> (23 - exp));
> > > > +}
> > > > +
> > > > +static int sps30_do_meas(struct sps30_state *state, int *pm2p5, int *pm10)
> > > > +{
> > > > +	/*
> > > > +	 * Internally sensor stores measurements in a following manner:
> > > > +	 *
> > > > +	 * PM1p0: upper two bytes, crc8, lower two bytes, crc8  
> > >
> > > So there are other particle sizes that this sensor reads?  Why
> > > are we mapping them down to just the two channel types?
> > >  
> > 
> > As said before, introducing PM1.0 and PM4.0 readings seems
> > a reasonable option.
> 
> Go for it.  Userspace who doesn't care won't read them.
> 
> > 
> > > > +	 * PM2p5: upper two bytes, crc8, lower two bytes, crc8
> > > > +	 * PM4p0: upper two bytes, crc8, lower two bytes, crc8
> > > > +	 * PM10:  upper two bytes, crc8, lower two bytes, crc8
> > > > +	 *
> > > > +	 * What follows next are number concentration measurements and
> > > > +	 * typical particle size measurement.
> > > > +	 *
> > > > +	 * Once data is read from sensor crc bytes are stripped off
> > > > +	 * hence we need 16 bytes of buffer space.
> > > > +	 */
> > > > +	int ret, tries = 5;
> > > > +	u8 buf[16];
> > > > +
> > > > +	while (tries--) {
> > > > +		ret = sps30_do_cmd(state, SPS30_READ_DATA_READY_FLAG, buf, 2);
> > > > +		if (ret)
> > > > +			return -EIO;
> > > > +
> > > > +		/* new measurements ready to be read */
> > > > +		if (buf[1] == 1)
> > > > +			break;
> > > > +
> > > > +		usleep_range(300000, 400000);
> > > > +	}
> > > > +
> > > > +	if (!tries)
> > > > +		return -ETIMEDOUT;
> > > > +
> > > > +	ret = sps30_do_cmd(state, SPS30_READ_DATA, buf, sizeof(buf));
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * All measurements come in IEEE754 single precision floating point
> > > > +	 * format but sensor itself is not precise enough (-+ 10% error)
> > > > +	 * to take full advantage of it. Hence converting result to int
> > > > +	 * to keep things simple.  
> > >
> > > Interesting.  We have talked about allowing floating point formats for
> > > sensors the provide them.  Would that be beneficial here?
> > >  
> > 
> > I recall this was discussed once but unfortunately do not
> > remember final conclusion. Anyway, in case of this device datasheet
> > states that readings have maximum error of -+10% in given range which
> > makes me think that using floats here is an overkill.
> > 
> > Example, reading of 1000.123412ug/m3 has error of -+100ug/m3.
> > Does it really matter if care about fractional part?

The datasheet specifies absolute precision, the relative precision is much
better. The noise level is at ca. 1%. We thus suggest using 16bit values with
2bits dedicated to fixed point decimals.

> 
> *laughs*.  It does make you wonder why they ended up as floating point
> unless it is sensitive at very low levels? Note I know nothing about
> particle sensing at all! 
> 

..used internally

> > 
> > Just out of curiosity, if IIO gets support for floats would it
> > be problematic to to add extra channel returning measurements in floats?
> > Or it rather will not fly..
> 
> It's hard for userspace to interpret the case of two channels measuring the
> same thing.  So it could be done (and we have once or twice done this)
> but it's messy.
> 
> > 
> > > > +	 */
> > > > +	*pm2p5 = sps30_ieee754_to_int(&buf[PM2p5 * 4]);
> > > > +	*pm10 = sps30_ieee754_to_int(&buf[PM10 * 4]);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static irqreturn_t sps30_trigger_handler(int irq, void *p)
> > > > +{
> > > > +	struct iio_poll_func *pf = p;
> > > > +	struct iio_dev *indio_dev = pf->indio_dev;
> > > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > > +	u32 buf[4]; /* PM2p5, PM10, timestamp */
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&state->lock);
> > > > +	ret = sps30_do_meas(state, &buf[0], &buf[1]);
> > > > +	mutex_unlock(&state->lock);
> > > > +	if (ret < 0)
> > > > +		goto err;
> > > > +
> > > > +	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> > > > +					   iio_get_time_ns(indio_dev));
> > > > +err:
> > > > +	iio_trigger_notify_done(indio_dev->trig);
> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +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(&state->lock);
> > > > +			switch (chan->channel2) {
> > > > +			case IIO_MOD_PM2p5:
> > > > +				ret = sps30_do_meas(state, val, val2);
> > > > +				break;
> > > > +			case IIO_MOD_PM10:
> > > > +				ret = sps30_do_meas(state, val2, val);
> > > > +				break;
> > > > +			default:
> > > > +				break;
> > > > +			}
> > > > +			mutex_unlock(&state->lock);
> > > > +			if (ret)
> > > > +				return ret;
> > > > +
> > > > +			return IIO_VAL_INT;
> > > > +		default:
> > > > +			return -EINVAL;
> > > > +		}
> > > > +		break;
> > > > +	default:  
> > > You can't get here.  Is this a warning suppression?
> > > If so just add a comment saying so to prevent it being removed
> > > by someone else later.
> > >  
> > 
> > Okay.
> > 
> > > > +		return -EINVAL;
> > > > +	}
> > > > +}
> > > > +
> > > > +static const struct iio_info sps30_info = {
> > > > +	.read_raw = sps30_read_raw,
> > > > +};
> > > > +  
> > > From a readability point of view, it would be helpful to pull
> > > the macro SPS30_CHAN down here in the code so we don't
> > > need to go jumping around to check what it is doing.
> > >  
> > 
> > Okay.
> > 
> > > > +static const struct iio_chan_spec sps30_channels[] = {
> > > > +	SPS30_CHAN(0, PM2p5),
> > > > +	SPS30_CHAN(1, PM10),
> > > > +	IIO_CHAN_SOFT_TIMESTAMP(2),
> > > > +};
> > > > +
> > > > +static const unsigned long sps30_scan_masks[] = { 0x03, 0x00 };
> > > > +
> > > > +static int sps30_probe(struct i2c_client *client)
> > > > +{
> > > > +	struct iio_dev *indio_dev;
> > > > +	struct sps30_state *state;
> > > > +	u8 buf[32] = { };
> > > > +	int ret;
> > > > +
> > > > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*state));
> > > > +	if (!indio_dev)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	state = iio_priv(indio_dev);
> > > > +	i2c_set_clientdata(client, indio_dev);
> > > > +	state->client = client;
> > > > +	indio_dev->dev.parent = &client->dev;
> > > > +	indio_dev->info = &sps30_info;
> > > > +	indio_dev->name = client->name;
> > > > +	indio_dev->channels = sps30_channels;
> > > > +	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> > > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > > +	indio_dev->available_scan_masks = sps30_scan_masks;
> > > > +
> > > > +	mutex_init(&state->lock);
> > > > +	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> > > > +
> > > > +	/*
> > > > +	 * Power-on-reset causes sensor to produce some glitch on i2c bus
> > > > +	 * and some controllers end up in error state. Recover simply
> > > > +	 * by placing something on the bus.
> > > > +	 */

Apologies for that. I talked to the firmware engineer and we'll do our best to
have it fixed in the next update scheduled for Q1 2019. The cause seems to be
the pin initialization causing the clock to be pulled down for ca. 2.5us.

> > > > +	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> > > > +	if (ret) {
> > > > +		dev_err(&client->dev, "failed to reset device\n");
> > > > +		return ret;
> > > > +	}
> > > > +	usleep_range(2500000, 3500000);
> > > > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);  
> > >
> > > Talk me through this logic.  We stop it then pretty much immediately
> > > start it again?  Needs a comment.
> > >  
> > 
> > The idea is to send some message after resetting sensor so that
> > i2c controller recovers from error state. I decided to send STOP_MEAS
> > as it's a NOP in this case. I'll try to do more testing with other
> > SPS30s and perhaps different boards.
> 
> Ah.  Makes sense.  Just add a comment to the code saying this and it's
> fine.

Not to shift the blame, but the recovery is likely controller specific and might
be better handled at that level. Consider the case where the faulty device is
present but the driver is not loaded after the initialization..

> 
> > 
> > > > +
> > > > +	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> > > > +	if (ret) {
> > > > +		dev_err(&client->dev, "failed to read serial number\n");
> > > > +		return ret;
> > > > +	}
> > > > +	dev_info(&client->dev, "serial number: %s\n", buf);
> > > > +
> > > > +	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> > > > +	if (ret) {  
> > >
> > > This is not unwound on error.   See comment on remove race below.
> > >  
> > > > +		dev_err(&client->dev, "failed to start measurement\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> > > > +					      sps30_trigger_handler, NULL);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return devm_iio_device_register(&client->dev, indio_dev);
> > > > +}
> > > > +
> > > > +static int sps30_remove(struct i2c_client *client)
> > > > +{
> > > > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > > +
> > > > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);  
> > > This looks like a race to me.
> > >  
> > 
> > Right, this needs fixing.
> > 
> > > You turn off the measurements before removing the interface to them.
> > > It's certainly not in the 'obviously' right category.
> > >
> > > As such I would either use a devm action to do this stop,
> > > or not use the managed interfaces for everything in probe
> > > after the equivalent start.
> > > The devm_add_action_or_reset would be the cleanest option I think..
> > > Will also fix the cleanup on error in probe.
> > >  
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct i2c_device_id sps30_id[] = {
> > > > +	{ "sps30" },
> > > > +	{ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(i2c, sps30_id);
> > > > +
> > > > +static const struct of_device_id sps30_of_match[] = {
> > > > +	{ .compatible = "sensirion,sps30" },
> > > > +	{ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, sps30_of_match);
> > > > +
> > > > +static struct i2c_driver sps30_driver = {
> > > > +	.driver = {
> > > > +		.name = "sps30",
> > > > +		.of_match_table = sps30_of_match,
> > > > +	},
> > > > +	.id_table = sps30_id,
> > > > +	.probe_new = sps30_probe,
> > > > +	.remove = sps30_remove,
> > > > +};
> > > > +module_i2c_driver(sps30_driver);
> > > > +
> > > > +MODULE_AUTHOR("Tomasz Duszynski <tduszyns@gmail.com>");
> > > > +MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver");
> > > > +MODULE_LICENSE("GPL v2");  
> > >  

Re: [PATCH 3/3] iio: chemical: sps30: add device tree support

[Jonathan Cameron]

On Sun, 2 Dec 2018 19:29:44 +0100
Tomasz Duszynski <tduszyns@gmail.com> wrote:

> On Sun, Nov 25, 2018 at 08:59:39AM +0000, Jonathan Cameron wrote:
> > On Sat, 24 Nov 2018 23:14:15 +0100
> > Tomasz Duszynski <tduszyns@gmail.com> wrote:
> >  
> > > Add device tree support for Sensirion SPS30 particulate
> > > matter sensor.
> > >
> > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>  
> > one comment inine, around the fact we are trying to move
> > to generic names in DT where ever possible.  Now we don't
> > have a suitable one (IIRC) yet so time to make one up ;)
> >
> > +CC Rob for his input on that.  
> > > ---
> > >  .../bindings/iio/chemical/sensirion,sps30.txt        | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt b/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> > > new file mode 100644
> > > index 000000000000..6eee2709b5b6
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,sps30.txt
> > > @@ -0,0 +1,12 @@
> > > +* Sensirion SPS30 particulate matter sensor
> > > +
> > > +Required properties:
> > > +- compatible: must be "sensirion,sps30"
> > > +- reg: the I2C address of the sensor
> > > +
> > > +Example:
> > > +
> > > +sps30@69 {  
> > We should define a generic type.  Rob, what would work for this
> > one?
> >
> > particlesensor@69?
> >  
> 
> Wouldn't air-pollution-sensor be somewhat more generic? At least
> wikipedia has some article about that. Various other names like
> particle-sensor, pm-sensor, particulate-matter-sensor,
> air-quality-sensor, tend to return more or less similar number
> of search hits. Which means there's no universal naming convention.

I have not strong feeling in favor of a particular choice. 
Happy with wherever the discussion converges.

Jonathan
> 
> > > +	compatible = "sensirion,sps30";
> > > +	reg = <0x69>;
> > > +};  
> >  

Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

[Tomasz Duszynski]

On Mon, Dec 03, 2018 at 11:30:45AM +0100, Andreas Brauchli wrote:
> From: Andreas Brauchli <a.brauchli@elementarea.net>
>
> On Sat, 1 Dec 2018 15:58:57 +0000, Jonathan Cameron wrote:
> > On Sun, 25 Nov 2018 20:05:09 +0100
> > Tomasz Duszynski <tduszyns@gmail.com> wrote:
> >
> > > On Sun, Nov 25, 2018 at 08:56:59AM +0000, Jonathan Cameron wrote:
> > > > On Sat, 24 Nov 2018 23:14:14 +0100
> > > > Tomasz Duszynski <tduszyns@gmail.com> 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 <andreas.brauchli@sensirion.com>
>
> > > > >
> > > > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>
> > > > 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 000000000000..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 <tduszyns@gmail.com>
> > > > > + *
> > > > > + * 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 <linux/crc8.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/i2c.h>
> > > > > +#include <linux/iio/buffer.h>
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/iio/sysfs.h>
> > > > > +#include <linux/iio/trigger_consumer.h>
> > > > > +#include <linux/iio/triggered_buffer.h>
> > > > > +#include <linux/module.h>
> > > > > +
> > > > > +#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
>

Good catch.

> > > > > +#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 paid for the low end to be certified (or there may be no
> > official test for higher levels?)
> >
> > >
> > > So having 12/32 bit setting, especially 32 storagebits protects against
> > > future changes in datasheet (which is btw preliminary) i.e updating
> > > measurement range.
>
> The final datasheet is expected in Q1 2019. You could add the following link
> which always points to the latest version:
>
> https://www.sensirion.com/file/datasheet_sps30
>

Personally, I dislike the idea of adding datasheet links to the drivers.
Rationale being that they become outdated pretty soon. Anyone interested
in the details can find the doc pretty easily himself.

> >
> > Not really because userspace is going to assume the value is 12 bits and
> > will probably mask it as such, thus any change requires userspace to cope
> > with it. Once you are doing that, might as well just change the padding
> > as well.
> >
> > >
> > > But, I guess that 16 for real and storage bits should be more
> > > that enough.
> >
> > Feels like it to me as well.
> >
> > >
> > > >
> > > > 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.
> >
> > Hmm. So potentially it will give readings with more?  If so we should
> > work out a hard justification for that limit.  Userspace will mask
> > against it (as the rest of the bits may be garbage - we don't force
> > them to 0 in other drivers and they often contain meta data if
> > not actual garbage).
>
> The SPS30 can output high values (e.g. 60,000), but the measurement principle
> only applies up to somewhere between 3000-5000 ug/m3. Anything above 3000 is
> highly imprecise.
>

In that case, clamping the measurements to 3000.00 would be fine I
guess.

> >
> > >
> > > >
> > > >
> > > > > +		.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!
> > > >
> > >
> > > Understood.
> > >
> > > > > +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));
> > > > > +	if (ret != sizeof(tmp))
> > > > > +		return ret < 0 ? ret : -EIO;
> > > > > +
> > > > > +	for (i = 0; i < sizeof(tmp); i += 3) {
> > > > > +		u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE);
> > > > > +
> > > > > +		if (crc != tmp[i + 2]) {
> > > > > +			dev_err(&state->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 };
> > > > > +
> > > > > +	switch (cmd) {
> > > > > +	case SPS30_START_MEAS:
> > > > > +		buf[2] = 0x03;
> > > > > +		buf[3] = 0x00;
> > > > > +		buf[4] = 0xac; /* precomputed crc */
> > > >
> > > > Could you put that in code and let the compiler do the pre compute?
> > > > Might be a little more elegant.
> > > >
> > >
> > > Okay.
> > >
> > > > > +		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_ieee754_to_int(const u8 *data)
> > > > > +{
> > > > > +	u32 val = ((u32)data[0] << 24) | ((u32)data[1] << 16) |
> > > > > +		  ((u32)data[2] << 8) | (u32)data[3],
> > > > Have a separate
> > > > u32 mantissa = (val << 9) >> 9 line.
> > > > What is this next line actually doing?  Kind of looks like
> > > > a mask?  If so just mask it with & as more readable.
> > > >
> > >
> > > Do you mean mantissa? We want to get rid of sign bit and 8 exponent bits.
> > Got it, but the above is the same as.
> >
> > val & 0x7ffffff; I think...  Shifting like this immediately made me
> > think you were manually sign extending (but it's unsigned so obviously not).
> >
> > It's a mask to extract just the mantissa, so code it as one.
> >
> >
> > >
> > > > > +	    mantissa = (val << 9) >> 9;
> > > > > +	int exp = (val >> 23) - 127;
> > > > > +
> > > > > +	if (!exp && !mantissa)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (exp < 0)
> > > > > +		return 0;
> > > > > +
> > > > > +	return (1 << exp) + (mantissa >> (23 - exp));
> > > > > +}
> > > > > +
> > > > > +static int sps30_do_meas(struct sps30_state *state, int *pm2p5, int *pm10)
> > > > > +{
> > > > > +	/*
> > > > > +	 * Internally sensor stores measurements in a following manner:
> > > > > +	 *
> > > > > +	 * PM1p0: upper two bytes, crc8, lower two bytes, crc8
> > > >
> > > > So there are other particle sizes that this sensor reads?  Why
> > > > are we mapping them down to just the two channel types?
> > > >
> > >
> > > As said before, introducing PM1.0 and PM4.0 readings seems
> > > a reasonable option.
> >
> > Go for it.  Userspace who doesn't care won't read them.
> >
> > >
> > > > > +	 * PM2p5: upper two bytes, crc8, lower two bytes, crc8
> > > > > +	 * PM4p0: upper two bytes, crc8, lower two bytes, crc8
> > > > > +	 * PM10:  upper two bytes, crc8, lower two bytes, crc8
> > > > > +	 *
> > > > > +	 * What follows next are number concentration measurements and
> > > > > +	 * typical particle size measurement.
> > > > > +	 *
> > > > > +	 * Once data is read from sensor crc bytes are stripped off
> > > > > +	 * hence we need 16 bytes of buffer space.
> > > > > +	 */
> > > > > +	int ret, tries = 5;
> > > > > +	u8 buf[16];
> > > > > +
> > > > > +	while (tries--) {
> > > > > +		ret = sps30_do_cmd(state, SPS30_READ_DATA_READY_FLAG, buf, 2);
> > > > > +		if (ret)
> > > > > +			return -EIO;
> > > > > +
> > > > > +		/* new measurements ready to be read */
> > > > > +		if (buf[1] == 1)
> > > > > +			break;
> > > > > +
> > > > > +		usleep_range(300000, 400000);
> > > > > +	}
> > > > > +
> > > > > +	if (!tries)
> > > > > +		return -ETIMEDOUT;
> > > > > +
> > > > > +	ret = sps30_do_cmd(state, SPS30_READ_DATA, buf, sizeof(buf));
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	/*
> > > > > +	 * All measurements come in IEEE754 single precision floating point
> > > > > +	 * format but sensor itself is not precise enough (-+ 10% error)
> > > > > +	 * to take full advantage of it. Hence converting result to int
> > > > > +	 * to keep things simple.
> > > >
> > > > Interesting.  We have talked about allowing floating point formats for
> > > > sensors the provide them.  Would that be beneficial here?
> > > >
> > >
> > > I recall this was discussed once but unfortunately do not
> > > remember final conclusion. Anyway, in case of this device datasheet
> > > states that readings have maximum error of -+10% in given range which
> > > makes me think that using floats here is an overkill.
> > >
> > > Example, reading of 1000.123412ug/m3 has error of -+100ug/m3.
> > > Does it really matter if care about fractional part?
>
> The datasheet specifies absolute precision, the relative precision is much
> better. The noise level is at ca. 1%. We thus suggest using 16bit values with
> 2bits dedicated to fixed point decimals.
>

Wondering why that was not mentioned in the datasheet in the first place
but good to know anyway. Then I am fine with adding that extra level of
precision i.e two extra decimal places.

> >
> > *laughs*.  It does make you wonder why they ended up as floating point
> > unless it is sensitive at very low levels? Note I know nothing about
> > particle sensing at all!
> >
>
> ..used internally
>
> > >
> > > Just out of curiosity, if IIO gets support for floats would it
> > > be problematic to to add extra channel returning measurements in floats?
> > > Or it rather will not fly..
> >
> > It's hard for userspace to interpret the case of two channels measuring the
> > same thing.  So it could be done (and we have once or twice done this)
> > but it's messy.
> >
> > >
> > > > > +	 */
> > > > > +	*pm2p5 = sps30_ieee754_to_int(&buf[PM2p5 * 4]);
> > > > > +	*pm10 = sps30_ieee754_to_int(&buf[PM10 * 4]);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static irqreturn_t sps30_trigger_handler(int irq, void *p)
> > > > > +{
> > > > > +	struct iio_poll_func *pf = p;
> > > > > +	struct iio_dev *indio_dev = pf->indio_dev;
> > > > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > > > +	u32 buf[4]; /* PM2p5, PM10, timestamp */
> > > > > +	int ret;
> > > > > +
> > > > > +	mutex_lock(&state->lock);
> > > > > +	ret = sps30_do_meas(state, &buf[0], &buf[1]);
> > > > > +	mutex_unlock(&state->lock);
> > > > > +	if (ret < 0)
> > > > > +		goto err;
> > > > > +
> > > > > +	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> > > > > +					   iio_get_time_ns(indio_dev));
> > > > > +err:
> > > > > +	iio_trigger_notify_done(indio_dev->trig);
> > > > > +
> > > > > +	return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +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(&state->lock);
> > > > > +			switch (chan->channel2) {
> > > > > +			case IIO_MOD_PM2p5:
> > > > > +				ret = sps30_do_meas(state, val, val2);
> > > > > +				break;
> > > > > +			case IIO_MOD_PM10:
> > > > > +				ret = sps30_do_meas(state, val2, val);
> > > > > +				break;
> > > > > +			default:
> > > > > +				break;
> > > > > +			}
> > > > > +			mutex_unlock(&state->lock);
> > > > > +			if (ret)
> > > > > +				return ret;
> > > > > +
> > > > > +			return IIO_VAL_INT;
> > > > > +		default:
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +		break;
> > > > > +	default:
> > > > You can't get here.  Is this a warning suppression?
> > > > If so just add a comment saying so to prevent it being removed
> > > > by someone else later.
> > > >
> > >
> > > Okay.
> > >
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static const struct iio_info sps30_info = {
> > > > > +	.read_raw = sps30_read_raw,
> > > > > +};
> > > > > +
> > > > From a readability point of view, it would be helpful to pull
> > > > the macro SPS30_CHAN down here in the code so we don't
> > > > need to go jumping around to check what it is doing.
> > > >
> > >
> > > Okay.
> > >
> > > > > +static const struct iio_chan_spec sps30_channels[] = {
> > > > > +	SPS30_CHAN(0, PM2p5),
> > > > > +	SPS30_CHAN(1, PM10),
> > > > > +	IIO_CHAN_SOFT_TIMESTAMP(2),
> > > > > +};
> > > > > +
> > > > > +static const unsigned long sps30_scan_masks[] = { 0x03, 0x00 };
> > > > > +
> > > > > +static int sps30_probe(struct i2c_client *client)
> > > > > +{
> > > > > +	struct iio_dev *indio_dev;
> > > > > +	struct sps30_state *state;
> > > > > +	u8 buf[32] = { };
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > > > > +		return -EOPNOTSUPP;
> > > > > +
> > > > > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*state));
> > > > > +	if (!indio_dev)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	state = iio_priv(indio_dev);
> > > > > +	i2c_set_clientdata(client, indio_dev);
> > > > > +	state->client = client;
> > > > > +	indio_dev->dev.parent = &client->dev;
> > > > > +	indio_dev->info = &sps30_info;
> > > > > +	indio_dev->name = client->name;
> > > > > +	indio_dev->channels = sps30_channels;
> > > > > +	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> > > > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > +	indio_dev->available_scan_masks = sps30_scan_masks;
> > > > > +
> > > > > +	mutex_init(&state->lock);
> > > > > +	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> > > > > +
> > > > > +	/*
> > > > > +	 * Power-on-reset causes sensor to produce some glitch on i2c bus
> > > > > +	 * and some controllers end up in error state. Recover simply
> > > > > +	 * by placing something on the bus.
> > > > > +	 */
>
> Apologies for that. I talked to the firmware engineer and we'll do our best to
> have it fixed in the next update scheduled for Q1 2019. The cause seems to be
> the pin initialization causing the clock to be pulled down for ca. 2.5us.
>

From what I see SDA is pulled low as well for the same amount of time and
interestingly in happens always in about ~10ms after reset.

> > > > > +	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> > > > > +	if (ret) {
> > > > > +		dev_err(&client->dev, "failed to reset device\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +	usleep_range(2500000, 3500000);
> > > > > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> > > >
> > > > Talk me through this logic.  We stop it then pretty much immediately
> > > > start it again?  Needs a comment.
> > > >
> > >
> > > The idea is to send some message after resetting sensor so that
> > > i2c controller recovers from error state. I decided to send STOP_MEAS
> > > as it's a NOP in this case. I'll try to do more testing with other
> > > SPS30s and perhaps different boards.
> >
> > Ah.  Makes sense.  Just add a comment to the code saying this and it's
> > fine.
>
> Not to shift the blame, but the recovery is likely controller specific and might
> be better handled at that level. Consider the case where the faulty device is
> present but the driver is not loaded after the initialization..
>
> >
> > >
> > > > > +
> > > > > +	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> > > > > +	if (ret) {
> > > > > +		dev_err(&client->dev, "failed to read serial number\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +	dev_info(&client->dev, "serial number: %s\n", buf);
> > > > > +
> > > > > +	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> > > > > +	if (ret) {
> > > >
> > > > This is not unwound on error.   See comment on remove race below.
> > > >
> > > > > +		dev_err(&client->dev, "failed to start measurement\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> > > > > +					      sps30_trigger_handler, NULL);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	return devm_iio_device_register(&client->dev, indio_dev);
> > > > > +}
> > > > > +
> > > > > +static int sps30_remove(struct i2c_client *client)
> > > > > +{
> > > > > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > > > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > > > +
> > > > > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> > > > This looks like a race to me.
> > > >
> > >
> > > Right, this needs fixing.
> > >
> > > > You turn off the measurements before removing the interface to them.
> > > > It's certainly not in the 'obviously' right category.
> > > >
> > > > As such I would either use a devm action to do this stop,
> > > > or not use the managed interfaces for everything in probe
> > > > after the equivalent start.
> > > > The devm_add_action_or_reset would be the cleanest option I think..
> > > > Will also fix the cleanup on error in probe.
> > > >
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct i2c_device_id sps30_id[] = {
> > > > > +	{ "sps30" },
> > > > > +	{ }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(i2c, sps30_id);
> > > > > +
> > > > > +static const struct of_device_id sps30_of_match[] = {
> > > > > +	{ .compatible = "sensirion,sps30" },
> > > > > +	{ }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, sps30_of_match);
> > > > > +
> > > > > +static struct i2c_driver sps30_driver = {
> > > > > +	.driver = {
> > > > > +		.name = "sps30",
> > > > > +		.of_match_table = sps30_of_match,
> > > > > +	},
> > > > > +	.id_table = sps30_id,
> > > > > +	.probe_new = sps30_probe,
> > > > > +	.remove = sps30_remove,
> > > > > +};
> > > > > +module_i2c_driver(sps30_driver);
> > > > > +
> > > > > +MODULE_AUTHOR("Tomasz Duszynski <tduszyns@gmail.com>");
> > > > > +MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver");
> > > > > +MODULE_LICENSE("GPL v2");
> > > >