[RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Johannes Winkelmann]

this is an initial version of the driver for the upcoming Sensirion
SHT C1 humidity and temperature sensor. First hardware samples are
being tested by our key customers, and we'd therefore appreciate to
get feedback on the driver.

Datasheet URLs will be set as soon as there's a final version available.

Signed-off-by: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
---
 Documentation/hwmon/shtc1           |   34 +++++
 drivers/hwmon/Kconfig               |   10 ++
 drivers/hwmon/Makefile              |    1 +
 drivers/hwmon/shtc1.c               |  282 +++++++++++++++++++++++++++++++++++
 include/linux/platform_data/shtc1.h |   24 +++
 5 files changed, 351 insertions(+)
 create mode 100644 Documentation/hwmon/shtc1
 create mode 100644 drivers/hwmon/shtc1.c
 create mode 100644 include/linux/platform_data/shtc1.h

diff --git a/Documentation/hwmon/shtc1 b/Documentation/hwmon/shtc1
new file mode 100644
index 0000000..ada89f5
--- /dev/null
+++ b/Documentation/hwmon/shtc1
@@ -0,0 +1,34 @@
+Kernel driver shtc1
+===================
+
+Supported chips:
+  * Sensirion SHTC1
+    Prefix: 'shtc1'
+    Addresses scanned: none
+    Datasheet: To be announced
+
+Author:
+  Johannes Winkelmann <johannes.winkelmann@sensirion.com>
+
+Description
+-----------
+
+This driver implements support for the Sensirion  SHTC1 chip, a humidity and
+temperature sensor. Temperature is measured in degrees celsius, relative
+humidity is expressed as a percentage.
+
+The device communicates with the I2C protocol. All sensors are set to the same
+I2C address 0x70, so an entry with I2C_BOARD_INFO("shtc1", 0x70) can be used
+in the board setup code.
+
+Furthermore, there are two configuration options by means of platform_data:
+1. blocking (pull the I2C clock line down while performing the measurement) or
+   non-blocking, mode. Blocking mode will guarantee the fastest result, but
+   the I2C bus will be busy during that time
+2. high or low accuracy. Using high accuracy is always recommended.
+
+sysfs-Interface
+---------------
+
+temp1_input - temperature input
+humidity1_input - humidity input
\ No newline at end of file
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 6f1d167..cd5dced 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -902,6 +902,16 @@ config SENSORS_SHT21
 	  This driver can also be built as a module.  If so, the module
 	  will be called sht21.
 
+config SENSORS_SHTC1
+	tristate "Sensiron SHTC1 humidity and temperature sensor."
+	depends on I2C
+	help
+	  If you say yes here you get support for the Sensiron SHTC1
+	  humidity and temperature sensor.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called shtc1.
+
 config SENSORS_S3C
 	tristate "Samsung built-in ADC"
 	depends on S3C_ADC
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e1eeac1..d6d11d4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -107,6 +107,7 @@ obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
 obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
+obj-$(CONFIG_SENSORS_SHTC1)	+= shtc1.o
 obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
diff --git a/drivers/hwmon/shtc1.c b/drivers/hwmon/shtc1.c
new file mode 100644
index 0000000..b91d9ba
--- /dev/null
+++ b/drivers/hwmon/shtc1.c
@@ -0,0 +1,282 @@
+/* Sensirion SHTC1 humidity and temperature sensor driver
+ *
+ * Copyright (C) 2012 Sensirion AG, Switzerland
+ * Author: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
+ *
+ * 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 sheet available soon
+ *  TODO: add link
+ */
+
+#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/delay.h>
+#include <linux/platform_data/shtc1.h>
+
+/* commands (high precision mode) */
+static const char shtc1_cmd_measure_blocking_hpm[]    = { 0x7C, 0xA2 };
+static const char shtc1_cmd_measure_nonblocking_hpm[] = { 0x78, 0x66 };
+
+/* commands (low precision mode) */
+static const char shtc1_cmd_measure_blocking_lpm[]    = { 0x64, 0x58 };
+static const char shtc1_cmd_measure_nonblocking_lpm[] = { 0x60, 0x9c };
+
+/* delays for non-blocking i2c commands */
+/* TODO: use max timings */
+static const int shtc1_nonblocking_wait_time_hpm = 10;
+static const int shtc1_nonblocking_wait_time_lpm =  1;
+
+
+#define SHTC1_CMD_LENGTH 2
+#define SHTC1_RESPONSE_LENGTH 6
+
+struct shtc1_data {
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	unsigned int valid:1;
+	unsigned long last_updated; /* In jiffies */
+
+	const char *command;
+	unsigned int nonblocking_wait_time;
+
+	struct shtc1_pdata setup;
+
+	int temperature;
+	int humidity;
+};
+
+static void shtc1_select_command(struct shtc1_data *data)
+{
+	if (data->setup.high_precision) {
+		data->command = data->setup.blocking_io ?
+				shtc1_cmd_measure_blocking_hpm :
+				shtc1_cmd_measure_nonblocking_hpm;
+		data->nonblocking_wait_time = shtc1_nonblocking_wait_time_hpm;
+
+	} else {
+		data->command = data->setup.blocking_io ?
+				shtc1_cmd_measure_blocking_lpm :
+				shtc1_cmd_measure_nonblocking_lpm;
+		data->nonblocking_wait_time = shtc1_nonblocking_wait_time_lpm;
+	}
+}
+
+static int shtc1_update_values(struct i2c_client *client,
+			       struct shtc1_data *data,
+			       char *buf,
+			       int bufsize)
+{
+	int ret = i2c_master_send(client, data->command, SHTC1_CMD_LENGTH);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to send command");
+		return -1;
+	}
+
+	/*
+	 * in blocking mode (clock stretching mode) the I2C bus
+	 * is blocked for other traffic, thus the call to i2c_master_recv()
+	 * will wait until the data is ready. for non blocking mode, we
+	 * have to wait ourselves, thus the msleep()
+	 */
+	if (!data->setup.blocking_io)
+		msleep(data->nonblocking_wait_time);
+
+	ret = i2c_master_recv(client, buf, bufsize);
+	if (ret != bufsize) {
+		dev_err(&client->dev, "failed to read values: %d", ret);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* sysfs attributes */
+static struct shtc1_data *shtc1_update_client(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct shtc1_data *data = i2c_get_clientdata(client);
+
+	char buf[SHTC1_RESPONSE_LENGTH];
+	int val;
+	int ret;
+
+	mutex_lock(&data->update_lock);
+
+	/*
+	 * initialize 'ret' in case we had a valid result before, but
+	 * read too quickly in which case we return the last values
+	 */
+	ret = !data->valid;
+
+	if (time_after(jiffies, data->last_updated + HZ / 10)
+	    || !data->valid) {
+		ret = shtc1_update_values(client, data, buf, sizeof(buf));
+
+		if (ret)
+			goto out;
+
+		/*
+		 * From datasheet:
+		 *   T = -45 + 175 * ST / 2^16
+		 *   RH = -10 + 120 * SRH / 2^16
+		 *
+		 * Adapted for integer fixed point (3 digit) arithmetic
+		 */
+		val = (buf[0] << 8) | buf[1];
+		data->temperature = ((21875 * val) >> 13) - 45000;
+		val = (buf[3] << 8) | buf[4];
+		data->humidity = ((15000 * val) >> 13) - 10000;
+
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+out:
+	mutex_unlock(&data->update_lock);
+
+	return ret == 0 ? data : NULL;
+}
+
+static ssize_t shtc1_show_temperature(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct shtc1_data *data = shtc1_update_client(dev);
+	if (!data)
+		return -ENODEV;
+
+	return sprintf(buf, "%d\n", data->temperature);
+}
+
+static ssize_t shtc1_show_humidity(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct shtc1_data *data = shtc1_update_client(dev);
+	if (!data)
+		return -ENODEV;
+
+	return sprintf(buf, "%d\n", data->humidity);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
+			  shtc1_show_temperature, NULL, 0);
+static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO,
+			  shtc1_show_humidity, NULL, 0);
+
+static struct attribute *shtc1_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_humidity1_input.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group shtc1_attr_group = {
+	.attrs = shtc1_attributes,
+};
+
+
+
+static int __devinit shtc1_probe(struct i2c_client *client,
+				 const struct i2c_device_id *id)
+{
+	struct shtc1_data *data;
+	int err;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_err(&client->dev,
+			"adapter does not support SMBus word transactions\n");
+		return -ENODEV;
+	}
+
+	data = devm_kzalloc(&client->dev, sizeof(struct shtc1_data),
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/* defaults: blocking, high precision mode */
+	data->setup.blocking_io = 1;
+	data->setup.high_precision = 1;
+
+	if (client->dev.platform_data)
+		data->setup = *(struct shtc1_pdata *)client->dev.platform_data;
+	shtc1_select_command(data);
+
+	i2c_set_clientdata(client, data);
+	mutex_init(&data->update_lock);
+
+	err = sysfs_create_group(&client->dev.kobj, &shtc1_attr_group);
+	if (err) {
+		dev_dbg(&client->dev, "could not create sysfs files\n");
+		return err;
+	}
+	data->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		dev_dbg(&client->dev, "unable to register hwmon device\n");
+		err = PTR_ERR(data->hwmon_dev);
+		goto fail_remove_sysfs;
+	}
+
+
+	dev_info(&client->dev, "initialized\n");
+
+	return 0;
+
+fail_remove_sysfs:
+	sysfs_remove_group(&client->dev.kobj, &shtc1_attr_group);
+
+	return err;
+}
+
+
+/**
+ * shtc1_remove() - remove device
+ * @client: I2C client device
+ */
+static int __devexit shtc1_remove(struct i2c_client *client)
+{
+	struct shtc1_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &shtc1_attr_group);
+
+	return 0;
+}
+
+/* Device ID table */
+static const struct i2c_device_id shtc1_id[] = {
+	{ "shtc1", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, c1_id);
+
+static struct i2c_driver shtc1_driver = {
+	.driver.name = "shtc1",
+	.probe       = shtc1_probe,
+	.remove      = __devexit_p(shtc1_remove),
+	.id_table    = shtc1_id,
+};
+
+module_i2c_driver(shtc1_driver);
+
+MODULE_AUTHOR("Johannes Winkelmann <johannes.winkelmann@sensirion.com>");
+MODULE_DESCRIPTION("Sensirion SHTC1 humidity and temperature sensor driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/platform_data/shtc1.h b/include/linux/platform_data/shtc1.h
new file mode 100644
index 0000000..3f83dce
--- /dev/null
+++ b/include/linux/platform_data/shtc1.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2012 Sensirion AG, Switzerland
+ * Author: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ */
+
+#ifndef __SHTC1_H_
+#define __SHTC1_H_
+
+struct shtc1_pdata {
+	unsigned int blocking_io:1;
+	unsigned int high_precision:1;
+};
+
+#endif
-- 
1.7.9.5

Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Oliver Neukum]

On Friday 20 July 2012 14:57:22 Johannes Winkelmann wrote:

> +/* sysfs attributes */
> +static struct shtc1_data *shtc1_update_client(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct shtc1_data *data = i2c_get_clientdata(client);
> +
> +	char buf[SHTC1_RESPONSE_LENGTH];

Is this used for DMA?

> +	int val;
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	/*
> +	 * initialize 'ret' in case we had a valid result before, but
> +	 * read too quickly in which case we return the last values
> +	 */
> +	ret = !data->valid;
> +
> +	if (time_after(jiffies, data->last_updated + HZ / 10)
> +	    || !data->valid) {
> +		ret = shtc1_update_values(client, data, buf, sizeof(buf));
> +
> +		if (ret)
> +			goto out;
> +
> +		/*
> +		 * From datasheet:
> +		 *   T = -45 + 175 * ST / 2^16
> +		 *   RH = -10 + 120 * SRH / 2^16
> +		 *
> +		 * Adapted for integer fixed point (3 digit) arithmetic
> +		 */
> +		val = (buf[0] << 8) | buf[1];
> +		data->temperature = ((21875 * val) >> 13) - 45000;
> +		val = (buf[3] << 8) | buf[4];

We have dedicated macros for conversion of endianness.

> +		data->humidity = ((15000 * val) >> 13) - 10000;
> +
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret == 0 ? data : NULL;
> +}
	Regards
		Oliver

Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Johannes Winkelmann]

On Fri, Jul 20, 2012 at 4:03 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Friday 20 July 2012 14:57:22 Johannes Winkelmann wrote:
>
>> +/* sysfs attributes */
>> +static struct shtc1_data *shtc1_update_client(struct device *dev)
>> +{
>> +     struct i2c_client *client = to_i2c_client(dev);
>> +     struct shtc1_data *data = i2c_get_clientdata(client);
>> +
>> +     char buf[SHTC1_RESPONSE_LENGTH];
>
> Is this used for DMA?

No

>
>> +     int val;
>> +     int ret;
>> +
>> +     mutex_lock(&data->update_lock);
>> +
>> +     /*
>> +      * initialize 'ret' in case we had a valid result before, but
>> +      * read too quickly in which case we return the last values
>> +      */
>> +     ret = !data->valid;
>> +
>> +     if (time_after(jiffies, data->last_updated + HZ / 10)
>> +         || !data->valid) {
>> +             ret = shtc1_update_values(client, data, buf, sizeof(buf));
>> +
>> +             if (ret)
>> +                     goto out;
>> +
>> +             /*
>> +              * From datasheet:
>> +              *   T = -45 + 175 * ST / 2^16
>> +              *   RH = -10 + 120 * SRH / 2^16
>> +              *
>> +              * Adapted for integer fixed point (3 digit) arithmetic
>> +              */
>> +             val = (buf[0] << 8) | buf[1];
>> +             data->temperature = ((21875 * val) >> 13) - 45000;
>> +             val = (buf[3] << 8) | buf[4];
>
> We have dedicated macros for conversion of endianness.

Like this:
		val = swab16p((__le16 *)buf);
		data->temperature = ((21875 * val) >> 13) - 45000;
		val = swab16p((__le16 *)(buf+2));
		data->humidity = ((15000 * val) >> 13) - 10000;


>
>> +             data->humidity = ((15000 * val) >> 13) - 10000;
>> +
>> +             data->last_updated = jiffies;
>> +             data->valid = 1;
>> +     }
>> +
>> +out:
>> +     mutex_unlock(&data->update_lock);
>> +
>> +     return ret == 0 ? data : NULL;
>> +}
>         Regards
>                 Oliver
>

Thanks, Johannes

Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Oliver Neukum]

On Friday 20 July 2012 17:13:51 Johannes Winkelmann wrote:
> On Fri, Jul 20, 2012 at 4:03 PM, Oliver Neukum <oneukum@suse.de> wrote:

> > We have dedicated macros for conversion of endianness.
> 
> Like this:
> 		val = swab16p((__le16 *)buf);
> 		data->temperature = ((21875 * val) >> 13) - 45000;
> 		val = swab16p((__le16 *)(buf+2));
> 		data->humidity = ((15000 * val) >> 13) - 10000;

I was thinking of be16_to_cpu()

	Regards
		Oliver

Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Johannes Winkelmann]

On Fri, Jul 20, 2012 at 5:14 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Friday 20 July 2012 17:13:51 Johannes Winkelmann wrote:
>> On Fri, Jul 20, 2012 at 4:03 PM, Oliver Neukum <oneukum@suse.de> wrote:
>
>> > We have dedicated macros for conversion of endianness.
>>
>> Like this:
>>               val = swab16p((__le16 *)buf);
>>               data->temperature = ((21875 * val) >> 13) - 45000;
>>               val = swab16p((__le16 *)(buf+2));
>>               data->humidity = ((15000 * val) >> 13) - 10000;
>
> I was thinking of be16_to_cpu()

Ok, fixed locally; I'll collect some more feedback and will resubmit v2

Thanks,
Johannes
-- 
Johannes Winkelmann
jw@smts.ch

Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Guenter Roeck]

On Fri, Jul 20, 2012 at 05:14:49PM +0200, Oliver Neukum wrote:
> On Friday 20 July 2012 17:13:51 Johannes Winkelmann wrote:
> > On Fri, Jul 20, 2012 at 4:03 PM, Oliver Neukum <oneukum@suse.de> wrote:
> 
> > > We have dedicated macros for conversion of endianness.
> > 
> > Like this:
> > 		val = swab16p((__le16 *)buf);
> > 		data->temperature = ((21875 * val) >> 13) - 45000;
> > 		val = swab16p((__le16 *)(buf+2));
> > 		data->humidity = ((15000 * val) >> 13) - 10000;
> 
> I was thinking of be16_to_cpu()
> 
be16_to_cpup(), maybe.

Guenter

Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Guenter Roeck]

On Fri, Jul 20, 2012 at 02:57:22PM +0200, Johannes Winkelmann wrote:
> this is an initial version of the driver for the upcoming Sensirion
> SHT C1 humidity and temperature sensor. First hardware samples are
> being tested by our key customers, and we'd therefore appreciate to
> get feedback on the driver.
> 
> Datasheet URLs will be set as soon as there's a final version available.
> 
> Signed-off-by: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
> ---
>  Documentation/hwmon/shtc1           |   34 +++++
>  drivers/hwmon/Kconfig               |   10 ++
>  drivers/hwmon/Makefile              |    1 +
>  drivers/hwmon/shtc1.c               |  282 +++++++++++++++++++++++++++++++++++
>  include/linux/platform_data/shtc1.h |   24 +++
>  5 files changed, 351 insertions(+)
>  create mode 100644 Documentation/hwmon/shtc1
>  create mode 100644 drivers/hwmon/shtc1.c
>  create mode 100644 include/linux/platform_data/shtc1.h
> 
> diff --git a/Documentation/hwmon/shtc1 b/Documentation/hwmon/shtc1
> new file mode 100644
> index 0000000..ada89f5
> --- /dev/null
> +++ b/Documentation/hwmon/shtc1
> @@ -0,0 +1,34 @@
> +Kernel driver shtc1
> +===================
> +
> +Supported chips:
> +  * Sensirion SHTC1
> +    Prefix: 'shtc1'
> +    Addresses scanned: none
> +    Datasheet: To be announced
> +
> +Author:
> +  Johannes Winkelmann <johannes.winkelmann@sensirion.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for the Sensirion  SHTC1 chip, a humidity and
> +temperature sensor. Temperature is measured in degrees celsius, relative
> +humidity is expressed as a percentage.
> +
> +The device communicates with the I2C protocol. All sensors are set to the same
> +I2C address 0x70, so an entry with I2C_BOARD_INFO("shtc1", 0x70) can be used
> +in the board setup code.
> +
Please also provide a reference to the other means to instantiate the driver.

> +Furthermore, there are two configuration options by means of platform_data:
> +1. blocking (pull the I2C clock line down while performing the measurement) or
> +   non-blocking, mode. Blocking mode will guarantee the fastest result, but
> +   the I2C bus will be busy during that time
> +2. high or low accuracy. Using high accuracy is always recommended.
> +
> +sysfs-Interface
> +---------------
> +
> +temp1_input - temperature input
> +humidity1_input - humidity input
> \ No newline at end of file

Please  add a newline to the last line.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 6f1d167..cd5dced 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -902,6 +902,16 @@ config SENSORS_SHT21
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called sht21.
>  
> +config SENSORS_SHTC1
> +	tristate "Sensiron SHTC1 humidity and temperature sensor."
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for the Sensiron SHTC1
> +	  humidity and temperature sensor.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called shtc1.
> +
>  config SENSORS_S3C
>  	tristate "Samsung built-in ADC"
>  	depends on S3C_ADC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e1eeac1..d6d11d4 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -107,6 +107,7 @@ obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
>  obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>  obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> +obj-$(CONFIG_SENSORS_SHTC1)	+= shtc1.o
>  obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
>  obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
> diff --git a/drivers/hwmon/shtc1.c b/drivers/hwmon/shtc1.c
> new file mode 100644
> index 0000000..b91d9ba
> --- /dev/null
> +++ b/drivers/hwmon/shtc1.c
> @@ -0,0 +1,282 @@
> +/* Sensirion SHTC1 humidity and temperature sensor driver
> + *
> + * Copyright (C) 2012 Sensirion AG, Switzerland
> + * Author: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
> + *
> + * 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 sheet available soon
> + *  TODO: add link
> + */
> +
> +#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/delay.h>
> +#include <linux/platform_data/shtc1.h>
> +
> +/* commands (high precision mode) */
> +static const char shtc1_cmd_measure_blocking_hpm[]    = { 0x7C, 0xA2 };
> +static const char shtc1_cmd_measure_nonblocking_hpm[] = { 0x78, 0x66 };
> +
> +/* commands (low precision mode) */
> +static const char shtc1_cmd_measure_blocking_lpm[]    = { 0x64, 0x58 };
> +static const char shtc1_cmd_measure_nonblocking_lpm[] = { 0x60, 0x9c };
> +
Since some of the values are outside the char range, unsigned char might be
more appropriate.

> +/* delays for non-blocking i2c commands */
> +/* TODO: use max timings */
> +static const int shtc1_nonblocking_wait_time_hpm = 10;
> +static const int shtc1_nonblocking_wait_time_lpm =  1;

I don't see a reason for using variables here. Please use defines.

> +
> +
One empty line is sufficient.

> +#define SHTC1_CMD_LENGTH 2
> +#define SHTC1_RESPONSE_LENGTH 6

Please align "2" and "6"

> +
> +struct shtc1_data {
> +	struct device *hwmon_dev;
> +	struct mutex update_lock;
> +	unsigned int valid:1;

Please use bool. Bitmap does not have any value here except that access to it
is more expensive.

> +	unsigned long last_updated; /* In jiffies */
> +
> +	const char *command;

const unsigned char * ?

> +	unsigned int nonblocking_wait_time;
> +
> +	struct shtc1_pdata setup;
> +
> +	int temperature;
> +	int humidity;
> +};
> +
> +static void shtc1_select_command(struct shtc1_data *data)
> +{
> +	if (data->setup.high_precision) {
> +		data->command = data->setup.blocking_io ?
> +				shtc1_cmd_measure_blocking_hpm :
> +				shtc1_cmd_measure_nonblocking_hpm;
> +		data->nonblocking_wait_time = shtc1_nonblocking_wait_time_hpm;
> +
> +	} else {
> +		data->command = data->setup.blocking_io ?
> +				shtc1_cmd_measure_blocking_lpm :
> +				shtc1_cmd_measure_nonblocking_lpm;
> +		data->nonblocking_wait_time = shtc1_nonblocking_wait_time_lpm;
> +	}
> +}
> +
> +static int shtc1_update_values(struct i2c_client *client,
> +			       struct shtc1_data *data,
> +			       char *buf,
> +			       int bufsize)
> +{
> +	int ret = i2c_master_send(client, data->command, SHTC1_CMD_LENGTH);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to send command");
> +		return -1;

Don't hide error codes. Replace with return ret;

> +	}
> +
> +	/*
> +	 * in blocking mode (clock stretching mode) the I2C bus
> +	 * is blocked for other traffic, thus the call to i2c_master_recv()
> +	 * will wait until the data is ready. for non blocking mode, we
> +	 * have to wait ourselves, thus the msleep()
> +	 */
> +	if (!data->setup.blocking_io)
> +		msleep(data->nonblocking_wait_time);
> +
You might want to consider using usleep_range() for a more accurate wait time.

> +	ret = i2c_master_recv(client, buf, bufsize);
> +	if (ret != bufsize) {
> +		dev_err(&client->dev, "failed to read values: %d", ret);
> +		return -1;

	return ret;

> +	}
> +
> +	return 0;
> +}
> +
> +/* sysfs attributes */
> +static struct shtc1_data *shtc1_update_client(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct shtc1_data *data = i2c_get_clientdata(client);
> +
> +	char buf[SHTC1_RESPONSE_LENGTH];

I would suggest to use unsigned char to avoid problems with sign extension in
the calculations below.

> +	int val;
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	/*
> +	 * initialize 'ret' in case we had a valid result before, but
> +	 * read too quickly in which case we return the last values
> +	 */
> +	ret = !data->valid;
> +
> +	if (time_after(jiffies, data->last_updated + HZ / 10)
> +	    || !data->valid) {

One line is sufficient above.

HZ / 10 is a pretty fast update rate given the relatively long time needed to
actually read values from the sensor. Other drivers use times around HZ or even
larger. Not that I absolutely want you to change it - your call - but you might
consider some larger value.

> +		ret = shtc1_update_values(client, data, buf, sizeof(buf));
> +
> +		if (ret)
> +			goto out;
> +
> +		/*
> +		 * From datasheet:
> +		 *   T = -45 + 175 * ST / 2^16
> +		 *   RH = -10 + 120 * SRH / 2^16
> +		 *
> +		 * Adapted for integer fixed point (3 digit) arithmetic
> +		 */
> +		val = (buf[0] << 8) | buf[1];
> +		data->temperature = ((21875 * val) >> 13) - 45000;
> +		val = (buf[3] << 8) | buf[4];
> +		data->humidity = ((15000 * val) >> 13) - 10000;
> +

Is the returned value and signed or unsigned ?

> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret == 0 ? data : NULL;

don't hide error codes.

Use ERR_PTR(), and IS_ERR() / PTR_ERR in calling code.
Consider using struct shtc1_data *ret and initialize ret with
	ret = data;
then use
	ret = ERR_PTR(err) if an error occurs.

or do it similar to the sht21 driver.

> +}
> +
> +static ssize_t shtc1_show_temperature(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct shtc1_data *data = shtc1_update_client(dev);
> +	if (!data)
> +		return -ENODEV;

This error code is wrong. The device _does_ exist. Better return the real error from
shtc1_update_client() and use it.

> +
> +	return sprintf(buf, "%d\n", data->temperature);
> +}
> +
> +static ssize_t shtc1_show_humidity(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct shtc1_data *data = shtc1_update_client(dev);
> +	if (!data)
> +		return -ENODEV;

Same here.

> +
> +	return sprintf(buf, "%d\n", data->humidity);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
> +			  shtc1_show_temperature, NULL, 0);
> +static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO,
> +			  shtc1_show_humidity, NULL, 0);
> +
> +static struct attribute *shtc1_attributes[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group shtc1_attr_group = {
> +	.attrs = shtc1_attributes,
> +};
> +
> +
> +

One empty line is sufficient.

> +static int __devinit shtc1_probe(struct i2c_client *client,
> +				 const struct i2c_device_id *id)
> +{
> +	struct shtc1_data *data;
> +	int err;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WORD_DATA)) {
> +		dev_err(&client->dev,
> +			"adapter does not support SMBus word transactions\n");

You are not using SMBUS word transactions. Why check for it ?

> +		return -ENODEV;
> +	}
> +
> +	data = devm_kzalloc(&client->dev, sizeof(struct shtc1_data),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	/* defaults: blocking, high precision mode */
> +	data->setup.blocking_io = 1;
> +	data->setup.high_precision = 1;
> +
> +	if (client->dev.platform_data)
> +		data->setup = *(struct shtc1_pdata *)client->dev.platform_data;
> +	shtc1_select_command(data);
> +
> +	i2c_set_clientdata(client, data);
> +	mutex_init(&data->update_lock);
> +
> +	err = sysfs_create_group(&client->dev.kobj, &shtc1_attr_group);
> +	if (err) {
> +		dev_dbg(&client->dev, "could not create sysfs files\n");
> +		return err;
> +	}
> +	data->hwmon_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		dev_dbg(&client->dev, "unable to register hwmon device\n");
> +		err = PTR_ERR(data->hwmon_dev);
> +		goto fail_remove_sysfs;
> +	}
> +
> +
One empty line is sufficient.

> +	dev_info(&client->dev, "initialized\n");
> +
> +	return 0;
> +
> +fail_remove_sysfs:
> +	sysfs_remove_group(&client->dev.kobj, &shtc1_attr_group);
> +
> +	return err;
> +}
> +
> +
One empty line is sufficient.

> +/**
> + * shtc1_remove() - remove device
> + * @client: I2C client device
> + */
> +static int __devexit shtc1_remove(struct i2c_client *client)
> +{
> +	struct shtc1_data *data = i2c_get_clientdata(client);
> +
> +	hwmon_device_unregister(data->hwmon_dev);
> +	sysfs_remove_group(&client->dev.kobj, &shtc1_attr_group);
> +
> +	return 0;
> +}
> +
> +/* Device ID table */
> +static const struct i2c_device_id shtc1_id[] = {
> +	{ "shtc1", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, c1_id);
> +
> +static struct i2c_driver shtc1_driver = {
> +	.driver.name = "shtc1",
> +	.probe       = shtc1_probe,
> +	.remove      = __devexit_p(shtc1_remove),
> +	.id_table    = shtc1_id,
> +};
> +
> +module_i2c_driver(shtc1_driver);
> +
> +MODULE_AUTHOR("Johannes Winkelmann <johannes.winkelmann@sensirion.com>");
> +MODULE_DESCRIPTION("Sensirion SHTC1 humidity and temperature sensor driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/shtc1.h b/include/linux/platform_data/shtc1.h
> new file mode 100644
> index 0000000..3f83dce
> --- /dev/null
> +++ b/include/linux/platform_data/shtc1.h
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (C) 2012 Sensirion AG, Switzerland
> + * Author: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __SHTC1_H_
> +#define __SHTC1_H_
> +
> +struct shtc1_pdata {
> +	unsigned int blocking_io:1;
> +	unsigned int high_precision:1;

Please use bool (or int). Bitmaps don't provide real value here, except making
access to the values more expensive (and may result in undesired side effects).

> +};
> +
> +#endif
> -- 
> 1.7.9.5
> 
> 

Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Johannes Winkelmann]

Hi Guenter,

Thanks a lot for the feedback; I've incorporated most of it in
preparation for a v2 of the patch; I have some comments on the points
I'm not certain about:

On Sat, Jul 21, 2012 at 7:01 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Jul 20, 2012 at 02:57:22PM +0200, Johannes Winkelmann wrote:
>> this is an initial version of the driver for the upcoming Sensirion
>> SHT C1 humidity and temperature sensor. First hardware samples are
>> being tested by our key customers, and we'd therefore appreciate to
>> get feedback on the driver.
>>
>> Datasheet URLs will be set as soon as there's a final version available.
>>
>> Signed-off-by: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
>> ---
>>  Documentation/hwmon/shtc1           |   34 +++++
>>  drivers/hwmon/Kconfig               |   10 ++
>>  drivers/hwmon/Makefile              |    1 +
>>  drivers/hwmon/shtc1.c               |  282 +++++++++++++++++++++++++++++++++++
>>  include/linux/platform_data/shtc1.h |   24 +++
>>  5 files changed, 351 insertions(+)
>>  create mode 100644 Documentation/hwmon/shtc1
>>  create mode 100644 drivers/hwmon/shtc1.c
>>  create mode 100644 include/linux/platform_data/shtc1.h
>>
>> diff --git a/Documentation/hwmon/shtc1 b/Documentation/hwmon/shtc1
>> new file mode 100644
>> index 0000000..ada89f5
>> --- /dev/null
>> +++ b/Documentation/hwmon/shtc1
>> @@ -0,0 +1,34 @@
>> +Kernel driver shtc1
>> +===================
>> +
>> +Supported chips:
>> +  * Sensirion SHTC1
>> +    Prefix: 'shtc1'
>> +    Addresses scanned: none
>> +    Datasheet: To be announced
>> +
>> +Author:
>> +  Johannes Winkelmann <johannes.winkelmann@sensirion.com>
>> +
>> +Description
>> +-----------
>> +
>> +This driver implements support for the Sensirion  SHTC1 chip, a humidity and
>> +temperature sensor. Temperature is measured in degrees celsius, relative
>> +humidity is expressed as a percentage.
>> +
>> +The device communicates with the I2C protocol. All sensors are set to the same
>> +I2C address 0x70, so an entry with I2C_BOARD_INFO("shtc1", 0x70) can be used
>> +in the board setup code.
>> +
> Please also provide a reference to the other means to instantiate the driver.

I've checked a couple of documentation files and haven't found what
else I should add, in fact the other sensor documents I checked didn't
cover this, except for the one from the sht21 after which I modelled
the document. Could you give me a pointer where to look?


>> +Furthermore, there are two configuration options by means of platform_data:
>> +1. blocking (pull the I2C clock line down while performing the measurement) or
>> +   non-blocking, mode. Blocking mode will guarantee the fastest result, but
>> +   the I2C bus will be busy during that time
>> +2. high or low accuracy. Using high accuracy is always recommended.
>> +
>> +sysfs-Interface
>> +---------------
>> +
>> +temp1_input - temperature input
>> +humidity1_input - humidity input
>> \ No newline at end of file
>
> Please  add a newline to the last line.
>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 6f1d167..cd5dced 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -902,6 +902,16 @@ config SENSORS_SHT21
>>         This driver can also be built as a module.  If so, the module
>>         will be called sht21.
>>
>> +config SENSORS_SHTC1
>> +     tristate "Sensiron SHTC1 humidity and temperature sensor."
>> +     depends on I2C
>> +     help
>> +       If you say yes here you get support for the Sensiron SHTC1
>> +       humidity and temperature sensor.
>> +
>> +       This driver can also be built as a module.  If so, the module
>> +       will be called shtc1.
>> +
>>  config SENSORS_S3C
>>       tristate "Samsung built-in ADC"
>>       depends on S3C_ADC
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index e1eeac1..d6d11d4 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -107,6 +107,7 @@ obj-$(CONFIG_SENSORS_SCH5627)     += sch5627.o
>>  obj-$(CONFIG_SENSORS_SCH5636)        += sch5636.o
>>  obj-$(CONFIG_SENSORS_SHT15)  += sht15.o
>>  obj-$(CONFIG_SENSORS_SHT21)  += sht21.o
>> +obj-$(CONFIG_SENSORS_SHTC1)  += shtc1.o
>>  obj-$(CONFIG_SENSORS_SIS5595)        += sis5595.o
>>  obj-$(CONFIG_SENSORS_SMM665) += smm665.o
>>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>> diff --git a/drivers/hwmon/shtc1.c b/drivers/hwmon/shtc1.c
>> new file mode 100644
>> index 0000000..b91d9ba
>> --- /dev/null
>> +++ b/drivers/hwmon/shtc1.c
>> @@ -0,0 +1,282 @@
>> +/* Sensirion SHTC1 humidity and temperature sensor driver
>> + *
>> + * Copyright (C) 2012 Sensirion AG, Switzerland
>> + * Author: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
>> + *
>> + * 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 sheet available soon
>> + *  TODO: add link
>> + */
>> +
>> +#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/delay.h>
>> +#include <linux/platform_data/shtc1.h>
>> +
>> +/* commands (high precision mode) */
>> +static const char shtc1_cmd_measure_blocking_hpm[]    = { 0x7C, 0xA2 };
>> +static const char shtc1_cmd_measure_nonblocking_hpm[] = { 0x78, 0x66 };
>> +
>> +/* commands (low precision mode) */
>> +static const char shtc1_cmd_measure_blocking_lpm[]    = { 0x64, 0x58 };
>> +static const char shtc1_cmd_measure_nonblocking_lpm[] = { 0x60, 0x9c };
>> +
> Since some of the values are outside the char range, unsigned char might be
> more appropriate.
>
>> +/* delays for non-blocking i2c commands */
>> +/* TODO: use max timings */
>> +static const int shtc1_nonblocking_wait_time_hpm = 10;
>> +static const int shtc1_nonblocking_wait_time_lpm =  1;
>
> I don't see a reason for using variables here. Please use defines.
>
>> +
>> +
> One empty line is sufficient.
>
>> +#define SHTC1_CMD_LENGTH 2
>> +#define SHTC1_RESPONSE_LENGTH 6
>
> Please align "2" and "6"
>
>> +
>> +struct shtc1_data {
>> +     struct device *hwmon_dev;
>> +     struct mutex update_lock;
>> +     unsigned int valid:1;
>
> Please use bool. Bitmap does not have any value here except that access to it
> is more expensive.
>
>> +     unsigned long last_updated; /* In jiffies */
>> +
>> +     const char *command;
>
> const unsigned char * ?
>
>> +     unsigned int nonblocking_wait_time;
>> +
>> +     struct shtc1_pdata setup;
>> +
>> +     int temperature;
>> +     int humidity;
>> +};
>> +
>> +static void shtc1_select_command(struct shtc1_data *data)
>> +{
>> +     if (data->setup.high_precision) {
>> +             data->command = data->setup.blocking_io ?
>> +                             shtc1_cmd_measure_blocking_hpm :
>> +                             shtc1_cmd_measure_nonblocking_hpm;
>> +             data->nonblocking_wait_time = shtc1_nonblocking_wait_time_hpm;
>> +
>> +     } else {
>> +             data->command = data->setup.blocking_io ?
>> +                             shtc1_cmd_measure_blocking_lpm :
>> +                             shtc1_cmd_measure_nonblocking_lpm;
>> +             data->nonblocking_wait_time = shtc1_nonblocking_wait_time_lpm;
>> +     }
>> +}
>> +
>> +static int shtc1_update_values(struct i2c_client *client,
>> +                            struct shtc1_data *data,
>> +                            char *buf,
>> +                            int bufsize)
>> +{
>> +     int ret = i2c_master_send(client, data->command, SHTC1_CMD_LENGTH);
>> +     if (ret < 0) {
>> +             dev_err(&client->dev, "failed to send command");
>> +             return -1;
>
> Don't hide error codes. Replace with return ret;
>
>> +     }
>> +
>> +     /*
>> +      * in blocking mode (clock stretching mode) the I2C bus
>> +      * is blocked for other traffic, thus the call to i2c_master_recv()
>> +      * will wait until the data is ready. for non blocking mode, we
>> +      * have to wait ourselves, thus the msleep()
>> +      */
>> +     if (!data->setup.blocking_io)
>> +             msleep(data->nonblocking_wait_time);
>> +
> You might want to consider using usleep_range() for a more accurate wait time.

I see. I'll double check the worst case timings and look into it again.

>
>> +     ret = i2c_master_recv(client, buf, bufsize);
>> +     if (ret != bufsize) {
>> +             dev_err(&client->dev, "failed to read values: %d", ret);
>> +             return -1;
>
>         return ret;
>
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/* sysfs attributes */
>> +static struct shtc1_data *shtc1_update_client(struct device *dev)
>> +{
>> +     struct i2c_client *client = to_i2c_client(dev);
>> +     struct shtc1_data *data = i2c_get_clientdata(client);
>> +
>> +     char buf[SHTC1_RESPONSE_LENGTH];
>
> I would suggest to use unsigned char to avoid problems with sign extension in
> the calculations below.
>
>> +     int val;
>> +     int ret;
>> +
>> +     mutex_lock(&data->update_lock);
>> +
>> +     /*
>> +      * initialize 'ret' in case we had a valid result before, but
>> +      * read too quickly in which case we return the last values
>> +      */
>> +     ret = !data->valid;
>> +
>> +     if (time_after(jiffies, data->last_updated + HZ / 10)
>> +         || !data->valid) {
>
> One line is sufficient above.
>
> HZ / 10 is a pretty fast update rate given the relatively long time needed to
> actually read values from the sensor. Other drivers use times around HZ or even
> larger. Not that I absolutely want you to change it - your call - but you might
> consider some larger value.

True; In the SHT21 driver we added the rate limiting to avoid self
heating; for C1, these tests are still ongoing; I'll discuss this with
the chip guys though.


>> +             ret = shtc1_update_values(client, data, buf, sizeof(buf));
>> +
>> +             if (ret)
>> +                     goto out;
>> +
>> +             /*
>> +              * From datasheet:
>> +              *   T = -45 + 175 * ST / 2^16
>> +              *   RH = -10 + 120 * SRH / 2^16
>> +              *
>> +              * Adapted for integer fixed point (3 digit) arithmetic
>> +              */
>> +             val = (buf[0] << 8) | buf[1];
>> +             data->temperature = ((21875 * val) >> 13) - 45000;
>> +             val = (buf[3] << 8) | buf[4];
>> +             data->humidity = ((15000 * val) >> 13) - 10000;
>> +
>
> Is the returned value and signed or unsigned ?

It's signed (and I've changed buf to unsigned char as per your suggestion).


>
>> +             data->last_updated = jiffies;
>> +             data->valid = 1;
>> +     }
>> +
>> +out:
>> +     mutex_unlock(&data->update_lock);
>> +
>> +     return ret == 0 ? data : NULL;
>
> don't hide error codes.
>
> Use ERR_PTR(), and IS_ERR() / PTR_ERR in calling code.
> Consider using struct shtc1_data *ret and initialize ret with
>         ret = data;
> then use
>         ret = ERR_PTR(err) if an error occurs.
>
> or do it similar to the sht21 driver.

I went the ERR_PTR() + IS_ERR()/PTR_ERR() route.

>
>> +}
>> +
>> +static ssize_t shtc1_show_temperature(struct device *dev,
>> +                                   struct device_attribute *attr,
>> +                                   char *buf)
>> +{
>> +     struct shtc1_data *data = shtc1_update_client(dev);
>> +     if (!data)
>> +             return -ENODEV;
>
> This error code is wrong. The device _does_ exist. Better return the real error from
> shtc1_update_client() and use it.
>
>> +
>> +     return sprintf(buf, "%d\n", data->temperature);
>> +}
>> +
>> +static ssize_t shtc1_show_humidity(struct device *dev,
>> +                                struct device_attribute *attr,
>> +                                char *buf)
>> +{
>> +     struct shtc1_data *data = shtc1_update_client(dev);
>> +     if (!data)
>> +             return -ENODEV;
>
> Same here.
>
>> +
>> +     return sprintf(buf, "%d\n", data->humidity);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
>> +                       shtc1_show_temperature, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO,
>> +                       shtc1_show_humidity, NULL, 0);
>> +
>> +static struct attribute *shtc1_attributes[] = {
>> +     &sensor_dev_attr_temp1_input.dev_attr.attr,
>> +     &sensor_dev_attr_humidity1_input.dev_attr.attr,
>> +     NULL
>> +};
>> +
>> +static const struct attribute_group shtc1_attr_group = {
>> +     .attrs = shtc1_attributes,
>> +};
>> +
>> +
>> +
>
> One empty line is sufficient.
>
>> +static int __devinit shtc1_probe(struct i2c_client *client,
>> +                              const struct i2c_device_id *id)
>> +{
>> +     struct shtc1_data *data;
>> +     int err;
>> +
>> +     if (!i2c_check_functionality(client->adapter,
>> +                                  I2C_FUNC_SMBUS_WORD_DATA)) {
>> +             dev_err(&client->dev,
>> +                     "adapter does not support SMBus word transactions\n");
>
> You are not using SMBUS word transactions. Why check for it ?

True; taken from the SHT21 driver. I removed it now.

>
>> +             return -ENODEV;
>> +     }
>> +
>> +     data = devm_kzalloc(&client->dev, sizeof(struct shtc1_data),
>> +                         GFP_KERNEL);
>> +     if (!data)
>> +             return -ENOMEM;
>> +
>> +     /* defaults: blocking, high precision mode */
>> +     data->setup.blocking_io = 1;
>> +     data->setup.high_precision = 1;
>> +
>> +     if (client->dev.platform_data)
>> +             data->setup = *(struct shtc1_pdata *)client->dev.platform_data;
>> +     shtc1_select_command(data);
>> +
>> +     i2c_set_clientdata(client, data);
>> +     mutex_init(&data->update_lock);
>> +
>> +     err = sysfs_create_group(&client->dev.kobj, &shtc1_attr_group);
>> +     if (err) {
>> +             dev_dbg(&client->dev, "could not create sysfs files\n");
>> +             return err;
>> +     }
>> +     data->hwmon_dev = hwmon_device_register(&client->dev);
>> +     if (IS_ERR(data->hwmon_dev)) {
>> +             dev_dbg(&client->dev, "unable to register hwmon device\n");
>> +             err = PTR_ERR(data->hwmon_dev);
>> +             goto fail_remove_sysfs;
>> +     }
>> +
>> +
> One empty line is sufficient.
>
>> +     dev_info(&client->dev, "initialized\n");
>> +
>> +     return 0;
>> +
>> +fail_remove_sysfs:
>> +     sysfs_remove_group(&client->dev.kobj, &shtc1_attr_group);
>> +
>> +     return err;
>> +}
>> +
>> +
> One empty line is sufficient.
>
>> +/**
>> + * shtc1_remove() - remove device
>> + * @client: I2C client device
>> + */
>> +static int __devexit shtc1_remove(struct i2c_client *client)
>> +{
>> +     struct shtc1_data *data = i2c_get_clientdata(client);
>> +
>> +     hwmon_device_unregister(data->hwmon_dev);
>> +     sysfs_remove_group(&client->dev.kobj, &shtc1_attr_group);
>> +
>> +     return 0;
>> +}
>> +
>> +/* Device ID table */
>> +static const struct i2c_device_id shtc1_id[] = {
>> +     { "shtc1", 0 },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, c1_id);
>> +
>> +static struct i2c_driver shtc1_driver = {
>> +     .driver.name = "shtc1",
>> +     .probe       = shtc1_probe,
>> +     .remove      = __devexit_p(shtc1_remove),
>> +     .id_table    = shtc1_id,
>> +};
>> +
>> +module_i2c_driver(shtc1_driver);
>> +
>> +MODULE_AUTHOR("Johannes Winkelmann <johannes.winkelmann@sensirion.com>");
>> +MODULE_DESCRIPTION("Sensirion SHTC1 humidity and temperature sensor driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/platform_data/shtc1.h b/include/linux/platform_data/shtc1.h
>> new file mode 100644
>> index 0000000..3f83dce
>> --- /dev/null
>> +++ b/include/linux/platform_data/shtc1.h
>> @@ -0,0 +1,24 @@
>> +/*
>> + * Copyright (C) 2012 Sensirion AG, Switzerland
>> + * Author: Johannes Winkelmann <johannes.winkelmann@sensirion.com>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#ifndef __SHTC1_H_
>> +#define __SHTC1_H_
>> +
>> +struct shtc1_pdata {
>> +     unsigned int blocking_io:1;
>> +     unsigned int high_precision:1;
>
> Please use bool (or int). Bitmaps don't provide real value here, except making
> access to the values more expensive (and may result in undesired side effects).
>
>> +};
>> +
>> +#endif
>> --
>> 1.7.9.5

Thanks, Johannes
-- 
Johannes Winkelmann

Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor

[Guenter Roeck]

Hi Johannes,

On Thu, Jul 26, 2012 at 03:53:12PM +0200, Johannes Winkelmann wrote:
> Hi Guenter,
> 
> Thanks a lot for the feedback; I've incorporated most of it in
> preparation for a v2 of the patch; I have some comments on the points
> I'm not certain about:
> 
[ ... ]
> >> +
> >> +The device communicates with the I2C protocol. All sensors are set to the same
> >> +I2C address 0x70, so an entry with I2C_BOARD_INFO("shtc1", 0x70) can be used
> >> +in the board setup code.
> >> +
> > Please also provide a reference to the other means to instantiate the driver.
> 
> I've checked a couple of documentation files and haven't found what
> else I should add, in fact the other sensor documents I checked didn't
> cover this, except for the one from the sht21 after which I modelled
> the document. Could you give me a pointer where to look?
> 
Just add a reference to Documentation/i2c/instantiating-devices.

Thanks,
Guenter