Attention is currently required from: Matt DeVillier, Michał Żygowski, MrChromebox, Nico Huber, Piotr Król.
Joel Linn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81310?usp=email )
Change subject: superio/ite: Unify it8772f with common code ......................................................................
Patch Set 5:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81310/comment/15e022bf_303fe69c : PS4, Line 7: it8722f
it87*7*2f
Done
https://review.coreboot.org/c/coreboot/+/81310/comment/3847aa89_aa99bc38 : PS4, Line 9: it8722f
it87*7*2f
Done
File src/mainboard/google/beltino/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/81310/comment/3fb18ed4_d791fc13 : PS4, Line 86: register "FAN2.mode" = "FAN_SMART_SOFTWARE"
IIUC, the old code would have left the PWM signal at 0% (datasheet […]
Yes, until the OS picks up fans will run at 50%. We could set it to 1% (as setting 0% would trigger the default value of 50% without further changes) but I am not sure if it's ok for the fan to be driven so low.
File src/superio/ite/common/env_ctrl.c:
https://review.coreboot.org/c/coreboot/+/81310/comment/dbdfcffc_8ab50d38 : PS4, Line 84: *
As by our coding style, no leading asterisk in the short comment form. e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/81310/comment/1fe2b9d2_d68b4588 : PS4, Line 86: reg != reg_new) {
Line break isn't needed, as we allow ~96 chars per line.
Done
https://review.coreboot.org/c/coreboot/+/81310/comment/a935cf06_99c545ca : PS4, Line 110: break;
Shouldn't we `return` here? […]
Done
https://review.coreboot.org/c/coreboot/+/81310/comment/8d3f4605_d758f697 : PS4, Line 186: if (!CONFIG(SUPERIO_ITE_IT8772F)) {
We shouldn't check for specific chips in common code. This should be a […]
Done
https://review.coreboot.org/c/coreboot/+/81310/comment/f8522c96_cbdaf3d2 : PS4, Line 313: if (CONFIG(SUPERIO_ITE_IT8772F)) { Note: I will remove this in the next patchset because contrary to the documentation which reserves these bits, they are enabled in the vendor firmware and seem to report some voltage with the it87 linux driver.
File src/superio/ite/it8728f/Kconfig:
https://review.coreboot.org/c/coreboot/+/81310/comment/147fe6e6_be7d3c54 : PS4, Line 8: select SUPERIO_ITE_ENV_CTRL_FAN16_CONFIG
I see the bits are reserved in the datasheet. But this should definitely […]
Done https://review.coreboot.org/c/coreboot/+/81499
File src/superio/ite/it8772f/superio.c:
https://review.coreboot.org/c/coreboot/+/81310/comment/4b4add7c_5b4c3357 : PS4, Line 26: conf = dev->chip_info;
It's already initialized like this.
Done