Messages in this thread | | | From | Charles Manning <> | Subject | Re: [PATCH 3/8] Add yaffs2 file system: guts code | Date | Mon, 6 Dec 2010 14:50:41 +1300 |
| |
On Wednesday 01 December 2010 11:23:53 you wrote: > On Tuesday 30 November 2010 22:57:29 Charles Manning wrote: > > I think I made these comments before, not sure what happened to them... > > > + > > +/* Robustification (if it ever comes about...) */ > > +static void yaffs_retire_block(struct yaffs_dev *dev, int flash_block); > > +static void yaffs_handle_chunk_wr_error(struct yaffs_dev *dev, int > > nand_chunk, + int erased_ok); > > +static void yaffs_handle_chunk_wr_ok(struct yaffs_dev *dev, int > > nand_chunk, + const u8 * data, > > + const struct yaffs_ext_tags *tags); > > +static void yaffs_handle_chunk_update(struct yaffs_dev *dev, int > > nand_chunk, + const struct yaffs_ext_tags *tags); > > It would be better to reorder the functions in each file so that > you don't need forward declarations. This generally makes reading > the code easier because it is what people expect to see. It > also makes it clearer where you have possible recursions in the code.
Hmmm.. I too prefer minimal use of forward declaration. Some of them are because I copied the layout of existing kernel code which uses fwd declarations a lot. eg. fs/jffs2/dir.c and many of the examples in Rubini & Corbet.
> > > + > > + T(YAFFS_TRACE_BUFFERS, > > + (TSTR("Out of temp buffers at line %d, other held by lines:"), > > + line_no)); > > + for (i = 0; i < YAFFS_N_TEMP_BUFFERS; i++) > > + T(YAFFS_TRACE_BUFFERS, > > + (TSTR(" %d "), dev->temp_buffer[i].line)); > > + > > + T(YAFFS_TRACE_BUFFERS, (TSTR(" " TENDSTR)));
> The tracing functions are rather obscure. I would recommend dropping > them all for now, in order to get the code included.
Yup that was a very ugly bunch of hackery to make WinCE unicode work.
I think I can pull the var-arg-foo to replace these with
yaffs_trace(YAFFS_TRACE_BUFFERS, "Out of temp buffers at line %d, other held by lines:",line_no); for (i = 0; i < YAFFS_N_TEMP_BUFFERS; i++) yaffs_trace(YAFFS_TRACE_BUFFERS," %d ", dev->temp_buffer[i].line); yaffs_trace(YAFFS_TRACE_BUFFERS, "\n");
Would that be OK?
I am loath to have to pull out useful code then plug it back in again.
> At a later > stage, you can add standard trace points. > > > + return YMALLOC(dev->data_bytes_per_chunk);
I'm getting rid of those..
The reason for thew wrapping was to make portable code.
I'm replacing these with kmalloc() and then providing kmalloc() which is just a wrapped malloc() for non-Linux use.
I am hoping to reduce this to YCHAR, _Y() and maybe one or two others.
-- Charles
| |