Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39360 )
Change subject: mb/google/dedede: Add camera support for WDoo ......................................................................
Patch Set 9:
(21 comments)
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... File src/mainboard/google/dedede/Kconfig:
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 78: # Select this option to enable camera ACPI support on the variant. Put this comment as a help string.
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 81: default n Keep it default n here. select VARIANT_HAS_ACPI_CAMERA under BOARD_GOOGLE_WADDLEDOO in Kconfig.name file.
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 173: Nit: Remove the extra space. Here and lines 175, 177, 179.
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 180: 0 Why PLTRST?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/include/baseboard/acpi/cam0.asl:
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 16: scope scope -> Scope
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 19: Method Always use a space after //. Here and everywhere. Always use period at the end of the comment.
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 19: of Nit: off
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 22: PP1200 Nit: PP2800. Here and PON
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 24: PP2800 Nit: PP1200. Here and PON
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 43: GPP_D12 Why assert WCAM RESET line for Front Camera? Shouldn't it be UCAM RESET line(GPP_D15) - here and rest of the file?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 51: //Enable PP1200 lane : STXS(GPP_D13) : //Enable PP2800 lane : STXS(GPP_D14) Can PON method be invoked instead of duplication?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 55: Sleep(5) Is that extra 5 ms sleep intentional?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 72: MCOF Why can't you turn off clock 0 outside the if block? Disable the power lanes if the other sensor is already off.
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 76: //Disable PP1200 lane : CTXS(GPP_D13) : //Disable PP2800 lane : CTXS(GPP_D14) POFF Method?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 82: } Shouldn't the UCAM reset line(GPP_D15) be deasserted after Powering off?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 132: 0x02 0x01
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/baseboard/include/baseboard/acpi/cam1.asl:
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 37: //Enable PP1200 lane : STXS(GPP_D13) : //Enable PP2800 lane : STXS(GPP_D14) PON Method?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 41: Sleep(5) Is this sleep intentional and required?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 58: MCOF(1) // Clock 1 Same comment as in the previous file.
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 63: CTXS(GPP_D13) : //Disable PP2800 lane : CTXS(GPP_D14) POFF Method?
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 67: } No need to de-assert WCAM RESET Line (GPP_D12)?