Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/9/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/9/src/drivers/intel/mipi_came... PS9, Line 264: if (!config->remote_name) { : if (cio2) : config->remote_name = acpi_device_path(cio2); : else : config->remote_name = DEFAULT_REMOTE_NAME; : } I think a potential issue got introduced during the refactoring. The original code for populating the "remote-endpoint" table had this defaulting of a NULL remote_name be unconditional so that NULL is never passed as part of calling:
acpi_dp_add_reference(remote, NULL, config->remote_name);
The code has now been moved into a camera_fill_sensor_defaults() function that's only called when the unrelated 'config->disable_ssdb_defaults' is false. It seems like this defaulting should occur unconditionally in the parent function (in which case this function can be called 'camera_fill_ssdb_defaults()'), or the checking of 'disable_ssdb_defaults' should instead occur in this function (e.g., do the 'config->remote_name' code first, followed by a 'if (!config->disable_ssdb_defaults) return;' followed by the rest of the function that handles ssdb.