Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41624 )
Change subject: drivers/intel/mipi_camera: Support for adding camera power resource ......................................................................
Patch Set 12:
(11 comments)
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 437: method_params[0] = config->clk_panel.clks[index].clknum; : method_params[1] = config->clk_panel.clks[index].freq; : acpigen_call_method("MCON", method_params, 2); This is why I don't particularly like acpigen_call_method(), because I don't find this any easier to read (and I'd argue less readable) than:
acpigen_emit_namestring("MCON"); acpigen_write_integer(config->clk_panel.clks[index].clknum); acpigen_write_integer(config->clk_panel.clks[]index].freq);
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 512: (config->pr0 This should check for again NULL and supply a default. Or is a custom name always required?
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 513: acpigen_write_name_integer("STA", 0); : acpigen_write_STA_ext("STA"); : : acpigen_write_method_serialized("_ON", 0); : acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_namestring("STA"); : acpigen_write_integer(0); : : add_power_resource(config, &config->on_seq); : : acpigen_write_store_op_to_namestr(1, "STA"); : acpigen_pop_len(); /* if */ : acpigen_pop_len(); /* _ON */ : : /* _OFF operations */ : acpigen_write_method_serialized("_OFF", 0); : acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_namestring("STA"); : acpigen_write_integer(1); : : add_power_resource(config, &config->off_seq); : : acpigen_write_store_op_to_namestr(0, "STA"); Why is the extra "STA" "variable" necessary? The OS should only call _ON and _OFF as appropriate, I don't think you need to guard against the OS calling it multiple times per suspend or resume.
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 599: acpigen_write_name("_DEP"); : acpigen_write_package(1); : acpigen_emit_namestring(config->dep); : acpigen_pop_len(); I still don't understand this. The ACPI spec says this about _DEP:
_DEP evaluates to a package and designates device objects that OSPM should assign a higher priority in start ordering due to future operation region accesses.
What operation region is being accessed from 2 places ?
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/chip.h:
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 69: clk_ct there could be a better name here, I'm not sure what the 'ct' refers to. maybe clk_config?
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 81: 2 symbolic constant please
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 84: gp_ctrl_panel gpio_... ?
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 85: 4 symbolic constant please
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 85: struct gp_ct gps[4]; is the struct for gp_ct necessary? could just have uint8_t gpios[4].
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 90: index what is the index for? Doesn't their relative order in operation_seq define the order they're applied in?
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 97: delay_ms should specify (in a comment or in the name) if it is pre- or post-op delay