Re: [PATCH 07/29] Staging: yaffs2: yaffs_checkptrw: Add files

[Jesper Juhl]

On Mon, 1 Nov 2010, Tracey Dent wrote:

> Adding files to yaffs2 directory.
> 
> Signed-off-by: Tracey Dent <tdent48227@xxxxxxxxx>
> ---
>  drivers/staging/yaffs2/yaffs_checkptrw.c |  400 ++++++++++++++++++++++++++++++
>  drivers/staging/yaffs2/yaffs_checkptrw.h |   34 +++
>  2 files changed, 434 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/yaffs2/yaffs_checkptrw.c
>  create mode 100644 drivers/staging/yaffs2/yaffs_checkptrw.h
> 
> diff --git a/drivers/staging/yaffs2/yaffs_checkptrw.c b/drivers/staging/yaffs2/yaffs_checkptrw.c
> new file mode 100644
> index 0000000..82d6874
> --- /dev/null
> +++ b/drivers/staging/yaffs2/yaffs_checkptrw.c
[snip]
> +static void yaffs2_checkpt_find_erased_block(yaffs_dev_t *dev)
> +{
> +	int  i;
> +	int blocks_avail = dev->n_erased_blocks - dev->param.n_reserved_blocks;
> +	T(YAFFS_TRACE_CHECKPOINT,
> +		(TSTR("allocating checkpt block: erased %d reserved %d avail %d next %d "TENDSTR),
> +		dev->n_erased_blocks, dev->param.n_reserved_blocks, blocks_avail, dev->checkpt_next_block));
> +
> +	if (dev->checkpt_next_block >= 0 &&
> +			dev->checkpt_next_block <= dev->internal_end_block &&
> +			blocks_avail > 0) {
> +

Ok, perhaps I've been writing C++ apps in userspace too much, but on first 
impulse I'd say that the 'i' variable should be declared here in this 
scope where it's first used and not in global function scope.

> +		for (i = dev->checkpt_next_block; i <= dev->internal_end_block; i++) {

[snip]
> +static void yaffs2_checkpt_find_block(yaffs_dev_t *dev)
> +{
> +	int  i;
> +	yaffs_ext_tags tags;
> +
> +	T(YAFFS_TRACE_CHECKPOINT, (TSTR("find next checkpt block: start:  blocks %d next %d" TENDSTR),
> +		dev->blocks_in_checkpt, dev->checkpt_next_block));
> +
> +	if (dev->blocks_in_checkpt < dev->checkpt_max_blocks)

same here.

> +		for (i = dev->checkpt_next_block; i <= dev->internal_end_block; i++) {


[snip]
> +int yaffs2_checkpt_open(yaffs_dev_t *dev, int writing)
> +{
> +
> +

Small thing, but I've seen this a lot in these patches - double blank 
lines (or more) where either a single one or zero would do just as well 
(or actually better).

It's a small thing, but the number of (relevant) code lines visible on the 
screen at once actually matters. Sure, a blank line between blocks often 
makes sense in order to visually seperate one block of code from another, 
but two (or more) lines often don't serve a purpose except prevent more 
relevant stuff to being visible - especially in cases like this where it's 
at the start of a function, I really don't see the point.


> +int yaffs2_get_checkpt_sum(yaffs_dev_t *dev, __u32 *sum)
> +{
> +	__u32 composite_sum;
> +	composite_sum =  (dev->checkpt_sum << 8) | (dev->checkpt_xor & 0xFF);
> +	*sum = composite_sum;
> +	return 1;
> +}

Why not

int yaffs2_get_checkpt_sum(yaffs_dev_t *dev, __u32 *sum)
{
     *sum  = (dev->checkpt_sum << 8) | (dev->checkpt_xor & 0xFF);
     return 1;
}

???


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

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