Hi David,
Thanks for your review.
On Tue, Jul 15, 2014 at 10:58:03PM +0800, David Laight wrote:
> From: Alan Stern
> > On Tue, 15 Jul 2014, Pratyush Anand wrote:
> ...
> > > +static int wait_for_ss_port_enable(struct usb_device *udev,
> > > + struct usb_hub *hub,
> > > + int *port1,
> > > + u16 *portchange,
> > > + u16 *portstatus)
> >
> > Continuation lines should be indented by 2 tab stops, not 5.
> >
> > > +{
> > > + int status, wait_count_20ms = 0;
> > > +
> > > + while (wait_count_20ms++ < 20) {
> > > + status = hub_port_status(hub, *port1, portstatus, portchange);
> > > + if (status || *portstatus & USB_PORT_STAT_CONNECTION)
> > > + return status;
> > > + msleep(20);
> > > + }
> > > + return hub_port_status(hub, *port1, portstatus, portchange);
> > > +}
> >
> > This might be a little clearer:
> >
> > int status = 0, delay_ms = 0;
> >
> > whle (delay_ms < 400) {
> > if (status || (*portstatus & USB_PORT_STAT_CONNECTION))
> > break;
> > msleep(20);
> > delay_ms += 20;
> > status = hub_port_status(hub, *port1, portstatus, portchange);
> > }
> > return status;
>
> I think I would write:
> for (ms_delay = 0; (ms_delay += 20) <= 400; msleep(20)) {
> status = hub_port_status(hub, *port1, portstatus, portchange);
> if (status)
> return status;
> if (*portstatus & USB_PORT_STAT_CONNECTION)
> return 0;
> }
> return 0;
I think Alan's loop will save call time of one hub_port_status in case
of good device, where CSC bit was already set before this function is
reached. Also its more readable for me. So may be I should go with that.
~Pratyush
>
> David
>
>
--
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
