<p><a href="https://review.coreboot.org/26211">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig">File src/mainboard/google/octopus/Kconfig:</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/26211/1/src/mainboard/google/octopus/Kconfig@38">Patch Set #1, Line 38:</a> <code style="font-family:monospace,monospace">default "bip" if BOARD_GOOGLE_BIP</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is not correct. The config option is referring to mainboard directory.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig@51">Patch Set #1, Line 51:</a> <code style="font-family:monospace,monospace">      default "variants/bip/devicetree.cb" if BOARD_GOOGLE_BIP</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should go before the unconditional default</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig@63">Patch Set #1, Line 63:</a> <code style="font-family:monospace,monospace">default "Google_Bip" if BOARD_GOOGLE_BIP</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should go before the unconditional default</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig@89">Patch Set #1, Line 89:</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;">config INCLUDE_NHLT_BLOBS<br>    bool "Include blobs for audio."<br>     select NHLT_DMIC_4CH_16B<br>      select NHLT_DA7219<br>    select NHLT_MAX98357<br><br>config INCLUDE_NHLT_BLOBS_BIP<br> bool "Include blobs for bip audio."<br> select NHLT_DMIC_4CH_16B<br>      select NHLT_MAX98357<br>  select NHLT_RT5682<br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I was thinking we could handle it differently than poppy:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">config INCLUDE_NHLT_BLOBS<br>       bool "Include blobs for audio"<br>       select INCLUDE_NHLT_BLOBS_BIP if BOARD_GOOGLE_BIP<br>       select INCLUDE_NHLT_BLOBS_BASEBOARD</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and then provide configs for INCLUDE_NHLT_BLOBS_BIP and INCLUDE_NHLT_BLOBS_BASEBOARD. Then, you don't have to add different include options in config.${VARIANT}. All can just select INCLUDE_NHLT_BLOBS.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/variants/bip/nhlt.c">File src/mainboard/google/octopus/variants/bip/nhlt.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/26211/1/src/mainboard/google/octopus/variants/bip/nhlt.c@22">Patch Set #1, Line 22:</a> <code style="font-family:monospace,monospace">__weak</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why weak for variant-specific implementation?</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/26211">change 26211</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/26211"/><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: Iee91518c03a0e9e6ed52bc54a60fc607730a0b7d </div>
<div style="display:none"> Gerrit-Change-Number: 26211 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Naveen Manohar <naveen.m@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Naveen Manohar <naveen.m@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 10 May 2018 06:07:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>