Hi Guenter, Thanks a lot for comments and patience - with the testing you are right, I'm still waiting for board, where I will be able to run latest kernel and have the chip wired in - sorry for this... Thanks, Tomas On Mon, 2014-05-05 at 18:39 -0700, Guenter Roeck wrote: > Hi Tomas, > > On 05/05/2014 06:03 PM, Tomas Pop wrote: > > Hi Guenter, > > > > Here is a corrected version, I tried to address your comments and I'm sending it > > again with a kind request for comments - we will do some more extensive testing > > before the final submission, but I would like to make the code as complete as > > as possible before. Here is a list of changes to the previous version: > > > Comments below. But I don't think you tested this with real HW, did you ? :-) > > > * positive, but incorrect return codes/lengths from i2c_mater_* are checked > > * usleep_range used instead of msleep > > * DEVICE_ATTR used instead of SENSOR_DEVICE_ATTR, ATTRIBITE_GROUPS used > > * devm_hwmon_device_register_with_groups instead of devm_hwmon_device_register > > * direct calls to sysfs_create_group and sysfs_create_group dropped > > * shtc1_remove function dropped > > * SHTW1 added to documentation and Kconfig > > * documentation and formating updates/corrections > > > > I did re-compile with C=1 as suggested and it is clear. > > > > Patch was generated against kernel v3.15-rc3 > > > > Thanks, > > Tomas > > > > Signed-off-by: Tomas Pop <tomas.pop@xxxxxxxxxxxxx> > > --- > > Documentation/hwmon/shtc1 | 41 ++++++ > > drivers/hwmon/Kconfig | 10 ++ > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/shtc1.c | 266 ++++++++++++++++++++++++++++++++++++ > > include/linux/platform_data/shtc1.h | 24 ++++ > > 5 files changed, 342 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..47c9ed1 > > --- /dev/null > > +++ b/Documentation/hwmon/shtc1 > > @@ -0,0 +1,41 @@ > > +Kernel driver shtc1 > > +=================== > > + > > +Supported chips: > > + * Sensirion SHTC1 > > + Prefix: 'shtc1' > > + Addresses scanned: none > > + Datasheet: Publicly available at the Sensirion website > > + http://www.sensirion.com/file/datasheet_shtc1 > > + > > + * Sensirion SHTW1 > > + Prefix: 'shtc1' > > + Addresses scanned: none > > + Datasheet: Not publicly available > > + > > +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. Driver can be used as well for SHTW1 > > +chip, which has the same electrical interface. > > + > > +The device communicates with the I2C protocol. All sensors are set to I2C > > +address 0x70. See Documentation/i2c/instantiating-devices for methods to > > +instantiate the device. > > + > > +There are two options configurable by means of shtc1_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 strongly recommended. > > + > > +sysfs-Interface > > +--------------- > > + > > +temp1_input - temperature input > > +humidity1_input - humidity input > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index bc196f4..f95ba5b 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -1114,6 +1114,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 humidity and temperature sensors. SHTC1 and compat." > > + depends on I2C > > + help > > + If you say yes here you get support for the Sensiron SHTC1 and SHTW1 > > + 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 c48f987..1cc5273 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -125,6 +125,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..c94f670 > > --- /dev/null > > +++ b/drivers/hwmon/shtc1.c > > @@ -0,0 +1,266 @@ > > +/* Sensirion SHTC1 humidity and temperature sensor driver > > + * > > + * Copyright (C) 2014 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. > > + * > > + */ > > + > > +#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 unsigned char shtc1_cmd_measure_blocking_hpm[] = { 0x7C, 0xA2 }; > > +static const unsigned char shtc1_cmd_measure_nonblocking_hpm[] = { 0x78, 0x66 }; > > + > > +/* commands (low precision mode) */ > > +static const unsigned char shtc1_cmd_measure_blocking_lpm[] = { 0x64, 0x58 }; > > +static const unsigned char shtc1_cmd_measure_nonblocking_lpm[] = { 0x60, 0x9c }; > > + > > +/* command for reading the ID register */ > > +static const unsigned char shtc1_cmd_read_id_reg[] = { 0xef, 0xc8 }; > > + > > +/* constants for reading the ID register */ > > +#define SHTC1_ID_REG_CONTENT 0x07 > > +#define SHTC1_ID_REG_MASK 0x1f > > + > > +/* delays for non-blocking i2c commands, both in us */ > > +#define SHTC1_NONBLOCKING_WAIT_TIME_HPM 14400 > > +#define SHTC1_NONBLOCKING_WAIT_TIME_LPM 1000 > > + > > +#define SHTC1_CMD_LENGTH 2 > > +#define SHTC1_RESPONSE_LENGTH 6 > > + > > +struct shtc1_data { > > + struct device *hwmon_dev; > > Not needed. But you need the i2c client here. > > > + struct mutex update_lock; > > + bool valid; > > + unsigned long last_updated; /* In jiffies */ > > + > > + const unsigned char *command; > > + unsigned int nonblocking_wait_time; > > + > > + struct shtc1_platform_data setup; > > + > > + int temperature; > > + int humidity; > > +}; > > + > > +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: %d\n", ret); > > + return ret; > > + } > > + if (ret != SHTC1_CMD_LENGTH) { > > + dev_err(&client->dev, "wrong nr bytes written: %d\n", ret); > > + return -EIO; > > + } > > Can you merge those into one ? Something like > > if (ret < SHTC1_CMD_LENGTH) { > dev_err(&client->dev, "failed to send command: %d\n", ret); > return ret < 0 ? ret : -EIO; > } > > > + > > + /* > > + * 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 > > s/for/For/ > > > + * have to wait ourselves. > > + */ > > + if (!data->setup.blocking_io) > > + usleep_range(data->nonblocking_wait_time, > > + data->nonblocking_wait_time + 2000); > > + > In the low precision case, this results in a wait time of 1 to 3 ms. > How about giving it a 1 ms range ? Seems to me that should be enough > and an acceptable trade-off. > > > + ret = i2c_master_recv(client, buf, bufsize); > > + if (ret != bufsize) { > > + dev_err(&client->dev, "failed to read values: %d\n", ret); > > + if (ret < 0) > > + return ret; > > + return -EIO; > > or simpler > return ret < 0 ? ret : -EIO; > > > + } > > + > > + 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); > > + > > That won't work with the new ABI (please test ;-). > You need something like > struct shtc1_data *data = dev_get_drvdata(dev); > struct i2c_client *client = data->client; > > This is because the attributes are now attached to the hwmon device, > no longer to the i2c device. The ABI saves the pointer to 'data' > in drvdata for the hwmon device, so you can get it with dev_get_drvdata(). > > > + unsigned 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 = 0; > > A bit excessive. We all know C, or should. Just make it > int ret = 0; > above; you don't need any comments here. > > > + > > + 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 = 100 * SRH / 2^16 > > + * > > + * Adapted for integer fixed point (3 digit) arithmetic. > > + */ > > + val = be16_to_cpup((__be16 *)buf); > > + data->temperature = ((21875 * val) >> 13) - 45000; > > + val = be16_to_cpup((__be16 *)(buf + 3)); > > + data->humidity = ((12500 * val) >> 13); > > + > > + data->last_updated = jiffies; > > + data->valid = 1; > > + } > > + > > +out: > > + mutex_unlock(&data->update_lock); > > + > > + return ret == 0 ? data : ERR_PTR(ret); > > +} > > + > > +static ssize_t shtc1_show_temperature(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + struct shtc1_data *data = shtc1_update_client(dev); > > + if (IS_ERR(data)) > > + return PTR_ERR(data); > > + > > + 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 (IS_ERR(data)) > > + return PTR_ERR(data); > > + > > + return sprintf(buf, "%d\n", data->humidity); > > +} > > + > > +static DEVICE_ATTR(temp1_input, S_IRUGO, shtc1_show_temperature, NULL); > > +static DEVICE_ATTR(humidity1_input, S_IRUGO, shtc1_show_humidity, NULL); > > + > > +static struct attribute *shtc1_attrs[] = { > > + &dev_attr_temp1_input.attr, > > + &dev_attr_humidity1_input.attr, > > + NULL > > +}; > > +ATTRIBUTE_GROUPS(shtc1); > > + > > +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_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct shtc1_data *data; > > + int err; > > + struct i2c_adapter *adap = client->adapter; > > + char id_reg[2]; > > + > I would suggest a local variable dev here > struct device *dev = &client->dev; > to save all the client->dev dereferences below. > > > + if (!i2c_check_functionality(adap, I2C_FUNC_I2C)) { > > + dev_err(&client->dev, > > + "adapter does not support plain i2c transactions\n"); > > + return -ENODEV; > > + } > > + > > + err = i2c_master_send(client, shtc1_cmd_read_id_reg, SHTC1_CMD_LENGTH); > > + if (err < 0) { > > + dev_err(&client->dev, "could not send read command: %d\n", err); > > read id command > > > + return -ENODEV; > > + } > > + if (err != SHTC1_CMD_LENGTH) { > > + dev_err(&client->dev, "num bytes send is wrong: %d\n", err); > > sent > > Also, I would prefer a single if statement. > > > + return -ENODEV; > > + } > > + err = i2c_master_recv(client, id_reg, sizeof(id_reg)); > > + if (err != sizeof(id_reg)) { > > + dev_err(&client->dev, "could not read ID register: %d\n", err); > > + return -ENODEV; > > + } > > + if ((id_reg[1] & SHTC1_ID_REG_MASK) != SHTC1_ID_REG_CONTENT) { > > + dev_err(&client->dev, "ID register doesn't match\n"); > > + return -ENODEV; > > + } > > + > > + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + /* defaults: blocking, high precision mode */ > > + data->setup.blocking_io = true; > > + data->setup.high_precision = true; > > + > > + if (client->dev.platform_data) > > + data->setup = *(struct shtc1_platform_data *)client->dev.platform_data; > > Line too long here. One way to solve it would be to use a local dev variable > as sugested above. > > > + shtc1_select_command(data); > > + i2c_set_clientdata(client, data); > > Not needed. But you need to save client for the update function. > data->client = client; > > > + mutex_init(&data->update_lock); > > + > > + data->hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev, > > You don't need to keep hwmon_dev in the data structure. Just use a local variable. > > > + client->name, > > + data, > > + shtc1_groups); > > + if (IS_ERR(data->hwmon_dev)) > > + dev_dbg(&client->dev, "unable to register hwmon device\n"); > > + > > + return PTR_ERR_OR_ZERO(data->hwmon_dev); > > +} > > + > > +/* device ID table */ > > +static const struct i2c_device_id shtc1_id[] = { > > + { "shtc1", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, shtc1_id); > > + > > +static struct i2c_driver shtc1_i2c_driver = { > > + .driver.name = "shtc1", > > + .probe = shtc1_probe, > > + .id_table = shtc1_id, > > +}; > > + > > +module_i2c_driver(shtc1_i2c_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..b06e3db > > --- /dev/null > > +++ b/include/linux/platform_data/shtc1.h > > @@ -0,0 +1,24 @@ > > +/* > > + * Copyright (C) 2014 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_platform_data { > > + bool blocking_io; > > + bool high_precision; > > +}; > > + > > +#endif /* __SHTC1_H_ */ > > > _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
