Hi Guenter I want to thank you again for the fast and detailed code review. I really appreciate it. Am 30.05.2016 um 17:45 schrieb Guenter Roeck:
Hi Pascal, On 05/30/2016 07:46 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.Getting there. Mostly nitpicks this time.Signed-off-by: Pascal Sachs <pascal.sachs@xxxxxxxxxxxxx> --- Patch was generated against kernel v4.7-rc1A changelog would be useful.
Sure, I will add one with the next submission
Yes, this is how it should behave. In single shot mode the sensor measures values on demand, which means every time the sysfs interface is called, a measurement is triggered. In periodic mode however the measurement process is handled internally by the sensor and read out only makes sense if a new reading is available.Documentation/hwmon/sht3x | 72 ++++ drivers/hwmon/Kconfig | 11 + drivers/hwmon/Makefile | 1 +drivers/hwmon/sht3x.c | 724 ++++++++++++++++++++++++++++++++++++include/linux/platform_data/sht3x.h | 25 ++ 5 files changed, 833 insertions(+) create mode 100644 Documentation/hwmon/sht3x create mode 100644 drivers/hwmon/sht3x.c create mode 100644 include/linux/platform_data/sht3x.h diff --git a/Documentation/hwmon/sht3x b/Documentation/hwmon/sht3x new file mode 100644 index 0000000..f5aa633 --- /dev/null +++ b/Documentation/hwmon/sht3x @@ -0,0 +1,72 @@ +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 update_interval sysfs interface.+The allowed update_interval in milliseconds are as follows: + * 0 single shot mode + * 2000 0.5 Hz periodic measurement + * 1000 1 Hz periodic measurement + * 500 2 Hz periodic measurement + * 250 4 Hz periodic measurement + * 100 10 Hz periodic measurement ++In the periodic measure mode, the sensor automatically triggers a measurement +with the configured update interval on the chip. When a temperature or humidity +reading exceeds the configured limits, the alert attribute is set to 1 and+the alert pin on the sensor is set to high.+When the temperature and humidity readings move back between the hysteresis +values, the alert bit is set to 0 and the alert pin on the sensor is set to+low. + +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+temp1_alarm: alarm flag is set to 1 if the temperature is outside the + configured limits. Alarm only works in periodic measure mode +humidity1_alarm: alarm flag is set to 1 if the humidity is outside the + configured limits. Alarm only works in periodic measure mode +update_interval: update interval, 0 for single shot, interval in msec + for periodic measurement. If the interval is not supported+ by the sensor, the next faster interval is chosen diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index ff94007..e0be99a 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1265,6 +1265,17 @@ 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 + select CRC8 + 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_SHTC1tristate "Sensiron humidity and temperature sensors. SHTC1 and compat."depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 2ef5b7c..406b18b 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -138,6 +138,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..2251117 --- /dev/null +++ b/drivers/hwmon/sht3x.c @@ -0,0 +1,724 @@ +/* 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/jiffies.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_read_status_reg[] = { 0xf3, 0x2d }; +static const unsigned char sht3x_cmd_clear_status_reg[] = { 0x30, 0x41 };+ +/* 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_POLYNOMIAL 0x31 +#define SHT3X_CRC8_INIT 0xFF + +enum sht3x_chips { + sht3x, + sts3x, +}; + +enum sht3x_limits { + limit_max = 0, + limit_max_hyst, + limit_min, + limit_min_hyst, +}; + +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_limit_commands { + const char read_command[SHT3X_CMD_LENGTH]; + const char write_command[SHT3X_CMD_LENGTH]; +}; + +static const struct sht3x_limit_commands limit_commands[] = { + /* temp1_max, humidity1_max */ + [limit_max] = { {0xe1, 0x1f}, {0x61, 0x1d} }, + /* temp_1_max_hyst, humidity1_max_hyst */ + [limit_max_hyst] = { {0xe1, 0x14}, {0x61, 0x16} }, + /* temp1_min, humidity1_min */ + [limit_min] = { {0xe1, 0x02}, {0x61, 0x00} }, + /* temp_1_min_hyst, humidity1_min_hyst */ + [limit_min_hyst] = { {0xe1, 0x09}, {0x61, 0x0B} }, +}; + +#define SHT3X_NUM_LIMIT_CMD ARRAY_SIZE(limit_commands) + +static const u16 mode_to_update_interval[] = { + 0,I think this is problematic. See below.+ 2000, + 1000, + 500, + 250, + 100, +}; + +struct sht3x_data { + struct i2c_client *client; + struct mutex i2c_lock; /* lock for sending i2c commands */ + struct mutex data_lock; /* lock for updating driver data */ + + u8 mode; + const unsigned char *command; + u32 wait_time; /* in us*/ + unsigned long last_update; /* last update in periodic mode*/ + + struct sht3x_platform_data setup; + + /* + * cached values for temperature and humidity and limits + * the limits arrays have the following order: + * max, max_hyst, min, min_hyst + */ + int temperature; + int temperature_limits[SHT3X_NUM_LIMIT_CMD]; + u32 humidity; + u32 humidity_limits[SHT3X_NUM_LIMIT_CMD]; +}; + +static u8 get_mode_from_update_interval(u16 value) +{ + size_t index; + u8 number_of_modes = ARRAY_SIZE(mode_to_update_interval); + + if (value == 0) + return 0; + + /* find next faster update interval */ + for (index = 1; index < number_of_modes; index++) { + if (mode_to_update_interval[index] <= value) + return index; + } + + return number_of_modes - 1; +} + +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->i2c_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->i2c_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;Why typecast to "int" ? raw is u16, return value is u32.+} + +static struct sht3x_data *sht3x_update_client(struct device *dev) +{ + struct sht3x_data *data = dev_get_drvdata(dev); + struct i2c_client *client = data->client; + u16 interval_ms = mode_to_update_interval[data->mode];If data->mode == 0, interval_ms == 0,+ unsigned long interval_jiffies = msecs_to_jiffies(interval_ms);... interval_jiffies == 1,+ unsigned char buf[SHT3X_RESPONSE_LENGTH]; + u16 val; + int ret = 0; + + mutex_lock(&data->data_lock); + /* only update once per interval in periodic mode */ + if (time_after(jiffies, data->last_update + interval_jiffies)) {... and the command is executed every other timer tick. Is that intentional ?
+ ret = sht3x_read_from_command(client, data, data->command, buf, + sizeof(buf), data->wait_time); + if (ret) + goto out; + + 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); + data->last_update = jiffies; + } + +out: + mutex_unlock(&data->data_lock); + if (ret) + return ERR_PTR(ret); + + return data; +} + +/* sysfs attributes */ +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);humidity is u32 -> %u.+} + +/* + * limits_update must only be called from probe or with data_lock held + */ +static int limits_update(struct sht3x_data *data) +{ + int ret; + u8 index; + int temperature; + u32 humidity; + u16 raw; + char buffer[SHT3X_RESPONSE_LENGTH]; + const struct sht3x_limit_commands *commands; + struct i2c_client *client = data->client; + + for (index = 0; index < SHT3X_NUM_LIMIT_CMD; index++) { + commands = &limit_commands[index]; + ret = sht3x_read_from_command(client, data, + commands->read_command, buffer, + SHT3X_RESPONSE_LENGTH, 0); + + if (ret) + return ret; + + raw = be16_to_cpup((__be16 *)buffer); + temperature = sht3x_extract_temperature((raw & 0x01ff) << 7); + humidity = sht3x_extract_humidity(raw & 0xfe00); + data->temperature_limits[index] = temperature; + data->humidity_limits[index] = humidity; + } + + return ret; +} + +static ssize_t temp1_limit_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int temperature_limit; + u8 index;How about: struct sht3x_data *data = dev_get_drvdata(dev); int temperature_limit = data->temperature_limits[index]; u8 index = to_sensor_dev_attr(attr)->index;
Ok, I can change that
OK, wasn't sure if it is appropriate to handle error and valid return in the same variable+ struct sht3x_data *data = dev_get_drvdata(dev); + + index = to_sensor_dev_attr(attr)->index; + temperature_limit = data->temperature_limits[index]; + + return scnprintf(buf, PAGE_SIZE, "%d\n", temperature_limit); +} + +static ssize_t humidity1_limit_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int humidity_limit; + u8 index; + struct sht3x_data *data = dev_get_drvdata(dev); + + index = to_sensor_dev_attr(attr)->index; + humidity_limit = data->humidity_limits[index]; +Initialize with declaration ?+ return scnprintf(buf, PAGE_SIZE, "%d\n", humidity_limit);data->humidity_limit[] is u32, so humidity_limit should be u32 as well.+} + +static size_t limit_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_limit_commands *commands; + + index = to_sensor_dev_attr(attr)->index;Initialize with declaration ?+ commands = &limit_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 limit_show() + */ + raw = ((u32)(temperature + 45000) * 24543) >> (16 + 7); + raw |= ((humidity * 42950) >> 16) & 0xfe00; + + *((__be16 *)position) = cpu_to_be16(raw); + position += SHT3X_WORD_LEN; + *position = crc8(sht3x_crc8_table, + position - SHT3X_WORD_LEN, + SHT3X_WORD_LEN, + SHT3X_CRC8_INIT); + + mutex_lock(&data->data_lock); + mutex_lock(&data->i2c_lock); + ret = i2c_master_send(client, buffer, sizeof(buffer)); + mutex_unlock(&data->i2c_lock); + + if (ret != sizeof(buffer)) + goto out; + + data->temperature_limits[index] = temperature; + data->humidity_limits[index] = humidity; + +out: + mutex_unlock(&data->data_lock); + if (ret != sizeof(buffer)) + return ret < 0 ? ret : -EIO; + + return count;I would prefer something like if (ret != sizeof(buffer)) { if (ret >= 0) ret = -EIO; goto out; } ret = count; data->temperature_limits[index] = temperature; data->humidity_limits[index] = humidity; out: mutex_unlock(&data->data_lock); return ret; which I think would be cleaner. See below though for locking issues.
+} + +static ssize_t temp1_limit_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t count) +{ + int temperature; + u32 humidity; + u8 index; + int ret; + struct sht3x_data *data = dev_get_drvdata(dev); + + ret = kstrtoint(buf, 0, &temperature); +No empty line here please.+ if (ret) + return -EINVAL;return ret;+ + temperature = clamp_val(temperature, -45000, 130000); + + index = to_sensor_dev_attr(attr)->index;Initialize with declaration ?+ humidity = data->humidity_limits[index]; +You need to read humidity with the lock active, to avoid inconsistencies ifhumidity is updated in parallel. Maybe lock()ret = limit_store(dev, attr, count, temperature, data->humidity_limits[index]);unlock();limit_store() only uses attr to get the index (again). Maybe just pass index ?
Yes, seems easier to understand
+ ret = limit_store(dev, attr, count, temperature, humidity); + + return ret; +} + +static ssize_t humidity1_limit_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t count) +{ + int temperature; + u32 humidity; + u8 index; + int ret; + struct sht3x_data *data = dev_get_drvdata(dev); + + ret = kstrtou32(buf, 0, &humidity); +No empty line here please.+ if (ret) + return -EINVAL;return ret;+ + humidity = clamp_val(humidity, 0, 100000); + index = to_sensor_dev_attr(attr)->index;Initialize index with declaration ?+ temperature = data->temperature_limits[index];Same problem as above with locking.+ + ret = limit_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 int status_register_read(struct device *dev, + struct device_attribute *attr, + char *buffer, int length) +{ + int ret; +No empty line here please.+ struct sht3x_data *data = dev_get_drvdata(dev); + struct i2c_client *client = data->client; ++ ret = sht3x_read_from_command(client, data, sht3x_cmd_read_status_reg,+ buffer, length, 0); + + return ret; +} + +static ssize_t temp1_alarm_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int ret; + int length = SHT3X_WORD_LEN + SHT3X_CRC8_LEN; + char buffer[length]; + + ret = status_register_read(dev, attr, buffer, length); +No empty line here please.+ if (ret) + return ret; + + return scnprintf(buf, PAGE_SIZE, "%d\n", !!(buffer[0] & 0x04)); +} + +static ssize_t humidity1_alarm_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int ret; + int length = SHT3X_WORD_LEN + SHT3X_CRC8_LEN; + char buffer[length]; + + ret = status_register_read(dev, attr, buffer, length); +No empty line here please.+ if (ret) + return ret; + + return scnprintf(buf, PAGE_SIZE, "%d\n", !!(buffer[0] & 0x08)); +} + +static ssize_t update_interval_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_update_interval[data->mode]);%u+} + +static ssize_t update_interval_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t count) +{ + u16 update_interval; + u8 mode; + int ret; + const char *command; + struct sht3x_data *data = dev_get_drvdata(dev); + struct i2c_client *client = data->client; + + ret = kstrtou16(buf, 0, &update_interval); +No empty line here please.+ if (ret) + return -EINVAL;return ret;That doesn't really mean anything here, since the check is outside the lock,+ + mode = get_mode_from_update_interval(update_interval); + + /* mode did not change */ + if (mode == data->mode) + return count; +and mode may change right after you checked it. Either check from within the lock or not at all.+ mutex_lock(&data->i2c_lock); + mutex_lock(&data->data_lock); + /* abort periodic measure */ + ret = i2c_master_send(client, sht3x_cmd_break, SHT3X_CMD_LENGTH); + if (ret != SHT3X_CMD_LENGTH) + goto out; + data->mode = 0; +Can you explain why you set data->mode = 0 here ?
To do any changes to the configuration while in periodic mode we have to send a break command to the sensor, which then falls back to single shot mode (mode = 0). Therefore for consistency reasons we set the mode to the value it actually is at this point.If setting the new mode fails, we are still in single shot mode. Like this we
ensure that the data->mode is always correct.
+ 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) + goto out; + } + + /* select mode and command */ + data->mode = mode; + sht3x_select_command(data); + +out: + mutex_unlock(&data->data_lock); + mutex_unlock(&data->i2c_lock);Reverse order would be better for lock/unlock since elsewhere you lock data_lockfirst, followed by i2c_lock.
Ok
+ 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_limit_show, temp1_limit_store, + limit_max); +static SENSOR_DEVICE_ATTR(humidity1_max, S_IRUGO | S_IWUSR, + humidity1_limit_show, humidity1_limit_store, + limit_max); +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IRUGO | S_IWUSR, + temp1_limit_show, temp1_limit_store, + limit_max_hyst); +static SENSOR_DEVICE_ATTR(humidity1_max_hyst, S_IRUGO | S_IWUSR, + humidity1_limit_show, humidity1_limit_store, + limit_max_hyst); +static SENSOR_DEVICE_ATTR(temp1_min, S_IRUGO | S_IWUSR, + temp1_limit_show, temp1_limit_store, + limit_min); +static SENSOR_DEVICE_ATTR(humidity1_min, S_IRUGO | S_IWUSR, + humidity1_limit_show, humidity1_limit_store, + limit_min); +static SENSOR_DEVICE_ATTR(temp1_min_hyst, S_IRUGO | S_IWUSR, + temp1_limit_show, temp1_limit_store, + limit_min_hyst); +static SENSOR_DEVICE_ATTR(humidity1_min_hyst, S_IRUGO | S_IWUSR, + humidity1_limit_show, humidity1_limit_store, + limit_min_hyst);+static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO, temp1_alarm_show, NULL, 0); +static SENSOR_DEVICE_ATTR(humidity1_alarm, S_IRUGO, humidity1_alarm_show,+ NULL, 0); +static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR, + update_interval_show, update_interval_store, 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_temp1_alarm.dev_attr.attr, + &sensor_dev_attr_humidity1_alarm.dev_attr.attr, + &sensor_dev_attr_update_interval.dev_attr.attr, + 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->last_update = 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->i2c_lock); + mutex_init(&data->data_lock); + + ret = limits_update(data); + if (ret) + return ret; + + if (id->driver_data == sts3x) + 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_ids[] = { + {"sht3x", sht3x}, + {"sts3x", sts3x}, + {} +}; + +MODULE_DEVICE_TABLE(i2c, sht3x_ids); + +static struct i2c_driver sht3x_i2c_driver = { + .driver.name = "sht3x", + .probe = sht3x_probe, + .id_table = sht3x_ids, +}; + +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/include/linux/platform_data/sht3x.h b/include/linux/platform_data/sht3x.hnew file mode 100644 index 0000000..46af9a0 --- /dev/null +++ b/include/linux/platform_data/sht3x.h @@ -0,0 +1,25 @@ +/* + * Copyright (C) 2015 Sensirion AG, Switzerland2016 ?
Yes, those years just change too fast ;-)
+ * 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_ */
-- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
