[coreboot-gerrit] Change in coreboot[master]: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )

Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor
......................................................................

Patch Set 10:

(5 comments)

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.

https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... 
PS10, Line 46: (void *)
See above.

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.

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

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 ?