Re: [PATCH v4 0/4] optimize and clean-up of OMAP NAND and ELM driver

[Brian Norris <computersforpeace@gmail.com>]

Hi Pekon,

On Mon, Dec 02, 2013 at 07:51:50PM +0000, Pekon Gupta wrote:
> Just a ping..
> Do you have any further updates on this series ?
> If this is clean, I just want to rebase and send last pending series for 
> omap2.c driver.

I'm sorry to delay this long, but this patch series is still rather
impenetrable for a third-party reader, and so I really can't give my sign-off.

For one, you don't give very good patch titles; 3 of the 4 have "optimize" in
the title, when in fact that "optimize" does not mean anything about
optimization, but rather, "unification", "consolidation", "cleanup", or
something else entirely, depending on which part of the patch you're talking
about... which brings me to the next point:

Patches 1 and 4 are doing much more than one thing. Your commit messages
-- rather than describing succintly a single change being made -- are a
description of a list of things that you did, some of which are quite
independent.

For instance, consider this list from patch 1:

>         This patch optimizes omap_elm_correct_data() in following ways:
>         (1) Removes dependency on specific reserved-byte (0x00) in OOB area,
>             instead Erased-page is identified by matching calc_ecc with a
>             pre-defined ECC syndrome of all(0xFF) data

This sounds like a self-contained task that should be done in its own patch.

>         (2) merges common code for BCH4_ECC and BCH8_ECC for scalability.

This sounds like a second independent task, so it should be in its own patch.

>         (3) handles incorrect elm_error_location beyond data+oob buffer.

This sounds independent. And is this a bugfix? The description doesn't make
this clear, and I certainly can't pinpoint this fix, amidst the other 3 tasks
here.

>         (4) removes check_erased_page(): Bit-flips in erased-page are handled
>             in same way as for programmed-page

This may be interrelated with (1), and so needs to be in the same patch.

Anyway, it's up to you how to order these patches, due to dependencies
between the task you list. But I really can't review patch 1 like this,
and patch 4 suffers a few of the same sort of problems.

If you need clarification on how you can best rework/resubmit this work,
please ask. I don't want this series to drag on to version 12+, so let's
focus on how to get this right now.

Brian