Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Paul Menzel. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 )
Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ......................................................................
Patch Set 17:
(10 comments)
File ite_ecfw.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/4fe5e38e_bafe7818 PS14, Line 90: wait_for_ext_crystal the name is confusing; 1 for this bit means `use internal clock until external crystal is ready`, so correct would be `no_wait_for_ext_crystal` or `int_osc_until_crystal_ready`
https://review.coreboot.org/c/flashrom/+/55715/comment/1f76e870_8b33b4ad PS14, Line 105: constant? yes
https://review.coreboot.org/c/flashrom/+/55715/comment/632a2032_7558fb3c PS14, Line 112: enum { : ESPI_IF = 0xa4, : LPC_IF, : }; : Actually, bit 0 controls LPC vs eSPI, so we could also have 0xa4 static and a bitfield. having the static value in the enum values isn't wrong, though.
struct ite_string { ... uint8_t _a4_byte :7; uint8_t espi_lpc :1; ... }
https://review.coreboot.org/c/flashrom/+/55715/comment/0823cbb4_33c98059 PS14, Line 315: = (blocks_1_2 ? 0x94 : 0x85); we know the bits / have a struct. let's use it. what about sth like this?
https://review.coreboot.org/c/flashrom/+/55715/comment/530c08d2_4830b917 PS14, Line 324: 0xaa this means "disable", let's give it a name or comment at least
File ite_ecfw.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/c10f2bd9_9deaeb9c PS17, Line 142: shutdown isn't it `force_off` or `wdg_reset`?
https://review.coreboot.org/c/flashrom/+/55715/comment/9f415055_a5d3e18c PS17, Line 208: 0 maybe add enum or define for the type(s)?
https://review.coreboot.org/c/flashrom/+/55715/comment/b1c1c10b_6ed06605 PS17, Line 782: if (!ec_write_reg(0xf9, 0x20, EC_MAX_STATUS_CHECKS) || : !ec_write_reg(0xfa, 0x02, EC_MAX_STATUS_CHECKS) || : !ec_write_reg(0xfb, 0x00, EC_MAX_STATUS_CHECKS) || : !ec_write_reg(0xf8, 0xb1, EC_MAX_STATUS_CHECKS)) { : msg_perr("Unable to initialize controller.\n"); : return 1; : } : drop; done in line #208 already
https://review.coreboot.org/c/flashrom/+/55715/comment/45538c55_dec5a924 PS17, Line 813: probe_ite_superio_support(ctx_data); don't we want to probe the sio before sending any ec commands and exit if it isn't compatible?
https://review.coreboot.org/c/flashrom/+/55715/comment/af59b67b_80b736f1 PS17, Line 835: : if (register_shutdown(ite_ecfw_shutdown, ctx_data)) : goto ite_ecfw_init_exit; : do we really want to reset the device, when flashrom fails to register the shutdown callback?