[PATCH] MTD: atmel_nand: Update driver to support Programmable HW ECC controller

[Xu, Hong]

Hi Ivan,

> -----Original Message-----
> From: Ivan Djelic [mailto:ivan.djelic at parrot.com]
> Sent: Tuesday, January 31, 2012 5:05 PM
> To: Xu, Hong
> Cc: dedekind1 at gmail.com; linux-mtd at lists.infradead.org;
dwmw2 at infradead.org
> Subject: Re: [PATCH] MTD: atmel_nand: Update driver to support
Programmable
> HW ECC controller
> 
[...]
> > +       while (err_nbr) {
> > +               byte_pos = (pmerrloc_readl_el(host->pmerrloc_base,
i) - 1)
> / 8;
> > +               bit_pos = (pmerrloc_readl_el(host->pmerrloc_base, i)
- 1)
> % 8;
> 
> Hello,
> do you really need to read this register twice?
> 

Not needed actually. It'll be fixed in next version.

> > +               dev_info(host->dev, "PMECC correction, byte_pos: %d
"
> > +                       "bit_pos: %d\n", byte_pos, bit_pos);
> > +
> > +               if (byte_pos < (sector_size + extra_bytes)) {
> > +                       tmp = sector_size + pmecc_readl(host->ecc,
SADDR);
> > +                       if (byte_pos < tmp) {
> 
> > +                               if (*(buf + byte_pos) & (1 <<
bit_pos))
> > +                                       *(buf + byte_pos) &=
> > +                                               (0xFF ^ (1 <<
bit_pos));
> > +                               else
> > +                                       *(buf + byte_pos) |= (1 <<
> bit_pos);
> 
> why not replace the 5 lines above with:  *(buf + byte_pos) ^= 1 <<
bit_pos;

Thanks. It'll be modified in next version.

> 
> > +                       } else {
> > +                               if (*(buf + byte_pos + ecc_size) &
> > +                                    (1 << bit_pos))
> > +                                       *(buf + byte_pos + ecc_size)
&=
> > +                                               (0xFF ^ (1 <<
bit_pos));
> > +                               else
> > +                                       *(buf + byte_pos + ecc_size)
|=
> > +                                               (1 << bit_pos);
> 
> same idea:   *(buf + byte_pos + ecc_size) ^= 1 << bit_pos;
> 

Ditto.

BR,
Eric

[...]
> BR,
> --
> Ivan