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 15:
(10 comments)
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 209: /* E6 : GPP_E6/IMGCLKOUT_3 */ Configure this strap?
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 221: /* E12 : GPP_E12/IMGCLKOUT_4 */ Configure this strap?
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/baseboard/include/baseboard/acpi/cam0.asl:
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2020 Intel Corporation : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ : There is some general clean-up of license headers going on. Please look at other files and use similar license header here and in the rest of the CL.
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 18: Name (STA0, Zero) Nit: Add an empty line between 2 entities(Name, Method, Device etc.). Here and the rest of the CL.
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 38: 0,1 Nit: space between ',' and 1.
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 39: IF(STA1) : { : /* Power signal are already ON */ : /* Assert Reset */ : CTXS(GPP_D15) : Sleep(5) /* 5 us */ : /* Deassert Reset */ : STXS(GPP_D15) : Sleep(5) /* 5 us */ : } : ELSE : { : PON() : /* Assert Reset */ : CTXS(GPP_D15) : Sleep(5) /* 5 us */ : /* Deassert Reset */ : STXS(GPP_D15) : Sleep(5) /* 5 us */ : } This can be re-flowed as
If (!STA1) { /* Other sensor is OFF, so turn on power signals. */ PON() } /* Assert Reset */ CTXS(GPP_D15) Sleep(5) /* 5 us */ /* Deassert Reset */ STXS(GPP_D15) Sleep(5) /* 5 us */
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 59: Store(1,STA0) Can we not use ASL2.0? i.e. STA0 = 1
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 66: IF(STA1) : { : /* Do nothing since the other sensor is ON */ : } : ELSE : { : POFF() : } Same comment like in line 58.
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 183: } : } : } : }) : } Nit: Extra Indentation on all these lines.
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/baseboard/include/baseboard/acpi/cam1.asl:
https://review.coreboot.org/c/coreboot/+/39360/15/src/mainboard/google/deded... PS15, Line 15: Please apply all the comments as in the cam0.asl