Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48885 )
Change subject: soc/intel/{skl,apl}, mb/*: unify dt panel settings ......................................................................
Patch Set 4: Code-Review+1
(2 comments)
I like the idea. I've noticed a few minor nits, which should be easy to address.
https://review.coreboot.org/c/coreboot/+/48885/4/src/mainboard/51nb/x210/dev... File src/mainboard/51nb/x210/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/48885/4/src/mainboard/51nb/x210/dev... PS4, Line 8: .backlight_pwm_hz = 200, This wasn't here before, so the value changes from 0 to 200. Is it intentional?
Another thing: I'd prefer to have the backlight PWM frequency after all the timing values, since the units are different (Hz vs. ms). This also matches the original order on the other devicetrees, as well as the declaration order on the new struct.
https://review.coreboot.org/c/coreboot/+/48885/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/graphics.h:
https://review.coreboot.org/c/coreboot/+/48885/4/src/soc/intel/common/block/... PS4, Line 15: /* Panel configuration for graphics_soc_panel_init */ nit: Strictly speaking, this isn't part of "SoC overrides": only functions can be overriden.
Another thing: maybe mention this is meant to be used in the devicetree?
/* Devicetree panel configuration for graphics_soc_panel_init */
-
Angel Pons (Code Review)