> From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
