Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40405 )
Change subject: soc/intel/common: Add support for LPSS UART in ACPI mode ......................................................................
Patch Set 21: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/40405/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40405/21//COMMIT_MSG@14 PS21, Line 14: driver nit: this should go to the next line
https://review.coreboot.org/c/coreboot/+/40405/21//COMMIT_MSG@15 PS21, Line 15: wouldn't been have hidden wouldn't have been hidden
https://review.coreboot.org/c/coreboot/+/40405/21/src/soc/intel/common/block... File src/soc/intel/common/block/uart/chip.h:
https://review.coreboot.org/c/coreboot/+/40405/21/src/soc/intel/common/block... PS21, Line 1: SPDX-License-Identifier: GPL-2.0-only */ : #include <stdint.h>
Jenkins seems not to care.
It's just for aesthetics
https://review.coreboot.org/c/coreboot/+/40405/21/src/soc/intel/common/block... PS21, Line 3: device/pci_ids.h
It is. See commit message.
Maybe add a comment to let people know this is so that mainboards can use these definitions in the devicetree?
https://review.coreboot.org/c/coreboot/+/40405/21/src/soc/intel/common/block... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/40405/21/src/soc/intel/common/block... PS21, Line 284: strncmp strcmp, maybe?
IMHO another switch-case like that of `uart_acpi_hid` would be cleaner