USB Storage, add meaningful return value

PATCH New Aras Vaichas 7 participants 10 messages Mar 24, 2008 – Mar 26, 2008

Share & tools

Aras Vaichas

author

2008-03-24 23:19 UTC #0 lore raw

PATCH 1/1 USB Storage, add meaningful return value

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

Wolfgang Denk

2008-03-25 00:07 UTC #1 lore raw

PATCH 1/1 USB Storage, add meaningful return value

Dear Aras,

Quoted 3 lines · in message <[email protected]> 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:

Review line 196

Quoted 2 lines

(quoted diff) +0 -0

@@ -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).

Review line 202

Quoted 6 lines

(quoted diff) +2 -0

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

...and then this should read

	} else {

Thanks.

Best regards,

Wolfgang Denk

Signature · 5 lines

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

Jerry Van Baren

2008-03-25 00:23 UTC #2 lore raw

PATCH 1/1 USB Storage, add meaningful return value

Quoted 24 lines · Wolfgang Denk

Dear Aras,
Quoted 2 lines · in message <[email protected]> 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:
Review line 196
Quoted 2 lines
(quoted diff) +0 -0
@@ -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).
Review line 202
Quoted 6 lines
(quoted diff) +2 -0
 			printf ("  Device %d: ", i);
 			dev_print(&usb_dev_desc[i]);
+			return 0;
 		}
 	else
+	{
...and then this should read

Quoted 7 lines

	} 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

Aras Vaichas

author

2008-03-25 01:09 UTC #3 lore raw

PATCH 1/1 USB Storage, add meaningful return value

Quoted 9 lines · Jerry Van Baren

Quoted 8 lines · Wolfgang Denk
Dear Aras,
Quoted 2 lines · in message <[email protected]> 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

Markus Klotzbücher

2008-03-25 07:36 UTC #4 lore raw

PATCH 1/1 USB Storage, add meaningful return value

Quoted 11 lines · Aras Vaichas

Quoted 9 lines · Jerry Van Baren
Quoted 8 lines · Wolfgang Denk
Dear Aras,
Quoted 2 lines · in message <[email protected]> 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

Jon Loeliger

2008-03-25 14:23 UTC #6 lore raw

PATCH 1/1 USB Storage, add meaningful return value

In reply to Jerry Van Baren

Quoted 17 lines · Jerry Van Baren

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");
}

Quoted 7 lines

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?

Scott Wood

2008-03-25 14:34 UTC #7 lore raw

PATCH 1/1 USB Storage, add meaningful return value

Quoted 3 lines · Jon Loeliger

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

Jon Loeliger

2008-03-25 14:36 UTC #8 lore raw

PATCH 1/1 USB Storage, add meaningful return value

Quoted 6 lines · Scott Wood

Quoted 3 lines · Jon Loeliger
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

Stefan Roese

2008-03-25 14:40 UTC #9 lore raw

PATCH 1/1 USB Storage, add meaningful return value

In reply to Jon Loeliger

Quoted 8 lines · Jon Loeliger

Quoted 6 lines
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? :)

Quoted 3 lines

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
=====================================================================

Markus Klotzbücher

2008-03-26 18:14 UTC #5 lore raw

PATCH 1/1 USB Storage, add meaningful return value

In reply to Aras Vaichas

Quoted 4 lines · Aras Vaichas

______________________________________________________________________
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.

Review

Quoted 12 lines

(quoted diff) +3 -2

-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.

Review

Quoted 8 lines

(quoted diff) +3 -1

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

I committed the following patch to fix this

common/usb_storage.c +4 -4

index 81d2f92..d263b6c 100644
@@ -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