Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41624 )
Change subject: drivers/intel/mipi_camera: Support adding camera power resource ......................................................................
Patch Set 14:
(17 comments)
https://review.coreboot.org/c/coreboot/+/41624/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41624/12//COMMIT_MSG@7 PS12, Line 7: drivers/intel/mipi_camera: Support for adding camera power resource
Please make it a statement by adding a verb (in imperative mood): […]
Done
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 428: int
unsigned int
Done
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 […]
I will consider moving acpigen_call_method to separate patch and use acpigen helper directly here.
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 445: printk(BIOS_ERR, "Unsupported clock action: %x\n",
Please add the consequence for the user. […]
Done
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 458: printk(BIOS_ERR, "Unsupported GPIO action: %x\n",
Please add the consequence for the user. […]
Done
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 464: printk(BIOS_ERR, "Unsupported power operation: %x\n", seq->ops[i].type);
Please add the consequence for the user. […]
Done
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. […]
we are already checking for 0 in if condition line 511. If config->pr0 is not set in devicetree it will have default value 0 it will never be NULL. JSL is always using different custom name for CAM0 and CAM1
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 537: acpigen_write_store_op_to_namestr(0, "STA"); This implementation is based on some of the older platforms PowerResource. Checking internally if this is required.
https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/poppy/... https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/poppy/...
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: […]
I think this can be removed. I will test once and remove it from SSDT generation patch.
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. […]
Done
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 81: 2
symbolic constant please
Done
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 84: gp_ctrl_panel
gpio_... […]
Done
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 85: 4
symbolic constant please
Done
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].
struct gp_ct can be extended to include other properties related to GPIO in the future.
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 applie […]
This is index to items in clock_ctrl_panel or gp_ctrl_panel
https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_cam... PS12, Line 91: enum action_type action;
Action type is not a good name? Why can’t a bool be used, and named `enable`?
There are plan to extend the operations types, so we might add more actions other than ENABLE/DISABLE. eg, we might add READ and WRITE incase if we start handling register operations.
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
Current we do not need and pre or post delay as there is a delay already part of the operation_type. Removing it form this structure.