Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46506 )
Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................
Patch Set 5:
(1 comment)
Furquan, Duncan, is this a bug in this driver or
https://review.coreboot.org/c/coreboot/+/46506/5/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/46506/5/src/drivers/uart/acpi/acpi.... PS5, Line 75: /* Use either Interrupt() or GpioInt() */ : if (config->irq_gpio.pin_count) { : acpi_device_write_gpio(&config->irq_gpio); : irq_gpio_index = uart_acpi_write_gpio(&config->irq_gpio, : &curr_index); : } else { : acpi_device_write_interrupt(&config->irq); : } : : /* Add enable/reset GPIOs if needed */ : if (uart_acpi_add_gpios_to_crs(config)) { : reset_gpio_index = uart_acpi_write_gpio(&config->reset_gpio, : &curr_index); : enable_gpio_index = uart_acpi_write_gpio(&config->enable_gpio, : &curr_index); : } Actually, I see the GPIOs are only added conditionally to _CRS, so I think it would actually need to be: ``` /* Use either Interrupt() or GpioInt() */ if (!config->irq_gpio.pin_count) acpi_device_write_interrupt(&config->irq);
/* Add enable/reset GPIOs if needed */ if (uart_acpi_add_gpios_to_crs(config)) { irq_gpio_index = uart_acpi_write_gpio(&config->irq_gpio, &curr_index); reset_gpio_index = uart_acpi_write_gpio(&config->reset_gpio, &curr_index); if (config->irq_gpio.pin_count) enable_gpio_index = uart_acpi_write_gpio(&config->enable_gpio, &curr_index); } ```
Furquan, Duncan, does this look right?