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 7:
(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
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 strdup()'d string in get_op_method_namestring(), you have it instead accept a `char *` that it can snprintf() into? It can't use sizeof() obviously either then and would need a length argument as well.
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 think?
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 think?
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
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
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 `#define ACPI_SIMPLE_NAMESTRING_ALLOC 5` which would imply you should allocate 5 bytes for a simple namestring (assuming it's nul-terminated)
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
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
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 642: struct const struct
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
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
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 659: struct const struct
https://review.coreboot.org/c/coreboot/+/43003/7/src/drivers/intel/mipi_came... PS7, Line 660: struct const struct
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 the first element of the enum, so that any data structures which contains that type and is zero-initialized (i.e., devicetree structures) will contain 'UNKNOWN' in that case and will not accidentally contain pseudo-real data. Same with ctrl_type