Re: [PATCH v3] USB device driver of Topcliff PCH

["Michał Nazarewicz" <m.nazarewicz@samsung.com>]

Just a few things I've noticed.  Do not consider it a proper review:


On Thu, 16 Sep 2010 15:39:37 +0200, Masayuki Ohtake <masa-korg@dsn.okisemi.com> wrote:
> +struct pch_udc_stp_dma_desc {
> +	u32 status;
> +	u32 reserved;
> +	u32 data12;
> +	u32 data34;
> +};

 From what I've seen, there is no reason why not make the above:

struct pch_udc_stp_dma_desc {
	u32 status;
	u32 reserved;
	struct usb_ctrlrequest request;
} __attribute((packed));


> +struct pch_udc_ep {
> +	struct usb_ep			ep;
> +	dma_addr_t			td_stp_phys;
> +	dma_addr_t			td_data_phys;
> +	struct pch_udc_stp_dma_desc	*td_stp;
> +	struct pch_udc_data_dma_desc	*td_data;
> +	struct pch_udc_dev		*dev;
> +	unsigned long			offset_addr;
> +	const struct usb_endpoint_descriptor	*desc;
> +	struct list_head		queue;
> +	unsigned			num:5;
> +	unsigned			in:1;
> +	unsigned			halted;

Can't this be made into bitfield as well?

> +	unsigned long			epsts;
> +};


> +union pch_udc_setup_data {
> +	u32		data[2];
> +	struct usb_ctrlrequest	request;
> +};

This should not be needed.  Just use struct pch_udc_stp_dma_desc as
described above.


> +static void pch_udc_csr_busy(struct pch_udc_dev *dev)
> +{
> +	unsigned int count = MAX_LOOP;

At this point, I would start wondering whether the MAX_LOOP macro is
really needed.  I'd remove the #define and just put a plain number
here.

> +
> +	/* Wait till idle */
> +	while ((pch_udc_readl(dev, UDC_CSR_BUSY_ADDR) & UDC_CSR_BUSY)
> +		&& --count)
> +		cpu_relax();
> +	if (!count)
> +		dev_err(&dev->pdev->dev, "%s: wait error", __func__);
> +}


> +static inline void pch_udc_vbus_session(struct pch_udc_dev *dev,
> +					  int is_active)
> +{
> +	if (is_active)
> +		pch_udc_clear_disconnect(dev);
> +	else
> +		pch_udc_set_disconnect(dev);
> +}


> +/**
> + * pch_udc_ep_clear_nak() - Set the bit 8 (CNAK field)
> + *				of the endpoint control register
> + * @ep:		reference to structure of type pch_udc_ep_regs
> + */
> +static void pch_udc_ep_clear_nak(struct pch_udc_ep *ep)
> +{
> +	unsigned int loopcnt = 0;
> +	struct pch_udc_dev *dev = ep->dev;
> +
> +	if (!(pch_udc_ep_readl(ep, UDC_EPCTL_ADDR) & UDC_EPCTL_NAK))
> +		return;
> +	if (!ep->in) {
> +		loopcnt = 100000;
> +		while (!(pch_udc_read_ep_status(ep) & UDC_EPSTS_MRXFIFO_EMP) &&
> +			--loopcnt)
> +			udelay(100);

This loop can take up to 10 seconds.  Is it desired?

> +		if (!loopcnt)
> +			dev_dbg(&dev->pdev->dev, "%s: RxFIFO not Empty "
> +				"loop count = %d", __func__, loopcnt);

Shouldn't that be dev_err()?

> +	}
> +	loopcnt = 100000;
> +	while ((pch_udc_read_ep_control(ep) & UDC_EPCTL_NAK) && --loopcnt) {
> +		pch_udc_ep_bit_set(ep, UDC_EPCTL_ADDR, UDC_EPCTL_CNAK);
> +		udelay(5);
> +	}

This loop can take up to half a second.  Is it desired?

> +	if (!loopcnt)
> +		dev_dbg(&dev->pdev->dev,
> +			"%s: Clear NAK not set for ep%d%s: counter=%d",
> +			__func__, ep->num, (ep->in ? "in" : "out"), loopcnt);

Shouldn't that be dev_err()?

> +}


> +static void pch_udc_ep_fifo_flush(struct pch_udc_ep *ep, int dir)
> +{
> +	unsigned int loopcnt = 0;
> +	struct pch_udc_dev *dev = ep->dev;
> +
> +	dev_dbg(&dev->pdev->dev, "%s: ep%d%s", __func__, ep->num,
> +		(ep->in ? "in" : "out"));
> +	if (dir) {	/* IN ep */
> +		pch_udc_ep_bit_set(ep, UDC_EPCTL_ADDR, UDC_EPCTL_F);
> +		return;
> +	}
> +
> +	if (pch_udc_read_ep_status(ep) & UDC_EPSTS_MRXFIFO_EMP)
> +		return;
> +	pch_udc_ep_bit_set(ep, UDC_EPCTL_ADDR, UDC_EPCTL_MRXFLUSH);
> +	/* Wait for RxFIFO Empty */
> +	loopcnt = 1000000;
> +	while (!(pch_udc_read_ep_status(ep) & UDC_EPSTS_MRXFIFO_EMP) &&
> +		--loopcnt)
> +		udelay(100);
> +	if (!loopcnt)
> +		dev_dbg(&dev->pdev->dev, "RxFIFO not Empty");

Same as in function above.  The loop can take up to 10 seconds plus if
loopcnt reaches zero error should be printed (IMO).

> +	pch_udc_ep_bit_clr(ep, UDC_EPCTL_ADDR, UDC_EPCTL_MRXFLUSH);
> +}


> +static int pch_udc_free_dma_chain(struct pch_udc_dev *dev,
> +						 struct pch_udc_request *req)
> +{
> +	struct pch_udc_data_dma_desc	*td;
> +	struct pch_udc_data_dma_desc	*td_last;
> +	u32 i;

Why u32?  Why not plain unsigned?

> +
> +	/* do not free first desc., will be done by free for request */
> +	td_last = req->td_data;
> +	td = phys_to_virt(td_last->next);
> +
> +	for (i = 1; i < req->chain_len; i++) {
> +		pci_pool_free(dev->data_requests, td,
> +			      (dma_addr_t) td_last->next);
> +		td_last = td;
> +		td = phys_to_virt(td_last->next);
> +	}
> +	return 0;
> +}

If I'm not mistaken, this can be refactored as:

static void pch_udc_free_dma_chain(struct pch_udc_dev *dev,
				   struct pch_udc_request *req)
{
	struct pch_udc_data_dma_desc *td = req->td_data;
	unsigned i = req->chain_len;

	for (; i > 1; --i) {
		dma_addr_t addr = (dma_addr_t)td->next;
		/* do not free first desc., will be done by free for request */
		td = phys_to_virt(addr);
		pci_pool_free(dev->data_requests, td, addr);
	}
}

> +static int pch_udc_create_dma_chain(struct pch_udc_ep *ep,
> +				     struct pch_udc_request *req,
> +				     unsigned long buf_len,
> +				     gfp_t gfp_flags)
> +{
> +	unsigned long bytes = req->req.length;
> +	unsigned int i;

If bytes and buf_len is unsigned long why i is unsigned int?

> +	dma_addr_t dma_addr;
> +	struct pch_udc_data_dma_desc	*td = NULL;
> +	struct pch_udc_data_dma_desc	*last = NULL;
> +	unsigned long txbytes;
> +	unsigned len;
> +
> +	pr_debug("%s: enter  bytes = %ld buf_len = %ld", __func__, bytes,
> +		 buf_len);
> +	/* unset L bit in first desc for OUT */
> +	if (!ep->in)
> +		req->td_data->status = PCH_UDC_BS_HST_BSY;
> +
> +	/* alloc only new desc's if not already available */
> +	len = req->req.length / buf_len;
> +	if (req->req.length % buf_len)
> +		len++;

	len = (bytes + buf_len - 1) / buf_len;

> +
> +	/* shorter chain already allocated before */
> +	if (req->chain_len > 1)
> +		pch_udc_free_dma_chain(ep->dev, req);
> +
> +	req->chain_len = len;
> +
> +	td = req->td_data;
> +	/* gen. required number of descriptors and buffers */
> +	for (i = buf_len; i < bytes; i += buf_len) {
> +		dma_addr = DMA_ADDR_INVALID;

No use to set.

> +		/* create or determine next desc. */
> +		td = pci_pool_alloc(ep->dev->data_requests, gfp_flags,
> +				    &dma_addr);
> +		if (!td)
> +			return -ENOMEM;
> +
> +		td->status = 0;

No use to set.

> +		td->dataptr = req->req.dma + i; /* assign buffer */
> +
> +		if ((bytes - i) >= buf_len)
> +			txbytes = buf_len;
> +		else /* short packet */
> +			txbytes = bytes - i;
> +		/* link td and assign tx bytes */
> +		if (i == buf_len) {
> +			req->td_data->next = dma_addr;
> +			/* set the count bytes */
> +			if (ep->in) {
> +				req->td_data->status = PCH_UDC_BS_HST_BSY |
> +						       buf_len;
> +				/* second desc */
> +				td->status = PCH_UDC_BS_HST_BSY | txbytes;
> +			} else {
> +				td->status = PCH_UDC_BS_HST_BSY;
> +			}
> +		} else {
> +			last->next = dma_addr;
> +			if (ep->in)
> +				td->status = PCH_UDC_BS_HST_BSY | txbytes;
> +			else
> +				td->status = PCH_UDC_BS_HST_BSY;
> +
> +		}
> +		last = td;
> +	}
> +	/* set last bit */
> +	if (td) {

This is always true.

> +		td->status |= PCH_UDC_DMA_LAST;
> +		/* last desc. points to itself */
> +		req->td_data_last = td;
> +		td->next = req->td_data_phys;
> +	}
> +	return 0;
> +}

The above function can (at least I think it can) be changed to a shorter version
with no memory error recovery:

static int pch_udc_create_dma_chain(struct pch_udc_ep *ep,
				    struct pch_udc_request *req,
				    unsigned long buf_len,
				    gfp_t gfp_flags)
{
	struct pch_udc_data_dma_desc *td = req->td_data, *last;
	unsigned long bytes = req->req.length, i = 0;
	dma_addr_t dma_addr;
	unsigned len = 1;

	pr_debug("%s: enter  bytes = %ld buf_len = %ld", __func__, bytes,
		 buf_len);

	if (req->chain_len > 1)
		pch_udc_free_dma_chain(ep->dev, req);

	for (; ; bytes -= buf_len, i += buf_len, ++len) {
		if (ep->in)
			td->status = PCH_UDC_BS_HST_BSY | min(buf_len, bytes);
		else
			td->status = PCH_UDC_BS_HST_BSY;

		if (bytes <= buf_len)
			break;

		last = td;
		td = pci_pool_alloc(ep->dev->data_requests, gfp_flags,
				    &dma_addr);
		if (!td)
			goto nomem;

		i += buf_len;
		td->dataptr = req->req.dma + i;
		last->next = dma_addr;
	}

	req->td_data_last = td;
	td->status |= PCH_UDC_DMA_LAST;
	td->next = req->td_data_phys;
	req->chain_len = len;

	return 0;

nomem:
	if (len > 1) {
		req->chain_len = len;
		pch_udc_free_dma_chain(ep->dev, req);
	}
	req->chain_len = 1;

	return -ENOMEM;
}


> +static int prepare_dma(struct pch_udc_ep *ep, struct pch_udc_request *req,
> +			  gfp_t gfp)
> +{
> +	int	retval = 0;

Actually, what's the use of initialising?

> +
> +	pr_debug("%s: enter  req->req.dma=0x%08x", __func__, req->req.dma);
> +	/* set buffer pointer */
> +	req->td_data->dataptr = req->req.dma;
> +	/* set last bit */
> +	req->td_data->status |= PCH_UDC_DMA_LAST;
> +
> +	/* Allocate and create a DMA chain */
> +	retval = pch_udc_create_dma_chain(ep, req, ep->ep.maxpacket, gfp);
> +	if (retval) {
> +		if (retval == -ENOMEM)
> +			pr_err("%s: Out of DMA memory", __func__);

I'd change this to a simple pr_err() without if (retval == -ENOMEM).  Eg.:

		pr_err("%s: could not create DMA chain: %d\n", __func__, retval);

Also, you've forgotten about \n at the end.

> +		return retval;
> +	}
> +	if (!ep->in)
> +		return retval;

Just "return 0;".

> +
> +	if (req->req.length <= ep->ep.maxpacket)
> +		/* write tx bytes */
> +		req->td_data->status = PCH_UDC_DMA_LAST | PCH_UDC_BS_HST_BSY |
> +				       req->req.length;
> +	/* if bytes < max packet then tx bytes must
> +	 * be written in packet per buffer mode
> +	 */
> +	if ((req->req.length < ep->ep.maxpacket) || !ep->num)
> +		/* write the count */
> +		req->td_data->status = (req->td_data->status &
> +					~PCH_UDC_RXTX_BYTES) | req->req.length;
> +	/* set HOST BUSY */
> +	req->td_data->status = (req->td_data->status &
> +				~PCH_UDC_BUFF_STS) | PCH_UDC_BS_HST_BSY;
> +	return retval;

Just "return 0;".

> +}


> +static int pch_udc_pcd_ep_enable(struct usb_ep *usbep,
> +				    const struct usb_endpoint_descriptor *desc)
> +{
> +	struct pch_udc_ep	*ep;
> +	struct pch_udc_dev	*dev;
> +	unsigned long		iflags;
> +
> +	if (!usbep || (usbep->name == ep0_string) || !desc ||
> +	    (desc->bDescriptorType != USB_DT_ENDPOINT) || !desc->wMaxPacketSize)
> +		return -EINVAL;
> +
> +	ep = container_of(usbep, struct pch_udc_ep, ep);
> +	dev = ep->dev;
> +
> +	dev_dbg(&dev->pdev->dev, "%s: ep %d", __func__, ep->num);
> +	if (!dev->driver || (dev->gadget.speed == USB_SPEED_UNKNOWN))
> +		return -ESHUTDOWN;
> +
> +	spin_lock_irqsave(&dev->lock, iflags);
> +	ep->desc = desc;
> +	ep->halted = 0;
> +	pch_udc_ep_enable(ep, &ep->dev->cfg_data, desc);
> +	ep->ep.maxpacket = le16_to_cpu(desc->wMaxPacketSize);
> +	pch_udc_enable_ep_interrupts(ep->dev, PCH_UDC_EPINT(ep->in, ep->num));
> +	dev_dbg(&dev->pdev->dev, "%s: %s enabled", __func__, usbep->name);
> +
> +	spin_unlock_irqrestore(&dev->lock, iflags);

Move the empty line from before unlock to after unlock, please.

> +	return 0;
> +}


> +/**
> + * pch_udc_free_request() - This function frees request structure.
> + *				It is called by gadget driver
> + * @usbep:	Reference to the USB endpoint structure
> + * @usbreq:	Reference to the USB request
> + */
> +static void pch_udc_free_request(struct usb_ep *usbep,
> +				  struct usb_request *usbreq)
> +{
> +	struct pch_udc_ep	*ep;
> +	struct pch_udc_request	*req;
> +	struct pch_udc_dev	*dev;
> +
> +	if (!usbep || !usbreq)
> +		return;
> +
> +	ep = container_of(usbep, struct pch_udc_ep, ep);
> +	req = container_of(usbreq, struct pch_udc_request, req);
> +	dev = ep->dev;
> +	dev_dbg(&dev->pdev->dev, "%s: %s  req = 0x%p", __func__, usbep->name,
> +		req);
> +
> +	if (!list_empty(&req->queue))
> +		dev_err(&dev->pdev->dev, "%s: %s  req = 0x%p  queue not empty",
> +			__func__, usbep->name, req);
> +
> +	if (req->td_data != NULL) {
> +		if (req->chain_len > 1)
> +			pch_udc_free_dma_chain(ep->dev, req);
> +		else
> +			pci_pool_free(ep->dev->data_requests, req->td_data,
> +				      req->td_data_phys);

The first td_data is never freed?  Or am I missing something?   I think the "else"
should be dropped and the second part should be done unconditionally.

> +	}
> +	kfree(req);
> +}



> +			/*
> +			* For IN trfr the descriptors will be programmed and
> +			* P bit will be set when
> +			* we get an IN token
> +			*/
> +
> +			while (pch_udc_read_ep_control(ep) & UDC_EPCTL_S)
> +				udelay(100);

Some kind of limit should be used here IMO.  What if hardware malfunctions
and the condition is never met?

> +			pch_udc_ep_clear_nak(ep);
> +			pch_udc_enable_ep_interrupts(ep->dev,
> +							 (1 << ep->num));
> +			/* enable DMA */
> +			pch_udc_set_dma(dev, DMA_DIR_TX);

> +/**
> + * pch_udc_pcd_dequeue() - This function de-queues a request packet.
> + *				It is called by gadget driver
> + * @usbep:	Reference to the USB endpoint structure
> + * @usbreq:	Reference to the USB request
> + * Returns
> + *	0:			Success
> + *	linux error number:	Failure
> + */
> +static int pch_udc_pcd_dequeue(struct usb_ep *usbep,
> +				struct usb_request *usbreq)
> +{
> +	struct pch_udc_ep	*ep;
> +	struct pch_udc_request	*req;
> +	struct pch_udc_dev	*dev;
> +	unsigned long		flags;

I would add "int ret = -EINVAL;" here,

> +
> +	ep = container_of(usbep, struct pch_udc_ep, ep);
> +	dev = ep->dev;
> +	if (!usbep || !usbreq || (!ep->desc && ep->num))
> +		return -EINVAL;
> +	dev_dbg(&dev->pdev->dev, "%s: enter  ep%d%s", __func__, ep->num,
> +		(ep->in ? "in" : "out"));
> +	req = container_of(usbreq, struct pch_udc_request, req);
> +	spin_lock_irqsave(&ep->dev->lock, flags);
> +	/* make sure it's still queued on this endpoint */
> +	list_for_each_entry(req, &ep->queue, queue) {
> +		if (&req->req == usbreq)
> +			break;

replace the above "if" with:

		if (&req->req == usbreq) {
			pch_udc_ep_set_nak(ep);
			if (!list_empty(&req->queue))
				complete_req(ep, req, -ECONNRESET);
			ret = 0;
			break;
		}

> +	}
> +
> +	if (&req->req != usbreq) {
> +		spin_unlock_irqrestore(&ep->dev->lock, flags);
> +		return -EINVAL;
> +	}
> +	pch_udc_ep_set_nak(ep);
> +	if (!list_empty(&req->queue))
> +		complete_req(ep, req, -ECONNRESET);
> +
> +	spin_unlock_irqrestore(&ep->dev->lock, flags);
> +	return 0;

And then, replace the whole section after the loop with:

	spin_unlock_irqrestore(&ep->dev->lock, flags);
	return ret;

> +}


> +static int pch_udc_pcd_set_halt(struct usb_ep *usbep, int halt)
> +{
> +	struct pch_udc_ep	*ep;
> +	struct pch_udc_dev	*dev;
> +	unsigned long iflags;
> +
> +	if (!usbep)
> +		return -EINVAL;
> +
> +	pr_debug("%s: %s: halt=%d", __func__, usbep->name, halt);
> +	ep = container_of(usbep, struct pch_udc_ep, ep);
> +	dev = ep->dev;
> +	if (!ep->desc && !ep->num) {
> +		dev_dbg(&dev->pdev->dev, "%s: ep->desc = 0x%x: ep->num = 0x%x",
> +			 __func__, (u32)(ep->desc), ep->num);
> +		return -EINVAL;
> +	}
> +	if (!ep->dev->driver || (ep->dev->gadget.speed == USB_SPEED_UNKNOWN)) {
> +		dev_dbg(&dev->pdev->dev, "%s: ep->dev->driver = 0x%x:"
> +			 " ep->dev->gadget.speed = 0x%x",
> +			 __func__, (u32)(ep->dev->driver),
> +				 ep->dev->gadget.speed);
> +		return -ESHUTDOWN;
> +	}
> +
> +	spin_lock_irqsave(&udc_stall_spinlock, iflags);
> +
> +	if (!list_empty(&ep->queue)) {
> +		dev_dbg(&dev->pdev->dev, "%s: list not empty", __func__);
> +		spin_unlock_irqrestore(&udc_stall_spinlock, iflags);
> +		return -EAGAIN;

Similarly as above, I don't like when a lock has two different unlocks.
It's better to use a "int ret;" above and a goto.  Even better, here you can
just use a chain if-else-if-...-else.

> +	}
> +	/* halt or clear halt */
> +	if (!halt) {
> +		pch_udc_ep_clear_stall(ep);
> +	} else {
> +		if (ep->num == PCH_UDC_EP0)
> +			ep->dev->stall = 1;
> +
> +		pch_udc_ep_set_stall(ep);
> +		pch_udc_enable_ep_interrupts(ep->dev,
> +					     PCH_UDC_EPINT(ep->in, ep->num));
> +	}
> +	spin_unlock_irqrestore(&udc_stall_spinlock, iflags);
> +	return 0;

Like so:

	spin_lock_irqsave(&udc_stall_spinlock, iflags);
	if (!list_empty(&ep->queue)) {
		dev_dbg(&dev->pdev->dev, "%s: list not empty", __func__);
		ret = -EAGAIN;
	} else if (!halt) {	/* halt or clear halt */
		pch_udc_ep_clear_stall(ep);
		ret = 0;
	} else {
		if (ep->num == PCH_UDC_EP0)
			ep->dev->stall = 1;
		pch_udc_ep_set_stall(ep);
		pch_udc_enable_ep_interrupts(ep->dev,
					     PCH_UDC_EPINT(ep->in, ep->num));
		ret = 0;
	}
	spin_unlock_irqrestore(&udc_stall_spinlock, iflags);
	return ret;

> +}


> +static int pch_udc_pcd_set_wedge(struct usb_ep *usbep)
> +{
> +	struct pch_udc_ep	*ep;
> +	struct pch_udc_dev	*dev;
> +	unsigned long iflags;
> +
> +	if (!usbep)
> +		return -EINVAL;
> +
> +	pr_debug("%s: %s:", __func__, usbep->name);
> +	ep = container_of(usbep, struct pch_udc_ep, ep);
> +	dev = ep->dev;
> +	if (!ep->desc && !ep->num) {
> +		dev_dbg(&dev->pdev->dev, "%s: ep->desc = 0x%x: ep->num = 0x%x",
> +			 __func__, (u32)(ep->desc), ep->num);
> +		return -EINVAL;
> +	}
> +	if (!ep->dev->driver || (ep->dev->gadget.speed == USB_SPEED_UNKNOWN)) {
> +		dev_dbg(&dev->pdev->dev, "%s: ep->dev->driver = 0x%x: "
> +			"ep->dev->gadget.speed = 0x%x", __func__,
> +			(u32)(ep->dev->driver), ep->dev->gadget.speed);
> +		return -ESHUTDOWN;
> +	}
> +
> +	spin_lock_irqsave(&udc_stall_spinlock, iflags);
> +
> +	if (!list_empty(&ep->queue)) {
> +		dev_dbg(&dev->pdev->dev, "%s: list not empty", __func__);
> +		spin_unlock_irqrestore(&udc_stall_spinlock, iflags);
> +		return -EAGAIN;

Like above, please add an "else" section and use an "int ret" to hold exit
status.  This will again allow to have only one "unlock".

> +	}
> +	/* halt */
> +	if (ep->num == PCH_UDC_EP0)
> +		ep->dev->stall = 1;
> +
> +	pch_udc_ep_set_stall(ep);
> +	pch_udc_enable_ep_interrupts(ep->dev, PCH_UDC_EPINT(ep->in, ep->num));
> +
> +	ep->dev->prot_stall = 1;
> +	spin_unlock_irqrestore(&udc_stall_spinlock, iflags);
> +	return 0;
> +}



> +static void pch_udc_pcd_fifo_flush(struct usb_ep *usbep)
> +{
> +	struct pch_udc_ep  *ep ;
> +
> +	if (!usbep)
> +		return;
> +
> +	pr_debug("%s: %s", __func__, usbep->name);
> +	ep = container_of(usbep, struct pch_udc_ep, ep);
> +	if (!ep->desc && ep->num)
> +		return;
> +	pch_udc_ep_fifo_flush(ep, ep->in);

A matter of taste I think, but why not:

	if (ep->desc || !ep->num)
		pch_udc_ep_fifo_flush(ep, ep->in);

> +}


> +static void pch_udc_start_next_txrequest(struct pch_udc_ep *ep)
> +{
> +	struct pch_udc_request *req;
> +	struct pch_udc_data_dma_desc *td_data;
> +	struct pch_udc_dev *dev = ep->dev;
> +
> +	if (pch_udc_read_ep_control(ep) & UDC_EPCTL_P)
> +		return;
> +
> +	if (list_empty(&ep->queue))
> +		return;
> +
> +	/* next request */
> +	req = list_entry(ep->queue.next, struct pch_udc_request, queue);
> +	if (req->dma_going)
> +		return;
> +
> +	dev_dbg(&dev->pdev->dev, "%s: Set request: req=%p req->td_data=%p",
> +		__func__, req, req->td_data);
> +	if (!req->td_data)
> +		return;
> +
> +	while (pch_udc_read_ep_control(ep) & UDC_EPCTL_S)
> +		udelay(100);

Again.  What if condition never gets true?

> +	req->dma_going = 1;
> +	/* Clear the descriptor pointer */
> +	pch_udc_ep_set_ddptr(ep, 0);
> +
> +	td_data = req->td_data;
> +	while (1) {
> +		td_data->status = (td_data->status & ~PCH_UDC_BUFF_STS) |
> +				   PCH_UDC_BS_HST_RDY;
> +		if ((td_data->status & PCH_UDC_DMA_LAST) == PCH_UDC_DMA_LAST)
> +			break;
> +		td_data = phys_to_virt(td_data->next);
> +	}
> +	/* Write the descriptor pointer */
> +	pch_udc_ep_set_ddptr(ep, req->td_data_phys);
> +	pch_udc_set_dma(ep->dev, DMA_DIR_TX);
> +	/* Set the poll demand bit */
> +	pch_udc_ep_set_pd(ep);
> +	pch_udc_enable_ep_interrupts(ep->dev, PCH_UDC_EPINT(ep->in, ep->num));
> +	pch_udc_ep_clear_nak(ep);
> +}


> +/**
> + * pch_udc_pcd_reinit() - This API initializes the endpoint structures
> + * @dev:	Reference to the driver structure
> + */
> +static void pch_udc_pcd_reinit(struct pch_udc_dev *dev)
> +{
> +	const char *const ep_string[] = {
> +		ep0_string, "ep0out", "ep1in", "ep1out", "ep2in", "ep2out",
> +		"ep3in", "ep3out", "ep4in", "ep4out", "ep5in", "ep5out",
> +		"ep6in", "ep6out", "ep7in", "ep7out", "ep8in", "ep8out",
> +		"ep9in", "ep9out", "ep10in", "ep10out", "ep11in", "ep11out",
> +		"ep12in", "ep12out", "ep13in", "ep13out", "ep14in", "ep14out",
> +		"ep15in", "ep15out",
> +	};
> +	int i;
> +
> +	dev->gadget.speed = USB_SPEED_UNKNOWN;
> +	INIT_LIST_HEAD(&dev->gadget.ep_list);
> +
> +	/* Initialize the endpoints structures */
> +	for (i = 0; i < PCH_UDC_EP_NUM; i++) {
> +		struct pch_udc_ep *ep = &dev->ep[i];
> +		memset(ep, 0, sizeof(*ep));

Why not just "memset(dev->ep, 0, sizeof dev->ep);" before the loop?

> +
> +		ep->desc = NULL;

Useless.  This has already been set by memset().

> +		ep->dev = dev;
> +		ep->halted = 1;
> +		ep->num = i / 2;
> +		ep->in = ((i & 1) == 0) ? 1 : 0;

Or "ep->in = ~i & 1;".

> +
> +		ep->ep.name = ep_string[i];
> +		ep->ep.ops = &pch_udc_ep_ops;
> +		if (ep->in)
> +			ep->offset_addr = ep->num * UDC_EP_REG_OFS;
> +		else
> +			ep->offset_addr = (UDC_EPINT_OUT_OFS + ep->num) *
> +					  UDC_EP_REG_OFS;
> +		/* need to set ep->ep.maxpacket and set Default Configuration?*/
> +		ep->ep.maxpacket = UDC_BULK_MAX_PKT_SIZE;
> +		list_add_tail(&ep->ep.ep_list, &dev->gadget.ep_list);
> +		INIT_LIST_HEAD(&ep->queue);
> +	}
> +	dev->ep[UDC_EP0IN_IDX].ep.maxpacket = UDC_EP0IN_MAX_PKT_SIZE;
> +	dev->ep[UDC_EP0OUT_IDX].ep.maxpacket = UDC_EP0OUT_MAX_PKT_SIZE;
> +
> +	dev->dma_addr = pci_map_single(dev->pdev, dev->ep0out_buf, 256,
> +				  PCI_DMA_FROMDEVICE);
> +
> +	/* remove ep0 in and out from the list.  They have own pointer */
> +	list_del_init(&dev->ep[UDC_EP0IN_IDX].ep.ep_list);
> +	list_del_init(&dev->ep[UDC_EP0OUT_IDX].ep.ep_list);
> +
> +	dev->gadget.ep0 = &dev->ep[UDC_EP0IN_IDX].ep;
> +	INIT_LIST_HEAD(&dev->gadget.ep0->ep_list);
> +}


> +static int init_dma_pools(struct pch_udc_dev *dev)
> +{
> +	struct pch_udc_stp_dma_desc	*td_stp;
> +	struct pch_udc_data_dma_desc	*td_data;
> +
> +	/* DMA setup */
> +	dev->data_requests = pci_pool_create("data_requests", dev->pdev,
> +		sizeof(struct pch_udc_data_dma_desc), 0, 0);
> +	if (!dev->data_requests) {
> +		dev_err(&dev->pdev->dev, "%s: can't get request data pool",
> +			__func__);
> +		return -ENOMEM;
> +	}
> +
> +	/* dma desc for setup data */
> +	dev->stp_requests = pci_pool_create("setup requests", dev->pdev,
> +		sizeof(struct pch_udc_stp_dma_desc), 0, 0);
> +	if (!dev->stp_requests) {

Why dev->data_requests? Something that has been created needs to be
destroyed during error recovery.

> +		dev_err(&dev->pdev->dev, "%s: can't get setup request pool",
> +			__func__);
> +		return -ENOMEM;
> +	}
> +	/* setup */
> +	td_stp = pci_pool_alloc(dev->stp_requests, GFP_KERNEL,
> +				&dev->ep[UDC_EP0OUT_IDX].td_stp_phys);
> +	if (!td_stp) {
> +		dev_err(&dev->pdev->dev,
> +			"%s: can't allocate setup dma descriptor", __func__);

Same here.

> +		return -ENOMEM;
> +	}
> +	dev->ep[UDC_EP0OUT_IDX].td_stp = td_stp;
> +
> +	/* data: 0 packets !? */
> +	td_data = pci_pool_alloc(dev->data_requests, GFP_KERNEL,
> +				&dev->ep[UDC_EP0OUT_IDX].td_data_phys);
> +	if (!td_data) {

And here.

> +		dev_err(&dev->pdev->dev,
> +			"%s: can't allocate data dma descriptor", __func__);
> +		return -ENOMEM;
> +	}
> +	dev->ep[UDC_EP0OUT_IDX].td_data = td_data;
> +	dev->ep[UDC_EP0IN_IDX].td_stp = NULL;
> +	dev->ep[UDC_EP0IN_IDX].td_stp_phys = 0;
> +	dev->ep[UDC_EP0IN_IDX].td_data = NULL;
> +	dev->ep[UDC_EP0IN_IDX].td_data_phys = 0;
> +	return 0;
> +}
> +


> +static void pch_udc_remove(struct pci_dev *pdev)
> +{
> +	struct pch_udc_dev	*dev = pci_get_drvdata(pdev);
> +
> +	dev_dbg(&pdev->dev, "%s: enter", __func__);
> +	/* gadget driver must not be registered */
> +	if (dev->driver)
> +		dev_err(&pdev->dev,
> +			"%s: gadget driver still bound!!!", __func__);
> +	/* dma pool cleanup */
> +	if (dev->data_requests)
> +		pci_pool_destroy(dev->data_requests);
> +
> +	if (dev->stp_requests) {
> +		/* cleanup DMA desc's for ep0in */
> +		if (dev->ep[UDC_EP0OUT_IDX].td_stp) {
> +			pci_pool_free(dev->stp_requests,
> +				dev->ep[UDC_EP0OUT_IDX].td_stp,
> +				dev->ep[UDC_EP0OUT_IDX].td_stp_phys);
> +		}
> +		if (dev->ep[UDC_EP0OUT_IDX].td_data) {
> +			pci_pool_free(dev->stp_requests,
> +				dev->ep[UDC_EP0OUT_IDX].td_data,
> +				dev->ep[UDC_EP0OUT_IDX].td_data_phys);
> +		}
> +		pci_pool_destroy(dev->stp_requests);
> +	}
> +
> +	pch_udc_exit(dev);
> +
> +	if (dev->irq_registered)
> +		free_irq(pdev->irq, dev);
> +	if (dev->base_addr)
> +		iounmap(dev->base_addr);
> +	if (dev->mem_region)
> +		release_mem_region(dev->phys_addr,
> +				   pci_resource_len(pdev, PCH_UDC_PCI_BAR));
> +	if (dev->active)
> +		pci_disable_device(pdev);
> +	if (dev->registered)
> +		device_unregister(&dev->gadget.dev);
> +	else
> +		kfree(dev);

Really? kfree(dev) in "else" clause?

> +
> +	pci_set_drvdata(pdev, NULL);
> +}

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--