[coreboot-gerrit] Change in coreboot[master]: mainboard/google/poppy/variants/nautilus - MIPI camera asl files for ...

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/23056 )

Change subject: mainboard/google/poppy/variants/nautilus - MIPI camera asl files for Nautilus
......................................................................


Patch Set 2:

(5 comments)

https://review.coreboot.org/#/c/23056/2/src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/ipu_mainboard.asl
File src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/ipu_mainboard.asl:

https://review.coreboot.org/#/c/23056/2/src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/ipu_mainboard.asl@18
PS2, Line 18: /* Define two ports for CIO2 device where endpoint of port0
            : 	is connected to CAM0 and endpoint of port1 is connected to CAM1 */
I imagine there is only one port and endpoint where only CAM0 is connected.


https://review.coreboot.org/#/c/23056/2/src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/ipu_mainboard.asl@25
PS2, Line 25: 		Package () { "port1", "PRT1" },
not needed, there is only one sensor.


https://review.coreboot.org/#/c/23056/2/src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/ipu_mainboard.asl@52
PS2, Line 52: Name (PRT1, Package () {
            : 		ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            : 		Package () {
            : 			Package () { "port", 1 }, /* csi 1 */
            : 		},
            : 		ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            : 		Package () {
            : 			Package () { "endpoint0", "EP10" },
            : 		}
            : 	})
Do we need this? Only one sensor on MIPI.


https://review.coreboot.org/#/c/23056/2/src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/mipi_camera.asl
File src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/mipi_camera.asl:

https://review.coreboot.org/#/c/23056/2/src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/mipi_camera.asl@16
PS2, Line 16: Scope (\_SB.PCI0.I2C2)
            : {
            : 	Device (PMIC)
            : 	{
            : 		Name (_HID, "INT3472") /* _HID: Hardware ID */
            : 		Name (_UID, Zero)  /* _UID: Unique ID */
            : 		Name (_DDN, "TPS68470 PMIC")  /* _DDN: DOS Device Name */
            : 		Name (CAMD, 0x64)
            : 
            : 		Method (_STA, 0, NotSerialized)  /* _STA: Status */
            : 		{
            : 			Return (0x0F)
            : 		}
            : 
            : 		Method (PMON, 0, Serialized) {
            : 			/*
            : 			 * Turn on 3V3_VDD. It takes around
There is a lot of code which is a copy of src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/mipi_camera.asl, specially the PMIC.
Can we utilize the common code from that file. Things may will turn murky. Feel free to split the original file.


https://review.coreboot.org/#/c/23056/2/src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/mipi_camera.asl@338
PS2, Line 338: /* Increment only if VSIC < 4 */
             : 					If (LLess (VSIC, 4)) {
For nautilus, this reference count will come down to 3 ideally (CAM0, VCM, NVM).
hmmmm...if we decide to split the common code then this value should also be configurable or derived from somewhere. Can't think of a right now.



-- 
To view, visit https://review.coreboot.org/23056
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebd41ac3631829bbb0b008761eb67c3db3e94638
Gerrit-Change-Number: 23056
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward #1001830
Gerrit-Reviewer: Rajmohan Mani <rajmohan.mani at intel.com>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi at intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Tue, 02 Jan 2018 08:52:40 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180102/c497114b/attachment.html>