linux-kernel - Re: [PATCH] Staging: comedi: Fix 80 characters limit and printk issues in unioxx5.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 4 Apr 2011 22:51:08 -0700
From:	Greg KH <greg@...ah.com>
To:	ellwync@...il.com
Cc:	gregkh@...e.de, arun.thomas@...il.com, julia@...u.dk,
	tj@...nel.org, john.d.sheehan@...il.com,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	Ellwyn C <ellwync@...glemail.com>
Subject: Re: [PATCH] Staging: comedi: Fix 80 characters limit and printk
 issues in unioxx5.c

On Tue, Mar 15, 2011 at 11:57:04PM +0000, ellwync@...il.com wrote:
> From: Ellwyn C <ellwync@...glemail.com>
> 
> This is a patch to the unioxx5.c file that fixes the 80 characters limit and
> printk warnings found by the checkpatch.pl tool
> 
> Signed-off-by: Ellwyn C <ellwync@...glemail.com>
> ---
>  drivers/staging/comedi/drivers/unioxx5.c |   61 +++++++++++++++++++-----------
>  1 files changed, 39 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/unioxx5.c b/drivers/staging/comedi/drivers/unioxx5.c
> index 598884e..0d91f53 100644
> --- a/drivers/staging/comedi/drivers/unioxx5.c
> +++ b/drivers/staging/comedi/drivers/unioxx5.c
> @@ -75,8 +75,10 @@ Devices: [Fastwel] UNIOxx-5 (unioxx5),
>  /* 'private' structure for each subdevice */
>  struct unioxx5_subd_priv {
>  	int usp_iobase;
> -	unsigned char usp_module_type[12];	/* 12 modules. each can be 70L or 73L */
> -	unsigned char usp_extra_data[12][4];	/* for saving previous written value for analog modules */
> +	unsigned char usp_module_type[12];
> +	/* 12 modules. each can be 70L or 73L */

No, it should be on the line before this, not after.

> +	unsigned char usp_extra_data[12][4];
> +	/* for saving previous written value for analog modules */
>  	unsigned char usp_prev_wr_val[3];	/* previous written value */
>  	unsigned char usp_prev_cn_val[3];	/* previous channel value */
>  };
> @@ -99,7 +101,8 @@ static int __unioxx5_digital_write(struct unioxx5_subd_priv *usp,
>  				   unsigned int *data, int channel, int minor);
>  static int __unioxx5_digital_read(struct unioxx5_subd_priv *usp,
>  				  unsigned int *data, int channel, int minor);
> -/* static void __unioxx5_digital_config(struct unioxx5_subd_priv* usp, int mode); */
> +/* static void __unioxx5_digital_config(struct unioxx5_subd_priv* usp, int mode)
> +; */

Don't put a ; on a new line, this is not needed at all.

>  static int __unioxx5_analog_write(struct unioxx5_subd_priv *usp,
>  				  unsigned int *data, int channel, int minor);
>  static int __unioxx5_analog_read(struct unioxx5_subd_priv *usp,
> @@ -139,8 +142,10 @@ static int unioxx5_attach(struct comedi_device *dev,
>  	dev->iobase = iobase;
>  	iobase += UNIOXX5_SUBDEV_BASE;
>  
> -	/* defining number of subdevices and getting they types (it must be 'g01')  */
> -	for (i = n_subd = 0, ba = iobase; i < 4; i++, ba += UNIOXX5_SUBDEV_ODDS) {
> +	/* defining number of subdevices and getting they types
> +	(it must be 'g01')  */
> +	for (i = n_subd = 0, ba = iobase; i < 4; i++, ba +=
> +	UNIOXX5_SUBDEV_ODDS) {

Ick, no, wrong way to indent this, sorry.

And see the proper way to handle multi-line comments, this isn't the way
to do this.

>  		id = inb(ba + 0xE);
>  		num = inb(ba + 0xF);
>  
> @@ -169,7 +174,7 @@ static int unioxx5_attach(struct comedi_device *dev,
>  			return -1;
>  	}
>  
> -	printk("attached\n");
> +	pr_info("attached\n");

This isnt needed at all.

>  	return 0;
>  }
>  
> @@ -181,7 +186,8 @@ static int unioxx5_subdev_read(struct comedi_device *dev,
>  	int channel, type;
>  
>  	channel = CR_CHAN(insn->chanspec);
> -	type = usp->usp_module_type[channel / 2];	/* defining module type(analog or digital) */
> +	type = usp->usp_module_type[channel / 2];
> +	/* defining module type(analog or digital) */

Same problem as above, and in other places in this patch.

Care to do it all over and resend?

thanks,

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