| Message ID | CAGFynZ83-pJTEB6XcPV4Dvv=zAaXk7aqkN3TQG1_WvPCvX0WjQ@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Joe Hershberger |
| Headers | show |
On Thursday 05 January 2012 19:30:57 Christian Daudt wrote: > Subject: [PATCH] Add support for MMC to fw_printenv/setenv > > This patch checks if the fd is MTD and if not (using an MTD-specific IOCTL) > and skips the flash unlock/erase/lock sequence if it is not MTD. > - fd_is_mtd function added to determine MTD/MMC > - flash_write_block made to not try MTD operations if mtd_type == > MTD_ABSENT - flash_read works with MMC devices now. seems reasonable, but i don't know anything about this code ;) -mike
Hi Christian, On Thu, Jan 5, 2012 at 6:30 PM, Christian Daudt <csd_b@daudt.org> wrote: > Changes from previous: > - Changed // to /* */ > - Ran through checkpatch.pl, cleaned up a number of line-too-big and > extra space in the code that was shifted due to being in the new 'if'. Your patch appears to be corrupt. Patchwork doesn't even find all the hunks. http://patchwork.ozlabs.org/patch/134561/ Please resend directly from git send-email or preferably with tools/patman. Thanks, -Joe
Hi Christian, On Thu, Jan 5, 2012 at 6:30 PM, Christian Daudt <csd_b@daudt.org> wrote: > Changes from previous: > - Changed // to /* */ > - Ran through checkpatch.pl, cleaned up a number of line-too-big and > extra space in the code that was shifted due to being in the new 'if'. > > Thanks, > csd > > > Subject: [PATCH] Add support for MMC to fw_printenv/setenv > > This patch checks if the fd is MTD and if not (using an MTD-specific IOCTL) > and skips the flash unlock/erase/lock sequence if it is not MTD. > - fd_is_mtd function added to determine MTD/MMC > - flash_write_block made to not try MTD operations if mtd_type == MTD_ABSENT > - flash_read works with MMC devices now. > > Signed-off-by: Christian Daudt <csd@broadcom.com> > --- > tools/env/fw_env.c | 103 ++++++++++++++++++++++++++++++++++++---------------- > 1 files changed, 71 insertions(+), 32 deletions(-) > > diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c > index 996682e..c760429 100644 > --- a/tools/env/fw_env.c > +++ b/tools/env/fw_env.c > @@ -211,6 +211,27 @@ static int flash_io (int mode); > static char *envmatch (char * s1, char * s2); > static int parse_config (void); > > + > +/* > + * Returns 1 if it is MTD and 0 if it is not MTD > + */ > +static int fd_is_mtd(int fd) > +{ > + struct mtd_info_user mtdinfo; > + int rc; > + > + rc = ioctl(fd, MEMGETINFO, &mtdinfo); > + if (rc < 0) { > + /* Failed MEMGETINFO, not MTD */ > + return 0; > + } else { > + /* Succeeded, MTD */ > + return 1; > + } > +} > + > + > + > #if defined(CONFIG_FILE) > static int get_config (char *); > #endif > @@ -836,31 +857,35 @@ static int flash_write_buf (int dev, int fd, > void *buf, size_t count, > > /* This only runs once on NOR flash and SPI-dataflash */ > while (processed < write_total) { > - rc = flash_bad_block (fd, mtd_type, &blockstart); > - if (rc < 0) /* block test failed */ > - return rc; > + if (mtd_type != MTD_ABSENT) { It would be really great if you organize this so as not to fully add another level of indentation here. It would be better to include all the accesses that need to check for mtd_type to functions (like flash_bad_block() and do the tests in them. Use a case structure in them where it makes sense (checking multiple states). Then you can simply call the functions and they may not do anything for a given flash type. So maybe add a flash_unlock(), flash_lock(), and flash_erase(). > + rc = flash_bad_block(fd, mtd_type, &blockstart); > + if (rc < 0) /* block test failed */ > + return rc; > > - if (blockstart + erasesize > top_of_range) { > - fprintf (stderr, "End of range reached, aborting\n"); > - return -1; > - } > + if (blockstart + erasesize > top_of_range) { > + fprintf(stderr, > + "End of range reached, aborting\n"); > + return -1; > + } > > - if (rc) { /* block is bad */ > - blockstart += blocklen; > - continue; > - } > + if (rc) { /* block is bad */ > + blockstart += blocklen; > + continue; > + } > > - erase.start = blockstart; > - ioctl (fd, MEMUNLOCK, &erase); > + erase.start = blockstart; > + ioctl(fd, MEMUNLOCK, &erase); > > - /* Dataflash does not need an explicit erase cycle */ > - if (mtd_type != MTD_DATAFLASH) > - if (ioctl (fd, MEMERASE, &erase) != 0) { > - fprintf (stderr, "MTD erase error on %s: %s\n", > - DEVNAME (dev), > - strerror (errno)); > - return -1; > - } > + /* Dataflash does not need an explicit erase cycle */ > + if (mtd_type != MTD_DATAFLASH) > + if (ioctl(fd, MEMERASE, &erase) != 0) { > + fprintf(stderr, > + "MTD erase error on %s: %s\n", > + DEVNAME(dev), > + strerror(errno)); > + return -1; > + } > + } > > if (lseek (fd, blockstart, SEEK_SET) == -1) { > fprintf (stderr, > @@ -878,7 +903,8 @@ static int flash_write_buf (int dev, int fd, void > *buf, size_t count, > return -1; > } > > - ioctl (fd, MEMLOCK, &erase); > + if (mtd_type != MTD_ABSENT) > + ioctl(fd, MEMLOCK, &erase); > > processed += blocklen; > block_seek = 0; > @@ -964,18 +990,31 @@ static int flash_read (int fd) > { > struct mtd_info_user mtdinfo; > int rc; > + int is_mtd; > > - rc = ioctl (fd, MEMGETINFO, &mtdinfo); > - if (rc < 0) { > - perror ("Cannot get MTD information"); > - return -1; > - } > + is_mtd = fd_is_mtd(fd); Why assign this to a variable when you only ever use it in the if statement? > > - if (mtdinfo.type != MTD_NORFLASH && > - mtdinfo.type != MTD_NANDFLASH && > - mtdinfo.type != MTD_DATAFLASH) { > - fprintf (stderr, "Unsupported flash type %u\n", mtdinfo.type); > - return -1; > + if (is_mtd) { The value of this is simply the result of the function you are about to call. Why do you need a separate function for this? It's only called this one place. > + rc = ioctl(fd, MEMGETINFO, &mtdinfo); > + if (rc < 0) { > + perror("Cannot get MTD information"); > + return -1; How about instead of erroring, you just mtdinfo.type = MTD_ABSENT; As is, it is impossible to ever reach this case. > + } > + > + if (mtdinfo.type != MTD_NORFLASH && > + mtdinfo.type != MTD_NANDFLASH && > + mtdinfo.type != MTD_DATAFLASH) { > + fprintf(stderr, "Unsupported flash type %u\n", > + mtdinfo.type); > + return -1; > + } > + } else { > + /* > + * Kinda hacky assuming !MTD means == MMC > + * but seems to be the easiest way to > + * determine that. > + */ This comment seems inappropriate... you aren't setting the type to MMC, you are setting it to MTD_ABSENT. Delete this comment. > + mtdinfo.type = MTD_ABSENT; > } > > DEVTYPE(dev_current) = mtdinfo.type; > -- > 1.7.1 -Joe
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 996682e..c760429 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -211,6 +211,27 @@ static int flash_io (int mode); static char *envmatch (char * s1, char * s2); static int parse_config (void); + +/* + * Returns 1 if it is MTD and 0 if it is not MTD + */ +static int fd_is_mtd(int fd) +{ + struct mtd_info_user mtdinfo; + int rc; + + rc = ioctl(fd, MEMGETINFO, &mtdinfo); + if (rc < 0) { + /* Failed MEMGETINFO, not MTD */ + return 0; + } else { + /* Succeeded, MTD */ + return 1; + } +} + + + #if defined(CONFIG_FILE) static int get_config (char *); #endif @@ -836,31 +857,35 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, /* This only runs once on NOR flash and SPI-dataflash */ while (processed < write_total) { - rc = flash_bad_block (fd, mtd_type, &blockstart); - if (rc < 0) /* block test failed */ - return rc; + if (mtd_type != MTD_ABSENT) { + rc = flash_bad_block(fd, mtd_type, &blockstart); + if (rc < 0) /* block test failed */ + return rc; - if (blockstart + erasesize > top_of_range) { - fprintf (stderr, "End of range reached, aborting\n"); - return -1; - } + if (blockstart + erasesize > top_of_range) { + fprintf(stderr, + "End of range reached, aborting\n"); + return -1; + } - if (rc) { /* block is bad */ - blockstart += blocklen; - continue; - } + if (rc) { /* block is bad */ + blockstart += blocklen; + continue; + } - erase.start = blockstart; - ioctl (fd, MEMUNLOCK, &erase); + erase.start = blockstart; + ioctl(fd, MEMUNLOCK, &erase); - /* Dataflash does not need an explicit erase cycle */ - if (mtd_type != MTD_DATAFLASH) - if (ioctl (fd, MEMERASE, &erase) != 0) { - fprintf (stderr, "MTD erase error on %s: %s\n", - DEVNAME (dev), - strerror (errno)); - return -1; - } + /* Dataflash does not need an explicit erase cycle */ + if (mtd_type != MTD_DATAFLASH) + if (ioctl(fd, MEMERASE, &erase) != 0) { + fprintf(stderr, + "MTD erase error on %s: %s\n", + DEVNAME(dev), + strerror(errno)); + return -1; + } + } if (lseek (fd, blockstart, SEEK_SET) == -1) {
Changes from previous: - Changed // to /* */ - Ran through checkpatch.pl, cleaned up a number of line-too-big and extra space in the code that was shifted due to being in the new 'if'. Thanks, csd Subject: [PATCH] Add support for MMC to fw_printenv/setenv This patch checks if the fd is MTD and if not (using an MTD-specific IOCTL) and skips the flash unlock/erase/lock sequence if it is not MTD. - fd_is_mtd function added to determine MTD/MMC - flash_write_block made to not try MTD operations if mtd_type == MTD_ABSENT - flash_read works with MMC devices now. Signed-off-by: Christian Daudt <csd@broadcom.com> --- tools/env/fw_env.c | 103 ++++++++++++++++++++++++++++++++++++---------------- 1 files changed, 71 insertions(+), 32 deletions(-) fprintf (stderr, @@ -878,7 +903,8 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, return -1; } - ioctl (fd, MEMLOCK, &erase); + if (mtd_type != MTD_ABSENT) + ioctl(fd, MEMLOCK, &erase); processed += blocklen; block_seek = 0; @@ -964,18 +990,31 @@ static int flash_read (int fd) { struct mtd_info_user mtdinfo; int rc; + int is_mtd; - rc = ioctl (fd, MEMGETINFO, &mtdinfo); - if (rc < 0) { - perror ("Cannot get MTD information"); - return -1; - } + is_mtd = fd_is_mtd(fd); - if (mtdinfo.type != MTD_NORFLASH && - mtdinfo.type != MTD_NANDFLASH && - mtdinfo.type != MTD_DATAFLASH) { - fprintf (stderr, "Unsupported flash type %u\n", mtdinfo.type); - return -1; + if (is_mtd) { + rc = ioctl(fd, MEMGETINFO, &mtdinfo); + if (rc < 0) { + perror("Cannot get MTD information"); + return -1; + } + + if (mtdinfo.type != MTD_NORFLASH && + mtdinfo.type != MTD_NANDFLASH && + mtdinfo.type != MTD_DATAFLASH) { + fprintf(stderr, "Unsupported flash type %u\n", + mtdinfo.type); + return -1; + } + } else { + /* + * Kinda hacky assuming !MTD means == MMC + * but seems to be the easiest way to + * determine that. + */ + mtdinfo.type = MTD_ABSENT; } DEVTYPE(dev_current) = mtdinfo.type;