Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43003 )
Change subject: drivers/intel/mipi_camera: Add guard variables to protect shared resource ......................................................................
Patch Set 5:
(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
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 .cnt=0 shouldn't be required, just `static struct camera_resource_manager res_mgr;`
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:
printk(BIOS_ERR, "Unsupported power operation: %x\n" "OS camera driver will likely not work", type);
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. Also, why not just return from the function if the action type is unknown?
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...
Could we maybe work with a tagged anonymous union instead? e.g.,
struct resource_config { enum action_type action; enum ctrl_type type; union { struct clk_config *clk_conf; struct gpio_config *gpio_conf; }; };
Technically it's not as type-safe (b/c of the tagged union), but if you add a few functions for creating them and getting values out (make_resource_config, etc.), then it could work.
I think it would greatly simplify this patch. WDYT?
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 502: uint8_t enum action_type
https://review.coreboot.org/c/coreboot/+/43003/5/src/drivers/intel/mipi_came... PS5, Line 502: uint8_t enum ctrl_type
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
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.
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
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
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 😊
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
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