* [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case
@ 2016-01-07 17:53 Brian Norris
2016-01-08 12:28 ` Boris Brezillon
2016-01-08 16:26 ` Harvey Hunt
0 siblings, 2 replies; 6+ messages in thread
From: Brian Norris @ 2016-01-07 17:53 UTC (permalink / raw)
To: linux-mtd; +Cc: Brian Norris, Alex Smith, Harvey Hunt
Using switch/case helps make this logic more clear and more robust. With
this structure:
* it's clear that this driver only support ECC_{HW,SOFT,SOFT_BCH}; and
* we can sanely handle new ECC unsupported modes (right now, this code
makes incorrect assumptions about the possible values in the
nand_ecc_modes_t enum; e.g., what happens with NAND_ECC_HW_OOB_FIRST?)
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Alex Smith <alex@alex-smith.me.uk>
Cc: Harvey Hunt <harvey.hunt@imgtec.com>
---
Compile tested only
drivers/mtd/nand/jz4780_nand.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
index 17eb9f264187..aabee8f5627e 100644
--- a/drivers/mtd/nand/jz4780_nand.c
+++ b/drivers/mtd/nand/jz4780_nand.c
@@ -171,29 +171,33 @@ static int jz4780_nand_init_ecc(struct jz4780_nand_chip *nand, struct device *de
chip->ecc.bytes = fls((1 + 8) * chip->ecc.size) *
(chip->ecc.strength / 8);
- if (nfc->bch && chip->ecc.mode == NAND_ECC_HW) {
+ switch (chip->ecc.mode) {
+ case NAND_ECC_HW:
+ if (!nfc->bch) {
+ dev_err(dev, "HW BCH selected, but BCH controller not found\n");
+ return -ENODEV;
+ }
+
chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
chip->ecc.calculate = jz4780_nand_ecc_calculate;
chip->ecc.correct = jz4780_nand_ecc_correct;
- } else if (!nfc->bch && chip->ecc.mode == NAND_ECC_HW) {
- dev_err(dev, "HW BCH selected, but BCH controller not found\n");
- return -ENODEV;
- }
-
- if (chip->ecc.mode == NAND_ECC_HW_SYNDROME) {
- dev_err(dev, "ECC HW syndrome not supported\n");
- return -EINVAL;
- }
-
- if (chip->ecc.mode != NAND_ECC_NONE)
+ /* fall through */
+ case NAND_ECC_SOFT:
+ case NAND_ECC_SOFT_BCH:
dev_info(dev, "using %s (strength %d, size %d, bytes %d)\n",
(nfc->bch) ? "hardware BCH" : "software ECC",
chip->ecc.strength, chip->ecc.size, chip->ecc.bytes);
- else
+ break;
+ case NAND_ECC_NONE:
dev_info(dev, "not using ECC\n");
+ break;
+ default:
+ dev_err(dev, "ECC mode %d not supported\n", chip->ecc.mode);
+ return -EINVAL;
+ }
- /* The NAND core will generate the ECC layout. */
- if (chip->ecc.mode == NAND_ECC_SOFT || chip->ecc.mode == NAND_ECC_SOFT_BCH)
+ /* The NAND core will generate the ECC layout for SW ECC */
+ if (chip->ecc.mode != NAND_ECC_HW)
return 0;
/* Generate ECC layout. ECC codes are right aligned in the OOB area. */
--
2.6.0.rc2.230.g3dd15c0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case
2016-01-07 17:53 [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case Brian Norris
@ 2016-01-08 12:28 ` Boris Brezillon
2016-01-08 12:30 ` Harvey Hunt
2016-01-08 16:26 ` Harvey Hunt
1 sibling, 1 reply; 6+ messages in thread
From: Boris Brezillon @ 2016-01-08 12:28 UTC (permalink / raw)
To: Brian Norris; +Cc: linux-mtd, Harvey Hunt, Alex Smith
On Thu, 7 Jan 2016 09:53:08 -0800
Brian Norris <computersforpeace@gmail.com> wrote:
> Using switch/case helps make this logic more clear and more robust. With
> this structure:
>
> * it's clear that this driver only support ECC_{HW,SOFT,SOFT_BCH}; and
>
> * we can sanely handle new ECC unsupported modes (right now, this code
> makes incorrect assumptions about the possible values in the
> nand_ecc_modes_t enum; e.g., what happens with NAND_ECC_HW_OOB_FIRST?)
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Alex Smith <alex@alex-smith.me.uk>
> Cc: Harvey Hunt <harvey.hunt@imgtec.com>
LGTM
Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Compile tested only
>
> drivers/mtd/nand/jz4780_nand.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
> index 17eb9f264187..aabee8f5627e 100644
> --- a/drivers/mtd/nand/jz4780_nand.c
> +++ b/drivers/mtd/nand/jz4780_nand.c
> @@ -171,29 +171,33 @@ static int jz4780_nand_init_ecc(struct jz4780_nand_chip *nand, struct device *de
> chip->ecc.bytes = fls((1 + 8) * chip->ecc.size) *
> (chip->ecc.strength / 8);
>
> - if (nfc->bch && chip->ecc.mode == NAND_ECC_HW) {
> + switch (chip->ecc.mode) {
> + case NAND_ECC_HW:
> + if (!nfc->bch) {
> + dev_err(dev, "HW BCH selected, but BCH controller not found\n");
> + return -ENODEV;
> + }
> +
> chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
> chip->ecc.calculate = jz4780_nand_ecc_calculate;
> chip->ecc.correct = jz4780_nand_ecc_correct;
> - } else if (!nfc->bch && chip->ecc.mode == NAND_ECC_HW) {
> - dev_err(dev, "HW BCH selected, but BCH controller not found\n");
> - return -ENODEV;
> - }
> -
> - if (chip->ecc.mode == NAND_ECC_HW_SYNDROME) {
> - dev_err(dev, "ECC HW syndrome not supported\n");
> - return -EINVAL;
> - }
> -
> - if (chip->ecc.mode != NAND_ECC_NONE)
> + /* fall through */
> + case NAND_ECC_SOFT:
> + case NAND_ECC_SOFT_BCH:
> dev_info(dev, "using %s (strength %d, size %d, bytes %d)\n",
> (nfc->bch) ? "hardware BCH" : "software ECC",
> chip->ecc.strength, chip->ecc.size, chip->ecc.bytes);
> - else
> + break;
> + case NAND_ECC_NONE:
> dev_info(dev, "not using ECC\n");
> + break;
> + default:
> + dev_err(dev, "ECC mode %d not supported\n", chip->ecc.mode);
> + return -EINVAL;
> + }
>
> - /* The NAND core will generate the ECC layout. */
> - if (chip->ecc.mode == NAND_ECC_SOFT || chip->ecc.mode == NAND_ECC_SOFT_BCH)
> + /* The NAND core will generate the ECC layout for SW ECC */
> + if (chip->ecc.mode != NAND_ECC_HW)
> return 0;
>
> /* Generate ECC layout. ECC codes are right aligned in the OOB area. */
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case
2016-01-08 12:28 ` Boris Brezillon
@ 2016-01-08 12:30 ` Harvey Hunt
2016-01-08 12:41 ` Boris Brezillon
0 siblings, 1 reply; 6+ messages in thread
From: Harvey Hunt @ 2016-01-08 12:30 UTC (permalink / raw)
To: Boris Brezillon, Brian Norris; +Cc: linux-mtd, Alex Smith
Hi,
On 08/01/16 12:28, Boris Brezillon wrote:
> On Thu, 7 Jan 2016 09:53:08 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
>
>> Using switch/case helps make this logic more clear and more robust. With
>> this structure:
>>
>> * it's clear that this driver only support ECC_{HW,SOFT,SOFT_BCH}; and
>>
>> * we can sanely handle new ECC unsupported modes (right now, this code
>> makes incorrect assumptions about the possible values in the
>> nand_ecc_modes_t enum; e.g., what happens with NAND_ECC_HW_OOB_FIRST?)
>>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>> Cc: Alex Smith <alex@alex-smith.me.uk>
>> Cc: Harvey Hunt <harvey.hunt@imgtec.com>
>
> LGTM
>
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>
If it helps, I can add my Tested-By later today?
Harvey
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case
2016-01-08 12:30 ` Harvey Hunt
@ 2016-01-08 12:41 ` Boris Brezillon
0 siblings, 0 replies; 6+ messages in thread
From: Boris Brezillon @ 2016-01-08 12:41 UTC (permalink / raw)
To: Harvey Hunt; +Cc: Brian Norris, linux-mtd, Alex Smith
On Fri, 8 Jan 2016 12:30:32 +0000
Harvey Hunt <harvey.hunt@imgtec.com> wrote:
> Hi,
>
> On 08/01/16 12:28, Boris Brezillon wrote:
> > On Thu, 7 Jan 2016 09:53:08 -0800
> > Brian Norris <computersforpeace@gmail.com> wrote:
> >
> >> Using switch/case helps make this logic more clear and more robust. With
> >> this structure:
> >>
> >> * it's clear that this driver only support ECC_{HW,SOFT,SOFT_BCH}; and
> >>
> >> * we can sanely handle new ECC unsupported modes (right now, this code
> >> makes incorrect assumptions about the possible values in the
> >> nand_ecc_modes_t enum; e.g., what happens with NAND_ECC_HW_OOB_FIRST?)
> >>
> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> >> Cc: Alex Smith <alex@alex-smith.me.uk>
> >> Cc: Harvey Hunt <harvey.hunt@imgtec.com>
> >
> > LGTM
> >
> > Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> >
>
> If it helps, I can add my Tested-By later today?
Yes, that would be good, I only reviewed the changes, a real test would
confirm it does not break anything.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case
2016-01-07 17:53 [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case Brian Norris
2016-01-08 12:28 ` Boris Brezillon
@ 2016-01-08 16:26 ` Harvey Hunt
2016-01-08 17:49 ` Brian Norris
1 sibling, 1 reply; 6+ messages in thread
From: Harvey Hunt @ 2016-01-08 16:26 UTC (permalink / raw)
To: Brian Norris, linux-mtd; +Cc: Alex Smith
Hi Brian,
On 07/01/16 17:53, Brian Norris wrote:
> Using switch/case helps make this logic more clear and more robust. With
> this structure:
>
> * it's clear that this driver only support ECC_{HW,SOFT,SOFT_BCH}; and
>
> * we can sanely handle new ECC unsupported modes (right now, this code
> makes incorrect assumptions about the possible values in the
> nand_ecc_modes_t enum; e.g., what happens with NAND_ECC_HW_OOB_FIRST?)
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Alex Smith <alex@alex-smith.me.uk>
> Cc: Harvey Hunt <harvey.hunt@imgtec.com>
> ---
> Compile tested only
>
> drivers/mtd/nand/jz4780_nand.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
> index 17eb9f264187..aabee8f5627e 100644
> --- a/drivers/mtd/nand/jz4780_nand.c
> +++ b/drivers/mtd/nand/jz4780_nand.c
> @@ -171,29 +171,33 @@ static int jz4780_nand_init_ecc(struct jz4780_nand_chip *nand, struct device *de
> chip->ecc.bytes = fls((1 + 8) * chip->ecc.size) *
> (chip->ecc.strength / 8);
>
> - if (nfc->bch && chip->ecc.mode == NAND_ECC_HW) {
> + switch (chip->ecc.mode) {
> + case NAND_ECC_HW:
> + if (!nfc->bch) {
> + dev_err(dev, "HW BCH selected, but BCH controller not found\n");
> + return -ENODEV;
> + }
> +
> chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
> chip->ecc.calculate = jz4780_nand_ecc_calculate;
> chip->ecc.correct = jz4780_nand_ecc_correct;
> - } else if (!nfc->bch && chip->ecc.mode == NAND_ECC_HW) {
> - dev_err(dev, "HW BCH selected, but BCH controller not found\n");
> - return -ENODEV;
> - }
> -
> - if (chip->ecc.mode == NAND_ECC_HW_SYNDROME) {
> - dev_err(dev, "ECC HW syndrome not supported\n");
> - return -EINVAL;
> - }
> -
> - if (chip->ecc.mode != NAND_ECC_NONE)
> + /* fall through */
> + case NAND_ECC_SOFT:
> + case NAND_ECC_SOFT_BCH:
> dev_info(dev, "using %s (strength %d, size %d, bytes %d)\n",
> (nfc->bch) ? "hardware BCH" : "software ECC",
> chip->ecc.strength, chip->ecc.size, chip->ecc.bytes);
> - else
> + break;
> + case NAND_ECC_NONE:
> dev_info(dev, "not using ECC\n");
> + break;
> + default:
> + dev_err(dev, "ECC mode %d not supported\n", chip->ecc.mode);
> + return -EINVAL;
> + }
>
> - /* The NAND core will generate the ECC layout. */
> - if (chip->ecc.mode == NAND_ECC_SOFT || chip->ecc.mode == NAND_ECC_SOFT_BCH)
> + /* The NAND core will generate the ECC layout for SW ECC */
> + if (chip->ecc.mode != NAND_ECC_HW)
> return 0;
>
> /* Generate ECC layout. ECC codes are right aligned in the OOB area. */
>
I've just tested this on a Ci20 and all seems well.
Tested-by: Harvey Hunt <harvey.hunt@imgtec.com>
Acked-by: Harvey Hunt <harvey.hunt@imgtec.com>
Thanks,
Harvey
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case
2016-01-08 16:26 ` Harvey Hunt
@ 2016-01-08 17:49 ` Brian Norris
0 siblings, 0 replies; 6+ messages in thread
From: Brian Norris @ 2016-01-08 17:49 UTC (permalink / raw)
To: Harvey Hunt; +Cc: linux-mtd, Alex Smith
On Fri, Jan 08, 2016 at 04:26:48PM +0000, Harvey Hunt wrote:
> Hi Brian,
>
> On 07/01/16 17:53, Brian Norris wrote:
> >Using switch/case helps make this logic more clear and more robust. With
> >this structure:
> >
> > * it's clear that this driver only support ECC_{HW,SOFT,SOFT_BCH}; and
> >
> > * we can sanely handle new ECC unsupported modes (right now, this code
> > makes incorrect assumptions about the possible values in the
> > nand_ecc_modes_t enum; e.g., what happens with NAND_ECC_HW_OOB_FIRST?)
> >
> >Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> >Cc: Alex Smith <alex@alex-smith.me.uk>
> >Cc: Harvey Hunt <harvey.hunt@imgtec.com>
>
> I've just tested this on a Ci20 and all seems well.
>
> Tested-by: Harvey Hunt <harvey.hunt@imgtec.com>
> Acked-by: Harvey Hunt <harvey.hunt@imgtec.com>
Great, applied to l2-mtd.git
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-08 17:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-07 17:53 [PATCH] mtd: jz4780_nand: replace if/else blocks with switch/case Brian Norris
2016-01-08 12:28 ` Boris Brezillon
2016-01-08 12:30 ` Harvey Hunt
2016-01-08 12:41 ` Boris Brezillon
2016-01-08 16:26 ` Harvey Hunt
2016-01-08 17:49 ` Brian Norris
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.