Here are a few comments, mixed in with your code.
Thanks!
#define DBUFMAX 65536
This is pretty big for a kernel buffer. You ought to be able to
use something smaller.
I agree. I dropped it to 8k, which is a nicer size -- It gives me about
seven YMODEM-1k packets.
It's not clear that these values are the best. It depends on the
characteristics of the transfers you expect.
I'm expecting mostly YMODEM-1k packets that will be acknowledged before more
are transmitted. I dropped buflen to 1536, and I dropped qlen to 4, which
should be more than plenty if I choose to use YMODEM-1k-g.
For portability, this value cannot be predefined. You have to get
it from the controller driver.
Appropriate code nabbed from zero.c.
if (ep == dev->out_ep) {
This is an unnecessary test. What other endpoint could it be?
Okay. I left a paranoia test in there anyway, but now the whole thing isn't
if'ed in. "Good coding style" change mostly, but it helps readability.
req->zero = (req->actual < req->length);
There's no point setting req->zero for an OUT transfer.
Okay. I have no idea what req->zero actually does. I think I grabbed it from
zero.c. Does req->zero mean "send a ZLP at the end"?
You might as well copy all the data that will fit. Is there any
reason to leave out an entire packet if it's a few bytes too long?
Nope, you're right. On a real serial FIFO, it'd just overflow. Sounds good
to me.
This next section is all dead code.
Whoops. Removed.
static ssize_t read_gchar(struct file* file, char* buf, size_t count, loff_t
*ppos)
Isn't buf supposed to be __user? And ditto for the write_gchar routine?
Changed.
if (!dev->configured)
return -ENXIO;
Whoops! You're still in TASK_INTERRUPTIBLE.
You are right, I am! That if is gone now, anyway. But, should I be doing
some signal stuff here, or will schedule() do that for me?
memcpy(buf, dev->databuf, readcnt);
Shouldn't this be copy_from_user?
copy_to_user, yeah. Thanks.
if (!dev->configured)
return 0;
Looks like these tests should go before the alloc_ep_req call. On the
other hand, I don't see how dev could ever be NULL.
if(dev) removed. Configured moved.
static loff_t null_lseek(struct file * file, loff_t offset, int orig)
{
return file->f_pos = 0;
}
Isn't there a standard kernel routine to do this for you?
Dunno. It's simple enough to have here, anyway.
I uploaded my new version to joshuawise.com/char.c again. Thanks!
Alan Stern
Thanks again for your time!
Joshua
-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course
Free Certification Exam for All Training Attendees Through End of 2005
Visit http://www.jboss.com/services/certification for more information
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course
Free Certification Exam for All Training Attendees Through End of 2005
Visit http://www.jboss.com/services/certification for more information
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
