Re: [RFC-PATCH] Fix loads command

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Antony,

On Sun, Aug 21, 2011 at 11:08:40PM +0400, Antony Pavlov wrote:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  commands/Kconfig  |    7 +-
>  commands/Makefile |    1 -
>  commands/loads.c  |  295 ++++++++++++++++++++++++++++++-----------------------
>  common/Makefile   |    1 +
>  4 files changed, 171 insertions(+), 133 deletions(-)

There seem to be many whitespace cleanups in this patch. Can you please
post a seperate patch for these for easier review?
I think the baudrate change can be made in unconditionally.

I don't know if you are interested in doing this and I don't mind at
all if you don't, but for a full fixing of these commands I would
suggest the following:

- There should be an internal C API for each load/save function for
  noninteractive use.
- Instead of writing to some sdram offset the commands should write
  to a file descriptor.
- There should be no more than one positional argument to each command.

A good start would be to cleanup the coding style and to make it compile
again.

I just saw that you copied baudrate switching code from loadb.c. It
tries to get the current baudrate with:

(int)simple_strtoul(dev_get_param(&cdev->class_dev, "baudrate"), NULL, 10);

I wonder if it wouldn't be simpler not to change the baudrate when
no baudrate change is requested.

Another thing: since the loads stuff depends on broken and obviously was
never used since U-Boot it would also be acceptable to remove it
completely and to do a fresh start.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox