[coreboot-gerrit] Change in coreboot[master]: mb/google/octopus: Add variant for BIP

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/26211 )

Change subject: mb/google/octopus: Add variant for BIP
......................................................................


Patch Set 1:

(5 comments)

https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig
File src/mainboard/google/octopus/Kconfig:

https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig@38
PS1, Line 38: default "bip" if BOARD_GOOGLE_BIP
This is not correct. The config option is referring to mainboard directory.


https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig@51
PS1, Line 51: 	default "variants/bip/devicetree.cb" if BOARD_GOOGLE_BIP
This should go before the unconditional default


https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig@63
PS1, Line 63: default "Google_Bip" if BOARD_GOOGLE_BIP
This should go before the unconditional default


https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/Kconfig@89
PS1, Line 89: config INCLUDE_NHLT_BLOBS
            : 	bool "Include blobs for audio."
            : 	select NHLT_DMIC_4CH_16B
            : 	select NHLT_DA7219
            : 	select NHLT_MAX98357
            : 
            : config INCLUDE_NHLT_BLOBS_BIP
            : 	bool "Include blobs for bip audio."
            : 	select NHLT_DMIC_4CH_16B
            : 	select NHLT_MAX98357
            : 	select NHLT_RT5682
            : 
I was thinking we could handle it differently than poppy:

config INCLUDE_NHLT_BLOBS
       bool "Include blobs for audio"
       select INCLUDE_NHLT_BLOBS_BIP if BOARD_GOOGLE_BIP
       select INCLUDE_NHLT_BLOBS_BASEBOARD

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.


https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/variants/bip/nhlt.c
File src/mainboard/google/octopus/variants/bip/nhlt.c:

https://review.coreboot.org/#/c/26211/1/src/mainboard/google/octopus/variants/bip/nhlt.c@22
PS1, Line 22: __weak
Why weak for variant-specific implementation?



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

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee91518c03a0e9e6ed52bc54a60fc607730a0b7d
Gerrit-Change-Number: 26211
Gerrit-PatchSet: 1
Gerrit-Owner: Naveen Manohar <naveen.m at intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Naveen Manohar <naveen.m at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Thu, 10 May 2018 06:07:19 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180510/c4255990/attachment.html>