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 6:
(11 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68791/comment/07b3ea9f_19c95232 PS4, Line 28:
forgot one issue: second fan on NV4x doesn't spin for unknown reasons
added
File src/ec/clevo/it5570e/acpi/common.asl:
https://review.coreboot.org/c/coreboot/+/68791/comment/ac415595_cf5e05f8 PS4, Line 3: #ifndef EC_GPE_SCI : #error EC_GPE_SCI must be defined by mainboard. : #endif : #ifndef EC_GPE_PWRB : #error EC_GPE_PWRB must be defined by mainboard. : #endif : #ifndef EC_GPE_SLPB : #error EC_GPE_PWRB must be defined by mainboard. : #endif : #ifndef EC_GPE_LID : #error EC_GPE_LID must be defined by mainboard. : #endif
This driver is vendor-specific anyways and all devices I've seen so far have it
Done
File src/ec/clevo/it5570e/commands.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/4b8876d2_27e4ba86 PS4, Line 36: goto end;
Instead of a `goto`, how about being a bit more explicit? […]
Done
https://review.coreboot.org/c/coreboot/+/68791/comment/4cb33433_2c32e710 PS4, Line 45: *buf = '\0';
thank you, I'll check/rework this
rewritten, can you check again, please?
https://review.coreboot.org/c/coreboot/+/68791/comment/fa62b636_ba095b31 PS4, Line 134: 3 3 should have been 2 \o/
https://review.coreboot.org/c/coreboot/+/68791/comment/ff42c047_6b950383 PS4, Line 134: if (state > 3) {
enum?
3 should have been 2 \o/
just a single value (2 = OPTION_CAMERA_KEEP) is used here... I've reworked this to use a enum but, that just increases unused LOC *shrug*
https://review.coreboot.org/c/coreboot/+/68791/comment/f49ab633_5ce4b3d9 PS4, Line 136: %d
%u for unsigned
done; fixed some others as well
File src/ec/clevo/it5570e/early_init.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/f9e80b32_63c7f9c3 PS4, Line 19: int
hmm, I think I had tried bool but got warnings... […]
bool works fine, no idea...
File src/ec/clevo/it5570e/ssdt.c:
https://review.coreboot.org/c/coreboot/+/68791/comment/5f70e50a_0b66ddf7 PS4, Line 31: return; heh, this comes from my nv41 testing branch... looks like I cherry-picked the wrong commit... -> removed
https://review.coreboot.org/c/coreboot/+/68791/comment/515ffa22_1cdfc6eb PS4, Line 45: (float)
yup, the fan curves work. […]
Test:
``` uint16_t xxx = (float)(get_uint_option("kbled_timeout", 15)-1) / 25.0 * 255 / 100 * 16.0; uint16_t yyy = (get_uint_option("kbled_timeout", 15)-1) / 25 * 255 / 100 * 16; uint16_t zzz = (get_uint_option("kbled_timeout", 15)-1) * 255 * 16 / 100 / 25;
printk(BIOS_DEBUG, "FLOATTEST %u\n", xxx); printk(BIOS_DEBUG, "FLOATTEST %u\n", yyy); printk(BIOS_DEBUG, "FLOATTEST %u\n", zzz); ```
kbled_timeout = 10
Result:
``` [DEBUG] FLOATTEST 14 [DEBUG] FLOATTEST 0 [DEBUG] FLOATTEST 14 ```
Looks fine to me. yyy of course becomes zero bc int(9/25)=0.
The calculation could be done without floating point (not sure what gcc does, though...), like in zzz, by reordering like this:
``` uint16_t ramp; ... ramp = 255 * 16 * (curve.speed[i + 1] - curve.speed[i]) / (curve.temperature[i + 1] - curve.temperature[i]) / 100; ```
https://review.coreboot.org/c/coreboot/+/68791/comment/11d04ed2_aed95982 PS4, Line 68: acpigen_emit_simple_namestring
need to test this... […]
acpigen_emit_namestring works fine; done