On 03/16/11 18:44, Vivien Didelot wrote:
> From: Jerome Oufella <jerome.oufella@xxxxxxxxxxxxxxxxxxxx>
>
> 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@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 */
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@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
