Re: [PATCH] drivers/block/floppy.c: stylistic cleanups

[Marcin Slusarz]

On Mon, Nov 30, 2009 at 08:13:40PM -0800, Joe Perches wrote:
> On Mon, 2009-11-30 at 09:28 -0800, Stephen Hemminger wrote:
> > Rather than playing with the dangling operator format which seems to be a coding
> > style that only David cares about. Why not go through and fix the really ugly old
> > drivers that need it. For a good horror experience, go look at the floppy driver.
> 
> Just for you Stephen, here's a cleaned up version.
> Now to see if it gets applied, which I rather doubt.
> 
> Changes:
> 
> Removed macro definitions and uses of
> 	IN, OUT, LAST_OUT, CLEARSTRUCT, and CHECK_RESET
> Used C99 initializers
> Removed assigns from if statements
> Converted printks without KERN_ levels to pr_info and pr_cont
> Removed unnecessary braces
> Used print_hex_dump
> Moved leading logical tests to end of previous line
> Surrounded still ugly CALL and ECALL macro with do {} while (0)
> 
> Checkpatch complaints before:
> total: 393 errors, 132 warnings, 4647 lines checked
> 
> after:
> total: 1 errors, 11 warnings, 5352 lines checked
> 
> Compile tested only, x86 allyesconfig
> 
> Signed-off-by: Joe Perches <joe@xxxxxxxxxxx>
> 
>  drivers/block/floppy.c | 1853 +++++++++++++++++++++++++++++++++---------------
>  1 files changed, 1279 insertions(+), 574 deletions(-)
> 
>
...
> @@ -515,11 +1035,12 @@ static DECLARE_WAIT_QUEUE_HEAD(fdc_wait);
>  static DECLARE_WAIT_QUEUE_HEAD(command_done);
>  
>  #define NO_SIGNAL (!interruptible || !signal_pending(current))
> -#define CALL(x) if ((x) == -EINTR) return -EINTR
> -#define ECALL(x) if ((ret = (x))) return ret;
> -#define _WAIT(x,i) CALL(ret=wait_til_done((x),i))
> -#define WAIT(x) _WAIT((x),interruptible)
> -#define IWAIT(x) _WAIT((x),1)
> +
> +#define CALL(x)		do { if ((x) == -EINTR) return -EINTR; } while (0)
> +#define ECALL(x)	do { if ((ret = (x))) return ret; } while (0)
> +#define _WAIT(x, i)	CALL(ret = wait_til_done((x), i))
> +#define WAIT(x)		_WAIT((x), interruptible)
> +#define IWAIT(x)	_WAIT((x), 1)

why not remove these macros too? (probably in a seperate patch)
macros which hide "return" are very annoying...

...
> @@ -909,10 +1431,12 @@ static int _lock_fdc(int drive, int interruptible, int line)
>  	return 0;
>  }
>  
> -#define lock_fdc(drive,interruptible) _lock_fdc(drive,interruptible, __LINE__)
> +#define lock_fdc(drive, interruptible)			\
> +	_lock_fdc(drive, interruptible, __LINE__)
>  
> -#define LOCK_FDC(drive,interruptible) \
> -if (lock_fdc(drive,interruptible)) return -EINTR;
> +#define LOCK_FDC(drive, interruptible)	    \
> +	if (lock_fdc(drive, interruptible)) \
> +		return -EINTR;

another annoying macro
  
>  /* unlocks the driver */
>  static inline void unlock_fdc(void)
...
> @@ -1809,7 +2339,10 @@ static void recalibrate_floppy(void)
>  	debugt("recalibrate floppy:");
>  	do_floppy = recal_interrupt;
>  	output_byte(FD_RECALIBRATE);
> -	LAST_OUT(UNIT(current_drive));
> +	if (UNIT(current_drive) < 0) {
> +		reset_fdc();
> +		return;
> +	}
>  }

unneeded return

>  
>  /*
...
> @@ -3479,143 +4010,163 @@ static int fd_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
>  	/* convert compatibility eject ioctls into floppy eject ioctl.
>  	 * We do this in order to provide a means to eject floppy disks before
>  	 * installing the new fdutils package */
> -	if (cmd == CDROMEJECT ||	/* CD-ROM eject */
> -	    cmd == 0x6470 /* SunOS floppy eject */ ) {
> +	if (cmd == CDROMEJECT || cmd == 0x6470) {

please add descriptive constant

>  		DPRINT("obsolete eject ioctl\n");
>  		DPRINT("please use floppycontrol --eject\n");
>  		cmd = FDEJECT;
>  	}

--
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/