Since you are only adding a single .[ch] pair of files, why create a whole new subdirectory?
As I told Greg in a private mail before, I am planning to add an fbdev interface ("lite" version of sisfb) later. This will increase the number of files to up to 10 or so.
+static void sisusb_free_urbs(struct sisusb_usb_data *sisusb) +{ + int i; + + for(i = 0; i < NUMOBUFS; i++) { + if(sisusb->sisurbout[i]) usb_free_urb(sisusb->sisurbout[i]); + sisusb->sisurbout[i] = NULL; + } + if(sisusb->sisurbin) usb_free_urb(sisusb->sisurbin); + sisusb->sisurbin = NULL;
There's no need to test for non-NULL pointers. Just call usb_free_urb.
Please, I like it that way. I don't want to rely on the smartness of any xxxfree() functions.
+/* Return 1 if ok, 0 if error (not all complete within timeout) */ +static int sisusb_wait_all_out_complete(struct sisusb_usb_data *sisusb) +{ + int timeout = 5 * HZ, i = 1; + DECLARE_WAITQUEUE(wait, current); + + if(sisusb_all_free(sisusb)) return 1; + + add_wait_queue(&sisusb->wait_q, &wait); + set_current_state(TASK_INTERRUPTIBLE); + while(1) { + if(timeout && (!(i = sisusb_all_free(sisusb)))) { + timeout = schedule_timeout(timeout); + } else { + set_current_state(TASK_RUNNING); + break; + } + } + remove_wait_queue(&sisusb->wait_q, &wait); + return i;
This seems a little strange. Why test whether every URB is free each time you go through the loop? Why not just test them one-by-one, i.e., wait for the next URB each iteration?
This is my sync function. The caller can rely on all URBs finished. There is another function for finding the first free URB.
I need to sync sometimes. Bulk data transfers to the PCI device on the other side of the net2280 need to be initialized by a setup-packet to a different endpoint. If I send data to this setup-endpoint before a previous large bulk transfer is finished, the device stalls. Hence, before any new transfer, I need to wait for all URBs to be finished.
+static int sisusb_bulkout_msg(struct sisusb_usb_data *sisusb, int index, unsigned int pipe, + void *data, int len, int *actual_length, int timeout, + unsigned int tflags, dma_addr_t transfer_dma)
+{
<...>
+ /* If OK, and if timeout > 0, wait for completion */
+ if((retval == 0) && timeout) {
+ if(sisusb->urbstatus[index] & SU_URB_BUSY) {
+ add_wait_queue(&sisusb->wait_q, &wait);
+ set_current_state(TASK_INTERRUPTIBLE);
+ while(1) {
+ if(timeout && (sisusb->urbstatus[index] & SU_URB_BUSY)) {
+ timeout = schedule_timeout(timeout);
+ } else {
+ set_current_state(TASK_RUNNING);
+ break;
+ }
+ }
+ remove_wait_queue(&sisusb->wait_q, &wait);
+ }
+ retval = urb->status;
+ if(retval == -ECONNRESET) retval = -ETIMEDOUT;
While this is technically okay, it's bad form since you're not supposed
to look at urb->status until the completion handler has run.
Erm... if the completion handler hasn't been run, the BUSY flag won't be clear. Unless the URB timed out...
Also, if the URB does time out you don't unlink it anywhere. So how can urb->status ever get set to -ECONNRESET?
... in which case you mean that I should call unlink()?
Is -ECONNRESET only set if the URB is (forcefully) unlinked (by me)?
Will the completion handler be called under ALL circumstances? In this case I could avoid the (own) timeout loop and just wait (forever) for the completion handler to clear the BUSY bit. Would that be correct?
Apart from the issues above, I am fighting two problems Oliver pointed out: (Statements below not neccessarily for Alan but all reading this)
1) Signals. As you see, I have commented out the signal_pending() stuff in the read and write routines. As long as they were there, it happended frequently that I received a sig 1 (SIGHUP) during data transfers from the X server. Why on earth do I get a SIGHUP here sometimes?! (The X server runs fine if I ignore signals.)
2) Endianess. I don't think that this device is made for big-endian machines. Sure, I could convert the control packets for be machines (as Oliver pointed out), but what about framebuffer data? On BE-PCI machines, AFAIK, the PCI bridge reverses the address lines in order to make the host PCI compliant (PCI is little endian). I don't have this automatic here. I would have to swap all framebuffer data I send over the wire. That's not only impractical, it's impossible because the kernel driver has no knowledge of eg. the color depth. How am I supposed to convert (swap) data if I don't know whether I have to deal with 16 or 32 bit data (which is sent as bytes)?
Has the net2280 any facility to swap PCI data?
Thomas
-- Thomas Winischhofer Vienna/Austria thomas AT winischhofer DOT net *** http://www.winischhofer.net/ twini AT xfree86 DOT org
------------------------------------------------------- The SF.Net email is sponsored by: Beat the post-holiday blues Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek. It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt _______________________________________________ [email protected] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
