[PATCH 0/3] mtd: nand: add enum nand_ecc_algo

[Rafał Miłecki]

On 23 March 2016 at 10:00, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> On Tue, 22 Mar 2016 23:54:11 +0100
> Rafał Miłecki <zajec5 at gmail.com> wrote:
>
>> Some time ago I started working on a new "nand-ecc-algo" property to allow
>> specifying ECC algorithm for hardware ECC mode as well.
>> Boris pointed out it's becoming a bit messy that way as we already have value
>> NAND_ECC_SOFT_BCH.
>> I suggested deprecating "soft_bch" value from nand-ecc-mode property and got
>> Boris agreed and no objections from others. So there is how I want to implement
>> this.
>> If you agree on this way & apply my patches, I'll start modifying NAND drivers
>> (similarly to the nandsim) and then will try to drop NAND_ECC_SOFT_BCH except
>> for handling backward compatibility.
>> Then finally we should be able to add "nand-ecc-algo" property support properly.
>
> I agree with patch 1 and 3, but I think we should create an
> of_get_nand_ecc_algo() helper right now instead of relying on the
> ecc->mode value to set ecc->algo (I'm referring to patch 2).

Thanks for the review!

I wanted to avoid adding support for "nand-ecc-algo" at this point.
This would result with setting ecc.algo which isn't used yet, so this
could be confusing.
However I agree with your point that adding this magic code to
nand_dt_init, just to replace it with of_get_nand_ecc_algo in the
future isn't too clever.

I'll send V2 soon, let's see if will agree on its implementation.


> This way you'll be able to completely drop NAND_ECC_SOFT_BCH value when
> all drivers have been patched.
> BTW, even if we deprecate the DT property, we still have to support the
> old format, so this extra "nand-ecc-mode" parsing will remain anyway.

This is what I mainly meant in "then will try to drop
NAND_ECC_SOFT_BCH except for handling backward compatibility" :)


> And has already said ealier, I think we should also move the ecc layout
> info somewhere else, and only leave ECC_NONE, ECC_HW and ECC_SOFT in
> nand_ecc_modes_t enum. I'm not asking you to do that though, just
> clarifying the separation I'd like to see ;).

Ack, hopefully I'll find time for this later.

-- 
Rafał