Re: [PATCH] hwmon: LM95245 driver

[Alexander Stein]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/