| Message ID | 1479853468-119195-1-git-send-email-joel.esponde@honeywell.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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@atmel.com] > Envoyé : mercredi 23 novembre 2016 11:39 > À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- > mtd@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
On 11/23/2016 03:27 PM, Esponde, Joel wrote: > Hi Cyrille, > > Thanks for your explanations and patience! > I will try to avoid these "noob" mistakes next time! :-) You can start by NOT top-posting, it's really frowned upon :) > Joël Esponde > Honeywell | Safety and Productivity Solutions > >> -----Message d'origine----- >> De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] >> Envoyé : mercredi 23 novembre 2016 11:39 >> À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- >> mtd@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 > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
> -----Message d'origine----- > De : Marek Vasut [mailto:marex@denx.de] > Envoyé : mercredi 23 novembre 2016 16:36 > À : Esponde, Joel <Joel.Esponde@Honeywell.com>; Cyrille Pitchen > <cyrille.pitchen@atmel.com>; linux-mtd@lists.infradead.org > Objet : Re: [PATCH v2] mtd: spi-nor: fixed spansion quad enable > > On 11/23/2016 03:27 PM, Esponde, Joel wrote: > > Hi Cyrille, > > > > Thanks for your explanations and patience! > > I will try to avoid these "noob" mistakes next time! :-) > > You can start by NOT top-posting, it's really frowned upon :) > I see there is still a long way to go! Joël Esponde Honeywell | Safety and Productivity Solutions
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) + return ret; + + /* read CR and check it */ ret = read_cr(nor); if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { dev_err(nor->dev, "Spansion Quad bit not set\n");