| Message ID | 1423594800-24214-7-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
Hi, Uwe Kleine-König wrote: > The PARAM command was long unimplemented and it probably wasn't > noticed because chip probing using only the few bytes returned by the > READID command are good enough in most cases to determine the chip in > use. > > Still to notice such a shortcoming earlier in the future would be nice > in case it's something more vital. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/mtd/nand/mxc_nand.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > index 0083b4ee4f33..372e0e38f59b 100644 > --- a/drivers/mtd/nand/mxc_nand.c > +++ b/drivers/mtd/nand/mxc_nand.c > @@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command, > memcpy32_fromio(host->data_buf, host->main_area0, 512); > host->buf_start = 0; > break; > + default: > + WARN_ONCE(1, "Unimplemented command (cmd=%u)\n", > + command); > + break; > } useless break; Lothar Waßmann
Hello Lothar, On Wed, Feb 11, 2015 at 09:42:56AM +0100, Lothar Waßmann wrote: > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > > index 0083b4ee4f33..372e0e38f59b 100644 > > --- a/drivers/mtd/nand/mxc_nand.c > > +++ b/drivers/mtd/nand/mxc_nand.c > > @@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command, > > memcpy32_fromio(host->data_buf, host->main_area0, 512); > > host->buf_start = 0; > > break; > > + default: > > + WARN_ONCE(1, "Unimplemented command (cmd=%u)\n", > > + command); > > + break; > > } > useless break; Do you mean the line break? That's right, I fixed it here for a later v2. But I guess you mean the (literal) break here. Right, it could be dropped without change in semantic, but I thought adding it matches the usually recommended style?! Best regards Uwe
Hi, Uwe Kleine-König wrote: > Hello Lothar, > > On Wed, Feb 11, 2015 at 09:42:56AM +0100, Lothar Waßmann wrote: > > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > > > index 0083b4ee4f33..372e0e38f59b 100644 > > > --- a/drivers/mtd/nand/mxc_nand.c > > > +++ b/drivers/mtd/nand/mxc_nand.c > > > @@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command, > > > memcpy32_fromio(host->data_buf, host->main_area0, 512); > > > host->buf_start = 0; > > > break; > > > + default: > > > + WARN_ONCE(1, "Unimplemented command (cmd=%u)\n", > > > + command); > > > + break; > > > } > > useless break; > Do you mean the line break? That's right, I fixed it here for a later > v2. But I guess you mean the (literal) break here. Right, it could be > dropped without change in semantic, but I thought adding it matches the > usually recommended style?! > Documentation/CodingStyle has this example: | default: | break; | } but there is no useful statement in the 'default' case, so the 'break' is necessary here. IMO this doesn't mandate to add a 'break' at the end of the default clause if there are actual statements in this path. Lothar Waßmann
Hello, On Wed, Feb 11, 2015 at 10:40:16AM +0100, Lothar Waßmann wrote: > Uwe Kleine-König wrote: > > On Wed, Feb 11, 2015 at 09:42:56AM +0100, Lothar Waßmann wrote: > > > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > > > > index 0083b4ee4f33..372e0e38f59b 100644 > > > > --- a/drivers/mtd/nand/mxc_nand.c > > > > +++ b/drivers/mtd/nand/mxc_nand.c > > > > @@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command, > > > > memcpy32_fromio(host->data_buf, host->main_area0, 512); > > > > host->buf_start = 0; > > > > break; > > > > + default: > > > > + WARN_ONCE(1, "Unimplemented command (cmd=%u)\n", > > > > + command); > > > > + break; > > > > } > > > useless break; > > Do you mean the line break? That's right, I fixed it here for a later > > v2. But I guess you mean the (literal) break here. Right, it could be > > dropped without change in semantic, but I thought adding it matches the > > usually recommended style?! > > > Documentation/CodingStyle has this example: > | default: > | break; > | } > but there is no useful statement in the 'default' case, so the > 'break' is necessary here. > IMO this doesn't mandate to add a 'break' at the end of the default > clause if there are actual statements in this path. OK, we agree that Documentation/CodingStyle isn't explict about this case here. For non-default case labels I already saw review requests to add an explicit break which I consider sensible. For default it's not that important but IMHO still good style. I want to keep it. Best regards Uwe
On Wed, Feb 11, 2015 at 10:40:16AM +0100, Lothar Waßmann wrote: > Hi, > > Uwe Kleine-König wrote: > > Hello Lothar, > > > > On Wed, Feb 11, 2015 at 09:42:56AM +0100, Lothar Waßmann wrote: > > > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > > > > index 0083b4ee4f33..372e0e38f59b 100644 > > > > --- a/drivers/mtd/nand/mxc_nand.c > > > > +++ b/drivers/mtd/nand/mxc_nand.c > > > > @@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command, > > > > memcpy32_fromio(host->data_buf, host->main_area0, 512); > > > > host->buf_start = 0; > > > > break; > > > > + default: > > > > + WARN_ONCE(1, "Unimplemented command (cmd=%u)\n", > > > > + command); > > > > + break; > > > > } > > > useless break; > > Do you mean the line break? That's right, I fixed it here for a later > > v2. But I guess you mean the (literal) break here. Right, it could be > > dropped without change in semantic, but I thought adding it matches the > > usually recommended style?! > > > Documentation/CodingStyle has this example: > | default: > | break; > | } > but there is no useful statement in the 'default' case, so the > 'break' is necessary here. > IMO this doesn't mandate to add a 'break' at the end of the default > clause if there are actual statements in this path. The 'default:' is not necessarily at the end. Dropping the 'break' in the last case makes it easy to forget to add the break when additional cases are added below the last one. IMO the 'break' should stay there. Sascha
diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c index 0083b4ee4f33..372e0e38f59b 100644 --- a/drivers/mtd/nand/mxc_nand.c +++ b/drivers/mtd/nand/mxc_nand.c @@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command, memcpy32_fromio(host->data_buf, host->main_area0, 512); host->buf_start = 0; break; + default: + WARN_ONCE(1, "Unimplemented command (cmd=%u)\n", + command); + break; } }
The PARAM command was long unimplemented and it probably wasn't noticed because chip probing using only the few bytes returned by the READID command are good enough in most cases to determine the chip in use. Still to notice such a shortcoming earlier in the future would be nice in case it's something more vital. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/mtd/nand/mxc_nand.c | 4 ++++ 1 file changed, 4 insertions(+)