Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44588 )
Change subject: mb/google/dedede/var/boten: Add LTE power on/off sequence ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44588/10/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/boten/gpio.c:
https://review.coreboot.org/c/coreboot/+/44588/10/src/mainboard/google/deded... PS10, Line 11: 1 Shouldn't this be 0 since it is being enabled by the ACPI methods?
https://review.coreboot.org/c/coreboot/+/44588/10/src/mainboard/google/deded... PS10, Line 45: 1 Shouldn't this be 0? Since it is being deasserted by the ACPI methods?
https://review.coreboot.org/c/coreboot/+/44588/10/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/boten/variant.c:
https://review.coreboot.org/c/coreboot/+/44588/10/src/mainboard/google/deded... PS10, Line 15: const struct gpio_with_delay lte_power_off_gpios[] = { : { : GPP_H17, /* WWAN_RST_L => LTE_RESET_R_ODL */ : 10, : }, : { : GPP_A10, /* WWAN_EN => LTE_PWR_OFF_ODL */ : 0 : }, : }; : : for (int i = 0; i < ARRAY_SIZE(lte_power_off_gpios); i++) { : gpio_output(lte_power_off_gpios[i].gpio, 0); : mdelay(lte_power_off_gpios[i].delay_msecs); : } This can simply be written as: ``` gpio_output(GPIO_H17, 0); mdelay(10); gpio_output(GPIO_A10, 0); ```
Do we really need to have a table? I think the sequence is straight forward enough to put here directly.
-
Furquan Shaikh (Code Review)