[lm-sensors] [RFC PATCH 1/3] hwmon: sht15: add checksum validation

[lm-sensors] [RFC PATCH 1/3] hwmon: sht15: add checksum validation

[Vivien Didelot]

From: Jerome Oufella <jerome.oufella@savoirfairelinux.com>

The sht15 sensor allows validating exchages to and from the device using
a crc8 function. Two utility functions to read individual bytes from
the device and reverse a byte have also been added.

Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
To: lm-sensors@lm-sensors.org
Cc: khali@linux-fr.org
---
 drivers/hwmon/sht15.c |  194 +++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 179 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
index 2c586fc..83822a6 100644
--- a/drivers/hwmon/sht15.c
+++ b/drivers/hwmon/sht15.c
@@ -1,6 +1,9 @@
 /*
  * sht15.c - support for the SHT15 Temperature and Humidity Sensor
  *
+ * Portions Copyright (c) 2010-2011 Savoir-faire Linux Inc.
+ *          Jerome Oufella <jerome.oufella@savoirfairelinux.com>
+ *
  * Copyright (c) 2009 Jonathan Cameron
  *
  * Copyright (c) 2007 Wouter Horre
@@ -9,8 +12,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  *
- * Currently ignoring checksum on readings.
  * Default resolution only (14bit temp, 12bit humidity)
+ * Checksum validation can be enabled via the checksumming sysfs attribute
  * Ignoring battery status.
  * Heater not enabled.
  * Timings are all conservative.
@@ -39,8 +42,9 @@
 #include <linux/slab.h>
 #include <asm/atomic.h>
 
-#define SHT15_MEASURE_TEMP	3
-#define SHT15_MEASURE_RH	5
+#define SHT15_MEASURE_TEMP	0x03
+#define SHT15_MEASURE_RH	0x05
+#define SHT15_SOFT_RESET	0x1E
 
 #define SHT15_READING_NOTHING	0
 #define SHT15_READING_TEMP	1
@@ -51,6 +55,9 @@
 #define SHT15_TSCKH		100	/* clock high */
 #define SHT15_TSU		150	/* data setup time */
 
+/* Min timings in msecs */
+#define SHT15_TSRST		11	/* soft reset time */
+
 /**
  * struct sht15_temppair - elements of voltage dependant temp calc
  * @vdd:	supply voltage in microvolts
@@ -72,6 +79,34 @@ static const struct sht15_temppair temppoints[] = {
 	{ 5000000, -40100 },
 };
 
+/* Table from crc data sheet, section 2.4 */
+static const u8 sht15_crc8_table[] = {
+	0x00, 0x31, 0x62, 0x53, 0xC4, 0xF5, 0xA6, 0x97,	0xB9, 0x88, 0xDB,
+	0xEA, 0x7D, 0x4C, 0x1F, 0x2E, 0x43, 0x72, 0x21, 0x10, 0x87, 0xB6,
+	0xE5, 0xD4, 0xFA, 0xCB, 0x98, 0xA9, 0x3E, 0x0F, 0x5C, 0x6D, 0x86,
+	0xB7, 0xE4, 0xD5, 0x42, 0x73, 0x20, 0x11, 0x3F, 0x0E, 0x5D, 0x6C,
+	0xFB, 0xCA, 0x99, 0xA8, 0xC5, 0xF4, 0xA7, 0x96, 0x01, 0x30, 0x63,
+	0x52, 0x7C, 0x4D, 0x1E, 0x2F, 0xB8, 0x89, 0xDA, 0xEB, 0x3D, 0x0C,
+	0x5F, 0x6E, 0xF9, 0xC8, 0x9B, 0xAA, 0x84, 0xB5, 0xE6, 0xD7, 0x40,
+	0x71, 0x22, 0x13, 0x7E, 0x4F, 0x1C, 0x2D, 0xBA, 0x8B, 0xD8, 0xE9,
+	0xC7, 0xF6, 0xA5, 0x94, 0x03, 0x32, 0x61, 0x50, 0xBB, 0x8A, 0xD9,
+	0xE8, 0x7F, 0x4E, 0x1D, 0x2C, 0x02, 0x33, 0x60, 0x51, 0xC6, 0xF7,
+	0xA4, 0x95, 0xF8, 0xC9, 0x9A, 0xAB, 0x3C, 0x0D, 0x5E, 0x6F, 0x41,
+	0x70, 0x23, 0x12, 0x85, 0xB4, 0xE7, 0xD6, 0x7A, 0x4B, 0x18, 0x29,
+	0xBE, 0x8F, 0xDC, 0xED, 0xC3, 0xF2, 0xA1, 0x90, 0x07, 0x36, 0x65,
+	0x54, 0x39, 0x08, 0x5B, 0x6A, 0xFD, 0xCC, 0x9F, 0xAE, 0x80, 0xB1,
+	0xE2, 0xD3, 0x44, 0x75, 0x26, 0x17, 0xFC, 0xCD, 0x9E, 0xAF, 0x38,
+	0x09, 0x5A, 0x6B, 0x45, 0x74, 0x27, 0x16, 0x81, 0xB0, 0xE3, 0xD2,
+	0xBF, 0x8E, 0xDD, 0xEC, 0x7B, 0x4A, 0x19, 0x28, 0x06, 0x37, 0x64,
+	0x55, 0xC2, 0xF3, 0xA0, 0x91, 0x47, 0x76, 0x25, 0x14, 0x83, 0xB2,
+	0xE1, 0xD0, 0xFE, 0xCF, 0x9C, 0xAD, 0x3A, 0x0B, 0x58, 0x69, 0x04,
+	0x35, 0x66, 0x57, 0xC0, 0xF1, 0xA2, 0x93, 0xBD, 0x8C, 0xDF, 0xEE,
+	0x79, 0x48, 0x1B, 0x2A, 0xC1, 0xF0, 0xA3, 0x92, 0x05, 0x34, 0x67,
+	0x56, 0x78, 0x49, 0x1A, 0x2B, 0xBC, 0x8D, 0xDE, 0xEF, 0x82, 0xB3,
+	0xE0, 0xD1, 0x46, 0x77, 0x24, 0x15, 0x3B, 0x0A, 0x59, 0x68, 0xFF,
+	0xCE, 0x9D, 0xAC
+};
+
 /**
  * struct sht15_data - device instance specific data
  * @pdata:	platform data (gpio's etc)
@@ -79,6 +114,7 @@ static const struct sht15_temppair temppoints[] = {
  * @wait_queue:	wait queue for getting values from device
  * @val_temp:	last temperature value read from device
  * @val_humid: 	last humidity value read from device
+ * @checksum_ok:last value read from device passed checksum verification
  * @flag:	status flag used to identify what the last request was
  * @valid:	are the current stored values valid (start condition)
  * @last_updat:	time of last update
@@ -95,6 +131,7 @@ static const struct sht15_temppair temppoints[] = {
  *		based upon it will be invalid.
  * @update_supply_work:	work struct that is used to update the supply_uV
  * @interrupt_handled:	flag used to indicate a hander has been scheduled
+ * @checksumming:	flag used to indicate the data checksum must be validated
  */
 struct sht15_data {
 	struct sht15_platform_data	*pdata;
@@ -102,6 +139,7 @@ struct sht15_data {
 	wait_queue_head_t		wait_queue;
 	uint16_t			val_temp;
 	uint16_t			val_humid;
+	u8				checksum_ok;
 	u8				flag;
 	u8				valid;
 	unsigned long			last_updat;
@@ -114,8 +152,46 @@ struct sht15_data {
 	int				supply_uV_valid;
 	struct work_struct		update_supply_work;
 	atomic_t			interrupt_handled;
+	u8				checksumming;
 };
 
+static u8 sht15_read_byte(struct sht15_data *);
+static void sht15_ack(struct sht15_data *);
+static void sht15_end_transmission(struct sht15_data *);
+
+/**
+ * reverse() - reverse a byte
+ * @byte:    byte to reverse.
+ *
+ * stolen from drivers/lirc/lirc_i2c.c
+ */
+static u8 reverse(u8 byte)
+{
+	u8 i, c;
+
+	for (c = 0, i = 0; i < 8; i++)
+		c |= ((byte & (1 << i)) ? 1 : 0) << (7 - i);
+	return c;
+}
+
+/**
+ * sht15_crc8() - compute crc8
+ * @data:	sht15 retrieved data
+ *
+ * This implements section 2 of the crc data sheet
+ */
+static u8 sht15_crc8(const unsigned char *data, unsigned char len)
+{
+	unsigned char crc = 0;
+
+	while (len--) {
+		crc = sht15_crc8_table[*data ^ crc];
+		data++;
+	}
+
+	return crc;
+}
+
 /**
  * sht15_connection_reset() - reset the comms interface
  * @data:	sht15 specific data
@@ -229,6 +305,19 @@ static int sht15_send_cmd(struct sht15_data *data, u8 cmd)
 	ret = sht15_wait_for_response(data);
 	return ret;
 }
+
+/**
+ * sht15_soft_reset() - send a soft reset command
+ * @data:	sht15 specific data
+ *
+ * As described in section 3.2 of the data sheet
+ */
+static inline void sht15_soft_reset(struct sht15_data *data)
+{
+	sht15_send_cmd(data, SHT15_SOFT_RESET);
+	msleep(SHT15_TSRST);
+}
+
 /**
  * sht15_update_single_val() - get a new value from device
  * @data:		device instance specific data
@@ -263,6 +352,17 @@ static inline int sht15_update_single_val(struct sht15_data *data,
 		sht15_connection_reset(data);
 		return -ETIME;
 	}
+
+	/*
+	 *  Perform checksum validation on the received data.
+	 *  Specification mentions that in case a checksum verification fails,
+	 *  a soft reset command must be sent to the device.
+	 */
+	if (data->checksumming && !data->checksum_ok) {
+		sht15_soft_reset(data);
+		return -EAGAIN;
+	}
+
 	return 0;
 }
 
@@ -366,8 +466,36 @@ static ssize_t sht15_show_humidity(struct device *dev,
 	ret = sht15_update_vals(data);
 
 	return ret ? ret : sprintf(buf, "%d\n", sht15_calc_humid(data));
+}
+
+static ssize_t sht15_show_checksum(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct sht15_data *data = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", data->checksumming);
+}
+
+static ssize_t sht15_store_checksum(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct sht15_data *data = dev_get_drvdata(dev);
+	long value;
+
+	mutex_lock(&data->read_lock);
+	if (strict_strtol(buf, 10, &value)) {
+		mutex_unlock(&data->read_lock);
+		return -EINVAL;
+	}
+
+	data->checksumming = !!value;
+	mutex_unlock(&data->read_lock);
+
+	return count;
+}
 
-};
 static ssize_t show_name(struct device *dev,
 			 struct device_attribute *attr,
 			 char *buf)
@@ -382,10 +510,15 @@ static SENSOR_DEVICE_ATTR(temp1_input,
 static SENSOR_DEVICE_ATTR(humidity1_input,
 			  S_IRUGO, sht15_show_humidity,
 			  NULL, 0);
+static DEVICE_ATTR(checksumming,
+			  S_IRUGO | S_IWUSR,
+			  sht15_show_checksum,
+			  sht15_store_checksum);
 static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
 static struct attribute *sht15_attrs[] = {
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_humidity1_input.dev_attr.attr,
+	&dev_attr_checksumming.attr,
 	&dev_attr_name.attr,
 	NULL,
 };
@@ -437,13 +570,35 @@ static void sht15_end_transmission(struct sht15_data *data)
 	ndelay(SHT15_TSCKL);
 }
 
-static void sht15_bh_read_data(struct work_struct *work_s)
+/**
+ * sht15_read_byte() - Read a byte back from the device
+ * @data:	device state.
+ */
+static u8 sht15_read_byte(struct sht15_data *data)
 {
 	int i;
+	u8 byte = 0;
+
+	for (i = 0; i < 8; ++i) {
+		byte <<= 1;
+		gpio_set_value(data->pdata->gpio_sck, 1);
+		ndelay(SHT15_TSCKH);
+		byte |= !!gpio_get_value(data->pdata->gpio_data);
+		gpio_set_value(data->pdata->gpio_sck, 0);
+		ndelay(SHT15_TSCKL);
+	}
+	return byte;
+}
+
+static void sht15_bh_read_data(struct work_struct *work_s)
+{
 	uint16_t val = 0;
+	u8 dev_checksum = 0;
+	u8 checksum_values[3];
 	struct sht15_data *data
 		= container_of(work_s, struct sht15_data,
 			       read_work);
+
 	/* Firstly, verify the line is low */
 	if (gpio_get_value(data->pdata->gpio_data)) {
 		/* If not, then start the interrupt again - care
@@ -458,16 +613,24 @@ static void sht15_bh_read_data(struct work_struct *work_s)
 			return;
 	}
 	/* Read the data back from the device */
-	for (i = 0; i < 16; ++i) {
-		val <<= 1;
-		gpio_set_value(data->pdata->gpio_sck, 1);
-		ndelay(SHT15_TSCKH);
-		val |= !!gpio_get_value(data->pdata->gpio_data);
-		gpio_set_value(data->pdata->gpio_sck, 0);
-		ndelay(SHT15_TSCKL);
-		if (i = 7)
-			sht15_ack(data);
+	val = sht15_read_byte(data);
+	val <<= 8;
+	sht15_ack(data);
+	val |= sht15_read_byte(data);
+
+	if (data->checksumming) {
+		/**
+		 *  Ask the device for a checksum and read it back.
+		 *  Note: the device sends the checksum byte reversed.
+		 */
+		sht15_ack(data);
+		dev_checksum = reverse(sht15_read_byte(data));
+		checksum_values[0] = (data->flag = SHT15_READING_TEMP) ? SHT15_MEASURE_TEMP : SHT15_MEASURE_RH;
+		checksum_values[1] = (u8) (val >> 8);
+		checksum_values[2] = (u8) val;
+		data->checksum_ok = (sht15_crc8(checksum_values, 3) = dev_checksum);
 	}
+
 	/* Tell the device we are done */
 	sht15_end_transmission(data);
 
@@ -538,6 +701,7 @@ static int __devinit sht15_probe(struct platform_device *pdev)
 	}
 	data->pdata = pdev->dev.platform_data;
 	data->supply_uV = data->pdata->supply_mv*1000;
+	data->checksumming = 1; /* Verify checksums by default */
 
 /* If a regulator is available, query what the supply voltage actually is!*/
 	data->reg = regulator_get(data->dev, "vcc");
@@ -583,7 +747,7 @@ static int __devinit sht15_probe(struct platform_device *pdev)
 	}
 	disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
 	sht15_connection_reset(data);
-	sht15_send_cmd(data, 0x1E);
+	sht15_soft_reset(data);
 
 	data->hwmon_dev = hwmon_device_register(data->dev);
 	if (IS_ERR(data->hwmon_dev)) {
-- 
1.7.1


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 1/3] hwmon: sht15: add checksum

[Jonathan Cameron]

On 03/16/11 18:44, Vivien Didelot wrote:
> From: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> 
> The sht15 sensor allows validating exchages
typo.
> to and from the device using
> a crc8 function. Two utility functions to read individual bytes from
> the device and reverse a byte have also been added.
This setting should definitely be configured via platform data.
If there is reason to have it on for a given device, that is unlikely
to only be true sometimes!

Otherwise, more or less fine.  A few minor suggestions inline.
> 
> Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> To: lm-sensors@lm-sensors.org
> Cc: khali@linux-fr.org
> ---
>  drivers/hwmon/sht15.c |  194 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 179 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 2c586fc..83822a6 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -1,6 +1,9 @@
>  /*
>   * sht15.c - support for the SHT15 Temperature and Humidity Sensor
>   *
> + * Portions Copyright (c) 2010-2011 Savoir-faire Linux Inc.
> + *          Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> + *
>   * Copyright (c) 2009 Jonathan Cameron
>   *
>   * Copyright (c) 2007 Wouter Horre
> @@ -9,8 +12,8 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   *
> - * Currently ignoring checksum on readings.
>   * Default resolution only (14bit temp, 12bit humidity)
> + * Checksum validation can be enabled via the checksumming sysfs attribute
>   * Ignoring battery status.
>   * Heater not enabled.
>   * Timings are all conservative.
> @@ -39,8 +42,9 @@
>  #include <linux/slab.h>
>  #include <asm/atomic.h>
>  
> -#define SHT15_MEASURE_TEMP	3
> -#define SHT15_MEASURE_RH	5
> +#define SHT15_MEASURE_TEMP	0x03
> +#define SHT15_MEASURE_RH	0x05
> +#define SHT15_SOFT_RESET	0x1E
>  
>  #define SHT15_READING_NOTHING	0
>  #define SHT15_READING_TEMP	1
> @@ -51,6 +55,9 @@
>  #define SHT15_TSCKH		100	/* clock high */
>  #define SHT15_TSU		150	/* data setup time */
>  
> +/* Min timings in msecs */
> +#define SHT15_TSRST		11	/* soft reset time */
> +
>  /**
>   * struct sht15_temppair - elements of voltage dependant temp calc
>   * @vdd:	supply voltage in microvolts
> @@ -72,6 +79,34 @@ static const struct sht15_temppair temppoints[] = {
>  	{ 5000000, -40100 },
>  };
>  
> +/* Table from crc data sheet, section 2.4 */
I'll just assume you got this right. The table google gave me was in
decimal and I'm feeling lazy.
> +static const u8 sht15_crc8_table[] = {
> +	0x00, 0x31, 0x62, 0x53, 0xC4, 0xF5, 0xA6, 0x97,	0xB9, 0x88, 0xDB,
> +	0xEA, 0x7D, 0x4C, 0x1F, 0x2E, 0x43, 0x72, 0x21, 0x10, 0x87, 0xB6,
> +	0xE5, 0xD4, 0xFA, 0xCB, 0x98, 0xA9, 0x3E, 0x0F, 0x5C, 0x6D, 0x86,
> +	0xB7, 0xE4, 0xD5, 0x42, 0x73, 0x20, 0x11, 0x3F, 0x0E, 0x5D, 0x6C,
> +	0xFB, 0xCA, 0x99, 0xA8, 0xC5, 0xF4, 0xA7, 0x96, 0x01, 0x30, 0x63,
> +	0x52, 0x7C, 0x4D, 0x1E, 0x2F, 0xB8, 0x89, 0xDA, 0xEB, 0x3D, 0x0C,
> +	0x5F, 0x6E, 0xF9, 0xC8, 0x9B, 0xAA, 0x84, 0xB5, 0xE6, 0xD7, 0x40,
> +	0x71, 0x22, 0x13, 0x7E, 0x4F, 0x1C, 0x2D, 0xBA, 0x8B, 0xD8, 0xE9,
> +	0xC7, 0xF6, 0xA5, 0x94, 0x03, 0x32, 0x61, 0x50, 0xBB, 0x8A, 0xD9,
> +	0xE8, 0x7F, 0x4E, 0x1D, 0x2C, 0x02, 0x33, 0x60, 0x51, 0xC6, 0xF7,
> +	0xA4, 0x95, 0xF8, 0xC9, 0x9A, 0xAB, 0x3C, 0x0D, 0x5E, 0x6F, 0x41,
> +	0x70, 0x23, 0x12, 0x85, 0xB4, 0xE7, 0xD6, 0x7A, 0x4B, 0x18, 0x29,
> +	0xBE, 0x8F, 0xDC, 0xED, 0xC3, 0xF2, 0xA1, 0x90, 0x07, 0x36, 0x65,
> +	0x54, 0x39, 0x08, 0x5B, 0x6A, 0xFD, 0xCC, 0x9F, 0xAE, 0x80, 0xB1,
> +	0xE2, 0xD3, 0x44, 0x75, 0x26, 0x17, 0xFC, 0xCD, 0x9E, 0xAF, 0x38,
> +	0x09, 0x5A, 0x6B, 0x45, 0x74, 0x27, 0x16, 0x81, 0xB0, 0xE3, 0xD2,
> +	0xBF, 0x8E, 0xDD, 0xEC, 0x7B, 0x4A, 0x19, 0x28, 0x06, 0x37, 0x64,
> +	0x55, 0xC2, 0xF3, 0xA0, 0x91, 0x47, 0x76, 0x25, 0x14, 0x83, 0xB2,
> +	0xE1, 0xD0, 0xFE, 0xCF, 0x9C, 0xAD, 0x3A, 0x0B, 0x58, 0x69, 0x04,
> +	0x35, 0x66, 0x57, 0xC0, 0xF1, 0xA2, 0x93, 0xBD, 0x8C, 0xDF, 0xEE,
> +	0x79, 0x48, 0x1B, 0x2A, 0xC1, 0xF0, 0xA3, 0x92, 0x05, 0x34, 0x67,
> +	0x56, 0x78, 0x49, 0x1A, 0x2B, 0xBC, 0x8D, 0xDE, 0xEF, 0x82, 0xB3,
> +	0xE0, 0xD1, 0x46, 0x77, 0x24, 0x15, 0x3B, 0x0A, 0x59, 0x68, 0xFF,
> +	0xCE, 0x9D, 0xAC
> +};
> +
>  /**
>   * struct sht15_data - device instance specific data
>   * @pdata:	platform data (gpio's etc)
> @@ -79,6 +114,7 @@ static const struct sht15_temppair temppoints[] = {
>   * @wait_queue:	wait queue for getting values from device
>   * @val_temp:	last temperature value read from device
>   * @val_humid: 	last humidity value read from device
> + * @checksum_ok:last value read from device passed checksum verification
>   * @flag:	status flag used to identify what the last request was
>   * @valid:	are the current stored values valid (start condition)
>   * @last_updat:	time of last update
> @@ -95,6 +131,7 @@ static const struct sht15_temppair temppoints[] = {
>   *		based upon it will be invalid.
>   * @update_supply_work:	work struct that is used to update the supply_uV
>   * @interrupt_handled:	flag used to indicate a hander has been scheduled
> + * @checksumming:	flag used to indicate the data checksum must be validated
>   */
>  struct sht15_data {
>  	struct sht15_platform_data	*pdata;
> @@ -102,6 +139,7 @@ struct sht15_data {
>  	wait_queue_head_t		wait_queue;
>  	uint16_t			val_temp;
>  	uint16_t			val_humid;
> +	u8				checksum_ok;
>  	u8				flag;
>  	u8				valid;
>  	unsigned long			last_updat;
> @@ -114,8 +152,46 @@ struct sht15_data {
>  	int				supply_uV_valid;
>  	struct work_struct		update_supply_work;
>  	atomic_t			interrupt_handled;
> +	u8				checksumming;
>  };
>  
Please don't introduce unwanted function prototypes.  If you need
to reorganise the code so they aren't needed.

> +static u8 sht15_read_byte(struct sht15_data *);
> +static void sht15_ack(struct sht15_data *);
> +static void sht15_end_transmission(struct sht15_data *);
> +
> +/**
> + * reverse() - reverse a byte
> + * @byte:    byte to reverse.
> + *
> + * stolen from drivers/lirc/lirc_i2c.c
> + */
> +static u8 reverse(u8 byte)
> +{
> +	u8 i, c;
> +
> +	for (c = 0, i = 0; i < 8; i++)

Ternary operatures in the middle something like this are really
really hard to read!
How about...
  c |= (!!(byte & (1 << i))) << (7 - i);
> +		c |= ((byte & (1 << i)) ? 1 : 0) << (7 - i);
> +	return c;
> +}
> +
> +/**
> + * sht15_crc8() - compute crc8
> + * @data:	sht15 retrieved data
> + *
> + * This implements section 2 of the crc data sheet
> + */
> +static u8 sht15_crc8(const unsigned char *data, unsigned char len)
> +{
> +	unsigned char crc = 0;
> +
> +	while (len--) {
> +		crc = sht15_crc8_table[*data ^ crc];
> +		data++;
> +	}
> +
> +	return crc;
> +}
> +
>  /**
>   * sht15_connection_reset() - reset the comms interface
>   * @data:	sht15 specific data
> @@ -229,6 +305,19 @@ static int sht15_send_cmd(struct sht15_data *data, u8 cmd)
>  	ret = sht15_wait_for_response(data);
>  	return ret;
>  }
> +
> +/**
> + * sht15_soft_reset() - send a soft reset command
> + * @data:	sht15 specific data
> + *
> + * As described in section 3.2 of the data sheet
> + */
> +static inline void sht15_soft_reset(struct sht15_data *data)
> +{
> +	sht15_send_cmd(data, SHT15_SOFT_RESET);
> +	msleep(SHT15_TSRST);
> +}
> +
>  /**
>   * sht15_update_single_val() - get a new value from device
>   * @data:		device instance specific data
> @@ -263,6 +352,17 @@ static inline int sht15_update_single_val(struct sht15_data *data,
>  		sht15_connection_reset(data);
>  		return -ETIME;
>  	}
> +
> +	/*
> +	 *  Perform checksum validation on the received data.
> +	 *  Specification mentions that in case a checksum verification fails,
> +	 *  a soft reset command must be sent to the device.
> +	 */
> +	if (data->checksumming && !data->checksum_ok) {
> +		sht15_soft_reset(data);
> +		return -EAGAIN;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -366,8 +466,36 @@ static ssize_t sht15_show_humidity(struct device *dev,
>  	ret = sht15_update_vals(data);
>  
>  	return ret ? ret : sprintf(buf, "%d\n", sht15_calc_humid(data));
> +}
> +
As stated above, platform data please, not sysfs for this.
> +static ssize_t sht15_show_checksum(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct sht15_data *data = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", data->checksumming);
> +}
> +
> +static ssize_t sht15_store_checksum(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct sht15_data *data = dev_get_drvdata(dev);
> +	long value;
> +
> +	mutex_lock(&data->read_lock);
> +	if (strict_strtol(buf, 10, &value)) {
> +		mutex_unlock(&data->read_lock);
> +		return -EINVAL;
> +	}
> +
> +	data->checksumming = !!value;
> +	mutex_unlock(&data->read_lock);
> +
> +	return count;
> +}
>  
> -};
>  static ssize_t show_name(struct device *dev,
>  			 struct device_attribute *attr,
>  			 char *buf)
> @@ -382,10 +510,15 @@ static SENSOR_DEVICE_ATTR(temp1_input,
>  static SENSOR_DEVICE_ATTR(humidity1_input,
>  			  S_IRUGO, sht15_show_humidity,
>  			  NULL, 0);
> +static DEVICE_ATTR(checksumming,
> +			  S_IRUGO | S_IWUSR,
> +			  sht15_show_checksum,
> +			  sht15_store_checksum);
>  static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>  static struct attribute *sht15_attrs[] = {
>  	&sensor_dev_attr_temp1_input.dev_attr.attr,
>  	&sensor_dev_attr_humidity1_input.dev_attr.attr,
> +	&dev_attr_checksumming.attr,
>  	&dev_attr_name.attr,
>  	NULL,
>  };
> @@ -437,13 +570,35 @@ static void sht15_end_transmission(struct sht15_data *data)
>  	ndelay(SHT15_TSCKL);
>  }
>  
> -static void sht15_bh_read_data(struct work_struct *work_s)
> +/**
> + * sht15_read_byte() - Read a byte back from the device
> + * @data:	device state.
> + */
> +static u8 sht15_read_byte(struct sht15_data *data)
>  {
>  	int i;
> +	u8 byte = 0;
> +
> +	for (i = 0; i < 8; ++i) {
> +		byte <<= 1;
> +		gpio_set_value(data->pdata->gpio_sck, 1);
> +		ndelay(SHT15_TSCKH);
> +		byte |= !!gpio_get_value(data->pdata->gpio_data);
> +		gpio_set_value(data->pdata->gpio_sck, 0);
> +		ndelay(SHT15_TSCKL);
> +	}
> +	return byte;
> +}
> +
> +static void sht15_bh_read_data(struct work_struct *work_s)
> +{
>  	uint16_t val = 0;
> +	u8 dev_checksum = 0;
> +	u8 checksum_values[3];
>  	struct sht15_data *data
>  		= container_of(work_s, struct sht15_data,
>  			       read_work);
> +
>  	/* Firstly, verify the line is low */
>  	if (gpio_get_value(data->pdata->gpio_data)) {
>  		/* If not, then start the interrupt again - care
> @@ -458,16 +613,24 @@ static void sht15_bh_read_data(struct work_struct *work_s)
>  			return;
>  	}
>  	/* Read the data back from the device */
> -	for (i = 0; i < 16; ++i) {
> -		val <<= 1;
> -		gpio_set_value(data->pdata->gpio_sck, 1);
> -		ndelay(SHT15_TSCKH);
> -		val |= !!gpio_get_value(data->pdata->gpio_data);
> -		gpio_set_value(data->pdata->gpio_sck, 0);
> -		ndelay(SHT15_TSCKL);
> -		if (i = 7)
> -			sht15_ack(data);
> +	val = sht15_read_byte(data);
> +	val <<= 8;
> +	sht15_ack(data);
> +	val |= sht15_read_byte(data);
> +
> +	if (data->checksumming) {
> +		/**
> +		 *  Ask the device for a checksum and read it back.
> +		 *  Note: the device sends the checksum byte reversed.
> +		 */
> +		sht15_ack(data);
> +		dev_checksum = reverse(sht15_read_byte(data));
That looks rather over 80 characters. Please run checkpatch.pl on this
before resending.
> +		checksum_values[0] = (data->flag = SHT15_READING_TEMP) ? SHT15_MEASURE_TEMP : SHT15_MEASURE_RH;
> +		checksum_values[1] = (u8) (val >> 8);
> +		checksum_values[2] = (u8) val;
> +		data->checksum_ok = (sht15_crc8(checksum_values, 3) = dev_checksum);
>  	}
> +
>  	/* Tell the device we are done */
>  	sht15_end_transmission(data);
>  
> @@ -538,6 +701,7 @@ static int __devinit sht15_probe(struct platform_device *pdev)
>  	}
>  	data->pdata = pdev->dev.platform_data;
>  	data->supply_uV = data->pdata->supply_mv*1000;
No. That changes the default.  By all means allow this to be turned on, but
making it the default may effect existing users.
> +	data->checksumming = 1; /* Verify checksums by default */
>  
>  /* If a regulator is available, query what the supply voltage actually is!*/
>  	data->reg = regulator_get(data->dev, "vcc");
> @@ -583,7 +747,7 @@ static int __devinit sht15_probe(struct platform_device *pdev)
>  	}
>  	disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
>  	sht15_connection_reset(data);
> -	sht15_send_cmd(data, 0x1E);
> +	sht15_soft_reset(data);
>  
>  	data->hwmon_dev = hwmon_device_register(data->dev);
>  	if (IS_ERR(data->hwmon_dev)) {


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 1/3] hwmon: sht15: add checksum

[Vivien Didelot]

Thanks for your review Jonathan.
On Mon, Mar 21, 2011 at 07:28:58PM +0000, Jonathan Cameron wrote:
> > to and from the device using
> > a crc8 function. Two utility functions to read individual bytes from
> > the device and reverse a byte have also been added.
> This setting should definitely be configured via platform data.
> If there is reason to have it on for a given device, that is unlikely
> to only be true sometimes!
Yes, it will.
> 
> Otherwise, more or less fine.  A few minor suggestions inline.

> > +/* Table from crc data sheet, section 2.4 */
> I'll just assume you got this right. The table google gave me was in
> decimal and I'm feeling lazy.
This is right, it has been taken from the SHT15 CRC calculation paper.

> Please don't introduce unwanted function prototypes.  If you need
> to reorganise the code so they aren't needed.
> 
> > +static u8 sht15_read_byte(struct sht15_data *);
> > +static void sht15_ack(struct sht15_data *);
> > +static void sht15_end_transmission(struct sht15_data *);
Ok, I'll try to rearrange the code.

> Ternary operatures in the middle something like this are really
> really hard to read!
> How about...
>   c |= (!!(byte & (1 << i))) << (7 - i);
> > +		c |= ((byte & (1 << i)) ? 1 : 0) << (7 - i);
> > +	return c;
It works too, I'll update this if you think that's better.

> That looks rather over 80 characters. Please run checkpatch.pl on this
> before resending.
> > +		checksum_values[0] = (data->flag = SHT15_READING_TEMP) ? SHT15_MEASURE_TEMP : SHT15_MEASURE_RH;
> > +		checksum_values[1] = (u8) (val >> 8);
> > +		checksum_values[2] = (u8) val;
> > +		data->checksum_ok = (sht15_crc8(checksum_values, 3) = dev_checksum);
Ok.

> No. That changes the default.  By all means allow this to be turned on, but
> making it the default may effect existing users.
> > +	data->checksumming = 1; /* Verify checksums by default */
That's true. It has been changed.

Regards,
Vivien.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 1/3] hwmon: sht15: add checksum

[Jonathan Cameron]

On 03/21/11 23:14, Vivien Didelot wrote:
> Thanks for your review Jonathan.
> On Mon, Mar 21, 2011 at 07:28:58PM +0000, Jonathan Cameron wrote:
>>> to and from the device using
>>> a crc8 function. Two utility functions to read individual bytes from
>>> the device and reverse a byte have also been added.
>> This setting should definitely be configured via platform data.
>> If there is reason to have it on for a given device, that is unlikely
>> to only be true sometimes!
> Yes, it will.
Sorry, you lost me.  It will be true that you want checksum's only sometimes?
If so please explain.
>>
>> Otherwise, more or less fine.  A few minor suggestions inline.
> 
>>> +/* Table from crc data sheet, section 2.4 */
>> I'll just assume you got this right. The table google gave me was in
>> decimal and I'm feeling lazy.
> This is right, it has been taken from the SHT15 CRC calculation paper.
Cool. I wasn't suggesting it wasn't, just admitting I was to lazy to
check there were no typos!
> 
>> Please don't introduce unwanted function prototypes.  If you need
>> to reorganise the code so they aren't needed.
>>
>>> +static u8 sht15_read_byte(struct sht15_data *);
>>> +static void sht15_ack(struct sht15_data *);
>>> +static void sht15_end_transmission(struct sht15_data *);
> Ok, I'll try to rearrange the code.
> 
>> Ternary operatures in the middle something like this are really
>> really hard to read!
>> How about...
>>   c |= (!!(byte & (1 << i))) << (7 - i);
>>> +		c |= ((byte & (1 << i)) ? 1 : 0) << (7 - i);
>>> +	return c;
> It works too, I'll update this if you think that's better.
They are both pretty hideous constructs and I guess the compiler may
well give the same output. Up to you.
> 
>> That looks rather over 80 characters. Please run checkpatch.pl on this
>> before resending.
>>> +		checksum_values[0] = (data->flag = SHT15_READING_TEMP) ? SHT15_MEASURE_TEMP : SHT15_MEASURE_RH;
>>> +		checksum_values[1] = (u8) (val >> 8);
>>> +		checksum_values[2] = (u8) val;
>>> +		data->checksum_ok = (sht15_crc8(checksum_values, 3) = dev_checksum);
> Ok.
> 
>> No. That changes the default.  By all means allow this to be turned on, but
>> making it the default may effect existing users.
>>> +	data->checksumming = 1; /* Verify checksums by default */
> That's true. It has been changed.

Thanks,

Jonathan

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 1/3] hwmon: sht15: add checksum

[Vivien Didelot]

On Tue, Mar 22, 2011 at 10:19:37AM +0000, Jonathan Cameron wrote:
> >> This setting should definitely be configured via platform data.
> >> If there is reason to have it on for a given device, that is unlikely
> >> to only be true sometimes!
> > Yes, it will.
> Sorry, you lost me.  It will be true that you want checksum's only sometimes?
> If so please explain.
Sorry, I meant that it will be configured via platform data, because as
we discussed, there's no need to change checksumming during the runtime.

Regards,
Vivien.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 1/3] hwmon: sht15: add checksum

[Jean Delvare]

On Wed, 16 Mar 2011 14:44:08 -0400, Vivien Didelot wrote:
> From: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> 
> The sht15 sensor allows validating exchages to and from the device using
> a crc8 function. Two utility functions to read individual bytes from
> the device and reverse a byte have also been added.
> 
> Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> To: lm-sensors@lm-sensors.org
> Cc: khali@linux-fr.org
> ---
>  drivers/hwmon/sht15.c |  194 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 179 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 2c586fc..83822a6 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -1,6 +1,9 @@
>  /*
>   * sht15.c - support for the SHT15 Temperature and Humidity Sensor
>   *
> + * Portions Copyright (c) 2010-2011 Savoir-faire Linux Inc.
> + *          Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> + *
>   * Copyright (c) 2009 Jonathan Cameron
>   *
>   * Copyright (c) 2007 Wouter Horre
> @@ -9,8 +12,8 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   *
> - * Currently ignoring checksum on readings.
>   * Default resolution only (14bit temp, 12bit humidity)
> + * Checksum validation can be enabled via the checksumming sysfs attribute
>   * Ignoring battery status.
>   * Heater not enabled.
>   * Timings are all conservative.
> @@ -39,8 +42,9 @@
>  #include <linux/slab.h>
>  #include <asm/atomic.h>
>  
> -#define SHT15_MEASURE_TEMP	3
> -#define SHT15_MEASURE_RH	5
> +#define SHT15_MEASURE_TEMP	0x03
> +#define SHT15_MEASURE_RH	0x05
> +#define SHT15_SOFT_RESET	0x1E
>  
>  #define SHT15_READING_NOTHING	0
>  #define SHT15_READING_TEMP	1
> @@ -51,6 +55,9 @@
>  #define SHT15_TSCKH		100	/* clock high */
>  #define SHT15_TSU		150	/* data setup time */
>  
> +/* Min timings in msecs */
> +#define SHT15_TSRST		11	/* soft reset time */
> +
>  /**
>   * struct sht15_temppair - elements of voltage dependant temp calc
>   * @vdd:	supply voltage in microvolts
> @@ -72,6 +79,34 @@ static const struct sht15_temppair temppoints[] = {
>  	{ 5000000, -40100 },
>  };
>  
> +/* Table from crc data sheet, section 2.4 */
> +static const u8 sht15_crc8_table[] = {

For this kind of table, you really want to put an explicit size (as the
code relies on it.) It is also a good practice to put 8 or 10 values per
line, so that the number of values can be checked immediately. While it
is correct that 11 * 23 + 3 = 256, it is more obvious that 8 * 32 = 256
or 10 * 25 + 6 = 256.

Another decision factor for the format of a CRC table is the way it
looks in the document you got it from. I've downloaded two versions of
the document, none has a table looking like yours. The main difference
is that they are all expressed in decimal (as Jonathan pointed out
already.) I fail to see the point of converting all the values to
hexadecimal in the driver. Now nobody can validate the values, all we
can do is hope that you got it right.

> +	0x00, 0x31, 0x62, 0x53, 0xC4, 0xF5, 0xA6, 0x97,	0xB9, 0x88, 0xDB,
> +	0xEA, 0x7D, 0x4C, 0x1F, 0x2E, 0x43, 0x72, 0x21, 0x10, 0x87, 0xB6,
> +	0xE5, 0xD4, 0xFA, 0xCB, 0x98, 0xA9, 0x3E, 0x0F, 0x5C, 0x6D, 0x86,
> +	0xB7, 0xE4, 0xD5, 0x42, 0x73, 0x20, 0x11, 0x3F, 0x0E, 0x5D, 0x6C,
> +	0xFB, 0xCA, 0x99, 0xA8, 0xC5, 0xF4, 0xA7, 0x96, 0x01, 0x30, 0x63,
> +	0x52, 0x7C, 0x4D, 0x1E, 0x2F, 0xB8, 0x89, 0xDA, 0xEB, 0x3D, 0x0C,
> +	0x5F, 0x6E, 0xF9, 0xC8, 0x9B, 0xAA, 0x84, 0xB5, 0xE6, 0xD7, 0x40,
> +	0x71, 0x22, 0x13, 0x7E, 0x4F, 0x1C, 0x2D, 0xBA, 0x8B, 0xD8, 0xE9,
> +	0xC7, 0xF6, 0xA5, 0x94, 0x03, 0x32, 0x61, 0x50, 0xBB, 0x8A, 0xD9,
> +	0xE8, 0x7F, 0x4E, 0x1D, 0x2C, 0x02, 0x33, 0x60, 0x51, 0xC6, 0xF7,
> +	0xA4, 0x95, 0xF8, 0xC9, 0x9A, 0xAB, 0x3C, 0x0D, 0x5E, 0x6F, 0x41,
> +	0x70, 0x23, 0x12, 0x85, 0xB4, 0xE7, 0xD6, 0x7A, 0x4B, 0x18, 0x29,
> +	0xBE, 0x8F, 0xDC, 0xED, 0xC3, 0xF2, 0xA1, 0x90, 0x07, 0x36, 0x65,
> +	0x54, 0x39, 0x08, 0x5B, 0x6A, 0xFD, 0xCC, 0x9F, 0xAE, 0x80, 0xB1,
> +	0xE2, 0xD3, 0x44, 0x75, 0x26, 0x17, 0xFC, 0xCD, 0x9E, 0xAF, 0x38,
> +	0x09, 0x5A, 0x6B, 0x45, 0x74, 0x27, 0x16, 0x81, 0xB0, 0xE3, 0xD2,
> +	0xBF, 0x8E, 0xDD, 0xEC, 0x7B, 0x4A, 0x19, 0x28, 0x06, 0x37, 0x64,
> +	0x55, 0xC2, 0xF3, 0xA0, 0x91, 0x47, 0x76, 0x25, 0x14, 0x83, 0xB2,
> +	0xE1, 0xD0, 0xFE, 0xCF, 0x9C, 0xAD, 0x3A, 0x0B, 0x58, 0x69, 0x04,
> +	0x35, 0x66, 0x57, 0xC0, 0xF1, 0xA2, 0x93, 0xBD, 0x8C, 0xDF, 0xEE,
> +	0x79, 0x48, 0x1B, 0x2A, 0xC1, 0xF0, 0xA3, 0x92, 0x05, 0x34, 0x67,
> +	0x56, 0x78, 0x49, 0x1A, 0x2B, 0xBC, 0x8D, 0xDE, 0xEF, 0x82, 0xB3,
> +	0xE0, 0xD1, 0x46, 0x77, 0x24, 0x15, 0x3B, 0x0A, 0x59, 0x68, 0xFF,
> +	0xCE, 0x9D, 0xAC
> +};
> +
>  /**
>   * struct sht15_data - device instance specific data
>   * @pdata:	platform data (gpio's etc)
> @@ -79,6 +114,7 @@ static const struct sht15_temppair temppoints[] = {
>   * @wait_queue:	wait queue for getting values from device
>   * @val_temp:	last temperature value read from device
>   * @val_humid: 	last humidity value read from device
> + * @checksum_ok:last value read from device passed checksum verification
>   * @flag:	status flag used to identify what the last request was
>   * @valid:	are the current stored values valid (start condition)
>   * @last_updat:	time of last update
> @@ -95,6 +131,7 @@ static const struct sht15_temppair temppoints[] = {
>   *		based upon it will be invalid.
>   * @update_supply_work:	work struct that is used to update the supply_uV
>   * @interrupt_handled:	flag used to indicate a hander has been scheduled
> + * @checksumming:	flag used to indicate the data checksum must be validated
>   */
>  struct sht15_data {
>  	struct sht15_platform_data	*pdata;
> @@ -102,6 +139,7 @@ struct sht15_data {
>  	wait_queue_head_t		wait_queue;
>  	uint16_t			val_temp;
>  	uint16_t			val_humid;
> +	u8				checksum_ok;
>  	u8				flag;
>  	u8				valid;
>  	unsigned long			last_updat;
> @@ -114,8 +152,46 @@ struct sht15_data {
>  	int				supply_uV_valid;
>  	struct work_struct		update_supply_work;
>  	atomic_t			interrupt_handled;
> +	u8				checksumming;
>  };
>  
> +static u8 sht15_read_byte(struct sht15_data *);
> +static void sht15_ack(struct sht15_data *);
> +static void sht15_end_transmission(struct sht15_data *);
> +
> +/**
> + * reverse() - reverse a byte
> + * @byte:    byte to reverse.
> + *
> + * stolen from drivers/lirc/lirc_i2c.c
> + */
> +static u8 reverse(u8 byte)
> +{
> +	u8 i, c;
> +
> +	for (c = 0, i = 0; i < 8; i++)
> +		c |= ((byte & (1 << i)) ? 1 : 0) << (7 - i);
> +	return c;
> +}
> +
> +/**
> + * sht15_crc8() - compute crc8
> + * @data:	sht15 retrieved data
> + *
> + * This implements section 2 of the crc data sheet
> + */
> +static u8 sht15_crc8(const unsigned char *data, unsigned char len)

You could make len an int, there is no benefit in limiting the maximum
length to 8 bits.

> +{
> +	unsigned char crc = 0;

This is inconsistent with the return type. Use unsigned char for both,
or u8 for both (preferably the latter, as the driver consistently uses
u8 everywhere.)

> +
> +	while (len--) {
> +		crc = sht15_crc8_table[*data ^ crc];
> +		data++;
> +	}
> +
> +	return crc;
> +}
> +
>  /**
>   * sht15_connection_reset() - reset the comms interface
>   * @data:	sht15 specific data
> @@ -229,6 +305,19 @@ static int sht15_send_cmd(struct sht15_data *data, u8 cmd)
>  	ret = sht15_wait_for_response(data);
>  	return ret;
>  }
> +
> +/**
> + * sht15_soft_reset() - send a soft reset command
> + * @data:	sht15 specific data
> + *
> + * As described in section 3.2 of the data sheet
> + */
> +static inline void sht15_soft_reset(struct sht15_data *data)
> +{
> +	sht15_send_cmd(data, SHT15_SOFT_RESET);
> +	msleep(SHT15_TSRST);
> +}
> +
>  /**
>   * sht15_update_single_val() - get a new value from device
>   * @data:		device instance specific data
> @@ -263,6 +352,17 @@ static inline int sht15_update_single_val(struct sht15_data *data,
>  		sht15_connection_reset(data);
>  		return -ETIME;
>  	}
> +
> +	/*
> +	 *  Perform checksum validation on the received data.
> +	 *  Specification mentions that in case a checksum verification fails,
> +	 *  a soft reset command must be sent to the device.
> +	 */
> +	if (data->checksumming && !data->checksum_ok) {
> +		sht15_soft_reset(data);
> +		return -EAGAIN;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -366,8 +466,36 @@ static ssize_t sht15_show_humidity(struct device *dev,
>  	ret = sht15_update_vals(data);
>  
>  	return ret ? ret : sprintf(buf, "%d\n", sht15_calc_humid(data));
> +}
> +
> +static ssize_t sht15_show_checksum(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct sht15_data *data = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", data->checksumming);
> +}
> +
> +static ssize_t sht15_store_checksum(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct sht15_data *data = dev_get_drvdata(dev);
> +	long value;
> +
> +	mutex_lock(&data->read_lock);
> +	if (strict_strtol(buf, 10, &value)) {
> +		mutex_unlock(&data->read_lock);
> +		return -EINVAL;
> +	}
> +
> +	data->checksumming = !!value;
> +	mutex_unlock(&data->read_lock);
> +
> +	return count;
> +}
>  
> -};
>  static ssize_t show_name(struct device *dev,
>  			 struct device_attribute *attr,
>  			 char *buf)
> @@ -382,10 +510,15 @@ static SENSOR_DEVICE_ATTR(temp1_input,
>  static SENSOR_DEVICE_ATTR(humidity1_input,
>  			  S_IRUGO, sht15_show_humidity,
>  			  NULL, 0);
> +static DEVICE_ATTR(checksumming,
> +			  S_IRUGO | S_IWUSR,
> +			  sht15_show_checksum,
> +			  sht15_store_checksum);
>  static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>  static struct attribute *sht15_attrs[] = {
>  	&sensor_dev_attr_temp1_input.dev_attr.attr,
>  	&sensor_dev_attr_humidity1_input.dev_attr.attr,
> +	&dev_attr_checksumming.attr,
>  	&dev_attr_name.attr,
>  	NULL,
>  };
> @@ -437,13 +570,35 @@ static void sht15_end_transmission(struct sht15_data *data)
>  	ndelay(SHT15_TSCKL);
>  }
>  
> -static void sht15_bh_read_data(struct work_struct *work_s)
> +/**
> + * sht15_read_byte() - Read a byte back from the device
> + * @data:	device state.
> + */
> +static u8 sht15_read_byte(struct sht15_data *data)
>  {
>  	int i;
> +	u8 byte = 0;
> +
> +	for (i = 0; i < 8; ++i) {
> +		byte <<= 1;
> +		gpio_set_value(data->pdata->gpio_sck, 1);
> +		ndelay(SHT15_TSCKH);
> +		byte |= !!gpio_get_value(data->pdata->gpio_data);
> +		gpio_set_value(data->pdata->gpio_sck, 0);
> +		ndelay(SHT15_TSCKL);
> +	}
> +	return byte;
> +}
> +
> +static void sht15_bh_read_data(struct work_struct *work_s)
> +{
>  	uint16_t val = 0;
> +	u8 dev_checksum = 0;
> +	u8 checksum_values[3];
>  	struct sht15_data *data
>  		= container_of(work_s, struct sht15_data,
>  			       read_work);
> +
>  	/* Firstly, verify the line is low */
>  	if (gpio_get_value(data->pdata->gpio_data)) {
>  		/* If not, then start the interrupt again - care
> @@ -458,16 +613,24 @@ static void sht15_bh_read_data(struct work_struct *work_s)
>  			return;
>  	}
>  	/* Read the data back from the device */
> -	for (i = 0; i < 16; ++i) {
> -		val <<= 1;
> -		gpio_set_value(data->pdata->gpio_sck, 1);
> -		ndelay(SHT15_TSCKH);
> -		val |= !!gpio_get_value(data->pdata->gpio_data);
> -		gpio_set_value(data->pdata->gpio_sck, 0);
> -		ndelay(SHT15_TSCKL);
> -		if (i = 7)
> -			sht15_ack(data);
> +	val = sht15_read_byte(data);
> +	val <<= 8;
> +	sht15_ack(data);
> +	val |= sht15_read_byte(data);
> +
> +	if (data->checksumming) {
> +		/**

Extra *.

> +		 *  Ask the device for a checksum and read it back.
> +		 *  Note: the device sends the checksum byte reversed.
> +		 */
> +		sht15_ack(data);
> +		dev_checksum = reverse(sht15_read_byte(data));
> +		checksum_values[0] = (data->flag = SHT15_READING_TEMP) ? SHT15_MEASURE_TEMP : SHT15_MEASURE_RH;
> +		checksum_values[1] = (u8) (val >> 8);
> +		checksum_values[2] = (u8) val;
> +		data->checksum_ok = (sht15_crc8(checksum_values, 3) = dev_checksum);
>  	}
> +
>  	/* Tell the device we are done */
>  	sht15_end_transmission(data);
>  
> @@ -538,6 +701,7 @@ static int __devinit sht15_probe(struct platform_device *pdev)
>  	}
>  	data->pdata = pdev->dev.platform_data;
>  	data->supply_uV = data->pdata->supply_mv*1000;
> +	data->checksumming = 1; /* Verify checksums by default */
>  
>  /* If a regulator is available, query what the supply voltage actually is!*/
>  	data->reg = regulator_get(data->dev, "vcc");
> @@ -583,7 +747,7 @@ static int __devinit sht15_probe(struct platform_device *pdev)
>  	}
>  	disable_irq_nosync(gpio_to_irq(data->pdata->gpio_data));
>  	sht15_connection_reset(data);
> -	sht15_send_cmd(data, 0x1E);
> +	sht15_soft_reset(data);

Not caused by this patch, but the probe function is racy: the end of
the initialization is done _after_ the sysfs attributes are created, so
users may access them before the driver is ready to honor the requests.
This should be fixed. Another bug in the probe function is that it
doesn't remove the sysfs attributes in the error path. I think some
regulator cleanups are missing as well in this path.

>  
>  	data->hwmon_dev = hwmon_device_register(data->dev);
>  	if (IS_ERR(data->hwmon_dev)) {


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors