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

[Cyrille Pitchen]

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