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
