Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 8:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 23: DPTF_GENERIC_PARTICIPANT_TYPE_TSR = 0x3, What about CPU(0x0) and Fan(0x4)?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 24: #ifndef CONFIG_DPTF_CPU_ADDR : CONFIG_DPTF_CPU_ADDR = 0x00040000ul, : #endif Isn't this the PCI device address for the CPU thermal device? Can we instead derive this from the struct device *dev? Are the thermal devices being added under this PCI device?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 68: acpigen_write_name_integer("_ADR", CONFIG_DPTF_CPU_ADDR); acpigen_write_ADR()?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 69: is_participant_used(config, DPTF_CPU) Is it a valid scenario to have DPTF_CPU as unused?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 92: acpigen_write_device("TCHG"); : acpigen_write_name("_HID"); : acpigen_emit_eisaid("INT3403"); : acpigen_write_name_integer("_UID", 0); : acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER); : acpigen_write_name_string("_STR", DEFAULT_CHARGER_NAME); : acpigen_write_STA(is_participant_used(config, DPTF_CHARGER) ? : ACPI_STATUS_DEVICE_ALL_ON : : ACPI_STATUS_DEVICE_ALL_OFF); Should this sequence be converted to a helper function -- dptf_write_generic_participant? It can be used for TCHG as well as TSR. Probably for TFN as well?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 104: 1 I believe this is starting from 1 because TCHG above uses UID 0? It might be a good idea to ensure that this uid account for all INT3403 devices so that if a new device gets added, we don't have to manually do the math.
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 112: acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_TSR); Why no "_STR" object?