Attention is currently required from: Bora Guvendik, Anil Kumar K, Cliff Huang, Selma Bensaid, Thejaswani Putta. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61354 )
Change subject: drivers/wwan/fm: Add Fibocom 5G WWAN ACPI support ......................................................................
Patch Set 2:
(11 comments)
Patchset:
PS2: Thanks Cliff!
One thing to note; right now we have the WWAN shut-down sequence hardcoded into the DSDT (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...), so after this lands, we have the opportunity to make the shut-down sequence into its own Method and then brya's `MPTS` method can just call that instead of hardcoding the sequence there 😊
File src/drivers/wwan/fm/acpi_fm350gl.c:
https://review.coreboot.org/c/coreboot/+/61354/comment/995f5cb8_32c9a60b PS2, Line 29: Can you add a comment here about what FHRF does?
https://review.coreboot.org/c/coreboot/+/61354/comment/77b3a674_01eff461 PS2, Line 38: assert `deassert` (it is active-low, so driving it high is deassertion, and the If just above means this code will only get called if PERST# is already low, i.e. active)
https://review.coreboot.org/c/coreboot/+/61354/comment/36c026e8_f9a7ad95 PS2, Line 42: assert `deassert` same reason as above
https://review.coreboot.org/c/coreboot/+/61354/comment/1e03d250_4c93bdd8 PS2, Line 44: warm reset Arg0 == 0, means cold reset and Arg0 == 1, means warm reset?
https://review.coreboot.org/c/coreboot/+/61354/comment/90e8e004_f08c4a1b PS2, Line 61: Can you add a comment here about what SHRF does?
https://review.coreboot.org/c/coreboot/+/61354/comment/a07ba0d2_f026d80e PS2, Line 90: acpi_device_path_join(parent_dev, "RTD3._OFS") nit: this is used twice, extract to a local variable
https://review.coreboot.org/c/coreboot/+/61354/comment/350a525e_ccd683a1 PS2, Line 90: acpigen_write_store_int_to_namestr(1, acpi_device_path_join(parent_dev, "RTD3._OFS")); To ensure that these _ON and _OFF calls are properly paired, I would rather see these incremented and decremented rather than set to 0 and 1
https://review.coreboot.org/c/coreboot/+/61354/comment/d6fc81fa_d621810d PS2, Line 106: acpi_device_path_join(parent_dev, "RTD3._OFS") nit: this is used twice, extract to a local variable
https://review.coreboot.org/c/coreboot/+/61354/comment/c3da10d6_6e5e3ee6 PS2, Line 106: acpigen_write_store_int_to_namestr(1, acpi_device_path_join(parent_dev, "RTD3._OFS")); To ensure that these _ON and _OFF calls are properly paired, I would rather see these incremented and decremented rather than set to 0 and 1
https://review.coreboot.org/c/coreboot/+/61354/comment/d000ca39_abd652c4 PS2, Line 167: .read_resources = noop_read_resources, : .set_resources = noop_set_resources, : .acpi_fill_ssdt = wwan_fm350gl_acpi_fill_ssdt, : .acpi_name = wwan_fm350gl_acpi_name, : }; nit: line these up like this (with tabs) ``` .read_resources = noop_read_resources, .set_resources = noop_set_resources, .acpi_fill_ssdt = wwan_fm350gl_acpi_fill_ssdt, .acpi_name = wwan_fm350gl_acpi_name, }; ```