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

[PATCH 0/3] 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.

Matt Ranostay (3):
  iio: chemical: vz89x: abstract chip configuration
  iio: chemical: vz89x: add support for VZ89TE part
  iio: chemical: vz89x: prevent corrupted buffer from being read

 drivers/iio/chemical/vz89x.c | 175 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 144 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH 1/3] 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 | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
index 652649da500f..f498228e044d 100644
--- a/drivers/iio/chemical/vz89x.c
+++ b/drivers/iio/chemical/vz89x.c
@@ -31,12 +31,21 @@
 #define VZ89X_VOC_TVOC_IDX		2
 #define VZ89X_VOC_RESISTANCE_IDX	3
 
+enum {
+	VZ89X,
+};
+
 struct vz89x_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	int (*xfer)(struct vz89x_data *data, u8 cmd);
+	int (*valid)(struct vz89x_data *data);
 
 	unsigned long last_update;
+	u8 cmd;
+	u8 write_size;
+	u8 read_size;
+
 	u8 buffer[VZ89X_REG_MEASUREMENT_SIZE];
 };
 
@@ -98,7 +107,7 @@ static int 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->read_size - 1] > 0);
 }
 
 static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
@@ -110,12 +119,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 = data->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 = data->read_size;
 	msg[1].buf = (char *) &data->buffer;
 
 	ret = i2c_transfer(client->adapter, msg, 2);
@@ -133,7 +142,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->read_size; i++) {
 		ret = i2c_smbus_read_byte(client);
 		if (ret < 0)
 			return ret;
@@ -151,11 +160,11 @@ static int vz89x_get_measurement(struct vz89x_data *data)
 	if (!time_after(jiffies, data->last_update + HZ))
 		return 0;
 
-	ret = data->xfer(data, VZ89X_REG_MEASUREMENT);
+	ret = data->xfer(data, data->cmd);
 	if (ret < 0)
 		return ret;
 
-	ret = vz89x_measurement_is_valid(data);
+	ret = data->valid(data);
 	if (ret)
 		return -EAGAIN;
 
@@ -254,6 +263,10 @@ static int vz89x_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
 	data->last_update = jiffies - HZ;
+	data->cmd = VZ89X_REG_MEASUREMENT;
+	data->write_size = 3;
+	data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
+	data->valid = &vz89x_measurement_is_valid;
 	mutex_init(&data->lock);
 
 	indio_dev->dev.parent = &client->dev;
@@ -268,13 +281,13 @@ static int vz89x_probe(struct i2c_client *client,
 }
 
 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" },
+	{ .compatible = "sgx,vz89x", .data = (void *) VZ89X },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
-- 
2.7.4

[PATCH 2/3] 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 | 149 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 123 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
index f498228e044d..0347761ebdba 100644
--- a/drivers/iio/chemical/vz89x.c
+++ b/drivers/iio/chemical/vz89x.c
@@ -19,20 +19,33 @@
 #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 VZ89TE_REG_MEASUREMENT		0x0c
+
+#define VZ89X_REG_WRITE_SIZE		3
+#define VZ89TE_REG_WRITE_SIZE		6
+
 #define VZ89X_REG_MEASUREMENT_SIZE	6
+#define VZ89TE_REG_MEASUREMENT_SIZE	7
 
 #define VZ89X_VOC_CO2_IDX		0
 #define VZ89X_VOC_SHORT_IDX		1
 #define VZ89X_VOC_TVOC_IDX		2
 #define VZ89X_VOC_RESISTANCE_IDX	3
 
+#define VZ89TE_VOC_TVOC_IDX		0
+#define VZ89TE_VOC_CO2_IDX		1
+#define VZ89TE_VOC_RESISTANCE_IDX	2
+
 enum {
 	VZ89X,
+	VZ89TE,
 };
 
 struct vz89x_data {
@@ -79,6 +92,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,
+		},
 	},
 };
 
@@ -110,12 +157,27 @@ static int vz89x_measurement_is_valid(struct vz89x_data *data)
 	return !!(data->buffer[data->read_size - 1] > 0);
 }
 
+/* VZ89TE device has a modified CRC-8 two complement check */
+static int vz89te_measurement_is_valid(struct vz89x_data *data)
+{
+	u8 crc = 0;
+	int i, sum = 0;
+
+	for (i = 0; i < (data->read_size - 1); i++) {
+		sum = crc + data->buffer[i];
+		crc = sum;
+		crc += sum / 256;
+	}
+
+	return !((0xff - crc) == data->buffer[data->read_size - 1]);
+}
+
 static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
 {
 	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;
@@ -173,11 +235,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];
+	u32 tmp = *((u32 *) ((u8 *) &data->buffer[chan->address]));
+
+	switch (chan->scan_type.endianness) {
+	case IIO_LE:
+		*val = le32_to_cpu(tmp) & GENMASK(23, 0);
+		break;
+	case IIO_BE:
+		*val = be32_to_cpu(tmp) >> 8;
+		break;
+	default:
+		return -EINVAL;
+	}
 
-	return buf[0] | (buf[1] << 8);
+	return 0;
 }
 
 static int vz89x_read_raw(struct iio_dev *indio_dev,
@@ -196,15 +271,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;
 		}
@@ -219,12 +294,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:
@@ -241,11 +316,20 @@ static const struct iio_info vz89x_info = {
 	.driver_module	= THIS_MODULE,
 };
 
+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);
+
 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)
@@ -260,13 +344,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->last_update = jiffies - HZ;
-	data->cmd = VZ89X_REG_MEASUREMENT;
-	data->write_size = 3;
-	data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
-	data->valid = &vz89x_measurement_is_valid;
 	mutex_init(&data->lock);
 
 	indio_dev->dev.parent = &client->dev;
@@ -274,24 +360,35 @@ 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);
+	switch (chip_id) {
+	case VZ89X:
+		indio_dev->channels = vz89x_channels;
+		indio_dev->num_channels = ARRAY_SIZE(vz89x_channels);
+		data->cmd = VZ89X_REG_MEASUREMENT;
+		data->write_size = VZ89X_REG_WRITE_SIZE;
+		data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
+		data->valid = &vz89x_measurement_is_valid;
+		break;
+	case VZ89TE:
+		indio_dev->channels = vz89te_channels;
+		indio_dev->num_channels = ARRAY_SIZE(vz89te_channels);
+		data->cmd = VZ89TE_REG_MEASUREMENT;
+		data->write_size = VZ89TE_REG_WRITE_SIZE;
+		data->read_size = VZ89TE_REG_MEASUREMENT_SIZE;
+		data->valid = &vz89te_measurement_is_valid;
+		break;
+	}
 
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static const struct i2c_device_id vz89x_id[] = {
 	{ "vz89x", VZ89X },
+	{ "vz89te", VZ89TE },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, vz89x_id);
 
-static const struct of_device_id vz89x_dt_ids[] = {
-	{ .compatible = "sgx,vz89x", .data = (void *) VZ89X },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
-
 static struct i2c_driver vz89x_driver = {
 	.driver = {
 		.name	= "vz89x",
-- 
2.7.4

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

[Peter Meerwald-Stadler]

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

comments below
 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>  drivers/iio/chemical/vz89x.c | 149 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 123 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
> index f498228e044d..0347761ebdba 100644
> --- a/drivers/iio/chemical/vz89x.c
> +++ b/drivers/iio/chemical/vz89x.c
> @@ -19,20 +19,33 @@
>  #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 VZ89TE_REG_MEASUREMENT		0x0c
> +
> +#define VZ89X_REG_WRITE_SIZE		3
> +#define VZ89TE_REG_WRITE_SIZE		6
> +
>  #define VZ89X_REG_MEASUREMENT_SIZE	6
> +#define VZ89TE_REG_MEASUREMENT_SIZE	7
>  
>  #define VZ89X_VOC_CO2_IDX		0
>  #define VZ89X_VOC_SHORT_IDX		1
>  #define VZ89X_VOC_TVOC_IDX		2
>  #define VZ89X_VOC_RESISTANCE_IDX	3
>  
> +#define VZ89TE_VOC_TVOC_IDX		0
> +#define VZ89TE_VOC_CO2_IDX		1
> +#define VZ89TE_VOC_RESISTANCE_IDX	2
> +
>  enum {
>  	VZ89X,
> +	VZ89TE,
>  };
>  
>  struct vz89x_data {
> @@ -79,6 +92,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 = {

scan_type with a scan_index of -1 is unexpected

> +			.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,
> +		},
>  	},
>  };
>  
> @@ -110,12 +157,27 @@ static int vz89x_measurement_is_valid(struct vz89x_data *data)
>  	return !!(data->buffer[data->read_size - 1] > 0);
>  }
>  
> +/* VZ89TE device has a modified CRC-8 two complement check */
> +static int vz89te_measurement_is_valid(struct vz89x_data *data)

return bool maybe?

> +{
> +	u8 crc = 0;
> +	int i, sum = 0;
> +
> +	for (i = 0; i < (data->read_size - 1); i++) {
> +		sum = crc + data->buffer[i];
> +		crc = sum;
> +		crc += sum / 256;
> +	}
> +
> +	return !((0xff - crc) == data->buffer[data->read_size - 1]);
> +}
> +
>  static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
>  {
>  	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;
> @@ -173,11 +235,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];
> +	u32 tmp = *((u32 *) ((u8 *) &data->buffer[chan->address]));
> +
> +	switch (chan->scan_type.endianness) {
> +	case IIO_LE:
> +		*val = le32_to_cpu(tmp) & GENMASK(23, 0);

could use le32_to_cpup() probably

> +		break;
> +	case IIO_BE:
> +		*val = be32_to_cpu(tmp) >> 8;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
> -	return buf[0] | (buf[1] << 8);
> +	return 0;
>  }
>  
>  static int vz89x_read_raw(struct iio_dev *indio_dev,
> @@ -196,15 +271,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;
>  		}
> @@ -219,12 +294,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:
> @@ -241,11 +316,20 @@ static const struct iio_info vz89x_info = {
>  	.driver_module	= THIS_MODULE,
>  };
>  
> +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);
> +
>  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)
> @@ -260,13 +344,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->last_update = jiffies - HZ;
> -	data->cmd = VZ89X_REG_MEASUREMENT;
> -	data->write_size = 3;
> -	data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
> -	data->valid = &vz89x_measurement_is_valid;
>  	mutex_init(&data->lock);
>  
>  	indio_dev->dev.parent = &client->dev;
> @@ -274,24 +360,35 @@ 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);
> +	switch (chip_id) {
> +	case VZ89X:
> +		indio_dev->channels = vz89x_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(vz89x_channels);

other drivers often use a constant chip_info struct which is used to 
declare chip variants (less code, more data)

> +		data->cmd = VZ89X_REG_MEASUREMENT;
> +		data->write_size = VZ89X_REG_WRITE_SIZE;
> +		data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
> +		data->valid = &vz89x_measurement_is_valid;
> +		break;
> +	case VZ89TE:
> +		indio_dev->channels = vz89te_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(vz89te_channels);
> +		data->cmd = VZ89TE_REG_MEASUREMENT;
> +		data->write_size = VZ89TE_REG_WRITE_SIZE;
> +		data->read_size = VZ89TE_REG_MEASUREMENT_SIZE;
> +		data->valid = &vz89te_measurement_is_valid;
> +		break;
> +	}
>  
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  
>  static const struct i2c_device_id vz89x_id[] = {
>  	{ "vz89x", VZ89X },
> +	{ "vz89te", VZ89TE },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, vz89x_id);
>  
> -static const struct of_device_id vz89x_dt_ids[] = {
> -	{ .compatible = "sgx,vz89x", .data = (void *) VZ89X },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
> -
>  static struct i2c_driver vz89x_driver = {
>  	.driver = {
>  		.name	= "vz89x",
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

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

[Matt Ranostay]

On Mon, Aug 22, 2016 at 10:42 PM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>
>> Add support the VZ89TE variant which removes the voc_short channel,
>> and has CRC check for data transactions.
>
> comments below
>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>  drivers/iio/chemical/vz89x.c | 149 +++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 123 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
>> index f498228e044d..0347761ebdba 100644
>> --- a/drivers/iio/chemical/vz89x.c
>> +++ b/drivers/iio/chemical/vz89x.c
>> @@ -19,20 +19,33 @@
>>  #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 VZ89TE_REG_MEASUREMENT               0x0c
>> +
>> +#define VZ89X_REG_WRITE_SIZE         3
>> +#define VZ89TE_REG_WRITE_SIZE                6
>> +
>>  #define VZ89X_REG_MEASUREMENT_SIZE   6
>> +#define VZ89TE_REG_MEASUREMENT_SIZE  7
>>
>>  #define VZ89X_VOC_CO2_IDX            0
>>  #define VZ89X_VOC_SHORT_IDX          1
>>  #define VZ89X_VOC_TVOC_IDX           2
>>  #define VZ89X_VOC_RESISTANCE_IDX     3
>>
>> +#define VZ89TE_VOC_TVOC_IDX          0
>> +#define VZ89TE_VOC_CO2_IDX           1
>> +#define VZ89TE_VOC_RESISTANCE_IDX    2
>> +
>>  enum {
>>       VZ89X,
>> +     VZ89TE,
>>  };
>>
>>  struct vz89x_data {
>> @@ -79,6 +92,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 = {
>
> scan_type with a scan_index of -1 is unexpected

Isn't that the typical usage to signal a non bufffered channel?.
But guess the lack of storage, real bits, and buffered trigger
register would signal that anyway.

>
>> +                     .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,
>> +             },
>>       },
>>  };
>>
>> @@ -110,12 +157,27 @@ static int vz89x_measurement_is_valid(struct vz89x_data *data)
>>       return !!(data->buffer[data->read_size - 1] > 0);
>>  }
>>
>> +/* VZ89TE device has a modified CRC-8 two complement check */
>> +static int vz89te_measurement_is_valid(struct vz89x_data *data)
>
> return bool maybe?

Sure why not.
>
>> +{
>> +     u8 crc = 0;
>> +     int i, sum = 0;
>> +
>> +     for (i = 0; i < (data->read_size - 1); i++) {
>> +             sum = crc + data->buffer[i];
>> +             crc = sum;
>> +             crc += sum / 256;
>> +     }
>> +
>> +     return !((0xff - crc) == data->buffer[data->read_size - 1]);
>> +}
>> +
>>  static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
>>  {
>>       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;
>> @@ -173,11 +235,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];
>> +     u32 tmp = *((u32 *) ((u8 *) &data->buffer[chan->address]));
>> +
>> +     switch (chan->scan_type.endianness) {
>> +     case IIO_LE:
>> +             *val = le32_to_cpu(tmp) & GENMASK(23, 0);
>
> could use le32_to_cpup() probably
>
>> +             break;
>> +     case IIO_BE:
>> +             *val = be32_to_cpu(tmp) >> 8;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>>
>> -     return buf[0] | (buf[1] << 8);
>> +     return 0;
>>  }
>>
>>  static int vz89x_read_raw(struct iio_dev *indio_dev,
>> @@ -196,15 +271,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;
>>               }
>> @@ -219,12 +294,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:
>> @@ -241,11 +316,20 @@ static const struct iio_info vz89x_info = {
>>       .driver_module  = THIS_MODULE,
>>  };
>>
>> +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);
>> +
>>  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)
>> @@ -260,13 +344,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->last_update = jiffies - HZ;
>> -     data->cmd = VZ89X_REG_MEASUREMENT;
>> -     data->write_size = 3;
>> -     data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
>> -     data->valid = &vz89x_measurement_is_valid;
>>       mutex_init(&data->lock);
>>
>>       indio_dev->dev.parent = &client->dev;
>> @@ -274,24 +360,35 @@ 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);
>> +     switch (chip_id) {
>> +     case VZ89X:
>> +             indio_dev->channels = vz89x_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(vz89x_channels);
>
> other drivers often use a constant chip_info struct which is used to
> declare chip variants (less code, more data)
>
>> +             data->cmd = VZ89X_REG_MEASUREMENT;
>> +             data->write_size = VZ89X_REG_WRITE_SIZE;
>> +             data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
>> +             data->valid = &vz89x_measurement_is_valid;
>> +             break;
>> +     case VZ89TE:
>> +             indio_dev->channels = vz89te_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(vz89te_channels);
>> +             data->cmd = VZ89TE_REG_MEASUREMENT;
>> +             data->write_size = VZ89TE_REG_WRITE_SIZE;
>> +             data->read_size = VZ89TE_REG_MEASUREMENT_SIZE;
>> +             data->valid = &vz89te_measurement_is_valid;
>> +             break;
>> +     }
>>
>>       return devm_iio_device_register(&client->dev, indio_dev);
>>  }
>>
>>  static const struct i2c_device_id vz89x_id[] = {
>>       { "vz89x", VZ89X },
>> +     { "vz89te", VZ89TE },
>>       { }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, vz89x_id);
>>
>> -static const struct of_device_id vz89x_dt_ids[] = {
>> -     { .compatible = "sgx,vz89x", .data = (void *) VZ89X },
>> -     { }
>> -};
>> -MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
>> -
>>  static struct i2c_driver vz89x_driver = {
>>       .driver = {
>>               .name   = "vz89x",
>>
>
> --
>
> Peter Meerwald-Stadler
> +43-664-2444418 (mobile)

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

[Peter Meerwald-Stadler]

> >>  struct vz89x_data {
> >> @@ -79,6 +92,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 = {
> >
> > scan_type with a scan_index of -1 is unexpected
> 
> Isn't that the typical usage to signal a non bufffered channel?.
> But guess the lack of storage, real bits, and buffered trigger
> register would signal that anyway.

scan_index == -1 signals that scan_type is not used for buffered reads, 
but then scan_type is used to store endianness

one *could* want a check/invariant that either 
scan_index >= 0 and scan_type must be provided, and
scan_index == -1 and scan_type must NOT be provided

no bit deal, maybe there is an easy way to store the channel endianness 
elsewhere; maybe a chip_info struct?

I haven't checked (yet) what other drivers do in this regards... 

p.

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

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

[Matt Ranostay]

On Tue, Aug 23, 2016 at 1:10 AM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>
>> >>  struct vz89x_data {
>> >> @@ -79,6 +92,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 = {
>> >
>> > scan_type with a scan_index of -1 is unexpected
>>
>> Isn't that the typical usage to signal a non bufffered channel?.
>> But guess the lack of storage, real bits, and buffered trigger
>> register would signal that anyway.
>
> scan_index == -1 signals that scan_type is not used for buffered reads,
> but then scan_type is used to store endianness
>
> one *could* want a check/invariant that either
> scan_index >= 0 and scan_type must be provided, and
> scan_index == -1 and scan_type must NOT be provided
>
> no bit deal, maybe there is an easy way to store the channel endianness
> elsewhere; maybe a chip_info struct?
>
> I haven't checked (yet) what other drivers do in this regards...

No real reason to do it here but since the struct already exists to
store this bit of information... maybe Jonathan has feedback either
way..

>
> p.
>
> --
>
> Peter Meerwald-Stadler
> +43-664-2444418 (mobile)

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

[Jonathan Cameron]

On 23/08/16 09:43, Matt Ranostay wrote:
> On Tue, Aug 23, 2016 at 1:10 AM, Peter Meerwald-Stadler
> <pmeerw@pmeerw.net> wrote:
>>
>>>>>  struct vz89x_data {
>>>>> @@ -79,6 +92,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 = {
>>>>
>>>> scan_type with a scan_index of -1 is unexpected
>>>
>>> Isn't that the typical usage to signal a non bufffered channel?.
>>> But guess the lack of storage, real bits, and buffered trigger
>>> register would signal that anyway.
>>
>> scan_index == -1 signals that scan_type is not used for buffered reads,
>> but then scan_type is used to store endianness
>>
>> one *could* want a check/invariant that either
>> scan_index >= 0 and scan_type must be provided, and
This first one could indeed be done if anyone wanted to.
It not being present and getting to a submission would rather signal
a lack of testing with standard tools, but I guess this could happen
by 'accident' if someone adds something like a temperature channel
and only ever checks the sysfs interface.
>> scan_index == -1 and scan_type must NOT be provided
Not so keen on this one.
>>
>> no bit deal, maybe there is an easy way to store the channel endianness
>> elsewhere; maybe a chip_info struct?
>>
>> I haven't checked (yet) what other drivers do in this regards...
> 
> No real reason to do it here but since the struct already exists to
> store this bit of information... maybe Jonathan has feedback either
> way..
It's fine to use it for this. Quite a few drivers IIRC have
done this in the past.


> 
>>
>> p.
>>
>> --
>>
>> Peter Meerwald-Stadler
>> +43-664-2444418 (mobile)

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

[Matt Ranostay]

On Mon, Aug 22, 2016 at 10:42 PM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>
>> Add support the VZ89TE variant which removes the voc_short channel,
>> and has CRC check for data transactions.
>
> comments below
>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>  drivers/iio/chemical/vz89x.c | 149 +++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 123 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
>> index f498228e044d..0347761ebdba 100644
>> --- a/drivers/iio/chemical/vz89x.c
>> +++ b/drivers/iio/chemical/vz89x.c
>> @@ -19,20 +19,33 @@
>>  #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 VZ89TE_REG_MEASUREMENT               0x0c
>> +
>> +#define VZ89X_REG_WRITE_SIZE         3
>> +#define VZ89TE_REG_WRITE_SIZE                6
>> +
>>  #define VZ89X_REG_MEASUREMENT_SIZE   6
>> +#define VZ89TE_REG_MEASUREMENT_SIZE  7
>>
>>  #define VZ89X_VOC_CO2_IDX            0
>>  #define VZ89X_VOC_SHORT_IDX          1
>>  #define VZ89X_VOC_TVOC_IDX           2
>>  #define VZ89X_VOC_RESISTANCE_IDX     3
>>
>> +#define VZ89TE_VOC_TVOC_IDX          0
>> +#define VZ89TE_VOC_CO2_IDX           1
>> +#define VZ89TE_VOC_RESISTANCE_IDX    2
>> +
>>  enum {
>>       VZ89X,
>> +     VZ89TE,
>>  };
>>
>>  struct vz89x_data {
>> @@ -79,6 +92,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 = {
>
> scan_type with a scan_index of -1 is unexpected
>
>> +                     .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,
>> +             },
>>       },
>>  };
>>
>> @@ -110,12 +157,27 @@ static int vz89x_measurement_is_valid(struct vz89x_data *data)
>>       return !!(data->buffer[data->read_size - 1] > 0);
>>  }
>>
>> +/* VZ89TE device has a modified CRC-8 two complement check */
>> +static int vz89te_measurement_is_valid(struct vz89x_data *data)
>
> return bool maybe?
>
>> +{
>> +     u8 crc = 0;
>> +     int i, sum = 0;
>> +
>> +     for (i = 0; i < (data->read_size - 1); i++) {
>> +             sum = crc + data->buffer[i];
>> +             crc = sum;
>> +             crc += sum / 256;
>> +     }
>> +
>> +     return !((0xff - crc) == data->buffer[data->read_size - 1]);
>> +}
>> +
>>  static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
>>  {
>>       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;
>> @@ -173,11 +235,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];
>> +     u32 tmp = *((u32 *) ((u8 *) &data->buffer[chan->address]));
>> +
>> +     switch (chan->scan_type.endianness) {
>> +     case IIO_LE:
>> +             *val = le32_to_cpu(tmp) & GENMASK(23, 0);
>
> could use le32_to_cpup() probably
>
>> +             break;
>> +     case IIO_BE:
>> +             *val = be32_to_cpu(tmp) >> 8;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>>
>> -     return buf[0] | (buf[1] << 8);
>> +     return 0;
>>  }
>>
>>  static int vz89x_read_raw(struct iio_dev *indio_dev,
>> @@ -196,15 +271,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;
>>               }
>> @@ -219,12 +294,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:
>> @@ -241,11 +316,20 @@ static const struct iio_info vz89x_info = {
>>       .driver_module  = THIS_MODULE,
>>  };
>>
>> +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);
>> +
>>  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)
>> @@ -260,13 +344,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->last_update = jiffies - HZ;
>> -     data->cmd = VZ89X_REG_MEASUREMENT;
>> -     data->write_size = 3;
>> -     data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
>> -     data->valid = &vz89x_measurement_is_valid;
>>       mutex_init(&data->lock);
>>
>>       indio_dev->dev.parent = &client->dev;
>> @@ -274,24 +360,35 @@ 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);
>> +     switch (chip_id) {
>> +     case VZ89X:
>> +             indio_dev->channels = vz89x_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(vz89x_channels);
>
> other drivers often use a constant chip_info struct which is used to
> declare chip variants (less code, more data)

Yeah probably should have done this.. will fix in v2

>
>> +             data->cmd = VZ89X_REG_MEASUREMENT;
>> +             data->write_size = VZ89X_REG_WRITE_SIZE;
>> +             data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
>> +             data->valid = &vz89x_measurement_is_valid;
>> +             break;
>> +     case VZ89TE:
>> +             indio_dev->channels = vz89te_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(vz89te_channels);
>> +             data->cmd = VZ89TE_REG_MEASUREMENT;
>> +             data->write_size = VZ89TE_REG_WRITE_SIZE;
>> +             data->read_size = VZ89TE_REG_MEASUREMENT_SIZE;
>> +             data->valid = &vz89te_measurement_is_valid;
>> +             break;
>> +     }
>>
>>       return devm_iio_device_register(&client->dev, indio_dev);
>>  }
>>
>>  static const struct i2c_device_id vz89x_id[] = {
>>       { "vz89x", VZ89X },
>> +     { "vz89te", VZ89TE },
>>       { }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, vz89x_id);
>>
>> -static const struct of_device_id vz89x_dt_ids[] = {
>> -     { .compatible = "sgx,vz89x", .data = (void *) VZ89X },
>> -     { }
>> -};
>> -MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
>> -
>>  static struct i2c_driver vz89x_driver = {
>>       .driver = {
>>               .name   = "vz89x",
>>
>
> --
>
> Peter Meerwald-Stadler
> +43-664-2444418 (mobile)

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

[Matt Ranostay]

On Mon, Aug 22, 2016 at 10:42 PM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>
>> Add support the VZ89TE variant which removes the voc_short channel,
>> and has CRC check for data transactions.
>
> comments below
>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>  drivers/iio/chemical/vz89x.c | 149 +++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 123 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
>> index f498228e044d..0347761ebdba 100644
>> --- a/drivers/iio/chemical/vz89x.c
>> +++ b/drivers/iio/chemical/vz89x.c
>> @@ -19,20 +19,33 @@
>>  #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 VZ89TE_REG_MEASUREMENT               0x0c
>> +
>> +#define VZ89X_REG_WRITE_SIZE         3
>> +#define VZ89TE_REG_WRITE_SIZE                6
>> +
>>  #define VZ89X_REG_MEASUREMENT_SIZE   6
>> +#define VZ89TE_REG_MEASUREMENT_SIZE  7
>>
>>  #define VZ89X_VOC_CO2_IDX            0
>>  #define VZ89X_VOC_SHORT_IDX          1
>>  #define VZ89X_VOC_TVOC_IDX           2
>>  #define VZ89X_VOC_RESISTANCE_IDX     3
>>
>> +#define VZ89TE_VOC_TVOC_IDX          0
>> +#define VZ89TE_VOC_CO2_IDX           1
>> +#define VZ89TE_VOC_RESISTANCE_IDX    2
>> +
>>  enum {
>>       VZ89X,
>> +     VZ89TE,
>>  };
>>
>>  struct vz89x_data {
>> @@ -79,6 +92,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 = {
>
> scan_type with a scan_index of -1 is unexpected
>
>> +                     .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,
>> +             },
>>       },
>>  };
>>
>> @@ -110,12 +157,27 @@ static int vz89x_measurement_is_valid(struct vz89x_data *data)
>>       return !!(data->buffer[data->read_size - 1] > 0);
>>  }
>>
>> +/* VZ89TE device has a modified CRC-8 two complement check */
>> +static int vz89te_measurement_is_valid(struct vz89x_data *data)
>
> return bool maybe?
>
>> +{
>> +     u8 crc = 0;
>> +     int i, sum = 0;
>> +
>> +     for (i = 0; i < (data->read_size - 1); i++) {
>> +             sum = crc + data->buffer[i];
>> +             crc = sum;
>> +             crc += sum / 256;
>> +     }
>> +
>> +     return !((0xff - crc) == data->buffer[data->read_size - 1]);
>> +}
>> +
>>  static int vz89x_i2c_xfer(struct vz89x_data *data, u8 cmd)
>>  {
>>       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;
>> @@ -173,11 +235,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];
>> +     u32 tmp = *((u32 *) ((u8 *) &data->buffer[chan->address]));
>> +
>> +     switch (chan->scan_type.endianness) {
>> +     case IIO_LE:
>> +             *val = le32_to_cpu(tmp) & GENMASK(23, 0);
>
> could use le32_to_cpup() probably

Except we need to apply the mask anyway...

>
>> +             break;
>> +     case IIO_BE:
>> +             *val = be32_to_cpu(tmp) >> 8;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>>
>> -     return buf[0] | (buf[1] << 8);
>> +     return 0;
>>  }
>>
>>  static int vz89x_read_raw(struct iio_dev *indio_dev,
>> @@ -196,15 +271,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;
>>               }
>> @@ -219,12 +294,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:
>> @@ -241,11 +316,20 @@ static const struct iio_info vz89x_info = {
>>       .driver_module  = THIS_MODULE,
>>  };
>>
>> +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);
>> +
>>  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)
>> @@ -260,13 +344,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->last_update = jiffies - HZ;
>> -     data->cmd = VZ89X_REG_MEASUREMENT;
>> -     data->write_size = 3;
>> -     data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
>> -     data->valid = &vz89x_measurement_is_valid;
>>       mutex_init(&data->lock);
>>
>>       indio_dev->dev.parent = &client->dev;
>> @@ -274,24 +360,35 @@ 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);
>> +     switch (chip_id) {
>> +     case VZ89X:
>> +             indio_dev->channels = vz89x_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(vz89x_channels);
>
> other drivers often use a constant chip_info struct which is used to
> declare chip variants (less code, more data)
>
>> +             data->cmd = VZ89X_REG_MEASUREMENT;
>> +             data->write_size = VZ89X_REG_WRITE_SIZE;
>> +             data->read_size = VZ89X_REG_MEASUREMENT_SIZE;
>> +             data->valid = &vz89x_measurement_is_valid;
>> +             break;
>> +     case VZ89TE:
>> +             indio_dev->channels = vz89te_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(vz89te_channels);
>> +             data->cmd = VZ89TE_REG_MEASUREMENT;
>> +             data->write_size = VZ89TE_REG_WRITE_SIZE;
>> +             data->read_size = VZ89TE_REG_MEASUREMENT_SIZE;
>> +             data->valid = &vz89te_measurement_is_valid;
>> +             break;
>> +     }
>>
>>       return devm_iio_device_register(&client->dev, indio_dev);
>>  }
>>
>>  static const struct i2c_device_id vz89x_id[] = {
>>       { "vz89x", VZ89X },
>> +     { "vz89te", VZ89TE },
>>       { }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, vz89x_id);
>>
>> -static const struct of_device_id vz89x_dt_ids[] = {
>> -     { .compatible = "sgx,vz89x", .data = (void *) VZ89X },
>> -     { }
>> -};
>> -MODULE_DEVICE_TABLE(of, vz89x_dt_ids);
>> -
>>  static struct i2c_driver vz89x_driver = {
>>       .driver = {
>>               .name   = "vz89x",
>>
>
> --
>
> Peter Meerwald-Stadler
> +43-664-2444418 (mobile)

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

[Peter Meerwald-Stadler]

> >> -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];
> >> +     u32 tmp = *((u32 *) ((u8 *) &data->buffer[chan->address]));
> >> +
> >> +     switch (chan->scan_type.endianness) {
> >> +     case IIO_LE:
> >> +             *val = le32_to_cpu(tmp) & GENMASK(23, 0);
> >
> > could use le32_to_cpup() probably
> 
> Except we need to apply the mask anyway...

but you may save the ugly casting

p. 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

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

[Matt Ranostay]

On Tue, Aug 23, 2016 at 9:23 PM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>
>> >> -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];
>> >> +     u32 tmp = *((u32 *) ((u8 *) &data->buffer[chan->address]));
>> >> +
>> >> +     switch (chan->scan_type.endianness) {
>> >> +     case IIO_LE:
>> >> +             *val = le32_to_cpu(tmp) & GENMASK(23, 0);
>> >
>> > could use le32_to_cpup() probably
>>
>> Except we need to apply the mask anyway...
>
> but you may save the ugly casting
>

Ah right but wouldn't for big endian case still require it?.....

> p.
>
> --
>
> Peter Meerwald-Stadler
> +43-664-2444418 (mobile)

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

[Matt Ranostay]

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

diff --git a/drivers/iio/chemical/vz89x.c b/drivers/iio/chemical/vz89x.c
index 0347761ebdba..c2a311c59190 100644
--- a/drivers/iio/chemical/vz89x.c
+++ b/drivers/iio/chemical/vz89x.c
@@ -54,6 +54,7 @@ struct vz89x_data {
 	int (*xfer)(struct vz89x_data *data, u8 cmd);
 	int (*valid)(struct vz89x_data *data);
 
+	bool is_valid;
 	unsigned long last_update;
 	u8 cmd;
 	u8 write_size;
@@ -220,8 +221,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, data->cmd);
 	if (ret < 0)
 		return ret;
@@ -230,7 +233,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