iio: chemical: sgpxx: Support Sensirion SGPxx sensors

Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors

Supported Features:

* Indoor Air Quality (IAQ) concentrations for
SGP30 and SGPC3:
- tVOC (in_concentration_voc_input)
SGP30 only:
- CO2eq (in_concentration_co2_input)
IAQ must first be initialized by writing a non-empty value to out_iaq_init. After initializing IAQ, at least one IAQ signal must be read out every second (SGP30) / every two seconds (SGPC3) for the sensor to correctly maintain its internal baseline
* Baseline support for IAQ (in_iaq_baseline, out_iaq_baseline)
* Gas concentration signals for
SGP30 and SGPC3:
- Ethanol (in_concentration_ethanol_raw)
SGP30 only:
- H2 (in_concentration_h2_raw)
* On-chip self test (in_selftest)
The self test interferes with IAQ operations. If needed, first retrieve the current baseline, then reset it after the self test
* Sensor interface version (in_feature_set_version)
* Sensor serial number (in_serial_id)
* Humidity compensation for SGP30
With the help of a humidity signal, the gas signals can be humidity-compensated.
* Checksummed I2C communication

For all features, refer to the data sheet or the documentation in Documentation/iio/chemical/sgpxx.txt for more details.

Signed-off-by Andreas Brauchli <[email protected]>

---
 Documentation/iio/chemical/sgpxx.txt | 112 +++++
 drivers/iio/chemical/Kconfig         |  13 +
 drivers/iio/chemical/Makefile        |   1 +
 drivers/iio/chemical/sgpxx.c         | 894 +++++++++++++++++++++++++++++++++++
 4 files changed, 1020 insertions(+)
 create mode 100644 Documentation/iio/chemical/sgpxx.txt
 create mode 100644 drivers/iio/chemical/sgpxx.c

Documentation/iio/chemical/sgpxx.txt new file +12 -0

new file mode 100644
index 000000000000..f49b2f365df3
@@ -0,0 +1,112 @@
+sgpxx: Industrial IO driver for Sensirion i2c Multi-Pixel Gas Sensors
+
+1. Overview
+
+The sgpxx driver supports the Sensirion SGP30 and SGPC3 multi-pixel gas sensors.
+
+Datasheets:
+https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
+https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf
+
+2. Modes of Operation
+

Documentation/iio/chemical/sgpxx.txt new file +15 -0

+compensation.
+
+2.5. On-chip self test
+
+    $ cat in_selftest
+
+in_selftest returns OK or FAILED.
+
+The self test interferes with IAQ operations. If needed, first save the current
+baseline, then restore it after the self test:
+
+    $ baseline=$(cat in_iaq_baseline)
+    $ cat in_selftest
+    $ echo init > out_iaq_init
+    $ echo -n $baseline > out_iaq_baseline

drivers/iio/chemical/Kconfig +13 -0

index 5cb5be7612b4..4574dd687513 100644
@@ -38,6 +38,19 @@ config IAQCORE
 	  iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
 	  sensors
 
+config SENSIRION_SGPXX
+	tristate "Sensirion SGPxx gas sensors"
+	depends on I2C
+	select CRC8
+	help
+	  Say Y here to build I2C interface support for the following
+	  Sensirion SGP gas sensors:
+	    * SGP30 gas sensor
+	    * SGPC3 gas sensor
+
+	  To compile this driver as module, choose M here: the
+	  module will be called sgpxx.
+
 config VZ89X
 	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
 	depends on I2C

drivers/iio/chemical/Makefile +1 -0

index a629b29d1e0b..6090a0ae3981 100644
@@ -6,4 +6,5 @@
 obj-$(CONFIG_ATLAS_PH_SENSOR)	+= atlas-ph-sensor.o
 obj-$(CONFIG_CCS811)		+= ccs811.o
 obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
+obj-$(CONFIG_SENSIRION_SGPXX)	+= sgpxx.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o

drivers/iio/chemical/sgpxx.c new file +32 -0

new file mode 100644
index 000000000000..aea55e41d4cc
@@ -0,0 +1,894 @@
+/*
+ * sgpxx.c - Support for Sensirion SGP Gas Sensors
+ *
+ * Copyright (C) 2017 Andreas Brauchli <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Datasheets:
+ * https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
+ * https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf
+ */
+
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/of_device.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/sysfs.h>
+
+#define SGP_WORD_LEN			2

drivers/iio/chemical/sgpxx.c new file +85 -0

+#define SGP30_MEASURE_INTERVAL_HZ	1
+#define SGPC3_MEASURE_INTERVAL_HZ	2
+#define SGP_SELFTEST_OK			0xd400
+
+DECLARE_CRC8_TABLE(sgp_crc8_table);
+
+enum sgp_product_id {
+	SGP30 = 0,
+	SGPC3
+};
+
+enum sgp30_channel_idx {
+	SGP30_IAQ_TVOC_IDX = 0,
+	SGP30_IAQ_CO2EQ_IDX,
+	SGP30_SIG_ETOH_IDX,
+	SGP30_SIG_H2_IDX,
+	SGP30_SET_AH_IDX,
+};
+
+enum sgpc3_channel_idx {
+	SGPC3_IAQ_TVOC_IDX = 10,
+	SGPC3_SIG_ETOH_IDX,
+};
+
+enum sgp_cmd {
+	SGP_CMD_IAQ_INIT		= SGP_CMD(0x2003),
+	SGP_CMD_IAQ_MEASURE		= SGP_CMD(0x2008),
+	SGP_CMD_GET_BASELINE		= SGP_CMD(0x2015),
+	SGP_CMD_SET_BASELINE		= SGP_CMD(0x201e),
+	SGP_CMD_GET_FEATURE_SET		= SGP_CMD(0x202f),
+	SGP_CMD_GET_SERIAL_ID		= SGP_CMD(0x3682),
+	SGP_CMD_MEASURE_TEST		= SGP_CMD(0x2032),
+
+	SGP30_CMD_MEASURE_SIGNAL	= SGP_CMD(0x2050),
+	SGP30_CMD_SET_ABSOLUTE_HUMIDITY = SGP_CMD(0x2061),
+
+	SGPC3_CMD_IAQ_INIT0		= SGP_CMD(0x2089),
+	SGPC3_CMD_IAQ_INIT16		= SGP_CMD(0x2024),
+	SGPC3_CMD_IAQ_INIT64		= SGP_CMD(0x2003),
+	SGPC3_CMD_IAQ_INIT184		= SGP_CMD(0x206a),
+	SGPC3_CMD_MEASURE_RAW		= SGP_CMD(0x2046),
+};
+
+enum sgp_measure_mode {
+	SGP_MEASURE_MODE_UNKNOWN,
+	SGP_MEASURE_MODE_IAQ,
+	SGP_MEASURE_MODE_SIGNAL,
+	SGP_MEASURE_MODE_ALL,
+};
+
+struct sgp_version {
+	u8 major;
+	u8 minor;
+};
+
+struct sgp_crc_word {
+	__be16 value;
+	u8 crc8;
+} __attribute__((__packed__));
+
+union sgp_reading {
+	u8 start;
+	struct sgp_crc_word raw_words[4];
+};
+
+struct sgp_data {
+	struct i2c_client *client;
+	struct mutex data_lock; /* mutex to lock access to data buffer */
+	struct mutex i2c_lock; /* mutex to lock access to i2c */
+	unsigned long last_update;
+
+	u64 serial_id;
+	u16 chip_id;
+	u16 feature_set;
+	u16 measurement_len;
+	int measure_interval_hz;
+	enum sgp_cmd measure_iaq_cmd;
+	enum sgp_cmd measure_signal_cmd;
+	enum sgp_measure_mode measure_mode;
+	char *baseline_format;
+	bool iaq_initialized;
+	u8 baseline_len;
+	union sgp_reading buffer;
+};
+

drivers/iio/chemical/sgpxx.c new file +5 -0

+};
+
+static const struct sgp_version supported_versions_sgpc3[] = {
+	{
+		.major = 0,

drivers/iio/chemical/sgpxx.c new file +13 -0

+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.address = SGP30_IAQ_TVOC_IDX,
+	},
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_CO2,
+		.datasheet_name = "CO2eq signal",
+		.scan_index = 1,
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.address = SGP30_IAQ_CO2EQ_IDX,
+	},

drivers/iio/chemical/sgpxx.c new file +21 -0

+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.output = 1,
+		.scan_index = 5
+	},
+};
+
+static const struct iio_chan_spec sgpc3_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_VOC,
+		.datasheet_name = "TVOC signal",
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.address = SGPC3_IAQ_TVOC_IDX,
+	},
+	{
+		.type = IIO_CONCENTRATION,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.address = SGPC3_SIG_ETOH_IDX,
+		.extend_name = "ethanol",

drivers/iio/chemical/sgpxx.c new file +86 -0

+	[SGP30] = {
+		.channels = sgp30_channels,
+		.num_channels = ARRAY_SIZE(sgp30_channels),
+	},
+	[SGPC3] = {
+		.channels = sgpc3_channels,
+		.num_channels = ARRAY_SIZE(sgpc3_channels),
+	},
+};
+
+/**
+ * sgp_verify_buffer() - verify the checksums of the data buffer words
+ *
+ * @data:       SGP data containing the raw buffer
+ * @word_count: Num data words stored in the buffer, excluding CRC bytes
+ *
+ * Return:      0 on success, negative error code otherwise
+ */
+static int sgp_verify_buffer(struct sgp_data *data, size_t word_count)
+{
+	size_t size = word_count * (SGP_WORD_LEN + SGP_CRC8_LEN);
+	int i;
+	u8 crc;
+	u8 *data_buf = &data->buffer.start;
+
+	for (i = 0; i < size; i += SGP_WORD_LEN + SGP_CRC8_LEN) {
+		crc = crc8(sgp_crc8_table, &data_buf[i], SGP_WORD_LEN,
+			   SGP_CRC8_INIT);
+		if (crc != data_buf[i + SGP_WORD_LEN]) {
+			dev_err(&data->client->dev, "CRC error\n");
+			return -EIO;
+		}
+	}
+	return 0;
+}
+
+/**
+ * sgp_read_from_cmd() - reads data from SGP sensor after issuing a command
+ * The caller must hold data->data_lock for the duration of the call.
+ * @data:        SGP data
+ * @cmd:         SGP Command to issue
+ * @word_count:  Num words to read, excluding CRC bytes
+ *
+ * Return:       0 on success, negative error otherwise.
+ */
+static int sgp_read_from_cmd(struct sgp_data *data,
+			     enum sgp_cmd cmd,
+			     size_t word_count,
+			     unsigned long duration_us)
+{
+	int ret;
+	struct i2c_client *client = data->client;
+	size_t size = word_count * (SGP_WORD_LEN + SGP_CRC8_LEN);
+	u8 *data_buf = &data->buffer.start;
+
+	mutex_lock(&data->i2c_lock);
+	ret = i2c_master_send(client, (const char *)&cmd, SGP_CMD_LEN);
+	if (ret != SGP_CMD_LEN) {
+		mutex_unlock(&data->i2c_lock);
+		return -EIO;
+	}
+	usleep_range(duration_us, duration_us + 1000);
+
+	ret = i2c_master_recv(client, data_buf, size);
+	mutex_unlock(&data->i2c_lock);
+
+	if (ret < 0)
+		ret = -ETXTBSY;
+	else if (ret != size)
+		ret = -EINTR;
+	else
+		ret = sgp_verify_buffer(data, word_count);
+
+	return ret;
+}
+
+/**
+ * sgp_i2c_write_from_cmd() - write data to SGP sensor with a command
+ * @data:       SGP data
+ * @cmd:        SGP Command to issue
+ * @buf:        Data to write
+ * @buf_size:   Data size of the buffer
+ *
+ * Return:      0 on success, negative error otherwise.
+ */
+static int sgp_write_from_cmd(struct sgp_data *data,

drivers/iio/chemical/sgpxx.c new file +426 -0

+		buf_idx += SGP_WORD_LEN;
+		buffer[buf_idx] = crc8(sgp_crc8_table,
+				       &buffer[buf_idx - SGP_WORD_LEN],
+				       SGP_WORD_LEN, SGP_CRC8_INIT);
+		buf_idx += SGP_CRC8_LEN;
+	}
+	mutex_lock(&data->i2c_lock);
+	ret = i2c_master_send(data->client, buffer, buffer_size);
+	if (ret != buffer_size) {
+		ret = -EIO;
+		goto unlock_return_count;
+	}
+	ret = 0;
+	/* Wait inside lock to ensure the chip is ready before next command */
+	usleep_range(duration_us, duration_us + 1000);
+
+unlock_return_count:
+	mutex_unlock(&data->i2c_lock);
+	return ret;
+}
+
+/**
+ * sgp_get_measurement() - retrieve measurement result from sensor
+ * The caller must hold data->data_lock for the duration of the call.
+ * @data:           SGP data
+ * @cmd:            SGP Command to issue
+ * @measure_mode:   SGP measurement mode
+ *
+ * Return:      0 on success, negative error otherwise.
+ */
+static int sgp_get_measurement(struct sgp_data *data, enum sgp_cmd cmd,
+			       enum sgp_measure_mode measure_mode)
+{
+	int ret;
+
+	/* if all channels are measured, we don't need to distinguish between
+	 * different measure modes
+	 */
+	if (data->measure_mode == SGP_MEASURE_MODE_ALL)
+		measure_mode = SGP_MEASURE_MODE_ALL;
+
+	/* Always measure if measure mode changed
+	 * SGP30 should only be polled once a second
+	 * SGPC3 should only be polled once every two seconds
+	 */
+	if (measure_mode == data->measure_mode &&
+	    !time_after(jiffies,
+			data->last_update + data->measure_interval_hz * HZ)) {
+		return 0;
+	}
+
+	ret = sgp_read_from_cmd(data, cmd, data->measurement_len,
+				SGP_CMD_DURATION_US);
+
+	if (ret < 0)
+		return ret;
+
+	data->measure_mode = measure_mode;
+	data->last_update = jiffies;
+
+	return 0;
+}
+
+static int sgp_absolute_humidity_store(struct sgp_data *data,
+				       int val, int val2)
+{
+	u32 ah;
+	u16 ah_scaled;
+
+	if (val < 0 || val > 256 || (val == 256 && val2 > 0))
+		return -EINVAL;
+
+	ah = val * 1000 + val2 / 1000;
+	/* ah_scaled = (u16)((ah / 1000.0) * 256.0) */
+	ah_scaled = (u16)(((u64)ah * 256 * 16777) >> 24);
+
+	/* ensure we don't disable AH compensation due to rounding */
+	if (ah > 0 && ah_scaled == 0)
+		ah_scaled = 1;
+
+	return sgp_write_from_cmd(data, SGP30_CMD_SET_ABSOLUTE_HUMIDITY,
+				  &ah_scaled, 1, SGP_CMD_HANDLING_DURATION_US);
+}
+
+static int sgp_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan, int *val,
+			int *val2, long mask)
+{
+	struct sgp_data *data = iio_priv(indio_dev);
+	struct sgp_crc_word *words;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		mutex_lock(&data->data_lock);
+		if (!data->iaq_initialized) {
+			dev_warn(&data->client->dev,
+				 "IAQ potentially uninitialized\n");
+		}
+		ret = sgp_get_measurement(data, data->measure_iaq_cmd,
+					  SGP_MEASURE_MODE_IAQ);
+		if (ret)
+			goto unlock_fail;
+		words = data->buffer.raw_words;
+		switch (chan->address) {
+		case SGP30_IAQ_TVOC_IDX:
+		case SGPC3_IAQ_TVOC_IDX:
+			*val = 0;
+			*val2 = be16_to_cpu(words[1].value);
+			ret = IIO_VAL_INT_PLUS_NANO;
+			break;
+		case SGP30_IAQ_CO2EQ_IDX:
+			*val = 0;
+			*val2 = be16_to_cpu(words[0].value);
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		mutex_unlock(&data->data_lock);
+		break;
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&data->data_lock);
+		ret = sgp_get_measurement(data, data->measure_signal_cmd,
+					  SGP_MEASURE_MODE_SIGNAL);
+		if (ret)
+			goto unlock_fail;
+		words = data->buffer.raw_words;
+		switch (chan->address) {
+		case SGP30_SIG_ETOH_IDX:
+			*val = be16_to_cpu(words[1].value);
+			ret = IIO_VAL_INT;
+			break;
+		case SGPC3_SIG_ETOH_IDX:
+		case SGP30_SIG_H2_IDX:
+			*val = be16_to_cpu(words[0].value);
+			ret = IIO_VAL_INT;
+			break;
+		}
+unlock_fail:
+		mutex_unlock(&data->data_lock);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+		case SGP30_SIG_ETOH_IDX:
+		case SGPC3_SIG_ETOH_IDX:
+		case SGP30_SIG_H2_IDX:
+			*val = 0;
+			*val2 = 1953125;
+			ret = IIO_VAL_INT_PLUS_NANO;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	return ret;
+}
+
+static int sgp_write_raw(struct iio_dev *indio_dev,
+			 struct iio_chan_spec const *chan,
+			 int val, int val2, long mask)
+{
+	struct sgp_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (chan->address) {
+	case SGP30_SET_AH_IDX:
+		ret = sgp_absolute_humidity_store(data, val, val2);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static ssize_t sgp_iaq_init_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+	u32 init_time;
+	enum sgp_cmd cmd;
+	int ret;
+
+	cmd = SGP_CMD_IAQ_INIT;
+	if (data->chip_id == SGPC3) {
+		ret = kstrtou32(buf, 10, &init_time);
+
+		if (ret)
+			return -EINVAL;
+
+		switch (init_time) {
+		case 0:
+			cmd = SGPC3_CMD_IAQ_INIT0;
+			break;
+		case 16:
+			cmd = SGPC3_CMD_IAQ_INIT16;
+			break;
+		case 64:
+			cmd = SGPC3_CMD_IAQ_INIT64;
+			break;
+		case 184:
+			cmd = SGPC3_CMD_IAQ_INIT184;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	mutex_lock(&data->data_lock);
+	ret = sgp_read_from_cmd(data, cmd, 0, SGP_CMD_DURATION_US);
+
+	if (ret < 0)
+		goto unlock_fail;
+
+	data->iaq_initialized = true;
+	mutex_unlock(&data->data_lock);
+	return count;
+
+unlock_fail:
+	mutex_unlock(&data->data_lock);
+	return ret;
+}
+
+static ssize_t sgp_iaq_baseline_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+	u32 baseline;
+	u16 baseline_word;
+	int ret, ix;
+
+	mutex_lock(&data->data_lock);
+	ret = sgp_read_from_cmd(data, SGP_CMD_GET_BASELINE, data->baseline_len,
+				SGP_CMD_DURATION_US);
+
+	if (ret < 0)
+		goto unlock_fail;
+
+	baseline = 0;
+	for (ix = 0; ix < data->baseline_len; ix++) {
+		baseline_word = be16_to_cpu(data->buffer.raw_words[ix].value);
+		baseline |= baseline_word << (16 * ix);
+	}
+
+	mutex_unlock(&data->data_lock);
+	return sprintf(buf, data->baseline_format, baseline);
+
+unlock_fail:
+	mutex_unlock(&data->data_lock);
+	return ret;
+}
+
+static ssize_t sgp_iaq_baseline_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+	int newline = (count > 0 && buf[count - 1] == '\n');
+	u16 words[2];
+	int ret = 0;
+
+	/* 1 word (4 chars) per signal */
+	if (count - newline == (data->baseline_len * 4)) {
+		if (data->baseline_len == 1)
+			ret = sscanf(buf, "%04hx", &words[0]);
+		else if (data->baseline_len == 2)
+			ret = sscanf(buf, "%04hx%04hx", &words[0], &words[1]);
+		else
+			return -EIO;
+	}
+
+	/* Check if baseline format is correct */
+	if (ret != data->baseline_len) {
+		dev_err(&data->client->dev, "invalid baseline format\n");
+		return -EIO;
+	}
+
+	ret = sgp_write_from_cmd(data, SGP_CMD_SET_BASELINE, words,
+				 data->baseline_len, SGP_CMD_DURATION_US);
+	if (ret < 0)
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t sgp_selftest_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+	u16 measure_test;
+	int ret;
+
+	mutex_lock(&data->data_lock);
+	data->iaq_initialized = false;
+	ret = sgp_read_from_cmd(data, SGP_CMD_MEASURE_TEST, 1,
+				SGP_SELFTEST_DURATION_US);
+
+	if (ret != 0)
+		goto unlock_fail;
+
+	measure_test = be16_to_cpu(data->buffer.raw_words[0].value);
+	mutex_unlock(&data->data_lock);
+
+	return sprintf(buf, "%s\n",
+		       measure_test ^ SGP_SELFTEST_OK ? "FAILED" : "OK");
+
+unlock_fail:
+	mutex_unlock(&data->data_lock);
+	return ret;
+}
+
+static ssize_t sgp_serial_id_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%llu\n", data->serial_id);
+}
+
+static ssize_t sgp_feature_set_version_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%hu.%hu\n", (data->feature_set & 0x00e0) >> 5,
+		       data->feature_set & 0x001f);
+}
+
+static int sgp_get_serial_id(struct sgp_data *data)
+{
+	int ret;
+	struct sgp_crc_word *words;
+
+	mutex_lock(&data->data_lock);
+	ret = sgp_read_from_cmd(data, SGP_CMD_GET_SERIAL_ID, 3,
+				SGP_CMD_DURATION_US);
+	if (ret != 0)
+		goto unlock_fail;
+
+	words = data->buffer.raw_words;
+	data->serial_id = (u64)(be16_to_cpu(words[2].value) & 0xffff)       |
+			  (u64)(be16_to_cpu(words[1].value) & 0xffff) << 16 |
+			  (u64)(be16_to_cpu(words[0].value) & 0xffff) << 32;
+
+unlock_fail:
+	mutex_unlock(&data->data_lock);
+	return ret;
+}
+
+static int setup_and_check_sgp_data(struct sgp_data *data,
+				    unsigned int chip_id)
+{
+	u16 minor, major, product, eng, ix, num_fs, reserved;
+	struct sgp_version *supported_versions;
+
+	product = (data->feature_set & 0xf000) >> 12;
+	reserved = (data->feature_set & 0x0e00) >> 9;
+	eng = (data->feature_set & 0x0100) >> 8;
+	major = (data->feature_set & 0x00e0) >> 5;
+	minor = data->feature_set & 0x001f;
+
+	/* driver does not match product */
+	if (product != chip_id) {
+		dev_err(&data->client->dev,
+			"sensor reports a different product: 0x%04hx\n",
+			product);
+		return -ENODEV;
+	}
+
+	if (reserved != 0)
+		dev_warn(&data->client->dev, "reserved bits set: 0x%04hx\n",
+			 reserved);
+
+	/* engineering samples are not supported */
+	if (eng != 0)
+		return -ENODEV;
+
+	data->iaq_initialized = false;
+	switch (product) {
+	case SGP30:
+		supported_versions =
+			(struct sgp_version *)supported_versions_sgp30;
+		num_fs = ARRAY_SIZE(supported_versions_sgp30);
+		data->measurement_len = SGP30_MEASUREMENT_LEN;
+		data->measure_interval_hz = SGP30_MEASURE_INTERVAL_HZ;
+		data->measure_iaq_cmd = SGP_CMD_IAQ_MEASURE;
+		data->measure_signal_cmd = SGP30_CMD_MEASURE_SIGNAL;
+		data->chip_id = SGP30;
+		data->baseline_len = 2;
+		data->baseline_format = "%08x\n";
+		data->measure_mode = SGP_MEASURE_MODE_UNKNOWN;
+		break;
+	case SGPC3:
+		supported_versions =
+			(struct sgp_version *)supported_versions_sgpc3;
+		num_fs = ARRAY_SIZE(supported_versions_sgpc3);
+		data->measurement_len = SGPC3_MEASUREMENT_LEN;
+		data->measure_interval_hz = SGPC3_MEASURE_INTERVAL_HZ;
+		data->measure_iaq_cmd = SGPC3_CMD_MEASURE_RAW;
+		data->measure_signal_cmd = SGPC3_CMD_MEASURE_RAW;
+		data->chip_id = SGPC3;
+		data->baseline_len = 1;
+		data->baseline_format = "%04x\n";
+		data->measure_mode = SGP_MEASURE_MODE_ALL;
+		break;
+	default:
+		return -ENODEV;
+	};
+
+	for (ix = 0; ix < num_fs; ix++) {
+		if (supported_versions[ix].major == major &&
+		    minor >= supported_versions[ix].minor)
+			return 0;
+	}
+
+	dev_err(&data->client->dev, "unsupported sgp version: %d.%d\n",
+		major, minor);

drivers/iio/chemical/sgpxx.c new file +48 -0

+static struct attribute *sgp_attributes[] = {
+	&iio_dev_attr_in_serial_id.dev_attr.attr,
+	&iio_dev_attr_in_feature_set_version.dev_attr.attr,
+	&iio_dev_attr_in_selftest.dev_attr.attr,
+	&iio_dev_attr_out_iaq_init.dev_attr.attr,
+	&iio_dev_attr_in_iaq_baseline.dev_attr.attr,
+	&iio_dev_attr_out_iaq_baseline.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group sgp_attr_group = {
+	.attrs = sgp_attributes,
+};
+
+static const struct iio_info sgp_info = {
+	.attrs		= &sgp_attr_group,
+	.read_raw	= sgp_read_raw,
+	.write_raw	= sgp_write_raw,
+};
+
+static const struct of_device_id sgp_dt_ids[] = {
+	{ .compatible = "sensirion,sgp30", .data = (void *)SGP30 },
+	{ .compatible = "sensirion,sgpc3", .data = (void *)SGPC3 },
+	{ }
+};
+
+static int sgp_probe(struct i2c_client *client,
+		     const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct sgp_data *data;
+	struct sgp_device *chip;
+	const struct of_device_id *of_id;
+	unsigned long chip_id;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	of_id = of_match_device(sgp_dt_ids, &client->dev);
+	if (!of_id)
+		chip_id = id->driver_data;
+	else
+		chip_id = (unsigned long)of_id->data;
+
+	chip = &sgp_devices[chip_id];
+	data = iio_priv(indio_dev);

drivers/iio/chemical/sgpxx.c new file +11 -0

+		return ret;
+
+	/* get feature set version and write it to client data */
+	ret = sgp_read_from_cmd(data, SGP_CMD_GET_FEATURE_SET, 1,
+				SGP_CMD_DURATION_US);
+	if (ret != 0)
+		return ret;
+
+	data->feature_set = be16_to_cpu(data->buffer.raw_words[0].value);
+
+	ret = setup_and_check_sgp_data(data, chip_id);

drivers/iio/chemical/sgpxx.c new file +28 -0

+	return 0;
+}
+
+static const struct i2c_device_id sgp_id[] = {
+	{ "sgp30", SGP30 },
+	{ "sgpc3", SGPC3 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, sgp_id);
+MODULE_DEVICE_TABLE(of, sgp_dt_ids);
+
+static struct i2c_driver sgp_driver = {
+	.driver = {
+		.name	= "sgpxx",
+		.of_match_table = of_match_ptr(sgp_dt_ids),
+	},
+	.probe = sgp_probe,
+	.remove = sgp_remove,
+	.id_table = sgp_id,
+};
+module_i2c_driver(sgp_driver);
+
+MODULE_AUTHOR("Andreas Brauchli <[email protected]>");
+MODULE_AUTHOR("Pascal Sachs <[email protected]>");
+MODULE_DESCRIPTION("Sensirion SGPxx gas sensors");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("0.5.0");

Hello,

some quick comments on this driver below

I think documentation is missing and the ABI is a bit problematic and unusual

Quoted 1 line

Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors

generally, we tend to avoid wildcard driver names; sgp30 would be preferred over sgpxx

Quoted 29 lines

Supported Features:

* Indoor Air Quality (IAQ) concentrations for
SGP30 and SGPC3:
- tVOC (in_concentration_voc_input)
SGP30 only:
- CO2eq (in_concentration_co2_input)
IAQ must first be initialized by writing a non-empty value to out_iaq_init. After initializing IAQ, at least one IAQ signal must be read out every second (SGP30) / every two seconds (SGPC3) for the sensor to correctly maintain its internal baseline
* Baseline support for IAQ (in_iaq_baseline, out_iaq_baseline)
* Gas concentration signals for
SGP30 and SGPC3:
- Ethanol (in_concentration_ethanol_raw)
SGP30 only:
- H2 (in_concentration_h2_raw)
* On-chip self test (in_selftest)
The self test interferes with IAQ operations. If needed, first retrieve the current baseline, then reset it after the self test
* Sensor interface version (in_feature_set_version)
* Sensor serial number (in_serial_id)
* Humidity compensation for SGP30
With the help of a humidity signal, the gas signals can be humidity-compensated.
* Checksummed I2C communication

Quoted 2 lines

For all features, refer to the data sheet or the documentation in Documentation/iio/chemical/sgpxx.txt for more details.

may some brief TODOs; heat controller?

Quoted 10 lines

Signed-off-by Andreas Brauchli <[email protected]>

---
 Documentation/iio/chemical/sgpxx.txt | 112 +++++
 drivers/iio/chemical/Kconfig         |  13 +
 drivers/iio/chemical/Makefile        |   1 +
 drivers/iio/chemical/sgpxx.c         | 894 +++++++++++++++++++++++++++++++++++
 4 files changed, 1020 insertions(+)
 create mode 100644 Documentation/iio/chemical/sgpxx.txt
 create mode 100644 drivers/iio/chemical/sgpxx.c

Quoted 6 lines

Documentation/iio/chemical/sgpxx.txt new file +0 -0

new file mode 100644
index 000000000000..f49b2f365df3

Documentation/iio/chemical/sgpxx.txt line 9

Quoted 9 lines

Documentation/iio/chemical/sgpxx.txt new file +9 -0

@@ -0,0 +1,112 @@
+sgpxx: Industrial IO driver for Sensirion i2c Multi-Pixel Gas Sensors
+
+1. Overview
+
+The sgpxx driver supports the Sensirion SGP30 and SGPC3 multi-pixel gas sensors.
+
+Datasheets:
+https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
+https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf

Quoted 8 lines

Documentation/iio/chemical/sgpxx.txt new file +8 -0

+
+2. Modes of Operation
+
+2.1. Driver Instantiation
+
+The sgpxx driver must be instantiated on the corresponding i2c bus with the
+product name (sgp30 or sgpc3) and i2c address (0x58).
+

Documentation/iio/chemical/sgpxx.txt line 22

Quoted 5 lines

Documentation/iio/chemical/sgpxx.txt new file +5 -0

+Example instantiation of an sgp30 on i2c bus 1 (i2c-1):
+
+    $ echo sgp30 0x58 | sudo tee /sys/bus/i2c/devices/i2c-1/new_device
+
+Using the wrong product name results in an instantiation error. Check dmesg.  

I'd rather drop this section, the only specific information is the I2C address

Documentation/iio/chemical/sgpxx.txt line 33

Quoted 12 lines

Documentation/iio/chemical/sgpxx.txt new file +11 -0

+
+2.2. Indoor Air Quality (IAQ) concentrations
+
+* tVOC (in_concentration_voc_input) at ppb precision (1e-9)
+* CO2eq (in_concentration_co2_input) at ppm precision (1e-6) -- SGP30 only
+
+2.2.1. IAQ Initialization
+Before Indoor Air Quality (IAQ) values can be read, the IAQ mode must be
+initialized by writing a non-empty value to out_iaq_init:
+
+    $ echo init > out_iaq_init  

can't this be done on probe()?

in any case, private API should be documented under Documentation/ABI/testing/sysfs-bus-iio-*

Documentation/iio/chemical/sgpxx.txt line 37

Quoted 3 lines

Documentation/iio/chemical/sgpxx.txt new file +3 -0

+After initializing IAQ, at least one IAQ signal must be read out every second
+(SGP30) / every two seconds (SGPC3) for the sensor to correctly maintain its
+internal baseline:  

shouldn't the driver do this?

Documentation/iio/chemical/sgpxx.txt line 46

Quoted 10 lines

Documentation/iio/chemical/sgpxx.txt new file +9 -0

+
+    SGP30:
+    $ watch -n1 cat in_concentration_voc_input
+
+    SGPC3:
+    $ watch -n2 cat in_concentration_voc_input
+
+For the first 15s of operation after writing to out_iaq_init, default values are
+retured by the sensor.  

typo: returned

Documentation/iio/chemical/sgpxx.txt line 53

Quoted 7 lines

Documentation/iio/chemical/sgpxx.txt new file +7 -0

+
+2.2.2. Pausing and Resuming IAQ
+
+For best performance and faster startup times, the baseline should be saved
+once every hour, after 12h of operation. The baseline is restored by writing a
+non-empty value to out_iaq_init, followed by writing an unmodified retrieved
+baseline value from in_iaq_baseline to out_iaq_baseline.  

the out_ prefix seems inappropriate here, the sensors doesn't output CO2 :)

handling calibration data in a generic way is difficult

Documentation/iio/chemical/sgpxx.txt line 64

Quoted 12 lines

Documentation/iio/chemical/sgpxx.txt new file +11 -0

+
+    Saving the baseline:
+    $ baseline=$(cat in_iaq_baseline)
+
+    Restoring the baseline:
+    $ echo init > out_iaq_init
+    $ echo -n $baseline > out_iaq_baseline
+
+2.3. Gas Concentration Signals
+
+* Ethanol (in_concentration_ethanol_raw)  

we'd need a IIO_MOD_ETHANOL?

Documentation/iio/chemical/sgpxx.txt line 65

Quoted 2 lines

Documentation/iio/chemical/sgpxx.txt new file +1 -0

+* H2 (in_concentration_h2_raw) -- SGP30 only  

we'd need a IIO_MOD_H2?

Documentation/iio/chemical/sgpxx.txt line 76

Quoted 11 lines

Documentation/iio/chemical/sgpxx.txt new file +11 -0

+
+The gas signals in_concentration_ethanol_raw and in_concentration_h2_raw may be
+used without prior write to out_iaq_init.
+
+2.4. Humidity Compensation (SGP30)
+
+The SGP30 features an on-chip humidity compensation that requires the
+(in-device) environment's absolute humidity.
+
+Set the absolute humidity by writing the absolute humidity concentration (in
+mg/m^3) to out_concentration_ah_raw. The absolute humidity is obtained by  

not sure about the out_ prefix again

absolute humidity is new, IIO has relative humidity so far (jus saying) relative humidity is in milli percent in IIO
temperature is in milli degree Celsius

Documentation/iio/chemical/sgpxx.txt line 87

Quoted 11 lines

Documentation/iio/chemical/sgpxx.txt new file +11 -0

+converting the relative humidity and temperature. The following units are used:
+AH in mg/m^3, RH in percent (0..100), T in degrees Celsius, and exp() being the
+base-e exponential function.
+
+                  RH            exp(17.62 * T)
+                ----- * 6.112 * --------------
+                100.0            243.12 + T
+  AH = 216.7 * ------------------------------- * 1000
+                        273.15 + T
+
+Writing a value of 0 to out_absolute_humidity disables the humidity  

out_absolute_humidity is the same as out_concentration_ah_raw?

Quoted 10 lines

Documentation/iio/chemical/sgpxx.txt new file +10 -0

+compensation.
+
+2.5. On-chip self test
+
+    $ cat in_selftest
+
+in_selftest returns OK or FAILED.
+
+The self test interferes with IAQ operations. If needed, first save the current
+baseline, then restore it after the self test:

Documentation/iio/chemical/sgpxx.txt line 105

Quoted 8 lines

Documentation/iio/chemical/sgpxx.txt new file +8 -0

+
+    $ baseline=$(cat in_iaq_baseline)
+    $ cat in_selftest
+    $ echo init > out_iaq_init
+    $ echo -n $baseline > out_iaq_baseline
+
+If the sensor's current operating duration is less than 12h the baseline should
+not be restored by skipping the last step.

Documentation/iio/chemical/sgpxx.txt line 112

Quoted 7 lines

Documentation/iio/chemical/sgpxx.txt new file +7 -0

+
+3. Sensor Interface
+
+    $ cat in_feature_set_version
+
+The SGP sensors' minor interface (feature set) version guarantees interface
+stability: a sensor with feature set 1.1 works with a driver for feature set 1.0  

really needed?

Quoted 73 lines

drivers/iio/chemical/Kconfig +13 -0

index 5cb5be7612b4..4574dd687513 100644
@@ -38,6 +38,19 @@ config IAQCORE
 	  iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
 	  sensors
 
+config SENSIRION_SGPXX
+	tristate "Sensirion SGPxx gas sensors"
+	depends on I2C
+	select CRC8
+	help
+	  Say Y here to build I2C interface support for the following
+	  Sensirion SGP gas sensors:
+	    * SGP30 gas sensor
+	    * SGPC3 gas sensor
+
+	  To compile this driver as module, choose M here: the
+	  module will be called sgpxx.
+
 config VZ89X
 	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
 	depends on I2C

drivers/iio/chemical/Makefile +1 -0

index a629b29d1e0b..6090a0ae3981 100644
@@ -6,4 +6,5 @@
 obj-$(CONFIG_ATLAS_PH_SENSOR)	+= atlas-ph-sensor.o
 obj-$(CONFIG_CCS811)		+= ccs811.o
 obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
+obj-$(CONFIG_SENSIRION_SGPXX)	+= sgpxx.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o

drivers/iio/chemical/sgpxx.c new file +32 -0

new file mode 100644
index 000000000000..aea55e41d4cc
@@ -0,0 +1,894 @@
+/*
+ * sgpxx.c - Support for Sensirion SGP Gas Sensors
+ *
+ * Copyright (C) 2017 Andreas Brauchli <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Datasheets:
+ * https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
+ * https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf
+ */
+
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/of_device.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/sysfs.h>
+
+#define SGP_WORD_LEN			2

drivers/iio/chemical/sgpxx.c line 42

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+#define SGP_CRC8_POLYNOMIAL		0x31
+#define SGP_CRC8_INIT			0xff
+#define SGP_CRC8_LEN			1
+#define SGP_CMD(cmd_word)		cpu_to_be16(cmd_word)
+#define SGP_CMD_DURATION_US		50000
+#define SGP_SELFTEST_DURATION_US	220000
+#define SGP_CMD_HANDLING_DURATION_US	10000
+#define SGP_CMD_LEN			SGP_WORD_LEN
+#define SGP30_MEASUREMENT_LEN		2
+#define SGPC3_MEASUREMENT_LEN		2  

both _MEASUREMENT_LEN needed, same value?

Quoted 61 lines

drivers/iio/chemical/sgpxx.c new file +61 -0

+#define SGP30_MEASURE_INTERVAL_HZ	1
+#define SGPC3_MEASURE_INTERVAL_HZ	2
+#define SGP_SELFTEST_OK			0xd400
+
+DECLARE_CRC8_TABLE(sgp_crc8_table);
+
+enum sgp_product_id {
+	SGP30 = 0,
+	SGPC3
+};
+
+enum sgp30_channel_idx {
+	SGP30_IAQ_TVOC_IDX = 0,
+	SGP30_IAQ_CO2EQ_IDX,
+	SGP30_SIG_ETOH_IDX,
+	SGP30_SIG_H2_IDX,
+	SGP30_SET_AH_IDX,
+};
+
+enum sgpc3_channel_idx {
+	SGPC3_IAQ_TVOC_IDX = 10,
+	SGPC3_SIG_ETOH_IDX,
+};
+
+enum sgp_cmd {
+	SGP_CMD_IAQ_INIT		= SGP_CMD(0x2003),
+	SGP_CMD_IAQ_MEASURE		= SGP_CMD(0x2008),
+	SGP_CMD_GET_BASELINE		= SGP_CMD(0x2015),
+	SGP_CMD_SET_BASELINE		= SGP_CMD(0x201e),
+	SGP_CMD_GET_FEATURE_SET		= SGP_CMD(0x202f),
+	SGP_CMD_GET_SERIAL_ID		= SGP_CMD(0x3682),
+	SGP_CMD_MEASURE_TEST		= SGP_CMD(0x2032),
+
+	SGP30_CMD_MEASURE_SIGNAL	= SGP_CMD(0x2050),
+	SGP30_CMD_SET_ABSOLUTE_HUMIDITY = SGP_CMD(0x2061),
+
+	SGPC3_CMD_IAQ_INIT0		= SGP_CMD(0x2089),
+	SGPC3_CMD_IAQ_INIT16		= SGP_CMD(0x2024),
+	SGPC3_CMD_IAQ_INIT64		= SGP_CMD(0x2003),
+	SGPC3_CMD_IAQ_INIT184		= SGP_CMD(0x206a),
+	SGPC3_CMD_MEASURE_RAW		= SGP_CMD(0x2046),
+};
+
+enum sgp_measure_mode {
+	SGP_MEASURE_MODE_UNKNOWN,
+	SGP_MEASURE_MODE_IAQ,
+	SGP_MEASURE_MODE_SIGNAL,
+	SGP_MEASURE_MODE_ALL,
+};
+
+struct sgp_version {
+	u8 major;
+	u8 minor;
+};
+
+struct sgp_crc_word {
+	__be16 value;
+	u8 crc8;
+} __attribute__((__packed__));
+
+union sgp_reading {

drivers/iio/chemical/sgpxx.c line 111

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+	u8 start;
+	struct sgp_crc_word raw_words[4];
+};
+
+struct sgp_data {
+	struct i2c_client *client;
+	struct mutex data_lock; /* mutex to lock access to data buffer */
+	struct mutex i2c_lock; /* mutex to lock access to i2c */

Quoted 16 lines

drivers/iio/chemical/sgpxx.c new file +16 -0

+	unsigned long last_update;
+
+	u64 serial_id;
+	u16 chip_id;
+	u16 feature_set;
+	u16 measurement_len;
+	int measure_interval_hz;
+	enum sgp_cmd measure_iaq_cmd;
+	enum sgp_cmd measure_signal_cmd;
+	enum sgp_measure_mode measure_mode;
+	char *baseline_format;
+	bool iaq_initialized;
+	u8 baseline_len;
+	union sgp_reading buffer;
+};
+

drivers/iio/chemical/sgpxx.c line 137

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+struct sgp_device {
+	const struct iio_chan_spec *channels;
+	int num_channels;
+};
+
+static const struct sgp_version supported_versions_sgp30[] = {
+	{
+		.major = 1,
+		.minor = 0,
+	}  

end with comma (,) so that it can be extended with minimal change

Quoted 5 lines

drivers/iio/chemical/sgpxx.c new file +5 -0

+};
+
+static const struct sgp_version supported_versions_sgpc3[] = {
+	{
+		.major = 0,

drivers/iio/chemical/sgpxx.c line 152

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+		.minor = 4,
+	}
+};
+
+static const struct iio_chan_spec sgp30_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_VOC,
+		.datasheet_name = "TVOC signal",
+		.scan_index = 0,  

scan_index only needed when adding buffer support

Quoted 13 lines

drivers/iio/chemical/sgpxx.c new file +13 -0

+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.address = SGP30_IAQ_TVOC_IDX,
+	},
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_CO2,
+		.datasheet_name = "CO2eq signal",
+		.scan_index = 1,
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.address = SGP30_IAQ_CO2EQ_IDX,
+	},

drivers/iio/chemical/sgpxx.c line 175

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+	{
+		.type = IIO_CONCENTRATION,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.address = SGP30_SIG_ETOH_IDX,
+		.extend_name = "ethanol",
+		.datasheet_name = "Ethanol signal",
+		.scan_index = 2,
+		.scan_type = {
+			.endianness = IIO_BE,  

scan_type only neededwhen adding buffer support

maybe add IIO_MOD_ETHANOL?

drivers/iio/chemical/sgpxx.c line 182

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +7 -0

+		},
+	},
+	{
+		.type = IIO_CONCENTRATION,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.address = SGP30_SIG_H2_IDX,  

maybe add IIO_MOD_H2?

drivers/iio/chemical/sgpxx.c line 195

Quoted 13 lines

drivers/iio/chemical/sgpxx.c new file +13 -0

+		.extend_name = "h2",
+		.datasheet_name = "H2 signal",
+		.scan_index = 3,
+		.scan_type = {
+			.endianness = IIO_BE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+	{
+		.type = IIO_CONCENTRATION,
+		.address = SGP30_SET_AH_IDX,
+		.extend_name = "ah",
+		.datasheet_name = "absolute humidty",  

typo: humidity

Quoted 13 lines

drivers/iio/chemical/sgpxx.c new file +13 -0

+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.output = 1,
+		.scan_index = 5
+	},
+};
+
+static const struct iio_chan_spec sgpc3_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_VOC,
+		.datasheet_name = "TVOC signal",
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),

drivers/iio/chemical/sgpxx.c line 216

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+		.address = SGPC3_IAQ_TVOC_IDX,
+	},
+	{
+		.type = IIO_CONCENTRATION,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.address = SGPC3_SIG_ETOH_IDX,
+		.extend_name = "ethanol",

drivers/iio/chemical/sgpxx.c line 226

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+		.datasheet_name = "Ethanol signal",
+		.scan_index = 0,
+		.scan_type = {
+			.endianness = IIO_BE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+static struct sgp_device sgp_devices[] = {  

const?

Quoted 48 lines

drivers/iio/chemical/sgpxx.c new file +48 -0

+	[SGP30] = {
+		.channels = sgp30_channels,
+		.num_channels = ARRAY_SIZE(sgp30_channels),
+	},
+	[SGPC3] = {
+		.channels = sgpc3_channels,
+		.num_channels = ARRAY_SIZE(sgpc3_channels),
+	},
+};
+
+/**
+ * sgp_verify_buffer() - verify the checksums of the data buffer words
+ *
+ * @data:       SGP data containing the raw buffer
+ * @word_count: Num data words stored in the buffer, excluding CRC bytes
+ *
+ * Return:      0 on success, negative error code otherwise
+ */
+static int sgp_verify_buffer(struct sgp_data *data, size_t word_count)
+{
+	size_t size = word_count * (SGP_WORD_LEN + SGP_CRC8_LEN);
+	int i;
+	u8 crc;
+	u8 *data_buf = &data->buffer.start;
+
+	for (i = 0; i < size; i += SGP_WORD_LEN + SGP_CRC8_LEN) {
+		crc = crc8(sgp_crc8_table, &data_buf[i], SGP_WORD_LEN,
+			   SGP_CRC8_INIT);
+		if (crc != data_buf[i + SGP_WORD_LEN]) {
+			dev_err(&data->client->dev, "CRC error\n");
+			return -EIO;
+		}
+	}
+	return 0;
+}
+
+/**
+ * sgp_read_from_cmd() - reads data from SGP sensor after issuing a command
+ * The caller must hold data->data_lock for the duration of the call.
+ * @data:        SGP data
+ * @cmd:         SGP Command to issue
+ * @word_count:  Num words to read, excluding CRC bytes
+ *
+ * Return:       0 on success, negative error otherwise.
+ */
+static int sgp_read_from_cmd(struct sgp_data *data,
+			     enum sgp_cmd cmd,
+			     size_t word_count,

drivers/iio/chemical/sgpxx.c line 282

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+			     unsigned long duration_us)
+{
+	int ret;
+	struct i2c_client *client = data->client;
+	size_t size = word_count * (SGP_WORD_LEN + SGP_CRC8_LEN);
+	u8 *data_buf = &data->buffer.start;
+
+	mutex_lock(&data->i2c_lock);

Quoted 4 lines

drivers/iio/chemical/sgpxx.c new file +4 -0

+	ret = i2c_master_send(client, (const char *)&cmd, SGP_CMD_LEN);
+	if (ret != SGP_CMD_LEN) {
+		mutex_unlock(&data->i2c_lock);
+		return -EIO;

drivers/iio/chemical/sgpxx.c line 294

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+	}
+	usleep_range(duration_us, duration_us + 1000);
+
+	ret = i2c_master_recv(client, data_buf, size);
+	mutex_unlock(&data->i2c_lock);
+
+	if (ret < 0)
+		ret = -ETXTBSY;

drivers/iio/chemical/sgpxx.c line 296

Quoted 2 lines

drivers/iio/chemical/sgpxx.c new file +2 -0

+	else if (ret != size)
+		ret = -EINTR;

drivers/iio/chemical/sgpxx.c line 300

Quoted 4 lines

drivers/iio/chemical/sgpxx.c new file +4 -0

+	else
+		ret = sgp_verify_buffer(data, word_count);
+
+	return ret;

Quoted 16 lines

drivers/iio/chemical/sgpxx.c new file +16 -0

+}
+
+/**
+ * sgp_i2c_write_from_cmd() - write data to SGP sensor with a command
+ * @data:       SGP data
+ * @cmd:        SGP Command to issue
+ * @buf:        Data to write
+ * @buf_size:   Data size of the buffer
+ *
+ * Return:      0 on success, negative error otherwise.
+ */
+static int sgp_write_from_cmd(struct sgp_data *data,
+			      enum sgp_cmd cmd,
+			      u16 *buf,
+			      size_t buf_size,
+			      unsigned long duration_us)

drivers/iio/chemical/sgpxx.c line 322

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+{
+	int ret, ix;
+	u16 buf_idx = 0;
+	u16 buffer_size = SGP_CMD_LEN + buf_size *
+		(SGP_WORD_LEN + SGP_CRC8_LEN);
+	u8 buffer[buffer_size];  

dynamically sized array, allowed?

drivers/iio/chemical/sgpxx.c line 325

Quoted 4 lines

drivers/iio/chemical/sgpxx.c new file +3 -0

+
+	/* assemble buffer */
+	*((u16 *)&buffer[0]) = cmd;  

be16

drivers/iio/chemical/sgpxx.c line 328

Quoted 3 lines

drivers/iio/chemical/sgpxx.c new file +3 -0

+	buf_idx += SGP_CMD_LEN;
+	for (ix = 0; ix < buf_size; ix++) {
+		*((u16 *)&buffer[buf_idx]) = ntohs(buf[ix] & 0xffff);

use cpu_to_be16() as everywhere else instead of ntohs()

buf is u16, so & 0xffff needed?

Quoted 33 lines

drivers/iio/chemical/sgpxx.c new file +33 -0

+		buf_idx += SGP_WORD_LEN;
+		buffer[buf_idx] = crc8(sgp_crc8_table,
+				       &buffer[buf_idx - SGP_WORD_LEN],
+				       SGP_WORD_LEN, SGP_CRC8_INIT);
+		buf_idx += SGP_CRC8_LEN;
+	}
+	mutex_lock(&data->i2c_lock);
+	ret = i2c_master_send(data->client, buffer, buffer_size);
+	if (ret != buffer_size) {
+		ret = -EIO;
+		goto unlock_return_count;
+	}
+	ret = 0;
+	/* Wait inside lock to ensure the chip is ready before next command */
+	usleep_range(duration_us, duration_us + 1000);
+
+unlock_return_count:
+	mutex_unlock(&data->i2c_lock);
+	return ret;
+}
+
+/**
+ * sgp_get_measurement() - retrieve measurement result from sensor
+ * The caller must hold data->data_lock for the duration of the call.
+ * @data:           SGP data
+ * @cmd:            SGP Command to issue
+ * @measure_mode:   SGP measurement mode
+ *
+ * Return:      0 on success, negative error otherwise.
+ */
+static int sgp_get_measurement(struct sgp_data *data, enum sgp_cmd cmd,
+			       enum sgp_measure_mode measure_mode)
+{

drivers/iio/chemical/sgpxx.c line 369

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+	int ret;
+
+	/* if all channels are measured, we don't need to distinguish between
+	 * different measure modes
+	 */
+	if (data->measure_mode == SGP_MEASURE_MODE_ALL)
+		measure_mode = SGP_MEASURE_MODE_ALL;
+

Quoted 92 lines

drivers/iio/chemical/sgpxx.c new file +92 -0

+	/* Always measure if measure mode changed
+	 * SGP30 should only be polled once a second
+	 * SGPC3 should only be polled once every two seconds
+	 */
+	if (measure_mode == data->measure_mode &&
+	    !time_after(jiffies,
+			data->last_update + data->measure_interval_hz * HZ)) {
+		return 0;
+	}
+
+	ret = sgp_read_from_cmd(data, cmd, data->measurement_len,
+				SGP_CMD_DURATION_US);
+
+	if (ret < 0)
+		return ret;
+
+	data->measure_mode = measure_mode;
+	data->last_update = jiffies;
+
+	return 0;
+}
+
+static int sgp_absolute_humidity_store(struct sgp_data *data,
+				       int val, int val2)
+{
+	u32 ah;
+	u16 ah_scaled;
+
+	if (val < 0 || val > 256 || (val == 256 && val2 > 0))
+		return -EINVAL;
+
+	ah = val * 1000 + val2 / 1000;
+	/* ah_scaled = (u16)((ah / 1000.0) * 256.0) */
+	ah_scaled = (u16)(((u64)ah * 256 * 16777) >> 24);
+
+	/* ensure we don't disable AH compensation due to rounding */
+	if (ah > 0 && ah_scaled == 0)
+		ah_scaled = 1;
+
+	return sgp_write_from_cmd(data, SGP30_CMD_SET_ABSOLUTE_HUMIDITY,
+				  &ah_scaled, 1, SGP_CMD_HANDLING_DURATION_US);
+}
+
+static int sgp_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan, int *val,
+			int *val2, long mask)
+{
+	struct sgp_data *data = iio_priv(indio_dev);
+	struct sgp_crc_word *words;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		mutex_lock(&data->data_lock);
+		if (!data->iaq_initialized) {
+			dev_warn(&data->client->dev,
+				 "IAQ potentially uninitialized\n");
+		}
+		ret = sgp_get_measurement(data, data->measure_iaq_cmd,
+					  SGP_MEASURE_MODE_IAQ);
+		if (ret)
+			goto unlock_fail;
+		words = data->buffer.raw_words;
+		switch (chan->address) {
+		case SGP30_IAQ_TVOC_IDX:
+		case SGPC3_IAQ_TVOC_IDX:
+			*val = 0;
+			*val2 = be16_to_cpu(words[1].value);
+			ret = IIO_VAL_INT_PLUS_NANO;
+			break;
+		case SGP30_IAQ_CO2EQ_IDX:
+			*val = 0;
+			*val2 = be16_to_cpu(words[0].value);
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		mutex_unlock(&data->data_lock);
+		break;
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&data->data_lock);
+		ret = sgp_get_measurement(data, data->measure_signal_cmd,
+					  SGP_MEASURE_MODE_SIGNAL);
+		if (ret)
+			goto unlock_fail;
+		words = data->buffer.raw_words;
+		switch (chan->address) {
+		case SGP30_SIG_ETOH_IDX:
+			*val = be16_to_cpu(words[1].value);
+			ret = IIO_VAL_INT;

drivers/iio/chemical/sgpxx.c line 469

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+			break;
+		case SGPC3_SIG_ETOH_IDX:
+		case SGP30_SIG_H2_IDX:
+			*val = be16_to_cpu(words[0].value);
+			ret = IIO_VAL_INT;
+			break;
+		}
+unlock_fail:

Quoted 24 lines

drivers/iio/chemical/sgpxx.c new file +24 -0

+		mutex_unlock(&data->data_lock);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+		case SGP30_SIG_ETOH_IDX:
+		case SGPC3_SIG_ETOH_IDX:
+		case SGP30_SIG_H2_IDX:
+			*val = 0;
+			*val2 = 1953125;
+			ret = IIO_VAL_INT_PLUS_NANO;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	return ret;
+}
+
+static int sgp_write_raw(struct iio_dev *indio_dev,
+			 struct iio_chan_spec const *chan,

drivers/iio/chemical/sgpxx.c line 501

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+			 int val, int val2, long mask)
+{
+	struct sgp_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (chan->address) {
+	case SGP30_SET_AH_IDX:
+		ret = sgp_absolute_humidity_store(data, val, val2);

drivers/iio/chemical/sgpxx.c line 446

Quoted 3 lines

drivers/iio/chemical/sgpxx.c new file +3 -0

+		break;
+	default:
+		ret = -EINVAL;

drivers/iio/chemical/sgpxx.c line 300

Quoted 3 lines

drivers/iio/chemical/sgpxx.c new file +3 -0

+	}
+
+	return ret;

drivers/iio/chemical/sgpxx.c line 513

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+}
+
+static ssize_t sgp_iaq_init_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{

Quoted 33 lines

drivers/iio/chemical/sgpxx.c new file +33 -0

+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+	u32 init_time;
+	enum sgp_cmd cmd;
+	int ret;
+
+	cmd = SGP_CMD_IAQ_INIT;
+	if (data->chip_id == SGPC3) {
+		ret = kstrtou32(buf, 10, &init_time);
+
+		if (ret)
+			return -EINVAL;
+
+		switch (init_time) {
+		case 0:
+			cmd = SGPC3_CMD_IAQ_INIT0;
+			break;
+		case 16:
+			cmd = SGPC3_CMD_IAQ_INIT16;
+			break;
+		case 64:
+			cmd = SGPC3_CMD_IAQ_INIT64;
+			break;
+		case 184:
+			cmd = SGPC3_CMD_IAQ_INIT184;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	mutex_lock(&data->data_lock);
+	ret = sgp_read_from_cmd(data, cmd, 0, SGP_CMD_DURATION_US);
+

drivers/iio/chemical/sgpxx.c line 554

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+	if (ret < 0)
+		goto unlock_fail;
+
+	data->iaq_initialized = true;
+	mutex_unlock(&data->data_lock);
+	return count;
+
+unlock_fail:

Quoted 22 lines

drivers/iio/chemical/sgpxx.c new file +22 -0

+	mutex_unlock(&data->data_lock);
+	return ret;
+}
+
+static ssize_t sgp_iaq_baseline_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+	u32 baseline;
+	u16 baseline_word;
+	int ret, ix;
+
+	mutex_lock(&data->data_lock);
+	ret = sgp_read_from_cmd(data, SGP_CMD_GET_BASELINE, data->baseline_len,
+				SGP_CMD_DURATION_US);
+
+	if (ret < 0)
+		goto unlock_fail;
+
+	baseline = 0;
+	for (ix = 0; ix < data->baseline_len; ix++) {

drivers/iio/chemical/sgpxx.c line 584

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+		baseline_word = be16_to_cpu(data->buffer.raw_words[ix].value);
+		baseline |= baseline_word << (16 * ix);
+	}
+
+	mutex_unlock(&data->data_lock);
+	return sprintf(buf, data->baseline_format, baseline);
+
+unlock_fail:

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+	mutex_unlock(&data->data_lock);
+	return ret;
+}
+
+static ssize_t sgp_iaq_baseline_store(struct device *dev,
+				      struct device_attribute *attr,

drivers/iio/chemical/sgpxx.c line 598

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+				      const char *buf, size_t count)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+	int newline = (count > 0 && buf[count - 1] == '\n');
+	u16 words[2];
+	int ret = 0;
+
+	/* 1 word (4 chars) per signal */

From reading (very briefly) the datasheet I gather this about

Quoted 36 lines

drivers/iio/chemical/sgpxx.c new file +36 -0

+	if (count - newline == (data->baseline_len * 4)) {
+		if (data->baseline_len == 1)
+			ret = sscanf(buf, "%04hx", &words[0]);
+		else if (data->baseline_len == 2)
+			ret = sscanf(buf, "%04hx%04hx", &words[0], &words[1]);
+		else
+			return -EIO;
+	}
+
+	/* Check if baseline format is correct */
+	if (ret != data->baseline_len) {
+		dev_err(&data->client->dev, "invalid baseline format\n");
+		return -EIO;
+	}
+
+	ret = sgp_write_from_cmd(data, SGP_CMD_SET_BASELINE, words,
+				 data->baseline_len, SGP_CMD_DURATION_US);
+	if (ret < 0)
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t sgp_selftest_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+	u16 measure_test;
+	int ret;
+
+	mutex_lock(&data->data_lock);
+	data->iaq_initialized = false;
+	ret = sgp_read_from_cmd(data, SGP_CMD_MEASURE_TEST, 1,
+				SGP_SELFTEST_DURATION_US);
+
+	if (ret != 0)

drivers/iio/chemical/sgpxx.c line 642

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+		goto unlock_fail;
+
+	measure_test = be16_to_cpu(data->buffer.raw_words[0].value);
+	mutex_unlock(&data->data_lock);
+
+	return sprintf(buf, "%s\n",
+		       measure_test ^ SGP_SELFTEST_OK ? "FAILED" : "OK");
+

drivers/iio/chemical/sgpxx.c line 653

Quoted 11 lines

drivers/iio/chemical/sgpxx.c new file +11 -0

+unlock_fail:
+	mutex_unlock(&data->data_lock);
+	return ret;
+}
+
+static ssize_t sgp_serial_id_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%llu\n", data->serial_id);

Quoted 32 lines

drivers/iio/chemical/sgpxx.c new file +32 -0

+}
+
+static ssize_t sgp_feature_set_version_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct sgp_data *data = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%hu.%hu\n", (data->feature_set & 0x00e0) >> 5,
+		       data->feature_set & 0x001f);
+}
+
+static int sgp_get_serial_id(struct sgp_data *data)
+{
+	int ret;
+	struct sgp_crc_word *words;
+
+	mutex_lock(&data->data_lock);
+	ret = sgp_read_from_cmd(data, SGP_CMD_GET_SERIAL_ID, 3,
+				SGP_CMD_DURATION_US);
+	if (ret != 0)
+		goto unlock_fail;
+
+	words = data->buffer.raw_words;
+	data->serial_id = (u64)(be16_to_cpu(words[2].value) & 0xffff)       |
+			  (u64)(be16_to_cpu(words[1].value) & 0xffff) << 16 |
+			  (u64)(be16_to_cpu(words[0].value) & 0xffff) << 32;
+
+unlock_fail:
+	mutex_unlock(&data->data_lock);
+	return ret;
+}

drivers/iio/chemical/sgpxx.c line 693

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+
+static int setup_and_check_sgp_data(struct sgp_data *data,
+				    unsigned int chip_id)
+{
+	u16 minor, major, product, eng, ix, num_fs, reserved;
+	struct sgp_version *supported_versions;
+
+	product = (data->feature_set & 0xf000) >> 12;

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+	reserved = (data->feature_set & 0x0e00) >> 9;
+	eng = (data->feature_set & 0x0100) >> 8;
+	major = (data->feature_set & 0x00e0) >> 5;
+	minor = data->feature_set & 0x001f;
+
+	/* driver does not match product */
+	if (product != chip_id) {
+		dev_err(&data->client->dev,
+			"sensor reports a different product: 0x%04hx\n",
+			product);

drivers/iio/chemical/sgpxx.c line 711

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+		return -ENODEV;
+	}
+
+	if (reserved != 0)
+		dev_warn(&data->client->dev, "reserved bits set: 0x%04hx\n",
+			 reserved);
+
+	/* engineering samples are not supported */

drivers/iio/chemical/sgpxx.c line 721

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+	if (eng != 0)
+		return -ENODEV;
+
+	data->iaq_initialized = false;
+	switch (product) {
+	case SGP30:
+		supported_versions =
+			(struct sgp_version *)supported_versions_sgp30;
+		num_fs = ARRAY_SIZE(supported_versions_sgp30);
+		data->measurement_len = SGP30_MEASUREMENT_LEN;

Quoted 33 lines

drivers/iio/chemical/sgpxx.c new file +33 -0

+		data->measure_interval_hz = SGP30_MEASURE_INTERVAL_HZ;
+		data->measure_iaq_cmd = SGP_CMD_IAQ_MEASURE;
+		data->measure_signal_cmd = SGP30_CMD_MEASURE_SIGNAL;
+		data->chip_id = SGP30;
+		data->baseline_len = 2;
+		data->baseline_format = "%08x\n";
+		data->measure_mode = SGP_MEASURE_MODE_UNKNOWN;
+		break;
+	case SGPC3:
+		supported_versions =
+			(struct sgp_version *)supported_versions_sgpc3;
+		num_fs = ARRAY_SIZE(supported_versions_sgpc3);
+		data->measurement_len = SGPC3_MEASUREMENT_LEN;
+		data->measure_interval_hz = SGPC3_MEASURE_INTERVAL_HZ;
+		data->measure_iaq_cmd = SGPC3_CMD_MEASURE_RAW;
+		data->measure_signal_cmd = SGPC3_CMD_MEASURE_RAW;
+		data->chip_id = SGPC3;
+		data->baseline_len = 1;
+		data->baseline_format = "%04x\n";
+		data->measure_mode = SGP_MEASURE_MODE_ALL;
+		break;
+	default:
+		return -ENODEV;
+	};
+
+	for (ix = 0; ix < num_fs; ix++) {
+		if (supported_versions[ix].major == major &&
+		    minor >= supported_versions[ix].minor)
+			return 0;
+	}
+
+	dev_err(&data->client->dev, "unsupported sgp version: %d.%d\n",
+		major, minor);

drivers/iio/chemical/sgpxx.c line 764

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+	return -ENODEV;
+}
+
+static IIO_DEVICE_ATTR(in_serial_id, 0444, sgp_serial_id_show, NULL, 0);
+static IIO_DEVICE_ATTR(in_feature_set_version, 0444,
+		       sgp_feature_set_version_show, NULL, 0);
+static IIO_DEVICE_ATTR(in_selftest, 0444, sgp_selftest_show, NULL, 0);
+static IIO_DEVICE_ATTR(out_iaq_init, 0220, NULL, sgp_iaq_init_store, 0);
+static IIO_DEVICE_ATTR(in_iaq_baseline, 0444, sgp_iaq_baseline_show, NULL, 0);
+static IIO_DEVICE_ATTR(out_iaq_baseline, 0220, NULL, sgp_iaq_baseline_store, 0);  

lot of private ABI; all needed?
needs documentation...

Quoted 24 lines

drivers/iio/chemical/sgpxx.c new file +24 -0

+static struct attribute *sgp_attributes[] = {
+	&iio_dev_attr_in_serial_id.dev_attr.attr,
+	&iio_dev_attr_in_feature_set_version.dev_attr.attr,
+	&iio_dev_attr_in_selftest.dev_attr.attr,
+	&iio_dev_attr_out_iaq_init.dev_attr.attr,
+	&iio_dev_attr_in_iaq_baseline.dev_attr.attr,
+	&iio_dev_attr_out_iaq_baseline.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group sgp_attr_group = {
+	.attrs = sgp_attributes,
+};
+
+static const struct iio_info sgp_info = {
+	.attrs		= &sgp_attr_group,
+	.read_raw	= sgp_read_raw,
+	.write_raw	= sgp_write_raw,
+};
+
+static const struct of_device_id sgp_dt_ids[] = {
+	{ .compatible = "sensirion,sgp30", .data = (void *)SGP30 },
+	{ .compatible = "sensirion,sgpc3", .data = (void *)SGPC3 },
+	{ }

drivers/iio/chemical/sgpxx.c line 797

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+};
+
+static int sgp_probe(struct i2c_client *client,
+		     const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct sgp_data *data;
+	struct sgp_device *chip;

drivers/iio/chemical/sgpxx.c line 807

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+	const struct of_device_id *of_id;
+	unsigned long chip_id;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	of_id = of_match_device(sgp_dt_ids, &client->dev);
+	if (!of_id)

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+		chip_id = id->driver_data;
+	else
+		chip_id = (unsigned long)of_id->data;
+
+	chip = &sgp_devices[chip_id];
+	data = iio_priv(indio_dev);

drivers/iio/chemical/sgpxx.c line 821

Quoted 8 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	crc8_populate_msb(sgp_crc8_table, SGP_CRC8_POLYNOMIAL);
+	mutex_init(&data->data_lock);
+	mutex_init(&data->i2c_lock);
+
+	/* get serial id and write it to client data */
+	ret = sgp_get_serial_id(data);

drivers/iio/chemical/sgpxx.c line 822

Quoted 2 lines

drivers/iio/chemical/sgpxx.c new file +2 -0

+
+	if (ret != 0)  

matter of taste: most drivers just do
if (ret)

Quoted 12 lines

drivers/iio/chemical/sgpxx.c new file +11 -0

+		return ret;
+
+	/* get feature set version and write it to client data */
+	ret = sgp_read_from_cmd(data, SGP_CMD_GET_FEATURE_SET, 1,
+				SGP_CMD_DURATION_US);
+	if (ret != 0)
+		return ret;
+
+	data->feature_set = be16_to_cpu(data->buffer.raw_words[0].value);
+
+	ret = setup_and_check_sgp_data(data, chip_id);

drivers/iio/chemical/sgpxx.c line 844

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+	if (ret < 0)
+		goto fail_free;
+
+	/* so initial reading will complete */
+	data->last_update = jiffies - data->measure_interval_hz * HZ;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &sgp_info;
+	indio_dev->name = dev_name(&client->dev);
+	indio_dev->modes = INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE;  

is INDIO_BUFFER_SOFTWARE implemented at this stage? maybe added in followup patch

drivers/iio/chemical/sgpxx.c line 850

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+
+	indio_dev->channels = chip->channels;
+	indio_dev->num_channels = chip->num_channels;
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (!ret)

drivers/iio/chemical/sgpxx.c line 853

Quoted 3 lines

drivers/iio/chemical/sgpxx.c new file +3 -0

+		return ret;
+
+	dev_err(&client->dev, "failed to register iio device\n");  

really message needed?

drivers/iio/chemical/sgpxx.c line 857

Quoted 4 lines

drivers/iio/chemical/sgpxx.c new file +4 -0

+
+fail_free:
+	mutex_destroy(&data->i2c_lock);
+	mutex_destroy(&data->data_lock);  

no need to explicitly destroy mutex

drivers/iio/chemical/sgpxx.c line 858

Quoted 2 lines

drivers/iio/chemical/sgpxx.c new file +1 -0

+	iio_device_free(indio_dev);  

no need to explicitly free devm_ stuff

drivers/iio/chemical/sgpxx.c line 866

Quoted 9 lines

drivers/iio/chemical/sgpxx.c new file +8 -0

+	return ret;
+}
+
+static int sgp_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	devm_iio_device_unregister(&client->dev, indio_dev);  

not needed

Quoted 30 lines

drivers/iio/chemical/sgpxx.c new file +28 -0

+	return 0;
+}
+
+static const struct i2c_device_id sgp_id[] = {
+	{ "sgp30", SGP30 },
+	{ "sgpc3", SGPC3 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, sgp_id);
+MODULE_DEVICE_TABLE(of, sgp_dt_ids);
+
+static struct i2c_driver sgp_driver = {
+	.driver = {
+		.name	= "sgpxx",
+		.of_match_table = of_match_ptr(sgp_dt_ids),
+	},
+	.probe = sgp_probe,
+	.remove = sgp_remove,
+	.id_table = sgp_id,
+};
+module_i2c_driver(sgp_driver);
+
+MODULE_AUTHOR("Andreas Brauchli <[email protected]>");
+MODULE_AUTHOR("Pascal Sachs <[email protected]>");
+MODULE_DESCRIPTION("Sensirion SGPxx gas sensors");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("0.5.0");

Quoted 3 lines · On Tue, 21 Nov 2017 22:46:07 +0100 (CET) Peter Meerwald-Stadler…

Hello,

some quick comments on this driver below

A few additional bits from me but I think Peter got most of the key stuff.

Quoted 3 lines

I think documentation is missing and the ABI is a bit problematic and unusual

Quoted 5 lines

Quoted 2 lines

Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors

generally, we tend to avoid wildcard driver names; sgp30 would be preferred over sgpxx

Quoted 30 lines

Quoted 27 lines

Supported Features:

* Indoor Air Quality (IAQ) concentrations for
SGP30 and SGPC3:
- tVOC (in_concentration_voc_input)
SGP30 only:
- CO2eq (in_concentration_co2_input)
IAQ must first be initialized by writing a non-empty value to out_iaq_init. After initializing IAQ, at least one IAQ signal must be read out every second (SGP30) / every two seconds (SGPC3) for the sensor to correctly maintain its internal baseline
* Baseline support for IAQ (in_iaq_baseline, out_iaq_baseline)
* Gas concentration signals for
SGP30 and SGPC3:
- Ethanol (in_concentration_ethanol_raw)
SGP30 only:
- H2 (in_concentration_h2_raw)
* On-chip self test (in_selftest)
The self test interferes with IAQ operations. If needed, first retrieve the current baseline, then reset it after the self test
* Sensor interface version (in_feature_set_version)
* Sensor serial number (in_serial_id)
* Humidity compensation for SGP30
With the help of a humidity signal, the gas signals can be humidity-compensated.
* Checksummed I2C communication

Quoted 4 lines

Quoted 2 lines

For all features, refer to the data sheet or the documentation in Documentation/iio/chemical/sgpxx.txt for more details.

may some brief TODOs; heat controller?

Quoted 20 lines

Quoted 20 lines

Signed-off-by Andreas Brauchli <[email protected]>

---
 Documentation/iio/chemical/sgpxx.txt | 112 +++++
 drivers/iio/chemical/Kconfig         |  13 +
 drivers/iio/chemical/Makefile        |   1 +
 drivers/iio/chemical/sgpxx.c         | 894 +++++++++++++++++++++++++++++++++++
 4 files changed, 1020 insertions(+)
 create mode 100644 Documentation/iio/chemical/sgpxx.txt
 create mode 100644 drivers/iio/chemical/sgpxx.c

Documentation/iio/chemical/sgpxx.txt new file +3 -0

new file mode 100644
index 000000000000..f49b2f365df3
@@ -0,0 +1,112 @@
+sgpxx: Industrial IO driver for Sensirion i2c Multi-Pixel Gas Sensors
+
+1. Overview

Quoted 6 lines

Documentation/iio/chemical/sgpxx.txt line 9

Quoted 6 lines

Documentation/iio/chemical/sgpxx.txt new file +6 -0

+
+The sgpxx driver supports the Sensirion SGP30 and SGPC3 multi-pixel gas sensors.
+
+Datasheets:
+https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
+https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf

Put this stuff in the driver itself.

Quoted 8 lines

Quoted 8 lines

Documentation/iio/chemical/sgpxx.txt new file +8 -0

+
+2. Modes of Operation
+
+2.1. Driver Instantiation
+
+The sgpxx driver must be instantiated on the corresponding i2c bus with the
+product name (sgp30 or sgpc3) and i2c address (0x58).
+

Quoted 8 lines

Documentation/iio/chemical/sgpxx.txt line 22

Quoted 5 lines

Documentation/iio/chemical/sgpxx.txt new file +5 -0

+Example instantiation of an sgp30 on i2c bus 1 (i2c-1):
+
+    $ echo sgp30 0x58 | sudo tee /sys/bus/i2c/devices/i2c-1/new_device
+
+Using the wrong product name results in an instantiation error. Check dmesg.  

I'd rather drop this section, the only specific information is the I2C address

Which belongs in the device tree bindings not here.

Quoted 14 lines

Documentation/iio/chemical/sgpxx.txt line 33

Quoted 12 lines

Documentation/iio/chemical/sgpxx.txt new file +11 -0

+
+2.2. Indoor Air Quality (IAQ) concentrations
+
+* tVOC (in_concentration_voc_input) at ppb precision (1e-9)
+* CO2eq (in_concentration_co2_input) at ppm precision (1e-6) -- SGP30 only
+
+2.2.1. IAQ Initialization
+Before Indoor Air Quality (IAQ) values can be read, the IAQ mode must be
+initialized by writing a non-empty value to out_iaq_init:
+
+    $ echo init > out_iaq_init  

can't this be done on probe()?

It very much needs to be - userspace code for this sort of sensor should be generic and hence not need to know about how to initialize your particular sensor. It will assume if the driver is there, the device is on - or will be dynamically enabled when it wants to talk to it.

Quoted 3 lines

in any case, private API should be documented under Documentation/ABI/testing/sysfs-bus-iio-*

Quoted 6 lines

Documentation/iio/chemical/sgpxx.txt line 37

Quoted 4 lines

Documentation/iio/chemical/sgpxx.txt new file +3 -0

+After initializing IAQ, at least one IAQ signal must be read out every second
+(SGP30) / every two seconds (SGPC3) for the sensor to correctly maintain its
+internal baseline:  

shouldn't the driver do this?

Absolutely!

As I understand it, the requirement is also to prevent the hardware being read more often than this so it needs to be under control of the driver.

Quoted 10 lines

Documentation/iio/chemical/sgpxx.txt line 46

Quoted 10 lines

Documentation/iio/chemical/sgpxx.txt new file +9 -0

+
+    SGP30:
+    $ watch -n1 cat in_concentration_voc_input
+
+    SGPC3:
+    $ watch -n2 cat in_concentration_voc_input
+
+For the first 15s of operation after writing to out_iaq_init, default values are
+retured by the sensor.  

In which case it should return -EBUSY from the read function and not garbage data (userspace in general has no way of knowing these chip specific things).

Quoted 1 line

typo: returned

Quoted 9 lines

Documentation/iio/chemical/sgpxx.txt line 53

Quoted 7 lines

Documentation/iio/chemical/sgpxx.txt new file +7 -0

+
+2.2.2. Pausing and Resuming IAQ
+
+For best performance and faster startup times, the baseline should be saved
+once every hour, after 12h of operation. The baseline is restored by writing a
+non-empty value to out_iaq_init, followed by writing an unmodified retrieved
+baseline value from in_iaq_baseline to out_iaq_baseline.  

the out_ prefix seems inappropriate here, the sensors doesn't output CO2 :)

Quoted 2 lines

handling calibration data in a generic way is difficult

We can do better than this though. Init is automatic - if you are updating the baseline and it needs to call it again, then do that in the write of the baseline not a separate init write.

Quoted 5 lines

Quoted 5 lines

Documentation/iio/chemical/sgpxx.txt new file +5 -0

+
+    Saving the baseline:
+    $ baseline=$(cat in_iaq_baseline)
+
+    Restoring the baseline:

Quoted 8 lines

Documentation/iio/chemical/sgpxx.txt line 64

Quoted 6 lines

Documentation/iio/chemical/sgpxx.txt new file +6 -0

+    $ echo init > out_iaq_init
+    $ echo -n $baseline > out_iaq_baseline
+
+2.3. Gas Concentration Signals
+
+* Ethanol (in_concentration_ethanol_raw)  

we'd need a IIO_MOD_ETHANOL?

Cool a new sensor type ;)

Quoted 4 lines

Documentation/iio/chemical/sgpxx.txt line 65

Quoted 2 lines

Documentation/iio/chemical/sgpxx.txt new file +1 -0

+* H2 (in_concentration_h2_raw) -- SGP30 only  

we'd need a IIO_MOD_H2?

Quoted 14 lines

Documentation/iio/chemical/sgpxx.txt line 76

Quoted 12 lines

Documentation/iio/chemical/sgpxx.txt new file +11 -0

+
+The gas signals in_concentration_ethanol_raw and in_concentration_h2_raw may be
+used without prior write to out_iaq_init.
+
+2.4. Humidity Compensation (SGP30)
+
+The SGP30 features an on-chip humidity compensation that requires the
+(in-device) environment's absolute humidity.
+
+Set the absolute humidity by writing the absolute humidity concentration (in
+mg/m^3) to out_concentration_ah_raw. The absolute humidity is obtained by  

not sure about the out_ prefix again

Not unless you are making the air around the sensor wet.

These are really calibration values being input to the device.
The ABI needs to reflect that rather than putting them through as output channels.

Quoted 2 lines

absolute humidity is new, IIO has relative humidity so far (jus saying)

Quoted 16 lines

relative humidity is in milli percent in IIO
temperature is in milli degree Celsius

Documentation/iio/chemical/sgpxx.txt line 87

Quoted 11 lines

Documentation/iio/chemical/sgpxx.txt new file +11 -0

+converting the relative humidity and temperature. The following units are used:
+AH in mg/m^3, RH in percent (0..100), T in degrees Celsius, and exp() being the
+base-e exponential function.
+
+                  RH            exp(17.62 * T)
+                ----- * 6.112 * --------------
+                100.0            243.12 + T
+  AH = 216.7 * ------------------------------- * 1000
+                        273.15 + T
+
+Writing a value of 0 to out_absolute_humidity disables the humidity  

out_absolute_humidity is the same as out_concentration_ah_raw?

Quoted 14 lines

Quoted 14 lines

Documentation/iio/chemical/sgpxx.txt new file +13 -0

+compensation.
+
+2.5. On-chip self test
+
+    $ cat in_selftest
+
+in_selftest returns OK or FAILED.
+
+The self test interferes with IAQ operations. If needed, first save the current
+baseline, then restore it after the self test:
+
+    $ baseline=$(cat in_iaq_baseline)
+    $ cat in_selftest

Quoted 5 lines

Documentation/iio/chemical/sgpxx.txt line 105

Quoted 5 lines

Documentation/iio/chemical/sgpxx.txt new file +5 -0

+    $ echo init > out_iaq_init
+    $ echo -n $baseline > out_iaq_baseline
+
+If the sensor's current operating duration is less than 12h the baseline should
+not be restored by skipping the last step.

Talk me through the usecase - is power up often enough or are there reasons to do it more often?

Quoted 9 lines

Documentation/iio/chemical/sgpxx.txt line 112

Quoted 7 lines

Documentation/iio/chemical/sgpxx.txt new file +7 -0

+
+3. Sensor Interface
+
+    $ cat in_feature_set_version
+
+The SGP sensors' minor interface (feature set) version guarantees interface
+stability: a sensor with feature set 1.1 works with a driver for feature set 1.0  

really needed?

Shouldn't be exposed to userspace. Worth checking in kernel perhaps and refusing to probe if we don't support the newer hardware.

Quoted 78 lines

Quoted 78 lines

drivers/iio/chemical/Kconfig +13 -0

index 5cb5be7612b4..4574dd687513 100644
@@ -38,6 +38,19 @@ config IAQCORE
 	  iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
 	  sensors
 
+config SENSIRION_SGPXX
+	tristate "Sensirion SGPxx gas sensors"
+	depends on I2C
+	select CRC8
+	help
+	  Say Y here to build I2C interface support for the following
+	  Sensirion SGP gas sensors:
+	    * SGP30 gas sensor
+	    * SGPC3 gas sensor
+
+	  To compile this driver as module, choose M here: the
+	  module will be called sgpxx.
+
 config VZ89X
 	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
 	depends on I2C

drivers/iio/chemical/Makefile +1 -0

index a629b29d1e0b..6090a0ae3981 100644
@@ -6,4 +6,5 @@
 obj-$(CONFIG_ATLAS_PH_SENSOR)	+= atlas-ph-sensor.o
 obj-$(CONFIG_CCS811)		+= ccs811.o
 obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
+obj-$(CONFIG_SENSIRION_SGPXX)	+= sgpxx.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o

drivers/iio/chemical/sgpxx.c new file +36 -0

new file mode 100644
index 000000000000..aea55e41d4cc
@@ -0,0 +1,894 @@
+/*
+ * sgpxx.c - Support for Sensirion SGP Gas Sensors
+ *
+ * Copyright (C) 2017 Andreas Brauchli <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Datasheets:
+ * https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
+ * https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf
+ */
+
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/of_device.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/sysfs.h>
+
+#define SGP_WORD_LEN			2
+#define SGP_CRC8_POLYNOMIAL		0x31
+#define SGP_CRC8_INIT			0xff
+#define SGP_CRC8_LEN			1
+#define SGP_CMD(cmd_word)		cpu_to_be16(cmd_word)

Quoted 8 lines

drivers/iio/chemical/sgpxx.c line 42

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+#define SGP_CMD_DURATION_US		50000
+#define SGP_SELFTEST_DURATION_US	220000
+#define SGP_CMD_HANDLING_DURATION_US	10000
+#define SGP_CMD_LEN			SGP_WORD_LEN
+#define SGP30_MEASUREMENT_LEN		2
+#define SGPC3_MEASUREMENT_LEN		2  

both _MEASUREMENT_LEN needed, same value?

Quoted 66 lines

Quoted 66 lines

drivers/iio/chemical/sgpxx.c new file +65 -0

+#define SGP30_MEASURE_INTERVAL_HZ	1
+#define SGPC3_MEASURE_INTERVAL_HZ	2
+#define SGP_SELFTEST_OK			0xd400
+
+DECLARE_CRC8_TABLE(sgp_crc8_table);
+
+enum sgp_product_id {
+	SGP30 = 0,
+	SGPC3
+};
+
+enum sgp30_channel_idx {
+	SGP30_IAQ_TVOC_IDX = 0,
+	SGP30_IAQ_CO2EQ_IDX,
+	SGP30_SIG_ETOH_IDX,
+	SGP30_SIG_H2_IDX,
+	SGP30_SET_AH_IDX,
+};
+
+enum sgpc3_channel_idx {
+	SGPC3_IAQ_TVOC_IDX = 10,
+	SGPC3_SIG_ETOH_IDX,
+};
+
+enum sgp_cmd {
+	SGP_CMD_IAQ_INIT		= SGP_CMD(0x2003),
+	SGP_CMD_IAQ_MEASURE		= SGP_CMD(0x2008),
+	SGP_CMD_GET_BASELINE		= SGP_CMD(0x2015),
+	SGP_CMD_SET_BASELINE		= SGP_CMD(0x201e),
+	SGP_CMD_GET_FEATURE_SET		= SGP_CMD(0x202f),
+	SGP_CMD_GET_SERIAL_ID		= SGP_CMD(0x3682),
+	SGP_CMD_MEASURE_TEST		= SGP_CMD(0x2032),
+
+	SGP30_CMD_MEASURE_SIGNAL	= SGP_CMD(0x2050),
+	SGP30_CMD_SET_ABSOLUTE_HUMIDITY = SGP_CMD(0x2061),
+
+	SGPC3_CMD_IAQ_INIT0		= SGP_CMD(0x2089),
+	SGPC3_CMD_IAQ_INIT16		= SGP_CMD(0x2024),
+	SGPC3_CMD_IAQ_INIT64		= SGP_CMD(0x2003),
+	SGPC3_CMD_IAQ_INIT184		= SGP_CMD(0x206a),
+	SGPC3_CMD_MEASURE_RAW		= SGP_CMD(0x2046),
+};
+
+enum sgp_measure_mode {
+	SGP_MEASURE_MODE_UNKNOWN,
+	SGP_MEASURE_MODE_IAQ,
+	SGP_MEASURE_MODE_SIGNAL,
+	SGP_MEASURE_MODE_ALL,
+};
+
+struct sgp_version {
+	u8 major;
+	u8 minor;
+};
+
+struct sgp_crc_word {
+	__be16 value;
+	u8 crc8;
+} __attribute__((__packed__));
+
+union sgp_reading {
+	u8 start;
+	struct sgp_crc_word raw_words[4];
+};
+

Quoted 4 lines

drivers/iio/chemical/sgpxx.c line 111

Quoted 4 lines

drivers/iio/chemical/sgpxx.c new file +4 -0

+struct sgp_data {
+	struct i2c_client *client;
+	struct mutex data_lock; /* mutex to lock access to data buffer */
+	struct mutex i2c_lock; /* mutex to lock access to i2c */

Given the i2c lock is only ever (other than during probe which doesn't matter) held when data_lock is also held, it provides not addition protection so drop it.

Quoted 21 lines

Quoted 21 lines

drivers/iio/chemical/sgpxx.c new file +20 -0

+	unsigned long last_update;
+
+	u64 serial_id;
+	u16 chip_id;
+	u16 feature_set;
+	u16 measurement_len;
+	int measure_interval_hz;
+	enum sgp_cmd measure_iaq_cmd;
+	enum sgp_cmd measure_signal_cmd;
+	enum sgp_measure_mode measure_mode;
+	char *baseline_format;
+	bool iaq_initialized;
+	u8 baseline_len;
+	union sgp_reading buffer;
+};
+
+struct sgp_device {
+	const struct iio_chan_spec *channels;
+	int num_channels;
+};

Quoted 8 lines

drivers/iio/chemical/sgpxx.c line 137

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+
+static const struct sgp_version supported_versions_sgp30[] = {
+	{
+		.major = 1,
+		.minor = 0,
+	}  

end with comma (,) so that it can be extended with minimal change

Quoted 10 lines

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +9 -0

+};
+
+static const struct sgp_version supported_versions_sgpc3[] = {
+	{
+		.major = 0,
+		.minor = 4,
+	}
+};
+

Quoted 8 lines

drivers/iio/chemical/sgpxx.c line 152

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+static const struct iio_chan_spec sgp30_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_VOC,
+		.datasheet_name = "TVOC signal",
+		.scan_index = 0,  

scan_index only needed when adding buffer support

Quoted 27 lines

Quoted 14 lines

drivers/iio/chemical/sgpxx.c new file +13 -0

+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.address = SGP30_IAQ_TVOC_IDX,
+	},
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_CO2,
+		.datasheet_name = "CO2eq signal",
+		.scan_index = 1,
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.address = SGP30_IAQ_CO2EQ_IDX,
+	},

drivers/iio/chemical/sgpxx.c line 175

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+	{
+		.type = IIO_CONCENTRATION,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.address = SGP30_SIG_ETOH_IDX,
+		.extend_name = "ethanol",
+		.datasheet_name = "Ethanol signal",
+		.scan_index = 2,
+		.scan_type = {
+			.endianness = IIO_BE,  

scan_type only neededwhen adding buffer support

Quoted 1 line

maybe add IIO_MOD_ETHANOL?

Add a new modifier, not an extended_name

Quoted 11 lines

drivers/iio/chemical/sgpxx.c line 182

Quoted 9 lines

drivers/iio/chemical/sgpxx.c new file +7 -0

+		},
+	},
+	{
+		.type = IIO_CONCENTRATION,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.address = SGP30_SIG_H2_IDX,  

maybe add IIO_MOD_H2?

Quoted 16 lines

Quoted 4 lines

drivers/iio/chemical/sgpxx.c new file +3 -0

+		.extend_name = "h2",
+		.datasheet_name = "H2 signal",
+		.scan_index = 3,

drivers/iio/chemical/sgpxx.c line 195

Quoted 10 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+		.scan_type = {
+			.endianness = IIO_BE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+	{
+		.type = IIO_CONCENTRATION,
+		.address = SGP30_SET_AH_IDX,
+		.extend_name = "ah",
+		.datasheet_name = "absolute humidty",  

typo: humidity

Quoted 16 lines

Quoted 16 lines

drivers/iio/chemical/sgpxx.c new file +15 -0

+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.output = 1,
+		.scan_index = 5
+	},
+};
+
+static const struct iio_chan_spec sgpc3_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_VOC,
+		.datasheet_name = "TVOC signal",
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.address = SGPC3_IAQ_TVOC_IDX,
+	},

Quoted 6 lines

drivers/iio/chemical/sgpxx.c line 216

Quoted 6 lines

drivers/iio/chemical/sgpxx.c new file +6 -0

+	{
+		.type = IIO_CONCENTRATION,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.address = SGPC3_SIG_ETOH_IDX,
+		.extend_name = "ethanol",

Add a new modifier, not an extended_name

Quoted 13 lines

drivers/iio/chemical/sgpxx.c line 226

Quoted 11 lines

drivers/iio/chemical/sgpxx.c new file +10 -0

+		.datasheet_name = "Ethanol signal",
+		.scan_index = 0,
+		.scan_type = {
+			.endianness = IIO_BE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+static struct sgp_device sgp_devices[] = {  

const?

Quoted 52 lines

Quoted 52 lines

drivers/iio/chemical/sgpxx.c new file +51 -0

+	[SGP30] = {
+		.channels = sgp30_channels,
+		.num_channels = ARRAY_SIZE(sgp30_channels),
+	},
+	[SGPC3] = {
+		.channels = sgpc3_channels,
+		.num_channels = ARRAY_SIZE(sgpc3_channels),
+	},
+};
+
+/**
+ * sgp_verify_buffer() - verify the checksums of the data buffer words
+ *
+ * @data:       SGP data containing the raw buffer
+ * @word_count: Num data words stored in the buffer, excluding CRC bytes
+ *
+ * Return:      0 on success, negative error code otherwise
+ */
+static int sgp_verify_buffer(struct sgp_data *data, size_t word_count)
+{
+	size_t size = word_count * (SGP_WORD_LEN + SGP_CRC8_LEN);
+	int i;
+	u8 crc;
+	u8 *data_buf = &data->buffer.start;
+
+	for (i = 0; i < size; i += SGP_WORD_LEN + SGP_CRC8_LEN) {
+		crc = crc8(sgp_crc8_table, &data_buf[i], SGP_WORD_LEN,
+			   SGP_CRC8_INIT);
+		if (crc != data_buf[i + SGP_WORD_LEN]) {
+			dev_err(&data->client->dev, "CRC error\n");
+			return -EIO;
+		}
+	}
+	return 0;
+}
+
+/**
+ * sgp_read_from_cmd() - reads data from SGP sensor after issuing a command
+ * The caller must hold data->data_lock for the duration of the call.
+ * @data:        SGP data
+ * @cmd:         SGP Command to issue
+ * @word_count:  Num words to read, excluding CRC bytes
+ *
+ * Return:       0 on success, negative error otherwise.
+ */
+static int sgp_read_from_cmd(struct sgp_data *data,
+			     enum sgp_cmd cmd,
+			     size_t word_count,
+			     unsigned long duration_us)
+{
+	int ret;