Greg: > 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.
What I'm a little stupid, what do you mean? a line that ends with Signed-off-by: Naranjo Manuel Francisco?. >> 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". This was my error, filename is too long, I will use the name you suggested. I'm using Mozilla Thunderbird, which email client do you suggest for sending patches?. > >> @@ -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". How do I call it then? Standard USB protocol? Or I can put it uses a modified version of the USB protocol implemented on usb-serial.c. > >> + * >> + * 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... Ok then I will put GNU/GPL v2, you are right GPL 3.0 is not going to be arround us for a time. >> + * >> + * (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. Great, I have a changelog too I will move all that to there. > >> + * >> + * >> + */ >> + >> +#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" Ok I will check that out :), I think that should work greatly, I also thought there were lots of Includes, > >> +#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. Ok no problem > >> +#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. > Ok, I will try to reorganize. >> + >> +/* 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. AIRcableUSB could be right? The (device) was for debugging, but I forgotten to remove it then. > >> + .short_name = "aircable_device", > > And we always know this is a "device" :) The same as before. I wanted to difference the usb_serial_device from the usb_driver. > >> + .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. 2.6.12, I have just taken a look at 2.6.17, and I have seen that usb_serial_device_type is gone, I suppose it has changed to usb_serial_driver. >> + >> +static struct usb_driver aircable_driver = { >> + .name = "AIRcable USB serial (driver)", > > Drop the spaces again please. :) Don't worry I will > >> + .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. I will correct that. Oliver Neukum also noticed that. I started another Thread, because I thought I have named badly this Thread, the other Thread is Re: [linux-usb-devel] [RFC][PATCH] AIRcable USB Bluetooth Dongle Driver. > And you have trailing spaces in the code :( I will correct that sorry. > > 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. I was trying to do that, but I could not figure out how exactly, and I probed this and worked so I let it. Now that I know a little more how usb-serial works I can correct that. The problem I was having was that the driver attaches himself to both interfaces. I have tried with num_interrupt_in = 0 num_interrupt_out= 0 num_bulk_in = 82 num_bulk_out = 02, and with out defining the num_interrupt but in both cases the driver attaches himself to both interfaces. > >> + } >> + 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. Sorry I guess that came from Windows Driver, sorry. > >> +{ >> + 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. I gotted it. > >> + 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? Ok I will change that. > > >> + >> + 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. Sorry but I can get what you want to mean with a flag. By a flag you mean a variable that will be accessed from the hole module ? :P. > >> + 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. I have tried to release the pointer before calling the return statement, and the hole thing crashed. > >> + >> + 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. I will try that, and see if it do not crashes. I guess you mean if I have n frames, I generate n urbs and send them no? > >> + >> + 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. Ok, Thanks Greg for taking a look at my source, I know you are busy, so I will make my best to use all the changes you commented. Luckly you said some comments at the start of the message, that makes me fell a little better, that means that I do not suck as much as I thought :). When all the changes are made, I will resend the driver. About the changelog, must I also submit the changelog file? Regards, Manuel 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
