On Thu, Sep 25, 2003 at 08:15:33PM -0700, Neil Whelchel wrote:
> Hello,
> This patch adds support for interrupt_out to match the existing
> interrupt_in support in usb-serial. I thought that it would be cleaner in
> the long run to have the support here as to reproducing similar
> functionality in (possibly many) other places.
> It also adds a few more debugging messages, and makes a few existing ones
> more specific. (I needed them for writing a driver, it is likely someone
> else might need them under similar conditions.)
Looks good, but I have a few minor comments:
> @@ -1026,14 +1043,16 @@
>
> /* descriptor matches, let's find the endpoints needed */
> /* check out the endpoints */
> + dbg("checking for endpoints");
> iface_desc = &interface->altsetting[0];
> + dbg("this device claims to have %d
> endpoints",iface_desc->desc.bNumEndpoints);
I don't think the first new dbg() line needs to be there, as the second
one will tell us this.
> for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> endpoint = &iface_desc->endpoint[i].desc;
>
> if ((endpoint->bEndpointAddress & 0x80) &&
> ((endpoint->bmAttributes & 3) == 0x02)) {
> /* we found a bulk in endpoint */
> - dbg("found bulk in");
> + dbg("found bulk in on endpoint %d",i);
Ah, that's a nice change, thanks.
> }
> -
> + dbg("found %d bulk_in, %d bulk_out, %d interrupt_in %d interrupt_out",
> num_bulk_in, num_bulk_out, num_interrupt_in, num_interrupt_out);
> #if defined(CONFIG_USB_SERIAL_PL2303) || defined(CONFIG_USB_SERIAL_PL2303_MODULE)
> /* BEGIN HORRIBLE HACK FOR PL2303 */
> /* this is needed due to the looney way its endpoints are set up */
Keep an extra line after the dbg() comment here please.
> + for (i = 0; i < num_interrupt_out; ++i) {
> + endpoint = interrupt_out_endpoint[i];
> + port = serial->port[i];
> + port->interrupt_out_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!port->interrupt_out_urb) {
> + dev_err(&interface->dev, "No free urbs available\n");
> + goto probe_error;
> + }
> + buffer_size = endpoint->wMaxPacketSize;
> + port->interrupt_out_size = buffer_size;
> + port->interrupt_out_endpointAddress = endpoint->bEndpointAddress;
> + port->interrupt_out_buffer = kmalloc (buffer_size, GFP_KERNEL);
> + if (!port->interrupt_out_buffer) {
> + dev_err(&interface->dev, "Couldn't allocate
> interrupt_out_buffer\n");
> + goto probe_error;
> + }
> + usb_fill_int_urb (port->interrupt_out_urb, dev,
> + usb_sndintpipe (dev,
> + endpoint->bEndpointAddress),
> + port->interrupt_out_buffer, buffer_size,
> + serial->type->write_int_callback, port,
> + endpoint->bInterval);
> + }
> +
> +
> +
Hm, so what happens if the driver does not define a write_int_callback
function? We don't provide a default one, so it might be pretty easy to
oops a driver this way. Care to provide a default dummy one?
> @@ -193,6 +202,8 @@
> * of the devices this structure can support.
> * @num_interrupt_in: the number of interrupt in endpoints this device will
> * have.
> + * @num_interrupt_out: the number of interrupt out endpoints this device will
> + * have.
> * @num_bulk_in: the number of bulk in endpoints this device will have.
> * @num_bulk_out: the number of bulk out endpoints this device will have.
> * @num_ports: the number of different ports this device will have.
> @@ -225,6 +236,7 @@
> char *short_name;
> const struct usb_device_id *id_table;
> char num_interrupt_in;
> + char num_interrupt_out;
I don't think you use this, right? Hm, looks like we don't use the
num_interrupt_in field either. I guess I could remove those, for some
reason I thought I did in the past...
thanks,
greg k-h
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
