From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Tue, 10 May 2016 20:19:09 +0200 From: Boris Brezillon To: Jorge Ramirez Cc: computersforpeace@gmail.com, dwmw2@infradead.org, matthias.bgg@gmail.com, robh@kernel.org, linux-mtd@lists.infradead.org, xiaolei.li@mediatek.com, linux-mediatek@lists.infradead.org, erin.lo@mediatek.com, daniel.thompson@linaro.org, blogic@openwrt.org Subject: Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND Message-ID: <20160510201909.5ee62b48@bbrezillon> In-Reply-To: <573224F5.4030003@linaro.org> References: <1461946642-1842-1-git-send-email-jorge.ramirez-ortiz@linaro.org> <1461946642-1842-3-git-send-email-jorge.ramirez-ortiz@linaro.org> <20160510141335.442d3d7b@bbrezillon> <5731F5E3.3020607@linaro.org> <573224F5.4030003@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tue, 10 May 2016 14:14:13 -0400 Jorge Ramirez wrote: > On 05/10/2016 10:53 AM, Jorge Ramirez wrote: > > On 05/10/2016 08:13 AM, Boris Brezillon wrote: > >>> + > >>> >+void mtk_ecc_disable(struct mtk_ecc *ecc, struct mtk_ecc_config > >>> *config) > >> If you save the mode somewhere in mtk_ecc (or just write in both ENC > >> and DEC registers), you won't need this extra config arg here. > > > > ok. > > actually as I was implementing this change (and it is a simple change to > do) I am not sure that it is a good idea to store the state of the > engine in the ecc_driver (ie: codec_x active sort of flag). And writing > to both set of registers looks ugly to me. > > the way the nand driver is written, it is the nand driver the one that > controls the ecc engine 'states' (init, enable_x,disable_x). Why do we > want the ecc engine to remember that it was running the encoder or the > decoder? I think keeping the parameter in this function makes more > sense, that way the ecc driver itself completely stateless. > > however let me see if there is a register I can poke to find out the > active codec (then I could remove the config parameter that you pointed > out without having to store state). I should be able to just read the > ECC control register to determine this. > > Ok, that's not so important either. It's just that it's more common to have an enable/config() function that takes some argument to let the driver know how it should operate and the disable() function that just disables everything, no matter what config you previously passed. If it's really inconvenient for you, then let's just keep it as it is right now. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com