hwmon: driver for Sensirion SHT21 humidity and temperature sensor
Subsystems (2)
PATCH 1/1 hwmon: driver for Sensirion SHT21 humidity and temperature sensor
Hi,
This is a driver for the new humidity and temperature sensors SHT21 and SHT25 from Sensirion.
Regards,
Urs
drivers/hwmon/Kconfig +10 -0
@@ -688,6 +688,16 @@ config SENSORS_SHT15 This driver can also be built as a module. If so, the module will be called sht15. +config SENSORS_SHT21 + tristate "Sensiron humidity and temperature sensors. SHT21 and compat." + depends on I2C + help + If you say yes here you get support for the Sensiron SHT21, SHT25 + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module + will be called sht21. + config SENSORS_S3C tristate "S3C24XX/S3C64XX Inbuilt ADC" depends on ARCH_S3C2410
drivers/hwmon/Makefile +1 -0
@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o +obj-$(CONFIG_SENSORS_SHT21) += sht21.o obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o obj-$(CONFIG_SENSORS_SMM665) += smm665.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
drivers/hwmon/sht21.c new file +392 -0
392 lines not on this page · raw message
Quoted 3 lines · Urs Fleisch
Hi,
This is a driver for the new humidity and temperature sensors SHT21 and SHT25 from Sensirion.
Firstly, I've cc'd the relevant mailing list and maintainers for hwmon drivers. Next, there are some bits in here that won't have passed through a run of checkpatch.pl so I'm guessing you haven't followed all the steps in Documentation/SubmittingPatches or the checklist. All these steps make for a cleaner path into the kernel.
Nice to see sensiron have finally adopted a standard bus type, much nicer driver than the mess that was needed for the sht15 :) I see from the datasheet that you can also use that protocol to talk to this device but people should definitely use this nice i2c driver if they can!
Excellent to see Sensiron producing drivers for their parts.
Other than formatting cleanups and requests for clarification my only real issue is the magic numbers involved in that user register. That must be broken out into easy to understand attributes. For those elements not covered by current hwmon standards, you should propose an option on the mailing lists along with appropriate documentation updates. If you are unsure on what these should be, please consult the maintainers cc'd to this email, they are both extremely helpful!
All in all, a nice simple driver that should be easy to fix up.
Thanks,
Jonathan
Quoted 68 lines
Regards,
Ursdrivers/hwmon/Kconfig +10
-0@@ -688,6 +688,16 @@ config SENSORS_SHT15 This driver can also be built as a module. If so, the module will be called sht15. +config SENSORS_SHT21 + tristate "Sensiron humidity and temperature sensors. SHT21 and compat." + depends on I2C + help + If you say yes here you get support for the Sensiron SHT21, SHT25 + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module + will be called sht21. + config SENSORS_S3C tristate "S3C24XX/S3C64XX Inbuilt ADC" depends on ARCH_S3C2410drivers/hwmon/Makefile +1
-0@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o +obj-$(CONFIG_SENSORS_SHT21) += sht21.o obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o obj-$(CONFIG_SENSORS_SMM665) += smm665.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.odrivers/hwmon/sht21.c new file +24
-0@@ -0,0 +1,392 @@ +/* Sensirion SHT21 humidity and temperature sensor driver +* +* Copyright (C) 2010 Urs Fleisch <[email protected]> +* +* 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 (5/2010) at +* http://www.sensirion.com/en/pdf/product_information/Datasheet-humidity-sensor-SHT21.pdf +*/ + +#include <linux/module.h> +#include <linux/init.h>
drivers/hwmon/sht21.c line 34
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+#include <linux/slab.h> +#include <linux/i2c.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/err.h> +#include <linux/mutex.h> +#include <linux/device.h> + +/* I2C commands bytes */ +enum sht21_command {
These are certainly not a natural enum. A set of '#define's will be clearer.
drivers/hwmon/sht21.c line 45
Quoted 11 lines
drivers/hwmon/sht21.c new file +11
-0+SHT21_TRIG_T_MEASUREMENT_HM =0xe3 , +SHT21_TRIG_RH_MEASUREMENT_HM =0xe5 , +SHT21_TRIG_T_MEASUREMENT_POLL =0xf3 , +SHT21_TRIG_RH_MEASUREMENT_POLL =0xf5 , +SHT21_USER_REG_W =0xe6 , +SHT21_USER_REG_R =0xe7 , +SHT21_SOFT_RESET =0xfe +}; + +/* User register bit masks */ +enum sht21_userreg {
Same, here. This really isn't a sensible enum.
Quoted 11 lines
drivers/hwmon/sht21.c new file +11
-0+SHT21_RES_12_14BIT =0x00 , +SHT21_RES_8_12BIT =0x01 , +SHT21_RES_10_13BIT =0x80 , +SHT21_RES_11_11BIT =0x81 , +SHT21_RES_MASK =0x81 , +SHT21_END_OF_BATTERY =0x40 , +SHT21_ENABLE_HEATER =0x04 , + SHT21_WRITABLE_MASK = 0x87 +}; + +/* Types of measurements */
drivers/hwmon/sht21.c line 66
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+enum sht21_measurement_index { +SHT21_TEMPERATURE , +SHT21_HUMIDITY , +SHT21_NUM_MEASUREMENTS +}; + +/** +* struct sht21 - SHT21 device specific data +* @hwmon_dev: device registered with hwmon +* @lock: mutex to ensure only one I2C transfer in progress at a time
Comment probably wants clarifying. I2C transfers can only happen one at a time anyway. What you are actually protecting is the value of control registers when you do a read - modify - write. It also protects various bits of this structure.
drivers/hwmon/sht21.c line 67
Quoted 1 line
drivers/hwmon/sht21.c new file +1
-0+* @userreg_orig: original value of user register
Why is this cached in here? Looks to be used as temporary cache inside probe then set back on the driver being removed? Is this appropriate? Typically unless there is a strong reason why things like the default value of this register are provided by plaform data for a given board.
drivers/hwmon/sht21.c line 68
Quoted 1 line
drivers/hwmon/sht21.c new file +1
-0+* @valid: !=0 if measurements are valid
Perhaps ammend the comment to say they are only invalid before the first measurement is taken?
drivers/hwmon/sht21.c line 81
Quoted 13 lines
drivers/hwmon/sht21.c new file +13
-0+* @last_update: time of last update (jiffies) +* @measurements: cached measurement values, temperature and humidity +*/ +struct sht21 { +struct device *hwmon_dev; +struct mutex lock; +u8 userreg_orig; +char valid; +unsigned long last_update; +int measurements[SHT21_NUM_MEASUREMENTS ]; +}; + +/* I2C command to be used for a specifc measurement */
const as well I think? Actually, given we only have one use of this I'd loose this array and simply have a switch (or if / else) inline.
Quoted 22 lines
drivers/hwmon/sht21.c new file +22
-0+static u8 sht21_measurement_commands[SHT21_NUM_MEASUREMENTS ] = { +SHT21_TRIG_T_MEASUREMENT_HM , +SHT21_TRIG_RH_MEASUREMENT_HM +}; + +/** +* sht21_temp_ticks_to_millicelsius() - convert raw temperature ticks to +* milli celsius +* @ticks: temperature ticks value received from sensor +*/ +static int sht21_temp_ticks_to_millicelsius (int ticks) +{ + ticks &= ~0x0003 ;/* clear status bits */ +/* Formula T = -46.85 + 175.72 * ST / 2^16 from data sheet 6.2, +optimized for integer fixed point (3 digits) arithmetic */ +return ((21965 * ticks) >>13 ) -46850 ; +} + +/** +* sht21_rh_ticks_to_per_cent_mille() - convert raw humidity ticks to +* one-thousandths of a percent relative humidity +* @ticks: humidity ticks value received from sensor
drivers/hwmon/sht21.c line 113
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+*/ +static int sht21_rh_ticks_to_per_cent_mille (int ticks) +{ + ticks &= ~0x0003 ;/* clear status bits */ +/* Formula RH = -6 + 125 * SRH / 2^16 from data sheet 6.1, +optimized for integer fixed point (3 digits) arithmetic */ +return ((15625 * ticks) >>13 ) -6000 ; +} + +typedef int (*convert_func )(int );
Don't do this. It just makes the code harder to read. Again, only used once so I'd just use a switch or if /else inline and get rid of this array.
Quoted 51 lines
drivers/hwmon/sht21.c new file +51
-0+ +/* Conversion function to be used for a specifc measurement */ +static convert_func sht21_convert_raw_value[SHT21_NUM_MEASUREMENTS ] = { + sht21_temp_ticks_to_millicelsius, + sht21_rh_ticks_to_per_cent_mille +}; + +/** +* sht21_update_measurements() - get updated measurements from device +* @client: I2C client device +* +* Returns SHT21 client data containing measurements. +*/ +static struct sht21 *sht21_update_measurements (struct i2c_client *client) +{ +struct sht21 *sht21 =i2c_get_clientdata (client); + +mutex_lock (&sht21->lock); +/* Data sheet 2.4: +* SHT2x should not be active for more than 10% of the time - e.g. maximum +* two measurements per second at 12bit accuracy shall be made. */ +if (time_after (jiffies, sht21->last_update +HZ /2 ) || !sht21->valid) { +int meas_idx; +for (meas_idx =0 ; meas_idx <SHT21_NUM_MEASUREMENTS ; ++meas_idx) { +int result =i2c_smbus_read_word_data (client, + sht21_measurement_commands[meas_idx]);. +/* SMBus specifies low byte first, but the SHT21 returns MSB first, +* so we have to swab16 the values */ +if (result >=0 ) + sht21->measurements[meas_idx] = + sht21_convert_raw_value[meas_idx](swab16 (result)); + } + sht21->last_update = jiffies; + sht21->valid =1 ; + } +mutex_unlock (&sht21->lock); + +return sht21; +} + +/** +* sht21_show_measurement() - show measurement value in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to +* +* Will be called on read access to a measurement sysfs attribute. +* Returns number of bytes written into buffer. +*/ +static ssize_t sht21_show_measurement(struct device *dev, + struct device_attribute *attr,
drivers/hwmon/sht21.c line 174
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+ char *buf) +{ +struct sensor_device_attribute *sda =to_sensor_dev_attr (attr); +struct sht21 *sht21 =sht21_update_measurements (to_i2c_client (dev)); + +return sprintf (buf,"%d\n" , sht21->measurements[sda->index]); +} + +/** +* sht21_show_userreg() - show user register value in sysfs
Sorry, but this isn't going to be acceptable. Classic case of magic numbers. This needs to be broken up into several different attributes.
We have:
* control over the measurement resolution (which is somewhat fiddly on this device)
* Battery voltage threshold > 2.25V
* Enable on chip heater
* OTP stuff. I think this basically a 'one shot' mode where the device flips back to default status after a single reading. Unless you have a user of this, I'd be tempted to just ignore that bit.
Quoted 47 lines
drivers/hwmon/sht21.c new file +47
-0+* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where user register value is written to +* +* Will be called on read access to user_register sysfs attribute. +* Returns number of bytes written into buffer, -EIO on error. +*/ +static ssize_t sht21_show_userreg (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct i2c_client *client =to_i2c_client (dev); +struct sht21 *sht21 =i2c_get_clientdata (client); +int result; +mutex_lock (&sht21->lock); + result =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +mutex_unlock (&sht21->lock); + +return result >=0 ?sprintf (buf,"%d\n" , result) : -EIO ; +} + +/** +* sht21_store_userreg() - store sysfs value in user register +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer containing value as string +* @count: number of characters in @buf +* +* Will be called on write access to user_register sysfs attribute. +* Returns @count, -EIO on error. +*/ +static ssize_t sht21_store_userreg (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +struct i2c_client *client =to_i2c_client (dev); +struct sht21 *sht21 =i2c_get_clientdata (client); +int result; +char *endptr; +unsigned long value =simple_strtoul (buf, &endptr,0 ); +if (endptr == buf || value >0xff ) +return -EINVAL ; + +mutex_lock (&sht21->lock); + result =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +if (result >=0 ) + result =i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , + (result & ~SHT21_WRITABLE_MASK ) | (value &SHT21_WRITABLE_MASK ));
drivers/hwmon/sht21.c line 231
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+mutex_unlock (&sht21->lock); + +return result >=0 ? count : -EIO ; +} + +/* sysfs attributes */ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, sht21_show_measurement, + NULL, SHT21_TEMPERATURE); +static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO, sht21_show_measurement, + NULL, SHT21_HUMIDITY);
So this one is the only one I have problem with. Other two attributes are standard (well humidity is pretty unusual but no one ever complained about the sht15 afaik!)
Quoted 40 lines
drivers/hwmon/sht21.c new file +40
-0+static SENSOR_DEVICE_ATTR(user_register, S_IRUGO | S_IWUSR, sht21_show_userreg, + sht21_store_userreg, 0); + +static struct attribute *sht21_attributes[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_humidity1_input.dev_attr.attr, + &sensor_dev_attr_user_register.dev_attr.attr, +NULL +}; + +static const struct attribute_group sht21_attr_group = { + .attrs = sht21_attributes, +}; + +/** +* sht21_probe() - probe device +* @client: I2C client device +* @id: device ID +* +* Called by the I2C core when an entry in the ID table matches a +* device's name. +* Returns 0 on success. +*/ +static int __devinit sht21_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct sht21 *sht21; +int status; +u8 userreg_changed; + +if (!i2c_check_functionality (client->adapter,I2C_FUNC_SMBUS_WORD_DATA )) { +dev_err (&client->dev,"adapter does not support SMBus word transactions\n" ); +return -ENODEV ; + } + + sht21 =kzalloc (sizeof (*sht21),GFP_KERNEL ); +if (!sht21) { +dev_dbg (&client->dev,"kzalloc failed\n" ); +return -ENOMEM ; + }
drivers/hwmon/sht21.c line 281
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+i2c_set_clientdata (client, sht21); + + status =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +if (status <0 ) { +dev_err (&client->dev,"error reading user register\n" ); + goto fail_free; + } + sht21->userreg_orig = status; + + userreg_changed = status ^SHT21_RES_MASK ;
This needs a comment. Why exclusive or with a mask?
drivers/hwmon/sht21.c line 293
Quoted 12 lines
drivers/hwmon/sht21.c new file +12
-0+ status =i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , userreg_changed); +if (status <0 ) { +dev_err (&client->dev,"error writing user register\n" ); + goto fail_restore_config; + } + status =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +if (status <0 ) { +dev_err (&client->dev,"error reading user register\n" ); + goto fail_restore_config; + } +if (status != userreg_changed) { +dev_err (&client->dev,"user register settings did not stick\n" );
This does seem a somewhat paranoid check. Is it really needed in a production driver? If this has been seen in the wild, then fair enough!
drivers/hwmon/sht21.c line 301
Quoted 8 lines
drivers/hwmon/sht21.c new file +8
-0+ status = -ENODEV ; + goto fail_restore_config; + } + status =i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , sht21->userreg_orig); +if (status <0 ) { +dev_err (&client->dev,"error restoring user register\n" ); + goto fail_restore_config; + }
comment to explain this please. We don't have an update so why are we setting last_update? Obviously valid will ensure we do a new capture whatever this value is set to.
Quoted 34 lines
drivers/hwmon/sht21.c new file +34
-0+ sht21->last_update = jiffies -HZ ; +mutex_init (&sht21->lock); + + status =sysfs_create_group (&client->dev.kobj, &sht21_attr_group); +if (status) { +dev_dbg (&client->dev,"could not create sysfs files\n" ); + goto fail_restore_config; + } + sht21->hwmon_dev =hwmon_device_register (&client->dev); +if (IS_ERR (sht21->hwmon_dev)) { +dev_dbg (&client->dev,"unable to register hwmon device\n" ); + status =PTR_ERR (sht21->hwmon_dev); + goto fail_remove_sysfs; + } + +dev_info (&client->dev,"initialized\n" ); + +return 0 ; + +fail_remove_sysfs: +sysfs_remove_group (&client->dev.kobj, &sht21_attr_group); +fail_restore_config: +i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , sht21->userreg_orig); +fail_free: +kfree (sht21); + +return status; +} + +/** +* sht21_remove() - remove device +* @client: I2C client device +*/ +static int __devexit sht21_remove(struct i2c_client *client)
drivers/hwmon/sht21.c line 345
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+{ +struct sht21 *sht21 =i2c_get_clientdata (client); +int status; + +hwmon_device_unregister (sht21->hwmon_dev); +sysfs_remove_group (&client->dev.kobj, &sht21_attr_group); + + status =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +if (status >=0 && status != sht21->userreg_orig) { +i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , sht21->userreg_orig);
Superflous brackets.
Quoted 47 lines
drivers/hwmon/sht21.c new file +47
-0+ } + +kfree (sht21); + +return 0 ; +} + +/* Device ID table */ +static const struct i2c_device_id sht21_id[] = { + {"sht21" ,0 }, + { } +}; +MODULE_DEVICE_TABLE (i2c, sht21_id); + +static struct i2c_driver sht21_driver = { + .driver.name ="sht21" , + .probe = sht21_probe, + .remove =__devexit_p (sht21_remove), + .id_table = sht21_id, +}; + +/** +* sht21_init() - initialize driver +* +* Called when kernel is booted or module is inserted. +* Returns 0 on success. +*/ +static int __init sht21_init (void ) +{ +return i2c_add_driver (&sht21_driver); +} +module_init (sht21_init); + +/** +* sht21_init() - clean up driver +* +* Called when module is removed. +*/ +static void __exit sht21_exit(void) +{ +i2c_del_driver (&sht21_driver); +} +module_exit (sht21_exit); + +MODULE_AUTHOR ("Urs Fleisch <[email protected]>" ); +MODULE_DESCRIPTION ("Sensirion SHT21 humidity and temperature sensor driver" ); +MODULE_LICENSE ("GPL" );
Hi Jonathan,
Thanks for reviewing the patch. I have fixed the issues you reported and the style problems.
Quoted 11 lines
Sorry, but this isn't going to be acceptable. Classic case of magic numbers. This needs to be broken up into several different attributes.
We have:
* control over the measurement resolution (which is somewhat fiddly on this device)
* Battery voltage threshold > 2.25V
* Enable on chip heaterSo this one is the only one I have problem with. Other two attributes are standard (well humidity is pretty unusual but no one ever complained about the sht15 afaik!)
The user_register attribute is now replaced by temp1_resolution_bits, humidity1_resolution_bits, in0_min_alarm, heater_enable attributes. in0_min_alarm is used for the "end of battery" state, the other attributes are non-standard. Unfortunately, the temperature and humidity resolutions are not independent. Is this acceptable?
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ userreg_changed = status ^ SHT21_RES_MASK;This needs a comment. Why exclusive or with a mask?
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (status != userreg_changed) { + dev_err(&client->dev, "user register settings did not stick\n");This does seem a somewhat paranoid check. Is it really needed in a production driver? If this has been seen in the wild, then fair enough!
The resolution bits of the user register are toggled and read back to make sure that this is really an SHT21 device.
Thanks,
Urs
---
drivers/hwmon/Kconfig +10 -0
@@ -688,6 +688,16 @@ config SENSORS_SHT15 This driver can also be built as a module. If so, the module will be called sht15. +config SENSORS_SHT21 + tristate "Sensiron humidity and temperature sensors. SHT21 and compat." + depends on I2C + help + If you say yes here you get support for the Sensiron SHT21, SHT25 + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module + will be called sht21. + config SENSORS_S3C tristate "S3C24XX/S3C64XX Inbuilt ADC" depends on ARCH_S3C2410
drivers/hwmon/Makefile +1 -0
@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o +obj-$(CONFIG_SENSORS_SHT21) += sht21.o obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o obj-$(CONFIG_SENSORS_SMM665) += smm665.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
drivers/hwmon/sht21.c new file +605 -0
605 lines not on this page · raw message
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 1 line · Urs Fleisch
Hi Jonathan,
When posting an updated driver, use the form
[PATCH V2] hwmon: driver...
That way it will be obvious to people that it isn't just a repost of the previous code.
Quoted 12 lines
Thanks for reviewing the patch. I have fixed the issues you reported and the style problems.
Quoted 7 lines
Sorry, but this isn't going to be acceptable. Classic case of magic numbers. This needs to be broken up into several different attributes.
We have:
* control over the measurement resolution (which is somewhat fiddly on this device)
* Battery voltage threshold > 2.25V
* Enable on chip heater
Quoted 6 lines
Quoted 3 lines
So this one is the only one I have problem with. Other two attributes are standard (well humidity is pretty unusual but no one ever complained about the sht15 afaik!)
The
user_registerattribute is now replaced bytemp1_resolution_bits,humidity1_resolution_bits,
Those two aren't in the documentation for hwmon. Guenter/Jean, how should this be done?
Is there actualy a use case that means these can't both be set by platform data? Also, if these are acceptable, should they have some means of indicating which combination of values are available?
Quoted 4 lines
in0_min_alarm,heater_enable
attributes.in0_min_alarmis used for the "end of battery" state, the other attributes are non-standard. Unfortunately, the temperature and humidity resolutions are not independent. Is this acceptable?
Up to maintainers. Other than trying to work out some way of indicating valid options for the two resolution attributes I'd be happy with these.
Quoted 10 lines
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ userreg_changed = status ^ SHT21_RES_MASK;This needs a comment. Why exclusive or with a mask?
drivers/hwmon/sht21.c line 513
Quoted 2 lines
drivers/hwmon/sht21.c new file +2
-0+if (status != userreg_changed) { +dev_err (&client->dev,"user register settings did not stick\n" );This does seem a somewhat paranoid check. Is it really needed in a production driver? If this has been seen in the wild, then fair enough!
The resolution bits of the user register are toggled and read back to make sure that this is really an SHT21 device.\
Rely on platform data to get this right rather than an adhoc test that might well pass on some random devices.
Sorry to say I missed some error handling issues the first time around. Please always provide userspace with the most detailed and correct error possible. Normally this is the one comming up from the bus subsystem.
Nearly there as far as I am concerned!
Thanks,
Jonathan
Quoted 147 lines
Thanks,
UrsSigned-off-by Urs Fleisch <[email protected]>---
drivers/hwmon/Kconfig +10
-0@@ -688,6 +688,16 @@ config SENSORS_SHT15 This driver can also be built as a module. If so, the module will be called sht15. +config SENSORS_SHT21 + tristate "Sensiron humidity and temperature sensors. SHT21 and compat." + depends on I2C + help + If you say yes here you get support for the Sensiron SHT21, SHT25 + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module + will be called sht21. + config SENSORS_S3C tristate "S3C24XX/S3C64XX Inbuilt ADC" depends on ARCH_S3C2410drivers/hwmon/Makefile +1
-0@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o +obj-$(CONFIG_SENSORS_SHT21) += sht21.o obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o obj-$(CONFIG_SENSORS_SMM665) += smm665.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.odrivers/hwmon/sht21.c new file +102
-0@@ -0,0 +1,607 @@ +/* Sensirion SHT21 humidity and temperature sensor driver +* +* Copyright (C) 2010 Urs Fleisch <[email protected]> +* +* 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 (5/2010) at +* http://www.sensirion.com/en/pdf/product_information/Datasheet-humidity-sensor-SHT21.pdf +*/ + +#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/mutex.h> +#include <linux/device.h> + +/* I2C commands bytes */ +#define SHT21_TRIG_T_MEASUREMENT_HM 0xe3 +#define SHT21_TRIG_RH_MEASUREMENT_HM 0xe5 +#define SHT21_USER_REG_W 0xe6 +#define SHT21_USER_REG_R 0xe7 + +/* User register bit masks */ +#define SHT21_RES_12_14BIT 0x00 +#define SHT21_RES_8_12BIT 0x01 +#define SHT21_RES_10_13BIT 0x80 +#define SHT21_RES_11_11BIT 0x81 +#define SHT21_RES_MASK 0x81 +#define SHT21_END_OF_BATTERY 0x40 +#define SHT21_ENABLE_HEATER 0x04 + +/** +* struct sht21 - SHT21 device specific data +* @hwmon_dev: device registered with hwmon +* @lock: mutex to protect measurement values and read-modify-write access to +* user register +* @valid: only 0 before first measurement is taken +* @last_update: time of last update (jiffies) +* @temperature: cached temperature measurement value +* @humidity: cached humidity measurement value +*/ +struct sht21 { +struct device *hwmon_dev; +struct mutex lock; +char valid; +unsigned long last_update; +int temperature; +int humidity; +}; + +/** +* sht21_temp_ticks_to_millicelsius() - convert raw temperature ticks to +* milli celsius +* @ticks: temperature ticks value received from sensor +*/ +static inline int sht21_temp_ticks_to_millicelsius (int ticks) +{ + ticks &= ~0x0003 ;/* clear status bits */ +/* Formula T = -46.85 + 175.72 * ST / 2^16 from data sheet 6.2, +optimized for integer fixed point (3 digits) arithmetic */ +return ((21965 * ticks) >>13 ) -46850 ; +} + +/** +* sht21_rh_ticks_to_per_cent_mille() - convert raw humidity ticks to +* one-thousandths of a percent relative humidity +* @ticks: humidity ticks value received from sensor +*/ +static inline int sht21_rh_ticks_to_per_cent_mille (int ticks) +{ + ticks &= ~0x0003 ;/* clear status bits */ +/* Formula RH = -6 + 125 * SRH / 2^16 from data sheet 6.1, +optimized for integer fixed point (3 digits) arithmetic */ +return ((15625 * ticks) >>13 ) -6000 ; +} + +/** +* sht21_update_measurements() - get updated measurements from device +* @client: I2C client device +* +* Returns SHT21 client data containing measurements. +*/ +static struct sht21 *sht21_update_measurements (struct i2c_client *client) +{ +struct sht21 *sht21 =i2c_get_clientdata (client); +
drivers/hwmon/sht21.c line 112
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+mutex_lock (&sht21->lock); +/* Data sheet 2.4: +* SHT2x should not be active for more than 10% of the time - e.g. +* maximum two measurements per second at 12bit accuracy shall be made. +*/ +if (time_after (jiffies, sht21->last_update +HZ /2 ) || !sht21->valid) { +int result =i2c_smbus_read_word_data (client, +SHT21_TRIG_T_MEASUREMENT_HM ); +/* SMBus specifies low byte first, but the SHT21 returns MSB +* first, so we have to swab16 the values */
If it is less than 0, we have an error that ought to be correctly handled. The correct option is to pass this all the way up to userspace.
Quoted 117 lines
drivers/hwmon/sht21.c new file +117
-0+if (result >=0 ) + sht21->temperature = +sht21_temp_ticks_to_millicelsius (swab16 (result)); + result =i2c_smbus_read_word_data (client, +SHT21_TRIG_RH_MEASUREMENT_HM ); +if (result >=0 ) + sht21->humidity = +sht21_rh_ticks_to_per_cent_mille (swab16 (result)); + sht21->last_update = jiffies; + sht21->valid =1 ; + } +mutex_unlock (&sht21->lock); + +return sht21; +} + +/** +* sht21_show_temperature() - show temperature measurement value in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to +* +* Will be called on read access to temp1_input sysfs attribute. +* Returns number of bytes written into buffer. +*/ +static ssize_t sht21_show_temperature (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct sht21 *sht21 =sht21_update_measurements (to_i2c_client (dev)); + +return sprintf (buf,"%d\n" , sht21->temperature); +} + +/** +* sht21_show_humidity() - show humidity measurement value in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to +* +* Will be called on read access to humidity1_input sysfs attribute. +* Returns number of bytes written into buffer. +*/ +static ssize_t sht21_show_humidity (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct sht21 *sht21 =sht21_update_measurements (to_i2c_client (dev)); + +return sprintf (buf,"%d\n" , sht21->humidity); +} + +/** +* sht21_read_user_register() - read user register +* @dev: device +* +* Returns byte value of user register, <0 on error. +*/ +static s32 sht21_read_user_register (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +struct sht21 *sht21 =i2c_get_clientdata (client); +s32 result; +mutex_lock (&sht21->lock); + result =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +mutex_unlock (&sht21->lock); +return result; +} + +/** +* sht21_modify_user_register() - modify user register +* @dev: device +* @mask: mask of bits to modify +* @value: bits to write into user register +* +* Returns <0 on error. +*/ +static s32 sht21_modify_user_register (struct device *dev,u8 mask,u8 value) +{ +struct i2c_client *client =to_i2c_client (dev); +struct sht21 *sht21 =i2c_get_clientdata (client); +s32 result; + +mutex_lock (&sht21->lock); + result =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +if (result >=0 ) + result =i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , + (result & ~mask) | (value & mask)); +mutex_unlock (&sht21->lock); + +return result; +} + +/** +* sht21_show_temperature_resolution() - show temperature measurement resolution +* bits in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where user register value is written to +* +* Will be called on read access to temp1_resolution_bits sysfs attribute. +* Writes "11", "12", "13" or "14". +* Returns number of bytes written into buffer, -EIO on error. +*/ +static ssize_t sht21_show_temperature_resolution (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +s32 userreg =sht21_read_user_register (dev); +if (userreg >=0 ) { +int resolution; +switch (userreg &SHT21_RES_MASK ) { +case SHT21_RES_8_12BIT : + resolution =12 ; +break ; +case SHT21_RES_10_13BIT : + resolution =13 ;
drivers/hwmon/sht21.c line 237
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+break ; +case SHT21_RES_11_11BIT : + resolution =11 ; +break ; +case SHT21_RES_12_14BIT : +default : + resolution =14 ; + } +return sprintf (buf,"%d\n" , resolution); + }
Please return the actual error not this generic one (e.g. return userreg)
Quoted 34 lines
drivers/hwmon/sht21.c new file +34
-0+return -EIO ; +} + +/** +* sht21_store_temperature_resolution() - set temperature measurement resolution +* bits from sysfs value +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer containing value "11", "12", "13" or "14" +* @count: number of characters in @buf +* +* Will be called on write access to temp1_resolution_bits sysfs attribute. +* Returns @count, -EIO on error. +*/ +static ssize_t sht21_store_temperature_resolution (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +s32 result; +u8 res_bits; +unsigned long value; +if (strict_strtoul (buf,10 , &value)) +return -EINVAL ; + +switch (value) { +case 11 : + res_bits =SHT21_RES_11_11BIT ; +break ; +case 12 : + res_bits =SHT21_RES_8_12BIT ; +break ; +case 13 : + res_bits =SHT21_RES_10_13BIT ; +break ;
drivers/hwmon/sht21.c line 281
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+case 14 : + res_bits =SHT21_RES_12_14BIT ; +break ; +default : +return -EINVAL ; + } + + result =sht21_modify_user_register (dev,SHT21_RES_MASK , res_bits); + +return result >=0 ? count : -EIO ;
Again, return the actual error rather than masking the extra information it may have.
Quoted 27 lines
drivers/hwmon/sht21.c new file +27
-0+} + +/** +* sht21_show_humidity_resolution() - show humidity measurement resolution +* bits in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where user register value is written to +* +* Will be called on read access to humidity1_resolution_bits sysfs attribute. +* Writes "8", "10", "11" or "12". +* Returns number of bytes written into buffer, -EIO on error. +*/ +static ssize_t sht21_show_humidity_resolution (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +s32 userreg =sht21_read_user_register (dev); +if (userreg >=0 ) { +int resolution; +switch (userreg &SHT21_RES_MASK ) { +case SHT21_RES_8_12BIT : + resolution =8 ; +break ; +case SHT21_RES_10_13BIT : + resolution =10 ; +break ;
drivers/hwmon/sht21.c line 318
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+case SHT21_RES_11_11BIT : + resolution =11 ; +break ; +case SHT21_RES_12_14BIT : +default : + resolution =12 ; + } +return sprintf (buf,"%d\n" , resolution); + } +return -EIO ;
ditto
Quoted 33 lines
drivers/hwmon/sht21.c new file +33
-0+} + +/** +* sht21_store_humidity_resolution() - set humidity measurement resolution +* bits from sysfs value +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer containing value "8", "10", "11" or "12" +* @count: number of characters in @buf +* +* Will be called on write access to humidity1_resolution_bits sysfs attribute. +* Returns @count, -EIO on error. +*/ +static ssize_t sht21_store_humidity_resolution (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +s32 result; +u8 res_bits; +unsigned long value; +if (strict_strtoul (buf,10 , &value)) +return -EINVAL ; + +switch (value) { +case 8 : + res_bits =SHT21_RES_8_12BIT ; +break ; +case 10 : + res_bits =SHT21_RES_10_13BIT ; +break ; +case 11 : + res_bits =SHT21_RES_11_11BIT ; +break ;
drivers/hwmon/sht21.c line 361
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+case 12 : + res_bits =SHT21_RES_12_14BIT ; +break ; +default : +return -EINVAL ; + } + + result =sht21_modify_user_register (dev,SHT21_RES_MASK , res_bits); + +return result >=0 ? count : -EIO ;
ditto
Quoted 11 lines
drivers/hwmon/sht21.c new file +11
-0+} + +/** +* sht21_show_end_of_battery() - show end of battery state in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where user register value is written to +* +* Will be called on read access to in0_min_alarm sysfs attribute. +* Writes "1" if VDD < 2.25V, else "0". +* Returns number of bytes written into buffer, -EIO on error.
drivers/hwmon/sht21.c line 382
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+*/ +static ssize_t sht21_show_end_of_battery (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +s32 userreg =sht21_read_user_register (dev); +if (userreg >=0 ) +return sprintf (buf,"%u\n" , !!(userreg &SHT21_END_OF_BATTERY )); +else +return -EIO ;
ditto
Quoted 224 lines
drivers/hwmon/sht21.c new file +223
-0+} + +/** +* sht21_show_heater_enable() - show on-chip heater state in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where user register value is written to +* +* Will be called on read access to heater_enable sysfs attribute. +* Writes "1" if heater is enabled, else "0". +* Returns number of bytes written into buffer, -EIO on error. +*/ +static ssize_t sht21_show_heater_enable (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +s32 userreg =sht21_read_user_register (dev); +if (userreg >=0 ) +return sprintf (buf,"%u\n" , !!(userreg &SHT21_ENABLE_HEATER )); +else +return -EIO ; +} + +/** +* sht21_store_heater_enable() - enable or disable heater from sysfs value +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer containing value "0" or "1" +* @count: number of characters in @buf +* +* Will be called on write access to heater_enable sysfs attribute. +* Returns @count, -EIO on error. +*/ +static ssize_t sht21_store_heater_enable (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +s32 result; +unsigned long value; +if (strict_strtoul (buf,10 , &value) || value >1 ) +return -EINVAL ; + + result =sht21_modify_user_register (dev,SHT21_ENABLE_HEATER , + value ?SHT21_ENABLE_HEATER :0 ); + +return result >=0 ? count : -EIO ; +} + + +/* sysfs attributes */ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, sht21_show_temperature, + NULL, 0); +static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO, sht21_show_humidity, + NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_resolution_bits, S_IRUGO | S_IWUSR, + sht21_show_temperature_resolution, sht21_store_temperature_resolution, + 0); +static SENSOR_DEVICE_ATTR(humidity1_resolution_bits, S_IRUGO | S_IWUSR, + sht21_show_humidity_resolution, sht21_store_humidity_resolution, 0); +static SENSOR_DEVICE_ATTR(in0_min_alarm, S_IRUGO, sht21_show_end_of_battery, + NULL, 0); +static SENSOR_DEVICE_ATTR(heater_enable, S_IRUGO | S_IWUSR, + sht21_show_heater_enable, sht21_store_heater_enable, 0); + +static struct attribute *sht21_attributes[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_humidity1_input.dev_attr.attr, + &sensor_dev_attr_temp1_resolution_bits.dev_attr.attr, + &sensor_dev_attr_humidity1_resolution_bits.dev_attr.attr, + &sensor_dev_attr_in0_min_alarm.dev_attr.attr, + &sensor_dev_attr_heater_enable.dev_attr.attr, +NULL +}; + +static const struct attribute_group sht21_attr_group = { + .attrs = sht21_attributes, +}; + +/** +* sht21_probe() - probe device +* @client: I2C client device +* @id: device ID +* +* Called by the I2C core when an entry in the ID table matches a +* device's name. +* Returns 0 on success. +*/ +static int __devinit sht21_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct sht21 *sht21; +int status; +u8 userreg_orig, userreg_changed; + +if (!i2c_check_functionality (client->adapter, +I2C_FUNC_SMBUS_WORD_DATA )) { +dev_err (&client->dev, +"adapter does not support SMBus word transactions\n" ); +return -ENODEV ; + } + + sht21 =kzalloc (sizeof (*sht21),GFP_KERNEL ); +if (!sht21) { +dev_dbg (&client->dev,"kzalloc failed\n" ); +return -ENOMEM ; + } +i2c_set_clientdata (client, sht21); + + status =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +if (status <0 ) { +dev_err (&client->dev,"error reading user register\n" ); + goto fail_free; + } + userreg_orig = status; + +/* The user register resolution bits are toggled and read back to make +* sure that we are really accessing an SHT21 device. */ + userreg_changed = userreg_orig ^SHT21_RES_MASK ; + status =i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , + userreg_changed); +if (status <0 ) { +dev_err (&client->dev,"error writing user register\n" ); + goto fail_restore_config; + } + status =i2c_smbus_read_byte_data (client,SHT21_USER_REG_R ); +if (status <0 ) { +dev_err (&client->dev,"error reading user register\n" ); + goto fail_restore_config; + } +if (status != userreg_changed) { +dev_err (&client->dev,"user register settings did not stick\n" ); + status = -ENODEV ; + goto fail_restore_config; + } + status =i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , + userreg_orig); +if (status <0 ) { +dev_err (&client->dev,"error restoring user register\n" ); + goto fail_restore_config; + } +mutex_init (&sht21->lock); + + status =sysfs_create_group (&client->dev.kobj, &sht21_attr_group); +if (status) { +dev_dbg (&client->dev,"could not create sysfs files\n" ); + goto fail_restore_config; + } + sht21->hwmon_dev =hwmon_device_register (&client->dev); +if (IS_ERR (sht21->hwmon_dev)) { +dev_dbg (&client->dev,"unable to register hwmon device\n" ); + status =PTR_ERR (sht21->hwmon_dev); + goto fail_remove_sysfs; + } + +dev_info (&client->dev,"initialized\n" ); + +return 0 ; + +fail_remove_sysfs: +sysfs_remove_group (&client->dev.kobj, &sht21_attr_group); +fail_restore_config: +i2c_smbus_write_byte_data (client,SHT21_USER_REG_W , userreg_orig); +fail_free: +kfree (sht21); + +return status; +} + +/** +* sht21_remove() - remove device +* @client: I2C client device +*/ +static int __devexit sht21_remove(struct i2c_client *client) +{ +struct sht21 *sht21 =i2c_get_clientdata (client); + +hwmon_device_unregister (sht21->hwmon_dev); +sysfs_remove_group (&client->dev.kobj, &sht21_attr_group); +kfree (sht21); + +return 0 ; +} + +/* Device ID table */ +static const struct i2c_device_id sht21_id[] = { + {"sht21" ,0 }, + { } +}; +MODULE_DEVICE_TABLE (i2c, sht21_id); + +static struct i2c_driver sht21_driver = { + .driver.name ="sht21" , + .probe = sht21_probe, + .remove =__devexit_p (sht21_remove), + .id_table = sht21_id, +}; + +/** +* sht21_init() - initialize driver +* +* Called when kernel is booted or module is inserted. +* Returns 0 on success. +*/ +static int __init sht21_init (void ) +{ +return i2c_add_driver (&sht21_driver); +} +module_init (sht21_init); + +/** +* sht21_init() - clean up driver +* +* Called when module is removed. +*/ +static void __exit sht21_exit(void) +{ +i2c_del_driver (&sht21_driver); +} +module_exit (sht21_exit); + +MODULE_AUTHOR ("Urs Fleisch <[email protected]>" ); +MODULE_DESCRIPTION ("Sensirion SHT21 humidity and temperature sensor driver" ); +MODULE_LICENSE ("GPL" );
PATCH v3 1/1 hwmon: driver for Sensirion SHT21 humidity and temperature sensor
Hi Jonathan,
Quoted 3 lines
Is there actualy a use case that means these can't both be set by platform data? Also, if these are acceptable, should they have some means of indicating which combination of values are available?
I can imagine a use case where an embedded device wants to operate in a low-power mode while still monitoring humidity and/or temperature. Measurement time and thus power consumption of the SHT21 are significantly smaller when using a low resolution. Or you can use a low resolution while waiting for a significant change in humidity and/or temperature and then switch to a higher resolution to actually report the measurement values.
Quoted 2 lines
Rely on platform data to get this right rather than an adhoc test that might well pass on some random devices.
I removed that probing stuff. As read and write addresses for the user register are different, it would probably not pass on another device, but it could disturb another device.
Quoted 3 lines
Sorry to say I missed some error handling issues the first time around. Please always provide userspace with the most detailed and correct error possible. Normally this is the one comming up from the bus subsystem.
All error codes should now be passed up to userspace.
Thanks again,
Urs
---
drivers/hwmon/Kconfig +10 -0
@@ -688,6 +688,16 @@ config SENSORS_SHT15 This driver can also be built as a module. If so, the module will be called sht15. +config SENSORS_SHT21 + tristate "Sensiron humidity and temperature sensors. SHT21 and compat." + depends on I2C + help + If you say yes here you get support for the Sensiron SHT21, SHT25 + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module + will be called sht21. + config SENSORS_S3C tristate "S3C24XX/S3C64XX Inbuilt ADC" depends on ARCH_S3C2410
drivers/hwmon/Makefile +1 -0
@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o +obj-$(CONFIG_SENSORS_SHT21) += sht21.o obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o obj-$(CONFIG_SENSORS_SMM665) += smm665.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
drivers/hwmon/sht21.c new file +591 -0
591 lines not on this page · raw message
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 24 lines · Urs Fleisch
Hi Jonathan,
Quoted 3 lines
Is there actualy a use case that means these can't both be set by platform data? Also, if these are acceptable, should they have some means of indicating which combination of values are available?
I can imagine a use case where an embedded device wants to operate in a low-power mode while still monitoring humidity and/or temperature. Measurement time and thus power consumption of the SHT21 are significantly smaller when using a low resolution. Or you can use a low resolution while waiting for a significant change in humidity and/or temperature and then switch to a higher resolution to actually report the measurement values.
Quoted 2 lines
Rely on platform data to get this right rather than an adhoc test that might well pass on some random devices.
I removed that probing stuff. As read and write addresses for the user register are different, it would probably not pass on another device, but it could disturb another device.
Quoted 3 lines
Sorry to say I missed some error handling issues the first time around. Please always provide userspace with the most detailed and correct error possible. Normally this is the one comming up from the bus subsystem.
Quoted 6 lines
All error codes should now be passed up to userspace.
Thanks again,
UrsSigned-off-by Urs Fleisch <[email protected]>
All the above shows up in the commit log if the patch is applied. Just adds additional work for the maintainers.
Quoted 1 line
---
[ ...]
Quoted 12 lines
drivers/hwmon/sht21.c new file +12
-0+ +/** +* sht21_show_temperature() - show temperature measurement value in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to +* +* Will be called on read access to temp1_input sysfs attribute. +* Returns number of bytes written into buffer, negative errno on error. +*/ +static ssize_t sht21_show_temperature(struct device *dev, + struct device_attribute *attr,
drivers/hwmon/sht21.c line 152
Quoted 10 lines
drivers/hwmon/sht21.c new file +10
-0+ char *buf) +{ +struct i2c_client *client =to_i2c_client (dev); +int result =sht21_update_measurements (client); +if (result >=0 ) { +struct sht21 *sht21 =i2c_get_clientdata (client); + +return sprintf (buf,"%d\n" , sht21->temperature); + } +return result;
Highly unusual way of detecting and returning errors. Common would be
if (result < 0) return result; /* other code */
ie keep the code flow intact. Makes the code much easier to read.
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 23 lines · Jonathan Cameron
Quoted 1 line · Urs Fleisch
Hi Jonathan,
When posting an updated driver, use the form
[PATCH V2] hwmon: driver...That way it will be obvious to people that it isn't just a repost of the previous code.
Quoted 16 lines
Thanks for reviewing the patch. I have fixed the issues you reported and the style problems.
Quoted 11 lines
Sorry, but this isn't going to be acceptable. Classic case of magic numbers. This needs to be broken up into several different attributes.
We have:
* control over the measurement resolution (which is somewhat fiddly on this device)
* Battery voltage threshold > 2.25V
* Enable on chip heaterSo this one is the only one I have problem with. Other two attributes are standard (well humidity is pretty unusual but no one ever complained about the sht15 afaik!)
Might make sense to document it in the ABI, though.
Quoted 8 lines
Quoted 2 lines
The
user_registerattribute is now replaced bytemp1_resolution_bits,humidity1_resolution_bits,Those two aren't in the documentation for hwmon. Guenter/Jean, how should this be done?
Is there actualy a use case that means these can't both be set by platform data? Also, if these are acceptable, should they have some means of indicating which combination of values are available?
Quoted 6 lines
Quoted 4 lines
in0_min_alarm,heater_enable
attributes.in0_min_alarmis used for the "end of battery" state, the other attributes are non-standard. Unfortunately, the temperature and humidity resolutions are not independent. Is this acceptable?Up to maintainers. Other than trying to work out some way of indicating valid options for the two resolution attributes I'd be happy with these.
I am opposed to non-ABI attributes. That doesn't mean that I am opposed to extending the ABI; if the ABI needs to be extended, it should be extended instead of providing non-standard attributes. But there should be a use case for extending the ABI - meaning new ABI attributes should provide value not just for one chip, but for others as well.
sysfs attributes should only be used for values expected to change during runtime. "resolution" and "heater_enable" sounds like it might fit better into platform data. System wide information can also be set using driver parameters.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
+
userreg_changed= status ^SHT21_RES_MASK;This needs a comment. Why exclusive or with a mask?
Quoted 2 lines
drivers/hwmon/sht21.c new file +2
-0+if (status != userreg_changed) { +dev_err (&client->dev,"user register settings did not stick\n" );
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
This does seem a somewhat paranoid check. Is it really needed in a production driver? If this has been seen in the wild, then fair enough!
The resolution bits of the user register are toggled and read back to make sure that this is really an SHT21 device.\
Rely on platform data to get this right rather than an adhoc test that might well pass on some random devices.
Yes.
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 23 lines · Jonathan Cameron
Quoted 1 line · Urs Fleisch
Hi Jonathan,
When posting an updated driver, use the form
[PATCH V2] hwmon: driver...That way it will be obvious to people that it isn't just a repost of the previous code.
Quoted 16 lines
Thanks for reviewing the patch. I have fixed the issues you reported and the style problems.
Quoted 11 lines
Sorry, but this isn't going to be acceptable. Classic case of magic numbers. This needs to be broken up into several different attributes.
We have:
* control over the measurement resolution (which is somewhat fiddly on this device)
* Battery voltage threshold > 2.25V
* Enable on chip heaterSo this one is the only one I have problem with. Other two attributes are standard (well humidity is pretty unusual but no one ever complained about the sht15 afaik!)
I have been thinking about this, and would like to get Jean's input.
Humidity doesn't really sound like "hardware monitoring"; more like environmental monitoring. But on the other side, even though humidity doesn't really monitor the HW, it does monitor operational conditions, and its reported values may have impact on system operation (eg cause a shutdown if humidity is too high, to prevent HW damage).
So question is if hwmon should include explicit environmental monitoring attributes or not, and if hwmon and environmental monitoring can and should be separated to start with (for example, what if any is the difference between environment temperature and hw monitoring temperature ?).
Thoughts, anyone ?
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 24 lines · Guenter Roeck
Quoted 23 lines · Jonathan Cameron
Quoted 1 line · Urs Fleisch
Hi Jonathan,
When posting an updated driver, use the form
[PATCH V2] hwmon: driver...That way it will be obvious to people that it isn't just a repost of the previous code.
Quoted 16 lines
Thanks for reviewing the patch. I have fixed the issues you reported and the style problems.
Quoted 11 lines
Sorry, but this isn't going to be acceptable. Classic case of magic numbers. This needs to be broken up into several different attributes.
We have:
* control over the measurement resolution (which is somewhat fiddly on this device)
* Battery voltage threshold > 2.25V
* Enable on chip heaterSo this one is the only one I have problem with. Other two attributes are standard (well humidity is pretty unusual but no one ever complained about the sht15 afaik!)
Quoted 7 lines
I have been thinking about this, and would like to get Jean's input.
Humidity doesn't really sound like "hardware monitoring"; more like environmental monitoring. But on the other side, even though humidity doesn't really monitor the HW, it does monitor operational conditions, and its reported values may have impact on system operation (eg cause a shutdown if humidity is too high, to prevent HW damage).
That was my original argument when I proposed the sht15 driver. No idea if anyone actually does this though!
Quoted 8 lines
So question is if hwmon should include explicit environmental monitoring attributes or not, and if hwmon and environmental monitoring can and should be separated to start with (for example, what if any is the difference between environment temperature and hw monitoring temperature ?).
Thoughts, anyone ?
I wondered exactly this when we added the sht15 driver. Never really came to a firm conclusion though. If you and Jean decide these shouldn't be in hwmon, I'm happy to take them both in IIO. Given our scope is much broader and we already have things like light sensors (also sometimes environment monitoring devices), that would work for me. There are a few sht15 users out there I know of beyond the imote2 sensor boards (which is how I encountered them). The imote2 users should be fine as I maintain the platform and most of the tools anyway, the others maybe have more issues with a move. Uses I know of are indeed more general environment monitoring.
Not seen one of these in conventional hardware monitoring but could be wrong. I guess, Urs will have a better idea of where these tend to be used?
Obviously there is an argument for an 'environment' monitoring subsystem, but my guess in Linus won't pull based on exactly the same issue he had with ALS when we proposed that. Linus won't take small subsystems for sensors given he wants them all in the same one. Actually he mostly wanted them to be in input but Dmitry quite rightly pushed back hard against that for exactly the reason you are wondering whether they ought to be in hwmon, avoidance of scope spread.
Quoted 5 lines
Guenter
Quoted 54 lines · Jonathan Cameron
Quoted 24 lines · Guenter Roeck
Quoted 23 lines · Jonathan Cameron
Quoted 1 line · Urs Fleisch
Hi Jonathan,
When posting an updated driver, use the form
[PATCH V2] hwmon: driver...That way it will be obvious to people that it isn't just a repost of the previous code.
Quoted 16 lines
Thanks for reviewing the patch. I have fixed the issues you reported and the style problems.
Quoted 11 lines
Sorry, but this isn't going to be acceptable. Classic case of magic numbers. This needs to be broken up into several different attributes.
We have:
* control over the measurement resolution (which is somewhat fiddly on this device)
* Battery voltage threshold > 2.25V
* Enable on chip heaterSo this one is the only one I have problem with. Other two attributes are standard (well humidity is pretty unusual but no one ever complained about the sht15 afaik!)
Quoted 7 lines
I have been thinking about this, and would like to get Jean's input.
Humidity doesn't really sound like "hardware monitoring"; more like environmental monitoring. But on the other side, even though humidity doesn't really monitor the HW, it does monitor operational conditions, and its reported values may have impact on system operation (eg cause a shutdown if humidity is too high, to prevent HW damage).
That was my original argument when I proposed the sht15 driver. No idea if anyone actually does this though!
Quoted 8 lines
So question is if hwmon should include explicit environmental monitoring attributes or not, and if hwmon and environmental monitoring can and should be separated to start with (for example, what if any is the difference between environment temperature and hw monitoring temperature ?).
Thoughts, anyone ?
I wondered exactly this when we added the sht15 driver. Never really came to a firm conclusion though. If you and Jean decide these shouldn't be in hwmon, I'm happy to take them both in IIO. Given our scope is much broader and we already have things like light sensors (also sometimes environment monitoring devices), that would work for me. There are a few sht15 users out there I know of beyond the imote2 sensor boards (which is how I encountered them). The imote2 users should be fine as I maintain the platform and most of the tools anyway, the others maybe have more issues with a move. Uses I know of are indeed more general environment monitoring.
Not seen one of these in conventional hardware monitoring but could be wrong. I guess, Urs will have a better idea of where these tend to be used?
Quoted 7 lines
Obviously there is an argument for an 'environment' monitoring subsystem, but my guess in Linus won't pull based on exactly the same issue he had with ALS when we proposed that. Linus won't take small subsystems for sensors given he wants them all in the same one. Actually he mostly wanted them to be in input but Dmitry quite rightly pushed back hard against that for exactly the reason you are wondering whether they ought to be in hwmon, avoidance of scope spread.
If I take off my hwmon hat for a minute, I would argue that hwmon is a bad term to start with, and that it should have been envmon to start with. That is how it was handled at companies I worked for previously, and how it makes sense to me. Reason is that the term tends to define the scope of a subsystem - if hwmon was named envmon, we would not even have this discussion.
Also, it would be desirable to have a linux kernel subsystem to map the scope of RFC 3433, the Entity Sensor MIB. That MIB does include humidity sensors. The hwmon subsystem seems to be the best fit for it.
So, in one sentence, for me it makes a lot of sense to explicitly include environmental monitoring in the scope of the hwmon subsystem.
Now, that is my personal opinion. I'd definitely like to get input from Jean on this.
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 3 lines
Uses I know of are indeed more general environment monitoring. Not seen one of these in conventional hardware monitoring but could be wrong. I guess, Urs will have a better idea of where these tend to be used?
Mobile phones could be a promising market for humidity sensors in a Linux system. Fujitsu already sells a mobile phone with built-in temperature and humidity sensors. So we hope to see also Linux-based mobile phones (e.g. Android, MeGoo) with a humidity sensor. Such a sensor could be used to monitor environmental conditions, but there could be other applications too, some of them may not make sense and others can not be foreseen. The humidity sensor could also be used to monitor the hardware of the phone, in this situation, hwmon makes sense. If a sensor can be used for various application, there is probably not one true category. There may be other sensors in hwmon, which can be used for applications other than hardware monitoring. I chose the hwmon directory because the sht15 was already there.
Quoted 1 line
Highly unusual way of detecting and returning errors.
If this is a problem, I can alter the code. I could also drop all user-register related attributes if they cause problems with the ABI.
Regards,
Urs
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 10 lines · Urs Fleisch
Quoted 3 lines
Uses I know of are indeed more general environment monitoring. Not seen one of these in conventional hardware monitoring but could be wrong. I guess, Urs will have a better idea of where these tend to be used?
Mobile phones could be a promising market for humidity sensors in a Linux system. Fujitsu already sells a mobile phone with built-in temperature and humidity sensors. So we hope to see also Linux-based mobile phones (e.g. Android, MeGoo) with a humidity sensor. Such a sensor could be used to monitor environmental conditions, but there could be other applications too, some of them may not make sense and others can not be foreseen. The humidity sensor could also be used to monitor the hardware of the phone, in this situation, hwmon makes sense. If a sensor can be used for various application, there is probably not one true category. There may be other sensors in hwmon, which can be used for applications other than hardware monitoring. I chose the hwmon directory because the sht15 was already there.
Quoted 1 line
Highly unusual way of detecting and returning errors.
If this is a problem, I can alter the code. I could also drop all user-register related attributes if they cause problems with the ABI.
Please.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
PATCH v4 1/1 hwmon: driver for Sensirion SHT21 humidity and temperature sensor
Thanks for the review, Jonathan and Guenter.
Changes:
- removed user register related sysfs attributes, keeping only temp1_input and humidity1_input
- cleaned up code flow when returning errors for better readability
Regards,
Urs
---
drivers/hwmon/Kconfig +10 -0
@@ -688,6 +688,16 @@ config SENSORS_SHT15 This driver can also be built as a module. If so, the module will be called sht15. +config SENSORS_SHT21 + tristate "Sensiron humidity and temperature sensors. SHT21 and compat." + depends on I2C + help + If you say yes here you get support for the Sensiron SHT21, SHT25 + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module + will be called sht21. + config SENSORS_S3C tristate "S3C24XX/S3C64XX Inbuilt ADC" depends on ARCH_S3C2410
drivers/hwmon/Makefile +1 -0
@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o +obj-$(CONFIG_SENSORS_SHT21) += sht21.o obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o obj-$(CONFIG_SENSORS_SMM665) += smm665.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
drivers/hwmon/sht21.c new file +300 -0
300 lines not on this page · raw message
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 6 lines · Urs Fleisch
Thanks for the review, Jonathan and Guenter.
Changes:
- removed user register related sysfs attributes, keeping onlytemp1_inputandhumidity1_input
- cleaned up code flow when returning errors for better readability
Short and clean. I do wonder if you want some of those users register bits available via platform data though. Perhaps that can be added in a later patch as and when someone needs them.
Quoted 5 lines
Regards,
UrsSigned-off-by Urs Fleisch <[email protected]>
Quoted 341 lines
---
drivers/hwmon/Kconfig +10
-0@@ -688,6 +688,16 @@ config SENSORS_SHT15 This driver can also be built as a module. If so, the module will be called sht15. +config SENSORS_SHT21 + tristate "Sensiron humidity and temperature sensors. SHT21 and compat." + depends on I2C + help + If you say yes here you get support for the Sensiron SHT21, SHT25 + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module + will be called sht21. + config SENSORS_S3C tristate "S3C24XX/S3C64XX Inbuilt ADC" depends on ARCH_S3C2410drivers/hwmon/Makefile +1
-0@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o +obj-$(CONFIG_SENSORS_SHT21) += sht21.o obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o obj-$(CONFIG_SENSORS_SMM665) += smm665.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.odrivers/hwmon/sht21.c new file +300
-0@@ -0,0 +1,300 @@ +/* Sensirion SHT21 humidity and temperature sensor driver +* +* Copyright (C) 2010 Urs Fleisch <[email protected]> +* +* 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 (5/2010) at +* http://www.sensirion.com/en/pdf/product_information/Datasheet-humidity-sensor-SHT21.pdf +*/ + +#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/mutex.h> +#include <linux/device.h> + +/* I2C command bytes */ +#define SHT21_TRIG_T_MEASUREMENT_HM 0xe3 +#define SHT21_TRIG_RH_MEASUREMENT_HM 0xe5 + +/** +* struct sht21 - SHT21 device specific data +* @hwmon_dev: device registered with hwmon +* @lock: mutex to protect measurement values +* @valid: only 0 before first measurement is taken +* @last_update: time of last update (jiffies) +* @temperature: cached temperature measurement value +* @humidity: cached humidity measurement value +*/ +struct sht21 { +struct device *hwmon_dev; +struct mutex lock; +char valid; +unsigned long last_update; +int temperature; +int humidity; +}; + +/** +* sht21_temp_ticks_to_millicelsius() - convert raw temperature ticks to +* milli celsius +* @ticks: temperature ticks value received from sensor +*/ +static inline int sht21_temp_ticks_to_millicelsius (int ticks) +{ + ticks &= ~0x0003 ;/* clear status bits */ +/* Formula T = -46.85 + 175.72 * ST / 2^16 from data sheet 6.2, +optimized for integer fixed point (3 digits) arithmetic */ +return ((21965 * ticks) >>13 ) -46850 ; +} + +/** +* sht21_rh_ticks_to_per_cent_mille() - convert raw humidity ticks to +* one-thousandths of a percent relative humidity +* @ticks: humidity ticks value received from sensor +*/ +static inline int sht21_rh_ticks_to_per_cent_mille (int ticks) +{ + ticks &= ~0x0003 ;/* clear status bits */ +/* Formula RH = -6 + 125 * SRH / 2^16 from data sheet 6.1, +optimized for integer fixed point (3 digits) arithmetic */ +return ((15625 * ticks) >>13 ) -6000 ; +} + +/** +* sht21_read_word_data() - read word from register +* @client: I2C client device +* @reg: I2C command byte +* +* Returns value, negative errno on error. +*/ +static inline int sht21_read_word_data (struct i2c_client *client,u8 reg) +{ +int ret =i2c_smbus_read_word_data (client, reg); +if (ret <0 ) +return ret; +/* SMBus specifies low byte first, but the SHT21 returns MSB +* first, so we have to swab16 the values */ +return swab16 (ret); +} + +/** +* sht21_update_measurements() - get updated measurements from device +* @client: I2C client device +* +* Returns 0 on success, else negative errno. +*/ +static int sht21_update_measurements (struct i2c_client *client) +{ +int ret =0 ; +struct sht21 *sht21 =i2c_get_clientdata (client); + +mutex_lock (&sht21->lock); +/* Data sheet 2.4: +* SHT2x should not be active for more than 10% of the time - e.g. +* maximum two measurements per second at 12bit accuracy shall be made. +*/ +if (time_after (jiffies, sht21->last_update +HZ /2 ) || !sht21->valid) { + ret =sht21_read_word_data (client,SHT21_TRIG_T_MEASUREMENT_HM ); +if (ret <0 ) + goto out; + sht21->temperature =sht21_temp_ticks_to_millicelsius (ret); + ret =sht21_read_word_data (client, +SHT21_TRIG_RH_MEASUREMENT_HM ); +if (ret <0 ) + goto out; + sht21->humidity =sht21_rh_ticks_to_per_cent_mille (ret); + sht21->last_update = jiffies; + sht21->valid =1 ; + } +out: +mutex_unlock (&sht21->lock); + +return ret >=0 ?0 : ret; +} + +/** +* sht21_show_temperature() - show temperature measurement value in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to +* +* Will be called on read access to temp1_input sysfs attribute. +* Returns number of bytes written into buffer, negative errno on error. +*/ +static ssize_t sht21_show_temperature (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct i2c_client *client =to_i2c_client (dev); +struct sht21 *sht21 =i2c_get_clientdata (client); +int ret =sht21_update_measurements (client); +if (ret <0 ) +return ret; +return sprintf (buf,"%d\n" , sht21->temperature); +} + +/** +* sht21_show_humidity() - show humidity measurement value in sysfs +* @dev: device +* @attr: device attribute +* @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to +* +* Will be called on read access to humidity1_input sysfs attribute. +* Returns number of bytes written into buffer, negative errno on error. +*/ +static ssize_t sht21_show_humidity (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct i2c_client *client =to_i2c_client (dev); +struct sht21 *sht21 =i2c_get_clientdata (client); +int ret =sht21_update_measurements (client); +if (ret <0 ) +return ret; +return sprintf (buf,"%d\n" , sht21->humidity); +} + +/* sysfs attributes */ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, sht21_show_temperature, + NULL, 0); +static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO, sht21_show_humidity, + NULL, 0); + +static struct attribute *sht21_attributes[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_humidity1_input.dev_attr.attr, +NULL +}; + +static const struct attribute_group sht21_attr_group = { + .attrs = sht21_attributes, +}; + +/** +* sht21_probe() - probe device +* @client: I2C client device +* @id: device ID +* +* Called by the I2C core when an entry in the ID table matches a +* device's name. +* Returns 0 on success. +*/ +static int __devinit sht21_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct sht21 *sht21; +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" ); +return -ENODEV ; + } + + sht21 =kzalloc (sizeof (*sht21),GFP_KERNEL ); +if (!sht21) { +dev_dbg (&client->dev,"kzalloc failed\n" ); +return -ENOMEM ; + } +i2c_set_clientdata (client, sht21); + +mutex_init (&sht21->lock); + + err =sysfs_create_group (&client->dev.kobj, &sht21_attr_group); +if (err) { +dev_dbg (&client->dev,"could not create sysfs files\n" ); + goto fail_free; + } + sht21->hwmon_dev =hwmon_device_register (&client->dev); +if (IS_ERR (sht21->hwmon_dev)) { +dev_dbg (&client->dev,"unable to register hwmon device\n" ); + err =PTR_ERR (sht21->hwmon_dev); + goto fail_remove_sysfs; + } + +dev_info (&client->dev,"initialized\n" ); + +return 0 ; + +fail_remove_sysfs: +sysfs_remove_group (&client->dev.kobj, &sht21_attr_group); +fail_free: +kfree (sht21); + +return err; +} + +/** +* sht21_remove() - remove device +* @client: I2C client device +*/ +static int __devexit sht21_remove(struct i2c_client *client) +{ +struct sht21 *sht21 =i2c_get_clientdata (client); + +hwmon_device_unregister (sht21->hwmon_dev); +sysfs_remove_group (&client->dev.kobj, &sht21_attr_group); +kfree (sht21); + +return 0 ; +} + +/* Device ID table */ +static const struct i2c_device_id sht21_id[] = { + {"sht21" ,0 }, + { } +}; +MODULE_DEVICE_TABLE (i2c, sht21_id); + +static struct i2c_driver sht21_driver = { + .driver.name ="sht21" , + .probe = sht21_probe, + .remove =__devexit_p (sht21_remove), + .id_table = sht21_id, +}; + +/** +* sht21_init() - initialize driver +* +* Called when kernel is booted or module is inserted. +* Returns 0 on success. +*/ +static int __init sht21_init (void ) +{ +return i2c_add_driver (&sht21_driver); +} +module_init (sht21_init); + +/** +* sht21_init() - clean up driver +* +* Called when module is removed. +*/ +static void __exit sht21_exit(void) +{ +i2c_del_driver (&sht21_driver); +} +module_exit (sht21_exit); + +MODULE_AUTHOR ("Urs Fleisch <[email protected]>" ); +MODULE_DESCRIPTION ("Sensirion SHT21 humidity and temperature sensor driver" ); +MODULE_LICENSE ("GPL" );
Quoted 10 lines · Jonathan Cameron
Quoted 6 lines · Urs Fleisch
Thanks for the review, Jonathan and Guenter.
Changes:
- removed user register related sysfs attributes, keeping onlytemp1_inputandhumidity1_input
- cleaned up code flow when returning errors for better readabilityShort and clean. I do wonder if you want some of those users register bits available via platform data though. Perhaps that can be added in a later patch as and when someone needs them.
Only thing missing is Documentation/hwmon/sht21.
Urs, can you add that ?
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 11 lines · Urs Fleisch
Thanks for the review, Jonathan and Guenter.
Changes:
- removed user register related sysfs attributes, keeping onlytemp1_inputandhumidity1_input
- cleaned up code flow when returning errors for better readabilityRegards,
UrsSigned-off-by Urs Fleisch <[email protected]>
Urs,
what tree is your patch based on ? I am having trouble applying it.
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
PATCH v5 1/1 hwmon: driver for Sensirion SHT21 humidity and temperature sensor
Quoted 3 lines
Only thing missing is
Documentation/hwmon/sht21.Urs, can you add that ?
I have added a file Documentation/hwmon/sht21 to the patch.
Quoted 1 line
what tree is your patch based on ? I am having trouble applying it.
I used the 2.6.32 kernel from the rowboat project
(http://code.google.com/p/rowboat/) because my sensor is connected to an OMAP3EVM board. To make the patch applicable for the latest kernel, I added a line to the Makefile patch. I could apply the patch without problems to the linux-next kernel, although patch reported fuzz with Kconfig. The patch below is now based on linux-next, so there should be no longer problems.
Thanks,
Urs
---
Documentation/hwmon/sht21 new file +48 -0
@@ -0,0 +1,49 @@ +Kernel driver sht21 +=========+ +Supported chips: + * Sensirion SHT21 + Prefix: 'sht21' + Addresses scanned: none + Datasheet: Publicly available at the Sensirion website + http://www.sensirion.com/en/pdf/product_information/Datasheet-humidity-sensor-SHT21.pdf + + * Sensirion SHT25 + Prefix: 'sht21' + Addresses scanned: none + Datasheet: Publicly available at the Sensirion website + http://www.sensirion.com/en/pdf/product_information/Datasheet-humidity-sensor-SHT25.pdf + +Author: + Urs Fleisch <[email protected]> + +Description +----------- + +The SHT21 and SHT25 are humidity and temperature sensors in a DFN package of +only 3 x 3 mm footprint and 1.1 mm height. The difference between the two +devices is the higher level of precision of the SHT25 (1.8% relative humidity, +0.2 degree Celsius) compared with the SHT21 (2.0% relative humidity, +0.3 degree Celsius). + +The devices communicate with the I2C protocol. All sensors are set to the same +I2C address 0x40, so an entry with I2C_BOARD_INFO("sht21", 0x40) can be used +in the board setup code. + +sysfs-Interface +--------------- + +temp1_input - temperature input +humidity1_input - humidity input + +Notes +----- + +The driver uses the default resolution settings of 12 bit for humidity and 14 +bit for temperature, which results in typical measurement times of 22 ms for +humidity and 66 ms for temperature. To keep self heating below 0.1 degree +Celsius, the device should not be active for more than 10% of the time, +e.g. maximum two measurements per second at the given resolution. + +Different resolutions, the on-chip heater, using the CRC checksum and reading +the serial number are not supported yet.
drivers/hwmon/Kconfig +10 -0
@@ -744,6 +744,16 @@ config SENSORS_SHT15 This driver can also be built as a module. If so, the module will be called sht15. +config SENSORS_SHT21 + tristate "Sensiron humidity and temperature sensors. SHT21 and compat." + depends on I2C + help + If you say yes here you get support for the Sensiron SHT21, SHT25 + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module + will be called sht21. + config SENSORS_S3C tristate "Samsung built-in ADC" depends on S3C_ADC
drivers/hwmon/Makefile +1 -0
@@ -91,6 +91,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o +obj-$(CONFIG_SENSORS_SHT21) += sht21.o obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o obj-$(CONFIG_SENSORS_SMM665) += smm665.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
drivers/hwmon/sht21.c new file +300 -0
300 lines not on this page · raw message
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Guenter Roeck
AppliedQuoted 13 lines · Urs Fleisch
Quoted 3 lines
Only thing missing is
Documentation/hwmon/sht21.Urs, can you add that ?
I have added a file
Documentation/hwmon/sht21to the patch.Quoted 1 line
what tree is your patch based on ? I am having trouble applying it.
I used the 2.6.32 kernel from the rowboat project
(http://code.google.com/p/rowboat/) because my sensor is connected to an OMAP3EVM board. To make the patch applicable for the latest kernel, I added a line to theMakefilepatch. I could apply the patch without problems to the linux-next kernel, although patch reported fuzz withKconfig. The patch below is now based on linux-next, so there should be no longer problems.
Quoted 4 lines
Thanks,
UrsSigned-off-by Urs Fleisch <[email protected]>
Applied to -next, with Jonathan's Acked-by and multi-line comments changed to adhere to Documentation/CodingStyle.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors