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 18:
(7 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
Ack
Done
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 41: thermal_config; : : thermal_config = get_thermal_config();
Ack
Done
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 56: pch_thermal_get_bar
Looking at further optimization, thinking of removing this function pch_thermal_get_bar(). […]
Ack
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. […]
Done
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 29: static uint16_t read16p(uintptr_t addr) : { : return read16((void *)addr); : } : : static void write16p(uintptr_t addr, uint16_t val) : { : write16((void *)addr, val); : }
These are not necessary. […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 105: read16p
yes, Tim's point is valid as now you already has thermalbar_pm then we can get rid of read16p and d […]
Done
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 109: write16p
See above.
Done