[PATCH v2 0/4] iio: chemical: vz89x: add multiple chip functionality

[PATCH v2 0/4] iio: chemical: vz89x: add multiple chip functionality

[Matt Ranostay]

Add support for multiple chip configuration, support for the vz89te part,
CRC check functionality, and buffer corruption check.

Changes from v1:
 * Move chip configuration to vz89_chip_data struct
 * Removed redundant mutex and use indio_dev->mlock instead

Matt Ranostay (4):
  iio: chemical: vz89x: abstract chip configuration
  iio: chemical: vz89x: add support for VZ89TE part
  iio: chemical: vz89x: prevent corrupted buffer from being read
  iio: chemical: vz89x: remove mutex and replace with
    iio_device_*_direct_mode

 drivers/iio/chemical/vz89x.c | 222 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 178 insertions(+), 44 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] iio: chemical: vz89x: abstract chip configuration

[Matt Ranostay]

Abstract chip configuration data to allow supporting multiple variants
of the VZ89 chemical sensor line.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/iio/chemical/vz89x.c | 80 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 62 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
index 652649da500f..aa6ebc084e19 100644
--- a/drivers/iio/chemical/vz89x.c
+++ b/drivers/iio/chemical/vz89x.c
@@ -19,25 +19,45 @@
 #include <linux/mutex.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
 #define VZ89X_REG_MEASUREMENT		0x09
-#define VZ89X_REG_MEASUREMENT_SIZE	6
+#define VZ89X_REG_MEASUREMENT_RD_SIZE	6
+#define VZ89X_REG_MEASUREMENT_WR_SIZE	3
 
 #define VZ89X_VOC_CO2_IDX		0
 #define VZ89X_VOC_SHORT_IDX		1
 #define VZ89X_VOC_TVOC_IDX		2
 #define VZ89X_VOC_RESISTANCE_IDX	3
 
+enum {
+	VZ89X,
+};
+
+struct vz89x_chip_data;
+
 struct vz89x_data {
 	struct i2c_client *client;
+	const struct vz89x_chip_data *chip;
 	struct mutex lock;
 	int (*xfer)(struct vz89x_data *data, u8 cmd);
 
 	unsigned long last_update;
-	u8 buffer[VZ89X_REG_MEASUREMENT_SIZE];
+	u8 buffer[VZ89X_REG_MEASUREMENT_RD_SIZE];
+};
+
+struct vz89x_chip_data {
+	bool (*valid)(struct vz89x_data *data);
+	const struct iio_chan_spec *channels;
+	u8 num_channels;
+
+	u8 cmd;
+	u8 read_size;
+	u8 write_size;
 };
 
 static const struct iio_chan_spec vz89x_channels[] = {
@@ -93,16 +113,17 @@ static const struct attribute_group vz89x_attrs_group = {
  * always zero, and by also confirming the VOC_short isn't zero.
  */
 
-static int vz89x_measurement_is_valid(struct vz89x_data *data)
+static bool vz89x_measurement_is_valid(struct vz89x_data *data)
 {
 	if (data->buffer[VZ89X_VOC_SHORT_IDX] == 0)
 		return 1;
 
-	return !!(data->buffer[VZ89X_REG_MEASUREMENT_SIZE - 1] > 0);
+	return !!(data->buffer[data->chip->read_size - 1] > 0);
 }
 
 static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
 {
+	const struct vz89x_chip_data *chip = data->chip;
 	struct i2c_client *client = data->client;
 	struct i2c_msg msg[2];
 	int ret;
@@ -110,12 +131,12 @@ static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
 
 	msg[0].addr = client->addr;
 	msg[0].flags = client->flags;
-	msg[0].len = 3;
+	msg[0].len = chip->write_size;
 	msg[0].buf  = (char *) &buf;
 
 	msg[1].addr = client->addr;
 	msg[1].flags = client->flags | I2C_M_RD;
-	msg[1].len = VZ89X_REG_MEASUREMENT_SIZE;
+	msg[1].len = chip->read_size;
 	msg[1].buf = (char *) &data->buffer;
 
 	ret = i2c_transfer(client->adapter, msg, 2);
@@ -133,7 +154,7 @@ static int vz89x_smbus_xfer(struct vz89x_data *data, u8 cmd)
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < VZ89X_REG_MEASUREMENT_SIZE; i++) {
+	for (i = 0; i < data->chip->read_size; i++) {
 		ret = i2c_smbus_read_byte(client);
 		if (ret < 0)
 			return ret;
@@ -145,17 +166,18 @@ static int vz89x_smbus_xfer(struct vz89x_data *data, u8 cmd)
 
 static int vz89x_get_measurement(struct vz89x_data *data)
 {
+	const struct vz89x_chip_data *chip = data->chip;
 	int ret;
 
 	/* sensor can only be polled once a second max per datasheet */
 	if (!time_after(jiffies, data->last_update + HZ))
 		return 0;
 
-	ret = data->xfer(data, VZ89X_REG_MEASUREMENT);
+	ret = data->xfer(data, chip->cmd);
 	if (ret < 0)
 		return ret;
 
-	ret = vz89x_measurement_is_valid(data);
+	ret = chip->valid(data);
 	if (ret)
 		return -EAGAIN;
 
@@ -232,11 +254,32 @@ static const struct iio_info vz89x_info = {
 	.driver_module	= THIS_MODULE,
 };
 
+static const struct vz89x_chip_data vz89x_chips[] = {
+	{
+		.valid = vz89x_measurement_is_valid,
+
+		.cmd = VZ89X_REG_MEASUREMENT,
+		.read_size = VZ89X_REG_MEASUREMENT_RD_SIZE,
+		.write_size = VZ89X_REG_MEASUREMENT_WR_SIZE,
+
+		.channels = vz89x_channels,
+		.num_channels = ARRAY_SIZE(vz89x_channels),
+	},
+};
+
+static const struct of_device_id vz89x_dt_ids[] = {
+	{ .compatible = "sgx,vz89x", .data = (void *) VZ89X },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
+
 static int vz89x_probe(struct i2c_client *client,
 		       const struct i2c_device_id *id)
 {
 	struct iio_dev *indio_dev;
 	struct vz89x_data *data;
+	const struct of_device_id *of_id;
+	int chip_id;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -251,8 +294,15 @@ static int vz89x_probe(struct i2c_client *client,
 	else
 		return -EOPNOTSUPP;
 
+	of_id = of_match_device(vz89x_dt_ids, &client->dev);
+	if (!of_id)
+		chip_id = id->driver_data;
+	else
+		chip_id = (unsigned long)of_id->data;
+
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
+	data->chip = &vz89x_chips[chip_id];
 	data->last_update = jiffies - HZ;
 	mutex_init(&data->lock);
 
@@ -261,24 +311,18 @@ static int vz89x_probe(struct i2c_client *client,
 	indio_dev->name = dev_name(&client->dev);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	indio_dev->channels = vz89x_channels;
-	indio_dev->num_channels = ARRAY_SIZE(vz89x_channels);
+	indio_dev->channels = data->chip->channels;
+	indio_dev->num_channels = data->chip->num_channels;
 
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static const struct i2c_device_id vz89x_id[] = {
-	{ "vz89x", 0 },
+	{ "vz89x", VZ89X },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, vz89x_id);
 
-static const struct of_device_id vz89x_dt_ids[] = {
-	{ .compatible = "sgx,vz89x" },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
-
 static struct i2c_driver vz89x_driver = {
 	.driver = {
 		.name	= "vz89x",
-- 
2.7.4

[PATCH v2 2/4] iio: chemical: vz89x: add support for VZ89TE part

[Matt Ranostay]

Add support the VZ89TE variant which removes the voc_short channel,
and has CRC check for data transactions.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/iio/chemical/vz89x.c | 113 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 98 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
index aa6ebc084e19..289d2919deea 100644
--- a/drivers/iio/chemical/vz89x.c
+++ b/drivers/iio/chemical/vz89x.c
@@ -34,8 +34,17 @@
 #define VZ89X_VOC_TVOC_IDX		2
 #define VZ89X_VOC_RESISTANCE_IDX	3
 
+#define VZ89TE_REG_MEASUREMENT		0x0c
+#define VZ89TE_REG_MEASUREMENT_RD_SIZE	7
+#define VZ89TE_REG_MEASUREMENT_WR_SIZE	6
+
+#define VZ89TE_VOC_TVOC_IDX		0
+#define VZ89TE_VOC_CO2_IDX		1
+#define VZ89TE_VOC_RESISTANCE_IDX	2
+
 enum {
 	VZ89X,
+	VZ89TE,
 };
 
 struct vz89x_chip_data;
@@ -47,7 +56,7 @@ struct vz89x_data {
 	int (*xfer)(struct vz89x_data *data, u8 cmd);
 
 	unsigned long last_update;
-	u8 buffer[VZ89X_REG_MEASUREMENT_RD_SIZE];
+	u8 buffer[VZ89TE_REG_MEASUREMENT_RD_SIZE];
 };
 
 struct vz89x_chip_data {
@@ -90,6 +99,40 @@ static const struct iio_chan_spec vz89x_channels[] = {
 		.info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
 		.address = VZ89X_VOC_RESISTANCE_IDX,
+		.scan_index = -1,
+		.scan_type = {
+			.endianness = IIO_LE,
+		},
+	},
+};
+
+static const struct iio_chan_spec vz89te_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_VOC,
+		.modified = 1,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_OFFSET) | BIT(IIO_CHAN_INFO_RAW),
+		.address = VZ89TE_VOC_TVOC_IDX,
+	},
+
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_CO2,
+		.modified = 1,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_OFFSET) | BIT(IIO_CHAN_INFO_RAW),
+		.address = VZ89TE_VOC_CO2_IDX,
+	},
+	{
+		.type = IIO_RESISTANCE,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.address = VZ89TE_VOC_RESISTANCE_IDX,
+		.scan_index = -1,
+		.scan_type = {
+			.endianness = IIO_BE,
+		},
 	},
 };
 
@@ -121,13 +164,28 @@ static bool vz89x_measurement_is_valid(struct vz89x_data *data)
 	return !!(data->buffer[data->chip->read_size - 1] > 0);
 }
 
+/* VZ89TE device has a modified CRC-8 two complement check */
+static bool vz89te_measurement_is_valid(struct vz89x_data *data)
+{
+	u8 crc = 0;
+	int i, sum = 0;
+
+	for (i = 0; i < (data->chip->read_size - 1); i++) {
+		sum = crc + data->buffer[i];
+		crc = sum;
+		crc += sum / 256;
+	}
+
+	return !((0xff - crc) == data->buffer[data->chip->read_size - 1]);
+}
+
 static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
 {
 	const struct vz89x_chip_data *chip = data->chip;
 	struct i2c_client *client = data->client;
 	struct i2c_msg msg[2];
 	int ret;
-	u8 buf[3] = { cmd, 0, 0};
+	u8 buf[6] = { cmd, 0, 0, 0, 0, 0xf3 };
 
 	msg[0].addr = client->addr;
 	msg[0].flags = client->flags;
@@ -186,11 +244,24 @@ static int vz89x_get_measurement(struct vz89x_data *data)
 	return 0;
 }
 
-static int vz89x_get_resistance_reading(struct vz89x_data *data)
+static int vz89x_get_resistance_reading(struct vz89x_data *data,
+					struct iio_chan_spec const *chan,
+					int *val)
 {
-	u8 *buf = &data->buffer[VZ89X_VOC_RESISTANCE_IDX];
+	u8 *tmp = (u8 *) &data->buffer[chan->address];
 
-	return buf[0] | (buf[1] << 8);
+	switch (chan->scan_type.endianness) {
+	case IIO_LE:
+		*val = le32_to_cpup((__le32 *) tmp) & GENMASK(23, 0);
+		break;
+	case IIO_BE:
+		*val = be32_to_cpup((__be32 *) tmp) >> 8;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int vz89x_read_raw(struct iio_dev *indio_dev,
@@ -209,15 +280,15 @@ static int vz89x_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
-		switch (chan->address) {
-		case VZ89X_VOC_CO2_IDX:
-		case VZ89X_VOC_SHORT_IDX:
-		case VZ89X_VOC_TVOC_IDX:
+		switch (chan->type) {
+		case IIO_CONCENTRATION:
 			*val = data->buffer[chan->address];
 			return IIO_VAL_INT;
-		case VZ89X_VOC_RESISTANCE_IDX:
-			*val = vz89x_get_resistance_reading(data);
-			return IIO_VAL_INT;
+		case IIO_RESISTANCE:
+			ret = vz89x_get_resistance_reading(data, chan, val);
+			if (!ret)
+				return IIO_VAL_INT;
+			break;
 		default:
 			return -EINVAL;
 		}
@@ -232,12 +303,12 @@ static int vz89x_read_raw(struct iio_dev *indio_dev,
 		}
 		break;
 	case IIO_CHAN_INFO_OFFSET:
-		switch (chan->address) {
-		case VZ89X_VOC_CO2_IDX:
+		switch (chan->channel2) {
+		case IIO_MOD_CO2:
 			*val = 44;
 			*val2 = 250000;
 			return IIO_VAL_INT_PLUS_MICRO;
-		case VZ89X_VOC_TVOC_IDX:
+		case IIO_MOD_VOC:
 			*val = -13;
 			return IIO_VAL_INT;
 		default:
@@ -265,10 +336,21 @@ static const struct vz89x_chip_data vz89x_chips[] = {
 		.channels = vz89x_channels,
 		.num_channels = ARRAY_SIZE(vz89x_channels),
 	},
+	{
+		.valid = vz89te_measurement_is_valid,
+
+		.cmd = VZ89TE_REG_MEASUREMENT,
+		.read_size = VZ89TE_REG_MEASUREMENT_RD_SIZE,
+		.write_size = VZ89TE_REG_MEASUREMENT_WR_SIZE,
+
+		.channels = vz89te_channels,
+		.num_channels = ARRAY_SIZE(vz89te_channels),
+	},
 };
 
 static const struct of_device_id vz89x_dt_ids[] = {
 	{ .compatible = "sgx,vz89x", .data = (void *) VZ89X },
+	{ .compatible = "sgx,vz89te", .data = (void *) VZ89TE },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
@@ -319,6 +401,7 @@ static int vz89x_probe(struct i2c_client *client,
 
 static const struct i2c_device_id vz89x_id[] = {
 	{ "vz89x", VZ89X },
+	{ "vz89te", VZ89TE },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, vz89x_id);
-- 
2.7.4

[PATCH v2 3/4] iio: chemical: vz89x: prevent corrupted buffer from being read

[Matt Ranostay]

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/iio/chemical/vz89x.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
index 289d2919deea..cd3870ead3fd 100644
--- a/drivers/iio/chemical/vz89x.c
+++ b/drivers/iio/chemical/vz89x.c
@@ -55,6 +55,7 @@ struct vz89x_data {
 	struct mutex lock;
 	int (*xfer)(struct vz89x_data *data, u8 cmd);
 
+	bool is_valid;
 	unsigned long last_update;
 	u8 buffer[VZ89TE_REG_MEASUREMENT_RD_SIZE];
 };
@@ -229,7 +230,10 @@ static int vz89x_get_measurement(struct vz89x_data *data)
 
 	/* sensor can only be polled once a second max per datasheet */
 	if (!time_after(jiffies, data->last_update + HZ))
-		return 0;
+		return data->is_valid ? 0 : -EAGAIN;
+
+	data->is_valid = false;
+	data->last_update = jiffies;
 
 	ret = data->xfer(data, chip->cmd);
 	if (ret < 0)
@@ -239,7 +243,7 @@ static int vz89x_get_measurement(struct vz89x_data *data)
 	if (ret)
 		return -EAGAIN;
 
-	data->last_update = jiffies;
+	data->is_valid = true;
 
 	return 0;
 }
-- 
2.7.4

[PATCH v2 4/4] iio: chemical: vz89x: remove mutex and replace with iio_device_*_direct_mode

[Matt Ranostay]

There is no reason to have a seperate mutex when indio_dev->mlock can
be used with the iio_device_*_direct_mode helpers. Also avoid a race
condition that is possible with multiple IIO_RESISTANCE reads.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/iio/chemical/vz89x.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
index cd3870ead3fd..5e5d4a9abe63 100644
--- a/drivers/iio/chemical/vz89x.c
+++ b/drivers/iio/chemical/vz89x.c
@@ -16,7 +16,6 @@
  */
 
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
 #include <linux/of.h>
@@ -52,7 +51,6 @@ struct vz89x_chip_data;
 struct vz89x_data {
 	struct i2c_client *client;
 	const struct vz89x_chip_data *chip;
-	struct mutex lock;
 	int (*xfer)(struct vz89x_data *data, u8 cmd);
 
 	bool is_valid;
@@ -277,26 +275,32 @@ static int vz89x_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&data->lock);
-		ret = vz89x_get_measurement(data);
-		mutex_unlock(&data->lock);
-
+		ret = iio_device_claim_direct_mode(indio_dev);
 		if (ret)
 			return ret;
 
+		ret = vz89x_get_measurement(data);
+		if (ret)
+			goto error_out;
+
 		switch (chan->type) {
 		case IIO_CONCENTRATION:
 			*val = data->buffer[chan->address];
-			return IIO_VAL_INT;
+			ret = IIO_VAL_INT;
+			break;
 		case IIO_RESISTANCE:
 			ret = vz89x_get_resistance_reading(data, chan, val);
 			if (!ret)
-				return IIO_VAL_INT;
+				ret = IIO_VAL_INT;
 			break;
 		default:
-			return -EINVAL;
+			ret = -EINVAL;
 		}
-		break;
+
+error_out:
+		iio_device_release_direct_mode(indio_dev);
+
+		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_RESISTANCE:
@@ -390,7 +394,6 @@ static int vz89x_probe(struct i2c_client *client,
 	data->client = client;
 	data->chip = &vz89x_chips[chip_id];
 	data->last_update = jiffies - HZ;
-	mutex_init(&data->lock);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &vz89x_info,
-- 
2.7.4

Re: [PATCH v2 1/4] iio: chemical: vz89x: abstract chip configuration

[Jonathan Cameron]

On 25/08/16 07:44, Matt Ranostay wrote:
> Abstract chip configuration data to allow supporting multiple variants
> of the VZ89 chemical sensor line.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
Looks good to me.

Applied to the togreg branch of iio.git. Initially pushed
out as testing for the autobuilders to play with it.

Peter, if you want (or anyone else) wants to add anything
I doubt I'll get this heading to Greg for at least a few
days.

Jonathan

> ---
>  drivers/iio/chemical/vz89x.c | 80 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 62 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
> index 652649da500f..aa6ebc084e19 100644
> --- a/drivers/iio/chemical/vz89x.c
> +++ b/drivers/iio/chemical/vz89x.c
> @@ -19,25 +19,45 @@
>  #include <linux/mutex.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
>  #define VZ89X_REG_MEASUREMENT		0x09
> -#define VZ89X_REG_MEASUREMENT_SIZE	6
> +#define VZ89X_REG_MEASUREMENT_RD_SIZE	6
> +#define VZ89X_REG_MEASUREMENT_WR_SIZE	3
>  
>  #define VZ89X_VOC_CO2_IDX		0
>  #define VZ89X_VOC_SHORT_IDX		1
>  #define VZ89X_VOC_TVOC_IDX		2
>  #define VZ89X_VOC_RESISTANCE_IDX	3
>  
> +enum {
> +	VZ89X,
> +};
> +
> +struct vz89x_chip_data;
> +
>  struct vz89x_data {
>  	struct i2c_client *client;
> +	const struct vz89x_chip_data *chip;
>  	struct mutex lock;
>  	int (*xfer)(struct vz89x_data *data, u8 cmd);
>  
>  	unsigned long last_update;
> -	u8 buffer[VZ89X_REG_MEASUREMENT_SIZE];
> +	u8 buffer[VZ89X_REG_MEASUREMENT_RD_SIZE];
> +};
> +
> +struct vz89x_chip_data {
> +	bool (*valid)(struct vz89x_data *data);
> +	const struct iio_chan_spec *channels;
> +	u8 num_channels;
> +
> +	u8 cmd;
> +	u8 read_size;
> +	u8 write_size;
>  };
>  
>  static const struct iio_chan_spec vz89x_channels[] = {
> @@ -93,16 +113,17 @@ static const struct attribute_group vz89x_attrs_group = {
>   * always zero, and by also confirming the VOC_short isn't zero.
>   */
>  
> -static int vz89x_measurement_is_valid(struct vz89x_data *data)
> +static bool vz89x_measurement_is_valid(struct vz89x_data *data)
>  {
>  	if (data->buffer[VZ89X_VOC_SHORT_IDX] == 0)
>  		return 1;
>  
> -	return !!(data->buffer[VZ89X_REG_MEASUREMENT_SIZE - 1] > 0);
> +	return !!(data->buffer[data->chip->read_size - 1] > 0);
>  }
>  
>  static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
>  {
> +	const struct vz89x_chip_data *chip = data->chip;
>  	struct i2c_client *client = data->client;
>  	struct i2c_msg msg[2];
>  	int ret;
> @@ -110,12 +131,12 @@ static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
>  
>  	msg[0].addr = client->addr;
>  	msg[0].flags = client->flags;
> -	msg[0].len = 3;
> +	msg[0].len = chip->write_size;
>  	msg[0].buf  = (char *) &buf;
>  
>  	msg[1].addr = client->addr;
>  	msg[1].flags = client->flags | I2C_M_RD;
> -	msg[1].len = VZ89X_REG_MEASUREMENT_SIZE;
> +	msg[1].len = chip->read_size;
>  	msg[1].buf = (char *) &data->buffer;
>  
>  	ret = i2c_transfer(client->adapter, msg, 2);
> @@ -133,7 +154,7 @@ static int vz89x_smbus_xfer(struct vz89x_data *data, u8 cmd)
>  	if (ret < 0)
>  		return ret;
>  
> -	for (i = 0; i < VZ89X_REG_MEASUREMENT_SIZE; i++) {
> +	for (i = 0; i < data->chip->read_size; i++) {
>  		ret = i2c_smbus_read_byte(client);
>  		if (ret < 0)
>  			return ret;
> @@ -145,17 +166,18 @@ static int vz89x_smbus_xfer(struct vz89x_data *data, u8 cmd)
>  
>  static int vz89x_get_measurement(struct vz89x_data *data)
>  {
> +	const struct vz89x_chip_data *chip = data->chip;
>  	int ret;
>  
>  	/* sensor can only be polled once a second max per datasheet */
>  	if (!time_after(jiffies, data->last_update + HZ))
>  		return 0;
>  
> -	ret = data->xfer(data, VZ89X_REG_MEASUREMENT);
> +	ret = data->xfer(data, chip->cmd);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = vz89x_measurement_is_valid(data);
> +	ret = chip->valid(data);
>  	if (ret)
>  		return -EAGAIN;
>  
> @@ -232,11 +254,32 @@ static const struct iio_info vz89x_info = {
>  	.driver_module	= THIS_MODULE,
>  };
>  
> +static const struct vz89x_chip_data vz89x_chips[] = {
> +	{
> +		.valid = vz89x_measurement_is_valid,
> +
> +		.cmd = VZ89X_REG_MEASUREMENT,
> +		.read_size = VZ89X_REG_MEASUREMENT_RD_SIZE,
> +		.write_size = VZ89X_REG_MEASUREMENT_WR_SIZE,
> +
> +		.channels = vz89x_channels,
> +		.num_channels = ARRAY_SIZE(vz89x_channels),
> +	},
> +};
> +
> +static const struct of_device_id vz89x_dt_ids[] = {
> +	{ .compatible = "sgx,vz89x", .data = (void *) VZ89X },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
> +
>  static int vz89x_probe(struct i2c_client *client,
>  		       const struct i2c_device_id *id)
>  {
>  	struct iio_dev *indio_dev;
>  	struct vz89x_data *data;
> +	const struct of_device_id *of_id;
> +	int chip_id;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -251,8 +294,15 @@ static int vz89x_probe(struct i2c_client *client,
>  	else
>  		return -EOPNOTSUPP;
>  
> +	of_id = of_match_device(vz89x_dt_ids, &client->dev);
> +	if (!of_id)
> +		chip_id = id->driver_data;
> +	else
> +		chip_id = (unsigned long)of_id->data;
> +
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
> +	data->chip = &vz89x_chips[chip_id];
>  	data->last_update = jiffies - HZ;
>  	mutex_init(&data->lock);
>  
> @@ -261,24 +311,18 @@ static int vz89x_probe(struct i2c_client *client,
>  	indio_dev->name = dev_name(&client->dev);
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	indio_dev->channels = vz89x_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(vz89x_channels);
> +	indio_dev->channels = data->chip->channels;
> +	indio_dev->num_channels = data->chip->num_channels;
>  
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  
>  static const struct i2c_device_id vz89x_id[] = {
> -	{ "vz89x", 0 },
> +	{ "vz89x", VZ89X },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, vz89x_id);
>  
> -static const struct of_device_id vz89x_dt_ids[] = {
> -	{ .compatible = "sgx,vz89x" },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
> -
>  static struct i2c_driver vz89x_driver = {
>  	.driver = {
>  		.name	= "vz89x",
> 

Re: [PATCH v2 2/4] iio: chemical: vz89x: add support for VZ89TE part

[Jonathan Cameron]

On 25/08/16 07:44, Matt Ranostay wrote:
> Add support the VZ89TE variant which removes the voc_short channel,
> and has CRC check for data transactions.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
Minor comment inline.

Applied to the togreg branch of iio.git - initially pushed
out as testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/chemical/vz89x.c | 113 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 98 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
> index aa6ebc084e19..289d2919deea 100644
> --- a/drivers/iio/chemical/vz89x.c
> +++ b/drivers/iio/chemical/vz89x.c
> @@ -34,8 +34,17 @@
>  #define VZ89X_VOC_TVOC_IDX		2
>  #define VZ89X_VOC_RESISTANCE_IDX	3
>  
> +#define VZ89TE_REG_MEASUREMENT		0x0c
> +#define VZ89TE_REG_MEASUREMENT_RD_SIZE	7
> +#define VZ89TE_REG_MEASUREMENT_WR_SIZE	6
> +
> +#define VZ89TE_VOC_TVOC_IDX		0
> +#define VZ89TE_VOC_CO2_IDX		1
> +#define VZ89TE_VOC_RESISTANCE_IDX	2
> +
>  enum {
>  	VZ89X,
> +	VZ89TE,
>  };
>  
>  struct vz89x_chip_data;
> @@ -47,7 +56,7 @@ struct vz89x_data {
>  	int (*xfer)(struct vz89x_data *data, u8 cmd);
>  
>  	unsigned long last_update;
> -	u8 buffer[VZ89X_REG_MEASUREMENT_RD_SIZE];
> +	u8 buffer[VZ89TE_REG_MEASUREMENT_RD_SIZE];
>  };
>  
>  struct vz89x_chip_data {
> @@ -90,6 +99,40 @@ static const struct iio_chan_spec vz89x_channels[] = {
>  		.info_mask_separate =
>  			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>  		.address = VZ89X_VOC_RESISTANCE_IDX,
> +		.scan_index = -1,
> +		.scan_type = {
> +			.endianness = IIO_LE,
> +		},
> +	},
> +};
> +
> +static const struct iio_chan_spec vz89te_channels[] = {
> +	{
> +		.type = IIO_CONCENTRATION,
> +		.channel2 = IIO_MOD_VOC,
> +		.modified = 1,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_OFFSET) | BIT(IIO_CHAN_INFO_RAW),
> +		.address = VZ89TE_VOC_TVOC_IDX,
> +	},
> +
> +	{
> +		.type = IIO_CONCENTRATION,
> +		.channel2 = IIO_MOD_CO2,
> +		.modified = 1,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_OFFSET) | BIT(IIO_CHAN_INFO_RAW),
> +		.address = VZ89TE_VOC_CO2_IDX,
> +	},
> +	{
> +		.type = IIO_RESISTANCE,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +		.address = VZ89TE_VOC_RESISTANCE_IDX,
> +		.scan_index = -1,
> +		.scan_type = {
> +			.endianness = IIO_BE,
> +		},
>  	},
>  };
>  
> @@ -121,13 +164,28 @@ static bool vz89x_measurement_is_valid(struct vz89x_data *data)
>  	return !!(data->buffer[data->chip->read_size - 1] > 0);
>  }
>  
> +/* VZ89TE device has a modified CRC-8 two complement check */
> +static bool vz89te_measurement_is_valid(struct vz89x_data *data)
> +{
> +	u8 crc = 0;
> +	int i, sum = 0;
> +
> +	for (i = 0; i < (data->chip->read_size - 1); i++) {
> +		sum = crc + data->buffer[i];
> +		crc = sum;
> +		crc += sum / 256;
> +	}
> +
> +	return !((0xff - crc) == data->buffer[data->chip->read_size - 1]);
> +}
> +
>  static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
>  {
>  	const struct vz89x_chip_data *chip = data->chip;
>  	struct i2c_client *client = data->client;
>  	struct i2c_msg msg[2];
>  	int ret;
> -	u8 buf[3] = { cmd, 0, 0};
> +	u8 buf[6] = { cmd, 0, 0, 0, 0, 0xf3 };
>  
>  	msg[0].addr = client->addr;
>  	msg[0].flags = client->flags;
> @@ -186,11 +244,24 @@ static int vz89x_get_measurement(struct vz89x_data *data)
>  	return 0;
>  }
>  
> -static int vz89x_get_resistance_reading(struct vz89x_data *data)
> +static int vz89x_get_resistance_reading(struct vz89x_data *data,
> +					struct iio_chan_spec const *chan,
> +					int *val)
>  {
> -	u8 *buf = &data->buffer[VZ89X_VOC_RESISTANCE_IDX];
> +	u8 *tmp = (u8 *) &data->buffer[chan->address];
>  
> -	return buf[0] | (buf[1] << 8);
> +	switch (chan->scan_type.endianness) {
> +	case IIO_LE:
> +		*val = le32_to_cpup((__le32 *) tmp) & GENMASK(23, 0);
> +		break;
> +	case IIO_BE:
> +		*val = be32_to_cpup((__be32 *) tmp) >> 8;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  
>  static int vz89x_read_raw(struct iio_dev *indio_dev,
> @@ -209,15 +280,15 @@ static int vz89x_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  
> -		switch (chan->address) {
> -		case VZ89X_VOC_CO2_IDX:
> -		case VZ89X_VOC_SHORT_IDX:
> -		case VZ89X_VOC_TVOC_IDX:
Hmm. This rework should probably have been in a separate 'pre' patch.
> +		switch (chan->type) {
> +		case IIO_CONCENTRATION:
>  			*val = data->buffer[chan->address];
>  			return IIO_VAL_INT;
> -		case VZ89X_VOC_RESISTANCE_IDX:
> -			*val = vz89x_get_resistance_reading(data);
> -			return IIO_VAL_INT;
> +		case IIO_RESISTANCE:
> +			ret = vz89x_get_resistance_reading(data, chan, val);
> +			if (!ret)
> +				return IIO_VAL_INT;
> +			break;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -232,12 +303,12 @@ static int vz89x_read_raw(struct iio_dev *indio_dev,
>  		}
>  		break;
>  	case IIO_CHAN_INFO_OFFSET:
> -		switch (chan->address) {
> -		case VZ89X_VOC_CO2_IDX:
> +		switch (chan->channel2) {
> +		case IIO_MOD_CO2:
>  			*val = 44;
>  			*val2 = 250000;
>  			return IIO_VAL_INT_PLUS_MICRO;
> -		case VZ89X_VOC_TVOC_IDX:
> +		case IIO_MOD_VOC:
>  			*val = -13;
>  			return IIO_VAL_INT;
>  		default:
> @@ -265,10 +336,21 @@ static const struct vz89x_chip_data vz89x_chips[] = {
>  		.channels = vz89x_channels,
>  		.num_channels = ARRAY_SIZE(vz89x_channels),
>  	},
> +	{
> +		.valid = vz89te_measurement_is_valid,
> +
> +		.cmd = VZ89TE_REG_MEASUREMENT,
> +		.read_size = VZ89TE_REG_MEASUREMENT_RD_SIZE,
> +		.write_size = VZ89TE_REG_MEASUREMENT_WR_SIZE,
> +
> +		.channels = vz89te_channels,
> +		.num_channels = ARRAY_SIZE(vz89te_channels),
> +	},
>  };
>  
>  static const struct of_device_id vz89x_dt_ids[] = {
>  	{ .compatible = "sgx,vz89x", .data = (void *) VZ89X },
> +	{ .compatible = "sgx,vz89te", .data = (void *) VZ89TE },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
> @@ -319,6 +401,7 @@ static int vz89x_probe(struct i2c_client *client,
>  
>  static const struct i2c_device_id vz89x_id[] = {
>  	{ "vz89x", VZ89X },
> +	{ "vz89te", VZ89TE },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, vz89x_id);
> 

Re: [PATCH v2 4/4] iio: chemical: vz89x: remove mutex and replace with iio_device_*_direct_mode

[Jonathan Cameron]

On 25/08/16 07:44, Matt Ranostay wrote:
> There is no reason to have a seperate mutex when indio_dev->mlock can
> be used with the iio_device_*_direct_mode helpers. Also avoid a race
> condition that is possible with multiple IIO_RESISTANCE reads.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
Hi Matt,

Hmm. I'm wondering a bit if I jumped the gun in the previous review
of a patch like this.

To my mind these functions are always about preventing mode switches
or multiple reads of the _raw attributes clashing.

I suppose the second case is true here.  If you were for example
also protecting the reading of a scale register, it would be
less obvious though and it's that blur I don't like.

I think I'd still prefer to keep the explicit mutex in place here
and fix the race more directly...

Close run thing though, you nearly had me persuaded :)

Jonathan
> ---
>  drivers/iio/chemical/vz89x.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
> index cd3870ead3fd..5e5d4a9abe63 100644
> --- a/drivers/iio/chemical/vz89x.c
> +++ b/drivers/iio/chemical/vz89x.c
> @@ -16,7 +16,6 @@
>   */
>  
>  #include <linux/module.h>
> -#include <linux/mutex.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
>  #include <linux/of.h>
> @@ -52,7 +51,6 @@ struct vz89x_chip_data;
>  struct vz89x_data {
>  	struct i2c_client *client;
>  	const struct vz89x_chip_data *chip;
> -	struct mutex lock;
>  	int (*xfer)(struct vz89x_data *data, u8 cmd);
>  
>  	bool is_valid;
> @@ -277,26 +275,32 @@ static int vz89x_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&data->lock);
> -		ret = vz89x_get_measurement(data);
> -		mutex_unlock(&data->lock);
> -
> +		ret = iio_device_claim_direct_mode(indio_dev);
>  		if (ret)
>  			return ret;
>  
> +		ret = vz89x_get_measurement(data);
> +		if (ret)
> +			goto error_out;
> +
>  		switch (chan->type) {
>  		case IIO_CONCENTRATION:
>  			*val = data->buffer[chan->address];
> -			return IIO_VAL_INT;
> +			ret = IIO_VAL_INT;
> +			break;
>  		case IIO_RESISTANCE:
>  			ret = vz89x_get_resistance_reading(data, chan, val);
>  			if (!ret)
> -				return IIO_VAL_INT;
> +				ret = IIO_VAL_INT;
>  			break;
>  		default:
> -			return -EINVAL;
> +			ret = -EINVAL;
>  		}
> -		break;
> +
> +error_out:
> +		iio_device_release_direct_mode(indio_dev);
> +
> +		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_RESISTANCE:
> @@ -390,7 +394,6 @@ static int vz89x_probe(struct i2c_client *client,
>  	data->client = client;
>  	data->chip = &vz89x_chips[chip_id];
>  	data->last_update = jiffies - HZ;
> -	mutex_init(&data->lock);
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &vz89x_info,
> 

Re: [PATCH v2 3/4] iio: chemical: vz89x: prevent corrupted buffer from being read

[Jonathan Cameron]

On 25/08/16 07:44, Matt Ranostay wrote:
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
Applied to the togreg branch of iio.git.
Initially pushed out as testing for the autobuilders to
play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/chemical/vz89x.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
> index 289d2919deea..cd3870ead3fd 100644
> --- a/drivers/iio/chemical/vz89x.c
> +++ b/drivers/iio/chemical/vz89x.c
> @@ -55,6 +55,7 @@ struct vz89x_data {
>  	struct mutex lock;
>  	int (*xfer)(struct vz89x_data *data, u8 cmd);
>  
> +	bool is_valid;
>  	unsigned long last_update;
>  	u8 buffer[VZ89TE_REG_MEASUREMENT_RD_SIZE];
>  };
> @@ -229,7 +230,10 @@ static int vz89x_get_measurement(struct vz89x_data *data)
>  
>  	/* sensor can only be polled once a second max per datasheet */
>  	if (!time_after(jiffies, data->last_update + HZ))
> -		return 0;
> +		return data->is_valid ? 0 : -EAGAIN;
> +
> +	data->is_valid = false;
> +	data->last_update = jiffies;
>  
>  	ret = data->xfer(data, chip->cmd);
>  	if (ret < 0)
> @@ -239,7 +243,7 @@ static int vz89x_get_measurement(struct vz89x_data *data)
>  	if (ret)
>  		return -EAGAIN;
>  
> -	data->last_update = jiffies;
> +	data->is_valid = true;
>  
>  	return 0;
>  }
>