Attention is currently required from: ChrisEric1 CECL, Thomas Heijligen, Nicholas Chin.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72057 )
Change subject: Add support for VIA VL805 USB Controller flashing ......................................................................
Patch Set 18: Code-Review+1
(12 comments)
File vl805.c:
https://review.coreboot.org/c/flashrom/+/72057/comment/97423f67_9684d6a3 PS18, Line 58: drop \n line 58.
https://review.coreboot.org/c/flashrom/+/72057/comment/7a8971b3_6a3883ad PS18, Line 70: unsigned int i, j; just scope iterators to loops.
https://review.coreboot.org/c/flashrom/+/72057/comment/bf1c5a6d_45ace785 PS18, Line 71: uint32_t outdata; : uint32_t indata; : unsigned int curwritecnt; : unsigned int curreadcnt; scope to respective loop constructs..
https://review.coreboot.org/c/flashrom/+/72057/comment/1fd2856e_92b7f100 PS18, Line 79: curwritecnt = min(4, writecnt - j); .. i.e., `uint32_t curwritecnt = min(4, writecnt - j);`
https://review.coreboot.org/c/flashrom/+/72057/comment/7817b7f8_8c257c24 PS18, Line 107: programmer `_mcu_` ?
https://review.coreboot.org/c/flashrom/+/72057/comment/7ab0abb3_2f644793 PS18, Line 117: /* Shutdown stuff. */ remove, adds no value.
https://review.coreboot.org/c/flashrom/+/72057/comment/e1fc2ba6_cfe4dfdf PS18, Line 118: dev just pass `vl805_data->dev` it is already well-typed.
https://review.coreboot.org/c/flashrom/+/72057/comment/78cf2a71_d10bb8c8 PS18, Line 130: .write_aai = default_spi_write_aai, no need for this default hook, see CB:67479
https://review.coreboot.org/c/flashrom/+/72057/comment/321f6dd2_3d390f74 PS18, Line 141: ditto
https://review.coreboot.org/c/flashrom/+/72057/comment/9fdd1cc9_1760a08d PS18, Line 146: nit: drop this \n
https://review.coreboot.org/c/flashrom/+/72057/comment/d96f61e8_3b654ddc PS18, Line 158: vl805_programmer_active(dev, 0x1) you seem to toggle on on line 152, read fw rev, toggle off and then immediately back on again?
https://review.coreboot.org/c/flashrom/+/72057/comment/9cba9c60_f807f0d9 PS18, Line 171: vl805_programmer_active(dev, 0x0); : : vl805_programmer_active(dev, 0x1); : same here? if this is from a trace it could be from some silly OOP structure that always does a toggle on->do thing->toggle off pattern which leads to a lot of this kind of procedural noise.
-
Edward O'Callaghan (Code Review)