On Tue, Jul 31, 2012 at 10:17:18PM +0200, Hartmut Knaack wrote: > 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. > > 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 lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
