Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41607 )
Change subject: drivers/intel/mipi_camera: Handle acpi_name and common code ......................................................................
Patch Set 32:
(20 comments)
https://review.coreboot.org/c/coreboot/+/41607/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41607/17//COMMIT_MSG@21 PS17, Line 21: in to
Done
Ack
https://review.coreboot.org/c/coreboot/+/41607/28//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41607/28//COMMIT_MSG@10 PS28, Line 10: , if acpi_name is not set in the devicetree and
@Paul, the second statement "moves some of the common code to separate methods. […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/6/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41607/6/src/drivers/intel/mipi_came... PS6, Line 188: /* Method (_DSM, 4, NotSerialized) */ : acpigen_write_method("_DSM", 0x4); : : /* ToBuffer (Arg0, Local0) */ : acpigen_write_to_buffer(ARG0_OP, LOCAL0_OP); : : /* If (LEqual (Local0, ToUUID(uuid))) */ : acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(LOCAL0_OP); : acpigen_write_uuid("822ace8f-2814-4174-a56b-5f029fe079ee"); : acpigen_write_return_string(config->sensor_name ? config->sensor_name : "UNKNOWN"); : acpigen_pop_len(); /* If */ : : /* If (LEqual (Local0, ToUUID(uuid))) */ : acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(LOCAL0_OP); : acpigen_write_uuid("26257549-9271-4ca4-bb43-c4899d5a4881"); : /* ToInteger (Arg2, Local1) */ : acpigen_write_to_integer(ARG2_OP, LOCAL1_OP); : : /* If (LEqual (Local1, 1)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 1); : acpigen_write_return_integer((config->ssdb.vcm_type ? 1 : 0) + : (config->ssdb.rom_type ? 1 : 0) + 1); : acpigen_pop_len(); /* If Arg2=1 */ : : /* If (LEqual (Local1, 2)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 2); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | : ((uint32_t)i2c_addr) << 8 | 0x0); /* type 0 for sensor */ : acpigen_pop_len(); /* If Arg2=2 */ : : if (config->ssdb.vcm_type) { : /* If (LEqual (Local1, 3)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 3); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 1 for vcm */ : ((uint32_t)config->vcm_address) << 8 | 0x01); : acpigen_pop_len(); /* If Arg2=3 */ : } : if (config->ssdb.rom_type) { : /* If (LEqual (Local1, 3 or 4)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 3 + (config->ssdb.vcm_type ? 1 : 0)); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 2 for rom */ : ((uint32_t)config->rom_address) << 8 | 0x02); : acpigen_pop_len(); /* If Arg2=3 or 4 */ : } : : acpigen_pop_len(); /* If uuid */ : : /* Return (Buffer (One) { 0x0 }) */ : acpigen_write_return_singleton_buffer(0x0); : : acpigen_pop_len(); /* Method _DSM */
I checked internally and _DSM is used only by the Windows drivers. […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/6/src/drivers/intel/mipi_came... PS6, Line 445: if (config->dep) { : acpigen_write_name("_DEP"); : acpigen_write_package(1); : acpigen_emit_namestring(config->dep); : acpigen_pop_len(); : }
Is the kernel driver using _DEP for something unusual? _DEP is used to "designate device objects tha […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 42: struct acpi_dp *ep_table = NULL; : struct acpi_dp *port_table = NULL; : struct acpi_dp *remote = NULL;
acpi_dp_new_table is internally calling malloc which calls die incase if it unable to allocated any […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 48: strdup
As mentioned above strdup is calling malloc
Ack
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 50: acpi_dp_add_integer
acpi_dp_add_integer is calling acpi_dp_new which calls malloc
Ack
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 91: *config
camera_fill_ssdt is be called only if required configs are enabled in devicetree. […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 216: config
As mentioned above.
Ack
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 272: 0
It's zero for all the existing systems in coreboot, though the docs in the Linux kernel have example […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 303: /* endpoint? */
acpi_graph_get_remote_endpoint() seems to confirm my guess, where a comment says that port and endpo […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_cam... PS11, Line 315: 180
So far only nautilus's cam0.asl specifies a "rotation" and it uses 180.
Ack
https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_cam... PS20, Line 27: CIO2
might be nice to get this spelled out the first time.
Ack
https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_cam... PS20, Line 50: EP%u0
You look like you're being purposeful having the table named EP10 instead of EP01 for example. […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_cam... PS20, Line 92: camera_pld_defaults
suggestion: apply_pld_defaults or fill_defaults like you have below
Ack
https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_cam... PS20, Line 139: camera_dsm_return
suggestion: address_for_dev_type
Ack
https://review.coreboot.org/c/coreboot/+/41607/25/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41607/25/src/drivers/intel/mipi_cam... PS25, Line 101: const char *scope = acpi_device_scope(dev);
Now I have moved the IPU device outside camera device scope and due to this now the device that is p […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/chip.h:
https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_cam... PS17, Line 174: uint32_t nvm_readonly;
The final output is going to be int so it was kept as int. […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/chip.h:
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... PS12, Line 148: uint8_t num_freq_entries; /* # of elements in link_freq */
Any specific reason to change them to native. […]
Ack
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... PS12, Line 167: uint32_t nvm_size;
Both works, but If there is not a explicit length requirement, the reader would think, that there is […]
Ack