Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk.

4 comments:

File flash.h:
- Patch Set #10, Line 171:
  Lets maybe remove this empty line. Enum is not that long, I don't thing it needs "sections".
File spi25_statusreg.c:
- Patch Set #9, 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:
- Patch Set #10, 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/Documentation/process/coding-style.rst (see section 3)
- Patch Set #10, 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.

To view, visit change 58475. To unsubscribe, or for help writing mail filters, visit settings.