Re: [PATCH v4] mtd: nand: Expand nand_ecc_layout, deprecate ioctl ECCGETLAYOUT

[Artem Bityutskiy <dedekind1@gmail.com>]

On Mon, 2010-08-30 at 13:20 +0300, Artem Bityutskiy wrote:
> On Tue, 2010-08-24 at 18:12 -0700, Brian Norris wrote:
> > My e-mail address has changed, since I am no longer working at Broadcom.
> > I will still be able to track messages to my old account if the MTD mailing
> > list is CC'd.
> 
> Oh, does it mean you will stop loving MTD and we won't see steady flow
> of improvements for you? :-( BTW, I think you have been doing great job
> - MTD subsystem needs love badly!
> 
> > Note that on the same subject (different thread) David suggested our new
> > struct be allocated dynamically:
> 
> Yes, but I agree with your arguments and also think it is ok to keep it
> simple for now. So I'm applying this to my l2 tree, and then it is up to
> dwmw2 to take it or not.
> 
> But I also have some requests about commentaries, so if you can re-send
> another version of this patch, it would be nice. But I take this one for
> now, it is good enough.
>  
> > +/*
> > + * Copies (and truncates, if necessary) data from the larger struct,
> > + * nand_ecclayout, to the smaller, deprecated layout struct,
> > + * nand_ecclayout_user. This is necessary only to suppport the deprecated
> > + * API ioctl ECCGETLAYOUT while allowing all new functionality to use
> > + * nand_ecclayout flexibly (i.e. the struct may change size in new
> > + * releases without requiring major rewrites).
> > + */
> 
> I think a similar comment should exist in linux/mtd/mtd.h. Indeed, that
> file is our API with user-space, and our users will probably look at it,
> and it is nice to document the situation with 'struct
> nand_ecclayout_user' there.
> 
> > +#define MTD_MAX_OOBFREE_ENTRIES_LARGE	32
> > +#define MTD_MAX_ECCPOS_ENTRIES_LARGE	448
> > +#define MTD_MAX_ECCPOS_ENTRIES_OLD	64	/* Previous maximum */
> > +/*
> > + * Correct ECC layout control structure. This replaces old nand_ecclayout
> > + * (mtd-abi.h) that is exported via ECCGETLAYOUT ioctl. It should be expandable
> > + *  in the future simply by the above macros.
> > + */
> 
> I find this comment confusing. First, "Correct ECC" -> "Internal ECC",
> because one could think "Correct ECC structure" means something like
> "structure which describes ECC corrections" or something like that.
> 
> Also, I'd avoid mentioning things like "old nand_ecclayout", because
> with time this will be confusing. Could you please imagine that you are
> an MTD newbie reading the code in 2012 - you have no idea what was
> happening in the past in the ancient 2010. 

Brian,

would you address the small things I noticed in a follow-up patch?
-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)