<p>Nico Huber <strong>posted comments</strong> on this change.</p><p><a href="https://review.coreboot.org/21480">View Change</a></p><p>Patch set 2:</p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for taking this up.</p><p>(7 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c">File src/device/dram/ddr2.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@116">Patch Set #2, Line 116:</a> <code style="font-family:monospace,monospace">static int spd_decode_tck_time(u8 c, u32 *tck)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd prefer the pointer to be the first argument (old writel vs<br>write32 debate).</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@119">Patch Set #2, Line 119:</a> <code style="font-family:monospace,monospace">    int ret = CB_SUCCESS;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We could just bail out directly and drop the variable. Not writing<br>`*tck` isn't bad, IMO. Just a matter of personal preference, I guess.</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@155">Patch Set #2, Line 155:</a> <code style="font-family:monospace,monospace"> * Returns cycle time in 1/256th ns.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(I hate comments)</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@157">Patch Set #2, Line 157:</a> <code style="font-family:monospace,monospace">u32</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">nit, er, int ;)</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@350">Patch Set #2, Line 350:</a> <code style="font-family:monospace,monospace">              printk(BIOS_WARNING, "  Invalid number of memory rows\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think, we should keep the indent. With low loglevel, the<br>message would be freestanding. Also we might want to give some<br>context for that case, e.g. "... in SPD" or "SPD decode: ..."?</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@440">Patch Set #2, Line 440:</a> <code style="font-family:monospace,monospace"> if (spd_decode_bcd_time(spd[10], &dimm->access_time[cl]) != CB_SUCCESS) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">line is too long</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@480">Patch Set #2, Line 480:</a> <code style="font-family:monospace,monospace">             printk(BIOS_WARNING,"  Invalid rank density.\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">space after comma, no space before the output?</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/21480">change 21480</a>. To unsubscribe, 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/21480"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Iee434d1fa1a9d911cc3683b88b260881ed6434ea </div>
<div style="display:none"> Gerrit-Change-Number: 21480 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> </div>
<div style="display:none"> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 10 Sep 2017 19:35:01 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>