Hi, On Wed, May 04, 2016 at 11:56:15PM +0300, Sergei Shtylyov wrote: > Hello. > > On 05/04/2016 11:46 PM, Bin Liu wrote: > > >>>>Commit 754fe4a92c07 ("usb: musb: Remove ifdefs for TX DMA for > >>>>musb_host.c") > >>>>looks incomplete: the DMA engine checks are done outside the Mentor/UX500 > >>>>handler but inside the CPPI/TUSB handler. Move the checks out of the > >>>>CPPI/ > >>>>TUSB handler into its caller, musb_tx_dma_program(). > >>>> > >>>>Signed-off-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com> > >>>> > >>>>--- > >>>> drivers/usb/musb/musb_host.c | 7 +++---- > >>>> 1 file changed, 3 insertions(+), 4 deletions(-) > >>>> > >>>>Index: usb/drivers/usb/musb/musb_host.c > >>>>=================================================================== > >>>>--- usb.orig/drivers/usb/musb/musb_host.c > >>>>+++ usb/drivers/usb/musb/musb_host.c > >>>>@@ -678,9 +678,6 @@ static int musb_tx_dma_set_mode_cppi_tus > >>>> { > >>>> struct dma_channel *channel = hw_ep->tx_channel; > >>>> > >>>>- if (!is_cppi_enabled(hw_ep->musb) && !tusb_dma_omap(hw_ep->musb)) > >>>>- return -ENODEV; > >>>>- > >>>> channel->actual_len = 0; > >> > >>>Since this function has only two lines now, does it make sense to get rid > >>>of it completely? > >> > >> That would be the reverse to what Tony's patches did. I think gcc > >>will inline this function anyway. > > > >I believe the intention of Tony's patch is to get rid of #ifdefs. Any > > Right. But while doing that, he tried to avoid the code motion. > > >further patch could do whatever is right to improve the code. I > >personally don't rely on compiler's optimization. I don't have strong > >opinion here, you make the call. > > I'll leave the things as they are then. > > >>>Regards, > >>>-Bin. > >>> > >>>> > >>>> /* > >>>>@@ -704,9 +701,11 @@ static bool musb_tx_dma_program(struct d > >>>> if (musb_dma_inventra(hw_ep->musb) || > >>>> musb_dma_ux500(hw_ep->musb)) > >>>> res = musb_tx_dma_set_mode_mentor(dma, hw_ep, qh, urb, > >>>> offset, &length, > >>>> &mode); > >>>>- else > >>>>+ else if (is_cppi_enabled(hw_ep->musb) || tusb_dma_omap(hw_ep->musb)) > >>>> res = musb_tx_dma_set_mode_cppi_tusb(dma, hw_ep, qh, > >>>> urb, > >>>> offset, &length, > >>>> &mode); > >>>>+ else > >>>>+ return false; > >> > >> Well, using ret = -EINVAL; would have been more appropriate, I'd > > s/ret/res/. > > >>respin if you won't mind? > > > >I don't mind respin at all. But the function return type is bool, so > >flase is better, right? > > It'll just bail out on the *if* below:
Ok, I now understand what you meant. I was thinking about the final code of your two patches together. No, please don't respin, I like your current version better. > > >>>> if (res) > >>>> return false; > >>>> > >> > >>MBR, Seregi > > >Regards, > >-Bin. > > MBR, Sergei Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html