Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41624 )
Change subject: drivers/intel/mipi_camera: Add camera power resource to SSDT ......................................................................
Patch Set 29:
(8 comments)
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 537: acpigen_write_store_op_to_namestr(0, "STA");
OS can guarantee for a specific device, _ON and _OFF are called appropriately. […]
Ack
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 513: acpigen_write_name_integer("STA", 0); : acpigen_write_STA_ext("STA"); : : acpigen_write_method_serialized("_ON", 0); : acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_namestring("STA"); : acpigen_write_integer(0); : : add_power_resource(config, &config->on_seq); : : acpigen_write_store_op_to_namestr(1, "STA"); : acpigen_pop_len(); /* if */ : acpigen_pop_len(); /* _ON */ : : /* _OFF operations */ : acpigen_write_method_serialized("_OFF", 0); : acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_namestring("STA"); : acpigen_write_integer(1); : : add_power_resource(config, &config->off_seq); : : acpigen_write_store_op_to_namestr(0, "STA");
Why is the extra "STA" "variable" necessary? The OS should only call _ON and _OFF as appropriate, I […]
Ack
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 599: acpigen_write_name("_DEP"); : acpigen_write_package(1); : acpigen_emit_namestring(config->dep); : acpigen_pop_len();
I think this can be removed. I will test once and remove it from SSDT generation patch.
Done
https://review.coreboot.org/c/coreboot/+/41624/18/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41624/18/src/drivers/intel/mipi_cam... PS18, Line 516: acpigen_write_name_integer
Yes it is required as per the camera team. Please find my earlier answer. […]
Ack
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/chip.h:
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 85: struct gp_ct gps[4];
struct gp_ct can be extended to include other properties related to GPIO in the future.
Ack
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 90: index
This is index to items in clock_ctrl_panel or gp_ctrl_panel
Done
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 91: enum action_type action;
There are plan to extend the operations types, so we might add more actions other than ENABLE/DISABL […]
Done
https://review.coreboot.org/c/coreboot/+/41624/4/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/chip.h:
https://review.coreboot.org/c/coreboot/+/41624/4/src/drivers/intel/mipi_came... PS4, Line 37: : struct clk_ct { : /* IMGCLKOUT_x being used for a port */ : uint32_t clknum; : /* frequency setting: 0:24Mhz, 1:19.2 Mhz */ : uint32_t freq; : } __packed; : : struct gp_ct { : uint32_t gp_num; : } __packed; : : struct clock_ctrl_panel { : struct clk_ct clks[2]; : } __packed; : : struct gp_ctrl_panel { : struct gp_ct gps[4]; : } __packed; : : struct operation_type { : enum ctrl_type type; : uint32_t index; : enum action_type action; : uint32_t delay_ms; : } __packed; : : struct operation_seq { : struct operation_type ops[5]; : uint32_t delay_ms[6]; : uint8_t ops_cnt; : } __packed;
I was planning to update clk_ct and gp_cnt to uint8_t but missed updating in last patch. […]
Done