Re: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Boris Brezillon <bbrezillon@xxxxxxxxxx>]

On Thu, 24 Jan 2019 15:52:03 +0000
"Bean Huo (beanhuo)" <beanhuo@xxxxxxxxxx> wrote:

> Hi, Boris
> 
> >> >>  struct nand_manufacturer_ops {
> >> >>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
> >> >> nand_manufacturer_ops {
> >> >>  	void (*cleanup)(struct nand_chip *chip);
> >> >>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
> >> >>  				      struct nand_onfi_params *p);
> >> >> +	int (*erase_pre)(struct nand_chip *chip, int page);  
> >> >
> >> >Let's move this hook to nand_chip and name it ->pre_erase() or  
> >> >->erase_preparation().  
> >> >  
> >>
> >> Can you tell us more reasons why moves this hook to nand_chip?  
> >
> >Because it's supposed to be a per-chip thing. I mean, not all of your chips have
> >this bug (at least I hope), so we want to only have a  
> >->pre_erase() implementation when it's really needed. The manufacturer  
> >specific ->init() hook will decide when it's appropriate to populate this -  
> >>pre_erase pointer based on the NAND chip id (or the NAND chip model).  
> >  
> 
> >> In my opinion, it is better to add this hook in nand_manufacturer_ops,
> >> since nand_manufacturer_ops Already exists in nand_chip, also this function is  
> >related to specific NAND manufacturer.  
> I still think, keep this hook in nand_manufacturer_ops is better, otherwise, just add one
> Function hook pointer in nand_chip, it is very weird.
> 

And I tell you it's not. What's the point of adding a hook at the
nand_manufacturer_ops level (which is the same for all NAND chips
produced by a manufacturer) if you then have to check whether a
specific chip has anything to do inside the hook itself. When we added
nand_manufacturer and the associated ops it was created to address
chip detection and initialization, nothing more. Anything that is
needed at runtime and is manufacturer specific should have a hook/flag
in nand_chip.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/