Subrata Banik 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 12:
(8 comments)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
I don't completely agree that adding this IP to common/block would be a maintenance issue. […]
yes Furquan, i agree on logical splitting part, but here thermal is not treated as in general IP like other existed into common/block. And taking part into PCI emueration, its kind of chipset programming specific to thermal register.
I'm okay if you are advocating about moving this module from common/pch to common/block. But my point is that if you like to actually try to make further common code based on PCH alignment which is reasonable might exist inside common/pch. In you see long back, intel CPU used to get common code based on southbridge common.
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 33: int
pch_thermal_trip is defined as uint8_t. […]
Make it to uint8_t as pch_thermal_trip static uint8_t get_thermal_config(void)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 45: uint16_t thermal_config = get_thermal_config();
And here you're casting the uint8_t to an int to a uint16_t
uint8_t thermal_config = get_thermal_config();
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 78: struct device *dev = pcidev_path_on_root(PCH_DEVFN_THERMAL);
Prefer separating declaration from assignment, please.
struct device *dev; uintptr_t thermalbar;
dev = pcidev_path_on_root(PCH_DEVFN_THERMAL); if (!dev) { printk(BIOS_ERR, "ERROR: PCH_DEVFN_THERMAL device not found!\n"); return; }
thermalbar = pch_thermal_get_bar(dev);
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 84: uintptr_t
Prefer variable definitions at the top of the scope, please.
look at above
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 89: or you can also create a helper function like below
static uint16_t read16p(uintptr_t addr) { return read16((void *)addr); }
reg16 = read16p(thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT);
in that way you can avoid creating temp variable as thermalbar_pm
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 91: void *)thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT);
Pointer arithmetic on void* pointers is illegal in C.
you can create a local variable for thermal_pm
uintptr_t thermalbar_pm = thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT; reg16 = read16((void *)thermalbar_pm);
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 95: (void *)thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT
Pointer arithmetic on void* pointers is illegal in C.
write16((void *)thermalbar_pm, reg16);