[lkml]   [2010]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 3/8] Add yaffs2 file system: guts code
    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.

    > +
    > + (TSTR("Out of temp buffers at line %d, other held by lines:"),
    > + line_no));
    > + for (i = 0; i < YAFFS_N_TEMP_BUFFERS; i++)
    > + (TSTR(" %d "), dev->temp_buffer[i].line));
    > +

    The tracing functions are rather obscure. I would recommend dropping
    them all for now, in order to get the code included. At a later
    stage, you can add standard trace points.

    > + return YMALLOC(dev->data_bytes_per_chunk);

    In general, don't wrap standard kernel API functions with your
    own abstractions, just use kmalloc here for instance.

    It is rather annoying when you want to understand code and it
    calls nonstandard functions that do almost the same that the
    standard API does.

    If you find something lacking in the existing API, feel free
    to make suggestions for improving it. Either it is a good idea
    and everyone will be happy about the useful new interface,
    or it is a bad idea and you shouldn't be using it in the first


     \ /
      Last update: 2010-11-30 23:27    [W:0.021 / U:7.044 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site