On Tue, Oct 19, 2004 at 08:47:18PM +0200, Hermann Kneissel wrote:
> +
> +#ifdef CONFIG_USB_SERIAL_DEBUG
> + static int debug = 1;
> +#else
> + static int debug;
> +#endif
Not needed, just use module_param() like the other usb-serial drivers do
today.
> +/* function prototypes */
<snip>
These can be removed by restructuring the code.
> +/* Parameters that may be passed into the module. */
> +static int vendor = -1;
> +static int product = -1;
I see you copied the visor driver's logic to do this. Do you really
want to do this? And you might want to mention where you copied all of
this code from in your comment block at the top of the file to be nice.
> +static struct usb_device_id id_table [] = {
> + /* same device id for GPSmap60C and GPS18 */
> + { USB_DEVICE(GARMIN_VENDOR_ID, 3 ),
> + .driver_info = (kernel_ulong_t)&gpsmap_probe },
> + { }, /* optional parameter entry */
> + { } /* Terminating entry */
> +};
> +
> +static struct usb_device_id id_table_combined [] = {
> + { USB_DEVICE(GARMIN_VENDOR_ID, 3) },
> + { }, /* optional parameter entry */
> + { } /* Terminating entry */
> +};
Why 2 tables? You only need one. And as you don't have different probe
functions, you can get rid of the driver_info stuff here.
> +#undef dbg
> +#define dbg_drv(garmin_data_p, format, arg...) \
> + do { \
> + if (debug&DEBUG_DRIVER) \
> + dev_printk(KERN_DEBUG, &garmin_data_p->port->dev, \
> + format "\n" , ## arg); \
> + } while (0)
> +
> +#define dbg_gsp(garmin_data_p, format, arg...) \
> + do { \
> + if (debug&DEBUG_PROT_GSP) \
> + dev_printk(KERN_DEBUG, &garmin_data_p->port->dev, \
> + format "\n", ## arg); \
> + } while (0)
> +
> +#define dbg_queue(garmin_data_p, format, arg...) \
> + do { \
> + if (debug&DEBUG_QUEUE) \
> + dev_printk(KERN_DEBUG, &garmin_data_p->port->dev, \
> + format "\n", ## arg); \
> + } while (0)
> +
> +#define dbg_nat(garmin_data_p, format, arg...) \
> + do { \
> + if (debug&DEBUG_PROT_NATIVE) \
> + dev_printk(KERN_DEBUG, &garmin_data_p->port->dev, \
> + format "\n" , ## arg); \
> + } while (0)
Please just use the dev_dbg(), dev_err() and friends. Do you really
need new dbg macros?
> +static inline int garmin_debug_data(int debug,
> + struct device *dev,
> + const char *function,
> + int size,
> + const unsigned char *data)
> +{
> + int i;
> +
> + if (debug) {
> + dev_printk(KERN_DEBUG, dev,
> + "%s - length = %d, data = ", function, size);
> + for (i = 0; i < size; ++i)
> + printk ("%.2x ", data[i]);
> + printk ("\n");
> + }
> + return debug;
> +}
There's a function in usb-serial.h for this. Why not use it?
> +static inline int get_int(const unsigned char *buf)
> +{
> + return (0xFF & buf[0]) | ((0xFF & buf[1]) << 8) |
> + ((0xFF & buf[2]) << 16) | ((0xFF & buf[3]) << 24);
> +}
Um, use the proper endian bitmove functions instead?
> +static inline int get_short(const unsigned char *buf)
> +{
> + return (0xFF & buf[0]) | ((0xFF & buf[1]) << 8);
> +}
Same here.
> +static inline void set_int(unsigned char *buf, int value)
> +{
> + buf[0] = 0xFF & value;
> + buf[1] = 0xFF & (value >> 8);
> + buf[2] = 0xFF & (value >> 16);
> + buf[3] = 0xFF & (value >> 24);
> +}
And here.
> +static struct list_head *list_first(struct list_head *head)
> +{
> + return head->next;
> +}
Heh, why is this needed? What wrong with just using the pointer?
> +/* send a packet back to the tty */
> +static int gsp_send_packet(struct garmin_data * garmin_data_p, __u8 pkt_id,
> + unsigned data_length, unsigned char *data)
> +{
> + __u8 pkt[512];
Don't put big structures like this on the stack (you do it in other
places too.) Make it dynamic. Is it also really needed?
> +static int garmin_write_room (struct usb_serial_port *port)
> +{
> + /*
> + * We really can take anything the user throws at us
> + * but let's pick a nice big number to tell the tty
> + * layer that we have lots of free space
> + */
> + return 2048;
> +}
> +
> +
> +static int garmin_chars_in_buffer (struct usb_serial_port *port)
> +{
> + /*
> + * We can't really account for how much data we
> + * have sent out, but hasn't made it through to the
> + * device, so just tell the tty layer that everything
> + * is flushed.
> + */
> + return 0;
> +}
Are these two comments really correct for this driver? Again, nice
cut-and-paste-without-attribution...
> +static void garmin_write_bulk_callback (struct urb *urb, struct pt_regs *regs)
> +{
> + struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
> + struct garmin_data * garmin_data_p
> + = (struct garmin_data *) usb_get_serial_port_data(port);
The casts aren't needed here (or anywhere else you do it.) Feel free to
drop it to reduce the code.
> +static int gpsmap_probe (struct usb_serial *serial,
> + const struct usb_device_id *id)
> +{
> + return 0;
> +}
What's this function here for?
> --- linux-2.6.9/drivers/usb/serial/Makefile 2004-10-18 23:53:22.000000000 +0200
> +++ linux-2.6.9-gps/drivers/usb/serial/Makefile 2004-10-19 20:24:52.000312993
> +0200
> @@ -29,6 +29,7 @@
> obj-$(CONFIG_USB_SERIAL_PL2303) += pl2303.o
> obj-$(CONFIG_USB_SERIAL_SAFE) += safe_serial.o
> obj-$(CONFIG_USB_SERIAL_VISOR) += visor.o
> +obj-$(CONFIG_USB_SERIAL_GARMIN) += garmin_gps.o
> obj-$(CONFIG_USB_SERIAL_WHITEHEAT) += whiteheat.o
> obj-$(CONFIG_USB_SERIAL_XIRCOM) += keyspan_pda.o
Put the files in alphabetical order. Any reason for putting it after
the visor driver?
thanks,
greg k-h
-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
