Attention is currently required from: Felix Singer, Nico Huber, Paul Menzel, Michael Niewöhner. Michał Żygowski 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:
(5 comments)
File ite_ecfw.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/8807b4b2_86501270 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 […]
The yway you explain it is confusing. `until external crystal is ready` it means it waits for external crystal when the bit is 1. Why should I reflect negative logic in the naming? When the bit is 0, then it DOESN'T wait for external crystal. Unless I am missing something in your comment above with bit explanation.
File ite_ecfw.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/d0f609e3_26ca98f9 PS17, Line 142: shutdown
isn't it `force_off` or `wdg_reset`?
Shutdown of the programmer not the EC. and Yes, it is wdg reset command that is sent to EC.
https://review.coreboot.org/c/flashrom/+/55715/comment/33a726ac_38c5bd24 PS17, Line 208: 0
maybe add enum or define for the type(s)?
Gladly. Just supply me with the values then please.
https://review.coreboot.org/c/flashrom/+/55715/comment/1be76ca3_6d53289d 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?
We probe for supported board anyway. If someone proceeds here with an unsupported mainboard, he has been warned. Also we simply probe Super I/O for the context purposes to alter the flashing process for IT5570 if needed. Do we know all Super I/O models that support this programmer? I don't mind adding them if I know more models that 100% support this flash update process.
https://review.coreboot.org/c/flashrom/+/55715/comment/c59d24f9_3bc1fd39 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?
Kind of unlikely to fail registering a shutdown. Almost every programmer has this call secured in an IF statement and handles errors. In our case even if we fail to register the shutdown we call it unconditionally in ite_ecfw_init_exit. Or do we know any other way of getting out of flashing mode of the EC?