Re: [PATCH 1/2] iio: chemical: sgpxx: Support Sensirion SGPxx sensors

[Jonathan Cameron]

On Tue, 21 Nov 2017 22:46:07 +0100 (CET)
Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> wrote:

> Hello,
> 
> some quick comments on this driver below
A few additional bits from me but I think Peter got most of the key
stuff.

> 
> I think documentation is missing and the ABI is a bit problematic and 
> unusual 
> 
> > Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors  
> 
> generally, we tend to avoid wildcard driver names; sgp30 would be 
> preferred over sgpxx
>  
> > 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.  
> 
> may some brief TODOs; heat controller?
>  
> > Signed-off-by: Andreas Brauchli <andreas.brauchli@xxxxxxxxxxxxx>
> > ---
> >  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
> > 
> > diff --git a/Documentation/iio/chemical/sgpxx.txt b/Documentation/iio/chemical/sgpxx.txt
> > new file mode 100644
> > index 000000000000..f49b2f365df3
> > --- /dev/null
> > +++ b/Documentation/iio/chemical/sgpxx.txt
> > @@ -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

Put this stuff in the driver itself.

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

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

> 
> in any case, private API should be documented under
> Documentation/ABI/testing/sysfs-bus-iio-*
> 
> > +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.

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

> 
> typo: returned
> 
> > +
> > +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

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.

> 
> > +
> > +    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?

Cool a new sensor type ;)

> 
> > +* H2 (in_concentration_h2_raw) -- SGP30 only  
> 
> we'd need a IIO_MOD_H2?
> 
> > +
> > +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.

> 
> 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
> 
> > +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?
> 
> > +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
> > +
> > +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?

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

> 
> > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > index 5cb5be7612b4..4574dd687513 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -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
> > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > index a629b29d1e0b..6090a0ae3981 100644
> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile
> > @@ -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
> > diff --git a/drivers/iio/chemical/sgpxx.c b/drivers/iio/chemical/sgpxx.c
> > new file mode 100644
> > index 000000000000..aea55e41d4cc
> > --- /dev/null
> > +++ b/drivers/iio/chemical/sgpxx.c
> > @@ -0,0 +1,894 @@
> > +/*
> > + * sgpxx.c - Support for Sensirion SGP Gas Sensors
> > + *
> > + * Copyright (C) 2017 Andreas Brauchli <andreas.brauchli@xxxxxxxxxxxxx>
> > + *
> > + * 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)
> > +#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?
> 
> > +#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 */

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.

> > +	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;
> > +};
> > +
> > +static const struct sgp_version supported_versions_sgp30[] = {
> > +	{
> > +		.major = 1,
> > +		.minor = 0,
> > +	}  
> 
> end with comma (,) so that it can be extended with minimal change
> 
> > +};
> > +
> > +static const struct sgp_version supported_versions_sgpc3[] = {
> > +	{
> > +		.major = 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
> 
> > +		.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,
> > +	},
> > +	{
> > +		.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?
Add a new modifier, not an extended_name

> 
> > +		},
> > +	},
> > +	{
> > +		.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?
> 
> > +		.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
> 
> > +		.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",
Add a new modifier, not an extended_name

> > +		.datasheet_name = "Ethanol signal",
> > +		.scan_index = 0,
> > +		.scan_type = {
> > +			.endianness = IIO_BE,
> > +		},
> > +	},
> > +	IIO_CHAN_SOFT_TIMESTAMP(2),
> > +};
> > +
> > +static struct sgp_device sgp_devices[] = {  
> 
> const?
> 
> > +	[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);
Given this is always done (I think) with the data lock held
I'm not sure you need any more serialization...
> > +	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;
-EBUSY
> > +	else if (ret != size)
> > +		ret = -EINTR;
-EIO is more common - please look at other drivers
when picking your error codes and do whatever they do...
(I had to look up these two rather obscure options!)

> > +	else
> > +		ret = sgp_verify_buffer(data, word_count);
> > +
> > +	return ret;
just return directly in each branch above (making the
else bits unnecessary

if (ret < 0)
	return -EBUSY; // or return ret; which would be better
if (ret != size)
	return -EIO;

return sgp_verific_buffer(data, word_count);

> > +}
> > +
> > +/**
> > + * 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)
> > +{
> > +	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?
Just make it big enough that we don't have to care
what size it is (max size).
> 
> > +
> > +	/* assemble buffer */
> > +	*((u16 *)&buffer[0]) = cmd;  
> 
> be16
> 
> > +	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?
> 
> > +		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

> > +	/* 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:
labels for gotos mid switch statement are ugly and hard
to follow. In this case only called form one place, just put the
code there.

> > +		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);
This is fun - making the world wet ;)
Joking aside, we need to handle this differently as it
isn't an output device but rather a calibration parameter.

 
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
return -EINVAL etc.
> > +	}
> > +
> > +	return ret;
Direct returns in the switch.
> > +}
> > +
> > +static ssize_t sgp_iaq_init_store(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  const char *buf, size_t count)
> > +{

Why is userspace having explicit control of init?

You init when you load the driver.  The device then needs
to do periodic readings to maintain the baseline algorithm.
All that should be wrapped up in the driver not pushed to userspace.

The basic aim of kernel driver frameworks is to allow generic
userspace code this driver looks like it needs some very careful
handling in userspace - you need to make the kernel driver do
that clever stuff for you.

> > +	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:
Only called in place, put this there.

> > +	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:
Given this only happens from one place put this in that if (ret < 0) block.

> > +	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 */
This definitely needs some explanation / documentation including
how it would be used.

I'm unconvinced you need to be this specfic on the formatting of a
sysfs attribute. Normally a given sysfs attribute should reflect only
one quantity - this is inherently 2 but the hardware requires
they be handled together so we may have to cope with this nastiness.

>From reading (very briefly) the datasheet I gather this about
restoring some tracked values after a power down or similar.

> > +	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");
> > +

How would userspace software know to do a self test?
Usual convention is to do it on module load and report the failure in the
kernel logs if there is one.

> > +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);
Not normally of much use except in perhaps debugging so we normally
only expose these as a kernel log message rather than cluttering the
sysfs abi.

> > +}
> > +
> > +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;

Add some macros to represent what is going with this break
up in to fields.

> > +	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 */

Comment should probably say why.  Incompatible interface?

> > +	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;
How about having a single chip_info structure with all this stuff
in and adding that to data

data->chip_info = sgp_chip_info[sgp30];

> > +		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);
> > +	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...
> 
> > +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;
Not convinced this local variable adds anything to readability.

> > +	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)

Seems a touch backward
if (of_id)
	chip_id = (unsigned long)of_id->data;
else
	chip_id = id->driver_data;

> > +		chip_id = id->driver_data;
> > +	else
> > +		chip_id = (unsigned long)of_id->data;
> > +
> > +	chip = &sgp_devices[chip_id];
> > +	data = iio_priv(indio_dev);
> > +	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);
No blank line between function call and it's error handling.
> > +
> > +	if (ret != 0)  
> 
> matter of taste: most drivers just do
> if (ret)

Also in kernel taste tends to lead to static analysers pointing it
out and hence patches to change it. I'd much rather we went
with Peter's suggestion in the first place :)

> 
> > +		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);
> > +	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
> 
> > +
> > +	indio_dev->channels = chip->channels;
> > +	indio_dev->num_channels = chip->num_channels;
> > +
> > +	ret = devm_iio_device_register(&client->dev, indio_dev);
> > +	if (!ret)
Please don't do this - have error paths indented if there is
anything to do in them, not the good path.

It's not what people expect to see so takes more effort to review.
(I refer you to Greg KH's presentations on the Grumpy maintainer
- make our lives easy and we'll be less grumpy)

> > +		return ret;
> > +
> > +	dev_err(&client->dev, "failed to register iio device\n");  
> 
> really message needed?
> 
> > +
> > +fail_free:
> > +	mutex_destroy(&data->i2c_lock);
> > +	mutex_destroy(&data->data_lock);  
> 
> no need to explicitly destroy mutex

Actually this is one of those fun ones.  There is a reason for
calling mutex_destroy and it is about lock debugging... Now we
almost never bother as if there aren't any bugs in our mutex
code it doesn't matter - but arguably perhaps we should.

/***
 * mutex_destroy - mark a mutex unusable
 * @lock: the mutex to be destroyed
 *
 * This function marks the mutex uninitialized, and any subsequent
 * use of the mutex is forbidden. The mutex must not be locked when
 * this function is called.
 */

Also if you did want to do this, then you'd also need to have
them in the remove function for consistency.

All in all, I'd probably drop them.

> 
> > +	iio_device_free(indio_dev);  
> 
> no need to explicitly free devm_ stuff

Particularly not without using devm_iio_device_free
as this will give a double free error.

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

Which incidentally means you don't need remove at all.

Please look at what devm_ calls actually do so you understand
why this isn't needed.

> 
> > +	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 <andreas.brauchli@xxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Pascal Sachs <pascal.sachs@xxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Sensirion SGPxx gas sensors");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_VERSION("0.5.0");
> >   
>