Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 )
Change subject: tests: Add unit test to run init/shutdown for mec1308.c ......................................................................
Patch Set 7:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/51487/comment/77061e68_df95ab50 PS6, Line 9: hwaccess_io.h : because those are needed to test lifecycle of mec1308.c
I would like to see one other example of ported i/o usage being addressed with the same test code to […]
Do you mean you would like to see more tests that are using mocked i/o, to ensure the approach works for different tests, and not only for one mec1308 test? If yes, this is not a problem - I was going to write lots of test anyway. I will see which driver is a good another example, and will add another test in this patch. And then maybe we'll split.
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/9283f8fc_e9c511f1 PS6, Line 88: /* : * Uncomment LOG_ME below for verbose logging. : * It is commented by default because of too many calls. : */ : /* LOG_ME; */
does cmocka have a mechanism to allow verbosity to be tuned while running a test?
So far I was able to print everything or nothing, but I will look more into this. Those comments were not meant to be final solution. There are several things to consider: the verbosity is needed most when someone is writing new test or debugging failed test, and less so when just running all existing tests to ensure they pass. There are lots of i/o calls, maybe I can just count how many times they were called. Or have VERBOSE flag or something. I plan to think more about this.
Just to be clear: in any case this patch is needed https://review.coreboot.org/c/flashrom/+/52496 , without it nothing is printed.
https://review.coreboot.org/c/flashrom/+/51487/comment/75a27607_7749bc98 PS6, Line 102: return ((port == 0x2e /* MEC1308_SIO_PORT1 */ : || port == 0x4e /* MEC1308_SIO_PORT2 */) : ? 0x20 /* MEC1308_DEVICE_ID_REG */ : : ((outb_val == 0x84 /* MEC1308_MBX_DATA_START */) : ? 0xaa /* MEC1308_CMD_PASSTHRU_SUCCESS */ : : 0));
such values and logic may want to be encoded and stashed away within […]
The tricky thing is: inb needs to know what was the value of the last outb call.
You are right it would be ideal to have this code inside mec1308_init_and_shutdown_test_success() , but it's not obvious how to do it. This is contained inside tests/ anyway, so it's easy and not risky to change the code.
What do you think?
-
Anastasia Klimchuk (Code Review)