Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41607 )
Change subject: drivers/intel/mipi_camera: Generate SSDT for camera ......................................................................
Patch Set 17: Code-Review+1
(6 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 into
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 14: {.type = IMGCLK, .index = (ind), .action = (act), .delay_ms = (delay) } Please add a space after { in the beginning.
https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_cam... PS17, Line 17: {.type = GPIO, .index = (ind), .action = (act), .delay_ms = (delay) } Ditto.
https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_cam... PS17, Line 125: * this is in unit of usec */ The coding style prefer no asterisk for concise multi-line comments.
https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_cam... PS17, Line 174: uint32_t nvm_readonly; That sounds like, it should be a boolean?
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 167: uint32_t nvm_size;
Any specific reason to change them to native. […]
Both works, but If there is not a explicit length requirement, the reader would think, that there is. The compiler and architecture use minimum length anyway [1]. (I know that coreboot code is small, so it doesn’t make a difference.)
[1]: http://notabs.org/coding/smallIntsBigPenalty.htm
-
Paul Menzel (Code Review)