David Hendricks has posted comments on this change. ( https://review.coreboot.org/25706 )
Change subject: linux_mtd: Initial import ......................................................................
Patch Set 5:
(17 comments)
Patch Set 4:
(17 comments)
Looks good over all. The only real issues that have to be fixed is a leaked `FILE *` and internal URLs, IIRC. Mostly everything else is in the "we can do better" category ;)
Thanks for the review. I did a sanity check with a Chromebook that uses MTD, but hit a snag when converting write() to fwrite(). I'll take another look tomorrow and update the man page as well.
https://review.coreboot.org/#/c/25706/4/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/25706/4/flashrom.c@352 PS4, Line 352: Remove the ".*" at the end since MTD files don't use minor numbers (AFAIK).
https://review.coreboot.org/#/c/25706/4/internal.c File internal.c:
https://review.coreboot.org/#/c/25706/4/internal.c@241 PS4, Line 241: msg_perr("Processor detection/init failed.\n"
A little nicer would be an inline stub in the header file in […]
Done. I also moved the call to the inline above the call to pci_init_common() since MTD will obviate the need for it if successful.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c File linux_mtd.c:
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@59 PS4, Line 59:
Using low-level read() might be ok here together with sysfs. […]
Done
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@94 PS4, Line 94: read_sysfs_string(sysfs_pat
Nit, same as `*endptr`?
Done... I think? LMK if I misunderstood your comment.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@131 PS4, Line 131: read_sysfs_int(sys
How portable is this?
Should be fairly portable and is supported by all modern versions of GCC and clang AFAIK. However now that you mention it I don't see mingw support so I went ahead and replaced it with a generic popcnt implementation.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@152 PS4, Line 152:
This only runs once on init, right? msg_pdbg() would be more […]
Done
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@167 PS4, Line 167: }
Nit, line break seems unnecessary.
Done
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@187 PS4, Line 187: : static int linux_mtd_re
I doubt, my browser would find much there.
Removed
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@193 PS4, Line 193:
Again, fread() is generally preferred. read() doesn't retry if […]
Done
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@224 PS4, Line 224: sers[0].eraseblocks[0].size; : unsigned int i;
...
Done
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@231 PS4, Line 231: perr(
Same as for read().
I encountered a "Bad file descriptor" error when attempting to convert this to fwrite(), but will look into it more tomorrow. Hopefully it was just a silly error.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@256 PS4, Line 256:
Bail out like above?
Done
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@294 PS4, Line 294: .read = linux_mtd_read,
Never closed? I would move it on top, initialize to `NULL` and […]
Yeah, may as well use read_sysfs_string(). Done.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@320 PS4, Line 320: char dev_path[32];
`goto linux_mtd_setup_exit;`
Done.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@326 PS4, Line 326: etup_exit;
Nit, why cast?
Hmmm, I thought there were compiler warnings earlier on, but I don't seem to be getting them now. I'll remove the cast for now.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@361 PS4, Line 361: n 0;
`*endptr` would also cover trailing data.
Done.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@364 PS4, Line 364: int linux_mtd_init(void)
Nits, missing full-stop, output should be broken before 80 chars.
Done
-
David Hendricks (Code Review)