Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33413 )
Change subject: mb/google/kukui: Add panels for Krane ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33413/15/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel_krane.c:
https://review.coreboot.org/c/coreboot/+/33413/15/src/mainboard/google/kukui... PS15, Line 36: INIT_DCS_CMD, 1, {0x11} Not super happy about having to match the size here manually. Can we make wrapper macros for that? You could do something like
#define INIT_DCS_CMD(...) _LCM_CMD(INIT_DCS_OPCODE, __VA_ARGS__, 2, 1, 0) #define _LCM_CMD(opcode, arg0, arg1, NARGS, ...) \ {opcode, NARGS, { _FILL_ARGS_##NARGS(arg0, arg1) } #define _FILL_ARGS_2(arg0, arg1) arg0, arg1 #define _FILL_ARGS_1(arg0, arg1) arg0 #define _FILL_ARGS_0(arg0, arg1)
(Can be expanded as needed, obviously.)
Alternatively, if we only need to support a relatively small amount of sizes per command, it might be easier to just have a different opcode for each size (e.g. INIT_DSC1(byte), INIT_DSC2(byte0, byte1), etc.).
https://review.coreboot.org/c/coreboot/+/33413/15/src/mainboard/google/kukui... PS15, Line 357: static struct panel_description *krane_panels[] = { We should split these into a separate CBFS file each like we have for SDRAM params. That way you don't need to load the super long one on boards that don't need it (and it will just get more important the more you add).