Re: [PATCH 1/1 v2] hwmon: add support for Sensirion SHT3x sensors

[Pascal Sachs]

Am 15.05.2016 um 03:34 schrieb Guenter Roeck:
> On 04/25/2016 01:42 AM, Pascal Sachs wrote:
>> From: David Frey <david.frey@xxxxxxxxxxxxx>
>>
>> This driver implements support for the Sensirion SHT3x-DIS chip,
>> a humidity and temperature sensor. Temperature is measured
>> in degrees celsius, relative humidity is expressed as a percentage.
>> In the sysfs interface, all values are scaled by 1000,
>> i.e. the value for 31.5 degrees celsius is 31500.
>>
>> Signed-off-by: Pascal Sachs <pascal.sachs@xxxxxxxxxxxxx>
>> ---
>> Patch was generated against kernel v4.6-rc5
>>
>>   Documentation/hwmon/sht3x           |  66 ++++
>>   drivers/hwmon/Kconfig               |  10 +
>>   drivers/hwmon/Makefile              |   1 +
>>   drivers/hwmon/sht3x.c               | 675
>> ++++++++++++++++++++++++++++++++++++
>>   drivers/hwmon/twl4030-madc-hwmon.c  |   1 +
>
> Please drop this change.

My bad, don't know how I could miss this :-(
>
>>   include/linux/platform_data/sht3x.h |  25 ++
>>   6 files changed, 778 insertions(+)
>>   create mode 100644 Documentation/hwmon/sht3x
>>   create mode 100644 drivers/hwmon/sht3x.c
>>   create mode 100644 include/linux/platform_data/sht3x.h
>>
>
> Please run checkpatch --strict over your patch. There are lots of
> unnecessary/undesirable empty lines and spaces. It would also tell you
> about the presumably left-over FIXME in twl4030-madc-hwmon.c.

Thank you, I added the --strict argument to our tests, so that we don't
miss such things in the future
>
>> diff --git a/Documentation/hwmon/sht3x b/Documentation/hwmon/sht3x
>> new file mode 100644
>> index 0000000..c04748d
>> --- /dev/null
>> +++ b/Documentation/hwmon/sht3x
>> @@ -0,0 +1,66 @@
>> +Kernel driver sht3x
>> +===================
>> +
>> +Supported chips:
>> +  * Sensirion SHT3x-DIS
>> +    Prefix: 'sht3x'
>> +    Addresses scanned: none
>> +    Datasheet:
>> http://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/Humidity/Sensirion_Humidity_Datasheet_SHT3x_DIS.pdf
>> +
>> +Author:
>> +  David Frey <david.frey@xxxxxxxxxxxxx>
>> +  Pascal Sachs <pascal.sachs@xxxxxxxxxxxxx>
>> +
>> +Description
>> +-----------
>> +
>> +This driver implements support for the Sensirion SHT3x-DIS chip, a
>> humidity
>> +and temperature sensor. Temperature is measured in degrees celsius,
>> relative
>> +humidity is expressed as a percentage. In the sysfs interface, all
>> values are
>> +scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
>> +
>> +The device communicates with the I2C protocol. Sensors can have the I2C
>> +addresses 0x44 or 0x45, depending on the wiring. See
>> +Documentation/i2c/instantiating-devices for methods to instantiate
>> the device.
>> +
>> +There are two options configurable by means of sht3x_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. By default,
>> non-blocking mode
>> +   is used. Make sure clock-stretching works properly on your device
>> if you
>> +   want to use blocking mode.
>> +2. high or low accuracy. High accuracy is used by default and using
>> it is
>> +   strongly recommended.
>> +
>> +The sht3x sensor supports a single shot mode as well as 5 periodic
>> measure
>> +modes, which can be controlled with the frequency sysfs interface.
>> +The allowed frequencies are as follows:
>> +  *     0  single shot mode
>> +  *   500   0.5 Hz periodic measurement
>> +  *  1000   1   Hz periodic measurement
>> +  *  2000   2   Hz periodic measurement
>> +  *  4000   4   Hz periodic measurement
>> +  * 10000  10   Hz periodic measurement
>> +
>> +While in periodic measure mode, read out of humidity and temperature
>> values are
>> +not supported. Nevertheless it is possible to read out the values
>> with maximal
>
> Really ? I seem to be missing this in the datasheet. Section 4.4
> suggests that
> both are supported. Besides, this would be really odd - what would be
> the point
> of periodic mode (or any mode, for that matter) if it doesn't really
> measure
> anything ?

When you start the periodic measurement, the sensor will measure
independently
in the background and keep up to 8 values in an internal buffer. Once
the client
reads out the buffer, the sensor will invalidate the buffer and
therefore return
an error on the I2C bus until at least one new measurement was generated.

This feature is used to automatically set the alert bit according to the
configured
limits and hysteresis values.

Our Product Manager for the SHT3x sensor told me to not handle this
behavior in
the driver by e.g. caching the last value, since it will confuse our
customers when the
hardware behaves differently then the driver.

If you have an other opinion and would like to share it, I'm open for
any input.
>
>> +the same frequency as the periodic measure mode.
>
> This text does not really make sense to me. Should one of those
> "periodic" be
> "single shot" ?

See above for an explanation. I will update the wording in the
documentation in the next patch.
>
>> +
>> +sysfs-Interface
>> +---------------
>> +
>> +temp1_input:        temperature input
>> +humidity1_input:    humidity input
>> +temp1_max:          temperature max value
>> +temp1_max_hyst:     temperature hysteresis value for max limit
>> +humidity1_max:      humidity max value
>> +humidity1_max_hyst: humidity hysteresis value for max limit
>> +temp1_min:          temperature min value
>> +temp1_min_hyst:     temperature hysteresis value for min limit
>> +humidity1_min:      humidity min value
>> +humidity1_min_hyst: humidity hysteresis value for min limit
>> +frequency:          Measurement frequency, 0 for single shot,
>> frequency in
>> +                    mHz for periodic measurement
>
> Please use standard attributes (update_interval).

OK
>
>> +soft_reset:         Soft reset the internal state of the sensor,
>> clear the
>> +                    status register, clear the alert and switch back to
>> +                    single shot mode
>
> Makes me really unhappy. It is not a standard attribute, there is no
> clear
> indication why it is needed, and it affects other attributes (mode)
> without updating the command pointer, thus probably breaking things.
>
> In general, if the chip is that unstable that it needs to be reset
> once in a while, that should be auto-detected if possible and be handled
> automatically. A manual "please reset me" attribute is the worst possible
> solution and should only be used if absolutely necessary.

The soft_reset is actually not really needed. It's just a way for the
user to
clear his configuration. I will remove this interface if it makes you
unhappy.
>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 5c2d13a..76f4c53 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1254,6 +1254,16 @@ config SENSORS_SHT21
>>         This driver can also be built as a module.  If so, the module
>>         will be called sht21.
>>
>> +config SENSORS_SHT3x
>> +    tristate "Sensiron humidity and temperature sensors. SHT3x and
>> compat."
>> +    depends on I2C && CRC8
>
> Per lib/Kconfig, drivers are supposed to select CRC8 if it is needed.
>
> [ Kind of surprising that it doesn't seem to be used anywhere, but maybe
>   I am missing something. ]

I was surprised as well, but it seems that no other driver is using the
CRC8 module
by now. I only saw some older drivers which implement a crc8 lookup
table by themself.
Maybe I'm missing something as well
>
>> +    help
>> +      If you say yes here you get support for the Sensiron SHT30 and
>> SHT31
>> +      humidity and temperature sensors.
>> +
>> +      This driver can also be built as a module.  If so, the module
>> +      will be called sht3x.
>> +
>>   config SENSORS_SHTC1
>>       tristate "Sensiron humidity and temperature sensors. SHTC1 and
>> compat."
>>       depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 58cc3ac..999ff2d 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -137,6 +137,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_SHT3x)    += sht3x.o
>>   obj-$(CONFIG_SENSORS_SHTC1)    += shtc1.o
>>   obj-$(CONFIG_SENSORS_SIS5595)    += sis5595.o
>>   obj-$(CONFIG_SENSORS_SMM665)    += smm665.o
>> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
>> new file mode 100644
>> index 0000000..97f72b8
>> --- /dev/null
>> +++ b/drivers/hwmon/sht3x.c
>> @@ -0,0 +1,675 @@
>> +/* Sensirion SHT3x-DIS humidity and temperature sensor driver.
>> + * The SHT3x comes in many different versions, this driver is for the
>> + * I2C version only.
>> + *
>> + * Copyright (C) 2016 Sensirion AG, Switzerland
>> + * Author: David Frey <david.frey@xxxxxxxxxxxxx>
>> + * Author: Pascal Sachs <pascal.sachs@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 <asm/page.h>
>> +#include <linux/crc8.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/platform_data/sht3x.h>
>> +
>> +/* commands (high precision mode) */
>> +static const unsigned char sht3x_cmd_measure_blocking_hpm[]    = {
>> 0x2c, 0x06 };
>> +static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] = {
>> 0x24, 0x00 };
>> +
>> +/* commands (low power mode) */
>> +static const unsigned char sht3x_cmd_measure_blocking_lpm[]    = {
>> 0x2c, 0x10 };
>> +static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] = {
>> 0x24, 0x16 };
>> +
>> +/* commands for periodic mode */
>> +static const unsigned char sht3x_cmd_measure_periodic_mode[]   = {
>> 0xe0, 0x00 };
>> +static const unsigned char sht3x_cmd_break[]                   = {
>> 0x30, 0x93 };
>> +
>> +/* other commands */
>> +static const unsigned char sht3x_cmd_clear_status_reg[]        = {
>> 0x30, 0x41 };
>> +static const unsigned char sht3x_cmd_soft_reset[]              = {
>> 0x30, 0xa2 };
>> +
>> +/* delays for non-blocking i2c commands, both in us */
>> +#define SHT3X_NONBLOCKING_WAIT_TIME_HPM  15000
>> +#define SHT3X_NONBLOCKING_WAIT_TIME_LPM   4000
>> +
>> +#define SHT3X_WORD_LEN         2
>> +#define SHT3X_CMD_LENGTH       2
>> +#define SHT3X_CRC8_LEN         1
>> +#define SHT3X_RESPONSE_LENGTH  6
>> +#define SHT3X_CRC8_LEN         1
>
> Defined twice.

Oops
>> +#define SHT3X_CRC8_POLYNOMIAL  0x31
>> +#define SHT3X_CRC8_INIT        0xFF
>> +#define SHT3X_ID_SHT           0
>> +#define SHT3X_ID_STS           1
>
> Please use an enum for the IDs.

Ok, makes sense
>> +
>> +DECLARE_CRC8_TABLE(sht3x_crc8_table);
>> +
>> +/* periodic measure commands (high precision mode) */
>> +static const char periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
>> +    /* 0.5 measurements per second */
>> +    {0x20, 0x32},
>> +    /* 1 measurements per second */
>> +    {0x21, 0x30},
>> +    /* 2 measurements per second */
>> +    {0x22, 0x36},
>> +    /* 4 measurements per second */
>> +    {0x23, 0x34},
>> +    /* 10 measurements per second */
>> +    {0x27, 0x37},
>> +};
>> +
>> +/* periodic measure commands (low power mode) */
>> +static const char periodic_measure_commands_lpm[][SHT3X_CMD_LENGTH] = {
>> +    /* 0.5 measurements per second */
>> +    {0x20, 0x2f},
>> +    /* 1 measurements per second */
>> +    {0x21, 0x2d},
>> +    /* 2 measurements per second */
>> +    {0x22, 0x2b},
>> +    /* 4 measurements per second */
>> +    {0x23, 0x29},
>> +    /* 10 measurements per second */
>> +    {0x27, 0x2a},
>> +};
>> +
>> +struct sht3x_alert_commands {
>> +    const char read_command[SHT3X_CMD_LENGTH];
>> +    const char write_command[SHT3X_CMD_LENGTH];
>> +};
>> +
>> +const struct sht3x_alert_commands alert_commands[] = {
>> +    /* temp1_max, humidity1_max */
>> +    { {0xe1, 0x1f}, {0x61, 0x1d} },
>> +    /* temp_1_max_hyst, humidity1_max_hyst */
>> +    { {0xe1, 0x14}, {0x61, 0x16} },
>> +    /* temp1_min, humidity1_min */
>> +    { {0xe1, 0x02}, {0x61, 0x00} },
>> +    /* temp_1_min_hyst, humidity1_min_hyst */
>> +    { {0xe1, 0x09}, {0x61, 0x0B} },
>> +};
>> +
>> +
>> +static const u16 mode_to_frequency[] = {
>> +        0,
>> +      500,
>> +     1000,
>> +     2000,
>> +     4000,
>> +    10000,
>> +};
>> +
>> +struct sht3x_data {
>> +    struct i2c_client *client;
>> +    struct mutex update_lock;
>> +
>> +    u8 mode;
>> +    const unsigned char *command;
>> +    u32 wait_time;            /* in us*/
>> +
>> +    struct sht3x_platform_data setup;
>> +
>> +    int temperature;    /* 1000 * temperature in dgr C */
>> +    u32 humidity;        /* 1000 * relative humidity in %RH */
>> +};
>> +
>> +
>> +static int find_index(const u16 *list, size_t size, u16 value)
>> +{
>
> Two of those parameters are really constants, since only
> mode_to_frequency[]
> and its size are passed as argument. Please reference the array directly.
>
>> +    size_t index = 0;
>> +
>> +    while (index < size && list[index] != value)
>> +        index++;
>> +
>> +    return index == size ? -1 : index;
>
> If you return an error, please use a proper error code,
> and pass it on in the calling code.
>
> I would suggest, however, to implement a best-match algorithm instead
> of expecting the user to find valid values.

Ok, will fix it
>
>> +}
>> +
>> +static int sht3x_read_from_command(struct i2c_client *client,
>> +                   struct sht3x_data *data,
>> +                   const char *command,
>> +                   char *buf, int length, u32 wait_time)
>> +{
>> +    int ret;
>> +
>> +    mutex_lock(&data->update_lock);
>> +
>> +    ret = i2c_master_send(client, command, SHT3X_CMD_LENGTH);
>> +
>> +    if (ret != SHT3X_CMD_LENGTH) {
>> +        ret = ret < 0 ? ret : -EIO;
>> +        goto out;
>> +    }
>> +
>> +    if (wait_time)
>> +        usleep_range(wait_time, wait_time + 1000);
>> +
>> +    ret = i2c_master_recv(client, buf, length);
>> +    if (ret != length) {
>> +        ret = ret < 0 ? ret : -EIO;
>> +        goto out;
>> +    }
>> +
>> +    ret = 0;
>> +out:
>> +    mutex_unlock(&data->update_lock);
>> +    return ret;
>> +
>> +}
>> +
>> +static int sht3x_extract_temperature(u16 raw)
>> +{
>> +    /*
>> +     * From datasheet:
>> +     * T = -45 + 175 * ST / 2^16
>> +     * Adapted for integer fixed point (3 digit) arithmetic.
>> +     */
>> +    return ((21875 * (int)raw) >> 13) - 45000;
>> +}
>> +
>> +static u32 sht3x_extract_humidity(u16 raw)
>> +{
>> +    /*
>> +     * From datasheet:
>> +     * RH = 100 * SRH / 2^16
>> +     * Adapted for integer fixed point (3 digit) arithmetic.
>> +     */
>> +    return (12500 * (int)raw) >> 13;
>> +}
>> +
>> +/* sysfs attributes */
>> +static struct sht3x_data *sht3x_update_client(struct device *dev)
>> +{
>> +    struct sht3x_data *data = dev_get_drvdata(dev);
>> +    struct i2c_client *client = data->client;
>> +    unsigned char buf[SHT3X_RESPONSE_LENGTH];
>> +    u16 val;
>> +    int ret;
>> +
>> +    ret = sht3x_read_from_command(client, data,  data->command,
>> +                      buf, sizeof(buf), data->wait_time);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> +
>> +    val = be16_to_cpup((__be16 *) buf);
>> +    data->temperature = sht3x_extract_temperature(val);
>> +    val = be16_to_cpup((__be16 *) (buf + 3));
>> +    data->humidity = sht3x_extract_humidity(val);
>> +
> Having this code not mutex-protected means that the variables can have
> more or less random values if there are multiple readers.
>

Good point
>> +    return data;
>> +}
>> +
>> +static ssize_t temp1_input_show(struct device *dev,
>> +                struct device_attribute *attr, char *buf)
>> +{
>> +    struct sht3x_data *data = sht3x_update_client(dev);
>> +
>> +    if (IS_ERR(data))
>> +        return PTR_ERR(data);
>> +
>> +    return sprintf(buf, "%d\n", data->temperature);
>> +}
>> +
>> +static ssize_t humidity1_input_show(struct device *dev,
>> +                    struct device_attribute *attr, char *buf)
>> +{
>> +    struct sht3x_data *data = sht3x_update_client(dev);
>> +
>> +    if (IS_ERR(data))
>> +        return PTR_ERR(data);
>> +
>> +    return sprintf(buf, "%d\n", data->humidity);
>> +}
>> +
>> +static int alert_read_raw(struct device *dev,
>> +              struct device_attribute *attr,
>> +              char *buffer, int length)
>> +{
>> +    int ret;
>> +    u8 index;
>> +    const struct sht3x_alert_commands *commands;
>> +    struct sht3x_data *data = dev_get_drvdata(dev);
>> +    struct i2c_client *client = data->client;
>> +
>> +    index = to_sensor_dev_attr(attr)->index;
>> +    commands = &alert_commands[index];
>> +
>> +    ret = sht3x_read_from_command(client, data, commands->read_command,
>> +                      buffer, length, 0);
>
> Normally, limit temperatures don't change. Reading it from the chip
> repeatedly doesn't really add much if any value, and in this case will
> cause substantial latencies. Why not just cache the values ? This
> would also simplify and speed up changing values a lot.

Yes, makes sense
>
>> +
>> +    return ret;
>> +}
>> +
>> +static ssize_t temp1_alert_read(struct device *dev,
>> +                struct device_attribute *attr,
>> +                int *temperature)
>> +{
>> +    int ret;
>> +    u16 raw;
>> +    char buffer[SHT3X_RESPONSE_LENGTH];
>> +
>> +    ret = alert_read_raw(dev, attr, buffer, SHT3X_RESPONSE_LENGTH);
>> +    if (ret)
>> +        return ret;
>> +
>> +    raw = be16_to_cpup((__be16 *) buffer);
>> +
>> +    /*
>> +     * lower 9 bits are temperature MSB
>> +     */
>> +    *temperature = sht3x_extract_temperature((raw & 0x01ff) << 7);
>> +
>> +    return 0;
>> +}
>> +
>> +
>> +static ssize_t temp1_alert_show(struct device *dev,
>> +                struct device_attribute *attr,
>> +                char *buf)
>> +{
>> +    int ret;
>> +    int temperature;
>> +
>> +    ret = temp1_alert_read(dev, attr, &temperature);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return scnprintf(buf, PAGE_SIZE, "%d\n", temperature);
>> +}
>> +
>> +
>> +static ssize_t humidity1_alert_read(struct device *dev,
>> +                    struct device_attribute *attr,
>> +                    u32 *humidity)
>> +{
>> +    int ret;
>> +    u16 raw;
>> +    char buffer[SHT3X_RESPONSE_LENGTH];
>> +
>> +    ret = alert_read_raw(dev, attr, buffer, SHT3X_RESPONSE_LENGTH);
>> +    if (ret)
>> +        return ret;
>> +
>> +    raw = be16_to_cpup((__be16 *) buffer);
>> +
>> +    /*
>> +     * top 7 bits are the humidity MSB,
>> +     */
>> +    *humidity = sht3x_extract_humidity(raw & 0xfe00);
>> +
>> +    return 0;
>> +}
>> +
>> +static ssize_t humidity1_alert_show(struct device *dev,
>> +                    struct device_attribute *attr,
>> +                    char *buf)
>> +{
>> +    int ret;
>> +    u32 humidity;
>> +
>> +    ret = humidity1_alert_read(dev, attr, &humidity);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return scnprintf(buf, PAGE_SIZE, "%d\n", humidity);
>> +}
>> +
>> +static size_t alert_store(struct device *dev,
>> +              struct device_attribute *attr,
>> +              size_t count,
>> +              int temperature,
>> +              u32 humidity)
>> +{
>> +    char buffer[SHT3X_CMD_LENGTH + SHT3X_WORD_LEN + SHT3X_CRC8_LEN];
>> +    char *position = buffer;
>> +    int ret;
>> +    u16 raw;
>> +    u8 index;
>> +    struct sht3x_data *data = dev_get_drvdata(dev);
>> +    struct i2c_client *client = data->client;
>> +    const struct sht3x_alert_commands *commands;
>> +
>> +    index = to_sensor_dev_attr(attr)->index;
>> +    commands = &alert_commands[index];
>> +
>> +    memcpy(position, commands->write_command, SHT3X_CMD_LENGTH);
>> +    position += SHT3X_CMD_LENGTH;
>> +    /*
>> +     * ST = (T + 45) / 175 * 2^16
>> +     * SRH = RH / 100 * 2^16
>> +     * adapted for fixed point arithmetic and packed the same as
>> +     * in alert_show()
>> +     */
>> +    raw = ((u32)(temperature + 45000) * 24543) >> (16 + 7);
>> +    raw |= (((u32)humidity * 42950) >> 16) & 0xfe00;
>
> humidity is already u32.
OK
>
>> +
>> +    *(__be16 *)(position) = cpu_to_be16(raw);
>> +    position += SHT3X_WORD_LEN;
>> +    *position = crc8(sht3x_crc8_table,
>> +             (position - SHT3X_WORD_LEN),
>
> Unnecessary ( )
>
OK
>> +             SHT3X_WORD_LEN,
>> +             SHT3X_CRC8_INIT);
>> +
>> +
>> +    ret = i2c_master_send(client, buffer, sizeof(buffer));
>> +    if (ret != sizeof(buffer))
>> +        return ret < 0 ? ret : -EIO;
>> +
>> +    return count;
>> +
>> +}
>> +
>> +static ssize_t temp1_alert_store(struct device *dev,
>> +                 struct device_attribute *attr,
>> +                 const char *buf,
>> +                 size_t count)
>> +{
>> +    int temperature;
>> +    u32 humidity;
>> +    int ret;
>> +
>> +    ret = kstrtoint(buf, 0, &temperature);
>> +
>> +    if (ret)
>> +        return -EINVAL;
>> +
>> +    temperature = clamp_val(temperature, -45000, 130000);
>> +
>> +    /*
>> +     * use old humidity alert value since we can only update
>> +     * temperature and humidity alerts together
>> +     */
>> +    ret = humidity1_alert_read(dev, attr, &humidity);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = alert_store(dev, attr, count, temperature, humidity);
>> +
>> +    return ret;
>> +}
>> +
>> +static ssize_t humidity1_alert_store(struct device *dev,
>> +                     struct device_attribute *attr,
>> +                     const char *buf,
>> +                     size_t count)
>> +{
>> +    int temperature;
>> +    u32 humidity;
>> +    int ret;
>> +
>> +    ret = kstrtou32(buf, 0, &humidity);
>> +
>> +    if (ret)
>> +        return -EINVAL;
>> +
>> +    humidity = clamp_val(humidity, 0, 100000);
>> +
>> +    /*
>> +     * use old temperature alert value since we can only update
>> +     * temperature and humidity alerts together
>> +     */
>> +    ret = temp1_alert_read(dev, attr, &temperature);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = alert_store(dev, attr, count, temperature, humidity);
>> +
>> +    return ret;
>> +}
>> +
>> +static void sht3x_select_command(struct sht3x_data *data)
>> +{
>> +    /*
>> +     * 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.
>> +     */
>> +    if (data->mode > 0) {
>> +        data->command = sht3x_cmd_measure_periodic_mode;
>> +        data->wait_time = 0;
>> +    } else if (data->setup.blocking_io) {
>> +        data->command = data->setup.high_precision ?
>> +                sht3x_cmd_measure_blocking_hpm :
>> +                sht3x_cmd_measure_blocking_lpm;
>> +        data->wait_time = 0;
>> +    } else {
>> +        if (data->setup.high_precision) {
>> +            data->command = sht3x_cmd_measure_nonblocking_hpm;
>> +            data->wait_time = SHT3X_NONBLOCKING_WAIT_TIME_HPM;
>> +        } else {
>> +            data->command = sht3x_cmd_measure_nonblocking_lpm;
>> +            data->wait_time = SHT3X_NONBLOCKING_WAIT_TIME_LPM;
>> +        }
>> +    }
>> +}
>> +
>> +static ssize_t frequency_show(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  char *buf) {
>> +    struct sht3x_data *data = dev_get_drvdata(dev);
>> +
>> +    return scnprintf(buf, PAGE_SIZE, "%d\n",
>> mode_to_frequency[data->mode]);
>> +}
>> +
>> +static ssize_t frequency_store(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   const char *buf,
>> +                   size_t count) {
>> +    u16 frequency;
>> +    int mode;
>> +    int ret;
>> +    const char *command;
>> +    struct sht3x_data *data = dev_get_drvdata(dev);
>> +    struct i2c_client *client = data->client;
>> +
>> +    ret = kstrtou16(buf, 0, &frequency);
>> +
>> +    if (ret)
>> +        return -EINVAL;
>> +
>> +    mode = find_index(mode_to_frequency, sizeof(mode_to_frequency),
>> +              frequency);
>> +
>> +    /* invalid frequency */
>> +    if (mode < 0)
>> +        return -EINVAL;
>> +
>> +    /* mode did not change */
>> +    if (mode == data->mode)
>> +        return count;
>> +
>> +    /* abort periodic measure */
>> +    ret = i2c_master_send(client, sht3x_cmd_break, SHT3X_CMD_LENGTH);
>> +    if (ret != SHT3X_CMD_LENGTH)
>> +        return ret < 0 ? ret : -EIO;
>> +    data->mode = 0;
>> +
>> +    if (mode > 0) {
>> +        if (data->setup.high_precision)
>> +            command = periodic_measure_commands_hpm[mode - 1];
>> +        else
>> +            command = periodic_measure_commands_lpm[mode - 1];
>> +
>> +        /* select mode */
>> +        ret = i2c_master_send(client, command, SHT3X_CMD_LENGTH);
>> +        if (ret != SHT3X_CMD_LENGTH)
>> +            return ret < 0 ? ret : -EIO;
>> +    }
>> +
>> +    /* select mode and command */
>> +    data->mode = mode;
>> +    sht3x_select_command(data);
>> +
> All that unprotected ? What happens if multiple processes write
> different values in parallel ?
>
I will rethink the mutex protection of the interface
>> +    return count;
>> +}
>> +
>> +static ssize_t soft_reset(struct device *dev,
>> +              struct device_attribute *attr,
>> +              const char *buf,
>> +              size_t count) {
>> +    int ret;
>> +    struct sht3x_data *data = dev_get_drvdata(dev);
>> +    struct i2c_client *client = data->client;
>> +
>> +    /* break if in periodic measure mode */
>> +    if (data->mode > 0) {
>> +        ret = i2c_master_send(client, sht3x_cmd_break,
>> +                      SHT3X_CMD_LENGTH);
>> +        if (ret != SHT3X_CMD_LENGTH)
>> +            return ret < 0 ? ret : -EIO;
>> +        data->mode = 0;
>> +    }
>> +
>> +    /* clear status register */
>> +    ret = i2c_master_send(client, sht3x_cmd_clear_status_reg,
>> +                  SHT3X_CMD_LENGTH);
>> +    if (ret != SHT3X_CMD_LENGTH)
>> +        return ret < 0 ? ret : -EIO;
>> +
>> +    /* soft reset */
>> +    ret = i2c_master_send(client, sht3x_cmd_soft_reset,
>> SHT3X_CMD_LENGTH);
>> +    if (ret != SHT3X_CMD_LENGTH)
>> +        return ret < 0 ? ret : -EIO;
>> +
>> +    return count;
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, temp1_input_show,
>> NULL, 0);
>> +static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO,
>> humidity1_input_show,
>> +              NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR,
>> temp1_alert_show,
>> +              temp1_alert_store, 0);
>> +static SENSOR_DEVICE_ATTR(humidity1_max, S_IRUGO | S_IWUSR,
>> +              humidity1_alert_show, humidity1_alert_store, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IRUGO | S_IWUSR,
>> temp1_alert_show,
>> +              temp1_alert_store, 1);
>> +static SENSOR_DEVICE_ATTR(humidity1_max_hyst, S_IRUGO | S_IWUSR,
>> +              humidity1_alert_show, humidity1_alert_store, 1);
>> +static SENSOR_DEVICE_ATTR(temp1_min, S_IRUGO | S_IWUSR,
>> temp1_alert_show,
>> +              temp1_alert_store, 2);
>> +static SENSOR_DEVICE_ATTR(humidity1_min, S_IRUGO | S_IWUSR,
>> +              humidity1_alert_show, humidity1_alert_store, 2);
>> +static SENSOR_DEVICE_ATTR(temp1_min_hyst, S_IRUGO | S_IWUSR,
>> +              temp1_alert_show, temp1_alert_store, 3);
>> +static SENSOR_DEVICE_ATTR(humidity1_min_hyst, S_IRUGO | S_IWUSR,
>> +              humidity1_alert_show, humidity1_alert_store, 3);
>> +static SENSOR_DEVICE_ATTR(frequency, S_IRUGO | S_IWUSR, frequency_show,
>> +              frequency_store, 0);
>> +static SENSOR_DEVICE_ATTR(soft_reset, S_IWUSR, NULL, soft_reset, 0);
>> +
>> +static struct attribute *sht3x_attrs[] = {
>> +    &sensor_dev_attr_temp1_input.dev_attr.attr,
>> +    &sensor_dev_attr_humidity1_input.dev_attr.attr,
>> +    &sensor_dev_attr_temp1_max.dev_attr.attr,
>> +    &sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
>> +    &sensor_dev_attr_humidity1_max.dev_attr.attr,
>> +    &sensor_dev_attr_humidity1_max_hyst.dev_attr.attr,
>> +    &sensor_dev_attr_temp1_min.dev_attr.attr,
>> +    &sensor_dev_attr_temp1_min_hyst.dev_attr.attr,
>> +    &sensor_dev_attr_humidity1_min.dev_attr.attr,
>> +    &sensor_dev_attr_humidity1_min_hyst.dev_attr.attr,
>> +    &sensor_dev_attr_frequency.dev_attr.attr,
>> +    &sensor_dev_attr_soft_reset.dev_attr.attr,
>
> Limit attributes without alarm attributes is kind of unusual.
> Looking into the datasheet, the chip does support a status register
> with alert bits. Any reason for not providing alert attributes ?
>
The alarm feature is only present in periodic mode. In this mode the sensor
measures by itself without the influence of the user (or a program). If
there
is an alert, the sensor set the alert pin to high. As long as there is
no e.g.
watchdog mechanism in place which notifies the driver, we can not know
when an alert happens without some sort of polling mechanism.
During periodic measurement the sensor measures temperature and
humidity without any interaction of the user.

We can of course implement an alarm interface which asks the status
register whenever it is called by the user.

I can't find a humidity alarm flag in the documentation? Is there a
reason for that or did nobody used this so far?
>> +    NULL
>> +};
>> +
>> +static struct attribute *sts3x_attrs[] = {
>> +    &sensor_dev_attr_temp1_input.dev_attr.attr,
>> +    NULL
>> +};
>> +
>> +ATTRIBUTE_GROUPS(sht3x);
>> +ATTRIBUTE_GROUPS(sts3x);
>> +
>> +static int sht3x_probe(struct i2c_client *client,
>> +               const struct i2c_device_id *id)
>> +{
>> +    int ret;
>> +    struct sht3x_data *data;
>> +    struct device *hwmon_dev;
>> +    struct i2c_adapter *adap = client->adapter;
>> +    struct device *dev = &client->dev;
>> +    const struct attribute_group **attribute_groups;
>> +
>> +    /*
>> +     * we require full i2c support since the sht3x uses multi-byte
>> read and
>> +     * writes as well as multi-byte commands which are not supported by
>> +     * the smbus protocol
>> +     */
>> +    if (!i2c_check_functionality(adap, I2C_FUNC_I2C))
>> +        return -ENODEV;
>> +
>> +    ret = i2c_master_send(client, sht3x_cmd_clear_status_reg,
>> +                  SHT3X_CMD_LENGTH);
>> +    if (ret != SHT3X_CMD_LENGTH)
>> +        return ret < 0 ? ret : -ENODEV;
>> +
>> +    data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    data->setup.blocking_io = false;
>> +    data->setup.high_precision = true;
>> +    data->mode = 0;
>> +    data->client = client;
>> +    crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);
>> +
>> +    if (client->dev.platform_data)
>> +        data->setup = *(struct sht3x_platform_data
>> *)dev->platform_data;
>> +    sht3x_select_command(data);
>> +    mutex_init(&data->update_lock);
>> +
>> +    if (id->driver_data == SHT3X_ID_STS)
>> +        attribute_groups = sts3x_groups;
>> +    else
>> +        attribute_groups = sht3x_groups;
>> +
>> +    hwmon_dev = devm_hwmon_device_register_with_groups(dev,
>> +            client->name,
>> +            data, attribute_groups);
>> +
>> +    if (IS_ERR(hwmon_dev))
>> +        dev_dbg(dev, "unable to register hwmon device\n");
>> +
>> +    return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +/* device ID table */
>> +static const struct i2c_device_id sht3x_id[] = {
>> +    {"sht3x", SHT3X_ID_SHT},
>> +    {"sts3x", SHT3X_ID_STS},
>> +    {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, sht3x_id);
>> +
>> +static struct i2c_driver sht3x_i2c_driver = {
>> +    .driver.name = "sht3x",
>> +    .probe       = sht3x_probe,
>> +    .id_table    = sht3x_id,
>> +};
>> +
>> +module_i2c_driver(sht3x_i2c_driver);
>> +
>> +MODULE_AUTHOR("David Frey <david.frey@xxxxxxxxxxxxx>");
>> +MODULE_AUTHOR("Pascal Sachs <pascal.sachs@xxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Sensirion SHT3x humidity and temperature sensor
>> driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/hwmon/twl4030-madc-hwmon.c
>> b/drivers/hwmon/twl4030-madc-hwmon.c
>> index b5caf7f..8201173 100644
>> --- a/drivers/hwmon/twl4030-madc-hwmon.c
>> +++ b/drivers/hwmon/twl4030-madc-hwmon.c
>> @@ -25,6 +25,7 @@
>>    */
>>   #include <linux/init.h>
>>   #include <linux/module.h>
>> +//FIXME this is used for clamp_val
>
> Drop.
OK, my bad :-(
>
>>   #include <linux/kernel.h>
>>   #include <linux/i2c/twl.h>
>>   #include <linux/device.h>
>> diff --git a/include/linux/platform_data/sht3x.h
>> b/include/linux/platform_data/sht3x.h
>> new file mode 100644
>> index 0000000..46af9a0
>> --- /dev/null
>> +++ b/include/linux/platform_data/sht3x.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * Copyright (C) 2015 Sensirion AG, Switzerland
>> + * Author: David Frey <david.frey@xxxxxxxxxxxxx>
>> + * Author: Pascal Sachs <pascal.sachs@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.
>> + *
>> + */
>> +
>> +#ifndef __SHT3X_H_
>> +#define __SHT3X_H_
>> +
>> +struct sht3x_platform_data {
>> +    bool blocking_io;
>> +    bool high_precision;
>> +};
>> +#endif /* __SHT3X_H_ */
>>
>