RE: [PATCH v2] Move DWC2 driver out of staging

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux