From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Thu, 28 Apr 2011 15:41:45 +0200
Subject: [U-Boot] [PATCH]: Smart Autoboot
In-Reply-To: <20110428125810.123810@gmx.net>
References: <20110428125810.123810@gmx.net>
Message-ID: <20110428134145.42843D52709@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear "Johannes Thoma",

In message <20110428125810.123810@gmx.net> you wrote:
> 
> I wrote a small patch that checks on autoboot if the image is a kernel
> image and if not assumes that it is a standalone image. If the image
> is kernel do_bootm is used else (unconditionally) do_go is used.
> This is handy if you are working with test applications and kernels,
> since you only need to reconfigure your DHCP server and don't need
> to re-flash u-boot.

The same can be done with minimal scripting, without changing any
code.  Your solution may work for you, but I don't think this is how
to solve such an issue in  general.

> Since I am new to u-boot I'd like to ask if the following patch
> is formatted correctly (I have used git format-patch) and if
> there is the possiblity to add this feature to mainline UBoot.

Second part of the question first:  I consider this a highly specific
feature which is not needed by many people, and if it was needed, if
can be implemented with mninimal scripting, so I don't think adding
this code to mainline is a good idea.

For the formal issues: most things look good, except:

> From c631d05e225fa3f87b0a5cad0bb033063e61018c Mon Sep 17 00:00:00 2001
> From: Johannes Thoma <johannes.thoma@gmx.at>
> Date: Thu, 28 Apr 2011 14:25:39 +0200
> Subject: [PATCH] Smart autoboot
> 
> We check if the image is a kernel image and use bootm command
> to boot the OS, else we do a go.
> 
> To use this feature, #define CONFIG_SMART_AUTOBOOT in your config
> and set the environment variable autostart to "yes". Then your
> DHCP driver can be used to either boot a kernel image or a
> u-boot application without changing anything on the target.

Signed-off-by: line missing.

> ---
>  README           |    5 +++++
>  common/cmd_net.c |   25 +++++++++++++++++++++++--
>  2 files changed, 28 insertions(+), 2 deletions(-)
...
> +extern int do_bootm (cmd_tbl_t *, int, int, char * const []);
> +#ifdef CONFIG_SMART_AUTOBOOT
> +extern int do_go (cmd_tbl_t *, int, int, char * const []);

externs should be avoided; use prototype declarations in the
respective header files.

> +
> +

Don't add too much white space - a single blank line is enough.

>  static int netboot_common (proto_t, cmd_tbl_t *, int , char * const []);
> 
>  int do_bootp (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> @@ -212,14 +218,29 @@ netboot_common (proto_t proto, cmd_tbl_t *cmdtp, int argc, char * const argv[])
> 
>         /* Loading ok, check if we should attempt an auto-start */
>         if (((s = getenv("autostart")) != NULL) && (strcmp(s,"yes") == 0)) {
> -               char *local_args[2];
> +               char *local_args[3];

Indentation must be done by TABs, not spaces.

...
> +#ifdef CONFIG_SMART_AUTOBOOT
> +               if (image_get_type((void*) load_addr) == IH_TYPE_KERNEL) 

Don't add trailing white space.


It appears you did not run checkpatch, which would have caught most
of these issues.

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
Abstainer: A weak person who yields to the temptation of denying him-
self a pleasure. A total abstainer is one who  abstains  from  every-
thing  but  abstention, and especially from inactivity in the affairs
of others.                                           - Ambrose Bierce