Attention is currently required from: Felix Singer, Michał Żygowski, Tim Wawrzynczak, Michał Kopeć, Arthur Heymans, Michael Niewöhner.
Angel Pons 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 6:
(7 comments)
File src/ec/clevo/it5570e/commands.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/1c40b227_d49b7c63 PS4, Line 41: while (recv_ec_data() != '$');
check the previous push. jenkins complained about unneeded braces and probably will complain with your approach, too.
Hmmm, let's see if CB:69077 makes Jenkins angry.
Actually, I don't really see the problem here
Granted, it's not *that* much of a problem, but Jenkins complaining about it is annoying.
https://review.coreboot.org/c/coreboot/+/68791/comment/e5e8f5c2_bbf7ec35 PS4, Line 45: *buf = '\0';
rewritten, can you check again, please?
Seems to be fine
https://review.coreboot.org/c/coreboot/+/68791/comment/aa886992_9781ad67 PS4, Line 119: else if (start > 100 || stop > 100) {
this was done for better readability. […]
Hmmm, let's find a loophole in the coding style. How about:
``` if (!state) { start = 0xff; stop = 0xff;
} else if (start > 100 || stop > 100) { printk(BIOS_ERR, "EC: invalid flexicharger settings: start/stop > 100%%\n");
} else if (start >= stop) { printk(BIOS_ERR, "EC: invalid flexicharger settings: start >= stop\n"); } ```
The braces make Jenkins happy, but the spacing is preserved.
https://review.coreboot.org/c/coreboot/+/68791/comment/bf84cf4a_973acd62 PS4, Line 122: else if (start >= stop) {
else should follow close brace '}' […]
See above
https://review.coreboot.org/c/coreboot/+/68791/comment/264d046b_09f0de54 PS4, Line 134: if (state > 3) {
3 should have been 2 \o/ […]
It looks much cleaner, thank you!
File src/ec/clevo/it5570e/commands.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/f5b2e0a6_239f7ca4 PS6, Line 120: printk(BIOS_ERR, "EC: invalid flexicharger settings: start/stop > 100%%\n"); Should the parameters be sanitized?
File src/ec/clevo/it5570e/ssdt.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/4d0d10e2_1bff3622 PS4, Line 119: goto pop;
We *want* to write the function in any case bc it's always called. […]
The idea is basically moving the if-block contents to a helper function:
``` static void ec_custom_fan_curve(const ec_config_t *config, int fan_cnt) { int curve_cnt = 0; /* Check curve count against fan count from EC */ for (int i = 0; i < IT5570E_MAX_FAN_CNT; i++) { if (*config->fan_curves[i].speed && *config->fan_curves[i].temperature) curve_cnt++; } if (curve_cnt != fan_cnt) { printk(BIOS_WARNING, "EC: Fan curve count (%d) does not match fan count (%d). " "Check your devicetree!\n", curve_cnt, fan_cnt); return; } /* * Check all curves. * Custom mode can only be enabled for all fans or none. Thus, all * custom curves must be valid before custom mode can be enabled. */ for (int i = 0; i < fan_cnt; i++) { if (!is_curve_valid(config->fan_curves[i])) { printk(BIOS_ERR, "EC: Fan %d curve invalid. Check your devicetree!\n", i); return; } } acpigen_write_debug_string("EC: Apply custom fan curve"); for (int i = 0; i < fan_cnt; i++) { write_fan_curve(config->fan_curves[i], i); } /* Enable custom fan mode */ acpigen_write_store_int_to_namestr(0x04, "FDAT"); acpigen_emit_namestring("SFCC"); acpigen_write_integer(0xd7); } ```