linux-kernel - Re: [PATCH] hwmon: LM95245 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 23 Jun 2011 11:14:52 +0200
From:	Alexander Stein <alexander.stein@...tec-electronic.com>
To:	guenter.roeck@...csson.com
Cc:	Jean Delvare <khali@...ux-fr.org>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] hwmon: LM95245 driver

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@...tec-electronic.com>
> 
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/