Hi Simon, Thank you for reviewing. I will split the patch as you suggested, and resubmit.
Vladimir > -----Original Message----- > From: Simon Glass [mailto:s...@chromium.org] > Sent: Monday, November 18, 2019 5:21 PM > To: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Heinrich Schuchardt > <xypron.g...@gmx.de>; Suji Velupiallai <suji.velupil...@broadcom.com> > Subject: Re: [PATCH v1 1/1] cmd: adding malloc, math, and strcmp commands > to u-boot > > Hi Vladimir, > > On Mon, 18 Nov 2019 at 16:27, Vladimir Olovyannikov > <vladimir.olovyanni...@broadcom.com> wrote: > > > > cmd: adding malloc, math, and strcmp u-boot commands. > > U-Boot > > > - malloc supports allocation of heap memory and free allocated memory > > via u-boot command line. > > U-Boot (please fix globally) > > > - math provides math commands such as "add", "sub", "mul", "div", > > "shift", ability to convert dec->hex. > > - strcmp provides string compare command feature for a script. > > > > All these commands are introduced to be used in u-boot scripts. > > > > Signed-off-by: Suji Velupiallai <suji.velupil...@broadcom.com> > > Signed-off-by: Vladimir Olovyannikov > > <vladimir.olovyanni...@broadcom.com> > > --- > > cmd/Kconfig | 21 ++++++++++++++ > > cmd/Makefile | 4 +++ > > cmd/malloc.c | 54 ++++++++++++++++++++++++++++++++++++ > > cmd/math.c | 78 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > cmd/strcmp.c | 28 +++++++++++++++++++ > > 5 files changed, 185 insertions(+) > > create mode 100644 cmd/malloc.c > > create mode 100644 cmd/math.c > > create mode 100644 cmd/strcmp.c > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig index cf982ff65e..f11903fe3d > > 100644 > > --- a/cmd/Kconfig > > +++ b/cmd/Kconfig > > @@ -1286,6 +1286,20 @@ config CMD_ITEST > > help > > Return true/false on integer compare. > > > > +config CMD_MALLOC > > + bool "malloc" > > + default y > > + help > > + Supports allocation of heap memory and free allocated memory > commands. > > + These commands are used by u-boot scripts. > > + > > +config CMD_MATH > > + bool "math" > > + default y > > + help > > + Provides math commands such as add, sub, mul, div, shift, > > + convert decimal to hex functionalities to be available in the > > script. > > + > > config CMD_SOURCE > > bool "source" > > default y > > @@ -1301,6 +1315,13 @@ config CMD_SETEXPR > > Also supports loading the value at a memory location into a > > variable. > > If CONFIG_REGEX is enabled, setexpr also supports a gsub > > function. > > I think this would be better as three patches. > > > > > +config CMD_STRCMP > > + bool "strcmp" > > + default y > > + help > > + Provides string compare command feature to u-boot scripts. > > U-Boot > > > + > > + > > endmenu > > > > menu "Android support commands" > > diff --git a/cmd/Makefile b/cmd/Makefile index 2d723ea0f0..942d60a0a2 > > 100644 > > --- a/cmd/Makefile > > +++ b/cmd/Makefile > > @@ -164,6 +164,10 @@ obj-$(CONFIG_CMD_GPT) += gpt.o > > obj-$(CONFIG_CMD_ETHSW) += ethsw.o > > obj-$(CONFIG_CMD_AXI) += axi.o > > > > +obj-$(CONFIG_CMD_MALLOC) += malloc.o > > +obj-$(CONFIG_CMD_MATH) += math.o > > +obj-$(CONFIG_CMD_STRCMP) += strcmp.o > > + > > # Power > > obj-$(CONFIG_CMD_PMIC) += pmic.o > > obj-$(CONFIG_CMD_REGULATOR) += regulator.o diff --git a/cmd/malloc.c > > b/cmd/malloc.c new file mode 100644 index 0000000000..e11e030a59 > > --- /dev/null > > +++ b/cmd/malloc.c > > @@ -0,0 +1,54 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright 2019 Broadcom > > + */ > > + > > +#include <common.h> > > +#include <command.h> > > +#include <environment.h> > > +#include <errno.h> > > +#include <malloc.h> > > + > > +static unsigned long get_value(const char *val) > > Needs a comment > > > +{ > > + char *env = env_get((char *)val); > > + > > + if (env) > > + return simple_strtoul(env, NULL, 16); > > + else > > + return simple_strtoul(val, NULL, 16); } > > + > > +static int do_malloc(cmd_tbl_t *cmdtp, int flag, int argc, char > > +*const argv[]) { > > + char numberbuf[32]; > > + void *mem; > > const? > > > + > > + if (argc < 3) > > + return cmd_usage(cmdtp); > > + > > + mem = memalign(ARCH_DMA_MINALIGN, get_value(argv[2])); > > + if (mem) { > > + sprintf(numberbuf, "%08x", (unsigned int)mem); > > I think (ulong) would be better > > > + env_set(argv[1], numberbuf); > > blank line before return (please fix globally) > > > + return 0; > > + } > > + return -EINVAL; > > You can't return errors from command functions. Try printing an error and > return CMD_RET_FAILURE instead. > > > +} > > + > > +static int do_free(cmd_tbl_t *cmdtp, int flag, int argc, char *const > > +argv[]) { > > + if (argc < 2) > > + return cmd_usage(cmdtp); > > + > > + free((void *)get_value(argv[1])); > > + env_set(argv[1], ""); > > + return 0; > > +} > > + > > +U_BOOT_CMD(malloc, 3, 0, do_malloc, > > + "Allocate memory from u-boot heap and store pointer in > environment variable.", > > + "target size\n"); > > Needs better help - e.g. list arguments > > > + > > +U_BOOT_CMD(free, 2, 0, do_free, > > + "Release memory from u-boot heap at target.", "target\n"); > > I wonder if it would be better to have a 'mem' command, then have 'mem > alloc', 'mem free', etc.? > > > diff --git a/cmd/math.c b/cmd/math.c > > new file mode 100644 > > index 0000000000..17de5ef70b > > --- /dev/null > > +++ b/cmd/math.c > > @@ -0,0 +1,78 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright 2010-2017 Broadcom > > + */ > > + > > +#include <common.h> > > +#include <command.h> > > +#include <environment.h> > > + > > +unsigned long long simple_strtoull(const char *cp, char **endp, > > + unsigned int base); > > Use #include <vsprintf.h> instead > > > + > > +static unsigned long long get_value(const char *val) > > Function comment > > > +{ > > + char *env = env_get((char *)val); > > + > > + if (env) > > + return simple_strtoull(env, NULL, 16); > > + else > > + return simple_strtoull(val, NULL, 16); } > > + > > +static unsigned long long get_value_base10(const char *val) { > > + char *env = env_get((char *)val); > > + > > + if (env) > > + return simple_strtoull(env, NULL, 10); > > + else > > + return simple_strtoull(val, NULL, 10); } > > + > > +/* > > + * Top level addenv command > > + */ > > +#define IS_MATH_CMD(cmd, args) ((strcmp(argv[1], cmd) == 0) && \ > > + (argc == (args))) > > Use a function instead of a macro. > > > + > > +static int do_math(cmd_tbl_t *cmdtp, int flag, int argc, char * const > > +argv[]) { > > + char numberbuf[32]; > > + unsigned long long i = 0; > > + > > + if (IS_MATH_CMD("add", 5)) > > + i = get_value(argv[3]) + get_value(argv[4]); > > It might be better to do something like: > > const char *left = NULL, *right = NULL; > > if (argc > 3) > left = argv[3]; > ... > > Then you can use 'left' and 'right' below. > > > + else if (IS_MATH_CMD("sub", 5)) > > + i = get_value(argv[3]) - get_value(argv[4]); > > + else if (IS_MATH_CMD("mul", 5)) > > + i = get_value(argv[3]) * get_value(argv[4]); > > + else if (IS_MATH_CMD("div", 5)) > > + i = get_value(argv[3]) / get_value(argv[4]); > > + else if (IS_MATH_CMD("shl", 5)) > > + i = get_value(argv[3]) << get_value(argv[4]); > > + else if (IS_MATH_CMD("d2h", 4)) > > + i = get_value_base10(argv[3]); > > + else > > + return cmd_usage(cmdtp); > > + > > + sprintf(numberbuf, "%llx", i); > > + env_set(argv[2], numberbuf); > > + return 0; > > +} > > + > > +U_BOOT_CMD(math, 5, 0, do_math, > > + "Environment variable math.", > > + "add a b c\n" > > + " - add b to c and store in a.\n" > > But also b and c can be numbers, right? You should mention that. > > > + "math sub a b c\n" > > + " - subtract b from c and store in a.\n" > > + "math mul a b c\n" > > + " - multiply b by c and store in a.\n" > > + "math div a b c\n" > > + " - divide b by c and store in a.\n" > > + "math shl a b c\n" > > + " - shift left b by c and store in a.\n" > > + "math d2h a b\n" > > + " - Convert b from decimal to hex and store in a.\n" > > +); > > diff --git a/cmd/strcmp.c b/cmd/strcmp.c new file mode 100644 index > > 0000000000..3abd270d40 > > --- /dev/null > > +++ b/cmd/strcmp.c > > @@ -0,0 +1,28 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright 2010-2017 Broadcom > > + */ > > + > > +#include <common.h> > > +#include <command.h> > > + > > +static int do_strcmp(cmd_tbl_t *cmdtp, int flag, int argc, char * > > +const argv[]) { > > + if (argc != 3) > > + return cmd_usage(cmdtp); > > + > > + if (strlen(argv[1]) != strlen(argv[2])) > > + return CMD_RET_FAILURE; > > + > > + /* Compare the temporary string to the given parameter */ > > + if (!strncmp(argv[1], argv[2], strlen(argv[2]))) > > + return CMD_RET_SUCCESS; > > + > > + return CMD_RET_FAILURE; > > +} > > + > > +U_BOOT_CMD(strcmp, 3, 0, do_strcmp, > > + "String to string comparison.", > > + "[str] [str]\n" > > str1 str2, I think > > > + " - Returns true if str are same, else returns false.\n" > > +); > > -- > > 2.17.1 > > > > Regards, > Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
