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

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar 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 6:

(4 comments)

https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... 
File src/soc/intel/cannonlake/thermal.c:

https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... 
PS6, Line 47: 	return (void *)(bar & ~PCI_BASE_ADDRESS_MEM_ATTR_MASK);
> PCI_BASE_ADDRESS_MEM_ATTR_MASK isn't necessary, the resource base doesn't contain attributes.

Ack

https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... 
PS6, Line 59: 	if (!config->pch_trip_temp) {
> config might be NULL

We will add one more if to check is config NULL or not. If yes, print error message and return.

https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... 
PS6, Line 90: 	if (!thermalbar)
> return of thermalbar not found. […]

Sure. Thanks for this finding.

https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... 
PS6, Line 95: ~0x1ff
> Please make this a #define'd MASK instead of magic numbers.

Ack