On Tue, Feb 21, 2017 at 10:06 AM, Boris Brezillon
[off-list ref] wrote:

On Tue, 21 Feb 2017 01:54:37 +0200
Andy Shevchenko [off-list ref] wrote:

quoted

On Tue, Feb 21, 2017 at 1:40 AM, Andy Shevchenko
[off-list ref] wrote:

quoted

On Mon, Feb 20, 2017 at 10:50 PM, Boris Brezillon
[off-list ref] wrote:

quoted

On Mon, 20 Feb 2017 21:38:03 +0100
Boris Brezillon [off-list ref] wrote:

quoted

On Mon, 20 Feb 2017 22:27:17 +0200
Andy Shevchenko [off-list ref] wrote:

quoted

On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
[off-list ref] wrote:

quoted

 drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
 drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------

Does -M -C help you?
At least it would help reviewers

No it doesn't, because files were not just moved around using git mv,
it's a complete rewrite of the driver. IIUC, you're about to review
this submission, or are you just trolling like last time?

My bad, I mistaken you with someone else. Sorry for being harsh, but my
explanation stands ;-).

No problem. I was asking since it so big and on first glance looks
like a partial copy (I dunno if parameter to -C makes it somehow
useful), though I can't review this. It's too big to me. Sorry I'm
really not trolling, just didn't read commit message carefully.

Okay, I very quickly looked into the code, what I noticed
- you like extra parens and empty lines in some cases (not big deal)

Can you point specific places where you think these are not needed?

1. For example,

#define ATMEL_NFC_CMD(pos, cmd)                        ((cmd) <<
(((pos) * 8) + 2))

 *events ^= (status & *events);

 (((x) * 0x4) + 0x28)

  memset(&si[1], 0, sizeof(s16) * ((2 * strength) - 1));

Perhaps more in the code. I'm not a LISP programmer.

2. For empty lines it's solely matter of style, I don't care. My motto
"less LOC better, but keep common sense in mind".

quoted

- some functions perhaps might have been refactored to have common
pieces in error handling, though I didn't read core carefully.

Again, be more precise.

3. I don't remember anymore, sorry. Something I would refactor.

quoted

Most important part I have noticed is a GPIO request.
I didn't get why you almost repeat gpiod_get() in case of platform data?
Shouldn't we have GPIO look up table?
Can we use builtin device properties (for GPIO and/or overall)?

Sorry but I don't get it. Can give an example of what you'd like me to
do?

4. First of all, why do you need this function in the first place?

+struct gpio_desc *
+atmel_nand_pdata_get_gpio(struct atmel_nand_controller *nc, int gpioid,
+                         const char *name, bool active_low,
+                         enum gpiod_flags flags)

5. BIT() macro:

   const unsigned int k = 1 << deg(poly);
   unsigned int nn = (1 << mm) - 1;

6. Why this casting (unsigned int) ?

 dev_dbg(pmecc->dev,
                       "Bit flip in %s area, byte %d: 0x%02x -> 0x%02x\n",
                       area, byte, *ptr, (unsigned int)(*ptr ^ BIT(bit)));

7. Question to all that distribution or whatever functions, don't you
have a common helper? Or each vendor requires different logic behind
it?

8. Have you checked what kernel library provides?

And I believe there are still issues like those. After, who is on
topic, might even find some logical and other issues...

P.S. TBH, so big change is unreviewable in meaningful time. To have a
comprehensive review I, for example, spend ~1h/250LOC, and
~2.5h/1000LOC, I would estimate ~4h/2000LOC. Imagine one to spend one
day for this. Any volunteer? Not me.

-- 
With Best Regards,
Andy Shevchenko