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 7:
(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
Done
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. […]
Done
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
Done
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 b […]
Done
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: […]
Done
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. […]
Done
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()
Done
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()
Done
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()
Done
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. […]
Done
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. […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 628: i < MAX_PWR_OPS
good catch 😊
Ack
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. […]
Done
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 673: _ON
method_name ?
Done
https://review.coreboot.org/c/coreboot/+/43003/6/src/drivers/intel/mipi_came... PS6, Line 687: _ON
method_name?
Done
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. […]
Done