[lm-sensors] [RFC PATCH 3/3] hwmon: sht15: add support for writing

[lm-sensors] [RFC PATCH 3/3] hwmon: sht15: add support for writing

[Vivien Didelot]

This patch adds support to change options on the SHT15 device, such as:
* The measurement resolution through temp_resolution sysfs attribute;
* The heater usage through the heater_enable sysfs attribute;
* The usage of reload from OTP through the otp_reload sysfs attribute.

It also fixes CRC calculation:
CRC calculation now initializes the CRC register with the value of the
lower nibble of the value of the status register, as described in the
SHT1x CRC calculation technical note.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
To: lm-sensors@lm-sensors.org
Cc: khali@linux-fr.org
Cc: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/hwmon/sht15.c |  185 +++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 164 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
index 62a1a03..49dfd32 100644
--- a/drivers/hwmon/sht15.c
+++ b/drivers/hwmon/sht15.c
@@ -13,9 +13,9 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  *
- * Default resolution only (14bit temp, 12bit humidity)
+ * Resolution can be changed via the temp_resolution sysfs attribute
  * Checksum validation can be enabled via the checksumming sysfs attribute
- * Heater not enabled.
+ * Heater can be enabled via the heater_enable sysfs attribute
  * Timings are all conservative.
  *
  * Data sheet available (1/2009) at
@@ -46,6 +46,7 @@
 #define SHT15_MEASURE_RH	0x05
 #define SHT15_SOFT_RESET	0x1E
 #define SHT15_READ_STATUS	0x07
+#define SHT15_WRITE_STATUS	0x06
 
 #define SHT15_READING_NOTHING	0
 #define SHT15_READING_TEMP	1
@@ -195,17 +196,20 @@ static u8 reverse(u8 byte)
 
 /**
  * sht15_crc8() - compute crc8
- * @data:	sht15 retrieved data
+ * @data:	sht15 specific data
+ * @value:	sht15 retrieved data
  *
  * This implements section 2 of the crc data sheet
  */
-static u8 sht15_crc8(const unsigned char *data, unsigned char len)
+static u8 sht15_crc8(struct sht15_data *data,
+		const unsigned char *value,
+		unsigned char len)
 {
-	unsigned char crc = 0;
+	unsigned char crc = reverse(data->val_status & 0x0F);
 
 	while (len--) {
-		crc = sht15_crc8_table[*data ^ crc];
-		data++;
+		crc = sht15_crc8_table[*value ^ crc];
+		value++;
 	}
 
 	return crc;
@@ -360,6 +364,40 @@ static inline void sht15_soft_reset(struct sht15_data *data)
 }
 
 /**
+ * sht15_send_status() - write the status register byte
+ * @data:	sht15 specific data
+ * @feature:	the option to update the status register with.
+ * @value:	the value to set.
+ *
+ * As described in figure 14 and table 5 of the data sheet.
+ */
+
+static int sht15_send_status(struct sht15_data *data, u8 feature, int value)
+{
+	int ret;
+	u8 status;
+
+	status = data->val_status;
+	if (value)
+		status |= feature;
+	else
+		status &= ~feature;
+
+	ret = sht15_send_cmd(data, SHT15_WRITE_STATUS);
+	if (ret)
+		return ret;
+	gpio_direction_output(data->pdata->gpio_data, 1);
+	ndelay(SHT15_TSU);
+	sht15_send_byte(data, status);
+	ret = sht15_wait_for_response(data);
+	if (ret)
+		return ret;
+	/* Needed for the next CRC calculation */
+	sht15_apply_status(data, status);
+	return 0;
+}
+
+/**
  * sht15_update_status() - get the status register from device
  * @data:	device instance specific data.
  *
@@ -382,7 +420,7 @@ static int sht15_update_status(struct sht15_data *data)
 		dev_checksum = reverse(sht15_read_byte(data));
 		checksum_values[0] = SHT15_READ_STATUS;
 		checksum_values[1] = status;
-		data->checksum_ok = (sht15_crc8(checksum_values, 2) = dev_checksum);
+		data->checksum_ok = (sht15_crc8(data, checksum_values, 2) = dev_checksum);
 	}
 
 	sht15_end_transmission(data);
@@ -489,6 +527,7 @@ error_ret:
 static inline int sht15_calc_temp(struct sht15_data *data)
 {
 	int d1 = temppoints[0].d1;
+	int d2 = (data->temp_resolution = 14) ? 10 : 40;
 	int i;
 
 	for (i = ARRAY_SIZE(temppoints) - 1; i > 0; i--)
@@ -501,7 +540,7 @@ static inline int sht15_calc_temp(struct sht15_data *data)
 			break;
 		}
 
-	return data->val_temp*10 + d1;
+	return data->val_temp * d2 + d1;
 }
 
 /**
@@ -510,20 +549,33 @@ static inline int sht15_calc_temp(struct sht15_data *data)
  *
  * This is the temperature compensated version as per section 4.2 of
  * the data sheet.
- **/
+ *
+ * The sensor is assumed to be V3, which is compatible with V4.
+ * Humidity conversion coefficients are shown in table 7 of the data sheet.
+ */
 static inline int sht15_calc_humid(struct sht15_data *data)
 {
 	int RHlinear; /* milli percent */
 	int temp = sht15_calc_temp(data);
+	int c2, c3;
+	int t2;
 
 	const int c1 = -4;
-	const int c2 = 40500; /* x 10 ^ -6 */
-	const int c3 = -28; /* x10 ^ -7 */
 
-	RHlinear = c1*1000
-		+ c2 * data->val_humid/1000
-		+ (data->val_humid * data->val_humid * c3)/10000;
-	return (temp - 25000) * (10000 + 80 * data->val_humid)
+	if (data->humidity_resolution = 12) {
+		c2 = 40500; /* x 10 ^ -6 */
+		c3 = -28; /* x 10 ^ -7 */
+		t2 = 80;
+	} else {
+		c2 = 648000; /* x 10 ^ -6 */
+		c3 = -7200; /* x 10 ^ -7 */
+		t2 = 1280;
+	}
+
+	RHlinear = c1 * 1000
+		+ c2 * data->val_humid / 1000
+		+ (data->val_humid * data->val_humid * c3) / 10000;
+	return (temp - 25000) * (10000 + t2 * data->val_humid)
 		/ 1000000 + RHlinear;
 }
 
@@ -553,6 +605,30 @@ static ssize_t sht15_show_heater(struct device *dev,
 	return ret ? ret : sprintf(buf, "%d\n", data->heater);
 }
 
+static ssize_t sht15_store_heater(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	int ret;
+	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;
+	}
+
+	ret = sht15_send_status(data, SHT15_STATUS_HEATER, !!value);
+	if (ret) {
+		mutex_unlock(&data->read_lock);
+		return ret;
+	}
+
+	mutex_unlock(&data->read_lock);
+	return count;
+}
+
 static ssize_t sht15_show_otp_reload(struct device *dev,
 			       struct device_attribute *attr,
 			       char *buf)
@@ -566,6 +642,30 @@ static ssize_t sht15_show_otp_reload(struct device *dev,
 	return ret ? ret : sprintf(buf, "%d\n", !(data->no_otp_reload));
 }
 
+static ssize_t sht15_store_otp_reload(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	int ret;
+	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;
+	}
+
+	ret = sht15_send_status(data, SHT15_STATUS_OTP_RELOAD, !value);
+	if (ret) {
+		mutex_unlock(&data->read_lock);
+		return ret;
+	}
+
+	mutex_unlock(&data->read_lock);
+	return count;
+}
+
 static ssize_t sht15_show_temp_resolution(struct device *dev,
 			       struct device_attribute *attr,
 			       char *buf)
@@ -579,6 +679,42 @@ static ssize_t sht15_show_temp_resolution(struct device *dev,
 	return ret ? ret : sprintf(buf, "%d\n", data->temp_resolution);
 }
 
+static ssize_t sht15_store_temp_resolution(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	int ret;
+	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;
+	}
+
+	switch (value) {
+	case 14:
+		value = 0;
+		break;
+	case 12:
+		value = 1;
+		break;
+	default:
+		mutex_unlock(&data->read_lock);
+		return -EINVAL;
+	}
+
+	ret = sht15_send_status(data, SHT15_STATUS_RESOLUTION, value);
+	if (ret) {
+		mutex_unlock(&data->read_lock);
+		return ret;
+	}
+
+	mutex_unlock(&data->read_lock);
+	return count;
+}
+
 static ssize_t sht15_show_humidity_resolution(struct device *dev,
 			       struct device_attribute *attr,
 			       char *buf)
@@ -661,12 +797,18 @@ static SENSOR_DEVICE_ATTR(humidity1_input,
 			  S_IRUGO, sht15_show_humidity,
 			  NULL, 0);
 static DEVICE_ATTR(battery_alarm, S_IRUGO, sht15_show_battery, NULL);
-static DEVICE_ATTR(heater_enable, S_IRUGO, sht15_show_heater, NULL);
-static DEVICE_ATTR(otp_reload, S_IRUGO, sht15_show_otp_reload, NULL);
+static DEVICE_ATTR(heater_enable,
+		S_IRUGO | S_IWUSR,
+		sht15_show_heater,
+		sht15_store_heater);
+static DEVICE_ATTR(otp_reload,
+		S_IRUGO | S_IWUSR,
+		sht15_show_otp_reload,
+		sht15_store_otp_reload);
 static DEVICE_ATTR(temp_resolution,
-		S_IRUGO,
+		S_IRUGO | S_IWUSR,
 		sht15_show_temp_resolution,
-		NULL);
+		sht15_store_temp_resolution);
 static DEVICE_ATTR(humidity_resolution,
 		S_IRUGO,
 		sht15_show_humidity_resolution,
@@ -794,7 +936,7 @@ static void sht15_bh_read_data(struct work_struct *work_s)
 		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);
+		data->checksum_ok = (sht15_crc8(data, checksum_values, 3) = dev_checksum);
 	}
 
 	/* Tell the device we are done */
@@ -868,6 +1010,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 */
+	data->val_status = 0;
 
 /* If a regulator is available, query what the supply voltage actually is!*/
 	data->reg = regulator_get(data->dev, "vcc");
-- 
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 3/3] hwmon: sht15: add support for

[Jonathan Cameron]

On 03/16/11 18:44, Vivien Didelot wrote:
> This patch adds support to change options on the SHT15 device, such as:
> * The measurement resolution through temp_resolution sysfs attribute;
But not the humidity one? That's rather inconsistent.
> * The heater usage through the heater_enable sysfs attribute;
> * The usage of reload from OTP through the otp_reload sysfs attribute.
It's still a cryptic name.  Use fine calibration might be better terminology?
> 
> It also fixes CRC calculation:
> CRC calculation now initializes the CRC register with the value of the
> lower nibble of the value of the status register, as described in the
> SHT1x CRC calculation technical note.
Hang on, so the original crc patch is broken?  Please rework this series
so that patch is valid in the first place!

Glad to see someone working with this driver. It's been a while since I
even checked it worked at all!  Thanks.

Jonathan
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Reviewed-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> To: lm-sensors@lm-sensors.org
> Cc: khali@linux-fr.org
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/hwmon/sht15.c |  185 +++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 164 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 62a1a03..49dfd32 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -13,9 +13,9 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   *
These docs kind of meant sense when they were a list of restrictions (basically a todo list).
I'd just drop them entirely now (other than the conservative timings one perhaps as I think
that is still true).
> - * Default resolution only (14bit temp, 12bit humidity)
> + * Resolution can be changed via the temp_resolution sysfs attribute
>   * Checksum validation can be enabled via the checksumming sysfs attribute
> - * Heater not enabled.
> + * Heater can be enabled via the heater_enable sysfs attribute
>   * Timings are all conservative.
>   *
>   * Data sheet available (1/2009) at
> @@ -46,6 +46,7 @@
>  #define SHT15_MEASURE_RH	0x05
>  #define SHT15_SOFT_RESET	0x1E
>  #define SHT15_READ_STATUS	0x07
> +#define SHT15_WRITE_STATUS	0x06
>  
>  #define SHT15_READING_NOTHING	0
>  #define SHT15_READING_TEMP	1
> @@ -195,17 +196,20 @@ static u8 reverse(u8 byte)
>  
>  /**
>   * sht15_crc8() - compute crc8
> - * @data:	sht15 retrieved data
> + * @data:	sht15 specific data
Definitely roll this into the first patch.
> + * @value:	sht15 retrieved data
>   *
>   * This implements section 2 of the crc data sheet
>   */
> -static u8 sht15_crc8(const unsigned char *data, unsigned char len)
> +static u8 sht15_crc8(struct sht15_data *data,
> +		const unsigned char *value,
> +		unsigned char len)
>  {
> -	unsigned char crc = 0;
> +	unsigned char crc = reverse(data->val_status & 0x0F);
>  
>  	while (len--) {
> -		crc = sht15_crc8_table[*data ^ crc];
> -		data++;
> +		crc = sht15_crc8_table[*value ^ crc];
> +		value++;
>  	}
>  
>  	return crc;
> @@ -360,6 +364,40 @@ static inline void sht15_soft_reset(struct sht15_data *data)
>  }
>  
>  /**
It's times like this when you wonder who decided to call it status on the
datasheet.  Now state would be a reasonable name, but the concept of changing
status seems a little odd!

> + * sht15_send_status() - write the status register byte
> + * @data:	sht15 specific data
> + * @feature:	the option to update the status register with.
> + * @value:	the value to set.
> + *
> + * As described in figure 14 and table 5 of the data sheet.
> + */
> +
> +static int sht15_send_status(struct sht15_data *data, u8 feature, int value)
> +{
> +	int ret;
> +	u8 status;
> +
Ah, so the cache in the previous patch now makes sense. Ideally it would have
only been introduced in this one, but doesn't really matter.
> +	status = data->val_status;
> +	if (value)
> +		status |= feature;
> +	else
> +		status &= ~feature;
> +
> +	ret = sht15_send_cmd(data, SHT15_WRITE_STATUS);
> +	if (ret)
> +		return ret;
> +	gpio_direction_output(data->pdata->gpio_data, 1);
> +	ndelay(SHT15_TSU);
> +	sht15_send_byte(data, status);
> +	ret = sht15_wait_for_response(data);
> +	if (ret)
> +		return ret;
> +	/* Needed for the next CRC calculation */
> +	sht15_apply_status(data, status);
> +	return 0;
> +}
> +
> +/**
>   * sht15_update_status() - get the status register from device
>   * @data:	device instance specific data.
>   *
> @@ -382,7 +420,7 @@ static int sht15_update_status(struct sht15_data *data)
>  		dev_checksum = reverse(sht15_read_byte(data));
>  		checksum_values[0] = SHT15_READ_STATUS;
>  		checksum_values[1] = status;
> -		data->checksum_ok = (sht15_crc8(checksum_values, 2) = dev_checksum);
> +		data->checksum_ok = (sht15_crc8(data, checksum_values, 2) = dev_checksum);
>  	}
>  
>  	sht15_end_transmission(data);
> @@ -489,6 +527,7 @@ error_ret:
>  static inline int sht15_calc_temp(struct sht15_data *data)
>  {
>  	int d1 = temppoints[0].d1;
This becomes cleaner if you make that a data->high_resolution flag
(and it's smaller to store tat)
> +	int d2 = (data->temp_resolution = 14) ? 10 : 40;
>  	int i;
>  
>  	for (i = ARRAY_SIZE(temppoints) - 1; i > 0; i--)
> @@ -501,7 +540,7 @@ static inline int sht15_calc_temp(struct sht15_data *data)
>  			break;
>  		}
>  
> -	return data->val_temp*10 + d1;
> +	return data->val_temp * d2 + d1;
>  }
>  
>  /**
> @@ -510,20 +549,33 @@ static inline int sht15_calc_temp(struct sht15_data *data)
>   *
>   * This is the temperature compensated version as per section 4.2 of
>   * the data sheet.
> - **/
> + *
> + * The sensor is assumed to be V3, which is compatible with V4.
> + * Humidity conversion coefficients are shown in table 7 of the data sheet.
> + */
>  static inline int sht15_calc_humid(struct sht15_data *data)
>  {
>  	int RHlinear; /* milli percent */
>  	int temp = sht15_calc_temp(data);
> +	int c2, c3;
> +	int t2;
>  
>  	const int c1 = -4;
> -	const int c2 = 40500; /* x 10 ^ -6 */
> -	const int c3 = -28; /* x10 ^ -7 */
>  
> -	RHlinear = c1*1000
> -		+ c2 * data->val_humid/1000
> -		+ (data->val_humid * data->val_humid * c3)/10000;
> -	return (temp - 25000) * (10000 + 80 * data->val_humid)
> +	if (data->humidity_resolution = 12) {
> +		c2 = 40500; /* x 10 ^ -6 */
> +		c3 = -28; /* x 10 ^ -7 */
> +		t2 = 80;
> +	} else {
> +		c2 = 648000; /* x 10 ^ -6 */
> +		c3 = -7200; /* x 10 ^ -7 */
> +		t2 = 1280;
> +	}
> +
> +	RHlinear = c1 * 1000
> +		+ c2 * data->val_humid / 1000
> +		+ (data->val_humid * data->val_humid * c3) / 10000;
> +	return (temp - 25000) * (10000 + t2 * data->val_humid)
>  		/ 1000000 + RHlinear;
>  }
>  
> @@ -553,6 +605,30 @@ static ssize_t sht15_show_heater(struct device *dev,
>  	return ret ? ret : sprintf(buf, "%d\n", data->heater);
>  }
>  
> +static ssize_t sht15_store_heater(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int ret;
> +	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;
> +	}
> +
> +	ret = sht15_send_status(data, SHT15_STATUS_HEATER, !!value);
> +	if (ret) {
> +		mutex_unlock(&data->read_lock);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&data->read_lock);
> +	return count;
> +}
> +
>  static ssize_t sht15_show_otp_reload(struct device *dev,
>  			       struct device_attribute *attr,
>  			       char *buf)
> @@ -566,6 +642,30 @@ static ssize_t sht15_show_otp_reload(struct device *dev,
>  	return ret ? ret : sprintf(buf, "%d\n", !(data->no_otp_reload));
>  }
>  
> +static ssize_t sht15_store_otp_reload(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int ret;
> +	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;
> +	}
> +
> +	ret = sht15_send_status(data, SHT15_STATUS_OTP_RELOAD, !value);
> +	if (ret) {
Check ret after the mutex is unlocked to simplify this code a touch.
> +		mutex_unlock(&data->read_lock);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&data->read_lock);
> +	return count;
> +}
> +
>  static ssize_t sht15_show_temp_resolution(struct device *dev,
>  			       struct device_attribute *attr,
>  			       char *buf)
> @@ -579,6 +679,42 @@ static ssize_t sht15_show_temp_resolution(struct device *dev,
>  	return ret ? ret : sprintf(buf, "%d\n", data->temp_resolution);
>  }
>  
> +static ssize_t sht15_store_temp_resolution(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int ret;
> +	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;
> +	}
> +
> +	switch (value) {
> +	case 14:
> +		value = 0;
> +		break;
> +	case 12:
> +		value = 1;
> +		break;
> +	default:
> +		mutex_unlock(&data->read_lock);
> +		return -EINVAL;
> +	}
> +
> +	ret = sht15_send_status(data, SHT15_STATUS_RESOLUTION, value);
> +	if (ret) {
> +		mutex_unlock(&data->read_lock);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&data->read_lock);
> +	return count;
> +}
> +
>  static ssize_t sht15_show_humidity_resolution(struct device *dev,
>  			       struct device_attribute *attr,
>  			       char *buf)
> @@ -661,12 +797,18 @@ static SENSOR_DEVICE_ATTR(humidity1_input,
>  			  S_IRUGO, sht15_show_humidity,
>  			  NULL, 0);
>  static DEVICE_ATTR(battery_alarm, S_IRUGO, sht15_show_battery, NULL);
> -static DEVICE_ATTR(heater_enable, S_IRUGO, sht15_show_heater, NULL);
> -static DEVICE_ATTR(otp_reload, S_IRUGO, sht15_show_otp_reload, NULL);
> +static DEVICE_ATTR(heater_enable,
> +		S_IRUGO | S_IWUSR,
> +		sht15_show_heater,
> +		sht15_store_heater);
> +static DEVICE_ATTR(otp_reload,
> +		S_IRUGO | S_IWUSR,
> +		sht15_show_otp_reload,
> +		sht15_store_otp_reload);
>  static DEVICE_ATTR(temp_resolution,
> -		S_IRUGO,
> +		S_IRUGO | S_IWUSR,
>  		sht15_show_temp_resolution,
> -		NULL);
> +		sht15_store_temp_resolution);
>  static DEVICE_ATTR(humidity_resolution,
>  		S_IRUGO,
>  		sht15_show_humidity_resolution,
> @@ -794,7 +936,7 @@ static void sht15_bh_read_data(struct work_struct *work_s)
>  		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);
> +		data->checksum_ok = (sht15_crc8(data, checksum_values, 3) = dev_checksum);
>  	}
>  
>  	/* Tell the device we are done */
> @@ -868,6 +1010,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 */
> +	data->val_status = 0;
>  
>  /* If a regulator is available, query what the supply voltage actually is!*/
>  	data->reg = regulator_get(data->dev, "vcc");


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

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: sht15: add support for

[Vivien Didelot]

Hi Jonathan,

On Mon, Mar 21, 2011 at 07:55:20PM +0000, Jonathan Cameron wrote:
> On 03/16/11 18:44, Vivien Didelot wrote:
> > This patch adds support to change options on the SHT15 device, such as:
> > * The measurement resolution through temp_resolution sysfs attribute;
> But not the humidity one? That's rather inconsistent.
It was updated by temp_resolution as well. You're right, that's not
consistent. I'll use another name for the platform data field, as you
suggested. See below.

> > * The heater usage through the heater_enable sysfs attribute;
> > * The usage of reload from OTP through the otp_reload sysfs attribute.
> It's still a cryptic name.  Use fine calibration might be better terminology?
For the moment, the field in the platform data is no_otp_reload, as in the
datasheet. What would you suggest, 'calibration' instead?

> > 
> > It also fixes CRC calculation:
> > CRC calculation now initializes the CRC register with the value of the
> > lower nibble of the value of the status register, as described in the
> > SHT1x CRC calculation technical note.
> Hang on, so the original crc patch is broken?  Please rework this series
> so that patch is valid in the first place!
The original patch is not broken. The CRC calculation uses the lower nibble
of the status register, which is 0 by default. This 'fix' comes here
because this patch allow to change the status register, that is to say the
lower nibble.
It does not make sense to fix it in the first patch, before add the support
to write the status register (even read it). Don't you agree?
> 
> Glad to see someone working with this driver. It's been a while since I
> even checked it worked at all!  Thanks.
;)
> 
> Jonathan
> >   * it under the terms of the GNU General Public License version 2 as
> >   * published by the Free Software Foundation.
> >   *
> These docs kind of meant sense when they were a list of restrictions (basically a todo list).
> I'd just drop them entirely now (other than the conservative timings one perhaps as I think
> that is still true).
Ok, should I remove this?

> >  /**
> >   * sht15_crc8() - compute crc8
> > - * @data:	sht15 retrieved data
> > + * @data:	sht15 specific data
> Definitely roll this into the first patch.
> > + * @value:	sht15 retrieved data
> >   *
> >   * This implements section 2 of the crc data sheet
> >   */
> > -static u8 sht15_crc8(const unsigned char *data, unsigned char len)
> > +static u8 sht15_crc8(struct sht15_data *data,
> > +		const unsigned char *value,
> > +		unsigned char len)
> >  {
> > -	unsigned char crc = 0;
> > +	unsigned char crc = reverse(data->val_status & 0x0F);
> >  
> >  	while (len--) {
> > -		crc = sht15_crc8_table[*data ^ crc];
> > -		data++;
> > +		crc = sht15_crc8_table[*value ^ crc];
> > +		value++;
See the comment above about the CRC calcultation fix.
> >  	}
> >  
> >  	return crc;
> > @@ -360,6 +364,40 @@ static inline void sht15_soft_reset(struct sht15_data *data)
> >  }
> >  
> >  /**
> It's times like this when you wonder who decided to call it status on the
> datasheet.  Now state would be a reasonable name, but the concept of changing
> status seems a little odd!
> 
You're right. Should I rename every sht15_*_status() functions into
sht15_*_state()?

> > + * sht15_send_status() - write the status register byte
> > + * @data:	sht15 specific data
> > + * @feature:	the option to update the status register with.
> > + * @value:	the value to set.
> > + *
> > + * As described in figure 14 and table 5 of the data sheet.
> > + */
> > +
> > +static int sht15_send_status(struct sht15_data *data, u8 feature, int value)
> > +{
> > +	int ret;
> > +	u8 status;
> > +
> Ah, so the cache in the previous patch now makes sense. Ideally it would have
> only been introduced in this one, but doesn't really matter.
> > +	status = data->val_status;
> > +	if (value)
> > +		status |= feature;
> > +	else
> > +		status &= ~feature;
> > +
> > +	ret = sht15_send_cmd(data, SHT15_WRITE_STATUS);
> > +	if (ret)
> > +		return ret;
> > +	gpio_direction_output(data->pdata->gpio_data, 1);
> > +	ndelay(SHT15_TSU);
> > +	sht15_send_byte(data, status);
> > +	ret = sht15_wait_for_response(data);
> > +	if (ret)
> > +		return ret;
> > +	/* Needed for the next CRC calculation */
> > +	sht15_apply_status(data, status);
> > +	return 0;
> > +}
> > +
> > +/**
> >   * sht15_update_status() - get the status register from device
> >   * @data:	device instance specific data.
> >   *
> > @@ -382,7 +420,7 @@ static int sht15_update_status(struct sht15_data *data)
> >  		dev_checksum = reverse(sht15_read_byte(data));
> >  		checksum_values[0] = SHT15_READ_STATUS;
> >  		checksum_values[1] = status;
> > -		data->checksum_ok = (sht15_crc8(checksum_values, 2) = dev_checksum);
> > +		data->checksum_ok = (sht15_crc8(data, checksum_values, 2) = dev_checksum);
> >  	}
> >  
> >  	sht15_end_transmission(data);
> > @@ -489,6 +527,7 @@ error_ret:
> >  static inline int sht15_calc_temp(struct sht15_data *data)
> >  {
> >  	int d1 = temppoints[0].d1;
> This becomes cleaner if you make that a data->high_resolution flag
> (and it's smaller to store tat)
Great idea! I think I'll rename the platform data field into low_resolution
as the default value is 0 which means high resolution.

> > +	mutex_lock(&data->read_lock);
> > +	if (strict_strtol(buf, 10, &value)) {
> > +		mutex_unlock(&data->read_lock);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = sht15_send_status(data, SHT15_STATUS_OTP_RELOAD, !value);
> > +	if (ret) {
> Check ret after the mutex is unlocked to simplify this code a touch.
> > +		mutex_unlock(&data->read_lock);
> > +		return ret;
> > +	}
> > +
> > +	mutex_unlock(&data->read_lock);
> > +	return count;
> > +}
Fine.

Thanks,
Vivien.

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

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: sht15: add support for

[Jonathan Cameron]

... 
>>> * The usage of reload from OTP through the otp_reload sysfs attribute.
>> It's still a cryptic name.  Use fine calibration might be better terminology?
> For the moment, the field in the platform data is no_otp_reload, as in the
> datasheet. What would you suggest, 'calibration' instead?
Could do. Naming is less critical for something only controlled via platform
data (as people writing those files will need to be data sheet diving
anyway). As long as it is well documented it probably doesn't matter what it
is called.
> 
>>>
>>> It also fixes CRC calculation:
>>> CRC calculation now initializes the CRC register with the value of the
>>> lower nibble of the value of the status register, as described in the
>>> SHT1x CRC calculation technical note.
>> Hang on, so the original crc patch is broken?  Please rework this series
>> so that patch is valid in the first place!
> The original patch is not broken. The CRC calculation uses the lower nibble
> of the status register, which is 0 by default. This 'fix' comes here
> because this patch allow to change the status register, that is to say the
> lower nibble.
> It does not make sense to fix it in the first patch, before add the support
> to write the status register (even read it). Don't you agree?
Ah. I understand - didn't read your comment properly.  Please just replace
the word 'fixes' with say 'updates' in the description.
>>>   *
>> These docs kind of meant sense when they were a list of restrictions (basically a todo list).
>> I'd just drop them entirely now (other than the conservative timings one perhaps as I think
>> that is still true).
> Ok, should I remove this?
Yes.
> 
>>>  /**
>>>   * sht15_crc8() - compute crc8
>>> - * @data:	sht15 retrieved data
>>> + * @data:	sht15 specific data
>> Definitely roll this into the first patch.
>>> + * @value:	sht15 retrieved data
>>>   *
>>>   * This implements section 2 of the crc data sheet
>>>   */
>>> -static u8 sht15_crc8(const unsigned char *data, unsigned char len)
>>> +static u8 sht15_crc8(struct sht15_data *data,
>>> +		const unsigned char *value,
>>> +		unsigned char len)
>>>  {
>>> -	unsigned char crc = 0;
>>> +	unsigned char crc = reverse(data->val_status & 0x0F);
>>>  
>>>  	while (len--) {
>>> -		crc = sht15_crc8_table[*data ^ crc];
>>> -		data++;
>>> +		crc = sht15_crc8_table[*value ^ crc];
>>> +		value++;
> See the comment above about the CRC calcultation fix.
>>>  	}
>>>  
>>>  	return crc;
>>> @@ -360,6 +364,40 @@ static inline void sht15_soft_reset(struct sht15_data *data)
>>>  }
>>>  
>>>  /**
>> It's times like this when you wonder who decided to call it status on the
>> datasheet.  Now state would be a reasonable name, but the concept of changing
>> status seems a little odd!
>>
> You're right. Should I rename every sht15_*_status() functions into
> sht15_*_state()?
Difficult choice.  Do we stick with silly data sheet naming or fix it.
Personally I think in this case fix it and go with the name _state.
...

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

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: sht15: add support for

[Jean Delvare]

On Mon, 21 Mar 2011 20:29:57 -0400, Vivien Didelot wrote:
> On Mon, Mar 21, 2011 at 07:55:20PM +0000, Jonathan Cameron wrote:
> > Hang on, so the original crc patch is broken?  Please rework this series
> > so that patch is valid in the first place!
> The original patch is not broken. The CRC calculation uses the lower nibble
> of the status register, which is 0 by default. This 'fix' comes here
> because this patch allow to change the status register, that is to say the
> lower nibble.
> It does not make sense to fix it in the first patch, before add the support
> to write the status register (even read it). Don't you agree?

I don't. Sure, the hardware default for the low nibble of the "status"
register is 0, and the driver was leaving it untouched so far, but that
doesn't mean that the BIOS or firmware didn't change it before the
sht15 driver got loaded. The patch adding support for checksum
validation should handle this case properly.

> > (...)
> > It's times like this when you wonder who decided to call it status on the
> > datasheet.  Now state would be a reasonable name, but the concept of changing
> > status seems a little odd!

I guess it was difficult to find a suitable name, given that the high
nibble of the register holds status bits and the low nibble holds
configuration bits.

> You're right. Should I rename every sht15_*_status() functions into
> sht15_*_state()?

My opinion on this (which you are free to listen to or ignore): "state"
is hardly better than "status" to describe this register. So I would
either stick to "status" to match the datasheet, or go for "config" to
reflect the nature of the writable bits of the register.

Good night,
-- 
Jean Delvare

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

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: sht15: add support for

[Vivien Didelot]

Excerpts from Jean Delvare's message of 2011-03-26 17:50:31 -0400:
> I don't. Sure, the hardware default for the low nibble of the "status"
> register is 0, and the driver was leaving it untouched so far, but that
> doesn't mean that the BIOS or firmware didn't change it before the
> sht15 driver got loaded. The patch adding support for checksum
> validation should handle this case properly.
That's right. I'll switch the two patches. Firstly, the status
register support, then the CRC support.
> I guess it was difficult to find a suitable name, given that the high
> nibble of the register holds status bits and the low nibble holds
> configuration bits.
> 
> > You're right. Should I rename every sht15_*_status() functions into
> > sht15_*_state()?
> 
> My opinion on this (which you are free to listen to or ignore): "state"
> is hardly better than "status" to describe this register. So I would
> either stick to "status" to match the datasheet, or go for "config" to
> reflect the nature of the writable bits of the register.
I think I'll keep "status" because it won't make sense to write a
SHT15_CONFIG_BATTERY to match the battery state bit.
> 
> Good night,

Regards,
Vivien.

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