Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG@11 PS1, Line 11: to match prototypes in i915.h and avoid conflicts.
looks like ironlake, sandybridge, and haswell all implement gtt_{read,write,poll} in their gma. […]
Well, if it's a clean solution to add to a pile depends on its looks. To me it looks like a pile of technical debt. And I fear if we export functions as an API, people might think that was done on purpose and use it (chances are low in this case, though).
It's not your fault, and I know you just wanted one prototype and ran into the `i915.h` mess. So I'll leave it be.
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... PS2, Line 635: gfx This can't be NULL (unless `chip` is negative). You can read the
&chip->gfx
above as
(const i915_gpu_controller_info *)((const uint8_t *) chip + offsetof(struct soc_intel_broadwell_config, gfx))
So it's at least the offset of `gfx` inside the config struct.
But we should check `chip` for NULL, e.g.
const struct soc_intel_broadwell_config *chip = device->chip_info;
if (chip) drivers_intel_gma_display_ssdt_generate(&chip->gfx);