hwmon: Driver for ADT7410
Subsystems (2)
PATCH 1/1 hwmon: Driver for ADT7410
Hi,
this patch brings basic support for the Analog Devices ADT7410 temperature sensor, based on the lm75 hwmon driver and the adt7410 iio driver. The following functionality has been implemented:
* get current temperature
* get/set minimum, maximum and critical temperature
* get/set hysteresis
* get alarm events for minimum, maximum and critical temperature
The ADT7410 provides some more configuration options, but would need non-standard sysfs symbols, so I prefer to get some feedback before implementing any:
* get/set resolution (13/16 bits): temp#_resolution (13/16)?
* get/set event mode (comparator/interrupt): temp#_eventmode (0/1)?
* get/set polarity of INT pin (active-high/active-low): temp#_INT (0/1)?
* get/set polarity of CT pin (active-high/active-low): temp#_CT (0/1)?
* get/set sampling mode (continous/one sample per second/one-shot/power-down): temp#_mode (0/1/2/3)?
* get/set fault queue (1 to 4 faults to trigger event): temp#_faultqueue (1/2/3/4)?
* get device id: temp#_id?
Besides, I saw that there are several sensors (including the ADT7410) which have just one hysteresis register, that stores a delta value that applies to min, max and critical temperature. How are the chances to create a standard symbol "temp#_hyst"?
All implemented sysfs-symbols have been sucessfully tested at temperatures of 15°C to 40°C.
---
Documentation/hwmon/adt7410 new file +47 -0
@@ -0,0 +1,47 @@ +Kernel driver adt7410 +===================== + +Supported chips: + * Analog Devices ADT7410 + Prefix: 'adt7410' + Addresses scanned: I2C 0x48 - 0x4B + Datasheet: Publicly available at the Analog Devices website + http://www.analog.com/static/imported-files/data_sheets/ADT7410.pdf + +Author: Hartmut Knaack <knaack.h <at> gmx.de> + +Description +----------- + +The ADT7410 is a temperature sensor with rated temperature range of -55°C to ++150°C. It has a high accuracy of +/-0.5°C and can be operated at a resolution +of 13 bits (0.0625°C) or 16 bits (0.0078°C). The sensor provides an INT pin to +indicate an excession of a set minimum or maximum temperature, as well as a +critical temperature (CT) pin to indicate an excession of a set critical +temperature. Both pins can be set up with a common hysteresis of 0°C - 15°C +and a fault queue ranging from 1 to 4 events. Both pins can individually set +to be active-low or active-high, while the whole device can either run in +comparator mode or interrupt mode. The ADT7410 supports continous temperature +sampling, as well as sampling one temperature value per second or even just +get one sample on demand for power saving. Besides, it can completely power +down its ADC, if power management is required. + +Configuration Notes +------------------- + +Since the device uses one hysteresis value, which is a delta to minimum, +maximum and critical temperature, the driver will represent it as +temp#_max_hyst. +The device is set to 16 bit resolution and comparator mode by default. + +sysfs-Interface +--------------- + +temp#_input - temperature input +temp#_min - temperature minimum setpoint +temp#_max - temperature maximum setpoint +temp#_crit - critical temperature setpoint +temp#_max_hyst - hysteresis for temperature maximum +temp#_min_alarm - temperature minimum alarm flag +temp#_max_alarm - temperature maximum alarm flag +temp#_crit_alarm - critical temperature alarm flag
drivers/hwmon/Kconfig +10 -0
@@ -179,6 +179,16 @@ config SENSORS_ADM9240 This driver can also be built as a module. If so, the module will be called adm9240. +config SENSORS_ADT7410 + tristate "Analog Devices ADT7410" + depends on I2C && EXPERIMENTAL + help + If you say yes here you get support for the Analog Devices + ADT7410 temperature monitoring chip. + + This driver can also be built as a module. If so, the module + will be called adt7410. + config SENSORS_ADT7411 tristate "Analog Devices ADT7411" depends on I2C && EXPERIMENTAL
drivers/hwmon/Makefile +1 -0
@@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_ADM9240) += adm9240.o obj-$(CONFIG_SENSORS_ADS1015) += ads1015.o obj-$(CONFIG_SENSORS_ADS7828) += ads7828.o obj-$(CONFIG_SENSORS_ADS7871) += ads7871.o +obj-$(CONFIG_SENSORS_ADT7410) += adt7410.o obj-$(CONFIG_SENSORS_ADT7411) += adt7411.o obj-$(CONFIG_SENSORS_ADT7462) += adt7462.o obj-$(CONFIG_SENSORS_ADT7470) += adt7470.o
drivers/hwmon/adt7410.c new file +539 -0
539 lines not on this page · raw message
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Guenter Roeck
maintainerQuoted 18 lines · Hartmut Knaack
Hi,
this patch brings basic support for the Analog Devices ADT7410 temperature sensor, based on the lm75 hwmon driver and the adt7410 iio driver. The following functionality has been implemented:* get current temperature
* get/set minimum, maximum and critical temperature
* get/set hysteresis
* get alarm events for minimum, maximum and critical temperatureThe ADT7410 provides some more configuration options, but would need non-standard sysfs symbols, so I prefer to get some feedback before implementing any:
* get/set resolution (13/16 bits): temp#_resolution (13/16)?
* get/set event mode (comparator/interrupt): temp#_eventmode (0/1)?
* get/set polarity of INT pin (active-high/active-low): temp#_INT (0/1)?
* get/set polarity of CT pin (active-high/active-low): temp#_CT (0/1)?
* get/set sampling mode (continous/one sample per second/one-shot/power-down): temp#_mode (0/1/2/3)?
* get/set fault queue (1 to 4 faults to trigger event): temp#_faultqueue (1/2/3/4)?
* get device id: temp#_id?
Quoted 5 lines
Besides, I saw that there are several sensors (including the ADT7410) which have just one hysteresis register, that stores a delta value that applies to min, max and critical temperature. How are the chances to create a standard symbol "temp#_hyst"?
All implemented sysfs-symbols have been sucessfully tested at temperatures of 15°C to 40°C.
Signed-off-by Hartmut Knaack <knaack.h <at> gmx.de>
Hi,
your mailer replaced all tabs with spaces, making a real review quite difficult. Please fix that.
Couple of comments (not a complete review):
- For configuration, we commonly assume it is taken care of by the BIOS. Other that that, if you think anything needs to be configurable, use platform data and/or device tree information. No sysfs attributes, please.
- Hysteresis is an absolute temperature, not a relative one. So the question for a generis attribute would be what it applies to. The current approach does not have that problem.
- Please use devm_ functions to allocate resources.
- The wrappers for adt7410_i2c_write_byte and adt7410_i2c_read_byte are really unnecessary. Please drop. You don't need error messages here, hopefully.
- Use i2c_smbus_read_word_swapped and i2c_smbus_write_word_swapped for the 16 bit functions instead of the local wrapper functions.
- Using "char" for "valid" makes the code more complex for most platforms. Consider bool instead.
- Your code hides error returns from i2c access functions by returning EIO instead of the actual error. Please don't do that.
- The access retry code in adt7410_update_device is really quite complex. Is this really needed ? In general, the sensor should be configured for continuous conversion, and it should not be necessary to wait for a conversion to complete. besides, we can not afford an active 240 ms wait loop, so if the loop should _really_ be needed, you would have to add a sleep function call.
- The detect function is quite weak. you should definitely also check bits 0..3 of the status register (must be 0), and consider checking against known chip revision numbers. Question is also if the chip is expected to show up in a PC type system; if not, it might make sense to drop the detect function entirely, given its weakness.
- Use SENSOR_DEVICE_ATTR_2 to encode secondary index values (instead of multiple show_xxx_alarm functions).
- Don't initialize variables unnecessarily.
- set_mask and clr_mask in the probe function are really not necessary.
- Please align function parameters to the opening (.
- No unnecessary ( ), please [no, it does not make the code easier to read, it just hides the really important ( )s].
- Please run the patch through checkpatch.pl. I can not really say for sure due to the tab problem how many issues there are, but I see at least one checkpatch error.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 4 lines · Guenter Roeck
Hi,
your mailer replaced all tabs with spaces, making a real review quite difficult. Please fix that.
It likes to do that, but I fixed it :-)
Quoted 6 lines
Couple of comments (not a complete review):
- For configuration, we commonly assume it is taken care of by the BIOS. Other that that, if you think anything needs to be configurable, use platform data and/or device tree information. No sysfs attributes, please.
I tend to regard this from the users point of view. My aim is to have it supported in openwrt, where you get your ready-to-flash image and install your custom kernel modules and software. It's fine to have some valid default configuration, maybe set by those ways you mentioned. But I would prefer to have some chance of runtime configuration. BIOS/bootloader is something you don't really want to mess with, platform data requires kernel (module) compilation, from what I know. Device tree might work, though I'm pretty unexperienced with that, yet.
Quoted 3 lines
- Hysteresis is an absolute temperature, not a relative one. So the question for a generis attribute would be what it applies to. The current approach does not have that problem.
From what I see, most sensors store hysteresis as absolute temperature (like lm75). But some store one offset values (like adt7410, adt7475), which have to get converted by the driver to appear like absolute values. In case of the adt7410, the offset applies to min, max and crit, and in my driver it is set with max_hyst (since I think that is the most common use). But if someone would only want to use alarm for min, it would be necessary to check/set max and set max_hyst, which I think is a bit confusing.
Quoted 1 line
- Please use
devm_functions to allocate resources.
fixed
Quoted 2 lines
- The wrappers for
adt7410_i2c_write_byteandadt7410_i2c_read_byteare really unnecessary. Please drop. You don't need error messages here, hopefully.
fixed
Quoted 2 lines
- Use
i2c_smbus_read_word_swappedandi2c_smbus_write_word_swappedfor the 16 bit functions instead of the local wrapper functions.
fixed
Quoted 2 lines
- Using "char" for "valid" makes the code more complex for most platforms. Consider bool instead.
fixed
Quoted 2 lines
- Your code hides error returns from i2c access functions by returning EIO instead of the actual error. Please don't do that.
fixed
Quoted 5 lines
- The access retry code in
adt7410_update_deviceis really quite complex. Is this really needed ? In general, the sensor should be configured for continuous conversion, and it should not be necessary to wait for a conversion to complete. besides, we can not afford an active 240 ms wait loop, so if the loop should _really_ be needed, you would have to add a sleep function call.
Changed to sleep up to 4 times for 100 ms.
Quoted 5 lines
- The detect function is quite weak. you should definitely also check bits 0..3 of the status register (must be 0), and consider checking against known chip revision numbers. Question is also if the chip is expected to show up in a PC type system; if not, it might make sense to drop the detect function entirely, given its weakness.
Added those checks. I personally would not need the detect function, just added it for completeness.
Quoted 2 lines
- Use
SENSOR_DEVICE_ATTR_2to encode secondary index values (instead of multipleshow_xxx_alarmfunctions).
fixed
Quoted 1 line
- Don't initialize variables unnecessarily.
fixed
Quoted 1 line
-
set_maskandclr_maskin the probe function are really not necessary.
fixed
Quoted 1 line
- Please align function parameters to the opening (.
fixed
Quoted 2 lines
- No unnecessary ( ), please [no, it does not make the code easier to read, it just hides the really important ( )s].
hopefully fixed.
Quoted 3 lines
- Please run the patch through checkpatch.pl. I can not really say for sure due to the tab problem how many issues there are, but I see at least one checkpatch error.
It complained about unneccessary parenthesis, before, but I thought it was safer to keep them.
Quoted 3 lines
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Guenter Roeck
maintainerHi Hartmut,
Quoted 5 lines · Hartmut Knaack
Quoted 4 lines · Guenter Roeck
Hi,
your mailer replaced all tabs with spaces, making a real review quite difficult. Please fix that.
Quoted 8 lines
It likes to do that, but I fixed it :-)
Quoted 6 lines
Couple of comments (not a complete review):
- For configuration, we commonly assume it is taken care of by the BIOS. Other that that, if you think anything needs to be configurable, use platform data and/or device tree information. No sysfs attributes, please.
I tend to regard this from the users point of view. My aim is to have it supported in openwrt, where you get your ready-to-flash image and install your custom kernel modules and software. It's fine to have some valid default configuration, maybe set by those ways you mentioned. But I would prefer to have some chance of runtime configuration. BIOS/bootloader is something you don't really want to mess with, platform data requires kernel (module) compilation, from what I know. Device tree might work, though I'm pretty unexperienced with that, yet.
Me too, but that is not an excuse. Ultimately, it is platform data, device tree, or nothing.
Quoted 4 lines
Quoted 3 lines
- Hysteresis is an absolute temperature, not a relative one. So the question for a generis attribute would be what it applies to. The current approach does not have that problem.
From what I see, most sensors store hysteresis as absolute temperature (like lm75). But some store one offset values (like adt7410, adt7475), which have to get converted by the driver to appear like absolute values. In case of the adt7410, the offset applies to min, max and crit, and in my driver it is set with
max_hyst(since I think that is the most common use). But if someone would only want to use alarm for min, it would be necessary to check/set max and setmax_hyst, which I think is a bit confusing.
I suggested to provide the hyst attribute for min, max, and crit. This way you cover it all. Sure, there are three attributes for one register, but that is ok. See the lm77 driver for an example.
Quoted 6 lines
Quoted 5 lines
- The access retry code in
adt7410_update_deviceis really quite complex. Is this really needed ? In general, the sensor should be configured for continuous conversion, and it should not be necessary to wait for a conversion to complete. besides, we can not afford an active 240 ms wait loop, so if the loop should _really_ be needed, you would have to add a sleep function call.Changed to sleep up to 4 times for 100 ms.
I'll really want to know why you think this is necessary before I accept it.
Quoted 6 lines
Quoted 5 lines
- The detect function is quite weak. you should definitely also check bits 0..3 of the status register (must be 0), and consider checking against known chip revision numbers. Question is also if the chip is expected to show up in a PC type system; if not, it might make sense to drop the detect function entirely, given its weakness.
Added those checks. I personally would not need the detect function, just added it for completeness.
I still don't feel comfortable with it. Do you have a use case where it is needed, or, in other words, do you know of a PC type system with such a sensor ? Otherwise it might be better to just drop it.
Quoted 3 lines
Quoted 2 lines
- No unnecessary ( ), please [no, it does not make the code easier to read, it just hides the really important ( )s].
hopefully fixed.
Not all of them, though.
Quoted 4 lines
Quoted 3 lines
- Please run the patch through checkpatch.pl. I can not really say for sure due to the tab problem how many issues there are, but I see at least one checkpatch error.
It complained about unneccessary parenthesis, before, but I thought it was safer to keep them.
Extra parenthesis make the code more difficult to read and understand. It gets difficult to find out what the coder really wanted to implement. So, really, no, it is not safer, it adds risk.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors