From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752176Ab2GURBk (ORCPT ); Sat, 21 Jul 2012 13:01:40 -0400 Received: from mail.active-venture.com ([67.228.131.205]:61237 "EHLO mail.active-venture.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752011Ab2GURBi (ORCPT ); Sat, 21 Jul 2012 13:01:38 -0400 X-Originating-IP: 108.223.40.66 Date: Sat, 21 Jul 2012 10:01:50 -0700 From: Guenter Roeck To: Johannes Winkelmann Cc: linux-kernel@vger.kernel.org, lm-sensors@lm-sensors.org, Jean Delvare , Johannes Winkelmann Subject: Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor Message-ID: <20120721170150.GA13086@roeck-us.net> References: <1342789042-13433-1-git-send-email-johannes.winkelmann@sensirion.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1342789042-13433-1-git-send-email-johannes.winkelmann@sensirion.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > + > +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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* 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 "); > +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 > + * > + * 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 > >