Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41607 )
Change subject: drivers/intel/mipi_camera: camera SSDT generation ......................................................................
Patch Set 12:
(10 comments)
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 40: = 0;
will update this in next patch.
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 54: struct acpi_dp *lanes = acpi_dp_new_table("data-lanes"); : uint32_t j;
in coreboot, we usually declare all local variables at the top of the function.
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 94: if (!config->pld.ignore_color) : config->pld.ignore_color = 1; : : if (!config->pld.visible) : config->pld.visible = 1; : : if (!config->pld.vertical_offset) : config->pld.vertical_offset = 0xffff; : : if (!config->pld.horizontal_offset) : config->pld.horizontal_offset = 0xffff; : : /* : * PLD_PANEL_TOP has a value of zero, so the following will change any instance : * of PLD_PANEL_TOP to PLD_PANEL_FRONT unless disable_pld_defaults is set. : */ : if (!config->pld.panel) : config->pld.panel = PLD_PANEL_FRONT; : : /* : * PLD_HORIZONTAL_POSITION_LEFT has a value of zero, so the following will : * change any instance of that value to PLD_HORIZONTAL_POSITION_CENTER unless : * disable_pld_defaults is set. : */ : if (!config->pld.horizontal_position) : config->pld.horizontal_position = PLD_HORIZONTAL_POSITION_CENTER; : : /* : * The desired default for |vertical_position| is PLD_VERTICAL_POSITION_UPPER, : * which has a value of zero so no work is needed to set a default. The same : * applies for setting |shape| to PLD_SHAPE_ROUND. : */
suggestion: break this out into a separate function
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 139: 0x4
4
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 148: 822ace8f-2814-4174-a56b-5f029fe079ee
a symbolic constant with a nice name would be helpful
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 156: 26257549-9271-4ca4-bb43-c4899d5a4881
a symbolic constant with a nice name would be helpful
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 167: acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | : ((uint32_t)i2c_addr) << 8 | 0x0); /* type 0 for sensor */ : acpigen_pop_len(); /* If Arg2=2 */ : : if (config->ssdb.vcm_type) { : /* If (LEqual (Local1, 3)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 3); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 1 for vcm */ : ((uint32_t)config->vcm_address) << 8 | 0x01); : acpigen_pop_len(); /* If Arg2=3 */ : } : : if (config->ssdb.rom_type) { : /* If (LEqual (Local1, 3 or 4)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 3 + (config->ssdb.vcm_type ? 1 : 0)); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 2 for rom */ : ((uint32_t)config->rom_address) << 8 | 0x02); : acpigen_pop_len(); /* If Arg2=3 or 4 */
suggestion: add a helper function for creating these return values, with maybe an enum per type, sen […]
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 229: if (!config->disable_ssdb_defaults) { : if (!config->ssdb.bdf_value) : config->ssdb.bdf_value = PCI_DEVFN(CIO2_PCI_DEV, CIO2_PCI_FN); : : if (!config->ssdb.platform) : config->ssdb.platform = PLATFORM_SKC; : : if (!config->ssdb.flash_support) : config->ssdb.flash_support = FLASH_DISABLE; : : if (!config->ssdb.privacy_led) : config->ssdb.privacy_led = PRIVACY_LED_A_16mA; : : if (!config->ssdb.mipi_define) : config->ssdb.mipi_define = MIPI_INFO_ACPI_DEFINED; : : if (!config->ssdb.mclk_speed) : config->ssdb.mclk_speed = CLK_FREQ_19_2MHZ; : : if (!config->ssdb.lanes_used) { : cio2_config = cio2 ? cio2->chip_info : NULL; : : if (!cio2_config) { : printk(BIOS_ERR, "Failed to get CIO2 config\n"); : } else if (cio2_config->device_type != INTEL_ACPI_CAMERA_CIO2) { : printk(BIOS_ERR, "Device type isn't CIO2: %u\n", : (u32)cio2_config->device_type); : } else if (config->ssdb.link_used >= cio2_config->cio2_num_ports) { : printk(BIOS_ERR, "%u exceeds CIO2's %u links\n", : (u32)config->ssdb.link_used, : (u32)cio2_config->cio2_num_ports); : } else { : config->ssdb.lanes_used = : cio2_config->cio2_lanes_used[config->ssdb.link_used]; : } : } : }
suggestion: same here, separate this out into a separate function
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 298: remote_name = "\_SB.PCI0.CIO2";
Could you add this to the rest of the defaults?
Done
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 566: if (config->device_type == INTEL_ACPI_CAMERA_CIO2) : return "CIO2"; : else if (config->device_type == INTEL_ACPI_CAMERA_IMGU) : return "IMGU"; : else if (config->device_type == INTEL_ACPI_CAMERA_PMIC) : return "PMIC"; : : switch (config->device_type) { : case INTEL_ACPI_CAMERA_SENSOR: : prefix = "CAM"; : break; : case INTEL_ACPI_CAMERA_VCM: : prefix = "VCM"; : break; : case INTEL_ACPI_CAMERA_NVM: : prefix = "NVM"; : break; : default: : prefix = "ERR"; /* Error */ : break; : }
This can just be one switch statement; you can just return from the cases that have all 4 characters […]
Done
-
Sugnan Prabhu S (Code Review)