Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32376 )
Change subject: superio/fintek/f71808a: Add more optional ramstage registers ......................................................................
Patch Set 4:
(2 comments)
If C supported algebraic data types, the issue not specified vs. zero would be easy to solve; that would just be some sort of sum type. But yeah, this definitely is a problem in the current code base, but I haven't gotten around to think about a proper generic solution. Often the default option is chosen to be zero, so that only things that are non-default need to be specified; this might not really work here though, since quite a few options are not booleans. An idea would be to introduce another setting (maybe do_pme_init) and only call f71808a_pme_init if this setting is true/1. Sure, this isn't the nicest solution, but it would reduce the the chance of breaking things for existing boards.
https://review.coreboot.org/#/c/32376/3/src/superio/fintek/f71808a/superio.c File src/superio/fintek/f71808a/superio.c:
https://review.coreboot.org/#/c/32376/3/src/superio/fintek/f71808a/superio.c... PS3, Line 48: .read_resources = pnp_read_resources, : .set_resources = pnp_set_resources, : .enable_resources = pnp_enable_resources, : .enable = pnp_alt_enable, : .init = f71808a_init, : .ops_pnp_mode = &pnp_conf_mode_8787_aa, i find it more readable with the right-hand sides being aligned
https://review.coreboot.org/#/c/32376/3/src/superio/fintek/f71808a/superio.c... PS3, Line 68: 0x07f8,
Many Finteks have 0x0ff8 here. I don't understand this mask so if anything's suspicious let me know. […]
to see what the masks do have a loot at the function pnp_get_ioresource in pnp_device.c. basically the number of zeros at the end encodes the size of the IO resource and the number of zeros at the beginning encodes the maximal address where the IO resource may be placed at. the reformatting makes the coding style inconsistent with the existing code.