Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 20 Dec 2011 12:51:08 +0200 | From | Felipe Balbi <> | Subject | Re: [PATCH 4/9] usb: renesas: gadget: use generic map/unmap routines |
| |
Hi,
On Mon, Dec 19, 2011 at 05:46:56PM -0800, Kuninori Morimoto wrote: > I tried this patch, but renesas_usbhs didn't work. > It seems have some bugs. > > 1. renesas_usbhs dma needs pkt->dma, but this patch didn't care it. > 2. dma direction seems wrong > ("dir" needs 0/1, not DMA_xxx_DEVICE for usb_gadget_xx_request()) > > And, renesas_usbhs can not use scatter/gather type dma > > # > # this is not super important, but > # > # int usb_gadget_map_request(struct usb_gadget *gadget, > # struct usb_request *req, int direction) > # > # this "direction = 0/1" is a little bit un-understandable for me. > # it is difficult to understand "which direction ?" from code. > # if "direction = DMA_TO_DEVICE/DMA_FROM_DEVICE, it is understandable ;P
I see. The thing is that we want usb_gadget_map/unmap_request() to handle that. In the long run, we might want to have a direction flag on the public struct usb_request and remove this extra parameter, but I'm still considering that :-)
> Can you please add below fix patch ?
thanks, will do as soon as you fix below :-)
> ------------------------------------------------ > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c > index cb92a1d..c467067 100644 > --- a/drivers/usb/renesas_usbhs/mod_gadget.c > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c > @@ -173,15 +173,32 @@ static int usbhsg_dma_map_ctrl(struct usbhs_pkt *pkt, int map) > struct usbhsg_uep *uep = usbhsg_pipe_to_uep(pipe); > struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep); > enum dma_data_direction dir; > + int ret; > > - dir = usbhs_pipe_is_dir_in(pipe) ? DMA_FROM_DEVICE : DMA_TO_DEVICE; > + dir = !usbhs_pipe_is_dir_in(pipe); > > if (map) { > - return usb_gadget_map_request(&gpriv->gadget, req, dir); > + if (req->num_sgs) /* it can not use scatter/gather */ > + return -EIO;
it would be better to through a big fat WARN() in this case. Gadget drivers are supposed to check whether you support SGs (by looking into gadget->sg_suported) before giving you a request with SGs.
> + if (pkt->dma != DMA_ADDR_INVALID) > + return -EIO;
no no, we want to remove the whole DMA_ADDR_INVALID thing. What we're doing now is that UDC is required to map the request buffer. So gadget drivers must not do it. We shouldn't have this DMA_ADDR_INVALID macro anymore :-)
-- balbi [unhandled content-type:application/pgp-signature] | ![\](/images/icornerr.gif) |