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:	Wed, 22 Jun 2011 12:15:49 -0700
From:	Guenter Roeck <guenter.roeck@...csson.com>
To:	Alexander Stein <alexander.stein@...tec-electronic.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 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 ?

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.

Chip address 0x29 is missing.

It would be nice if the driver would support limit, hysteresis, alarm,
and fault attributes.

Thanks,
Guenter

--
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/