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

[Felix Held (Code Review)]

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

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


Patch Set 1:

(6 comments)

https://review.coreboot.org/#/c/29475/1/src/drivers/i2c/nct7802y/nct7802y.h 
File src/drivers/i2c/nct7802y/nct7802y.h:

https://review.coreboot.org/#/c/29475/1/src/drivers/i2c/nct7802y/nct7802y.h@39 
PS1, Line 39: << 0
the << 0 isn't really needed here and in the 3 lines below. if it's for consistency with the other bit definitions, I'm not opposed to keep that though.


https://review.coreboot.org/#/c/29475/1/src/drivers/i2c/nct7802y/nct7802y.h@55 
PS1, Line 55: tab
maybe rename tab to fan, so it's clearer that that's the fan id. same for the line below


https://review.coreboot.org/#/c/29475/1/src/drivers/i2c/nct7802y/nct7802y.h@93 
PS1, Line 93: unset_mask
clear_mask?


https://review.coreboot.org/#/c/29475/1/src/drivers/i2c/nct7802y/nct7802y_fan.c 
File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/#/c/29475/1/src/drivers/i2c/nct7802y/nct7802y_fan.c@32 
PS1, Line 32: 3
use NCT7802Y_FAN_CNT here


https://review.coreboot.org/#/c/29475/1/src/drivers/i2c/nct7802y/nct7802y_fan.c@68 
PS1, Line 68: div
for the case smart.mode == SMART_FAN_DUTY, div = 1 is likely wrong (comment in chip.h says that target should be in % and if I read this correctly, the register expects the value being between 0 and 255)


https://review.coreboot.org/#/c/29475/1/src/drivers/i2c/nct7802y/nct7802y_fan.c@82 
PS1, Line 82: 		return;
print a warning, when config is 0?



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

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I35ea79e12941804e398c6304a08170a776f4ca76
Gerrit-Change-Number: 29475
Gerrit-PatchSet: 1
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: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Sun, 16 Dec 2018 22:03:23 +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/77c03af2/attachment.html>