Daniel Kang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 )
Change subject: src/mb/google/volteer: Add camera ACPI configuration ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/acpi/mipi_camera.asl:
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 166: If ((STA == Zero))
Why is this check required? Does the _ON routine get called multiple times?
I thought this would be needed in case the two cameras share the same power rail. But now I don't think this is needed. Will remove this.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 172: #if CONFIG(BOARD_GOOGLE_VOLTEER)
This check should not be here. It is not at all scalable. […]
This will be handled in SSDT properly.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 180: Sleep(2) /* reset pulse width */
Does this match the datasheet?
In chapter 2.7 of the data sheet, "The reset pulse width should be greater than or equal to 2ms." I added this delay to meet this.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 195: If ((STA == One))
Same question as above. […]
I thought this would be needed in case the two cameras share the same power rail. But now I don't think this is needed. Will remove this.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 198: Sleep(1) /* t0+t1 */
Why is this sleep required?
The min values of t1 is 512 XVCLK cycles, so this is less than 1ms but added just 1.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 50: /* Camera */ : #include <soc/intel/tigerlake/acpi/ipu.asl> : #include "acpi/mipi_camera.asl"
This is not correct. You cannot just unconditionally add it here. […]
This will be handled in SSDT properly.