Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41607 )
Change subject: drivers/intel/mipi_camera: camera SSDT generation ......................................................................
Patch Set 12:
(13 comments)
https://review.coreboot.org/c/coreboot/+/41607/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41607/12//COMMIT_MSG@7 PS12, Line 7: drivers/intel/mipi_camera: camera SSDT generation
Please make it a statement by adding a verb (in imperative mood). […]
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12//COMMIT_MSG@9 PS12, Line 9: Some boards & variants are using raw asl files to define the ACPI devices : (and related properties) required by Linux MIPI camera drivers. : The mipi_camera driver can provide a SSDB property, but it's : missing some properties needed for Linux drivers.
Please re-flow for 75 characters per line.
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12//COMMIT_MSG@12 PS12, Line 12: missing some properties needed for Linux drivers.
Please mention the properties.
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12//COMMIT_MSG@21 PS12, Line 21: asl
ASL
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12//COMMIT_MSG@22 PS12, Line 22:
Please mention one difference.
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... PS12, Line 42: uint32_t i, j;
Please use the native types `unsigned int` or `size_t`.
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... PS12, Line 607: prefix = "ERR"; /* Error */
Should a debug message be added for this case?
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... PS12, Line 612: know
knows?
I will update this in next patch.
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 131: * General settings that can be used for any type (but doesn't
don’t?
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... PS12, Line 132: * necessarily need to be used/specified for each device.
Missing closing ).
I will update this in next patch.
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 */
Please use native types.
Any specific reason to change them to native. Other structures in this file are already using non native types.
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... PS12, Line 163: * you can use zero for a value.
Please re-flow for at least 80 characters per line (maybe even 96).
I will update this in next patch.
https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_cam... PS12, Line 167: uint32_t nvm_size;
Can native types be used here?
Any specific reason to change them to native. Other structures in this file are already using non native types.