Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... PS4, Line 34: google_chromeec_smbios_system_sku
Yes, to get rid of the wrapping indirect, what I was saying is that the the function is named without the googleness in mind in the general code path of arch/x86/smbios.c and wanted to keep it so that it isn't over-fitted to our needs.
Why is googleness required in the general code path? It can still make a call to smbios_system_sku() and instead of mainboard implementing it, implementation for Google boards with Chrome EC will be provided by this file.
Do you mind if we can treat that as a separate bug and scope those changes to that?
Sure, we can go ahead with this change, but to be very honest, I don't think we need to change anything except rename this function as smbios_system_sku(). Example: https://review.coreboot.org/cgit/coreboot.git/tree/src/ec/google/chromeec/ec.... Weak definition is provided in lib/coreboot_table.c, but its definition for Google boards is provided by ec_boardid.c.
In fact, that makes me think -- Why are we even adding google_chromeec_get_board_sku()? Shouldn't google_chromeec_get_board_sku() be just named as sku_id()?
Basically, I am thinking of this file as providing default implementations of common functions handling system SKU for Google boards using Chrome EC. These function names don't need to be Google specific. It is just the implementation that is provided. That will allow us to reduce redundant code across mainboards where they would simply end up calling this EC function.
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 2: * Use SPDX-License?
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 4: Inc LLC
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 25: static MAYBE_STATIC_NONZERO since this is added to early stages as well.