RE: [PATCH 3/3] mtd: cfi_cmdset_0002: Remove goto statemnet from do_write_buffer()

[IKEGAMI Tokunori <ikegami@xxxxxxxxxxxxxxxxxxxx>]

Thank you so much for your comments.

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx]
> Sent: Friday, October 19, 2018 5:33 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@xxxxxxxxxxxxxxxxxx; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 3/3] mtd: cfi_cmdset_0002: Remove goto statemnet from
> do_write_buffer()
> 
> In the subject: s/statemnet/statement/

[Ikegami] Will do fix this sorry.

> 
> On Fri, 19 Oct 2018 17:13:20 +0900
> Tokunori Ikegami <ikegami@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > Also change to call xip_enable() once only.
> 
> Just like for the other patches I'd like to have more explanation in
> the commit message. And please split the commit in 2 commits since you
> seem to do 2 unrelated changes:
> - drop a label+gotos
> - call xip_enable() only once.

[Ikegami] Yes will do I think so.
Also I will do add additional refactoring patches to remove goto statement more and reduce the longer functions lines later.

> 
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Fabio Bettoni <fbettoni@xxxxxxxxx>
> > Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> > Cc: Joakim Tjernlund <Joakim.Tjernlund@xxxxxxxxxxxx>
> > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 48
> +++++++++++++++--------------
> >  1 file changed, 25 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index c2e51768a02c..060edd2f1693 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1883,39 +1883,41 @@ 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);
> > +		/* 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);
> > +	}
> > +
> > +	xip_enable(map, chip, 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/