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 ;)

View Change

17 comments:

• File Makefile:

  • Patch Set #4, Line 346: # Android is handled internally as separate OS, but it supports CONFIG_LINUX_SPI and CONFIG_MSTARDDC_SPI

    sigh, outdated comment (I hate comments)

• File internal.c:

  • Patch Set #4, Line 241: #if CONFIG_LINUX_MTD == 1

    A little nicer would be an inline stub in the header file in
    case of !CONFIG_LINUX_MTD. Then we wouldn't clutter the C file
    with #if.

• File linux_mtd.c:

  • Patch Set #4, Line 59: read

    Using low-level read() might be ok here together with sysfs.
    But I'm not inclined to check and would prefer stream functions
    (fread()) instead.

  • Patch Set #4, Line 94: endptr != &buf[strlen(buf)]

    Nit, same as `*endptr`?

  • Patch Set #4, Line 131: __builtin_popcount

    How portable is this?

  • Patch Set #4, Line 152: msg_pspew("%s: device_name: \"%s\", is_writeable: %d, "

    This only runs once on init, right? msg_pdbg() would be more
    appropriate, IMHO.

  • Patch Set #4, Line 167: flash->chip->block_erasers[0].eraseblocks[0].count =

    Nit, line break seems unnecessary.

  • Patch Set #4, Line 187:

    See, for example,
    		 * https://b/35573113
    

    I doubt, my browser would find much there.

  • Patch Set #4, Line 193: if (read(dev_fd, buf + i, step) != step) {

    Again, fread() is generally preferred. read() doesn't retry if
    it's interrupted and may return a short byte count even if more
    data is available.

    As we loop here anyway, this could also be solved by not bailing
    out on a short byte count. More specifically

        if ((step = read(...)) <= 0 && errno != EINTR) {
               ...
               return 1;
        }

    IIRC (I usually just use fread() if available).

  • Patch Set #4, Line 224:

    See, for example,
    	 * https://b/35573113
    

    ...

  • Patch Set #4, Line 231: write

    Same as for read().

  • Patch Set #4, Line 256: use MEMGETREGIONCOUNT/MEMGETREGIONINFO ioctls */

    Bail out like above?

  • Patch Set #4, Line 294: FILE *fp;

    Never closed? I would move it on top, initialize to `NULL` and
    below linux_mtd_setup_exit: `if (fp != NULL) fclose(fp);`.

    Hmm, actually that might be overkill, we'd only need 2x fclose()
    after the fread().

    Or just use read_sysfs_string()?

  • Patch Set #4, Line 320: return 1;

    `goto linux_mtd_setup_exit;`

  • Patch Set #4, Line 326: (const char *)

    Nit, why cast?

  • Patch Set #4, Line 361: (param == endptr)

    `*endptr` would also cover trailing data.

  • Patch Set #4, Line 364: "device number\n", param);

    Nits, missing full-stop, output should be broken before 80 chars.

To view, visit change 25706. To unsubscribe, or for help writing mail filters, visit settings.