From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756769AbbBLQ03 (ORCPT ); Thu, 12 Feb 2015 11:26:29 -0500 Received: from smtp09.smtpout.orange.fr ([80.12.242.131]:53321 "EHLO smtp.smtpout.orange.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756752AbbBLQ02 (ORCPT ); Thu, 12 Feb 2015 11:26:28 -0500 X-ME-Helo: beldin X-ME-Date: Thu, 12 Feb 2015 17:26:22 +0100 X-ME-IP: 109.220.218.8 From: Robert Jarzmik To: Antoine Tenart Cc: Boris Brezillon , thomas.petazzoni@free-electrons.com, zmxu@marvell.com, linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, ezequiel.garcia@free-electrons.com, jszhang@marvell.com, computersforpeace@gmail.com, dwmw2@infradead.org, linux-arm-kernel@lists.infradead.org, sebastian.hesselbarth@gmail.com Subject: Re: [PATCH 5/9] mtd: pxa3xx_nand: add support for the Marvell Berlin nand controller References: <1422367816-4257-1-git-send-email-antoine.tenart@free-electrons.com> <1422367816-4257-6-git-send-email-antoine.tenart@free-electrons.com> <20150209005503.3fad39c7@bbrezillon> <871tlx7dgu.fsf@free.fr> <20150211163343.GH11169@kwain> X-URL: http://belgarath.falguerolles.org/ Date: Thu, 12 Feb 2015 17:26:20 +0100 In-Reply-To: <20150211163343.GH11169@kwain> (Antoine Tenart's message of "Wed, 11 Feb 2015 17:33:43 +0100") Message-ID: <87k2zn5c5v.fsf@free.fr> User-Agent: Gnus/5.130008 (Ma Gnus v0.8) Emacs/24.3.92 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Antoine Tenart writes: >> All these ifs per variant will add complexity to the current driver, won't they >> ? > > Given the current state of this driver I believe this would be a better > idea to first rework it to use the nand framework properly. Then it will > be possible to have a look on what we have, to decide whether or not a > mrvl-nand-lib is needed. I don't fully agree to delay. You're taking code, making sometimes copy-paste, for the berlin specific functions, and I think you're taking the bugs as well, which duplicates the fix effort. For example, in nand_cmdfunc_berlin(): + if (info->reg_ndcr & NDCR_DWIDTH_M) + column /= 2; This is I think a copy-paste from its twin nand_cmdfunc(). Now consider what happens if the command is _not_ a read command, but an ONFI READID command (ONFI being specified by column=0x20 if memory serves me well). This is the reason I don't really like this patch. And this is also the reason why I believe you're in a perfect timing to improve things, by grouping the common function, and keeping the bugs in only one place, not many. Cheers. -- Robert