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 8:
(15 comments)
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 490: struct
const struct
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 551: method_name = get_op_method_namestring(res_config, res_index);
Hm, since we don't need method_name to have a very long lifetime, what about, instead of returning a […]
I have moved acpigen_emit_namestring to get_op_method_namestring and renamed the function to add_guarded_method_namestring. This method is called from only one place. There is not much code in call_guarded_method unlike before when we made this change. Is it ok to move back code from the method add_guarded_method_namestring to call_guarded_method.
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 562: acpigen_emit_namestring("MCON");
I'm wondering if this should include a CondRefOf op to check for MCON's existence first? what do you […]
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 566: acpigen_emit_namestring("MCOF");
I'm wondering if this should include a CondRefOf op to check for MCOF's existence first? what do you […]
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 561: if (action == ENABLE) { : acpigen_emit_namestring("MCON"); : acpigen_write_integer(clk_config->clknum); : acpigen_write_integer(clk_config->freq); : } else if (action == DISABLE) { : acpigen_emit_namestring("MCOF"); : acpigen_write_integer(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); : }
switch-case conveys the intent more clearly, since `action`'s value is the only condition
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 579: : 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); : }
switch-case conveys the intent more clearly, since `action`'s value is the only condition
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 619: 5
We're using this `5` an awful lot, maybe we should add a constant into acpigen.h? Something like […]
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 627: char method_name[5], varname[5];
nit: declare on separate lines
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 631: acpigen_write_method_serialized(method_name, 0);
nit: blank line after snprintfs
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 642: struct
const struct
res_config is being modified by resource_set_action_type called in this function, so res_config cannot be const
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 644: char method_name[5], varname[5];
nit: declare on separate lines
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 648: acpigen_write_method_serialized(method_name, 0);
nit: blank line after snprintfs
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 659: struct
const struct
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 660: struct
const struct
Done
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/chip.h:
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 80: ENABLE = 1,
nit: for enums like this, with an 'UNKNOWN' option, it's often convenient to have that defined as th […]
Done