Re: [PATCH 01/08] staging:dgap: remove digi debug and tracing code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I have some comments on this patch.  It's all minor stuff you can fix in
a later patch.  Don't redo this one.  Staging has more relaxed standards
on whitespace than mm/.  We don't want to slow you down by making you
redo stuff and I don't want to review it more than once if I can avoid
that.

On Fri, Feb 28, 2014 at 12:42:08PM -0500, Mark Hounschell wrote:
> The original debug and tracing code is no longer required.
> This patch removes it
> 
> Signed-off-by: Mark Hounschell <markh@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
> -	if ((tty->count == 1) && (un->un_open_count != 1)) {
> +	if ((tty->count == 1) && (un->un_open_count != 1))
>  		/*
>  		 * Uh, oh.  tty->count is 1, which means that the tty
>  		 * structure will be freed.  un_open_count should always
> @@ -2648,27 +2479,19 @@ static void dgap_tty_close(struct tty_struct *tty, struct file *file)
>  		 * one, we've got real problems, since it means the
>  		 * serial port won't be shutdown.
>  		 */
> -		APR(("tty->count is 1, un open count is %d\n", un->un_open_count));
>  		un->un_open_count = 1;
> -	}

Multi-line indent blocks get curly braces for readability.

> @@ -4909,8 +4587,8 @@ static void dgap_do_wait_for_bios(struct board_t *brd)
>  	/* Gave up on board after too long of time taken */
>  	err1 = readw(addr + SEQUENCE);
>  	err2 = readw(addr + ERROR);
> -	APR(("***WARNING*** %s failed diagnostics.  Error #(%x,%x).\n",
> -		brd->name, err1, err2));
> +	pr_warn("dgap: %s failed diagnostics.  Error #(%x,%x).\n",
> +		brd->name, err1, err2);
>  	brd->state = BOARD_FAILED;
>  	brd->dpastatus = BD_NOBIOS;
>  }

In the end we're going to want all these things to be dev_warn() and not
pr_warn().  You can remove the "dgap:" prefix because dev_warn() adds a
prefix.  pr_warn() has a way to add a prefix as well, but it's moot
since this will eventually be dev_warn().

Speaking of pr_warn(), also this:

> +               if ((brd->type == PPCM) && (true_count == 64))
> +                       pr_warn("dgap: %s configured for %d ports, has %d ports.\nPlease make SURE the EBI cable running from the card\nto each EM module is plugged into EBIIN!\n",
> +                               brd->name, brd->nasync, true_count);

You can't put newlines in the middle of a print statement.  The lines
are supposed to be prefixed or it messes up dmesg slightly.

> @@ -5980,11 +5623,8 @@ static int dgap_param(struct tty_struct *tty)
>  
>  		if ((iindex >= 0) && (iindex < 4) && (jindex >= 0) && (jindex < 16)) {
>  			baud = bauds[iindex][jindex];
> -		} else {
> -			DPR_IOCTL(("baud indices were out of range (%d)(%d)",
> -				iindex, jindex));
> +		} else
>  			baud = 0;
> -		}
>  

I think "} else" will trigger a new checkpatch.pl warning.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux