> From: Matthijs Kooijman [mailto:matth...@stdin.nl]
> Sent: Monday, August 12, 2013 12:36 AM
> To: Paul Zimmerman
> Cc: gre...@linuxfoundation.org; linux-...@vger.kernel.org; 
> de...@linuxdriverproject.org;
> swar...@wwwdotorg.org; gordon.hollingwo...@gmail.com; sk...@netbsd.org; Dom 
> Cobley
> Subject: Re: [PATCH 2/3] staging: dwc2: add NAK holdoff patch from downstream 
> Pi kernel
> 
> Hi Paul,
> 
> > Add the NAK holdoff patch from the downstream Raspberry Pi kernel.
> > This allows the transfer scheduler to better handle "cheeky devices
> > that just hold off using NAKs".
> 
> > @@ -365,6 +366,7 @@ struct dwc2_hsotg {
> >     u8 otg_port;
> >     u32 *frame_list;
> >     dma_addr_t frame_list_dma;
> > +   int next_sched_frame;
> 
> This variable is still not really used, I think. Most of the mentions in
> the patch are assignments, except for these two:
> 
> > +           if (list_empty(&hsotg->periodic_sched_inactive) ||
> > +               dwc2_frame_num_le(qh->sched_frame, hsotg->next_sched_frame))
> > +                   hsotg->next_sched_frame = qh->sched_frame;
> > +
> ...
> > +                           if (!dwc2_frame_num_le(hsotg->next_sched_frame,
> > +                                                  qh->sched_frame))
> > +                                   hsotg->next_sched_frame =
> > +                                                   qh->sched_frame;
> 
> However, these two "uses" of the variable have only the single purpose
> of updating the variable itself, no other behaviour is influenced by it.
> In effect, the variable does not influence the code at all and can be
> dropped, significantly simplifying this patch.

Yep, that's kind of embarrassing. I will have to go back and read the
downstream driver code again to figure out how this code is supposed to
work. Thanks for the review.

-- 
Paul


_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to