Attention is currently required from: Iru Cai (vimacs). Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44975 )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Patch Set 5:
(9 comments)
File src/ec/dell/mec5055/early_init.c:
PS5: Is it necessary to have a separate file for this one function?
https://review.coreboot.org/c/coreboot/+/44975/comment/b1723764_e2d8c94a PS5, Line 7: u8 buf[32], c; Initialise the buffer?
u8 buf[32] = { 0 };
File src/ec/dell/mec5055/mec5055.h:
https://review.coreboot.org/c/coreboot/+/44975/comment/a70a7920_2e876765 PS5, Line 6: #include <stddef.h> : #include <stdint.h> Already provided by `types.h`
File src/ec/dell/mec5055/mec5055.c:
https://review.coreboot.org/c/coreboot/+/44975/comment/e2215c4c_0db335c3 PS5, Line 4: #include <console/console.h> nit: also #include <types.h> here
https://review.coreboot.org/c/coreboot/+/44975/comment/e9b14db8_f65fc15c PS5, Line 21: 0x20 This 0x20 is the same numeric value as the `32` in early_init.c, why not #define it?
https://review.coreboot.org/c/coreboot/+/44975/comment/a88642d2_8ecb3441 PS5, Line 21: start >= 0x20 This check is redundant. Either `count` is positive (non-zero) and the other check is true, or `count` is zero and the code does nothing.
https://review.coreboot.org/c/coreboot/+/44975/comment/3e72768b_f9683ae4 PS5, Line 56: write arguments to EC[2:] I *really* don't like this kind of buffer handling. Why not have separate parameters for the command and the buffer?
https://review.coreboot.org/c/coreboot/+/44975/comment/abc34e4c_1c2ca782 PS5, Line 59: result may starts at EC[0] or EC[1] Why this difference? Does the EC handle some commands differently?
https://review.coreboot.org/c/coreboot/+/44975/comment/f9b8b8f4_9d68c812 PS5, Line 59: starts no `s` at the end: start