Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 7:
(9 comments)
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 36: #define DEVICE_ENABLE_REG 0x30 : #define DEVICE_HIGH_ADDRESS_REG 0x60 : #define DEVICE_LOW_ADDRESS_REG 0x61 : #define DEVICE_IRQ_REG 0x70 not fintek specific and already defined in pnp_def.h
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 77: #define FINTEK_ENTRY_KEY 0x87 : #define FINTEK_EXIT_KEY 0xAA smells like pnp_conf_mode_8787_aa
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 79: #define FINTEK_BASE_ADDRESS 0x4E can probably also be 0x2e; this is usually configured in the devicetree
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 80: #define FINTEK_INDEX_IO FINTEK_BASE_ADDRESS : #define FINTEK_DATA_IO (FINTEK_BASE_ADDRESS + 1) generic for most SIO chips, so probably a redefinition
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 87: void fintek_enable_device(u8 ldn, u16 base, u8 irq); this should be done via devicetree and not by reimplementing functionality
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 67: //{ &ops, F81803A_WDT, PNP_IO0, 0x7f8 }, oh, this one wasn't addressed yet
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 71: read_register more or less redefinition of pnp_read_config
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 77: write_register more or less redefinition of pnp_write_config
https://review.coreboot.org/c/coreboot/+/33623/7/src/superio/fintek/f81803a/... PS7, Line 83: fintek_enable_device is reimplements existing functionality. please use the devicetree instead. also this function reimplements a lot of existing building blocks that could be used to configure SIO settings