| Message ID | 20181025163219.25788-4-ikegami@allied-telesis.co.jp |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | mtd: cfi_cmdset_0002: Fix flash write issue for OpenWrt Project | expand |
On Fri, 26 Oct 2018 01:32:11 +0900 Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote: > For a maintainability by reducing the goto statement remove it from > do_write_buffer(). I guess that's a matter of taste, but I don't find this version more readable than the previous one. Any strong reason to get rid of the op_done label? > > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp> > Cc: Fabio Bettoni <fbettoni@gmail.com> > Co: Hauke Mehrtens <hauke@hauke-m.de> > Co: Koen Vandeputte <koen.vandeputte@ncentric.com> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: linux-mtd@lists.infradead.org > --- > Changes since v2: > - None. > > Changes since v1: > - Split the patch v1 3/3. > > drivers/mtd/chips/cfi_cmdset_0002.c | 46 +++++++++++++++-------------- > 1 file changed, 24 insertions(+), 22 deletions(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index c2e51768a02c..deffafab067e 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > > if (chip_good(map, adr, datum)) { > xip_enable(map, chip, adr); > - goto op_done; > + break; > } > > - if (time_after(jiffies, timeo)) > + if (time_after(jiffies, timeo)) { > + ret = -EIO; > break; > + } > > /* Latency issues. Drop the lock, wait a while and retry */ > UDELAY(map, chip, adr, 1); > } > > - /* > - * Recovery from write-buffer programming failures requires > - * the write-to-buffer-reset sequence. Since the last part > - * of the sequence also works as a normal reset, we can run > - * the same commands regardless of why we are here. > - * See e.g. > - * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf > - */ > - cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, > - cfi->device_type, NULL); > - cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, > - cfi->device_type, NULL); > - cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi, > - cfi->device_type, NULL); > - xip_enable(map, chip, adr); > - /* FIXME - should have reset delay before continuing */ > + if (ret) { > + /* > + * Recovery from write-buffer programming failures requires > + * the write-to-buffer-reset sequence. Since the last part > + * of the sequence also works as a normal reset, we can run > + * the same commands regardless of why we are here. > + * See e.g. > + * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf > + */ > + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, > + cfi->device_type, NULL); > + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, > + cfi->device_type, NULL); > + cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi, > + cfi->device_type, NULL); > + xip_enable(map, chip, adr); > + /* FIXME - should have reset delay before continuing */ > > - printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n", > - __func__, adr); > + printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n", > + __func__, adr); > + } > > - ret = -EIO; > - op_done: > chip->state = FL_READY; > DISABLE_VPP(map); > put_chip(map, chip, adr);
> -----Original Message----- > From: Boris Brezillon [mailto:boris.brezillon@bootlin.com] > Sent: Monday, November 05, 2018 10:15 PM > To: IKEGAMI Tokunori > Cc: boris.brezillon@free-electrons.com; Fabio Bettoni; PACKHAM Chris; > Joakim Tjernlund; linux-mtd@lists.infradead.org > Subject: Re: [PATCH v3 03/11] mtd: cfi_cmdset_0002: Remove goto statement > from do_write_buffer() > > On Fri, 26 Oct 2018 01:32:11 +0900 > Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote: > > > For a maintainability by reducing the goto statement remove it from > > do_write_buffer(). > > I guess that's a matter of taste, but I don't find this version more > readable than the previous one. Any strong reason to get rid of the > op_done label? No there is no strong reason. I have just thought also that it is better for the maintainability. Regards, Ikegami > > > > > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp> > > Cc: Fabio Bettoni <fbettoni@gmail.com> > > Co: Hauke Mehrtens <hauke@hauke-m.de> > > Co: Koen Vandeputte <koen.vandeputte@ncentric.com> > > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > > Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com> > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > Cc: linux-mtd@lists.infradead.org > > --- > > Changes since v2: > > - None. > > > > Changes since v1: > > - Split the patch v1 3/3. > > > > drivers/mtd/chips/cfi_cmdset_0002.c | 46 > +++++++++++++++-------------- > > 1 file changed, 24 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > b/drivers/mtd/chips/cfi_cmdset_0002.c > > index c2e51768a02c..deffafab067e 100644 > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > > @@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct > map_info *map, struct flchip *chip, > > > > if (chip_good(map, adr, datum)) { > > xip_enable(map, chip, adr); > > - goto op_done; > > + break; > > } > > > > - if (time_after(jiffies, timeo)) > > + if (time_after(jiffies, timeo)) { > > + ret = -EIO; > > break; > > + } > > > > /* Latency issues. Drop the lock, wait a while and retry > */ > > UDELAY(map, chip, adr, 1); > > } > > > > - /* > > - * Recovery from write-buffer programming failures requires > > - * the write-to-buffer-reset sequence. Since the last part > > - * of the sequence also works as a normal reset, we can run > > - * the same commands regardless of why we are here. > > - * See e.g. > > - * > http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Bu > ffer_Prog_Page_Buffer_Read_AN.pdf > > - */ > > - cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, > > - cfi->device_type, NULL); > > - cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, > > - cfi->device_type, NULL); > > - cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi, > > - cfi->device_type, NULL); > > - xip_enable(map, chip, adr); > > - /* FIXME - should have reset delay before continuing */ > > + if (ret) { > > + /* > > + * Recovery from write-buffer programming failures > requires > > + * the write-to-buffer-reset sequence. Since the last > part > > + * of the sequence also works as a normal reset, we can > run > > + * the same commands regardless of why we are here. > > + * See e.g. > > + * > http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Bu > ffer_Prog_Page_Buffer_Read_AN.pdf > > + */ > > + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, > map, cfi, > > + cfi->device_type, NULL); > > + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, > map, cfi, > > + cfi->device_type, NULL); > > + cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, > map, cfi, > > + cfi->device_type, NULL); > > + xip_enable(map, chip, adr); > > + /* FIXME - should have reset delay before continuing */ > > > > - printk(KERN_WARNING "MTD %s(): software timeout, > address:0x%.8lx.\n", > > - __func__, adr); > > + printk(KERN_WARNING "MTD %s(): software timeout, > address:0x%.8lx.\n", > > + __func__, adr); > > + } > > > > - ret = -EIO; > > - op_done: > > chip->state = FL_READY; > > DISABLE_VPP(map); > > put_chip(map, chip, adr);
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index c2e51768a02c..deffafab067e 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -1884,38 +1884,40 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, if (chip_good(map, adr, datum)) { xip_enable(map, chip, adr); - goto op_done; + break; } - if (time_after(jiffies, timeo)) + if (time_after(jiffies, timeo)) { + ret = -EIO; break; + } /* Latency issues. Drop the lock, wait a while and retry */ UDELAY(map, chip, adr, 1); } - /* - * Recovery from write-buffer programming failures requires - * the write-to-buffer-reset sequence. Since the last part - * of the sequence also works as a normal reset, we can run - * the same commands regardless of why we are here. - * See e.g. - * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf - */ - cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, - cfi->device_type, NULL); - cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, - cfi->device_type, NULL); - cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi, - cfi->device_type, NULL); - xip_enable(map, chip, adr); - /* FIXME - should have reset delay before continuing */ + if (ret) { + /* + * Recovery from write-buffer programming failures requires + * the write-to-buffer-reset sequence. Since the last part + * of the sequence also works as a normal reset, we can run + * the same commands regardless of why we are here. + * See e.g. + * http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf + */ + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, + cfi->device_type, NULL); + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, + cfi->device_type, NULL); + cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, chip->start, map, cfi, + cfi->device_type, NULL); + xip_enable(map, chip, adr); + /* FIXME - should have reset delay before continuing */ - printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n", - __func__, adr); + printk(KERN_WARNING "MTD %s(): software timeout, address:0x%.8lx.\n", + __func__, adr); + } - ret = -EIO; - op_done: chip->state = FL_READY; DISABLE_VPP(map); put_chip(map, chip, adr);
For a maintainability by reducing the goto statement remove it from do_write_buffer(). Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp> Cc: Fabio Bettoni <fbettoni@gmail.com> Co: Hauke Mehrtens <hauke@hauke-m.de> Co: Koen Vandeputte <koen.vandeputte@ncentric.com> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Cc: linux-mtd@lists.infradead.org --- Changes since v2: - None. Changes since v1: - Split the patch v1 3/3. drivers/mtd/chips/cfi_cmdset_0002.c | 46 +++++++++++++++-------------- 1 file changed, 24 insertions(+), 22 deletions(-)