Attention is currently required from: Máté Kukri (mkukri), Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55250 )
Change subject: [WIP] src/superio/smsc: Add support for the SCH5555 ......................................................................
Patch Set 1:
(10 comments)
File src/superio/smsc/sch5555/sch5555.h:
https://review.coreboot.org/c/coreboot/+/55250/comment/4b6c7a18_30238035 PS1, Line 8: #define SMSC_DATA 0x2f This could be different on another mainboard.
https://review.coreboot.org/c/coreboot/+/55250/comment/818cbacd_0fee441d PS1, Line 51: #define EMI_IOBASE 0xa00 : #define RUNTIME_IOBASE 0xa40 Aren't these mainboard-specific?
https://review.coreboot.org/c/coreboot/+/55250/comment/ccd69108_fa1d68a3 PS1, Line 60: uint8_t #include <types.h>
File src/superio/smsc/sch5555/sch5555_bootblock.c:
https://review.coreboot.org/c/coreboot/+/55250/comment/e61578dc_667b4a53 PS1, Line 15: smsc_write8(LDN_SELECT, LDN_GLOBAL); I'd make a helper function for this:
void smsc_select_ldn(uint8_t ldn) { smsc_write8(LDN_SELECT, ldn); }
https://review.coreboot.org/c/coreboot/+/55250/comment/d37a148d_4c237c27 PS1, Line 26: uint8_t #include <types.h>
https://review.coreboot.org/c/coreboot/+/55250/comment/a39e9d34_95be6ce4 PS1, Line 45: smsc_write8(LDN_ACTIVE, 0x01); // Activate This and the `LDN_ACTIVE` macro should be dropped in favor of `pnp_set_enable()` from device/pnp_ops.h
File src/superio/smsc/sch5555/sch5555_common.c:
https://review.coreboot.org/c/coreboot/+/55250/comment/f0bf9d22_962c9c45 PS1, Line 20: uint8_t #include <types.h>
https://review.coreboot.org/c/coreboot/+/55250/comment/94aead3b_ebb21b82 PS1, Line 22: outb(offset, SMSC_INDEX); : return inb(SMSC_DATA); Seems like you could use `pnp_read_config()` from device/pnp_ops.h
https://review.coreboot.org/c/coreboot/+/55250/comment/8dece12e_da9ff2f2 PS1, Line 37: val |= smsc_read8(offset); : val |= smsc_read8(offset + 1) << 8; : val |= smsc_read8(offset + 2) << 16; : val |= smsc_read8(offset + 3) << 24; How about:
val |= smsc_read16(offset); val |= smsc_read16(offset + 2) << 16;
Similar story for writes
https://review.coreboot.org/c/coreboot/+/55250/comment/b8cd6eb9_e596d9c7 PS1, Line 46: outb(offset, SMSC_INDEX); : outb(value, SMSC_DATA); Seems like you could use `pnp_write_config()` from device/pnp_ops.h
-
Angel Pons (Code Review)