Attention is currently required from: Sean Rhodes. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55128 )
Change subject: src/mainboard: Add Star Labs labtop series ......................................................................
Patch Set 15:
(10 comments)
Patchset:
PS15:
Does this mean the patch has to submitted again?
Yes. I would create a separate change for the KBL and the CML boards, which should be easier to review. With the current state of affairs, using a variant setup with different SoCs is too impractical. I'd do something like what Purism does: mb/starlabs/labtop_kbl and mb/starlabs/labtop_cml.
PS15: Looks like I forgot to send these comments
File src/mainboard/starlabs/labtop/Kconfig:
https://review.coreboot.org/c/coreboot/+/55128/comment/a5e42e4b_98e03efb PS15, Line 26: # select HAVE_IFD_BIN : # select HAVE_ME_BIN
Nope. See previous comment.
Which one is the previous comment?
File src/mainboard/starlabs/labtop/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/55128/comment/20676505_49e6aab7 PS15, Line 6: /* Power button device. */ : Device (PWRB) : { : Name (_HID, EisaId ("PNP0C0C")) : Name (PBST, One) : : Method (_STA, 0, NotSerialized) : { : Return (0x0F) : } : }
For what reason?
See ACPI spec, section 4.8.2.2.1 Power Button
The fixed power button should be good enough. I recall having a PWRB device in ASL caused two power button devices to appear. CB:27272 has a good explanation about it.
File src/mainboard/starlabs/labtop/mainboard.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/928e9402_7b30369c PS15, Line 33: #if CONFIG(BOARD_STARLABS_LABTOP_CML)
How?
Just use C code, and let the compiler optimise out the unreachable paths:
if (CONFIG(BOARD_STARLABS_LABTOP_CML)) return "L4"; else return "L3-U"; endif
https://review.coreboot.org/c/coreboot/+/55128/comment/6366463c_9a331925 PS15, Line 52: /* Override smbios_mainboard_board_type */ : smbios_board_type smbios_mainboard_board_type(void) : { : return SMBIOS_BOARD_TYPE_MOTHERBOARD; : }
Why?
It's the default board type since CB:50180
File src/mainboard/starlabs/labtop/ramstage.c:
https://review.coreboot.org/c/coreboot/+/55128/comment/e8b18f5d_d25e16f6 PS15, Line 10: #if CONFIG(BOARD_STARLABS_LABTOP_CML)
Is there any advantage? Not being difficult, simply interested.
It's cleaner. Plus, FSP likes to reconfigure GPIOs: having coreboot configure GPIOs after FSP is done can prevent weird issues if FSP decides it knows better.
File src/mainboard/starlabs/labtop/variants/cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/55128/comment/2cb0907b_9018fc56 PS15, Line 140: device pci 00.0 on end
It will always be present on our hardware, can remove though
I would remove it for the sake of correctness. The only difference it makes is that the `on_mainboard` field of the device gets unset.
File src/mainboard/starlabs/labtop/variants/cml/include/variant/hda_verb.h:
https://review.coreboot.org/c/coreboot/+/55128/comment/00a34c34_73ea9eb1 PS15, Line 36: /* ONE DOES NOT SIMPLY : MAKE IT WORK WITH WINDOWS */
A comment is necassary. […]
Well, we can have the best of both worlds: the helpful reasoning and the meme 😉
File src/mainboard/starlabs/labtop/variants/kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/55128/comment/c954dec1_21e8fd51 PS15, Line 24: .up_delay_ms= 200,// T3 : .down_delay_ms= 0,// T10 : .cycle_delay_ms = 500,// T12 : .backlight_on_delay_ms=50,// T7 : .backlight_off_delay_ms = 0,// T9 : .backlight_pwm_hz = 200,
As far as I know, nit is an achaic term for brightness of a display. […]
In the context of code reviews, it means something is a minor issue. This one, for instance, is just a matter of aesthetics. I... honestly don't know where this meaning of "nit" comes from, though.