<p><a href="https://review.coreboot.org/c/coreboot/+/21194">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/chip.h">File src/drivers/i2c/lm96000/chip.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/chip.h@35">Patch Set #4, Line 35:</a> <code style="font-family:monospace,monospace">enum lm96000_fan_mode {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/chip.h@36">Patch Set #4, Line 36:</a> <code style="font-family:monospace,monospace">    LM96000_FAN_IGNORE      = 0x00,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">is there a case for the ignore mode? if no fan is connetced, I'd rather use the disable mode</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.h">File src/drivers/i2c/lm96000/lm96000.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.h@37">Patch Set #4, Line 37:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">#define LM96000_TACH_TO_RPM(t) (((t) & 0xfffc) && (((t) & 0xfffc) != 0xfffc) \<br>                                   ? (60 * 90000 / ((t) & 0xfffc)) \<br>                                 : 0)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">seems to be unused</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.h@56">Patch Set #4, Line 56:</a> <code style="font-family:monospace,monospace">#define LM96000_ZONE_RANGE(zone)        (0x5f + (zone))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same definition as LM96000_FAN_FREQ(fan). would it be a good idea to merge these two definitions?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.c">File src/drivers/i2c/lm96000/lm96000.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.c@36">Patch Set #4, Line 36:</a> <code style="font-family:monospace,monospace">unset_mask</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">clear_mask</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.c@104">Patch Set #4, Line 104:</a> <code style="font-family:monospace,monospace">tach</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">tach & 0xff</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21194/4/src/drivers/i2c/lm96000/lm96000.c@171">Patch Set #4, Line 171:</a> <code style="font-family:monospace,monospace">? :</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">seems that there's something missing here</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/c/coreboot/+/21194">change 21194</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/c/coreboot/+/21194"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ie7df3107bffb7f8e45e71c4c1fbe4eb0a9e3cd03 </div>
<div style="display:none"> Gerrit-Change-Number: 21194 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Felix Held <felix-coreboot@felixheld.de> </div>
<div style="display:none"> Gerrit-Reviewer: Martin Roth <martinroth@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 16 Dec 2018 22:48:56 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>