Attention is currently required from: Felix Singer, Michał Żygowski, Tim Wawrzynczak, Michał Kopeć, Angel Pons, Arthur Heymans.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68791 )
Change subject: ec/clevo/it5570e: add driver for EC used on various Clevo laptops ......................................................................
Patch Set 7:
(9 comments)
File src/ec/clevo/it5570e/commands.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/e3df0c50_b3f10846 PS4, Line 41: while (recv_ec_data() != '$');
Looks like CB:69077 is fine :O Personally, I still prefer the variant without `do {}`. […]
well, at least I tried... the linter checks regarding if/do/while are pretty complex, so I went your way ;)
https://review.coreboot.org/c/coreboot/+/68791/comment/a457d3df_22949adf PS4, Line 119: else if (start > 100 || stop > 100) {
Hmmm, let's find a loophole in the coding style. How about: […]
Done
https://review.coreboot.org/c/coreboot/+/68791/comment/d3c099d0_3fbaea4b PS4, Line 122: else if (start >= stop) {
See above
Done
File src/ec/clevo/it5570e/commands.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/0eaffcf6_b2731e92 PS6, Line 120: printk(BIOS_ERR, "EC: invalid flexicharger settings: start/stop > 100%%\n");
ouch, the plan was to just return bc I'm not a fan of trying to guess what the user intended. […]
Done
File src/ec/clevo/it5570e/ec.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/02a640fe_2c3086fd PS4, Line 63: version = ec_read_fw_version();
uhm, the data is read during runtime,
done as discussed; was just a misunderstanding on my side
https://review.coreboot.org/c/coreboot/+/68791/comment/401ea0a4_a1852f23 PS4, Line 94: power_unit = 1 << (msr_read(MSR_PKG_POWER_SKU_UNIT) & 0xf);
I don't think so. This is being read at runtime. […]
done as discussed
https://review.coreboot.org/c/coreboot/+/68791/comment/8abee0c7_8c3109ac PS4, Line 124: if (dev->path.type == DEVICE_PATH_GENERIC && dev->path.generic.id == 0) {
braces {} are not necessary for any arm of this statement […]
Done
File src/ec/clevo/it5570e/ssdt.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/f835d8c6_437dc2d6 PS4, Line 45: (float)
Test: […]
-> done without fp
https://review.coreboot.org/c/coreboot/+/68791/comment/06e0a1df_139970c0 PS4, Line 140: for (int i = 0; i < fan_cnt; i++) {
not necessary but also not forbidden. […]
heh, I just realized that it's already mixed -> done