On Mon, 25 Jul 2005, Nick Sillik wrote:
> Please take a look at this patch, it has been tested as thoroughly as
> possible as I an having only one arch at my disposal (x86) although I
> see no reason why it wouldn't work with others. To get it tested a
> little better I would like if it would be merged with -mm.
>
> I added all fixes Alan mentioned (kcalloc instead of kalloc and no
> kfree(onetouch)) I've included the final diff for the device.
Here are just a few comments, interspersed with your code. These are all
minor matters that don't need to be changed now -- you can include them
with your next update.
Alan Stern
--- linux-2.6.13-rc1-mm1/drivers/usb/storage/onetouch.c 1969-12-31
19:00:00.000000000 -0500
+++ linux-2.6.13-rc1-mm1-onetouch/drivers/usb/storage/onetouch.c
2005-07-14 21:28:09.000000000 -0400
@@ -0,0 +1,205 @@
+/*
+ * Support for the Maxtor OneTouch USB hard drive's button
+ *
+ * Current development and maintenance by:
+ * Copyright (c) 2005 Nick Sillik <[EMAIL PROTECTED]>
+ *
+ * Initial work by:
+ * Copyright (c) 2003 Erik Thyrén <[EMAIL PROTECTED]>
Try to avoid non-ASCII characters in comments. They usually show up
wrong.
+int onetouch_connect_input(struct us_data *ss)
+{
+ struct usb_device *udev = ss->pusb_dev;
+ struct usb_host_interface *interface;
+ struct usb_endpoint_descriptor *endpoint;
+ struct usb_onetouch *onetouch;
+ int pipe, maxp;
+ char path[64];
You ought to avoid putting something that large on the stack.
+
+ interface = ss->pusb_intf->cur_altsetting;
+
+ endpoint = &interface->endpoint[2].desc;
Although a lot of drivers do this sort of thing, I think it's bad form to
examine endpoint[2] without first verifying that there really are at least
three endpoints.
+ if(!(endpoint->bEndpointAddress & 0x80))
We have a perfectly good constant called USB_DIR_IN; use it.
+ return -ENODEV;
+ if((endpoint->bmAttributes & 3) != 3)
We also have USB_ENDPOINT_XFERTYPE_MASK and USB_ENDPOINT_XFER_INT. All
these constants are defined in include/linux/usb_ch9.h. If you just use
magic numbers like 3, people reading this code will wonder what you're
trying to accomplish.
+ if (!(onetouch = kcalloc(1, sizeof(struct usb_onetouch), GFP_KERNEL)))
+ return -ENOMEM;
+
+ onetouch->data = usb_buffer_alloc(udev, ONETOUCH_PKT_LEN, SLAB_ATOMIC,
&onetouch->data_dma);
Wrap lines that extend beyond 80 columns.
+ usb_make_path(udev, path, 64);
+ sprintf(onetouch->phys, "%s/input0", path);
Here you could pass onetouch->phys to usb_make_path and then call strcat.
If you don't want to do that, at least use sizeof(path) instead of 64.
+void onetouch_release_input(void *onetouch_)
+{
+ struct usb_onetouch *onetouch = (struct usb_onetouch *) onetouch_;
+
+ if (onetouch) {
+ usb_kill_urb(onetouch->irq);
+ input_unregister_device(&onetouch->dev);
+ usb_free_urb(onetouch->irq);
+ usb_buffer_free(onetouch->udev, ONETOUCH_PKT_LEN,
+ onetouch->data, onetouch->data_dma);
+ printk(KERN_INFO "Maxtor Onetouch %04x:%04x Deregistered\n",
+ onetouch->dev.id.vendor, onetouch->dev.id.product);
Why not use onetouch->name, like you did for the registration message?
-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
