Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58475 )
Change subject: spi25_statusreg: make register read/write functions generic ......................................................................
Patch Set 10:
(4 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58475/comment/84c90471_c1321d0c PS10, Line 171: Lets maybe remove this empty line. Enum is not that long, I don't thing it needs "sections".
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58475/comment/017c2461_c5456c24 PS9, Line 120: msg_cerr("Cannot read register: not supported by chip\n");
The `if (write_cmd_len == 0)` is used to check this. […]
Maybe I am missing something, but how nested ifs prevent using an else statement? You wrap conditional body in curly brackets anyway?
In this patch, why it can't be for spi_write_register if (reg == STATUS1) { doing something } else { error message }
and in the next patch that will be if (reg == STATUS1) { doing something else if (reg == STATUS2) { doing something } else { error message }
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58475/comment/a842ec9a_8ca056da PS10, Line 119: } : else { : I am sorry I should have noticed this earlier, but `else` (and `else if`) needs to be on the same line with closing brace. This applies everywhere in this file, and in other files ofc
We are following Linux kernel coding style https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... (see section 3)
https://review.coreboot.org/c/flashrom/+/58475/comment/e9a8b624_2f7fd789 PS10, Line 130: flash, sizeof(read_cmd), sizeof(readarr), : &read_cmd, readarr); This can fit into 112 chars right? It helps to git grep when all function arguments are on one line. 112 is a lot, fitting in 80 is better, but for function calls having all args on one line is useful.
-
Anastasia Klimchuk (Code Review)