RE: [PATCH v3 03/11] mtd: cfi_cmdset_0002: Remove goto statement from do_write_buffer()

[IKEGAMI Tokunori <ikegami@xxxxxxxxxxxxxxxxxxxx>]

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx]
> Sent: Monday, November 05, 2018 10:15 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@xxxxxxxxxxxxxxxxxx; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Fabio Bettoni <fbettoni@xxxxxxxxx>
> > Co: Hauke Mehrtens <hauke@xxxxxxxxxx>
> > Co: Koen Vandeputte <koen.vandeputte@xxxxxxxxxxxx>
> > Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> > Cc: Joakim Tjernlund <Joakim.Tjernlund@xxxxxxxxxxxx>
> > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx
> > ---
> > 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);


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/