On Wed, 1 Dec 2010, Charles Manning wrote:

quoted text
> Signed-off-by: Charles Manning <cdhmanning@gmail.com>
> ---
>  fs/yaffs2/yaffs_checkptrw.c |  420 +++++++++++++++++++++++++++++++++++++++++++
>  fs/yaffs2/yaffs_checkptrw.h |   33 ++++
>  fs/yaffs2/yaffs_ecc.c       |  298 ++++++++++++++++++++++++++++++
>  fs/yaffs2/yaffs_ecc.h       |   44 +++++
>  4 files changed, 795 insertions(+), 0 deletions(-)
>  create mode 100644 fs/yaffs2/yaffs_checkptrw.c
>  create mode 100644 fs/yaffs2/yaffs_checkptrw.h
>  create mode 100644 fs/yaffs2/yaffs_ecc.c
>  create mode 100644 fs/yaffs2/yaffs_ecc.h
> 

Just a few small comments below.


[...]
quoted text
> +int yaffs2_get_checkpt_sum(struct yaffs_dev *dev, u32 * sum)
> +{
> +	u32 composite_sum;
> +	composite_sum = (dev->checkpt_sum << 8) | (dev->checkpt_xor & 0xFF);
> +	*sum = composite_sum;
> +	return 1;
> +}

Why not get rid of the redundant local variable and simply do

  *sum = (dev->checkpt_sum << 8) | (dev->checkpt_xor & 0xFF);

??


[...]
quoted text
> +int yaffs2_checkpt_wr(struct yaffs_dev *dev, const void *data, int n_bytes)
> +{
> +	int i = 0;
> +	int ok = 1;
> +
> +	u8 *data_bytes = (u8 *) data;

'data' is a 'const void *' and this casts away const. Probably fine, just 
makes my toes curl.


[...]
quoted text
> +int yaffs2_checkpt_rd(struct yaffs_dev *dev, void *data, int n_bytes)
> +{
> +	int i = 0;
> +	int ok = 1;
> +	struct yaffs_ext_tags tags;
> +
> +	int chunk;
> +	int realigned_chunk;
> +
> +	u8 *data_bytes = (u8 *) data;

here you are not casting away const, but since 'data' is a void pointer 
the cast is just completely superfluous and should just go away.


[...]
quoted text
> +			if (dev->checkpt_cur_block < 0)
> +				ok = 0;
> +			else {

Nitpicking, but this should be 

  if (dev->checkpt_cur_block < 0) {
    ok = 0;
  } else {
  ...

curly brace on both branches since it's used on one of them (according to 
CodingStyle).


[...]
quoted text
> +int yaffs_checkpt_close(struct yaffs_dev *dev)
> +{
> +
> +	if (dev->checkpt_open_write) {
> +		if (dev->checkpt_byte_offs != 0)
> +			yaffs2_checkpt_flush_buffer(dev);
> +	} else if (dev->checkpt_block_list) {
> +		int i;
> +		for (i = 0;
> +		     i < dev->blocks_in_checkpt
> +		     && dev->checkpt_block_list[i] >= 0; i++) {
> +			int blk = dev->checkpt_block_list[i];
> +			struct yaffs_block_info *bi = NULL;
> +			if (dev->internal_start_block <= blk
> +			    && blk <= dev->internal_end_block)
> +				bi = yaffs_get_block_info(dev, blk);
> +			if (bi && bi->block_state == YAFFS_BLOCK_STATE_EMPTY)
> +				bi->block_state = YAFFS_BLOCK_STATE_CHECKPOINT;
> +			else {
> +				/* Todo this looks odd... */
> +			}

1) nitpicking about curly braces again (see above).
2) try grep'ing the source for "todo", "xxx", "fixme" and similar, then 
check the age of those comments. If things like this are not addressed (by 
other than a "todo" comment) on initial commit they stand a good chance of 
never getting addressed...


-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/