Hello Matt Delco,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42468
to review the following change.
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
drivers/intel/mipi_camera: SSDT changes to add DSM
This change updates mipi_camera driver to add DSM section to SSDT.
Change-Id: Ic60e972b6aebad171a7b77fe0d99781693adfb20 Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c 1 file changed, 83 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/42468/1
diff --git a/src/drivers/intel/mipi_camera/camera.c b/src/drivers/intel/mipi_camera/camera.c index 81cd968..a3a9323 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -13,6 +13,86 @@ #define SENSOR_NAME_UUID "822ace8f-2814-4174-a56b-5f029fe079ee" #define SENSOR_TYPE_UUID "26257549-9271-4ca4-bb43-c4899d5a4881"
+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; + uint16_t i2c_bus = dev->bus ? dev->bus->secondary : 0xFFFF; + uint16_t i2c_addr = dev->path.i2c.device; + + switch (dev_type) { + case DEV_TYPE_SENSOR: + return (((uint32_t)i2c_bus) << 24 | ((uint32_t)i2c_addr) << 8 | + dev_type); + case DEV_TYPE_VCM: + return (((uint32_t)i2c_bus) << 24 | ((uint32_t)config->vcm_address) << 8 | + dev_type); + case DEV_TYPE_ROM: + return (((uint32_t)i2c_bus) << 24 | ((uint32_t)config->rom_address) << 8 | + dev_type); + } + + return 0; +} + +static void camera_generate_dsm(const struct device *dev) +{ + struct drivers_intel_mipi_camera_config *config = dev->chip_info; + int local1_ret = (config->ssdb.vcm_type ? 1 : 0) + (config->ssdb.rom_type ? 1 : 0) + 1; + int next_local1 = 1; + /* Method (_DSM, 4, NotSerialized) */ + acpigen_write_method("_DSM", 4); + + /* ToBuffer (Arg0, Local0) */ + acpigen_write_to_buffer(ARG0_OP, LOCAL0_OP); + + /* If (LEqual (Local0, ToUUID(uuid))) */ + acpigen_write_if(); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(LOCAL0_OP); + acpigen_write_uuid(SENSOR_NAME_UUID); + acpigen_write_return_string(config->sensor_name ? config->sensor_name : "UNKNOWN"); + acpigen_pop_len(); /* If */ + + /* If (LEqual (Local0, ToUUID(uuid))) */ + acpigen_write_if(); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(LOCAL0_OP); + acpigen_write_uuid(SENSOR_TYPE_UUID); + /* ToInteger (Arg2, Local1) */ + acpigen_write_to_integer(ARG2_OP, LOCAL1_OP); + + /* If (LEqual (Local1, 1)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, next_local1++); + acpigen_write_return_integer(local1_ret); + acpigen_pop_len(); /* If Arg2=1 */ + + /* If (LEqual (Local1, 2)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, next_local1++); + acpigen_write_return_integer(address_for_dev_type(dev, DEV_TYPE_SENSOR)); + acpigen_pop_len(); /* If Arg2=2 */ + + if (config->ssdb.vcm_type) { + /* If (LEqual (Local1, 3)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, next_local1++); + acpigen_write_return_integer(address_for_dev_type(dev, DEV_TYPE_VCM)); + acpigen_pop_len(); /* If Arg2=3 */ + } + + if (config->ssdb.rom_type) { + /* If (LEqual (Local1, 3 or 4)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, next_local1); + acpigen_write_return_integer(address_for_dev_type(dev, DEV_TYPE_ROM)); + acpigen_pop_len(); /* If Arg2=3 or 4 */ + } + + acpigen_pop_len(); /* If uuid */ + + /* Return (Buffer (One) { 0x0 }) */ + acpigen_write_return_singleton_buffer(0x0); + + acpigen_pop_len(); /* Method _DSM */ +} + static void camera_fill_sensor_defaults(struct drivers_intel_mipi_camera_config *config) { if (!config->ssdb.platform) @@ -63,6 +143,9 @@ if (!config->disable_ssdb_defaults) camera_fill_sensor_defaults(config);
+ /* _DSM */ + camera_generate_dsm(dev); + ep00 = acpi_dp_new_table("EP00"); acpi_dp_add_integer(ep00, "endpoint", 0); acpi_dp_add_integer(ep00, "clock-lanes", 0);
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... PS2, Line 19: 0xFFFF if this is chosen, do you still want to return a valid address? or should you return 0?
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... PS2, Line 40: int local1_ret = (config->ssdb.vcm_type ? 1 : 0) + (config->ssdb.rom_type ? 1 : 0) + 1; nit: the '+ 1' gets a little lost at the end, maybe put it first? 1 + .... + .... ?
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/+/42468
to look at the new patch set (#3).
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
drivers/intel/mipi_camera: SSDT changes to add DSM
This change updates mipi_camera driver to add DSM section to SSDT.
Change-Id: Ic60e972b6aebad171a7b77fe0d99781693adfb20 Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c 1 file changed, 83 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/42468/3
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/+/42468
to look at the new patch set (#4).
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
drivers/intel/mipi_camera: SSDT changes to add DSM
This change updates mipi_camera driver to add DSM section to SSDT.
Change-Id: Ic60e972b6aebad171a7b77fe0d99781693adfb20 Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c 1 file changed, 83 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/42468/4
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... PS2, Line 19: 0xFFFF
if this is chosen, do you still want to return a valid address? or should you return 0?
@Matt is it ok to return 0 if dev->bus is not valid
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... PS2, Line 40: int local1_ret = (config->ssdb.vcm_type ? 1 : 0) + (config->ssdb.rom_type ? 1 : 0) + 1;
nit: the '+ 1' gets a little lost at the end, maybe put it first? 1 + .... + .... […]
Done
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... PS2, Line 19: 0xFFFF
@Matt is it ok to return 0 if dev->bus is not valid
It's probably okay. I don't know how easy or hard it is to configure a 'dev' that doesn't have a dev->bus populated, so I'm not sure if it's worth trying to log an error message for such a case. The other 'return 0' seems to be impossible (for now), though I'm wondering if an approach like the following would be a worthwhile refactor:
uint16_t i2c_addr = 0; switch (dev_type) { case DEV_TYPE_SENSOR: i2c_addr = dev->path.i2c.device; break; case DEV_TYPE_VCM: i2c_addr = config->vcm_address; break; case DEV_TYPE_ROM: i2c_addr = config->rom_address; break; default: return 0; } return (((uint32_t)i2c_bus) << 24 | ((uint32_t)i2c_addr) << 8 | dev_type);
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... PS2, Line 19: 0xFFFF
It's probably okay. […]
+1 I think that looks clearer
Hello Matt Delco, build bot (Jenkins), Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42468
to look at the new patch set (#6).
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
drivers/intel/mipi_camera: SSDT changes to add DSM
This change updates mipi_camera driver to add DSM section to SSDT.
Change-Id: Ic60e972b6aebad171a7b77fe0d99781693adfb20 Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c 1 file changed, 85 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/42468/6
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42468/2/src/drivers/intel/mipi_came... PS2, Line 19: 0xFFFF
+1 I think that looks clearer
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42468/6/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42468/6/src/drivers/intel/mipi_came... PS6, Line 21: = 0 nit: doesn't need to be initialized, it's never used uninitialized.
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/+/42468
to look at the new patch set (#7).
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
drivers/intel/mipi_camera: SSDT changes to add DSM
This change updates mipi_camera driver to add DSM section to SSDT.
Change-Id: Ic60e972b6aebad171a7b77fe0d99781693adfb20 Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c 1 file changed, 85 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/42468/7
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42468/6/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42468/6/src/drivers/intel/mipi_came... PS6, Line 21: = 0
nit: doesn't need to be initialized, it's never used uninitialized.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 7: Code-Review+2
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
Patch Set 16: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42468 )
Change subject: drivers/intel/mipi_camera: SSDT changes to add DSM ......................................................................
drivers/intel/mipi_camera: SSDT changes to add DSM
This change updates mipi_camera driver to add DSM section to SSDT.
Change-Id: Ic60e972b6aebad171a7b77fe0d99781693adfb20 Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42468 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 1 file changed, 85 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 5230a2a..c4cacce 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -14,6 +14,88 @@ #define SENSOR_TYPE_UUID "26257549-9271-4ca4-bb43-c4899d5a4881" #define DEFAULT_ENDPOINT 0
+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; + uint16_t i2c_bus = dev->bus ? dev->bus->secondary : 0xFFFF; + uint16_t i2c_addr; + + switch (dev_type) { + case DEV_TYPE_SENSOR: + i2c_addr = dev->path.i2c.device; + break; + case DEV_TYPE_VCM: + i2c_addr = config->vcm_address; + break; + case DEV_TYPE_ROM: + i2c_addr = config->rom_address; + break; + default: + return 0; + } + + return (((uint32_t)i2c_bus) << 24 | ((uint32_t)i2c_addr) << 8 | dev_type); +} + +static void camera_generate_dsm(const struct device *dev) +{ + struct drivers_intel_mipi_camera_config *config = dev->chip_info; + int local1_ret = 1 + (config->ssdb.vcm_type ? 1 : 0) + (config->ssdb.rom_type ? 1 : 0); + int next_local1 = 1; + /* Method (_DSM, 4, NotSerialized) */ + acpigen_write_method("_DSM", 4); + + /* ToBuffer (Arg0, Local0) */ + acpigen_write_to_buffer(ARG0_OP, LOCAL0_OP); + + /* If (LEqual (Local0, ToUUID(uuid))) */ + acpigen_write_if(); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(LOCAL0_OP); + acpigen_write_uuid(SENSOR_NAME_UUID); + acpigen_write_return_string(config->sensor_name ? config->sensor_name : "UNKNOWN"); + acpigen_pop_len(); /* If */ + + /* If (LEqual (Local0, ToUUID(uuid))) */ + acpigen_write_if(); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(LOCAL0_OP); + acpigen_write_uuid(SENSOR_TYPE_UUID); + /* ToInteger (Arg2, Local1) */ + acpigen_write_to_integer(ARG2_OP, LOCAL1_OP); + + /* If (LEqual (Local1, 1)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, next_local1++); + acpigen_write_return_integer(local1_ret); + acpigen_pop_len(); /* If Arg2=1 */ + + /* If (LEqual (Local1, 2)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, next_local1++); + acpigen_write_return_integer(address_for_dev_type(dev, DEV_TYPE_SENSOR)); + acpigen_pop_len(); /* If Arg2=2 */ + + if (config->ssdb.vcm_type) { + /* If (LEqual (Local1, 3)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, next_local1++); + acpigen_write_return_integer(address_for_dev_type(dev, DEV_TYPE_VCM)); + acpigen_pop_len(); /* If Arg2=3 */ + } + + if (config->ssdb.rom_type) { + /* If (LEqual (Local1, 3 or 4)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, next_local1); + acpigen_write_return_integer(address_for_dev_type(dev, DEV_TYPE_ROM)); + acpigen_pop_len(); /* If Arg2=3 or 4 */ + } + + acpigen_pop_len(); /* If uuid */ + + /* Return (Buffer (One) { 0x0 }) */ + acpigen_write_return_singleton_buffer(0x0); + + acpigen_pop_len(); /* Method _DSM */ +} + static void camera_fill_sensor_defaults(struct drivers_intel_mipi_camera_config *config) { if (config->disable_ssdb_defaults) @@ -66,6 +148,9 @@
camera_fill_sensor_defaults(config);
+ /* _DSM */ + camera_generate_dsm(dev); + ep00 = acpi_dp_new_table("EP00"); acpi_dp_add_integer(ep00, "endpoint", DEFAULT_ENDPOINT); acpi_dp_add_integer(ep00, "clock-lanes", 0);
-
Matt Delco (Code Review) -
Patrick Georgi (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sugnan Prabhu S (Code Review)
-
Tim Wawrzynczak (Code Review)