[coreboot-gerrit] Change in ...coreboot[master]: drivers/i2c/lm96000: Add new hardware-monitoring IC

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/21194 )

Change subject: drivers/i2c/lm96000: Add new hardware-monitoring IC
......................................................................


Patch Set 4:

(7 comments)

https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/chip.h 
File src/drivers/i2c/lm96000/chip.h:

https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/chip.h@35 
PS4, Line 35: enum lm96000_fan_mode {
maybe add a comment that the lower 3 bits correspond to what gets written in th hardware register and the MSB is a flag, that doesn't map directly to a register


https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/chip.h@36 
PS4, Line 36: 	LM96000_FAN_IGNORE	= 0x00,
is there a case for the ignore mode? if no fan is connetced, I'd rather use the disable mode


https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.h 
File src/drivers/i2c/lm96000/lm96000.h:

https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.h@37 
PS4, Line 37: #define LM96000_TACH_TO_RPM(t) (((t) & 0xfffc) && (((t) & 0xfffc) != 0xfffc) \
            : 					? (60 * 90000 / ((t) & 0xfffc)) \
            : 					: 0)
seems to be unused


https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.h@56 
PS4, Line 56: #define LM96000_ZONE_RANGE(zone)	(0x5f + (zone))
same definition as LM96000_FAN_FREQ(fan). would it be a good idea to merge these two definitions?


https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.c 
File src/drivers/i2c/lm96000/lm96000.c:

https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.c@36 
PS4, Line 36: unset_mask
clear_mask


https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.c@104 
PS4, Line 104: tach
tach & 0xff


https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.c@171 
PS4, Line 171: ? :
seems that there's something missing here



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/21194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie7df3107bffb7f8e45e71c4c1fbe4eb0a9e3cd03
Gerrit-Change-Number: 21194
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus at gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot at felixheld.de>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
Gerrit-Comment-Date: Sun, 16 Dec 2018 22:48:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181216/c499341e/attachment.html>