On Tue, Jul 04, 2006 at 09:46:09AM -0300, Manuel Naranjo wrote: > Here is the patch I generetaded using a kernel 2.6.16.18 (The one > included on the CD of drivers development). This is a working version, > that can handle transfers at to 180 bytes, the one i was asking is a > slight different. On the aircable_write there is a for next, and what i > did was sending the URB on every interaction. I can generate a diff > patch to show mi idea, but I'm sending this version because I know it > works quite well. > > Thanks, > Manuel
A few small comments: You forgot a "Signed-off-by:" line. > > diff -uprN -X Documentation/dontdiff > a/drivers/usb/serial/AIRcable-USB-serial.c > b/drivers/usb/serial/AIRcable-USB-serial.c > --- a/drivers/usb/serial/AIRcable-USB-serial.c 1969-12-31 > 21:00:00.000000000 -0300 > +++ b/drivers/usb/serial/AIRcable-USB-serial.c 2006-07-04 > 09:34:10.000000000 -0300 Your email client wrapped the patch :( Also, don't InterCap the file name, this should just be called "aircable.c". > @@ -0,0 +1,309 @@ > +/* > + * AIRcable USB Bluetooth dondgle Driver. > + * > + * This driver is a modified version of USB Serial Converter driver, > + * that lets AIRcable devices work. > + * > + * The AIRcable USB Bluetooth Dongles has two interfaces, the first > + * one is usless for serial working, I think it must be for firmware > + * updating. > + * The second interfaces has a bulk in and a bulk out, and is the one > + * used for serial transactions. > + * AIRcable USB Bluetooth Dongles uses a modified version of the standar > + * USB serial generic protocol, it sends data in packages of 124 bytes, > + * and adds at the begging of each package 4 bytes for header. See > + * AIRcable-USB-serial.h for Header Information. There is no "usb serial generic protocol". > + * > + * I have also taken some info from a Greg Kroah-Hartman article > + * url: http://www.linuxjournal.com/article/6573 > + * > + * Copyright (C) 2006 > + * Manuel Francisco Naranjo ([EMAIL PROTECTED]) > + * > + * License: GNU/GPL v2 or newer. Are you sure about the "or newer"? Who knows what the 3.0 version of the GPL is going to look like... > + * > + * (03/07/2006) manuelnaranjo > + * Trying to fix a bug, if more than 1 dongle is connected to the > + * computer and there are transfers of more than 180 bytes (without > + * including the Header) the driver crashes. > + * > + * (23/06/2006) manuelnaranjo > + * v0.1 Started Don't put changelog info in .c files, that goes in the changelog information instead. > + * > + * > + */ > + > +#include <linux/config.h> > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/tty.h> > +#include <linux/tty_flip.h> > +#include <linux/module.h> > +#include <linux/usb.h> > +#include <linux/slab.h> > +#include <../drivers/usb/serial/usb-serial.h> Huh? It should just be "usb-serial.h" > +#include "AIRcable-USB-serial.h" Why a separate .h file? Just put that stuff into the .c file please. > + > + > +static int debug; > + > +/* > + * Version Information > + */ > +#define DRIVER_VERSION "v1.0b2" > +#define DRIVER_AUTHOR "Naranjo, Manuel Francisco. email: > [EMAIL PROTECTED]" Just do: #define DRIVER_AUTHOR "Naranjo, Manuel Francisco <[EMAIL PROTECTED]>" instead. > +#define DRIVER_DESC "AIRcable USB Driver" > + > + > +/* ID table that will be registered with USB core */ > +static struct usb_device_id id_table [] = { > + { USB_DEVICE(AIRCABLE_VID, AIRCABLE_USB_PID) }, > + { }, > +}; > +MODULE_DEVICE_TABLE (usb, id_table); > + > + > + > +/* Methods declaration */ > +int aircable_probe (struct usb_serial *serial, const struct > usb_device_id *id); > +int aircable_write(struct usb_serial_port *port, const unsigned char > *buf, int count); > +void aircable_read_bulk_callback(struct urb *urb, struct pt_regs *regs); These should all be static. And you can probably just reorginize the code and not need the pre-definitions at all. > + > +/* All device info needed for AIRcable USB device */ > +static struct usb_serial_device_type aircable_device = { > + .owner = THIS_MODULE, > + .name = "AIRcable USB serial (device)", name should not have any spaces. > + .short_name = "aircable_device", And we always know this is a "device" :) > + .id_table = id_table, > + .num_ports = 1, > + .probe= aircable_probe, > + .write= aircable_write, > + .read_bulk_callback= aircable_read_bulk_callback, > +}; What kernel did you build this against? 2.6.17 will not work with this. > + > +static struct usb_driver aircable_driver = { > + .name = "AIRcable USB serial (driver)", Drop the spaces again please. > + .probe = usb_serial_probe, > + .disconnect = usb_serial_disconnect, > + .id_table = id_table, > +}; > + > +/* Methods implementation */ > + > +/* Based on serial_probe */ > +int aircable_probe (struct usb_serial *serial, const struct > usb_device_id *id) > +{ > + struct usb_host_interface *iface_desc = > serial->interface->cur_altsetting; > + struct usb_endpoint_descriptor *endpoint; > + int num_bulk_out=0; > + > + int i; > + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) { > + endpoint = &iface_desc->endpoint[i].desc; > + > + if (((endpoint->bEndpointAddress & 0x80) == 0x00) && > + ((endpoint->bmAttributes & 3) == 0x02)) { > + dbg("found bulk out on endpoint %d", i); > > + ++num_bulk_out; > + } > + } > + > + if (num_bulk_out == 0) { > + dbg("Invalid interface, discarding.\n"); > + return -5; What's -5? Use proper error values please. And you have trailing spaces in the code :( You also don't need this function, if you specify the number of bulk in and out endpoints in your serial type structure, the core will enforce it. > + } > + return 0; > +} > + > + > + > +/* > + * Based on Generic_write and AIRcable Windows driver > + * Thanks Juergen Kienhoefer from AIRcable for the support, and > + * giving me access to their Windows Driver. > + */ > +int aircable_write(struct usb_serial_port *port, const unsigned char > *pSrcBuf, int count) "buffer" instead of "pSrcBuf" please. This is Linux, not some other operating system. Please read the CodingStyle file for details. > +{ > + struct usb_serial *serial = port->serial; > + unsigned char *pBuf; No intercaps please. And no "p" to specify a pointer, the compiler checks it for you. > + int result; > + unsigned long no_headers; > + unsigned long payload_length; > + unsigned long length; > + unsigned long i; > + unsigned long offset; > + unsigned long src_offset; Why "unsigned long"? int would be fine, right? > + > + dbg("%s - port %d", __FUNCTION__, port->number); > + > + if (count == 0) { > + dbg("%s - write request of 0 bytes", __FUNCTION__); > + return (0); > + } > + > + /* only do something if we have a bulk out endpoint */ > + if (serial->num_bulk_out) { > + if (port->write_urb->status == -EINPROGRESS) { Checking the status is not safe. Please use a flag instead. > + dbg("%s - already writing", __FUNCTION__); > + return (0); > + } > + > + dbg("Writting: %s", pSrcBuf); > + > + no_headers = (count / MAX_HCI_FRAMESIZE) + 1; > + > + pBuf = kmalloc(count + (no_headers *HCI_HEADER_LENGTH > ),GFP_KERNEL); > + memset (pBuf, 0, count + (no_headers *HCI_HEADER_LENGTH )); kzalloc() instead. And when do you free this memory? It looks like it leaks. > + > + payload_length = count; > + length = 0; > + > + for(i = 0; i < no_headers; i++) { > + if(payload_length >= MAX_HCI_FRAMESIZE) > + length = MAX_HCI_FRAMESIZE; > + else > + length = payload_length; > + payload_length -= length; > + > + offset = i * (MAX_HCI_FRAMESIZE + HCI_HEADER_LENGTH); > + src_offset = i * MAX_HCI_FRAMESIZE; > + pBuf[offset] = HCI_HEADER_0; > + pBuf[offset+1] = HCI_HEADER_1; > + pBuf[offset+2] = (unsigned char) length; > + pBuf[offset+3] = (unsigned char)(length >> 8); > + > + memcpy(pBuf + offset + HCI_HEADER_LENGTH, pSrcBuf + > src_offset, length); > + } Why not just divide up the writes into framesize urbs and send them all down that way? It would simpilify the logic a lot, and perhaps fix your bug. > + > + port->write_urb->transfer_buffer = pBuf; > + > + /* set up our urb */ > + usb_fill_bulk_urb (port->write_urb, serial->dev, > + usb_sndbulkpipe (serial->dev, > port->bulk_out_endpointAddress), > + port->write_urb->transfer_buffer, (count + > (no_headers > *HCI_HEADER_LENGTH )), > + serial->type->write_bulk_callback, port); > + > + /* send the data out the bulk port */ > + result = usb_submit_urb(port->write_urb, GFP_ATOMIC); > + > + if (result) > + dev_err(&port->dev, "%s - failed submitting write urb, > error %d\n", > __FUNCTION__, result); > + else > + result = count; > + > + return result; > + > + kfree(pBuf); > + } > + > + /* no bulk out, so return 0 bytes written */ > + return (0); return is not a function call, drop the () please. thanks, greg k-h Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&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
