[coreboot-gerrit] Change in ...coreboot[master]: soc/intel/cannonlake: Set correct temperature threshold for PCH Therm...

[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 correct temperature threshold for PCH Thermal Sensor
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/chip.h 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/chip.h@425 
PS3, Line 425: 	/* PCH Trip Temperature */
> What are the units?

Agreed.  When a variable contains a value that is in specific units, I often add a '_C' for example to indicate that the unit.

https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c 
File src/soc/intel/cannonlake/thermal.c:

https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@7... 
PS3, Line 78: ltt_value = (trip_temp + 50) * 2;
Macros to calculate this would be helpful; you could also then use them to define MAX_TRIP_TEMP, and perhaps DEFAULT_TRIP_TEMp as well.