Change in coreboot[master]: drivers/intel/mipi_camera: Add guard to protect camera resources
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43003 )
Change subject: drivers/intel/mipi_camera: Add guard to protect camera resources ......................................................................
Patch Set 6:
(14 comments)
https://review.coreboot.org/c/coreboot/+/43003/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43003/5//COMMIT_MSG@7 PS5, Line 7: drivers/intel/mipi_camera: Add guard variables to protect shared resource : : This change updates the mipi_camera driver to handle shared power resource : between multiple cameras. This is achieved by adding a guard variable and : methods manipulate the guard variable and guard the call to actual method : while enables or disables the resource. This protects the shared lines : from being enabled or disabled multiple times while the resources are : being used by multiple cameras. :
reflow for 72 characters wide
Done.
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 26: .cnt = 0
it should get initialized to 0 as it's a file-level static variable, so . […]
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 477: printk(BIOS_ERR, "Unsupported power operation: %x\n", type); : printk(BIOS_ERR, "OS camera driver will likely not work");
nit: this can be combined into one printk call: […]
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 485: if (action == ENABLE) { : snprintf(method_name, sizeof(method_name), ENABLE_METHOD_FORMAT, : res_index); : } else if (action == DISABLE) { : snprintf(method_name, sizeof(method_name), DISABLE_METHOD_FORMAT, : res_index); : } else { : snprintf(method_name, sizeof(method_name), UNKNOWN_METHOD_FORMAT, : res_index); : acpigen_write_debug_string("Unsupported action"); : printk(BIOS_ERR, "Unsupported resource action: %x\n", action); : }
a switch statement may be more clear on intention here. […]
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 502: uint8_t
enum action_type
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 502: uint8_t
enum ctrl_type
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 502: void *res_config
Sugnan, I'm not a big fan of the void* for res_config... […]
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 569: printk(BIOS_ERR, "Unsupported power operation: %x\n", seq->ops[i].type); : printk(BIOS_ERR, "OS camera driver will likely not work\n");
nit: you can squash this into one printk call
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 571: res_config = NULL; : break; : } : : if (res_config == NULL) : continue;
just return from within the `default:` block, I think it's easier to see the intent.
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 582: printk(BIOS_ERR, "Unable to add guarded camera resource\n"); : printk(BIOS_ERR, "OS camera driver will likely not work\n");
nit: you can squash this into one printk call
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 596: acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_namestring(varname); : acpigen_write_integer(0);
acpigen_write_if_lequal_namestr_int
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 602: : acpigen_emit_byte(ADD_OP); : acpigen_emit_namestring(varname); : acpigen_emit_byte(0x01); : acpigen_emit_namestring(varname); : acpigen_pop_len(); /* _ON */
there is an INCREMENT_OP 😊
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 612: acpigen_emit_byte(SUBTRACT_OP); : acpigen_emit_namestring(varname); : acpigen_emit_byte(0x01); : acpigen_emit_namestring(varname);
DECREMENT_OP
Done
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 616: acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_namestring(varname); : acpigen_write_integer(0);
acpigen_write_if_lequal_namestr_int
Done
-
Sugnan Prabhu S (Code Review)