On Sun, Aug 11, 2013 at 12:50:19PM -0700, Paul Zimmerman wrote:
> +static int dwc2_find_single_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh
> *qh)
> +{
> + unsigned short utime = qh->usecs;
> + int done = 0;
> + int i = 0;
> + int ret = -1;
> +
> + while (!done) {
I don't care for these while (!done) loops. If there is nothing
else to use as a limitter then just do while (1). In this case, we
are count from 0 to 7 so it should be:
static int dwc2_find_single_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
{
unsigned short utime = qh->usecs;
int i;
for (i = 0; i < 8; i++) {
if (utime <= hsotg->frame_usecs[i]) {
hsotg->frame_usecs[i] -= utime;
qh->frame_usecs[i] += utime;
return i;
}
}
return -1;
}
Notice how I've separated out the success and failure paths?
Now we don't it's immediately clear what the success and return
values are. The error code is returned to dwc2_schedule_periodic()
which has some spaghetti code and then prints a misleading error
message.
> + /* At the start hsotg->frame_usecs[i] = max_uframe_usecs[i] */
> + if (utime <= hsotg->frame_usecs[i]) {
> + hsotg->frame_usecs[i] -= utime;
> + qh->frame_usecs[i] += utime;
> + ret = i;
> + done = 1;
> + } else {
> + i++;
> + if (i == 8)
> + done = 1;
> + }
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * use this for FS apps that can span multiple uframes
> + */
> +static int dwc2_find_multi_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh
> *qh)
> +{
> + unsigned short utime = qh->usecs;
> + unsigned short xtime;
> + int t_left = utime;
No need to set t_left here.
> + int done = 0;
> + int i = 0;
> + int j;
> + int ret = -1;
> +
> + while (!done) {
for (i = 0; i < 8; i++) {
> + if (hsotg->frame_usecs[i] <= 0) {
This test is worrying because ->frame_usecs[] is unsigned and can't
be less than zero.
> + i++;
> + if (i == 8) {
> + ret = -1;
> + done = 1;
> + }
> + continue;
> + }
This chunk becomes:
if (hsotg->frame_usecs[i] <= 0)
continue;
> +
> + /*
> + * we need n consecutive slots so use j as a start slot
> + * j plus j+1 must be enough time (for now)
> + */
> + xtime = hsotg->frame_usecs[i];
> + for (j = i + 1; j < 8; j++) {
> + /*
> + * if we add this frame remaining time to xtime we may
> + * be OK, if not we need to test j for a complete frame
> + */
> + if (xtime + hsotg->frame_usecs[j] < utime) {
> + if (hsotg->frame_usecs[j] <
> + max_uframe_usecs[j]) {
> + ret = -1;
> + break;
> + }
> + }
> + if (xtime >= utime) {
> + ret = i;
I would like to move the code from after the loop to here but we
would run into the 80 character limit. One option is to add a
variable and then test it when we have fewer indents, but a better
option is to create a new function. Anyway, here is what it looks
like:
t_left = utime;
for (j = i; j < 8; j++) {
t_left -= hsotg->frame_usecs[j];
if (t_left <= 0) {
qh->frame_usecs[j] +=
hsotg->frame_usecs[j] +
t_left;
hsotg->frame_usecs[j] = -t_left;
return i;
}
qh->frame_usecs[j] +=
hsotg->frame_usecs[j];
hsotg->frame_usecs[j] = 0;
}
I'm not sure we should be saying "j = i" or if it should be
"j = i + 1". I removed the "t_left > 0" because we just return
directly now.
> + break;
> + }
> + /* add the frame time to x time */
> + xtime += hsotg->frame_usecs[j];
> + /* we must have a fully available next frame or break */
> + if (xtime < utime &&
> + hsotg->frame_usecs[j] == max_uframe_usecs[j]) {
> + ret = -1;
> + break;
> + }
> + }
Now that I've removed the "ret" and "done" variables the rest of
this loop can be deleted.
> + if (ret >= 0) {
> + t_left = utime;
> + for (j = i; t_left > 0 && j < 8; j++) {
> + t_left -= hsotg->frame_usecs[j];
> + if (t_left <= 0) {
> + qh->frame_usecs[j] +=
> + hsotg->frame_usecs[j] + t_left;
> + hsotg->frame_usecs[j] = -t_left;
> + ret = i;
Btw, "ret" had already been set to i at this point but it's a
tangled mess of code and it's almost impossible for anyone to know
what's going on.
> + done = 1;
> + } else {
> + qh->frame_usecs[j] +=
> + hsotg->frame_usecs[j];
> + hsotg->frame_usecs[j] = 0;
> + }
> + }
> + } else {
> + i++;
> + if (i == 8) {
> + ret = -1;
> + done = 1;
> + }
> + }
> + }
> +
> + return ret;
return -1;
regards,
dan carpenter
--
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
