Re: [lm-sensors] [PATCH 3/3] hwmon: (sht15) add checksum validation

From: Jonathan Cameron <jic23@cam.ac.uk>
To: lm-sensors@vger.kernel.org
Subject: Re: [lm-sensors] [PATCH 3/3] hwmon: (sht15) add checksum validation
Date: Fri, 08 Apr 2011 18:33:07 +0000	[thread overview]
Message-ID: <4D9F54E3.4080306@cam.ac.uk> (raw)
In-Reply-To: <1302198246-22212-4-git-send-email-vivien.didelot@savoirfairelinux.com>

On 04/07/11 18:44, Vivien Didelot wrote:
> From: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> 
> The sht15 sensor allows validating exchanges to and from the device using a
> crc8 function. An utility function to reverse a byte has also been added.
Couple of tiny nitpicks.
> 
> Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> ---
>  Documentation/hwmon/sht15 |    9 ++-
>  drivers/hwmon/sht15.c     |  219 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/sht15.h     |    2 +
>  3 files changed, 209 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/hwmon/sht15 b/Documentation/hwmon/sht15
> index 19cbfc7..ab71862 100644
> --- a/Documentation/hwmon/sht15
> +++ b/Documentation/hwmon/sht15
> @@ -5,6 +5,7 @@ Authors:
>    * Wouter Horre
>    * Jonathan Cameron
>    * Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> +  * Jerome Oufella <jerome.oufella@savoirfairelinux.com>
>  
>  Supported chips:
>    * Sensirion SHT10
> @@ -37,11 +38,17 @@ humidity, or 12 bits for temperature and 8 bits for humidity.
>  Some options may be set directly in the sht15_platform_data structure
>  or via sysfs attributes.
>  
> -Note: The regulator supply name is set to "vcc".
> +Note:
Notes ;)
> +  * The regulator supply name is set to "vcc".
> +  * If a CRC validation fails, a soft reset command is sent, which resets
> +    status register to its hardware default value, but the driver will try to
> +    restore the previous device configuration.
>  
>  Platform data
>  -------------
>  
> +* checksum:
> +  set it to 1 to enable CRC validation of the readings (default to 0).
>  * no_otp_reload:
>    flag to indicate not to reload from OTP (default to 0).
>  * low_resolution:
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 5fa5585..152c9ac 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -2,6 +2,7 @@
>   * 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>
>   *          Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>   *
>   * Copyright (c) 2009 Jonathan Cameron
> @@ -37,6 +38,7 @@
>  #define SHT15_MEASURE_RH		0x05
>  #define SHT15_WRITE_STATUS		0x06
>  #define SHT15_READ_STATUS		0x07
> +#define SHT15_SOFT_RESET		0x1E
>  
>  #define SHT15_READING_NOTHING		0
>  #define SHT15_READING_TEMP		1
> @@ -47,6 +49,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 */
> +
>  /* Status Register Bits */
>  #define SHT15_STATUS_LOW_RESOLUTION	0x01
>  #define SHT15_STATUS_NO_OTP_RELOAD	0x02
> @@ -72,6 +77,42 @@ static const struct sht15_temppair temppoints[] = {
>  	{ 5000000, -40100 },
>  };
>  
> +/* Table from CRC datasheet, section 2.4 */
> +static const u8 sht15_crc8_table[] = {
> +	0,	49,	98,	83,	196,	245,	166,	151,
> +	185,	136,	219,	234,	125,	76,	31,	46,
> +	67,	114,	33,	16,	135,	182,	229,	212,
> +	250,	203,	152,	169,	62,	15,	92,	109,
> +	134,	183,	228,	213,	66,	115,	32,	17,
> +	63,	14,	93,	108,	251,	202,	153,	168,
> +	197,	244,	167,	150,	1,	48,	99,	82,
> +	124,	77,	30,	47,	184,	137,	218,	235,
> +	61,	12,	95,	110,	249,	200,	155,	170,
> +	132,	181,	230,	215,	64,	113,	34,	19,
> +	126,	79,	28,	45,	186,	139,	216,	233,
> +	199,	246,	165,	148,	3,	50,	97,	80,
> +	187,	138,	217,	232,	127,	78,	29,	44,
> +	2,	51,	96,	81,	198,	247,	164,	149,
> +	248,	201,	154,	171,	60,	13,	94,	111,
> +	65,	112,	35,	18,	133,	180,	231,	214,
> +	122,	75,	24,	41,	190,	143,	220,	237,
> +	195,	242,	161,	144,	7,	54,	101,	84,
> +	57,	8,	91,	106,	253,	204,	159,	174,
> +	128,	177,	226,	211,	68,	117,	38,	23,
> +	252,	205,	158,	175,	56,	9,	90,	107,
> +	69,	116,	39,	22,	129,	176,	227,	210,
> +	191,	142,	221,	236,	123,	74,	25,	40,
> +	6,	55,	100,	85,	194,	243,	160,	145,
> +	71,	118,	37,	20,	131,	178,	225,	208,
> +	254,	207,	156,	173,	58,	11,	88,	105,
> +	4,	53,	102,	87,	192,	241,	162,	147,
> +	189,	140,	223,	238,	121,	72,	27,	42,
> +	193,	240,	163,	146,	5,	52,	103,	86,
> +	120,	73,	26,	43,	188,	141,	222,	239,
> +	130,	179,	224,	209,	70,	119,	36,	21,
> +	59,	10,	89,	104,	255,	206,	157,	172
> +};
> +
>  /**
>   * struct sht15_data - device instance specific data
>   * @pdata:		platform data (gpio's etc).
> @@ -80,6 +121,8 @@ static const struct sht15_temppair temppoints[] = {
>   * @val_temp:		last temperature value read from device.
>   * @val_humid:		last humidity value read from device.
>   * @val_status:		last status register value read from device.
> + * @checksum_ok:	last value read from the device passed CRC validation.
> + * @checksumming:	flag used to enable the data validation with CRC.
>   * @flag:		status flag used to identify what the last request was.
>   * @measures_valid:	are the current stored measures valid (start condition).
>   * @status_valid:	is the current stored status valid (start condition).
> @@ -106,6 +149,8 @@ struct sht15_data {
>  	uint16_t			val_temp;
>  	uint16_t			val_humid;
>  	u8				val_status;
> +	u8				checksum_ok;
Why a u8? It is boolean. Obviously that'll work in a u8 but it almost
implicitly implies it might contain some data.

> +	u8				checksumming;
If it's a flag, perhaps u8 implies it might be something other than 0 or 1.
Admittedly this is true of some of the other elements, but we live and learn.

>  	u8				flag;
>  	u8				measures_valid;
>  	u8				status_valid;
> @@ -123,6 +168,40 @@ struct sht15_data {
>  };
>  
>  /**
> + * reverse() - reverse a byte
> + * @byte:    byte to reverse.
> + */
Rather generic name, liable to clash sometime in the future.
Obviously it's a fairly generic function, but best call it
sht15_reverse or similar.
> +static u8 reverse(u8 byte)
> +{
> +	u8 i, c;
> +
> +	for (c = 0, i = 0; i < 8; i++)
> +		c |= (!!(byte & (1 << i))) << (7 - i);
> +	return c;
> +}
> +
> +/**
> + * sht15_crc8() - compute crc8
> + * @data:	sht15 specific data.
> + * @value:	sht15 retrieved data.
> + *
> + * This implements section 2 of the CRC datasheet.
> + */
> +static u8 sht15_crc8(struct sht15_data *data,
> +		const u8 *value,
> +		int len)
> +{
> +	u8 crc = reverse(data->val_status & 0x0F);
> +
> +	while (len--) {
> +		crc = sht15_crc8_table[*value ^ crc];
> +		value++;
> +	}
> +
> +	return crc;
> +}
> +
> +/**
>   * sht15_connection_reset() - reset the comms interface
>   * @data:	sht15 specific data
>   *
> @@ -242,6 +321,45 @@ static int sht15_send_cmd(struct sht15_data *data, u8 cmd)
>  }
>  
>  /**
> + * sht15_soft_reset() - send a soft reset command
> + * @data:	sht15 specific data.
> + *
> + * As described in section 3.2 of the datasheet.
> + */
> +static int sht15_soft_reset(struct sht15_data *data)
> +{
> +	int ret;
> +
> +	ret = sht15_send_cmd(data, SHT15_SOFT_RESET);
> +	if (ret)
> +		return ret;
> +	msleep(SHT15_TSRST);
> +	/* device resets default hardware status register value */
> +	data->val_status = 0;
> +	return 0;
> +}
> +
> +/**
> + * sht15_ack() - send a ack
> + * @data:	sht15 specific data.
> + *
> + * Each byte of data is acknowledged by pulling the data line
> + * low for one clock pulse.
> + */
> +static void sht15_ack(struct sht15_data *data)
> +{
> +	gpio_direction_output(data->pdata->gpio_data, 0);
> +	ndelay(SHT15_TSU);
> +	gpio_set_value(data->pdata->gpio_sck, 1);
> +	ndelay(SHT15_TSU);
> +	gpio_set_value(data->pdata->gpio_sck, 0);
> +	ndelay(SHT15_TSU);
> +	gpio_set_value(data->pdata->gpio_data, 1);
> +
> +	gpio_direction_input(data->pdata->gpio_data);
> +}
> +
> +/**
>   * sht15_end_transmission() - notify device of end of transmission
>   * @data:	device state.
>   *
> @@ -312,6 +430,9 @@ static int sht15_update_status(struct sht15_data *data)
>  {
>  	int ret = 0;
>  	u8 status;
> +	u8 previous_config;
> +	u8 dev_checksum = 0;
> +	u8 checksum_vals[2];
>  	int timeout = HZ;
>  
>  	mutex_lock(&data->read_lock);
> @@ -322,8 +443,40 @@ static int sht15_update_status(struct sht15_data *data)
>  			goto error_ret;
>  		status = sht15_read_byte(data);
>  
> +		if (data->checksumming) {
> +			sht15_ack(data);
> +			dev_checksum = reverse(sht15_read_byte(data));
> +			checksum_vals[0] = SHT15_READ_STATUS;
> +			checksum_vals[1] = status;
> +			data->checksum_ok = (sht15_crc8(data, checksum_vals, 2)
> +					= dev_checksum);
> +		}
> +
>  		sht15_end_transmission(data);
>  
> +		/*
> +		 * 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) {
> +			previous_config = data->val_status & 0x07;
> +			ret = sht15_soft_reset(data);
> +			if (ret)
> +				goto error_ret;
> +			if (previous_config) {
> +				ret = sht15_send_status(data, previous_config);
> +				if (ret) {
> +					dev_err(data->dev,
> +						"CRC validation failed, unable "
> +						"to restore device settings\n");
> +					goto error_ret;
> +				}
> +			}
> +			ret = -EAGAIN;
> +			goto error_ret;
> +		}
> +
>  		data->val_status = status;
>  		data->status_valid = 1;
>  		data->last_status = jiffies;
> @@ -346,6 +499,7 @@ static int sht15_measure(struct sht15_data *data,
>  			 int timeout_msecs)
>  {
>  	int ret;
> +	u8 previous_config;
>  
>  	ret = sht15_send_cmd(data, command);
>  	if (ret)
> @@ -369,6 +523,29 @@ static int sht15_measure(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) {
> +		previous_config = data->val_status & 0x07;
> +		ret = sht15_soft_reset(data);
> +		if (ret)
> +			return ret;
> +		if (previous_config) {
> +			ret = sht15_send_status(data, previous_config);
> +			if (ret) {
> +				dev_err(data->dev,
> +					"CRC validation failed, unable "
> +					"to restore device settings\n");
> +				return ret;
> +			}
> +		}
> +		return -EAGAIN;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -633,28 +810,11 @@ static irqreturn_t sht15_interrupt_fired(int irq, void *d)
>  	return IRQ_HANDLED;
>  }
>  
> -/**
> - * sht15_ack() - Send an ack to the device
> - *
> - * Each byte of data is acknowledged by pulling the data line
> - * low for one clock pulse.
> - */
> -static void sht15_ack(struct sht15_data *data)
> -{
> -	gpio_direction_output(data->pdata->gpio_data, 0);
> -	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 1);
> -	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_sck, 0);
> -	ndelay(SHT15_TSU);
> -	gpio_set_value(data->pdata->gpio_data, 1);
> -
> -	gpio_direction_input(data->pdata->gpio_data);
> -}
> -
>  static void sht15_bh_read_data(struct work_struct *work_s)
>  {
>  	uint16_t val = 0;
> +	u8 dev_checksum = 0;
> +	u8 checksum_vals[3];
>  	struct sht15_data *data
>  		= container_of(work_s, struct sht15_data,
>  			       read_work);
> @@ -679,6 +839,21 @@ static void sht15_bh_read_data(struct work_struct *work_s)
>  	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_vals[0] = (data->flag = SHT15_READING_TEMP) ?
> +			SHT15_MEASURE_TEMP : SHT15_MEASURE_RH;
> +		checksum_vals[1] = (u8) (val >> 8);
> +		checksum_vals[2] = (u8) val;
> +		data->checksum_ok
> +			= (sht15_crc8(data, checksum_vals, 3) = dev_checksum);
> +	}
> +
>  	/* Tell the device we are done */
>  	sht15_end_transmission(data);
>  
> @@ -750,6 +925,8 @@ static int __devinit sht15_probe(struct platform_device *pdev)
>  	}
>  	data->pdata = pdev->dev.platform_data;
>  	data->supply_uV = data->pdata->supply_mv * 1000;
> +	if (data->pdata->checksum)
> +		data->checksumming = 1;
>  	if (data->pdata->no_otp_reload)
>  		status |= SHT15_STATUS_NO_OTP_RELOAD;
>  	if (data->pdata->low_resolution)
> @@ -808,7 +985,9 @@ 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);
> +	ret = sht15_soft_reset(data);
> +	if (ret)
> +		goto err_release_irq;
>  
>  	/* write status with platform data options */
>  	if (status) {
> diff --git a/include/linux/sht15.h b/include/linux/sht15.h
> index 363982b..ef09665 100644
> --- a/include/linux/sht15.h
> +++ b/include/linux/sht15.h
> @@ -19,6 +19,7 @@
>   * @gpio_sck:		no. of gpio to which the data clock is connected.
>   * @supply_mv:		supply voltage in mv. Overridden by regulator if
>   *			available.
> + * @checksum:		flag to indicate the checksum should be validated.
>   * @no_otp_reload:	flag to indicate no reload from OTP.
>   * @low_resolution:	flag to indicate the temp/humidity resolution to use.
>   */
> @@ -26,6 +27,7 @@ struct sht15_platform_data {
>  	int gpio_data;
>  	int gpio_sck;
>  	int supply_mv;
> +	int checksum;
>  	int no_otp_reload;
>  	int low_resolution;
>  };


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