Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42469/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42469/12/src/drivers/intel/mipi_cam... PS12, Line 19: if (!config->pld.ignore_color) : config->pld.ignore_color = 1; : : if (!config->pld.visible) : config->pld.visible = 1;
Won't this effectively just always set ignore_color and visible to 1? Either the devicetree sets it […]
To keep one or both of these as zero someone would have to set disable_pld_defaults to essentially disable this function.
This defaulting scheme is a bit coarse but I couldn't think of a decent scheme to allow more fine-grain defaulting without adding a per-variable bool or special value that indicates whether that variable should be set to a default value or not. The current scheme is simpler and works for the currently-known boards (at least at the time I initially drafted the change) but two tradeoffs for the future are 1) if someone needs to disable/override the default for a variable then they have to disable defaulting (which now also means the value of the other variables will need to be explicitly set in config), and 2) if someone wants to add a new variable and/or change how the value of a variable gets set to a default then they may have to study all the configs that use mipi_camera to make sure they're not breaking/changing another board.
An example of this is how camera_fill_sensor_defaults() sets:
config->ssdb.platform = PLATFORM_SKC;
At the time all the boards were using skylake but now there will be a few others and there might be a better way to default this (e.g., #include a "soc/pci_devs.h" [or similar] that declares the default value for the platform).