[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1399064369.3019.83.camel@ltli10.sensirion.lokal>
Date: Fri, 02 May 2014 13:59:29 -0700
From: Tomas Pop <tomas.pop.mff@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: "jdelvare@...e.de" <jdelvare@...e.de>, lm-sensors@...sensors.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
johannes.winkelmann@...sirion.com
Subject: Re: [RFC][PATCH v2] hwmon: add support for Sensirion SHTC1 sensor
Hi Guenter, thanks for comments! I will include them in third version,
but I have still few questions...
On Don, 2014-05-01 at 19:06 -0700, Guenter Roeck wrote:
> On 05/01/2014 04:05 PM, Tomas Pop wrote:
> > One more time this patch sent with correct settings of my email client
> > - I'm sorry for this.
> >
> > This is a second version of the driver for Sensirion SHTC1 humidity and
> > temperature sensor. Initial version was submitted in July 2012.
> > http://www.gossamer-threads.com/lists/linux/kernel/1569130#1569130
> >
> > We included suggested corrections formerly discussed in this list after
> > initial submission, but since it is quite a while, we are re-submitting
> > it again as a request for comments. Here is a list of important changes
> > to the initial version:
> >
> > * returning real error codes instead of -1 or -ENODEV
> > * using boolean variables instead of bitmaps where possible
> > * macros be16_to_cpup used for conversion of indianneess
> > * corrected formula for decoding of humidity and temperature values
> > * documentation update
> >
> > Patch was generated against kernel v3.15-rc3
> >
> > Signed-off-by: Tomas Pop <tomas.pop@...sirion.com>
> > ---
> > Documentation/hwmon/shtc1 | 38 +++++
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/shtc1.c | 323 ++++++++++++++++++++++++++++++++++++
> > include/linux/platform_data/shtc1.h | 24 +++
> > 5 files changed, 396 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..6a72ae2d
> > --- /dev/null
> > +++ b/Documentation/hwmon/shtc1
> > @@ -0,0 +1,38 @@
> > +Kernel driver shtc1
> > +===================
> > +
> > +Supported chips:
> > + * Sensirion SHTC1
> > + Prefix: 'shtc1'
> > + Addresses scanned: none
> > + Datasheet: Publicly available at the Sensirion website
> > + http://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/Humidity/Sensirion_Humidity_SHTC1_Datasheet.pdf
>
> Ok to add SHTW1 here if it is known to work.
> Just say:
> Datasheet: Not publicly available
Actually, there is no way to find out, if you are speaking
to SHTC1 or SHTW1. (i.e., the id is the same for both).
So I will add it here and we will provide link to data-sheet
later in a separate patch.
> > +
> > +Author:
> > + Johannes Winkelmann <johannes.winkelmann@...sirion.com>
> > +
> > +Description
> > +-----------
> > +
> > +This driver implements support for the Sensirion SHTC1 chip, a humidity and
>
> Two spaces
>
> > +temperature sensor. Temperature is measured in degrees celsius, relative
> > +humidity is expressed as a percentage. Driver can be used as well for SHTW1
> > +chip, that has the same electrical interface, but datasheet has not been
>
> ... for SHTW1, which has the same electrical interface.
>
> > +yet published.
> > +
>
> Either add support for the second now, or don't mention it at all
> (especially if the chip has a different ID and you don't want to add
> that ID at this point for some reason).
>
> > +The device communicates with the I2C protocol. All sensors are set to the same
>
> ... are set to I2C address 0x70.
>
> > +I2C address 0x70, so an entry with I2C_BOARD_INFO("shtc1", 0x70) can be used
> > +in the board setup code. See Documentation/i2c/instantiating-devices for
> > +other methods to instantiate the device.
> > +
> I would suggest to just refer to the instantiating-devices document and drop
> the I2C_BOARD_INFO example.
>
> > +Furthermore, there are two configuration options by means of platform_data:
>
> options configurable by means ...
>
> > +1. blocking (pull the I2C clock line down while performing the measurement) or
> > + non-blocking, mode. Blocking mode will guarantee the fastest result, but
>
> non-blocking mode (no comma)
>
> > + the I2C bus will be busy during that time
>
> that time.
>
> > +2. high or low accuracy. Using high accuracy is always recommended.
> > +
> > +sysfs-Interface
> > +---------------
> > +
> > +temp1_input - temperature input
> > +humidity1_input - humidity input
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index bc196f4..4d58149 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 humidity
>
> and SHTW1 ?
I will add SHTW1 here as well
>
> > + 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..96d398b
> > --- /dev/null
> > +++ b/drivers/hwmon/shtc1.c
> > @@ -0,0 +1,323 @@
> > +/* Sensirion SHTC1 humidity and temperature sensor driver
> > + *
> > + * Copyright (C) 2012 Sensirion AG, Switzerland
>
> 2014 ?
>
> > + * Author: Johannes Winkelmann <johannes.winkelmann@...sirion.com>
> > + *
> Is Johannes still involved ?
Yes, Johannes is aware of this re-submission, but I will try to take
care about the process this time. I will add him in CC.
>
> > + * 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.
> > + *
> > + * Data sheet available soon
> > + * TODO: add link
>
> Datasheet is referenced in documentation, so you don't need the link here.
> Better to keep it in one please, then there is only one place to change
> if it needs to be updated.
> > + */
> > +
> > +#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 and constants for reading the ID register */
> > +static const unsigned char shtc1_cmd_read_id_reg[] = { 0xef, 0xc8 };
> > +static const unsigned char shtc1_id_reg_content = 0x07;
> > +static const unsigned char shtc1_id_reg_mask = 0x1f;
> > +
> Please use defines here. There is no benefit to have one-byte constants.
> If the W1 chip has a different ID, you might as well add it in now and detect it.
>
> > +/* delays for non-blocking i2c commands */
> > +#define SHTC1_NONBLOCKING_WAIT_TIME_HPM 10
> > +#define SHTC1_NONBLOCKING_WAIT_TIME_LPM 1
>
> delay in what ? ms ? us ? seconds ?
> Please specify.
>
The delays are in miliseconds. You mean to use it in names - e.g.,
SHTC1_NONBLOCKING_WAIT_TIME_LPM_MS? Or comment is enough?
May be I should then rename
also data->nonblocking_wait_time to data->nonblocking_wait_time_ms...
> > +
> > +#define SHTC1_CMD_LENGTH 2
> > +#define SHTC1_RESPONSE_LENGTH 6
> > +
> > +struct shtc1_data {
> > + struct device *hwmon_dev;
> > + 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 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;
> > + }
> > +}
> > +
> Please move this function down, just before the probe function.
>
> > +static int shtc1_update_values(struct i2c_client *client,
> > + struct shtc1_data *data,
> > + char *buf,
> > + int bufsize)
>
> Please align continuation lines with (, and limit the number of continuation lines
> where possible (eg, buf and bufsize fit into one line).
>
> > +{
> > + int ret = i2c_master_send(client, data->command, SHTC1_CMD_LENGTH);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to send command: %d", ret);
>
> Newline missing.
>
> > + return ret;
> > + }
>
> There is another error condition: the function returns the number of bytes written,
> which can be smaller than SHTC1_CMD_LENGTH.
>
> > +
> > + /*
> > + * 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().
> > + *
> > + * TODO: consider usleep_range
>
> Please do ... and fix all the other TODOs in case I missed any.
Sure. I checked the documentation and if I understand it correctly, we should really use in this case something like:
if (!data->setup.blocking_io)
usleep_range(data->nonblocking_wait_time_min_us,
data->nonblocking_wait_time_max_us);
where max_time will be min_time plus some fixed margin, e.g, 2000us
sounds this reasonable to you?
>
> > + */
> > + 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);
>
> Newline.
>
> The return value can be >= 0, meaning the calling code can get confused.
> Please make sure that the function always returns a valid error code.
Ok, so return -EIO or better -ENODEV?
>
> > + 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);
> > +
> > + 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 = !data->valid;
>
> So ret gets pre-initialized with 1 if valid == 0. Doesn't really make sense.
> Just initialize it with 0 above.
>
> int ret = 0;
Sure, thanks.
>
> > +
> > + if (time_after(jiffies, data->last_updated + HZ / 10) || !data->valid) {
> > + ret = shtc1_update_values(client, data, buf, sizeof(buf));
> > +
> Unnecessary empty line.
>
> > + 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));
>
> Spaces before and after '+'
>
> Just wondering .. what is in buf[2] and buf[5] ?
There are two CRC-8 checksums:
buf[2] for humidity, buf[5] for temperature value. We would prefer
not to check CRC it in this version of the driver...
>
> > + data->humidity = ((12500 * val) >> 13);
> > +
> > + data->last_updated = jiffies;
> > + data->valid = 1;
>
> = true;
>
> > + }
> > +
> > +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 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);
>
> You don't need SENSOR_DEVICE_ATTR here. DEVICE_ATTR is sufficient.
>
> Another option would be to store temperature and humidity in an array
> and use the last parameter of SENSOR_DEVICE_ATTR to index the array.
> This way you would only need a single show function. I'll leave
> that up to you.
Ok, then we will use DEVICE_ATTR...
>
> > +
> > +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,
> > +};
>
> Please use ATTRIBUTE_GROUPS().
>
> > +
> > +static int shtc1_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct shtc1_data *data;
>
> A local variable
> struct device *dev = &client->dev;
> would save you a number of dereference operations.
>
> > + int err;
> > + struct i2c_adapter *adap = client->adapter;
> > + char id_reg[2];
> > +
> > + 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);
> > +
> Please no empty lines before error checks.
>
> > + if (err < 0) {
>
> Also err != SHTC1_CMD_LENGTH.
This is similar to my question above, do you think it is better in that
case return -EIO or -ENODEV?
>
> > + dev_err(&client->dev, "could not send read command: %d\n", err);
> > + 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(struct shtc1_data),
>
> sizeof(*data) is preferred.
>
> > + GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + /* defaults: blocking, high precision mode */
> > + data->setup.blocking_io = 1;
> > + data->setup.high_precision = 1;
> > +
> = true;
> for both.
>
> > + if (client->dev.platform_data)
> > + data->setup = *(struct shtc1_platform_data *)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");
> > + goto fail_free;
> > + return err;
>
> goto or return, but not both (doesn't this create a warning about unreachable code ?).
> goto is unnecessary, actually. return err is just fine.
I think, there were no warnings, but of course you are right...
>
> > + }
> > + data->hwmon_dev = hwmon_device_register(&client->dev);
>
> Please use devm_hwmon_device_register_with_groups().
>
> struct device *hwmon_dev;
> ...
> hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev, client->name,
> data, sht1c_groups);
> return PTR_ERR_OR_ZERO(hwmon_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;
> > + }
> > +
> > +
> No more than one empty line, please.
>
> > + dev_info(&client->dev, "initialized\n");
>
> Unnecessary noise.
>
> > +
> > + return 0;
> > +
> > +fail_remove_sysfs:
> > + sysfs_remove_group(&client->dev.kobj, &shtc1_attr_group);
> > +fail_free:
> > + kfree(data);
>
> Unnecessary kfree.
I will remove these two lines at all.
>
> > +
> > + return err;
> > +}
> > +
> > +/**
> > + * shtc1_remove() - remove device
> > + * @client: I2C client device
> > + */
> > +static int 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;
> > +}
>
> You can drop this entire function with the new hwmon API.
>
> > +
> > +/* 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,
> > + .remove = shtc1_remove,
> > + .id_table = shtc1_id,
> > +};
> > +
> > +/**
> > + * shtc1_init() - initialize driver
> > + *
> > + * Called when kernel is booted or module is inserted.
> > + * Returns 0 on success.
> > + */
> > +static int __init shtc1_init(void)
> > +{
> > + return i2c_add_driver(&shtc1_i2c_driver);
> > +}
> > +module_init(shtc1_init);
> > +
> > +/*
> > + * shtc1_exit() - clean up driver
> > + *
> > + * Called when module is removed.
> > + */
> > +static void __exit shtc1_exit(void)
> > +{
> > + i2c_del_driver(&shtc1_i2c_driver);
> > +}
> > +
> > +module_exit(shtc1_exit);
>
> Please use module_i2c_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..4629364
> > --- /dev/null
> > +++ b/include/linux/platform_data/shtc1.h
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Copyright (C) 2012 Sensirion AG, Switzerland
>
> 2014 ?
>
> > + * 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_platform_data {
> > + bool blocking_io;
> > + bool high_precision;
> > +};
> > +
>
> Please no empty line at the end.
>
> Wondering ... do you plan to implement devicetree support (or have customers who need it) ?
>
> I could imagine that a chip like this will be used, for example, on beagleboard, which would
> require devicetree support.
>
Right now, I'm not aware of any particular time plan to implement device
tree support, but this may change, if somebody will request it...
> Thanks,
> Guenter
>
Thanks,
Tomas
--
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