On Wed, 16 Mar 2011 14:44:08 -0400, Vivien Didelot wrote:
> From: Jerome Oufella <jerome.oufella@xxxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
> To: lm-sensors@xxxxxxxxxxxxxx
> Cc: khali@xxxxxxxxxxxx
> ---
> 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@xxxxxxxxxxxxxxxxxxxx>
> + *
> * 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@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
