On Thu, Jul 06, 2006 at 10:56:33PM -0300, Manuel Naranjo wrote:
> 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?.
Please read the file, Documentation/SubmittingPatches for what it
signifies and the proper format of it.
> >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?.
I like mutt.
> >>@@ -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.
You don't really have to call it anything :)
> >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.
No, the kernel already has a changelog from git. You don't need to
include your own.
> >>+
> >>+/* 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.
All lowercase please.
> >>+ .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.
Why? They really are different things and show up in different places
in sysfs. Having the same name is just fine.
> >>+ .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.
Yes.
> >>+
> >>+ 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.
Yes, see the current usb-serial drivers for how this is handled.
> >>+ 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.
No, free it in your urb callback, when you are done with the memory.
> >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?
Yes.
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
