Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... File src/soc/intel/common/block/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 29: get_thermal_config
Maybe rename this to get_thermal_trip_temp ?
Ack
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 41: thermal_config; : : thermal_config = get_thermal_config();
see above, maybe rename variable to thermal_trip_temp ?
Ack
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 56: pch_thermal_get_bar
Since this function is local to this file, and its only purpose is to get the BAR of the thermal dev […]
Looking at further optimization, thinking of removing this function pch_thermal_get_bar(). We can directly call 'res = find_resource(dev, PCI_BASE_ADDRESS_0)' in function pch_thermal_configuration() at below line 85.
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 84: : thermalbar = pch_thermal_get_bar(dev); : if (!thermalbar) { : printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return; : } Adding below code snippet as discussed in above comment at line 56.
+ struct resource *res; - thermalbar = pch_thermal_get_bar(); - if (!thermalbar) { + res = find_resource(dev, PCI_BASE_ADDRESS_0); + if (!res) { printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); return; } + /* Get the base address of the resource */ + thermalbar = res->base;
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 95: void
nit: consider uint16_t*
Ack
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 99: void
nit: consider uint16_t*
Ack