hwmon: Honeywell Humidicon HIH-6130/HIH-6131 humidity and temperature sensor dr

Hi,

This is a new driver for the Honeywell Humidicon HIH-6130 humidity sensor.

I don't claim originality, the driver is mostly identical to the existing Sensiron sht21 driver with the necessary changes to the probe, update_measurements and conversion functions necessary to use the Honeywell sensors.

My first kernel patch, please go easy on me as I'm bound to be doing something stupid :)

drivers/hwmon/Kconfig +7 -0

+	help
+	  If you say yes here you get support for Honeywell HIH-613x
+	  Humidicon humidity sensors.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called hih613x.
+
 config SENSORS_CORETEMP
 	tristate "Intel Core/Core2/Atom temperature sensor"
 	depends on X86 && PCI && EXPERIMENTAL

drivers/hwmon/Makefile +1 -0

index e1eeac1..b92447c 100644
@@ -58,6 +58,7 @@ obj-$(CONFIG_SENSORS_G760A)	+= g760a.o
 obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
 obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
 obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
+obj-$(CONFIG_SENSORS_HIH613X)	+= hih613x.o
 obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
 obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
 obj-$(CONFIG_SENSORS_IBMAEM)	+= ibmaem.o

drivers/hwmon/hih613x.c new file +39 -0

new file mode 100644
index 0000000..0c8693c
@@ -0,0 +1,297 @@
+/* Honeywell HIH-613x humidity and temperature sensor driver
+ *
+ * Copyright (C) 2012 Iain Paton <[email protected]>
+ *
+ * heavily based on the sht21 driver
+ * Copyright (C) 2010 Urs Fleisch <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * Data sheets available (6/2012) at
+ * http://sensing.honeywell.com/index.php?ci_id106&la_id=1&defIdD872
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+
+/**
+ * struct hih613x - HIH-613x device specific data
+ * @hwmon_dev: device registered with hwmon

drivers/hwmon/hih613x.c new file +4 -0

+	unsigned long last_update;
+	int temperature;
+	int humidity;
+};

drivers/hwmon/hih613x.c new file +5 -0

+}
+
+/**
+ * hih613x_rh_ticks_to_per_cent_mille() - convert raw humidity ticks to
+ * one-thousandths of a percent relative humidity

drivers/hwmon/hih613x.c new file +4 -0

+}
+
+/**
+ * hih613x_update_measurements() - get updated measurements from device

drivers/hwmon/hih613x.c new file +7 -0

+	unsigned char tmp[4];
+	struct i2c_msg msgs[1] = {
+		{
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.len = 4,
+			.buf = tmp,

drivers/hwmon/hih613x.c new file +9 -0

+	}
+out:
+	mutex_unlock(&hih613x->lock);
+
+	return ret >= 0 ? 0 : ret;
+}
+
+/**
+ * hih613x_show_temperature() - show temperature measurement value in sysfs

drivers/hwmon/hih613x.c new file +11 -0

+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct hih613x *hih613x = i2c_get_clientdata(client);
+	int ret = hih613x_update_measurements(client);
+	if (ret < 0)
+		return ret;
+	return sprintf(buf, "%d\n", hih613x->temperature);
+}
+
+/**
+ * hih613x_show_humidity() - show humidity measurement value in sysfs

drivers/hwmon/hih613x.c new file +32 -0

+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct hih613x *hih613x = i2c_get_clientdata(client);
+	int ret = hih613x_update_measurements(client);
+	if (ret < 0)
+		return ret;
+	return sprintf(buf, "%d\n", hih613x->humidity);
+}
+
+/* sysfs attributes */
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, hih613x_show_temperature,
+	NULL, 0);
+static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO, hih613x_show_humidity,
+	NULL, 0);
+
+static struct attribute *hih613x_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_humidity1_input.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group hih613x_attr_group = {
+	.attrs = hih613x_attributes,
+};
+
+/**
+ * hih613x_probe() - probe device
+ * @client: I2C client device
+ * @id: device ID
+ *
+ * Called by the I2C core when an entry in the ID table matches a
+ * device's name.

drivers/hwmon/hih613x.c new file +11 -0

+		return -ENOMEM;
+	}
+
+	i2c_set_clientdata(client, hih613x);
+
+	mutex_init(&hih613x->lock);
+
+	err = sysfs_create_group(&client->dev.kobj, &hih613x_attr_group);
+	if (err) {
+		dev_dbg(&client->dev, "could not create sysfs files\n");
+		return err;

drivers/hwmon/hih613x.c new file +31 -0

+
+	return 0;
+
+fail_remove_sysfs:
+	sysfs_remove_group(&client->dev.kobj, &hih613x_attr_group);
+	return err;
+}
+
+/**
+ * hih613x_remove() - remove device
+ * @client: I2C client device
+ */
+static int __devexit hih613x_remove(struct i2c_client *client)
+{
+	struct hih613x *hih613x = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(hih613x->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &hih613x_attr_group);
+
+	return 0;
+}
+
+/* Device ID table */
+static const struct i2c_device_id hih613x_id[] = {
+	{ "hih613x", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, hih613x_id);
+
+static struct i2c_driver hih613x_driver = {
+	.driver.name = "hih613x",

Please run the patch through checkpatch and fix the warnings and errors (too long lines and missing sign-off).

Please provide Documentation/hwmon/hih613x (or, rather, hih6130 - see next paragraph).

Please spell out the chip types. "hih613x" could include anything from HIH6130 to HIH6139 (or even HIH613Z); if any of those is ever released, things would get confusing. I would suggest to name the driver hih6130 and refer to the chips as HIH6130/6131 as in the data sheet.

drivers/hwmon/hih613x.c line 68

Quoted 3 lines

drivers/hwmon/hih613x.c new file +3 -0

+	 * Formula T = ( ticks / ( 2^14 - 2 ) ) * 165 -40 from data sheet section 5.0,
+	 */
+	return ((ticks * 165000) / 16382) - 40000;

Would DIV_ROUND_CLOSEST make sense here ? Also, is it guaranteed that you don't get an integer overflow ?

drivers/hwmon/hih613x.c line 83

Quoted 1 line

drivers/hwmon/hih613x.c new file +1 -0

+	return (ticks * 100000) / 16382;

Would DIV_ROUND_CLOSEST make sense here ?
Is it guaranteed that there won't be an integer overflow ?

drivers/hwmon/hih613x.c line 114

Quoted 5 lines

drivers/hwmon/hih613x.c new file +5 -0

+	/*
+	 * Unlike the SHT2x the HIH-613x datasheet doesn't give limits on the number of
+	 * measurements per second we can make. However it makes no sense to allow polling
+	 * for humidity or temperature more than a couple of times a second anyway.
+	 */

It isn't really relevant what SHT2x does here. More important would be to mention that a measurement takes a long time, so you want to limit it. Since a measurement cycle takes 40 ms, you might actually want to consider increasing the timeout to HZ or even HZ + HZ/2.

Also, is there another accuracy ? I didn't find it in the documentation. If the measurement cycle can be longer based on the configuration, reading the measurement results might always fail, which would be bad.

drivers/hwmon/hih613x.c line 137

Quoted 1 line

drivers/hwmon/hih613x.c new file +1 -0

+		t = (tmp[0]<<8) + tmp[1];

Coding style: spaces before and after <<

drivers/hwmon/hih613x.c line 164

Quoted 3 lines

drivers/hwmon/hih613x.c new file +3 -0

+static ssize_t hih613x_show_temperature(struct device *dev,
+	struct device_attribute *attr,
+	char *buf)

I don't really like the alignment here. I would suggest to align with (, and use one less line.

drivers/hwmon/hih613x.c line 256

Quoted 1 line

drivers/hwmon/hih613x.c new file +1 -0

+	dev_info(&client->dev, "initialized\n");

Is this valuable or just noise ?

On 06/21/2012 06:42 PM, Guenter Roeck wrote:

Quoted 7 lines

drivers/hwmon/hih613x.c line 68

Quoted 4 lines

drivers/hwmon/hih613x.c new file +3 -0

+	 * Formula T = ( ticks / ( 2^14 - 2 ) ) * 165 -40 from data sheet section 5.0,
+	 */
+	return ((ticks * 165000) / 16382) - 40000;

Would DIV_ROUND_CLOSEST make sense here ? Also, is it guaranteed that you don't get an integer overflow ?

I didn't know about DIV_ROUND_CLOSEST, so yes I can use it. What do I gain from doing so ? If I read it right it'll force a round up rather than a round down ?

The stated accuracy of the sensor, 5% RH and +-1C, seems like a round up or round down would be lost in the noise.
So what's preferred ? Either way seems fine to me - unless I'm missing something.

For the integer overflow I was thinking it was ok, but I can see that with a 32 bit signed int the ticks * 165000 could end up giving me a negative number. So let me try re-arranging this.

On the subject of using int, I suppose I've assumed an int will never be less than 32 bits. Is that valid across different arches ? Or should I be using a different type like s32 or s64 to ensure it'll always work ?

Quoted 4 lines

drivers/hwmon/hih613x.c line 83

Quoted 1 line

drivers/hwmon/hih613x.c new file +1 -0

+	return (ticks * 100000) / 16382;

Would DIV_ROUND_CLOSEST make sense here ?
Is it guaranteed that there won't be an integer overflow ?

the ticks value can only ever be max 2^14, so I don't believe there's a problem with this one. Same question as above on whether I should use a fixed type to avoid problems if the size of int changes.

I guess nothing in here is fast path, especially with i2c and a 40ms delay, but I assume just changing these to 64 bit is going to incurr some penalty on a 32 bit arch. Is there a preferred way to deal with this ?

Quoted 10 lines

drivers/hwmon/hih613x.c line 114

Quoted 5 lines

drivers/hwmon/hih613x.c new file +5 -0

+	/*
+	 * Unlike the SHT2x the HIH-613x datasheet doesn't give limits on the number of
+	 * measurements per second we can make. However it makes no sense to allow polling
+	 * for humidity or temperature more than a couple of times a second anyway.
+	 */

It isn't really relevant what SHT2x does here. More important would be to mention that a measurement takes a long time, so you want to limit it. Since a measurement cycle takes 40 ms, you might actually want to consider increasing the timeout to HZ or even HZ + HZ/2.

I was originally going to leave this out altogether, but a couple of things made me keep it.
You do a single i2c read to get 4 bytes that includes humidity and temperature, you can do a 2 byte read to get just humidity, but can't get temperature on it's own. Testing my patch to the sensors command to add humidity display showed three reads going out onto the bus within ~180ms, all getting essentially the same data. So rather than do that it seemed better to put this in.

So thinking about this again, and re-reading the datasheet, essentially the 40ms is probably irrelevant in the larger picture. I can't find anything to say that there's any problem with requesting a reading every 40ms, however there's response times for given volume of airflow of up to 8 seconds for humidity and 30 seconds for temperature.
So it's clear that we're not going to get anything meaningful by reading every 40ms and if the rate of change required that frequency then you probably need a better sensor and a real-time OS. So I'll change and re-word the comment.

Quoted 3 lines

Also, is there another accuracy ? I didn't find it in the documentation. If the measurement cycle can be longer based on the configuration, reading the measurement results might always fail, which would be bad.

There are hints in the docs that either the device is capable of more or perhaps they will release a different sensor with different capabilities.
For example there's at least one mention of being able to configure it for SPI, but no details on how to do that.
If there is another accuracy setting it'll most likely be configured by one of the reserved EEPROM locations, the actual measurement and data read are somewhat simple and don't give much in the way of options.
As we'd have to enter command mode within either 3ms or 10ms of sensor power on, doing this isn't possible without additional hardware.

There's only one mention of the measurement cycle time that I could find and I'd say it's meaning is somewhat ambiguous.
So we could maybe add a loop, with a limited number of retries, check the status bits and break on valid data. How many retries to use would be a guess though.

Quoted 3 lines

drivers/hwmon/hih613x.c line 137

Quoted 1 line

drivers/hwmon/hih613x.c new file +1 -0

+		t = (tmp[0]<<8) + tmp[1];

Coding style: spaces before and after <<

I don't remember checkpatch complaining about it, but ok :)

Also I wasn't sure if these would be properly portable, is there a helper function that can take care of that for me? I couldn't find one, but I can't believe nobody else does this.

Quoted 5 lines

drivers/hwmon/hih613x.c line 164

Quoted 3 lines

drivers/hwmon/hih613x.c new file +3 -0

+static ssize_t hih613x_show_temperature(struct device *dev,
+	struct device_attribute *attr,
+	char *buf)

I don't really like the alignment here. I would suggest to align with (, and use one less line.

Hmm, like this:
static ssize_t hih6130_show_temperature(struct device *dev,
struct device_attribute *attr, char *buf)

makes an 82 char line and checkpatch hates me :)