On Wednesday 09 May 2007, Yoshihiro Shimoda wrote:
>
> I would like to submit Renesas M66592 udc driver.
>
> The M66592 is Renesas USB 2.0 peripheral controller.
> This controller supports USB high-speed.
I always like to see more high speed controller drivers
work with Linux. And vendors supporting Linux by
providing drivers for their hardware ... :)
Thanks ... I'm still looking at this. Here's some quick feedback:
Overall it looks pretty clean. There are some whitespace
bugs (indents should ONLY be tabs etc) but mostly it doesn't
look bad.
It looks a bit light on comments ... like, there really
aren't any. In over 40 KBytes of code, I'm sure there
is at least one puzzling thing that will need comments
so that someone else can make sense of it... I did find
one comment though, which is completely unrelated to
what the code does:
> /* if complete is true, gadget driver complete function is not call */
> static void control_end(struct m66592 *m66592, unsigned ccpl)
> {
> m66592->ep[0].internal_ccpl = ccpl;
> pipe_start(m66592, 0);
> m66592_bset(m66592, M66592_CCPL, M66592_DCPCTR);
> }
Problem is, there is no "complete" flag, and nothing there
ever calls the complete() routine anyway. I happened on
that call when wondering how some SET_FEATURE requests were
handled ... they seem to be dropped on the floor.
The alloc_buffer()/free_buffer() is wrong; it should be
returning dma-coherent memory. But almost nobody seems
to get that right, and I'm thinking of just ripping those
calls out of the programming interface ... so for now I
won't worry about that.
It's OK not to have an unbind() method ... but it's not
OK to unregister a gadget driver without one.
Those suspend/resume methods are pointless -- remove them.
(The state they change is going to be removed soon.)
You should be able to report bus-powered status, so that
for example a device can consume power from USB (maybe to
recharge its battery or something).
> The driver has been tested Gadget Zero, Ethernet Gadget,
> File-backed Storage Gadget, and passed usbtest script.
And I imagine it took a bit of effort to get that working... :)
But not, I'm assuming, the highspeed electrical tests since
it doesn't support the relevant high speed test modes.
(Unless hardware automagic intercepts them?)
Or USBCV, since I don't think I noticed support for remote
wakeup, and I know that USBCV will test that ... you seem
to stall every attempt to set the remote wakeup feature,
and certainly can't report whether it's been set.
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
