Varshit B Pandya 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 10:
(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.
Done
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. […]
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 180: 0
Why PLTRST?
This GPIOs are controlled by the Kernel, so kept 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
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 19: of
Nit: off
Done
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 22: PP1200
Nit: PP2800. […]
Done
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 24: PP2800
Nit: PP1200. […]
Done
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 res […]
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 55: Sleep(5)
Is that extra 5 ms sleep intentional?
Removed
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 […]
Done
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?
Done
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?
Kept this outside the IF/ELSE block
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 132: 0x02
0x01
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/39360/9/src/mainboard/google/dedede... PS9, Line 41: Sleep(5)
Is this sleep intentional and required?
Removed
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.
Done
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?
Done
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)?
Done