Hi,
On Sunday 27 January 2008, Borislav Petkov wrote:
From: Borislav Petkov [EMAIL PROTECTED]
Some member names are self-explanatory, so remove their respective
comments. Also, explain the exact purpose of struct members in comments
in the struct definition instead of using excessively long member names
thus replacing then with a shorter, more handy version.
Finally, remove unused members:
Could you factor out dead code removal to a separate (pre-)patch?
- last_frame_position: only being written to once
- firmware_revision: used once, remove from struct idetape_tape_t and deal
with
it locally
- firmware_revision_num: only written to once
- tape_still_time_begin: completely unused
- tape_still_time: never written to; remove corresponding code chunk
- uncontrolled_last_pipeline_head: only once written to
Signed-off-by: Borislav Petkov [EMAIL PROTECTED]
---
drivers/ide/ide-tape.c | 673 +++
1 files changed, 329 insertions(+), 344 deletions(-)
Even if this patch contains only trivial changes, the amount of them
and the fact that it intermixes different logical changes (shortening
names, dead code removal and comments beautification) makes it somehow
non-trivial to review.
General comment:
please have some mercy on the reviewer (in this case me ;) and spread
the changes across more patches (it should also be easier for you since
with more patches it is more likely that the more changes get applied
the first time and you will have less code to recast/resubmit).
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 4690f71..31edb0c 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -240,9 +240,9 @@ typedef struct idetape_stage_s {
} idetape_stage_t;
/*
- * Most of our global data which we need to save even as we leave the
- * driver due to an interrupt or a timer event is stored in a variable
- * of type idetape_tape_t, defined below.
+ * Most of our global data which we need to save even as we leave the driver
due
+ * to an interrupt or a timer event is stored in a variable of type
+ * idetape_tape_t, defined below.
hmm, this comment looks ugly now because of an extra long first line...
[...]
/* Next free packet command storage space */
- int pc_stack_index;
+ int pc_stack_idx;
struct request rq_stack[IDETAPE_PC_STACK];
/* We implement a circular array */
- int rq_stack_index;
+ int rq_stack_idx;
it is not worth doing it for the sake of two characters saved,
(especially when we take into the account that both variables are
removed by patch #26)
[...]
/* Current block */
- unsigned int first_frame_position;
- unsigned int last_frame_position;
+ unsigned int first_frm_pos;
'first_frame' would be more appropriate
unsigned int blocks_in_buffer;
this one is write-only, can be removed
[...]
- /* Current character device data transfer direction */
- idetape_chrdev_dir_t chrdev_direction;
+ /* current chrdev data transfer direction */
+ idetape_chrdev_dir_t chrdev_dir;
this change belongs to patch #21
- /*
- * Device information
- */
- /* Usually 512 or 1024 bytes */
- unsigned short tape_block_size;
+ /* tape block size, usually 512 or 1024 bytes */
+ unsigned short blk_sz;
how's about blk_size?
[...]
- struct request *active_data_request;
- /* Data buffer size (chosen based on the tape's recommendation */
+ /* active data request */
+ struct request *act_data_rq;
active_data_rq?
+ /* Data buffer size chosen based on the tape's recommendation */
int stage_size;
idetape_stage_t *merge_stage;
- int merge_stage_size;
+ int merge_stage_sz;
not worth it
[...]
/* The first stage which will be removed from the pipeline */
idetape_stage_t *first_stage;
- /* The currently active stage */
idetape_stage_t *active_stage;
- /* Will be serviced after the currently active request */
there is also 'first_stage' so the above comment seems valueable
idetape_stage_t *next_stage;
/* New requests will be added to the pipeline here */
idetape_stage_t *last_stage;
@@ -385,21 +366,16 @@ typedef struct ide_tape_obj {
/* Status/Action flags: long for set_bit */
unsigned long flags;
/* protects the ide-tape queue */
- spinlock_t spinlock;
+ spinlock_t que_lock;
'lock' or 'queue_lock' would be more appropriate
[...]
@@ -426,49 +400,39 @@ typedef struct ide_tape_obj {
int tape_head;
int last_tape_head;
- /*
- * Speed control at the tape buffers input/output
- */
- unsigned long insert_time;
- int insert_size;
- int insert_speed;
- int max_insert_speed;
- int measure_insert_time;
+ /* Speed control at the tape buffers input/output */
+ ulong ins_time;
please
