| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120721170150.GA13086@roeck-us.net>
Date: Sat, 21 Jul 2012 10:01:50 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Johannes Winkelmann <johannes.winkelmann@...il.com>
Cc: linux-kernel@...r.kernel.org, lm-sensors@...sensors.org,
Jean Delvare <khali@...ux-fr.org>,
Johannes Winkelmann <johannes.winkelmann@...sirion.com>
Subject: Re: [RFC][PATCH] hwmon: add support for Sensirion C1 sensor
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@...sirion.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@...sirion.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@...sirion.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@...sirion.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@...sirion.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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists