[U-Boot] [patch 1/2] fix USB initialisation procedure

[Jerry Van Baren]

Stefan Roese wrote:
> On Thursday 09 October 2008, Markus Klotzbücher wrote:
>> On Thu, Oct 09, 2008 at 03:19:22PM +0200, Wolfgang Denk wrote:
>>> Dear Jean-Christophe PLAGNIOL-VILLARD,
>>>
>>> In message <20081009121044.GA25278 at game.jcrosoft.org> you wrote:
>>>>>  	if (dev->status == 0)
>>>>>  		return dev->act_len;
>>>> please add {} to if too or remove the else
>>>>
>>>>> -	else
>>>>> +	else {
>>>>> +		/* Let's wait a while for the timeout to elaps.
>>>>> +		 * it has no real use, but it keeps the interface happy. */
>>>>> +		wait_ms(timeout);
>>>>>  		return -1;
>>>>> +	}
>>> Good catch.
>> Quite honest, I think this is *way* to pedantic. I'd really prefer to
>> let people who are contributing significantly do their work instead of
>> bugging them with such rare coding style violations.
> 
> ACK from me here. "Minor" coding style violations like these braces issue or 
> too long lines should not delay patches.
> 
>>> Actually the "else" should be removed.
>> How so?
> 
> Because of the "return dev->act_len;" above. :)
> 
> Best regards,
> Stefan

Also agreed.

While we are painting the bike shed, I would suggest a better structure 
would be to flip the conditional so that the normal flow goes out the 
end of the module and the abnormal flow returns early.  (The comment is 
also a coding standards violation for multiline comments and has two 
typos in it.)

	if (dev->status != 0) {
		/*
		 * Let's wait a while for the timeout to elapse.
		 * It has no real use, but it keeps the interface happy.
		 */
		wait_ms(timeout);
		return -1;
	}

	return dev->act_len;
}

Hmmm, looking at the original changeset, my above point has some 
validity.  While we (gcc) can do a flow analysis and recognize that 
something will be returned in all cases due to the if/else, it doesn't 
jump out at a casual observation of the code.  On the surface, the code 
looks like it can fall off the end of the function (e.g. return "void"), 
which would be very wrong.  Making it obvious that the Right Thing[tm] 
is returned: priceless.

> --- u-boot-git-22092008.orig/common/usb.c	2008-10-08 10:51:49.000000000 +0200
> +++ u-boot-git-22092008/common/usb.c	2008-10-08 10:51:50.000000000 +0200
> @@ -196,15 +196,14 @@ int usb_control_msg(struct usb_device *d
>  	if (timeout == 0)
>  		return (int)size;
> 
> -	while (timeout--) {
> -		if (!((volatile unsigned long)dev->status & USB_ST_NOT_PROC))
> -			break;
> -		wait_ms(1);
> -	}
>  	if (dev->status == 0)
>  		return dev->act_len;
> -	else
> +	else {
> +		/* Let's wait a while for the timeout to elaps.
> +		 * it has no real use, but it keeps the interface happy. */
> +		wait_ms(timeout);
>  		return -1;
> +	}
>  }

I like green paint,
gvb