[PATCH v2 1/2] mtd: nand: add 'use_oob' argument to NAND {read,write}_page interfaces

[Shmulik Ladkani]

Hi Brian,

On Tue, 24 Apr 2012 20:42:02 -0700 Brian Norris <computersforpeace at gmail.com> wrote:
> > (still not completely happy with my suggestion, as the boolean
> > approach is not that elegant as you previously noted...)
> 
> I agree, but it seems like the primary alternative (using a 'uint8_t
> *oob' parameter) necessitates many hacks such that 'oob' is mostly
> reduced to a boolean. So I think I'll try to make the best of a
> boolean, since we need *some* solution.

Agreed, it is the primary (and reasonable) alternative.
(I was only hoping for an even better suggestion, but none so far)

> > IMO having a parameter called 'use_oob', but not using the parameter
> > value is very confusing.
> >
> > Maybe we need an indication that states "hey, you may skip filling the
> > oob_poi if you don't want to, that's okay (but if you'd like to fill
> > it for your own purposes, it's okay too)"...
> > More of a 'oob_must_be_read' or 'oob_expected' for the read case (and
> > 'oob_must_be_written' or 'oob_was_placed' for write).
> >
> > (tried to come-up with better names, no success ;-)
> 
> I'm not sure about the naming. I thought 'use_oob' was better than
> 'has_oob'. I also was working for name that:
> 
> (1) could be used for both read and write
> (2) wasn't too long
> 
> If I ignore (1) and add in:
> 
> (3) an indication that "you may skip filling the oob_poi if you don't
> want to" (only when the boolean is false)
> 
> then I might come up with something like:
> 
> for 'read' functions: oob_expected
> for 'write' functions: oob_provided
> 
> Or maybe something that hits all (1)-(3): oob_required. Then, saying
> "oob_required = false" doesn't mean that you *can't* use oob_poi.

I like all of your suggestions (oob_expected, oob_provided, and
also oob_required). Better than 'has_oob' or 'use_oob'.

These names implicitly suggest that we don't have to immediately port
those implementors that do fill/use 'oob_poi' even if it's not really
required.

> >> @@ -1139,7 +1142,7 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip,
> >>    uint8_t *ecc_code = chip->buffers->ecccode;
> >>    uint32_t *eccpos = chip->ecc.layout->eccpos;
> >>
> >> -   chip->ecc.read_page_raw(mtd, chip, buf, page);
> >> +   chip->ecc.read_page_raw(mtd, chip, buf, use_oob, page);
> >
> > Should pass 'true' instead of 'use_oob'.
> > That's because 'nand_read_page_swecc' later uses 'chip->oob_poi' and
> > expects it to be read.
> > (relevant for few other calls, e.g. nand_write_page_swecc)
> 
> Right, thanks. By "few other calls," did you mean that only
> nand_write_page_swecc and nand_read_page_swecc were relevant? I didn't
> find any others I missed that were used the same way.

Took another look at nand_base.c, only these two (for some reason I
initially thought, mistakenly, that the pattern is more widespread).

> > BTW you used 'bool', however convention for some other boolean parameters
> > around nand_base.c is 'int'.
> > (no preference, just like things consistent)
> 
> For reference, are the instances of 'int sndcmd' the only cases of
> int-used-as-boolean that you found? bool is clearer IMO, but for the
> sake of consistency I can switch. But now I just noticed docg4.c and
> denali.c use 'bool'...

I only looked at nand_base.c. Seems like 'int' more used: int getchip,
int allowbbt, int cached, int raw.

Also, grepping 'bool' within include/linux/mtd/* yielded just one
result; obviously there are interfaces that get or return a boolean.

I'm not familiar with current coding preference (kernel global or mtd
local), just prefer to be consistent.

Regards,
Shmulik