Hi,

This is a driver for the new humidity and temperature sensors SHT21 and SHT25 from Sensirion.

Regards,
Urs


diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 95ccbe3..4a96429 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -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
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 33c2ee1..7439653 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -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
diff --git a/drivers/hwmon/sht21.c b/drivers/hwmon/sht21.c
new file mode 100644
index 0000000..7d3ec82
--- /dev/null
+++ b/drivers/hwmon/sht21.c
@@ -0,0 +1,392 @@
+/* Sensirion SHT21 humidity and temperature sensor driver
+ *
+ * Copyright (C) 2010 Urs Fleisch <urs.fleisch@sensirion.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the ...
[ message continues ]

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,

These are certainly not a natural enum.  A set of '#define's
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
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
Perhaps ammend the comment to say they are only invalid before the first
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)
Don't do this. It just makes the code harder to read. Again, ...
[ message continues ]

Hi Jonathan,

Thanks for reviewing the patch. I have fixed the issues you reported and the

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

The resolution bits of the user register are toggled and read back to make
sure that this is really an SHT21 device.

Thanks,
Urs

Signed-off-by: Urs Fleisch <urs.fleisch@sensirion.com>
---
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 95ccbe3..4a96429 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -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
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 33c2ee1..7439653 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -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
diff --git a/drivers/hwmon/sht21.c b/drivers/hwmon/sht21.c
new file mode 100644
index 0000000..7d3ec82
--- /dev/null
+++ b/drivers/hwmon/sht21.c
@@ -0,0 +1,607 @@
+/* Sensirion SHT21 humidity and temperature sensor driver
+ ...
[ message continues ]

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
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
Up to maintainers.  Other than trying to work out some way of indicating valid
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,

If it is less than 0, we have an error that ought to be correctly
Again, return the actual error rather than masking the extra information it

--

[ message continues ]

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

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

All error codes should now be passed up to userspace.

Thanks again,
Urs

Signed-off-by: Urs Fleisch <urs.fleisch@sensirion.com>
---
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 95ccbe3..4a96429 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -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
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 33c2ee1..7439653 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -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
diff --git a/drivers/hwmon/sht21.c b/drivers/hwmon/sht21.c
new file mode 100644
index 0000000..7d3ec82
--- /dev/null
+++ ...
[ message continues ]

All the above shows up in the commit log if the patch is applied. Just adds

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

[ message continues ]

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

Guenter
--

[ message continues ]

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


--

[ message continues ]

That was my original argument when I proposed the sht15 driver. No idea
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

--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

Please.

Thanks,
Guenter


--

[ message continues ]