Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/37258 )
Change subject: Add compatibility mode ......................................................................
Patch Set 4:
(13 comments)
https://review.coreboot.org/c/em100/+/37258/3/em100.h File em100.h:
https://review.coreboot.org/c/em100/+/37258/3/em100.h@202 PS3, Line 202: autocorrect_image
Can you add a comment as to what this does?
Done
https://review.coreboot.org/c/em100/+/37258/3/em100.h@202 PS3, Line 202: int
What does the return value here mean?
Done
https://review.coreboot.org/c/em100/+/37258/3/em100.c File em100.c:
https://review.coreboot.org/c/em100/+/37258/3/em100.c@787 PS3, Line 787: C
don't want to use -e ?
for ... ?
I'm not opposed to change this, but what was your rationale?
https://review.coreboot.org/c/em100/+/37258/3/image.c File image.c:
https://review.coreboot.org/c/em100/+/37258/3/image.c@26 PS3, Line 26: PTR_IN_RANGE
Seems like this could be a function
Done
https://review.coreboot.org/c/em100/+/37258/3/image.c@26 PS3, Line 26: #define PTR_IN_RANGE(ptr, base, limit) \ : ((const char *)(ptr) >= (base) && \ : (const char *)&(ptr)[1] <= (base) + (limit))
This is only ever used like PTR_IN_RANGE(...) ? ... : NULL. […]
Done
https://review.coreboot.org/c/em100/+/37258/3/image.c@30 PS3, Line 30: platform
Where is this set?
Right now it is not. Removed.
https://review.coreboot.org/c/em100/+/37258/3/image.c@69 PS3, Line 69: find_fd
Could use function comments throughout
Done
https://review.coreboot.org/c/em100/+/37258/3/image.c@75 PS3, Line 75: 0x0FF0A55A
Maybe define this somewhere?
Done
https://review.coreboot.org/c/em100/+/37258/3/image.c@92 PS3, Line 92: 0xff) << 4
Lots of magic shifts and values in this file... […]
It seems the functions are somewhat self-explanatory, given the only thing they do is extract the function pointers from the image. I suppose I could change them to FLMAP0_FCBA_OFFSET and _MASK but I don't see how that would increase readability of this code, making it 3 lines instead of one.
https://review.coreboot.org/c/em100/+/37258/3/image.c@102 PS3, Line 102: is_platform_ifd_2(void)
I'd rather move this logic into `get_ifd_version`
Done
https://review.coreboot.org/c/em100/+/37258/3/image.c@168 PS3, Line 168: 6
This should be up to the user though, and the user has requested a change. […]
Done
https://review.coreboot.org/c/em100/+/37258/3/image.c@187 PS3, Line 187: fcba->flcomp &= ~(1 << 30);
Seems like this line should move into ifd_set_spi_frequency()
Done
https://review.coreboot.org/c/em100/+/37258/3/image.c@205 PS3, Line 205: ifd_set_em100_mode
Print a message to indicate what happened?
Done