On 19/02/17 15:04, Jonathan Cameron wrote: > On 19/02/17 14:58, Tomasz Duszynski wrote: >> Add sht21 relative humidity and temperature sensor driver. There already >> exists an in-kernel driver under hwmon but with limited functionality >> like humidity and temperature always measured together at predefined >> resolutions, etc. >> >> New iio driver comes without limitations of predecessor, uses non-blocking i2c >> communication mode and adds triggered buffer support thus is suited for more >> use cases. >> >> Device datasheet can be found under: >> >> https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/ \ >> Dokumente/2_Humidity_Sensors/Sensirion_Humidity_Sensors_SHT21_Datasheet_V4.pdf >> >> Signed-off-by: Tomasz Duszynski <tduszyns@xxxxxxxxx> > Hi Tomasz, > > I haven't looked at this yet, but one thing we have to do whenever suggesting > replacement of an hwmon driver is to convince the hwmon guys that it is a > good idea. > > As such I've cc'd Guenter, Jean and the list. > > Jonathan > > p.s Probably won't get to this today as running out of time. Depending on > how mad the week is it might be next weekend before I get a chance. Very quick review follows so as not to leave you without feedback. I may well have missed stuff though as did this in a few minutes... J >> --- >> drivers/iio/humidity/Kconfig | 10 + >> drivers/iio/humidity/Makefile | 1 + >> drivers/iio/humidity/sht21.c | 519 ++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 530 insertions(+) >> create mode 100644 drivers/iio/humidity/sht21.c >> >> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig >> index 866dda133336..93247ecc9324 100644 >> --- a/drivers/iio/humidity/Kconfig >> +++ b/drivers/iio/humidity/Kconfig >> @@ -35,6 +35,16 @@ config HTU21 >> This driver can also be built as a module. If so, the module will >> be called htu21. >> >> +config SHT21 >> + tristate "Sensirion SHT21 relative humidity and temperature sensor" >> + depends on I2C >> + help >> + Say yes here to build support for the Sensirion SHT21 relative >> + humidity and temperature sensor. >> + >> + To compile this driver as a module, choose M here: the module >> + will be called sht21. >> + >> config SI7005 >> tristate "SI7005 relative humidity and temperature sensor" >> depends on I2C >> diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile >> index c9f089a9a6b8..f2472fd2cc44 100644 >> --- a/drivers/iio/humidity/Makefile >> +++ b/drivers/iio/humidity/Makefile >> @@ -5,5 +5,6 @@ >> obj-$(CONFIG_DHT11) += dht11.o >> obj-$(CONFIG_HDC100X) += hdc100x.o >> obj-$(CONFIG_HTU21) += htu21.o >> +obj-$(CONFIG_SHT21) += sht21.o >> obj-$(CONFIG_SI7005) += si7005.o >> obj-$(CONFIG_SI7020) += si7020.o >> diff --git a/drivers/iio/humidity/sht21.c b/drivers/iio/humidity/sht21.c >> new file mode 100644 >> index 000000000000..199933b87297 >> --- /dev/null >> +++ b/drivers/iio/humidity/sht21.c >> @@ -0,0 +1,519 @@ >> +/* >> + * Sensirion SHT21 relative humidity and temperature sensor driver >> + * >> + * Copyright (C) 2017 Tomasz Duszynski <tduszyns@xxxxxxxxx> >> + * >> + * 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. >> + * >> + * 7-bit I2C slave address: 0x40 >> + */ >> + >> +#include <linux/bitops.h> >> +#include <linux/delay.h> >> +#include <linux/i2c.h> >> +#include <linux/iio/buffer.h> >> +#include <linux/iio/iio.h> >> +#include <linux/iio/sysfs.h> >> +#include <linux/iio/trigger_consumer.h> >> +#include <linux/iio/triggered_buffer.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/of.h> >> + >> +#define SHT21_DRV_NAME "sht21" >> + >> +#define SHT21_TRIGGER_T_MEASUREMENT 0xF3 >> +#define SHT21_TRIGGER_RH_MEASUREMENT 0xF5 >> + >> +#define SHT21_WRITE_USER_REG 0xE6 >> +#define SHT21_READ_USER_REG 0xE7 >> +#define SHT21_SOFT_RESET 0xFE >> + >> +#define SHT21_USER_REG_RES_8_12 BIT(0) >> +#define SHT21_USER_REG_RES_10_13 BIT(7) >> +#define SHT21_USER_REG_RES_11_11 (BIT(7) | BIT(0)) >> + >> +#define SHT21_USER_REG_ENABLE_HEATER BIT(2) >> + >> +enum { >> + RES_12_14, >> + RES_8_12, >> + RES_10_13, >> + RES_11_11, >> +}; >> + >> +/* >> + * Combined sampling frequency i.e measuring RH and T together, which seems >> + * to be common case for pressure/humidity sensors, was chosen so that sensor >> + * is active for only 10% of time thus avoiding self-heating effect. >> + * >> + * Note that sampling frequencies are higher when measuring RH or T separately. >> + * >> + * Following table shows how available resolutions, combined sampling frequency >> + * and frequencies for RH and T when measured separately are related. >> + * >> + * +-------------------+-------------+---------+--------+ >> + * | RH / T resolution | RH + T [Hz] | RH [Hz] | T [Hz] | >> + * +-------------------+-------------+---------+--------+ >> + * | 12 / 14 | 0.88 | 3.45 | 1.18 | >> + * +-------------------+-------------+---------+--------+ >> + * | 8 / 12 | 3.85 | 25 | 4.55 | >> + * +-------------------+-------------+---------+--------+ >> + * | 10 / 13 | 1.93 | 11.11 | 2.33 | >> + * +-------------------+-------------+---------+--------+ >> + * | 11 / 11 | 2.28 | 6.67 | 9.09 | >> + * +-------------------+-------------+---------+--------+ >> + */ >> +static const struct { >> + int freq; >> + int freq2; >> + >> + int rh_meas_time; >> + int t_meas_time; >> +} sht21_freq_meas_time_tbl[] = { >> + [RES_12_14] = { 0, 880000, 29000, 85000 }, >> + [RES_8_12] = { 3, 850000, 4000, 22000 }, >> + [RES_10_13] = { 1, 930000, 9000, 43000 }, >> + [RES_11_11] = { 2, 280000, 15000, 11000 } >> +}; >> + >> +struct sht21_state { >> + struct i2c_client *client; >> + struct mutex lock; >> + int res; >> +}; >> + >> +static int sht21_verify_crc(const u8 *data, int len) >> +{ >> + int i, j; >> + u8 crc = 0; >> + >> + for (i = 0; i < len; i++) { >> + crc ^= data[i]; >> + >> + for (j = 0; j < 8; j++) { >> + if (crc & 0x80) >> + crc = (crc << 1) ^ 0x31; >> + else >> + crc = crc << 1; >> + } >> + } >> + >> + return crc == 0; >> +} >> + >> +/* every chunk of data read from sensor has crc byte appended */ >> +static int sht21_read_data(struct sht21_state *state, int cmd, int *data) >> +{ >> + int delay, ret = 0; >> + __be32 tmp = 0; >> + >> + switch (cmd) { >> + case SHT21_READ_USER_REG: >> + ret = i2c_smbus_read_i2c_block_data(state->client, cmd, >> + 2, (u8 *)&tmp); >> + break; >> + case SHT21_TRIGGER_RH_MEASUREMENT: >> + case SHT21_TRIGGER_T_MEASUREMENT: >> + ret = i2c_smbus_write_byte(state->client, cmd); >> + if (ret) >> + break; >> + >> + delay = cmd == SHT21_TRIGGER_RH_MEASUREMENT ? >> + sht21_freq_meas_time_tbl[state->res].rh_meas_time : >> + sht21_freq_meas_time_tbl[state->res].t_meas_time; >> + >> + usleep_range(delay, delay + 1000); >> + >> + ret = i2c_master_recv(state->client, (u8 *)&tmp, 3); >> + if (ret < 0) >> + break; >> + >> + /* >> + * Sensor should not be active more that 10% of time >> + * otherwise it heats up and returns biased measurements. >> + */ >> + delay *= 9; >> + usleep_range(delay, delay + 1000); >> + >> + break; >> + } >> + >> + if (ret < 0) >> + return ret; >> + >> + if (!sht21_verify_crc((u8 *)&tmp, ret)) { >> + dev_err(&state->client->dev, "Data integrity check failed\n"); >> + return -EINVAL; >> + } >> + >> + /* crc byte no longer needed so drop it here */ >> + *data = be32_to_cpu(tmp) >> ((sizeof(tmp) - ret + 1) * 8); >> + >> + return 0; >> +} >> + >> +static int sht21_write_data(struct sht21_state *state, int cmd, int *data) >> +{ >> + int ret; No point in wrapping these two up. Just use the functions where they are relevant directly. This just makes the code harder to follow. >> + >> + if (!data) >> + ret = i2c_smbus_write_byte(state->client, cmd); >> + else >> + ret = i2c_smbus_write_byte_data(state->client, cmd, *data); >> + >> + return ret; >> +} >> + >> +static int sht21_trigger_measurement(struct sht21_state *state, int cmd, int *data) >> +{ >> + int ret; >> + >> + mutex_lock(&state->lock); >> + ret = sht21_read_data(state, cmd, data); >> + if (ret) { >> + mutex_unlock(&state->lock); >> + return ret; >> + } >> + mutex_unlock(&state->lock); >> + >> + return 0; >> +} >> + >> +static int sht21_trigger_rh_measurement(struct sht21_state *state, int *data) >> +{ This is doing more than just triggering the measurement. The naming should reflect that. >> + int ret = sht21_trigger_measurement(state, SHT21_TRIGGER_RH_MEASUREMENT, data); >> + if (ret) >> + return ret; >> + >> + *data >>= 2; >> + *data = (((s64)*data * 125000) >> 14) - 6000; >> + >> + return 0; >> +} >> + >> +static int sht21_trigger_t_measurement(struct sht21_state *state, int *data) >> +{ >> + int ret = sht21_trigger_measurement(state, SHT21_TRIGGER_T_MEASUREMENT, data); >> + if (ret) >> + return ret; >> + >> + *data >>= 2; >> + *data = (((s64)*data * 175720) >> 14) - 46850; >> + >> + return 0; >> +} >> + >> +static int sht21_set_resolution(struct sht21_state *state, int res) >> +{ >> + int ret, data; >> + >> + ret = sht21_read_data(state, SHT21_READ_USER_REG, &data); >> + if (ret) >> + return ret; >> + >> + data &= ~SHT21_USER_REG_RES_11_11; >> + >> + switch (res) { >> + case RES_12_14: >> + break; >> + case RES_8_12: >> + data |= SHT21_USER_REG_RES_8_12; >> + break; >> + case RES_10_13: >> + data |= SHT21_USER_REG_RES_10_13; >> + break; >> + case RES_11_11: >> + data |= SHT21_USER_REG_RES_11_11; >> + break; >> + } >> + >> + ret = sht21_write_data(state, SHT21_WRITE_USER_REG, &data); >> + if (ret) >> + return ret; >> + >> + state->res = res; >> + >> + return 0; >> +} >> + >> +static int sht21_soft_reset(struct sht21_state *state) >> +{ >> + int ret = sht21_write_data(state, SHT21_SOFT_RESET, NULL); >> + if (ret) { >> + dev_err(&state->client->dev, "Failed to reset device\n"); >> + return ret; >> + } >> + >> + msleep(15); >> + >> + return 0; >> +} >> + >> +static irqreturn_t sht21_trigger_handler(int irq, void *p) >> +{ >> + struct iio_poll_func *pf = p; >> + struct iio_dev *indio_dev = pf->indio_dev; >> + struct sht21_state *state = iio_priv(indio_dev); >> + int buf[4], ret, i, j = 0; >> + >> + for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) { >> + ret = i == 0 ? sht21_trigger_rh_measurement(state, &buf[j++]) : >> + sht21_trigger_t_measurement(state, &buf[j++]); This is a for loop that really just obscures what is going on. I'd open code it. >> + if (ret) >> + goto out; >> + } >> + >> + iio_push_to_buffers_with_timestamp(indio_dev, buf, >> + iio_get_time_ns(indio_dev)); >> +out: >> + iio_trigger_notify_done(indio_dev->trig); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int sht21_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *channel, int *val, >> + int *val2, long mask) >> +{ >> + struct sht21_state *state = iio_priv(indio_dev); >> + int ret; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_PROCESSED: >> + if (iio_buffer_enabled(indio_dev)) >> + return -EBUSY; You've randomly mixed checking this by hand and using the claim_direct functions. Please use them everywhere. They aren't race prone (which this is). >> + >> + switch (channel->type) { >> + case IIO_HUMIDITYRELATIVE: >> + ret = sht21_trigger_rh_measurement(state, val); >> + if (ret) >> + return ret; >> + >> + return IIO_VAL_INT; >> + case IIO_TEMP: >> + ret = sht21_trigger_t_measurement(state, val); >> + if (ret) >> + return ret; >> + >> + return IIO_VAL_INT; >> + default: >> + return -EINVAL; >> + } >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + *val = sht21_freq_meas_time_tbl[state->res].freq; >> + *val2 = sht21_freq_meas_time_tbl[state->res].freq2; >> + >> + return IIO_VAL_INT_PLUS_MICRO; >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static int sht21_write_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int val, int val2, long mask) >> +{ >> + struct sht21_state *state = iio_priv(indio_dev); >> + int i, ret; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + for (i = 0; i < ARRAY_SIZE(sht21_freq_meas_time_tbl); i++) >> + if (val == sht21_freq_meas_time_tbl[i].freq && >> + val2 == sht21_freq_meas_time_tbl[i].freq2) >> + break; >> + >> + if (i == ARRAY_SIZE(sht21_freq_meas_time_tbl)) >> + return -EINVAL; >> + >> + ret = iio_device_claim_direct_mode(indio_dev); >> + if (ret) >> + return ret; >> + >> + mutex_lock(&state->lock); >> + ret = sht21_set_resolution(state, i); >> + mutex_unlock(&state->lock); >> + >> + iio_device_release_direct_mode(indio_dev); >> + >> + return ret; >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static ssize_t sht21_show_heater(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> + struct sht21_state *state = iio_priv(indio_dev); >> + int data, ret; >> + >> + ret = sht21_read_data(state, SHT21_READ_USER_REG, &data); >> + if (ret) >> + return ret; >> + >> + return sprintf(buf, "%d\n", !!(data & SHT21_USER_REG_ENABLE_HEATER)); >> +} >> + >> +static ssize_t sht21_write_heater(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) >> +{ >> + struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> + struct sht21_state *state = iio_priv(indio_dev); >> + int val, data, ret; >> + >> + ret = kstrtoint(buf, 10, &val); >> + if (ret) >> + return ret; >> + if (val != 0 && val != 1) >> + return -EINVAL; >> + >> + mutex_lock(&state->lock); >> + ret = sht21_read_data(state, SHT21_READ_USER_REG, &data); >> + if (ret) { >> + mutex_unlock(&state->lock); >> + return ret; >> + } >> + >> + if (val) >> + data |= SHT21_USER_REG_ENABLE_HEATER; >> + else >> + data &= ~SHT21_USER_REG_ENABLE_HEATER; >> + >> + ret = sht21_write_data(state, SHT21_WRITE_USER_REG, &data); >> + mutex_unlock(&state->lock); >> + >> + return ret ? ret : len; >> +} >> + >> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.88 1.93 2.28 3.85"); >> +static IIO_DEVICE_ATTR(heater_enable, S_IRUGO | S_IWUSR, >> + sht21_show_heater, sht21_write_heater, 0); >> + >> +static struct attribute *sht21_attributes[] = { >> + &iio_const_attr_sampling_frequency_available.dev_attr.attr, >> + &iio_dev_attr_heater_enable.dev_attr.attr, >> + NULL >> +}; >> + >> +static const struct attribute_group sht21_attribute_group = { >> + .attrs = sht21_attributes, >> +}; >> + >> +static const struct iio_info sht21_info = { >> + .driver_module = THIS_MODULE, >> + .read_raw = sht21_read_raw, >> + .write_raw = sht21_write_raw, >> + .attrs = &sht21_attribute_group, >> +}; >> + >> +#define SHT21_CHANNEL(_type, _index) { \ >> + .type = _type, \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \ >> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ >> + .scan_index = _index, \ >> + .scan_type = { \ >> + .sign = 's', \ >> + .realbits = 32, \ Really? does it provide true 32bit data? >> + .storagebits = 32, \ >> + .endianness = IIO_CPU, \ >> + }, \ >> +} >> + >> +static const struct iio_chan_spec sht21_channels[] = { >> + SHT21_CHANNEL(IIO_HUMIDITYRELATIVE, 0), >> + SHT21_CHANNEL(IIO_TEMP, 1), >> + IIO_CHAN_SOFT_TIMESTAMP(2), >> +}; >> + >> +static int sht21_probe(struct i2c_client *client, const struct i2c_device_id *id) >> +{ >> + struct iio_dev *indio_dev; >> + struct sht21_state *data; >> + int ret; >> + >> + if (!i2c_check_functionality(client->adapter, >> + I2C_FUNC_SMBUS_WRITE_BYTE | >> + I2C_FUNC_SMBUS_BYTE_DATA | >> + I2C_FUNC_SMBUS_READ_I2C_BLOCK)) >> + return -ENODEV; >> + >> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >> + if (!indio_dev) >> + return -ENOMEM; >> + >> + data = iio_priv(indio_dev); >> + i2c_set_clientdata(client, indio_dev); >> + data->client = client; >> + mutex_init(&data->lock); >> + >> + indio_dev->dev.parent = &client->dev; >> + indio_dev->name = id->name; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->info = &sht21_info; >> + indio_dev->channels = sht21_channels; >> + indio_dev->num_channels = ARRAY_SIZE(sht21_channels); >> + >> + ret = sht21_soft_reset(data); >> + if (ret) >> + return ret; >> + >> + ret = iio_triggered_buffer_setup(indio_dev, NULL, >> + sht21_trigger_handler, NULL); >> + if (ret) >> + return ret; >> + >> + ret = iio_device_register(indio_dev); >> + if (ret) >> + iio_triggered_buffer_cleanup(indio_dev); >> + >> + return ret; >> +} >> + >> +static int sht21_remove(struct i2c_client *client) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >> + >> + iio_device_unregister(indio_dev); >> + iio_triggered_buffer_cleanup(indio_dev); There are devm versions of both of the thing for the two setup functions you are unwinding here. Use them and you can drop the remove function entirely. >> + >> + return 0; >> +} >> + >> +static const struct i2c_device_id sht21_id[] = { >> + { "sht21", 0 }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(i2c, sht21_id); >> + >> +static const struct of_device_id sht21_of_match[] = { >> + { .compatible = "sensirion,sht21" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, sht21_of_match); >> + >> +static struct i2c_driver sht21_driver = { >> + .driver = { >> + .name = SHT21_DRV_NAME, >> + .of_match_table = of_match_ptr(sht21_of_match), >> + }, >> + .id_table = sht21_id, >> + .probe = sht21_probe, >> + .remove = sht21_remove, >> +}; >> +module_i2c_driver(sht21_driver); >> + >> +MODULE_DESCRIPTION("Sensirion SHT21 relative humidity and temperature sensor driver"); >> +MODULE_AUTHOR("Tomasz Duszynski <tduszyns@xxxxxxxxx>"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.11.1 >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
