Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32549 )
Change subject: SKL: Add ACPI brightness control ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/#/c/32549/5/src/soc/intel/common/block/graphics/... File src/soc/intel/common/block/graphics/graphics.c:
https://review.coreboot.org/#/c/32549/5/src/soc/intel/common/block/graphics/... PS5, Line 38: } The only platform specific part seems to be the pointer in the chip config. So we could also move the implementation here and add a weak intel_igd_get_controller_info(dev) that returns NULL.
https://review.coreboot.org/#/c/32549/5/src/soc/intel/skylake/acpi/globalnvs... File src/soc/intel/skylake/acpi/globalnvs.asl:
https://review.coreboot.org/#/c/32549/5/src/soc/intel/skylake/acpi/globalnvs... PS5, Line 73: BRTL, 32, // 0x54 - 0x57 Brightness Level I don't see this used anywhere.
https://review.coreboot.org/#/c/32549/5/src/soc/intel/skylake/acpi/globalnvs... PS5, Line 74: CSTE, 16, // 0x58 - 0x59 Current display state : NSTE, 16, // 0x5a - 0x5b Next display state Why add this to GNVS? It seems they are only used in ASL code. So wouldn't it be the same to just add them as named variables? (e.g. `Name (CSTE, 0)`)
https://review.coreboot.org/#/c/32549/5/src/soc/intel/skylake/graphics.c File src/soc/intel/skylake/graphics.c:
https://review.coreboot.org/#/c/32549/5/src/soc/intel/skylake/graphics.c@128 PS5, Line 128: 0x2
Add info on this value as comment. If possible make this as #define.
The `dev` here should be the same as the `dev` passed to gma_ssdt() below...
https://review.coreboot.org/#/c/32549/5/src/soc/intel/skylake/graphics.c@130 PS5, Line 130: return NULL; ...and it should only be called when the device is enabled.