Overall, all of these look fine, I only have a few minor cleanup
comments here and there...


On Sat, Feb 11, 2006 at 03:52:27PM +0100, Hansjoerg Lipp wrote:
> --- linux-2.6.16-rc2/drivers/isdn/gigaset/gigaset.h   1970-01-01 
> 01:00:00.000000000 +0100
> +++ linux-2.6.16-rc2-gig/drivers/isdn/gigaset/gigaset.h       2006-02-11 
> 15:20:26.000000000 +0100
> @@ -0,0 +1,938 @@
> +/* Siemens Gigaset 307x driver
> + * Common header file for all connection variants
> + *
> + * Written by Stefan Eilers <[EMAIL PROTECTED]>
> + *        and Hansjoerg Lipp <[EMAIL PROTECTED]>
> + *
> + * Version: $Id: gigaset.h,v 1.97.4.26 2006/02/04 18:28:16 hjlipp Exp $

$Id: isn't needed within the kernel tree :)

> + * 
> ===========================================================================
> + */
> +
> +#ifndef GIGASET_H
> +#define GIGASET_H
> +
> +#include <linux/config.h>
> +#include <linux/kernel.h>
> +#include <linux/compiler.h>
> +#include <linux/types.h>
> +#include <asm/atomic.h>
> +#include <linux/spinlock.h>
> +#include <linux/isdnif.h>
> +#include <linux/usb.h>
> +#include <linux/skbuff.h>
> +#include <linux/netdevice.h>
> +#include <linux/ppp_defs.h>
> +#include <linux/timer.h>
> +#include <linux/interrupt.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/list.h>

asm #includes should be at the end of the list.

> +
> +#define GIG_VERSION {0,5,0,0}
> +#define GIG_COMPAT  {0,4,0,0}
> +
> +#define MAX_REC_PARAMS 10                         /* Max. number of params 
> in response string */
> +#define MAX_RESP_SIZE 512                         /* Max. size of a response 
> string */
> +#define HW_HDR_LEN 2                              /* Header size used to 
> store ack info */

No tabs here, yet in other places in the file, you have tabs.  Try using
them everywhere.  Also try to follow the 80 column rule where ever
possible, as per the Documentation/CodingStyle file.

> +#define MAX_EVENTS 64                          /* size of event queue */
> +
> +#define RBUFSIZE 8192
> +#define SBUFSIZE 4096                                /* sk_buff payload size 
> */
> +
> +#define MAX_BUF_SIZE (SBUFSIZE - 2)          /* Max. size of a data packet 
> from LL */
> +#define TRANSBUFSIZE 768                     /* bytes per skb for 
> transparent receive */
> +
> +/* compile time options */
> +#define GIG_MAJOR 0
> +
> +#define GIG_MAYINITONDIAL
> +#define GIG_RETRYCID
> +#define GIG_X75
> +
> +#define MAX_TIMER_INDEX 1000
> +#define MAX_SEQ_INDEX   1000
> +
> +#define GIG_TICK (HZ / 10)

What is this needed for?  Timeouts should be in real units, right?

> +/* redefine syslog macros to prepend module name instead of entire source 
> path */
> +/* The space before the comma in ", ##" is needed by gcc 2.95 */

gcc 2.95 isn't supported by the kernel anymore :)

> +#undef info
> +#define info(format, arg...) printk(KERN_INFO "%s: " format "\n", 
> THIS_MODULE ? THIS_MODULE->name : "gigaset_hw" , ## arg)

Care to use the dev_info(), dev_err() and other dev_* friends instead of
rolling your own?  It gives you a much easier and standardised way of
identifying the driver and individual device that the message is
happening for.

thanks,

greg k-h


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Reply via email to