On Wednesday 22 June 2011 21:15:49 Guenter Roeck wrote: > On Tue, 2011-06-21 at 05:52 -0400, Alexander Stein wrote: > > An hwmon driver for the National Semiconductor LM95245 dual temperature > > sensors chip. > > > > Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxxxxxxxx> > > Overall pretty well written, only I am a bit at loss why you try to deal > with the unsigned temperature registers. That adds a bit of complexity > to the driver. Given that the signed temperature value range is up to > 127 degrees C, and that the chip is only rated up to 125 degrees C, the > added complexity doesn't seem to be worth it. > > Can you elaborate ? Well, AFAIK the 125 degrees C limit is for the chip itself. I didn't found anything about a remote diode limit. Also the remote TCRIT and OS limit range up to 255 degrees C. Another point is the optional offset register (not implemented, see below). I could not found much about it, but I guess this is immediately added to the remote temperature register. > Other comments: > > For the interval attribute, idea would be to write the value into the > conversion rate register. Your code does not match the interval with the > rate programmed into the chip (which is the idea), nor does it update > the rate if the interval is changed. Well, I noticed that. But I went the way lm95241 does. I'm also unsure which interval to choose, if user specify a unsupported interval. Choose the next small or the next greater one? Maybe you can give me a hint here. > Chip address 0x29 is missing. Nice catch. > It would be nice if the driver would support limit, hysteresis, alarm, > and fault attributes. Let's see if I find time for that. Thanks for the review though. Regards, Alexander _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
