On Tue, Nov 01, 2016 at 12:55:18PM +0200, Felipe Balbi wrote:
>
> Hi,
>
> Bin Liu <b-liu@xxxxxx> writes:
> > On Wed, Sep 28, 2016 at 04:05:36PM +0300, Felipe Balbi wrote:
> >> We have introduced a helper to calculate multiplier
> >> value from wMaxPacketSize. Start using it.
> >>
> >> Cc: Bin Liu <b-liu@xxxxxx>
> >> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/usb/musb/musb_gadget.c | 6 +++---
> >> drivers/usb/musb/musb_host.c | 2 +-
> >> 2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> >> index bff4869a57cd..8a0882cc446d 100644
> >> --- a/drivers/usb/musb/musb_gadget.c
> >> +++ b/drivers/usb/musb/musb_gadget.c
> >> @@ -974,8 +974,8 @@ static int musb_gadget_enable(struct usb_ep *ep,
> >> goto fail;
> >>
> >> /* REVISIT this rules out high bandwidth periodic transfers */
> >> - tmp = usb_endpoint_maxp(desc);
> >
> > The bit 10~0 in tmp is also needed in the original code,
>
> no it's not
>
> >> - if (tmp & ~0x07ff) {
> >> + tmp = usb_endpoint_maxp_mult(desc) - 1;
> >
> > but here bit 10~0 is lost.
>
> here's the whole thing unpatched:
>
> > /* REVISIT this rules out high bandwidth periodic transfers */
> > tmp = usb_endpoint_maxp(desc);
> > if (tmp & ~0x07ff) {
>
> so, if we have a mult setting
>
> > int ok;
> >
> > if (usb_endpoint_dir_in(desc))
> > ok = musb->hb_iso_tx;
> > else
> > ok = musb->hb_iso_rx;
> >
> > if (!ok) {
> > musb_dbg(musb, "no support for high bandwidth ISO");
> > goto fail;
> > }
> > musb_ep->hb_mult = (tmp >> 11) & 3;
>
> then we save the zero-based value in hb_mult
>
> > } else {
> > musb_ep->hb_mult = 0;
>
> otherwise, set it to 0.
>
> IOW, this could be rewritten as:
>
> int ok;
>
> [...]
>
> /* REVISIT this rules out high bandwidth periodic transfers */
> tmp = usb_endpoint_maxp_mult(desc) - 1;
> if (usb_endpoint_dir_in(desc))
> ok = musb->hb_iso_tx & tmp;
> else
> ok = musb->hb_iso_rx & tmp;
>
> if (!ok) {
> musb_dbg(musb, "no support for high bandwidth ISO");
> goto fail;
> }
> musb_ep->hb_mult = tmp;
I like this new version, one level less in indentation.
>
> And nothing would change. There is, however, one small problem with this
> patch, but that's a few lines after this chunk. Here's the incremental
> patch to fix it:
>
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index 0e3404ce9465..47304560f105 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -992,7 +992,7 @@ static int musb_gadget_enable(struct usb_ep *ep,
> musb_ep->hb_mult = 0;
> }
>
> - musb_ep->packet_sz = tmp & 0x7ff;
This is the problem I meant, tmp is used here again.
> + musb_ep->packet_sz = usb_endpoint_maxp(desc);
This should fix it.
Regards,
-Bin.
--
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
