From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kmu-office.ch ([2a02:418:6a02::a2]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1fiyC4-0005C3-6H for linux-mtd@lists.infradead.org; Fri, 27 Jul 2018 08:33:49 +0000 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Date: Fri, 27 Jul 2018 10:33:29 +0200 From: Stefan Agner To: Miquel Raynal Cc: Boris Brezillon , Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , linux-mtd@lists.infradead.org, Wenyou Yang , Josh Wu , Lucas Stach Subject: Re: [PATCH v5 12/17] mtd: rawnand: tegra: convert driver to nand_scan() In-Reply-To: <20180727100640.715d6cce@xps13> References: <20180725133152.30898-1-miquel.raynal@bootlin.com> <20180725133152.30898-13-miquel.raynal@bootlin.com> <20180726200120.0dd4bcb7@bbrezillon> <81b38ddf26e981adf6a0564708d2954e@agner.ch> <20180727100640.715d6cce@xps13> Message-ID: List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 27.07.2018 10:06, Miquel Raynal wrote: > Hi Stefan, > > Stefan Agner wrote on Fri, 27 Jul 2018 09:13:09 +0200: > >> On 26.07.2018 20:01, Boris Brezillon wrote: >> > On Thu, 26 Jul 2018 18:29:36 +0200 >> > Stefan Agner wrote: >> > >> >> On 25.07.2018 15:31, Miquel Raynal wrote: >> >> > Two helpers have been added to the core to do all kind of controller >> >> > side configuration/initialization between the detection phase and the >> >> > final NAND scan. Implement these hooks so that we can convert the driver >> >> > to just use nand_scan() instead of the nand_scan_ident() + >> >> > nand_scan_tail() pair. >> >> > >> >> >> >> While the patch looks technically correct, I wonder whether the driver >> >> now does what we expect it from attach logically... >> >> >> >> E.g. shouldn't we get the wp_gpio in attach? >> > >> > Well, this series does things mechanically to avoid breaking drivers (we >> > just move all the code between ident and tail into the attach hook), >> > but any resource that is not needed for the identification phase and is >> > tied to the NAND chip could/should be requested in the attach hook (the >> > WP pin is such a resource). >> >> Ok, that makes completely sense and I agree with that approach! However, >> it is not obvious when looking at the series. >> >> Can we mention that fact in the commit log, e.g. something like: >> >> "To avoid breaking this patch converts the driver mechanically by just >> moving all the code between ...ident and ..tail into the attach hook. >> Ideally driver should request all resources tied to the NAND chip in the >> attach hook." > > As Boris said, the whole change was somehow mechanic, I already had a > hard time understanding what each driver wanted to achieve between > those two functions, I did not dig any further as it was already very > time consuming. A cleanup of many drivers would be appreciated though. Yeah sure, and your work is very much appreciated! Until closer look and Boris' confirmation it really wasn't clear to me whether things are on purpose the way they are or because this patch is seen as a first (mechanical) step... > > As this would apply to the 30 patches of the series and because I > already merged all of them, I think I'll pass for this one, even if I > completely agree with the request. Ok, at least it is documented on the ML now :-) -- Stefan