On Tue, Feb 21, 2017 at 1:20 PM, Boris Brezillon
[off-list ref] wrote:

On Tue, 21 Feb 2017 13:02:21 +0200
Andy Shevchenko [off-list ref] wrote:

quoted

On Tue, Feb 21, 2017 at 12:26 PM, Boris Brezillon
[off-list ref] wrote:

quoted

On Tue, 21 Feb 2017 12:03:45 +0200
Andy Shevchenko [off-list ref] wrote:

So, it's a matter of taste.

Yes, and I'm not objecting this.

quoted

quoted

quoted

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)

Because I don't want to duplicate the code done in
atmel_nand_pdata_get_gpio() each time I have to convert a GPIO number
into a GPIO descriptor, and that is needed to support platforms that
haven't moved to DT yet

They should use GPIO lookup tables.

We don't encourage people to use platform data anymore.

We have unified device properties for something like "timeout-us", we
have look up tables when you need specifics like pwm, gpio, pinctrl,
...

Abusing platform data with pointers is also not welcome.

quoted

(in this case, avr32).

It's dead de facto.

When last time did you compile kernel for it? What was the version of kernel?
Did it get successfully?

When are we going to remove avr32 support from kernel completely?

I'll let Nicolas answer that one.

In any case it's discouraging to use platform data for GPIOs and plain
GPIO pin numbering.

Note that I sometime prefer to keep (1 << X).

Example:

#define PMECC_CFG_READ_OP                       (0 << 12)
#define PMECC_CFG_WRITE_OP                      (1 << 12)

I understand that.

Okay, so the code in pmecc.c. See, it's hard to follow a review when
you don't comment inline.

It's hard to review (n+1) thousands of LOC.

quoted

quoted

quoted

8. Have you checked what kernel library provides?

I think so, but again, this is really vague, what kind of
open-coded functions do you think could be replaced with core libraries
helpers?

I dunno, I'm asking you. Usually if I see a pattern I got a clue to
check lib/ and similar places. From time to time I discover something
new and interesting there.

If you're talking about the code in pmecc.c, yes, I already mentioned
in the header that it should be reworked to use some helpers from
lib/bch.c, but that's not the point of this series, and is left as
future improvements.

OK.

quoted

Yes, because my point is *split* this to be reviewable.

And how do you do with new drivers?

To be more pedantic the new drivers do not have "minus" thousands LOC.

Do you ask people to split their
submissions in micro changes?

To logical ones.

I'm regularly reviewing drivers that are
several thousands LOC, and I don't ask people to split things just
because it's too long. When I ask them to split in different commits,
it's because they are doing several unrelated changes at once.

What did prevent you to:
1. Introduce new driver
2. Switch to new driver
3. Remove old one.

...if you are not splitting it in the first place?

Note that I considered refactoring the existing driver in smaller
steps, but it's almost impossible, because the code is too messy and I
would end up with a huge series of patches that is not easier to review.

I can object this, but it will be no point except waste of time to
this discussion.

It's good that you considered several options. I suppose someone who
is on topic can do comprehensive review.

-- 
With Best Regards,
Andy Shevchenko