* Bin Liu <b-...@ti.com> [160504 14:10]:
> 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.
Yeah looks fine to me. I intentionally tried to avoid any kind of code
changes to not break the driver while adding the multiarch support..
Tony
--
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
