[PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Brian Norris]

Hi again Huang,

I had some initial comments a week ago (below) which haven't been
addressed. Can you please address them? I see that you are continuing
to send out v2 patches based on l2-mtd, where Artem already pulled in
your first 3 patches. But further work depends on the answer to my
questions for the first 3 patches.

On Tue, Apr 16, 2013 at 8:56 PM, Brian Norris
<computersforpeace at gmail.com> wrote:
> On Tue, Apr 16, 2013 at 7:10 PM, Huang Shijie <b32955 at freescale.com> wrote:
>> 于 2013年04月16日 21:55, Artem Bityutskiy 写道:
>>
>>> Unfortunately no-one reviewed your patches. Let me CC Brian.
>>>
>> I CCed Brian already.
>> I also hope some one could reviews this patch set.
>
> Things go by the wayside at times... sigh. Sorry for the delay. I do
> plan to fully review this series.
>
> Initial comments:
>
> What are the semantics for these new ecc_strength and ecc_size fields
> for chips which aren't encoded in nand_ids.c (or aren't
> ONFI-compliant)? We can't necessarily say that '0' means "no data
> provided" - it is perfectly valid to have a value of 0, e.g., for EZ
> NAND (or something like that) which manages its own ECC on-die. I
> think there are even a few words in the ONFI spec about it. Basically,
> I'm trying to point out that there is a difference between those chips
> which we can't/don't detect their ecc_{strength,size} and those which
> need no ECC.

We need to straighten out these semantics now. Perhaps the fields in
struct nand_chip can be signed, where -1 is "unknown" and 0 is "no ECC
required."

> Also, I might consider different names for these fields so that you
> don't just rely on the in-code comments to differentiate the nand_chip
> and nand_ecc_ctrl values. Perhaps min_ecc_strength and ecc_size?
> (can't think of a great distinguishing name for the second one)

The naming can be changed after your series, if it seems worth it.

> Finally, it's probably fine to increase NAND_MAX_OOBSIZE (and
> MTD_MAX_ECCPOS_ENTRIES_LARGE) yet again, but these really need to
> die... I have a (yet untested) patch series for killing
> NAND_MAX_OOBSIZE ready.
>
> I'll try to fully review the series within the week.
>
> Brian