Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 11:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/chip.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... PS11, Line 38: pch_trip_temp
pch_thermal_trip ?
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/i... File src/soc/intel/common/pch/include/thermal.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/i... PS11, Line 1: /*
shouldn't this file move into soc/intel/common/pch/include/intelpch/thermal.h ? […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/Kconfig:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 5: This option allows to configure PCH thermal registers.
append at end of this line "for supported PCH"
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 25: #include "thermal.h"
#include <intelblocks/thermal.h> […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 27:
either use tab or space, don't mix please
After define string I use space and after the macro name I use tab in this file.
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 74:
make get_thermal_config one static as its been local already
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 90: printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return;
How you are making sure that BAR would have been allocated already ? if you don;t have plan to alloc […]
00:0x12.0 being used in https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/...