Tim Wawrzynczak 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:
(17 comments)
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 28: static struct resource_config *alloc_resource_config(void) : { : return (struct resource_config *) malloc(sizeof(struct resource_config)); : } : this is unnecessary, see comment below on line #625
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 42: if (res_config) : return res_config->action; : else : return UNKNOWN_ACTION; : } nit: ternary operator would be shorter, e.g. return res_config ? res_config->action : UNKNOWN_ACTION
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 56: if (res_config) : return res_config->type; : else : return UNKNOWN_CTRL; ternary would be shorter
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 65: if (res_config) Here you either need to check that `type` is set to CLOCK, and/or set it as well, or else it could be an undefined access of res_config->clk_conf. if (res_config) { res_config->type = CLOCK; res_config->clk_conf = clk_conf; }
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 533: switch (type) { : case IMGCLK: : clk_config = resource_get_clk_config(res_config); : res_id = clk_config->clknum; : break; : case GPIO: : gpio_config = resource_get_gpio_config(res_config); : res_id = gpio_config->gpio_num; : break; : default: : printk(BIOS_ERR, "Unsupported power operation: %x\n" : "OS camera driver will likely not work", type); : return; : } : : res_index = get_resource_index(res_id, type); you can push the logic for getting res_id into get_resource_index(), i.e., this chunk can just be:
res_index = get_resource_index(res_config, type), where get_resource_index can handle the `type` from res_config
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 550: if (res_index != -1) { : switch (action) { : case ENABLE: : snprintf(method_name, sizeof(method_name), ENABLE_METHOD_FORMAT, : res_index); : break; : case DISABLE: : snprintf(method_name, sizeof(method_name), DISABLE_METHOD_FORMAT, : res_index); : break; : default: : 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); : return; : } suggestion: a helper function e.g., `get_op_method_namestring()` would clean this up here
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 587: acpigen_write_byte nit: it's safer (and easier) to use acpigen_write_integer()
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 588: acpigen_write_byte nit: it's safer (and easier) to use acpigen_write_integer()
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 591: acpigen_write_byte nit: it's safer (and easier) to use acpigen_write_integer()
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 584: clk_config = resource_get_clk_config(res_config); : if (action == ENABLE) { : acpigen_emit_namestring("MCON"); : acpigen_write_byte(clk_config->clknum); : acpigen_write_byte(clk_config->freq); : } else if (action == DISABLE) { : acpigen_emit_namestring("MCOF"); : acpigen_write_byte(clk_config->clknum); : } else { : acpigen_write_debug_string("Unsupported clock action"); : printk(BIOS_ERR, "Unsupported clock action: %x\n" : "OS camera driver will likely not work", action); : } suggestion: a helper function, e.g. `add_clk_op(const struct clk_config*)` would make this function shorter
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 600: gpio_config = resource_get_gpio_config(res_config); : if (action == ENABLE) { : acpigen_soc_set_tx_gpio(gpio_config->gpio_num); : } else if (action == DISABLE) { : acpigen_soc_clear_tx_gpio(gpio_config->gpio_num); : } else { : acpigen_write_debug_string("Unsupported GPIO action"); : printk(BIOS_ERR, "Unsupported GPIO action: %x\n" : "OS camera driver will likely not work\n", action); : } : break; suggestion: a helper function, e.g. `add_gpio_op(const struct gpio_config*)` would make this function shorter
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 625: struct resource_config *res_config = alloc_resource_config(); You can just create a e.g. `struct resource_config res_config` on the stack, and then just pass it below like so: `resource_set_*_config(&res_config, &config->...);` .
`res_config` does not escape this function's lifetime, so it's safe to keep on the stack.
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 628: i < MAX_PWR_OPS good catch 😊
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 673: _ON method_name ?
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 662: snprintf(method_name, sizeof(method_name), ENABLE_METHOD_FORMAT, : res_mgr.cnt); : acpigen_write_method_serialized(method_name, 0); : acpigen_write_if_lequal_namestr_int(varname, 0); : resource_set_ctrl_type(res_config, seq->ops[i].type); : resource_set_action_type(res_config, ENABLE); : add_power_operation(res_config); : acpigen_pop_len(); /* if */ : : acpigen_emit_byte(INCREMENT_OP); : acpigen_emit_namestring(varname); : acpigen_pop_len(); /* _ON */ suggestion: a helper function, e.g. `write_enable_method()` would be nice
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 687: _ON method_name?
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 675: snprintf(method_name, sizeof(method_name), DISABLE_METHOD_FORMAT, : res_mgr.cnt); : acpigen_write_method_serialized(method_name, 0); : acpigen_emit_byte(DECREMENT_OP); : acpigen_emit_namestring(varname); : : acpigen_write_if_lequal_namestr_int(varname, 0); : resource_set_ctrl_type(res_config, seq->ops[i].type); : resource_set_action_type(res_config, DISABLE); : add_power_operation(res_config); : acpigen_pop_len(); /* if */ : : acpigen_pop_len(); /* _ON */ suggestion: a helper function, e.g. `write_disable_method()` would be nice