Message-ID: <CAEYbzYsTQttoOnkoYqw9RkDO1zUc3qpyg0CzEY11_=oqdk0H-Q@mail.gmail.com>
Date:	Thu, 26 Jul 2012 15:53:12 +0200
From:	Johannes Winkelmann <johannes.winkelmann@...il.com>
To:	Guenter Roeck <linux@...ck-us.net>
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

Hi Guenter,

Thanks a lot for the feedback; I've incorporated most of it in
preparation for a v2 of the patch; I have some comments on the points
I'm not certain about:

On Sat, Jul 21, 2012 at 7:01 PM, Guenter Roeck <linux@...ck-us.net> wrote:
> 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.

I've checked a couple of documentation files and haven't found what
else I should add, in fact the other sensor documents I checked didn't
cover this, except for the one from the sht21 after which I modelled
the document. Could you give me a pointer where to look?


>> +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.

I see. I'll double check the worst case timings and look into it again.

>
>> +     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.

True; In the SHT21 driver we added the rate limiting to avoid self
heating; for C1, these tests are still ongoing; I'll discuss this with
the chip guys though.


>> +             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 ?

It's signed (and I've changed buf to unsigned char as per your suggestion).


>
>> +             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.

I went the ERR_PTR() + IS_ERR()/PTR_ERR() route.

>
>> +}
>> +
>> +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 ?

True; taken from the SHT21 driver. I removed it now.

>
>> +             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

Thanks, Johannes
-- 
Johannes Winkelmann
--
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