Re: USB: m66592-udc: peripheral controller driver for M66592

> Thanks ... I'm still looking at this.  Here's some quick feedback:
Thank you very much for your feedback. It is really informative for me.
I would like to quick reply to your comment now and I will
modify driver code and test before re-submit reviced code.

>> /* 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.
You are right. As you pointed this flag does not mean "complete",
I will maintain comment as follows to eliminate confusion
"If internal_ccpl is true, this driver does not call
gadget->complete()in transfer_complete() function."

> 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.
Yes, that's true
I will modify to return corect dma-coherent value.

> Those suspend/resume methods are pointless -- remove them.
> (The state they change is going to be removed soon.)
Understood. I will remove this.

> 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).
I will support set_selfpowered method as well and modify
get_status() to report exact value.

> 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?)
This device hardware has highspeed electrical test register,
however current driver does not utilize this capabilty.
It should be supported in future driver ( as I am not sure
how to use this mode now, I will confirm this with device
design team )

> 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.
Yes, as you pointed out current device driver does not handle USBCV
correctly.
I will support this and check it works correctly.


-------------------------------------------------------------------------
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/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel