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:
(5 comments)
Patch Set 10:
general suggestion, looks like this CL is similar to what we has in SKL/KBL here https://review.coreboot.org/22419
now it we decided to have such files in CNL and may be going forward every core design then why don't we move generic implementation into "src/soc/intel/common/pch/thermal". this code block meant for all common PCH started from gen6
Uploaded new patch set with this common code change.
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 34: void *
I think returning uintptr_t would be appropriate here.
Ack. Incorporated in latest patch set.
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 46: (void *)
See above.
Ack
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 52: static
Why is this static? It gets reassigned to the same pointer every time it's called.
Done
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 87: void *thermalbar = pch_thermal_get_bar(dev);
See above about uintptr_t
Ack
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 96: CATASTROPHIC_TRIP_POINT_VALUE
This is used as a mask, could you rename it CATASTROPHIC_TRIP_POINT_MASK ?
Done