> From: Dan Carpenter [mailto:[email protected]]
> Sent: Monday, November 25, 2013 6:18 AM
>
> 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?
I'll take a closer look at this.
> 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.
Right.
> 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.
Himangi Saraogi already submitted a patch to clean this up. Greg has
applied it to the staging-next branch of his staging tree. Can you
check that out and see if addresses all of your concerns?
--
Paul
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
