From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752009AbdBULCb (ORCPT ); Tue, 21 Feb 2017 06:02:31 -0500 Received: from mail-qk0-f193.google.com ([209.85.220.193]:34891 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751623AbdBULCX (ORCPT ); Tue, 21 Feb 2017 06:02:23 -0500 MIME-Version: 1.0 In-Reply-To: <20170221112641.6276c001@bbrezillon> References: <1487593718-20752-1-git-send-email-boris.brezillon@free-electrons.com> <1487593718-20752-2-git-send-email-boris.brezillon@free-electrons.com> <20170220213803.7ba5591e@bbrezillon> <20170220215009.0ecbf5a1@bbrezillon> <20170221090610.6d531b94@bbrezillon> <20170221112641.6276c001@bbrezillon> From: Andy Shevchenko Date: Tue, 21 Feb 2017 13:02:21 +0200 Message-ID: Subject: Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver To: Boris Brezillon Cc: Richard Weinberger , "open list:MEMORY TECHNOLOGY..." , Nicolas Ferre , Alexandre Belloni , Haavard Skinnemoen , Hans-Christian Egtvedt , "linux-kernel@vger.kernel.org" , Wenyou Yang , Josh Wu , David Woodhouse , Brian Norris , Marek Vasut , Cyrille Pitchen , linux-arm Mailing List , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , devicetree Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 21, 2017 at 12:26 PM, Boris Brezillon wrote: > On Tue, 21 Feb 2017 12:03:45 +0200 > Andy Shevchenko wrote: >> 1. For example, >> >> #define ATMEL_NFC_CMD(pos, cmd) ((cmd) << >> (((pos) * 8) + 2)) > > Well, I like to explicitly put parenthesis even when operator > precedence guarantees the order of the calculation ('*' is preceding > '+'). That's my point. I'm not a LISP programmer. Personally I think it makes readability worse. > For the parenthesis around (cmd) and (pos), they are required to > guarantee that things like ATMEL_NFC_CMD(x + y, cmd) are working > correctly. I know that. >> >> 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) > > 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? >> 5. BIT() macro: > We could probably use BIT() in a few places. There are more places including data structures assignments. > Again, this has been copied from the old driver. I'll have a closer > look. Exactly. You overlooked due to enormous LOC in the one change. See my point below. >> 7. Question to all that distribution or whatever functions, don't you >> have a common helper? Or each vendor requires different logic behind >> it? > > What are you talking about? nand_chip hooks? That long arithmetic with some data. >> 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. >> 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. > > I'm not asking you to review the whole driver, but you started to > comment on the code without pointing clearly to the things you wanted > me to address. Yes, because my point is *split* this to be reviewable. -- With Best Regards, Andy Shevchenko