Hello Matt Delco,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42469
to review the following change.
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
drivers/intel/mipi_camera: SSDT changes to add PLD
This change updates mipi_camera driver to add PLD section to SSDT.
Change-Id: If65b9cbabca95e9645d8e5023ce7fd78b0625d1e Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 53 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/42469/1
diff --git a/src/drivers/intel/mipi_camera/camera.c b/src/drivers/intel/mipi_camera/camera.c index a3a9323..ddafccf 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -13,6 +13,53 @@ #define SENSOR_NAME_UUID "822ace8f-2814-4174-a56b-5f029fe079ee" #define SENSOR_TYPE_UUID "26257549-9271-4ca4-bb43-c4899d5a4881"
+static void apply_pld_defaults(struct drivers_intel_mipi_camera_config *config) +{ + if (!config->pld.ignore_color) + config->pld.ignore_color = 1; + + if (!config->pld.visible) + config->pld.visible = 1; + + if (!config->pld.vertical_offset) + config->pld.vertical_offset = 0xffff; + + if (!config->pld.horizontal_offset) + config->pld.horizontal_offset = 0xffff; + + /* + * PLD_PANEL_TOP has a value of zero, so the following will change any instance of + * PLD_PANEL_TOP to PLD_PANEL_FRONT unless disable_pld_defaults is set. + */ + if (!config->pld.panel) + config->pld.panel = PLD_PANEL_FRONT; + + /* + * PLD_HORIZONTAL_POSITION_LEFT has a value of zero, so the following will change any + * instance of that value to PLD_HORIZONTAL_POSITION_CENTER unless disable_pld_defaults + * is set. + */ + if (!config->pld.horizontal_position) + config->pld.horizontal_position = PLD_HORIZONTAL_POSITION_CENTER; + + /* + * The desired default for |vertical_position| is PLD_VERTICAL_POSITION_UPPER, which + * has a value of zero so no work is needed to set a default. The same applies for + * setting |shape| to PLD_SHAPE_ROUND. + */ +} + +static void camera_generate_pld(const struct device *dev) +{ + struct drivers_intel_mipi_camera_config *config = dev->chip_info; + + if (config->use_pld && !config->disable_pld_defaults) + apply_pld_defaults(config); + + if (config->use_pld) + acpigen_write_pld(&config->pld); +} + static uint32_t address_for_dev_type(const struct device *dev, uint8_t dev_type) { struct drivers_intel_mipi_camera_config *config = dev->chip_info; @@ -140,6 +187,8 @@ const char *vcm_name = NULL; struct acpi_dp *lens_focus = NULL;
+ camera_generate_pld(dev); + if (!config->disable_ssdb_defaults) camera_fill_sensor_defaults(config);
diff --git a/src/drivers/intel/mipi_camera/chip.h b/src/drivers/intel/mipi_camera/chip.h index f55ee1d..95a3ebb 100644 --- a/src/drivers/intel/mipi_camera/chip.h +++ b/src/drivers/intel/mipi_camera/chip.h @@ -4,6 +4,7 @@ #define __INTEL_MIPI_CAMERA_CHIP_H__
#include <stdint.h> +#include <acpi/acpi_pld.h>
#define MAX_PWDB_ENTRIES 12 #define MAX_PORT_ENTRIES 4 @@ -134,6 +135,9 @@ const char *sensor_name; /* default "UNKNOWN" */ const char *remote_name; /* default "_SB.PCI0.CIO2" */ const char *vcm_name; /* defaults to |vcm_address| device */ + bool use_pld; + bool disable_pld_defaults; + struct acpi_pld pld; uint16_t rom_address; /* I2C to use if ssdb.rom_type != 0 */ uint16_t vcm_address; /* I2C to use if ssdb.vcm_type != 0 */ /* Settings specific to nvram. Many values, if left as zero, will be assigned a default.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42469/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42469/2/src/drivers/intel/mipi_came... PS2, Line 55: : if (config->use_pld && !config->disable_pld_defaults) : apply_pld_defaults(config); : : if (config->use_pld) : acpigen_write_pld(&config->pld); nit: if (config->use_pld) { if (!config->disable_pld_defaults) apply_pld_defaults();
acpigen_write_pld(&config->pld); }
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42469
to look at the new patch set (#3).
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
drivers/intel/mipi_camera: SSDT changes to add PLD
This change updates mipi_camera driver to add PLD section to SSDT.
Change-Id: If65b9cbabca95e9645d8e5023ce7fd78b0625d1e Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 54 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/42469/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42469/3/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42469/3/src/drivers/intel/mipi_came... PS3, Line 57: if(!config->disable_pld_defaults) space required before the open parenthesis '('
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42469
to look at the new patch set (#4).
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
drivers/intel/mipi_camera: SSDT changes to add PLD
This change updates mipi_camera driver to add PLD section to SSDT.
Change-Id: If65b9cbabca95e9645d8e5023ce7fd78b0625d1e Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 54 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/42469/4
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42469/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42469/2/src/drivers/intel/mipi_came... PS2, Line 55: : if (config->use_pld && !config->disable_pld_defaults) : apply_pld_defaults(config); : : if (config->use_pld) : acpigen_write_pld(&config->pld);
nit: […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 5: Code-Review+2
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42469
to look at the new patch set (#10).
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
drivers/intel/mipi_camera: SSDT changes to add PLD
This change updates mipi_camera driver to add PLD section to SSDT.
Change-Id: If65b9cbabca95e9645d8e5023ce7fd78b0625d1e Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 54 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/42469/10
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42469/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42469/12/src/drivers/intel/mipi_cam... PS12, Line 19: if (!config->pld.ignore_color) : config->pld.ignore_color = 1; : : if (!config->pld.visible) : config->pld.visible = 1; Won't this effectively just always set ignore_color and visible to 1? Either the devicetree sets it to 1 or it gets set to 1 here, so why bother with a config option?
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42469/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42469/12/src/drivers/intel/mipi_cam... PS12, Line 19: if (!config->pld.ignore_color) : config->pld.ignore_color = 1; : : if (!config->pld.visible) : config->pld.visible = 1;
Won't this effectively just always set ignore_color and visible to 1? Either the devicetree sets it […]
To keep one or both of these as zero someone would have to set disable_pld_defaults to essentially disable this function.
This defaulting scheme is a bit coarse but I couldn't think of a decent scheme to allow more fine-grain defaulting without adding a per-variable bool or special value that indicates whether that variable should be set to a default value or not. The current scheme is simpler and works for the currently-known boards (at least at the time I initially drafted the change) but two tradeoffs for the future are 1) if someone needs to disable/override the default for a variable then they have to disable defaulting (which now also means the value of the other variables will need to be explicitly set in config), and 2) if someone wants to add a new variable and/or change how the value of a variable gets set to a default then they may have to study all the configs that use mipi_camera to make sure they're not breaking/changing another board.
An example of this is how camera_fill_sensor_defaults() sets:
config->ssdb.platform = PLATFORM_SKC;
At the time all the boards were using skylake but now there will be a few others and there might be a better way to default this (e.g., #include a "soc/pci_devs.h" [or similar] that declares the default value for the platform).
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 12: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42469/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42469/12/src/drivers/intel/mipi_cam... PS12, Line 19: if (!config->pld.ignore_color) : config->pld.ignore_color = 1; : : if (!config->pld.visible) : config->pld.visible = 1;
To keep one or both of these as zero someone would have to set disable_pld_defaults to essentially d […]
Thanks for the background Matt.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
Patch Set 16: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42469 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add PLD ......................................................................
drivers/intel/mipi_camera: SSDT changes to add PLD
This change updates mipi_camera driver to add PLD section to SSDT.
Change-Id: If65b9cbabca95e9645d8e5023ce7fd78b0625d1e Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42469 Reviewed-by: Rizwan Qureshi rizwan.qureshi@intel.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 54 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Rizwan Qureshi: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/drivers/intel/mipi_camera/camera.c b/src/drivers/intel/mipi_camera/camera.c index c4cacce..7c0774d 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -14,6 +14,54 @@ #define SENSOR_TYPE_UUID "26257549-9271-4ca4-bb43-c4899d5a4881" #define DEFAULT_ENDPOINT 0
+static void apply_pld_defaults(struct drivers_intel_mipi_camera_config *config) +{ + if (!config->pld.ignore_color) + config->pld.ignore_color = 1; + + if (!config->pld.visible) + config->pld.visible = 1; + + if (!config->pld.vertical_offset) + config->pld.vertical_offset = 0xffff; + + if (!config->pld.horizontal_offset) + config->pld.horizontal_offset = 0xffff; + + /* + * PLD_PANEL_TOP has a value of zero, so the following will change any instance of + * PLD_PANEL_TOP to PLD_PANEL_FRONT unless disable_pld_defaults is set. + */ + if (!config->pld.panel) + config->pld.panel = PLD_PANEL_FRONT; + + /* + * PLD_HORIZONTAL_POSITION_LEFT has a value of zero, so the following will change any + * instance of that value to PLD_HORIZONTAL_POSITION_CENTER unless disable_pld_defaults + * is set. + */ + if (!config->pld.horizontal_position) + config->pld.horizontal_position = PLD_HORIZONTAL_POSITION_CENTER; + + /* + * The desired default for |vertical_position| is PLD_VERTICAL_POSITION_UPPER, which + * has a value of zero so no work is needed to set a default. The same applies for + * setting |shape| to PLD_SHAPE_ROUND. + */ +} + +static void camera_generate_pld(const struct device *dev) +{ + struct drivers_intel_mipi_camera_config *config = dev->chip_info; + + if (config->use_pld) { + if (!config->disable_pld_defaults) + apply_pld_defaults(config); + + acpigen_write_pld(&config->pld); + } +} + static uint32_t address_for_dev_type(const struct device *dev, uint8_t dev_type) { struct drivers_intel_mipi_camera_config *config = dev->chip_info; @@ -146,6 +194,8 @@ const char *vcm_name = NULL; struct acpi_dp *lens_focus = NULL;
+ camera_generate_pld(dev); + camera_fill_sensor_defaults(config);
/* _DSM */ diff --git a/src/drivers/intel/mipi_camera/chip.h b/src/drivers/intel/mipi_camera/chip.h index c962c9a..32b11c1 100644 --- a/src/drivers/intel/mipi_camera/chip.h +++ b/src/drivers/intel/mipi_camera/chip.h @@ -4,6 +4,7 @@ #define __INTEL_MIPI_CAMERA_CHIP_H__
#include <stdint.h> +#include <acpi/acpi_pld.h>
#define DEFAULT_LINK_FREQ 450000000 #define MAX_PWDB_ENTRIES 12 @@ -138,6 +139,9 @@ const char *sensor_name; /* default "UNKNOWN" */ const char *remote_name; /* default "_SB.PCI0.CIO2" */ const char *vcm_name; /* defaults to |vcm_address| device */ + bool use_pld; + bool disable_pld_defaults; + struct acpi_pld pld; uint16_t rom_address; /* I2C to use if ssdb.rom_type != 0 */ uint16_t vcm_address; /* I2C to use if ssdb.vcm_type != 0 */ /*