Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40405 )
Change subject: soc/intel/cannonlake: Add support for UARTs on PCH-H ......................................................................
Patch Set 12: Code-Review+1
(11 comments)
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... File src/soc/intel/cannonlake/pch_h.c:
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 2: /* This file is part of the coreboot project. */ drop this comment, but leave a blank line between the SPDX identifier and the #includes
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 15: double blank line
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 18: switch (dev->path.pci.devfn)
that open brace { should be on the previous line
agreed.
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 20: // UART0 Such useful comments! I don't think we need them, do we? 😄
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 22: struct acpi_irq irq maybe declare this outside of the switch? that way, no braces are needed.
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 43: Another double blank line
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 52: spurious blank line
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 92: switch (dev->path.pci.devfn)
that open brace { should be on the previous line
Would be good to fix
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 94: // UART0 We can live without these comments
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 112: double empty line
https://review.coreboot.org/c/coreboot/+/40405/12/src/soc/intel/cannonlake/p... PS12, Line 119: .acpi_fill_ssdt = pch_h_uart_fill_ssdt, We can do away with one tab less