Furquan Shaikh 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)
Posting some comments for problems that I see with current implementation. I haven't gone through the entire change in detail, but we need to move to SSDT ASAP.
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?
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. We cannot keep adding configs for each variant in this file.
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?
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 195: If ((STA == One)) Same question as above. Do you see _OFF getting called multiple times?
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 198: Sleep(1) /* t0+t1 */ Why is this sleep required?
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. There can be different variants of volteer which either do not use MIPI camera or use a very different configuration than volteer. This is not going to work.