Mathew King has posted comments on this change. ( https://review.coreboot.org/c/em100/+/37258 )
Change subject: Add compatibility mode ......................................................................
Patch Set 3:
(4 comments)
I really like this change, it should make working with an EM100 much easier.
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: int What does the return value here mean?
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: #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. It might be nice to incorporate that into the macro. I see that this is copied from ifdtool.c and it looks like it may be useful there too.
https://review.coreboot.org/c/em100/+/37258/3/image.c@75 PS3, Line 75: 0x0FF0A55A Maybe define this somewhere?
https://review.coreboot.org/c/em100/+/37258/3/image.c@102 PS3, Line 102: is_platform_ifd_2(void) Maybe just pass in a platform id here to avoid the static platform variable that is not being assigned to anything currently.