RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Artem Bityutskiy <dedekind@infradead.org>]

On Fri, 2008-08-22 at 11:09 +0530, Singh, Vimal wrote:
> @@ -154,10 +156,12 @@ int nand_calculate_ecc(struct mtd_info *
>  {
>         int i;
>         const uint32_t *bp = (uint32_t *)buf;
> +       /* 256 or 512 bytes/ecc  */
> +       uint32_t j =(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;

"j" is not descriptive. And I guess "const" modifier may be used for it.
Not sure it'll give any performance benefit, but it may affect gcc
optimizations. I would instead introduce:

const int eccsize_mult = ((struct nand_chip *)mtd->priv)->ecc.size >> 8;

>         uint32_t cur;           /* current value in buffer */
> -       /* rp0..rp15 are the various accumulated parities (per byte) */
> +       /* rp0..rp15..rp17 are the various accumulated parities (per byte) */
>         uint32_t rp0, rp1, rp2, rp3, rp4, rp5, rp6, rp7;
> -       uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15;
> +       uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15, rp16, rp17;
>         uint32_t par;           /* the cumulative parity for all data */
>         uint32_t tmppar;        /* the cumulative parity for this iteration;
>                                    for rp12 and rp14 at the end of the loop */
> @@ -169,6 +173,8 @@ int nand_calculate_ecc(struct mtd_info *
>         rp10 = 0;
>         rp12 = 0;
>         rp14 = 0;
> +       rp16 = 0;
> +       rp17 = 0; /* to make cmpiler happy */
> 
>         /*
>          * The loop is unrolled a number of times;
> @@ -180,7 +186,7 @@ int nand_calculate_ecc(struct mtd_info *
>          * needed for calculating rp12, rp14 and par
>          * also used as a performance improvement for rp6, rp8 and rp10
>          */
> -       for (i = 0; i < 4; i++) {
> +       for (i = 0; i < 4*j; i++) {
Here I would rather use:

for (i = 0; i < eccsize_mult << 2; i++) {

to avoid multiplication.

> +               code[2] =
> +                   (invparity[par & 0xf0] << 7) |
> +                   (invparity[par & 0x0f] << 6) |
> +                   (invparity[par & 0xcc] << 5) |
> +                   (invparity[par & 0x33] << 4) |
> +                   (invparity[par & 0xaa] << 3) |
> +                   (invparity[par & 0x55] << 2) |
> +                   3;
> +       else if (j == 2)
this should be just else, not else if.

> +               code[2] =
> +                   (invparity[par & 0xf0] << 7) |
> +                   (invparity[par & 0x0f] << 6) |
> +                   (invparity[par & 0xcc] << 5) |
> +                   (invparity[par & 0x33] << 4) |
> +                   (invparity[par & 0xaa] << 3) |
> +                   (invparity[par & 0x55] << 2) |
> +                   (invparity[rp17] << 1) |
> +                   (invparity[rp16] << 0);


> -               byte_addr = (addressbits[b1] << 4) + addressbits[b0];
> +               if (j == 1)
> +                       byte_addr = (addressbits[b1] << 4) + addressbits[b0];
> +               else if (j == 2)
> +                       byte_addr = (addressbits[b2 & 0x3] << 8) +
> +                                   (addressbits[b1] << 4) + addressbits[b0];
Similar.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)