Hi Joe,
> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershber...@gmail.com]
> Sent: Friday, June 26, 2015 1:31 AM
> To: Ciubotariu Codrin Constantin-B43658
> Cc: u-boot; Joe Hershberger; Sun York-R58495
> Subject: Re: [U-Boot] [PATCH 04/11 v2] drivers/net/vsc9953: Refractor the
> parser
> for VSC9953 commands
>
> > static struct vsc9953_info vsc9953_l2sw = { @@ -575,6 +576,10 @@ void
> > vsc9953_init(bd_t *bis) }
> >
> > #ifdef CONFIG_VSC9953_CMD
>
> I'd like to see this moved to its own file in common... maybe
> "common/cmd_ethsw.c". I'd also like to see this #define change to something
> like
> "CONFIG_CMD_ETHSW".
>
> These changes don't necessarily need to be part of this series, since it
> already
> got in as is, but if you feel motivated, I would recommend you add a patch
> before this one that moves it.
I could move this parser in common/do_ethsw.c and rename the define. I guess
this would imply that upcoming drivers for Ethernet L2 Switches could use the
same commands while calling their specific functions.
> > -/* function to interpret commands starting with "ethsw " */ -static
> > int do_ethsw(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > argv[])
> > +struct command_def {
> > + int cmd_to_keywords[VSC9953_MAX_CMD_PARAMS];
> > + int cmd_keywords_nr;
> > + int port;
> > + int err;
>
> Remove this. Just use a return value.
Ok, I will remove "err" and all the other references to it.
> > +#define VSC9953_PORT_CONF_HELP "[port <port_no>] { enable | disable |
> > +show } " \
> > +"- enable/disable a port; show shows a port's configuration"
>
> Probably better to define this down by the use.
Ok.
> > return -1;
>
> Please use "return CMD_RET_USAGE;" from include/command.h.
Ok, I wasn't aware of these macros. I will make use them in this patch set when
it's appropriate.
> > +/* match optional keywords */
> > +static void cmd_keywords_opt_check(struct command_def *parsed_cmd,
> > + int *argc_val) {
> > + int i, keyw_opt_id, argc_val_max;
>
> Use a single space. Put each var on a separate line.
Ok, I will make sure this applies throughout the whole patch set.
>
> > +
> > + /* remember the best match */
> > + argc_val_max = *argc_val;
> > +
> > + for (i = 0; i < ARRAY_SIZE(cmd_opt_def); i++) {
> > + keyw_opt_id = 0;
> > + while (keyw_opt_id + *argc_val <
> > + parsed_cmd->cmd_keywords_nr &&
> > + cmd_opt_def[i].cmd_keyword[keyw_opt_id] != id_key_end
> &&
> > + parsed_cmd->cmd_to_keywords[keyw_opt_id + *argc_val]
> > ==
> > + cmd_opt_def[i].cmd_keyword[keyw_opt_id])
> > + keyw_opt_id++;
>
> It might help to break this up a bit and use some intermediate variables to
> make
> the code more readable.
Ok.
>
> > + if (keyw_opt_id && keyw_opt_id + *argc_val <=
> > + parsed_cmd->cmd_keywords_nr &&
> > + cmd_opt_def[i].cmd_keyword[keyw_opt_id] == id_key_end &&
> > + (*argc_val + keyw_opt_id > argc_val_max))
>
> This could benefit from a comment describing what you expect to be verified.
Ok.
> > +
> > + for (i = 0; i < ARRAY_SIZE(cmd_def); i++) {
> > + keyword_id = 0;
> > + while (keyword_id + *argc_val < parsed_cmd->cmd_keywords_nr
> > &&
> > + cmd_def[i].cmd_keyword[keyword_id] != id_key_end &&
> > + parsed_cmd->cmd_to_keywords[keyword_id + *argc_val]
> > ==
> > + cmd_def[i].cmd_keyword[keyword_id])
> > + keyword_id++;
>
> It might help to break this up a bit and use some intermediate variables to
> make
> the code more readable.
Ok.
>
> > + if (keyword_id && keyword_id + *argc_val ==
> > + parsed_cmd->cmd_keywords_nr &&
> > + cmd_def[i].cmd_keyword[keyword_id] == id_key_end)
> > + {
>
> This could benefit from a comment describing what you expect to be verified.
Ok.
> > +/* find all the keywords in the command */ static void
> > +keywords_find(int argc, char * const argv[],
>
> Make this function return an int.
Ok. I will also make the changes along this patch to check it’s return code.
> > +static void command_def_cleanup(struct command_def *parsed_cmd) {
> > + /* Nothing to do for now */
>
> Then why define it?
This function is populated later by another patch and it frees a dynamically
allocated buffer. You suggested to use a static buffer instead, so I guess I
will remove this functions since it's no longer needed.
> > + if (!parsed_cmd.cmd_function) {
> > + rc = -1;
> > + goto __ret_cmd_cleanup;
> > + }
>
> I think this whole if statement is unneeded since this same test already
> exists
> in keywords_find().
Ok.
Thanks and best regards,
Codrin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
