Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

[Andy Shevchenko]

On Tue, Feb 21, 2017 at 1:20 PM, Boris Brezillon
<boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 21 Feb 2017 13:02:21 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>> On Tue, Feb 21, 2017 at 12:26 PM, Boris Brezillon
>> <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
>> > On Tue, 21 Feb 2017 12:03:45 +0200
>> > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> So, it's a matter of taste.

Yes, and I'm not objecting this.

>> >> 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.
>>
>> > (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.

>> >> 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.

>> 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