Greg KH wrote:

>On Fri, Mar 04, 2005 at 04:08:44PM -0500, Wen Xiong wrote:
>  
>
>>+/************************************************************************
>>+ * Structure used with ioctl commands for DIGI parameters.
>>+ ************************************************************************/
>>+struct digi_t {
>>+	unsigned short	digi_flags;		/* Flags (see above)	*/
>>+	unsigned short	digi_maxcps;		/* Max printer CPS	*/
>>+	unsigned short	digi_maxchar;		/* Max chars in print queue */
>>+	unsigned short	digi_bufsize;		/* Buffer size		*/
>>+	unsigned char	digi_onlen;		/* Length of ON string	*/
>>+	unsigned char	digi_offlen;		/* Length of OFF string	*/
>>+	char		digi_onstr[DIGI_PLEN];	/* Printer on string	*/
>>+	char		digi_offstr[DIGI_PLEN];	/* Printer off string	*/
>>+	char		digi_term[DIGI_TSIZ];	/* terminal string	*/
>>+};
>>    
>>
>
>Oops, don't use _t for a structure name please.
>
>  
>
>>+#ifndef __JSM_DRIVER_H
>>+#define __JSM_DRIVER_H
>>+
>>+#include <linux/kernel.h>
>>+#include <linux/version.h>
>>+#include <linux/types.h>	/* To pick up the varions Linux types */
>>+#include <linux/tty.h>		
>>+#include <linux/serial_core.h>
>>+#include <linux/serial_reg.h>
>>+#include <linux/interrupt.h>	/* For irqreturn_t type */
>>+#include <linux/module.h>	
>>+#include <linux/moduleparam.h>	
>>+#include <linux/kdev_t.h>	
>>+#include <linux/pci.h>
>>+#include <linux/pci_ids.h>
>>+#include <linux/device.h>
>>+#include <linux/config.h>
>>    
>>
>
>Don't put header files in header files if you can help it.  It really
>isn't needed here, and odds are, you are including files you don't
>really need for each of the different driver .c files.  The build will
>go faster if you don't do that.
>
>Also, check your ordering, some of those .h files already included the
>ones above it.
>
>  
>
>>+#include "digi.h"		/* Digi specific ioctl header */
>>    
>>
>
>Why do you have your own ioctls?  Please do not add any new ones to the
>kernel.
>
>  
>
>>+#define DRVSTR	"jsm"		/* Driver name string */
>>    
>>
>
>What is this for?
>
>  
>
>>+#define DPRINTK(nlevel, klevel, fmt, args...) \
>>+	(void)((DBG_##nlevel & debug) && \
>>+	printk(KERN_##klevel "%s: " fmt, \
>>+		__FUNCTION__, ## args)); 
>>    
>>
>
>Please use dev_dbg() or at least dev_printk() for this.  It provides
>consistancy with the rest of the kernel, and it helps identify your
>device much better.
>
>  
>
>>+#define JSM_MAJOR(x)	(imajor(x))
>>+#define JSM_MINOR(x)	(iminor(x))
>>    
>>
>
>Not needed, please don't use.
>
>  
>
>>+#ifndef _POSIX_VDISABLE
>>+#define	_POSIX_VDISABLE '\0'
>>+#endif
>>    
>>
>
>What would have defined that before?
>
>  
>
>>+/*
>>+ * Our Global Variables.
>>+ */
>>+extern struct	uart_driver jsm_uart_driver;
>>+extern struct	board_ops jsm_neo_ops;
>>+extern int	debug;
>>+extern int	rawreadok;
>>    
>>
>
>Both of these are bad global variable names.
>
>  
>
>>+extern int	jsm_driver_state;	/* The state of the driver	*/
>>+extern char	*jsm_driver_state_text[];/* Array of driver state text */
>>+
>>+extern spinlock_t jsm_board_head_lock;
>>+static LIST_HEAD(jsm_board_head);
>>    
>>
>
>Hm, static variable in a header file?  bad...
>
>  
>
>>+/*************************************************************************
>>+ *
>>+ * Prototypes for non-static functions used in more than one module
>>+ *
>>+ *************************************************************************/
>>+extern char *jsm_ioctl_name(int cmd);
>>+extern int get_jsm_board_number(void);
>>    
>>
>
>Bad name for a global function, put the "jsm" at the front please.
>
>thanks,
>
>greg k-h
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>  
>
Thanks for your reviewing!
Since some tools in Digi company need these new ioctls to access device 
driver. I still keep these new ioctls.

Thanks,
wendy