[coreboot-gerrit] Change in coreboot[master]: driver/intel/mipi_camera: Add MIPI CSI camera SSDT generator

[V Sowmya (Code Review)]

V Sowmya has posted comments on this change. ( https://review.coreboot.org/18967 )

Change subject: driver/intel/mipi_camera: Add MIPI CSI camera SSDT generator
......................................................................


Patch Set 1:

(12 comments)

> (11 comments)

https://review.coreboot.org/#/c/18967/1//COMMIT_MSG
Commit Message:

PS1, Line 9: Add SSDT generator for MIPI CSI camera which will generate required
           : SSDB and CLDB structures for device specific data and PWDB structure
           : for power sequencing data which are used by the Intel kernel drivers.
           : * SSDB: Sensor specific database for camera sensor.
           : * CLDB: Control logic database for camera PMIC.
           : * PWDB: Power database for all the camera devices.
           : * CAMD: ACPI object to specify the camera device type.
> Add SSDT generator for MIPI CSI camera to create ACPI objects used by the I
Done


PS1, Line 17: b:36580624
> b/36580624
b:36580624 is correct


https://review.coreboot.org/#/c/18967/1/src/drivers/intel/mipi_camera/Kconfig
File src/drivers/intel/mipi_camera/Kconfig:

Line 1: config DRIVERS_INTEL_MIPI_CAMERA
> Are all future cameras going to be similar?
Yes that’s the plan, these ACPI objects should be applicable to future I2C based MIPI cameras. Also, This is the current design, we don’t intend to change.
But, it depends on how the kernel driver patches transform during the upstream process


https://review.coreboot.org/#/c/18967/1/src/drivers/intel/mipi_camera/camera.c
File src/drivers/intel/mipi_camera/camera.c:

Line 73: 	/* Fill Power Sequencing Data */
> So power sequencing data is common for all the device types? But there's ot
Power sequencing data defines specific power action required to be done during the sensor power on and each camera device should contain this ACPI object.
whereas CLDB and SSDB ACPI objects are specific to PMIC and camera sensor respectively.


Line 74: 	acpigen_write_method_serialized("PWDB",0);
> space after ',' -- applies to other instances in this file as well.
Done


https://review.coreboot.org/#/c/18967/1/src/drivers/intel/mipi_camera/chip.h
File src/drivers/intel/mipi_camera/chip.h:

PS1, Line 19: #include <arch/acpi_device.h>
> But what should be included is stdint.h since those types are being directl
Done


Line 29: } intel_camera_device_type;
> Why is everything typedef'd?
Done


Line 72: } __attribute__((packed))intel_ssdb;
> spaces between attributes and names
Done


Line 83: 	char name[32];
> const char *.
To align with the kernel implementation we have defined this as an array. Since there can be varying number of pwdb entries for a device, kernel driver expects the fixed sized structure to retrieve power sequencing data.


Line 94: 	uint8_t num_pwdb_entries;
> Why is the number separate from the array?
There can be varying number of PWDB entries for a given device, the maximum entries that we have come across is 10 for a sensor in our RVP board, where as there is 1 for VCM in poppy.
To handle this varying number of entries and to write valid/required amount of buffer in ACPI we decided to have that number in place.


Line 97: 	const char chip_name[16];
> These don't need to be arrays. Just make them const char *.
Done


Line 98: 	uint64_t acpi_uid;
> Why 64-bit uid?
Done


-- 
To view, visit https://review.coreboot.org/18967
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ief9e56d12b64081897613bf1c7abcdf915470b99
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: V Sowmya <v.sowmya at intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan at intel.com>
Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar at intel.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Naresh Solanki <naresh.solanki at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes