Attention is currently required from: Edward O'Callaghan, Neill Corlett. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 )
Change subject: Add mediatek_i2c_spi interface. ......................................................................
Patch Set 1:
(18 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/61288/comment/a3148a58_b1d0d2b2 PS1, Line 7: . nit: no period at the end of commit summaries
File mediatek_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/61288/comment/91632cdc_d79530a7 PS1, Line 29: #define ISP_PORT 0x92 >> 1 : #define DEBUG_PORT 0xb2 >> 1 Please wrap these in parentheses to avoid operation order problems:
#define ISP_PORT (0x92 >> 1) #define DEBUG_PORT (0xb2 >> 1)
https://review.coreboot.org/c/flashrom/+/61288/comment/d5daf8c5_22b52d58 PS1, Line 36: nit: double blank line
https://review.coreboot.org/c/flashrom/+/61288/comment/d4f34959_79672189 PS1, Line 55: uint8_t* buf The `*` unary operator should have a space before, not after:
uint8_t *buf
Same applies to the rest of the file.
https://review.coreboot.org/c/flashrom/+/61288/comment/0bd79ba2_e26992b6 PS1, Line 150: len We know `len` is non-zero, special case is handled above. But I'd still keep this check here for clarity.
https://review.coreboot.org/c/flashrom/+/61288/comment/661ab922_d83e71a9 PS1, Line 181: 2 ARRAY_SIZE(data)
Macro is defined in flash.h
https://review.coreboot.org/c/flashrom/+/61288/comment/398b455c_0c633cdd PS1, Line 187: size_t len = 1; Variable seems to be unnecessary
https://review.coreboot.org/c/flashrom/+/61288/comment/8746b7f5_189ef02d PS1, Line 211: 3 ARRAY_SIZE(data)
https://review.coreboot.org/c/flashrom/+/61288/comment/8cb891a9_2aa73584 PS1, Line 242: GPIO line
Is this board depended? Should this perhaps become a programmer parameter if so? i.e. […]
+1
https://review.coreboot.org/c/flashrom/+/61288/comment/07329d84_73348af5 PS1, Line 249: 0x426, 7 What are these magic numbers?
https://review.coreboot.org/c/flashrom/+/61288/comment/f6dbcc3e_4c2edac9 PS1, Line 255: 0x428, 7 What are these magic numbers?
https://review.coreboot.org/c/flashrom/+/61288/comment/ef3219e0_8ee3973a PS1, Line 266: { Please move the brace to the following line
https://review.coreboot.org/c/flashrom/+/61288/comment/283c3c0c_87002743 PS1, Line 269: unsigned char This assumes that the size of `unsigned char` is always 1 byte. I'd use `uint8_t`.
https://review.coreboot.org/c/flashrom/+/61288/comment/549b8fe8_504aa10f PS1, Line 271: sizeof Along with the changes above, this should then be `ARRAY_SIZE`
https://review.coreboot.org/c/flashrom/+/61288/comment/75c76fe8_12ce18ed PS1, Line 295: 0x7F Please use lowercase for hex
https://review.coreboot.org/c/flashrom/+/61288/comment/32f8d483_3729467c PS1, Line 330: { Please move the brace to the following line
https://review.coreboot.org/c/flashrom/+/61288/comment/3daa57d7_8c972182 PS1, Line 348: } If both attempts fail, can we assume that entering ISP has failed?
https://review.coreboot.org/c/flashrom/+/61288/comment/f751156c_40623b5b PS1, Line 360: { Please move the brace to the following line