Change in coreboot[master]: drivers/acpi: Add device tree driver to generate thermal zone
Attention is currently required from: Raul Rangel, Furquan Shaikh. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54132 )
Change subject: drivers/acpi: Add device tree driver to generate thermal zone ......................................................................
Patch Set 1:
(7 comments)
File src/drivers/acpi/thermal_zone/chip.h:
https://review.coreboot.org/c/coreboot/+/54132/comment/4cd81c1f_494c6242 PS1, Line 13: /* Name of the thermal zone */ : const char *name; suggestion: since this is for _STR, maybe use `description` instead of `name` ? My first thought was that this was the ACPI name
https://review.coreboot.org/c/coreboot/+/54132/comment/50da6b48_20135236 PS1, Line 20: polling_period suggestion: suffix with `_ms`
https://review.coreboot.org/c/coreboot/+/54132/comment/30d1a4ba_f696c185 PS1, Line 28: acpi_thermal_zone_passive_config I don't think the struct name is required
https://review.coreboot.org/c/coreboot/+/54132/comment/b4483278_bf3fa1dd PS1, Line 51: acpigen_write_thermal_zone_get_temperature that is one heck of a name there 😊 suggestion: `mainboard_acpigen_write_tz_get_temp` ?
https://review.coreboot.org/c/coreboot/+/54132/comment/bfec19e7_49ccdff0 PS1, Line 52: drivers_acpi_thermal_zone_config I wonder if it's always necessary to pass this whole structure to this...
File src/drivers/acpi/thermal_zone/thermal_zone.c:
https://review.coreboot.org/c/coreboot/+/54132/comment/0585f761_be27322f PS1, Line 57: K deci-kelvin
https://review.coreboot.org/c/coreboot/+/54132/comment/306d9abe_4bcd177c PS1, Line 87: if (acpigen_write_thermal_zone_get_temperature(config) != CB_SUCCESS) : printk(BIOS_ERR, "Failed to write _TMP for %s.%s\n", scope, name); Does this callback need to have a return value? If it's just to print an error, it seems like the callback could print something more specific anyway?
-
Tim Wawrzynczak (Code Review)