Re: [PATCH 4/9] usb: renesas: gadget: use generic map/unmap routines

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]