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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx>
> +
> +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@xxxxxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxxxxx>");
> +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@xxxxxxxxxxxxx>
> + *
> + * 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
> 
> 

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


[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux