Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 10 May 2016 14:14:13 -0400
Jorge Ramirez <jorge.ramirez-ortiz@linaro.org> 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