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 4:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/54132/comment/ffdc7705_88526e32 PS4, Line 7: device tree driver nit: usually called a "chip driver"
https://review.coreboot.org/c/coreboot/+/54132/comment/67c8c68d_663cd706 PS4, Line 14: extra tab
https://review.coreboot.org/c/coreboot/+/54132/comment/3efa6789_ca07cb05 PS4, Line 56: TSRD (Zero)) Not true anymore đ
File src/drivers/acpi/thermal_zone/Kconfig:
https://review.coreboot.org/c/coreboot/+/54132/comment/905696a7_ee787cae PS4, Line 5: A little help text here wouldn't hurt đ Maybe a reference to Chapter 11 of the specification ?
File src/drivers/acpi/thermal_zone/chip.h:
https://review.coreboot.org/c/coreboot/+/54132/comment/67803677_4899a056 PS4, Line 45: ( Tn - Tn-1 ) + _TC2 * (Tn - Tt) One parenthesized expression has extra spaces, please be consistent
File src/drivers/acpi/thermal_zone/thermal_zone.c:
https://review.coreboot.org/c/coreboot/+/54132/comment/152e36a5_8e19b3bc PS4, Line 13: #define TZ_DEVICE_PATH "\_TZ" : #define DEFAULT_TC1 2 : #define DEFAULT_TC2 5 : #define DEFAULT_TSP 10 nit: line up the right sides
https://review.coreboot.org/c/coreboot/+/54132/comment/a00e6d7f_f70fa479 PS4, Line 18: #define CELSIUS_TO_DECI_KELVIN(temp_c) ((temp_c) * 10 + 2731) : #define SECONDS_TO_DECI_SECONDS(s) ((s) * 10) nit: line these right sides up too
https://review.coreboot.org/c/coreboot/+/54132/comment/42bd0d8c_1c468653 PS4, Line 23: char *name; : : if (dev->path.type != DEVICE_PATH_GENERIC) : return NULL; : : name = malloc(ACPI_NAME_BUFFER_SIZE); : snprintf(name, ACPI_NAME_BUFFER_SIZE, "TM%02X", dev->path.generic.id); : name[4] = '\0'; suggestion: many chip drivers with simple names like this just keep a static buffer here, e.g.:
``` static char name[ACPI_NAME_BUFFER_SIZE];
snprintf(....); return name; ```
also snprintf will make sure it ends with `\0`
https://review.coreboot.org/c/coreboot/+/54132/comment/db83bedb_5164fb8e PS4, Line 43: if (dev->bus->dev->path.type == DEVICE_PATH_ROOT) hmm, this is a little troublesome (coverity will definitely throw a fit here đ).
Is this even required anymore? This is an ACPI 1.0 namespace; it was deprecated in ACPI 2.0:
``` ACPI 1.0 Thermal Zone namespace. ACPI 1.0 requires all Thermal Zone objects to be defined under this namespace. Thermal Zone object definitions may now be defined under the _SB namespace. ACPI-compatible systems may maintain the _TZ namespace for compatibility with ACPI 1.0 operating systems. An ACPI-compatible namespace may define Thermal Zone objects in either the _SB or _TZ scope but not both. ```
https://review.coreboot.org/c/coreboot/+/54132/comment/02d0d771_72fd5e52 PS4, Line 94: if (!config->passive_config.time_constant_1 : || !config->passive_config.time_constant_2 : || !config->passive_config.time_sampling_period) { : acpigen_write_name_integer("_TC1", DEFAULT_TC1); : acpigen_write_name_integer("_TC2", DEFAULT_TC2); : acpigen_write_name_integer("_TSP", : SECONDS_TO_DECI_SECONDS(DEFAULT_TSP)); : } else { : acpigen_write_name_integer("_TC1", : config->passive_config.time_constant_1); : acpigen_write_name_integer("_TC2", : config->passive_config.time_constant_2); : acpigen_write_name_integer( : "_TSP", SECONDS_TO_DECI_SECONDS( : config->passive_config.time_sampling_period)); : } WDYT about treating each one separately for defaults?