Attention is currently required from: Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56911 )
Change subject: tests: add init_shutdown test for realtek_mst_i2c_spi ......................................................................
Patch Set 2:
(5 comments)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/56911/comment/6e8a138b_0bd1ace7 PS2, Line 307: 0x10ec This number has some meaning, also I see it is used in other mocks, so surely it is important. Could you please add a short comment here on what this is? Does it have to be exactly this number, or any number, but the same in all mocks? Maybe it can be #define REALTEK_MOCK_FD ?
https://review.coreboot.org/c/flashrom/+/56911/comment/c6e004be_1b7d5a11 PS2, Line 319: printf("ioctl(0x10ec, I2C_SLAVE, %#lx)\n", addr); This looks like a debugging statement, not sure we need it. Wrap invocations log themselves already (that first line LOG_ME does it), which should be sufficient for a normal situation, when tests pass. Same for the rest of printf, let's remove. I use them a lot when writing a test, but once test is ready that would just make log larger. (and when tests fail that would be a more targeted debugging anyway).
For future, if I discover a way to tune verbosity level in cmocka, we may add debug-only messages.
https://review.coreboot.org/c/flashrom/+/56911/comment/2a7af034_ea6cbf70 PS2, Line 320: 0x4a This number also needs a comment
https://review.coreboot.org/c/flashrom/+/56911/comment/d916089d_b9c5cc24 PS2, Line 341: printf("i2c set register address %#x\n", bytes_buf[0]); : return 1; If you remove printf statements, this looks like one error path which returns -1 and one normal path which returns sz?
https://review.coreboot.org/c/flashrom/+/56911/comment/46a57ea7_61298014 PS2, Line 365: T typo
-
Anastasia Klimchuk (Code Review)