Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/23135 )
Change subject: superio: Add ASpeed AST2400 ......................................................................
Patch Set 17:
(8 comments)
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/ast2400/ast2400.... File src/superio/aspeed/ast2400/ast2400.h:
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/ast2400/ast2400.... PS17, Line 21: #include <arch/io.h> no need for arch/io.h
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/ast2400/ast2400.... PS17, Line 22: #include <superio/aspeed/common/aspeed.h> why include it here if it's only linked in romstage?
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/ast2400/ast2400.... PS17, Line 32: #define AST2400_MAILBOX 0xE /* Mailbox */ one more tab to align with ILPC2AHB
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/ast2400/ast2400.... PS17, Line 37: void aspeed2400_enable_serial(u16 port, u16 device, u16 iobase); seems to be unused
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/ast2400/superio.... File src/superio/aspeed/ast2400/superio.c:
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/ast2400/superio.... PS17, Line 22: #include <console/console.h> some includes aren't required
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/ast2400/superio.... PS17, Line 62: { &ops, AST2400_MAILBOX, PNP_IO0 | PNP_IRQ0, 0xfffe, }, would it make sense to add SMBIOS type 38 here to advertise KCS?
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/common/Kconfig File src/superio/aspeed/common/Kconfig:
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/common/Kconfig@4 PS17, Line 4: ## Copyright (C) 2009 Ronald G. Minnich If applicable only your Copyright should be mentioned here
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/common/aspeed.h File src/superio/aspeed/common/aspeed.h:
https://review.coreboot.org/#/c/23135/17/src/superio/aspeed/common/aspeed.h@... PS17, Line 21: #include <arch/io.h> no need for arch/io.h