I have reviewed the second half of the driver now.
drivers/staging/dwc2/hcd_ddma.c
616 static void dwc2_fill_host_dma_desc(struct dwc2_hsotg *hsotg,
617 struct dwc2_host_chan *chan,
618 struct dwc2_qtd *qtd, struct dwc2_qh *qh,
619 int n_desc)
620 {
621 struct dwc2_hcd_dma_desc *dma_desc = &qh->desc_list[n_desc];
622 int len = chan->xfer_len;
623
624 if (len > MAX_DMA_DESC_SIZE)
625 len = MAX_DMA_DESC_SIZE - chan->max_packet + 1;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I don't understand what we are doing here. Shouldn't the condition and
the assignment match?
626
drivers/staging/dwc2/hcd_intr.c
930 if (!len) {
931 qtd->complete_split = 0;
932 qtd->isoc_split_offset = 0;
933 return 0;
934 }
935
936 frame_desc->actual_length += len;
937
938 if (chan->align_buf && len) {
^^^
This can be removed.
939 dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
940 dma_sync_single_for_cpu(hsotg->dev, qtd->urb->dma,
941 qtd->urb->length, DMA_FROM_DEVICE);
942 memcpy(qtd->urb->buf + frame_desc->offset +
943 qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
944 dma_sync_single_for_device(hsotg->dev, qtd->urb->dma,
945 qtd->urb->length, DMA_FROM_DEVICE);
In drivers/staging/dwc2/hcd_queue.c there are a coup nasty
while(!done) loops. The whole point of a while loop is the you put the
end condition inside the condition part of the loop. Saying
"while (!done)" is crap because it looks useful but provides no actual
information about when the loop is over. If there are more than one
done condition then use break statements. In this case we are just
iterating over an array and there is a C language construct called a
"for loop" to express it.
OLD CODE:
344 static int dwc2_find_single_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
345 {
346 unsigned short utime = qh->usecs;
347 int done = 0;
348 int i = 0;
349 int ret = -1;
350
351 while (!done) {
352 /* At the start hsotg->frame_usecs[i] = max_uframe_usecs[i] */
353 if (utime <= hsotg->frame_usecs[i]) {
354 hsotg->frame_usecs[i] -= utime;
355 qh->frame_usecs[i] += utime;
356 ret = i;
357 done = 1;
358 } else {
359 i++;
360 if (i == 8)
361 done = 1;
362 }
363 }
364
365 return ret;
366 }
NEW CODE (written using a for loop).
344 static int dwc2_find_single_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
345 {
346 unsigned short utime = qh->usecs;
347 int i;
348
349 for (i = 0; i < 8; i++) {
350 /* At the start hsotg->frame_usecs[i] = max_uframe_usecs[i] */
351 if (utime <= hsotg->frame_usecs[i]) {
352 hsotg->frame_usecs[i] -= utime;
353 qh->frame_usecs[i] += utime;
354 break;
355 }
356 }
357 if (i == 8)
358 return -1;
359 return i;
360 }
The while (!done) loop in dwc2_find_multi_uframe() is confusing and I
suspect it may be buggy. You might need to use a continue statement...
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
