Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/em100/+/37258 )
Change subject: Add compatibility mode ......................................................................
Patch Set 3:
(9 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?
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 ?
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
https://review.coreboot.org/c/em100/+/37258/3/image.c@30 PS3, Line 30: platform Where is this set?
https://review.coreboot.org/c/em100/+/37258/3/image.c@69 PS3, Line 69: find_fd Could use function comments throughout
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...could we define these values, or is it not worth it?
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. Perhaps just show a warning but continue makingg the changes?
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()
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?