[coreboot-gerrit] [L] Change in coreboot[main]: superio/ite: Unify it8722f with common code

[Nico Huber (Code Review)]

Attention is currently required from: Joel Linn, Matt DeVillier, Michał Żygowski, MrChromebox, Piotr Król.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81310?usp=email )

Change subject: superio/ite: Unify it8722f with common code
......................................................................

Patch Set 4: Code-Review+1

(11 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/81310/comment/2bdf3cbc_10bce208 :
PS4, Line 7: it8722f
it87*7*2f

https://review.coreboot.org/c/coreboot/+/81310/comment/cf4130fa_d3c66185 :
PS4, Line 9: it8722f
it87*7*2f

Patchset:

PS4: 
Mostly nits.

I've reviewed this primarily to avoid regressions. Didn't check every single
register (not those that are unused by current configurations).

File src/mainboard/google/beltino/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/81310/comment/aea93725_4094c9a7 :
PS4, Line 86: 					register "FAN2.mode" = "FAN_SMART_SOFTWARE"
IIUC, the old code would have left the PWM signal at 0% (datasheet
default for register 0x73). The new code would set 50%.

Not sure if it matters, software mode is probably expected to be 
controlled by the OS.

File src/superio/ite/common/env_ctrl.c:

https://review.coreboot.org/c/coreboot/+/81310/comment/52ce9518_31ddb09d :
PS4, Line 84: * 
As by our coding style, no leading asterisk in the short comment form. e.g.
```
/* Short, multiline comment. Without
   leading asterisk on the second line. */
```

https://review.coreboot.org/c/coreboot/+/81310/comment/db2b379e_27a04e19 :
PS4, Line 86: 			    reg != reg_new) {
Line break isn't needed, as we allow ~96 chars per line.

https://review.coreboot.org/c/coreboot/+/81310/comment/57d64884_d0e92e8f :
PS4, Line 110: 		break;
Shouldn't we `return` here?

Could also move this case to the top.

https://review.coreboot.org/c/coreboot/+/81310/comment/548390f1_f0d4362c :
PS4, Line 186: 		if (!CONFIG(SUPERIO_ITE_IT8772F)) {
We shouldn't check for specific chips in common code. This should be a
Kconfig feature, e.g.
```
config SUPERIO_ITE_ENV_CTRL_FULLSPEED_SETTING
    bool
```

https://review.coreboot.org/c/coreboot/+/81310/comment/660acab2_3f1e85f0 :
PS4, Line 188: 		}
Nit, while we allow {} for single line blocks, it's preferred to keep a
style choice throughout a source file. No braces for single lines seems
to be prevailing here.

File src/superio/ite/it8728f/Kconfig:

https://review.coreboot.org/c/coreboot/+/81310/comment/1f3708bc_030eab77 :
PS4, Line 8: 	select SUPERIO_ITE_ENV_CTRL_FAN16_CONFIG
I see the bits are reserved in the datasheet. But this should definitely
be a separate patch. It's risky, the datasheet could be wrong.

File src/superio/ite/it8772f/superio.c:

https://review.coreboot.org/c/coreboot/+/81310/comment/753372c0_8a6501e1 :
PS4, Line 26: 		conf = dev->chip_info;
It's already initialized like this.