[PATCH v2] mtd: spi-nor: fixed spansion quad enable

[Esponde, Joel]

Hi Cyrille,

Thanks for your explanations and patience!
I will try to avoid these "noob" mistakes next time! :-)

Joël Esponde
Honeywell | Safety and Productivity Solutions

> -----Message d'origine-----
> De : Cyrille Pitchen [mailto:cyrille.pitchen at atmel.com]
> Envoyé : mercredi 23 novembre 2016 11:39
> À : Esponde, Joel <Joel.Esponde at Honeywell.com>; linux-
> mtd at lists.infradead.org
> Objet : Re: [PATCH v2] mtd: spi-nor: fixed spansion quad enable
> 
> Hi Joel,
> 
> Le 22/11/2016 à 23:24, Joël Esponde a écrit :
> > With the S25FL127S nor flash part, each writing to the configuration register
> takes hundreds of ms. During that  time, no more accesses to the flash
> should be done (even reads).
> >
> > This commit adds a wait loop after the register writing until the flash
> finishes its work.
> >
> > This issue could make rootfs mounting fail when the latter was done too
> much closely to this quad enable bit setting step. And in this case, a driver as
> UBIFS may try to recover the filesystem and may broke it completely.
> 
> Just few formal remarks about common usage and according to
> Documentation/process/submitting-patches.rst:
> - The summary phrase of the subject line should describe changes in
> imperative
>   mood, so please use something like
>   "mtd: spi-nor: fix spansion_quad_enable..." instead of
>   "mtd: spi-nor: fixED spansion_quad_enable...".
> - The commit message should be line wrapped at 75 columns.
> - You must add your "Signed-off-by:". It is mandatory to know who's
> introduced
>   the commit when browsing the logs.
> 
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..a945122 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -1255,7 +1255,11 @@ static int spansion_quad_enable(struct spi_nor
> *nor)
> >  		return -EINVAL;
> >  	}
> >
> > -	/* read back and check it */
> > +	ret = spi_nor_wait_till_ready(nor);
> > +	if (ret)
> I agree this is not done in macronix_quad_enable() but maybe a dev_err()
> message may notify about the reason of the failure, just like what is done for
> write_sr_cr() and read_cr().
> This point is optional!
> 
> > +		return ret;
> > +
> It seems there is a tab at the beginning of the empty line above.
> 
> > +	/* read CR and check it */
> Your patch deals with the missing call of spi_nor_wait_till_ready() so there is
> no strong reason to also modify the comment about read_cr().
> The general idea is to make the patch modify only what needs to be.
> 
> >  	ret = read_cr(nor);
> >  	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
> >  		dev_err(nor->dev, "Spansion Quad bit not set\n");
> >
> 
> Finally, it may help you to run the scripts/checkpatch.pl tool on your patch so
> this script reports you any warnings and errors in the syntax before
> submitting your patch.
> 
> Best regards,
> 
> Cyrille