Oleg --
Comments in line below.
Quoting Oleg Verych <[EMAIL PROTECTED]>:
> pp-by: Oleg Verych
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 148
> +++++++++++++++++++++-------------
> drivers/usb/serial/ti_usb_3410_5052.h | 27 +++---
> 2 files changed, 109 insertions(+), 66 deletions(-)
>
> Index: linux-2.6.21-rc1/drivers/usb/serial/ti_usb_3410_5052.h
> ===================================================================
> --- linux-2.6.21-rc1.orig/drivers/usb/serial/ti_usb_3410_5052.h
> 2007-02-23
> 02:19:57.950025000 +0100
> +++ linux-2.6.21-rc1/drivers/usb/serial/ti_usb_3410_5052.h 2007-02-23
> 04:02:08.977190250 +0100
> @@ -21,8 +21,8 @@
> #define _TI_3410_5052_H_
>
> /* Configuration ids */
> -#define TI_BOOT_CONFIG 1
> -#define TI_ACTIVE_CONFIG 2
> +#define TI_BOOT_CONFIG 1 /* boot config to get
> firmware */
> +#define TI_ACTIVE_CONFIG 2 /* actual working device config */
>
> /* Vendor and product ids */
> #define TI_VENDOR_ID 0x0451
> @@ -206,19 +206,24 @@
> #define TI_CODE_DATA_ERROR 0x03
> #define TI_CODE_MODEM_STATUS 0x04
>
> -/* Download firmware max packet size */
> -#define TI_DOWNLOAD_MAX_PACKET_SIZE 64
> -
> -/* Firmware image header */
> -struct ti_firmware_header {
> - __le16 wLength;
> - __u8 bCheckSum;
> -} __attribute__((packed));
These came from reading the TI bootloader firmware sources.
The TI_DOWNLOAD_MAX_PACKET_SIZE is a constraint of the bootloader.
The header structure also matches the TI bootloader firmware.
I do not want to change these.
> -
> /* UART addresses */
> #define TI_UART1_BASE_ADDR 0xFFA0 /* UART 1 base address */
> #define TI_UART2_BASE_ADDR 0xFFB0 /* UART 2 base address */
> #define TI_UART_OFFSET_LCR 0x0002 /* UART MCR register offset */
> #define TI_UART_OFFSET_MCR 0x0004 /* UART MCR register offset */
>
> +/* Firmware */
> +#define TI_FW_PACKET_SIZE 64
> +#define TI_MAX_FIRMWARE_SIZE 16284
> +
> +#define ti_fw_file_3410 "umpf3410.i51"
> +#define ti_fw_file_5052 "umpf5052.i51"
These are not the right names for the firmware files. Please see
my patch at http://www.brimson.com/downloads/ti_usb_multitech-1.1.tgz
for the firmware files names we are using. These names are specific
to just one device.
> +
> +typedef union {
> + __le32 a; /* all */
> + struct {
> + __le32 sz : 16, cs : 8;
> + } d;
> +} ti_firmware_header_t;
The header is 24 bits, not 32. Why use a union here?
> +
> #endif /* _TI_3410_5052_H_ */
> Index: linux-2.6.21-rc1/drivers/usb/serial/ti_usb_3410_5052.c
> ===================================================================
> --- linux-2.6.21-rc1.orig/drivers/usb/serial/ti_usb_3410_5052.c
> 2007-02-23
> 02:20:04.314422750 +0100
> +++ linux-2.6.21-rc1/drivers/usb/serial/ti_usb_3410_5052.c 2007-02-23
> 05:00:54.477096862 +0100
> @@ -33,20 +33,15 @@
> #include <asm/semaphore.h>
> #include <linux/usb.h>
> #include <linux/usb/serial.h>
> -
> +#include <linux/firmware.h>
> #include "ti_usb_3410_5052.h"
> -#include "ti_fw_3410.h" /* firmware image for 3410 */
> -#include "ti_fw_5052.h" /* firmware image for 5052 */
No. This will break the driver for any user who upgrades the
kernel and does not have the firmware files. We talked about
this before--I do not want to do that. I want to keep the
firmware in the driver--we can load firmware if a firmware
file is found, but if not, we need to fall back to the firmware
image in the driver.
Again see my patch at http://www.brimson.com/downloads/ti_usb_multitech-1.1.tgz
to see how I want this done. I will submit this patch myself in a few days.
> -
>
> /* Defines */
>
> -#define TI_DRIVER_VERSION "v0.9"
> +#define TI_DRIVER_VERSION "v0.92"
> #define TI_DRIVER_AUTHOR "Al Borchers <[EMAIL PROTECTED]>"
> #define TI_DRIVER_DESC "TI USB 3410/5052 Serial Driver"
>
> -#define TI_FIRMWARE_BUF_SIZE 16284
> -
> #define TI_WRITE_BUF_SIZE 1024
>
> #define TI_TRANSFER_TIMEOUT 2
> @@ -143,8 +138,7 @@
> 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);
> +static int ti_fw_change(struct ti_device *tdev, const char *filename);
>
> /* circular buffer */
> static struct circ_buf *ti_buf_alloc(void);
> @@ -384,13 +378,8 @@
>
> /* if we have only 1 configuration, download firmware */
> if (dev->descriptor.bNumConfigurations == 1) {
> -
> - if (tdev->td_is_3410)
> - status = ti_download_firmware(tdev, ti_fw_3410,
> - sizeof(ti_fw_3410));
> - else
> - status = ti_download_firmware(tdev, ti_fw_5052,
> - sizeof(ti_fw_5052));
> + status = ti_fw_change(tdev, tdev->td_is_3410 ?
> + ti_fw_file_3410 : ti_fw_file_5052);
> if (status)
> goto free_tdev;
>
> @@ -1595,57 +1584,106 @@
> }
>
>
> -static int ti_download_firmware(struct ti_device *tdev,
> - unsigned char *firmware, unsigned int firmware_size)
> +static int ti_fw_change(struct ti_device *tdev, const char *filename)
> {
> - int status = 0;
> - int buffer_size;
> - int pos;
> - int len;
> - int done;
> - __u8 cs = 0;
> - __u8 *buffer;
> - struct usb_device *dev = tdev->td_serial->dev;
> - struct ti_firmware_header *header;
> - unsigned int pipe = usb_sndbulkpipe(dev,
> - tdev->td_serial->port[0]->bulk_out_endpointAddress);
> +#define udev (tdev->td_serial->dev)
> +#define fw_endpoint (tdev->td_serial->port[0]->bulk_out_endpointAddress)
I much prefer a local variable to a #define for this.
>
> + const struct firmware *fw_data_ptr;
> + size_t size;
> + int w;
>
> - buffer_size = TI_FIRMWARE_BUF_SIZE + sizeof(struct ti_firmware_header);
> - buffer = kmalloc(buffer_size, GFP_KERNEL);
> - if (!buffer) {
> - dev_err(&dev->dev, "%s - out of memory\n", __FUNCTION__);
> - return -ENOMEM;
> + dbg("%s() start; requesting firmware from userspace.",__FUNCTION__);
> +
> + w = request_firmware(&fw_data_ptr, filename, &udev->dev);
> + if (w) {
> + dev_err(&udev->dev, "userspace firmware helper failed.\n");
> + return w;
> }
>
> - memcpy(buffer, firmware, firmware_size);
> - memset(buffer+firmware_size, 0xff, buffer_size-firmware_size);
> + size = fw_data_ptr->size;
>
> - for(pos = sizeof(struct ti_firmware_header); pos < buffer_size; pos++)
> - cs = (__u8)(cs + buffer[pos]);
> + if (likely(size <= TI_MAX_FIRMWARE_SIZE)) {
> + ti_firmware_header_t h;
>
> - header = (struct ti_firmware_header *)buffer;
> - header->wLength = cpu_to_le16((__u16)(buffer_size - sizeof(struct
> ti_firmware_header)));
> - header->bCheckSum = cs;
> -
> - dbg("%s - downloading firmware", __FUNCTION__);
> - for (pos = 0; pos < buffer_size; pos += done) {
> - len = min(buffer_size - pos, TI_DOWNLOAD_MAX_PACKET_SIZE);
> - status = usb_bulk_msg(dev, pipe, buffer+pos, len, &done, 1000);
> - if (status)
> - break;
> + if (size % TI_FW_PACKET_SIZE) {
> + dev_err(&udev->dev, "firmware size isn't %u modulo.\n"
> + "Care to provide one.\n", TI_FW_PACKET_SIZE);
> + return -EIO;
> + }
> +
> + /* constructing header: size_LSB size_MSB CRCB */
> + h.a = 0x000000;
> + h.d.sz = cpu_to_le16(size);
> +
> + /* w is zero here and used as index */
> + do {
> + h.d.cs += fw_data_ptr->data[w++];
> + } while (w < size);
> +
> + /* using `w' as buffer */
> + w = (int) h.a;
> + dbg("cs: %#x; w: %#x", h.d.cs, w);
> + } else {
> + dev_err(&udev->dev,"firmware is too big.\n");
> + return -EFBIG;
> }
>
> - kfree(buffer);
> + dbg("starting downloading %#zx bytes of firmware.", size);
>
> - if (status) {
> - dev_err(&dev->dev, "%s - error downloading firmware, %d\n",
> __FUNCTION__,
> status);
> - return status;
> - }
> + do {
> + u8 *fw;
> + int gone;
> + unsigned int pipe = usb_sndbulkpipe(udev, fw_endpoint);
> +
> + /* XXX implement retry? */
> + w = usb_bulk_msg(udev, pipe, &w, 3, &gone, 1024);
> + if (gone != 3) {
> + if (!w)
> + w = -EIO;
> + break;
> + }
>
> - dbg("%s - download successful", __FUNCTION__);
> + /*
> + * 3-4 12-bit pages, this is not much for kmalloc(),
> + * why request_firmware() doesn't allocate with it?
> + */
> + fw = kmalloc(size, GFP_KERNEL);
> + if (!fw) {
> + w = -ENOMEM;
> + break;
> + }
>
> - return 0;
> + memcpy(fw, fw_data_ptr->data, size);
> +
> + size /= TI_FW_PACKET_SIZE;
> +
> + do {
> + w = usb_bulk_msg(udev, pipe, fw, TI_FW_PACKET_SIZE,
> + &gone, 1024);
> + if (gone != TI_FW_PACKET_SIZE) {
> + /*
> + * unless bulk_msg can be sent partially,
> + * `if' above can be removed
> + */
> + if (!w)
> + w = -EIO;
> + break;
> + } else {
> + fw += TI_FW_PACKET_SIZE;
> + }
> + } while (--size);
> +
> + kfree(fw);
> + } while (0);
> +
> + release_firmware(fw_data_ptr);
> +
> + dbg("%s() done with result %#x.\n",__FUNCTION__, w);
> +
> + return w;
> +#undef fw_endpoint
> +#undef dev
> }
>
>
>
> --
>
>
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
