lkml.org 
[lkml]   [2010]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] USB device driver of Topcliff PCH
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--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2010-09-20 10:45    [W:0.054 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site