On Fri, Nov 19, 2004 at 08:34:42PM -0600, Al Borchers wrote:
> +static unsigned char ti_fw_3410[] = {
> +/* struct ImageHdr {
> + * WORD wLength;
> + * BYTE bCheckSum;
> + * };
> + */
Um, WORD? BYTE? Ick, no. Please at least change the comment here and
in the other place you have it. Just put the same structure that you
properly have in the .h file here.
> diff -urN -X dontdiff
> linux-2.6.10-rc2-bk3.orig/drivers/usb/serial/ti_usb_3410_5052.c
> linux-2.6.10-rc2-bk3.new/drivers/usb/serial/ti_usb_3410_5052.c
> --- linux-2.6.10-rc2-bk3.orig/drivers/usb/serial/ti_usb_3410_5052.c
> 1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.10-rc2-bk3.new/drivers/usb/serial/ti_usb_3410_5052.c
> 2004-11-19 19:18:26.000000000 -0600
> @@ -0,0 +1,1840 @@
> +/* vi: ts=8 sw=8
> + *
> + * TI 3410/5052 USB Serial Driver
> + *
> + * Copyright (C) 2004 Texas Instruments
> + *
> + * This driver is based on the Linux io_ti driver, which is
> + * Copyright (C) 2000-2002 Inside Out Networks
> + * Copyright (C) 2001-2002 Greg Kroah-Hartman
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * For questions or problems with this driver, contact Texas Instruments
> + * technical support, or Al Borchers <[EMAIL PROTECTED]>, or
> + * Peter Berger <[EMAIL PROTECTED]>.
> + *
> + * This driver needs this hotplug script in /etc/hotplug/usb/ti_usb_3410_5052
> + * to set the device configuration.
> + *
> + * #!/bin/bash
> + *
> + * BOOT_CONFIG=1
> + * ACTIVE_CONFIG=2
> + *
> + * CONFIG_PATH=/sys${DEVPATH%/?*}/bConfigurationValue
> + *
> + * if test "$ACTION" = "add"
> + * then
> + * if test `cat $CONFIG_PATH` -eq $BOOT_CONFIG
> + * then
> + * echo $ACTIVE_CONFIG > $CONFIG_PATH
> + * fi
> + * fi
Why not put this script in /etc/hotplug.d/usb/ and add a check for your
device ids in it? That way you aren't relying on the hotplug scripts
(and it will work properly for systems that don't use the hotplug module
loading scripts, and with the next revision of the hotplug module
loading package, see lkml and linux-hotplug for a preview of that.)
> +/* Function Declarations */
> +
> +static int ti_startup(struct usb_serial *serial);
> +static void ti_shutdown(struct usb_serial *serial);
> +static int ti_open(struct usb_serial_port *port, struct file *file);
> +static void ti_close(struct usb_serial_port *port, struct file *file);
> +static int ti_write(struct usb_serial_port *port, const unsigned char *data,
> + int count);
> +static int ti_write_room(struct usb_serial_port *port);
> +static int ti_chars_in_buffer(struct usb_serial_port *port);
> +static void ti_throttle(struct usb_serial_port *port);
> +static void ti_unthrottle(struct usb_serial_port *port);
> +static int ti_ioctl(struct usb_serial_port *port, struct file *file,
> unsigned int cmd, unsigned long arg);
> +static void ti_set_termios(struct usb_serial_port *port,
> + struct termios *old_termios);
> +static int ti_tiocmget(struct usb_serial_port *port, struct file *file);
> +static int ti_tiocmset(struct usb_serial_port *port, struct file *file,
> + unsigned int set, unsigned int clear);
> +static void ti_break(struct usb_serial_port *port, int break_state);
> +static void ti_interrupt_callback(struct urb *urb, struct pt_regs *regs);
> +static void ti_bulk_in_callback(struct urb *urb, struct pt_regs *regs);
> +static void ti_bulk_out_callback(struct urb *urb, struct pt_regs *regs);
> +
> +static void ti_recv(struct device *dev, struct tty_struct *tty,
> + unsigned char *data, int length);
> +static void ti_send(struct ti_port *tport);
> +static int ti_set_mcr(struct ti_port *tport, unsigned int mcr);
> +static int ti_get_lsr(struct ti_port *tport);
> +static int ti_get_serial_info(struct ti_port *tport,
> + struct serial_struct __user *ret_arg);
> +static int ti_set_serial_info(struct ti_port *tport,
> + struct serial_struct *new_arg);
> +static void ti_handle_new_msr(struct ti_port *tport, __u8 msr);
> +
> +static void ti_drain(struct ti_port *tport, unsigned long timeout, int
> flush);
> +
> +static void ti_stop_read(struct ti_port *tport, struct tty_struct *tty);
> +static int ti_restart_read(struct ti_port *tport, struct tty_struct *tty);
> +
> +static int ti_command_out_sync(struct ti_device *tdev, __u8 command,
> + __u16 moduleid, __u16 value, __u8 *data, int size);
> +static int ti_command_in_sync(struct ti_device *tdev, __u8 command,
> + __u16 moduleid, __u16 value, __u8 *data, int size);
> +
> +static int ti_write_byte(struct ti_device *tdev, unsigned long addr,
> + __u8 mask, __u8 byte);
> +
> +static int ti_download_firmware(struct ti_device *tdev,
> + unsigned char *firmware, unsigned int firmware_size);
Surely some of these forward declarations can be removed by just
shuffling the code around?
> +/* circular buffer */
> +static struct ti_buf *ti_buf_alloc(unsigned int size);
> +static void ti_buf_free(struct ti_buf *tb);
> +static void ti_buf_clear(struct ti_buf *tb);
> +static unsigned int ti_buf_data_avail(struct ti_buf *tb);
> +static unsigned int ti_buf_space_avail(struct ti_buf *tb);
> +static unsigned int ti_buf_put(struct ti_buf *tb, const char *buf,
> + unsigned int count);
> +static unsigned int ti_buf_get(struct ti_buf *tb, char *buf,
> + unsigned int count);
Why not use the in-kernel implemtation of a circular buffer? I've been
thinking about that for the other usb-serial drivers that also roll
their own data structure for this. We shouldn't add yet-another one if
we can help it.
> +/* Global Data */
> +
No global data? delete the comment :)
> +/* module parameters */
> +static int debug;
> +static int low_latency = TI_DEFAULT_LOW_LATENCY;
> +static int closing_wait = TI_DEFAULT_CLOSING_WAIT;
> +static ushort vendor_3410[TI_EXTRA_VID_PID_COUNT];
> +static int vendor_3410_count;
> +static ushort product_3410[TI_EXTRA_VID_PID_COUNT];
> +static int product_3410_count;
> +static ushort vendor_5052[TI_EXTRA_VID_PID_COUNT];
> +static int vendor_5052_count;
> +static ushort product_5052[TI_EXTRA_VID_PID_COUNT];
> +static int product_5052_count;
> +
> +/* supported devices */
> +static struct usb_device_id ti_id_table_3410[1+TI_EXTRA_VID_PID_COUNT+1] = {
> + { USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
> +};
Why have a number for the array size?
> +static struct usb_device_id ti_id_table_5052[4+TI_EXTRA_VID_PID_COUNT+1] = {
> + { USB_DEVICE(TI_VENDOR_ID, TI_5052_BOOT_PRODUCT_ID) },
> + { USB_DEVICE(TI_VENDOR_ID, TI_5152_BOOT_PRODUCT_ID) },
> + { USB_DEVICE(TI_VENDOR_ID, TI_5052_EEPROM_PRODUCT_ID) },
> + { USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
> +};
Same here?
Ah, you are trying to do what the visor driver did in adding a new
device id. Just put two empty '{ }' at the end of the structure then.
And I really wish the usb core could handle this, like pci does, instead
of forcing every driver to do it themselves... but people are working
on that too...
> +/* Module */
> +
> +MODULE_AUTHOR(TI_DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(TI_DRIVER_DESC);
> +MODULE_LICENSE("GPL");
No MODULE_VERSION() ?
> diff -urN -X dontdiff
> linux-2.6.10-rc2-bk3.orig/drivers/usb/serial/ti_usb_3410_5052.h
> linux-2.6.10-rc2-bk3.new/drivers/usb/serial/ti_usb_3410_5052.h
> --- linux-2.6.10-rc2-bk3.orig/drivers/usb/serial/ti_usb_3410_5052.h
> 1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.10-rc2-bk3.new/drivers/usb/serial/ti_usb_3410_5052.h
> 2004-11-19 15:27:01.000000000 -0600
Is another header file really needed?
Other than those minor comments, looks good.
thanks,
greg k-h
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://productguide.itmanagersjournal.com/
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
