<p style="white-space: pre-wrap; word-wrap: break-word;">Lots of style comments.  Welcome to coreboot.  :)</p><p><a href="https://review.coreboot.org/27619">View Change</a></p><p>18 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.h">File src/soc/amd/common/block/psp/psp.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/27619/1/src/soc/amd/common/block/psp/psp.h@4">Patch Set #1, Line 4:</a> <code style="font-family:monospace,monospace">2017</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is this a new file, or copied from somewhere else?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.h@34">Patch Set #1, Line 34:</a> <code style="font-family:monospace,monospace">G              </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">change to spaces to match the rest of the file?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.h@35">Patch Set #1, Line 35:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Extra newline at the end.  This is what's causing the jenkins failure.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c">File src/soc/amd/common/block/psp/psp.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/27619/1/src/soc/amd/common/block/psp/psp.c@12">Patch Set #1, Line 12:</a> <code style="font-family:monospace,monospace"> * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You probably didn't mean to make this change</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@27">Patch Set #1, Line 27:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">extra line</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@37">Patch Set #1, Line 37:</a> <code style="font-family:monospace,monospace">static BOOLEAN psp_bar_init_early(VOID);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We don't generally have function definitions for static functions.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@38">Patch Set #1, Line 38:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">extra line</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@46">Patch Set #1, Line 46:</a> <code style="font-family:monospace,monospace">BOOLEAN</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We don't really use BOOLEAN in coreboot.  It's only used in vendorcode</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@49">Patch Set #1, Line 49:</a> <code style="font-family:monospace,monospace">PspMmioSize;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No camelcase unless it's needed for interfacing with vendorcode please.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@61">Patch Set #1, Line 61:</a> <code style="font-family:monospace,monospace">//</code></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">For multi-line comments, please use a style like this:<br>/*<br> * Comment<br> */</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@96">Patch Set #1, Line 96:</a> <code style="font-family:monospace,monospace"> 0x34);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does this need to be split to a separate line?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@110">Patch Set #1, Line 110:</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;">/**<br>  Return the PspMMIO MMIO location<br><br>  @param[in] PspMmio Pointer to Psp MMIO address<br><br>  @retval BOOLEAN  0: Error, 1 Success<br>**/<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same issue with the comment style, with the additional * for doxygen:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/**<br> *<br> *<br> */</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@117">Patch Set #1, Line 117:</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;">static BOOLEAN<br>get_psp_bar3_addr(<br>   IN OUT   u32 *PspMmio<br> )<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Single line?  We don't typically use BOOLEAN, or IN & OUT in coreboot code:</p><p style="white-space: pre-wrap; word-wrap: break-word;">static int get_psp_bar3_addr(u32 psp_mmio_addr)<br>{</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@134">Patch Set #1, Line 134:</a> <code style="font-family:monospace,monospace">BIT12</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You mention updating the magic numbers in a todo, but just changing this to a BAR3_HIDE #define would eliminate the need for the above comment.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@140">Patch Set #1, Line 140:</a> <code style="font-family:monospace,monospace">__func__</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We probably don't need to print out __func__ all over the place.  I'd just print the message and leave that off.  Generally you can find the correct function if you just search for the error message, and it makes the log a lot cleaner if you leave them out.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@148">Patch Set #1, Line 148:</a> <code style="font-family:monospace,monospace">BIOS_DEBUG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You might want to look at what you're printing vs what's really useful.  Just printing the address of the PSP isn't really going to be useful most of the time, so you might want to change this to BIOS_SPEW.</p><p style="white-space: pre-wrap; word-wrap: break-word;">On the other hand, when we fail above, that might be better as a BIOS_WARNING.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@187">Patch Set #1, Line 187:</a> <code style="font-family:monospace,monospace">, base=0x%x...\</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Here and above, do we care what base is if the function failed?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@287">Patch Set #1, Line 287:</a> <code style="font-family:monospace,monospace">      </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You might want to make sure your editor isn't configured to change this for some reason.</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/27619">change 27619</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/27619"/><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: I2740ceb945736c6e413f7d0bd0c41a19e19c7d5a </div>
<div style="display:none"> Gerrit-Change-Number: 27619 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Charles Marslett <charles.marslett@amd.corp-partner.google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Charles Marslett <charles.marslett@silverbackltd.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-CC: Martin Roth <martinroth@google.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 24 Jul 2018 21:09:35 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>