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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
