* [U-Boot] [PATCH]: Smart Autoboot
2011-04-28 12:58 [U-Boot] [PATCH]: Smart Autoboot Johannes Thoma
@ 2011-04-28 13:41 ` Wolfgang Denk
2011-04-28 14:14 ` Johannes Thoma
0 siblings, 1 reply; 3+ messages in thread
From: Wolfgang Denk @ 2011-04-28 13:41 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 3+ messages in thread