[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Aras Vaichas]

This patch changes the "usb storage" command to return success iff it
finds a USB storage device, otherwise it returns error.

This enables you to check for a USB storage device before trying to
access it:

if usb storage; then fatload usb 0 $loadaddr $filename; fi


______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email 
______________________________________________________________________
-------------- next part --------------
A non-text attachment was scrubbed...
Name: u-boot-1.3.2-USB-storage-test.patch
Type: text/x-patch
Size: 1289 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080325/190ad0d1/attachment.bin 

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Wolfgang Denk]

Dear Aras,

in message <47E836F8.6090702@magtech.com.au> you wrote:
> 
> This patch changes the "usb storage" command to return success iff it
> finds a USB storage device, otherwise it returns error.

Thanks. I appreciate your contribution, but please fix the coding
style:

> @@ -196,9 +196,13 @@
>  		for (i = 0; i < usb_max_devs; i++) {

There is probably a { missing before this line (as this is not a
single-line statement).

>  			printf ("  Device %d: ", i);
>  			dev_print(&usb_dev_desc[i]);
> +			return 0;
>  		}
>  	else
> +	{

...and then this should read 

	} else {

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The explanation requiring the fewest assumptions is the  most  likely
to be correct.                                    -- William of Occam

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Jerry Van Baren]

Wolfgang Denk wrote:
> Dear Aras,
> 
> in message <47E836F8.6090702@magtech.com.au> you wrote:
>> This patch changes the "usb storage" command to return success iff it
>> finds a USB storage device, otherwise it returns error.
> 
> Thanks. I appreciate your contribution, but please fix the coding
> style:
> 
>> @@ -196,9 +196,13 @@
>>  		for (i = 0; i < usb_max_devs; i++) {
> 
> There is probably a { missing before this line (as this is not a
> single-line statement).
> 
>>  			printf ("  Device %d: ", i);
>>  			dev_print(&usb_dev_desc[i]);
>> +			return 0;
>>  		}
>>  	else
>> +	{
> 
> ...and then this should read 
> 
> 	} else {
> 
> Thanks.
> 
> Best regards,
> 
> Wolfgang Denk

Just in case Wolfgang was too terse, I'll add to his critique... the 
original code is the origin of the coding violations:

void usb_stor_info(void)
{
         int i;

         if (usb_max_devs > 0)
                 for (i = 0; i < usb_max_devs; i++) {
                         printf ("  Device %d: ", i);
                         dev_print(&usb_dev_desc[i]);
                 }
         else
                 printf("No storage devices, perhaps not 'usb 
start'ed..?\n");
}

It doesn't have braces on the "if (usb_max_devs > 0)" which is 
syntactically OK but the source of the coding violation.  Please add 
braces after the "if" and "} else {" per Wolfgang's comments.

Thanks for making the code a little better and a little prettier, ;-)
gvb

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Aras Vaichas]

Jerry Van Baren wrote:
> Wolfgang Denk wrote:
>> Dear Aras,
>>
>> in message <47E836F8.6090702@magtech.com.au> you wrote:
>>> This patch changes the "usb storage" command to return success iff it
>>> finds a USB storage device, otherwise it returns error.
>>
>> Thanks. I appreciate your contribution, but please fix the coding
>> style:
Done.


______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email 
______________________________________________________________________
-------------- next part --------------
A non-text attachment was scrubbed...
Name: u-boot-1.3.2-USB-storage-test.patch
Type: text/x-patch
Size: 1327 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080325/67f12a9e/attachment.bin 

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Markus Klotzbücher]

Aras Vaichas <arasv@magtech.com.au> writes:

> Jerry Van Baren wrote:
>> Wolfgang Denk wrote:
>>> Dear Aras,
>>>
>>> in message <47E836F8.6090702@magtech.com.au> you wrote:
>>>> This patch changes the "usb storage" command to return success iff it
>>>> finds a USB storage device, otherwise it returns error.
>>>
>>> Thanks. I appreciate your contribution, but please fix the coding
>>> style:
> Done.

Thanks, i'll pick this one up. Please remember to add a proper
Signed-off-by: line next time.

Best regards

Markus Klotzbuecher

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Markus Klotzbücher]

Aras Vaichas <arasv@magtech.com.au> writes:

> ______________________________________________________________________
> This email has been scanned by the MessageLabs Email Security System.
> For more information please visit http://www.messagelabs.com/email 
> ______________________________________________________________________--- a/include/usb.h	2008-03-19 13:47:18.000000000 +1100

Its my fault for not paying attention correctly, but this line
essentially broke git-am so that this chunk didn't get applied
correctly.

> -void usb_stor_info(void)
> +int usb_stor_info(void)
>  {
>  	int i;
>  
> -	if (usb_max_devs > 0)
> +	if (usb_max_devs > 0) {
>  		for (i = 0; i < usb_max_devs; i++) {
>  			printf ("  Device %d: ", i);
>  			dev_print(&usb_dev_desc[i]);
> +			return 0;
>  		}

Returning here will result in only the first of all storage devices to
be printed.

> -	else
> +	} else {
>  		printf("No storage devices, perhaps not 'usb start'ed..?\n");
> +		return 1;
> +	}
>  }
>  
>  /*********************************************************************************

I committed the following patch to fix this

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 81d2f92..d263b6c 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -196,12 +196,12 @@ int usb_stor_info(void)
 		for (i = 0; i < usb_max_devs; i++) {
 			printf ("  Device %d: ", i);
 			dev_print(&usb_dev_desc[i]);
-			return 0;
 		}
-	} else {
-		printf("No storage devices, perhaps not 'usb start'ed..?\n");
-		return 1;
+		return 0;
 	}
+	
+	printf("No storage devices, perhaps not 'usb start'ed..?\n");
+	return 1;
 }
 
 /*********************************************************************************


Best regards

     Markus

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Jon Loeliger]

Jerry Van Baren wrote:

> Just in case Wolfgang was too terse, I'll add to his critique... the 
> original code is the origin of the coding violations:
> 
> void usb_stor_info(void)
> {
>          int i;
> 
>          if (usb_max_devs > 0)
>                  for (i = 0; i < usb_max_devs; i++) {
>                          printf ("  Device %d: ", i);
>                          dev_print(&usb_dev_desc[i]);
>                  }
>          else
>                  printf("No storage devices, perhaps not 'usb 
> start'ed..?\n");
> }
> 
> It doesn't have braces on the "if (usb_max_devs > 0)" which is 
> syntactically OK but the source of the coding violation.  Please add 
> braces after the "if" and "} else {" per Wolfgang's comments.
> 
> Thanks for making the code a little better and a little prettier, ;-)
> gvb
> 


Hey Stefan,

What do you think?  Wouldn't a policy of _always_ using
braces even for single sub-statements have just made this
a _correct_ no-brainer from the onset? :-)

jdl

PS -- No, no point.  Why do you ask?

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Scott Wood]

On Tue, Mar 25, 2008 at 09:23:43AM -0500, Jon Loeliger wrote:
> What do you think?  Wouldn't a policy of _always_ using
> braces even for single sub-statements have just made this
> a _correct_ no-brainer from the onset? :-)

Yeah, but it'd be ugly. :-)

-Scott

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Jon Loeliger]

Scott Wood wrote:
> On Tue, Mar 25, 2008 at 09:23:43AM -0500, Jon Loeliger wrote:
>> What do you think?  Wouldn't a policy of _always_ using
>> braces even for single sub-statements have just made this
>> a _correct_ no-brainer from the onset? :-)
> 
> Yeah, but it'd be ugly. :-)

Luckily, everyone is entitled to their own wrong opinion. :-)

jdl

[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Stefan Roese]

On Tuesday 25 March 2008, Jon Loeliger wrote:
> > It doesn't have braces on the "if (usb_max_devs > 0)" which is
> > syntactically OK but the source of the coding violation.  Please add
> > braces after the "if" and "} else {" per Wolfgang's comments.
> >
> > Thanks for making the code a little better and a little prettier, ;-)
> > gvb
>
> Hey Stefan,

Why me? :)

> What do you think?  Wouldn't a policy of _always_ using
> braces even for single sub-statements have just made this
> a _correct_ no-brainer from the onset? :-)

I think I read some time on LKML, that braces should be used on all multi-line 
paragraphs. So even one statement with a comment should have braces. That's 
what I would prefer. And braces on both parts of the "else" if one needs 
them. So something like is what I would vote for:

	if (a == b) {
		/* some comment */
		c = 1;
	} else {
		c = 2;
	}

But I still prefer using no braces on single line paragraph:

	if (a == b)
		c = 1;
	else
		c = 2;


Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================